expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.15k stars 15.59k forks source link

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

Open ItsRLuo opened 2 weeks ago

ItsRLuo commented 2 weeks ago

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.

Mouri-P commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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!