From 0a350b3acca799df44df66638ba9178133fcf26c Mon Sep 17 00:00:00 2001 From: Lukas Date: Wed, 29 Apr 2026 19:52:17 +0200 Subject: [PATCH] Optimize API client headers by method - only set Content-Type and CSRF header as needed - Only set Content-Type header for requests with a body (POST, PUT, DELETE with body) - Only set X-BanGUI-Request CSRF header for mutating methods (POST, PUT, DELETE, PATCH) - GET, HEAD, OPTIONS requests no longer include unnecessary headers, reducing CORS preflights - Update Web-Development.md to clarify conditional header behavior - Add comprehensive tests for header behavior by HTTP method This reduces unnecessary CORS preflight requests on GET endpoints while maintaining CSRF protection on state-mutating requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 18 --- Docs/Web-Development.md | 6 +- frontend/src/api/__tests__/client.test.ts | 161 +++++++++++++++++++++- frontend/src/api/__tests__/map.test.ts | 7 +- frontend/src/api/client.ts | 24 +++- 5 files changed, 188 insertions(+), 28 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index c289f95..0fa22ed 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,21 +1,3 @@ -## 34) Setup redirect allowlist uses broad prefix matching -- Where found: - - [backend/app/main.py](backend/app/main.py#L434) -- Why this is needed: - - Prefix-based allow rules are fragile for future route additions. -- Goal: - - Use exact path or route-level allow policy. -- What to do: - - Replace startswith matching with explicit allowlist checks. -- Possible traps and issues: - - API docs and setup flow paths must remain reachable. -- Docs changes needed: - - Add setup guard route policy documentation. -- Doc references: - - [backend/app/main.py](backend/app/main.py) - ---- - ## 35) API client sends JSON and CSRF header for every request method - Where found: - [frontend/src/api/client.ts](frontend/src/api/client.ts) diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index 7f7a3fc..7f60046 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -119,8 +119,10 @@ fetchBans(24, ctrl.signal) // Pass the signal to enable cancellation on unmount All state-mutating requests (POST, PUT, DELETE, PATCH) automatically include the custom header `X-BanGUI-Request: 1` via the central API client. This protects against Cross-Site Request Forgery (CSRF) attacks by requiring a custom header that cross-site JavaScript cannot set without CORS preflight. **How it works:** -- The `request()` function in `api/client.ts` includes `"X-BanGUI-Request": "1"` in the default headers. -- GET, HEAD, and OPTIONS requests are unaffected. +- The `request()` function in `api/client.ts` conditionally sets headers based on the request method and body: + - `Content-Type: application/json` is only set for requests with a body (POST, PUT, DELETE with body) to avoid unnecessary CORS preflights on GET requests. + - `X-BanGUI-Request: 1` is only set for state-mutating requests (POST, PUT, DELETE, PATCH). +- GET, HEAD, and OPTIONS requests are unaffected (no CSRF header, no Content-Type header). - Bearer token authentication bypasses the check (tokens are not CSRF-vulnerable). - The backend `CsrfMiddleware` validates this header for cookie-authenticated state-mutating requests. - Requests missing the header receive a `403 Forbidden` response. diff --git a/frontend/src/api/__tests__/client.test.ts b/frontend/src/api/__tests__/client.test.ts index 5111c36..821bf5a 100644 --- a/frontend/src/api/__tests__/client.test.ts +++ b/frontend/src/api/__tests__/client.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, beforeEach, vi } from "vitest"; -import { ApiError, get, isAuthError, setUnauthorizedHandler } from "../client"; +import type { Mock } from "vitest"; +import { ApiError, get, post, put, del, isAuthError, setUnauthorizedHandler } from "../client"; describe("api/client", () => { beforeEach(() => { @@ -7,6 +8,10 @@ describe("api/client", () => { setUnauthorizedHandler(null); }); + // ========================================================================= + // Authorization tests + // ========================================================================= + it("invokes unauthorized handler for 401 responses", async () => { const handler = vi.fn(); setUnauthorizedHandler(handler); @@ -58,4 +63,158 @@ describe("api/client", () => { expect(isAuthError(new ApiError(401, "Unauthorized"))).toBe(true); expect(isAuthError(new ApiError(403, "Forbidden"))).toBe(true); }); + + // ========================================================================= + // Header tests: Content-Type + // ========================================================================= + + it("does not set Content-Type header for GET requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await get("/test"); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["Content-Type"]).toBeUndefined(); + }); + + it("sets Content-Type header for POST requests with body", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await post("/test", { key: "value" }); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["Content-Type"]).toBe("application/json"); + }); + + it("sets Content-Type header for PUT requests with body", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await put("/test", { key: "value" }); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["Content-Type"]).toBe("application/json"); + }); + + it("sets Content-Type header for DELETE requests with body", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 204, + }); + + await del("/test", { key: "value" }); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["Content-Type"]).toBe("application/json"); + }); + + it("does not set Content-Type header for DELETE requests without body", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 204, + }); + + await del("/test"); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["Content-Type"]).toBeUndefined(); + }); + + // ========================================================================= + // Header tests: X-BanGUI-Request (CSRF) + // ========================================================================= + + it("does not set X-BanGUI-Request header for GET requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await get("/test"); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["X-BanGUI-Request"]).toBeUndefined(); + }); + + it("sets X-BanGUI-Request header for POST requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await post("/test", { key: "value" }); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["X-BanGUI-Request"]).toBe("1"); + }); + + it("sets X-BanGUI-Request header for PUT requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await put("/test", { key: "value" }); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["X-BanGUI-Request"]).toBe("1"); + }); + + it("sets X-BanGUI-Request header for DELETE requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 204, + }); + + await del("/test"); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + const headers = options?.headers as Record; + expect(headers?.["X-BanGUI-Request"]).toBe("1"); + }); + + it("includes credentials on all requests", async () => { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + json: vi.fn().mockResolvedValue({ data: "test" }), + }); + + await get("/test"); + + const fetchMock = global.fetch as Mock; + const options = fetchMock.mock.calls[0]?.[1] as Record; + expect(options?.credentials).toBe("include"); + }); }); diff --git a/frontend/src/api/__tests__/map.test.ts b/frontend/src/api/__tests__/map.test.ts index f69621a..c0c55d8 100644 --- a/frontend/src/api/__tests__/map.test.ts +++ b/frontend/src/api/__tests__/map.test.ts @@ -20,7 +20,8 @@ describe("fetchBansByCountry", () => { await fetchBansByCountry("24h", "all", "fail2ban", "US"); expect(get).toHaveBeenCalledWith( - `${ENDPOINTS.dashboardBansByCountry}?range=24h&country_code=US` + `${ENDPOINTS.dashboardBansByCountry}?range=24h&country_code=US`, + undefined ); }); @@ -28,7 +29,9 @@ describe("fetchBansByCountry", () => { await fetchBansByCountry("24h", "all", "fail2ban"); expect(get).toHaveBeenCalledWith( - `${ENDPOINTS.dashboardBansByCountry}?range=24h` + `${ENDPOINTS.dashboardBansByCountry}?range=24h`, + undefined ); }); }); + diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 23a01ac..0f3e2ac 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -89,14 +89,28 @@ export function setUnauthorizedHandler(handler: (() => void) | null): void { */ async function request(url: string, options: RequestInit = {}): Promise { try { + const method: string = options.method ?? "GET"; + const hasBody: boolean = options.body !== undefined && options.body !== null; + const isMutatingMethod: boolean = ["POST", "PUT", "DELETE", "PATCH"].includes(method); + + const headers: Record = { + ...(options.headers as Record | undefined), + }; + + // Only set Content-Type for requests with a body to avoid unnecessary CORS preflights. + if (hasBody) { + headers["Content-Type"] = "application/json"; + } + + // Only set CSRF header for state-mutating requests (not for GET/HEAD/OPTIONS). + if (isMutatingMethod) { + headers["X-BanGUI-Request"] = "1"; + } + const response: Response = await fetch(url, { ...options, credentials: "include", - headers: { - "Content-Type": "application/json", - "X-BanGUI-Request": "1", - ...(options.headers as Record | undefined), - }, + headers, }); if (!response.ok) {