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>
This commit is contained in:
@@ -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
|
## 21) Silent auth error swallow in fetch error utility
|
||||||
- Where found:
|
- Where found:
|
||||||
- [frontend/src/utils/fetchError.ts](frontend/src/utils/fetchError.ts)
|
- [frontend/src/utils/fetchError.ts](frontend/src/utils/fetchError.ts)
|
||||||
|
|||||||
@@ -971,10 +971,43 @@ HttpOnly cookies provide superior protection against XSS (Cross-Site Scripting)
|
|||||||
|
|
||||||
### Error Handling
|
### Error Handling
|
||||||
|
|
||||||
When an API request returns 401 or 403:
|
**Auth Error Handling Contract:**
|
||||||
1. The `client.ts` module dispatches a `SESSION_EXPIRED_EVENT`.
|
|
||||||
2. The `AuthProvider` listener handles it by clearing `isAuthenticated` and redirecting to `/login`.
|
The frontend employs a **dual-handler approach** to ensure 401/403 auth errors are never silently swallowed:
|
||||||
3. Hooks must use `handleFetchError` (from `utils/fetchError.ts`) to avoid displaying auth errors as user-facing error messages.
|
|
||||||
|
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.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -20,6 +20,17 @@
|
|||||||
* The `isAuthenticated` state persists in `sessionStorage` to survive page
|
* The `isAuthenticated` state persists in `sessionStorage` to survive page
|
||||||
* refreshes within the browser tab but is cleared on tab close. The session
|
* refreshes within the browser tab but is cleared on tab close. The session
|
||||||
* cookie itself persists according to the backend's cookie settings.
|
* 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, {
|
import React, {
|
||||||
@@ -32,6 +43,7 @@ import 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 } from "../api/client";
|
||||||
|
import { setAuthErrorHandler } from "../utils/fetchError";
|
||||||
import { SessionValidationLoading } from "../components/SessionValidationLoading";
|
import { SessionValidationLoading } from "../components/SessionValidationLoading";
|
||||||
import { useSessionValidation } from "../hooks/useSessionValidation";
|
import { useSessionValidation } from "../hooks/useSessionValidation";
|
||||||
|
|
||||||
@@ -103,8 +115,12 @@ export function AuthProvider({
|
|||||||
setUnauthorizedHandler((): void => {
|
setUnauthorizedHandler((): void => {
|
||||||
handleSessionExpired();
|
handleSessionExpired();
|
||||||
});
|
});
|
||||||
|
setAuthErrorHandler((): void => {
|
||||||
|
handleSessionExpired();
|
||||||
|
});
|
||||||
return (): void => {
|
return (): void => {
|
||||||
setUnauthorizedHandler(null);
|
setUnauthorizedHandler(null);
|
||||||
|
setAuthErrorHandler(null);
|
||||||
};
|
};
|
||||||
}, [handleSessionExpired]);
|
}, [handleSessionExpired]);
|
||||||
|
|
||||||
|
|||||||
@@ -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 { ApiError } from "../../api/client";
|
||||||
import { handleFetchError, createStringErrorAdapter } from "../fetchError";
|
import { handleFetchError, createStringErrorAdapter, setAuthErrorHandler } from "../fetchError";
|
||||||
|
|
||||||
describe("utils/fetchError", () => {
|
describe("utils/fetchError", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
// Clear handler before each test
|
||||||
|
setAuthErrorHandler(null);
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
// Clean up after each test
|
||||||
|
setAuthErrorHandler(null);
|
||||||
|
});
|
||||||
|
|
||||||
it("ignores AbortError errors", () => {
|
it("ignores AbortError errors", () => {
|
||||||
const setError = vi.fn();
|
const setError = vi.fn();
|
||||||
handleFetchError(new DOMException("Aborted", "AbortError"), setError, "fallback");
|
handleFetchError(new DOMException("Aborted", "AbortError"), setError, "fallback");
|
||||||
expect(setError).not.toHaveBeenCalled();
|
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();
|
const setError = vi.fn();
|
||||||
handleFetchError(new ApiError(401, "Unauthorized"), setError, "fallback");
|
handleFetchError(new ApiError(401, "Unauthorized"), setError, "fallback");
|
||||||
|
expect(authHandler).toHaveBeenCalledOnce();
|
||||||
expect(setError).not.toHaveBeenCalled();
|
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", () => {
|
it("sets fallback for normal errors", () => {
|
||||||
const setError = vi.fn();
|
const setError = vi.fn();
|
||||||
handleFetchError(new Error("Oops"), createStringErrorAdapter(setError), "fallback");
|
handleFetchError(new Error("Oops"), createStringErrorAdapter(setError), "fallback");
|
||||||
|
|||||||
@@ -1,6 +1,45 @@
|
|||||||
import type { FetchError } from "../types/api";
|
import type { FetchError } from "../types/api";
|
||||||
import { isAuthError, isAbortError } 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.
|
* Extract user-friendly message from a FetchError.
|
||||||
*
|
*
|
||||||
@@ -27,14 +66,24 @@ export function getErrorMessage(error: FetchError): string {
|
|||||||
*
|
*
|
||||||
* Handles three error cases:
|
* Handles three error cases:
|
||||||
* 1. Request was aborted — silently ignored (expected cleanup)
|
* 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
|
* 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.
|
* Supports both typed FetchError and backward-compatible string error setters.
|
||||||
*
|
*
|
||||||
* @param err - The caught error (any value caught from Promise.catch)
|
* @param err - The caught error (any value caught from Promise.catch)
|
||||||
* @param setError - State setter (accepts either FetchError | null or string | null)
|
* @param setError - State setter (accepts either FetchError | null or string | null)
|
||||||
* @param fallback - Default error message if err is not an Error instance
|
* @param fallback - Default error message if err is not an Error instance
|
||||||
|
*
|
||||||
|
* @see setAuthErrorHandler
|
||||||
*/
|
*/
|
||||||
export function handleFetchError(
|
export function handleFetchError(
|
||||||
err: unknown,
|
err: unknown,
|
||||||
@@ -49,8 +98,21 @@ export function handleFetchError(
|
|||||||
return;
|
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 (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;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user