search

pkgjs / parseargs

Polyfill of `util.parseArgs()`
Apache License 2.0
121 stars 9 forks source link

security: switch to using Map during parsing #99

Closed bcoe closed 2 years ago

bcoe commented 2 years ago

Once we merge into the Node.js codebase, getting security patches out the door becomes more painful.

I've had to fix several prototype related bugs/security notifications over the years by virtue of using a POJO when parsing. Here's an example as to how this can cause weird behavior:

> const obj = {__proto__: {a: 99}}
undefined
> obj.a
99

I think it would be worth preemptively moving our implementation to using a Map, and then converting the Map into an object as a final step.

shadowspawn commented 2 years ago

The issues I looked at that were reported against Yargs and Minimist were due to supporting structured options, where --foo.bar is parsed into { foo: { bar: true } }. We are doing single level assignment in parseArgs, although we do also have arrays.

I'm not against using SafeMap, but I don't think we are exposed to the same risks.

(Previous PR using SafeMap without tests: #65)

bcoe commented 2 years ago

The issues I looked at that were reported against Yargs and Minimist were due to supporting structured options, where --foo.bar

I agree, potentially I'm being too paranoid. I wasn't sure if:

const obj = {__proto__: 99}

Is cause for a alarm? as far as I can tell the key just get dropped.

shadowspawn commented 2 years ago

There is now a test in StoreOptionValue to return early if the key is __proto__ (thanks to @bcoe).

See also #108 for low tech map, the null prototype object.

I suggest SafeMap is more churn than useful and close this issue?

bcoe commented 2 years ago

:+1: let's close this issue and concentrate on https://github.com/pkgjs/parseargs/issues/104