fix(auth): move refresh token out of localStorage into an httpOnly cookie - #51
Merged
Merged
Conversation
…okie The refresh token was persisted to localStorage via zustand's persist middleware alongside the access token, readable by any script running on the page. Since the refresh token rotates on use rather than expiring quickly, it effectively grants indefinite re-authentication -- a far more valuable XSS target than the short-lived access token. Backend (api/app/app/modules/auth/): - schemas.py: new TokenResponse (access_token only) is what actually goes in JSON bodies now. Token (internal, has refresh_token) is used only for service-layer return values. - router.py: login/refresh/OAuth callbacks now set the refresh token via Set-Cookie (httpOnly, Secure in production only, SameSite=Lax, scoped to /api/auth) instead of returning it in the response body. /refresh and /logout read the cookie via request.cookies instead of a request body field -- RefreshRequest/LogoutRequest are gone, no body needed. A failed refresh clears the cookie. Frontend: - authStore.ts: dropped the refreshToken field/setters entirely -- there's nothing for the client to store; the cookie is invisible to JS by design and the browser attaches it automatically. - lib/api.ts: apiFetch/refreshAccessToken/logoutAndClear all pass credentials: "include" instead of reading/sending a refresh token from state; refresh no longer needs a body. - LoginPage.tsx / AuthCallbackPage.tsx: store only the access token; the callback URL no longer carries a refresh_token query param (the backend sets the cookie directly on the OAuth redirect response, before the browser ever lands on that page). - DashboardLayout.tsx: was using a raw fetch() for /api/auth/me instead of apiFetch, bypassing the auto-refresh path entirely and logging the user out on any 401 instead of trying to refresh first (this is issue #6's "direct fetch in layout" item, not a new bug -- fixing it here because it directly blocked verifying this change and undoing the whole point of a working refresh flow). Verified end-to-end with a real browser (Playwright) against a live docker-compose stack: - After login, localStorage's persisted auth state contains only {token: ...} -- no refresh_token anywhere in it. - document.cookie is empty (httpOnly working), but the browser's actual cookie jar (page.context().cookies(), which sees httpOnly cookies) shows refresh_token with httpOnly: true, path: /api/auth, sameSite: Lax. - Corrupted the in-memory access token, reloaded: network trace shows 401 on /api/auth/me -> POST /api/auth/refresh (200) -> retried /api/auth/me succeeds (200) -- the user stays on /dashboard instead of being bounced to /login, and localStorage now holds a new access token, confirming the whole silent-refresh flow works without the refresh token ever touching JS. - Logged out and confirmed the refresh_token cookie is gone from the browser's cookie jar afterward. Backend: ruff/mypy/pytest clean (53/53). Frontend: lint/test (28/28)/ build clean. Fixes #34
📝 WalkthroughWalkthroughAuthentication now stores refresh tokens in HTTP-only ChangesAuthentication cookie flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant LoginPage
participant AuthAPI
participant AuthStore
Browser->>LoginPage: submit credentials
LoginPage->>AuthAPI: POST login with credentials included
AuthAPI-->>Browser: access token and HTTP-only refresh cookie
LoginPage->>AuthStore: setToken(access token)
Browser->>AuthAPI: request with access token and cookie
AuthAPI-->>Browser: 401 response
Browser->>AuthAPI: POST refresh with cookie credentials
AuthAPI-->>Browser: new access token and rotated cookie
Browser->>AuthStore: setToken(new access token)
Browser->>AuthAPI: retry original request
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…and formalize AI attribution policy
![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 (1)
apps/frontend/src/stores/authStore.ts (1)
35-37: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExisting sessions keep the old
refreshTokeninlocalStorage.
partializeonly affects future writes. On upgrade, a returning user's persistedloomy-authblob still contains the previously storedrefreshToken, and zustandpersistdoes not re-serialize on rehydrate — the stale token remains readable to JS until the next state change overwrites it. This leaves a window that undercuts the issue's goal of removing the refresh token from client-readable storage.Add a
versionbump +migrateto strip it on load:🛡️ Force cleanup of the legacy field
{ name: "loomy-auth", + version: 1, + migrate: (persisted) => { + if (persisted && typeof persisted === "object") { + delete (persisted as Record<string, unknown>).refreshToken; + } + return persisted as AuthState; + }, partialize: (s) => ({ token: s.token }), },Please confirm zustand
persistwrites back the migrated state on rehydrate for the installed 5.0.11.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/frontend/src/stores/authStore.ts` around lines 35 - 37, Update the zustand persist configuration in authStore’s “loomy-auth” storage to bump the persistence version and add a migrate function that removes the legacy refreshToken while preserving the supported token state. Confirm against the installed zustand 5.0.11 behavior that migrated state is written back during rehydration, and ensure future persistence remains limited to token via partialize.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/app/app/modules/auth/router.py`:
- Around line 57-66: Update _set_refresh_cookie to use SameSite=None with
secure=True when the frontend and API are configured on different registrable
domains, while retaining SameSite=Lax for same-site deployments; ensure the
cross-site configuration always enables Secure so refresh_token is sent to the
refresh and logout endpoints.
---
Nitpick comments:
In `@apps/frontend/src/stores/authStore.ts`:
- Around line 35-37: Update the zustand persist configuration in authStore’s
“loomy-auth” storage to bump the persistence version and add a migrate function
that removes the legacy refreshToken while preserving the supported token state.
Confirm against the installed zustand 5.0.11 behavior that migrated state is
written back during rehydration, and ensure future persistence remains limited
to token via partialize.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aacd894d-44f0-449f-b547-d396066cbf09
📒 Files selected for processing (8)
api/app/app/modules/auth/router.pyapi/app/app/modules/auth/schemas.pyapps/frontend/src/components/layout/DashboardLayout.tsxapps/frontend/src/lib/api.test.tsapps/frontend/src/lib/api.tsapps/frontend/src/pages/auth/AuthCallbackPage.tsxapps/frontend/src/pages/auth/LoginPage.tsxapps/frontend/src/stores/authStore.ts
|
The preview deployment for Loomy UI failed. 🔴 Open Build Logs | Open Application Logs Last updated at: 2026-07-10 10:02:05 CET |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The refresh token was persisted to
localStoragevia zustand'spersistmiddleware alongside the access token, readable by any script running on the page. Since the refresh token rotates on use rather than expiring quickly, it effectively grants indefinite re-authentication -- a far more valuable XSS target than the short-lived access token.Changes
Backend (
api/app/app/modules/auth/)schemas.py: newTokenResponse(access_token only) is what actually goes in JSON bodies now.Token(internal, still hasrefresh_token) is used only for service-layer return values.router.py: login/refresh/OAuth callbacks now set the refresh token viaSet-Cookie(httponly,securein production only,samesite=lax, scoped to/api/auth) instead of returning it in the response body./refreshand/logoutread the cookie viarequest.cookiesinstead of a request body field --RefreshRequest/LogoutRequestare gone, no body needed anymore. A failed refresh clears the cookie.Frontend
authStore.ts: dropped therefreshTokenfield/setters entirely -- there's nothing for the client to store; the cookie is invisible to JS by design and the browser attaches it automatically.lib/api.ts:apiFetch/refreshAccessToken/logoutAndClearall passcredentials: "include"instead of reading/sending a refresh token from state; refresh no longer needs a body.LoginPage.tsx/AuthCallbackPage.tsx: store only the access token; the OAuth callback URL no longer carries arefresh_tokenquery param (the backend sets the cookie directly on the redirect response, before the browser ever lands on that page).DashboardLayout.tsx: was using a rawfetch()for/api/auth/meinstead ofapiFetch, bypassing the auto-refresh path entirely and logging the user out on any 401 instead of trying to refresh first. This is issue Fix redundant React effect in the dashboard and standardize auth fetching in the layout to use the shared API client. #6's "direct fetch in layout" item, not a new bug -- I fixed it here because it directly blocked verifying this PR and defeats the whole point of a working silent-refresh flow.Test plan
ruff check .,mypy .,pytest-- all clean (53/53 backend tests)npm run lint,npm run test(28/28),npm run build-- all cleanlocalStorage's persisted auth state contains only{token: ...}-- no refresh token anywhere in itdocument.cookieis empty (httpOnly working), but the browser's actual cookie jar (which can see httpOnly cookies) showsrefresh_tokenwithhttpOnly: true,path: /api/auth,sameSite: Lax401on/api/auth/me→POST /api/auth/refresh(200) → retried/api/auth/mesucceeds (200) -- stayed on/dashboardinstead of bouncing to/login, andlocalStorageheld a new access token afterward, confirming the silent-refresh flow works entirely without the refresh token ever touching JSrefresh_tokencookie is gone from the browser's cookie jar afterwardFixes #34
Summary by CodeRabbit
Security
Authentication
Bug Fixes