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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions index.html
Expand Up @@ -392,6 +392,7 @@
<li data-name="toPath">- <a href="#toPath">toPath</a></li>
<li data-name="has">- <a href="#has">has</a></li>
<li data-name="get">- <a href="#get">get</a></li>
<li data-name="set">- <a href="#set">set</a></li>
<li data-name="property">- <a href="#property">property</a></li>
<li data-name="propertyOf">- <a href="#propertyOf">propertyOf</a></li>
<li data-name="matcher">- <a href="#matcher">matcher</a></li>
Expand Down Expand Up @@ -1951,6 +1952,20 @@ <h2 id="objects">Object Functions</h2>
=&gt; 2
_.get({a: 10}, 'b', 100);
=&gt; 100
</pre>

<p id="set">
<b class="header">set</b><code>_.set(object, path, value)</code><br>
Sets the value on <b>path</b> of <b>object</b>. <b>path</b> may be
specified as a simple key, or as an array of object keys or array
indexes. If the property does not exist, it will be created.
Returns mutated <b>object</b>.
</p>
<pre>
_.set({a: 10}, 'a', 50);
=&gt; {a: 50}
_.set({a: 10}, ['b', 0, 'value'], 50);
=&gt; {a: 10, b: [{value: 50}]}
</pre>

<p id="has">
Expand Down
1 change: 1 addition & 0 deletions modules/index.js
Expand Up @@ -62,6 +62,7 @@ export { default as tap } from './tap.js';
export { default as get } from './get.js';
export { default as has } from './has.js';
export { default as mapObject } from './mapObject.js';
export { default as set } from './set.js';

// Utility Functions
// -----------------
Expand Down
37 changes: 37 additions & 0 deletions modules/set.js
@@ -0,0 +1,37 @@
import isObject from './isObject.js';
import toPath from './_toPath.js';
import contains from './contains.js';


var arrayIndex = /^\d+$/;

// 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.

if (path.length === 1) {
obj[key] = value;
return;
}

if (!isObject(obj[key])) {
var nextKey = path[1];
obj[key] = arrayIndex.test(nextKey) ? [] : {};
}

return deepSet(obj[key], path.slice(1), value);
}

// Set the value on `path` of `object`.
// If any property in `path` does not exist it will be created.
// Returns mutated object (`obj`).
export default function set(obj, path, value) {
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.


deepSet(obj, path, value);

return obj;
}
35 changes: 35 additions & 0 deletions test/objects.js
Expand Up @@ -1255,4 +1255,39 @@
assert.deepEqual(_.mapObject(protoObj, _.identity), {a: 1}, 'ignore inherited values from prototypes');

});


QUnit.test('set', function(assert) {
// returns first argument if first argument is primitive type
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('string', ['some', 'path'], 'some value'), 'string');
assert.strictEqual(_.set(true, ['some', 'path'], 'some value'), true);
if (typeof BigInt != 'undefined') {
assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));
}

// returns the same object if `path` is empty
assert.deepEqual(_.set({random: {object: true}}, [], 'some value'), {random: {object: true}});

assert.throws(
function impossibleSetProto() {
return _.set({obj: {}}, ['my', '__proto__', 'path'], 'my prototype');
},
'throws an exception if we want to change `__proto__`'
);

assert.deepEqual(_.set({}, 'path', 'my value'), {path: 'my value'});
assert.deepEqual(_.set({}, 123, 'my value'), {123: 'my value'});
assert.deepEqual(_.set({x: {l: 10}}, ['x', 'l'], 'changed'), {x: {l: 'changed'}});
assert.deepEqual(_.set({x: 10}, ['my', 'path'], 'my value'), {x: 10, my: {path: 'my value'}});
assert.deepEqual(_.set({x: 10}, ['my', 0, 'path'], 'my value'), {x: 10, my: [{path: 'my value'}]});
assert.deepEqual(_.set({x: 10}, [0], 'my value'), {x: 10, '0': 'my value'});
assert.deepEqual(_.set({x: 10}, [0, 1], 'my value'), {x: 10, '0': [undefined, 'my value']});
assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
// #2961
assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': ['my value']});
assert.deepEqual(_.set({x: 10}, [0, 'my'], 'my value'), {x: 10, '0': {my: 'my value'}});
});
}());
182 changes: 108 additions & 74 deletions underscore-esm.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion underscore-esm.js.map

Large diffs are not rendered by default.