feat: access control to prototype properties via whitelist

Disallow access to prototype properties and methods by default. Access to properties is always checked via `Object.prototype.hasOwnProperty.call(parent, propertyName)`. New runtime options: - **allowedProtoMethods**: a string-to-boolean map of property-names that are allowed if they are methods of the parent object. - **allowedProtoProperties**: a string-to-boolean map of property-names that are allowed if they are properties but not methods of the parent object. ```js const template = handlebars.compile('{{aString.trim}}') const result = template({ aString: ' abc ' }) // result is empty, because trim is defined at String prototype ``` ```js const template = handlebars.compile('{{aString.trim}}') const result = template({ aString: ' abc ' }, { allowedProtoMethods: { trim: true } }) // result = 'abc' ``` Implementation details: The method now "container.lookupProperty" handles the prototype-checks and the white-lists. It is used in - JavaScriptCompiler#nameLookup - The "lookup"-helper (passed to all helpers as "options.lookupProperty") - The "lookup" function at the container, which is used for recursive lookups in "compat" mode Compatibility: - **Old precompiled templates work with new runtimes**: The "options.lookupPropery"-function is passed to the helper by a wrapper, not by the compiled templated. - **New templates work with old runtimes**: The template contains a function that is used as fallback if the "lookupProperty"-function cannot be found at the container. However, the runtime-options "allowedProtoProperties" and "allowedProtoMethods" only work with the newest runtime. BREAKING CHANGE: - access to prototype properties is forbidden completely by default
handlebars-lang · Jan 8, 2020 · 33a3b46 · 33a3b46
1 parent 164b7ff
commit 33a3b46
Show file tree

Hide file tree

