Robustness: rework isConstructorOrProto

[shadowspawn]

The isConstructorOrProto fix does not look right for the v0.2.x branch. I looked at various commits and suspect the back port of the fix itself went somewhat awry. This PR makes the code match the mainline.

Here are the relevant lines of code from the main branch and from the original commit and adapted commit adding isConstructorOrProto.

Main line

• minimist/index.js

  Lines 19 to 21 in 6901ee2

  	function isConstructorOrProto(obj, key) {
  	return (key === 'constructor' && typeof obj[key] === 'function') || key === '__proto__';
  	}

• minimist/index.js

  Line 85 in 6901ee2

  if (isConstructorOrProto(o, key)) { return; }

• minimist/index.js

  Line 99 in 6901ee2

  if (isConstructorOrProto(o, lastKey)) { return; }

Original change

• minimist/index.js

  Lines 247 to 249 in c2b9819

  	function isConstructorOrProto (obj, key) {
  	return key === 'constructor' && typeof obj[key] === 'function' || key === '__proto__';
  	}

• minimist/index.js

  Line 73 in c2b9819

  if (isConstructorOrProto(o, key)) return;

• minimist/index.js

  Line 82 in c2b9819

  if (isConstructorOrProto(o, key)) return;

Adapted change (suspect)

• minimist/index.js

  Lines 9 to 11 in ef9153f

  	function isConstructorOrProto(obj, key) {
  	return key === 'constructor' && (typeof obj[key] === 'function' || key === '__proto__');
  	}

• minimist/index.js

  Lines 28 to 30 in ef9153f

  	if (key === '__proto__' || isConstructorOrProto(o, key)) {
  	return;
  	}

• minimist/index.js

  Line 44 in ef9153f

  if (key === '__proto__') { return; }

[codecov-commenter]

Codecov Report

Merging #24 (3dbebff) into v0.2.x (c0b2661) will not change coverage.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           v0.2.x      #24   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files           1        1           
  Lines         138      138           
  Branches       60       60           
=======================================
  Hits          136      136           
  Misses          2        2           

Impacted Files	Coverage Δ
index.js	98.55% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ljharb]

Since this fails to handle constructor properly, could we add a failing test that would have caught this? Presumably the default branch lacks the same test (it'll get pulled in when i merge in the backport).

[ljharb]

woof, good catch

[ljharb]

at least this wasn't wrong for "proto", but now it's better :-)

[shadowspawn]

I'll look at adding tests for constructor, and the last key. (It took me a while to untangle the code, but I hopefully learnt enough on the way to write the tests now!)

[shadowspawn]

There are separate checks against the leading keys in a dotted option name than against the last key. If the checks for __proto__ or constructor are wrong in the checks of the last key then it won't lead to prototype pollution as such, but will lead to less consistent behaviour between --a.constructor.prototype.b and --a.constructor.

The pollution checks on the v0.2.x branch missed the constructor check on the last key in the dotted option name. So again this does not allow prototype pollution, but does mean there is different behaviour between the main branch and the v0.2.x branch.

This PR adds a test which fails if the constructor check is missing for the last key. (And a matching test for __proto__.)

The tests were run against the previous code in #25 to show the failure.

[shadowspawn]

To be clear, after testing I do not think this PR is a security fix. Just a tidy-up to make the dotted option key handling more consistent.