Cookie not set when response is rewritten (experimental.rewriting)

[atej]

Astro Info

Astro                    v4.10.2
Node                     v20.13.1
System                   macOS (arm64)
Package Manager          npm
Output                   server
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Using Astro.cookie.set does not actually set a cookie for a rewritten request.

What's the expected result?

A cookie should be set.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-xcsrpa?file=src%2Fpages%2F1.astro

Participation

• I am willing to submit a pull request for this issue.

[ematipico]

I'm sorry, I checked the reproduction, and I don't get it. From the index I need to go to the page /1. Why would I need to expect to see a cookie that was never set? (The index doesn't set any cookies)

[atej]

The initial console.log would be expected to be undefined but I was expecting subsequent page refreshes to log the cookie value.

But went through this - #11247 (comment). I think it's the same issue. Cannot manipulate the request once it's been consumed. I was expecting the cookie to be set as page /2 (to which /1 is rewritten to) does so. But I guess the Astro.cookies.set is being silently ignored in the case of the rewrite.

[ematipico]

Can you make sure to leave proper instructions in the reproduction, so we will be able to triage it and give a proper answer?

That issue is about the body of the request, which behaves differently from headers.

[atej]

Added instructions.

[matthewp]

Thanks @atej !

[matthewp]

@ascorbic some context on this one (you may know some or most of this already, sorry): The Set-Cookie header is the one header that is different from the rest in that it must be a separate header for each cookie you want to set, whereas most headers can be merged if there are duplicates. The Headers global did not account for this, because it was made for use in browser service worker where setting a cookie in a response is not a thing.

As a result, there used to be a different ways to set Set-Cookie headers in each host adapter. And so that's why Astro.cookies.set does not directly set the header on to the response. Instead that object is attached to the request via a symbol (weird, i know), and then each host grabs the cookies and sets them in their host-specific way.

I believe (you will want to research to confirm) that now Set-Cookie can be attached as a header via headers.append('Set-Cookie', value). In which case, our hack work-around might not be needed.

In any case, the likely cause of this bug is that rewrite creates a new Request object and the AstroCookies objet is not being copied over. The simplest fix would just be to do that. But it might also be worth investigating if this hack workaround can be avoided entirely.

[ascorbic]

I think you're right, and headers.append('Set-Cookie', value) will work. I'll give that a try.

[ascorbic]

Looking at it a but I think there are too many public methods that expect the current behaviour to make it a safe refactor right now. Maybe in Astro 5 it can be simplified. For now I'll fix the immediate issue.