fix: preserve multiple set-cookie headers#60
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils.ts (1)
117-122:⚠️ Potential issue | 🟠 MajorAra ara~ Did you forget something, baka? ♡
There's an inconsistency between how the
Headersbranch and the plain object branch handleset-cookie:
Missing precedence guard: The
Headersbranch (lines 109-110) skipssetHeaders's cookies ifresponseHeadersalready has set-cookie. But this plain object branch just blindly appends without checking~! That's not very smart, is it? (´・ω・`)Array handling: According to
src/ws.ts,set.headers['set-cookie']can be an array. Appending an array directly will stringify it incorrectly instead of preserving individual cookies~ How embarrassing ♡🍪 Here's a fix, since you obviously need help~
else for (const key in setHeaders) - if (key === 'set-cookie') - headers.append(key, setHeaders[key] as any) - else if (!responseHeaders.has(key)) + if (key === 'set-cookie') { + if (headers.has('set-cookie')) continue + + const cookies = setHeaders[key] + if (Array.isArray(cookies)) + for (const cookie of cookies) + headers.append('set-cookie', cookie) + else + headers.append('set-cookie', cookies as string) + } else if (!responseHeaders.has(key)) headers.set(key, setHeaders[key] as any)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 117 - 122, The plain-object branch that iterates setHeaders currently appends 'set-cookie' entries without the precedence check and without handling arrays; update the loop in src/utils.ts so that when key === 'set-cookie' you first check responseHeaders.has('set-cookie') and skip appending if true (matching the Headers-branch behavior), and when appending handle both string and string[] values by appending each cookie individually (use Array.isArray on setHeaders['set-cookie'] to decide whether to iterate and call headers.append for each item or append the single value); leave the non-'set-cookie' path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils.ts`:
- Around line 117-122: The plain-object branch that iterates setHeaders
currently appends 'set-cookie' entries without the precedence check and without
handling arrays; update the loop in src/utils.ts so that when key ===
'set-cookie' you first check responseHeaders.has('set-cookie') and skip
appending if true (matching the Headers-branch behavior), and when appending
handle both string and string[] values by appending each cookie individually
(use Array.isArray on setHeaders['set-cookie'] to decide whether to iterate and
call headers.append for each item or append the single value); leave the
non-'set-cookie' path unchanged.
2 participants
Problem
The Node adapter rebuilds
responseHeaderswith:This collapses duplicate header names, which is especially problematic for
Set-Cookie.In OAuth callback flows, downstream handlers can return multiple
Set-Cookieheaders.On Bun this worked, but on the Node adapter one of the cookies could be dropped during header merging.
In my reproduction this broke Better Auth on the OAuth callback route:
codeandstateFix
Preserve multiple
Set-Cookieheaders when merging response headers.Related
This looks related to elysiajs/elysia#1547, which reports another
Set-Cookieloss path in Elysia.Summary by CodeRabbit