Skip to content

fix: keep caption transcript data inside generated scripts - #3847

Merged
jrusso1020 merged 3 commits into
mainfrom
fix/security-caption-script-data
Sep 10, 2026
Merged

fix: keep caption transcript data inside generated scripts#3847
jrusso1020 merged 3 commits into
mainfrom
fix/security-caption-script-data

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

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.

Comment thread skills/pr-to-video/scripts/captions.test.mjs Fixed

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 after JSON.stringify, so it only rewrites string contents; structural JSON never contains <, and < decodes identically under both JSON.parse and the JS parser. Word text and id values 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 parsed GROUPS payload 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 asserting querySelector("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-server entry 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: generateCaptionHtmlextractTranscript returns identical text/id for 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 generateCaptionHtml is only exported from captions/index.ts (no in-repo caller); all 13 hyperframes-creative/frame-presets/*/caption-skin.html skins consume GROUPS via .textContent = String(w.text) and the default emitter via s.textContent = w.text + " ". No innerHTML/insertAdjacentHTML/document.write sink anywhere in the skins or emitters. Other interpolations in generateJs (// Group: ${groupId}, className) are derived from group-N/sanitized ids, not transcript text. Every other JSON.stringify in 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.stringify emits them raw and Chromium/JSON.parse both 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*>/gi opened 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 the var GROUPS = script without any tag regex and clear the alert. Alternatively, html.indexOf("var GROUPS =") and slicing to the next </script is 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,309 and the four captions.mjs sites repeat the same .replace(/</g, "\\u003c") literal; a tiny scriptSafeJson(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):

  1. generator.ts:262 generateJs (cyclomatic 11 / cognitive 20 / CRAP 37.1) — pre-existing; this PR adds 2 lines (109→111) and no branches (the wordId ternary already existed). Not made worse in complexity.
  2. generator.ts:109 buildGroupStyleDecls (cyclomatic 37, CRITICAL) — pre-existing and untouched by this PR.
  3. Duplication generator.test.ts:11-19parser.test.ts:200-208 (buildTestModel helper) — 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 miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 now indexOf("var GROUPS ="), then a lower-cased indexOf("</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 21677a84 approval is this test file plus skills-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 --check reports the manifest in sync (20 skills); the hash bump is the regenerated value.
  • The red Test: runtime contract on fa6f75ed was main's regression from #3845 (lint-runtime-preview-guards on 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 on 91aca1de.
  • 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)

@jrusso1020
jrusso1020 merged commit c98d6fb into main Sep 10, 2026
48 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-caption-script-data branch September 10, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants