From dbe04946cefb03c0e31e9ff4d275bb051105eca5 Mon Sep 17 00:00:00 2001 From: Jakob Linskeseder Date: Fri, 27 Mar 2026 00:38:10 +0100 Subject: [PATCH] Improve security mitigations Instead of validating the AST in the parser, fix the compiler instead by handling the types in a safe way. --- lib/handlebars/compiler/base.js | 67 ---------- lib/handlebars/compiler/compiler.js | 51 ++++---- lib/handlebars/utils.js | 27 ++++ spec/compiler.js | 183 ++++++++++++---------------- spec/security.js | 83 +++++++++++-- 5 files changed, 207 insertions(+), 204 deletions(-) diff --git a/lib/handlebars/compiler/base.js b/lib/handlebars/compiler/base.js index 7ce7d70a..1dd5af1a 100644 --- a/lib/handlebars/compiler/base.js +++ b/lib/handlebars/compiler/base.js @@ -1,7 +1,6 @@ import parser from './parser'; import WhitespaceControl from './whitespace-control'; import * as Helpers from './helpers'; -import Exception from '../exception'; import { extend } from '../utils'; export { parser }; @@ -12,9 +11,6 @@ extend(yy, Helpers); export function parseWithoutProcessing(input, options) { // Just return if an already-compiled AST was passed in. if (input.type === 'Program') { - // When a pre-parsed AST is passed in, validate all node values to prevent - // code injection via type-confused literals. - validateInputAst(input); return input; } @@ -36,66 +32,3 @@ export function parse(input, options) { return strip.accept(ast); } - -function validateInputAst(ast) { - validateAstNode(ast); -} - -function validateAstNode(node) { - if (node == null) { - return; - } - - if (Array.isArray(node)) { - node.forEach(validateAstNode); - return; - } - - if (typeof node !== 'object') { - return; - } - - if (node.type === 'PathExpression') { - if (!isValidDepth(node.depth)) { - throw new Exception( - 'Invalid AST: PathExpression.depth must be an integer' - ); - } - if (!Array.isArray(node.parts)) { - throw new Exception('Invalid AST: PathExpression.parts must be an array'); - } - for (let i = 0; i < node.parts.length; i++) { - if (typeof node.parts[i] !== 'string') { - throw new Exception( - 'Invalid AST: PathExpression.parts must only contain strings' - ); - } - } - } else if (node.type === 'NumberLiteral') { - if (typeof node.value !== 'number' || !isFinite(node.value)) { - throw new Exception('Invalid AST: NumberLiteral.value must be a number'); - } - } else if (node.type === 'BooleanLiteral') { - if (typeof node.value !== 'boolean') { - throw new Exception( - 'Invalid AST: BooleanLiteral.value must be a boolean' - ); - } - } - - Object.keys(node).forEach(propertyName => { - if (propertyName === 'loc') { - return; - } - validateAstNode(node[propertyName]); - }); -} - -function isValidDepth(depth) { - return ( - typeof depth === 'number' && - isFinite(depth) && - Math.floor(depth) === depth && - depth >= 0 - ); -} diff --git a/lib/handlebars/compiler/compiler.js b/lib/handlebars/compiler/compiler.js index 33bd2b11..857dfd34 100644 --- a/lib/handlebars/compiler/compiler.js +++ b/lib/handlebars/compiler/compiler.js @@ -1,7 +1,13 @@ /* eslint-disable new-cap */ import Exception from '../exception'; -import { isArray, indexOf, extend } from '../utils'; +import { + isArray, + indexOf, + extend, + sanitizeDepth, + sanitizeParts +} from '../utils'; import AST from './ast'; const slice = [].slice; @@ -243,7 +249,7 @@ Compiler.prototype = { name = path.parts[0], isBlock = program != null || inverse != null; - this.opcode('getContext', path.depth); + this.opcode('getContext', sanitizeDepth(path.depth)); this.opcode('pushProgram', program); this.opcode('pushProgram', inverse); @@ -288,29 +294,32 @@ Compiler.prototype = { }, PathExpression: function(path) { - this.addDepth(path.depth); - this.opcode('getContext', path.depth); + // Sanitize untrusted AST values at the compiler boundary. + // javascript-compiler.js trusts all opcode arguments to be safe. + const depth = sanitizeDepth(path.depth); + const parts = sanitizeParts(path.parts); - let name = path.parts[0], + this.addDepth(depth); + this.opcode('getContext', depth); + + let name = parts[0], scoped = AST.helpers.scopedId(path), - blockParamId = !path.depth && !scoped && this.blockParamIndex(name); + blockParamId = !depth && !scoped && this.blockParamIndex(name); if (blockParamId) { - this.opcode('lookupBlockParam', blockParamId, path.parts); + this.opcode( + 'lookupBlockParam', + [Number(blockParamId[0]), Number(blockParamId[1])], + parts + ); } else if (!name) { // Context reference, i.e. `{{foo .}}` or `{{foo ..}}` this.opcode('pushContext'); } else if (path.data) { this.options.data = true; - this.opcode('lookupData', path.depth, path.parts, path.strict); + this.opcode('lookupData', depth, parts, path.strict); } else { - this.opcode( - 'lookupOnContext', - path.parts, - path.falsy, - path.strict, - scoped - ); + this.opcode('lookupOnContext', parts, path.falsy, path.strict, scoped); } }, @@ -319,11 +328,11 @@ Compiler.prototype = { }, NumberLiteral: function(number) { - this.opcode('pushLiteral', number.value); + this.opcode('pushLiteral', Number(number.value)); }, BooleanLiteral: function(bool) { - this.opcode('pushLiteral', bool.value); + this.opcode('pushLiteral', bool.value === true ? 'true' : 'false'); }, UndefinedLiteral: function() { @@ -410,16 +419,16 @@ Compiler.prototype = { pushParam: function(val) { let value = val.value != null ? val.value : val.original || ''; + let depth = sanitizeDepth(val.depth); if (this.stringParams) { if (value.replace) { value = value.replace(/^(\.?\.\/)*/g, '').replace(/\//g, '.'); } - - if (val.depth) { - this.addDepth(val.depth); + if (depth) { + this.addDepth(depth); } - this.opcode('getContext', val.depth || 0); + this.opcode('getContext', depth); this.opcode('pushStringParam', value, val.type); if (val.type === 'SubExpression') { diff --git a/lib/handlebars/utils.js b/lib/handlebars/utils.js index 6fa43b23..7190948e 100644 --- a/lib/handlebars/utils.js +++ b/lib/handlebars/utils.js @@ -114,3 +114,30 @@ export function blockParams(params, ids) { export function appendContextPath(contextPath, id) { return (contextPath ? contextPath + '.' : '') + id; } + +/** + * Coerce an untrusted depth value to a safe non-negative integer. + * Returns `0` for any value that is not a finite, non-negative number. + * + * @param {unknown} depth - The depth value to sanitize. + * @returns {number} A non-negative integer. + */ +export function sanitizeDepth(depth) { + let number = Number(depth); + if (!Number.isFinite(number) || number < 0) { + return 0; + } + return Math.floor(number); +} + +/** + * Return a sanitized copy of a PathExpression AST node's parts array. + * Coerces each element to a string, or returns an empty array if parts + * is not an array. + * + * @param {unknown} parts - The parts value to sanitize. + * @returns {string[]} A safe string array. + */ +export function sanitizeParts(parts) { + return Array.isArray(parts) ? parts.map(String) : []; +} diff --git a/spec/compiler.js b/spec/compiler.js index d51f2f1b..26d082af 100644 --- a/spec/compiler.js +++ b/spec/compiler.js @@ -128,121 +128,90 @@ describe('compiler', function() { ); }); - it('should reject AST with invalid PathExpression depth', function() { - shouldThrow( - function() { - Handlebars.compile({ - type: 'Program', - body: [ - { - type: 'MustacheStatement', - escaped: true, - strip: { open: false, close: false }, - path: { - type: 'PathExpression', - data: false, - depth: '0', - parts: ['this'], - original: 'this' - }, - params: [] - } - ] - })(); - }, - Error, - 'Invalid AST: PathExpression.depth must be an integer' - ); + function createPathExpressionAST(depth, parts) { + return { + type: 'Program', + body: [ + { + type: 'MustacheStatement', + escaped: true, + strip: { open: false, close: false }, + path: { + type: 'PathExpression', + data: false, + depth: depth, + parts: parts, + original: 'this' + }, + params: [] + } + ] + }; + } + + it('should safely handle AST with non-integer PathExpression depth', function() { + // depth '0' is coerced to 0 via Number(), compiles safely + var result = Handlebars.compile(createPathExpressionAST('0', ['this']))(); + expect(result).to.be.a('string'); }); - it('should reject AST with non-array PathExpression parts', function() { - shouldThrow( - function() { - Handlebars.compile({ - type: 'Program', - body: [ - { - type: 'MustacheStatement', - escaped: true, - strip: { open: false, close: false }, - path: { - type: 'PathExpression', - data: false, - depth: 0, - parts: 'this', - original: 'this' - }, - params: [] - } - ] - })(); - }, - Error, - 'Invalid AST: PathExpression.parts must be an array' - ); + it('should safely handle AST with negative PathExpression depth', function() { + // Negative depth is clamped to 0 + var result = Handlebars.compile(createPathExpressionAST(-1, ['this']))(); + expect(result).to.be.a('string'); }); - it('should reject AST with non-string PathExpression part', function() { - shouldThrow( - function() { - Handlebars.compile({ - type: 'Program', - body: [ - { - type: 'MustacheStatement', - escaped: true, - strip: { open: false, close: false }, - path: { - type: 'PathExpression', - data: false, - depth: 0, - parts: [1], - original: 'this' - }, - params: [] - } - ] - })(); - }, - Error, - 'Invalid AST: PathExpression.parts must only contain strings' - ); + it('should safely handle AST with fractional PathExpression depth', function() { + // Fractional depth is floored to an integer + var result = Handlebars.compile(createPathExpressionAST(0.5, ['this']))(); + expect(result).to.be.a('string'); }); - it('should reject AST with invalid BooleanLiteral value type', function() { - shouldThrow( - function() { - Handlebars.compile({ - type: 'Program', - body: [ - { - type: 'MustacheStatement', - escaped: true, - strip: { open: false, close: false }, - path: { - type: 'PathExpression', - data: false, - depth: 0, - parts: ['if'], - original: 'if' - }, - params: [ - { - type: 'BooleanLiteral', - value: 'true', - original: true - } - ] - } - ] - })(); - }, - Error, - 'Invalid AST: BooleanLiteral.value must be a boolean' - ); + it('should safely handle AST with non-array PathExpression parts', function() { + // Non-array parts are coerced to empty array, compiles safely + var result = Handlebars.compile(createPathExpressionAST(0, 'this'))(); + expect(result).to.be.a('string'); }); - it('should ignore loc metadata while validating AST nodes', function() { + it('should safely handle AST with non-string PathExpression part', function() { + // Non-string parts are coerced to strings via String() + var result = Handlebars.compile(createPathExpressionAST(0, [1]))(); + expect(result).to.be.a('string'); + }); + + it('should safely handle AST with non-boolean BooleanLiteral value type', function() { + // The compiler coerces BooleanLiteral.value via === true before + // emitting a pushLiteral opcode, so a non-boolean value like the + // string 'true' becomes the literal 'false'. + var loc = { + source: null, + start: { line: 1, column: 0 }, + end: { line: 1, column: 10 } + }; + var result = Handlebars.compile({ + type: 'Program', + body: [ + { + type: 'MustacheStatement', + escaped: true, + strip: { open: false, close: false }, + loc: loc, + path: { + type: 'BooleanLiteral', + value: 'true', + original: true, + loc: loc + }, + params: [] + } + ] + })(); + // 'true' !== true, so the compiler emits pushLiteral('false'). + // Handlebars does not render falsy values, so the output is empty. + expect(result).to.equal(''); + }); + + it('should ignore loc metadata in AST nodes', function() { equal( Handlebars.compile({ type: 'Program', diff --git a/spec/security.js b/spec/security.js index d43a01e5..435538a5 100644 --- a/spec/security.js +++ b/spec/security.js @@ -450,6 +450,11 @@ describe('security issues', function() { } function createInjectedProgram() { + var loc = { + source: null, + start: { line: 1, column: 0 }, + end: { line: 1, column: 20 } + }; return { type: 'Program', body: [ @@ -460,12 +465,14 @@ describe('security issues', function() { open: false, close: false }, + loc: loc, path: { type: 'PathExpression', data: false, depth: 0, parts: ['lookup'], - original: 'lookup' + original: 'lookup', + loc: loc }, params: [ { @@ -473,12 +480,14 @@ describe('security issues', function() { data: false, depth: 0, parts: [], - original: 'this' + original: 'this', + loc: loc }, { type: 'NumberLiteral', value: '{},{})) + (Function) + (({}', - original: 1 + original: 1, + loc: loc } ] } @@ -486,11 +495,13 @@ describe('security issues', function() { }; } - it('should reject AST NumberLiteral type confusion in compile()', function() { - expect(function() { - var template = Handlebars.compile(createInjectedProgram()); - template({}); - }).to.throw(/Invalid AST/); + it('should neutralize AST NumberLiteral type confusion in compile()', function() { + // The compiler coerces NumberLiteral.value via Number() before + // emitting a pushLiteral opcode, so a type-confused string value + // becomes NaN, preventing code injection. + var template = Handlebars.compile(createInjectedProgram()); + var result = template({}); + expect(result).to.not.contain('Function'); }); it('should reject AST objects passed via dynamic partial lookup', function() { @@ -499,7 +510,61 @@ describe('security issues', function() { template({ payload: createInjectedProgram() }); - }).to.throw(/Invalid AST|could not be found/); + }).to.throw(/could not be found/); + }); + + it('should sanitize param depth in stringParams mode', function() { + // pushParam passes val.depth directly to the getContext opcode. + // In stringParams mode, getContext stores the depth in lastContext, + // which contextName interpolates into generated code as + // 'depths[' + depth + ']'. A malicious depth string can escape the + // bracket expression and inject arbitrary code at template runtime. + // + // With sanitization the depth becomes 0, producing 'depth0' (safe). + // Without sanitization the injected expression executes and throws. + var loc = { + source: null, + start: { line: 1, column: 0 }, + end: { line: 1, column: 20 } + }; + var maliciousAST = { + type: 'Program', + body: [ + { + type: 'MustacheStatement', + escaped: true, + strip: { open: false, close: false }, + loc: loc, + path: { + type: 'PathExpression', + data: false, + depth: 0, + parts: ['lookup'], + original: 'lookup', + loc: loc + }, + params: [ + { + type: 'PathExpression', + data: false, + depth: 'function(){throw new Error("INJECTION")}()', + parts: [], + original: '', + loc: loc + } + ] + } + ] + }; + + var template = Handlebars.compile(maliciousAST, { + stringParams: true + }); + // After sanitization the depth is 0, so the template runs without + // executing the injected throw expression. + expect(function() { + template({}); + }).to.not.throw(); }); });