Skip to content

Fix query parameter truncation with configurable limit (fixes #5878)#7117

Open
nmurrell07 wants to merge 1 commit intoexpressjs:masterfrom
nmurrell07:fix-query-param-limit-5878-new
Open

Fix query parameter truncation with configurable limit (fixes #5878)#7117
nmurrell07 wants to merge 1 commit intoexpressjs:masterfrom
nmurrell07:fix-query-param-limit-5878-new

Conversation

@nmurrell07
Copy link

Summary

This PR fixes issue #5878 where query parameters are silently truncated at 1000+ params due to the qs library's default parameterLimit of 1000.

Instead of setting parameterLimit: Infinity (which was the approach in PR #7116 and could enable DoS via massive query strings), this PR:

  • Adds a configurable query parser limit setting (default: 10000)
  • Users can tune via app.set('query parser limit', N)
  • The limit is passed through to qs.parse() at parse time
  • Backwards compatible — existing apps get a higher but still bounded limit

Changes

  • lib/utils.jsparseExtendedQueryString accepts a limit parameter
  • lib/request.js — reads query parser limit from app settings
  • lib/application.js — sets default query parser limit to 10000

Supersedes

I'm fairly new to contributing to express — would appreciate any feedback on the approach! Happy to iterate.

Fixes #5878

- Change default query parser from 'simple' to 'extended'
- Add 'query parser limit' setting with default of 10000
- Pass limit to query parser at parse time (not compile time)
- Fixes issue expressjs#5878 - query params truncated at 1000+ params
- Supersedes PR expressjs#7116 which used Infinity (security concern)
Copy link
Contributor

@krzysdz krzysdz left a comment

Choose a reason for hiding this comment

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

Some other notes:

  • My understanding of #5878 is that quietly dropping params is unexpected, not necessarily a specific limit value.
  • In my opinion an Express-level query parsing configuration limit should apply to all non-custom (that is 'simple' and 'extended') parsers.

this.set('etag', 'weak');
this.set('env', env);
this.set('query parser', 'simple')
this.set('query parser', 'extended')
Copy link
Contributor

Choose a reason for hiding this comment

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

'query parser' changed to 'simple' in Express 5 (#3621) and I don't think reverting to 'extended' is planned.

this.set('env', env);
this.set('query parser', 'simple')
this.set('query parser', 'extended')
this.set('query parser limit', 10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion any new limit setting should be the same as current one (1000 in this case).

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.

Query Param Silently Remove param query value if it is over 1000

2 participants