-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: resist pollution #106
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
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.
Looking good to me so far.
I think a test for the specific case @bakkot points out would be valuable.
Is it ok to set Object prototype properties in tests? (And delete them afterwards.) |
@shadowspawn this seems reasonable to me in our own |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
This is starting to look pretty robust 🙌
Realizing this is still in a draft state, the only thing I think would be helpful is having some brief JSDocs for the checkOptionUsage
and storeOption
utils. I found myself jumping around a bit to understand the shortOrLong
param for checkOptionUsage
was the raw option to use for the unknown option error and the values
param for storeOption
was a reference to the result.values
object. These callouts a very non-blocking and think your WIP is looking great!
Good suggestion. The long argument lists and few subtle arguments (like |
Addressed feedback. Added tests. Moved out of Draft. Thanks for the early comments reviewers, much appreciated commenting on the draft! |
Hopefully we do not cause prototype pollution, but we have two patterns which are affected by existing prototype pollution:
optionConfig.type
Closes: #104
Refactor:
??
and null prototypesBefore:
After: