fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads - #1113
Open
spokodev wants to merge 2 commits into
Open
fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads#1113spokodev wants to merge 2 commits into
spokodev 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.
Comment on lines
+4
to
+5
| 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).
Suggested change
| 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!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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