Fixed: set multiple cookie when using corsify fn

secondphantom commented 7 months ago

Description

The existing method did not allow for the setting of duplicate header keys, such as 'set-cookie'. Therefore, it has been modified to allow for duplicate keys by using headers.append().

Related Issue

kwhitley/itty-router#207

Type of Change (select one and follow subtasks)

[x] Bug fix (non-breaking change which fixes an issue)

afonsocrg commented 7 months ago

@secondphantom I don't know why but this seems to break the docs call. The function successfully returns the correct headers, but when I get the docs endpoint it blocks and never returns a response. Do you have any idea what may cause this issue? I've looked at it for some time now but didn't get it yet...

afonsocrg commented 7 months ago

Okay, I think I found the issue:

The initial response headers (rHeaders) are being set with 'content-type': 'application/json' (line 22), and then we are appending that header to the existing response headers:

Object.entries({
    ...rHeaders, // <= HERE
    ...allowOrigin,
    'content-type': headers.get('content-type'), // <= AND HERE
}).forEach(([name, value]) => headers.append(name, value as string))

This makes the content-type header have weird values (e.g. 'content-type' => 'text/html; charset=UTF-8, text/html; charset=UTF-8', and 'content-type' => 'application/json, application/json'), and I believe that's the root of this issue. This behavior did not happen before because we were using a JS Object to represent every header field, so there would not be any duplicate content-type.

Removing the content-type from rHeaders (line 22) and from the new response headers (line 82) solved this issue for me, and I believe fixes this bug. I don't see a need to force the content-type to be application/json when checking CORS, so I think there is no problem doing so.

secondphantom commented 7 months ago

@afonsocrg Thank you for taking the time to review my pull request in such detail and for your constructive suggestion regarding the content-type header issue. I'll proceed to update the pull request as recommended.

kwhitley commented 6 months ago

Tagging this in #226 to make sure this case is covered...

kwhitley commented 6 months ago

Mentioned this in the issue thread, but this has been covered in #226 - thanks again for the work and patience on this one!

I'll be closing these as soon as v5.x is released.

secondphantom commented 6 months ago

I appreciate the update and am looking forward to the release of v5.x.

kwhitley commented 6 months ago

This has been fully addressed in v5 - thanks again!

kwhitley / itty-router