Skip to content

fix(annotator): cap the CourtListener response body before parsing - #268

Open
williamzujkowski wants to merge 1 commit into
mainfrom
fix/223-annotator-response-cap
Open

fix(annotator): cap the CourtListener response body before parsing#268
williamzujkowski wants to merge 1 commit into
mainfrom
fix/223-annotator-response-cap

Conversation

@williamzujkowski

Copy link
Copy Markdown
Collaborator

Closes #223 item 3. Items 1 and 2 are addressed below but not in this diff.

The gap

fetchWithRetry called await response.json() with no bound. A misbehaving or redirected endpoint could return an unbounded body and OOM the importer — a crash rather than a handled error.

The fix — 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. Cheap, handles the honest case.
  2. A streaming read that aborts the moment the running total passes the cap. This is the layer that matters: 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 with arrayBuffer() and checking afterwards would defeat the purpose — the OOM happens during the read, not after.

Cap is 8 MiB. Search results are tens of kilobytes, so that is 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 and would be a cap in name only here.

A regression I introduced and caught

The first version called response.headers.get(...) and response.body.getReader() directly. Two existing tests then began failing at ~3s each:

// the existing stub
({ ok: true, status: 200, json: async () => body }) as unknown as Response

No headers, no body. The TypeError was swallowed by the retry loop and re-tried with backoff — which is why it presented as a timeout rather than an error.

A real Response always has both, so the stub is unfaithful. But the right fix is still to tolerate it: assuming more of the object than the guard needs turns a size guard into an availability bug, and the retry loop hides the cause. The reader now optional-chains headers and treats a missing body as "no stream to bound", deferring to the object's own parse. No test was changed to accommodate the production code.

Verification

  • 11/11 client.test.ts pass · tsc --noEmit 0 errors · eslint clean
  • Mutation-verified, each layer separately:
Mutation Result
Drop the Content-Length pre-check rejects up-front when Content-Length declares more than the cap fails
Drop the streaming cap rejects a body that exceeds the cap while streaming, despite an honest-looking Content-Length fails

Four new tests, including the benign case (an ordinary search page still parses) and malformed JSON returning an error Result rather than throwing past the retry loop.

On the other two items

  • Item 1 (host pinning) is already fixed on main. courtListenerSourceUrl resolves via new URL(absoluteUrl, base), pins the origin, and additionally rejects userinfo (https://evil@www.courtlistener.com/…) — which the issue did not ask for. Worth ticking off in security/robustness: harden annotator CourtListener client (host-pinning, shared retry, response cap) #223.
  • Item 2 (shared fetchWithRetry) is still open. The shared version returns Result<Response>, so adopting it means restructuring the annotator's JSON parse and 401 mapping — a real change to retry semantics in security-sensitive code, and it deserves its own PR rather than riding along here.

Known duplication

readJsonCapped reimplements the fetcher's exceedsContentLengthLimit + readBytesCapped pair because @civic-source/annotator does not depend on @civic-source/fetcher. Hoisting both into @civic-source/shared is the DRY fix; I left it out rather than bundle a cross-package refactor into a security change. Happy to do it as a follow-up if you want it.

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.
@williamzujkowski
williamzujkowski requested a review from a team as a code owner September 8, 2026 22:41
@williamzujkowski

Copy link
Copy Markdown
Collaborator Author

Not merging this without your review — that was unanimous.

I put the merge decision to a 7-voter panel. The option that included admin-overriding this PR's REVIEW_REQUIRED protection got zero votes. Both the seats that voted to merge nothing at all and the seats that voted to merge other PRs agreed on this one, for the same reason:

Possessing an admin token does not establish authority to waive another organization's protection.

and

The proposer also introduced and then fixed a regression on this same PR (the headers assumption breaking two tests via a swallowed TypeError), which is precisely the profile of change that benefits from a second reader.

That second point is fair and I'd rather it be said here than not: the first version of this reader assumed response.headers existed, two existing tests started failing at ~3s each, and the retry loop swallowed the TypeError as a transient failure. I caught and fixed it — but a security-path change where the author already tripped once is exactly what a required review is for.

What this needs: a reviewer. The change is green (3/3 checks, matching a merged control PR in this repo), 11/11 tests, both cap layers mutation-verified independently. Nothing is blocked on me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant