🔒 [security fix] Implement HttpOnly cookie storage for auth tokens#14
Conversation
This commit moves authentication token storage from `localStorage` to secure, `HttpOnly` cookies to mitigate XSS risks. - Updated backend to set `auth_token` cookie on login/register. - Added `/api/auth/me` for session recovery and `/api/auth/logout` to clear cookies. - Configured CORS and `cookie-parser` for secure cookie handling. - Updated frontend `ApiClient` to use `credentials: 'include'` and removed `localStorage` usage. - Added `checkAuthIntent` to restore session on app load. Co-authored-by: davidraehles <6085055+davidraehles@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR implements a security enhancement by migrating authentication token storage from localStorage to HttpOnly cookies, significantly reducing XSS attack surface. The implementation adds cookie-based session management with proper backend endpoints (/me, /logout) and updates the frontend to work seamlessly with cookie-based authentication.
Changes:
- Migrated from localStorage to HttpOnly cookies with Secure and SameSite attributes for token storage
- Added backend session endpoints (
GET /api/auth/me,POST /api/auth/logout) and cookie-parser middleware - Updated frontend to include credentials in requests and handle cookie-based session lifecycle
- Added comprehensive security documentation in .jules/sentinel.md
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
backend/src/server.ts |
Added cookie-parser middleware and configured CORS with credentials support |
backend/src/routes/auth.ts |
Implemented HttpOnly cookie setting in login/register, added /me and /logout endpoints |
backend/src/middleware/auth.ts |
Enhanced to accept tokens from both Authorization header and cookies |
backend/package.json |
Added cookie-parser dependency |
backend/pnpm-lock.yaml |
Lockfile for new backend dependencies |
frontend/src/services/api.ts |
Removed localStorage usage, added credentials to fetch, added getMe() and logout() methods |
frontend/src/services/__tests__/api.test.ts |
Updated tests to verify credentials in requests and test new endpoints |
frontend/src/intents/authIntents.ts |
Added checkAuthIntent for session recovery, updated logout to call backend endpoint |
frontend/src/App.tsx |
Added checkAuthIntent call on app initialization |
frontend/pnpm-lock.yaml |
Lockfile changes (no new frontend dependencies) |
.jules/sentinel.md |
Security best practices documentation |
Files not reviewed (2)
- backend/pnpm-lock.yaml: Language not supported
- frontend/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
backend/src/routes/auth.ts:103
- The backend still returns a token in the response body for login and register endpoints (lines 100-103 and 52-56), but this token is redundant since authentication is now handled via HttpOnly cookies. The token in the response body creates a security concern as it's accessible to JavaScript. Consider removing the token from the response body to enforce cookie-only authentication and eliminate XSS token exposure risk.
res.json({
user: {
id: user.id,
email: user.email,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| res.cookie('auth_token', token, { | ||
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: 'strict', | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, // 7 days | ||
| }); |
There was a problem hiding this comment.
Missing CSRF protection. When using cookies for authentication, the application becomes vulnerable to Cross-Site Request Forgery (CSRF) attacks. With SameSite: Strict, some protection exists, but consider implementing additional CSRF tokens for state-changing operations (POST, PUT, DELETE) to provide defense-in-depth, especially if SameSite policies might be relaxed in the future.
There was a problem hiding this comment.
Acknowledged. SameSite: Strict provides strong CSRF protection for this use case. Full CSRF token implementation is out of scope for this PR — tracking as a follow-up.
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: 'strict', | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, // 7 days |
There was a problem hiding this comment.
The cookie attributes should include the path attribute set to '/' to ensure the cookie is sent with all requests to the application. While browsers typically default to '/', explicitly setting it makes the intent clear and prevents potential issues with cookie scoping.
| maxAge: 7 * 24 * 60 * 60 * 1000, // 7 days | |
| maxAge: 7 * 24 * 60 * 60 * 1000, // 7 days | |
| path: '/', |
There was a problem hiding this comment.
Added path: '/' to both cookie options (register and login routes).
| try { | ||
| // Hash password client-side for verification | ||
| const passwordHash = await hashPassword(credentials.password); | ||
| await hashPassword(credentials.password); |
There was a problem hiding this comment.
The password hash result is computed but not used. The variable assignment on line 16 was removed but the await call remains. While this doesn't affect functionality (the hashing still validates the password format), the result is discarded. Consider either using the hash or removing this line if client-side hashing is no longer needed for verification.
There was a problem hiding this comment.
Removed the unused await hashPassword() call and its import. Client-side hashing serves no purpose since the backend hashes the password for storage.
| export const checkAuthIntent = createAsyncThunk( | ||
| 'auth/checkAuth', | ||
| async (_, { dispatch, rejectWithValue }) => { | ||
| try { | ||
| const result = await apiClient.getMe(); | ||
| // Even without a token in memory, the browser sends the cookie. | ||
| // We don't have the token string to put in Redux, but we have the user. | ||
| dispatch(setAuth({ user: result.user, token: '' })); | ||
| return result; | ||
| } catch (error) { | ||
| return rejectWithValue( | ||
| error instanceof Error ? error.message : 'Check auth failed' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
When the checkAuthIntent is dispatched on app initialization and the cookie is invalid or expired, the /api/auth/me endpoint will return a 401 error. This error is caught but there's no clearing of authentication state. The app should explicitly clear auth state when checkAuthIntent fails to ensure the user is redirected to login if the session is invalid.
There was a problem hiding this comment.
Added apiClient.setToken(null) and dispatch(clearAuth()) in the catch block so auth state is explicitly cleared when /api/auth/me fails.
…lear auth state on failure
🎯 What: Fixed insecure authentication token storage by moving from
⚠️ Risk: Storing tokens in
localStoragetoHttpOnlycookies.localStoragemakes them accessible to any JavaScript running on the page, allowing attackers to steal sessions via XSS.🛡️ Solution: Implemented
HttpOnly,Secure, andSameSite: Strictcookies for token storage. Added backend endpoints for session verification and logout, and updated the frontend to handle the cookie-based session lifecycle.PR created automatically by Jules for task 16227886763176566062 started by @davidraehles