querystring module swallows `__proto__` key

[WebReflection]

If a URL contains __proto__=123 that has no meaning for any other server out there and it's handled like a regular string.

However, the querystring module swallows the key, probably trying to set the string "123" as returned object prototype.

Since every other key is set as {configurable: true, writable: true, enumerable: true, value: decodedValue} I think in the very special key in out && !hasOwnProperty.call(out, key) case the returned out object should have properties set as such:

Object.defineProperty(
  out,
  key,
  {
    configurable: true,
    enumerable: true,
    writable: true,
    value: options.decodeURIComponent(value)
});

This would grant consistency with any sort of possible dangerous key inherited, as setter or getter, through the Object.prototype.

• Version: v5.7.1
• Platform: Linux archibold 4.4.3-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Fri Feb 26 15:09:29 CET 2016 x86_64 GNU/Linux (it's just ArchLinux)
• Subsystem: querystring

Best Regards

[MylesBorins]

@WebReflection thanks for reporting this!

It would appear we are already testing for similar edge cases in our tests, specifically for hasOwnProperty, valueOf, and __defineGetter__

Adding __proto__ to the list causes an unexpected assertion to be thrown

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: 'hasOwnProperty=x&toString=foo&valueOf=bar&__defineGetter__=baz&__proto__=biz' == 'hasOwnProperty=x&toString=foo&valueOf=bar&__defineGetter__=baz'
    at /Users/thealphanerd/code/node/master/test/parallel/test-querystring.js:132:10
    at Array.forEach (native)
    at Object.<anonymous> (/Users/thealphanerd/code/node/master/test/parallel/test-querystring.js:131:13)
    at Module._compile (module.js:417:34)
    at Object.Module._extensions..js (module.js:426:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:451:10)
    at startup (node.js:151:18)
    at node.js:965:3

It looks like something weird going on with the test itself... but very well could be due to weirdness of overwriting the __proto__. I'm going to dig

[MylesBorins]

So atm this seems to be associated with the way that assigned properties on objects work in js.

The __proto__ key cannot get overwritten with a non prototype (such as a string or a number) afaik. If you attempt to set __proto__ even with Object.define as suggested above it will simply be a no-op and Obj.__proto__ will still be equal to {}

So, AFAIK, we are kinda stuck on this platform being unable to parse __proto__ as is, due to the fact that assigning that as a key to any object, and not including a prototype, is essentially a no op.

There is definitely something subtle that I am missing here, and would love to see someone else sanity check this, but my current prognosis is that this is going to be a can't fix, simply due to the nature of the language 😭

[WebReflection]

Object.defineProperty wouldn't suffer any problem
On Mar 10, 2016 7:19 PM, "Myles Borins" notifications@github.com wrote:

So atm this seems to be associated with the way that assigned properties
on objects work in js.

The proto key cannot get overwritten with a non prototype (such as a
string or a number) afaik. If you attempt to set proto even with
Object.define as suggested above it will simply be a no-op and
Obj.proto will still be equal to {}

So, AFAIK, we are kinda stuck on this platform being unable to parse
proto as is, due to the fact that assigning that as a key to any
object, and not including a prototype, is essentially a no op.

There is definitely something subtle that I am missing here, and would
love to see someone else sanity check this, but my current prognosis is
that this is going to be a can't fix, simply due to the nature of the
language 😭

—
Reply to this email directly or view it on GitHub
#5642 (comment).

[MylesBorins]

@WebReflection it just struck me that I needed to be passing __proto__ as a string.

Womp.

So now here is my next thought. We are now in a place where the only edge case where we would need to do this is with __proto__. How can we guard against this edge case without having to do a check on every instance

[Trott]

Possibly related, especially the discussion around performance: #728

[WebReflection]

You already do the hasOwnProperty check .... So: if key in obj &&
!hasOwnProperty.call(obj, key) use defineProperty
On Mar 10, 2016 8:06 PM, "Rich Trott" notifications@github.com wrote:

Possibly related, especially the discussion around performance: #728
#728

—
Reply to this email directly or view it on GitHub
#5642 (comment).

[WebReflection]

And that solves all edge cases. Sorry I'm typing with my phone, cheers
On Mar 10, 2016 8:13 PM, "Andrea Giammarchi" andrea.giammarchi@gmail.com
wrote:

You already do the hasOwnProperty check .... So: if key in obj &&
!hasOwnProperty.call(obj, key) use defineProperty
On Mar 10, 2016 8:06 PM, "Rich Trott" notifications@github.com wrote:

Possibly related, especially the discussion around performance: #728
#728

—
Reply to this email directly or view it on GitHub
#5642 (comment).

[MylesBorins]

@WebReflection where are you seeing that afaik we use a key array lookup https://github.com/nodejs/node/blob/master/lib/querystring.js#L270

[WebReflection]

I haven't checked your implementation , I was giving a hint. You have
apparently a not so good algo there , perf wise, I'll have a look when I
won't be on a phone. Cheers
On Mar 10, 2016 9:17 PM, "Myles Borins" notifications@github.com wrote:

@WebReflection https://github.com/WebReflection where are you seeing
that afaik we use a key array lookup
https://github.com/nodejs/node/blob/master/lib/querystring.js#L270

—
Reply to this email directly or view it on GitHub
#5642 (comment).

[WebReflection]

midnight here, so I won't probably answer any other question but you can check the PR I've just pushed and verified via the test suite provided by this repo.

Hope it's clear what I meant when I've said key in obj is the red herring you want to eventually double check without trusting own keys array only.