node: preserve pre-existing Set-Cookie headers in sendNodeResponse#200
node: preserve pre-existing Set-Cookie headers in sendNodeResponse#200
Conversation
Reported as #144. When a caller sets headers on the Node response (e.g. via nodeRes.setHeader("set-cookie", ...)) before handing the response off to sendNodeResponse, Node's writeHead silently replaces every header of the same name rather than appending. Any Set-Cookie the caller had registered therefore disappears as soon as we call writeHead(status, rawHeaders). The scenario shows up in TanStack Start's dev-server plugin and in JWT auth middleware that sets an access-token cookie via setHeader and expects the refresh-token cookie from the Web Response to merge with it. Fix: before writeHead, pull every header already on nodeRes, strip them off (so writeHead doesn't double-emit through its own pending- headers channel), and prepend them onto the rawHeaders array. For Set-Cookie we preserve every line unconditionally; for other header names the incoming Response value still wins (so no behaviour change on e.g. Content-Type, Cache-Control) and only pre-existing headers that the Response did NOT also specify are kept. The HTTP/2 path and Deno path share the same merged array. Fixes #144 Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
📝 WalkthroughWalkthroughThe Node adapter's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapters/_node/send.ts`:
- Around line 63-114: The loop in mergeExistingHeaders inconsistently skips
nodeRes.removeHeader when an existing non-Set-Cookie header is going to be
overridden (the continue on line 98), which can leave duplicate headers emitted
by writeHead; change the loop so removeHeader is always called for every
existing header (call nodeRes.removeHeader?(name) before the continue/after
handling values) while preserving the special-case logic that concatenates
Set-Cookie entries into preserved, and update any tests (add a case that sets
nodeRes.setHeader("set-cookie", "a=1") and returns a Response with one or more
set-cookie values on both HTTP/1 and HTTP/2) to validate merged Set-Cookie
behavior through writeHead; refer to mergeExistingHeaders, existingNames,
incomingNonSetCookieNames, nodeRes.getHeader, nodeRes.removeHeader, and
writeHead when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03758be7-4db4-497e-8ad7-e42081dd5dc8
📒 Files selected for processing (1)
src/adapters/_node/send.ts
| const merged = mergeExistingHeaders(nodeRes, rawHeaders); | ||
| const writeHeaders: any = globalThis.Deno ? merged : merged.flat(); | ||
| if (nodeRes.req?.httpVersion === "2.0") { | ||
| // @ts-expect-error | ||
| nodeRes.writeHead(status, writeHeaders); | ||
| } else { | ||
| // @ts-expect-error | ||
| nodeRes.writeHead(status, statusText, writeHeaders); | ||
| } | ||
| } | ||
|
|
||
| function mergeExistingHeaders( | ||
| nodeRes: NodeServerResponse, | ||
| incoming: [string, string][], | ||
| ): [string, string][] { | ||
| const existingNames = | ||
| typeof nodeRes.getHeaderNames === "function" | ||
| ? nodeRes.getHeaderNames() | ||
| : []; | ||
| if (existingNames.length === 0) { | ||
| return incoming; | ||
| } | ||
|
|
||
| // For every Set-Cookie line set via setHeader, keep it and | ||
| // concatenate with the incoming ones so all cookies survive. For | ||
| // other single-valued headers, let the incoming Response value win | ||
| // if both sides specify the same name (preserves prior behaviour), | ||
| // otherwise preserve the pre-existing header. | ||
| const incomingNonSetCookieNames = new Set( | ||
| incoming.map(([k]) => k.toLowerCase()).filter((k) => k !== "set-cookie"), | ||
| ); | ||
| const preserved: [string, string][] = []; | ||
| for (const name of existingNames) { | ||
| const lower = name.toLowerCase(); | ||
| if (lower !== "set-cookie" && incomingNonSetCookieNames.has(lower)) { | ||
| continue; | ||
| } | ||
| const value = nodeRes.getHeader?.(name); | ||
| if (Array.isArray(value)) { | ||
| for (const v of value) { | ||
| preserved.push([name, String(v)]); | ||
| } | ||
| } else if (value !== undefined) { | ||
| preserved.push([name, String(value)]); | ||
| } | ||
| // Drop from nodeRes so writeHead doesn't also emit the header | ||
| // through its own pending-headers channel and duplicate it. | ||
| nodeRes.removeHeader?.(name); | ||
| } | ||
|
|
||
| return preserved.length > 0 ? [...preserved, ...incoming] : incoming; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js 20/22/24 http OutgoingMessage.writeHead, when the response already has headers set via setHeader (so [kOutHeaders] exists), does passing a flat array like ["set-cookie","a","set-cookie","b"] as the rawHeaders argument preserve both Set-Cookie lines, or does the slow-path call setHeader for each pair and cause the later value to overwrite the earlier one?
💡 Result:
In Node.js 20/22/24, when headers are already set via setHeader (kOutHeaders exists), passing a flat rawHeaders array like ["set-cookie","a","set-cookie","b"] to OutgoingMessage.writeHead preserves both Set-Cookie lines as separate headers. It does not overwrite via the slow-path setHeader calls. Prior to a fix merged in December 2023 (commit 4b76ccea, included in Node.js 21.0.0+ and backported to 20.x/22.x), the behavior was buggy: duplicates were collapsed/lost even with rawHeaders arrays after setHeader calls, as writeHead looped over the array calling setHeader (which overwrites) instead of preserving raw duplicates. The fix explicitly handles this case in lib/_http_server.js writeHead: when obj is an array (rawHeaders format) and headers exist, it first removes all conflicting existing headers (via removeHeader for each key), then appends each pair using appendHeader (which supports multiples like Set-Cookie). This ensures raw duplicates are preserved while giving writeHead headers precedence (per docs), without falling to the old setHeader overwrite behavior. Official docs confirm writeHead accepts rawHeaders arrays in request.rawHeaders format (flattened [key,value,...]) and merges with setHeader headers, writeHead taking precedence. The test added in the fix verifies res.rawHeaders retains both ["array-val","1","array-val","2"] even after prior setHeader. {value: Preserves both Set-Cookie lines as separate headers (no overwrite), citations: [23,24,66,67,68,69,70,71,72,73], confidence: high}
Citations:
- 1: http: preserve raw header duplicates in writeHead after setHeader calls nodejs/node#50394
- 2: nodejs/node@4b76ccea95
- 3: https://nodejs.org/docs/latest-v24.x/api/http.html
- 4: https://nodejs.org/dist/latest-v24.x/docs/api/http.html
- 5: https://nodejs.org/docs/latest-v25.x/api/http.html
- 6: https://nodejs.org/download/release/v22.12.0/docs/api/http.html
- 7: https://nodejs.org/docs/v22.12.0/api/http.html
- 8: nodejs/node@3e70b7d863
- 9: https://nodejs.org/dist/latest-v22.x/docs/api/http.html
- 10: nodejs/node@ae5141cb8a
🏁 Script executed:
#!/bin/bash
# Look at the PR's new tests (if any) covering the multi-Set-Cookie + prior setHeader scenario.
fd -t f -e ts -e js . test | xargs rg -n -C3 -i 'set-cookie'
rg -nP -C3 'setHeader\s*\(\s*["'\'']set-cookie'
rg -nP -C3 'mergeExistingHeaders|getHeaderNames\(\)'Repository: h3js/srvx
Length of output: 2015
🏁 Script executed:
#!/bin/bash
# Check package.json for Node version requirements
cat package.json | grep -A 5 '"engines"'
# List test files to find fixture and tests
fd -t f -e ts -e js . test | head -20
# Look at the fixture file mentioned
fd -t f -n '_fixture.ts' testRepository: h3js/srvx
Length of output: 896
🏁 Script executed:
#!/bin/bash
# Search for the x-set-with-node fixture mentioned in the review
rg -n 'x-set-with-node' test/ --type ts
# Look for tests that combine setHeader + set-cookie with incoming headers
rg -n -B2 -A5 'setHeader.*set-cookie|set-cookie.*setHeader' test/ --type tsRepository: h3js/srvx
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Check if there are tests specifically for mergeExistingHeaders or the scenario in question
rg -n -B3 -A10 'beforeEach|describe.*merge|describe.*header' test/ --type ts | head -100Repository: h3js/srvx
Length of output: 816
🏁 Script executed:
#!/bin/bash
# Read the fixture file to understand the x-set-with-node test
cat test/_fixture.ts | head -100Repository: h3js/srvx
Length of output: 2972
🏁 Script executed:
#!/bin/bash
# Look at the complete context around x-set-with-node
rg -C15 'x-set-with-node' test/_fixture.tsRepository: h3js/srvx
Length of output: 1063
🏁 Script executed:
#!/bin/bash
# Check test/node-headers.test.ts for Set-Cookie tests
cat test/node-headers.test.tsRepository: h3js/srvx
Length of output: 1956
🏁 Script executed:
#!/bin/bash
# Read the relevant section of send.ts to see the continue branch
sed -n '90,110p' src/adapters/_node/send.tsRepository: h3js/srvx
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Search for `#144` and `#40` references in the codebase
rg -n '#144|#40' src/ test/Repository: h3js/srvx
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Search for any Set-Cookie related tests in the test directory
rg -n -i 'set-cookie' test/ --type tsRepository: h3js/srvx
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check for CI configuration to see what Node versions are tested
fd -t f -e yml -e yaml . .github/workflows/ 2>/dev/null | head -5
cat .github/workflows/*.yml 2>/dev/null | grep -A10 -B2 'node.*version\|node-version' | head -40Repository: h3js/srvx
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Look at the complete mergeExistingHeaders function to understand the flow
sed -n '74,115p' src/adapters/_node/send.tsRepository: h3js/srvx
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comment explaining why line 98 continue skips removeHeader
git log -p --all -S 'incomingNonSetCookieNames.has' -- src/adapters/_node/send.ts 2>/dev/null | head -150Repository: h3js/srvx
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check git blame for the continue line
git blame -L 96,100 src/adapters/_node/send.ts 2>/dev/nullRepository: h3js/srvx
Length of output: 496
Line 98: Inconsistent header removal defeats stated intent.
When a non-set-cookie header from existingNames appears in incomingNonSetCookieNames (i.e., incoming will override it), the continue skips the nodeRes.removeHeader() call. However, the comment above line 107 states the goal is to "Drop from nodeRes so writeHead doesn't also emit the header through its own pending-headers channel and duplicate it." This creates inconsistent code intent: set-cookie and non-overridden headers are removed, but overridden headers are left in [kOutHeaders]. Consider always calling removeHeader inside the loop for clarity, even when the slot will be overwritten by the incoming value.
Test coverage: Add an explicit test case where nodeRes.setHeader("set-cookie", "a=1") is called by middleware and the Web Response also carries one or more set-cookie values, on both HTTP/1 and HTTP/2, to ensure the merged array survives writeHead correctly. The current x-set-with-node fixture only sets a non-cookie header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/_node/send.ts` around lines 63 - 114, The loop in
mergeExistingHeaders inconsistently skips nodeRes.removeHeader when an existing
non-Set-Cookie header is going to be overridden (the continue on line 98), which
can leave duplicate headers emitted by writeHead; change the loop so
removeHeader is always called for every existing header (call
nodeRes.removeHeader?(name) before the continue/after handling values) while
preserving the special-case logic that concatenates Set-Cookie entries into
preserved, and update any tests (add a case that sets
nodeRes.setHeader("set-cookie", "a=1") and returns a Response with one or more
set-cookie values on both HTTP/1 and HTTP/2) to validate merged Set-Cookie
behavior through writeHead; refer to mergeExistingHeaders, existingNames,
incomingNonSetCookieNames, nodeRes.getHeader, nodeRes.removeHeader, and
writeHead when making the change.
|
Thanks for PR. Considering overhead of this patch, i very much appreciate if you can provide a reasonable minimal reproduction (not tanstack start) As i mentioned in related issue, If framework itself somehow sets multiple set-cookies, this can be handled by a middleware or plugin for framework. |
|
Fair call on the repro — the behaviour I was seeing came from the tanstack-start adapter layering its own Closing this since the merge you're outlining would impose a permanent cost on the Node path for all users just to tolerate a pattern that shouldn't be in use. I'll take the "frameworks own |
2 participants
Fixes #144.
Problem
When a caller sets a header on the Node response before invoking
sendNodeResponse(e.g.nodeRes.setHeader("set-cookie", "a=...")), Node'swriteHead(status, rawHeaders)silently replaces every header of the same name rather than appending. AnySet-Cookiethe caller had registered disappears the momentsendNodeResponsecallswriteHeadwith the Web Response's own headers.The reporter hit this in TanStack Start's dev-server plugin while setting both access-token and refresh-token cookies: the access-token cookie set via
setHeaderwas silently dropped as soon as the Web Response'sSet-CookiereachedwriteHead.Fix
Before calling
writeHead, merge any headers already present onnodeRes:getHeaderNames()/getHeader().Set-Cookie, preserve every line unconditionally so all cookies survive (both the ones onnodeResand the ones in the Web Response).Content-Type,Cache-Control, etc.); when only the caller specified it, keep the caller's value.nodeRessowriteHeaddoesn't emit them twice through its own pending-headers channel.HTTP/2 path, HTTP/1 path and Deno path share the same merged array so the fix covers all three.
Signed off per DCO.
Summary by CodeRabbit