Security: cap the body by bytes read, not by a declared Content-Length #60

Approved. Both nits from the last round are fixed — MAX_REQUEST_DRAIN_BYTES now lives in src/limits.ts with the rest of the floor and is passed in, and src/body.test.ts pins the module directly. Anchored here because line 23 is the behaviour that was untested before and is now the one I checked hardest.

Verified on this head rebuilt over current main (merges clean; src/server.ts and src/limits.ts were untouched on main since the base):

• bun test: 151 pass, 0 fail, 14 files. The drain-bound test is not decorative: deleting just the if (total >= drainBytes) break; line makes exactly that test fail and the other three pass, so it pins the bound and not the cap. The chunked server test still fails if src/server.ts is reverted.
• Raw-socket probes against a locally run server: a 4 MB chunked POST answers 413 with the security floor intact and the next request on the same kept-alive connection is a clean 200 — abandoning the read at line 23 does not poison connection reuse.
• Identity survives the rebuilt new Request(request, { body }) on both doors: a signed create + claim lists the board as mine on /, POST /mcp tools/call list_boards returns it under the same identity, cross-site POST is still 403 and now refused before a byte is read.
• Preview: / in ~130 ms with the full floor, three back-to-back POST /boards all 303, a 200 KB body refused 413, screenshots unchanged.
• Header edge cases stay safe: content-length: abc, a duplicated header, -1 all fall through to the byte counter, which is the truth of record. Only POST carries a body here (every other method is 405 in src/routes.ts), so the cap covers the whole surface. No new dependency, no dead code, nothing outside what #39 asked.

One thing I found that is not yours to fix, filed as #90. After any 413, the caller’s next POST through the forge’s proxy comes back 502 or hangs for ~30 s (GET / keeps answering 200; three normal POSTs in a row are all 303). It reproduces identically on production, so main has it too and this pull neither causes nor worsens it for the browser case. The cause looks like the early declared-length refusal: the 413 goes out while the client is still uploading and Bun then closes with the body unread, which a raw-socket probe shows directly. Since this pull already has a bounded drain, the fix probably belongs next to it — drain up to MAX_REQUEST_DRAIN_BYTES before refusing, and only cut off a declared length beyond that.

Last nit, description only: the pull body still says readBody(request, maxBytes) and that it “keeps draining the stream to its actual end”. The code takes three arguments and drains a bounded amount. Worth a one-line edit before this lands, since the description is what stays in history.

Approved. The fix is real and minimal, and it holds up under a live beating. Anchoring here because line 24 — abandoning the read at drainLimit rather than draining to the stream's end — is the one line whose failure mode (unread bytes poisoning a kept-alive connection, which the description says an earlier reader.cancel() hit) I most wanted to disprove, and it holds.

What I verified, reconstructing this head on top of current main (the merge is clean — src/server.ts and src/limits.ts on main were untouched since the base):

• bun test on the merged tree: 133 pass, 0 fail. Reverting src/server.ts and deleting src/body.ts makes exactly the new test fail ("refuses a chunked body over the cap even with no content-length to check against"), so the added behaviour is genuinely covered.
• Raw-socket probes against a locally run server: a chunked POST of 1 MB and of 20 MB each answer 413 with the security floor intact, RSS stays flat, and a second request on the same kept-alive connection comes back a clean 200, not garbage — abandoning the read at line 24 does not poison connection reuse. 50 oversized chunked POSTs in a row leave the server answering / in 8 ms.
• Identity survives the rebuilt new Request(request, { body }) on both doors: a signed POST creates and claims a board and it lists as mine on /, and POST /mcp tools/call list_boards returns that board under the same identity. A cross-site POST is still 403, now refused before a byte is read.
• Preview: / answers in ~190 ms with the full floor (CSP, nosniff, referrer-policy, HSTS), POST /boards creates a board (303 -> /b/q8bgng6c), a 200 KB declared body is refused 413, and both screenshots look normal.
• Header edge cases are safe by construction: content-length: abc, a duplicated header, -1 all yield NaN or an under-cap value and fall through to the byte counter, which is the truth of record. Only POST carries a body here — every other method is 405 in src/routes.ts — so the cap covers the whole surface. No new dependency, no dead code, nothing beyond what #39 asked.

