refactor(markdown): single canonical sanitize schema for both renderers (MUL-4922)#5578
Open
NevilleQingNY wants to merge 1 commit into
Open
refactor(markdown): single canonical sanitize schema for both renderers (MUL-4922)#5578NevilleQingNY wants to merge 1 commit into
NevilleQingNY wants to merge 1 commit into
Conversation
…rs (MUL-4922) Phase 1 of the RichContent convergence: collapse the duplicated security base shared by the two product-level Markdown chains. Chat (packages/ui/markdown/Markdown.tsx) and Issue/Comment (packages/views/editor/readonly-content.tsx) each carried a verbatim fork of the rehype-sanitize schema and urlTransform, and the forks had already drifted: readonly whitelisted <mark> for `==highlight==`, chat did not. A security-relevant allow-list maintained in two places means every future XSS fix has to land twice, and missing one is a hole — this is the hardest reason for the sweep, ahead of the user-visible feature drift. - Extract markdownSanitizeSchema + markdownUrlTransform into packages/ui/markdown/sanitize.ts and export from the package index. Both chains now import the single copy; no local forks remain. - The canonical schema is the union, so chat gains the <mark> tag name. This is the one intentional behavior delta: <mark> is inert and admits no attributes, and chat needs it anyway once ==highlight== converges. - Annotate the schema as rehype-sanitize's Options: exporting it makes the previously-inferred hast-util-sanitize type unnameable across packages. Adds a cross-surface contract test that runs one set of security fixtures (script, event handlers, javascript: href, data:image vs data:text/html, mark, slash://) through BOTH surfaces and asserts identical outcomes — the mechanism that stops a third fork from growing back. Code-block rendering is deliberately not asserted cross-surface yet: chat highlights with Shiki, readonly with lowlight, so emitted class tokens still differ. Converging them is the RichCodeBlock phase and needs the highlight-engine decision first; only the schema-level allow-list is shared here. Verified: pnpm typecheck (6 workspaces), pnpm lint (0 errors), pnpm vitest run in packages/views (228 files, 2665 tests, all passing). Co-authored-by: multica-agent <github@multica.ai> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 1 of the MUL-4922 RichContent convergence: collapse the duplicated security base shared by the two product-level Markdown chains. Pure convergence — no surface migration, no visual change.
Why this one first
Chat (
packages/ui/markdown/Markdown.tsx:92-145) and Issue/Comment (packages/views/editor/readonly-content.tsx:66-122) each carried a verbatim fork of the rehype-sanitize schema andurlTransform— and the forks had already drifted: readonly whitelisted<mark>for==highlight==, chat did not.A security-relevant allow-list maintained in two places means every future XSS fix has to land twice, and missing one is a hole. That is a harder reason than the user-visible feature drift, and it is why this is step ①.
Changes
markdownSanitizeSchema+markdownUrlTransformintopackages/ui/markdown/sanitize.ts, exported from the package index. Both chains import the single copy; no local forks remain (verified by grep — zeroconst sanitizeSchema/function urlTransform/defaultSchemaoutside the canonical file).Options— exporting it makes the previously-inferredhast-util-sanitizetype unnameable across packages (TS2742).One intentional behavior delta
The canonical schema is the union, so chat gains the
<mark>tag name.<mark>is inert and admits no attributes, and chat needs it anyway once==highlight==converges. Everything else is byte-identical between the old forks — themarktagName was the only difference.Tests
Adds
packages/views/common/rich-content-sanitize-contract.test.tsx— one set of security fixtures run through both surfaces asserting identical outcomes:<script>stripped · event-handler attrs stripped ·javascript:href neutralized ·data:image/*preserved ·data:text/htmlblanked · http(s) src intact ·<mark>allowed ·slash://allowedThis is the anti-drift mechanism: a schema change landing on only one chain now fails CI.
Deliberately out of scope
Code-block rendering is not asserted cross-surface yet — chat highlights with Shiki, readonly with lowlight, so emitted class tokens still differ. Converging them is the RichCodeBlock phase and needs the highlight-engine decision (Shiki vs lowlight) ratified first. Only the schema-level allow-list is shared here.
Phases ② (RichCodeBlock dispatcher + surface-agnostic leaves) and ③ (Chat switch + streaming fence gating + full contract fixtures) follow as separate PRs, per the approved phased migration.
Verification
pnpm typecheck— 6 workspaces, passpnpm lint— 0 errors (19 pre-existing warnings, none on touched files)pnpm vitest runinpackages/views— 228 files, 2665 tests, all passingRefs MUL-4922
🤖 Generated with Claude Code