-
-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Robustness: rework isConstructorOrProto #24
Robustness: rework isConstructorOrProto #24
Conversation
- modify implementation of isConstructorOrProto to match main branch - call isConstructorOrProto on last key too
Codecov Report
📣 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -7,7 +7,7 @@ function isNumber(x) { | |||
} | |||
|
|||
function isConstructorOrProto(obj, key) { | |||
return key === 'constructor' && (typeof obj[key] === 'function' || key === '__proto__'); | |||
return (key === 'constructor' && typeof obj[key] === 'function') || key === '__proto__'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woof, good catch
if (key === '__proto__' || isConstructorOrProto(o, key)) { | ||
if (isConstructorOrProto(o, key)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least this wasn't wrong for "proto", but now it's better :-)
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!) |
There are separate checks against the leading keys in a dotted option name than against the last key. If the checks for The pollution checks on the v0.2.x branch missed the This PR adds a test which fails if the The tests were run against the previous code in #25 to show the failure. |
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. |
15bf9f3
to
3dbebff
Compare
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
minimist/index.js
Line 85 in 6901ee2
minimist/index.js
Line 99 in 6901ee2
Original change
minimist/index.js
Lines 247 to 249 in c2b9819
minimist/index.js
Line 73 in c2b9819
minimist/index.js
Line 82 in c2b9819
Adapted change (suspect)
minimist/index.js
Lines 9 to 11 in ef9153f
minimist/index.js
Lines 28 to 30 in ef9153f
minimist/index.js
Line 44 in ef9153f