Skip to content

fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads - #1113

Open
spokodev wants to merge 2 commits into
node-formidable:masterfrom
spokodev:fix/octetstream-size-limits
Open

fix: enforce maxFileSize/maxTotalFileSize on octet-stream uploads#1113
spokodev wants to merge 2 commits into
node-formidable:masterfrom
spokodev:fix/octetstream-size-limits

Conversation

@spokodev

@spokodev spokodev commented Jul 1, 2026

Copy link
Copy Markdown

The octet-stream upload path does not enforce the documented maxFileSize / maxTotalFileSize limits, unlike the multipart path.

Steps to reproduce

POST a 256KB body with Content-Type: application/octet-stream to a form configured with maxFileSize: 1024:

const form = formidable({ maxFileSize: 1024, maxTotalFileSize: 2048 });
form.parse(req, (err, fields, files) => {
  console.log(err, Object.keys(files).length);
});
// request.end(Buffer.alloc(256 * 1024, 0x42));

ACTUAL: err is undefined, one file is returned with size 262144, and the over-sized file is committed to disk via file.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 in src/plugins/octetstream.js writes each chunk straight to the file with no size check, and it bypasses _handlePart() in src/Formidable.js where 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 maxFileSize and maxTotalFileSize as 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 existing FormidableError codes biggerThanMaxFileSize / biggerThanTotalMaxFileSize when a cap is exceeded, mirroring _handlePart. In-limit uploads are unaffected. Because the octet-stream file is tracked in openedFiles, the shared _error cleanup calls file.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 with maxFileSize: 1024 must be rejected with code 1016 and return no files. Verified it fails on the current source (the over-sized upload is accepted with err null) 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-stream uploads bypassed the maxFileSize and maxTotalFileSize limits 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 local fileSize counter and increments this._totalFileSize on each chunk, aborting via _error() with the correct FormidableError codes (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 asserts err.code === 1016 and an empty files object. The maxTotalFileSize (code 1009) branch remains untested.

Confidence Score: 5/5

  • Safe to merge — the fix is narrowly scoped to the octet-stream data handler and the error cleanup path is the same one used by multipart.
  • The size-check logic is correct, _error() properly destroys the partial file before any over-limit bytes are committed, and the existing once-wrapped callback prevents double-invocation. The only nit is that the maxFileSize / maxTotalFileSize check 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.
  • No files require special attention.

Important Files Changed

Filename Overview
src/plugins/octetstream.js Adds per-chunk maxFileSize and maxTotalFileSize enforcement to the octet-stream data handler. Logic is correct and cleanup via _error/file.destroy() works as in the multipart path. Minor: check order is reversed vs. the multipart implementation, so the reported error code differs when both limits are crossed simultaneously.
test/integration/octet-stream.test.js Adds an integration test for maxFileSize enforcement on a 256 KB octet-stream body. Correctly asserts err.code === 1016 and an empty files object. The maxTotalFileSize branch (code 1009) has no test coverage.

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:[...]})"
Loading

Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile

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";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

Fix in Codex Fix in Claude Code

Comment on lines +56 to +86
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));
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Fix in Codex Fix in Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant