#7226 - fixes NodeJS adapter for multiple set-cookie headers (and other header issues) #7227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
May 27, 2023 22:01
to safely handle multiple set-cookie headers when converting from a WebAPI Response to a NodeJS ServerResponse Modifies the existing nodeMiddleware logic which first set AstroCookies on ServerResponse.setHeader(...) and then called ServerResponse.writeHead(status, Response.headers) which means any that if the WebAPI Response had any set-cookie headers on it, they would replace anything from AstroCookies. The new logic delegates appending AstroCookie values onto the WebAPI Response Headers object, so that a single unified function safely converts the WebAPI Response Headers into a NodeJS compatible OutgoingHttpHeaders object utilizing the new standard Headers.getSetCookie() function provided by the undici WebAPI polyfills. Plus extensive test coverage.
🦋 Changeset detectedLatest commit: b8f9026 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: integration
natemoo-re
approved these changes
Jun 6, 2023
Merged
|
Thanks for merging! I found the astro e2e/integration test harness stuff is really nicely done and was very easy to utilize for real-world integration/adapter test coverage |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7226
What it does
Utilizes the new standard WebAPI Fetch Headers.getSetCookie() function to safely handle multiple set-cookie headers when converting from a WebAPI Response to a NodeJS ServerResponse
Modifies the existing nodeMiddleware logic which used to first set AstroCookies on
ServerResponse.setHeader(...)and then calledServerResponse.writeHead(status, Response.headers), which meant any that if the WebAPI Response had anyset-cookieheaders on it, they would replace anything fromAstroCookies(they were not merged, so anyAstroCookieswould get lost during response writing, and, multipleset-cookieheaders from the Response.headers were written incorrectly as a CSV).The new logic delegates appending
AstroCookiesvalues onto the WebAPI Response Headers object, so that a new single unified function safely converts from the WebAPI Response Headers into a NodeJS compatibleOutgoingHttpHeadersobject utilizing the new standardHeaders.getSetCookie()function provided by the existing undici WebAPI polyfills.Plus extensive test coverage.
Notes
The Fetch WebAPI standard has been updated with a function
Headers.getSetCookie()(see https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie)The undici library that @astrojs/webapi utilizes for WebAPI polyfills on NodeJS added support for
Headers.getSetCookie()in 5.19.0 (see https://github.com/nodejs/undici/releases/tag/v5.19.0)Since Astro is already using undici@^5.22.0, this means Astro code already had access to this method without having to change any dependencies.
I think that this is probably the best most standards compliant way to fix this problem, and other [unrelated] existing pieces of code that are using copy/pasted set-cookie heading parsing logic (such as https://github.com/withastro/astro/blob/main/packages/integrations/netlify/src/netlify-functions.ts#LL140C26-L140C26) might want to consider changing to using
Headers.getSetCookie()where appropriate since going forward it's a standards compliant way of dealing with the set-cookie special case in the WebAPI Headers type.