From d7a036b99780bb7600892025f2d5bf103526054d Mon Sep 17 00:00:00 2001 From: nazarli-shabnam Date: Fri, 10 Jul 2026 13:53:03 +0400 Subject: [PATCH] fix(auth): move refresh token out of localStorage into an httpOnly cookie 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 --- api/app/app/modules/auth/router.py | 72 +++++++++++++++---- api/app/app/modules/auth/schemas.py | 21 +++--- .../src/components/layout/DashboardLayout.tsx | 6 +- apps/frontend/src/lib/api.test.ts | 24 ++++--- apps/frontend/src/lib/api.ts | 53 ++++++-------- .../src/pages/auth/AuthCallbackPage.tsx | 12 ++-- apps/frontend/src/pages/auth/LoginPage.tsx | 8 +-- apps/frontend/src/stores/authStore.ts | 21 ++---- 8 files changed, 123 insertions(+), 94 deletions(-) diff --git a/api/app/app/modules/auth/router.py b/api/app/app/modules/auth/router.py index 4b8e888..0795f45 100644 --- a/api/app/app/modules/auth/router.py +++ b/api/app/app/modules/auth/router.py @@ -1,6 +1,6 @@ import secrets -from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from fastapi.responses import RedirectResponse from sqlalchemy.orm import Session @@ -28,13 +28,12 @@ EmailVerificationConfirm, EmailVerificationRequest, LoginRequest, - LogoutRequest, LogoutResponse, PasswordResetConfirm, PasswordResetRequest, - RefreshRequest, SimpleStatusResponse, Token, + TokenResponse, ) from app.modules.auth.service import ( get_or_create_oauth_user, @@ -49,12 +48,31 @@ router = APIRouter(prefix="/auth", tags=["auth"]) +REFRESH_COOKIE_NAME = "refresh_token" +# Scoped to /api/auth so it's only ever sent on refresh/logout, not on +# every request to the API. +REFRESH_COOKIE_PATH = "/api/auth" -def _oauth_redirect_url(token: Token, invite_token: str | None) -> str: + +def _set_refresh_cookie(response: Response, refresh_token: str) -> None: + response.set_cookie( + key=REFRESH_COOKIE_NAME, + value=refresh_token, + max_age=settings.refresh_token_expire_days * 24 * 60 * 60, + httponly=True, + secure=settings.environment == "production", + samesite="lax", + path=REFRESH_COOKIE_PATH, + ) + + +def _clear_refresh_cookie(response: Response) -> None: + response.delete_cookie(key=REFRESH_COOKIE_NAME, path=REFRESH_COOKIE_PATH) + + +def _oauth_redirect_url(access_token: str, invite_token: str | None) -> str: base = f"{settings.frontend_url.rstrip('/')}/auth/callback" - params = [f"token={token.access_token}"] - if token.refresh_token: - params.append(f"refresh_token={token.refresh_token}") + params = [f"token={access_token}"] if invite_token: params.append(f"invite_token={invite_token}") return f"{base}?{'&'.join(params)}" @@ -67,9 +85,10 @@ def get_current_user_profile( return to_user_response(current_user) -@router.post("/login", response_model=Token) +@router.post("/login", response_model=TokenResponse) def login_endpoint( data: LoginRequest, + response: Response, db: Session = Depends(get_db), _rl: None = Depends(enforce_login_rate_limit), ) -> Token: @@ -79,23 +98,36 @@ def login_endpoint( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid email or password", ) + if token.refresh_token: + _set_refresh_cookie(response, token.refresh_token) return token -@router.post("/refresh", response_model=Token) -def refresh_endpoint(data: RefreshRequest) -> Token: - token = rotate_refresh_token(data.refresh_token) +@router.post("/refresh", response_model=TokenResponse) +def refresh_endpoint(request: Request, response: Response) -> Token: + refresh_token = request.cookies.get(REFRESH_COOKIE_NAME) + if not refresh_token: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Missing refresh token", + ) + token = rotate_refresh_token(refresh_token) if not token: + _clear_refresh_cookie(response) raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or expired refresh token", ) + if token.refresh_token: + _set_refresh_cookie(response, token.refresh_token) return token @router.post("/logout", response_model=LogoutResponse) -def logout_endpoint(data: LogoutRequest) -> LogoutResponse: - logout(data.refresh_token) +def logout_endpoint(request: Request, response: Response) -> LogoutResponse: + refresh_token = request.cookies.get(REFRESH_COOKIE_NAME) + logout(refresh_token) + _clear_refresh_cookie(response) return LogoutResponse() @@ -205,7 +237,12 @@ async def github_callback( token_pair = issue_token_pair(str(user.id)) raw_invite = state_data.get("invite_token") invite_token = raw_invite if isinstance(raw_invite, str) and raw_invite else None - return RedirectResponse(url=_oauth_redirect_url(token_pair, invite_token)) + redirect = RedirectResponse( + url=_oauth_redirect_url(token_pair.access_token, invite_token) + ) + if token_pair.refresh_token: + _set_refresh_cookie(redirect, token_pair.refresh_token) + return redirect @router.get("/google") @@ -246,4 +283,9 @@ async def google_callback( token_pair = issue_token_pair(str(user.id)) raw_invite = state_data.get("invite_token") invite_token = raw_invite if isinstance(raw_invite, str) and raw_invite else None - return RedirectResponse(url=_oauth_redirect_url(token_pair, invite_token)) + redirect = RedirectResponse( + url=_oauth_redirect_url(token_pair.access_token, invite_token) + ) + if token_pair.refresh_token: + _set_refresh_cookie(redirect, token_pair.refresh_token) + return redirect diff --git a/api/app/app/modules/auth/schemas.py b/api/app/app/modules/auth/schemas.py index cff050c..0e678ac 100644 --- a/api/app/app/modules/auth/schemas.py +++ b/api/app/app/modules/auth/schemas.py @@ -2,22 +2,27 @@ class Token(BaseModel): + """Internal shape returned by the auth service layer. Never sent to + the client as-is -- routes use TokenResponse (no refresh_token) and + set the refresh token as an httpOnly cookie instead.""" + access_token: str token_type: str = "bearer" refresh_token: str | None = None -class LoginRequest(BaseModel): - email: EmailStr - password: str - +class TokenResponse(BaseModel): + """What actually goes in the JSON body of login/refresh/OAuth + responses. The refresh token travels only via the httpOnly + `refresh_token` cookie, never in a response client-side JS can read.""" -class RefreshRequest(BaseModel): - refresh_token: str + access_token: str + token_type: str = "bearer" -class LogoutRequest(BaseModel): - refresh_token: str | None = None +class LoginRequest(BaseModel): + email: EmailStr + password: str class LogoutResponse(BaseModel): diff --git a/apps/frontend/src/components/layout/DashboardLayout.tsx b/apps/frontend/src/components/layout/DashboardLayout.tsx index a288d24..eeba841 100644 --- a/apps/frontend/src/components/layout/DashboardLayout.tsx +++ b/apps/frontend/src/components/layout/DashboardLayout.tsx @@ -9,8 +9,6 @@ import { useWorkspaces } from "@/hooks/useWorkspaces"; import { useAuthStore } from "@/stores/authStore"; import { useDashboardStore } from "@/stores/dashboardStore"; -const API_BASE = import.meta.env.VITE_API_URL || "http://localhost:8000"; - export interface DashboardOutletContext { workspaces: { id: string; name: string; owner_id: string }[]; selectedWorkspaceId: string | null; @@ -58,9 +56,7 @@ export function DashboardLayout() { navigate("/login"); return; } - fetch(`${API_BASE}/api/auth/me`, { - headers: { Authorization: `Bearer ${token}` }, - }) + apiFetch("/api/auth/me") .then((res) => { if (!res.ok) { logout(); diff --git a/apps/frontend/src/lib/api.test.ts b/apps/frontend/src/lib/api.test.ts index a27480d..1de81a0 100644 --- a/apps/frontend/src/lib/api.test.ts +++ b/apps/frontend/src/lib/api.test.ts @@ -15,7 +15,6 @@ describe("apiFetch refresh error handling", () => { vi.useFakeTimers(); useAuthStore.setState({ token: "old-access-token", - refreshToken: "refresh-token-1", user: null, }); }); @@ -43,8 +42,9 @@ describe("apiFetch refresh error handling", () => { expect(result.status).toBe(401); // A network-level failure during refresh must NOT be treated as a - // confirmed-dead session: auth state should be untouched. - expect(useAuthStore.getState().refreshToken).toBe("refresh-token-1"); + // confirmed-dead session: the access token should be untouched. The + // refresh token itself lives only in an httpOnly cookie the browser + // manages -- there's nothing client-side to assert on for it. expect(useAuthStore.getState().token).toBe("old-access-token"); expect(fetchMock).toHaveBeenCalledTimes(3); }); @@ -59,7 +59,6 @@ describe("apiFetch refresh error handling", () => { const result = await apiFetch("/api/boards"); expect(result.status).toBe(401); - expect(useAuthStore.getState().refreshToken).toBeNull(); expect(useAuthStore.getState().token).toBeNull(); }); @@ -68,9 +67,7 @@ describe("apiFetch refresh error handling", () => { .fn() .mockResolvedValueOnce(new Response(null, { status: 401 })) .mockRejectedValueOnce(new TypeError("network error")) - .mockResolvedValueOnce( - jsonResponse({ access_token: "new-token", refresh_token: "refresh-token-2" }), - ) + .mockResolvedValueOnce(jsonResponse({ access_token: "new-token" })) .mockResolvedValueOnce(new Response(null, { status: 200 })); vi.stubGlobal("fetch", fetchMock); @@ -80,6 +77,17 @@ describe("apiFetch refresh error handling", () => { expect(result.status).toBe(200); expect(useAuthStore.getState().token).toBe("new-token"); - expect(useAuthStore.getState().refreshToken).toBe("refresh-token-2"); + }); + + it("sends credentials: include so the httpOnly refresh_token cookie is attached", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 200 })); + vi.stubGlobal("fetch", fetchMock); + + await apiFetch("/api/boards"); + + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit]; + expect(init.credentials).toBe("include"); }); }); diff --git a/apps/frontend/src/lib/api.ts b/apps/frontend/src/lib/api.ts index 15dca06..d094997 100644 --- a/apps/frontend/src/lib/api.ts +++ b/apps/frontend/src/lib/api.ts @@ -59,14 +59,13 @@ async function refreshAccessToken(): Promise { if (refreshInFlight) return refreshInFlight; const run = async (): Promise => { - const refreshToken = useAuthStore.getState().refreshToken; - if (!refreshToken) return null; - + // The refresh token itself is never visible to JS -- it lives only + // in the httpOnly `refresh_token` cookie (scoped to /api/auth) and + // is sent automatically by the browser via credentials: "include". const attemptRefresh = () => fetch(`${API_BASE}/api/auth/refresh`, { method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ refresh_token: refreshToken }), + credentials: "include", }); let res: Response; @@ -91,22 +90,17 @@ async function refreshAccessToken(): Promise { if (!res.ok) { // A real response from the server saying the refresh token itself - // is invalid/expired -- this is a confirmed-dead session. + // is invalid/expired (or there was no cookie at all) -- this is a + // confirmed-dead session. useAuthStore.getState().logout(); return null; } - const data: { - access_token?: string; - refresh_token?: string | null; - } = await res.json(); + const data: { access_token?: string } = await res.json(); if (!data.access_token) { useAuthStore.getState().logout(); return null; } - useAuthStore.getState().setTokens({ - token: data.access_token, - refreshToken: data.refresh_token ?? null, - }); + useAuthStore.getState().setToken(data.access_token); return data.access_token; }; @@ -117,17 +111,13 @@ async function refreshAccessToken(): Promise { } export async function logoutAndClear(): Promise { - const refreshToken = useAuthStore.getState().refreshToken; - if (refreshToken) { - try { - await fetch(`${API_BASE}/api/auth/logout`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ refresh_token: refreshToken }), - }); - } catch { - // Clear locally even if the server call fails; refresh TTL bounds damage. - } + try { + await fetch(`${API_BASE}/api/auth/logout`, { + method: "POST", + credentials: "include", + }); + } catch { + // Clear locally even if the server call fails; refresh TTL bounds damage. } useAuthStore.getState().logout(); } @@ -139,19 +129,20 @@ export async function apiFetch( const token = getToken(); const response = await fetch(`${API_BASE}${path}`, { ...options, + credentials: "include", headers: buildHeaders(options.headers, token), }); - // Skip the refresh path itself to avoid infinite recursion. - if ( - response.status === 401 && - !path.startsWith("/api/auth/refresh") && - useAuthStore.getState().refreshToken - ) { + // Skip the refresh path itself to avoid infinite recursion. There's no + // client-visible way to know whether a refresh_token cookie exists, so + // any other 401 is worth one refresh attempt -- the backend just + // returns 401 again immediately if there's no cookie to use. + if (response.status === 401 && !path.startsWith("/api/auth/refresh")) { const fresh = await refreshAccessToken(); if (fresh) { return fetch(`${API_BASE}${path}`, { ...options, + credentials: "include", headers: buildHeaders(options.headers, fresh), }); } diff --git a/apps/frontend/src/pages/auth/AuthCallbackPage.tsx b/apps/frontend/src/pages/auth/AuthCallbackPage.tsx index d91566b..74f2336 100644 --- a/apps/frontend/src/pages/auth/AuthCallbackPage.tsx +++ b/apps/frontend/src/pages/auth/AuthCallbackPage.tsx @@ -1,6 +1,7 @@ /** - * OAuth callback — API redirects here with ?token=...&refresh_token=... - * Stores the token pair and navigates to the dashboard. + * OAuth callback — API redirects here with ?token=... . The refresh + * token is never in this URL: the backend sets it as an httpOnly cookie + * directly on the redirect response, before the browser ever lands here. */ import { useEffect } from "react"; import { useNavigate, useSearchParams } from "react-router-dom"; @@ -10,9 +11,8 @@ import { useAuthStore } from "@/stores/authStore"; export function AuthCallbackPage() { const [searchParams] = useSearchParams(); const navigate = useNavigate(); - const setTokens = useAuthStore((s) => s.setTokens); + const setToken = useAuthStore((s) => s.setToken); const token = searchParams.get("token"); - const refreshToken = searchParams.get("refresh_token"); const inviteToken = searchParams.get("invite_token"); const API_BASE = import.meta.env.VITE_API_URL || "http://localhost:8000"; @@ -22,7 +22,7 @@ export function AuthCallbackPage() { return; } - setTokens({ token, refreshToken: refreshToken ?? null }); + setToken(token); const acceptMaybe = async () => { if (inviteToken) { await fetch( @@ -39,7 +39,7 @@ export function AuthCallbackPage() { navigate("/dashboard", { replace: true }); }; void acceptMaybe(); - }, [token, refreshToken, inviteToken, navigate, setTokens, API_BASE]); + }, [token, inviteToken, navigate, setToken, API_BASE]); return (
diff --git a/apps/frontend/src/pages/auth/LoginPage.tsx b/apps/frontend/src/pages/auth/LoginPage.tsx index 241d4c2..13812f4 100644 --- a/apps/frontend/src/pages/auth/LoginPage.tsx +++ b/apps/frontend/src/pages/auth/LoginPage.tsx @@ -13,7 +13,7 @@ export function LoginPage() { const { t } = useI18n(); const navigate = useNavigate(); const [searchParams] = useSearchParams(); - const setTokens = useAuthStore((s) => s.setTokens); + const setToken = useAuthStore((s) => s.setToken); const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); const [error, setError] = useState(""); @@ -26,6 +26,7 @@ export function LoginPage() { try { const res = await fetch(`${API_BASE}/api/auth/login`, { method: "POST", + credentials: "include", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ email, password }), }); @@ -34,10 +35,7 @@ export function LoginPage() { setError(formatApiError(data.detail, "Login failed")); return; } - setTokens({ - token: data.access_token, - refreshToken: data.refresh_token ?? null, - }); + setToken(data.access_token); const inviteToken = searchParams.get("invite_token"); if (inviteToken) { const acceptRes = await fetch( diff --git a/apps/frontend/src/stores/authStore.ts b/apps/frontend/src/stores/authStore.ts index cfdb5d7..98ebc56 100644 --- a/apps/frontend/src/stores/authStore.ts +++ b/apps/frontend/src/stores/authStore.ts @@ -13,15 +13,12 @@ export interface AuthUser { } interface AuthState { + // Short-lived access token only. The refresh token lives exclusively + // in an httpOnly cookie set by the backend -- it's never readable by + // JS, so it has no place in this client-side store. token: string | null; - refreshToken: string | null; user: AuthUser | null; setToken: (token: string | null) => void; - setRefreshToken: (token: string | null) => void; - setTokens: (tokens: { - token: string | null; - refreshToken?: string | null; - }) => void; setUser: (user: AuthUser | null) => void; logout: () => void; } @@ -30,22 +27,14 @@ export const useAuthStore = create()( persist( (set) => ({ token: null, - refreshToken: null, user: null, setToken: (token) => set({ token }), - setRefreshToken: (refreshToken) => set({ refreshToken }), - setTokens: ({ token, refreshToken }) => - set((s) => ({ - token, - refreshToken: - refreshToken === undefined ? s.refreshToken : refreshToken, - })), setUser: (user) => set({ user }), - logout: () => set({ token: null, refreshToken: null, user: null }), + logout: () => set({ token: null, user: null }), }), { name: "loomy-auth", - partialize: (s) => ({ token: s.token, refreshToken: s.refreshToken }), + partialize: (s) => ({ token: s.token }), }, ), );