fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads - #1113
fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads#1113spokodev wants to merge 2 commits into
Conversation
The octet-stream upload path wrote every chunk to disk without checking the documented maxFileSize/maxTotalFileSize limits, unlike the multipart path in _handlePart. Accumulate per-file and total sizes and abort via _error with the existing FormidableError codes when a cap is exceeded, mirroring the multipart implementation. The over-limit file is removed through the shared _error cleanup, so no partial bytes remain on disk.
| import * as errors from "../FormidableError.js"; | ||
| import FormidableError from "../FormidableError.js"; |
There was a problem hiding this comment.
The two separate import statements can be merged into a single combined import, which is the pattern used throughout the codebase (e.g.
import FormidableError, * as errors from "./FormidableError.js" in Formidable.js).
| import * as errors from "../FormidableError.js"; | |
| import FormidableError from "../FormidableError.js"; | |
| import FormidableError, * as errors from "../FormidableError.js"; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| test("octet stream enforces maxFileSize", (done) => { | ||
| const PORT2 = PORT + 1; | ||
| const server = createServer((req, res) => { | ||
| const form = formidable({ maxFileSize: 1024, maxTotalFileSize: 2048 }); | ||
|
|
||
| form.parse(req, (err, fields, files) => { | ||
| // a 256KB octet-stream body must be rejected, not written to disk | ||
| assert(err, "expected an error for over-sized octet-stream upload"); | ||
| strictEqual(err.code, 1016); // biggerThanMaxFileSize | ||
| strictEqual(Object.keys(files).length, 0); | ||
|
|
||
| res.end(); | ||
| server.close(); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| server.listen(PORT2, (err) => { | ||
| assert(!err, "should not have error, but be falsey"); | ||
|
|
||
| const request = _request({ | ||
| port: PORT2, | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/octet-stream", | ||
| }, | ||
| }); | ||
|
|
||
| request.end(Buffer.alloc(256 * 1024, 0x42)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
maxTotalFileSize branch is never exercised
The test sets maxFileSize: 1024 and maxTotalFileSize: 2048, then sends 256 KB. Because fileSize > maxFileSize fires first (on the very first chunk), the _totalFileSize > maxTotalFileSize branch in octetstream.js lines 68-77 is never reached. The new error code 1009 (biggerThanTotalMaxFileSize) path has zero test coverage. A second case — e.g. maxFileSize: 4096, maxTotalFileSize: 1024 with a 2 KB body — would exercise it and guard against a future regression there.
The octet-stream upload path does not enforce the documented
maxFileSize/maxTotalFileSizelimits, unlike the multipart path.Steps to reproduce
POST a 256KB body with
Content-Type: application/octet-streamto a form configured withmaxFileSize: 1024:ACTUAL:
errisundefined, one file is returned with size 262144, and the over-sized file is committed to disk viafile.end().EXPECTED:
err.code === 1016(biggerThanMaxFileSize), no file returned, and nothing left on disk. This matches how the multipart path already behaves.Root cause
The octet-stream plugin's
_parser.on("data", ...)handler insrc/plugins/octetstream.jswrites each chunk straight to the file with no size check, and it bypasses_handlePart()insrc/Formidable.jswhere the multipart size caps are enforced. As a result a raw octet-stream body of any size is accepted regardless of the configured limits.The README documents
maxFileSizeandmaxTotalFileSizeas limiting each file and the batch respectively, with defaults, and does not exempt octet-stream.Fix
Accumulate the per-file and running total sizes before each write and abort via
this._error(...)with the existingFormidableErrorcodesbiggerThanMaxFileSize/biggerThanTotalMaxFileSizewhen a cap is exceeded, mirroring_handlePart. In-limit uploads are unaffected. Because the octet-stream file is tracked inopenedFiles, the shared_errorcleanup callsfile.destroy(), which unlinks the partial file, so no bytes remain on disk (the same cleanup the multipart path relies on).Authority
CWE-770 (allocation without limits), plus the library's own documented contract and its multipart implementation, which enforces exactly these caps.
Tests
Added an integration case in
test/integration/octet-stream.test.js: a 256KB octet-stream body withmaxFileSize: 1024must be rejected with code 1016 and return no files. Verified it fails on the current source (the over-sized upload is accepted witherrnull) and passes with the fix, with the tmp directory left empty afterwards.Suite status: 92 passed / 3 skipped across 14 jest suites, and 11/11 node tests.
Greptile Summary
This PR fixes a gap where
application/octet-streamuploads bypassed themaxFileSizeandmaxTotalFileSizelimits that were already enforced on the multipart path. The fix adds per-chunk size accounting in the_parser.on("data")handler and calls the shared_error()cleanup (which destroys the partial file) on violation.src/plugins/octetstream.js: Introduces a localfileSizecounter and incrementsthis._totalFileSizeon each chunk, aborting via_error()with the correctFormidableErrorcodes (1016 / 1009) before any write occurs — ensuring no oversized bytes land on disk.test/integration/octet-stream.test.js: Adds an integration test that POSTs a 256 KB body against a 1 KB limit and assertserr.code === 1016and an emptyfilesobject. ThemaxTotalFileSize(code 1009) branch remains untested.Confidence Score: 5/5
_error()properly destroys the partial file before any over-limit bytes are committed, and the existingonce-wrapped callback prevents double-invocation. The only nit is that themaxFileSize/maxTotalFileSizecheck order is swapped relative to the multipart path, which can produce a different error code when both limits are crossed in the same chunk — but this is an edge case with no functional impact on the normal reject-and-clean-up behaviour.Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Formidable participant OctetStreamPlugin participant File Client->>Formidable: POST /upload (application/octet-stream) Formidable->>OctetStreamPlugin: init() — opens file, starts parser OctetStreamPlugin->>File: file.open() loop Each data chunk Client->>Formidable: chunk Formidable->>OctetStreamPlugin: _parser.emit("data", buffer) alt "NEW: fileSize > maxFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanMaxFileSize 1016) Formidable->>File: file.destroy() Formidable-->>Client: "error callback (err.code=1016, files={})" else "NEW: _totalFileSize > maxTotalFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanTotalMaxFileSize 1009) Formidable->>File: file.destroy() Formidable-->>Client: "error callback (err.code=1009, files={})" else Within limits OctetStreamPlugin->>File: file.write(buffer) end end Client->>Formidable: end OctetStreamPlugin->>File: file.end() Formidable-->>Client: "success callback (files={file:[...]})"Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile