From 198887808780bbef9dba67a8af68ece091d5baa7 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Sun, 17 Nov 2019 21:29:47 +0100 Subject: [PATCH] fix: add more properties required to be enumerable - __defineGetter__, __defineSetter__, __lookupGetter__, __proto__ --- lib/handlebars/compiler/compiler.js | 15 ++-------- .../compiler/javascript-compiler.js | 6 ++-- lib/handlebars/helpers/lookup.js | 4 ++- spec/env/common.js | 29 +++++++++++++------ spec/security.js | 24 +++++++++++++-- 5 files changed, 49 insertions(+), 29 deletions(-) diff --git a/lib/handlebars/compiler/compiler.js b/lib/handlebars/compiler/compiler.js index 894f7a7c0..b56ca5c26 100644 --- 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 index b21df1d8a..4acdb50dd 100644 --- 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 index 0654cc393..cba452451 100644 --- 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 index 3903e35d4..ac9421845 100644 --- 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 index 876b67292..18bfa5dd7 100644 --- 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(''); + }); + }); });