Read the chunk-size line strictly as 1*HEXDIG - #42
Conversation
dechunk trimmed the size line before parsing it, so it decoded four framings
RFC 9112 §7.1 has no production for: " 5", "5 ", "\t5" and " 0 " all decoded
on main. §11.2 names that leniency as the request-smuggling primitive, and
`web` decodes untrusted client request bodies through this function
(web-decode-body answers 400 on an Error), so the server accepted framing a
conformant proxy in front of it would reject or split differently.
parse-size-line now takes the size verbatim: the maximal HEXDIG run at the
head of the line, which must be non-empty, and after it the line must either
end or continue into a chunk extension. The BWS that §7.1.1 allows before the
`;` is still accepted, since the grammar has it; trailing whitespace with no
extension behind it is not. A stray CR before the terminating CRLF ("5\r\r\n")
is rejected for the same reason, where the trim used to swallow it. Uppercase
hex, leading zeros, extensions, trailer sections and multi-chunk bodies are
unchanged, and the error message now quotes the whole offending line.
Five of the six new rejection assertions fail on main; the sixth (a bare
";name=value" with no digits before it) pins behaviour parse-hex already had.
488 passed / 0 failed.
There was a problem hiding this comment.
Build & Tests
carp -x test/http.carp — 488 passed, 0 failed, matching the PR. CI green on both legs. carp -x gendocs.carp regenerates docs/TransferEncoding.html byte-for-byte.
Findings
I ran a differential of TransferEncoding.dechunk between origin/master and 0e8bffb: 63 hand-built size-line shapes plus 2,500 grammar-aware fuzz cases (hex digits decorated with random BWS, stray bytes and chunk extensions, with the declared size always correct so the size line is the only variable).
The tightening is clean. Across all 2,563 inputs:
- 0 cases went master-
ERRto branch-OK— no new leniency anywhere; - 0 cases where both accept but the decoded body differs;
- 343 in the fuzz set and 15 in the hand-built set went master-
OKto branch-ERR, and every one of them has SP, HTAB, CR, VT or FF in the size line. Nothing outside that class flipped.
The BWS-before-; allowance survives (39 fuzz cases matching HEXDIG+ [SP/HTAB]+ ; still decode) and trailing BWS with no extension is rejected, so parse-size-line (http.carp:671) does what §7.1.1 says and no more.
Test teeth confirmed without reverting anything. My master run decodes 5, HTAB + 5, 5 + SP, 5 + CR and 0, and errors on ;name=value — exactly the 5-of-6 split the PR claims.
One thing the description undersells
The PR says "One shape is newly rejected that wasn't in the original list: a stray CR". There are four more, all the same String.trim-whitespace class, none of them listed:
| size line | master |
0e8bffb |
|---|---|---|
5 + HTAB |
OK <hello> |
rejected |
CR + 5 |
OK <hello> |
rejected |
5 + VT (0x0B) |
OK <hello> |
rejected |
5 + FF (0x0C) |
OK <hello> |
rejected |
All four are correct to reject — String.trim counts them as whitespace, 1*HEXDIG does not. But this PR's shape is "here is each case, decided against the ABNF", and these are the cases where the change is wider than that table says. Worth folding into the list rather than leaving a reader to find them.
Not a bounds problem, in case it reads like one
parse-size-line mixes String.length (which is strlen, so bytes) with String.char-at, which looks like the byte-versus-character trap that bites elsewhere in core String. It isn't: String_char_MINUS_at is (uint8_t)(*s)[i], byte-indexed, so the two agree. All three loops are guarded by (< _ len) and byte-slice is only ever called with digits <= len. A high-byte size line is rejected rather than mis-indexed — confirmed in the sweep, no crashes across 2,563 inputs.
Blast radius is as stated
The only dechunk consumer in the org is web/web.carp:2053, and web.carp:37 pins carpentry-org/http@0.4.2, so nothing turns red until web bumps. Confirmed by sweeping every clone.
Still accepted, unchanged, not introduced here
A bare 5; with no extension name decodes, though chunk-ext-name = token wants at least one character. Pre-existing, harmless while extensions are ignored — noting it only so it isn't mistaken for something this PR tightened.
Merge ordering
This is the strict half of the pair. carpentry-org/http-client#22 ports dechunk with its String.trim, so merging both as they stand leaves the client accepting framings this rejects — I confirmed that on #22's branch directly and left the detail there, including a fifth divergent shape beyond the four in your comment. Nothing to change on this side; this is the one that's right.
Verdict: merge
A pure tightening, verified as such rather than assumed: no input became more permissive, no decoded body changed, and every newly-rejected shape is whitespace the ABNF has no production for.
TransferEncoding.dechunktrimmed the chunk-size line before parsing it, so itdecoded framings RFC 9112 §7.1 has no production for. All four of these decode
on
mastertoday and none of them should:master" 5\r\nhello\r\n0\r\n\r\n"OK <hello>"5 \r\nhello\r\n0\r\n\r\n"OK <hello>"\t5\r\nhello\r\n0\r\n\r\n"OK <hello>" 0 \r\n\r\n"OK <>chunk-size = 1*HEXDIG— no surrounding whitespace — and §11.2 names lenientchunk framing as the request-smuggling primitive. This matters beyond
conformance because
webis an HTTP server that decodes untrusted clientrequest bodies through exactly this function:
web-decode-bodycallsTransferEncoding.dechunk (Request.body &req)and answers 400 on anError.A conformant proxy in front of the Carp server would reject or re-frame input
the server itself accepts, which is the smuggling shape.
What changed
parse-size-linereplaces the trim-then-parse step. It takes the maximalHEXDIG run at the head of the line, requires it to be non-empty, and then
requires the rest of the line to be either empty or the start of a chunk
extension.
parse-hexand its overflow guard are untouched; the error messagenow quotes the whole offending line rather than the trimmed fragment.
Each case on the size line, decided against the ABNF:
chunk-size.chunk-extis*( BWS ";" … ), so with zero repetitions there is no BWS to spend.;— accepted. RFC 9112 §7.1.1 putsBWSthere explicitly (this is one of the places 9112 differs from 7230'schunk-ext), so rejecting it would be stricter than the grammar. Pinned by a
test so the choice is deliberate rather than incidental.
;extwith no digits — rejected. This wasalready true (
parse-hexrejects an empty string); confirmed and pinned.\nas the line terminator — already rejected, unchanged:String.find-crlfonly ever splits on CRLF, so a lone LF stays inside thesize line and fails the HEXDIG scan.
One shape is newly rejected that wasn't in the original list: a stray CR before
the terminating CRLF (
"5\r\r\n…").String.trimtreated that CR aswhitespace and dropped it; the verbatim scan does not.
The docstring cited RFC 7230 §4.1 while claiming a conformance the code did not
have. It now cites RFC 9112 §7.1 (which is where the BWS allowance lives) and
says the size line is read strictly.
docs/TransferEncoding.htmlisregenerated to match.
Tests
Eight assertions in
test/http.carp. Five of the six new rejectionassertions fail on
master— verified by revertinghttp.carpalone andre-running, not by assuming:
The sixth (
";name=value"with no digits before it) passes onmastertoo —it pins behaviour
parse-hexalready had rather than a new rejection. The twoacceptance assertions cover the BWS-before-
;case and an uppercase hex size;chunk extensions, multi-chunk bodies, trailer sections and the empty body were
already covered and still pass.
488 passed / 0 failed.
angler(built from itsmainHEAD,185a9a2) andcarp-fmt --checkare clean on both changed files, andcarp -x gendocs.carpregenerates without error.
Blast radius
This is a deliberate tightening, not a bug fix that restores an intended
behaviour: input that used to decode is now an
Error, and inwebthatbecomes a 400.
webpins a releasedhttp, so nothing goes red today — ittakes effect only when
webpicks the change up. The diff is confined to thesize-line parse and its tests.
Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.