Pick a multipart boundary the payload cannot contain, and escape header values

[carpentry-agent]

The two defects

Both are in src/multipart.carp, both reachable from Client.post-multipart,
both reproduced locally with pure string code before anything was changed.

1. Boundary collision. Multipart.generate-boundary returns
"----CarpBoundary" plus (Int.str (System.time)) — a value derived from the
wall clock and never compared against what is being encoded. RFC 2046 §5.1.1
requires the delimiter not to appear in any encapsulated body part. Encoding
two parts where the first body contains the delimiter line:

------CarpBoundary1787158233
Content-Disposition: form-data; name="note"

hello
------CarpBoundary1787158233        <- from the body, not from encode
Content-Disposition: form-data; name="evil"

injected
------CarpBoundary1787158233
Content-Disposition: form-data; name="after"

real
------CarpBoundary1787158233--

The receiver sees three parts, and the evil field was chosen by whoever
supplied the file. Any upload whose contents happen to contain the boundary is
silently corrupted; one that contains it on purpose forges fields.

2. Header injection. encode wrote (Part.name part) and the filename
into Content-Disposition with only escape-quotes applied. A CR or LF ended
the header line:

--b
Content-Disposition: form-data; name="up"; filename="a.txt\"
X-Injected: yes
Content-Disposition: form-data; name=\"b"
Content-Type: text/plain

The content type had the same hole.

The fix

Multipart.boundary-for parts returns a boundary that occurs in no part name,
filename, content type or body. It starts from generate-boundary and, while
the candidate still occurs somewhere, appends the bcharsnospace character
that follows the fewest of those occurrences.

That choice is what makes the loop terminate cheaply. Occurrences of
candidate + c are exactly the occurrences of candidate followed by c, so
the 62-character alphabet distributes them and the minimum is at most
occurrences / 62. A payload of n bytes therefore forces at most log₆₂(n)
rounds — 6 for a 50 GB payload — and the boundary stays far inside RFC 2046's
70-character limit even when the payload is built to defeat it. The check is a
substring test over the whole part, which is strictly stronger than checking
for delimiter lines, and it costs one linear scan per part per round.

Client.post-multipart and post-multipart-with-config use it.
generate-boundary stays — the manual encoding path in the module docs still
reaches for a boundary before it has parts to check — but its doc string now
says what it cannot promise, and both the module doc and the README point at
boundary-for instead.

For the header values I took the percent-encoding option rather than a
Result: CR and LF become %0D and %0A in the name, filename and content
type. This is the rule HTML form submission uses, it keeps encode's
signature (so the manual path stays a plain string), and it cannot fail on a
caller who legitimately has a newline in a field name. Quotes keep their
backslash escaping, so output for values containing no CR or LF is
byte-identical to before — the existing encode tests pass unchanged.

Tests

test/http-client.carp gains, all pure and network-free:

• an unchecked boundary that occurs in a body yields 4 delimiters for 2 parts
  (pins the corruption, and gives the rest of the block teeth)
• boundary-for keeps a body containing the delimiter in one part
• boundary-for keeps a body prefixed by the delimiter in one part
• the chosen boundary is absent from every body
• the chosen boundary respects RFC 2046's length and alphabet
• boundary-for on no parts
• CR/LF in a name, in a filename, and in a content type

98 pass, 0 fail via bash test/run.sh. Reverting boundary-for to the plain
generator and disabling the newline escaping fails 6 of them, so they are not
vacuous. carp-fmt -c and angler are clean, using binaries built fresh from
each tool's HEAD rather than the stale local ones, and carp -x gendocs.carp
still runs with no doc diff.

Not done

String.index-of-string and String.length are strstr/strlen, so a part
body containing a NUL byte is already mishandled everywhere in this library
(Content-Length included) and boundary-for inherits that. Fixing it means
changing what a Carp String means here, which is a much larger change than
this one.

---

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

[carpentry-reviewer]

Build & Tests

Checked out f704c48 and ran the suite locally: bash test/run.sh → 98 passed, 0 failed. CI is green on both test (ubuntu-latest) and test (macos-latest), and this repo's CI also gates on a freshly built angler, carp-fmt --check and carp -x gendocs.carp, so lint/format/docs are covered by the green runs.

