Improve security mitigations

[jaylinski]

Instead of validating the AST in the parser, fix the compiler instead by handling the types in a safe way.

• Check for performance regression
• Upstream to master

[Copilot]

Pull request overview

This PR shifts “untrusted AST” hardening from the parser (parseWithoutProcessing) to the compiler/codegen path, aiming to neutralize type-confusion based code injection by coercing AST-provided values into safe primitives during compilation.

Changes:

• Remove AST value validation from parseWithoutProcessing (no longer throws Invalid AST for malformed nodes).
• Add numeric/boolean-safe opcodes (pushNumber, pushBoolean) and coerce getContext depth.
• Update security/compiler specs to expect safe handling/coercion rather than parser-time rejection.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
spec/security.js	Adjusts the untrusted-AST regression test to check for neutralization rather than “Invalid AST” rejection.
spec/compiler.js	Updates compiler tests to accept malformed AST values and assert safe compilation behavior.
lib/handlebars/compiler/javascript-compiler.js	Adds pushNumber / pushBoolean and coerces getContext depth.
lib/handlebars/compiler/compiler.js	Sanitizes PathExpression depth/parts, and routes Number/Boolean literals through new opcodes.
lib/handlebars/compiler/base.js	Removes AST validation when a pre-parsed Program is passed to parsing entrypoints.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kibertoad]

@jaylinski Here's what Claude has to say about this PR, when I asked it to dig deeper into perf implications:

1. Cost shifts from rare path → hot path

Before: validateInputAst ran only when the caller passed a pre-parsed AST (minority use). Compiling from a string
string skipped it entirely.
After: every compile pays sanitization at every PathExpression, NumberLiteral, BooleanLiteral, and pushParam. For the
dominant "compile from string template" path this is net-new work.

For a typical template with N path expressions, this adds roughly N × (Number + isFinite + Math.floor + Array.isArray

• .map(String)) per compile.

2. sanitizeParts allocates + copies unconditionally

lib/handlebars/utils.js:
return Array.isArray(parts) ? parts.map(String) : [];
map(String) always creates a new array and calls String() on every element, even when parts is already the untouched
['foo','bar'] the parser just produced. That's the 99.9% path.

Suggested fast-path:
if (!Array.isArray(parts)) return [];
for (let i = 0; i < parts.length; i++) {
if (typeof parts[i] !== 'string') return parts.map(String);
}
return parts;
Zero allocation for parser-produced ASTs, same safety for untrusted ASTs. Also worth noting that PathExpression and
ambiguousSexpr both touch path.parts — sanitizeParts is called in PathExpression but ambiguousSexpr still reads
path.parts[0] from the raw node on line 239 of the PR's compiler.js. Minor consistency gap, not a perf issue.

3. Redundant coercion on trusted values

this.opcode('lookupBlockParam', [Number(blockParamId[0]), Number(blockParamId[1])], parts);
blockParamId comes from blockParamIndex, which returns [depth, indexOf(...)] — both are loop-counter / Array.indexOf
integers from trusted internal code, never from the AST. The Number() wrap is defensive-but-unnecessary and also
allocates a new array. Not a hot-hot path but worth removing.

4. sanitizeDepth called twice on the same value

In PathExpression the depth is sanitized once; in pushParam it's sanitized again for each param that's itself a
PathExpression (because pushParam accepts the val and then PathExpression runs). Tiny, but measurable across deep
nested expressions.

5. Net-net

Most of this is micro-cost. The real fix for perf would be:

• inline the checks instead of function-call indirection (V8 will inline them anyway once warm, but cold-compile still
  pays)
