minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

Robustness: rework isConstructorOrProto #24

Closed shadowspawn closed 1 year ago

shadowspawn commented 1 year ago

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

Original change

Adapted change (suspect)

codecov-commenter commented 1 year ago

Codecov Report

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

:mega: 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.

shadowspawn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.