Hacker News Reader

Comment by mmsc | Hacker News Reader

Note however, that proposal does not cover some other types of prototype pollution, such as:

  > let config = {};
  > Object.assign(config, JSON.parse('{"__proto__": {"isAdmin": true}}'));
  console.log({}.isAdmin); // true!

or:

  > console.log({}['constructor'] ? {}['constructor']('THIS MUST NOT BE EXPOSED') :  'pub')
  [String: 'THIS MUST NOT BE EXPOSED']

View on HN

For the first case, it doesn't work because Object.assign() does not copy the prototype or non-enumerable properties, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Refe...

  > let config = {};
  > Object.assign(config, JSON.parse('{"__proto__": {"isAdmin": true}}'));
  console.log({}.isAdmin); // undefined

That being said, it _will_ happen if you use your own merge() function like the TC-39 proposal demonstrates, but its because you are using the [] syntax to implement it which can affect __proto__

Side note, JSON.parse() also doesn't let you set the actual prototype:

  > JSON.parse('{"__proto__": {"isAdmin": true}}')
  { ['__proto__']: { isAdmin: true } }
  > JSON.parse('{"__proto__": {"isAdmin": true}}').isAdmin
  undefined

A normal JS object can do it, but of course that isn't attacker controlled unless you are using `eval()`, in which case the battle is lost anyway.

  > {"__proto__": {"isAdmin": true}}.isAdmin
  true

But even if JSON itself doesn't set the actual prototype, combining it with a user-written merge() function that copies __proto__ will indeed pollute.

  > Object.entries(JSON.parse('{"__proto__": {"isAdmin": true}}')).reduce((o, [k, v]) => o[k] = v, {}).isAdmin
  true

That was a typo, yeah. It should be

  console.log(config.isAdmin); // true!

Reply on news.ycombinator.com