fix: add more properties required to be enumerable

- __defineGetter__, __defineSetter__, __lookupGetter__, __proto__
handlebars-lang · Nov 18, 2019 · 1988878 · 1988878
1 parent 886ba86
commit 1988878
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 29 deletions.
diff --git a/lib/handlebars/compiler/compiler.js b/lib/handlebars/compiler/compiler.js
@@ -54,9 +54,7 @@ Compiler.prototype = {
 
     options.blockParams = options.blockParams || [];
 
-    // These changes will propagate to the other compiler components
-    let knownHelpers = options.knownHelpers;
-    options.knownHelpers = {
+    options.knownHelpers = extend(Object.create(null), {
       'helperMissing': true,
       'blockHelperMissing': true,
       'each': true,
@@ -65,15 +63,7 @@ Compiler.prototype = {
       'with': true,
       'log': true,
       'lookup': true
-    };
-    if (knownHelpers) {
-      // the next line should use "Object.keys", but the code has been like this a long time and changing it, might
-      // cause backwards-compatibility issues... It's an old library...
-      // eslint-disable-next-line guard-for-in
-      for (let name in knownHelpers) {
-          this.options.knownHelpers[name] = knownHelpers[name];
-      }
-    }
+    }, options.knownHelpers);
 
     return this.accept(program);
   },
@@ -369,7 +359,6 @@ Compiler.prototype = {
     if (isEligible && !isHelper) {
       let name = sexpr.path.parts[0],
           options = this.options;
-
       if (options.knownHelpers[name]) {
         isHelper = true;
       } else if (options.knownHelpersOnly) {

diff --git a/lib/handlebars/compiler/javascript-compiler.js b/lib/handlebars/compiler/javascript-compiler.js
@@ -2,6 +2,7 @@ import { COMPILER_REVISION, REVISION_CHANGES } from '../base';
 import Exception from '../exception';
 import {isArray} from '../utils';
 import CodeGen from './code-gen';
+import {dangerousPropertyRegex} from '../helpers/lookup';
 
 function Literal(value) {
   this.value = value;
@@ -13,9 +14,8 @@ JavaScriptCompiler.prototype = {
   // PUBLIC API: You can override these methods in a subclass to provide
   // alternative compiled forms for name lookup and buffering semantics
   nameLookup: function(parent, name/* , type*/) {
-    const isEnumerable = [ this.aliasable('container.propertyIsEnumerable'), '.call(', parent, ',"constructor")'];
-
-    if (name === 'constructor') {
+    if (dangerousPropertyRegex.test(name)) {
+      const isEnumerable = [ this.aliasable('container.propertyIsEnumerable'), '.call(', parent, ',', JSON.stringify(name), ')'];
       return ['(', isEnumerable, '?', _actualLookup(), ' : undefined)'];
     }
     return _actualLookup();

diff --git a/lib/handlebars/helpers/lookup.js b/lib/handlebars/helpers/lookup.js
@@ -1,9 +1,11 @@
+export const dangerousPropertyRegex = /^(constructor|__defineGetter__|__defineSetter__|__lookupGetter__|__proto__)$/;
+
 export default function(instance) {
   instance.registerHelper('lookup', function(obj, field) {
     if (!obj) {
       return obj;
     }
-    if (String(field) === 'constructor' && !obj.propertyIsEnumerable(field)) {
+    if (dangerousPropertyRegex.test(String(field)) && !obj.propertyIsEnumerable(field)) {
       return undefined;
     }
     return obj[field];

diff --git a/spec/env/common.js b/spec/env/common.js
@@ -142,24 +142,35 @@ HandlebarsTestBench.prototype.withMessage = function(message) {
 };
 
 HandlebarsTestBench.prototype.toCompileTo = function(expectedOutputAsString) {
+  expect(this._compileAndExeute()).to.equal(expectedOutputAsString);
+};
+
+// see chai "to.throw" (https://www.chaijs.com/api/bdd/#method_throw)
+HandlebarsTestBench.prototype.toThrow = function(errorLike, errMsgMatcher, msg) {
   var self = this;
+  expect(function() {
+    self._compileAndExeute();
+  }).to.throw(errorLike, errMsgMatcher, msg);
+};
+
+HandlebarsTestBench.prototype._compileAndExeute = 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;
-
-  var template = compile(this.templateAsString, this.compileOptions);
-  var output = template(this.input, combinedRuntimeOptions);
-
-  if (output !== expectedOutputAsString) {
-    // Error message formatted so that IntelliJ-Idea shows "diff"-button
-    // https://stackoverflow.com/a/10945655/4251384
-    throw new AssertError(this.message + '\nexpected:' + expectedOutputAsString + 'but was:' + output);
-  }
+  return combinedRuntimeOptions;
 };
diff --git a/spec/security.js b/spec/security.js
@@ -114,13 +114,31 @@ describe('security issues', function() {
     describe('GH-1563', function() {
         it('should not allow to access constructor after overriding via __defineGetter__', function() {
             if (({}).__defineGetter__ == null || ({}).__lookupGetter__ == null) {
-                return; // Browser does not support this exploit anyway
+                return this.skip(); // Browser does not support this exploit anyway
             }
-            shouldCompileTo('{{__defineGetter__ "undefined" valueOf }}' +
+            expectTemplate('{{__defineGetter__ "undefined" valueOf }}' +
                 '{{#with __lookupGetter__ }}' +
                 '{{__defineGetter__ "propertyIsEnumerable" (this.bind (this.bind 1)) }}' +
                 '{{constructor.name}}' +
-                '{{/with}}', {}, '');
+                '{{/with}}')
+                .withInput({})
+                .toThrow(/Missing helper: "__defineGetter__"/);
         });
     });
+
+    describe('GH-1595', function() {
+      it('properties, that are required to be enumerable', function() {
+        expectTemplate('{{constructor}}').withInput({}).toCompileTo('');
+        expectTemplate('{{__defineGetter__}}').withInput({}).toCompileTo('');
+        expectTemplate('{{__defineSetter__}}').withInput({}).toCompileTo('');
+        expectTemplate('{{__lookupGetter__}}').withInput({}).toCompileTo('');
+        expectTemplate('{{__proto__}}').withInput({}).toCompileTo('');
+
+        expectTemplate('{{lookup "constructor"}}').withInput({}).toCompileTo('');
+        expectTemplate('{{lookup "__defineGetter__"}}').withInput({}).toCompileTo('');
+        expectTemplate('{{lookup "__defineSetter__"}}').withInput({}).toCompileTo('');
+        expectTemplate('{{lookup "__lookupGetter__"}}').withInput({}).toCompileTo('');
+        expectTemplate('{{lookup "__proto__"}}').withInput({}).toCompileTo('');
+      });
+    });
 });