Merge pull request #402 from bigcommerce/trac-8385

fix: TRAC-838 resolve unsafe JSONStringify helper

Author: parthshahp

helpers/3p/object.js

@@ -2,6 +2,7 @@
 
 var hasOwn = Object.hasOwnProperty;
 var utils = require('./utils/');
+var common = require('../lib/common.js');
 
 /**
  * Expose `helpers`
@@ -209,7 +210,9 @@ helpers.JSONstringify = function(obj, indent) {
   if (!utils.isNumber(indent)) {
     indent = 0;
   }
-  return JSON.stringify(obj, null, indent);
+  // Escape characters that are unsafe when this output is embedded inside an
+  // HTML <script> tag (e.g. JSON-LD), preventing reflected XSS via `</script>`.
+  return common.escapeJsonForHtml(JSON.stringify(obj, null, indent));
 };
 
 /**

helpers/json.js

@@ -4,7 +4,7 @@ const common = require('./lib/common.js');
 const factory = globals => {
     return function(data) {
         data = common.unwrapIfSafeString(globals.handlebars, data);
-        return JSON.stringify(data);
+        return common.escapeJsonForHtml(JSON.stringify(data));
     };
 };
 

helpers/lib/common.js

@@ -11,7 +11,7 @@ function isValidURL(val) {
 
 /*
  * Based on https://github.com/jonschlinkert/get-value/blob/2.0.6/index.js with some enhancements.
- * 
+ *
  * - Performs "hasOwnProperty" checks for safety.
  * - Now accepts Handlebars.SafeString paths.
  */
@@ -67,6 +67,38 @@ function unwrapIfSafeString(handlebars, val) {
     return val;
 }
 
+// Maps the char code of characters that are valid inside a JSON string but
+// dangerous when that JSON is emitted (unescaped) inside an HTML <script> tag,
+// to their \uXXXX escape sequences. Encoding `<` and `/` prevents the HTML
+// parser from ever seeing a closing tag such as `</script>`; `>` guards
+// against `-->`/`]]>`; U+2028/U+2029 are illegal in JS string literals. Every
+// replacement is still valid JSON and round-trips through `JSON.parse`.
+const HTML_UNSAFE_JSON_ESCAPES = {
+    0x3c: '\\u003c', // <
+    0x3e: '\\u003e', // >
+    0x2f: '\\u002f', // /
+    0x2028: '\\u2028',
+    0x2029: '\\u2029',
+};
+
+const HTML_UNSAFE_JSON_REGEX = /[<>/\u2028\u2029]/g;
+
+/**
+ * Escape the output of `JSON.stringify` so it can be safely embedded inside an
+ * HTML <script> tag. Encodes `<`, `>`, `/` and the U+2028/U+2029 line
+ * separators as unicode escape sequences. The result is still valid JSON and
+ * round-trips through `JSON.parse`.
+ *
+ * @param {string} jsonString - The output of JSON.stringify
+ * @returns {string} - The HTML-safe JSON string
+ */
+function escapeJsonForHtml(jsonString) {
+    if (typeof jsonString !== 'string') {
+        return jsonString;
+    }
+    return jsonString.replace(HTML_UNSAFE_JSON_REGEX, char => HTML_UNSAFE_JSON_ESCAPES[char.charCodeAt(0)]);
+}
+
 const maximumPixelSize = 5120;
 
 /**
@@ -79,7 +111,7 @@ function appendLossyParam(url, lossy) {
     if (!lossy || typeof lossy !== 'boolean') {
         return url;
     }
-    
+
     const urlObj = new URL(url);
     urlObj.searchParams.set('compression', 'lossy');
     return urlObj.toString();
@@ -89,6 +121,7 @@ module.exports = {
     isValidURL,
     getValue,
     unwrapIfSafeString,
+    escapeJsonForHtml,
     maximumPixelSize,
     appendLossyParam
 };

spec/helpers/3p/object.js

@@ -165,5 +165,13 @@ describe('object', function() {
       expect(res).to.equal('{"name":"Halle","age":4,"userid":"Nicole"}');
       done();
     });
+
+    it('should escape HTML-unsafe characters so output is safe inside a <script> tag:', function(done) {
+      var fn = hbs.compile('{{{JSONstringify data}}}');
+      var res = fn({data: 'Music</script><script>alert(1)</script>'});
+      expect(res).to.equal('"Music\\u003c\\u002fscript\\u003e\\u003cscript\\u003ealert(1)\\u003c\\u002fscript\\u003e"');
+      expect(res).to.not.contain('</script>');
+      done();
+    });
   });
 });

spec/helpers/json.js

@@ -10,7 +10,8 @@ describe('json helper', function() {
         image_with_2_qs: {
             data: urlData_2_qs
         },
-        object: { a: 1, b: "hello" }
+        object: { a: 1, b: "hello" },
+        xss: 'Music</script><script>alert(1)</script>'
     };
 
     const runTestCases = testRunner({context});
@@ -27,7 +28,7 @@ describe('json helper', function() {
         runTestCases([
             {
                 input: '{{{json (getImage image_with_2_qs)}}}',
-                output: '"https://cdn.example.com/path/to/original/image.png?c=2&imbypass=on"',
+                output: '"https:\\u002f\\u002fcdn.example.com\\u002fpath\\u002fto\\u002foriginal\\u002fimage.png?c=2&imbypass=on"',
             },
         ], done);
     });
@@ -40,4 +41,13 @@ describe('json helper', function() {
             },
         ], done);
     });
+
+    it('should escape HTML-unsafe characters so output is safe inside a <script> tag', function(done) {
+        runTestCases([
+            {
+                input: '{{{json xss}}}',
+                output: '"Music\\u003c\\u002fscript\\u003e\\u003cscript\\u003ealert(1)\\u003c\\u002fscript\\u003e"',
+            },
+        ], done);
+    });
 });