fix(core): revert Function constructor regression in sub-composition scripts

[miguel-heygen]

Summary

Reverts the Function constructor change from commit 3bb0d1ef that broke all sub-composition scripts using document.getElementById(). The Function constructor creates functions with global scope ([[Scope]] = global), losing access to the composition-scoped DOM proxy — native method calls on proxy-returned elements throw "Illegal invocation".

Closes #1074

Root cause

3bb0d1ef changed wrapScopedCompositionScript from an inline IIFE to a Function-constructor invocation:

-  (function(document, gsap, window, __hyperframes) {
-    ${source}
-  }).call(window, __hfScopedDocument, ...);
+  var __hfScript = Function("document", "gsap", "window", "__hyperframes", sourceLiteral);
+  __hfScript.call(window, __hfScopedDocument, ...);

The Function constructor's scope chain is always global — the composition-scoped __hf* helpers are only available as named parameters, not via closure. This breaks the DOM proxy's value.bind(target) chain for native methods like canvas.getContext("2d").

Affected surface

All registry shader-transition blocks (swirl-vortex, sdf-iris, glitch, etc.) and any user composition where a sub-comp script calls document.getElementById() followed by a native DOM method.

Verification

Reproduced with the reporter's minimal repro (sub-comp with <canvas id="gl-canvas"> + document.getElementById("gl-canvas").getContext("2d")):

• Before fix: TypeError: Illegal invocation / null from getElementById
• After fix: Render completes successfully (3.4KB, 1.5s)

Test plan

• Render a composition with sub-comp scripts calling document.getElementById — no errors
• Render a shader-transition composition (WebGL sub-comp) — canvas initializes correctly
• Render a GSAP-only sub-comp — unchanged behavior (was already working)
• bun run --cwd packages/core test passes

[jrusso1020]

Verdict: COMMENT (would-be APPROVE pending one clarification)

Clean 3+/3- revert. Root cause analysis is sound — Function constructor's [[Scope]] = global semantic kills the composition-scoped DOM proxy. Holding for James's stamp greenlight.

One concern — original commit's motivation isn't addressed

3bb0d1ef was titled "wrap bundled composition scripts as string literals". That title implies the motivation was bundle-time safety — avoiding scenarios where sub-comp script content interferes with the outer bundle parse (e.g., a literal </script> token, regex-looking content, etc.). The original commit also added 26 lines of tests in compositionScoping.test.ts — those likely pin specific source-content safety cases.

The revert restores the inline IIFE pattern, which embeds the source verbatim in the bundle. If the source contains something that breaks bundle parsing (the case 3bb0d1ef was guarding against), the revert reintroduces that issue.

Two questions for the author:

1. Were the compositionScoping.test.ts tests added in 3bb0d1ef updated or removed in this revert? If they were testing the source-content safety pattern, they'd fail on this revert — but the diff shows only 3 lines changed in one file, no test changes. Either those tests don't exist anymore (got removed in a prior change), or they pass through both shapes.

2. Is there a known source pattern that broke pre-3bb0d1ef and motivated the change? If yes, the revert needs to ALSO handle that case (e.g., escape </script> in the inlined source, or use a different scope-preserving mechanism like eval). If no, the original commit may have been preemptive and the revert is safe.

The production regression is urgent, so I lean toward "ship if Magi confirms the original safety case wasn't load-bearing OR re-verifies through testing." But the next person walking this code will hit the same trap if the motivation isn't documented.

Test coverage gap

The reporter's repro (<canvas> + document.getElementById().getContext("2d") in a sub-comp script) isn't pinned as a regression test. Adding it to compositionScoping.test.ts would prevent the next attempt at Function-constructor-style wrapping from re-introducing the same crash. Worth one fixture before merge.

Sanity checks

• CI: 9 success / 20 in_progress / 0 failures.
• sourceLiteral removal is consistent: JSON.stringify(source) was only used by the Function(...) call, now unused. Correctly removed. ✓
• The IIFE pattern matches what was there pre-3bb0d1ef — no novel pattern introduced.

Summary

Ship-safe revert if Magi confirms the source-content safety case from 3bb0d1ef isn't load-bearing. Otherwise, the fix needs to address both the scope issue AND the source-content issue.

— Rames Jusso

[vanceingalls]

Review

Verdict: REQUEST CHANGES

The IIFE revert correctly restores closure scope and fixes the Illegal invocation regression. But it reintroduces the </script> injection vector that 3bb0d1ef was deliberately hardening against — and it leaves two tests that 3bb0d1ef added in a broken state. Both are blockers.

---

[blocker] IIFE revert reintroduces the </script> injection vector

3bb0d1ef's commit message is "fix(core): wrap bundled composition scripts as string literals" and its test fixture is explicit:

window.payload = "</script><script>window.pwned = true;</script>";

The fix used Function(...) so that user-authored source is passed as a string literal, not interpolated raw into the wrapping script. With the IIFE revert, ${source} is spliced directly into the script body again. When the bundler serializes the DOM to HTML, a <script> element whose .textContent contains the literal substring </script> will terminate the script block early — the HTML parser treats <script> as a raw-text element with no escape mechanism. < doesn't work there. 3bb0d1ef was a real fix; reverting it re-opens the vector.

The right fix preserves closure scope and neutralizes the injection. One clean path:

const safeSource = source.replace(/<\/(script)/gi, "<\\/$1");
// then use safeSource in the IIFE:
(function(document, gsap, window, __hyperframes) {
${safeSource}
}).call(window, __hfScopedDocument, ...);

