feat(core): sanitize rich text on the way into a composition

[miguel-heygen]

What

A rich-text patch operation, guarded by one sanitizer, with stable element ids. No UI.

Why

Studio's patch vocabulary was inline-style, attribute, html-attribute and text-content. text-content assigns textContent, and the text-field model escapes markup on the way out and refuses a change in child structure, so a styled span had no route into a composition file.

How

One sanitizer in packages/core, applied at the server write boundary because that is where the composition file is changed and a client is not a trust boundary. The browser-side preview/editor integration lands later in this stack; this PR does not claim a browser caller.

Tags and style properties use a deliberately narrow allowlist, and an unexpected tag loses its formatting rather than its words. Spans introduced by an edit get their ids in the same write, so no follow-up write can race it.

The sanitizer walks the parsed tree iteratively in post-order so deeply nested input cannot exhaust the call stack. New ids are seeded through the same composition-template traversal used by the canonical id pass, so existing ids inside nested templates remain collision-safe.

The sanitizer stays as a small DOM-agnostic core policy instead of adding DOMPurify or sanitize-html: either library would still need the same custom style-declaration allowlist, while adding a runtime dependency and either a browser-window or server-only assumption to code intended to behave the same across DOM implementations.

Untrusted markup is parsed into linkedom's inert document before the sanitizer walks it. The API documentation explicitly forbids assigning untrusted HTML to a live DOM and sanitizing afterward.

This protects rich-text edits on the write path. It does not retroactively sanitize a composition that was hand-authored, imported, or already on disk; those entry paths must apply their own trust-boundary policy.

Test plan

• 62 sanitizer tests across jsdom and linkedom, including canonical active-content payloads, fixed-point behavior, and a deeply nested non-recursive traversal
• 68 focused source-mutation tests, including collision avoidance for ids inside composition templates
• Sanitizing is idempotent; disallowed tags are stripped while their text survives

First of the feature PRs: the persistence contract the editor is built on.

[vanceingalls]

Findings

Thoughtfully-designed sanitizer for a deliberately narrow contract (inline formatting only), 45 cross-parser tests + 84 source-mutation tests, operates on a parsed inert DOM — which removes most classic hand-rolled-sanitizer footguns. I don't have a PoC bypass against the current allowlist, so not blocking. But some things worth addressing before the browser half lands.

1. Custom sanitizer instead of a well-known library. packages/core/src/utils/richTextSanitize.ts is hand-rolled where DOMPurify or sanitize-html are the standard picks for this shape of problem. The design is defensible — narrow allowlist, unwrap-unknown, works on a parsed tree — and the style-property allowlist is unusual enough that a library would still leave custom code. History says custom sanitizers grow bypasses. Please note in the PR body why DOMPurify was ruled out (bundle weight? style-property surgery? both?), so a future reviewer for a follow-up doesn't repeat the debate.

2. [Discrepancy] — client-side sanitize call site is missing. PR body says sanitize runs "on both ends of the trip: in the browser so the preview shows what will be saved, and on the server". The only caller in this PR is packages/studio-server/src/helpers/sourceMutation.ts:257. packages/studio/src/utils/sourcePatcher.ts:94 extends PatchOperation to include "rich-text", but the switches in applyPatch (:527-533) and applyPatchByTarget (:549-555) have cases only for inline-style / attribute / html-attribute / text-content — no rich-text arm. So rich-text ops fall through and no-op on the client. Either wire the browser call here, or explicitly say in the body: "server-only in 2/5; browser side lands in N/5". Otherwise the coverage claim doesn't match the code.

3. opTarget.innerHTML = op.value runs BEFORE sanitize at sourceMutation.ts:255-259. Server-side this is safe because linkedom is inert. This is exactly the mXSS pattern in a live browser DOM — image onerror, script src, iframe load handlers all fire during innerHTML assignment, before sanitizeRichTextChildren gets a turn. If the browser-side wire-up in the next stack PR copies this shape, it's a bypass on a plate. Please add a JSDoc warning on sanitizeRichTextChildren: "callers must parse into an inert document (linkedom / DOMParser with <template>), never assign to a live DOM's innerHTML."

4. UNSAFE_VALUE regex doesn't handle CSS character escapes. richTextSanitize.ts:98 blocks url(|expression(|javascript:|vbscript:|@import|</. CSS lets you write \75rl(x) for url(x) or \6a\61vascript: for javascript:. Current allowlist properties (color/typography) don't consume URLs so today this is unreachable. If the property allowlist ever grows to background-image, list-style-image, border-image, mask-image, cursor, content, the escape bypass becomes real. Either normalize CSS escapes before matching, or add an inline comment freezing the allowlist to non-URL-consuming properties.

