-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#7226 - fixes NodeJS adapter for multiple set-cookie headers (and other header issues) #7227
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is incredibly thorough with great tests, thanks for contributing!
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 |
…er header issues) (#7227) * 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 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. * #7226 - changeset for NodeJS adapter set-cookie fix * fixing all double quotes to single quotes --------- Co-authored-by: Alex Sherwin <alex.sherwin@acadia.inc> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
…er header issues) (#7227) * 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 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. * #7226 - changeset for NodeJS adapter set-cookie fix * fixing all double quotes to single quotes --------- Co-authored-by: Alex Sherwin <alex.sherwin@acadia.inc> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
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-cookie
headers on it, they would replace anything fromAstroCookies
(they were not merged, so anyAstroCookies
would get lost during response writing, and, multipleset-cookie
headers from the Response.headers were written incorrectly as a CSV).The new logic delegates appending
AstroCookies
values onto the WebAPI Response Headers object, so that a new single unified function safely converts from the WebAPI Response Headers into a NodeJS compatibleOutgoingHttpHeaders
object 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.