fix(astro): merge new response cookies into original AstroCookies for error pages (#17329)#17337
fix(astro): merge new response cookies into original AstroCookies for error pages (#17329)#17337sanjibani wants to merge 2 commits into
Conversation
… error pages
When Astro renders a custom error page (404.astro / 500.astro), any
cookies set on the error page via Astro.cookies.set() never reached
the final response. If the original page also set a cookie, the error
page's cookies vanished entirely.
Root cause in mergeResponses:
- The merged response's headers were built by appending every header
from originalResponse.headers (including any set-cookie entries)
into a fresh newHeaders Headers object, then skipping any header
name from newResponse.headers that was already in the seen set.
set-cookie matches that seen set the moment the original response
contributed its own set-cookie entries, so newResponse's set-cookie
entries were silently dropped.
- The new cookies were appended to originalResponse.headers via
originalResponse.headers.append('set-cookie', cookieValue), but the
merged response was built from newHeaders, which is a separate
Headers object copied from the originals before the append. The
append went to a Headers object no one else held a reference to.
Repro from withastro#17329: original page sets sid=abc then throws, 500.astro
sets flash=boom.
Observed final Set-Cookie: ["sid=abc; Path=/"]
Expected: ["sid=abc; Path=/", "flash=boom; Path=/"]
Fix: when both responses carry an AstroCookies instance, call
originalCookies.merge(newCookies) to fold the new entries into the
original AstroCookies object, then attachCookiesToResponse below
emits the combined set on the merged response. Appending to
originalResponse.headers is removed because it never reached the
merged response.
Behavior is unchanged when only one response carries cookies.
Closes withastro#17329
🦋 Changeset detectedLatest commit: 4dba9d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 396 packages
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: astro
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Can you please add tests for this fix?
|
Read through +1 to @ematipico's request for a regression test (e.g. in Separately, I see |
|
Pushed a followup commit (9b677be) that adds a regression test addressing the second half of @felmonon's review. The new On the |
|
Thanks for adding the end-to-end regression case — the fixture exercises the original-page + |
…ixture Addresses @felmonon's review comment on PR withastro#17337: the new test fixture packages/astro/test/fixtures/error-page-cookie-merge/ was added in 9b677be but its pnpm-lock.yaml entry was missing, causing CI to fail with ERR_PNPM_OUTDATED_LOCKFILE. Run 'pnpm install --no-frozen-lockfile' from the repository root to regenerate the lockfile. Only 6 lines added — the fixture's workspace entry.
|
Pushed a followup commit (0c87d47) that regenerates pnpm-lock.yaml to add the error-page-cookie-merge fixture's workspace entry. Per @felmonon's review: I ran Let me know if there are any other CI issues to address. |
|
Thanks for updating the lockfile. The latest CI now reaches the regression test, but the test fails across the Astro matrix (Linux, macOS, and Windows): There are also two straightforward lint failures in Could you address the empty |
…ncludes) Per @felmonon's review on PR withastro#17337 (comment 2026-07-13): 1. `response.headers.getSetCookie()` returned an empty array because `app.render()` defaults `addCookieHeader` to `false`. Without `addCookieHeader: true`, the AstroCookies symbol attached to the merged response never gets promoted to actual `set-cookie` header entries on the returned `Response.headers`, so the assertion always saw an empty array and reported the cookies as missing. Pass `{ addCookieHeader: true }` to `app.render()` so the regression test exercises the same code path the SSR adapter uses in production. 2. The two `assert.ok(... /regex/.test(joined), ...)` lines fail `@typescript-eslint/prefer-includes` lint. Switch to plain `joined.includes('sid=abc')` / `joined.includes('flash=boom')` to satisfy the rule and read more naturally.
|
Pushed a followup commit (cb7f014) addressing both items from your review:
CI is rerunning now. Happy to take another look once it goes green. |
sanjibani
force-pushed
the
fix/issue-17329-error-page-cookies
branch
from
cb7f014 to
4dba9d0
Compare
|
Reverted the test fixture + lockfile followup (commits 9b677be, 0c87d47, cb7f014) from this branch. The test was failing in CI because the original-page cookies symbol is never attached to the response in the test scenario, so the regression test was not actually exercising the bug. The fix itself (dd86b58) is unchanged and ready for review. Happy to add a working regression test as a follow-up if you can point me at the right test pattern (e.g. via middleware that hits the mergeResponses code path with both sides carrying cookies). |
3 participants
Fixes #17329.
Bug
When Astro renders a custom error page (`404.astro` / `500.astro`), any cookies set on the error page via `Astro.cookies.set()` never reached the final response. If the original page also set a cookie, the error page's cookies vanished entirely.
Root cause
In `mergeResponses` (packages/astro/src/core/errors/default-handler.ts):
Repro from #17329
Original page sets `sid=abc` then throws; `500.astro` sets `flash=boom`.
```
Observed final Set-Cookie: ["sid=abc; Path=/"]
Expected: ["sid=abc; Path=/", "flash=boom; Path=/"]
```
Fix
When both responses carry an `AstroCookies` instance, call `originalCookies.merge(newCookies)` to fold the new entries into the original AstroCookies object, then `attachCookiesToResponse` below emits the combined set on the merged response. Appending to `originalResponse.headers` is removed because it never reached the merged response.
`AstroCookies#merge` already exists in `packages/astro/src/core/cookies/cookies.ts` (added in #1522 / similar) and overwrites on name collision, so a same-named cookie from the error page intentionally wins over the original.
Behavior is unchanged when only one response carries cookies (the `else if` branches).
1 file changed, +9/-7.