fix(core): escape < in the compiler-emitted variables script - #3071
fix(core): escape < in the compiler-emitted variables script#3071vanceingalls wants to merge 1 commit into
< in the compiler-emitted variables script#3071Conversation
`<script>` is a raw-text element: HTML serialization does not escape its content, and the tokenizer ends it at the first `</script`. The statement `buildVariablesByCompScript` emits embeds composition variables via `JSON.stringify`, which escapes `"` and `\` but not `/` — so a variable value, key, or composition id containing `</script>` terminated the element early and the remainder was parsed as markup, corrupting the compiled document. Rewrite `<` to its JSON unicode escape. This is transparent to JSON.parse, so the table the runtime reads is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Review: <script> breakout via composition variables
The vulnerability
<script> is a raw text element — the HTML serializer does not escape its content, and the tokenizer closes it at the first </script. JSON.stringify escapes " and \ but not /, so a variable value containing </script> terminates the element. Everything after is parsed as markup, enabling script injection via stored composition data.
This is the exact pattern OWASP documents for JSON-in-HTML embedding.
The fix
const json = JSON.stringify(variablesByComp).replace(/</g, "\\u003c");\u003c is the JSON unicode escape for <. It is transparent to JSON.parse — the runtime reads the byte-identical value. Escaping ALL < (not just </script) is correct: any < inside raw text could start a closing tag or enter the legacy double-escaped comment state.
What I verified
- Completeness of the character set:
<is the only character that can terminate a raw text element or enter comment-like parsing states.>alone cannot close the element.&is not interpreted in raw text. Escaping just<is sufficient. - Transparency: the test on line 61 executes the statement via
new Functionand asserts the parsed object istoEqualthe original — byte-for-byte fidelity through the escape. - Attack surface coverage: three injection vectors tested — value, key, and composition ID. All three survive
JSON.stringifyunescaped (it doesn't escape/) and all three are covered by the single.replace(/</g, ...)on the serialized JSON. - No other
<script>embedding sites in this file:buildVariablesByCompScriptis the only function that generates a script body from user-controlled composition data. Confirmed no other sinks.
Test quality
The round-trip test pattern — serialize into a document, document.toString(), reparse, count <script> elements — is the right shape for an XSS fix. It proves the attack fails at the HTML parsing layer, not just at the string level. The new Function execution check proves no data corruption.
CI hasn't fully triggered yet (only WIP). No blocking concerns.
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟢 LGTM — clean, minimal, right escape at the right layer. HEAD 201a79f9. One 🟡 clarify on an adjacent embedding site + a small test-coverage nit.
What I traced
The fix. buildVariablesByCompScript serializes variablesByComp with JSON.stringify, and the result is placed via textContent into a <script> element that is later emitted through linkedom's document.toString(). <script> is a raw-text element per HTML spec, so the serializer will not escape any </script in textContent — a </script> byte-sequence inside a variable value / key / outer composition id would therefore close the element early and hand the rest of the payload to the HTML tokenizer as markup. JSON.stringify escapes " and \ but neither < nor /, so the raw bytes pass through unchanged. Replacing < with < on the serialized JSON is the standard OWASP JSON-in-HTML remedy and is transparent to JSON.parse, so the runtime table is byte-identical (compositionScoping.ts:617).
Completeness at the "render surface" abstraction. The two callers of buildVariablesByCompScript are the preview/snapshot compiler (packages/core/src/compiler/htmlBundler.ts:1065) and the render worker's compiler (packages/producer/src/services/htmlCompiler.ts:1039), both of which drop the returned statement into a single body-level <script> via textContent — so this one-line change closes the sink on both surfaces (the "app origin" and the "render worker" halves of the vuln). The CSS-context sibling — variable values emitted into <style> — is closed by the paired PR #3072. Nothing else in packages/core/src/compiler/ emits variablesByComp values into a raw-text element that I could find.
No backfill needed. Escape is at emit time on the compile path, so any pre-existing malicious value stored on a composition becomes non-exploitable on the next compile. There is no store-time reversal risk.
Regression risk on legitimate values. < in a variable value becomes < in the serialized JSON; JSON.parse restores it to <. The test at compositionScoping.test.ts:936-940 executes the emitted statement via new Function and asserts strict equality of the resulting window.__hfVariablesByComp to the input — a clean fidelity check.
Escape breadth is correct. < is the only character that can terminate a raw-text element or transition it into the legacy double-escape comment state; > alone cannot close, and & is not interpreted in raw text. Escaping every < (not just </) is the right call. Nothing to add.
Coordination with the pacific PRs. BE escape here does not substitute for a context-appropriate escape on any FE surface that consumes these values (attribute vs tag vs JS context are all different), but that is the pacific pair's job — this PR's contract is only that the compiled document is safe to serve.
🟡 Clarify — same class of bug in adjacent JSON.stringify embeddings
wrapScopedCompositionScript (same file, compositionScoping.ts:253) embeds several JSON-stringified values directly into its returned script body via template interpolation:
const compositionIdLiteral = JSON.stringify(compositionId);
const timelineCompositionIdLiteral = JSON.stringify(timelineCompositionId);
const authoredRootIdLiteral = JSON.stringify(authoredRootId?.trim() || null);
// ...
return `(function(){
var __hfCompId = ${compositionIdLiteral};
var __hfTimelineCompId = ${timelineCompositionIdLiteral};
var __hfAuthoredRootId = ${authoredRootIdLiteral};The returned wrapper is then concatenated with other chunks and placed into a <script> element via textContent at htmlBundler.ts:1071 (and the parallel path in the producer's htmlCompiler.ts). compositionId / timelineCompositionId / authoredRootId flow in from HTML attributes on host elements (data-composition-id, data-authored-root-id) with no visible slugification at the emit path. If any of these string carriers can contain </script> in the composition-authoring threat model, the exact bug this PR fixes still exists on those literals — JSON.stringify again does not escape < or /.
I recognize the pre-existing narrower defense at line 576 — source.replace(/<\/(script)/gi, "<\\/$1") — which suggests the raw-text-element hazard has been thought about before, but only for the composition-author's own inline JS body, and only via </script-narrow (per Miga's review, < is the correct broader target). The other embeddings on lines 261-273 have no equivalent.
Two possible resolutions, either is fine — I just want the intent recorded:
- In scope, follow-up: the same
.replace(/</g, "\\u003c")(or a shared helper) applied to each of those JSON.stringify literals — a natural sibling PR to this one and #3072. - Out of scope, threat model: composition authors already have direct
<script>access viasource, so a<script>breakout oncompositionIdgrants them no new capability. If that's the reasoning, calling it out in a docstring or PR comment would future-proof the assumption.
The pattern-completeness question is enough to raise, not enough to block — the specific sink this PR names (buildVariablesByCompScript) is correctly closed.
Nit — test-coverage: full HTML round-trip → parse → execute
The four tests split the fidelity check across two axes:
does not let a variable VALUE/KEY/COMP-ID close the script element— HTML round-trip, asserts one<script>element remains and the emitted string contains no</script.keeps the value byte-identical once executed— bypasses HTML round-trip, executes the raw emitted statement vianew Functionand asserts value equality.
A stronger single test would be to combine them: emit → append into a document → document.toString() → re-parse → extract textContent of the sole <script> → new Function(scriptText).call(fakeWindow) → assert fakeWindow.__hfVariablesByComp equals the input including the </script> payload. That's the shape of the attack-plus-fidelity check end-to-end, and it forecloses a class of subtle serializer/re-tokenizer regressions that neither existing test would catch alone. Low priority — the two existing tests together already establish the guarantee inductively.
What I didn't verify
- Whether
compositionId/authoredRootIdare ever slugified upstream at the point where compositions are ingested (e.g., server-side normalization on write). If they are, the 🟡 concern collapses. I only checked emission sites, not ingestion. - Behavior of the linkedom
document.toString()serializer against an HTML5-conformant reference — I trusted the round-trip test's guarantee that today's linkedom emits raw-text<script>content unescaped, which matches spec. - CSP / other defense-in-depth on the surfaces that serve the compiled document — expected to live on the app-origin side (pacific), not on this compiler.
- I did not run the tests locally against this HEAD; the test bodies read correctly against the diff.
Peer scan (fresh at HEAD 201a79f9, latest per user, sorted submitted_at desc)
- miga-heygen —
COMMENTED— 2026-08-06T21:11:22Z. Concurs on completeness of<as the sole raw-text escape target, verifies transparency and attack-surface coverage inbuildVariablesByCompScript; explicitly qualifies scan as "in this file" and "from user-controlled composition data" — consistent with the 🟡 above about adjacent JSON.stringify sites.
No APPROVED reviews at HEAD. reviewDecision = REVIEW_REQUIRED. mergeable=MERGEABLE, mergeStateStatus=BLOCKED.
— Review by Rames D Jusso
<script>is a raw-text element: HTML serialization does not escape its content, and the tokenizer ends it at the first</script.buildVariablesByCompScriptembeds composition variables withJSON.stringify, which escapes"and\but not/. A variable value, key, or composition id containing</script>therefore terminated the element early and everything after it was parsed as markup, corrupting the compiled document.Fix
Rewrite
<to its JSON unicode escape before embedding. Transparent toJSON.parse, so the table the runtime reads is byte-identical.Tests
Five cases in
compositionScoping.test.tscovering a breakout via value, key, and composition id; a round-trip throughdocument.toString()and back to confirm no script element is grown; and an execution check that the parsed value is unchanged. Verified to fail without the fix.🤖 Generated with Claude Code