fix: remove query param limit (1000) to prevent silent truncation#7089
fix: remove query param limit (1000) to prevent silent truncation#7089KJyang-0114 wants to merge 2 commits intoexpressjs:masterfrom
Conversation
Before: console.error(err.stack || err.toString()) After: console.error(err) This preserves error.cause, nested errors, and async stack traces that are lost when using err.stack alone. Fixes expressjs#6462
Issue: query params with >1000 values were silently truncated because querystring.parse and qs.parse have default parameterLimit of 1000. Fix: Set parameterLimit to Infinity for both simple and extended query parsers. This ensures express doesn't silently lose data without warning, similar to how body-parser returns an error when limit is exceeded. Fixes expressjs#5878
| function logerror(err) { | ||
| /* istanbul ignore next */ | ||
| if (this.get('env') !== 'test') console.error(err.stack || err.toString()); | ||
| if (this.get('env') !== 'test') console.error(err); |
There was a problem hiding this comment.
This is an unrelated change
| case 'simple': | ||
| fn = querystring.parse; | ||
| fn = function(str) { | ||
| return querystring.parse(str, undefined, undefined, { parameterLimit: Infinity }); |
There was a problem hiding this comment.
querystring.parse does not have an option named parameterLimit
|
Thank you for the review. You're right that unlimited parameters could be a DoS vector. To address this concern, I can modify the PR to:
This way:
Would this approach work? Alternatively, should I add a warning note in the docs about the DoS risk when increasing the limit? Also noting there's a similar PR #7009 - would it make sense to combine efforts? |
|
You're right - doesn't have . The fix only applies to the extended query parser (qs.parse), which is what Express uses by default. I can update the PR to:
Would that approach work? Or should I close this PR in favor of the alternative approach of requiring users to provide their own query parser? |
2 participants
Summary
Fixes #5878 - Query Param Silently Remove param query value if it is over 1000
Problem
When query params have >1000 values, they are silently truncated. Unlike body-parser, which returns an error when the limit is exceeded, Express silently loses data without warning.
Solution
Keep the default at 1000 for backward compatibility, but add a new
query.parameterLimitapp setting:This approach:
Related