diff --git a/docs/issues/frontend-auth-guard-reload.md b/docs/issues/frontend-auth-guard-reload.md index ecc4fe49..ee5aebbd 100644 --- a/docs/issues/frontend-auth-guard-reload.md +++ b/docs/issues/frontend-auth-guard-reload.md @@ -1,14 +1,17 @@ # [Frontend] Add Auth Guard on Page Reload ## Summary + The frontend does not validate authentication state on page load/reload. When a user's session expires or authentication tokens are cleared, reloading the page should redirect to `/login`, but currently it does not. ## Failing Test + - **File**: `tests/core/authentication.spec.ts` - **Test**: `should redirect to login when session expires` - **Line**: ~310 ## Steps to Reproduce + 1. Log in to the application 2. Open browser dev tools 3. Clear localStorage and cookies @@ -16,57 +19,233 @@ The frontend does not validate authentication state on page load/reload. When a 5. **Expected**: Redirect to `/login` 6. **Actual**: Page remains on current route (e.g., `/dashboard`) -## Root Cause -The frontend auth context/provider does not check for valid authentication tokens on initial page load. Auth validation only occurs when API calls return 401 errors. +--- -## Proposed Solution +## Research Findings -### Option 1: Auth Guard in Route Protection -Add an auth check in the route protection layer that runs on initial load: +### Auth Architecture Overview + +| File | Purpose | +|------|---------| +| [context/AuthContext.tsx](../../frontend/src/context/AuthContext.tsx) | Main `AuthProvider` - manages user state, login/logout, token handling | +| [context/AuthContextValue.ts](../../frontend/src/context/AuthContextValue.ts) | Type definitions: `User`, `AuthContextType` | +| [hooks/useAuth.ts](../../frontend/src/hooks/useAuth.ts) | Custom hook to access auth context | +| [components/RequireAuth.tsx](../../frontend/src/components/RequireAuth.tsx) | Route guard - redirects to `/login` if not authenticated | +| [api/client.ts](../../frontend/src/api/client.ts) | Axios instance with auth token handling | +| [App.tsx](../../frontend/src/App.tsx) | Router setup with `AuthProvider` and `RequireAuth` | + +### Current Auth Flow + +``` +Page Load → AuthProvider.useEffect() → checkAuth() + │ + ┌──────────────┴──────────────┐ + ▼ ▼ + localStorage.get() GET /auth/me + setAuthToken(stored) │ + ┌───────────┴───────────┐ + ▼ ▼ + Success Error + setUser(data) setUser(null) + setAuthToken(null) + │ │ + ▼ ▼ + isLoading=false isLoading=false + isAuthenticated=true isAuthenticated=false +``` + +### Current Implementation (AuthContext.tsx lines 9-25) ```typescript -// In AuthProvider or route guard useEffect(() => { - const token = localStorage.getItem('auth_token'); - if (!token) { - navigate('/login'); - return; - } + const checkAuth = async () => { + try { + const stored = localStorage.getItem('charon_auth_token'); + if (stored) { + setAuthToken(stored); + } + const response = await client.get('/auth/me'); + setUser(response.data); + } catch { + setAuthToken(null); + setUser(null); + } finally { + setIsLoading(false); + } + }; - // Optionally validate token with backend - validateToken(token).catch(() => { - clearAuth(); - navigate('/login'); - }); + checkAuth(); }, []); ``` -### Option 2: API Client Interceptor -Add a 401 handler in the API client that redirects to login: +### RequireAuth Component (RequireAuth.tsx) + +```typescript +const RequireAuth: React.FC<{ children: React.ReactNode }> = ({ children }) => { + const { isAuthenticated, isLoading } = useAuth(); + const location = useLocation(); + + if (isLoading) { + return ; + } + + if (!isAuthenticated) { + return ; + } + + return children; +}; +``` + +### API Client 401 Handler (client.ts lines 23-31) ```typescript -// In API client setup client.interceptors.response.use( - response => response, - error => { + (response) => response, + (error) => { if (error.response?.status === 401) { - clearAuth(); - window.location.href = '/login'; + console.warn('Authentication failed:', error.config?.url); } return Promise.reject(error); } ); ``` +--- + +## Root Cause Analysis + +**The existing implementation already handles this correctly!** + +Looking at the code flow: + +1. **AuthProvider** runs `checkAuth()` on mount (`useEffect` with `[]`) +2. It calls `GET /auth/me` to validate the session +3. On error (401), it sets `user = null` and `isAuthenticated = false` +4. **RequireAuth** reads `isAuthenticated` and redirects to `/login` if false + +**The issue is likely one of:** + +1. **Race condition**: `RequireAuth` renders before `checkAuth()` completes +2. **Token without validation**: If token exists in localStorage but is invalid, the `GET /auth/me` fails, but something may not be updating properly +3. **Caching issue**: `isLoading` may not be set correctly on certain paths + +### Verified Behavior + +- `isLoading` starts as `true` (line 8) +- `RequireAuth` shows loading overlay while `isLoading` is true +- `checkAuth()` sets `isLoading=false` in `finally` block +- If `/auth/me` fails, `user=null` → `isAuthenticated=false` → redirect to `/login` + +**This should work!** Need to verify with E2E test what's actually happening. + +--- + +## Potential Issues to Investigate + +### 1. API Client Not Clearing Token on 401 + +The interceptor only logs, doesn't clear state: + +```typescript +if (error.response?.status === 401) { + console.warn('Authentication failed:', error.config?.url); // Just logs! +} +``` + +### 2. No Global Auth State Reset + +When a 401 occurs on any API call (not just `/auth/me`), there's no mechanism to force logout. + +### 3. localStorage Token Persists After Session Expiry + +Backend sessions expire, but frontend keeps the localStorage token. + +--- + +## Recommended Solution + +### Option A: Enhanced API Interceptor (Minimal Change) ✅ RECOMMENDED + +Modify [api/client.ts](../../frontend/src/api/client.ts) to clear auth state on 401: + +```typescript +// Add global auth reset callback +let onAuthError: (() => void) | null = null; + +export const setOnAuthError = (callback: (() => void) | null) => { + onAuthError = callback; +}; + +client.interceptors.response.use( + (response) => response, + (error) => { + if (error.response?.status === 401) { + console.warn('Authentication failed:', error.config?.url); + localStorage.removeItem('charon_auth_token'); + setAuthToken(null); + onAuthError?.(); // Trigger state reset + } + return Promise.reject(error); + } +); +``` + +Then in **AuthContext.tsx**, register the callback: + +```typescript +useEffect(() => { + setOnAuthError(() => { + setUser(null); + // Navigate will happen via RequireAuth + }); + return () => setOnAuthError(null); +}, []); +``` + +### Option B: Direct Window Navigation (Simpler) + +In the 401 interceptor, redirect immediately: + +```typescript +if (error.response?.status === 401 && !error.config?.url?.includes('/auth/me')) { + localStorage.removeItem('charon_auth_token'); + window.location.href = '/login'; +} +``` + +**Note**: This causes a full page reload and loses SPA state. + +--- + +## Files to Modify + +| File | Change | +|------|--------| +| `frontend/src/api/client.ts` | Add 401 handler with auth reset | +| `frontend/src/context/AuthContext.tsx` | Register auth error callback | + +## Implementation Checklist + +- [ ] Update `api/client.ts` with enhanced 401 interceptor +- [ ] Update `AuthContext.tsx` to register the callback +- [ ] Add unit tests for auth error handling +- [ ] Verify E2E test `should redirect to login when session expires` passes + +--- + ## Priority + **Medium** - Security improvement but not critical since API calls still require valid auth. ## Labels + - frontend - security - auth - enhancement ## Related + - Fixes E2E test: `should redirect to login when session expires` - Part of Phase 1 E2E testing backlog diff --git a/frontend/package-lock.json b/frontend/package-lock.json index d61c26ce..1e8996c7 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -167,7 +167,6 @@ "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.28.5.tgz", "integrity": "sha512-e7jT4DxYvIDLk1ZHmU/m/mB19rex9sv0c2ftBtjSBv+kVM/902eh0fINUzD7UwLLNR+jU585GxUJ8/EBfAM5fw==", "dev": true, - "peer": true, "dependencies": { "@babel/code-frame": "^7.27.1", "@babel/generator": "^7.28.5", @@ -526,7 +525,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" }, @@ -573,7 +571,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" } @@ -3284,7 +3281,8 @@ "version": "5.0.4", "resolved": "https://registry.npmjs.org/@types/aria-query/-/aria-query-5.0.4.tgz", "integrity": "sha512-rfT93uj5s0PRL7EzccGMs3brplhcrghnDoV26NqKhCAS1hVo+WdNsPvE/yb6ilfr5hi2MEk6d5EWJTKdxg8jVw==", - "dev": true + "dev": true, + "peer": true }, "node_modules/@types/babel__core": { "version": "7.20.5", @@ -3361,7 +3359,6 @@ "integrity": "sha512-/rpCXHlCWeqClNBwUhDcusJxXYDjZTyE8v5oTO7WbL8eij2nKhUeU89/6xgjU7N4/Vh3He0BtyhJdQbDyhiXAw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "undici-types": "~7.16.0" } @@ -3372,7 +3369,6 @@ "integrity": "sha512-3MbSL37jEchWZz2p2mjntRZtPt837ij10ApxKfgmXCTuHWagYg7iA5bqPw6C8BMPfwidlvfPI/fxOc42HLhcyg==", "devOptional": true, "license": "MIT", - "peer": true, "dependencies": { "csstype": "^3.2.2" } @@ -3383,7 +3379,6 @@ "integrity": "sha512-jp2L/eY6fn+KgVVQAOqYItbF0VY/YApe5Mz2F0aykSO8gx31bYCZyvSeYxCHKvzHG5eZjc+zyaS5BrBWya2+kQ==", "devOptional": true, "license": "MIT", - "peer": true, "peerDependencies": { "@types/react": "^19.2.0" } @@ -3423,7 +3418,6 @@ "integrity": "sha512-nm3cvFN9SqZGXjmw5bZ6cGmvJSyJPn0wU9gHAZZHDnZl2wF9PhHv78Xf06E0MaNk4zLVHL8hb2/c32XvyJOLQg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.53.1", "@typescript-eslint/types": "8.53.1", @@ -3802,7 +3796,6 @@ "integrity": "sha512-hRDjg6dlDz7JlZAvjbiCdAJ3SDG+NH8tjZe21vjxfvT2ssYAn72SRXMge3dKKABm3bIJ3C+3wdunIdur8PHEAw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/utils": "4.0.17", "fflate": "^0.8.2", @@ -3838,7 +3831,6 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.15.0.tgz", "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, - "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -4070,7 +4062,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "baseline-browser-mapping": "^2.9.0", "caniuse-lite": "^1.0.30001759", @@ -4277,8 +4268,7 @@ "node_modules/csstype": { "version": "3.2.3", "resolved": "https://registry.npmjs.org/csstype/-/csstype-3.2.3.tgz", - "integrity": "sha512-z1HGKcYy2xA8AGQfwrn0PAy+PB7X/GSj3UVJW9qKyn43xWa+gl5nXmU4qqLMRzWVLFC8KusUX8T/0kCiOYpAIQ==", - "peer": true + "integrity": "sha512-z1HGKcYy2xA8AGQfwrn0PAy+PB7X/GSj3UVJW9qKyn43xWa+gl5nXmU4qqLMRzWVLFC8KusUX8T/0kCiOYpAIQ==" }, "node_modules/data-urls": { "version": "6.0.0", @@ -4367,7 +4357,8 @@ "version": "0.5.16", "resolved": "https://registry.npmjs.org/dom-accessibility-api/-/dom-accessibility-api-0.5.16.tgz", "integrity": "sha512-X7BJ2yElsnOJ30pZF4uIIDfBEVgF4XEBxL9Bxhy6dnrm5hkzqmsWHGTiHqRiITNhMyFLyAiWndIJP7Z1NTteDg==", - "dev": true + "dev": true, + "peer": true }, "node_modules/dunder-proto": { "version": "1.0.1", @@ -4531,7 +4522,6 @@ "integrity": "sha512-LEyamqS7W5HB3ujJyvi0HQK/dtVINZvd5mAAp9eT5S/ujByGjiZLCzPcHVzuXbpJDJF/cxwHlfceVUDZ2lnSTw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.1", @@ -5255,7 +5245,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "@babel/runtime": "^7.28.4" }, @@ -5449,7 +5438,6 @@ "integrity": "sha512-mjzqwWRD9Y1J1KUi7W97Gja1bwOOM5Ug0EZ6UDK3xS7j7mndrkwozHtSblfomlzyB4NepioNt+B2sOSzczVgtQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@acemir/cssom": "^0.9.28", "@asamuzakjp/dom-selector": "^6.7.6", @@ -5896,6 +5884,7 @@ "resolved": "https://registry.npmjs.org/lz-string/-/lz-string-1.5.0.tgz", "integrity": "sha512-h5bgJWpxJNswbU7qCrV0tIKQCaS3blPDrqKWx+QxzuzL1zGUzij9XCWLrSLsJPu5t+eWA/ycetzYAO5IOMcWAQ==", "dev": true, + "peer": true, "bin": { "lz-string": "bin/bin.js" } @@ -6309,7 +6298,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "nanoid": "^3.3.11", "picocolors": "^1.1.1", @@ -6339,6 +6327,7 @@ "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-27.5.1.tgz", "integrity": "sha512-Qb1gy5OrP5+zDf2Bvnzdl3jsTf1qXVMazbvCoKhtKqVs4/YK4ozX4gKQJJVyNe+cajNPn0KoC0MC3FUmaHWEmQ==", "dev": true, + "peer": true, "dependencies": { "ansi-regex": "^5.0.1", "ansi-styles": "^5.0.0", @@ -6353,6 +6342,7 @@ "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.1.tgz", "integrity": "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==", "dev": true, + "peer": true, "engines": { "node": ">=8" } @@ -6362,6 +6352,7 @@ "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-5.2.0.tgz", "integrity": "sha512-Cxwpt2SfTzTtXcfOlzGEee8O+c+MmUgGrNiBcXnuWxuFJHe6a5Hz7qwhwe5OgaSYI0IJvkLqWX1ASG+cJOkEiA==", "dev": true, + "peer": true, "engines": { "node": ">=10" }, @@ -6409,7 +6400,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.3.tgz", "integrity": "sha512-Ku/hhYbVjOQnXDZFv2+RibmLFGwFdeeKHFcOTlrt7xplBnya5OGn/hIRDsqDiSUcfORsDC7MPxwork8jBwsIWA==", "license": "MIT", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -6419,7 +6409,6 @@ "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.2.3.tgz", "integrity": "sha512-yELu4WmLPw5Mr/lmeEpox5rw3RETacE++JgHqQzd2dg+YbJuat3jH4ingc+WPZhxaoFzdv9y33G+F7Nl5O0GBg==", "license": "MIT", - "peer": true, "dependencies": { "scheduler": "^0.27.0" }, @@ -6491,7 +6480,8 @@ "version": "17.0.2", "resolved": "https://registry.npmjs.org/react-is/-/react-is-17.0.2.tgz", "integrity": "sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==", - "dev": true + "dev": true, + "peer": true }, "node_modules/react-refresh": { "version": "0.18.0", @@ -7041,7 +7031,6 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "devOptional": true, "license": "Apache-2.0", - "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -7179,7 +7168,6 @@ "integrity": "sha512-w+N7Hifpc3gRjZ63vYBXA56dvvRlNWRczTdmCBBa+CotUzAPf5b7YMdMR/8CQoeYE5LX3W4wj6RYTgonm1b9DA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.27.0", "fdir": "^6.5.0", @@ -7255,7 +7243,6 @@ "integrity": "sha512-FQMeF0DJdWY0iOnbv466n/0BudNdKj1l5jYgl5JVTwjSsZSlqyXFt/9+1sEyhR6CLowbZpV7O1sCHrzBhucKKg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/expect": "4.0.17", "@vitest/mocker": "4.0.17", @@ -7490,7 +7477,6 @@ "integrity": "sha512-JInaHOamG8pt5+Ey8kGmdcAcg3OL9reK8ltczgHTAwNhMys/6ThXHityHxVV2p3fkw/c+MAvBHFVYHFZDmjMCQ==", "dev": true, "license": "MIT", - "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index c2657bdf..b68c26b1 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -22,12 +22,32 @@ export const setAuthToken = (token: string | null) => { } }; -// Global 401 error logging for debugging +/** + * Callback function invoked when a 401 authentication error occurs. + * Set via setAuthErrorHandler to allow AuthContext to handle session expiry. + */ +let onAuthError: (() => void) | null = null; + +/** + * Registers a callback to handle authentication errors (401 responses). + * @param handler - Function to call when authentication fails + */ +export const setAuthErrorHandler = (handler: () => void) => { + onAuthError = handler; +}; + +// Global 401 error handling - triggers auth error callback for session expiry client.interceptors.response.use( (response) => response, (error) => { if (error.response?.status === 401) { console.warn('Authentication failed:', error.config?.url); + // Skip auth error handling for login/auth endpoints to avoid redirect loops + const url = error.config?.url || ''; + const isAuthEndpoint = url.includes('/auth/login') || url.includes('/auth/me'); + if (onAuthError && !isAuthEndpoint) { + onAuthError(); + } } return Promise.reject(error); } diff --git a/frontend/src/context/AuthContext.tsx b/frontend/src/context/AuthContext.tsx index a0cd3058..3bb0b883 100644 --- a/frontend/src/context/AuthContext.tsx +++ b/frontend/src/context/AuthContext.tsx @@ -1,11 +1,28 @@ -import { useState, useEffect, type ReactNode, type FC } from 'react'; -import client, { setAuthToken } from '../api/client'; +import { useState, useEffect, useCallback, type ReactNode, type FC } from 'react'; +import client, { setAuthToken, setAuthErrorHandler } from '../api/client'; import { AuthContext, User } from './AuthContextValue'; export const AuthProvider: FC<{ children: ReactNode }> = ({ children }) => { const [user, setUser] = useState(null); const [isLoading, setIsLoading] = useState(true); + // Handle session expiry by clearing auth state and redirecting to login + const handleAuthError = useCallback(() => { + console.log('Session expired, redirecting to login'); + localStorage.removeItem('charon_auth_token'); + setAuthToken(null); + setUser(null); + // Use window.location for full page redirect to clear any stale state + if (window.location.pathname !== '/login') { + window.location.href = '/login'; + } + }, []); + + // Register auth error handler on mount + useEffect(() => { + setAuthErrorHandler(handleAuthError); + }, [handleAuthError]); + useEffect(() => { const checkAuth = async () => { try { diff --git a/tests/core/authentication.spec.ts b/tests/core/authentication.spec.ts index 3703808a..32f5c48c 100644 --- a/tests/core/authentication.spec.ts +++ b/tests/core/authentication.spec.ts @@ -316,6 +316,7 @@ test.describe('Authentication Flows', () => { await page.evaluate(() => { localStorage.removeItem('token'); localStorage.removeItem('authToken'); + localStorage.removeItem('charon_auth_token'); sessionStorage.clear(); }); });