make returning plain objects and allowing prototype overwriting properties optional

[nlf]

this is a short term solution before 5.0.0 which will change everything. Essentially the default behavior will match the 2.x releases, with the difference that you may set the plainObjects option to true to get the behavior from the 3.x releases.

If you don't want plain objects but also don't want to lose keys that would overwrite prototype properties, you can set the prefixPrototypes option to true and those keys will be prefixed with an underscore '_' and passed along. It's not perfect, but at least the keys aren't silently ignored.

Would appreciate your feedback on this one @hueniverse @dougwilson

[dougwilson]

To me, this seems to make the situation potentially worse, especially for user-confusion, because of the following not being the same:

$ node -pe 'JSON.parse('"'"'{"hasOwnProperty":"yes"}'"'"')'
{ hasOwnProperty: 'yes' }

$ node -pe 'require("querystring").parse("hasOwnProperty=yes")'
{ hasOwnProperty: 'yes' }

$ node -pe 'require("qs").parse("hasOwnProperty=yes")'
{}

Can there be an option to use the default prototype and just overwrite properties, even if it's something you have to opt into?

[dougwilson]

lol, please ignore my above comment, as it was technically only based off reading the source code. I see there is prefixPrototypes, my bad!

[nlf]

prefixPrototypes is what allows things that would overwrite a prototype property at all, by prefixing them with a string. if it's set to false then prototypes would be ignored like they were in 2.x. I can see your point about confusion though, so I can get rid of that option and add an allowPrototypes instead that will let people shoot themselves in the foot if they so wish.

[dougwilson]

So I had made that comment in haste :) Let me really look through it to see what real comments I have :)

[hueniverse]

Looks fine to me. Can't make everyone happy and I really don't care about prototype names in payload. This is JS. You are fucked anyway you look at it.

[dougwilson]

Ok, so this PR seems fine in general. My only real comment is that I don't think the global environment should affect the functionality of the library:

$ node -pe 'require("qs").parse("hide=1")'
{ hide: '1' }

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});require("qs").parse("hide=1")'
{}

JSON.parse and the built-in query string does not have this issue, though:

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});JSON.parse('"'"'{"hide":"1"}'"'"')'
{ hide: '1' }

$ node -pe 'Object.defineProperty(Object.prototype,"hide",{value:function(){},writable:true});require("querystring").parse("hide=1")'
{ hide: '1' }

[nlf]

That would be resolved by exchanging prefixPrototypes with allowPrototypes, the latter being an option to let you overwrite properties on Object.prototype.

[nlf]

Ok, changed this around so instead of optionally adding a prefix to properties that would overwrite the object prototype it instead optionally lets you shoot yourself in the foot, just like JSON.parse. It's off by default, but it's there.

[AdriVanHoudt]

This is only breaking when using { prototype: false } right?

[nlf]

this reverts the breaking change made in 3.x, so it's in itself another breaking change, though odds are if you don't use any of the prototype stuff on parsed results you'll never notice.

[AdriVanHoudt]

ok, thanks for the explanation

[hueniverse]

Might be worth to explicitly note this is a security risk.