Showing 12 changed files with 354 additions and 73 deletions.
diff --git a/lib/handlebars/compiler/javascript-compiler.js b/lib/handlebars/compiler/javascript-compiler.js
@@ -2,7 +2,6 @@ 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,27 +12,8 @@ function JavaScriptCompiler() {}
 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*/) {
-    if (dangerousPropertyRegex.test(name)) {
-      const isEnumerable = [
-        this.aliasable('container.propertyIsEnumerable'),
-        '.call(',
-        parent,
-        ',',
-        JSON.stringify(name),
-        ')'
-      ];
-      return ['(', isEnumerable, '?', _actualLookup(), ' : undefined)'];
-    }
-    return _actualLookup();
-
-    function _actualLookup() {
-      if (JavaScriptCompiler.isValidJavaScriptVariableName(name)) {
-        return [parent, '.', name];
-      } else {
-        return [parent, '[', JSON.stringify(name), ']'];
-      }
-    }
+  nameLookup: function(parent, name /*,  type */) {
+    return this.internalNameLookup(parent, name);
   },
   depthedLookup: function(name) {
     return [this.aliasable('container.lookup'), '(depths, "', name, '")'];
@@ -69,6 +49,12 @@ JavaScriptCompiler.prototype = {
     return this.quotedString('');
   },
   // END PUBLIC API
+  internalNameLookup: function(parent, name) {
+    this.lookupPropertyFunctionIsUsed = true;
+    return ['lookupProperty(', parent, ',', JSON.stringify(name), ')'];
+  },
+
+  lookupPropertyFunctionIsUsed: false,
 
   compile: function(environment, options, context, asObject) {
     this.environment = environment;
@@ -131,7 +117,11 @@ JavaScriptCompiler.prototype = {
     if (!this.decorators.isEmpty()) {
       this.useDecorators = true;
 
-      this.decorators.prepend('var decorators = container.decorators;\n');
+      this.decorators.prepend([
+        'var decorators = container.decorators, ',
+        this.lookupPropertyFunctionVarDeclaration(),
+        ';\n'
+      ]);
       this.decorators.push('return fn;');
 
       if (asObject) {
@@ -248,6 +238,10 @@ JavaScriptCompiler.prototype = {
       }
     });
 
+    if (this.lookupPropertyFunctionIsUsed) {
+      varDeclarations += ', ' + this.lookupPropertyFunctionVarDeclaration();
+    }
+
     let params = ['container', 'depth0', 'helpers', 'partials', 'data'];
 
     if (this.useBlockParams || this.useDepths) {
@@ -335,6 +329,17 @@ JavaScriptCompiler.prototype = {
     return this.source.merge();
   },
 
+  lookupPropertyFunctionVarDeclaration: function() {
+    return `
+      lookupProperty = container.lookupProperty || function(parent, propertyName) {
+        if (Object.prototype.hasOwnProperty.call(parent, propertyName)) {
+          return parent[propertyName];
+        }
+        return undefined
+    }
+    `.trim();
+  },
+
   // [blockValue]
   //
   // On stack, before: hash, inverse, program, value
@@ -1241,6 +1246,9 @@ JavaScriptCompiler.prototype = {
   }
 })();
 
+/**
+ * @deprecated May be removed in the next major version
+ */
 JavaScriptCompiler.isValidJavaScriptVariableName = function(name) {
   return (
     !JavaScriptCompiler.RESERVED_WORDS[name] &&

diff --git a/lib/handlebars/helpers/lookup.js b/lib/handlebars/helpers/lookup.js
@@ -1,16 +1,9 @@
-export const dangerousPropertyRegex = /^(constructor|__defineGetter__|__defineSetter__|__lookupGetter__|__proto__)$/;
-
 export default function(instance) {
-  instance.registerHelper('lookup', function(obj, field) {
+  instance.registerHelper('lookup', function(obj, field, options) {
     if (!obj) {
+      // Note for 5.0: Change to "obj == null" in 5.0
       return obj;
     }
-    if (
-      dangerousPropertyRegex.test(String(field)) &&
-      !Object.prototype.propertyIsEnumerable.call(obj, field)
-    ) {
-      return undefined;
-    }
-    return obj[field];
+    return options.lookupProperty(obj, field);
   });
 }
diff --git a/lib/handlebars/internal/createNewLookupObject.js b/lib/handlebars/internal/createNewLookupObject.js
@@ -0,0 +1,11 @@
+import { extend } from '../utils';
+
+/**
+ * Create a new object with "null"-prototype to avoid truthy results on prototype properties.
+ * The resulting object can be used with "object[property]" to check if a property exists
+ * @param {...object} sources a varargs parameter of source objects that will be merged
+ * @returns {object}
+ */
+export function createNewLookupObject(...sources) {
+  return extend(Object.create(null), ...sources);
+}
diff --git a/lib/handlebars/internal/wrapHelper.js b/lib/handlebars/internal/wrapHelper.js
@@ -0,0 +1,8 @@
+export function wrapHelper(helper, transformOptionsFn) {
+  let wrapper = function(/* dynamic arguments */) {
+    const options = arguments[arguments.length - 1];
+    arguments[arguments.length - 1] = transformOptionsFn(options);
+    return helper.apply(this, arguments);
+  };
+  return wrapper;
+}
diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js
@@ -7,6 +7,8 @@ import {
   REVISION_CHANGES
 } from './base';
 import { moveHelperToHooks } from './helpers';
+import { wrapHelper } from './internal/wrapHelper';
+import { createNewLookupObject } from './internal/createNewLookupObject';
 
 export function checkRevision(compilerInfo) {
   const compilerRevision = (compilerInfo && compilerInfo[0]) || 1,
@@ -69,13 +71,17 @@ export function template(templateSpec, env) {
     }
     partial = env.VM.resolvePartial.call(this, partial, context, options);
 
-    let optionsWithHooks = Utils.extend({}, options, { hooks: this.hooks });
+    let extendedOptions = Utils.extend({}, options, {
+      hooks: this.hooks,
+      allowedProtoMethods: this.allowedProtoMethods,
+      allowedProtoProperties: this.allowedProtoProperties
+    });
 
     let result = env.VM.invokePartial.call(
       this,
       partial,
       context,
-      optionsWithHooks
+      extendedOptions
     );
 
     if (result == null && env.compile) {
@@ -84,7 +90,7 @@ export function template(templateSpec, env) {
         templateSpec.compilerOptions,
         env
       );
-      result = options.partials[options.name](context, optionsWithHooks);
+      result = options.partials[options.name](context, extendedOptions);
     }
     if (result != null) {
       if (options.indent) {
@@ -118,10 +124,26 @@ export function template(templateSpec, env) {
       }
       return obj[name];
     },
+    lookupProperty: function(parent, propertyName) {
+      let result = parent[propertyName];
+      if (Object.prototype.hasOwnProperty.call(parent, propertyName)) {
+        return result;
+      }
+      const whitelist =
+        typeof result === 'function'
+          ? container.allowedProtoMethods
+          : container.allowedProtoProperties;
+
+      if (whitelist[propertyName] === true) {
+        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];
         }
       }
@@ -229,7 +251,9 @@ export function template(templateSpec, env) {
 
   ret._setup = function(options) {
     if (!options.partial) {
-      container.helpers = Utils.extend({}, env.helpers, options.helpers);
+      let mergedHelpers = Utils.extend({}, env.helpers, options.helpers);
+      wrapHelpersToPassLookupProperty(mergedHelpers, container);
+      container.helpers = mergedHelpers;
 
       if (templateSpec.usePartial) {
         // Use mergeIfNeeded here to prevent compiling global partials multiple times
@@ -247,13 +271,21 @@ export function template(templateSpec, env) {
       }
 
       container.hooks = {};
+      container.allowedProtoProperties = createNewLookupObject(
+        options.allowedProtoProperties
+      );
+      container.allowedProtoMethods = createNewLookupObject(
+        options.allowedProtoMethods
+      );
 
       let keepHelperInHelpers =
         options.allowCallsToHelperMissing ||
         templateWasPrecompiledWithCompilerV7;
       moveHelperToHooks(container, 'helperMissing', keepHelperInHelpers);
       moveHelperToHooks(container, 'blockHelperMissing', keepHelperInHelpers);
     } else {
+      container.allowedProtoProperties = options.allowedProtoProperties;
+      container.allowedProtoMethods = options.allowedProtoMethods;
       container.helpers = options.helpers;
       container.partials = options.partials;
       container.decorators = options.decorators;
@@ -405,3 +437,17 @@ function executeDecorators(fn, prog, container, depths, data, blockParams) {
   }
   return prog;
 }
+
+function wrapHelpersToPassLookupProperty(mergedHelpers, container) {
+  Object.keys(mergedHelpers).forEach(helperName => {
+    let helper = mergedHelpers[helperName];
+    mergedHelpers[helperName] = passLookupPropertyOption(helper, container);
+  });
+}
+
+function passLookupPropertyOption(helper, container) {
+  const lookupProperty = container.lookupProperty;
+  return wrapHelper(helper, options => {
+    return Utils.extend({ lookupProperty }, options);
+  });
+}
diff --git a/spec/blocks.js b/spec/blocks.js
@@ -275,6 +275,7 @@ describe('blocks', function() {
         'Goodbye cruel OMG!'
       );
     });
+
     it('block with deep recursive pathed lookup', function() {
       var string =
         '{{#outer}}Goodbye {{#inner}}cruel {{omg.yes}}{{/inner}}{{/outer}}';

diff --git a/spec/helpers.js b/spec/helpers.js
@@ -1328,4 +1328,15 @@ describe('helpers', function() {
       );
     });
   });
+
+  describe('the lookupProperty-option', function() {
+    it('should be passed to custom helpers', function() {
+      expectTemplate('{{testHelper}}')
+        .withHelper('testHelper', function testHelper(options) {
+          return options.lookupProperty(this, 'testProperty');
+        })
+        .withInput({ testProperty: 'abc' })
+        .toCompileTo('abc');
+    });
+  });
 });
diff --git a/spec/javascript-compiler.js b/spec/javascript-compiler.js
@@ -81,4 +81,29 @@ describe('javascript-compiler api', function() {
       shouldCompileTo('{{foo}}', { foo: 'food' }, 'food_foo');
     });
   });
+
+  describe('#isValidJavaScriptVariableName', function() {
+    // It is there and accessible and could be used by someone. That's why we don't remove it
+    // it 4.x. But if we keep it, we add a test
+    // This test should not encourage you to use the function. It is not needed any more
+    // and might be removed in 5.0
+    ['test', 'abc123', 'abc_123'].forEach(function(validVariableName) {
+      it("should return true for '" + validVariableName + "'", function() {
+        expect(
+          handlebarsEnv.JavaScriptCompiler.isValidJavaScriptVariableName(
+            validVariableName
+          )
+        ).to.be.true();
+      });
+    });
+    [('123test', 'abc()', 'abc.cde')].forEach(function(invalidVariableName) {
+      it("should return true for '" + invalidVariableName + "'", function() {
+        expect(
+          handlebarsEnv.JavaScriptCompiler.isValidJavaScriptVariableName(
+            invalidVariableName
+          )
+        ).to.be.false();
+      });
+    });
+  });
 });
diff --git a/spec/regressions.js b/spec/regressions.js
@@ -390,7 +390,7 @@ describe('Regressions', function() {
     it('should compile and execute templates', function() {
       var newHandlebarsInstance = Handlebars.create();
 
-      registerTemplate(newHandlebarsInstance);
+      registerTemplate(newHandlebarsInstance, compiledTemplateVersion7());
       newHandlebarsInstance.registerHelper('loud', function(value) {
         return value.toUpperCase();
       });
@@ -405,19 +405,39 @@ describe('Regressions', function() {
 
       shouldThrow(
         function() {
-          registerTemplate(newHandlebarsInstance);
+          registerTemplate(newHandlebarsInstance, compiledTemplateVersion7());
           newHandlebarsInstance.templates['test.hbs']({});
         },
         Handlebars.Exception,
         'Missing helper: "loud"'
       );
     });
 
-    // This is a only slightly modified precompiled templated from compiled with 4.2.1
-    function registerTemplate(Handlebars) {
+    it('should pass "options.lookupProperty" to "lookup"-helper, even with old templates', function() {
+      var newHandlebarsInstance = Handlebars.create();
+      registerTemplate(
+        newHandlebarsInstance,
+        compiledTemplateVersion7_usingLookupHelper()
+      );
+
+      newHandlebarsInstance.templates['test.hbs']({});
+
+      expect(
+        newHandlebarsInstance.templates['test.hbs']({
+          property: 'a',
+          test: { a: 'b' }
+        })
+      ).to.equal('b');
+    });
+
+    function registerTemplate(Handlebars, compileTemplate) {
       var template = Handlebars.template,
         templates = (Handlebars.templates = Handlebars.templates || {});
-      templates['test.hbs'] = template({
+      templates['test.hbs'] = template(compileTemplate);
+    }
+
+    function compiledTemplateVersion7() {
+      return {
         compiler: [7, '>= 4.0.0'],
         main: function(container, depth0, helpers, partials, data) {
           return (
@@ -435,7 +455,29 @@ describe('Regressions', function() {
           );
         },
         useData: true
-      });
+      };
+    }
+
+    function compiledTemplateVersion7_usingLookupHelper() {
+      // This is the compiled version of "{{lookup test property}}"
+      return {
+        compiler: [7, '>= 4.0.0'],
+        main: function(container, depth0, helpers, partials, data) {
+          return container.escapeExpression(
+            helpers.lookup.call(
+              depth0 != null ? depth0 : container.nullContext || {},
+              depth0 != null ? depth0.test : depth0,
+              depth0 != null ? depth0.property : depth0,
+              {
+                name: 'lookup',
+                hash: {},
+                data: data
+              }
+            )
+          );
+        },
+        useData: true
+      };
     }
   });