Skip to content

Commit

Permalink
Add more safety precautions for preventing prototype pollution in mer…
Browse files Browse the repository at this point in the history
…geObjects() (was not possible before either)
  • Loading branch information
MrShoenel committed Nov 4, 2020
1 parent f3763f0 commit 61afb93
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 4 deletions.
13 changes: 9 additions & 4 deletions lib/tools/Objects.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* and nested objects of them.
*
* @param {Object} obj The Object to deep-clone
* @returns {Object} A copy of the given object.
*/
const deepCloneObject = obj => JSON.parse(JSON.stringify(obj));

Expand All @@ -19,8 +20,11 @@ const deepCloneObject = obj => JSON.parse(JSON.stringify(obj));
* properties that are actually Class-instances will be copied (i.e. it
* will point to the same instance in the merged object).
*
* @param {...Object} objects Two or more Objects to merge.
* @returns {Object} the result of the merge
* @param {...Object} objects Two or more Objects to merge. If only one
* Object is passed, it is returned as-is.
* @throws {Error} If no object is passed, needs one or more.
* @returns {Object} The result of the merge. The resulting Object will be
* created without a prototype.
*/
const mergeObjects = (...objects) => {
if (objects.length === 0) {
Expand All @@ -29,9 +33,10 @@ const mergeObjects = (...objects) => {
return objects[0];
}

const target = {};
const target = Object.create(null);

objects.reduce((prev, next) => {
// Object.keys() does not enumerate __proto__.
for (const key of Object.keys(next)) {
const nextType = Object.prototype.toString.call(next[key]);

Expand All @@ -55,7 +60,7 @@ const mergeObjects = (...objects) => {
// Check if the current value is a class instance (ctor different from Object):
try {
const proto = Object.getPrototypeOf(next[key]);
if (proto.constructor !== Object) {
if (proto === null || proto.constructor !== Object) {
prev[key] = next[key];
break;
}
Expand Down
25 changes: 25 additions & 0 deletions test/test_Tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,31 @@ describe('Tools', () => {
done();
});

it('should merge objects and prevent prototype pollution', done => {
assert.isFalse('polluted' in global);
assert.isFalse('polluted' in global.__proto__);

// Even the explicit '__proto__'-property is ignored by Object.keys()!
assert.deepStrictEqual(Object.keys({ __proto__: 41, foo: 40 }), ['foo']);
const result = mergeObjects({ '__proto__': { polluted: 42 } }, { xx: 43 });

expect(result).to.deep.equal({ xx: 43 });

assert.isFalse('polluted' in global);
assert.isFalse('polluted' in global.__proto__);

done();
});

it('should handle the case of Objects without prototype', done => {
const bla = Object.create(null);
const result = mergeObjects({}, { bla });

assert.strictEqual(result.bla, bla);

done();
});

it('should also merge custom classes', done => {
class Xx123 {
get [Symbol.toStringTag]() {
Expand Down

0 comments on commit 61afb93

Please sign in to comment.