From 7af1c12db6222ab4b689bb60820628209d295049 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Thu, 9 Jan 2020 23:00:32 +0100 Subject: [PATCH 1/4] feat: default options for controlling proto access This commmit adds the runtime options - `allowProtoPropertiesByDefault` (boolean, default: false) and - `allowProtoMethodsByDefault` (boolean, default: false)` which can be used to allow access to prototype properties and functions in general. Specific properties and methods can still be disabled from access via `allowedProtoProperties` and `allowedProtoMethods` by setting the corresponding values to false. The methods `constructor`, `__defineGetter__`, `__defineSetter__`, `__lookupGetter__` and the property `__proto__` will be disabled, even if the allow...ByDefault-options are set to true. In order to allow access to those properties and methods, they have to be explicitly set to true in the 'allowedProto...'-options. A warning is logged when the a proto-access it attempted and denied by default (i.e. if no option is set by the user to make the access decision explicit) --- ...pObject.js => create-new-lookup-object.js} | 0 lib/handlebars/internal/proto-access.js | 55 ++++ lib/handlebars/runtime.js | 29 +- spec/security.js | 275 +++++++++++------- types/index.d.ts | 6 +- types/test.ts | 6 +- 6 files changed, 251 insertions(+), 120 deletions(-) rename lib/handlebars/internal/{createNewLookupObject.js => create-new-lookup-object.js} (100%) create mode 100644 lib/handlebars/internal/proto-access.js diff --git a/lib/handlebars/internal/createNewLookupObject.js b/lib/handlebars/internal/create-new-lookup-object.js similarity index 100% rename from lib/handlebars/internal/createNewLookupObject.js rename to lib/handlebars/internal/create-new-lookup-object.js diff --git a/lib/handlebars/internal/proto-access.js b/lib/handlebars/internal/proto-access.js new file mode 100644 index 000000000..59bf14448 --- /dev/null +++ b/lib/handlebars/internal/proto-access.js @@ -0,0 +1,55 @@ +import { createNewLookupObject } from './create-new-lookup-object'; + +export function createProtoAccessControl(runtimeOptions) { + let defaultMethodWhiteList = Object.create(null); + defaultMethodWhiteList['constructor'] = false; + defaultMethodWhiteList['__defineGetter__'] = false; + defaultMethodWhiteList['__defineSetter__'] = false; + defaultMethodWhiteList['__lookupGetter__'] = false; + + let defaultPropertyWhiteList = Object.create(null); + // eslint-disable-next-line no-proto + defaultPropertyWhiteList['__proto__'] = false; + + return { + properties: { + whitelist: createNewLookupObject( + defaultPropertyWhiteList, + runtimeOptions.allowedProtoProperties + ), + defaultValue: runtimeOptions.allowProtoPropertiesByDefault + }, + methods: { + whitelist: createNewLookupObject( + defaultMethodWhiteList, + runtimeOptions.allowedProtoMethods + ), + defaultValue: runtimeOptions.allowProtoMethodsByDefault + } + }; +} + +export function resultIsAllowed(result, protoAccessControl, propertyName) { + if (typeof result === 'function') { + return checkWhiteList(protoAccessControl.methods, propertyName); + } else { + return checkWhiteList(protoAccessControl.properties, propertyName); + } +} + +function checkWhiteList(protoAccessControlForType, propertyName) { + if (protoAccessControlForType.whitelist[propertyName] !== undefined) { + return protoAccessControlForType.whitelist[propertyName] === true; + } + if (protoAccessControlForType.defaultValue !== undefined) { + return protoAccessControlForType.defaultValue; + } + + // eslint-disable-next-line no-console + console.error( + `Handlebars: Access has been denied to resolve the property "${propertyName}" because it is not an "own property" of its parent.\n` + + `You can add a runtime option to disable the check or this warning:\n` + + `See http://localhost:8080/api-reference/runtime-options.html#options-to-control-prototype-access for details` + ); + return false; +} diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 7d63e87b8..67a500d79 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -8,7 +8,10 @@ import { } from './base'; import { moveHelperToHooks } from './helpers'; import { wrapHelper } from './internal/wrapHelper'; -import { createNewLookupObject } from './internal/createNewLookupObject'; +import { + createProtoAccessControl, + resultIsAllowed +} from './internal/proto-access'; export function checkRevision(compilerInfo) { const compilerRevision = (compilerInfo && compilerInfo[0]) || 1, @@ -73,8 +76,7 @@ export function template(templateSpec, env) { let extendedOptions = Utils.extend({}, options, { hooks: this.hooks, - allowedProtoMethods: this.allowedProtoMethods, - allowedProtoProperties: this.allowedProtoProperties + protoAccessControl: this.protoAccessControl }); let result = env.VM.invokePartial.call( @@ -126,15 +128,14 @@ export function template(templateSpec, env) { }, lookupProperty: function(parent, propertyName) { let result = parent[propertyName]; + if (result == null) { + return result; + } if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { return result; } - const whitelist = - typeof result === 'function' - ? container.allowedProtoMethods - : container.allowedProtoProperties; - if (whitelist[propertyName] === true) { + if (resultIsAllowed(result, container.protoAccessControl, propertyName)) { return result; } return undefined; @@ -237,6 +238,7 @@ export function template(templateSpec, env) { ) ); } + main = executeDecorators( templateSpec.main, main, @@ -247,6 +249,7 @@ export function template(templateSpec, env) { ); return main(context, options); } + ret.isTop = true; ret._setup = function(options) { @@ -271,12 +274,7 @@ export function template(templateSpec, env) { } container.hooks = {}; - container.allowedProtoProperties = createNewLookupObject( - options.allowedProtoProperties - ); - container.allowedProtoMethods = createNewLookupObject( - options.allowedProtoMethods - ); + container.protoAccessControl = createProtoAccessControl(options); let keepHelperInHelpers = options.allowCallsToHelperMissing || @@ -284,8 +282,7 @@ export function template(templateSpec, env) { moveHelperToHooks(container, 'helperMissing', keepHelperInHelpers); moveHelperToHooks(container, 'blockHelperMissing', keepHelperInHelpers); } else { - container.allowedProtoProperties = options.allowedProtoProperties; - container.allowedProtoMethods = options.allowedProtoMethods; + container.protoAccessControl = options.protoAccessControl; // internal option container.helpers = options.helpers; container.partials = options.partials; container.decorators = options.decorators; diff --git a/spec/security.js b/spec/security.js index 7bf9d89a0..bf0be1485 100644 --- a/spec/security.js +++ b/spec/security.js @@ -149,176 +149,251 @@ describe('security issues', function() { }); }); - describe('GH-1595', 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 this "constructor"}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{lookup this "__defineGetter__"}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{lookup this "__defineSetter__"}}') - .withInput({}) - .toCompileTo(''); + describe('GH-1595: dangerous properties', function() { + var templates = [ + '{{constructor}}', + '{{__defineGetter__}}', + '{{__defineSetter__}}', + '{{__lookupGetter__}}', + '{{__proto__}}', + '{{lookup this "constructor"}}', + '{{lookup this "__defineGetter__"}}', + '{{lookup this "__defineSetter__"}}', + '{{lookup this "__lookupGetter__"}}', + '{{lookup this "__proto__"}}' + ]; + + templates.forEach(function(template) { + describe('access should be denied to ' + template, function() { + it('by default', function() { + expectTemplate(template) + .withInput({}) + .toCompileTo(''); + }); + it(' with proto-access enabled', function() { + expectTemplate(template) + .withInput({}) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: true, + allowProtoMethodsByDefault: true + }) + .toCompileTo(''); + }); + }); + }); + }); + describe('GH-1631: disallow access to prototype functions', function() { + function TestClass() {} - expectTemplate('{{lookup this "__lookupGetter__"}}') - .withInput({}) - .toCompileTo(''); + TestClass.prototype.aProperty = 'propertyValue'; + TestClass.prototype.aMethod = function() { + return 'returnValue'; + }; - expectTemplate('{{lookup this "__proto__"}}') - .withInput({}) - .toCompileTo(''); + afterEach(function() { + sinon.restore(); }); - describe('GH-1631: disallow access to prototype functions', function() { - function TestClass() {} + describe('control access to prototype methods via "allowedProtoMethods"', function() { + checkProtoMethodAccess({}); + + describe('in compat mode', function() { + checkProtoMethodAccess({ compat: true }); + }); - TestClass.prototype.aProperty = 'propertyValue'; - TestClass.prototype.aMethod = function() { - return 'returnValue'; - }; + function checkProtoMethodAccess(compileOptions) { + it('should be prohibited by default and log a warning', function() { + var spy = sinon.spy(console, 'error'); - describe('control access to prototype methods via "allowedProtoMethods"', function() { - it('should be prohibited by default', function() { expectTemplate('{{aMethod}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .toCompileTo(''); + + expect(spy.calledOnce).to.be.true(); + expect(spy.args[0][0]).to.match(/Handlebars: Access has been denied/); }); - it('can be allowed', function() { + it('can be allowed, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ allowedProtoMethods: { aMethod: true } }) .toCompileTo('returnValue'); + + expect(spy.callCount).to.equal(0); }); - it('should be prohibited by default (in "compat" mode)', function() { + it('can be turned on by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) - .toCompileTo(''); + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoMethodsByDefault: true + }) + .toCompileTo('returnValue'); + + expect(spy.callCount).to.equal(0); }); - it('can be allowed (in "compat" mode)', function() { + it('can be turned off by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) + .withCompileOptions(compileOptions) .withRuntimeOptions({ - allowedProtoMethods: { - aMethod: true - } + allowProtoMethodsByDefault: false }) - .toCompileTo('returnValue'); - }); + .toCompileTo(''); - 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'); + expect(spy.callCount).to.equal(0); }); - 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 }) + it('can be turned off, if turned on by default', function() { + expectTemplate('{{aMethod}}') + .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ + allowProtoMethodsByDefault: true, allowedProtoMethods: { - trim: true + aMethod: false } }) - .toCompileTo('abc'); + .toCompileTo(''); }); + } + + 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" and "allowProtoPropertiesByDefault', function() { + checkProtoPropertyAccess({}); + + describe('in compat-mode', function() { + checkProtoPropertyAccess({ compat: true }); + }); + + function checkProtoPropertyAccess(compileOptions) { + it('should be prohibited by default and log a warning', function() { + var spy = sinon.spy(console, 'error'); + + expectTemplate('{{aProperty}}') + .withInput(new TestClass()) + .withCompileOptions(compileOptions) + .toCompileTo(''); + + expect(spy.calledOnce).to.be.true(); + expect(spy.args[0][0]).to.match(/Handlebars: Access has been denied/); + }); + + it('can be explicitly prohibited by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); - describe('control access to prototype non-methods via "allowedProtoProperties"', function() { - it('should be prohibited by default', function() { expectTemplate('{{aProperty}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: false + }) .toCompileTo(''); + + expect(spy.callCount).to.equal(0); }); - it('can be turned on', function() { + it('can be turned on, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aProperty}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ allowedProtoProperties: { aProperty: true } }) .toCompileTo('propertyValue'); + + expect(spy.callCount).to.equal(0); }); - it('should be prohibited by default (in "compat" mode)', function() { + it('can be turned on by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aProperty}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) - .toCompileTo(''); + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: true + }) + .toCompileTo('propertyValue'); + + expect(spy.callCount).to.equal(0); }); - it('can be turned on (in "compat" mode)', function() { + it('can be turned off, if turned on by default', function() { expectTemplate('{{aProperty}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) + .withCompileOptions(compileOptions) .withRuntimeOptions({ + allowProtoPropertiesByDefault: true, allowedProtoProperties: { - aProperty: true + aProperty: false } }) - .toCompileTo('propertyValue'); + .toCompileTo(''); }); - }); + } + }); - 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); - }); + 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(); - }); + afterEach(function() { + sinon.restore(); + }); - it('should work with simple properties', function() { - expectTemplate('{{aProperty}}') - .withInput({ aProperty: 'propertyValue' }) - .toCompileTo('propertyValue'); - }); + 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'); - }); + it('should work with Array.prototype.length', function() { + expectTemplate('{{anArray.length}}') + .withInput({ anArray: ['a', 'b', 'c'] }) + .toCompileTo('3'); }); }); }); diff --git a/types/index.d.ts b/types/index.d.ts index 606741f5a..1fa83680d 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -30,8 +30,10 @@ declare namespace Handlebars { data?: any; blockParams?: any[]; allowCallsToHelperMissing?: boolean; - allowedProtoProperties?: { [name: string]: boolean } - allowedProtoMethods?: { [name: string]: boolean } + allowedProtoProperties?: { [name: string]: boolean }; + allowedProtoMethods?: { [name: string]: boolean }; + allowProtoPropertiesByDefault?: boolean; + allowProtoMethodsByDefault?: boolean; } export interface HelperOptions { diff --git a/types/test.ts b/types/test.ts index b081ae4fb..7a34b7eba 100644 --- a/types/test.ts +++ b/types/test.ts @@ -241,12 +241,14 @@ function testExceptionWithNodeTypings() { let stack: string | undefined = exception.stack; } -function testProtoPropertyControlOptions() { +function testProtoAccessControlControlOptions() { Handlebars.compile('test')( {}, { allowedProtoMethods: { allowedMethod: true, forbiddenMethod: false }, - allowedProtoProperties: { allowedProperty: true, forbiddenProperty: false } + allowedProtoProperties: { allowedProperty: true, forbiddenProperty: false }, + allowProtoMethodsByDefault: true, + allowProtoPropertiesByDefault: false, } ); } From 575d8772e2ccf05da235c596dd3405ae74194e1b Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Fri, 10 Jan 2020 17:05:23 +0100 Subject: [PATCH 2/4] fix: use "logger" instead of console.error ... to be graceful with older browser without "console" --- lib/handlebars/internal/proto-access.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/handlebars/internal/proto-access.js b/lib/handlebars/internal/proto-access.js index 59bf14448..a10f5124c 100644 --- a/lib/handlebars/internal/proto-access.js +++ b/lib/handlebars/internal/proto-access.js @@ -1,4 +1,5 @@ import { createNewLookupObject } from './create-new-lookup-object'; +import * as logger from '../logger'; export function createProtoAccessControl(runtimeOptions) { let defaultMethodWhiteList = Object.create(null); @@ -44,9 +45,9 @@ function checkWhiteList(protoAccessControlForType, propertyName) { if (protoAccessControlForType.defaultValue !== undefined) { return protoAccessControlForType.defaultValue; } - // eslint-disable-next-line no-console - console.error( + logger.log( + 'error', `Handlebars: Access has been denied to resolve the property "${propertyName}" because it is not an "own property" of its parent.\n` + `You can add a runtime option to disable the check or this warning:\n` + `See http://localhost:8080/api-reference/runtime-options.html#options-to-control-prototype-access for details` From 1f0834b1a2937150923f9de849b9612bd1969d11 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Fri, 10 Jan 2020 17:23:31 +0100 Subject: [PATCH 3/4] Update release notes --- release-notes.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/release-notes.md b/release-notes.md index d75b32024..ac4fc5041 100644 --- a/release-notes.md +++ b/release-notes.md @@ -2,7 +2,22 @@ ## Development -[Commits](https://github.com/wycats/handlebars.js/compare/v4.6.0...master) +[Commits](https://github.com/wycats/handlebars.js/compare/v4.7.0...master) + +## v4.7.0 - January 10th, 2020 + +Features: + +- feat: default options for controlling proto access - 7af1c12, #1635 + - This makes it possible to disable the prototype access restrictions added in 4.6.0 + - an error is logged in the console, if access to prototype properties is attempted and denied + and no explicit configuration has taken place. + +Compatibility notes: + +- no compatibilities are expected + +[Commits](https://github.com/wycats/handlebars.js/compare/v4.6.0...v4.7.0) ## v4.6.0 - January 8th, 2020 From 0d5c807017f8ba6c6d947f9d6852033c8faa2e49 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Fri, 10 Jan 2020 17:24:06 +0100 Subject: [PATCH 4/4] v4.7.0 --- components/bower.json | 2 +- components/handlebars.js.nuspec | 2 +- components/package.json | 2 +- lib/handlebars/base.js | 2 +- package.json | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/components/bower.json b/components/bower.json index c4501c0f9..5bb095bc1 100644 --- a/components/bower.json +++ b/components/bower.json @@ -1,6 +1,6 @@ { "name": "handlebars", - "version": "4.6.0", + "version": "4.7.0", "main": "handlebars.js", "license": "MIT", "dependencies": {} diff --git a/components/handlebars.js.nuspec b/components/handlebars.js.nuspec index 34886aea7..91f7baf80 100644 --- a/components/handlebars.js.nuspec +++ b/components/handlebars.js.nuspec @@ -2,7 +2,7 @@ handlebars.js - 4.6.0 + 4.7.0 handlebars.js Authors https://github.com/wycats/handlebars.js/blob/master/LICENSE https://github.com/wycats/handlebars.js/ diff --git a/components/package.json b/components/package.json index 8e66182b2..d040cda4f 100644 --- a/components/package.json +++ b/components/package.json @@ -1,6 +1,6 @@ { "name": "handlebars", - "version": "4.6.0", + "version": "4.7.0", "license": "MIT", "jspm": { "main": "handlebars", diff --git a/lib/handlebars/base.js b/lib/handlebars/base.js index 88ba0ddcc..a80e0dde4 100644 --- a/lib/handlebars/base.js +++ b/lib/handlebars/base.js @@ -4,7 +4,7 @@ import { registerDefaultHelpers } from './helpers'; import { registerDefaultDecorators } from './decorators'; import logger from './logger'; -export const VERSION = '4.6.0'; +export const VERSION = '4.7.0'; export const COMPILER_REVISION = 8; export const LAST_COMPATIBLE_COMPILER_REVISION = 7; diff --git a/package.json b/package.json index dadcbb543..62e640504 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "handlebars", "barename": "handlebars", - "version": "4.6.0", + "version": "4.7.0", "description": "Handlebars provides the power necessary to let you build semantic templates effectively with no frustration", "homepage": "http://www.handlebarsjs.com/", "keywords": [