fix: resist pollution

[shadowspawn]

Hopefully we do not cause prototype pollution, but we have two patterns which are affected by existing prototype pollution:

• destructuring assignment in function arguments
• accessing object properties, especially of option configuration, like optionConfig.type

Closes: #104

Refactor:

• be rigorous about getting only own properties
• use more ?? and null prototypes
• test string type value is a string (idea from earlier PR)
• split "check" and "store" into two functions (reduce max number of arguments, separation of concerns)
• some minor changes for hopefully clearer code

Before:

% git switch main
Switched to branch 'main'
Your branch is up to date with 'upstream/main'.
% npx tape test/pollution.js
TAP version 13
# when prototype has multiple then ignored
not ok 1 should be deeply equivalent
  ...
# when prototype has type then ignored
not ok 2 should throw
  ...
# when prototype has short then ignored
not ok 3 should throw
  ...
# when prototype has strict then ignored
not ok 4 should throw
  ...
# when prototype has args then ignored
not ok 5 should be falsy
  ...

1..5
# tests 5
# pass  0
# fail  5

After:

% git switch -
Switched to branch 'feature/pollution-proof'
Your branch is up to date with 'origin/feature/pollution-proof'.
% npx tape test/pollution.js
TAP version 13
# when prototype has multiple then ignored
ok 1 should be deeply equivalent
# when prototype has type then ignored
ok 2 should throw
# when prototype has short then ignored
ok 3 should throw
# when prototype has strict then ignored
ok 4 should throw
# when prototype has args then ignored
ok 5 should be falsy

1..5
# tests 5
# pass  5

# ok

[bakkot]

As mentioned here, the result object should be created as { __proto__: null } rather than {}: otherwise code like

let { values } = parseArgs({ toString: { type: 'boolean' }, args: [] });

if (values.toString) {
  // etc
}

is broken.

[bakkot]

On line 88:

  const optionConfig = hasOptionConfig ? options[longOption] : {};

should be

  const optionConfig = hasOptionConfig ? options[longOption] : { __proto__: null };

because it is used unguarded on line 113:

  if (optionConfig.multiple) {

which would be tripped up if someone did Object.prototype.multiple = true.

Though it's probably better to change the optionConfig.multiple bit to if (ObjectGetOwn(optionConfig, 'multiple'), so that it's consistent with the other hasOwn checks.

[aaronccasanova]

In addition to @bakkot's callout we should update the strict conditionals to use the get own util.

  const hasOptionConfig = ObjectHasOwn(options, longOption);
  const optionConfig = hasOptionConfig ? options[longOption] : { __proto__: null };
  if (strict) {
    if (!hasOptionConfig) {
      ...
    }
    const shortOptionErr = getOwn(optionConfig, 'short') ? `-${optionConfig.short}, ` : '';
    if (getOwn(optionConfig, 'type') === 'string' && optionValue == null) {
      ...
    }
    if (getOwn(optionConfig, 'type') === 'boolean' && optionValue != null) {
      ...
    }
  }

[shadowspawn]

Thanks for transferring comment @bakkot : https://github.com/pkgjs/parseargs/pull/106#issuecomment-1098172983

I have hidden as spun up new issue for just that: #108

[shadowspawn]

Is it ok to set Object prototype properties in tests? (And delete them afterwards.)

[bcoe]

Is it ok to set Object prototype properties in tests? (And delete them afterwards).

@shadowspawn this seems reasonable to me in our own prototype-pollution.js test file. I think I'd avoid upstreaming these into the Node.js codebase (out of concern for side-effects).

[shadowspawn]

I tried separate routines for checkOptionUsage and storeOption. I like the separation of concerns, but it wasn't a big win for number of arguments. Opinions welcome. (I am wondering about recombining.)

https://github.com/pkgjs/parseargs/pull/106/commits/6d2351a60c175b951546022d4e20fe8c07b3d6d4

[shadowspawn]

Finished full run-through adopting paranoid access wrappers to get own properties. 🤞

Covered:

• https://github.com/pkgjs/parseargs/pull/106#issuecomment-1098191114
• https://github.com/pkgjs/parseargs/pull/106#issuecomment-1098180442

[shadowspawn]

some brief JSDocs for the checkOptionUsage and storeOption utils

Good suggestion. The long argument lists and few subtle arguments (like shortOrLong) have been irking me.

[shadowspawn]

Addressed feedback. Added tests. Moved out of Draft.

Thanks for the early comments reviewers, much appreciated commenting on the draft!