5. Style-declaration splitter is paren-aware but not quote-aware. splitDeclarations at :173. A font-family value with a ; inside quotes — font-family: "Roboto Mono; a"; color: red — splits at the wrong ;. Not a security bug (each fragment still faces the allowlist + UNSAFE_VALUE), but it silently loses legitimate font-family values with embedded semicolons. Cheap fix: track '/" state the way sourcePatcher.ts:38-62 already does.

6. Missing OWASP test payloads. 45 sanitizer tests cover the intended surface well. Please add a few canonical payloads so future refactors can't silently regress:

• <img src=x onerror=alert(1)> — assert unwrap happens before any onerror surface
• <a href="javascript:alert(1)">click</a> — <a> currently unwrapped, assert positively
• <svg><script>alert(1)</script></svg> — SVG opaque, assert whole subtree removed
• <span style="color: expression(alert(1))"> — IE legacy, cheap to keep
• <span style="color: &#106;avascript:alert(1)"> — HTML-entity in attribute
• <span style="color: JAVASCRIPT:alert(1)"> — case fold; already handled by /i but pin it

7. SAFE_ATTR_VALUE = /^[A-Za-z0-9_:-]+$/ (:82) allows : inside data-hf-id / data-hf-text-key. Not a security issue — these aren't URLs. Tests show child:1 text-key format. Consider narrowing the class or adding a code comment stating why : is deliberate (selector format) so a future tightener doesn't break the design panel.

8. stampNewChildIds scans the whole document body per patch operation. sourceMutation.ts:180-192 — parent.ownerDocument?.body ?? parent → .querySelectorAll("[data-hf-id]") walks every existing hf-id in the file, then a second full-subtree walk. O(document × patches). Not blocking; worth measuring in a follow-up perf run.

9. No load-time re-sanitize / backward-compat pass. Compositions on disk from before this PR aren't touched. Legitimate for HeyGen's local-author threat model, but be explicit in the body: this defends the write path from a rich-text edit; it does not retroactively clean a composition that someone hand-authored or pasted with hostile markup. When compositions start being imported from the published catalog (later in this stack?), the import path needs its own sanitize hook.

10. CI status at head 3ca092e. Aggregate FAILURE on Test ("Producer unit/integration tests did not succeed") and regression ("One or more regression shards failed") — both are roll-up jobs so the specific shards need drilling. Six checks still IN_PROGRESS. The sanitizer file itself is what Test covers, so a red there is load-bearing. Please investigate before I re-verify.

Verdict

COMMENT. Sanitizer design is defensible, no bypass demonstrated, tests are cross-parser. But: coverage claim in the body doesn't match the code (client-side sanitize is missing here), sanitize-after-innerHTML pattern will bite if copy-pasted to the browser, and CI is red on the two lanes that matter. Please address #1 (justification), #2 (coverage-claim reconciliation), #3 (JSDoc warning), #6 (OWASP payload tests), and the CI failures. Happy to re-review at the new head.

— Via

[james-russo-rames-d-jusso]

Reviewed at 3ca092e3.

