fix: allow cookies to be set in rewritten responses

[ascorbic]

Changes

Ensures that set-cookie headers are set when rewriting a request.

Fixes #11265

Testing

Adds a test case for a rewritten request

Docs

Just a bugfix

[changeset-bot]

🦋 Changeset detected

Latest commit: a5412a7

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

[matthewp]

I don't think we should append the headers here. They are already appended further up in the response, here:

astro/packages/astro/src/core/app/index.ts

Lines 346 to 350 in cb4d078

	if (addCookieHeader) {
	for (const setCookieHeaderValue of App.getSetCookieFromResponse(response)) {
	response.headers.append('set-cookie', setCookieHeaderValue);
	}
	}

What I think is happening in this bug is that the Response has changed because of rewriting, so the AstroCookies object is not attached to it anymore. So what I think you can do is call attachCookiesToResponse here and then when the cookies are consumed further up the chain they will be appended to the response there.

[ascorbic]

Looks like they actually need to be merged, so I took a different approach

[ematipico]

I have mixed feelings about this method, I would prefer to return a new instance instead of manipulating the existing one. Essentially, try to use a pure/functional approach and avoid side-effects.

I would called this method fork instead

[matthewp]

Why? In this case there are already 2 AstroCookies instances created (once for each response), what does adding a 3rd accomplish? This is not immutable style code, and this function is not mutating the argument, it's mutating itself.

[ematipico]

I don't think this is the correct fix. I think the fix should be done here:

astro/packages/astro/src/core/render-context.ts

Line 237 in bc3c329

this.cookies = new AstroCookies(this.request);

And here

astro/packages/astro/src/core/render-context.ts

Line 432 in bc3c329

this.cookies = new AstroCookies(this.request);

And the code should be

this.cookies = this.cookies.fork(this.request)

fork will return a new instance of AstroCookies, and the creation of these new cookies is confined inside the rewrite function, while the proposed solution seems to be more broad

[matthewp]

Ok, that plan makes sense then. 👍

[ascorbic]

I don't think will help with cookies that are set on the response. It only merges the request cookies.

[ascorbic]

I'm happy to change it to a pure function, but I don't think it can be done at the point the cookies are first assigned.

[ematipico]

What confuses me is the term "rewriting a response" from the initial issue. Astro.rewrite, actually, rewrites a Request, and you'll get Response, eventually. But there's no "rewritten response" involved. So I wonder how the issue is related to rewriting? I am a bit confused 😅 maybe I am missing something

[ascorbic]

It refers to responses to a rewritten request. If page /a has Astro.rewrite('/b'), and /b sets a cookie, currently the cookie from b is not returned.

[ematipico]

Should we add another test where in the first page we set a lorem value, then in the rewritten page we set a ipsum value, and assert that the cookie has ipsum as value?

[ascorbic]

@ematipico and I had a chat and agreed this approach