This is what browsers expect (<\/script> is valid JS and safe for raw-text element parsing). Alternatively, the replacement can target only </script (no closing >), which is sufficient since that's what the parser triggers on.

---

[blocker] Two tests from 3bb0d1ef will fail under the reverted IIFE

The PR reverts compositionScoping.ts but does not update the tests added in the same commit:

• compositionScoping.test.ts — "wraps scoped composition script source as a string literal" asserts expect(wrapped).toContain('Function("document", "gsap", "window", "__hyperframes", '). The IIFE form doesn't emit that.
• htmlBundler.test.ts line 761 — same assertion: expect(bundled).toContain('Function("document", "gsap", "window", "__hyperframes",').

These tests are pinning the security hardening intent. They need to be updated to reflect the new approach (e.g., assert the IIFE is present, assert <\/script> in output for the injection test case rather than the string-literal form).

CI is still pending so this hasn't surfaced yet, but it will.

---

[important] wrapInlineScriptWithErrorBoundary still uses Function(...) — no DOM proxy involved, no scope regression there

wrapInlineScriptWithErrorBoundary (introduced by 3bb0d1ef for unscoped scripts) uses Function(source).call(window) with no scoped proxies passed. The regression (proxy value.bind(target) chain breaking) doesn't apply here, so that function can stay as-is. Just calling this out explicitly so the fix doesn't over-correct.

---

Timeline note

Both 3bb0d1ef (string-literal hardening) and the sub-comp scoping regression landed in v0.6.43 — same release. The hardening commit came in at 08:41, the v0.6.43 tag was cut at 13:35. If there's a hotpatch window before v0.6.43 widens, the cleanest path may be to fix both issues in a single commit rather than chaining a revert + follow-up hardening re-add.

---

Suggested minimal fix diff shape:

// In wrapScopedCompositionScript, replace the sourceLiteral removal with a safe interpolation:
const safeSource = source.replace(/<\/(script)/gi, "<\\/$1");
// ... keep sourceLiteral removed, keep IIFE form, use safeSource:
      (function(document, gsap, window, __hyperframes) {
${safeSource}
      }).call(window, __hfScopedDocument, __hfScopedGsap, __hfScopedWindow, __hfScopedHyperframes);

Then update the two security tests to assert the IIFE form and <\/script> escape pattern instead of Function(...).

— Vai

[jrusso1020]

Aligning with Vai's REQUEST_CHANGES. My prior review flagged the 3bb0d1ef motivation as "pending clarification" — Vai dug into the test fixture (window.payload = "</script><script>window.pwned = true;</script>") and confirmed it was deliberate </script>-injection hardening, not preemptive. Should have graded as a blocker, not a "pending clarification."

The right fix is IIFE + escape (preserves both the scope semantics AND the XSS hardening), as Vai outlined:

const escapedSource = source.replace(/<\/(script)/gi, "<\\/$1");
// then use escapedSource in the IIFE template

Plus the two existing tests from 3bb0d1ef (compositionScoping.test.ts + htmlBundler.test.ts asserting the Function(...) form) need updating to assert the new IIFE-with-escape form. The pure revert in this PR drops the security test coverage entirely.

Deferring to Vai's CHANGES_REQUESTED.

— Rames Jusso

[jrusso1020]

Verified at 6d97c3d8:

1. IIFE restored ✓ — closure scope preserved for the DOM proxy
2. </script> escape added ✓ — source.replace(/<\/(script)/gi, "<\\/$1") preserves 3bb0d1ef's security hardening
3. Two existing tests updated ✓ — both now assert the IIFE-with-escape shape; compositionScoping.test.ts test renamed to "escapes </script> in scoped composition script source to prevent injection" with explicit expect(wrapped).not.toContain("</script><script>") + expect(wrapped).toContain("<\\/script>")

Clean fix that addresses both the runtime regression AND preserves the security invariant the original commit was guarding. Re-approved.

— Rames Jusso

[vanceingalls]

Re-reviewing at commit 6d97c3d. All three blockers from the prior round are resolved.

IIFE restored — wrapScopedCompositionScript now emits a proper inline IIFE:

(function(document, gsap, window, __hyperframes) {
  ${source…}
}).call(window, __hfScopedDocument, …);

Function constructor is gone, sourceLiteral / JSON.stringify removed. Closure scope is correctly re-established — document, gsap, window, and __hyperframes shadow the globals before the authored code runs, same contract as before.

</script> escaping — the escape is applied inline at interpolation time:

${source.replace(/<\/(script)/gi, "<\\/$1")}

Regex is correct: case-insensitive, captures the tag name so the replacement is <\/script> (forward-slash escape that browsers accept inside a script block). Applied to source before it lands in the template literal, not after. No bypass via </SCRIPT>, </Script>, etc.

Tests updated — both test files updated to match:

• compositionScoping.test.ts: injection test now asserts IIFE signature, .not.toContain("</script><script>"), and .toContain("<\\/script>"). Correct.
• htmlBundler.test.ts: updated to IIFE signature. The tl.to(".title" change (single → double quotes) is a correct consequence of the source now being interpolated as-is rather than JSON-stringified — the authored source had single quotes, which are preserved verbatim in the IIFE output.

wrapInlineScriptWithErrorBoundary still uses Function(JSON.stringify(source)) — that's fine, it's a different call path for inline (non-scoped) scripts and not changed here.

CI: still running at review time, but the fast required checks (Lint, Format, Fallow audit, Studio load smoke, Test: runtime contract) are all green. No concerns.

LGTM. Ship it.

— Vai