perf(urlencoded): optimize parameter counting for better memory efficiency

[Ayoub-Mabrouk]

The previous implementation used body.split('&') which always processed the entire request body and allocated a full array, regardless of the parameter limit.

The new implementation:

• Counts '&' characters iteratively without array allocation
• Exits immediately when the limit is reached
• Reduces time complexity from O(n) worst-case always to O(min(n, limit))
• Added test case to verify empty body handling with parameterLimit option.

This particularly improves resilience against malicious requests with thousands of parameters attempting to exhaust server resources.

[bjohansebas]

Thank you for the contribution, this has already been resolved in our recent security patch.

GHSA-wqch-xfxh-vrr4

[Ayoub-Mabrouk]

Thank you for the contribution, this has already been resolved in our recent security patch.

GHSA-wqch-xfxh-vrr4

Hey @bjohansebas , just wanted to flag something about the parameterCount fix that went into the security patch.

I had opened PR #652 back on Oct 29 to fix this exact issue - basically the same DoS vulnerability you patched. My version kept the original behavior while adding the early exit logic.

The thing is, the implementation that got merged (commit b204886) doesn't actually return the same values as the old code did.

Quick example with "a=1&b=2&c=3":

• Old version returned 2
• My PR also returned 2
• Current patch returns 4

Or for "a=1" with no ampersands:

• Old version returned 0
• My PR returned 0
• Current patch returns 1

The issue is the do-while loop always runs at least once and counts iterations instead of actual & characters, so the numbers are off.

Not trying to reopen my PR or anything, just thought someone should know since this changes behavior that might be relied on elsewhere. Could cause weird issues down the line even though it's technically internal.

[Phillip9587]

Hey @Ayoub-Mabrouk, you are right. The security fix implementation is off by 1 compared to the original implementation. But it is actually more correct then the old implementation with some small exceptions:

"user=bob"
    2.2.1: 1
    1.20.3: 0
    2.2.0: 0

This actually is one parameter so both the old 1.x and the split based version were wrong.

"a=1&b=2&c=3"
    2.2.1: 3
    1.20.3: 2
    2.2.0: 2

Same here 1.x and split based are wrong

"&a=1&b=2&c=3"
    2.2.1: 4
    1.20.3: 3
    2.2.0: 3

All our tests are written without the leading "&" so don't know if this is valid?

""
    2.2.1: 1
    1.20.3: 0
    2.2.0: 0

This is the case that needs some fixup i think. But it is not critical as arrayLimit is set to Math.max(100, parameterCount).

I already talked to @UlisesGascon and he suggested opening a PR which adds tests and documentation for the expected behaviour of this function.

[Ayoub-Mabrouk]

Hi @UlisesGascon, I’ve added the fix for parameterCount along with tests covering all edge cases

[blakeembrey]

Should it just defer to qs and handle the error by enabling this option? https://github.com/ljharb/qs/blob/e8b32388dd8d095def77f5f21d3fdb7ca0c49cbc/lib/parse.js#L72-L74

[Ayoub-Mabrouk]

@bjohansebas @Phillip9587 as you can see I've implemented a fix again by removing the doWhile and added tests for it.
Is there any feedback on this? Or should this change appear on another pr?

[bjohansebas]

Should it just defer to qs and handle the error by enabling this option? https://github.com/ljharb/qs/blob/e8b32388dd8d095def77f5f21d3fdb7ca0c49cbc/lib/parse.js#L72-L74

Probably it would be good to delegate it to qs, but there would be a breaking change since it would no longer throw that error, which would most likely be for a major version. Additionally, it would be good to run benchmarks with and without using parameterCount and see how much it improves or worsens, because I feel we’re repeating work that qs already does for us. I haven’t analyzed it in depth, so feel free to run those tests.

@bjohansebas @Phillip9587 as you can see I've implemented a fix again by removing the doWhile and added tests for it.
Is there any feedback on this? Or should this change appear on another pr?

It’s on my to-do list to review, but leave this PR open, there’s no need to open more PRs.

[ljharb]

I could implement a change in qs behind an option, that express sets - i'm unclear on what the change would be though.

[Ayoub-Mabrouk]

@blakeembrey Should it just defer to qs and handle the error by enabling this option? https://github.com/ljharb/qs/blob/e8b32388dd8d095def77f5f21d3fdb7ca0c49cbc/lib/parse.js#L72-L74

Hi @blakeembrey, I implemented your suggestion. Just to give some context, I had opened PR #652 back in October to fix the DoS issue with parameterCount that I mentioned in my earlier comment. @Phillip9587 committed the security patch (b204886) before my PR could be reviewed, which was good to get the fix out quickly. However, the implementation that got merged had a bug in the counting logic (the do-while loop issue I mentioned earlier, it counts iterations instead of actual & characters, returning incorrect values). So this PR is fixing that bug in the merged code and adding the missing tests.

Here's what I found:

I refactored the code to defer parsing to qs as you suggested, but kept the early exit since it's important for DoS protection. The new implementation uses countAndValidateParameters() that throws immediately when the limit is exceeded (before qs gets called), and then qs handles all the parsing work. Since we already validated, qs uses its default throwOnLimitExceeded: false which is what we want.

After implementing this, I realized the existing code was already doing most of what you suggested: it had early exit (qs never got called if limit exceeded), it was already deferring parsing to qs (since the default is false, qs doesn't check again), and the behavior was already optimal.

I tried the pure deferral approach (removing the manual counting, using throwOnLimitExceeded: true), but that would be slower because qs would do a full string split even when the limit's exceeded, which loses the early exit benefit that's important for DoS protection.

So the new code maintains the same optimal behavior as before, just with cleaner structure. It defers parsing to qs as you suggested, while keeping the early exit for performance.

---

@bjohansebas Probably it would be good to delegate it to qs, but there would be a breaking change since it would no longer throw that error, which would most likely be for a major version. Additionally, it would be good to run benchmarks with and without using parameterCount and see how much it improves or worsens, because I feel we're repeating work that qs already does for us.

@bjohansebas I did some quick timing tests and found that keeping the early exit is important. When the limit is exceeded, the pure deferral approach (using throwOnLimitExceeded: true) would be slower because qs does a full string split before throwing, while the early exit approach avoids that entirely. For DoS protection, that early exit matters.

Regarding the "repeating work" concern, the existing code already had this optimization, and the new code keeps it while still deferring parsing to qs. So we're not really duplicating work: we do a quick pre-check for early exit, then qs handles all the parsing. If you'd like, I can run more thorough benchmarks to quantify the difference.

---

@ljharb I could implement a change in qs behind an option, that express sets - i'm unclear on what the change would be though.

@ljharb Thank you for offering! After implementing this, I don't think we need any changes to qs. The existing throwOnLimitExceeded option works perfectly for this, we just use the default (false) since we already validate the limit before calling qs. So no changes needed on your end.

What do you all think? Is keeping the early exit while deferring parsing what you had in mind, or would you prefer going full deferral despite the performance trade-off?