fix: keep caption transcript data inside generated scripts - #3847
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed head 21677a84fc69bbc1767ab82ca2b9fd1f4aa44114 in an isolated worktree (merge base cd1a996b). Diff vs base: 7 files, +84/-12 — the three captions.mjs copies are byte-identical (same sha256) and skills-manifest.json hashes match the regenerated copies.
Strengths
packages/studio/src/captions/generator.ts:280,306,309— the escape is applied afterJSON.stringify, so it only rewrites string contents; structural JSON never contains<, and<decodes identically under bothJSON.parseand the JS parser. Word text andidvalues survive unchanged (verified below).skills/pr-to-video/scripts/captions.test.mjs:221-259— regressions drive the real CLI (captions.mjs build) across all three workflows × default/skin emitters, and assert on the parsedGROUPSpayload rather than on the escaped source. Good shape.packages/studio/src/captions/generator.test.ts:35-62— parsing the output through a real HTML parser (happy-dom) and assertingquerySelector("img, svg")is null is the right oracle for a script-boundary fix.
Verified locally
- Skills suite at head: 39/39 (6 new). Reverting the four
.replace(/</g, "\\u003c")sites: the same 6 go red (33/39). Studio vitest would not start in my environment (@hyperframes/studio-serverentry unresolved — environmental), so I replicated the Studio regression as a standalone probe using happy-dom +extractTranscript: at head 6/6 boundary/decoding cases pass (</ScRiPt>…<img onerror>,</script\t>,</script/>,<!--<script>-->,> & &&, quotes/backslashes, U+2028/U+2029/emoji); with the escape reverted all cases fail and the<img>becomes a live element. - Round-trip:
generateCaptionHtml→extractTranscriptreturns identicaltext/idfor every case above. The one case that fails round-trip (x ]; y) fails identically at base and head — that is the parser's pre-existing];anchor, out of scope here and unchanged by this PR. - Consumers: Studio's
generateCaptionHtmlis only exported fromcaptions/index.ts(no in-repo caller); all 13hyperframes-creative/frame-presets/*/caption-skin.htmlskins consumeGROUPSvia.textContent = String(w.text)and the default emitter vias.textContent = w.text + " ". NoinnerHTML/insertAdjacentHTML/document.writesink anywhere in the skins or emitters. Other interpolations ingenerateJs(// Group: ${groupId},className) are derived fromgroup-N/sanitized ids, not transcript text. Every otherJSON.stringifyin the three skill script dirs writes to a file, not into markup. - Escape sufficiency: inside a
<script>the HTML tokenizer only reacts to</script(any case, followed by whitespace///>) and<!--(escaped-state nesting). Both start with<, so<alone closes every variant.>and&are inert in script data (no entity decoding), and U+2028/U+2029 are legal in JS string literals since ES2019 —JSON.stringifyemits them raw and Chromium/JSON.parseboth accept them.<-only is sufficient for this boundary.
Important
skills/pr-to-video/scripts/captions.test.mjs:244— the inline-script locator regex/<script(?:\s[^>]*)?>([\s\S]*?)<\/script\s*>/giopened a new CodeQL alert (#930,js/bad-tag-filter, high) on this PR; the CodeQL check on this head is red because of it (non-required). It's test-only and has no security impact, but this PR exists to close a CodeQL-identified boundary, and the body doesn't mention the new alert. The rule's recommendation is to use a parser rather than regex: happy-dom is already a root devDependency and the Studio test in this PR uses it —new Window()+template.innerHTML = html+querySelectorAll("script")would find thevar GROUPS =script without any tag regex and clear the alert. Alternatively,html.indexOf("var GROUPS =")and slicing to the next</scriptis enough since the test only needs the data statement. Recommend fixing before merge so a high alert doesn't land on main; not blocking the code change itself.
Nits
packages/studio/src/captions/generator.ts:280,306,309and the fourcaptions.mjssites repeat the same.replace(/</g, "\\u003c")literal; a tinyscriptSafeJson(value)helper per file would keep the next stringify site from forgetting it.
Fallow — three inherited findings excluded by the changed-code gate (ran fallow audit --base cd1a996b locally; CI Fallow job green):
generator.ts:262 generateJs(cyclomatic 11 / cognitive 20 / CRAP 37.1) — pre-existing; this PR adds 2 lines (109→111) and no branches (thewordIdternary already existed). Not made worse in complexity.generator.ts:109 buildGroupStyleDecls(cyclomatic 37, CRITICAL) — pre-existing and untouched by this PR.- Duplication
generator.test.ts:11-19↔parser.test.ts:200-208(buildTestModelhelper) — pre-existing and untouched; the new test block lives at :35-62.
CI at this head: Build, Typecheck, Test: runtime contract, Render on windows-latest, regression, Semantic PR title, Preflight, Preview parity, Fallow all green; Test green. CodeQL (non-required) red per the finding above.
Verdict: APPROVE
Reasoning: The <-only escape is provably sufficient for the script boundary, is applied at every JSON-into-script site in Studio and all three byte-identical workflow copies, and every regression goes red when the escape is reverted; the only defect is a test-only regex that opens a new high CodeQL alert, which is worth clearing before merge but does not affect the shipped fix.
— Miga (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review of head 91aca1de. This head is a plain merge of main (c752b89b) over the fix commit fa6f75ed; the branch patch against its merge-base is byte-identical to the fa6f75ed patch (range-diff shows both PR commits unchanged), so everything below was verified on that patch.
Strengths
skills/pr-to-video/scripts/captions.test.mjs:244-248— the regex tag filter CodeQL flagged is gone. The locator is nowindexOf("var GROUPS ="), then a lower-casedindexOf("</script", start)and a slice, with an assertion on each missing marker, so a malformed emit fails with a named reason instead of a silent no-match.- Production source is untouched: the diff since my
21677a84approval is this test file plusskills-manifest.json.
Verified
- Focused caption suite at the fix: 39/39 pass.
- Mutation, revert one emitter's
<escape in the pr-to-video copy only: exactly two fail (the byte-identical-copies guard and the pr-to-video skin regression), the expected pair. - Mutation, make the close-tag search case-sensitive (
</SCRIPT): all six CLI regressions fail, so the case-insensitive close is load-bearing rather than decorative. gen-skills-manifest.ts --checkreports the manifest in sync (20 skills); the hash bump is the regenerated value.- The red
Test: runtime contractonfa6f75edwas main's regression from #3845 (lint-runtime-preview-guardson a file that does not exist in this branch), fixed on main by #3850. This merge picks that up and every required check is green on91aca1de. - CodeQL #930: the latest analysis on the PR merge ref reports 0 results. Not dismissed or modified by me.
Nits
- None new. The optional serializer-helper refactor stays out of scope for this fix, as the author states.
Verdict: APPROVE
Reasoning: The only change since the prior approval is a test-side locator that removes the flagged regex, both mutations land on exactly the tests they should, production source is unchanged, and all required checks pass on this exact head.
— Miga (pr-review)
Caption words containing
</script>could close the generated script element and introduce markup when the resulting HTML was parsed. Escape<in JSON embedded in Studio's caption generator and both the preset-skin and default emitters in all three caption workflow scripts. Decoded caption text and word IDs retain their original values.This fixes the separate generator boundary identified during review of security alert #102. It does not change the caption parser or claim to close #102. Studio's generator currently has no repository caller beyond its barrel export; the workflow CLI emitters are exercised directly.
Validation: full build; 4,817 Studio tests passed (18 todo), 627 skill tests passed (2 skipped); focused HTML-parser/round-trip regression plus six real CLI cases across three workflows and two emitter modes. Original source fails the Studio boundary regression and all six CLI regressions. Lint, formatting, strict types, manifest regeneration and signed commit hooks passed. Fallow's changed-code gate passed with three inherited findings excluded; no suppressions added.