fix: validate that limit option is non-negative

[abhu85]

Summary

This PR adds validation to reject negative limit values like '-100kb' that were previously silently accepted.

Problem

Currently, body-parser accepts negative limit values without validation:

app.use(bodyParser.json({ limit: '-100kb' }));
// No error, middleware added successfully

This is problematic because:

• Invalid configuration is silently accepted
• The behavior of a negative limit is undefined
• Bugs in consuming applications may go unnoticed

Solution

Added validation in normalizeOptions() to throw a TypeError when a negative limit is provided:

if (limit !== null && limit < 0) {
  throw new TypeError('option limit must be a non-negative number')
}

This applies to all parsers (json, urlencoded, raw, text) since they all use normalizeOptions().

Test Plan

• Added test for negative string limit ('-100kb')
• Added test for negative number limit (-1024)
• Added tests to verify zero limit is still accepted (0 and '0kb')
• All existing 262 tests pass
• 4 new tests added (266 total)

Fixes #705

[Phillip9587]

Related to #698. I think we should also throw an error/warn the user when using a zero limit because the middleware would return a HTTP 413 "request entity too large" (from raw-body) on every request.

[abhu85]

Thanks for the feedback @Phillip9587!

You make a good point about zero limits - they would effectively make the middleware unusable since every request with a body would get HTTP 413.

I see you have a more comprehensive fix in #698. Would you prefer:

1. I update this PR to also reject zero limits (making limit > 0 required)
2. Keep this PR focused on negative limits (per [BUG] body-parser Accepts Negative Limit Values #705) and let fix: improve limit option validation #698 handle the zero case
3. Close this PR if fix: improve limit option validation #698 already covers negative limit validation

Happy to go whichever direction works best for the project!

[Phillip9587]

Hey @abhu85, let's keep this PR open for now and wait for other maintainer feedback. We can merge #698 and than follow-up with your PR. Throwing on a zero limit and on negative limits may be considered a major as it changes how the module behaves.

[UlisesGascon]

I think is a good addition, but semver-major at this stage probably someone is relaying on this bug. I agree with the @Phillip9587 concerns 👍

[abhu85]

@Phillip9587 Any idea when #698 might be merged? Happy to rebase this PR once that lands.

[Phillip9587]

@abhu85 I'm sorry. Still waiting for reviews and a descicion from the team.

[jonchurch]

We've had a discussion recently at the express level about how we feel about adding runtime validation like this.

We're still evolving our stance, but this stands out to me as a good example.

I'm wary of this kind of validation for a few reasons. One is the simple "opening a can of worms" idea, or stated another way, if we validate the content of the options beyond their types, when do we stop validating?

The second is, why do we even need to do this validation? As in, what abstraction or design decision has led us to accept a valid input type that has unclear user intent but a unique outcome (see below)?

In this case, it is very clear that a user would have done this by mistake. The result of passing a negative limit is that all requests with a body (even Content-Length: 0 funnily enough) will be 413'd by raw-body. Passing zero would accept Content-Length: 0, and I don't think that difference from a negative value is intentional on raw-body's or the user's end.

That is a bug IMO in raw-body. A negative limit is mathematically nonsense here. I think raw-body should validate the input itself as it actually uses it, and it already validates its other inputs. That way every consumer gets the fix, not just body-parser.

However, a major rev to raw-body with a throw for negatives is functionally the same as a major rev here and doing the same.

[jonchurch]

I dont think we should throw on zero btw.

It is a choice that seems silly, but there is no other way to express "accept only Content-Length: 0" which is spec valid.

[jonchurch]

Semver on this is eh, major bc of the throw but no extremely clear broken state.

But even moreso, this is low enough priority that it can just wait for a major rev

[jonchurch]

lgtm as long as we dont throw on zero👍

leaving a block as a reminder to target next major branch

We dont have one pushed rn and Im on mobile

[abhu85]

Thanks for the review feedback @jonchurch!

I understand this should target the next major branch. Since there isn't one pushed yet, should I:

1. Keep this PR open and you'll retarget it when the branch exists
2. Close this PR and resubmit when the branch is ready
3. Something else?

Happy to do whatever works best for the project's workflow.