Skip to content

Commit 1001eab

Browse files
committed
fix: two real CORS/Content-Type bugs found by live-testing the admin console
Found by actually running the server + admin console through a real browser session for the first time — neither bug was visible to any existing test, since app.inject()-based integration tests bypass CORS entirely and admin-console unit tests mock fetch directly. 1. @fastify/cors defaults to methods: GET,HEAD,POST only. Every PUT/ PATCH/DELETE call the admin console makes — quota saves, break-glass policy, user/group/policy/service-principal updates, policy/ invitation deletes — was silently failing CORS preflight from a real browser. Fixed by passing an explicit methods list. 2. authorizedRequest() unconditionally set Content-Type: application/json even on bodyless POST calls (activateComputePolicy, createAccessReviewCampacampaign, verifyAiInferenceDeployment). Fastify correctly 400s an empty body declared as JSON. Fixed to only set the header when a body is actually present, matching the pattern already used correctly in the desktop app's own shared-backend-client.ts. Both verified end-to-end against a real server (in-memory stores, a throwaway fake OIDC provider) through an actual browser session — a real OAuth login, a real signed compute-policy created via server/scripts/sign-compute-policy.js and activated, a real quota save — not just re-run against the existing test suite. Added regression tests for both (CORS preflight methods, bodyless-POST headers) so neither can regress silently again.
1 parent 1085880 commit 1001eab

4 files changed

Lines changed: 62 additions & 5 deletions

File tree