Read this through the adversarial-bypass / client-server-parity / idempotence-and-id-race dimensions, plus the D1 receipt-parity cross-check. No XSS bypass found across 20 traced payloads (<script>, <img onerror>, <svg onload>, <iframe javascript:>, <a javascript:>, template/noscript/math/svg opaque wrappers, <b><script>..., CSS url(/expression(/@import, entity-encoded schemes, DOM-clobbering names, and byte-identical no-ops); every one traced to a code path that neutralizes it. The design — parse first into an inert tree, walk with a small formatting allowlist, unwrap unknown while keeping their text — is defensible for the deliberately narrow contract (inline formatting).

Via's earlier COMMENT covers most of what I'd raise inline — I strongly concur on:

• #1 Custom sanitizer vs library — real defense-in-depth ask. A one-line rationale in the PR body (bundle weight? style-property surgery? both?) closes the "why not DOMPurify" audit for future reviewers.
• #2 Client-side sanitize call site is missing. I confirmed this independently: sourcePatcher.ts:94 adds "rich-text" to the type union, but applyPatch (:527-533) and applyPatchByTarget (:549-555) have cases only for inline-style / attribute / html-attribute / text-content — no case "rich-text". A workspace-wide grep finds ZERO consumers of sanitizeRichTextChildren under packages/studio/. As Via noted, either wire it or narrow the browser type union so this doesn't fall through as a silent no-op. Docstring at richTextSanitize.ts:6-9 and the PR body both claim "both ends"; the code has one end.
• #3 innerHTML = op.value runs BEFORE sanitize at sourceMutation.ts:255-256. Safe on the server because linkedom is inert. This is a great call-out — if D3+ copies the ordering to the browser end, live-DOM innerHTML = fires onerror / onload / <script> synchronously before the sanitizer walks, and the design becomes an mXSS bypass on a plate. Load-bearing JSDoc on sanitizeRichTextChildren warning "callers must parse into an inert document" is the right guard. See my 🔮 note below — I'll cross-check on D3+.
• #4 CSS char escapes (\75rl(), #5 quote-unaware ; splitter, #6 OWASP payload tests, #7 : in SAFE_ATTR_VALUE, #8 O(document × patches) perf, #9 no backward-compat retro-sweep — all clean.

Findings Via didn't call out

Two are inline anchors; the rest are body-level nits.

1. Inline: stampNewChildIds template-descent divergence. sourceMutation.ts:181 uses plain querySelectorAll("[data-hf-id]") for its id-seed set — but linkedom (the server-side parser this file targets) doesn't descend <template> content on querySelectorAll. The SAME file already has querySelectorAllWithTemplates(root, selector) at :58-74 — defined specifically to close that gap — and uses it at :86 and :118. stampNewChildIds's seed walk diverges from the pattern its own file established, so a data-hf-id living inside a composition <template> is invisible to the collision-avoidance set, and a freshly-minted rich-text id could hash-collide with it. This is [[feedback_sibling_primitive_pattern_divergence_check]] — the mechanism is already in the file, stampNewChildIds just doesn't use it.

2. Inline: recursion depth is unbounded. sanitizeRichTextChildren at richTextSanitize.ts:98-128 calls itself on every element (:119), no depth cap. A pathological input like "<b>".repeat(15000) + "x" + "</b>".repeat(15000) overflows Node's call stack (~10-15k frames). Server-side DoS not XSS — a mutation returns 500 rather than persisting, but a wired-up rich-text write UI (D3+) that lets a user paste a deeply nested tree from another editor makes the crash trivially reachable. Cheap fix: convert recursion to an explicit stack, or add an early depth-limit check.

Body-level nits Via didn't call out

• No idempotence property test. Traced idempotent (attribute strip re-normalizes ${property}: ${value}; unknown-tag unwrap converges; opaque-drop is total; stampNewChildIds skips already-stamped elements). The closest test is sourceMutation.richText.test.ts:141-149 ("does not accumulate markup when the same value is written twice"), but that's patch(patch(X)) === patch(X) at the wrapper layer — not sanitize(sanitize(X)) === sanitize(X) on the sanitizer's own output. A one-liner assertion added to richTextSanitize.test.ts (over the existing 45 test inputs) pins the sanitizer's fixed-point directly.

• No shape validation on op.value at the HTTP boundary. files.ts:2716-2727 calls parseMutationBody + findUnsafeDomPatchValues but nothing enforces op.value is a string for rich-text or that op.type is one of the five union members. A client sending {type: "rich-text", value: {}} sets innerHTML = "[object Object]" — sanitizer catches it as inert text so not exploitable — but a zod schema next to isElementPatchRequest (:250-256) closes it cheaply and future-proofs against a sixth op type that treats non-string values differently.

Positive verifications worth naming

• D1 receipt-parity holds. Every server write introduced by this PR routes through writeFileWithReceipt (single-file at files.ts:2712 → :2749) or commitElementPatchBatchesWithReceipts (batch at :2766 → :403-418), and both call recordFileWriteReceipt (fileVersion.ts:26) before their writeFileSync. My HF#3206 review's implicit ask ("does every new writer post a receipt?") lands green.
• Server-side order is correct: innerHTML = op.value (:255) → sanitizeRichTextChildren(opTarget) (:256) → stampNewChildIds(opTarget) (:257). Sanitize before stamp means ids never land on elements that would have been unwrapped.
• Per-op sanitizer coverage is complete: only rich-text treats its value as HTML; text-content uses .textContent = (never parsed as markup), inline-style writes through patchStyleAttrString, attribute/html-attribute are attribute-value paths with their own allowlists. Nothing bypasses to writeFileSync.
• Cross-parser tests — richTextSanitize.test.ts:8-26 parametrizes over jsdom AND linkedom. Correct shape for a browser+server surface, even if only the server actually calls the sanitizer today.

🔮 Forward-looking (D3+ lens)

FL1 — Client-side innerHTML = value; sanitize() ordering is an mXSS trap. When the browser-side case "rich-text": wiring lands (D3? D4?), copying the server's ordering to a live DOM will fire image onerror / iframe onload / etc. synchronously during the innerHTML write, before sanitizeRichTextChildren gets a turn. The right shape is to parse into an inert host — document.createElement("template").innerHTML = value (template's content is a DocumentFragment that never activates until moved into a live tree), or new DOMParser().parseFromString(value, "text/html") (the resulting document is inert) — then sanitize the inert tree, then move nodes into the live DOM. Via's #3 JSDoc ask on the sanitizer is exactly the right guard-rail; the review time to catch this is the PR that adds the browser call. Check on D3+: does the browser-side patcher parse into <template> or DOMParser (inert), or does it write directly to a live element's innerHTML?

FL2 — Recursion-depth DoS becomes user-reachable in the browser. The server crash surface I flagged above (unbounded recursion on pathological nesting) is currently gated by the write-endpoint's own error handling (500 to the client, no partial write). When rich-text ships client-side, a paste from an external editor with deeply-nested formatting can crash the browser tab BEFORE the network write. Not security, just terrible UX. Check on D3+: is there a depth cap either at the sanitizer or at the paste-handling / normalization layer?

FL3 — Idempotence assumes deterministic id generation. mintHfId at sourceMutation.ts uses content-hashed FNV1a — deterministic — and stampNewChildIds skips already-stamped elements, so idempotence holds. If a future PR switches id generation to anything randomized (uuid, counter-with-time-seed), sanitize(sanitize(x)) === sanitize(x) breaks by construction because the second pass would stamp new random ids on unstamped elements. Check on D3+: id generator stays deterministic.

CI note

At current head (3ca092e3), Test (which covers the sanitizer package) is PASS — improved since Via's read. regression roll-up shows FAIL from a prior run at 31460615049, but the current run at 31460693785 has 8/9 shards PASS with shard-2 still IN_PROGRESS. Windows Tests still pending. Worth double-checking shard-2 lands green before Via's re-verify pass.

What I didn't verify

• Did not read the 45+84 tests in full; sampled the ones covering the top adversarial categories.
• Did not audit whether any of the pre-existing raw file-write routes (files.ts:2364-2377 — POST /projects/:id/files/*) should ALSO route through the sanitizer when writing a composition source. Pre-existing, not this PR's fix to make — flagging for Miguel's awareness.
• Did not run either sanitizer live against fuzz input. Trusting the 45 cross-parser cases.

Otherwise LGTM on the design and the mechanism. The two inline findings + Via's #2/#3 are the ones I'd want in before merge; the rest of Via's set are cleanups and mine are nits + forward-looking notes.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 verified at 0d017e51.

Delta from 3ca092e3 → 0d017e51 is 2 files (richTextSanitize.ts +35/-14, richTextSanitize.test.ts +32/-3) plus the PR body/docs rewrite. Everything in Miguel's changelog verifies clean:

• Scope narrowed to server-write-boundary only. Sanitizer file header at richTextSanitize.ts:1-14 now says "The server write boundary applies it unconditionally before returning composition bytes." PR body reinforces: "The browser-side preview/editor integration lands later in this stack; this PR does not claim a browser caller." This directly retires Via's #2 (client-side call site missing) — Miguel resolved by retracting the claim rather than wiring it, which is the cleaner move.
• Inert-DOM requirement documented at the sanitizer API. New JSDoc at sanitizeRichTextChildren (:97-104) warns "Never assign untrusted HTML to a live DOM element and then call this function: active content can run before sanitization begins." That's exactly Via's #3 ask and my 🔮 FL1 in one paragraph. Load-bearing guardrail for D3+.
• Canonical adversarial payloads added, cross-parser. richTextSanitize.test.ts:152-172 — 6 it.each rows (image onerror, javascript: href, SVG <script>, CSS expression(), entity-encoded javascript:, case-folded JAVASCRIPT:), each run against both jsdom + linkedom. Covers the Via #6 categories and my R1 payload trace.
• Quote-aware declaration splitter with regression test. splitDeclarations at :198-213 now tracks a quote: "'" | '"' | null state alongside paren depth; new test at :185-189 asserts font-family: "Roboto Mono; a" survives with color: red intact. Retires Via's #5.
• CSS-escape/property-allowlist constraint pinned in code. New comment at :48-51 — "Keep this list limited to properties whose grammar cannot fetch a resource. Adding a URL-consuming property also requires decoding CSS escapes before UNSAFE_VALUE can be a sufficient guard." That's Via's #4 turned into a load-bearing invariant guarding future edits to FORMATTING_STYLE_PROPS.
• : in SAFE_ATTR_VALUE documented as deliberate. JSDoc at :61-66 — "text keys use selector-like tokens such as child:1; neither allowed attribute is interpreted as a URL." Retires Via's #7.
• DOMPurify rationale + retro-sweep caveat in PR body. "either library would still need the same custom style-declaration allowlist, while adding a runtime dependency and either a browser-window or server-only assumption" — Via's #1. Plus the explicit "does not retroactively sanitize a composition that was hand-authored, imported, or already on disk; those entry paths must apply their own trust-boundary policy" retires Via's #9.

R1 items that survive R2 (unaddressed, non-blocking for the narrowed scope):

• My R1 inline at sourceMutation.ts:181 (stampNewChildIds template-descent divergence) — still uses plain querySelectorAll("[data-hf-id]") at the seed walk while the same file's querySelectorAllWithTemplates (:58-74) is used by two other consumers. The scope narrowing doesn't retire this — the risk is entirely server-side and this PR is the server-side write path. Non-blocking (needs an actual composition with a <template>[data-hf-id] inside to bite) but a real deferred correctness debt. Fine to close in a follow-up.
• My R1 inline at richTextSanitize.ts:129 (unbounded recursion) — still no depth cap. Server-side today: exception propagates to a 500 response, no partial write, bounded. Becomes user-reachable if D3+ ever adds a browser paste-handling call site that lets external-editor HTML in (my 🔮 FL2). Not blocking a server-only-boundary merge.

Both are fine as follow-ups; noting them so the "R1 addressed" claim stays honest.

Positive verifications (unchanged from R1):

• No XSS bypass across 20 traced payloads.
• Server-side order is correct: innerHTML = op.value → sanitizeRichTextChildren → stampNewChildIds.
• D1 receipt-parity holds — every new writer routes through writeFileWithReceipt / commitElementPatchBatchesWithReceipts.

CI: new head 0d017e51 just pushed, most checks pending (Preflight + Detect changes green). Recommend waiting for Tests + Tests on windows-latest + regression roll-up before merge.

LGTM on my end. Stamp routes per standing rule — tagging Rames Jusso (<@U0ARJFN5S6Q>) for the actual approval click.

— Review by Rames D Jusso

[vanceingalls]

Reviewed delta at head 0d017e51e.

Ask 1 (Coverage claim mismatch) — ADDRESSED

You took option B — updated the body rather than wiring the client. Body now reads: "One sanitizer in packages/core, applied at the server write boundary because that is where the composition file is changed and a client is not a trust boundary. The browser-side preview/editor integration lands later in this stack; this PR does not claim a browser caller." Coverage claim now matches shipped surface. sourcePatcher.ts:94 still carries "rich-text" as a shared-vocab discriminant with no case arm — harmless (falls to default: return html), and consistent with the disclaimed scope.

Ask 2 (mXSS JSDoc) — ADDRESSED

packages/core/src/utils/richTextSanitize.ts:89-93 — JSDoc above sanitizeRichTextChildren: "When the children came from untrusted markup, callers must parse that markup into an inert document (for example linkedom or a detached DOMParser document) first. Never assign untrusted HTML to a live DOM element and then call this function: active content can run before sanitization begins."

Ask 3 (DOMPurify justification) — ADDRESSED

Body: "The sanitizer stays as a small DOM-agnostic core policy instead of adding DOMPurify or sanitize-html: either library would still need the same custom style-declaration allowlist, while adding a runtime dependency and either a browser-window or server-only assumption to code intended to behave the same across DOM implementations."

Ask 4 (CSS-escape allowlist freeze) — ADDRESSED

richTextSanitize.ts:44-46 inline on FORMATTING_STYLE_PROPS: "Keep this list limited to properties whose grammar cannot fetch a resource. Adding a URL-consuming property also requires decoding CSS escapes before UNSAFE_VALUE can be a sufficient guard."

Ask 5 (splitDeclarations quote-awareness) — ADDRESSED

richTextSanitize.ts:194-210 — splitDeclarations tracks quote: "'" | '"' | null via isQuoteDelimiter, and isDeclarationSeparator only fires when quote === null && depth === 0. Locked by test "keeps a quoted semicolon inside a style value" (richTextSanitize.test.ts:180-184, font-family: "Roboto Mono; a"; color: red).

Ask 6 (OWASP payload tests) — ADDRESSED

richTextSanitize.test.ts:135-159 — parameterized it.each block runs across BOTH parsers via the outer describe.each(PARSERS) (jsdom + linkedom):

• <img src=x onerror=alert(1)>safe
• <a href="javascript:alert(1)">safe</a>
• <svg><script>alert(1)</script></svg>safe
• <span style="color: expression(alert(1))">safe</span>
• <span style="color: &#106;avascript:alert(1)">safe</span>
• <span style="color: JAVASCRIPT:alert(1)">safe</span>

Ask 7 (SAFE_ATTR_VALUE : comment) — ADDRESSED

richTextSanitize.ts:63-65 JSDoc on SAFE_ATTR_VALUE: "a bare token, nothing else. : is deliberate because text keys use selector-like tokens such as child:1; neither allowed attribute is interpreted as a URL."

CI at head

Head SHA confirmed 0d017e51efdfa92816108fe7d1112fb419df2364. Fresh push re-queued the pipeline. Snapshot: 16 SUCCESS, 8 SKIPPED, 1 NEUTRAL, 0 FAILURE, 29 IN_PROGRESS. Previously-red Test and regression re-running clean through Preflight so far. No red state observed.

Cross-reviewer note

Rames reviewed the pre-fix head (3ca092e3) at 05:24Z with an independent adversarial pass across 20 traced payloads (including opaque wrappers template/noscript/math/svg, <iframe javascript:>, DOM-clobbering names, byte-identical no-ops) and found no XSS bypass. Two independent reviewers converged on "design is defensible, no PoC bypass" on the sanitizer itself.

New findings

None. Delta is text-only (body + JSDoc + inline comments) plus behavioral hardening (quote-aware splitter + OWASP payload coverage). No stale docstrings, no test semantic drift, no scope creep.

Verdict

APPROVE at 0d017e51e. All 7 R1 asks addressed with concrete evidence. OWASP payload matrix runs across both DOM implementations. Quote-aware splitter locked by targeted regression. CSS-escape unreachable-today is now documented as a load-bearing constraint on the property allowlist. Two independent reviewers found no XSS bypass. Ship on Test / regression shards landing green.

— Via

[vanceingalls]

Reviewed delta 0d017e51e..eed9540dc — one commit, fix(core): harden rich text sanitizer traversal, four files touched.

Rames anchor 1 (canonical walkCompositionDescendants for stamp) — ADDRESSED

packages/studio-server/src/helpers/sourceMutation.ts:180-192 — stampNewChildIds now uses walkCompositionDescendants(root, (el) => ...) from @hyperframes/parsers/hf-ids instead of root.querySelectorAll("[data-hf-id]"). The canonical walk descends INTO nested composition <template> contents (which raw querySelectorAll skips, since template content sits on .content as a DocumentFragment, not queryable from the parent). Import added at sourceMutation.ts:3-11.

Regression pinned at sourceMutation.test.ts:572-587: source has <template data-composition-id="nested"><p data-hf-id="hf-3x72"></template> + a plain <h1 id="title"> being patched. Asserts (a) the nested hf-3x72 is not duplicated in output (toHaveLength(1)), and (b) the newly-minted id on the rich-text span is not equal to hf-3x72. Closes the deterministic-collision hazard.

Rames anchor 2 (iterative post-order traversal) — ADDRESSED

packages/core/src/utils/richTextSanitize.ts:107-152 — the previous recursive sanitizeRichTextChildren(element) call inside the child loop is replaced with an explicit SanitizerFrame[] stack, discriminated by phase: "visit" | "sanitize". Comment names the motivation: "Post-order without recursion: adversarially deep pasted markup must not exhaust either the server or browser call stack." Descendants are pushed in .reverse() so pop-order matches document order.

Regression pinned at richTextSanitize.test.ts:215-220: builds a 15,000-deep <b><b>…</b></b> nesting via parseWithLinkedom and asserts sanitizeRichTextChildren does not throw. This would fail against the prior recursive implementation via Maximum call stack size exceeded.

Bonus (explicit fixed-point coverage) — ADDRESSED

richTextSanitize.test.ts:204-210 — new "is a fixed point" test, runs across both parsers via describe.each(PARSERS). Sanitizes once, records innerHTML, sanitizes again, asserts unchanged. Genuine invariant test — a mutation that made sanitize non-idempotent would fail.

Package plumbing note (non-blocking)

packages/core/package-subpaths.json and packages/core/package.json declare ./rich-text-sanitize as a new exported subpath, so sourceMutation.ts can import ... from "@hyperframes/core/rich-text-sanitize". Clean cross-package import pattern, no surprises.

My R1 asks — no regressions

All 7 R1 asks (server-only body, mXSS JSDoc, DOMPurify justification, CSS-escape doc, quote-aware splitter, OWASP payloads, SAFE_ATTR_VALUE : comment) remain intact at eed9540dc. Delta only added hardening + tests.

CI at head

7 SUCCESS / 8 SKIPPED / 0 FAILURE / 19 IN_PROGRESS. Fresh pipeline re-queued. Previously-red Test + regression will re-run; no red state at scan.

Verdict

APPROVE at eed9540dc. Both Rames anchors addressed with targeted regression tests, plus a genuine fixed-point invariant. Iterative traversal is the right kind of defense-in-depth — an adversarially deep paste no longer trips the call stack on either the server (linkedom) or browser (when the browser wiring lands). Ship on required checks landing green.

— Via

[james-russo-rames-d-jusso]

R3 verified at eed9540d.

Both R1 inlines that survived R2 are now cleanly closed. Fix quality is unusually good — Miguel didn't paper them over, he replaced the mechanism.

R1 inline #1 — stampNewChildIds template-descent divergence. sourceMutation.ts:186 now threads the seed walk through walkCompositionDescendants from @hyperframes/parsers/hf-ids instead of plain querySelectorAll. This is a better fix than my suggested local querySelectorAllWithTemplates: walkCompositionDescendants at packages/parsers/src/hfIds.ts:140 uses isCompositionTemplate to distinguish composition-templates (which get walked) from plain templates (which stay inert), and it's the canonical primitive already shared across the parsers surface. Regression test at sourceMutation.test.ts:576-586 pins the exact deterministic collision the fix prevents — a template carrying data-hf-id="hf-3x72" remains unique after a rich-text patch that would otherwise have minted the same id. Clean.

R1 inline #2 — recursion depth cap. sanitizeRichTextChildren at richTextSanitize.ts:116-153 is now iterative post-order with typed SanitizerFrame variants ({phase:"visit"} | {phase:"sanitize"}). Trace confirms post-order semantics preserved: descendants push onto the stack AFTER the parent's sanitize frame, so they pop and complete before the parent gets its unwrap/stripAttributes call — same shape as the recursive version. 15,000-level nesting regression at richTextSanitize.test.ts:215-220 asserts no stack growth. Comment at :120-121 names the intent ("adversarially deep pasted markup must not exhaust either the server or browser call stack") — pins the invariant for future edits.

Bonus — R1 body nit #1 closed. Fixed-point property test at richTextSanitize.test.ts:205-211 now runs sanitize(sanitize(x)) === sanitize(x) on both jsdom and linkedom, directly asserting the sanitizer's own idempotence (my earlier nit — the only existing near-check was at the patch-wrapper layer).

One follow-up nit (unchanged from R1, still non-blocking): the stamp loop at sourceMutation.ts:191 (parent.querySelectorAll("*")) still uses plain querySelectorAll. Not a real risk for this PR because the sanitizer removes <template> (opaque tag) before the stamp fires — no rich-text-mediated template can survive to need stamping. Would be a future concern only if a new patch type ever stamps unstamped elements without going through the sanitizer. Fine as-is.

Positive verifications:

• All R2-verified items (docstring narrowing, inert-DOM warning, canonical adversarial payloads, quote-aware ; splitter, SAFE_ATTR_VALUE : invariant, FORMATTING_STYLE_PROPS URL-consuming constraint, DOMPurify/backward-compat PR-body caveats) unchanged at R3 head.
• No XSS bypass across the 20-payload adversarial trace + Miguel's 6 new canonical it.each payloads.
• Server-side patch order still correct: innerHTML = op.value → sanitize → stampNewChildIds.
• Iterative traversal preserves the recursive version's ordering guarantees; captured element in a sanitize frame is only touched by its own handler (not by descendant sanitization), so no detached-element hazards.

Fresh peer state: Vance already APPROVED at R3 (eed9540d, 05:49:57Z) — merge unblocked as soon as required CI is green.

LGTM.

— Review by Rames D Jusso

[jrusso1020]

Post-merge note. I was finishing an independent pass at eed9540dc when this merged, so my stamp never landed — recording what I verified anyway, because one item is a correction to a claim that now appears in three places and this is 1 of 5 in the stack.

The traversal rewrite is equivalent — measured, not reasoned

Replacing recursion with an explicit frame stack is the kind of change where an ordering slip is invisible in review and catastrophic in a sanitizer: if children stop being cleaned before an unwrap promotes them, unsanitized nodes move up into the kept tree.

The diff makes this checkable, because every policy surface is byte-identical between the two heads — FORMATTING_TAGS, FORMATTING_STYLE_PROPS, FORMATTING_ATTRS, SAFE_ATTR_VALUE, OPAQUE_TAGS, UNSAFE_VALUE, and all of unwrap / stripAttributes / filterStyle / splitDeclarations. Only the walk changed. So I ran both implementations side by side over a seeded corpus of 3,011 inputs (the canonical payloads plus randomly generated trees mixing formatting, structural, opaque and unknown tags, comments, unbalanced markup, and hostile style/attribute values):

corpus=3011   old-vs-new mismatches=0   active-content-in-output=0   all idempotent

Zero divergence. Post-order is preserved for the reason the code implies: the sanitize frame is pushed before the children, so it pops after all of them, and the children are reversed so they pop in document order. The rewrite is a strict improvement.

The step the payload sweeps didn't reach: serialization

Both prior reviews exercised the sanitizer's DOM output. This boundary writes composition bytes, and those bytes are re-parsed by a real browser at render time — a separate step with its own escaping behaviour.

The input that probes it is a value that is legal for an allowlisted property and still carries a quote. filterStyle accepts it, correctly by its own rules:

<span style='font-family: "x onmouseover=alert(1) y="'>hi</span>

If the serializer emitted that quote raw it would close the style attribute and onmouseover would land as a real attribute. Running the actual path (innerHTML → sanitize → outerHTML → re-parse):

out: <span style="font-family: &quot;x onmouseover=alert(1) y=&quot;">hi</span>
re-parsed event handlers: none

Not a bypass. Worth naming anyway, because what makes it safe lives in the serializer, not in this module: UNSAFE_VALUE deliberately doesn't screen quotes, and nothing here would notice if the serializer changed. The existing note above FORMATTING_STYLE_PROPS already records one such dependency; a companion sentence about attribute-escaping at write time would put the second load-bearing assumption next to the first.

Correction: the 15,000-nesting test doesn't exercise the production path

This is the one thing I'd want fixed, and it's a docs/test-shape issue rather than a defect in the sanitizer.

The new regression builds its tree with parseWithLinkedom, which calls parseHTML() on a whole document string. Production doesn't do that. sourceMutation.ts:260 does:

opTarget.innerHTML = op.value;

Those are different code paths in linkedom, and they have very different depth ceilings:

depth	parseHTML(<document>) (the test)	innerHTML setter (production)
2000	ok	ok
3000	ok	RangeError
15000	ok	RangeError

So op.value deep enough to matter throws RangeError: Maximum call stack size exceeded at line 260, before sanitizeRichTextChildren is ever called, and nothing catches it. The test passes honestly — it proves the traversal no longer grows the stack — but it cannot observe this, because it reaches the sanitizer through a parser path the operation never uses.

The consequence is narrow, and I don't think it blocks: this is the local Studio server, so the realistic effect is that a pathological paste 500s your own dev server rather than anything remote, and the depth ceiling was there before this PR too. But the PR body currently says the iterative walk means "deeply nested input cannot exhaust the call stack," and Via and Rames D Jusso have both since cited the 15,000 test as closing that concern. That claim is true of the sanitizer and not true of the operation, and right now nothing in the suite would catch the difference.

Cheapest honest fix is to narrow the sentence — the walk no longer contributes stack growth, while the parse step still bounds input depth. If you'd rather actually close it, the bound belongs on op.value before line 260, not inside the sanitizer.

Confirmed: the template-descent fix is real in the runtime

Worth recording that this one holds in linkedom specifically and not just in spec-DOM, since that's what the server runs:

body.querySelectorAll("[data-hf-id]") => ["hf-1"]     // "hf-9" lives inside a <template>
descends into <template>: false | template.content exists: true

So the previous seeding really could mint a duplicate, and routing through walkCompositionDescendants is the right fix. Agreed with Rames D Jusso that leaving the assignment loop on plain querySelectorAll is fine — TEMPLATE is in OPAQUE_TAGS, so no rich-text patch can put a template inside the stamped subtree.

Small one on the client-side type widening

packages/studio/src/utils/sourcePatcher.ts gains the union member and nothing else, and has no innerHTML/outerHTML/insertAdjacentHTML anywhere — so the "no browser caller" framing holds, which I checked because a browser package being in the diff is exactly where that kind of claim goes stale.

Both switches there end in default: return html, so a rich-text op sent to the client patcher is silently dropped and looks identical to "applied, nothing changed." An explicit case "rich-text": return html; with a server-side-only comment, or an exhaustiveness guard, would make that legible when the browser half lands later in the stack.

Net

No bypass found on any axis I could construct, including the serialization step the earlier sweeps didn't cover, and the traversal rewrite is provably equivalent over 3,011 inputs. Nothing here would have blocked the merge. The nesting-claim correction is worth a sentence in the body before D2 builds on it.

— Rames Jusso (James's assistant)