diff --git a/backend/app/api/deps.py b/backend/app/api/deps.py index fe4fdf87..ee738c77 100644 --- a/backend/app/api/deps.py +++ b/backend/app/api/deps.py @@ -49,6 +49,20 @@ async def get_current_user( return user +async def get_refresh_token_payload( + token: Annotated[str, Depends(oauth2_scheme)] +) -> dict: + """Extract and validate a refresh token from the Authorization header.""" + payload = decode_token(token) + if payload is None or payload.get("type") != "refresh": + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid refresh token", + headers={"WWW-Authenticate": "Bearer"}, + ) + return payload + + async def get_current_active_user( current_user: Annotated[User, Depends(get_current_user)] ) -> User: diff --git a/backend/app/api/endpoints/auth.py b/backend/app/api/endpoints/auth.py index 97b79708..34bd7e75 100644 --- a/backend/app/api/endpoints/auth.py +++ b/backend/app/api/endpoints/auth.py @@ -12,13 +12,12 @@ from app.core.security import ( get_password_hash, create_access_token, create_refresh_token, - decode_token ) from app.models.user import User from app.models.invite_code import InviteCode from app.schemas.user import UserCreate, UserResponse, UserLogin from app.schemas.token import Token -from app.api.deps import get_current_user +from app.api.deps import get_current_user, get_refresh_token_payload router = APIRouter(prefix="/auth", tags=["authentication"]) @@ -154,17 +153,10 @@ async def login_json( @router.post("/refresh", response_model=Token) async def refresh_token( - refresh_token: str, + payload: Annotated[dict, Depends(get_refresh_token_payload)], db: Annotated[AsyncSession, Depends(get_db)] ): """Refresh access token using refresh token.""" - payload = decode_token(refresh_token) - if payload is None or payload.get("type") != "refresh": - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid refresh token" - ) - user_id = payload.get("sub") result = await db.execute(select(User).where(User.id == user_id)) user = result.scalar_one_or_none() diff --git a/backend/scripts/seed_trees.py b/backend/scripts/seed_trees.py index 32e54658..bd15396f 100644 --- a/backend/scripts/seed_trees.py +++ b/backend/scripts/seed_trees.py @@ -3327,19 +3327,29 @@ async def create_tree(client: httpx.AsyncClient, token: str, tree_data: dict) -> """Create a tree via the API. Returns None if tree already exists.""" headers = {"Authorization": f"Bearer {token}"} + # Mark as default/system tree (public and visible to all) + tree_data["is_default"] = True + tree_data["is_public"] = True + # Check if tree with same name exists list_response = await client.get(f"{API_BASE_URL}/trees", headers=headers) if list_response.status_code == 200: existing_trees = list_response.json() for tree in existing_trees: if tree["name"] == tree_data["name"]: + # Ensure existing trees have correct visibility flags + if not tree.get("is_public") or not tree.get("is_default"): + patch_response = await client.put( + f"{API_BASE_URL}/trees/{tree['id']}", + json={"is_public": True, "is_default": True}, + headers=headers + ) + if patch_response.status_code == 200: + print(f" [UPDATE] Tree '{tree_data['name']}' visibility updated (ID: {tree['id']})") + return None print(f" [SKIP] Tree '{tree_data['name']}' already exists (ID: {tree['id']})") return None - # Mark as default/system tree (public and visible to all) - tree_data["is_default"] = True - tree_data["is_public"] = True - # Create the tree response = await client.post( f"{API_BASE_URL}/trees", diff --git a/docs/plans/2026-02-04-token-refresh-fix-design.md b/docs/plans/2026-02-04-token-refresh-fix-design.md new file mode 100644 index 00000000..95763e25 --- /dev/null +++ b/docs/plans/2026-02-04-token-refresh-fix-design.md @@ -0,0 +1,81 @@ +# Token Refresh Fix Design + +> **Date:** 2026-02-04 +> **Status:** Approved +> **Issue:** After idle period, app shows "Failed to load trees" with 401 cascade + +--- + +## Problem + +When the access token expires (15 minutes), the Axios response interceptor attempts to refresh it by calling `POST /api/v1/auth/refresh`. This call fails because of a request/response mismatch: + +- **Frontend** sends the refresh token in the `Authorization: Bearer ` header +- **Backend** expects `refresh_token` as a bare `str` query/body parameter + +FastAPI cannot extract a bare string parameter from the Authorization header, so the refresh call returns 422 (missing required parameter). The interceptor catches this as a generic failure, but the error propagates to the calling component rather than redirecting to login — leaving the user stuck on a broken page. + +Additionally, when multiple API calls (trees, folders, categories) all return 401 simultaneously, each independently triggers its own refresh attempt, creating a race condition. + +## Root Cause + +`backend/app/api/endpoints/auth.py:156-158`: +```python +async def refresh_token( + refresh_token: str, # FastAPI expects query/body param + db: ... +) +``` + +`frontend/src/api/client.ts:37-41`: +```typescript +await axios.post(url, null, { + headers: { Authorization: `Bearer ${refreshToken}` } // Sent as header +}) +``` + +## Solution + +### 1. Backend: Add `get_refresh_token_payload` dependency + +Create a proper FastAPI dependency in `deps.py` that extracts and validates a refresh token from the Authorization header, mirroring the existing `get_current_user` pattern: + +```python +async def get_refresh_token_payload( + token: Annotated[str, Depends(oauth2_scheme)] +) -> dict: + payload = decode_token(token) + if payload is None or payload.get("type") != "refresh": + raise HTTPException(status_code=401, detail="Invalid refresh token") + return payload +``` + +### 2. Backend: Refactor refresh endpoint + +Change `refresh_token()` in `auth.py` to use the new dependency instead of a bare string parameter. + +### 3. Frontend: Add refresh queue (single-flight pattern) + +When multiple requests hit 401 simultaneously, only the first triggers the actual refresh call. Others queue up and retry with the new token once the refresh completes. + +### 4. Frontend: Sync auth store after refresh + +Add a `setTokens` action to `authStore.ts`. Call it from the interceptor after a successful refresh so the Zustand store stays consistent with localStorage. + +## Files Changed + +| File | Change | +|------|--------| +| `backend/app/api/deps.py` | Add `get_refresh_token_payload` dependency | +| `backend/app/api/endpoints/auth.py` | Use new dependency in refresh endpoint | +| `frontend/src/api/client.ts` | Refresh queue + auth store sync | +| `frontend/src/store/authStore.ts` | Add `setTokens` action | + +## Testing + +- Set `ACCESS_TOKEN_EXPIRE_MINUTES=1` temporarily to reproduce quickly +- Verify silent refresh: login, wait >1 min, interact — no errors visible +- Verify concurrent requests: page load after expiry triggers one refresh, all requests succeed +- Verify refresh token expiry: after 7 days, clean redirect to login +- Run existing backend tests (`pytest`) to confirm no regressions +- Run frontend build (`npm run build`) to confirm no compile errors diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 2cf8d380..71a43b74 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -1,4 +1,5 @@ import axios, { type AxiosError, type InternalAxiosRequestConfig } from 'axios' +import { useAuthStore } from '@/store/authStore' const API_BASE_URL = import.meta.env.VITE_API_URL || 'http://localhost:8000' @@ -21,45 +22,97 @@ apiClient.interceptors.request.use( (error) => Promise.reject(error) ) +// Refresh queue: when multiple requests hit 401 simultaneously, +// only the first triggers the actual refresh. Others wait for the result. +let isRefreshing = false +let refreshSubscribers: ((token: string) => void)[] = [] +let refreshFailSubscribers: ((error: unknown) => void)[] = [] + +function onRefreshed(token: string) { + refreshSubscribers.forEach(cb => cb(token)) + refreshSubscribers = [] + refreshFailSubscribers = [] +} + +function onRefreshFailed(error: unknown) { + refreshFailSubscribers.forEach(cb => cb(error)) + refreshSubscribers = [] + refreshFailSubscribers = [] +} + // Response interceptor - handle token refresh apiClient.interceptors.response.use( (response) => response, async (error: AxiosError) => { const originalRequest = error.config as InternalAxiosRequestConfig & { _retry?: boolean } - // If 401 and not already retrying, attempt token refresh - if (error.response?.status === 401 && !originalRequest._retry) { - originalRequest._retry = true - - const refreshToken = localStorage.getItem('refresh_token') - if (refreshToken) { - try { - const response = await axios.post(`${API_BASE_URL}/api/v1/auth/refresh`, null, { - headers: { - Authorization: `Bearer ${refreshToken}`, - }, - }) - - const { access_token, refresh_token } = response.data - localStorage.setItem('access_token', access_token) - localStorage.setItem('refresh_token', refresh_token) - - // Retry original request with new token - if (originalRequest.headers) { - originalRequest.headers.Authorization = `Bearer ${access_token}` - } - return apiClient(originalRequest) - } catch (refreshError) { - // Refresh failed - clear tokens and redirect to login - localStorage.removeItem('access_token') - localStorage.removeItem('refresh_token') - window.location.href = '/login' - return Promise.reject(refreshError) - } - } + // Only handle 401s that haven't already been retried + if (error.response?.status !== 401 || originalRequest._retry) { + return Promise.reject(error) } - return Promise.reject(error) + originalRequest._retry = true + + const refreshToken = localStorage.getItem('refresh_token') + if (!refreshToken) { + return Promise.reject(error) + } + + // If a refresh is already in progress, queue this request + if (isRefreshing) { + return new Promise((resolve, reject) => { + refreshSubscribers.push((newToken: string) => { + if (originalRequest.headers) { + originalRequest.headers.Authorization = `Bearer ${newToken}` + } + resolve(apiClient(originalRequest)) + }) + refreshFailSubscribers.push((refreshError: unknown) => { + reject(refreshError) + }) + }) + } + + // This is the first 401 — perform the refresh + isRefreshing = true + + try { + const response = await axios.post(`${API_BASE_URL}/api/v1/auth/refresh`, null, { + headers: { + Authorization: `Bearer ${refreshToken}`, + }, + }) + + const { access_token, refresh_token } = response.data + localStorage.setItem('access_token', access_token) + localStorage.setItem('refresh_token', refresh_token) + + // Sync Zustand auth store + useAuthStore.getState().setTokens({ + access_token, + refresh_token, + token_type: 'bearer', + }) + + isRefreshing = false + onRefreshed(access_token) + + // Retry original request with new token + if (originalRequest.headers) { + originalRequest.headers.Authorization = `Bearer ${access_token}` + } + return apiClient(originalRequest) + } catch (refreshError) { + isRefreshing = false + onRefreshFailed(refreshError) + + // Refresh failed — clear tokens and redirect to login + localStorage.removeItem('access_token') + localStorage.removeItem('refresh_token') + useAuthStore.getState().logout() + window.location.href = '/login' + return Promise.reject(refreshError) + } } ) diff --git a/frontend/src/store/authStore.ts b/frontend/src/store/authStore.ts index 58b7e908..457697c3 100644 --- a/frontend/src/store/authStore.ts +++ b/frontend/src/store/authStore.ts @@ -15,6 +15,7 @@ interface AuthState { register: (data: UserCreate) => Promise logout: () => Promise fetchUser: () => Promise + setTokens: (token: Token) => void clearError: () => void setLoading: (loading: boolean) => void } @@ -85,6 +86,7 @@ export const useAuthStore = create()( } }, + setTokens: (token: Token) => set({ token }), clearError: () => set({ error: null }), setLoading: (loading: boolean) => set({ isLoading: loading }), }),