‎admin-console/src/lib/api/client.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ const {
2020
deletePolicy,
2121
listUsers,
2222
invokeBreakGlass,
23+
createAccessReviewCampaign,
2324
decideAccessReviewItem,
2425
setBreakGlassPolicy,
2526
proposePolicyVersion,
@@ -81,6 +82,20 @@ describe("api/client", () => {
8182
expect((init?.headers as Record<string, string>).Authorization).toBe("Bearer token-123");
8283
});
8384

85+
it("never sends Content-Type: application/json on a bodyless POST — a real Fastify server 400s an empty body declared as JSON", async () => {
86+
// Regression test: caught by an actual browser session against a
87+
// real server, not by any test that existed before this one — every
88+
// mock in this file resolves regardless of what headers were sent,
89+
// so this specific bug (activateComputePolicy, and every other
90+
// bodyless POST helper in this file, all silently 400ing in
91+
// production) had no test coverage at all until now.
92+
vi.mocked(fetch).mockResolvedValueOnce(jsonResponse(200, { id: "campaign-1" }));
93+
await createAccessReviewCampaign("org-1");
94+
const [, init] = vi.mocked(fetch).mock.calls[0];
95+
expect(init?.body).toBeUndefined();
96+
expect((init?.headers as Record<string, string>)["Content-Type"]).toBeUndefined();
97+
});
98+
8499
it("sends the correct method, path, and JSON body for a POST endpoint", async () => {
85100
vi.mocked(fetch).mockResolvedValueOnce(jsonResponse(201, { id: "u1", organizationId: "org-1" }));
86101
await createUser("org-1", { externalSubject: "idp|new", displayName: "New User" });

‎admin-console/src/lib/api/client.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,20 @@ async function authorizedRequest(path: string, init?: RequestInit): Promise<Resp
4141
}
4242
let response: Response;
4343
try {
44-
response = await fetch(`${API_BASE_URL}${path}`, {
45-
...init,
46-
headers: { ...(init?.headers ?? {}), Authorization: `Bearer ${user.access_token}`, "Content-Type": "application/json" },
47-
});
44+
// Content-Type: application/json only when there actually is a body
45+
// — sending it on a bodyless call (activateComputePolicy,
46+
// createAccessReviewCampaign, verifyAiInferenceDeployment, ...)
47+
// declares a JSON body that never arrives, and Fastify's body
48+
// parser correctly rejects that as a 400 ("Body cannot be empty
49+
// when content-type is set to 'application/json'"). Caught by an
50+
// actual browser session against a real server — every call site
51+
// using this helper for a bodyless POST was silently broken, and no
52+
// existing test could have caught it (admin-console unit tests mock
53+
// fetch directly, so they never exercise a real Content-Type/body
54+
// mismatch the way an actual HTTP client does).
55+
const headers: Record<string, string> = { ...(init?.headers as Record<string, string> ?? {}), Authorization: `Bearer ${user.access_token}` };
56+
if (init?.body !== undefined) headers["Content-Type"] = "application/json";
57+
response = await fetch(`${API_BASE_URL}${path}`, { ...init, headers });
4858
} catch (err) {
4959
throw new ApiError(0, undefined, `Could not reach the admin API: ${err instanceof Error ? err.message : String(err)}`);
5060
}

‎server/src/app.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3470,6 +3470,27 @@ describe("CORS (@fastify/cors, gated on adminConsoleOrigin — see config.ts)",
34703470
expect(response.headers["access-control-allow-origin"]).toBe("https://admin.example-hospital.test");
34713471
});
34723472

3473+
it("allows PUT, PATCH, and DELETE preflights — every mutating admin-console call (quota, break-glass policy, user/group/policy updates, deletes) needs these, not just GET/POST", async () => {
3474+
// Caught by an actual browser session against a real server, not by
3475+
// any prior test: @fastify/cors's own default `methods` is
3476+
// 'GET,HEAD,POST' only (verified directly against the installed
3477+
// @fastify/cors@11 package) — the one preflight test that existed
3478+
// before this only ever asserted a GET preflight, so a real
3479+
// browser's PUT-method quota save silently failed CORS while every
3480+
// .inject()-based integration test (which bypasses CORS entirely)
3481+
// stayed green.
3482+
const app = buildTestApp("https://admin.example-hospital.test");
3483+
for (const method of ["PUT", "PATCH", "DELETE"]) {
3484+
const response = await app.inject({
3485+
method: "OPTIONS",
3486+
url: "/me",
3487+
headers: { origin: "https://admin.example-hospital.test", "access-control-request-method": method, "access-control-request-headers": "authorization" },
3488+
});
3489+
expect(response.statusCode, `${method} preflight`).toBe(204);
3490+
expect(response.headers["access-control-allow-methods"], `${method} preflight`).toContain(method);
3491+
}
3492+
});
3493+
34733494
it("never sets Access-Control-Allow-Credentials (auth here is a header, never an ambient cookie)", async () => {
34743495
const app = buildTestApp("https://admin.example-hospital.test");
34753496
const response = await app.inject({ method: "GET", url: "/health", headers: { origin: "https://admin.example-hospital.test" } });

‎server/src/app.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,18 @@ export function buildApp(options: BuildAppOptions): FastifyInstance {
295295
// do. See config.ts's adminConsoleOrigin doc comment for why this
296296
// is opt-in (unset = no CORS plugin at all) rather than a default
297297
// wildcard/allow-list.
298-
fastify.register(cors, { origin: options.adminConsoleOrigin, credentials: false });
298+
//
299+
// methods must be explicit: @fastify/cors's own default is
300+
// 'GET,HEAD,POST' only (verified directly against the installed
301+
// @fastify/cors@11 package, not assumed) — every PUT/PATCH/DELETE
302+
// route the admin console calls (quota, break-glass policy, user/
303+
// group/policy/service-principal updates, policy/invitation
304+
// deletes) would otherwise fail preflight from a real browser. This
305+
// was caught by an actual browser session against a real server,
306+
// not by any existing test — app.inject()-based integration tests
307+
// bypass CORS entirely, and admin-console unit tests mock fetch
308+
// directly, so neither exercises a real preflight.
309+
fastify.register(cors, { origin: options.adminConsoleOrigin, credentials: false, methods: ["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE"] });
299310
}
300311

301312
// Routes are registered inside a nested plugin, not directly on

0 commit comments

Comments
 (0)