• skip sanitizeParts allocation on the fast path (Escaping quotes #2 above is the biggest lever)

Recommendation: ask the author to run npm test with time, and a synthetic "compile 10k templates" bench against 4.x
baseline. Specifically measure the compile-from-string path since that regressed from zero to nonzero cost.

Correctness concerns beyond perf

Behavioral change for BooleanLiteral

Old code: pushLiteral(bool.value) — for a valid AST, value is a boolean and renders as true/false.
New code: pushLiteral(bool.value === true ? 'true' : 'false').

For the valid case this is equivalent (boolean → number/string interpolation in generated code produces the same).
But:

• If bool.value === false, old: pushLiteral(false), emits false in code. New: emits 'false' string → pushLiteral
  outputs it raw → still false in code. OK.
• If bool.value is anything truthy but not === true (e.g., 1, 'true'), old emitted the raw value (potentially
  dangerous); new silently produces false. That's the stated goal, but the previous mitigation threw. This is a
  silent-downgrade: buggy AST-generating tooling that used to get a loud error will now quietly render wrong output.
  Worth calling out in the PR description.

NumberLiteral → NaN

Number('{},{})) + ...') → NaN. pushLiteral(NaN) emits the literal NaN into generated JS. Safe (no code injection), but
the test only asserts result.not.contain('Function') — not that the template errors loudly. Same silent-downgrade
observation as above.

sanitizeDepth clamps negative to 0, old code rejected

Old mitigation threw on depth: -1. New mitigation clamps to 0 and happily compiles — {{../foo}} with a forged depth:
-1 becomes {{foo}} silently. Probably fine, but a conforming-AST-producer that happened to emit -1 (bug) now gets
silent incorrect behavior instead of an error.

Summary

• Security-wise: the PR correctly moves the trust boundary into the compiler and also patches a real gap in pushParam
  that the original mitigation missed. Good.
• Perf-wise: the author shipped without measuring. The main fixable regression is sanitizeParts allocating on every
  PathExpression for the common case — a fast-path check that returns the original array when all elements are strings
  would eliminate it. Other costs are micro.
• Philosophy shift: moves from "reject bad AST loudly" to "coerce to something safe silently." Safer against
  injection, friendlier to fuzzers, but more likely to mask upstream bugs in tools that generate ASTs. Worth being
  explicit about this tradeoff in the PR description.

Would push the author to: (a) add a compile-path bench, (b) fix the sanitizeParts fast path, (c) drop the Number()
wraps on trusted blockParamId, (d) document the silent-coercion behavior change.

[jaylinski]

@kibertoad Thanks, will have a look!

[kibertoad]

@jaylinski

Overview

PR #2143 reworks the AST-injection hardening (GHSA-2w6w-674q-4c4q and friends). Instead of validateInputAst throwing on a malformed pre-parsed AST in parseWithoutProcessing, it removes that validator entirely and coerces untrusted values to safe primitives at the compiler boundary:
sanitizeDepth/sanitizeParts in utils.js, Number() coercion on NumberLiteral and blockParams.length, and === true coercion on BooleanLiteral. The security goal is met, and it closes a real gap the old mitigation missed (pushParam previously passed val.depth raw). But the two specific performance fixes from the earlier review were not applied, and the author's own "Check for performance regression" box is still unchecked.

Did it address the previous concerns? Mostly no on perf.

The prior review's top actionable item (sanitizeParts fast-path) and item (c) (drop the Number() wraps) were not implemented. The depth-clamping concerns from Copilot were addressed (by routing through sanitizeDepth, which floors and clamps), and the pushParam injection gap was genuinely closed.

Findings (most severe first)

• lib/handlebars/utils.js (sanitizeParts) — Unconditional allocation on the hottest compile path; the exact fast-path the prior review recommended was not applied. Array.isArray(parts) ? parts.map(String) : [] allocates a new array and calls String() on every element of every PathExpression, including the 99.9% case of parser-produced ASTs whose parts are already strings. Before this PR, compiling from a string template paid zero
  sanitization cost; now every path expression pays it. The recommended guard (return the original array when all elements are already strings) eliminates the allocation with identical safety and was skipped.

  • PR process — "Check for performance regression" is unchecked and no benchmark accompanies the PR. The change moves validation cost from a rare path (caller passes a pre-parsed AST) to the hot path (every compile/precompile). The prior review explicitly asked for a "compile 10k templates"
    bench against the 4.x baseline; without it, the regression in the first finding is shipping unmeasured.
  • lib/handlebars/compiler/compiler.js:313 (lookupBlockParam) — Redundant coercion on trusted values, flagged in the prior review and not removed.
    [Number(blockParamId[0]), Number(blockParamId[1])] wraps values that come from blockParamIndex, i.e. a loop counter and indexOf result, never from the AST. The Number() calls are dead defense and allocate a fresh array on every block-param path expression.
  • lib/handlebars/compiler/compiler.js:299-327 (altitude) — Sanitization is applied inside the PathExpression visitor, not at the actual trust boundary, so classification still reads raw values. classifySexpr → AST.helpers.simpleId reads raw path.depth and path.parts.length, and helperSexpr/ambiguousSexpr read raw path.parts[0], all before the visitor sanitizes. A type-confused depth: '0' (string, truthy) makes simpleId treat the path as depthed while sanitizeDepth('0') yields 0 in the emitted opcodes, so classification and codegen disagree and the path can miscompile. The old validator rejected this loudly; sanitizing per-visitor instead of normalizing the node once leaves the rest of compilation reading untrusted values.

• lib/handlebars/compiler/compiler.js:425 (pushParam) — let depth = sanitizeDepth(val.depth) runs for every param unconditionally, but depth is only used in the stringParams branch. In the common non-stringParams path it is computed and discarded, then this.accept(val) re-enters PathExpression which sanitizes the same depth again. Duplicated work per parameter; the prior review noted this double-sanitization.

  • spec/security.js (NumberLiteral test) — Weak assertion that Copilot flagged and the author kept. expect(result).to.not.contain('Function') passes for any output that happens to omit the substring Function, so a regression that injects different code, or that changes the output for unrelated reasons, slips through. Asserting the concrete safe output (e.g. the rendered string equals a known value, or NaN is produced) would actually pin the behavior.
  • lib/handlebars/compiler/compiler.js:334,338 and utils.js sanitizeDepth (behavior change, undocumented) — The mitigation philosophy flipped from "reject bad AST loudly" to "coerce silently," and the PR description does not call this out. NumberLiteral type confusion now renders NaN instead of throwing, a non-boolean BooleanLiteral renders as false, and a forged negative depth is clamped to 0 and compiles. This is safe against injection but masks bugs in upstream AST-producing tooling that previously got a loud Invalid AST error. Worth documenting as an intentional behavior change.

  Net: security intent is sound and one real gap is closed, but this should not merge as a "security mitigation improvement" while the performance regression it introduces is both unmeasured and has a known, previously-recommended fix sitting unapplied. I'd push back for: the sanitizeParts fast-path, removal of the Number() wraps on blockParamId, a compile-path benchmark, and a one-line note in the description about the silent-coercion behavior change.

[c240030]

Hi @jaylinski have you fixed and merged? It seems like the RCE is still alive on 4.7.9

[jaylinski]

Hi @jaylinski have you fixed and merged? It seems like the RCE is still alive on 4.7.9

Will do soon.

[jaylinski]

Superseded by #2185.