Query string array parsing behavior changed in version 6.14.1

[francescorivola]

Follow test is passing in version 6.14.0, but it fails in 6.14.1.

import { deepEqual } from "node:assert/strict";
import { describe, it } from "node:test";
import qs from "qs";

describe("qs module", function () {
  it("should parse query strings as an array", function () {
    const values = Array.from({ length: 21 }, (_, index) => "value" + index);
    const queryString = values.map((id) => `id=${id}`).join("&");
    const parsed = qs.parse(queryString);
    deepEqual(parsed.id, values);
  });
});

Likely related to 3086902

After upgrade due to an npm audit report we have observed test failures. Our express application is allowing up to 50 ids in an endpoint query string param. I am not sure if was intended but TBH I think it could be dangerous introducing this behavior change as part of a patch version.

Please, let me know if you need any other information, glad to help.

Thank you very much.

[ljharb]

It was introduced as a CVE fix - meaning, your current setup should not have worked in the first place, and on v6.14.0 your application is vulnerable. If you want up to 50, then pass the arrayLimit option set to 50 in the parse call, and stick with v6.14.1.

[francescorivola]

Hi @ljharb. Thank you very much for your clarification and your time.

[kroleg]

But qs is usually a nested dependency so there is no way to override arrayLimit because qs.parse is called somewhere in third party code. Also there is no way to change defaults for qs and even if there was one different libs can depend on different versions of qs so there is no easy way to set defaults for all.

I see 2 possible solutions:

1. Raise default from 20 to 100 or even bigger (original commit doesn't have any justification for value). Not a solid solution because there might still be people who need bigger value.
2. (solid) Add a way to override it globally via env var (making sure it works in non-node.js runtimes or at least doesn't fail)

  function getEnvVar (name) {
      try {
          // Node.js / Bun
          if (typeof process !== 'undefined' && process.env) {
              return process.env[name];
          }
          // Deno
          if (typeof Deno !== 'undefined' && Deno.env && typeof Deno.env.get === 'function') {
              return Deno.env.get(name);
          }
      } catch (e) {
          // Deno throws without --allow-env permission, ignore
          // can probably log a warning
      }
      return undefined;
  }

  function parseEnvInt (val, fallback) {
      if (val === undefined || val === null || val === '') {
          return fallback;
      }
      var parsed = parseInt(val, 10);
      return isNaN(parsed) ? fallback : parsed;
  };

  var defaults = {
      arrayLimit: parseEnvInt(getEnvVar('QS_ARRAY_LIMIT'), 20),
      depth: parseEnvInt(getEnvVar('QS_DEPTH'), 5),
      parameterLimit: parseEnvInt(getEnvVar('QS_PARAMETER_LIMIT'), 1000),
      // ... other options use hardcoded defaults
  };

So it will be used like QS_ARRAY_LIMIT=100 node app.js

I can provide a PR for solution 2

[ljharb]

I don't think an env var makes much sense, and if something is using qs transitively and not providing you with a means to configure it, then that's something you should file an issue on the intervening project about.

[tolgap]

The decision of setting arrayLimit to 20 by default is absolutely a breaking change.

If it was set to 25, it wouldn't have been a breaking change for us. But maybe for someone else it would have been.

If this was the solution for DOS attacks, why go as low as 20? There are many nodejs apps that use query params for 25/50/100 items per page.

[ljharb]

The issue isn't the specific number - it's that this was a vulnerability. The array limit was always meant to protect you here.

20 is the existing default, and it probably was pretty arbitrary when first selected.

To be clear: "this version broke something for me" is NOT the same as "this version is a breaking change". This is not a breaking change; it's a bug fix. If something broke, it was because you were incorrectly relying on the lack of arrayLimit protection.

[kroleg]

So i investigated this vulnerability and it seems like it might not cause a DoS at all.

If you get qs 6.14.0 and run slightly modified (added qs version) POC code from there:

const qs = require('qs');

console.log('qs version:', require('qs/package.json').version);
console.log('');

const attack = 'a[]=' + Array(10000).fill('x').join('&a[]=');
const result = qs.parse(attack, { arrayLimit: 100 });
console.log(result.a.length);  // Output: 10000 (should be max 100)

it outputs:

code/test-qs-dos $ node poc.js
qs version: 6.14.0

1000

So arrayLimit is bypassed, yes. But parameterLimit still kicks in and prevents array from growing to 10000.

Am i missing something?

[kroleg]

So if above is correct then arrayLimit fix can be declared as breaking change and reverted. Then patched in 7.x as your point below is no longer valid

If I agreed it was a breaking change, then I would be compelled to revert it, and publish a new major, and tons of people on v6 would be left vulnerable. (source)

[kroleg]

Additionally for unindexed arrays like a[]=1&a[]=2&...&a[]=10000 with applied arrayLimit resulting object would use similar (or slightly more) memory as array, so even without parameterLimit enabling arrayLimit for unindexed arrays does nothing as protection against DoS. That;s probably why it wasn't implemented back then