Skip to content

fix: improve limit option validation#698

Open
Phillip9587 wants to merge 2 commits intoexpressjs:masterfrom
Phillip9587:limit-option-validation
Open

fix: improve limit option validation#698
Phillip9587 wants to merge 2 commits intoexpressjs:masterfrom
Phillip9587:limit-option-validation

Conversation

@Phillip9587
Copy link
Member

This PR fixes an issue where invalid limit options were silently accepted and effectively disabled request size limit enforcement.

Currently, when a user passed an invalid value (such as an unparseable string or NaN), bytes.parse() would return null, causing the request size limit validation to be skipped entirely. This happened without any error or warning, leaving users unaware that the configured limit was not being applied.

This PR makes the behavior explicit and predictable: null and undefined fall back to the default limit (100kb), valid values continue to work as before, and invalid values now throw a clear error instead of silently disabling the limit. This prevents misconfiguration and ensures request size limits are always enforced as expected.

This PR represents one of two possible approaches: either throwing an Error or logging a warning when an invalid option is provided. Throwing an Error seemed preferable, as it fails fast and makes configuration issues immediately visible.

At the moment there is also no explicit way to disable request size limit validation entirely. A possible follow-up for the next major version could be to change the behavior so that passing null explicitly disables the request limit check.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a security issue where invalid limit option values were silently accepted, causing request size limit enforcement to be disabled without warning. The fix makes validation explicit: null/undefined now fall back to the default 100kb limit, valid values continue to work as before, and invalid values throw a clear error immediately.

Changes:

  • Modified normalizeOptions in lib/utils.js to explicitly check for null/undefined limit values and throw errors for invalid values instead of silently accepting them
  • Added comprehensive test coverage for edge cases including undefined, null, 0, NaN, booleans, objects, and invalid strings

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/utils.js Updated limit validation logic to explicitly handle null/undefined with default value and throw TypeError for invalid limit values returned by bytes.parse()
test/utils.js Added 8 new test cases covering undefined, null, zero, string without unit, invalid strings, NaN, booleans, and objects; removed old test that expected null for invalid input

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
@Phillip9587 Phillip9587 requested a review from ctcpip February 2, 2026 11:20
Copy link
Member

@efekrskl efekrskl left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants