Closed Courey closed 5 days ago
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.
🤖 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!
🤖 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!
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:
System Info
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:
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 https://github.com/remix-run/remix/commit/9d3b090df737d867c7c1ec75bd8181fca007094a#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 https://github.com/remix-run/remix/commit/ad83e53c42b9da24c89408a14d5154563de3b477#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408L91-R93
The solution would be reverting that portion of the change to be the fixed version instead of the buggy version.