Fix Prototype pollution

[d3v53c]

📊 Metadata *

Please enter the direct URL for this bounty on huntr.dev. This is compulsory and will help us process your bounty submission quicker.

Bounty URL: https://www.huntr.dev/bounties/1-npm-yup

⚙️ Description *

Prototype Pollution refers to the ability to inject properties into existing JavaScript language construct prototypes, such as objects.
JavaScript allows all Object attributes to be altered, including their magical attributes such as proto, constructor and prototype.
An attacker manipulates these attributes to overwrite, or pollute, a JavaScript application object prototype of the base object by injecting other values.
Properties on the Object.prototype are then inherited by all the JavaScript objects through the prototype chain.

💻 Technical Description *

Fixed by avoiding setting magical attributes.

🐛 Proof of Concept (PoC) *

// poc.js
let yup = require('yup');
const payload = JSON.parse('{"proto":{"polluted":"Yes! Its Polluted"}}');
yup.setLocale(payload);
console.log({}.polluted)

🔥 Proof of Fix (PoF) *

Before:

After:

👍 User Acceptance Testing (UAT)

After fix functionality is unaffected

[JamieSlome]

@jquense - one of our users has proposed this fix. We could either throw an exception here or we could skip the iteration, what are your thoughts?

Cheers! 🍰

[JamieSlome]

@d3v53c - an alternative fix could be to continue the forEach loop instead of throwing an error?

Thoughts? 🍰

[d3v53c]

@d3v53c - an alternative fix could be to continue the forEach loop instead of throwing an error?

Thoughts? 🍰

Yeah, I agree.

[JamieSlome]

@d3v53c - also, I think we would only need to skip the proto key as this affects the global scope which in this instance could have the significant impact, perhaps we only run the if statement on __proto__?

[JamieSlome]

@d3v53c - also, can we implement a unit test in test/setLocale.js to show that the global is proto is not affected when the POC provided in the disclosure is executed?

Then we are good to merge!

Cheers! 🍰

[d3v53c]

@d3v53c - also, can we implement a unit test in test/setLocale.js to show that the global is proto is not affected when the POC provided in the disclosure is executed?

Then we are good to merge!

Cheers! 🍰

Keeping the check for '__ proto __' and 'proto' as per @JamieSlome . Added a unit test for the poc provided. Please let me know if I'm missing anything and have a look on the unit test case.

[JamieSlome]

Please can you review the requested changes?

Cheers! 🍰

[d3v53c]

Commited the changes.

[ljharb]

Note that the actual test suite is broken on this project, so your test won't run if you do npm test.

[ljharb]

See #2 for an alternative fix that also fixes the entire test suite.

[JamieSlome]

@d3v53c - I tried running the tests for this fix, and @ljharb is correct in pointing out that they are not runnable.

In this instance, we will favour the alternative fix (#2) that addresses the unit test fix.

Thanks for your attempt @d3v53c! 🍰

[huntr-helper]

Sorry d3v53c, we enjoyed reviewing your fix but it has not been selected this time. If this bounty has not been closed, please feel free to try again with a new pull request!

We appreciate your effort and look forward to reviewing more of your fixes in the future! 🔨😎

Come join us on Discord