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

Pushed.

The POST body-size guard previously trusted the declared content-length header; a chunked request (no content-length at all) fell through and could stream unbounded bytes into memory before any handler's own field limits kicked in.

src/body.ts adds readBody(request, maxBytes), which reads the request's body stream chunk by chunk, counting real bytes as they arrive. Once the running total crosses the cap, it stops retaining chunks (bounding memory) but keeps draining the stream to its actual end rather than cancelling it — cancelling mid-stream on a kept-alive connection leaves unread bytes on the socket that get misread as the start of the next request, which is what an early reader.cancel() attempt in this fix demonstrated (a follow-up request came back mangled). src/server.ts now calls readBody for every POST, returns 413 if it comes back oversized, and otherwise rebuilds the Request with the buffered bytes for the router and handlers downstream (request.formData()/request.json()) to consume normally. The same-origin check now runs before the body is read at all, so a cross-site POST doesn't cost us reading its body first.

Added a regression test in src/server.test.ts that POSTs a ReadableStream body (no content-length, sent chunked) larger than MAX_REQUEST_BODY_BYTES and asserts a 413, plus that the server answers normally afterward — the only case previously untested was the truthful-header one.

bun test passes (114 tests, run repeatedly to rule out timing flakiness from the streaming test).

Closes #39

probe

Housekeeping: my review is the line comment on src/server.ts:67 and its verdict is changes requested — the forge stores a review as an anchored comment, so the verdict lives in that body rather than in the pull's review counters. The one-word probe comment above was an accidental API probe of mine and has no meaning; there is no delete route for it, so please ignore it.

Changes requested on #60, posted as the review anchored to src/server.ts:67.

The byte-counting cap is correct and I verified it end to end: bun test green at c42e807 (114 pass), the new chunked test fails without the fix (reverting src/server.ts to main's version in a scratch worktree gives 200 instead of 413), the preview returns 413 for both a chunked and a declared 200 KB body and keeps serving, the cap boundary is exact (64 KiB in, one byte more refused), and rebuilding the Request preserves method, signed identity headers and the form body — a chunked signed POST /b/:id/columns still returns 303 and still emits on the board's event stream; POST /mcp unaffected.

Blocking finding: dropping the content-length pre-check turns a free refusal into a full read. Measured on a raw socket declaring 32 MiB, main answers 413 after ~2.7 MB was written, this branch answers only after all 33,554,432 bytes have been written and read; a 64 MiB chunked body is drained in full too, before any identity or rate-limit check. The fix is additive — keep the header check as a fast path and let readBody remain the truth — plus bound the drain in src/body.ts. I also could not reproduce the drain's stated benefit: after a 413 (drained) and after a 403 (never read) a follow-up request on the same connection goes unanswered, identically on main and on the branch, while the control (normal 303 then GET) is answered. Second, non-blocking: the README "Limits" bullet and the MAX_REQUEST_BODY_BYTES comment still say bodies are "refused before they're read", which is now the opposite of the code.

Two housekeeping notes: the probe comment on the pull is a stray API probe of mine (no delete route exists), and this forge records a review as an anchored comment, so the pull's review counters stay 0 — I left a short conversation comment saying both, and the verdict lives in the review body.

Pushed both requested fixes to #60.

Blocking finding: restored the cheap content-length pre-check in src/server.ts ahead of readBody, so a declared length over 64KB is refused before a single byte is read again — readBody still remains the truth for chunked and lying requests. Added a raw-socket test (src/server.test.ts) that opens a connection, declares an oversized content-length, and never writes the body; it times out without the fix and passes with it, since fetch() can't send a lying Content-Length (it's a forbidden header there).

src/body.ts now bounds the drain to 4 × maxBytes instead of reading to the stream's end, so a chunked or lying-header body over the cap stops early rather than being fully consumed — closing the DoS gap the reviewer measured (33 MB/64 MB read in full) while keeping the "read, don't cancel" behavior that avoids a mangled subsequent response.

Non-blocking: corrected the stale "refused before it's read" comments in README.md and src/limits.ts to describe both the header fast-path and the counted-as-they-arrive path.

I didn't pursue a bounded cancel of the underlying stream (e.g. reader.cancel()) since the reviewer's own testing showed a refused connection is already unusable for reuse either way, drained or not — bounding the drain length gets the DoS protection without reintroducing the mangled-response bug from cancelling.

bun test passes (115 pass, 0 fail).

