Skip to content

Read the chunk-size line strictly as 1*HEXDIG - #42

Merged
hellerve merged 1 commit into
masterfrom
claude/chunk-size-abnf
Aug 27, 2026
Merged

Read the chunk-size line strictly as 1*HEXDIG#42
hellerve merged 1 commit into
masterfrom
claude/chunk-size-abnf

Conversation

@carpentry-agent

Copy link
Copy Markdown

TransferEncoding.dechunk trimmed the chunk-size line before parsing it, so it
decoded framings RFC 9112 §7.1 has no production for. All four of these decode
on master today and none of them should:

body on 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 lenient
chunk framing as the request-smuggling primitive. This matters beyond
conformance because web is an HTTP server that decodes untrusted client
request bodies through exactly this function: web-decode-body calls
TransferEncoding.dechunk (Request.body &req) and answers 400 on an Error.
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-line replaces the trim-then-parse step. It takes the maximal
HEXDIG 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-hex and its overflow guard are untouched; the error message
now quotes the whole offending line rather than the trimmed fragment.

Each case on the size line, decided against the ABNF:

  • leading whitespace or tab — rejected; nothing precedes chunk-size.
  • trailing whitespace with no extension — rejected; chunk-ext is
    *( BWS ";" … ), so with zero repetitions there is no BWS to spend.
  • whitespace between the size and ;accepted. RFC 9112 §7.1.1 puts
    BWS there explicitly (this is one of the places 9112 differs from 7230's
    chunk-ext), so rejecting it would be stricter than the grammar. Pinned by a
    test so the choice is deliberate rather than incidental.
  • empty size line, or a bare ;ext with no digits — rejected. This was
    already true (parse-hex rejects an empty string); confirmed and pinned.
  • a bare \n as the line terminator — already rejected, unchanged:
    String.find-crlf only ever splits on CRLF, so a lone LF stays inside the
    size line and fails the HEXDIG scan.
  • uppercase hex — legal, still decodes.

One shape is newly rejected that wasn't in the original list: a stray CR before
the terminating CRLF ("5\r\r\n…"). String.trim treated that CR as
whitespace 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.html is
regenerated to match.

Tests

Eight assertions in test/http.carp. Five of the six new rejection
assertions fail on master
— verified by reverting http.carp alone and
re-running, not by assuming:

Test 'dechunk rejects a chunk size line with leading whitespace' failed
Test 'dechunk rejects a chunk size line starting with a tab' failed
Test 'dechunk rejects a chunk size with trailing whitespace and no extension' failed
Test 'dechunk rejects a stray CR between the chunk size and the CRLF' failed
Test 'dechunk rejects a whitespace-padded terminating chunk' failed
	Passed: 483	Failed: 5

The sixth (";name=value" with no digits before it) passes on master too —
it pins behaviour parse-hex already had rather than a new rejection. The two
acceptance 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 its main HEAD, 185a9a2) and
carp-fmt --check are clean on both changed files, and carp -x gendocs.carp
regenerates 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 in web that
becomes a 400. web pins a released http, so nothing goes red today — it
takes effect only when web picks the change up. The diff is confined to the
size-line parse and its tests.


Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

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.

@carpentry-reviewer carpentry-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build & Tests

carp -x test/http.carp488 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-ERR to 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-OK to 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.

@hellerve
hellerve merged commit 7943ba8 into master Aug 27, 2026
2 checks passed
@hellerve
hellerve deleted the claude/chunk-size-abnf branch August 27, 2026 00:05
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