From 6b8b29571e8311ad27098925208ee35b0b785fe2 Mon Sep 17 00:00:00 2001 From: Michael Chihlas Date: Wed, 4 Feb 2026 20:41:37 -0500 Subject: [PATCH] fix: token refresh and seed tree visibility Fix broken JWT token refresh that caused "Failed to load trees" after idle timeout. The refresh endpoint expected token as query param but frontend sent it as Authorization header. Added proper dependency (get_refresh_token_payload) and refresh queue to handle concurrent 401s. Also fix seed trees not being visible to non-admin users by updating the seed script to set is_public/is_default on existing trees. Co-Authored-By: Claude Opus 4.5 --- backend/app/api/deps.py | 14 +++ backend/app/api/endpoints/auth.py | 12 +- backend/scripts/seed_trees.py | 18 ++- .../2026-02-04-token-refresh-fix-design.md | 81 ++++++++++++ frontend/src/api/client.ts | 115 +++++++++++++----- frontend/src/store/authStore.ts | 2 + 6 files changed, 197 insertions(+), 45 deletions(-) create mode 100644 docs/plans/2026-02-04-token-refresh-fix-design.md 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 }), }),