From b916cc2a3bb97cbacea8170c21d0a9ff69ecbbd8 Mon Sep 17 00:00:00 2001 From: William Zujkowski Date: Tue, 8 Sep 2026 18:41:29 -0400 Subject: [PATCH] fix(annotator): cap the CourtListener response body before parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #223 item 3. `fetchWithRetry` called `await response.json()` with no bound, so a misbehaving or redirected endpoint could return an unbounded body and OOM the importer. Search results are tens of kilobytes; the cap is 8 MiB, far above any legitimate page and far below what threatens the runner. Deliberately not the fetcher's MAX_DOWNLOAD_BYTES (300 MiB) — that bounds bulk XML downloads and would be a cap in name only here. Two layers, because either alone is insufficient: 1. Content-Length, when it declares more than the cap, rejects before a byte of body is read. 2. A streaming read that aborts the moment the running total exceeds the cap. Content-Length is absent on a chunked response and attacker- controlled on a redirected one, so a server that lies walks straight past layer 1. Buffering via arrayBuffer() and checking afterwards would defeat the purpose: the OOM happens during the read. Mirrors the fetcher's exceedsContentLengthLimit + readBytesCapped pair. The logic is duplicated rather than shared because @civic-source/annotator does not depend on @civic-source/fetcher; hoisting both into @civic-source/shared is the DRY fix and is left as a follow-up rather than bundled into a security change. The reader optional-chains `headers` and tolerates a missing `body`. A real Response always has both, but the existing tests stub fetch with `{ ok, status, json }` and nothing else. Assuming more than is needed would turn a size guard into an availability bug — and the retry loop would have swallowed the TypeError as a transient failure, which is how this surfaced: two existing tests started taking 3s and failing. Items 1 and 2 of #223 are untouched. Item 1 (host pinning) is already fixed on main by `courtListenerSourceUrl`, which also rejects userinfo — more than the issue asked for. Item 2 wants the shared fetchWithRetry, which restructures retry semantics and deserves its own change. --- .../annotator/src/__tests__/client.test.ts | 56 +++++++++++- packages/annotator/src/client.ts | 91 ++++++++++++++++++- packages/annotator/src/constants.ts | 13 +++ 3 files changed, 156 insertions(+), 4 deletions(-) diff --git a/packages/annotator/src/__tests__/client.test.ts b/packages/annotator/src/__tests__/client.test.ts index 2edaaff..7878f6f 100644 --- a/packages/annotator/src/__tests__/client.test.ts +++ b/packages/annotator/src/__tests__/client.test.ts @@ -1,8 +1,8 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { createLogger } from '@civic-source/shared'; -import { CourtListenerClient, isCourtListenerResult } from '../client.js'; -import { COURTLISTENER_RATE_LIMITER, RATE_LIMIT_PER_HOUR } from '../constants.js'; +import { CourtListenerClient, isCourtListenerResult, readJsonCapped } from '../client.js'; +import { COURTLISTENER_RATE_LIMITER, RATE_LIMIT_PER_HOUR, MAX_API_RESPONSE_BYTES } from '../constants.js'; describe('COURTLISTENER_RATE_LIMITER (#230)', () => { it('sustains exactly RATE_LIMIT_PER_HOUR tokens per hour (not the old ~7200)', () => { @@ -85,3 +85,55 @@ describe('CourtListenerClient.searchByStatute (#237)', () => { expect(result.value).toEqual([]); }); }); + +describe('readJsonCapped — response size cap (#223 item 3)', () => { + const CAP = MAX_API_RESPONSE_BYTES; + + /** A Response whose body streams `chunks`, with an optional content-length. */ + function streaming(chunks: Uint8Array[], contentLength?: string): Response { + const stream = new ReadableStream({ + start(controller) { + for (const c of chunks) controller.enqueue(c); + controller.close(); + }, + }); + const headers = new Headers(contentLength === undefined ? {} : { 'content-length': contentLength }); + return new Response(stream, { headers }); + } + + it('parses a normal body', async () => { + // The benign population: an ordinary search page must still work. + const body = new TextEncoder().encode(JSON.stringify({ results: [VALID] })); + const result = await readJsonCapped(streaming([body], String(body.byteLength))); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.value).toEqual({ results: [VALID] }); + }); + + it('rejects up-front when Content-Length declares more than the cap', async () => { + const result = await readJsonCapped(streaming([new Uint8Array(8)], String(CAP + 1))); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error.message).toMatch(/declares .* over the .*-byte cap/); + }); + + it('rejects a body that exceeds the cap while streaming, despite an honest-looking Content-Length', async () => { + // THE case the Content-Length check alone cannot catch: the header is + // absent or lying, so the only defence is aborting mid-read. Buffering + // first and checking after would already have spent the memory. + const chunk = new Uint8Array(1024 * 1024); // 1 MiB + const chunks = Array.from({ length: 9 }, () => chunk); // 9 MiB > 8 MiB cap + const result = await readJsonCapped(streaming(chunks, '32')); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error.message).toMatch(/exceeded the .*-byte cap/); + }); + + it('returns an error Result for malformed JSON rather than throwing', async () => { + const body = new TextEncoder().encode('{ not json'); + const result = await readJsonCapped(streaming([body])); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error.message).toMatch(/Malformed JSON/); + }); +}); diff --git a/packages/annotator/src/client.ts b/packages/annotator/src/client.ts index fc5a4d2..9be71b6 100644 --- a/packages/annotator/src/client.ts +++ b/packages/annotator/src/client.ts @@ -5,8 +5,94 @@ import { SEARCH_ENDPOINT, COURTLISTENER_RATE_LIMITER, DEFAULT_PAGE_SIZE, + MAX_API_RESPONSE_BYTES, } from './constants.js'; +/** + * Read a response body as JSON without letting an unbounded body reach memory + * (#223 item 3). + * + * Two layers, because either alone is insufficient: + * + * 1. `Content-Length`, when present and over the cap, rejects before a single + * byte of body is read. This is the cheap path and handles the honest case. + * 2. A streaming read that aborts the moment the running total exceeds the cap. + * Necessary because Content-Length is absent on a chunked response and is + * attacker-controlled on a redirected one — a server that lies about it + * would walk straight past layer 1. Buffering via `arrayBuffer()` and then + * checking the length would defeat the purpose: the OOM happens during the + * read, not after it. + * + * Mirrors the fetcher's `exceedsContentLengthLimit` + `readBytesCapped` pair. + * The logic is duplicated rather than shared because `@civic-source/annotator` + * does not depend on `@civic-source/fetcher`; hoisting both into + * `@civic-source/shared` would be the DRY fix and is left as a follow-up rather + * than bundled into a security change. + */ +export async function readJsonCapped(response: Response): Promise> { + // Optional-chained because this must not assume more of the object than it + // needs: a real Response always carries `headers` and `body`, but test doubles + // and polyfilled fetches routinely supply only `.json()`. Throwing on those + // would turn a size guard into an availability bug, and the retry loop would + // swallow the TypeError as a transient failure. + const declared = Number(response.headers?.get('content-length')); + if (Number.isFinite(declared) && declared > MAX_API_RESPONSE_BYTES) { + return err( + new Error( + `Response body declares ${String(declared)} bytes, over the ${String(MAX_API_RESPONSE_BYTES)}-byte cap` + ) + ); + } + + const body = response.body; + if (body === null || body === undefined) { + // No readable stream to bound — an empty body, or a Response-like without + // one. Nothing can grow during the read, so defer to the object's own + // parse; the cap has no work to do here. + try { + return ok((await response.json()) as unknown); + } catch (error: unknown) { + return err( + new Error( + `Malformed JSON response: ${error instanceof Error ? error.message : String(error)}` + ) + ); + } + } + + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + if (value === undefined) continue; + total += value.byteLength; + if (total > MAX_API_RESPONSE_BYTES) { + await reader.cancel(); + return err( + new Error(`Response body exceeded the ${String(MAX_API_RESPONSE_BYTES)}-byte cap`) + ); + } + chunks.push(value); + } + } finally { + reader.releaseLock(); + } + + return parseJsonResult(Buffer.concat(chunks).toString('utf-8')); +} + +/** Parse JSON into a Result rather than throwing past the retry loop. */ +function parseJsonResult(text: string): Result { + try { + return ok(JSON.parse(text) as unknown); + } catch (error: unknown) { + return err(new Error(`Malformed JSON response: ${error instanceof Error ? error.message : String(error)}`)); + } +} + /** Raw result shape from the CourtListener search API */ export interface CourtListenerResult { caseName: string; @@ -114,8 +200,9 @@ export class CourtListenerClient { }); if (response.ok) { - const data: unknown = await response.json(); - return ok(data); + const parsed = await readJsonCapped(response); + if (!parsed.ok) return parsed; + return ok(parsed.value); } if (response.status === 401) { diff --git a/packages/annotator/src/constants.ts b/packages/annotator/src/constants.ts index 7c6681d..ffc4e13 100644 --- a/packages/annotator/src/constants.ts +++ b/packages/annotator/src/constants.ts @@ -49,6 +49,19 @@ export const COURT_PRIORITY: Record = { District: 2, }; +/** + * Hard cap on a CourtListener API response body, in bytes (#223 item 3). + * + * These are JSON search results — a page of opinions is tens of kilobytes. + * 8 MiB is far above any legitimate response and far below what would + * threaten the importer, whose failure mode without a cap is an OOM on the + * runner rather than a handled error. + * + * Deliberately NOT the fetcher's MAX_DOWNLOAD_BYTES (300 MiB): that bounds + * bulk XML downloads, and reusing it here would be a cap in name only. + */ +export const MAX_API_RESPONSE_BYTES = 8 * 1024 * 1024; + /** * Validate that the COURTLISTENER_API_TOKEN environment variable is set. * Returns the token string or throws a descriptive error.