More pollution protection?

[shadowspawn]

Prompted by @aaronccasanova in https://github.com/pkgjs/parseargs/pull/74#discussion_r848765003, I started thinking through related pollution scenarios (https://github.com/pkgjs/parseargs/pull/74#discussion_r848863123).

If this is something we care about, we have room for improvement. 🙊

const { parseArgs } = require('@pkgjs/parseargs');

// Somebody trampled all over prototypes...
Object.prototype.strict = false;
Object.prototype.args = ['--pollution', 'happened'];
Object.prototype.pollution = { type: 'string'};

const knownOptions = {
    with: { short: 'w', type: 'string' },
    bool: { short: 'b', type: 'boolean' }
};

console.log(parseArgs({ options: knownOptions }));

% node pollution.js --with VALUE -b
{ values: { pollution: 'happened' }, positionals: [] }

[bcoe]

I think the bigger concern for prototype pollution is untrusted input to the CLI modifying the prototype, vs., the user having mucked with proto in their application, and this affecting our parse.

This makes me think that the bigger security concern would be if we ever supported dot properties; as @shadowspawn has mentioned.

[ljharb]

We do not need to be concerned with the user attacking themselves, I’d think.

[bakkot]

We do not need to be concerned with the user attacking themselves, I’d think.

While I agree it's not a security concern, I also think it's nice for core modules like this not to break when users start mucking about with built-ins, so +1 to being defensive from me.

[shadowspawn]

We do not need to be concerned with the user attacking themselves, I’d think.

The code modifying the prototype may not be from the user.

For interest: We jump through a lot of hoops using primordials. Is modifying prototype functions considered a significant issue because "helpful" libraries have done it in the past, including perhaps left over monkey patches?

[aaronccasanova]

I started thinking through related pollution scenarios.

I too have been trying to think through scenarios and think @bakkot caught one in comment #106 that could definitely be abused.

// node pollution.js
// node pollution.js --deploy
const { parseArgs } = require("./index");

function someNpmModule() {
  // Breaks program if author didn't explicitly set the `multiple` config
  Object.prototype.multiple = true;
}

someNpmModule();

const cli = parseArgs({
  options: {
    deploy: { type: "boolean" },
  },
});
// cli: { values: { deploy: [ true ] }, positionals: [] }

console.log("cli:", cli);

if (cli.values.deploy === true) {
  console.log("Run deploy...");
} else {
  // This always prints since `cli.values.deploy` is undefined or an array.
  console.log("Fail deploy...");
}