fix: check dangerous property access in strict-mode (handlebars-lang#…

…1736)
bitwiseman · May 18, 2021 · e61e016 · e61e016
1 parent 16bd606
commit e61e016
Show file tree

Hide file tree

Showing 6 changed files with 221 additions and 20 deletions.
diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js
@@ -69,12 +69,28 @@ export function template(templateSpec, env) {
       if (!(name in obj)) {
         throw new Exception('"' + name + '" not defined in ' + obj);
       }
-      return obj[name];
+      return container.lookupProperty(obj, name);
+    },
+    lookupProperty: function(parent, propertyName) {
+      let result = parent[propertyName];
+      if (result == null) {
+        return result;
+      }
+      if (Object.prototype.hasOwnProperty.call(parent, propertyName)) {
+        return result;
+      }
+
+      if (!Utils.dangerousPropertyRegex.test(String(propertyName))) {
+        return result;
+      }
+
+      return undefined;
     },
     lookup: function(depths, name) {
       const len = depths.length;
       for (let i = 0; i < len; i++) {
-        if (depths[i] && depths[i][name] != null) {
+        let result = depths[i] && container.lookupProperty(depths[i], name);
+        if (result != null) {
           return depths[i][name];
         }
       }

diff --git a/package.json b/package.json
@@ -36,6 +36,9 @@
     "babel-loader": "^5.0.0",
     "babel-runtime": "^5.1.10",
     "benchmark": "~1.0",
+    "chai": "^4.2.0",
+    "chai-diff": "^1.0.1",
+    "dirty-chai": "^2.0.1",
     "dustjs-linkedin": "^2.0.2",
     "eco": "~1.1.0-rc-3",
     "grunt": "~0.4.1",

diff --git a/spec/.eslintrc b/spec/.eslintrc
@@ -3,12 +3,11 @@
     "CompilerContext": true,
     "Handlebars": true,
     "handlebarsEnv": true,
-
     "shouldCompileTo": true,
     "shouldCompileToWithPartials": true,
     "shouldThrow": true,
+    "expectTemplate": true,
     "compileWithPartials": true,
-
     "console": true,
     "require": true,
     "suite": true,
@@ -22,15 +21,17 @@
     "stop": true,
     "ok": true,
     "strictEqual": true,
-    "define": true
+    "define": true,
+    "expect": true,
+    "chai": true
   },
   "env": {
     "mocha": true
   },
   "rules": {
     // Disabling for tests, for now.
     "no-path-concat": 0,
-
+    "dot-notation": 0,
     "no-var": 0
   }
 }
diff --git a/spec/env/common.js b/spec/env/common.js
@@ -72,3 +72,121 @@ global.shouldThrow = function(callback, type, msg) {
     throw new Error('It failed to throw');
   }
 };
