From 0f80d8019f4a49b635648d72c028223d1eb9c7c4 Mon Sep 17 00:00:00 2001 From: tjcouch-sil Date: Mon, 25 Mar 2024 16:12:28 -0500 Subject: [PATCH 1/3] Fixed github-reported prototype pollution vulnerability --- .../mui-utils/src/deepmerge/deepmerge.test.ts | 24 ++++++++++++++++++- packages/mui-utils/src/deepmerge/deepmerge.ts | 2 +- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/mui-utils/src/deepmerge/deepmerge.test.ts b/packages/mui-utils/src/deepmerge/deepmerge.test.ts index db39f72df32499..1e527985e8c8b0 100644 --- a/packages/mui-utils/src/deepmerge/deepmerge.test.ts +++ b/packages/mui-utils/src/deepmerge/deepmerge.test.ts @@ -4,7 +4,7 @@ import deepmerge from './deepmerge'; describe('deepmerge', () => { // https://snyk.io/blog/after-three-years-of-silence-a-new-jquery-prototype-pollution-vulnerability-emerges-once-again/ - it('should not be subject to prototype pollution', () => { + it('should not be subject to prototype pollution via __proto__', () => { deepmerge({}, JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), { clone: false, }); @@ -12,6 +12,28 @@ describe('deepmerge', () => { expect({}).not.to.have.property('isAdmin'); }); + // https://cwe.mitre.org/data/definitions/915.html + it('should not be subject to prototype pollution via constructor', () => { + deepmerge( + {}, + JSON.parse('{ "myProperty": "a", "constructor" : { "prototype": { "isAdmin" : true } } }'), + { + clone: true, + }, + ); + + expect({}).not.to.have.property('isAdmin'); + }); + + // https://cwe.mitre.org/data/definitions/915.html + it('should not be subject to prototype pollution via prototype', () => { + deepmerge({}, JSON.parse('{ "myProperty": "a", "prototype": { "isAdmin" : true } }'), { + clone: false, + }); + + expect({}).not.to.have.property('isAdmin'); + }); + it('should merge objects across realms', function test() { if (!/jsdom/.test(window.navigator.userAgent)) { // vm is only available in Node.js. diff --git a/packages/mui-utils/src/deepmerge/deepmerge.ts b/packages/mui-utils/src/deepmerge/deepmerge.ts index a8035a0c470177..ef730592fc83bd 100644 --- a/packages/mui-utils/src/deepmerge/deepmerge.ts +++ b/packages/mui-utils/src/deepmerge/deepmerge.ts @@ -42,7 +42,7 @@ export default function deepmerge( if (isPlainObject(target) && isPlainObject(source)) { Object.keys(source).forEach((key) => { // Avoid prototype pollution - if (key === '__proto__') { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { return; } From 17740f20c1f2ae7136c9d689724e0cba43b6f2e6 Mon Sep 17 00:00:00 2001 From: tjcouch-sil Date: Wed, 22 May 2024 13:37:39 +0900 Subject: [PATCH 2/3] Reworked deepmerge's prototype pollution check to use Object.prototype.hasOwnProperty --- packages/mui-utils/src/deepmerge/deepmerge.test.ts | 12 ++++++++++++ packages/mui-utils/src/deepmerge/deepmerge.ts | 12 ++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/mui-utils/src/deepmerge/deepmerge.test.ts b/packages/mui-utils/src/deepmerge/deepmerge.test.ts index 1e527985e8c8b0..07d3f83ea3a091 100644 --- a/packages/mui-utils/src/deepmerge/deepmerge.test.ts +++ b/packages/mui-utils/src/deepmerge/deepmerge.test.ts @@ -34,6 +34,18 @@ describe('deepmerge', () => { expect({}).not.to.have.property('isAdmin'); }); + it('should appropriately copy the fields without prototype pollution', () => { + const result = deepmerge( + {}, + JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), + ); + + // @ts-expect-error __proto__ is not on this object type + // eslint-disable-next-line no-proto + expect(result.__proto__).to.have.property('isAdmin'); + expect({}).not.to.have.property('isAdmin'); + }) + it('should merge objects across realms', function test() { if (!/jsdom/.test(window.navigator.userAgent)) { // vm is only available in Node.js. diff --git a/packages/mui-utils/src/deepmerge/deepmerge.ts b/packages/mui-utils/src/deepmerge/deepmerge.ts index ef730592fc83bd..e04e90a7851d7e 100644 --- a/packages/mui-utils/src/deepmerge/deepmerge.ts +++ b/packages/mui-utils/src/deepmerge/deepmerge.ts @@ -41,12 +41,12 @@ export default function deepmerge( if (isPlainObject(target) && isPlainObject(source)) { Object.keys(source).forEach((key) => { - // Avoid prototype pollution - if (key === '__proto__' || key === 'constructor' || key === 'prototype') { - return; - } - - if (isPlainObject(source[key]) && key in target && isPlainObject(target[key])) { + if ( + isPlainObject(source[key]) && + // Avoid prototype pollution + Object.prototype.hasOwnProperty.call(target, key) && + isPlainObject(target[key]) + ) { // Since `output` is a clone of `target` and we have narrowed `target` in this block we can cast to the same type. (output as Record)[key] = deepmerge(target[key], source[key], options); } else if (options.clone) { From 4c06baee44b78f1f2743581ecb1fee946ad55aad Mon Sep 17 00:00:00 2001 From: tjcouch-sil Date: Thu, 23 May 2024 09:24:46 +0900 Subject: [PATCH 3/3] Added positive assertions to tests --- .../mui-utils/src/deepmerge/deepmerge.test.ts | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/packages/mui-utils/src/deepmerge/deepmerge.test.ts b/packages/mui-utils/src/deepmerge/deepmerge.test.ts index 07d3f83ea3a091..9cb09790761553 100644 --- a/packages/mui-utils/src/deepmerge/deepmerge.test.ts +++ b/packages/mui-utils/src/deepmerge/deepmerge.test.ts @@ -5,16 +5,23 @@ import deepmerge from './deepmerge'; describe('deepmerge', () => { // https://snyk.io/blog/after-three-years-of-silence-a-new-jquery-prototype-pollution-vulnerability-emerges-once-again/ it('should not be subject to prototype pollution via __proto__', () => { - deepmerge({}, JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), { - clone: false, - }); + const result = deepmerge( + {}, + JSON.parse('{ "myProperty": "a", "__proto__" : { "isAdmin" : true } }'), + { + clone: false, + }, + ); + // @ts-expect-error __proto__ is not on this object type + // eslint-disable-next-line no-proto + expect(result.__proto__).to.have.property('isAdmin'); expect({}).not.to.have.property('isAdmin'); }); // https://cwe.mitre.org/data/definitions/915.html it('should not be subject to prototype pollution via constructor', () => { - deepmerge( + const result = deepmerge( {}, JSON.parse('{ "myProperty": "a", "constructor" : { "prototype": { "isAdmin" : true } } }'), { @@ -22,15 +29,22 @@ describe('deepmerge', () => { }, ); + expect(result.constructor.prototype).to.have.property('isAdmin'); expect({}).not.to.have.property('isAdmin'); }); // https://cwe.mitre.org/data/definitions/915.html it('should not be subject to prototype pollution via prototype', () => { - deepmerge({}, JSON.parse('{ "myProperty": "a", "prototype": { "isAdmin" : true } }'), { - clone: false, - }); + const result = deepmerge( + {}, + JSON.parse('{ "myProperty": "a", "prototype": { "isAdmin" : true } }'), + { + clone: false, + }, + ); + // @ts-expect-error prototype is not on this object type + expect(result.prototype).to.have.property('isAdmin'); expect({}).not.to.have.property('isAdmin'); }); @@ -44,7 +58,7 @@ describe('deepmerge', () => { // eslint-disable-next-line no-proto expect(result.__proto__).to.have.property('isAdmin'); expect({}).not.to.have.property('isAdmin'); - }) + }); it('should merge objects across realms', function test() { if (!/jsdom/.test(window.navigator.userAgent)) {