fix(core): contain generated HTML CSS and script contexts - #3800
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The encoder itself is ordered correctly (& before delimiters), and all four JSON metadata call sites use it consistently. The composition-ID breakout is closed for a normal browser parse.
Blocker — the advertised parser round-trip is still broken. packages/core/src/generators/hyperframes.ts:373 now survives one DOMParser pass, but the public parseHtml path performs two: ensureHfIds(html) parses and serializes with linkedom, then DOMParser.parseFromString parses that serialization again (packages/parsers/src/htmlParser.ts:180-183). Linkedom decodes " to a literal " in the attribute and does not re-escape that ampersand when serializing, so the second parse decodes it again.
Executed at exact head with the same marker used by the new tests (" ' & < > ' "):
stylesreturns a corrupted fallback string because JSON no longer parses;data-keyframesis silently dropped;data-zoom-keyframesbecomes[];data-variable-valuesis silently dropped.
The new DOM tests at packages/core/src/generators/hyperframes.test.ts:40-59 exercise only one parse, so they pass while the source→parseHtml path named by #438 is unchanged. Please pin all four through real parseHtml(generateHyperframesHtml(...)) coverage (including variableValues) and fix the double-parse/serialization boundary. Do not solve this by double-encoding the generator: direct browser consumers parse once and would then receive the wrong value.
CI is still running, but this source-level failure is independently reproducible and blocking regardless of its outcome.
Verdict: REQUEST CHANGES
Reasoning: #437 is fixed, but #438 and the sibling metadata round-trips still fail through the public parser path the patch claims to repair.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The original blocker is genuinely fixed for generated lowercase documents: all four metadata values now survive the public parser, and the tests finally pin variableValues as well as styles/keyframes/zoom.
Blocker — parseHtml now changes HTML parser semantics for every caller. At packages/parsers/src/htmlParser.ts:181, the function consumes linkedom's document directly instead of using the caller/runtime DOMParser as before. Linkedom preserves HTML attribute-name case; the native/jsdom parser normalized it. That makes valid case-insensitive HTML disappear from the timeline.
Executed at exact head against the base:
<!doctype html><HTML DATA-RESOLUTION="square"><BODY>
<DIV ID="x" DATA-START="2" DATA-DURATION="3" DATA-TRACK-INDEX="4" DATA-NAME="UP">
<DIV>hello</DIV>
</DIV>
</BODY></HTML>
base: 1 text element, start=2 duration=3 track=4, resolution=square
head: 0 elements, resolution=portrait
Uppercasing only DATA-START is enough to drop the element. The 930-test result does not probe this boundary because parseHtml no longer uses the jsdom environment at all; it always returns the linkedom document. It also invalidates the explicit test split documented in packages/parsers/src/htmlParser.linkedom.test.ts:3-10 (CLI/linkedom-specific tests vs the rest under jsdom).
A bounded differential over 843 checked-in HTML sources found 842 byte-semantic matches; the sole difference was an unhydrated Git-LFS pointer, correctly rejected as plain text. That is useful blast-radius evidence, but it does not cover valid mixed-case HTML. The narrow fix is to parse once with the existing runtime DOMParser, then assign hf-ids inside that document via a DOM-agnostic helper shared with ensureHfIds; do not hard-wire parseHtml to linkedom. Keeping that helper internal also avoids unintentionally adding parseHtmlWithHfIds to both public parser export surfaces (@hyperframes/parsers and /hf-ids).
#904 audit: the prior alert's five flows are four new test DOMParser calls plus the production parseHtml call, all reaching the already context-encoded root attribute interpolation. If the same alert survives at the final corrected head, it is eligible for a scoped false positive disposition (not used in tests, because one flow is production). I would not dismiss it while this parser regression remains or before confirming the new scan is the same flow.
CI is still running; this is independently reproducible and blocks regardless of its result.
Verdict: REQUEST CHANGES
Reasoning: Metadata round-trips are fixed, but the mechanism regresses standards-valid mixed-case HTML across the public parser and silently removes clips.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The document ownership is now right: parseHtml uses its runtime parser once, the DOM-only assignment helper is private, and all four metadata round-trips remain fixed. A differential over 843 checked-in HTML sources is 843/843 semantic matches against base.
Blocker — first-pass IDs now diverge between the two parser contexts. contentKey hashes raw attribute names at packages/parsers/src/hfIdAssignment.ts:42-50. Native/jsdom HTML parsing lowercases attribute names; linkedom (used by ensureHfIds) preserves authored uppercase names. The same valid, unpinned mixed-case document therefore gets different IDs from the two public paths:
source: <DIV ID="x" DATA-START="2" DATA-DURATION="3" DATA-NAME="UP">…</DIV>
ensureHfIds (linkedom): data-hf-id="hf-u5ey"
parseHtml (jsdom/native): id="hf-wxsa"
lowercase control: both id="hf-wxsa"
That violates this file's own load-bearing invariant at packages/parsers/src/hfIdAssignment.ts:66-72: disk-persist and in-memory/parser mint contexts must agree or drag/edit targeting splits. The new mixed-case regression at packages/parsers/src/htmlParser.test.ts:15-28 proves the semantic fields, but does not assert ID parity, so it passes over the divergence.
Please canonicalize HTML attribute names for hashing and case-insensitive contract checks in the DOM-only assignment layer, then add a direct parity test between ensureHfIds(html) and parseHtml(html) for mixed-case input. Be careful to cover the other case-sensitive reads in that layer too (data-hf-* exclusion/pinning and data-composition-id template detection), not only the .map() line.
The 843-file differential is a good non-regression control; this case is absent because those checked-in sources use lowercase attributes. CI and the new CodeQL scan are still running. #904 remains unactioned; my prior source classification stands only if the final scan reports the same five post-encoder flows.
Verdict: REQUEST CHANGES
Reasoning: Parser normalization and metadata are fixed, but the new one-pass split mints different stable IDs for the same standards-valid HTML across the two supported parser contexts.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The HTML-namespace fixes are correct: mixed-case attributes, pinned IDs, editor-state exclusion, and both composition-template forms now agree. The assignment helper remains private, and the 843-file corpus differential is still 843/843 against base.
Blocker — the namespace carve-out leaves the same ID split live inside SVG. attributeName at packages/parsers/src/hfIdAssignment.ts:42-44 preserves raw attribute case for non-HTML namespaces. But both supported paths parse an HTML composition: native/jsdom applies the HTML parser's SVG foreign-attribute adjustments, while linkedom preserves authored case. The same source therefore still hashes differently before and after ensureHfIds persistence:
<svg><linearGradient VIEWBOX="0 0 1 1"></linearGradient></svg>
runtime/native assignment: hf-ilai
ensureHfIds/linkedom: hf-tc22
<linearGradient GRADIENTUNITS="userSpaceOnUse">
runtime/native assignment: hf-cgck
ensureHfIds/linkedom: hf-lah0
A second probe with uppercase ID CLASS ARIA-LABEL DATA-HF-STATE also splits (hf-s650 vs hf-82gb). This is the exact invariant at packages/parsers/src/hfIdAssignment.ts:76-82; SVG descendants are editable/stamped by this walker, so this is not an inert namespace edge. The added tests cover HTML elements and templates, but no foreign-content node.
Please canonicalize the hash independently of either parser's foreign-attribute behavior and add direct native-assignment vs ensureHfIds parity for an SVG descendant with mixed-case standard and data-hf-* attributes. If XML elements outside text/html truly need case-sensitive IDs, make that an explicit input/mode; namespaceURI alone cannot distinguish XML parsing from SVG embedded in an HTML document. Lowercasing names for the hash does not mutate or erase the actual SVG attribute spelling.
Static-analysis audit: exact-head JavaScript CodeQL is still running. Prior #904 is the same five post-encoder flows already audited. Prior #905 traces parsed data-zoom-keyframes text through the now-encoded generator and back into DOMParser; it is likewise structurally contained. If those exact flows recur after this final parity fix, I approve scoped false positive dispositions for #904 and #905. Neither should be dismissed before confirming the final instances.
CI is pending; this source-level parity failure blocks independently.
Verdict: REQUEST CHANGES
Reasoning: HTML and template parity are fixed, but mixed-case SVG attributes still mint different stable IDs across the runtime and persisted paths.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The ID blocker is cleared. All four SVG controls now converge across native assignment and ensureHfIds, including pinned IDs and idempotence; viewBox remains intact. The 843-file parser differential remains 843/843 against base.
Blocker — final CodeQL #904 is a real, different unsanitized option. The exact alert location is packages/core/src/generators/hyperframes.ts:379, columns 123–137. Those columns are ${resolutionAttr}, not ${customStylesAttr}. resolutionAttr is built from raw options.resolution at line 376:
const resolutionAttr = ` data-resolution="${resolution}"`;CanvasResolution is only a TypeScript union; this is a published JavaScript library function. With includeStyles: false, an external JS caller can pass a quote-bearing string and create sibling attributes/tags on <html> exactly as #437 did through compositionId. The six final #904 flows (five generator tests plus production parseHtml) all originate at the whole options object, which is why the alert survived the metadata encoder.
Please apply the attribute encoder here and add a direct DOM breakout test for resolution. Since resolution is a closed semantic enum, runtime validation against the canonical resolution set would also be appropriate; encoding is still the required output-context boundary.
#905 disposition: APPROVE scoped false positive. Its exact source is parsed data-zoom-keyframes text at packages/parsers/src/htmlParser.ts:477; the only route back to the line-183 DOMParser sink is through validated JSON structure, JSON.stringify, and escapeHtmlAttributeValue on data-zoom-keyframes. I reproduced entity-bearing zoom IDs through that round-trip, and no HTML syntax survives the encoder. Do not classify #905 as used in tests: the sink is production parser code. Dismiss #905 only; do not dismiss #904.
The JavaScript analysis itself passed in 8m10s; the native CodeQL gate is red on these two alerts. Other required checks are still settling.
Verdict: REQUEST CHANGES
Reasoning: ID/parser behavior is now correct, but #904 identifies a live attribute breakout through the still-unescaped public resolution option. #905 is an independently verified false positive.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
All previously identified blockers are clear at exact head 6b7e59d6.
packages/core/src/generators/hyperframes.ts:376now applies the same complete attribute encoder to the publicresolutionoption. The reflected JavaScript-caller regression atpackages/core/src/generators/hyperframes.test.ts:57-70pins the actual untyped package boundary without changing invalid-value behavior.packages/parsers/src/hfIdAssignment.ts:42-49now canonicalizes hash input without mutating SVG attributes. I reran the four native/persisted SVG cases:VIEWBOX,GRADIENTUNITS, mixedID/CLASS/ARIA/DATA-HF-STATE, and pinnedDATA-HF-IDall converge; persistence is idempotent andviewBoxretains its value.- The public parser remains single-parse, all four JSON metadata attributes round-trip, and an independent 843-file corpus differential is 843/843 against base.
Exact-head JavaScript CodeQL passed in 8m03s. #904 is fixed in the final scan; #905 is dismissed only as the scoped false positive previously authorized, with the correct production-flow evidence. No new PR alerts appeared; #439–441 remain the explicitly separate follow-up set. Build, typecheck, lint, Fallow, producer tests, runtime contract, CLI smoke, preview parity, and the render lane are green; some broader regression/Windows test/perf children are still running, with no failure. Merge should continue to wait for the required aggregators.
Nit: the PR body still says 32 generator / 930 parser tests; the final reported counts are 33 / 940.
Verdict: APPROVE
Reasoning: Every live source-to-sink and cross-parser ID mismatch is closed, the final CodeQL graph confirms #904 fixed, and #905's lone dismissal matches the audited safe path.
— Magi
jrusso1020
changed the title
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Fresh review of exact head 3bb18ca8. The context-specific work is materially sound: richTextHtml inert-parses, applies the existing rich-text allowlist, then serializes the sanitized tree explicitly (packages/core/src/generators/richTextHtml.ts:13-41); every ordinary element attribute and media URL now crosses an encoder/validator (packages/core/src/generators/hyperframes.ts:500-654); and selector, number, ease, style/script raw-text boundaries are separated rather than sharing one generic escape. The live PR scan confirms #437, #438, and #440 are gone.
Blocker — empty captions still do not survive the public generator/parser round-trip. The generator correctly changed the sentinel to element.content ?? element.name at packages/core/src/generators/hyperframes.ts:581, but parseHtml still reads it with textEl?.textContent || name at packages/parsers/src/htmlParser.ts:270-273. The new parameterized test at packages/core/src/generators/hyperframes.test.ts:111-117 stops after one DOM parse, so it proves the generated node is empty but not the public round-trip claimed in the body.
Executed at this head:
generated inner text: ""
parseHtml(generateHyperframesHtml(...)).elements[0].content: "Title"
Use the nullish distinction at the parser boundary too (textEl?.textContent ?? name) and pin the empty case through parseHtml, not only DOMParser.
Blocker — the native security gate is correctly still open on intended executable inputs, and the public trust contract is not documented. The live PR-ref alerts are #439, #441, and #906–#919. I traced them rather than treating the count as one class:
- #906–#916 are scoped false positives: those branches concatenate only strings produced by
elementSelector+JSON.stringify,finiteNumber, JSON-encoded ease, or fixed media-sync source (packages/core/src/generators/hyperframes.ts:287-326, 785-859). I approve individual false-positive dispositions for those exact instances if the head and flows remain unchanged. - #439, #441, and #917–#919 are not false positives.
stylesis intentionally inserted as authored CSS;serializeValueintentionally returns__raw:source verbatim (packages/parsers/src/gsapSerialize.ts:303-308); andserializeGsapAnimationsaccepts rawpreamble/postamblesource (:294-299). That is a legitimate composition-author capability, but it is also exactly the CodeQL library-input threat model.SerializeOptionsand the public functions have no trusted-input warning (packages/core/src/generators/hyperframes.ts:90-100), whiledocs/packages/core.mdx:65-72currently presentsanimationsandstyleswithout one. The official query recommendation is to document code-construction functions that require trusted inputs. Add shipped JSDoc/docs forstyles,animations/__raw,includeScripts,generateHyperframesHtml,generateHyperframesStyles, andserializeGsapAnimations; then disposition these accurate intentional-capability alerts as accepted risk / won’t-fix rather than false positive, or redesign the raw API boundary. The PR cannot merge while this native gate is open in any case.
Fallow assessment (non-blocking): two clone groups are parallel regression fixtures and are clearer left explicit. The production clone at packages/core/src/generators/hyperframes.ts:760-773 and :810-822 duplicates visibility bookend emission across keyframed/default modes. Both use the same selector/numeric guards today and differ in opacity-source semantics, so I would not block this security fix on extracting it, but it is the only warning with future security-drift cost.
Local verification: 46 generator + 62 sanitizer tests pass; the focused hf-id/html-parser set is 79 pass / 2 todo. I also executed the empty-caption round-trip above. Hosted checks have no failure, with two broader children still running; the security alert gate is independently blocking.
Verdict: REQUEST CHANGES
Reasoning: The five original output contexts are substantially contained, but the advertised empty-caption round-trip is still false, and the remaining real raw CSS/JS capabilities need an explicit public trusted-input contract plus accurate native-alert disposition before this can ship.
— Magi
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Both blockers from review 5151148600 are cleared at exact head 3ccbcf85.
- Empty content now uses the same nullish contract on both sides: generation at
packages/core/src/generators/hyperframes.ts:587and parsing atpackages/parsers/src/htmlParser.ts:272. The regression atpackages/core/src/generators/hyperframes.test.ts:111-120now asserts both the browser DOM andparseHtml(generateHyperframesHtml(...)); it fails on3bb18ca8and passes here for empty and entity-looking content. - The authored-code trust boundary is now shipped, not just described in the PR.
SerializeOptions,generateHyperframesStyles,generateGsapTimelineScript, andgenerateHyperframesHtmlcarry direct warnings (packages/core/src/generators/hyperframes.ts:90-102, 134-139, 217-222, 338-343);serializeGsapAnimationsdocuments__raw,timelineVar,preamble, andpostamble(packages/parsers/src/gsapSerialize.ts:225-235); Core README and public docs state that encoding is not a sandbox and require isolated execution (packages/core/README.md:25-38,docs/packages/core.mdx:78-89). I built the parser declarations and confirmed the serializer warning survives into the shipped.d.ts.
Final CodeQL disposition, pinned to merge ref 1e400993:
- False positive: #906, #907, #908, #909, #910, #911, #914, #915, #916, #920, #921. Every one terminates at the test's
new Functionsink, but the production source legs are generated selectors passed throughelementSelector+JSON.stringify, finite-number guards, JSON-encoded ease, or fixed media-sync source (packages/core/src/generators/hyperframes.ts:296-331, 794, 863-864). These are safe generated JavaScript, not test-only code, sofalse positiveis the accurate disposition. - Accurate intentional capability / accepted risk (
won't fix): #439, #441, #917, #918, #919. #439 is authored CSS at the style-tag boundary; #441 is the executable script-tag boundary; #917-#919 coverserializeGsapAnimations' code-bearing inputs (packages/parsers/src/gsapSerialize.ts:304-305). The test sink is intentional proof that those outputs execute; it does not make themused in tests. The new trust/isolation documentation satisfies the CodeQL recommendation for this deliberately executable public API. - #912 and #913 are absent from the final open set. Do not disposition them as part of this head.
The three Fallow clone groups are unchanged; my prior non-blocking assessment stands. Local generator verification is 46/46. Exact-head JavaScript analysis, docs, build, typecheck, lint, and Fallow pass; broader regression/Windows children are still running with no failure. Merge must continue to wait for those required aggregators and for every alert above to receive only its specified individual disposition.
Verdict: APPROVE
Reasoning: Empty-caption round-trip and the public trusted-code contract are now complete, and the final alert set cleanly separates eleven safe generated-code false positives from five documented intentional code/CSS capabilities.
— Magi
3 participants
Generated composition metadata, element attributes, CSS, and timeline JavaScript previously allowed plain input values to escape their output context. This change encodes attributes, contains raw-text closing tags, serializes JS strings, and validates generated numeric/CSS values. It targets CodeQL alerts #437–441 and the resolution breakout found during review (#904).
Rich text uses the existing inline-formatting sanitizer: supported nested formatting and typography survive, unsafe opaque content is removed, unsupported structural tags retain their words, and empty captions stay empty. Executable source URLs are rejected. Authored CSS and explicit
__raw:JavaScript remain supported. The clip parser continues to flatten inner formatting to text.The parser now parses once with its runtime DOMParser and assigns stable IDs in that same document. Shared assignment keeps native and persisted IDs consistent across mixed-case HTML/SVG, pinned IDs, and composition templates, avoiding metadata double-decoding.
Validation: 46 generator tests, 62 rich-text sanitizer tests, and 940 parser tests pass; core/runtime typechecks, lint/format and signed commit hooks pass. Twelve expanded regression cases fail against the prior generator. Magi independently verified earlier parser changes against 843 checked-in HTML files with no semantic differences. The expanded head requires fresh independent review and CodeQL verification before merge.
Alert #905 was dismissed separately as a false positive after Magi's exact source-to-sink review (5150508416); no broad suppressions or workflow changes.