qs.parse generates invalid and unexpected boolean values in some special cases

[n8tz]

Hi !

We found an unexpected qs.parse output with a boolean as the value and a value as the key using a specific query string construct.

The bug seems to occur when qs.parse needs to transcribe a property that has already been created with an object as the value.

Here an simple example of this behavior :

qs.parse("foo[bar]=stuffs&foo=123")

will output :

{ foo:{ '123': true, bar: 'stuffs' } }

Hgd :)

[ljharb]

What structure would you expect from this nonsensical query string? (meaning, what do you think the proper behavior should be)

[ljharb]

Note that we do have existing tests that rely on this behavior https://github.com/ljharb/qs/blob/24c19cc7164b4a18b1c0190fa8466cd93f18ae92/test/parse.js#L604-L630

Perhaps it's just that the documentation needs to be updated?

[n8tz]

Well if qs include test to check if its bugs remains what can i do ? :D

The QS documentation say it can only output "string" values in the readme, Moreover, when we switch the definition order the result is different:

Example : qs.parse("foo=123&foo[bar]=stuffs") Return { foo : [ '123', { bar: 'stuffs' } ] }

Which seems to be the normal behaviors.

So for what i understand this is breaking at least 3 things :

• It create boolean value while it's specified it must only output "strings"
• It give differents results depending the definition order
• It use a value to create a key

So for me it should return an array with the object first and the value of the 2nd parameter in the second index.

[ljharb]

The existence of the tests suggests it's not a bug, but is intentional behavior. For any software, the docs aren't necessarily authoritative - docs can have bugs just like anything else.

So for me it should return an array with the object first and the value of the 2nd parameter in the second index.

That syntax would be foo[]=123&foo[][bar]=stuffs. I do see what you mean about reversing the order - but order matters when parsing a query string, so it's not expected to be able to reverse the order and get the same results.

To be honest, I'd expect either of these query strings to throw an exception, since they're nonsensical, but obviously that would be a breaking change.

"Specified to only output strings" is a bug in the documentation. That's not how qs has ever behaved.

I'll look further into this, but I'm still not sure how it'd be possible to do something sensible.

[n8tz]

Well, i was thinking that as qs is a converter, it should work like a mathematical iso function and logically apply the same rules to the same cases :

qs.parse("foo=123&foo=stuffs")

Give [ foo : ['123', 'stuff'] }

So this is saying "successive same keys generate array" the same way as "foo[]=123&foo[]=stuffs" I believe it should be the same if there was an object first or a string first

That's said, i understand you're point of view... moreover qs is an old lib with historical choices from the js stone age

But it is also a central library for all modern JS / node applications... this type of behavior is a risk for all developers and projects because hardly anyone can really consider them despite the time and money involved.

Maybe one day or another qs should make a new major version without this kind of historical bugs, and people who really want theses behaviors will stay on the previous major..

[ljharb]

It's not about "JS stone age", it's the way query strings work on the web and on many popular webservers across many languages.

The web can't have a major version, so these constraints can never be shed.

---

When you have "no brackets", or "only brackets on repeated things" - which is a coherent query string - qs does the expected and intuitive thing. Similarly, when a given key that's a container (foo) always represents the same kind of container (array, or object, but not both), qs also works fine.

This issue is about what qs should do with an incoherent query string. One thing we could do in a semver-major (but probably won't, because of the pain it would cause users) is throw an exception for cases like these. However, to fix it in the current major line, we'd need to define some kind of reasonable behavior for it.