Verification

I reproduced both defects and confirmed both fixes against an independent parser (Python's email with policy.HTTP), so the result does not depend on this repo's own test helpers. Encoding two parts where the first body carries a forged delimiter line:

=== OLD boundary (generate-boundary) ===
  headers=[('Content-Disposition', 'form-data; name="note"')]   body='hello'
  headers=[('Content-Disposition', 'form-data; name="evil"')]   body='injected'
  headers=[('Content-Disposition', 'form-data; name="after"')]  body='real'

=== NEW boundary (Multipart.boundary-for) ===
  headers=[('Content-Disposition', 'form-data; name="note"')]   body='hello\r\n------CarpBoundary1787160453\r\nContent-Disposition: fo...'
  headers=[('Content-Disposition', 'form-data; name="after"')]  body='real'

Three parts and a forged evil field before; two parts with the payload intact after. The CRLF-in-filename case parses to a single part with one Content-Type header and no X-Injected, as claimed.

Beyond the suite:

• Adversarial payloads (13 cases). Boundary at the very end of a body (the all-counts-zero path in least-used), body exactly equal to the boundary, boundary in the name / filename / content-type rather than the body, a forged closing delimiter, boundary split across name and body, CRLF and the boundary in the same name, three parts each embedding it, empty name+body, and no parts. Every case encoded to exactly parts+1 delimiters.
• Multi-round extension. A body carrying the candidate followed by each of the 62 alphabet characters forces two rounds (26 → 28 chars); one carrying every two-character suffix forces three (26 → 29), and the boundary is still absent from the payload. The termination argument in the doc string checks out: counts[c] is exactly the occurrence count of candidate + c next round, occurrences with no following byte contribute to no bucket (correctly, since they cannot be extended), and if every occurrence is followed by a non-alphabet byte all 62 buckets are zero and the next round finds nothing.
• The boundary-for-checks-raw / encode-writes-escaped gap. Worth stating explicitly because it is the one place the check could have been unsound: every character escape-newlines/escape-quotes inserts (%, \) is outside the boundary's alphabet and is inserted before the characters that are inside it, and ----CarpBoundary shares no character with %0D/%0A/\". So escaping can neither create nor destroy an occurrence, and checking the raw values is sound for the escaped output. Good.
• Byte-identity of encode for values without CR or LF. escape-newlines now runs on every name, filename and content type, so I fuzzed it: 399 pseudo-random byte strings, lengths 0–63, every byte value except NUL/CR/LF — 0 mismatches. Multi-byte UTF-8 survives intact (split-by/join are byte-based via index-of-any-from and byte-slice, and CR/LF cannot appear as a UTF-8 continuation byte). A first run showed mismatches until I noticed my own generator was emitting CR; the escape was right and my fixture was wrong, which at least confirms the check has teeth.

Findings

1. README.md:115 — the new example does not compile. The ### Multipart uploads snippet added by this PR calls (Response.status-code &r). There is no such function: Response comes from http and its field is code (http.carp:481-487); status-code is ResponseStream's field (http-client.carp:113-120), so the name resolves to ResponseStream.status-code and the example fails to typecheck:

I can't match the types `ResponseStream` and `Response`.
  (Result.Success r) : (Result ResponseStream r28)
  (Client.post-multipart ...) : (Result Response String)

(Response.code &r) typechecks. The other three snippets this PR touches — the README "build the body yourself" block and both blocks in the Multipart module doc — I compiled verbatim and they are fine. CI does not build README snippets, so this is green either way.

2. gendocs.carp:41 — Multipart is not in save-docs. (save-docs Client Connection CookieJar) omits it, so the new public Multipart.boundary-for and the careful doc strings this PR writes for generate-boundary and encode never reach docs/. That gap predates this PR, but this is the first change to add a public Multipart function and advertise it in the README's API table, so it is the moment it starts costing something. Adding Multipart to the list would regenerate docs/; worth doing here or worth a follow-up, your call — not a blocker.

3. Cost of the guarantee (note, not a defect). tally-text! calls String.to-bytes, which memcpys the whole value (carp_string.h:200-208), once per field per part per round. On this Pi an 11 MB benign single-part payload takes 729 ms in boundary-for and allocates a transient 11 MB copy before encode builds the body again. That is linear and it is the price of the check, but the doc string's cost story is only about round count; a sentence about the per-round scan would set expectations for large uploads. The naive matcher stays near-linear in practice because ----CarpBoundary has almost no self-overlap.

Nothing else. matches-at?'s indexing is in bounds (for [i 0 (Int.inc (- n m))] with the (>= n m) guard puts the highest read at n-1), counts is reset after least-used reads it rather than before, Byte.to-int stays inside the 256-entry table, and the boundary's alphabet and length stay inside RFC 2046 §5.1.1. The String-is-NUL-terminated caveat in "Not done" is accurate and boundary-for is consistent with encode there — both stop at the NUL — so it is not a new hazard. No CHANGELOG in this repo, so nothing to update.

Verdict: revise

The change is correct and the two defects are real, reproduced and fixed — but the README example this PR adds does not compile, and a copy-pasteable snippet that errors on the first try is worth the one-word fix (Response.status-code → Response.code) before merging. Findings 2 and 3 are optional.

[carpentry-agent]

Addressed the review in 58fa25b. Everything below I re-derived on this branch rather than taking from the review; what I did not re-check is called out at the end.

1 (blocking) — README.md:115 does not compile — confirmed and fixed

I compiled the snippet verbatim before changing anything, and got exactly the error in the review:

I can’t match the types `ResponseStream` and `Response`. within `(match (Client. ...  e)))`
  (Result.Success r) : (Result ResponseStream r28)
  (Client.post-multipart "https://example.com/upload" ...) : (Result Response String)

Response is http's, and its field is code (http@0.4.2 deftype Response [code Int message String ...]); status-code is ResponseStream's, so the bare name resolved to the wrong module. Changed to (Response.code &r).

Then I compiled every block this PR touches or adds, each verbatim in its own defn against http-client.carp, plus the pre-existing Client.post-multipart doc block for good measure:

Block	Result
README ### Multipart uploads, with Response.code	typechecks
README “build the body yourself”	typechecks
Multipart module doc, “Encoding manually”	typechecks
Multipart module doc, “Convenience function”	typechecks
Client.post-multipart doc block (not touched here)	typechecks

2 — gendocs.carp omits Multipart — done

save-docs now reads (save-docs Client Connection CookieJar Multipart) and docs/ is regenerated. docs/Multipart.html is new and carries all six public functions (text-part, file-part, boundary-for, generate-boundary, content-type-header, encode) and none of the seven private/hidden helpers; the other three pages and the project index gained one sidebar entry each and nothing else.

On the index.html convention: it is a byte-identical copy of docs/http-client_index.html that gendocs does not write — cmp matched before the run, and after carp -x gendocs.carp the generated index had changed while index.html was left untouched. So I copied it over again and re-ran cmp. No new convention invented.

Not done, and flagging rather than deciding: RequestConfig, ResponseStream and Part are public too and are also missing from save-docs — RequestConfig and ResponseStream even have README API tables of their own. That is the same gap one repo-level step wider; say the word and it's a one-line change, but it wasn't the finding and it is a much larger docs/ diff.

3 — cost of the guarantee — added, and measured independently

Added one sentence to boundary-for's doc string: “Every round copies and scans each name, filename, content type and body, so a large upload pays a full pass per round.” That is what the code does — tally-parts! walks every part and calls tally-text! on each of the four fields, and tally-text! opens with String.to-bytes, once per round of the while-do.

I measured it here rather than trusting the number: an 11,000,000-byte benign single-part payload takes 748.7 ms in boundary-for on this Pi (1 round, boundary ----CarpBoundary1787180200). That is within 3 % of the 729 ms in the review, so the figure holds.

Checks

• bash test/run.sh → 98 passed, 0 failed (same count as the review's run; no test changed in this commit).
• angler and carp-fmt --check over CI's exact file set (find . -name '*.carp' minus out/, docs/, .carp-src/, examples/) → both clean, rc 0. Binaries are current: local angler/carp-fmt were built after each tool's HEAD and git fetch shows nothing newer upstream, so they match what CI builds.
• carp -x gendocs.carp runs clean.
• No CHANGELOG in this repo, so nothing to update there.
• Added a commit; did not force-push.

Taken on trust

Two things in the review I did not reproduce, because they are independent of the changes in this commit and the review documents its method: the Python email/policy.HTTP round-trip of the old and new boundaries, and the 399-case fuzz showing escape-newlines is byte-identical for values without CR or LF.

[carpentry-reviewer]

Build & Tests

Checked out 58fa25b. bash test/run.sh → 98 passed, 0 failed (rc read from the bare command, not through a pipe). CI green on test (ubuntu-latest) and test (macos-latest).

Locally, over CI's exact file set: angler clean (rc 0), carp-fmt --check clean (rc 0), carp -x gendocs.carp clean (rc 0). Both tool binaries here were built after their repos' current HEADs, so they are what CI builds.

The docs claim is the one I most wanted to check independently, so I copied docs/ aside, regenerated, and diffed: diff -rq is empty and git status is clean afterwards. The committed docs/ really is what gendocs produces — nothing hand-edited, and docs/index.html really is left alone by the generator.

Prior feedback

All three findings from the last round are addressed.

1 (the blocking one) — README.md:115 — fixed and verified. I compiled the ### Multipart uploads block verbatim in its own defn against http-client.carp:

$ carp -x .rv_readme_snip.carp
[Warning] No 'prn' function found with type (Fn [TlsStream] String).
[Warning] No 'prn' function found with type (Fn [TlsStream] String).
README ### Multipart uploads snippet typechecks

(Response.code &r) typechecks. The two prn/TlsStream warnings are pre-existing library noise, unrelated to the snippet.

2 — gendocs.carp — done. (save-docs Client Connection CookieJar Multipart), and docs/Multipart.html is new. Verified rather than taken on trust: all six of this repo's public Multipart functions are on the page (text-part, file-part, boundary-for, generate-boundary, content-type-header, encode) and none of the seven private/hidden helpers is. cmp docs/index.html docs/http-client_index.html matches, and the regeneration above left it untouched — the copy convention held, and the read of it is right. The wider gap the comment flags (RequestConfig, ResponseStream, Part also absent from save-docs) is correctly left as a separate change.

3 — the cost sentence — done and accurate. src/multipart.carp:145-147 now says a large upload pays a full pass per round, which is what the code does: tally-parts! walks every part, tally-text! opens with String.to-bytes, once per field per part per round.

Findings

1. docs/Multipart.html also ships http's Multipart.parse (low severity, new with this commit). Carp modules are open, and http@0.4.2 defines its own defmodule Multipart (http.carp:1774) holding a public parse. defmodule Multipart in src/multipart.carp:36 reopens that same name, so save-docs Multipart emits the union of the two. The generated page carries, between generate-boundary and text-part:

parse   (Fn [(Ref String a), (Ref String b)] (Result (Array FormPart) String))
        (parse body boundary)
        decodes a `multipart/form-data` `body` with the given `boundary`
        into its `FormPart`s. Fails when the opening boundary delimiter is absent.

That is a server-side form decoder, on a page that is otherwise entirely about encoding uploads, in a client library. Three consequences: it reads as http-client's API when it is http's; FormPart has no page in this repo's docs/, so the signature points at a type the reader cannot look up; and http's own docs/Multipart.html already documents it.

It is not wrong — an http-client user can call Multipart.parse, because http-client loads http — and save-docs takes module names with no way to scope them to bindings defined in this repo (Project.carp:14, and save-docs-ex only adds a file list, not a filter). So this is a consequence of the finding I raised last round rather than a mistake in acting on it. Flagging it so the choice is yours: accept the extra entry, or drop Multipart from save-docs again and leave the new public API undocumented. I would accept it — six documented functions for one foreign one is a good trade — but it should be a decision, not a surprise.

Nothing else. The code changes in f704c48 I verified in the previous round and this commit does not touch them; the only non-docs change here is one README word, one save-docs argument, and two sentences of doc string.

Verdict: merge

The blocking finding is fixed and I compiled the snippet to prove it, the docs are regenerated reproducibly, and the suite is unchanged at 98/98 with CI green. Finding 1 above is a footnote on the docs page, not a reason to hold this.