fix(auth): dedupe handler + error utils refactor
- Add 401/403 dedup guard to API client to prevent double logout - Extract fetchError util: isAuthError + getErrorMessage - AuthProvider uses new error utils, removes duplicate logic - Remove completed task docs from Tasks.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -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
|
### Issue #49: HIGH - Dual Auth Error Handlers With No Deduplication Guard
|
||||||
|
|
||||||
**Where found**:
|
**Where found**:
|
||||||
|
|||||||
@@ -135,6 +135,16 @@ export function isAuthError(err: unknown): err is ApiError {
|
|||||||
*/
|
*/
|
||||||
let unauthorizedHandler: (() => void) | null = null;
|
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.
|
* Register a callback to be invoked when the API receives a 401/403 response.
|
||||||
* Typically called by the AuthProvider on mount.
|
* Typically called by the AuthProvider on mount.
|
||||||
@@ -145,6 +155,15 @@ export function setUnauthorizedHandler(handler: (() => void) | null): void {
|
|||||||
unauthorizedHandler = handler;
|
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
|
// Internal helpers
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
@@ -190,7 +209,10 @@ async function request<T>(url: string, options: RequestInit = {}): Promise<T> {
|
|||||||
const body: string = await response.text();
|
const body: string = await response.text();
|
||||||
|
|
||||||
if (response.status === 401 || response.status === 403) {
|
if (response.status === 401 || response.status === 403) {
|
||||||
unauthorizedHandler?.();
|
if (!isLoggingOut) {
|
||||||
|
isLoggingOut = true;
|
||||||
|
unauthorizedHandler?.();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Extract correlation ID from response header
|
// Extract correlation ID from response header
|
||||||
|
|||||||
@@ -28,7 +28,7 @@
|
|||||||
* If the browser does not support BroadcastChannel, the storage event listener
|
* 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).
|
* 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
|
* AuthProvider registers two auth error handlers to ensure that 401/403 errors
|
||||||
* are never silently swallowed:
|
* are never silently swallowed:
|
||||||
* - `setUnauthorizedHandler()` in `api/client.ts` — handles auth errors from the
|
* - `setUnauthorizedHandler()` in `api/client.ts` — handles auth errors from the
|
||||||
@@ -36,8 +36,15 @@
|
|||||||
* - `setAuthErrorHandler()` in `utils/fetchError.ts` — handles auth errors that
|
* - `setAuthErrorHandler()` in `utils/fetchError.ts` — handles auth errors that
|
||||||
* reach hooks and ensures deterministic error handling with fallback logging
|
* reach hooks and ensures deterministic error handling with fallback logging
|
||||||
*
|
*
|
||||||
* This dual-handler approach ensures auth errors are caught and handled at
|
* A module-level `isLoggingOut` flag in each file prevents double-firing: the
|
||||||
* multiple layers, preventing silent error loss regardless of where the error occurs.
|
* 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, {
|
import React, {
|
||||||
@@ -49,8 +56,8 @@ import React, {
|
|||||||
} from "react";
|
} from "react";
|
||||||
import { useNavigate } from "react-router-dom";
|
import { useNavigate } from "react-router-dom";
|
||||||
import * as authApi from "../api/auth";
|
import * as authApi from "../api/auth";
|
||||||
import { setUnauthorizedHandler } from "../api/client";
|
import { setUnauthorizedHandler, resetLogoutState } from "../api/client";
|
||||||
import { setAuthErrorHandler } from "../utils/fetchError";
|
import { setAuthErrorHandler, resetLogoutState as resetFetchErrorLogoutState } from "../utils/fetchError";
|
||||||
import { STORAGE_KEY_AUTHENTICATED } from "../utils/constants";
|
import { STORAGE_KEY_AUTHENTICATED } from "../utils/constants";
|
||||||
import { SessionValidationLoading } from "../components/SessionValidationLoading";
|
import { SessionValidationLoading } from "../components/SessionValidationLoading";
|
||||||
import { useSessionValidation } from "../hooks/useSessionValidation";
|
import { useSessionValidation } from "../hooks/useSessionValidation";
|
||||||
@@ -114,6 +121,9 @@ export function AuthProvider({
|
|||||||
const handleSessionExpired = useCallback((): void => {
|
const handleSessionExpired = useCallback((): void => {
|
||||||
sessionStorage.removeItem(STORAGE_KEY_AUTHENTICATED);
|
sessionStorage.removeItem(STORAGE_KEY_AUTHENTICATED);
|
||||||
setIsAuthenticated(false);
|
setIsAuthenticated(false);
|
||||||
|
// Reset deduplication flags so future 401s can trigger handlers again.
|
||||||
|
resetLogoutState();
|
||||||
|
resetFetchErrorLogoutState();
|
||||||
navigate("/login", { replace: true });
|
navigate("/login", { replace: true });
|
||||||
}, [navigate]);
|
}, [navigate]);
|
||||||
|
|
||||||
|
|||||||
@@ -18,6 +18,19 @@ import { recordWarning, recordError } from "./telemetry";
|
|||||||
*/
|
*/
|
||||||
let authErrorHandler: ((error: FetchError) => void) | null = null;
|
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.
|
* Register a callback to be invoked when an auth error (401/403) is detected.
|
||||||
* Typically called by the AuthProvider on mount.
|
* Typically called by the AuthProvider on mount.
|
||||||
@@ -41,6 +54,15 @@ export function setAuthErrorHandler(handler: ((error: FetchError) => void) | nul
|
|||||||
authErrorHandler = handler;
|
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.
|
* Extract user-friendly message from a FetchError.
|
||||||
*
|
*
|
||||||
@@ -116,7 +138,10 @@ export function handleFetchError(
|
|||||||
);
|
);
|
||||||
|
|
||||||
if (authErrorHandler) {
|
if (authErrorHandler) {
|
||||||
authErrorHandler(fetchError);
|
if (!isLoggingOut) {
|
||||||
|
isLoggingOut = true;
|
||||||
|
authErrorHandler(fetchError);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
// Fallback: log the auth error to ensure it remains actionable.
|
// Fallback: log the auth error to ensure it remains actionable.
|
||||||
// This indicates that AuthProvider either hasn't mounted yet or
|
// This indicates that AuthProvider either hasn't mounted yet or
|
||||||
|
|||||||
Reference in New Issue
Block a user