Repository navigation
Base64 Content-Transfer-Encoding parts are decoded per-chunk, corrupting stored data #280
Description
Summary
Form.prototype.onParsePartData decodes each network chunk of a Content-Transfer-Encoding: base64 part as an independent base64 string, with no decoder state carried between chunks. Node's base64 decoder drops the 1 to 3 trailing characters that do not complete a 4-character group, and because no remainder is kept, those characters are lost. As a result the bytes multiparty writes to disk depend on where the incoming data was split into chunks rather than on the content of the request, so the same upload can be stored differently depending on network chunking or an upstream proxy's buffering.
The project already avoids this on the header and field paths by using a stateful string_decoder (headerFieldDecoder / headerValueDecoder, and handleField). The base64 part path is the one place a stateful decoder is needed and not used, so this is a self-contained fix that mirrors code already in the file. Good for a first contribution.
Affected code
index.js (unchanged since the streams2 rewrite in f19bc82 and released on 2.1.0:
Form.prototype.onParsePartData = function(b) {
if (this.partTransferEncoding === 'base64') {
this.backpressure = ! this.destStream.write(b.toString('ascii'), 'base64');
} else {
this.backpressure = ! this.destStream.write(b);
}
}b is a slice of a single network chunk, so each call runs a fresh Buffer.from(str, 'base64') and any incomplete trailing quantum is discarded. b.toString('ascii') also masks the high bit, so a byte >= 0x80 becomes a different valid base64 character instead of being rejected.
How to reproduce
Send a Content-Transfer-Encoding: base64 part and split the writes so the base64 body does not arrive on 4-character boundaries. Two controls isolate the cause:
- The same file sent base64-encoded in a single write is stored correctly.
- The same file sent as a binary part split across several writes is stored correctly.
- Only base64 and chunking together corrupt the stored file, on every attempt (not a race).
Suggested approach
this is a quite brainstorm and might not be the final implementation (just some ideas)
- In
clearPartVars, initialise a remainder buffer:self.partB64Remainder = ''. - In
onParsePartData, prepend the remainder, decode only the complete 4-character quantums, and keep the leftover for the next chunk. Uselatin1instead ofasciiso high-bit bytes are not turned into a different valid base64 character:
Form.prototype.onParsePartData = function(b) {
if (this.partTransferEncoding === 'base64') {
var s = this.partB64Remainder + b.toString('latin1');
var cut = s.length - (s.length % 4);
this.partB64Remainder = s.slice(cut);
this.backpressure = ! this.destStream.write(Buffer.from(s.slice(0, cut), 'base64'));
} else {
this.backpressure = ! this.destStream.write(b);
}
}- In
onParsePartEnd, flushpartB64Remainderbefores.end(), rejecting a remainder of length 1 (never valid base64) rather than discarding it.
How to verify
Add a test that uploads a base64 part whose body is written across chunk boundaries that do not align to 4 characters, and assert the stored file equals the original bytes. A matching control that sends the same body in a single write should already pass. Run npm test and npm run lint.
Acceptance criteria
- A base64 part split across arbitrary chunk boundaries decodes to the same bytes as the single-write case.
- A regression test covers the split-chunk case.
- Existing tests and lint pass.
Reported and initial analysis done by @euriconicacio (internal ref).
Activity