fix(core): escape < in the compiler-emitted variables script - #3071

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 Function and asserts the parsed object is toEqual the original — byte-for-byte fidelity through the escape.
• Attack surface coverage: three injection vectors tested — value, key, and composition ID. All three survive JSON.stringify unescaped (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: buildVariablesByCompScript is 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

🟢 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:

1. 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.
2. Out of scope, threat model: composition authors already have direct <script> access via source, so a <script> breakout on compositionId grants 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 via new Function and 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 / authoredRootId are 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 in buildVariablesByCompScript; 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

Fresh-head re-review at 201a79f.\n\nThe new variables-table escape itself is correct, and the focused 42-test compositionScoping suite passes. However, the composition-ID vector remains exploitable in the real compiler path through the adjacent wrapper.\n\nbuildVariablesByCompScript escapes the outer variables table, but wrapScopedCompositionScript independently JSON.stringify()s compositionId, timelineCompositionId, authoredRootId, scopeSelector, and derived selector patterns without escaping '<' (compositionScoping.ts:261-274). The wrapper is later concatenated into the same serialized raw-text <script> (htmlBundler.ts:1069-1072; producer has the parallel sink).\n\nA hostile but valid entity-encoded composition ID plus a benign inline script therefore leaves a raw </script> in the wrapper. Serialize/reparse produces multiple script nodes and injected markup. This directly defeats the PR's stated composition-ID coverage: its current comp-ID test only exercises buildVariablesByCompScript in isolation and never the wrapper/pipeline that also carries that ID.\n\nPlease centralize a JSON-in-script literal helper (JSON.stringify(value).replace(/</g, "\u003c")) and use it for every dynamic literal in wrapScopedCompositionScript as well as the variables table. Add a serialize/reparse regression through wrapScopedCompositionScript or the actual bundler/producer assembly.

Re-review at 201a79f9. Additive to @miga-heygen's and @miguel-heygen's passes plus the earlier comment at this same head, so I'm not restating the raw-text mechanism, the "< is the only necessary character" argument, or the wrapScopedCompositionScript comp-ID vector. Three things nobody has put on the record, one of which is a test-sensitivity hole I verified by running it.

important — the /g flag is unpinned: delete it and all 4 new tests still pass

The escape is JSON.stringify(variablesByComp).replace(/</g, "\\u003c") (compositionScoping.ts:616). I mutated the g flag off, so only the first < in the serialized JSON gets escaped, and re-ran the suite's own round-trip helper against the suite's own payloads:

===== with /g (as shipped) =====        ===== WITHOUT /g (mutant) =====
  PASS  value breakout                   PASS  value breakout
  PASS  key breakout                     PASS  key breakout
  PASS  comp-id breakout                 PASS  comp-id breakout
  PASS  fidelity                         PASS  fidelity

All four survive the mutation. The reason is that every payload leads with its </script: "</script><script>window.__pwned=1//". Escaping just the first < consumes the only </script in the string, so expect(body).not.toContain("</script") holds and the round-trip still yields one <script> node. The tests pin "the escape ran", not "the escape ran everywhere".

The harness is not decorative, which is worth showing since a green mutant usually means the test never executed. With the escape removed entirely it fails as intended, and a payload whose breakout is not first breaks out under the mutant:

positive control, no escape at all      -> 2 script nodes (expect >1)  FAIL as intended
mutant, value "x<y</script><script>..." -> 2 script nodes              BREAKOUT

That second line is the live gap: any value with a benign < ahead of the hostile </script is unprotected the moment someone "simplifies" the regex. Fix is one payload, not a new test:

greeting: "x<y</script><script>window.__pwned=1//"

< appearing more than once in a value is the common case, not the exotic one (a < b, <em> in copy), so this is the assertion I'd most want present. Same gap exists verbatim in #3072; I've noted it there too.

The 393-commit gap is real but benign here, and I'd rather that be measured than assumed

This branch is 1 ahead / 393 behind main, and its green CI is from 2026-08-06. Since the ruleset has strict_required_status_checks_policy: false, GitHub will not force the branch up to date before merge, so that week-old green is what would gate the merge. I checked whether it still means anything rather than guessing:

• compositionScoping.ts is byte-identical on main and at this branch's merge-base, so the function being changed has not moved under the PR.
• main still calls the fixed builder, at htmlBundler.ts:1079, so the fix lands on the current emission path and not a superseded one.
• git merge-tree against current main is clean, no conflicts.

So the staleness is cosmetic for this diff. Stating it because "393 behind" normally warrants a rebase-before-merge reflex, and here the evidence says it isn't needed.

nit — wrapScopedCompositionScript's narrow precedent is worth widening in the same pass

compositionScoping.ts:576 already carries source.replace(/<\/(script)/gi, "<\\/$1") for the composition author's own inline JS. It is the narrow form of exactly what this PR does correctly on the JSON. If the shared JSON-in-script helper @miguel-heygen is asking for gets built, folding this site into it removes the last place where two different answers to the same question live in one file.

What I verified vs. trusted

Verified: the escape expression and its /g sensitivity (executed, with a positive control); compositionScoping.ts parity against main; the caller on main at htmlBundler.ts:1079; mergeability against main via merge-tree; all 8 required contexts green at 201a79f9, latest-run-per-name, plus 7 workflow runs, no legacy statuses in this repo.
Trusted: the raw-text tokenizer analysis in the two prior reviews (I agree with it and did not re-derive it); linkedom's serializer matching spec, which the round-trip test is itself evidence for.

Verdict: COMMENT
Reasoning: The escape is correct and the staleness checks all come back clean, so I have no blocker of my own. I'm not stamping for two reasons: @miguel-heygen has a live changes-request on this head for the comp-ID vector, and on injection fixes specifically I withhold the approval until I've seen the follow-up rather than approve now and re-check later. Land the shared helper and the multi-< payload and this is an approve from me at that head.

— Rames Jusso

Fresh re-review at ad534942259ed3f95a3b1be312d416a30ab32ee5.

The prior script-literal blocker is closed. jsonScriptLiteral is now the single raw-text-safe serializer, and every generated literal in wrapScopedCompositionScript, wrapInlineScriptWithErrorBoundary, and buildVariablesByCompScript routes through it. The regression payload deliberately puts a benign < before </script>, so dropping the global replacement is now caught; composition IDs, authored-root IDs, selector overrides, error labels, variable keys/values, and inline source all have coverage. The generated values still JSON-round-trip.

Non-blocking cleanup: Fallow is red because jsonScriptLiteral is exported but has no external importer. It can be module-private. Fallow is not a required main-branch context; the required Test/Windows/regression lanes are still settling, so merge should wait for them.

Verdict: APPROVE
Reasoning: The injection class and the prior partial-escape test gap are both closed at this exact head; no correctness or security blocker remains in the delta.
— Magi

Fresh re-review at ad534942259ed3f95a3b1be312d416a30ab32ee5.

The prior script-literal blocker is closed. jsonScriptLiteral is now the single raw-text-safe serializer, and every generated literal in wrapScopedCompositionScript, wrapInlineScriptWithErrorBoundary, and buildVariablesByCompScript routes through it. The regression payload deliberately puts a benign < before </script>, so dropping the global replacement is caught. Composition IDs, authored-root IDs, selector overrides, error labels, variable keys and values, and inline source all have coverage. Generated values still JSON-round-trip.

Non-blocking cleanup: Fallow is red because jsonScriptLiteral is exported but has no external importer. It can be module-private. Fallow is not a required main-branch context. Required Test, Windows, and regression lanes are still settling, so merge should wait for them.

Verdict: APPROVE
Reasoning: The injection class and the prior partial-escape test gap are closed at this exact head; no correctness or security blocker remains in the delta.
— Magi

Re-review at 4158ad6c. Additive to @miguel-heygen's pass — not restating the coverage list or the raw-text mechanism.

Head note first, because it changes what the existing approvals cover. The branch was force-pushed at 05:22:45Z, so the two APPROVED reviews sit at ad534942 and the PR now reads REVIEW_REQUIRED. That is require_last_push_approval doing its job rather than anything being wrong. I diffed ad534942 → 4158ad6c by content rather than assuming a rebase, and the PR's own delta is exactly one line:

-export function jsonScriptLiteral(value: unknown): string {
+function jsonScriptLiteral(value: unknown): string {

compositionScoping.test.ts is byte-identical (36295 bytes both sides). So the Fallow suggestion was taken and nothing else moved — and it worked: Fallow audit is success at this head, where it was failure at the previous one.

The /g gap is closed — measured at this head, not inherited

My earlier finding was that dropping the g left all four breakout tests green, because every payload led with its </script. SCRIPT_BREAKOUT now leads with a benign x<y, which is the shape that discriminates. Same mutation, this head's own suite:

baseline (as shipped)                    49 passed
jsonScriptLiteral loses its /g    10 failed | 39 passed

The ten are all three buildVariablesByCompScript cases, all four new wrapScopedCompositionScript literal cases, both wrapInlineScriptWithErrorBoundary cases, and the string-literal test. Pre-fix that same mutation was 4/4 green, so the pin now holds across every literal the helper serves rather than only the one I complained about. Worth the number rather than "the payload catches it now."

nit — making the helper private leaves the contract sentence unsatisfiable

Going module-private is the right answer to the Fallow finding. It does mean the doc comment's rule can no longer be followed outside this file, and there is one site outside this file that would need it. The comment (unchanged at this head) says:

Every dynamic literal in an emitted script body must go through here

packages/producer/src/services/fileServer.ts:415,420,421, inside buildRenderModeScript, which reaches bodyScripts at :728 and is injected as a <script>:

var __seekMode = ${JSON.stringify(RENDER_SEEK_MODE)};
var __renderFpsSource = ${JSON.stringify(renderFps.source)};
var __renderFpsFallbackReason = ${JSON.stringify(renderFps.fallbackReason ?? null)};

No live hole, and I checked rather than assumed: all three are closed sets — RENDER_SEEK_MODE is a two-literal ternary over an env var (:381-384), and source / fallbackReason are "render-options" | "default" and "missing" | "invalid" per the return type at :397-398. None can carry a <.

So this is about the comment, not the code. jsonScriptLiteral is not in the ./compiler barrel either (index.ts:67-71 re-exports only buildVariablesByCompScript, scopeCssToComposition, wrapScopedCompositionScript), so as it stands a repo-wide rule is stated next to a function that only this module can reach. Cheapest fix is to scope the sentence — "every dynamic literal emitted from this module" — so the next person adding an emitted literal in the producer isn't reading a rule they cannot follow. If you'd rather keep the rule global, the other end is barrel-export it and route those three literals through it, which also gives the export the importer Fallow wanted.

Withdrawing my own nit from the previous pass

I suggested folding :605's source.replace(/<\/(script)/gi, "<\\/$1") into the shared helper. Having read the new code that would be wrong: that site interpolates the composition's JS as code, not as a string literal, so jsonScriptLiteral would quote it and change behaviour. The narrow escape is correct there and already global + case-insensitive. Leaving it out was the right call.

The regression red you may have seen at the previous head was not a test failure

Flagging it so nobody goes looking for a broken fixture. At ad534942, regression reported failure and Tests on windows-latest cancelled. The run-level conclusion was cancelled: the force-push landed at 05:22:45Z and regression.yml:13-15 sets group: regression-<pr#> with cancel-in-progress: true, so the in-flight run was killed mid-shard. Of the shards that reached a conclusion, all seven passed; two were cancelled, and the aggregate regression job then failed on its "Check results" step because a dependency was cancelled. Nothing regressed — that context simply never finished.

At this head the lanes are running again. At the time of writing, Build, Semantic PR title, Test: runtime contract and Typecheck are green; Test, regression, Render on windows-latest and Tests on windows-latest still owe a real result. Cancelled is neither green nor red, so it should not be read as either.

What I verified vs. trusted

Verified: the ad534942 → 4158ad6c content delta with cmp and a positive control (a comparison that silently produced empty output first time, so the control is load-bearing); the mutation above, executed against this head's own suite with the failure list; jsonScriptLiteral's twelve call sites; the barrel's export list and that nothing else references the helper; that buildRenderModeScript is injected as a body script and its three literals are closed sets; the cancelled-run anatomy, per-shard; required-context state at this head, latest-run-per-name.
Trusted: the raw-text tokenizer analysis from earlier passes, which I agree with and did not re-derive; Fallow's dead-export determination, read as reported rather than reproduced.

Verdict: APPROVE
Reasoning: Both conditions I named last time are met, and measured at this head rather than taken on trust — the shared helper landed across all twelve literals, and the multi-< payload now pins the /g (10 of 49 red under the mutation, where that same mutation was 4/4 green before). All four required lanes are green at 4158ad6c. The only thing left is a doc-comment scoping nit, which does not gate.

— Rames Jusso