Multiple set-cookie response headers combined into a single one

[sergey-be]

Describe the bug

Instead of sending multiple set-cookie headers as stated in the specification, MSW combines them into a single comma-separated value.

Environment

• msw: 0.27.1
• nodejs: 12.13, 14.16
• npm: 6.11.0

To Reproduce

import superagent from 'superagent'
import { rest } from 'msw'
import { setupServer } from 'msw/node'

describe('MSW', () => {
  const backendMock = setupServer(
    rest.get('/msw', (req, res, ctx) =>
      res(
        ctx.status(200),
        ctx.set('set-cookie', 'abc=1'),
        ctx.set('set-cookie', 'xyz=0; Secure'), // using ctx.set('set-cookie', [...]) form doesn't fix the issue
      ),
    ),
  )

  beforeAll(() => {
    backendMock.listen()
  })

  afterAll(() => {
    backendMock.close()
  })

  it('should send multiple "set-cookie" headers', async () => {
    const response = await superagent.get('/msw')
    
    expect(response.res.rawHeaders.filter(item => item === 'set-cookie')).toHaveLength(2)
    // response.res.rawHeaders = [ 'x-powered-by', 'msw', 'set-cookie', 'xyz=0; Secure, abc=1' ]
  })
})

Expected behavior

Multiple headers are sent, i.e response.res.rawHeaders = [ 'x-powered-by', 'msw', 'set-cookie', 'abc=1', 'set-cookie', 'xyz=0; Secure' ]

[kettanaito]

Hey, @sergey-be. Thanks for raising this.

Some context: we're using the headers-utils library as a middle-ground between various environments and their ways to construct and handled Headers, both request and response ones. We use the Headers instance as the universal value. That class behaves exactly as you've described: it merges two headers with the same name into a comma-separated list of values:

const headers = new Headers()
headers.set('set-cookie', '123')
headers.append('set-cookie', '456')
headers.get('set-cookie') // "123, 456"

I believe I won't be wrong to conclude that this issue is relevant only in the Node.js context, as browsers don't operate with the rawHeaders. That means we may consider introducing special handling for the headers that allow being set multiple times somewhere along the response handling pipeline. I suggest adding it to node-request-interceptor (the library that powers mocking in Node.js), specifically when setting response headers in the ClientRequest override.

On the other hand, according to RFC2616, it's perfectly valid to represent multiple message-header fields as a single header where each field-value is adjoined in a comma-separated list:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.
— RFC2616

Judged by that statement, there seems to be no violation of the spec by keeping multiple Set-Cookie headers in a single comma-separated list.

[kettanaito]

I'm closing this because I don't think this is a violation of the specification.

If you find this obstructing your usage, please consider opening a pull request to the headers-utils package that would handle the Set-Cookie headers in a unique way.

[hugo]

Hi @kettanaito. Actually, according to rfc6265:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.

Because it's not valid to set the Set-Cookie header when using the fetch API, it's not considered an issue that the Headers object will handle this incorrectly. A browser will never send that header. On the server side, however, it is an issue.

[kettanaito]

Thanks for the insights, @hugo!

The Headers class we construct is a polyfill around the native window.Headers, so it may include certain server-like behaviors for the sake of unifying the headers handling on both browser and server sides. In MSW case, the "server" is represented in a response resolver:

(req, res, ctx) => {
  return res(ctx.set('Set-Cookie', serializedCookie))
}

I wonder if there are any implications with how the Set-Cookie is set on the response/document right now.

[hugo]

My knowledge of service workers is limited, but I think this might be a case where a clean translation of a Web API to the server isn't entirely possible, because of the different semantics. By the looks of it, there is a proposal for how to handle cookies within a service worker — browser support isn't great.

In the browser, msw already sidesteps this issue nicely by using document.cookie to set cookies directly on document when called. On the server, I think the solution is twofold:

1. headers-utils is updated to special case set-cookie and store an array of values
2. In the interceptor special case the set-cookie header to construct the rawHeaders correctly around here

If that sounds sensible I'd be happy to (at least try to) produce PRs to make the change. But I'm sure there's nuance here around header-utils wanting to be a generic, compatible, polyfill versus needing special behaviour for this one edge case.

[GetafixIT]

I've been looking into setting multiple cookies myself. I've not been able to do that.

I've tried adding multiple "Set-Cookie" headers and I've tried adding multiple cookies as one (comma separated).

When using multiple Set-Cookies only the last is respected. Using one Set-Cookies with comma separated values, only the first is respected.

From what I can see browsers seem to need multiple Set-cookie headers to work.

Is it's possible using MSW @kettanaito & if so how?

[kettanaito]

@GetafixIT, you can set multiple Set-Cookie headers in Fetch API like this:

new Headers([
  ['Set-Cookie', 'a=b'],
  ['Set-Cookie', 'c=d']
])

Headers merge the values of that header into a comma-separated string.

