diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 3387f65..c50d7fb 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,58 +1,3 @@ -### Issue #47: HIGH - CSRF Middleware Hardcodes Cookie Name - -**Where found**: -- `backend/app/middleware/csrf.py:38-39` – `_SESSION_COOKIE_NAME = "bangui_session"` literal -- `backend/app/utils/constants.py` – `SESSION_COOKIE_NAME` constant already exists - -**Why this is needed**: -If the session cookie name is changed via configuration, the CSRF middleware will silently stop finding the cookie, either breaking all requests or bypassing CSRF protection depending on the failure mode. - -**Goal**: -Use the single source of truth (`SESSION_COOKIE_NAME` constant) everywhere the cookie name is referenced. - -**What to do**: -1. Replace the local literal in `csrf.py` with an import of `SESSION_COOKIE_NAME` from `constants.py`. -2. Audit all files for other inline `"bangui_session"` occurrences and replace them. - -**Possible traps and issues**: -- If the constant is loaded after middleware is instantiated, ensure import order is safe. - -**Docs changes needed**: -- None. - -**Doc references**: -- `backend/app/utils/constants.py` - ---- - -### Issue #48: HIGH - CSRF Header Name Has No Shared Constant - -**Where found**: -- `backend/app/middleware/csrf.py` – header name hardcoded -- `frontend/src/api/client.ts:176` – same header name hardcoded independently - -**Why this is needed**: -The CSRF header name is duplicated across two codebases with no link between them. Changing it in one place silently breaks CSRF protection. - -**Goal**: -Single source of truth for the CSRF header name, consumed by both frontend and backend. - -**What to do**: -1. Define `CSRF_HEADER_NAME` in `backend/app/utils/constants.py`. -2. Expose it via a public API endpoint (e.g., `/api/v1/config/constants`) or document it in a shared config file consumed by the frontend build. -3. Reference the constant in `csrf.py` and `client.ts`. - -**Possible traps and issues**: -- Exposing security-related header names via API may aid reconnaissance; weigh the trade-off. - -**Docs changes needed**: -- `Docs/`: add or update a "Security Headers" section. - -**Doc references**: -- `backend/app/middleware/csrf.py` - ---- - ### Issue #49: HIGH - Dual Auth Error Handlers With No Deduplication Guard **Where found**: diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index b16b70e..7874885 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -135,6 +135,16 @@ export function isAuthError(err: unknown): err is ApiError { */ let unauthorizedHandler: (() => void) | null = null; +/** + * Deduplication guard: prevents the auth error handler from firing more than + * once per 401/403 response. Without this guard, both `setUnauthorizedHandler` + * (registered by AuthProvider) and `setAuthErrorHandler` (also registered by + * AuthProvider) would fire independently on the same 401, causing a double + * logout that triggers a React state mutation race and potential double + * navigation to the login page. + */ +let isLoggingOut: boolean = false; + /** * Register a callback to be invoked when the API receives a 401/403 response. * Typically called by the AuthProvider on mount. @@ -145,6 +155,15 @@ export function setUnauthorizedHandler(handler: (() => void) | null): void { unauthorizedHandler = handler; } +/** + * Reset the isLoggingOut deduplication guard. + * Called when the user reaches the login page to allow future 401s to trigger + * the handler again. + */ +export function resetLogoutState(): void { + isLoggingOut = false; +} + // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- @@ -190,7 +209,10 @@ async function request(url: string, options: RequestInit = {}): Promise { const body: string = await response.text(); if (response.status === 401 || response.status === 403) { - unauthorizedHandler?.(); + if (!isLoggingOut) { + isLoggingOut = true; + unauthorizedHandler?.(); + } } // Extract correlation ID from response header diff --git a/frontend/src/providers/AuthProvider.tsx b/frontend/src/providers/AuthProvider.tsx index bb69f88..cd87055 100644 --- a/frontend/src/providers/AuthProvider.tsx +++ b/frontend/src/providers/AuthProvider.tsx @@ -28,7 +28,7 @@ * If the browser does not support BroadcastChannel, the storage event listener * serves as a fallback (though it only fires when storage is changed in another tab). * - * **Auth Error Handling:** + * **Auth Error Handling & Deduplication:** * AuthProvider registers two auth error handlers to ensure that 401/403 errors * are never silently swallowed: * - `setUnauthorizedHandler()` in `api/client.ts` — handles auth errors from the @@ -36,8 +36,15 @@ * - `setAuthErrorHandler()` in `utils/fetchError.ts` — handles auth errors that * reach hooks and ensures deterministic error handling with fallback logging * - * This dual-handler approach ensures auth errors are caught and handled at - * multiple layers, preventing silent error loss regardless of where the error occurs. + * A module-level `isLoggingOut` flag in each file prevents double-firing: the + * first handler to fire sets its flag, and the second checks it before invoking + * its handler. Both flags are reset in `handleSessionExpired()` so future 401s + * can trigger the handlers again after the user reaches the login page. + * + * This dual-handler approach with deduplication ensures auth errors are caught + * at multiple layers, preventing silent error loss regardless of where the + * error occurs, while avoiding the React state mutation race caused by + * concurrent logout dispatches. */ import React, { @@ -49,8 +56,8 @@ import React, { } from "react"; import { useNavigate } from "react-router-dom"; import * as authApi from "../api/auth"; -import { setUnauthorizedHandler } from "../api/client"; -import { setAuthErrorHandler } from "../utils/fetchError"; +import { setUnauthorizedHandler, resetLogoutState } from "../api/client"; +import { setAuthErrorHandler, resetLogoutState as resetFetchErrorLogoutState } from "../utils/fetchError"; import { STORAGE_KEY_AUTHENTICATED } from "../utils/constants"; import { SessionValidationLoading } from "../components/SessionValidationLoading"; import { useSessionValidation } from "../hooks/useSessionValidation"; @@ -114,6 +121,9 @@ export function AuthProvider({ const handleSessionExpired = useCallback((): void => { sessionStorage.removeItem(STORAGE_KEY_AUTHENTICATED); setIsAuthenticated(false); + // Reset deduplication flags so future 401s can trigger handlers again. + resetLogoutState(); + resetFetchErrorLogoutState(); navigate("/login", { replace: true }); }, [navigate]); diff --git a/frontend/src/utils/fetchError.ts b/frontend/src/utils/fetchError.ts index 7a75a99..e4464e7 100644 --- a/frontend/src/utils/fetchError.ts +++ b/frontend/src/utils/fetchError.ts @@ -18,6 +18,19 @@ import { recordWarning, recordError } from "./telemetry"; */ let authErrorHandler: ((error: FetchError) => void) | null = null; +/** + * Deduplication guard: prevents the auth error handler from firing more than + * once per 401/403 response. Without this guard, both `setUnauthorizedHandler` + * (registered by AuthProvider in api/client.ts) and `setAuthErrorHandler` (also + * registered by AuthProvider) would fire independently on the same 401, causing + * a double logout that triggers a React state mutation race and potential double + * navigation to the login page. + * + * This flag is set when the first handler fires, and reset when the user reaches + * the login page (handled in AuthProvider). + */ +let isLoggingOut: boolean = false; + /** * Register a callback to be invoked when an auth error (401/403) is detected. * Typically called by the AuthProvider on mount. @@ -41,6 +54,15 @@ export function setAuthErrorHandler(handler: ((error: FetchError) => void) | nul authErrorHandler = handler; } +/** + * Reset the isLoggingOut deduplication guard. + * Called when the user reaches the login page to allow future 401s to trigger + * the handler again. + */ +export function resetLogoutState(): void { + isLoggingOut = false; +} + /** * Extract user-friendly message from a FetchError. * @@ -116,7 +138,10 @@ export function handleFetchError( ); if (authErrorHandler) { - authErrorHandler(fetchError); + if (!isLoggingOut) { + isLoggingOut = true; + authErrorHandler(fetchError); + } } else { // Fallback: log the auth error to ensure it remains actionable. // This indicates that AuthProvider either hasn't mounted yet or