-
-
Notifications
You must be signed in to change notification settings - Fork 16.1k
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
Comments
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 })); |
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. |
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? |
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! |
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. |
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. |
Filed ljharb/qs#515 in case anyone wants to discuss and implement it. |
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 defaultparameterLimit
of 1000 which then it won't parse any more value after. It seems inexpress body parser
, this issue also exists but it returns an error if it is over a limit. https://github.com/expressjs/body-parser#parameterlimitI 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
it should either return an error(similar to body parser), because I think this shouldn't silently remove value without alerting the engineer
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.
The text was updated successfully, but these errors were encountered: