Fix SSR Apollo cookie overwrite in server-side GraphQL requests#3985
Fix SSR Apollo cookie overwrite in server-side GraphQL requests#3985xDipzz wants to merge 7 commits intoOWASP:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbitRelease Notes
WalkthroughAdds utilities to merge Cookie and CSRF headers for server-side Apollo requests, refactors the Apollo client to use them and exposes a helper to build the server Cookie header, and adds unit tests covering cookie/CSRF header merge behaviors and edge cases (duplicates, missing token, preserving other headers). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (3)
frontend/__tests__/unit/server/apolloCookieHeader.test.ts (1)
3-71: LGTM — good coverage of the happy paths and edge cases.The 6 tests collectively cover: lowercase key, undefined headers, multi-token deduplication, case mixing, null CSRF, and non-cookie header preservation. One gap to consider adding:
Missing: test for
csrfToken = ''(empty string). The!csrfTokenguard inmergeCookieHeadertreats''identically tonull— existing cookies (including a stalecsrftoken) are preserved whileX-CSRFTokenis set to''. Without a test, this behavior is undocumented and unprotected against future refactoring.💡 Suggested additional test
+ test('treats empty string csrf token same as null: preserves existing cookies without replacing csrftoken', () => { + const headers = mergeApolloHeadersWithCsrf( + { + Cookie: 'sessionid=session123; csrftoken=oldvalue', + }, + '' + ) + + // '' is falsy → mergeCookieHeader early-returns; stale csrftoken is NOT replaced + expect(headers.Cookie).toBe('sessionid=session123; csrftoken=oldvalue') + expect(headers['X-CSRFToken']).toBe('') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/server/apolloCookieHeader.test.ts` around lines 3 - 71, Missing unit test for csrfToken = '': add a test case to ensure mergeCookieHeader and mergeApolloHeadersWithCsrf treat an empty-string CSRF token like null (preserve existing cookies including any stale csrftoken and set X-CSRFToken to ''). Add a new test similar to the "preserves incoming cookies when csrf token is missing" case but pass '' as the csrf argument, assert that the Cookie header remains unchanged (including any existing csrftoken) and that headers['X-CSRFToken'] === '' and that no duplicate csrftoken entries are created (use the existing mergeCookieHeader helper in the test to validate deduplication).frontend/src/server/apolloClient.ts (2)
9-10: Two independentcookies()calls per request — consider sharing the cookie store.Both
getCsrfTokenOnServerandgetRequestCookieHeaderOnServereach callawait cookies()per invocation, resulting in two calls per GraphQL request inside thesetContextcallback. While functionally safe (both see the same immutable request context), it is redundant.♻️ Proposed refactor — read cookie store once
- const csrfToken = await getCsrfTokenOnServer() - const requestCookieHeader = await getRequestCookieHeaderOnServer() + const cookieStore = await cookies() + const csrfCookie = cookieStore.get('csrftoken') + const csrfToken = csrfCookie?.value ?? await fetchCsrfTokenServer() + const requestCookieHeader = cookieStore + .getAll() + .map(({ name, value }) => `${name}=${value}`) + .join('; ')This keeps the two exported helpers intact for external consumers while avoiding the duplicate
cookies()call inside the hot auth-link path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/server/apolloClient.ts` around lines 9 - 10, Both getCsrfTokenOnServer and getRequestCookieHeaderOnServer individually call await cookies(), causing two redundant cookies() reads per request in the Apollo setContext path; fix by reading the cookie store once inside the setContext callback (call cookies() once), then pass that single cookie store into both helpers (either by adding an optional parameter to getCsrfTokenOnServer/getRequestCookieHeaderOnServer or by creating an internal helper that accepts the cookie store) so the exported helpers remain unchanged for external callers but the hot path re-uses the same cookie store.
52-59:getRequestCookieHeaderOnServeris exported but used only internally.It's called only within
createApolloClientin the same file. Removing the export would reduce unnecessary public API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/server/apolloClient.ts` around lines 52 - 59, The helper getRequestCookieHeaderOnServer is exported but only used internally by createApolloClient; remove the unnecessary export to shrink the public API surface by changing getRequestCookieHeaderOnServer to a module-private function (e.g., drop the export keyword or convert it to a non-exported const/async function) and keep its call sites (createApolloClient) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/server/apolloCookieHeader.ts`:
- Around line 20-22: Replace the falsy check on csrfToken with an explicit null
comparison so only a null value triggers the early return: change the condition
from if (!csrfToken) to if (csrfToken === null) in the code path that currently
returns existingCookies.join('; '); this ensures empty-string CSRF tokens are
treated as valid values and remain consistent with mergeApolloHeadersWithCsrf
which may set X-CSRFToken to ''.
---
Nitpick comments:
In `@frontend/__tests__/unit/server/apolloCookieHeader.test.ts`:
- Around line 3-71: Missing unit test for csrfToken = '': add a test case to
ensure mergeCookieHeader and mergeApolloHeadersWithCsrf treat an empty-string
CSRF token like null (preserve existing cookies including any stale csrftoken
and set X-CSRFToken to ''). Add a new test similar to the "preserves incoming
cookies when csrf token is missing" case but pass '' as the csrf argument,
assert that the Cookie header remains unchanged (including any existing
csrftoken) and that headers['X-CSRFToken'] === '' and that no duplicate
csrftoken entries are created (use the existing mergeCookieHeader helper in the
test to validate deduplication).
In `@frontend/src/server/apolloClient.ts`:
- Around line 9-10: Both getCsrfTokenOnServer and getRequestCookieHeaderOnServer
individually call await cookies(), causing two redundant cookies() reads per
request in the Apollo setContext path; fix by reading the cookie store once
inside the setContext callback (call cookies() once), then pass that single
cookie store into both helpers (either by adding an optional parameter to
getCsrfTokenOnServer/getRequestCookieHeaderOnServer or by creating an internal
helper that accepts the cookie store) so the exported helpers remain unchanged
for external callers but the hot path re-uses the same cookie store.
- Around line 52-59: The helper getRequestCookieHeaderOnServer is exported but
only used internally by createApolloClient; remove the unnecessary export to
shrink the public API surface by changing getRequestCookieHeaderOnServer to a
module-private function (e.g., drop the export keyword or convert it to a
non-exported const/async function) and keep its call sites (createApolloClient)
unchanged.
|
you have to address sonarcloud issues |
Thanks ,SonarCloud shows Quality Gate passed with 1 new issue. I’ll review it and address it. |
|
Good fix — the root cause was correctly identified. The old code was setting A couple things I'd flag though: On the cookie merging side, |
Thanks for the review. I agree on least-privilege and header-size concerns, but I didn’t |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please resolve conflicts
@arkid15r Conflicts have been resolved and the branch is up to date with main. Please let me know if any further changes are required. |
|
4 participants
Proposed change
Resolves #3940
This PR fixes SSR Apollo cookie propagation so authenticated context is preserved during server-side GraphQL requests.
What changed
frontend/src/server/apolloCookieHeader.tswith utilities to merge cookies safely and keep exactly onecsrftokenvalue.frontend/src/server/apolloClient.tsto build request cookies fromnext/headers(cookies().getAll()), then merge with server CSRF token viamergeApolloHeadersWithCsrf(...).frontend/__tests__/unit/server/apolloCookieHeader.test.tsfor:csrftokenwithout duplicates,Cookie/cookie,Verification
pnpm --dir frontend exec jest __tests__/unit/server/apolloCookieHeader.test.ts --runInBand --coverage=falsePASS(6/6 tests)Checklist
make check-testlocally: all warnings addressed, tests passed