Update Cookies (Error Coming because of different domain names of fro…#21
Conversation
…ntend and backend ), the csrf header was not being able to set by the frontend
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR refactors CSRF token validation from database-backed hash verification to a simpler cookie-header comparison approach. It removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the auth cookie/CORS/CSRF setup to better support frontend/backend running on different (sub)domains, and simplifies CSRF verification logic.
Changes:
- Removed
csrfTokenHashpersistence/usage from session creation, refresh flow, and theSessionmodel. - Updated cookie configuration to optionally set a production
domain(viaPARENT_DOMAIN) and reformatted cookie-setting helpers. - Simplified CSRF middleware to validate
x-csrf-tokenagainst thecsrf-tokencookie and tightened origin allowlisting; updated CORS origin handling to a callback-based allowlist.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/auth/utils/createSessionPayload.ts | Stops storing CSRF token hash in new session payloads. |
| src/modules/auth/auth.service.ts | Removes CSRF hash selection/update during refresh; still rotates CSRF token returned to client. |
| src/modules/auth/auth.controller.ts | Adds production cookie domain support and refactors cookie-setting formatting. |
| src/models/Session.ts | Removes csrfTokenHash from the session schema. |
| src/middleware/security/requireCsrf.ts | Replaces DB-backed CSRF verification with cookie/header equality + origin check. |
| src/config/corsConfig.ts | Switches CORS origin to a callback allowlist behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const origin = req.headers.origin as string | undefined; | ||
|
|
||
| if (origin && !ALLOWED_ORIGINS.includes(origin)) { | ||
| throw new UnauthorizedError('Invalid origin'); | ||
| } |
There was a problem hiding this comment.
req.headers.origin can be string | string[] | undefined at runtime (multiple Origin headers). Casting to string can cause false rejections (array won’t match the allowlist). Normalize it the same way you do for x-csrf-token (pick the first element if it’s an array) before checking ALLOWED_ORIGINS.
| callback(null, true); | ||
| } else { | ||
| console.warn('Blocked by CORS:', origin); | ||
| return callback(null, false); |
There was a problem hiding this comment.
In the CORS origin callback, callback(null, false) disables CORS headers but still allows the request to continue through the app (it’s not an actual rejection). If the intent is to actively reject disallowed origins (especially to fail preflights loudly and reduce server work), consider passing an error to the callback (or returning a 403) instead of false.
| return callback(null, false); | |
| return callback(new Error(`Blocked by CORS: ${origin}`)); |
| const cookieOptions: CookieOptions = { | ||
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', | ||
| domain: process.env.NODE_ENV === 'production' ? process.env.PARENT_DOMAIN : undefined, | ||
| maxAge: ACCESS_AGE, | ||
| }; |
There was a problem hiding this comment.
cookieOptions now sets a domain in production, but logoutController still clears cookies without specifying the same domain/secure/sameSite options. In browsers, clearCookie() won’t remove a cookie that was set with a Domain attribute unless you clear it with matching options, which can leave auth cookies stuck in prod. Consider centralizing cookie options for both set + clear (and ensure PARENT_DOMAIN is validated so this doesn’t silently misconfigure cookies).
| const csrfCookie = req.cookies['csrf-token']; | ||
|
|
||
| try { | ||
| verifyToken(csrfTokenValue, session.csrfTokenHash); | ||
| } catch { | ||
| throw new UnauthorizedError(); | ||
| if (!csrfCookie || csrfCookie !== csrfToken) { | ||
| throw new UnauthorizedError('Invalid CSRF token'); | ||
| } |
There was a problem hiding this comment.
This middleware no longer validates the CSRF token against a server-side secret (previously SessionModel + csrfTokenHash), and instead accepts any request where the header matches a non-HttpOnly cookie. That’s a security regression because it removes per-session binding and makes CSRF protection fully dependent on the attacker being unable to set a cookie for the site (which becomes more relevant now that cookies can be shared via Domain). Consider reintroducing session-bound verification (DB-backed hash, or an HMAC/signed token derived from sessionId/refresh token) rather than raw equality.
…ntend and backend ), the csrf header was not being able to set by the frontend
Summary by CodeRabbit
Security
Chores