To mock that with MSW, I think this should work:

res(
  ctx.set('Set-Cookie', 'a=b'),
  ctx.set('Set-Cookie', 'c=d')
)

I also highly encourage you to upgrade to msw@next where mocking response cookies will be much easier following the Fetch API standard:

http.post('/sign-in', () => {
  return new Response(null, {
    headers: new Headers([
      ['Set-Cookie', 'a=b'],
      ['Set-Cookie', 'c=d'],
    ])
  })
})

[GetafixIT]

@kettanaito thanks for you reply :)

I've tried your first suggestion:

res(
  ctx.set('Set-Cookie', 'a=b'),
  ctx.set('Set-Cookie', 'c=d')
)

This doesn't work. Only the second header is respected. I think this is due to the mapping reference in the underlaying Symbol.

I will try upgrading and report back.

[GetafixIT]

For visibility of the issue with "msw": "^0.49.0" "msw": "^1.3.2",

Given the following mock response:

return res(
      ctx.set(
        "Set-Cookie",
        "AAAAAA=true; expires=Thu, 05 Oct 3023 16:44:57 GMT; Secure; Path=/de/;"
      ),
      ctx.set(
        "Set-Cookie",
        "BBBBBB=true; expires=Thu, 05 Oct 3023 16:44:57 GMT; Secure; Path=/de/;"
      ),
      ctx.json({})
    );

The browser received the following header:

But the browser only stores the BBBBB cookie despite both appearing in the header comma separated:

This is true in:
Chrome: 116.0.5845.110 (Official Build) (arm64)
Firefox: 118.0.1 (64-bit)

[GetafixIT]

Using msw@next which currently resolves to "msw": "^0.0.0-fetch.rc-20",

I get the following error from my setupServer script:

error - ./mocks/server.js:1:0 Module not found: Package path ./node is not exported from package app/node_modules/msw

[sergey-be]

But the browser only stores the BBBBB cookie despite both appearing in the header comma separated:

This is correct browser behaviour according to the set-cookie header parsing algoritm in the spec. A browser deems the second cookie as an unknown attribute of the first cookie and ignores it. Multiple cookies cannot be folded into a single set-cookie header, the spec clearly says that.

[GetafixIT]

Hey @sergey-be

That maybe be so in the spec, however, it also means MSW is unable to set multiple cookies in the browser... a common practice.

Perhaps the spec is detailing the way browsers should handle 1 Set-Cookie header with multiple strings in it. But that is not the same things as how to set multiple cookies ... or at least the browsers seem to require multiple Set-Cookie headers to actually set multiple cookies. I could well be wrong... I have not read the spec :)

Despite my lack of reading...

When I run un-mocked, there are multiple Set-Cookie headers present and the browser respects them, setting multiple cookies. This is the behaviour I'm trying to mock.

For example, here are the headers sent by my BE:

And respected by the browser:

[kettanaito]

@GetafixIT, try to remove node_modules and reinstall dependencies. Also, make sure to use Node.js v18+ for msw@next. That should resolve the export error you've mentioned. If it doesn't, could you please submit a reproduction repository for it?

[kettanaito]

Regarding the issue, you may be stumbling upon the Fetch API spec limitation. This is nothing specific to MSW. If you set multiple Set-Cookie headers using the Headers class in the browser, you will get the same behavior: multiple cookie headers joined under a single key in a comma-separated string.

You get the right behavior when receiving the multi-cookie header from the server because the browser sets it differently behind the scenes. Also note that this behavior is specific only to the Set-Cookie header. Other headers support multi-value by value1, value2 strings, this is correct per spec.

[kettanaito]

Note that this is purely a representational issue. The only thing that matters if whether MSW parses that multi-cookie header and applies it correctly to document.cookie. Does it, @GetafixIT?

Headers can't represent multiple cookies (or any headers with the same name, at that point) because it's object-based and you cannot have two keys with the same name in a single object.

[GetafixIT]

@kettanaito I will take another look Next week and get back to you.

[GetafixIT]

@kettanaito I've been debugging this today... and thank you for your suggestions.

From what I can see, the issue is with the cookie boolean option 'httpOnly'. When I set this option in the mock, those cookies aren't set. So, if I try to strictly mimic the API the cookies don't get set.

This is the root cause of the other missing cookies in my case. So I can now set multiple cookies so long as they're not httpOnly. I can work with that.

The httpOnly option should prevent cookies from being read by JavaScript. I figured you should be able to set them though, however I did a quick test in the browser - it seems you can't anymore.

[kettanaito]

That's interesting. Thanks for looking into it, @GetafixIT!

In V2, we are using the HttpResponse class that allows MSW to spy on the headers being set. I think that way we should be able to detect an intention to set a response cookie even if that cookie has the httpOnly flag set. It'd be a nice integration test to add once V2 is out.

Have you tested this on msw@next?

[GetafixIT]

@kettanaito I have not yet. Will post here once I have..