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

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] &&

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);
   });
 }

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);
+}

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;
+}

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,6 +271,12 @@ export function template(templateSpec, env) {
       }
 
       container.hooks = {};
+      container.allowedProtoProperties = createNewLookupObject(
+        options.allowedProtoProperties
+      );
+      container.allowedProtoMethods = createNewLookupObject(
+        options.allowedProtoMethods
+      );
 
       let keepHelperInHelpers =
         options.allowCallsToHelperMissing ||
@@ -254,6 +284,8 @@ export function template(templateSpec, env) {
       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);
+  });
+}

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}}';

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');
+    });
+  });
 });

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();
+      });
+    });
+  });
 });

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,7 +405,7 @@ describe('Regressions', function() {
 
       shouldThrow(
         function() {
-          registerTemplate(newHandlebarsInstance);
+          registerTemplate(newHandlebarsInstance, compiledTemplateVersion7());
           newHandlebarsInstance.templates['test.hbs']({});
         },
         Handlebars.Exception,
@@ -413,11 +413,31 @@ describe('Regressions', function() {
       );
     });
 
-    // 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
+      };
     }
   });
 

spec/security.js

@@ -19,7 +19,7 @@ describe('security issues', function() {
         .toCompileTo('');
     });
 
-    it('should allow the "constructor" property to be accessed if it is enumerable', function() {
+    it('should allow the "constructor" property to be accessed if it is an "ownProperty"', function() {
       shouldCompileTo(
         '{{constructor.name}}',
         {
@@ -40,7 +40,7 @@ describe('security issues', function() {
       );
     });
 
-    it('should allow the "constructor" property to be accessed if it is enumerable', function() {
+    it('should allow the "constructor" property to be accessed if it is an "own property"', function() {
       shouldCompileTo(
         '{{lookup (lookup this "constructor") "name"}}',
         {
@@ -51,27 +51,6 @@ describe('security issues', function() {
         'here we go'
       );
     });
-
-    it('should allow prototype properties that are not constructors', function() {
-      function TestClass() {}
-
-      Object.defineProperty(TestClass.prototype, 'abc', {
-        get: function() {
-          return 'xyz';
-        }
-      });
-
-      shouldCompileTo(
-        '{{#with this as |obj|}}{{obj.abc}}{{/with}}',
-        new TestClass(),
-        'xyz'
-      );
-      shouldCompileTo(
-        '{{#with this as |obj|}}{{lookup obj "abc"}}{{/with}}',
-        new TestClass(),
-        'xyz'
-      );
-    });
   });
 
   describe('GH-1558: Prevent explicit call of helperMissing-helpers', function() {
@@ -171,38 +150,183 @@ describe('security issues', function() {
   });
 
   describe('GH-1595', function() {
-    it('properties, that are required to be enumerable', function() {
+    it('properties, that are required to be own properties', function() {
       expectTemplate('{{constructor}}')
         .withInput({})
         .toCompileTo('');
+
       expectTemplate('{{__defineGetter__}}')
         .withInput({})
         .toCompileTo('');
+
       expectTemplate('{{__defineSetter__}}')
         .withInput({})
         .toCompileTo('');
+
       expectTemplate('{{__lookupGetter__}}')
         .withInput({})
         .toCompileTo('');
+
       expectTemplate('{{__proto__}}')
         .withInput({})
         .toCompileTo('');
 
-      expectTemplate('{{lookup "constructor"}}')
+      expectTemplate('{{lookup this "constructor"}}')
         .withInput({})
         .toCompileTo('');
-      expectTemplate('{{lookup "__defineGetter__"}}')
+
+      expectTemplate('{{lookup this "__defineGetter__"}}')
         .withInput({})
         .toCompileTo('');
-      expectTemplate('{{lookup "__defineSetter__"}}')
+
+      expectTemplate('{{lookup this "__defineSetter__"}}')
         .withInput({})
         .toCompileTo('');
-      expectTemplate('{{lookup "__lookupGetter__"}}')
+
+      expectTemplate('{{lookup this "__lookupGetter__"}}')
         .withInput({})
         .toCompileTo('');
-      expectTemplate('{{lookup "__proto__"}}')
+
+      expectTemplate('{{lookup this "__proto__"}}')
         .withInput({})
         .toCompileTo('');
     });
+
+    describe('GH-1631: disallow access to prototype functions', function() {
+      function TestClass() {}
+
+      TestClass.prototype.aProperty = 'propertyValue';
+      TestClass.prototype.aMethod = function() {
+        return 'returnValue';
+      };
+
+      describe('control access to prototype methods via "allowedProtoMethods"', function() {
+        it('should be prohibited by default', function() {
+          expectTemplate('{{aMethod}}')
+            .withInput(new TestClass())
+            .toCompileTo('');
+        });
+
+        it('can be allowed', function() {
+          expectTemplate('{{aMethod}}')
+            .withInput(new TestClass())
+            .withRuntimeOptions({
+              allowedProtoMethods: {
+                aMethod: true
+              }
+            })
+            .toCompileTo('returnValue');
+        });
+
+        it('should be prohibited by default (in "compat" mode)', function() {
+          expectTemplate('{{aMethod}}')
+            .withInput(new TestClass())
+            .withCompileOptions({ compat: true })
+            .toCompileTo('');
+        });
+
+        it('can be allowed (in "compat" mode)', function() {
+          expectTemplate('{{aMethod}}')
+            .withInput(new TestClass())
+            .withCompileOptions({ compat: true })
+            .withRuntimeOptions({
+              allowedProtoMethods: {
+                aMethod: true
+              }
+            })
+            .toCompileTo('returnValue');
+        });
+
+        it('should cause the recursive lookup by default (in "compat" mode)', function() {
+          expectTemplate('{{#aString}}{{trim}}{{/aString}}')
+            .withInput({ aString: '  abc  ', trim: 'trim' })
+            .withCompileOptions({ compat: true })
+            .toCompileTo('trim');
+        });
+
+        it('should not cause the recursive lookup if allowed through options(in "compat" mode)', function() {
+          expectTemplate('{{#aString}}{{trim}}{{/aString}}')
+            .withInput({ aString: '  abc  ', trim: 'trim' })
+            .withCompileOptions({ compat: true })
+            .withRuntimeOptions({
+              allowedProtoMethods: {
+                trim: true
+              }
+            })
+            .toCompileTo('abc');
+        });
+      });
+
+      describe('control access to prototype non-methods via "allowedProtoProperties"', function() {
+        it('should be prohibited by default', function() {
+          expectTemplate('{{aProperty}}')
+            .withInput(new TestClass())
+            .toCompileTo('');
+        });
+
+        it('can be turned on', function() {
+          expectTemplate('{{aProperty}}')
+            .withInput(new TestClass())
+            .withRuntimeOptions({
+              allowedProtoProperties: {
+                aProperty: true
+              }
+            })
+            .toCompileTo('propertyValue');
+        });
+
+        it('should be prohibited by default (in "compat" mode)', function() {
+          expectTemplate('{{aProperty}}')
+            .withInput(new TestClass())
+            .withCompileOptions({ compat: true })
+            .toCompileTo('');
+        });
+
+        it('can be turned on (in "compat" mode)', function() {
+          expectTemplate('{{aProperty}}')
+            .withInput(new TestClass())
+            .withCompileOptions({ compat: true })
+            .withRuntimeOptions({
+              allowedProtoProperties: {
+                aProperty: true
+              }
+            })
+            .toCompileTo('propertyValue');
+        });
+      });
+
+      describe('compatibility with old runtimes, that do not provide the function "container.lookupProperty"', function() {
+        beforeEach(function simulateRuntimeWithoutLookupProperty() {
+          var oldTemplateMethod = handlebarsEnv.template;
+          sinon.replace(handlebarsEnv, 'template', function(templateSpec) {
+            templateSpec.main = wrapToAdjustContainer(templateSpec.main);
+            return oldTemplateMethod.call(this, templateSpec);
+          });
+        });
+
+        afterEach(function() {
+          sinon.restore();
+        });
+
+        it('should work with simple properties', function() {
+          expectTemplate('{{aProperty}}')
+            .withInput({ aProperty: 'propertyValue' })
+            .toCompileTo('propertyValue');
+        });
+
+        it('should work with Array.prototype.length', function() {
+          expectTemplate('{{anArray.length}}')
+            .withInput({ anArray: ['a', 'b', 'c'] })
+            .toCompileTo('3');
+        });
+      });
+    });
   });
 });
+
+function wrapToAdjustContainer(precompiledTemplateFunction) {
+  return function templateFunctionWrapper(container /*, more args */) {
+    delete container.lookupProperty;
+    return precompiledTemplateFunction.apply(this, arguments);
+  };
+}

types/index.d.ts

@@ -30,6 +30,8 @@ declare namespace Handlebars {
       data?: any;
       blockParams?: any[];
       allowCallsToHelperMissing?: boolean;
+      allowedProtoProperties?: { [name: string]: boolean }
+      allowedProtoMethods?: { [name: string]: boolean }
   }
 
   export interface HelperOptions {

types/test.ts

@@ -240,3 +240,13 @@ function testExceptionWithNodeTypings() {
   let fileName: string = exception.fileName;
   let stack: string | undefined = exception.stack;
 }
+
+function testProtoPropertyControlOptions() {
+  Handlebars.compile('test')(
+    {},
+    {
+      allowedProtoMethods: { allowedMethod: true, forbiddenMethod: false },
+      allowedProtoProperties: { allowedProperty: true, forbiddenProperty: false }
+    }
+  );
+}