fix: drop split CRLFCRLF terminator bytes from the header buffer

[spokodev]

When a part's \r\n\r\n header terminator is split across two push() calls as ...\r then \n\r\n, the parser leaves a stray \r on the last header value of that part.

Repro (public API)

const body = Buffer.from(
  '--BOUND\r\n' +
  'Content-Disposition: form-data; name="f"; filename="a.txt"\r\n' +
  'Content-Type: image/png\r\n\r\n' +
  'DATA\r\n--BOUND--\r\n', 'binary')

// split so the \r\n\r\n after image/png breaks as "...image/png\r" | "\n\r\n"
// whole input  -> file mimetype = "image/png"
// this split   -> file mimetype = "image/png\r"   (trailing 0x0D)

A direct HeaderParser repro: Content-Type: text/plain\r\n\r\n pushed as …text/plain\r then \n\r\n yields the value text/plain\r.

Cause (deps/dicer/lib/HeaderParser.js)

On the first push, no terminator is found, so the whole chunk — including the leading \r of the terminator — is appended to this.buffer. On the next push the split-terminator branch matches (tail ends with a prefix of \r\n\r\n, data starts with the rest), sets appendEnd = 0 and finishes, but never removes the \r already in this.buffer. _parseHeader only strips full \r\n line terminators, so the lone trailing \r stays glued to the last header value.

Impact

Parsing becomes dependent on transport chunking: the same bytes give a different result depending on where the TCP/HTTP stream is split. The last header of any part (commonly Content-Type/mimetype, also content-transfer-encoding and the RFC 5987 filename*) intermittently carries a trailing \r. Code that compares or switches on the mimetype, or builds a filename, then gets silently wrong data on a fraction of requests. Pure 1-byte chunking does not trigger it (the tail only ever holds one byte), which is why the existing tests missed it.

Fix

Track how many terminator-prefix bytes were buffered on the previous push (splitTail) and trim them from this.buffer before finishing. splitTail is 0 in the common case, so the slice is a no-op there (no new branch, branchless to keep coverage intact).

Tests

Added a dicer-headerparser case for the ...\r | \n\r\n split asserting the value has no trailing \r. It fails on the current code and passes with the fix. Full unit suite green (211 passing) with coverage above the configured thresholds; an exhaustive 2-chunk split of the repro body is now invariant at every split point.

[mcollina]

Thanks for the detailed writeup — I reproduced the bug and the diagnosis is correct. The whole-chunk path (data.indexOf(B_DCRLF)) excludes the terminator via appendEnd = pos, the split path doesn't, and _parseHeader only strips complete \r\n. Confirmed that only the 1-byte-prefix split is actually broken: at splitTail = 2 the buffer ends in a well-formed \r\n, and at splitTail = 3 the dangling \r line hits the no-colon return, which is why the existing ['Foo: bar\r', '\n\r', '\nextra'] case passes today.

One correction to the impact section: this isn't intermittent, it's fully attacker-controlled. Sending the body as two TCP segments split at the terminator CR reproduces it 100% of the time through a real http.createServer:

[1] body in one write:            mimeType = "application/x-php"
[2] split at the terminator CR:   mimeType = "application/x-php\r"

So an app that blocklists on mimeType, transferEncoding, or an unquoted / RFC 5987 filename can be bypassed deterministically. (Quoted filename="a.php" is unaffected — the closing quote absorbs the CR. Allowlists fail closed, so the exposure is blocklist-shaped.)

Please fix before this lands

The slice assumes the last splitTail bytes of this.buffer are the terminator prefix. That doesn't hold once maxHeaderSize truncation has kicked in: if a previous push set maxed, the \r was never appended, and the slice eats real header content.

Repro — 'Foo: bar\r' then '\n\r\n':

maxHeaderSize	whole input	split, main	split, this PR
8	bar	bar	ba
9	bar	bar\r (the bug)	bar

So on the truncation path the patch swaps one wrong output for another. Guarding on !this.maxed is sufficient — splitTail is only guaranteed to be the buffer's tail when nothing was dropped:

if (found) {
  if (!this.maxed) {
    this.buffer = this.buffer.slice(0, this.buffer.length - splitTail)
  }
  this._finish()
  return end
}

That gives up the branchless property, so please add a dicer-headerparser case with a small maxHeaderSize to keep coverage at 100%. I'd rather have a branch with a test than branchless-but-wrong.

Also, this.nread -= splitTail is dead code — _finish() sets this.nread = 0 on the next line. Drop it.

Otherwise the change is good: with the patch applied the full suite is green (228 passing, 100% coverage), and an exhaustive two-chunk split of a multipart body is invariant at every split point.

[spokodev]

You're right about the truncation path — once maxed, the trailing \r was never buffered, so the slice ate real header content. Fixed:

• Guarded the slice on !this.maxed.
• Dropped this.nread -= splitTail (dead — _finish() zeroes nread on the next line).
• Added a dicer-headerparser case with maxHeaderSize: 8 covering the maxed branch.

npm run test:unit is green at 100% branches (229 passing). Conceding the branchless property for a tested branch, as you said.