From 72c4a0ed047ccfc1a7d57e14c52ff1b7ed912fcf Mon Sep 17 00:00:00 2001 From: Lukas Date: Tue, 28 Apr 2026 09:45:08 +0200 Subject: [PATCH] fix: prevent silent auth error swallowing in fetch error utility - Add setAuthErrorHandler() registration mechanism to utils/fetchError.ts - Implement fallback logging when auth errors (401/403) occur without registered handler - Update AuthProvider to register both API client and fetch error handlers - Ensure auth errors are handled deterministically at multiple layers - Add comprehensive tests for auth error handler registration and fallback logging - Update Web-Development.md documentation with auth error handling contract Fixes issue #21: Silent auth errors are now caught and logged if the handler is not registered, preventing actionable errors from being silently swallowed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 18 ----- Docs/Web-Development.md | 41 ++++++++++-- frontend/src/providers/AuthProvider.tsx | 16 +++++ .../src/utils/__tests__/fetchError.test.ts | 40 ++++++++++- frontend/src/utils/fetchError.ts | 66 ++++++++++++++++++- 5 files changed, 154 insertions(+), 27 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 7b5265e..33ceb3c 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,21 +1,3 @@ -## 20) Loading UX lacks progressive/skeleton states -- Where found: - - [frontend/src/pages](frontend/src/pages) -- Why this is needed: - - Blank loading regions reduce perceived responsiveness. -- Goal: - - Introduce standardized loading placeholders. -- What to do: - - Build shared skeleton components for tables/charts/forms. -- Possible traps and issues: - - Skeleton mismatch with actual layout can cause content shift. -- Docs changes needed: - - Add loading UX component guidance. -- Doc references: - - [Docs/Web-Design.md](Docs/Web-Design.md) - ---- - ## 21) Silent auth error swallow in fetch error utility - Where found: - [frontend/src/utils/fetchError.ts](frontend/src/utils/fetchError.ts) diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index c10b400..708bebf 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -971,10 +971,43 @@ HttpOnly cookies provide superior protection against XSS (Cross-Site Scripting) ### Error Handling -When an API request returns 401 or 403: -1. The `client.ts` module dispatches a `SESSION_EXPIRED_EVENT`. -2. The `AuthProvider` listener handles it by clearing `isAuthenticated` and redirecting to `/login`. -3. Hooks must use `handleFetchError` (from `utils/fetchError.ts`) to avoid displaying auth errors as user-facing error messages. +**Auth Error Handling Contract:** + +The frontend employs a **dual-handler approach** to ensure 401/403 auth errors are never silently swallowed: + +1. **API Client Layer** (`api/client.ts`): + - When `api/client.ts` receives a 401/403 response, it invokes `setUnauthorizedHandler()` (set by `AuthProvider`) + - This catches auth errors at the HTTP boundary before they reach hooks + - Prevents errors from being lost in error translation layers + +2. **Hook Utility Layer** (`utils/fetchError.ts`): + - When `handleFetchError()` encounters a 401/403 error, it: + - Invokes the registered `authErrorHandler()` (also set by `AuthProvider`), OR + - Falls back to logging a warning if no handler is registered + - This acts as a safety net for auth errors that escape the API client layer + - Ensures deterministic error handling with fallback logging + +3. **AuthProvider** (`providers/AuthProvider.tsx`): + - Registers both handlers on mount: `setUnauthorizedHandler()` and `setAuthErrorHandler()` + - Both handlers call `handleSessionExpired()`, which clears `isAuthenticated` and redirects to `/login` + - Cleans up handlers on unmount to prevent stale closures + +**Usage in Hooks:** + +Hooks must use `handleFetchError()` (from `utils/fetchError.ts`) when catching errors. It automatically: +- Silently ignores abort errors (expected cleanup) +- Invokes the registered auth handler for 401/403 errors (prevents display in component state) +- Passes other errors to the provided state setter + +```ts +// In a hook's .catch() handler: +.catch((err: unknown) => { + handleFetchError(err, setError, "Failed to load data"); + // Auth errors are handled globally; non-auth errors are set in component state +}); +``` + +This design ensures auth errors are caught and handled deterministically at multiple layers, regardless of where they originate. --- diff --git a/frontend/src/providers/AuthProvider.tsx b/frontend/src/providers/AuthProvider.tsx index fdef29d..e8c7df5 100644 --- a/frontend/src/providers/AuthProvider.tsx +++ b/frontend/src/providers/AuthProvider.tsx @@ -20,6 +20,17 @@ * The `isAuthenticated` state persists in `sessionStorage` to survive page * refreshes within the browser tab but is cleared on tab close. The session * cookie itself persists according to the backend's cookie settings. + * + * **Auth Error Handling:** + * 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 + * API client layer before they reach hooks + * - `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. */ import React, { @@ -32,6 +43,7 @@ import React, { import { useNavigate } from "react-router-dom"; import * as authApi from "../api/auth"; import { setUnauthorizedHandler } from "../api/client"; +import { setAuthErrorHandler } from "../utils/fetchError"; import { SessionValidationLoading } from "../components/SessionValidationLoading"; import { useSessionValidation } from "../hooks/useSessionValidation"; @@ -103,8 +115,12 @@ export function AuthProvider({ setUnauthorizedHandler((): void => { handleSessionExpired(); }); + setAuthErrorHandler((): void => { + handleSessionExpired(); + }); return (): void => { setUnauthorizedHandler(null); + setAuthErrorHandler(null); }; }, [handleSessionExpired]); diff --git a/frontend/src/utils/__tests__/fetchError.test.ts b/frontend/src/utils/__tests__/fetchError.test.ts index 0f0121e..e356123 100644 --- a/frontend/src/utils/__tests__/fetchError.test.ts +++ b/frontend/src/utils/__tests__/fetchError.test.ts @@ -1,20 +1,54 @@ -import { describe, expect, it, vi } from "vitest"; +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; import { ApiError } from "../../api/client"; -import { handleFetchError, createStringErrorAdapter } from "../fetchError"; +import { handleFetchError, createStringErrorAdapter, setAuthErrorHandler } from "../fetchError"; describe("utils/fetchError", () => { + beforeEach(() => { + // Clear handler before each test + setAuthErrorHandler(null); + }); + + afterEach(() => { + // Clean up after each test + setAuthErrorHandler(null); + }); + it("ignores AbortError errors", () => { const setError = vi.fn(); handleFetchError(new DOMException("Aborted", "AbortError"), setError, "fallback"); expect(setError).not.toHaveBeenCalled(); }); - it("ignores auth errors", () => { + it("calls registered auth error handler on 401", () => { + const authHandler = vi.fn(); + setAuthErrorHandler(authHandler); const setError = vi.fn(); handleFetchError(new ApiError(401, "Unauthorized"), setError, "fallback"); + expect(authHandler).toHaveBeenCalledOnce(); expect(setError).not.toHaveBeenCalled(); }); + it("calls registered auth error handler on 403", () => { + const authHandler = vi.fn(); + setAuthErrorHandler(authHandler); + const setError = vi.fn(); + handleFetchError(new ApiError(403, "Forbidden"), setError, "fallback"); + expect(authHandler).toHaveBeenCalledOnce(); + expect(setError).not.toHaveBeenCalled(); + }); + + it("logs warning when auth error occurs without registered handler", () => { + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const setError = vi.fn(); + handleFetchError(new ApiError(401, "Unauthorized"), setError, "fallback"); + expect(consoleWarnSpy).toHaveBeenCalledOnce(); + const firstCall = consoleWarnSpy.mock.calls[0]; + expect(firstCall).toBeDefined(); + expect(String(firstCall?.[0])).toContain("Auth error (401) without registered handler"); + expect(setError).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + }); + it("sets fallback for normal errors", () => { const setError = vi.fn(); handleFetchError(new Error("Oops"), createStringErrorAdapter(setError), "fallback"); diff --git a/frontend/src/utils/fetchError.ts b/frontend/src/utils/fetchError.ts index 34783c9..aff9f05 100644 --- a/frontend/src/utils/fetchError.ts +++ b/frontend/src/utils/fetchError.ts @@ -1,6 +1,45 @@ import type { FetchError } from "../types/api"; import { isAuthError, isAbortError } from "../types/api"; +// --------------------------------------------------------------------------- +// Auth error handler registration +// --------------------------------------------------------------------------- + +/** + * Module-level callback invoked when an auth error (401/403) is encountered + * in handleFetchError and no other error handler has processed it. + * + * Set via setAuthErrorHandler() and called to ensure auth errors are never + * silently swallowed. If no handler is registered, auth errors are logged as + * warnings to ensure they remain actionable. + * + * @see setAuthErrorHandler + */ +let authErrorHandler: ((error: FetchError) => void) | null = null; + +/** + * Register a callback to be invoked when an auth error (401/403) is detected. + * Typically called by the AuthProvider on mount. + * + * If a handler is registered, it will be called when handleFetchError detects + * a 401/403 error. If no handler is registered, auth errors will be logged + * as warnings to the console. + * + * @param handler - Callback to invoke on auth error. Pass `null` to clear. + * + * @example + * // In AuthProvider: + * useEffect(() => { + * setAuthErrorHandler((error) => { + * handleSessionExpired(); + * }); + * return () => setAuthErrorHandler(null); + * }, [handleSessionExpired]); + */ +export function setAuthErrorHandler(handler: ((error: FetchError) => void) | null): void { + authErrorHandler = handler; +} + /** * Extract user-friendly message from a FetchError. * @@ -27,14 +66,24 @@ export function getErrorMessage(error: FetchError): string { * * Handles three error cases: * 1. Request was aborted — silently ignored (expected cleanup) - * 2. Auth error (401/403) — silently handled by AuthProvider (do not display) + * 2. Auth error (401/403) — invokes registered auth handler with fallback logging * 3. Other error — stored in component state or notified via callback * + * **Auth Error Handling Contract:** + * When a 401/403 auth error is detected, handleFetchError ensures deterministic + * error handling by invoking the registered `authErrorHandler` (typically + * AuthProvider's session expiration callback). If no handler is registered, + * the error is logged as a warning to ensure it remains actionable. + * + * This prevents silent error swallowing and ensures auth errors are never lost. + * * Supports both typed FetchError and backward-compatible string error setters. * * @param err - The caught error (any value caught from Promise.catch) * @param setError - State setter (accepts either FetchError | null or string | null) * @param fallback - Default error message if err is not an Error instance + * + * @see setAuthErrorHandler */ export function handleFetchError( err: unknown, @@ -49,8 +98,21 @@ export function handleFetchError( return; } - // Auth errors are handled globally by AuthProvider — do not display locally + // Auth errors are handled globally with registered handler or fallback logging. + // This ensures auth errors are never silently swallowed. if (isAuthError(fetchError)) { + if (authErrorHandler) { + authErrorHandler(fetchError); + } else { + // Fallback: log the auth error to ensure it remains actionable. + // This indicates that AuthProvider either hasn't mounted yet or + // failed to register its handler. + console.warn( + `Auth error (${fetchError.status}) without registered handler. ` + + `AuthProvider may not be mounted or handler registration failed.`, + fetchError, + ); + } return; }