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

Feature/set #2961

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature/set #2961

wants to merge 5 commits into from

Conversation

AzatCoder
Copy link

Hi there!
Let's add function set to underscore.
#2948

Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing!

I want to merge your branch, but it needs a bit more work. I suggest you work in the following order:

  • I requested a change in the tests (last comment). Please make that change before changing the logic and verify that it fails initially.
  • I pointed out three logical mistakes in my comments below: isArray/isObject in _set.js:14, isNumber in _set.js:16 and _.toPath in set.js:8. Before addressing them, please add tests that expose them (i.e., that fail initially).
  • Your code is currently vulnerable to prototype pollution. Add a test that demonstrates this.
  • Make the edited tests pass and address the remainder of my comments, possibly taking inspiration from the code I suggested in Possibility to update a nested object's property in a non-mutable way #2948.
  • Add an entry to the index.html to document the new set function. (I know the CONTRIBUTING.md states you shouldn't update the documentation. That is meant to refer only to the annotated sources and the changelog, both of which are updated on release. You are welcome to clarify that part of the CONTRIBUTING.md if you feel like it.)

modules/_set.js Outdated
import isObject from './isObject.js';


export default function set (obj, path, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three changes needed for this internal function as a whole:

  • It is not used anywhere except in the public set, so it does not need to be in a separate module (compare isEqual). Please move it into ./set.js. You can call it deepSet (cf. deepGet).
  • Please add a top comment with a description, like all other functions.
  • (minor) The general convention in Underscore is to have no space between the function name and the parameter list, so please remove it.
Suggested change
export default function set (obj, path, value) {
export default function set(obj, path, value) {

modules/_set.js Outdated


export default function set (obj, path, value) {
var key = String(path[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think calling String adds any value here.

modules/_set.js Outdated
return;
}

if (!isArray(obj[key]) || !isObject(obj[key])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition doesn't quite do what you want. Lifting the negation, you get !(isArray(obj[key]) && isObject(obj[key])). However, isArray(obj) implies isObject(obj), so this entire expression reduces to just !isArray(obj[key]). In other words, this block is executed if obj[key] is any non-array value, even if it is an object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, my bad. It should be if (!isObject(obj[key])) {...}

modules/_set.js Outdated

if (!isArray(obj[key]) || !isObject(obj[key])) {
var nextKey = path[1];
obj[key] = isNumber(nextKey) ? [] : {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNumber('123') returns false, which is not what you want in this case. You need to match nextKey against a regular expression, as I demonstrated in #2948.

modules/set.js Outdated
import _set from './_set.js';


// set the value in given path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Underscore is literate code. Please elaborate a bit and write full sentences.

modules/set.js Outdated


// set the value in given path
export default function set (obj, path, value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export default function set (obj, path, value) {
export default function set(obj, path, value) {

modules/set.js Outdated

// set the value in given path
export default function set (obj, path, value) {
if (!isObject(obj) || !isArray(path)) return obj;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, Underscore functions allow you to pass a single path component as a bare string or number instead of an array. Take path through _.toPath instead of rejecting it if it is not an array.

modules/set.js Outdated
// set the value in given path
export default function set (obj, path, value) {
if (!isObject(obj) || !isArray(path)) return obj;
if (path.length === 0) return obj;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking path.length here and then again in the internal set function. Not a big deal, but slightly inefficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't see other option better than this. I like this implementation. Please let me know if you have an idea how to improve it.

Copy link
Collaborator

@jgonggrijp jgonggrijp Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to the approach I suggested in #2948. If you do postorder assignment instead of preorder assignment, you can check for path.length === 0 only in the internal function and immediately return value if true. You can avoid lookahead beyond path[0] in that case by replacing obj instead of obj[key].

Referring again to #2948, I also realize that your code is still vulnerable to prototype pollution. You need to fix that as well.

test/objects.js Outdated
assert.strictEqual(_.set(undefined, ['some', 'path'], 'some value'), undefined);
assert.strictEqual(_.set(null, ['some', 'path'], 'some value'), null);
assert.strictEqual(_.set(12, ['some', 'path'], 'some value'), 12);
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to support old environments, so you cannot just sprinkle post-ES3 features in the tests (or the source code, for that matter). To fix, make this line conditional on feature detection:

Suggested change
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
if (typeof BigInt != 'undefined') {
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
}

You'll find similar code elsewhere in this module.

test/objects.js Outdated
Comment on lines 1279 to 1280
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this difference. Numeric keys are implicitly converted to strings. Array indices are actually strings, too. The second test should generate the same value as the first.

Suggested change
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': ['my value']});

@AzatCoder
Copy link
Author

Thank you for quick reviewing, @jgonggrijp

@coveralls
Copy link

coveralls commented Jul 22, 2022

Coverage Status

Coverage increased (+0.08%) to 95.299% when pulling b5f481a on AzatCoder:feature/set into a15d1af on jashkenas:master.

jasonvarga added a commit to statamic/cms that referenced this pull request Jul 23, 2022
e.g. you can do setFieldValue('replicator.1.foo', 'bar') to update the foo field on the second replicator set to bar

This uses the eventual underscore 'set' method from jashkenas/underscore#2961. Once it's merged we can call the method directly and not need to include these files.
Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite happy! I just think we should avoid using _.contains for this particular function, to keep the treeshakers happy, and add two more safeguards against prototype pollution.

Nearly there!

path = toPath(path);

if (!isObject(obj) || !path.length) return obj;
if (contains(path, '__proto__')) throw new Error('Prototype assignment attempted');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things. Firstly, I'm in favor of reusing our own function in principle. However, in this case the use of _.contains means that _.get will come out a bit "fatter" when people try to treeshake it. For this reason, I suggest checking the current key inside deepGet instead.

Secondly, I just realized we need to check not only for __proto__ but also for constructor and prototype. Otherwise, attackers can (for example) still pollute String methods.

// Internal function of `set`.
function deepSet(obj, path, value) {
var key = path[0];

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right place to check that key is not one of '__proto__', 'constructor' or 'prototype'.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it will mutate before throwing an error.

const obj = { value: 50 };

try {
  _.set(obj, [ 'key', '__proto__' ], {});
} catch(e) {
  console.log(e.message);  //  Prototype assignment attempted
  console.log(obj);        //  { value: 50, key: {} }  -  `obj` was mutated :(
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I realized that as well. I can think of three options (from least to most preferred, in my opinion):

  1. Stick with _.contains, accept the double iteration and the greater weight after treeshaking.
  2. Accept that obj gets somewhat corrupted. At least the prototype pollution attempt is detected and unsuccessful.
  3. Switch to postorder assignment as I suggested before, which has neither of these problems.

I didn't mention it because I was ready to accept option 2, but I would prefer option 3.

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