fix(core): lint leaked head text in compositions#1727
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adds a head_leaked_text HyperFrames lint error that catches three failure shapes where code/CSS text leaks into the rendered video frame as visible document text:
- Orphan CSS rules in
<head>outside any<style>block (the documented bug from the duplicated</style>then bare CSS). - Stray
</style>/</script>closing tags in<head>. - Markdown code fences in
<head>(the LLM-generated-composition failure mode).
The mechanic in core.ts:70-89: extract every <head>...</head>, strip valid block-elements (<style>/<script>/<template>/<title>) via HEAD_BLOCKS_TO_IGNORE_PATTERN, then scan the residue with three more regexes (markdown-fence, orphan-CSS, stray-close). The rule emits a single finding on first leak per head; severity "error" blocks at lintHyperframeHtml callers.
Verdict: REQUEST CHANGES — one BLOCK at 910b4f4971f9d90c9e958c665de271808c7d63fc, plus several NIT-grade cleanups. The block is straightforward and a one-character fix; once that's in I expect to LGTM the next push.
🚫 BLOCK — CodeQL is right: HEAD_BLOCKS_TO_IGNORE_PATTERN's closing tag missing \s* produces false-positive head_leaked_text errors
GitHub Advanced Security (CodeQL "Bad HTML filtering regexp", code-scanning/678) flagged that HEAD_BLOCKS_TO_IGNORE_PATTERN at core.ts:60-61 doesn't match closing tags with whitespace before >:
const HEAD_BLOCKS_TO_IGNORE_PATTERN =
/<(?:style|script|template|title)\b[^>]*>[\s\S]*?<\/(?:style|script|template|title)>/gi;
// ^ no \s* before >That alone would just be a defensive XSS-filter heuristic (and we're not doing XSS filtering here, we're doing dev-input lint), so I'd normally let it pass. But the same file ALREADY has the \s*-before-> shape in the SIBLING regex four lines later:
const STRAY_HEAD_CLOSE_PATTERN = /<\/(?:style|script)\s*>/i;
// ^ \s* allowedSo a perfectly valid <script>...</script > (with a trailing space — hand-edits, some formatters, certain template-engine outputs all produce these) would fall through this sequence:
HEAD_BLOCKS_TO_IGNORE_PATTERN.replace(...)— does not match (missing\s*) → the script block is NOT stripped, its contents stay inwithoutValidBlocks.STRAY_HEAD_CLOSE_PATTERN.exec(withoutValidBlocks)— DOES match the</script >→ returns a "stray close" finding.
Net effect: a clean composition with a single whitespace-padded </script > gets a head_leaked_text error pointing at its perfectly-valid script tag. With severity: "error" that blocks the lint at all callers (preview/render bundling per the PR body).
Fix is a single character on each closing-tag group:
const HEAD_BLOCKS_TO_IGNORE_PATTERN =
/<(?:style|script|template|title)\b[^>]*>[\s\S]*?<\/(?:style|script|template|title)\s*>/gi;Worth a regression test exercising <script>js</script > (or <style>css</style\n>) inside an otherwise-clean head, asserting finding is undefined.
The inconsistency between the two regexes reads as oversight, not design — confirm or fix.
NIT findings (non-blocking)
-
NIT (markdown-fence language coverage) —
MARKDOWN_CODE_FENCE_PATTERN = /```(?:html|css|js|javascript|typescript|ts)?(?:\s|$)[\s\S]*?```/icovers the most common LLM outputs but silently misses fences fortsx,jsx,json,bash,sh,python,py,rust,go,md,markdown,yaml,toml, etc. The no-language fence (\``\n) IS caught (the language alternative is optional) but```unknownlang\n...\n```slips through both the fence-check (no language match) AND falls through to orphan-CSS (only matches if content containsselector { prop: value }). LLM-generated content with a Rust or Python pseudo-code block inwould land in production. Widening the language group to[a-z]or\w` closes this without false-positive risk (markdown fences with language tags are always alphanumeric). -
NIT (test coverage gap —
</script>stray-close) —STRAY_HEAD_CLOSE_PATTERNalternates overstyle|script, but the only stray-close test (core.test.ts:174) exercises</style>. A one-line test against</script>would lock in the regex's alternation correctness. (Doubly relevant after the BLOCK-fix above adds the\s*variant — both alternations and the whitespace tolerance should be exercised.) -
NIT (test coverage gap —
@-rule orphan CSS) —ORPHAN_CSS_RULE_PATTERN's first alternative@[a-z-]+[^{}<]*covers@media,@keyframes,@font-face,@import, etc. No test exercises this branch. A composition with an orphan@media (max-width: 100px) { .x { color: red; } }outside any<style>block is realistic enough to deserve direct coverage. Also worth verifying the nested-brace case is handled the way it appears to be: outer@media { .x { foo: bar } }won't match the outer rule (because[^{}]*:[^{}]*requires no inner braces between the outer open/close), but the inner.x { foo: bar }would match as a class-selector orphan — so the rule still fires, just on the inner. Worth a comment or test pinning that behavior. -
NIT (multiple-leak reporting) —
findLeakedTextInHeadreturns the FIRST leaked text and stops (core.ts:74-87). If a composition has both an orphan CSS rule AND a markdown fence, the author fixes one, runs lint again, finds the other, fixes that, runs lint again, finds the next, etc. Returning all findings in a single pass (push to an array rather than early-return) would consolidate the developer loop. Minor — usually one fix exposes the rest as the iteration goes, but for LLM-generated outputs with multiple leak shapes you'd want them all at once. Author discretion. -
NIT (stray-close snippet boundary) — when
STRAY_HEAD_CLOSE_PATTERNmatches, the snippet iswithoutValidBlocks.slice(strayCloseMatch.index, strayCloseMatch.index + 160)(core.ts:84-85). 160 is hardcoded, thentruncateSnippet(core.ts:131) likely truncates further. IftruncateSnippet's default is <160, the extra context is wasted; if >160, useful context is lost. Either align the 160-slice withtruncateSnippet's default truncation length, or factortruncateSnippet's ceiling into a shared constant. Trivial. -
NIT (unclosed
<style>becomes head_leaked_text) — interesting consequence worth confirming intentional: a head with<style>foo { color: red; }and no closing</style>would not be stripped byHEAD_BLOCKS_TO_IGNORE_PATTERN(which requires the close). After HTML-tag stripping leavesfoo { color: red; }as residue,ORPHAN_CSS_RULE_PATTERNmatches. Net result: the rule reportshead_leaked_texton an unclosed<style>. That's probably the right behavior (unclosed style blocks are malformed), but it isn't tested or documented. A short test pinning "unclosed<style>reportshead_leaked_text" both nails down behavior and gives a useful diagnostic when a user encounters it. Author discretion — could also argue for a more specificunclosed_style_blockcode, buthead_leaked_textis descriptive enough since the user-visible symptom is identical.
Side observation (not a finding)
The severity: "error" choice is well-calibrated — the failure mode here (text leaking into a rendered video frame) is user-visible and hard to spot in preview compositing if the LLM/author isn't watching for it. Aligning with severity: "error" on the surrounding rules (orphaned-script, missing-composition-id, etc.) sets the right bar for "blocks CI."
CI state
CI on 910b4f4 is mid-flight at read time, but Lint, Typecheck, Build, SDK unit/contract/smoke, Studio: load smoke, Test: runtime contract, Test: skills and the early CodeQL passes are all green. CodeQL Bad-HTML-Regexp is the single annotation, and it IS the finding I'm blocking on. Other shards / Tests / regression-shards in progress. Test: skills SKIPPED (path-filtered — no skills/ changes).
Prior reviewer state
github-advanced-security[bot] posted a COMMENTED review at 910b4f4 (23:45 UTC) with one inline alert on core.ts:61. That alert is the BLOCK above. No human reviewers at read time; Rames also @-mentioned on the ping.
Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at HEAD 910b4f4971f9d90c9e958c665de271808c7d63fc (base main). No human peer reviews yet — only github-advanced-security[bot] posted (empty boilerplate). Looped Via on the runtime-side cross-coverage angle (does the runtime ALSO reject head-text leaks at parse/render time, making this lint defense-in-depth or sole gate?) — leaving that lane for her.
Pure-additive +168/-0: new head_leaked_text rule slotted between root_missing_* and missing_timeline_registry in coreRules. Severity error, no <noscript> / <base> in the ignore list, message + fixHint follow the sibling rules' shape. Wrote a node probe against the 4 regexes and exercised ~25 edge shapes; ReDoS is fine (V8 handles all probed inputs <1ms).
⚠️ Concerns
1. HTML-commented CSS in <head> is a confirmed false positive — packages/core/src/lint/rules/core.ts:120-122.
The rule reads ctx.rawSource, which is html || "" before stripHtmlComments runs (see packages/core/src/lint/context.ts:32-36). Authors commenting out CSS while iterating (very common in composition authoring) will trip:
<head><!-- .particle { position: absolute } --></head>
→ flags as head_leaked_text: ".particle { position: absolute }"
<head><!-- ```css\n.x{color:red}\n``` --></head>
→ flags as head_leaked_text: "```css ... ```"
Browsers do NOT render HTML-comment content, so this is a true false positive — opposite of the PR's stated goal ("Browsers render this as visible text in the video"). Suggest scanning the comment-stripped source instead of rawSource, OR pre-stripping <!-- ... --> inside each head match before the three sub-patterns run.
2. <noscript> content in <head> false-positives — packages/core/src/lint/rules/core.ts:60-61.
HEAD_BLOCKS_TO_IGNORE_PATTERN covers style | script | template | title but not noscript. <noscript> is a valid HTML5 head element (sibling treats it as such — see isStudioTimelineElement at core.ts:27 already enumerates "noscript" alongside head-block tags). Probed:
<head><noscript>JS disabled. .alert{color:red}</noscript></head>
→ flags orphan: ".alert{color:red}"
<head><noscript>```html disabled```</noscript></head>
→ flags codefence
For server-rendered video the <noscript> body is dead weight, but compositions sometimes carry one for the Studio preview iframe. Add noscript to HEAD_BLOCKS_TO_IGNORE_PATTERN.
3. Unclosed <head> is silently skipped — packages/core/src/lint/rules/core.ts:70.
/<head\b[^>]*>([\s\S]*?)<\/head>/gi is non-greedy and requires a matching </head>. Compositions missing the close tag (which are exactly the kind of malformed input that produces leak symptoms in production) get zero head-scan output:
<head><style>body{margin:0}</style>orphan: .x{color:red}<body></body>
→ CLEAN (no </head>, so no match — orphan invisible to rule)
Either (a) extend to allow EOF or next <body> as a soft terminator, or (b) document that malformed-close compositions need to pass the existing structural lint first. The PR's stated root cause ("malformed head markup … duplicated </style> followed by CSS rules") sits one keystroke from the unclosed-head case.
🟡 Questions
4. Negative-test coverage gap — packages/core/src/lint/rules/core.test.ts:249-262.
Only one negative test covers the positive <title>/<meta>/<link>/<style> quartet. Per the FP-suppression rubric, the defensive bug-pinning negative tests I'd want to see explicit for this rule:
- HTML-commented-out CSS (
<!-- .x{c:y} -->) → should NOT flag (per concern 1). <noscript>with prose containing a colon/braces → should NOT flag (per concern 2).- A
<title>whose text contains</style>or backticks substring → should NOT flag (currently safe — confirmed via probe — but worth pinning to prevent regression when concern 1's fix touches the strip order). <head>with only<meta charset>-style void tags (no closing siblings) → should NOT flag.
5. Sub-composition <template> heads — packages/core/src/lint/rules/core.ts:120.
Most rules early-return on options.isSubComposition or <template-prefixed input (see core.ts:139). head_leaked_text does NOT — it scans rawSource, which for a sub-composition wrapping <template><html><head>... still hits the inner <head>. Probably fine (you'd want to lint leaks in sub-compositions too), but worth one explicit test to lock the contract.
6. Cross-repo / pipeline rollout.
PR body says "downstream generation paths should run HyperFrames lint before accepting generated composition artifacts once this rule is available in their installed @hyperframes/core package." Where does this rule actually run today — Studio author save? CI on apps/*? Server-side accept-gate before render? Severity error means existing compositions in the wild that have leaked head text get rejected on their next round-trip. Is there a known dirty-cohort cleanup plan, or is this gating only net-new authoring? (Per feedback_prevention_only_pr_existing_state_q.)
💭 Notes
7. Stray-close path may emit confusing snippets — packages/core/src/lint/rules/core.ts:84-87.
STRAY_HEAD_CLOSE_PATTERN matches case-INsensitively but withoutValidBlocks.slice(strayCloseMatch.index, +160) is anchored on a regex run without g flag against a withoutValidBlocks whose leading whitespace has been collapsed to single spaces. The 160-char snippet sometimes shows only </style> with no surrounding context (see test fixture on core.test.ts:182 — expect(finding?.snippet).toContain("</style>") is satisfied by literally that string). Consider widening the slice or anchoring on the original headContent so the author sees what came BEFORE the stray close.
8. Orphan-CSS regex generic-tag arm — packages/core/src/lint/rules/core.ts:67.
[a-z][\w-]*(?:\s+[.#:[\w-][^{}<]*)?\s*\{[^{}]*:[^{}]*\} matches an element-name selector like body { ... }. This is sound for raw orphan CSS but matches against residualText after HTML_TAG_PATTERN strips all tags. So a stray text fragment like note: { foo: bar } inside a non-CSS context (e.g. a stray JSON snippet) would also flag. Edge case — unlikely in real heads, but if head_leaked_text ever moves to scan <body> text in a follow-up, this regex is too permissive.
✅ Good
- Sibling consistency:
severity: "error",codesnake_case,fixHint-with-backticks,truncateSnippet(...)shape — all match the other ten rules incoreRules. - Case-insensitive: probed
<STYLE>...(uppercase) — correctly ignored. - Multiple
<head>blocks: probed a<template>-wrapped + a real<head>— both scanned, second flags correctly. - ReDoS-safe: all 4 regexes stay sub-millisecond against
n=10000pathological inputs. - Bare CSS comment in head (
<head>/* .x { c: r } */</head>) correctly flags — that is visible text in the rendered frame. - Rule executes via
coreRulespush without needing extra wiring inhyperframeLinter.ts.
Lane note: I'm leaving runtime-side cross-coverage for Via (does the player/runtime ALSO reject head-text leak at parse or render time? If yes, this lint is defense-in-depth and the FP risk in concerns 1-2 is lower-stakes; if no, this lint becomes the sole gate and the FP risk gets sharper). Not blocking on her layer.
Not blocking — concerns 1-3 are real but the rule already pays for itself on the orphan-CSS path. Worth a follow-up to close the FP holes before the rule starts producing noisy author-side errors.
— Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Addendum — Via posted independently at 910b4f4 ~1 min before mine landed. Layering rather than re-deriving.
Confirm Via's 🚫 BLOCK (CodeQL #678 — missing \s* before > in HEAD_BLOCKS_TO_IGNORE_PATTERN)
Reproduced via my probe against the rule's regex set. The CodeQL alert is real, the failure mode Via described is exact:
<head><script>window.x=1</script ></head> → flags head_leaked_text (stray: "</script >")
<head><style>body{margin:0}</style ></head> → flags head_leaked_text (orphan: "body{margin:0}")
<head><script>window.x=1</script\n></head> → flags head_leaked_text (stray: "</script\n>")
<head><script>window.x=1</script\t></head> → flags head_leaked_text (stray: "</script\t>")
Inconsistency with STRAY_HEAD_CLOSE_PATTERN at packages/core/src/lint/rules/core.ts:63 (which DOES allow \s*) is the smoking gun. +1 to Via — block this, one-character fix, add the </script > regression test she called out.
Confirm Via's NIT 1 (markdown-fence language gap)
Also reproduced — rust, tsx, json, and unknown-lang fences all slip past MARKDOWN_CODE_FENCE_PATTERN:
<head>```rust\nfn x(){}\n```</head> → CLEAN (silent miss)
<head>```tsx\nconst x = <X/>;\n```</head> → CLEAN
<head>```json\n{"a":1}\n```</head> → CLEAN
<head>```mystery\nblah\n```</head> → CLEAN
Widening the language group to [a-z]* or \w* closes the hole without adding FP risk (markdown-fence language identifiers are always alphanumeric by spec).
Overlap reconciliation with my findings
- My concern #3 (unclosed
<head>) and Via's NIT 6 (unclosed<style>) are adjacent but distinct: hers is "open<style>→ rule fires as orphan-CSS" (which is arguably correct behavior); mine is "no</head>close → rule silently no-ops" (which is silent miss). Both worth pinning with tests. - My concern #4 negative-test gap and Via's NIT 2 + NIT 3 (stray-
</script>test,@-rule test) compose into a single negative-tests-and-uncovered-branches expansion. Worth one consolidated test-coverage push. - My concerns #1 (HTML-comment FP) and #2 (
<noscript>FP) are not in Via's review — leaving them as my contribution.
Net: Via's BLOCK + the two FP holes I flagged are independent. The BLOCK is the higher-priority fix (it produces FP errors on perfectly-valid composition input today); my FP holes are lower-rate but real. Recommend all five land together.
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Addendum to my R1 review — reconciling with Rames's parallel read (posted ~3min after mine). His read sharpened two false-positives I missed in my pass and added the unclosed-head gap. Full credit on those; bundling them into the BLOCK before merge is the right call.
Additional findings (concur with Rames)
-
🚫 BLOCK (concur with Rames's #2 on the direction — sharpening the reproduction) — Rames's call to switch from
rawSourcetosourceis the right architectural fix;context.ts:32-36showsrawSource = htmlwhilesource = stripHtmlComments(rawSource). But the specific FP shape needs precision: the orphan-CSS path incore.ts:77-82runsHTML_TAG_PATTERN.replace(...)(which is/<[^>]+>/g) over the head residue, and an HTML comment<!-- .x { color: red; } -->has no>until the closing-->so the whole comment matches as one tag and gets stripped. So the orphan-CSS check on<head><!-- .x { color: red; } --></head>does NOT reproduce. The real FPs land on the two checks that run onwithoutValidBlocksBEFOREHTML_TAG_PATTERN:- Markdown-fence check (line 74-75): a comment containing a fenced block —
<!-- ```css\n.x{color:red;}\n``` -->— survivesHEAD_BLOCKS_TO_IGNORE_PATTERN(only style/script/template/title), sowithoutValidBlocksretains the fence markers.MARKDOWN_CODE_FENCE_PATTERNmatches → false-positivehead_leaked_texton commented-out fences. - Stray-close check (line 84-85): a comment containing
</style>or</script>— the exact shape an author might write while debugging — survives both regex strips (becauseHTML_TAG_PATTERNtruncates at the first>inside</style>, leaving the comment-tail in residue).STRAY_HEAD_CLOSE_PATTERNmatches → false-positive on commented-out close tags.
Either path lands the same architectural fix Rames called: switch the rule's source fromrawSourcetosource, which pre-strips comments viastripHtmlComments(the linear+fixpoint comment stripper noted incontext.ts:34-35). Doing it that way cleans up all comment-related FPs in one motion, including ones we haven't enumerated. Sibling rules incore.tsalready usesourcefor exactly this reason.
- Markdown-fence check (line 74-75): a comment containing a fenced block —
-
🚫 BLOCK (concur with Rames's #3) —
<noscript>excluded fromHEAD_BLOCKS_TO_IGNORE_PATTERNbut treated as a head-block elsewhere in the same file —HEAD_BLOCKS_TO_IGNORE_PATTERNstripsstyle|script|template|titlebut notnoscript. MeanwhileisStudioTimelineElementatcore.ts:27(and the surrounding rules) already recognizenoscriptas a head-block. That asymmetry means a perfectly valid<head><noscript><style>...</style></noscript></head>runs through the new rule with<noscript>and its body NOT stripped → after HTML_TAG_PATTERN strips the tags, the CSS body falls through to orphan-CSS detection. Addnoscriptto the alternation:<(?:style|script|template|title|noscript)\b.... -
🟡 NIT (concur with Rames's #4) —
head_leaked_textuses the non-greedy<head\b[^>]*>([\s\S]*?)</head>pattern atcore.ts:70, which silently skips a composition with no</head>close. An unclosed<head>is itself a malformed composition and the lint should report it (under a different code:head_unclosedor similar). The current behavior is "file with unclosed head passes lint, then breaks at render time" — exactly the failure shape this rule is meant to prevent. Worth either widening to<head\b[^>]*>([\s\S]*?)(?:</head>|<body|$)to capture the implicit head boundary, or adding a separate "unclosed head" rule.
Runtime cross-coverage angle (Rames's lane handoff)
Rames tossed me "does the player ALSO reject head-text leak — defense-in-depth vs sole gate?" Reading the architecture:
-
The lint is currently the sole gate. The PR body explicitly says "Downstream generation paths should run HyperFrames lint before accepting generated composition artifacts once this rule is available in their installed
@hyperframes/corepackage." That's a coordination commitment, not a code-level enforcement — any caller that skips lint (custom CI configs, locally-disabled rules, agent-generated compositions that bypass the standard generation path) leaks head text straight to render. -
The runtime can't realistically reject this late. By the time the player runtime is loaded into the browser, the browser has already parsed the head and is about to paint. A runtime-side scrub would have to run before first paint, which is a fragile timing dependency. Wrong layer.
-
Producer compile is the right second gate. The producer's
htmlCompiler.tsis the layer that has the HTML in-hand before any browser launch. AcompileForRender-time pass that runslintHyperframeHtmland refuses to compile onseverity: "error"findings (or strips known-safe leaks deterministically) would close the gap without depending on the upstream caller having opted into lint. That's the producer's existing job — refuse to render unrenderable compositions. -
Not blocking for THIS PR — the rule itself is correct (once Rames's + my false-positives are fixed), and adding a producer-side second gate is a coordinated follow-up. Worth filing as a follow-up issue: "Wire
lintHyperframeHtmlintocompileForRenderas a hard gate" — keeps the producer's quality bar consistent regardless of how the composition got authored.
Net updated verdict
Still REQUEST CHANGES at 910b4f4 — now with three BLOCK-grade findings (CodeQL whitespace-close from R1, rawSource comment-strip, noscript missing from ignore list) plus the unclosed-head NIT and the original six NITs. All three BLOCK fixes are tiny; the producer-side second-gate work is a separate follow-up issue. Rames's read is the load-bearing one for findings #1 and #2 here — full credit.
Addendum by Via
4 participants
Summary
head_leaked_textHyperFrames lint error for code/CSS text leaked into<head>outside valid blocks</style>/</script>closes, and markdown code fences<title>, metadata, links, and normal<style>blocks cleanRoot cause
A composition can contain malformed head markup such as a duplicated
</style>followed by CSS rules. Browsers treat the remaining text as visible document text, so it appears in the rendered frame. The existing linter parsed valid<style>blocks but did not scan residual raw<head>text after those blocks were removed.Validation
bunx oxfmt packages/core/src/lint/rules/core.ts packages/core/src/lint/rules/core.test.tsbunx oxlint packages/core/src/lint/rules/core.ts packages/core/src/lint/rules/core.test.tsbun run --filter @hyperframes/core test --run src/lint(234 tests)bun run --filter @hyperframes/core build:hyperframes-runtimebun run --filter @hyperframes/core typecheckhead_leaked_textbefore preview/render bundlingNotes
This PR adds the OSS lint rule. Downstream generation paths should run HyperFrames lint before accepting generated composition artifacts once this rule is available in their installed
@hyperframes/corepackage.