CSRF Update#24
Conversation
… DB checks for csrf only JWT stateless checking
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis pull request refactors CSRF token handling across the authentication flow. CSRF tokens transition from simple hex strings to JWT-based tokens bound to sessions and users. The CSRF middleware adds strict origin/referer validation for unsafe HTTP methods. CORS configuration adds PATCH method support, and token generation logic is redistributed across auth and session modules. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as CSRF Middleware<br/>(requireCsrf)
participant JWT as JWT Verifier
participant SessionStore as Session/User Store
participant Handler as Route Handler
Client->>Middleware: Unsafe Request<br/>(PUT/POST/DELETE/PATCH)<br/>with csrf-token header<br/>and csrf_token cookie
Middleware->>Middleware: Validate origin/<br/>referer header present
alt Missing origin/referer
Middleware->>Client: 401 UnauthorizedError
end
Middleware->>Middleware: Validate origin<br/>format & allowlist
alt Invalid/disallowed origin
Middleware->>Client: 401 UnauthorizedError
end
Middleware->>Middleware: Extract CSRF token<br/>from header & cookie
Middleware->>JWT: jwt.verify(token,<br/>CSRF_TOKEN_SECRET)
alt JWT verification fails
JWT->>Middleware: Error
Middleware->>Client: 401 UnauthorizedError
end
JWT->>Middleware: Decoded payload<br/>{userId, sessionId}
Middleware->>SessionStore: Verify decoded.sessionId<br/>matches req.sessionId
alt Session mismatch
Middleware->>Client: 401 UnauthorizedError
end
Middleware->>SessionStore: If present in token,<br/>verify decoded.userId<br/>equals req.user?.id
alt User binding mismatch
Middleware->>Client: 401 UnauthorizedError
end
Middleware->>Handler: next() - Token valid
Handler->>Client: Proceed with request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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 updates the CSRF implementation by moving from random CSRF tokens to JWT-based CSRF tokens and adjusts related cookie/refresh-session behavior across the auth flow.
Changes:
- Introduces JWT-signed CSRF tokens with dedicated secret/expiry env vars.
- Renames the CSRF cookie to
csrf_tokenand updates CSRF middleware validation accordingly. - Adjusts token refresh response/cookie setting and expands CORS methods to include
PATCH.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/tokens.ts | Adds CSRF JWT env config and updates CSRF token generation. |
| src/utils/generateAllCookieTokens.ts | Stops returning CSRF token from the “all cookie tokens” helper. |
| src/types/Auth.ts | Removes csrfToken from RefreshTokenOutput. |
| src/modules/user/user.controller.ts | Changes get-profile response formatting and imports. |
| src/modules/auth/utils/createSessionPayload.ts | Generates CSRF JWT alongside access/refresh tokens. |
| src/modules/auth/auth.service.ts | Removes CSRF token rotation from refresh-tokens service output. |
| src/modules/auth/auth.controller.ts | Renames CSRF cookie, returns CSRF token in login/register/refresh responses, updates refresh-session handling. |
| src/middleware/security/requireCsrf.ts | Switches CSRF validation to JWT verification + session/user binding. |
| src/config/corsConfig.ts | Adds PATCH to allowed CORS methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const refreshTokenController = async (req: Request, res: Response) => { | ||
| const reqRefreshToken = req.cookies['refresh-token']; | ||
| const userAgent = req.headers['user-agent'] || 'unknown'; | ||
| const ip = req.headers['x-forwarded-for'] || req.headers['x-real-ip'] || req.ip || req.socket.remoteAddress; | ||
| const csrfToken = req.cookies?.['csrf_token']; | ||
|
|
||
| if (!reqRefreshToken) { | ||
| throw new UnauthenticatedError('No refresh token, authentication failed'); | ||
| } | ||
|
|
||
| if (!csrfToken) { | ||
| throw new UnauthenticatedError('Missing CSRF token'); | ||
| } | ||
|
|
||
| const tokens = await refreshTokens(reqRefreshToken, userAgent, ip); | ||
| setAuthCookies(res, tokens); | ||
| const allTokens = { ...tokens, csrfToken }; | ||
| setAuthCookies(res, allTokens); |
There was a problem hiding this comment.
refreshTokenController only checks that the csrf_token cookie exists, but it does not require/validate an x-csrf-token header (double-submit) and does not verify the CSRF JWT against the refresh token’s session/user. A cross-site POST can still trigger a refresh because the browser will attach both cookies automatically. Require a header token (and compare to the cookie) and verify the CSRF token is bound to the refresh token’s decoded sessionId/userId (or otherwise apply CSRF middleware logic tailored for refresh).
| .cookie('access-token', tokens.accessToken, { ...cookieOptions, maxAge: ACCESS_AGE }) | ||
| .cookie('refresh-token', tokens.refreshToken, { ...cookieOptions, maxAge: REFRESH_AGE }) | ||
| .cookie('csrf-token', tokens.csrfToken, { ...cookieOptions, httpOnly: false, maxAge: CSRF_AGE }); | ||
| .cookie('csrf_token', tokens.csrfToken, { | ||
| ...cookieOptions, | ||
| httpOnly: false, | ||
| maxAge: REFRESH_AGE, | ||
| }); |
There was a problem hiding this comment.
Cookie name changed from csrf-token to csrf_token. This will break existing logged-in sessions after deploy because clients will still have the old cookie, and csrfMiddleware now only reads csrf_token. Consider a migration period where you accept both cookie names (and/or set/clear both) so existing sessions can continue without forcing users to re-authenticate.
| .clearCookie('access-token') | ||
| .clearCookie('refresh-token') | ||
| .clearCookie('csrf-token') | ||
| .clearCookie('csrf_token') |
There was a problem hiding this comment.
logoutController now clears only csrf_token. If a user still has the legacy csrf-token cookie, it will remain set after logout. Consider clearing both cookie names during the transition to avoid leaving stale CSRF cookies behind.
| .clearCookie('csrf_token') | |
| .clearCookie('csrf_token') | |
| .clearCookie('csrf-token') |
| const headerTokenRaw = req.headers['x-csrf-token']; | ||
| const headerToken = Array.isArray(headerTokenRaw) ? headerTokenRaw[0] : headerTokenRaw; | ||
|
|
||
| const csrfCookieRaw = req.cookies['csrf-token']; | ||
| const cookieToken = req.cookies?.['csrf_token']; | ||
|
|
||
| if (!csrfHeader || !csrfCookieRaw) { | ||
| return next(new UnauthorizedError('CSRF credentials missing')); | ||
| if (!headerToken || !cookieToken) { | ||
| return next(new UnauthorizedError('Missing CSRF token')); | ||
| } | ||
|
|
||
| const csrfToken = normalizeToken(csrfHeader); | ||
| const csrfCookie = normalizeToken(csrfCookieRaw); | ||
| if (headerToken !== cookieToken) { | ||
| return next(new UnauthorizedError('CSRF token mismatch')); | ||
| } |
There was a problem hiding this comment.
The new CSRF check does a raw headerToken !== cookieToken comparison without normalization/decoding. In practice, cookie values can be quoted or URL-encoded by clients/proxies, which the previous normalizeToken handled; this change can cause valid requests to be rejected. Consider restoring normalization (decode + trim quotes) and using a timing-safe comparison to avoid regressions.
| import { BadRequestError, UnauthorizedError } from '@middleware/error/index.js'; | ||
|
|
||
| export const getProfileController = async (req: Request, res: Response) => { | ||
| const userId = req.user!.id; | ||
| const user = await getProfile(userId); | ||
| return sendSuccess(res, user, 'Profile retrieved successfully'); | ||
|
|
||
| return res.status(200).json({ | ||
| message: 'Profile retrieved successfully', | ||
| data: user, | ||
| }); |
There was a problem hiding this comment.
UnauthorizedError is imported but not used, and getProfileController bypasses sendSuccess, which is used throughout this controller for consistent response shaping/sanitization. Remove the unused import and consider using sendSuccess(res, user, 'Profile retrieved successfully') for consistency.
| const CSRF_TOKEN_EXPIRES_IN: SignOptions['expiresIn'] = process.env | ||
| .CSRF_TOKEN_EXPIRES_IN as SignOptions['expiresIn']; | ||
| const CSRF_TOKEN_SECRET: string = process.env.CSRF_TOKEN_SECRET!; |
There was a problem hiding this comment.
CSRF_TOKEN_EXPIRES_IN / CSRF_TOKEN_SECRET are read with non-null assertions. If these env vars are missing or misconfigured, CSRF token generation/verification will fail at runtime with a hard crash or auth errors. Consider validating required env vars at startup (or providing a safe default) so misconfiguration is detected early and reported clearly.
Updated CSRF logic
Summary by CodeRabbit
New Features
Security Enhancements