feat!: create result.values with null prototype

[shadowspawn]

Simple code change, with repercussions for tests and documentation.

  const result = {
    values: { __proto__: null }, // <------- null prototype
    positionals: []
  };

Closes: #108

Removed the recently added protections when storing option values in values .

First cut at documentation follows. I gave two shallow examples, just pick one? I went further than the existing reference which didn't unblock my problems. (The caveats and work-arounds could go on a canonical page, if there is somewhere appropriate. I didn't go looking for that, out of scope for my current efforts!)

---

The values object returned by parseArgs() does not prototypically inherit from the JavaScript Object to avoid name clashes with your options. This means that typical Object methods such as values.toString(), values.hasOwnProperty(), and others are not defined and will not work. If you need to convert to a full Object:

const { values } = parseArgs({ strict: false }); // values has null prototype
const obj1 = { ... values }; // obj1 has Object prototype
const obj2 = Object.assign({}, values); // obj2 has Object prototype

const result = parseArgs({ strict: false }); // result.values has null prototype
const objResult = structuredClone(result); // objResult.values has Object prototype

---

Reference documentation: https://nodejs.org/api/querystring.html#querystringparsestr-sep-eq-options

The object returned by the querystring.parse() method does not prototypically inherit from the JavaScript Object. This means that typical Object methods such as obj.toString(), obj.hasOwnProperty(), and others are not defined and will not work.

[ljharb]

Should this be accompanied by removing a bunch of defensive coding that's no longer needed when setting/accessing things off of result.values?

[shadowspawn]

Yes, but I am not assuming which lands first.

[bcoe]

I'm on the fence about this one, do we want to take away object helper methods from users, for the benefit of having less confusing behavior around edge-cases like a key called "toString"? If someone has a need for this key name, couldn't they use hasOwnProperty to handle the edge case themselves?

[ljharb]

Nobody uses object helper methods, precisely because of the risk of collision.

[bcoe]

Nobody uses object helper methods, precisely because of the risk of collision.

Argument lands for me, if we want to go this route 👍

[shadowspawn]

Nobody uses object helper methods, precisely because of the risk of collision.

This is a generalisation which is not true in my experience. I see lots of use of object helper methods. I can't know whether because of carefully reasoned balance of the convenience vs the theoretical risk, or dismissal of the risk, or ignorance.

To check my impression, I tried searched for "javascript hasownproperty" and opened the top four links. All were showing how to use the helper method. Some of the pages did also mention or demonstrate the possibility of hasownproperty being missing (e.g. null prototype) or overwritten and explain alternatives. (And this seems like the helper function mostly likely to go into the issues, given its purpose!)

Only one of the pages actually pointed away from using the helper function with a note at the top of the page:

Note: Object.hasOwn() is recommended over hasOwnProperty(), in browsers where it is supported.

One of the pages introduced an alternative in a small trailing section with :

It's highly unlikely that you will encounter such cases but it's good to be aware of the possibility.

[ljharb]

Let's say you're right, and users will be confused by this.

So? The result is hopefully that they'll be educated on how the language works, and their code will be better as a result.

The alternative is that code using parseArgs has a CVE, or worse, is exploited without a CVE. Some amount of confusion from people who misunderstand the language seems far preferable to the likely potential for security exploits ("prototype pollution" CVEs are one of the most popular flavors of the last few years in the npm ecosystem, in my experience).

[bcoe]

The alternative is that code using parseArgs has a CVE

I tend to agree, let's err on the side of caution. I really want to avoid having to cut a patch Node.js release for a sporius prototype pollution vulnerability.

[shadowspawn]

The alternative

(That was a pretty loaded comparison, but the concerns are real.)

[shadowspawn]

From an earlier discussion (#32 (comment))

@bcoe said:

I think we should create all our objects with null prototypes, Object.create(null).

and @ljharb replied:

That’s good, but isn’t sufficient to protect against prototype pollution because of the dunder proto setter on Object.prototype.

What's the issue here? Is { __proto__: null } different? Or was this that (Safe)Map is even safer, which was the other thing being discussed?

[ljharb]

Yes, it's different. Using it syntactically in an object literal is grammar, and has nothing to do with the setter, which comes into play if someone does foo.__proto__ = bar (and not foo['__proto__'] = bar, i believe)

[bakkot]

and not foo['__proto__'] = bar, i believe

The setter does also apply there. It's just a normal accessor on Object.prototype; it works like every other accessor.

[shadowspawn]

Updated original comment with notes and crack at documentation. I didn't soak up the rest of the utils page for style.

[shadowspawn]

Both the reviews were from when there was just one line of code, so bumped for re-review as much to make sure you were not misrepresented as to give you the opportunity!

[ljharb]

i'm not actually sure we should be returning here - we can just set a data property of the string "__proto__", no?

[shadowspawn]

My concern is it is fine in the values we return, but is it a potential foot-gun when the author copies it out into some full object? Or does who-knows-what? We are going to great lengths to make this as safe as we can, why would we allow it?

(I might have missed the point though. Not sure what you meant by string "proto".)

[ljharb]

It would only be a footgun if a user is relying on the (deletable) Object.prototype.__proto__ accessor. Otherwise, it'd just be a string property.

If we want to disallow it, why not throw in strict mode?

[shadowspawn]

Existing behaviour. I care enough about it to add a comment showing it is deliberate. But not to do work on it in this PR. 😄

[ljharb]

lol totally fine, we can look at it later.

[bcoe]

will wait to approve until I see edits to tests,