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

Break loop on magical key? #22

Closed
lukeed opened this issue Jan 27, 2021 · 2 comments
Closed

Break loop on magical key? #22

lukeed opened this issue Jan 27, 2021 · 2 comments

Comments

@lukeed
Copy link
Owner

lukeed commented Jan 27, 2021

Right now, as of 2.1.0, if a key named __proto__, constructor, or prototype is found, that assignment is skipped: https://github.com/lukeed/dset/blob/master/src/index.js#L6

But, in preparing for the 3.0 version, I'm left wondering if those keys should break the loop entirely?
The reasoning is that if those keys are showing up, it's (most likely) a malicious call in the first place, so we might as well throw away the entire operation.

Without breaking, something like this is still possible:

let user = { age: 123 };
dset(user, '__proto__.isAdmin', true);
//=> skip "__proto__" key
//=> assign "isAdmin" key to root object

console.log(user);
//=> { age: 123, isAdmin: true }

While still mutated, this is still considered safe because only the given user object is affect. New objects (or new User objects) are not polluted – aka, do not inherit – a new isAdmin = true base property.

The above is effectively the same thing as this:

let user = { age: 123 };

dset(user, 'isAdmin', true);
//=> assign "isAdmin" key to root object

console.log(user);
//=> { age: 123, isAdmin: true }

Breaking on "__proto__" key (and the others) would change the 1st snippet such that nothing changes on the user object. Its final value would be { age: 123 } instead of the current { age: 123, isAdmin: true }.

@maraisr
Copy link
Contributor

maraisr commented Jan 27, 2021

Just wanted to voice my opinion of this matter;

To me this feels like the right approach, meaning that dset(obj, 'a.__proto__.x'. 123) to produce { a: {} }, rather than hoist. Meaning we ignore all property south of a restricted word.

@lukeed
Copy link
Owner Author

lukeed commented Jan 27, 2021

FWIW, it looks like lodash/set does something equivalent to "breaking the loop" when they encounter a magical property too:

var input1 = {};
lodash(input1, '__proto__.foo', 123);
console.log(input1);  // => {}
// (dset currently) { foo: 123 }

var input2 = {};
lodash(input2, 'a.prototype.foo', 123);
console.log(input2);  // => {a: {} }
// (dset currently) {a: {foo: 123 }}

lukeed added a commit that referenced this issue Jan 28, 2021
- Verified values w/ lodash
- Closes #22
@lukeed lukeed mentioned this issue Jan 28, 2021
@lukeed lukeed closed this as completed in 0a11c8a Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants