fix(server&node-fetch): handle multiple set-cookie headers correctly with uWebSockets.js integration
#2821
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdjusts Headers initialization to correctly accumulate Set-Cookie values, prevents duplicate Set-Cookie emission in the uWebSockets adapter, adds cross-adapter cookie tests, and includes a changeset documenting dependency patch updates and the bug-fix. (30 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Yoga
participant uwsAdapter
participant uWS
Client->>Yoga: HTTP Request
Yoga->>uwsAdapter: Response (headers + body)
rect rgba(200,230,255,0.18)
note over uwsAdapter: Iterate response headers inside cork
uwsAdapter->>uwsAdapter: isSetCookieHandled = false
alt header == "set-cookie" and not handled
uwsAdapter->>uWS: write each cookie value (iterate array)
uwsAdapter->>uwsAdapter: isSetCookieHandled = true
else header == "set-cookie" and already handled
uwsAdapter->>uwsAdapter: skip header
else
uwsAdapter->>uWS: write header normally
end
end
uWS-->>Client: HTTP Response
sequenceDiagram
participant Caller as Headers.init(...)
participant Headers
rect rgba(220,255,220,0.18)
note over Headers: Build internal header map from various initializers
Caller->>Headers: entries (array | Headers-like | object)
alt value is Array
Headers->>Headers: push(...values) for "set-cookie"
else value is non-null
Headers->>Headers: push(single value)
else
Headers->>Headers: skip undefined/null
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/node-fetch |
0.8.1-alpha-20251009163820-923716c6485f7bf24a93cafed705c3fc6bccb2e6 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.10.13-alpha-20251009163820-923716c6485f7bf24a93cafed705c3fc6bccb2e6 |
npm ↗︎ unpkg ↗︎ |
✅
|
✅
|
✅
|
✅
|
✅
|
✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/server/test/cookies.spec.ts (1)
8-10: Consider adding more context to the hapi skip comment.While the skip is acceptable for this PR scope (focused on uWebSockets.js), consider adding a link to an issue or brief explanation of what "issues" hapi has with multiple Set-Cookie headers for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/smooth-worlds-pull.md(1 hunks)packages/node-fetch/src/Headers.ts(3 hunks)packages/server/src/uwebsockets.ts(1 hunks)packages/server/test/cookies.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/test/cookies.spec.ts (2)
packages/server/test/test-fetch.ts (1)
runTestsForEachFetchImpl(15-102)packages/server/test/test-server.ts (1)
runTestsForEachServerImpl(494-517)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: unit / node 24
- GitHub Check: e2e / aws-lambda
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: unit / bun
- GitHub Check: unit / deno
- GitHub Check: server (native)
- GitHub Check: server (uws)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
🔇 Additional comments (6)
packages/server/src/uwebsockets.ts (1)
234-251: Correct fix for set-cookie duplication bug.The guard prevents multiple invocations of
getSetCookie()during header iteration. When the Headers polyfill yields multipleset-cookieentries (one per cookie), this fix ensuresgetSetCookie()is called only once, preventing the exponential duplication reported in the linked issues.packages/node-fetch/src/Headers.ts (3)
80-85: LGTM! Correctly handles array-valued set-cookie headers.The array spread and null-check ensure multiple cookie values are accumulated properly during initialization.
94-99: LGTM! Correctly handles array-valued set-cookie headers.The logic mirrors the array-based initialization path and correctly accumulates cookie values while preventing them from being added to the regular header map.
111-116: LGTM! Correctly handles array-valued set-cookie headers.The logic is sound: array values are spread, single values are pushed. The outer null-check at line 107 ensures no null/undefined values are processed.
.changeset/smooth-worlds-pull.md (1)
1-6: LGTM! Changeset correctly documents the bug fix.The description accurately reflects the changes made to handle multiple set-cookie headers in the uWebSockets.js integration.
packages/server/test/cookies.spec.ts (1)
11-25: LGTM! Test comprehensively validates the fix.The test correctly verifies that:
- Two distinct set-cookie headers are preserved without duplication
- Cookie attributes (SameSite, Secure) are maintained
- The fix works across all supported fetch and server implementations (except hapi)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/utils.ts (1)
307-326: Bug: drops additional Set-Cookie values whenHeaders.getSetCookieis unavailableIn the per-header fallback, if
getSetCookieis missing, only the firstset-cookievalue is written; subsequent ones are skipped due tosetCookiesSetshort-circuit. This loses cookies on fetch impls that don’t exposegetSetCookiebut do iterate multipleset-cookieentries.Fix by accumulating values and setting
set-cookieonce after the loop.- // @ts-expect-error - setHeaders exist - if (serverResponse.setHeaders && globalThis.process?.versions?.node?.startsWith('2')) { + // @ts-expect-error - setHeaders exist + if (serverResponse.setHeaders && globalThis.process?.versions?.node?.startsWith('2')) { // @ts-expect-error - setHeaders exist serverResponse.setHeaders(fetchResponse.headers); } else { - let setCookiesSet = false; - fetchResponse.headers.forEach((value, key) => { - if (key === 'set-cookie') { - if (setCookiesSet) { - return; - } - setCookiesSet = true; - const setCookies = fetchResponse.headers.getSetCookie?.(); - if (setCookies) { - serverResponse.setHeader('set-cookie', setCookies); - return; - } - } - serverResponse.setHeader(key, value); - }); + let setCookiesSet = false; + let setCookiesFallback: string[] | null = null; + fetchResponse.headers.forEach((value, key) => { + if (key === 'set-cookie') { + if (setCookiesSet) { + // accumulate additional values for fallback + if (setCookiesFallback) setCookiesFallback.push(value); + return; + } + setCookiesSet = true; + const setCookies = fetchResponse.headers.getSetCookie?.(); + if (setCookies?.length) { + serverResponse.setHeader('set-cookie', setCookies); + return; + } + // Fallback: collect values seen during iteration, set after loop + setCookiesFallback = [value]; + return; + } + serverResponse.setHeader(key, value); + }); + if (setCookiesFallback?.length) { + serverResponse.setHeader('set-cookie', setCookiesFallback); + } }
🧹 Nitpick comments (2)
packages/server/src/utils.ts (1)
307-310: Make Node version gating future-proof
startsWith('2')will disablesetHeaderson Node 30+ and matches the hypothetical Node 2.x. Parse the major version numerically instead.- if (serverResponse.setHeaders && globalThis.process?.versions?.node?.startsWith('2')) { + const nodeMajor = + Number(globalThis.process?.versions?.node?.split?.('.')?.[0] ?? 0); + if (serverResponse.setHeaders && nodeMajor >= 20) {packages/server/test/cookies.spec.ts (1)
21-24: Avoid ordering assumptions in cookie assertionsDifferent adapters may emit multiple Set-Cookie headers in different orders. Assert membership and length to reduce flakiness.
- expect(cookies).toEqual([ - 'name=value0; SameSite=None; Secure', - 'name=value1; SameSite=Strict; Secure', - ]); + expect(cookies).toHaveLength(2); + expect(cookies).toEqual( + expect.arrayContaining([ + 'name=value0; SameSite=None; Secure', + 'name=value1; SameSite=Strict; Secure', + ]), + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/server/src/utils.ts(1 hunks)packages/server/test/cookies.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/server/test/cookies.spec.ts (2)
packages/server/test/test-fetch.ts (1)
runTestsForEachFetchImpl(15-102)packages/server/test/test-server.ts (1)
runTestsForEachServerImpl(494-517)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: e2e / aws-lambda
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: unit / node 24
- GitHub Check: unit / node 18
- GitHub Check: unit / node 20
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: server (uws)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: server (ponyfill)
🔇 Additional comments (1)
packages/server/test/cookies.spec.ts (1)
11-18: Nice: minimal, adapter-agnostic reproUsing Response + headers.append twice mirrors real usage and validates cross-adapter behavior. LGTM.
…y with `uWebSockets.js` integration
ardatan
force-pushed
the
fix-multiple-cookies
branch
from
2ff4979 to
a983c44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/smooth-worlds-pull.md(1 hunks)packages/node-fetch/src/Headers.ts(3 hunks)packages/server/src/utils.ts(1 hunks)packages/server/src/uwebsockets.ts(1 hunks)packages/server/test/cookies.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/test/cookies.spec.ts
- packages/server/src/uwebsockets.ts
🔇 Additional comments (2)
packages/node-fetch/src/Headers.ts (1)
80-84: LGTM! Set-Cookie array handling is correct.The changes properly handle both array and single-value set-cookie headers across all three initialization paths (array-based, Headers-like, and plain object). The null checks prevent undefined values from being pushed, and spreading array values ensures multiple cookies are accumulated correctly.
Also applies to: 94-98, 111-114
.changeset/smooth-worlds-pull.md (1)
1-6: LGTM! Changeset accurately documents the fix.The patch-level designation and description appropriately capture the scope of the bug fix for multiple set-cookie headers with uWebSockets.js integration.
enisdenjo
approved these changes
Oct 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes graphql-hive/graphql-yoga#3316 (comment)
Fixes #2709
While iterating headers, if the value is an array like
set-cookie, handle it correctly.Also do not set
setCookiesmultiple times while passing headers to uWebSockets.js