Skip to content

Commit

Permalink
fix: add more properties required to be enumerable
Browse files Browse the repository at this point in the history
- __defineGetter__, __defineSetter__, __lookupGetter__, __proto__
  • Loading branch information
nknapp committed Nov 18, 2019
1 parent 886ba86 commit 1988878
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 29 deletions.
15 changes: 2 additions & 13 deletions lib/handlebars/compiler/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
},
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions lib/handlebars/compiler/javascript-compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion lib/handlebars/helpers/lookup.js
Original file line number Diff line number Diff line change
@@ -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];
Expand Down
29 changes: 20 additions & 9 deletions spec/env/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
24 changes: 21 additions & 3 deletions spec/security.js
Original file line number Diff line number Diff line change
Expand Up @@ -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('');
});
});
});

0 comments on commit 1988878

Please sign in to comment.