Repository navigation
Security improvements - #2185
Merged
Merged
Conversation
Update the minimist dependency to the latest 1.2.x release. Also sync the version in package-lock.json with package.json, which the v4.7.9 release commit did not update.
The `--root` option only strips a prefix from template names, but its description could be read as limiting which files the CLI reads. State explicitly that it is not a filesystem boundary.
Instead of validating the AST in the parser, the compiler now checks every AST value that it writes into generated code: path depths and parts, literal values, content strings, hash pair keys and block params, including in stringParams and trackIds mode. The checks now sit where the values are used, so ASTs changed after parsing are covered too. A missing `depth` is treated as 0 and a missing or partial `loc` no longer breaks compilation or error reporting, because hand-built ASTs often omit both. The docs and type definitions now state that `compile` and `precompile` turn an AST into code, so the AST must come from the application itself and never from untrusted input. Fixes GHSA-8r5x-fm3f-whwj.
`Compiler#accept` and `Visitor#accept` call the method named by `node.type`. A crafted type such as `accept`, `constructor` or `pushParam` could therefore call an unrelated method with the node as its argument. The Compiler now accepts only node types the parser produces. A Visitor also accepts types for which a subclass or instance defines its own handler, but never methods of `Visitor` or `Object.prototype`.
`resolvePartial` treated any value with a truthy `call` property as a compiled partial, so an object from context data could reach `env.compile()` and be compiled as a template. Only functions are now treated as compiled partials, and only strings are compiled at render time. Anything else is rejected with an error. A function partial that returns nothing now throws a clear error instead of being passed on to the compiler. Partials registered as an AST are no longer compiled; pass them through `Handlebars.compile()` first.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Generated JavaScript remains invalid for some ES5 inputs, and runtime edge cases can incorrectly mask errors or reject safe values.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Hardens Handlebars compilation and runtime behavior against code injection, prototype access, unsafe partials, and denial-of-service cases.
Changes:
- Validates AST values and restricts visitor/compiler dispatch.
- Safely escapes generated JavaScript and improves runtime protections.
- Adds security, performance, and iterator regression tests.
| File | Description |
|---|---|
types/index.d.ts |
Documents untrusted AST risks. |
spec/whitespace-control.js |
Tests linear whitespace processing. |
spec/visitor.js |
Tests safe custom-node dispatch. |
spec/utils.js |
Tests non-function toHTML handling. |
spec/security.js |
Adds security regression coverage. |
spec/precompiler.js |
Tests generated-output sanitization. |
spec/partials.js |
Tests partial validation and recursion. |
spec/expected/help.menu.txt |
Updates CLI help fixture. |
spec/compiler.js |
Expands AST validation tests. |
spec/builtins.js |
Tests lazy iterator behavior and cleanup. |
package.json |
Updates minimist. |
package-lock.json |
Synchronizes dependency metadata. |
lib/precompiler.js |
Sanitizes names and source-map comments. |
lib/handlebars/utils.js |
Tightens SafeString detection. |
lib/handlebars/runtime.js |
Hardens partial and prototype handling. |
lib/handlebars/internal/proto-access.js |
Detects prototype constructors. |
lib/handlebars/helpers/each.js |
Streams iterables and closes them on failure. |
lib/handlebars/exception.js |
Supports partial AST locations. |
lib/handlebars/compiler/whitespace-control.js |
Removes quadratic regex behavior. |
lib/handlebars/compiler/visitor.js |
Restricts node dispatch. |
lib/handlebars/compiler/node-types.js |
Defines allowed AST node types. |
lib/handlebars/compiler/javascript-compiler.js |
Escapes generated literals and locations. |
lib/handlebars/compiler/compiler.js |
Validates AST fields before code generation. |
lib/handlebars/compiler/code-gen.js |
Escapes script delimiters and strings. |
lib/handlebars/compiler/base.js |
Moves validation to the compiler boundary. |
docs/compiler-api.md |
Documents AST and output-safety requirements. |
bin/handlebars |
Clarifies --root behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A prototype's own `constructor` (e.g. `Function.prototype.constructor`) passed the own-property check in `lookupProperty`. With prototype method access enabled (e.g. `allowProtoMethodsByDefault`), templates could reach the `Function` constructor through `__proto__`. Such lookups now go through the prototype-access checks, which deny `constructor` unless explicitly allowed. `escapeExpression` called `toHTML` on any value with such a property, so context data with a non-function `toHTML` key, e.g. from parsed JSON, made rendering throw. Only values with a callable `toHTML` are now treated as SafeStrings. Fixes GHSA-p8wg-vrv2-v86f.
jaylinski
force-pushed
the
security-improvements
branch
from
October 5, 2026 21:49
60113c1 to
0115c57
Compare
Precompiled templates are often inlined in `<script>` tags. Strings in the generated code containing `</script` could end the script element early, and `<!--` followed by `<script` could keep it from ending where intended. `quotedString` now escapes the `<` of these sequences as `\u003C`, and the JavaScript compiler uses it instead of `JSON.stringify` for lookup names, partial indents and tracked ids. Source locations, which include the `srcName` option, and the template names written by the CLI are escaped the same way. `quotedString` now builds on `JSON.stringify`, so it also escapes control characters and surrogates, and it skips escaping entirely when a string has nothing to escape, which is true of almost every lookup name. Compilation doesn't slow down. The `nameLookup` example in the compiler API docs now quotes with `quotedString` as well, and the docs say that overrides must do the same. Fixes GHSA-xw65-4hp5-5hc7.
`#each` collected every value of an iterable before rendering the block. It now renders each value as it goes, like `for...of`, and closes the iterator if the block throws, so generators can run their cleanup code. Values added to the iterable while the block renders, e.g. to a `Set`, are now visited too, as with `for...of`. The regexes that find trailing whitespace in whitespace control were retried at every position of a whitespace run, which takes quadratic time on long runs. They are now anchored so that matching takes linear time.
The CLI strips line terminators from the `--map` value before writing it into the sourceMappingURL comment. With `--min`, uglify drops that comment and writes its own from the raw value, so a newline in it ended the comment and the rest of the value ran as JavaScript. The sanitized value is now passed to uglify as well. The comment also kept a raw "<", so a value containing `</script` or `<!--` could end or change the parsing of an enclosing <script> element. "<" is now percent-encoded, which keeps the URL pointing to the same file.
jaylinski
force-pushed
the
security-improvements
branch
from
October 5, 2026 22:02
0115c57 to
c3cb63c
Compare
Contributor
|
@jaylinski did you assess perf impact of these changes? was it reasonable? |
Member
Author
@kibertoad Yes, it was reasonable. But I fully relied on Opus 5.5 to assess it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🆕