forwarding of request cookie headers comma separated instead of semi-colon separated

[Courey]

Reproduction

if you use version 2.8.1, and in an auth loader you set
return redirect(someAuthURL, { headers: { 'Set-Cookie': cookie } })
it will work as expected.

if you then upgrade to version 2.9.2, the cookies you passed into the redirect are comma separated and break auth since there are no headers named for what you are looking for since they end up as one giant single value header that is _session with all the other headers as its value:

{"_session":"[REDACTED], _session.legacy=[REDACTED], csrfToken=[REDACTED], X-CSRFToken=[REDACTED], csrf=[REDACTED], session=[REDACTED]"}

System Info

## This is the version that breaks:
  System:
    OS: macOS 13.4
    CPU: (12) arm64 Apple M2 Max
    Memory: 31.86 GB / 96.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.9.0 - ~/.nvm/versions/node/v20.9.0/bin/node
    Yarn: 1.22.21 - ~/dev/gcn.nasa.gov/node_modules/.bin/yarn
    npm: 10.1.0 - ~/.nvm/versions/node/v20.9.0/bin/npm
  Browsers:
    Chrome: 126.0.6478.115
    Safari: 16.5
  npmPackages:
    @remix-run/architect: ^2.9.2 => 2.9.2
    @remix-run/css-bundle: ^2.9.2 => 2.9.2
    @remix-run/dev: ^2.9.2 => 2.9.2
    @remix-run/eslint-config: ^2.9.2 => 2.9.2
    @remix-run/node: ^2.9.2 => 2.9.2
    @remix-run/react: ^2.9.2 => 2.9.2

## this is the version where it works
  System:
    OS: macOS 13.4
    CPU: (12) arm64 Apple M2 Max
    Memory: 30.14 GB / 96.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.9.0 - ~/.nvm/versions/node/v20.9.0/bin/node
    Yarn: 1.22.21 - ~/dev/temporary/node_modules/.bin/yarn
    npm: 10.1.0 - ~/.nvm/versions/node/v20.9.0/bin/npm
  Browsers:
    Chrome: 126.0.6478.115
    Safari: 16.5
  npmPackages:
    @remix-run/architect: ^2.8.1 => 2.8.1
    @remix-run/css-bundle: ^2.8.1 => 2.8.1
    @remix-run/dev: ^2.8.1 => 2.8.1
    @remix-run/eslint-config: ^2.8.1 => 2.8.1
    @remix-run/node: ^2.8.1 => 2.8.1
    @remix-run/react: ^2.8.1 => 2.8.1

Used Package Manager

npm

Expected Behavior

I would expect when I have requestCookies in @remix-run/architect/dist/server.js in the createRemixHeaders function, that when they are forwarded they will be semi-colon separated values.

Actual Behavior

It returns the cookie as comma separated values instead of semi-colon. This breaks parsing
This image of console logs and associated log messages should clearly show the difference. You can see that when they were joined, they are correct, but when they are appended they are comma separated because that's what append does.

This means when it hits the decoder in the createCookieFactory in cookie.ts it returns this:

{"_session":"[REDACTED], _session.legacy=[REDACTED], csrfToken=[REDACTED], X-CSRFToken=[REDACTED], csrf=[REDACTED], session=[REDACTED]"}

Which means there is ONLY a _session header with ALL the headers in them.

I did a fair bit of digging. You can see that originally this was created with the buggy loop in 9d3b090#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408R95-R98

then the issue was fixed in https://github.com/remix-run/remix/pull/1709/files#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408L102-R102

then it was re-introduced in ad83e53#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408L91-R93

The solution would be reverting that portion of the change to be the fixed version instead of the buggy version.

[lpsinger]

This unit test ought to have caught this, but it did not because the various fetch implementations treat the Cookie field differently. Consider the following code snippet:

const h = new Headers()
h.append('cookie', 'foo')
h.append('cookie', 'bar')
console.log(h.get('cookie'))

The following implementations print 'foo, bar', with a comma: @remix-run/web-fetch@4.4.2, Safari 17.5, Firefox 127.0.2, Chrome 126.0.6478.127

The following implementations print 'foo; bar', with a semicolon: Node.js 20.14.0, undici@6.19.2

The tests are probably run with either native fetch or undici, although the default Remix configuration (without the feature flag nativeFetch set) probably uses @remix-run/web-fetch. So the test passes but the bug it's checking for appears in production.

I think that it is pretty clear that the browser along with @remix-run/web-fetch are doing what the the fetch standard says: they are joining multiple occurrences of headers with a comma and a space; the standard does not specify any special treatment for the cookie header. It is Node and undici that are violating the spec here --- although IMHO their behavior is a better design than the standard!

So I concur with @Courey: the fix here is to bring back the more portable method of reading the request cookie header from https://github.com/remix-run/remix/pull/1709/files#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408L102-R102. That won't make a difference for the (evidently not standards-conforming) Node.js native fetch or undici, but it will fix the matter for the legacy Remix fetch polyfill, and presumably any other JavaScript runtimes that do conform to the spec.

[github-actions]

🤖 Hello there,

We just published version 2.10.3-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 2.10.3 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!