Change `arrayLimit` default to 0

[yanickrochon]

Here are two tests :

var req = 'items[20][id]=1';

console.log(JSON.stringify(Qs.parse(req), null, 2));
// {
//   "items": [
//     {
//       "id": "1"
//     }
//   ]
// }

var req = 'items[21][id]=1';

console.log(JSON.stringify(Qs.parse(req), null, 2));
// {
//   "items": {
//     "21": {
//       "id": "1"
//     }
//   }
// }

... why?

[yanickrochon]

Nevermind, I just saw the arrayLimit option.

[wearhere]

I am curious though, why? This is a severe gotcha when using request and express, since by default request uses qs to serialize querystrings using the indices format, and express uses qs to parse querystrings: this means that the behavior of your route handler changes depending on the length of the input.

Test case: run this script and reload http://localhost:5010/request a bunch of times, observing the response changing.

var _ = require('underscore');
var express = require('express');
var request = require('request');

var app = express();

var PORT = 5010;

app.get('/request', function(req, res) {
  request.get(`http://localhost:${PORT}/respond`, {
    qs: {
      // Produce an array of 1-100 strings.
      foo: _.range(_.random(1, 100)).map(String)
    }
  }).pipe(res);
});

app.get('/respond', function(req, res) {
  // Sometimes 'Array', sometimes 'Object'
  res.send(req.query.foo.constructor.name);
});

app.listen(PORT);

[ljharb]

@wearhere my guess (it was from before i took over) primarily performance. If there isn't a limit, then someone could DOS your server by sending a very large request.

[wearhere]

Oh, it would make sense to me if the array were truncated at some point.

But isn't the array completely parsed at present? It just happens to be completely parsed as the keys of an object once it becomes sufficiently large.

[yanickrochon]

I believe this is to prevent someone sending foo[999999999999]: LOL and produce an array with 100M elements :stuck_out_tongue:

But then again, having everything be objects instead of arrays would be fine as well, and would be more consistent, relying on the server's max request size as limit.

Also, I expected foo[21]: bar to be { 21: 'bar' }, not [ 'bar' ].

[wearhere]

I believe this is to prevent someone sending foo[999999999999]: LOL

Ahh, as opposed to, I had been thinking, foo[0]=LOL&foo[1]=LOL&…foo[999999999999]=LOL.

having everything be objects instead of arrays would be fine as well, and would be more consistent, relying on the server's max request size as limit.

That sounds great to me. There are two other ways to serialize arrays (foo[]=0&foo[]=1 and foo=0&foo=1), so it'd make perfect sense to reserve foo[0]=0&foo[1]=1 for serializing objects, especially since that would resemble the way to serialize objects with non-numeric keys (foo[a]=0&foo[b]=1).

[yanickrochon]

And that foo[bar]=LOL&foo[2]=bleh would have foo[2] = 'bleh'; anyway. Exactly.

[ljharb]

I believe you can get that behavior now by setting arrayLimit to 0?

[wearhere]

That's a good stopgap. express doesn't expose configuration for its querystring parsing, but asking them to do so is a reasonable request. (EDIT: see bottom.)

To make parsing unambiguous, I am advocating that you make the following changes, though:

• make arrayLimit: 0 the default for parsing
• make either 'brackets' or 'repeat' the default arrayFormat

thus the indices format would come to exclusively represent objects.

I think that this change is within your power; given that there are multiple (non-standardized) methods of parsing querystrings as arrays, it seems that this project is setting the de-facto standard. And a change in that standard would be difficult, but benefit consumers across the Node ecosystem, given that so many projects use request and express together.

---

EDIT: express does support providing a custom query string parsing function , but not configuring the options to their default invocation of qs.parse. Filed here, with the suggestion that they just adopt arrayLimit: 0 as a default.

Also filed an issue against request to use arrayFormat: 'brackets' by default.

[ljharb]

I see the argument for making arrayLimit default to zero - it means there's no "magic number" at which param parsing changes, and anyone who wants the behavior can simply set it, so even though it'd be a breaking change, it wouldn't be a hard one to recover from.

Is the argument for changing arrayFormat that it would match this parsing behavior? If so, I'd pick "brackets", since "repeat" is an abomination on the query string spec :-)

This does seem like a reasonable breaking change to make in the next version.

[wearhere]

I see the argument for making arrayLimit default to zero - it means there's no "magic number" at which param parsing changes, and anyone who wants the behavior can simply set it, so even though it'd be a breaking change, it wouldn't be a hard one to recover from.

Sounds great!

Is the argument for changing arrayFormat that it would match this parsing behavior?

Yup. Otherwise qs would no longer be internally consistent in the following sense:

var params = { a: [ 'one', 'two' ] };
expect(qs.parse(qs.stringify(params))).toEqual(params);

If so, I'd pick "brackets", since "repeat" is an abomination on the query string spec :-)

Sounds good to me (and agreed! 😄) That'd also be consistent with how $.param works, fwiw:

decodeURIComponent($.param({ a: ['one', 'two' ] })) === 'a[]=one&a[]=two'

Thanks for your consideration!

[cdimitroulas]

After banging my face against the keyboard for an hour trying to understand why my query was failing I realised that the qs library's arrayLimit option was to blame :sweat_smile:

Would it be possible to add a warning or some kind of output from the library (at least when NODE_ENV=development) when the arrayLimit is breached? That would probably prevent a lot of head-scratching and keyboard-face-smashing for people in the future.

[ljharb]

@cdimitroulas meaning, when if the array limit wasn't there, it would recurse?

We could add that, but I think it would be incredibly noisy for folks that are intentionally setting the arrayLimit option.

[cdimitroulas]

True, that's a very good point. Perhaps it could be a one-time only output?

[ljharb]

That would require qs to hold state, which wouldn't work if it was duplicated in the dep graph, and also would increase its memory footprint.

[cdimitroulas]

Alright it doesn't seem like there will be a nice way to handle this so I guess the changes which will set the default arrayLimit to 0 in the future version are more than good enough :) thanks for the discussion either way

[ljharb]

hmm - 0 and false are basically equivalent here, and I suspect that any default arrayLimit value will cause confusion. I'm not convinced now that changing the default of arrayLimit is worth it.

I'll definitely change arrayFormat to brackets in the next major, of course.