Skip to content
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

Unable to set multiple "set-cookie" headers #29

Closed
aleehedl opened this issue Jun 13, 2022 · 12 comments
Closed

Unable to set multiple "set-cookie" headers #29

aleehedl opened this issue Jun 13, 2022 · 12 comments

Comments

@aleehedl
Copy link

aleehedl commented Jun 13, 2022

According to MDN, setting multiple cookies in an HTTP response should be done by using multiple "Set-Cookie" headers.

To send multiple cookies, multiple Set-Cookie headers should be sent in the same response.

As far as I know, this is not possible with the headers-polyfill.

> const  h = new Headers();
> h.set('set-cookie', 'foo=bar');
> h.set('set-cookie', 'bar=baz');
> h.raw()
-> { 'set-cookie': 'bar=baz' }

There should be a way for setting multiple set-cookie entries. Comma-separated value (set-cookie: foo=bar, bar=baz) is deprecated and not implemented by modern browsers.

@kettanaito
Copy link
Member

Hey, @aleehedl. Thanks for raising this.

Just as with the native Headers, the .set() method overrides any given cookie. That's why you're only getting the latest value of "set-cookie" when calling .raw().

If you wish to set multiple headers, use .append():

const { Headers } = require("headers-polyfill")

let h = new Headers()
h.append('set-cookie', 'foo=bar')
h.append('set-cookie', 'bar=baz')

h.get('set-cookie') // "foo=bar, bar=baz"

This is the same behavior as the native Headers have, so I believe the library complies well to the specification in this regard.

@aleehedl
Copy link
Author

aleehedl commented Jun 13, 2022

Thanks for the response @kettanaito.

The comma-separated value isn't usable with cookies, because browsers don't accept multiple cookies in a single set-cookie header. See the link from MDN. There should be a way for adding multiple headers with the same name (multiple 'Set-Cookie' headers). So that in the response the cookies would be in the following form:

Response headers:
Connection: keep-alive
Content-Type: text/plain
Set-Cookie: a=1
Set-Cookie: b=2

@kettanaito
Copy link
Member

I don't think the issue you're describing is directly related to the Headers polyfill library. In regards to the Set-Cookie behavior, it acts just as the Fetch API Headers does: multiple values of the same header are joined into a comma-separated string. You can try it out in your browser's console:

let h = new Headers()
h.append('Set-Cookie', 'foo=bar')
h.append('Set-Cookie', 'bar=baz')
h.get('Set-Cookie')

I may not understand the issue fully here. Could you please elaborate more on what exactly you're trying to do, in what context, etc.? If a reproduction repository or a sandbox can help you share that info, please create that.

@aleehedl
Copy link
Author

aleehedl commented Jun 13, 2022

Here's a simple repo:
https://github.com/aleehedl/msw-cookies

There are two servers you can start: server.ts using msw/http-middleware which cannot do what I want. And these node-server.ts which sets two Set-Cookie headers correctly.

And a possbily running codesandbox:
https://codesandbox.io/s/relaxed-vaughan-fuo4gq?file=/index.js
Live mode: https://fuo4gq.sse.codesandbox.io/

The main thing is: Browser doesn't set two cookies when the values come in a single Set-Cookie header.

@kettanaito
Copy link
Member

Thank you for preparing those!

So it seems like an issue with how MSW handles cookies, not the Headers polyfill issue. The Headers function as expected, keeping all headers in a name: value(s) pairs. I think somewhere during the serialization of the mocked cookies is when the issue is happening.

The main thing is: Browser doesn't set two cookies when the values come in a single Set-Cookie header.

About that. In the browser, you cannot set Set-Cookie headers on a Response instance. That's a security violation and it's forbidden. You can try it out yourself:

const res = new Response(null, { headers: { 'Set-Cookie': 'foo=bar, bar=baz' } })
res.headers.get('Set-Cookie')

To account for that, MSW reads your mocked cookies and forwards them onto the document directly, writing them as document.cookie string (implementation). It looks like we still set the Set-Cookie header so you can inspect it in the Network but it won't be available in the response you receive. That's correct, the Set-Cookie isn't available on actual responses either. It's the browser that reads that cookie and forwards it onto the document, which MSW does for the browser in case of mocked responses.

1 similar comment
@kettanaito
Copy link
Member

Thank you for preparing those!

So it seems like an issue with how MSW handles cookies, not the Headers polyfill issue. The Headers function as expected, keeping all headers in a name: value(s) pairs. I think somewhere during the serialization of the mocked cookies is when the issue is happening.

The main thing is: Browser doesn't set two cookies when the values come in a single Set-Cookie header.

About that. In the browser, you cannot set Set-Cookie headers on a Response instance. That's a security violation and it's forbidden. You can try it out yourself:

const res = new Response(null, { headers: { 'Set-Cookie': 'foo=bar, bar=baz' } })
res.headers.get('Set-Cookie')

To account for that, MSW reads your mocked cookies and forwards them onto the document directly, writing them as document.cookie string (implementation). It looks like we still set the Set-Cookie header so you can inspect it in the Network but it won't be available in the response you receive. That's correct, the Set-Cookie isn't available on actual responses either. It's the browser that reads that cookie and forwards it onto the document, which MSW does for the browser in case of mocked responses.

@kettanaito kettanaito reopened this Jun 13, 2022
@kettanaito
Copy link
Member

Oh, I see what may be off. In MSW, the ctx.cookie transformer is using headers.set('Set-Cookie', value). That's the same issue as we've talked about before: using .set() will override any previously existing header value instead of joining header values.

I will create an issue in the MSW repository where we can follow up.

@kettanaito
Copy link
Member

Opened in mswjs/msw#1290.

@gucong3000
Copy link

https://developer.mozilla.org/en-US/docs/Web/API/Headers/getSetCookie

This function may help solve this problem.

@kettanaito
Copy link
Member

Thanks for the suggestion, @gucong3000. However, it's also mentioned the first thing in the Examples that response.headers are explicitly stripped by the browser for security considerations (otherwise the getSetCookie() API would conflict with the HttpOnly nature of cookies).

You can use the getSetCookie() API only on the Headers instance. That's a limitation that we cannot rely on in MSW, so this API cannot be used.

@gucong3000
Copy link

@kettanaito Thanks for your work. I'm using this package under node.js. Great work.
Headers are used not only for response, but also for request.
https://developer.mozilla.org/en-US/docs/Web/API/Request/headers

@kettanaito
Copy link
Member

Thanks for your kind words!

Yeah, Headers can be used to declare any headers. In the context of this issue, accessing the response headers was a pre-requisite so I discussed it respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants