From 1b7e7c5f1c1db1af86526d802dec64d19cee51aa Mon Sep 17 00:00:00 2001 From: mnespor Date: Wed, 2 Oct 2019 00:31:34 -0500 Subject: [PATCH 1/6] Copy own __proto__ safely - When the src object has an own __proto__ property, avoid modifying the result object's prototype --- index.js | 19 ++++++++++++++++--- test/prototype-poisoning.js | 11 +++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 test/prototype-poisoning.js diff --git a/index.js b/index.js index 9c143c7..324d3d3 100644 --- a/index.js +++ b/index.js @@ -36,18 +36,31 @@ function getKeys(target) { return Object.keys(target).concat(getEnumerableOwnPropertySymbols(target)) } +function setProperty(target, key, value) { + if (key === '__proto__') { + Object.defineProperty(target, key, { + enumerable: true, + configurable: true, + value: value, + writable: true + }) + } else { + target[key] = value + } +} + function mergeObject(target, source, options) { var destination = {} if (options.isMergeableObject(target)) { getKeys(target).forEach(function(key) { - destination[key] = cloneUnlessOtherwiseSpecified(target[key], options) + setProperty(destination, key, cloneUnlessOtherwiseSpecified(target[key], options)) }) } getKeys(source).forEach(function(key) { if (!options.isMergeableObject(source[key]) || !target[key]) { - destination[key] = cloneUnlessOtherwiseSpecified(source[key], options) + setProperty(destination, key, cloneUnlessOtherwiseSpecified(source[key], options)) } else { - destination[key] = getMergeFunction(key, options)(target[key], source[key], options) + setProperty(destination, key, getMergeFunction(key, options)(target[key], source[key], options)) } }) return destination diff --git a/test/prototype-poisoning.js b/test/prototype-poisoning.js new file mode 100644 index 0000000..4818a99 --- /dev/null +++ b/test/prototype-poisoning.js @@ -0,0 +1,11 @@ +var merge = require('../') +var test = require('tape') + +test('merging objects with own __proto__', function(t) { + var user = {} + var malicious = JSON.parse('{ "__proto__": { "admin": true } }') + var mergedObject = merge(user, malicious) + t.ok(mergedObject.__proto__.admin, 'nested properties should be merged') + t.notOk(mergedObject.admin, 'the destination should have an unchanged prototype') + t.end() +}) \ No newline at end of file From 392700de786151758a941a71cd536ed40410cd0f Mon Sep 17 00:00:00 2001 From: mnespor Date: Wed, 2 Oct 2019 21:32:25 -0500 Subject: [PATCH 2/6] more restrictive copying - Only assign values if the target doesn't have the property, or if the target has that property as own and enumerable. - See https://github.com/TehShrike/deepmerge/pull/164#issuecomment-537687543 - Reduce the likelihood of surprises when merging non-plain objects --- index.js | 24 ++++++++++-------------- test/prototype-poisoning.js | 29 +++++++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index 324d3d3..e010010 100644 --- a/index.js +++ b/index.js @@ -36,31 +36,27 @@ function getKeys(target) { return Object.keys(target).concat(getEnumerableOwnPropertySymbols(target)) } -function setProperty(target, key, value) { - if (key === '__proto__') { - Object.defineProperty(target, key, { - enumerable: true, - configurable: true, - value: value, - writable: true - }) - } else { - target[key] = value - } +function propertyIsPlain(target, key) { + return target[key] === undefined + || (target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) } function mergeObject(target, source, options) { var destination = {} if (options.isMergeableObject(target)) { getKeys(target).forEach(function(key) { - setProperty(destination, key, cloneUnlessOtherwiseSpecified(target[key], options)) + destination[key] = cloneUnlessOtherwiseSpecified(target[key], options) }) } getKeys(source).forEach(function(key) { + if (!propertyIsPlain(target, key)) { + return + } + if (!options.isMergeableObject(source[key]) || !target[key]) { - setProperty(destination, key, cloneUnlessOtherwiseSpecified(source[key], options)) + destination[key] = cloneUnlessOtherwiseSpecified(source[key], options) } else { - setProperty(destination, key, getMergeFunction(key, options)(target[key], source[key], options)) + destination[key] = getMergeFunction(key, options)(target[key], source[key], options) } }) return destination diff --git a/test/prototype-poisoning.js b/test/prototype-poisoning.js index 4818a99..07bd465 100644 --- a/test/prototype-poisoning.js +++ b/test/prototype-poisoning.js @@ -5,7 +5,32 @@ test('merging objects with own __proto__', function(t) { var user = {} var malicious = JSON.parse('{ "__proto__": { "admin": true } }') var mergedObject = merge(user, malicious) - t.ok(mergedObject.__proto__.admin, 'nested properties should be merged') - t.notOk(mergedObject.admin, 'the destination should have an unchanged prototype') + t.notOk(mergedObject.__proto__.admin, 'non-plain properties should not be merged') + t.notOk(mergedObject.admin, 'the destination should have an unmodified prototype') + t.end() +}) + +test('merging objects with plain and non-plain properties', function(t) { + var plainSymbolKey = Symbol('plainSymbolKey') + var parent = { + parentKey: 'should be undefined' + } + + var target = Object.create(parent) + target.plainKey = 'should be replaced' + target[plainSymbolKey] = 'should also be replaced' + + var source = { + parentKey: 'foo', + plainKey: 'bar', + newKey: 'baz', + [plainSymbolKey]: 'qux' + } + + var mergedObject = merge(target, source) + t.equal(undefined, mergedObject.parentKey, 'inherited properties of target should be removed, not merged or ignored') + t.equal('bar', mergedObject.plainKey, 'enumerable own properties of target should be merged') + t.equal('baz', mergedObject.newKey, 'properties not yet on target should be merged') + t.equal('qux', mergedObject[plainSymbolKey], 'enumerable own symbol properties of target should be merged') t.end() }) \ No newline at end of file From 18e6e9ff47edf26ce22435e78e0b0085f2332dd4 Mon Sep 17 00:00:00 2001 From: mnespor Date: Wed, 2 Oct 2019 21:34:17 -0500 Subject: [PATCH 3/6] style: whitespace --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index e010010..3bb9b24 100644 --- a/index.js +++ b/index.js @@ -51,7 +51,7 @@ function mergeObject(target, source, options) { getKeys(source).forEach(function(key) { if (!propertyIsPlain(target, key)) { return - } + } if (!options.isMergeableObject(source[key]) || !target[key]) { destination[key] = cloneUnlessOtherwiseSpecified(source[key], options) From 75e62073ec8d84688d384977334fceb6fadda8af Mon Sep 17 00:00:00 2001 From: mnespor Date: Thu, 3 Oct 2019 20:08:22 -0500 Subject: [PATCH 4/6] detect absent keys in a better way --- index.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 3bb9b24..770b4ae 100644 --- a/index.js +++ b/index.js @@ -37,8 +37,14 @@ function getKeys(target) { } function propertyIsPlain(target, key) { - return target[key] === undefined - || (target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) + var keyInTarget + try { + keyInTarget = (key in target) + } catch (unused) { + keyInTarget = false + } + + return !keyInTarget || (target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) } function mergeObject(target, source, options) { From 9e2bb7b4ce3983349d6de3261f630b12cf9bedb5 Mon Sep 17 00:00:00 2001 From: mnespor Date: Wed, 9 Oct 2019 22:32:27 -0500 Subject: [PATCH 5/6] make prototype poisoning defense more permissive - see discussion in https://github.com/TehShrike/deepmerge/pull/164 - add test cases for custom string merging and objects with null prototype --- index.js | 15 +++++-------- test/prototype-poisoning.js | 44 ++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/index.js b/index.js index 770b4ae..7c6d257 100644 --- a/index.js +++ b/index.js @@ -36,15 +36,10 @@ function getKeys(target) { return Object.keys(target).concat(getEnumerableOwnPropertySymbols(target)) } -function propertyIsPlain(target, key) { - var keyInTarget - try { - keyInTarget = (key in target) - } catch (unused) { - keyInTarget = false - } - - return !keyInTarget || (target.hasOwnProperty(key) && target.propertyIsEnumerable(key)) +// Protects from prototype poisoning. '__proto__' is one of few truthy non-enumerable +// properties. +function propertyIsUnsafe(target, key) { + return target && target[key] && !Object.propertyIsEnumerable.call(target, key) } function mergeObject(target, source, options) { @@ -55,7 +50,7 @@ function mergeObject(target, source, options) { }) } getKeys(source).forEach(function(key) { - if (!propertyIsPlain(target, key)) { + if (propertyIsUnsafe(target, key)) { return } diff --git a/test/prototype-poisoning.js b/test/prototype-poisoning.js index 07bd465..fc79cf3 100644 --- a/test/prototype-poisoning.js +++ b/test/prototype-poisoning.js @@ -1,5 +1,6 @@ var merge = require('../') var test = require('tape') +var isMergeableObject = require('is-mergeable-object') test('merging objects with own __proto__', function(t) { var user = {} @@ -33,4 +34,45 @@ test('merging objects with plain and non-plain properties', function(t) { t.equal('baz', mergedObject.newKey, 'properties not yet on target should be merged') t.equal('qux', mergedObject[plainSymbolKey], 'enumerable own symbol properties of target should be merged') t.end() -}) \ No newline at end of file +}) + +// the following cases come from the thread here: https://github.com/TehShrike/deepmerge/pull/164 +test('merging strings works with a custom string merge', function(t) { + var target = { name: "Alexander" } + var source = { name: "Hamilton" } + function customMerge(key, options) { + if (key === 'name') { + return function(target, source, options) { + return target[0] + '. ' + source.substring(0, 3) + } + } else { + return merge + } + } + + function mergeable(target) { + return isMergeableObject(target) || (typeof target === 'string' && target.length > 1) + } + + t.equal('A. Ham', merge(target, source, { customMerge: customMerge, isMergeableObject: mergeable }).name) + t.end() +}) + +test('merging objects with null prototype', function(t) { + var target = Object.create(null) + var source = Object.create(null) + target.wheels = 4 + target.trunk = { toolbox: ['hammer'] } + source.trunk = { toolbox: ['wrench'] } + source.engine = 'v8' + var expected = { + wheels: 4, + engine: 'v8', + trunk: { + toolbox: ['hammer', 'wrench' ] + } + } + + t.deepEqual(expected, merge(target, source)) + t.end() +}) From c576569fdf9ddcfa13573820978fe876124781ac Mon Sep 17 00:00:00 2001 From: mnespor Date: Thu, 10 Oct 2019 21:35:31 -0500 Subject: [PATCH 6/6] continue refining propertyIsUnsafe #164 --- index.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 7c6d257..8e169b6 100644 --- a/index.js +++ b/index.js @@ -36,10 +36,17 @@ function getKeys(target) { return Object.keys(target).concat(getEnumerableOwnPropertySymbols(target)) } -// Protects from prototype poisoning. '__proto__' is one of few truthy non-enumerable -// properties. +// Protects from prototype poisoning and unexpected merging up the prototype chain. function propertyIsUnsafe(target, key) { - return target && target[key] && !Object.propertyIsEnumerable.call(target, key) + try { + return (key in target) // Properties are safe to merge if they don't exist in the target yet, + && !(Object.hasOwnProperty.call(target, key) // unsafe if they exist up the prototype chain, + && Object.propertyIsEnumerable.call(target, key)) // and also unsafe if they're nonenumerable. + } catch (unused) { + // Counterintuitively, it's safe to merge any property on a target that causes the `in` operator to throw. + // This happens when trying to copy an object in the source over a plain string in the target. + return false + } } function mergeObject(target, source, options) {