qs.parse silently drops properties

[dougwilson]

I'm just opening a new issue so that old one does not get lost. This is a general continuation as a new issue from #73 (comment) and onwards.

Because qs silently discards properties, it makes this library unsuitable for any real REST API. It only works for "toy" web sites that have a very explicit list of keys in all routes. With this module, one cannot make a route that contains customer-defined properties, as they will evidently choose one that is silently dropped just because traffic happens to flow through Node.js.

The main other parsers doesn't particularly have this issue:

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

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

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

[hueniverse]

Is this a bug?

[nlf]

technically? it's a side effect of not allowing properties that match those on the object prototype. the solution is switching to a plain object for storage so we don't care about the prototype

[nlf]

labeled this as a breaking change due to the returned objects being a plain object now

[hueniverse]

This might not have been the right solution. So now these objects fail in places where the result is being treated as an object with a default prototype. I think a better approach would be to add a prefix to the keys instead when overlapping with prototype methods and have some logic if the prefixed version is also present (e.g. some kind of escaping).

[dougwilson]

I still think it should work like JSON.parse, which uses the default prototype and doesn't have any dropping issue. I tried to update body-parser to 3.x but encountered lots of user push back. Same for Express req.query.

[nlf]

JSON.parse does have overriding issues

> var o = JSON.parse('{"hasOwnProperty":"toaster"}');
> o.hasOwnProperty('test');
TypeError: string is not a function
    at repl:1:3
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
    at REPLServer.Interface._line (readline.js:553:8)
    at REPLServer.Interface._ttyWrite (readline.js:830:14)
    at ReadStream.onkeypress (readline.js:109:10)

Prefixing keys that would overwrite properties from the prototype would work, but then it's kind of awkward to access them since the consumer would have to be aware that they'll be prefixed

[dougwilson]

This issue I opened is not about overriding issues; it is about dropping properties.

[nlf]

Right, but one of the main concerns of this module was to resolve the security concerns present in the original, one of which being that it's possible to override object prototype properties. I can't sacrifice one to allow the other, so we need to figure out some way to accommodate both.

[dougwilson]

Regardless, I am indifferent about the module's direction :) I need W3C parsing compliance and with the closure of #63 I will simply be moving to a different module rather than 3.0 (especially with some massive Express community blow back when I tried to update this module to 3.0 because of the null prototype issue).

[nlf]

Yeah, given all the bike shedding and random issues, I'm very seriously considering dropping all the extraneous crap and going back to #63 instead. This module has become a bit of a nightmare to maintain.