Skip to content
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 potential prototype pollution #12

Merged
merged 1 commit into from
Feb 11, 2022
Merged

fix potential prototype pollution #12

merged 1 commit into from
Feb 11, 2022

Conversation

gfauchart
Copy link
Contributor

@gfauchart gfauchart commented Jan 27, 2022

Following up on #11, this PR aims to fix the Nexus IQ alerts.

Hopefully, it would fix this report: https://huntr.dev/bounties/1-npm-unset-value/
(the fix is heavily inspired by the code on your other repo set-values)

Let me know if you have any comments!

@gfauchart
Copy link
Contributor Author

@jonschlinkert @danez , Did you get a chance to have a look?
(sorry to use the @ I will only use this once in case you missed this PR)

@prantlf
Copy link

prantlf commented Nov 16, 2022

I wonder why you didn't dispute the claim. I admit that using unset to modify an object's prototype is suspicious, but input sanitisation is not usualy done in such low-level methods.

unset({}, '__proto__.toLocaleString')
unset(Object.prototype, 'toLocaleString')

They could complain about Object.assign, but unset-value was an easier target ;-)

Object.assign(({})['__proto__'], { toString: undefined })
Object.assign(Object.prototype, { toString: undefined })

Except for the __proto__ property, which is unlikely to be used in real-world software, you disabled constructor and prototype anywhere, which can have valid real-world use cases, for example:

obj = { constructor: { fixed: true } }
unset(obj, 'constructor.fixed')

I think that reports from Mitre, Veracode, Snyk and other companies, which base their business on reporting vulnerabilities, should be validated more rigorously. In any case, this is about your package and you can decide about its interface an implementation, hopefully freely and not after blackmailing ;-)

But strictly speaking, this was a breaking change and you released it as a patch version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants