Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ItsRLuo opened this issue Aug 29, 2024 · 7 comments · May be fixed by #5901
Open

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

ItsRLuo opened this issue Aug 29, 2024 · 7 comments · May be fixed by #5901
Labels

Comments

@ItsRLuo
Copy link

ItsRLuo commented Aug 29, 2024

Issue

The issue here is that if I have a really long query param(over 1000) ie. test?ids[]=1&ids[]=2..., it will truncate the value after length over 1000. This is because the qs library has a default parameterLimit of 1000 which then it won't parse any more value after. It seems in express body parser, this issue also exists but it returns an error if it is over a limit. https://github.com/expressjs/body-parser#parameterlimit
image

I know you can override the default query parser with my own, however I think this is very dangerous because the api shouldn't silently return incorrect value without warning. This issue is also coming from 2 layer of library deep so it is not easy to figure out for user of expressjs in my opinion.

Fix

  1. it should either return an error(similar to body parser), because I think this shouldn't silently remove value without alerting the engineer

  2. Alternatively we should set the parameterLimit limit to infinite(in qs options), this way if the user want to change the limit, they can knowingly change it, the users who are not aware of this won't be affected.

I can help with the PR if the above makes sense.

@ItsRLuo ItsRLuo added the bug label Aug 29, 2024
Mouri-P pushed a commit to Mouri-P/express that referenced this issue Sep 3, 2024
@Mouri-P
Copy link

Mouri-P commented Sep 3, 2024

I think the exported query parse middleware should allow an overwrite of req.query

  return function query(req, res, next){
    var val = parseUrl(req).query;
    req.query = queryparse(val, opts);

    next();
  };

instead of

  return function query(req, res, next){
    if (!req.query) {
      var val = parseUrl(req).query;
      req.query = queryparse(val, opts);
    }

    next();
  };

so that the users can do

app.use(express.query({ parameterLimit: 10000 }));

A quick workaround for this is

app.use(function (req, res, next) {
  (req.query = undefined), next();
}, express.query({ parameterLimit: 10000 }));

@ItsRLuo
Copy link
Author

ItsRLuo commented Sep 5, 2024

oh nice fix, that would work for my case, although I guess my main concern is still on that it silently removes query param if someone is not aware what is happening.

@Mouri-P
Copy link

Mouri-P commented Sep 6, 2024

While I agree that this could be problematic for an unsuspecting user, throwing warnings whenever default values are used might not be the best solution, as it could lead to noise. Better documentation on these default behaviors would help clarify what’s happening.

Moreover, passing 1000 elements as query parameters is not a widely used or recommended practice. Most web servers and browsers have a maximum URL length (typically around 2,000 characters), and sending a large number of elements in the query string can exceed this limit. It also makes URLs harder to read and maintain, and can expose sensitive data in server logs and browser history.

For such large datasets, using a POST request with a JSON body is generally preferred.

@ItsRLuo Your thoughts?

@ItsRLuo
Copy link
Author

ItsRLuo commented Sep 9, 2024

Those are very good best practices, I do still think it make sense to error or at least not remove values. A example is if you have a query param that is less than 1000 limit but with a large size, you will get a 414 URI Too Long error in your request, this makes so much sense to me so you can actually debug and figure out what was wrong instead of silently having a data integrity issue.

However I do realized it is a rare case if at all you would want to have such large query parameter in your request, so I am fine with what you have to fix this issue as well!

@ljharb
Copy link

ljharb commented Oct 25, 2024

fwiw I’d be willing to accept an option in qs that throws when exceeding the parameterLimit instead of silently truncating - then express could pass through both options so users can configure them as needed.

@wesleytodd
Copy link
Member

If this option was available we would make sure it is passed through and tested on this end.

Also, for OP: I would consider not passing that many query params. There are more efficient ways to pass data to the server and what you describe of passing more than 1000 would for sure be an area you can improve your application by removing.

@ljharb
Copy link

ljharb commented Oct 27, 2024

Filed ljharb/qs#515 in case anyone wants to discuss and implement it.

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 a pull request may close this issue.

4 participants