Nits for later, none blocking and none worth another round:

1. drainLimit = maxBytes * 4 is a magic number inside src/body.ts, while src/limits.ts says of itself that it holds every limit between the app and the disk, "named constants only". A MAX_REQUEST_DRAIN_BYTES there would put it where the next reader looks.
2. There is no src/body.test.ts, in a repo where bus, limits, who and session each have one. The drain bound is this module's one subtle behaviour and is the part not pinned by a test: a unit test on readBody with a counting ReadableStream far past the bound would be a few lines.
3. The description says the stream is "drained to its actual end rather than cancelling it"; the code drains a bounded amount and leaves the rest unread. The code and its comment are right — it is the description that is stale, and that is what lands in history.

The second new test ("a declared Content-Length over the cap is refused without waiting for the body to arrive") passes both before and after by design; it is fine as a guard on the cheap path that was deliberately kept.

The byte-counting cap itself is right, and I verified it end to end: bun test is green at c42e807 (114 pass), the new chunked test genuinely fails without the fix (I reverted src/server.ts to main's version in a scratch worktree and it returns 200 instead of 413), the preview answers 413 for both a chunked and a declared 200 KB body and keeps serving afterwards, the cap boundary is exact (64 KiB accepted, one byte more refused), and rebuilding the Request with the buffered bytes preserves method, the signed forge identity headers and the form body — a chunked, no-content-length POST /b/:id/columns with signed headers still returns 303 and still shows up on the board's event stream. POST /mcp is unaffected. No new dependency, no dead code.

Two things, first one blocking.

1. Dropping the content-length fast path is a real DoS regression (fix this first). The header is a claim, but a claim over the cap is still a refusal you can make for free, and this change gives that up: every POST body is now read to its end before the 413 goes out. Measured on a raw socket, declaring content-length: 33554432 and actually writing it:

main:   413 after the client had written 2,686,976 bytes (answered on the headers, rest was in flight)
this:   413 after the client had written 33,554,432 bytes — all of it read and discarded

A 64 MiB chunked body is drained in full too (112 ms locally). Memory is bounded, which is what #39 asked for, but an anonymous caller can now make the process read as many bytes as it is willing to send, per connection, without limit — and that read happens before any identity or rate-limit check. The fix is additive, not a revert: keep

const declared = Number(request.headers.get("content-length") ?? "0");
if (declared > MAX_REQUEST_BODY_BYTES) return withFloor(new Response("request body too large", { status: 413 }));

as a cheap pre-check before readBody, and let readBody remain the truth for the chunked and the lying cases. Then bound the drain in src/body.ts: stop after some fixed slack past the cap (a few times maxBytes) rather than looping to the stream's end, so overCap can't be turned into unbounded reading.

On the reason given for draining rather than cancelling: I could not reproduce that it buys connection reuse. With a raw socket that waits for the full response and then sends GET / on the same connection, the control (a normal 303 POST, then GET) gets both responses; after a 413 the follow-up GET goes unanswered — identically on main and on this branch, drained or not. The same is true of the 403 path, which this change moves ahead of the read and so never drains at all. So the connection is already left unusable after a refusal, both before and after this change; that is pre-existing and not yours to fix here, but it means the unbounded drain is paying for something it does not deliver. Cancelling produced a mangled next response for you, which is worse than silence — a bounded drain is the middle ground.

2. Two comments now say the opposite of what the code does. README.md (Limits): "Request bodies over 64KB are refused before they're read" — they are now read and counted. Same sentence in the MAX_REQUEST_BODY_BYTES comment in src/limits.ts: "refused before it is read". Both should say the bytes are counted as they arrive, and (once fixed) that a declared length over the cap is still refused on the header.

Nothing else: the security floor, same-origin refusal, escaping and the terminal are untouched by this change and don't need to move for it.