Fixed #14883, possible prototype pollution through chart configuration

highcharts · Jan 7, 2021 · fead359 · fead359
1 parent 2ed7a26
commit fead359
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 9 deletions.
diff --git a/js/Core/Utilities.js b/js/Core/Utilities.js
@@ -451,6 +451,10 @@ function merge() {
             copy = {};
         }
         objectEach(original, function (value, key) {
+            // Prototype pollution (#14883)
+            if (key === '__proto__' || key === 'constructor') {
+                return;
+            }
             // Copy the contents of objects, but not arrays or DOM nodes
             if (isObject(value, true) &&
                 !isClass(value) &&

diff --git a/samples/unit-tests/highcharts/constructors/demo.js b/samples/unit-tests/highcharts/constructors/demo.js
@@ -9,16 +9,25 @@ QUnit.test('Highcharts', function (assert) {
         }
     });
 
-    chart = new Highcharts.Chart({
-        chart: {
-            renderTo: 'container'
+    const options = JSON.parse(`{
+        "chart": {
+            "__proto__": {
+                "prototypePollution": true
+            },
+            "renderTo": "container"
         },
-        series: [
-            {
-                data: [1, 2, 3, 4]
-            }
-        ]
-    });
+        "series": [{
+            "data": [1, 2, 3, 4]
+        }]
+    }`);
+
+    chart = new Highcharts.Chart(options);
+
+    assert.strictEqual(
+        window.prototypePollution,
+        undefined,
+        'Prototype pollution through JSON options should not be possible'
+    );
 
     assert.strictEqual(
         chart.series[0].yData.join(','),

diff --git a/samples/unit-tests/utilities/utilities/demo.js b/samples/unit-tests/utilities/utilities/demo.js
@@ -63,6 +63,38 @@
         );
     });
 
+    QUnit.test('Merge', assert => {
+
+        // test filtering of __proto__
+        const objProto = JSON.parse(`{
+            "__proto__": {
+                "pollutedByProto": true
+            }
+        }`);
+        Highcharts.merge({}, objProto);
+        assert.strictEqual(
+            typeof pollutedByProto, // eslint-disable-line no-undef
+            'undefined',
+            'The prototype (and window) should not be polluted through merge'
+        );
+
+        // test filtering of constructor
+        const objConstructor = JSON.parse(`{
+            "constructor": {
+                "prototype": {
+                    "pollutedByConstructor": true
+                }
+            }
+        }`);
+        Highcharts.merge({}, objConstructor);
+        assert.strictEqual(
+            typeof {}.pollutedByConstructor, // eslint-disable-line no-undef
+            'undefined',
+            'The prototype (and window) should not be polluted through merge'
+        );
+
+    });
+
     QUnit.test('PInt', function (assert) {
         // test without a base defined
         assertEquals(assert, 'base not defined', 15, pInt('15'));

diff --git a/ts/Core/Utilities.ts b/ts/Core/Utilities.ts
@@ -611,6 +611,11 @@ function merge<T>(): T {
 
             objectEach(original, function (value, key): void {
 
+                // Prototype pollution (#14883)
+                if (key === '__proto__' || key === 'constructor') {
+                    return;
+                }
+
                 // Copy the contents of objects, but not arrays or DOM nodes
                 if (isObject(value, true) &&
                     !isClass(value) &&