Skip to content

fix: enforce parts limit after exceeding it - #1446

Open
deepakganesh78 wants to merge 1 commit into
expressjs:mainfrom
deepakganesh78:fix/issue1418-parts-limit
Open

fix: enforce parts limit after exceeding it#1446
deepakganesh78 wants to merge 1 commit into
expressjs:mainfrom
deepakganesh78:fix/issue1418-parts-limit

Conversation

@deepakganesh78

Copy link
Copy Markdown

Fixes #1418

Reproduction

A multipart request with exactly two parts (field0 plus tiny0) and limits: { parts: 2 } currently completes parsing and then fails with LIMIT_PART_COUNT at the closing boundary.

Root cause

Busboy emits partsLimit when its internal boundary counter reaches the configured value. Multer forwards that event directly, so a request with exactly the allowed number of fields/files is treated as over the limit.

Fix

Multer now removes parts from the limits passed to Busboy and counts actual field/file parts itself. It raises LIMIT_PART_COUNT only when the request exceeds the configured number of parts; the behavior for over-limit requests is unchanged.

Compatibility notes

This changes limits.parts to the documented max number of fields plus files. Requests exactly at the limit are now accepted; requests over the limit still fail with LIMIT_PART_COUNT.

Validation

  • Regression test fails without the fix: npm test -- --grep "should allow parts up to the limit" fails with LIMIT_PART_COUNT (0 passing, 1 failing).
  • Targeted validation: npm test -- --grep "parts limit|parts up to" -> 2 passing.
  • Lint: npm run lint -> passed.
  • Full suite on Windows/Node v24: npm test -> 72 passing, 12 failing. The same checkout before this patch reports 71 passing, 12 failing; failures are pre-existing Windows fixture line-ending/path assertions (e.g. expected 1778 but got 1803, and /testforme- path separator check).

Count multipart parts in Multer so requests with exactly the configured number of fields and files are accepted, while requests that exceed the limit still fail with LIMIT_PART_COUNT.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ran this on Windows 11, Node 24.18.0, against main at 4203ccd.

The off-by-one is real and the fix discriminates. This suite is nondeterministic on Windows (base runs vary 70–71 passing / 12–13 failing on the same commit), so raw counts are useless — I compared failing-test sets with the lib reverted against the PR head, same test files both sides:

  • Fails only with the old lib: Error Handling should allow parts up to the limit — the PR's new test, i.e. a request with exactly parts: 2 parts wrongly aborts with LIMIT_PART_COUNT today, exactly as #1418 describes.
  • Fails only on the PR: nothing. (The other difference in the reverted run was the known-flaky Select Field before-all hook, which comes and goes on identical commits here.)

Run directly, the three parts-limit tests pass on the PR head.

On the implementation: both the field and file handlers route through incrementPartCount(), so the count covers all part kinds; the typeof/isFinite guard means no behaviour change when parts is unset; and the object spread matches existing usage in this file (lines ~259/269), so no style drift.

One semantic shift worth a sentence in the description: since parts is now stripped from the limits busboy sees, busboy will begin parsing the N+1th part — headers included — before multer aborts, where previously busboy stopped emitting at its own boundary. For well-formed requests that's invisible; for a hostile stream it means the abort lands one part-header later than before. I don't think it matters given LIMIT_PART_COUNT still fires and the request is torn down, but it's the kind of thing a future reader will wonder about, and the PR description is the right place to say it was considered.

Also worth noting for reviewers: busboyLimits() returns the original object untouched when parts isn't set — no copy — so there's no per-request allocation cost in the common path. Nice touch.

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.

Parts Limit is misleading

2 participants