fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads#1113
fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads#1113spokodev wants to merge 1 commit 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 enforces
maxFileSizeandmaxTotalFileSizeonapplication/octet-streamuploads, closing a gap where those documented limits were silently ignored on the octet-stream path while the multipart path already respected them.src/plugins/octetstream.js: Adds per-chunk size accounting (fileSize,this._totalFileSize) and callsthis._error()with the existingbiggerThanMaxFileSize/biggerThanTotalMaxFileSizeerror codes when a limit is exceeded, causing_error()to destroy and unlink the partial file via the sharedopenedFilescleanup.test/integration/octet-stream.test.js: Adds an integration test sending a 256 KB body againstmaxFileSize: 1024, asserting error code 1016 and an empty files object. ThemaxTotalFileSizebranch (error code 1009) is not yet covered by a test.Confidence Score: 4/5
Safe to merge; the core enforcement logic is correct and the cleanup path (file.destroy → unlink) is reused from the existing multipart implementation.
The fix is small and correctly wired: size counters are accumulated before each write, errors short-circuit via the established _error() path, and endfn's existing this.error guard prevents _parser.end() from firing so the end-handler race is not a concern. The only gaps are a cosmetic duplicate import and a missing test case for the maxTotalFileSize branch, neither of which affects the correctness of the shipped behaviour.
The new biggerThanTotalMaxFileSize branch in src/plugins/octetstream.js (lines 68-77) is not covered by any test.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant Formidable participant OctetStreamPlugin participant PersistentFile Client->>Formidable: POST /upload (Content-Type: application/octet-stream) Formidable->>OctetStreamPlugin: init() — open file, push to openedFiles OctetStreamPlugin->>PersistentFile: file.open() loop Per data chunk Client->>Formidable: chunk Formidable->>OctetStreamPlugin: _parser.emit("data", buffer) OctetStreamPlugin->>OctetStreamPlugin: "fileSize += buffer.length" OctetStreamPlugin->>OctetStreamPlugin: "_totalFileSize += buffer.length" alt "fileSize > maxFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanMaxFileSize 1016) Formidable->>PersistentFile: file.destroy() → unlink Formidable-->>Client: "callback(err, {}, {})" else "_totalFileSize > maxTotalFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanTotalMaxFileSize 1009) Formidable->>PersistentFile: file.destroy() → unlink Formidable-->>Client: "callback(err, {}, {})" else within limits OctetStreamPlugin->>PersistentFile: file.write(buffer) end end alt No error OctetStreamPlugin->>PersistentFile: file.end() Formidable-->>Client: "callback(null, {}, { file: [...] })" else Error already set Formidable->>Formidable: endfn checks this.error → returns (parser.end skipped) end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant Formidable participant OctetStreamPlugin participant PersistentFile Client->>Formidable: POST /upload (Content-Type: application/octet-stream) Formidable->>OctetStreamPlugin: init() — open file, push to openedFiles OctetStreamPlugin->>PersistentFile: file.open() loop Per data chunk Client->>Formidable: chunk Formidable->>OctetStreamPlugin: _parser.emit("data", buffer) OctetStreamPlugin->>OctetStreamPlugin: "fileSize += buffer.length" OctetStreamPlugin->>OctetStreamPlugin: "_totalFileSize += buffer.length" alt "fileSize > maxFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanMaxFileSize 1016) Formidable->>PersistentFile: file.destroy() → unlink Formidable-->>Client: "callback(err, {}, {})" else "_totalFileSize > maxTotalFileSize" OctetStreamPlugin->>Formidable: _error(biggerThanTotalMaxFileSize 1009) Formidable->>PersistentFile: file.destroy() → unlink Formidable-->>Client: "callback(err, {}, {})" else within limits OctetStreamPlugin->>PersistentFile: file.write(buffer) end end alt No error OctetStreamPlugin->>PersistentFile: file.end() Formidable-->>Client: "callback(null, {}, { file: [...] })" else Error already set Formidable->>Formidable: endfn checks this.error → returns (parser.end skipped) endReviews (1): Last reviewed commit: "fix: enforce maxFileSize and maxTotalFil..." | Re-trigger Greptile