Merged main into this branch. The only real conflict was the destructured import list in src/server.test.ts — combined MAX_REQUEST_BODY_BYTES (this branch) with MAX_SUBSCRIBERS_PER_BOARD/boardCreationBucket (main's rate-limit-reset work) into one import. Everything else auto-merged cleanly, including main's new SSE-aware HEAD handling in src/server.ts sitting alongside this branch's Content-Length fast-path and bounded drain. bun test passes (133 pass, 0 fail); pushed as fd28509.

Review submitted on pull #60 as an approval, anchored to src/body.ts:24 (the drainLimit break), commit fd28509.

What I checked, in the merged state (this head applies cleanly on current main — src/server.ts and src/limits.ts were untouched there since the base):

• bun test: 133 pass, 0 fail. With src/server.ts reverted and src/body.ts removed, exactly the new chunked-body test fails, so the added behaviour is really covered.
• Raw-socket probes against a locally run server: chunked POSTs of 1 MB and 20 MB both answer 413 with the security floor intact, RSS flat, and the next request on the same kept-alive connection is a clean 200 — abandoning the read at drainLimit does not poison connection reuse, which was the failure mode the pull description says an earlier reader.cancel() hit. 50 oversized POSTs in a row leave / answering in 8 ms.
• Identity survives the rebuilt new Request(request, { body }) on both doors: a signed create + claim lists the board as mine, POST /mcp tools/call list_boards returns it under the same identity, cross-site POST is still 403 and now refused before any byte is read.
• Preview: / in ~190 ms with CSP/nosniff/referrer-policy/HSTS, board creation works (303 → /b/q8bgng6c), a 200 KB declared body is 413, screenshots normal.

Nits recorded in the review, none blocking: drainLimit = maxBytes * 4 belongs in src/limits.ts; there is no src/body.test.ts pinning the drain bound; and the pull description says the stream is drained to its end while the code (correctly) drains a bounded amount.

One API note: POST …/pulls/60/reviews rejects a state-plus-body payload with 422 bad_anchor — it only accepts comments anchored to path/line/side/commitOid, and the state field is not reflected in the pull's review counters. My verdict is therefore the first word of the anchored comment. My clone is unchanged; nothing was pushed.

Addressed both non-blocking nits from the approval on #60:

• MAX_REQUEST_DRAIN_BYTES (4 × MAX_REQUEST_BODY_BYTES) now lives in src/limits.ts next to the other named limits, instead of as a magic multiplier inside readBody; readBody takes it as an explicit parameter.
• Added src/body.test.ts, pinning the one behavior that wasn't under test: a body far larger than the cap stops being read at the drain bound rather than the stream's end (asserted by counting pulls on an effectively unbounded stream), plus the at-cap, over-cap, and empty-body cases.

On the third nit: I agree the description in my earlier message was stale ("drained to its actual end") — the code and its comment were already correct (bounded drain), that was just a wording slip on my part in the write-up, not in the repo, so there was nothing to fix in code.

bun test: 137 pass, 0 fail.

Approved. One review posted on #60, anchored to src/body.ts:23 at head 2b301fc, plus issue #90 for the one problem I found that is not this pull's.

What I checked, with the head rebuilt over current main (b6cf7bd; merges clean):

• bun test: 151 pass, 0 fail across 14 files. The new drain-bound test is load-bearing — deleting only the if (total >= drainBytes) break; line makes exactly that test fail while the other three pass. The chunked server test still fails if src/server.ts is reverted.
• Both round-1 nits are fixed: MAX_REQUEST_DRAIN_BYTES moved into src/limits.ts and is passed in, and src/body.test.ts now exists.
• Raw-socket probes locally: a 4 MB chunked POST answers 413 with the floor intact, and the next request on the same kept-alive connection is a clean 200. Signed identity survives the rebuilt Request on the page and at POST /mcp; cross-site POST is 403 before a byte is read.
• Preview: / in ~130 ms, three back-to-back board creations all 303, a 200 KB body refused 413, screenshots unchanged.

The problem I found, filed as #90 rather than blocking: after any 413, the caller's next POST through the forge's proxy returns 502 or hangs ~30 s, while GET / stays healthy. It reproduces identically on production, so it is pre-existing on main — the early declared-length refusal answers while the client is still uploading and the connection then closes with the body unread. The issue carries the curl repro, the raw-socket evidence, and two candidate fixes.

Remaining nit, in the review: the pull description still describes readBody(request, maxBytes) draining "to its actual end", which the code no longer does. My clone is unchanged and nothing was pushed.