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>
This commit is contained in:
@@ -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
|
## 35) API client sends JSON and CSRF header for every request method
|
||||||
- Where found:
|
- Where found:
|
||||||
- [frontend/src/api/client.ts](frontend/src/api/client.ts)
|
- [frontend/src/api/client.ts](frontend/src/api/client.ts)
|
||||||
|
|||||||
@@ -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.
|
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:**
|
**How it works:**
|
||||||
- The `request()` function in `api/client.ts` includes `"X-BanGUI-Request": "1"` in the default headers.
|
- The `request()` function in `api/client.ts` conditionally sets headers based on the request method and body:
|
||||||
- GET, HEAD, and OPTIONS requests are unaffected.
|
- `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).
|
- Bearer token authentication bypasses the check (tokens are not CSRF-vulnerable).
|
||||||
- The backend `CsrfMiddleware` validates this header for cookie-authenticated state-mutating requests.
|
- The backend `CsrfMiddleware` validates this header for cookie-authenticated state-mutating requests.
|
||||||
- Requests missing the header receive a `403 Forbidden` response.
|
- Requests missing the header receive a `403 Forbidden` response.
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { describe, expect, it, beforeEach, vi } from "vitest";
|
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", () => {
|
describe("api/client", () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
@@ -7,6 +8,10 @@ describe("api/client", () => {
|
|||||||
setUnauthorizedHandler(null);
|
setUnauthorizedHandler(null);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// =========================================================================
|
||||||
|
// Authorization tests
|
||||||
|
// =========================================================================
|
||||||
|
|
||||||
it("invokes unauthorized handler for 401 responses", async () => {
|
it("invokes unauthorized handler for 401 responses", async () => {
|
||||||
const handler = vi.fn();
|
const handler = vi.fn();
|
||||||
setUnauthorizedHandler(handler);
|
setUnauthorizedHandler(handler);
|
||||||
@@ -58,4 +63,158 @@ describe("api/client", () => {
|
|||||||
expect(isAuthError(new ApiError(401, "Unauthorized"))).toBe(true);
|
expect(isAuthError(new ApiError(401, "Unauthorized"))).toBe(true);
|
||||||
expect(isAuthError(new ApiError(403, "Forbidden"))).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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
const headers = options?.headers as Record<string, string>;
|
||||||
|
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<string, unknown>;
|
||||||
|
expect(options?.credentials).toBe("include");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -20,7 +20,8 @@ describe("fetchBansByCountry", () => {
|
|||||||
await fetchBansByCountry("24h", "all", "fail2ban", "US");
|
await fetchBansByCountry("24h", "all", "fail2ban", "US");
|
||||||
|
|
||||||
expect(get).toHaveBeenCalledWith(
|
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");
|
await fetchBansByCountry("24h", "all", "fail2ban");
|
||||||
|
|
||||||
expect(get).toHaveBeenCalledWith(
|
expect(get).toHaveBeenCalledWith(
|
||||||
`${ENDPOINTS.dashboardBansByCountry}?range=24h`
|
`${ENDPOINTS.dashboardBansByCountry}?range=24h`,
|
||||||
|
undefined
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -89,14 +89,28 @@ export function setUnauthorizedHandler(handler: (() => void) | null): void {
|
|||||||
*/
|
*/
|
||||||
async function request<T>(url: string, options: RequestInit = {}): Promise<T> {
|
async function request<T>(url: string, options: RequestInit = {}): Promise<T> {
|
||||||
try {
|
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<string, string> = {
|
||||||
|
...(options.headers as Record<string, string> | 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, {
|
const response: Response = await fetch(url, {
|
||||||
...options,
|
...options,
|
||||||
credentials: "include",
|
credentials: "include",
|
||||||
headers: {
|
headers,
|
||||||
"Content-Type": "application/json",
|
|
||||||
"X-BanGUI-Request": "1",
|
|
||||||
...(options.headers as Record<string, string> | undefined),
|
|
||||||
},
|
|
||||||
});
|
});
|
||||||
|
|
||||||
if (!response.ok) {
|
if (!response.ok) {
|
||||||
|
|||||||
Reference in New Issue
Block a user