+
+
+global.expectTemplate = function(templateAsString) {
+  return new HandlebarsTestBench(templateAsString);
+};
+
+function HandlebarsTestBench(templateAsString) {
+  this.templateAsString = templateAsString;
+  this.helpers = {};
+  this.partials = {};
+  this.decorators = {};
+  this.input = {};
+  this.message =
+    'Template' + templateAsString + ' does not evaluate to expected output';
+  this.compileOptions = {};
+  this.runtimeOptions = {};
+}
+
+HandlebarsTestBench.prototype.withInput = function(input) {
+  this.input = input;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withHelper = function(name, helperFunction) {
+  this.helpers[name] = helperFunction;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withHelpers = function(helperFunctions) {
+  var self = this;
+  Object.keys(helperFunctions).forEach(function(name) {
+    self.withHelper(name, helperFunctions[name]);
+  });
+  return this;
+};
+
+HandlebarsTestBench.prototype.withPartial = function(name, partialAsString) {
+  this.partials[name] = partialAsString;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withPartials = function(partials) {
+  var self = this;
+  Object.keys(partials).forEach(function(name) {
+    self.withPartial(name, partials[name]);
+  });
+  return this;
+};
+
+HandlebarsTestBench.prototype.withDecorator = function(
+  name,
+  decoratorFunction
+) {
+  this.decorators[name] = decoratorFunction;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withDecorators = function(decorators) {
+  var self = this;
+  Object.keys(decorators).forEach(function(name) {
+    self.withDecorator(name, decorators[name]);
+  });
+  return this;
+};
+
+HandlebarsTestBench.prototype.withCompileOptions = function(compileOptions) {
+  this.compileOptions = compileOptions;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withRuntimeOptions = function(runtimeOptions) {
+  this.runtimeOptions = runtimeOptions;
+  return this;
+};
+
+HandlebarsTestBench.prototype.withMessage = function(message) {
+  this.message = message;
+  return this;
+};
+
+HandlebarsTestBench.prototype.toCompileTo = function(expectedOutputAsString) {
+  expect(this._compileAndExecute()).to.equal(
+    expectedOutputAsString,
+    this.message
+  );
+};
+
+// see chai "to.throw" (https://www.chaijs.com/api/bdd/#method_throw)
+HandlebarsTestBench.prototype.toThrow = function(errorLike, errMsgMatcher) {
+  var self = this;
+  expect(function() {
+    self._compileAndExecute();
+  }).to.throw(errorLike, errMsgMatcher, this.message);
+};
+
+HandlebarsTestBench.prototype._compileAndExecute = function() {
+  var compile =
+    Object.keys(this.partials).length > 0
+      ? CompilerContext.compileWithPartial
+      : CompilerContext.compile;
+
+  var combinedRuntimeOptions = this._combineRuntimeOptions();
+
+  var template = compile(this.templateAsString, this.compileOptions);
+  return template(this.input, combinedRuntimeOptions);
+};
+
+HandlebarsTestBench.prototype._combineRuntimeOptions = function() {
+  var self = this;
+  var combinedRuntimeOptions = {};
+  Object.keys(this.runtimeOptions).forEach(function(key) {
+    combinedRuntimeOptions[key] = self.runtimeOptions[key];
+  });
+  combinedRuntimeOptions.helpers = this.helpers;
+  combinedRuntimeOptions.partials = this.partials;
+  combinedRuntimeOptions.decorators = this.decorators;
+  return combinedRuntimeOptions;
+};
diff --git a/spec/env/node.js b/spec/env/node.js
@@ -1,5 +1,11 @@
 require('./common');
 
+var chai = require('chai');
+var dirtyChai = require('dirty-chai');
+
+chai.use(dirtyChai);
+global.expect = chai.expect;
+
 global.Handlebars = require('../../lib');
 
 global.CompilerContext = {

diff --git a/spec/security.js b/spec/security.js
@@ -1,17 +1,30 @@
 describe('security issues', function() {
     describe('GH-1495: Prevent Remote Code Execution via constructor', function() {
-        it('should not allow constructors to be accessed', function() {
-            shouldCompileTo('{{constructor.name}}', {}, '');
-            shouldCompileTo('{{lookup (lookup this "constructor") "name"}}', {}, '');
-        });
+      checkPropertyAccess({});
+
+      describe('in compat-mode', function() {
+        checkPropertyAccess({ compat: true });
+      });
 
+      describe('in strict-mode', function() {
+        checkPropertyAccess({ strict: true });
+      });
+
+
+      function checkPropertyAccess(compileOptions) {
         it('should allow the "constructor" property to be accessed if it is enumerable', function() {
-            shouldCompileTo('{{constructor.name}}', {'constructor': {
-                'name': 'here we go'
-            }}, 'here we go');
-            shouldCompileTo('{{lookup (lookup this "constructor") "name"}}', {'constructor': {
-                'name': 'here we go'
-            }}, 'here we go');
+          expectTemplate('{{constructor.name}}')
+            .withCompileOptions(compileOptions)
+            .withInput({'constructor': {
+              'name': 'here we go'
+            }})
+            .toCompileTo('here we go');
+          expectTemplate('{{lookup (lookup this "constructor") "name"}}')
+            .withCompileOptions(compileOptions)
+            .withInput({'constructor': {
+              'name': 'here we go'
+            }})
+            .toCompileTo('here we go');
         });
 
         it('should allow prototype properties that are not constructors', function() {
@@ -24,11 +37,55 @@ describe('security issues', function() {
                 }
             });
 
-            shouldCompileTo('{{#with this}}{{this.abc}}{{/with}}',
-                new TestClass(), 'xyz');
-            shouldCompileTo('{{#with this}}{{lookup this "abc"}}{{/with}}',
-                new TestClass(), 'xyz');
+
+            expectTemplate('{{#with this}}{{this.abc}}{{/with}}')
+              .withCompileOptions(compileOptions)
+              .withInput(new TestClass())
+              .toCompileTo('xyz');
+
+            expectTemplate('{{#with this}}{{lookup this "abc"}}{{/with}}')
+              .withCompileOptions(compileOptions)
+              .withInput(new TestClass())
+              .toCompileTo('xyz');
+        });
+
+        it('should not allow constructors to be accessed', function() {
+          expectTemplate('{{lookup (lookup this "constructor") "name"}}')
+            .withCompileOptions(compileOptions)
+            .withInput({})
+            .toCompileTo('');
+          if (compileOptions.strict) {
+            expectTemplate('{{constructor.name}}')
+              .withCompileOptions(compileOptions)
+              .withInput({})
+              .toThrow(TypeError);
+          } else {
+            expectTemplate('{{constructor.name}}')
+              .withCompileOptions(compileOptions)
+              .withInput({})
+              .toCompileTo('');
+          }
         });
+
+        it('should not allow __proto__ to be accessed', function() {
+          expectTemplate('{{lookup (lookup this "__proto__") "name"}}')
+            .withCompileOptions(compileOptions)
+            .withInput({})
+            .toCompileTo('');
+          if (compileOptions.strict) {
+            expectTemplate('{{__proto__.name}}')
+              .withCompileOptions(compileOptions)
+              .withInput({})
+              .toThrow(TypeError);
+          } else {
+            expectTemplate('{{__proto__.name}}')
+              .withCompileOptions(compileOptions)
+              .withInput({})
+              .toCompileTo('');
+          }
+        });
+
+      }
     });
 
     describe('GH-1595', function() {