Move refresh token to an httpOnly cookie, keep access token in memory (#5)#14
Merged
Merged
Conversation
… memory Addresses GitHub issue #5 (automated security review). Refresh and access tokens were stored in browser-managed storage (localforage / IndexedDB), making long-lived session credentials readable by any JavaScript in the page — a meaningful XSS blast radius for a key-management app. Server: - register @fastify/cookie. - mint the refresh token into an httpOnly, SameSite=Lax cookie (Secure in production) scoped to /api/auth, and strip it from the JSON body (issueSession). Applies to register, login, totp/verify, passkey login. - /auth/refresh reads the token from the cookie (not the body), rotates, and re-sets the cookie; /auth/logout reads the cookie, revokes the session, and clears it. Web: - stop persisting tokens in localforage; the access token lives in memory only. - restore() recovers a session on load by calling /auth/refresh (the cookie is sent automatically) and then fetching the profile. - logout() clears local state and calls the server to clear the cookie + revoke. - api client sends credentials: 'include' so the cookie travels with requests. Both dev (Vite proxy) and prod (Caddy /api* -> server) are same-origin, so SameSite=Lax covers CSRF for the state-changing auth routes. Tests: controller sets/reads/clears the cookie and never returns the refresh token; store restore()/logout() exercised against mocked fetch. Server 92/92, web 37/37; both apps type-check/build; lint (0 errors) + format:check clean. Operator note: deploying logs users out once (no persisted tokens to migrate).
…th (#5 follow-up) Follow-up to the panel review of #5 (cursor + Sonnet, both PASS; opencode unavailable that run). Addresses the actionable findings: - clearCookie now sends the same attributes (httpOnly/secure/sameSite/path) as setCookie, via a shared refreshCookieAttrs() — some browsers won't delete a Secure cookie otherwise. Also aligns the JWT_REFRESH_EXPIRES_IN default ('7d') with the service so the cookie maxAge and DB session expiry can't diverge. - await auth.logout() at every call site (AppLayout, SettingsSecurityView x3, LoginView x2): the server-side revoke + cookie clear must complete before navigation, or a fast reload could re-authenticate via the still-present cookie. - e2e clearAuthStorage now clears cookies (the session is a cookie now), and its comment is corrected. - shared-types AuthTokensSchema.refreshToken marked @deprecated (delivered via cookie, no longer in the body). - Tests: cookie set + body-stripped for login and passkey-login (incl. the verified=false early return that sets no cookie). server 95/95, web 37/37; both builds clean; lint:security + format:check clean. Deferred to a follow-up (design trade-offs, flagged by cursor as non-blocking): an api-client 401->refresh interceptor for long-lived tabs, and avoiding refresh-token rotation on every reload / multi-tab rotation races (needs a non-rotating session endpoint or reuse-detection grace).
Browser regression for the cookie change in this PR, validated against a real stack (Dockerized Postgres/Redis + Vite + Nest): - refresh token is set only as an httpOnly, SameSite=Lax cookie — never in the register/login response body and never in JS-accessible storage - a full page reload restores the session by exchanging the cookie at /api/auth/refresh (the access token is memory-only) - logout clears the cookie server-side (matching attributes), so a protected route bounces back to /login afterwards
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
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.
Phase 5 (final) of the phased security-review response. Independent of #10–#13.
Closes
Problem
The web app persisted both access and refresh tokens in browser-managed storage (
localforage/ IndexedDB) and sent the access token as a Bearer header. Any JavaScript in the page (e.g. via XSS or a malicious extension) could read long-lived session credentials — a meaningful risk for an app that manages encrypted private keys.Approach
Adopt the recommended pattern: refresh token in an httpOnly cookie, access token in memory only.
Server
@fastify/cookie.issueSession()moves the refresh token into anhttpOnly,SameSite=Laxcookie (Securein production), scoped topath=/api/auth, and strips it from the JSON body. Applied to register, login,totp/verify, and passkey login.POST /auth/refreshnow reads the refresh token from the cookie (not the body), rotates it, and re-sets the cookie; returns only{ accessToken }.POST /auth/logoutreads the cookie, revokes the session, and clears the cookie.Web
localforageis gone from the auth store.restore()recovers a session on load by calling/auth/refresh(cookie sent automatically) then fetching the profile.logout()clears local state immediately and calls the server to clear the cookie + revoke the session (previously logout was client-side only).credentials: 'include'.Why SameSite=Lax is sufficient here
Both dev (Vite proxies
/api→ :3000) and prod (Caddy proxies/api*→server:3000on the same:8080origin) are same-origin for the browser, soSameSite=Laxcookies cover CSRF on the state-changing auth routes without needing a separate CSRF token.Acceptance criteria (from the issue)
Operator note
Tests
/refreshreads/rotates from the cookie and rejects when missing;/logoutrevokes + clears. (server 92/92)restore()recovers via the refresh cookie;logout()clears state and calls the server; api client sends credentials. (web 37/37)pnpm lint0 errors;lint:securityclean;format:checkclean.Reviewer notes
security/detect-object-injectionlint warnings (not errors) remain on the cookie reads (req.cookies?.[REFRESH_COOKIE], a constant key — false positive); the dedicatedlint:securityconfig is clean.