fix: keep repeated extended query params as arrays beyond 20 values

[wwenrr]

Summary

• fix extended query parser behavior for repeated keys with >20 values
• set arrayLimit: 1000 in parseExtendedQueryString so repeated key form (a=1&a=2...) keeps returning arrays
• add regression test for 21 repeated tenancyIds values

Why

Issue #7147 reports that in 4.22.x repeated query keys switch from array to object once count exceeds 20, which is a behavior break for existing apps.

Testing

• ran test suite in Docker (node:22)
• validated req.query extended parser test including new regression case

[krzysdz]

As I wrote in #7147 (comment), this is something that, to my knowledge, has not been decided on. There are 3 potential solutions:

1. Do not fix it and guide users to configure their own "query parser" with appropriate options.
2. Follow body-parser and introduce new logic that sets arrayLimit to the number of parameters (expressjs/body-parser#689)
3. Set arrayLimit to an arbitrary higher value <= 1000. 1000 is the parameterLimit and highest value that makes sense (since qs 6.14.2 it restricts the array length to at most 1000 entries, in earlier versions (<=6.14.1) it restricted the index to 1000, so 1001 elements). This, however, also significantly raises the default limit allowing much larger sparse arrays using array[idx] notation.

This PR and #7181 (for 4.x) choose the 3rd option, but the opinions in Slack thread from January were in favour of the 1st. There was no final decision on this, as @jonchurch was asked for his opinion on this and then everyone forgot about it.

This approach probably does not open any app for DoS, because creating arrays up to 1000 elements has been possible before, just using repetitions. qs has quadratic time complexity when parsing arrays (both indexed and using repetitions), but for 1000 elements it's not a problem. The one potentially affected pattern that I can think of right now is

for (const [k, v] of Object.entries(req.query)) {
    if (Array.isArray(v)) {
        for (let i = 0; i < v.length; ++i) {
            // ...
        }
    }
}

This (with these changes) would in total run 1000 (parameterLimit) * 1000 (arrayLimit) inner loop iterations, compared to 1000 * 21 (with qs <= 6.14.1) or 1000 * 20 (qs >= 6.14.2) - a 50x increase.

[bjohansebas]

My two cents (I don’t have time to go deep into this right now (my plate is full)), but hopefully @expressjs/triagers @expressjs/express-collaborators can help here, is that we shouldn’t leave it as is. We should come up with a solution that isn’t a breaking change, because this issue is an unintentional breaking change and it can break applications without people realizing it.

[GroophyLifefor]

Hi Sebastian, I’ll run my local tests as soon as possible and see what contributions I can make.

[GroophyLifefor]

krzysdz has summarized it clearly (he/she always do it, and it makes things so much easier), so I was able to review the relevant issues and PRs and weigh the pros and cons. I will proceed with three potential solutions.

1. Keep it as it is
2. set arrayLimit to the number of parameters
3. default arrayLimit to a high(acutally max) value to make it safe

I believe the first approach is the best one because deciding on an array limit based on the number of parameters or setting it to the maximum, means directly modifying the library, and doing so is only reasonable if it benefits Express specifically. At the moment, I don’t see a direct benefit to Express from the second and third options, so I think the qs options should be left at the user level, and if an issue arises on the qs side due to these options, the user should address it themselves. In conclusion, the qs library has deemed 20 to be the appropriate default value. We can respect the qs library’s default setting.