fix(web): reject 2xx responses whose body is not JSON (#194)

BlackHole1 · web-flow · commit 6f172d30d4b7 · 2026-07-25T10:54:19.000-04:00
## Summary - `readJson` no longer collapses a failed JSON parse into `null` typed as the payload - a 2xx response whose body has bytes that are not JSON now raises `ApiError` with the status - the tolerant paths that were intentional stay: non-JSON error bodies still fall back to the status message, and an empty 2xx body still resolves to `null` ## Why ```ts const payload = (await response.json().catch(() => null)) as unknown; if (!response.ok) { throw new ApiError(...); } return payload as T; // null, typed as T ``` The `catch(() => null)` exists so that an error response carrying HTML or an empty body still produces a usable `ApiError`. But it applies to successful responses too, so any transport-level corruption under a 200 is handed back as `null` wearing the payload's type. Nothing fails at the boundary; the app crashes later, wherever a caller first reads a property. That is not hypothetical. #193 fixes a Cloudflare deployment where the Workers runtime re-encoded an already-gzipped body, so `/api/auth/session` returned 200 with `1f 8b` bytes. The symptom operators actually saw was a `TypeError` reading `authenticated` — several frames away from the cause, with a green network tab. This is independent of #193 and does not overlap with it: that PR stops producing the bad body, this one stops the client from disguising a bad body as a valid payload. ## Notes - The body is read once via `response.text()` and parsed locally, so the 2xx and non-2xx paths can differ. `parseJson` returns `undefined` for a non-JSON body, which `JSON.parse` can never produce for a valid one. - No `/api/*` endpoint returns an empty 2xx body today (every `DELETE` handler responds with JSON), but empty bodies keep resolving to `null` so a future `204` is not a behavior change. ## Validation - `npx vitest run` — 57 files / 528 tests pass - new `web/src/api.test.ts` covers all five paths; the malformed-2xx case fails against the previous implementation (`expected null to be an instance of ApiError`) - `npm run lint`, `oxfmt --check .`, `tsc -p src/tsconfig.json --noEmit`, and `tsc -p web/tsconfig.json --noEmit` are clean
diff --git a/web/src/api.test.ts b/web/src/api.test.ts
@@ -0,0 +1,50 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { apiDelete, ApiError, apiGet } from "./api";
+
+describe("readJson", () => {
+  afterEach(() => {
+    vi.unstubAllGlobals();
+  });
+
+  it("returns the parsed payload for successful responses", async () => {
+    stubFetch(Response.json({ authenticated: true }));
+
+    await expect(apiGet("/api/auth/session")).resolves.toEqual({ authenticated: true });
+  });
+
+  it("rejects successful responses whose body is not JSON", async () => {
+    // A gzip payload reaching the client undecoded is the real-world case: the
+    // status is 200, so nothing else in the app treats the response as failed.
+    stubFetch(new Response(new Uint8Array([0x1f, 0x8b, 0x08, 0x00]), { status: 200 }));
+
+    const error = await apiGet("/api/auth/session").catch((reason: unknown) => reason);
+    expect(error).toBeInstanceOf(ApiError);
+    expect((error as ApiError).status).toBe(200);
+    expect((error as ApiError).message).toMatch(/not JSON/);
+  });
+
+  it("keeps returning null for successful responses with an empty body", async () => {
+    stubFetch(new Response(null, { status: 204 }));
+
+    await expect(apiDelete("/api/runtime-tokens/token-1")).resolves.toBeNull();
+  });
+
+  it("reports the server error message when the failed body is JSON", async () => {
+    stubFetch(Response.json({ errorMessage: "Connection not found." }, { status: 404 }));
+
+    await expect(apiGet("/api/connections/example")).rejects.toThrow("Connection not found.");
+  });
+
+  it("falls back to the status when the failed body is not JSON", async () => {
+    stubFetch(new Response("<html>502</html>", { status: 502 }));
+
+    await expect(apiGet("/api/providers")).rejects.toThrow("Request failed with 502");
+  });
+});
+
+function stubFetch(response: Response): void {
+  vi.stubGlobal(
+    "fetch",
+    vi.fn(async () => response),
+  );
+}
diff --git a/web/src/api.ts b/web/src/api.ts
@@ -65,13 +65,32 @@ function headersFor(options: RequestOptions, json = false): Headers {
 }
 
 async function readJson<T>(response: Response): Promise<T> {
-  const payload = (await response.json().catch(() => null)) as unknown;
+  const payload = parseJson(await response.text());
   if (!response.ok) {
     throw new ApiError(response.status, errorMessage(payload) ?? `Request failed with ${response.status}`);
   }
+  // A successful response whose body is not JSON means something rewrote it in
+  // transit. Returning the failed parse as T would hand the caller a null typed
+  // as the payload, and the first property read off it crashes far from the
+  // cause; a compressing proxy did exactly that to the whole dashboard once.
+  if (payload === undefined) {
+    throw new ApiError(response.status, `Request succeeded with ${response.status} but the response body was not JSON`);
+  }
   return payload as T;
 }
 
+/** Returns `undefined` for a body that is not JSON. `JSON.parse` never does. */
+function parseJson(body: string): unknown {
+  if (body === "") {
+    return null;
+  }
+  try {
+    return JSON.parse(body) as unknown;
+  } catch {
+    return undefined;
+  }
+}
+
 function errorMessage(payload: unknown): string | undefined {
   if (!payload || typeof payload !== "object") {
     return undefined;
