Unable to parse back a stringified object when too nested

[dubzzz]

Hello 👋

I attempted to run some property based testing tests against the library using fast-check and discovered what seems to be potential bugs of the library.

Here is one of them:

const source = {"a":{"b":[{"c":{"d":{"e":["f"]}}},"g"]}};
const stringified = qs.stringify(source); // got: a%5Bb%5D%5B0%5D%5Bc%5D%5Bd%5D%5Be%5D%5B0%5D=f&a%5Bb%5D%5B1%5D=g
const parsed = qs.parse(stringified); // got: {"a":{"b":[{"c":{"d":{"e":{"[0]":"f"}}}},"g"]}}

The code that discovered this issue is:

const fc = require('fast-check');
const qs = require('qs');
const assert = require('assert');

const depthIdentifier = 'rec';
const alphaNumString = fc.stringOf(fc.mapToConstant(
  { num: 26, build: v => String.fromCharCode(v + 0x61) }, // a-Z
  { num: 26, build: v => String.fromCharCode(v + 65) },   // A-Z
  { num: 10, build: v => String.fromCharCode(v + 0x30) }, // 0-9
), {minLength: 1});

// I restricted the set of possible keys and values to only alpha numeric entries, meaning we can only have strings made of A-Za-z0-9
// and added some extra restrictions onto the set of possible keys to forbid some dubious values: toString, valueOf and number-like
const key = alphaNumString.filter(k =>String(Number(k)) !== k && k !== "constructor" && k !== "toString" && k !== "valueOf");
const value = alphaNumString;

// The builder for source
const sourceArbitrary = fc.letrec(tie => ({
  self: fc.dictionary(key, tie('value'), {minKeys: 1, depthIdentifier}),
  singleValue: fc.oneof({depthIdentifier}, value, tie('self')),
  value: fc.oneof({depthIdentifier}, tie('singleValue'), fc.array(tie('singleValue'), {minLength: 1, depthIdentifier}))
})).self;

fc.assert(fc.property(
  sourceArbitrary,
  (source) => {
    const stringified = qs.stringify(source);
    const parsed = qs.parse(stringified, {arrayLimit:Number.MAX_SAFE_INTEGER});
    assert.deepEqual(parsed, source);
  }
))

Here is a link to a playable version: https://runkit.com/dubzzz/qs-issue

[ljharb]

This isn’t a bug, this is a security feature. When parsing, you can override the default depth option (like you’ve done with arrayLimit) if you want to parse deeper structures.

[ljharb]

(separately, keys like toString etc should be fine; if your goal was to prevent those, you’d need to filter out everything on Object.prototype anyways, not just those two)

[dubzzz]

Let's close the issue, thanks for the answer