fix(frontend): restore sanitization in custom streamdown rehype chains - #4987
Conversation
Streamdown 2.5 replaces its entire default rehype chain [rehype-raw, rehype-sanitize, rehype-harden] with whatever array the caller passes via the rehypePlugins prop. Every custom chain in this repo therefore rendered LLM/stored markdown without any sanitization: - Artifact markdown previews (markdown-preview-plugins.ts + artifact-file-detail.tsx) parse raw HTML via rehypeRaw, so a generated .md artifact could inject <style>/<iframe>/on* handlers into the workspace DOM (stored XSS; only javascript: anchors were blocked by the ArtifactLink component). - The memory settings summary (memory-settings-page.tsx) spread the shared preset without component overrides, so a hostile <a href="javascript:..."> in stored memory content rendered as a clickable anchor. Fix strategy: - Add rehype-sanitize (already resolved in the lockfile via streamdown) as a direct dependency and re-insert a [rehypeSanitize, schema] step in the shared preset (core/streamdown/plugins.ts). It runs after rehypeRaw (raw HTML must be parsed into hast before it can be cleaned) and before rehypeKatex/rehypeSlug (their output is trusted and would otherwise be filtered or clobbered) - the same raw -> sanitize -> math ordering streamdown itself uses. - The schema extends rehype-sanitize's GitHub-style defaultSchema (the base of streamdown's own sanitize schema) so legitimate authored artifact HTML (tables, details, images, alignment/size attributes) keeps working while script/iframe/style, on* handlers and non-allow-listed URL schemes (javascript:, data:, ...) are dropped. The only extensions are tel: hrefs and the math-inline/math-display class markers remark-math emits and rehype-katex detects. - Position rehypeSlug after the sanitize step in the artifact chain so sanitize's id clobbering (id="x" -> id="user-content-x") cannot break the heading anchors it creates. - Pass a: createMarkdownLinkComponent() on the memory settings page as defense in depth, matching the chat rendering path. Unit tests feed a hostile payload (<a href="javascript:...">, <img onerror>, <script>, <iframe>, <style>, ontoggle) through both render paths and assert no executable/clickable equivalent survives, plus regression guards for heading anchors, legitimate HTML and KaTeX math rendering.
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 3939326. The raw → sanitize → KaTeX ordering and hostile-payload coverage look good, and the focused rendering suite (6/6), ESLint, TypeScript, and diff checks pass. I found two issues:
[P2] Preserve DOM-clobber protection for generated heading IDs
frontend/src/components/workspace/artifacts/markdown-preview-plugins.ts intentionally inserts rehypeSlug after rehypeSanitizeStep. That means an untrusted heading such as ## current creates an unprefixed id="current" only after rehype-sanitize has had its chance to apply the default user-content- clobber prefix. Generated heading IDs are the exact DOM-clobbering case that this sanitizer behavior protects. Please retain a safe prefix for slug-generated IDs and translate fragment links to it (or use an equivalent scoped-anchor mechanism); the current regression test should not pin an unprefixed ID.
[P2] Restore the lockfile libc platform selectors
Adding rehype-sanitize as a direct dependency only requires the importer entry because 6.0.0 is already resolved transitively. This patch also removes libc: [glibc|musl] from 64 unrelated native package records, including Sharp, Next SWC, Rollup, and Lightning CSS. Those selectors are part of pnpm platform filtering, so this introduces unrelated cross-platform installation behavior. Please regenerate with the repository-pinned package manager or otherwise restore those fields so the lockfile diff is limited to the direct dependency.
…inimal lockfile Review follow-ups on the sanitization change: - Heading anchors: rehypeScopedSlug replaces rehype-slug in the artifact chain. It runs after the sanitize step (so raw-HTML headings are also anchored) but keeps rehype-sanitize's user-content- id prefix — an untrusted heading like "## current" cannot mint an unprefixed id="current" (the DOM-clobbering shape the sanitizer guards against). In-page fragment links are translated to the prefixed anchors so they still resolve; external URLs, bare "#", already-prefixed fragments and sanitize-prefixed raw-HTML ids are left untouched. - Lockfile: regenerated as a minimal diff — only the two direct-dependency importer entries (rehype-sanitize, github-slugger for the scoped slug) are added; the libc platform selectors on the 64 native package records are preserved byte-for-byte instead of being dropped by lockfile normalization. Full frontend suite: 1034 tests passing; tsc and prettier clean.
|
Thanks for the review — both points addressed in 7130e00: P2 (DOM-clobber): the artifact chain now uses a P2 (lockfile): regenerated as a minimal diff — base lockfile restored, with only the two direct-dependency importer entries added (rehype-sanitize, plus github-slugger used by the scoped slug; both already resolved transitively). The Full frontend suite: 1034 passing; |
…view The scoped slug plugin gives generated heading ids rehype-sanitize's user-content- prefix and translates fragment links to match, so the anchor-scroll test must locate the prefixed id.
|
Follow-up fixes on the last two CI rounds: the import-order lint error is corrected (github-slugger now precedes the hast type import), and the artifact anchor-scroll e2e test now expects the |
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed the latest revision (51a64be). The focused security tests, TypeScript, and Prettier pass, but targeted regression probes found the two inline issues below. The current artifact-anchor E2E failure confirms the first issue.
| * HTML `<h2 id="x">`) keep that id. | ||
| */ | ||
| export function rehypeScopedSlug() { | ||
| const slugger = new GithubSlugger(); |
There was a problem hiding this comment.
[P1] Reset the slugger for every transformed tree
Streamdown caches the unified processor by plugin name, so this GithubSlugger instance survives across parses. Rendering the same ## Stable heading twice produces user-content-stable-heading and then user-content-stable-heading-1; subsequent renders keep incrementing. This is why the updated artifact-anchor E2E currently cannot find h2#user-content-概述. Please call slugger.reset() at the start of the returned tree transformer (as rehype-slug does) and add a regression test that renders identical artifact Markdown twice.
| // order matters). | ||
| rehypePlugins: [ | ||
| rehypeRaw, | ||
| rehypeSanitizeStep, |
There was a problem hiding this comment.
[P2] Keep generated footnote hrefs aligned with sanitized IDs
Remark-rehype already emits footnote pairs such as href="#user-content-fn-1" and id="user-content-fn-1". This sanitize step applies the default clobber prefix to the ID again, yielding id="user-content-user-content-fn-1", but leaves the href unchanged; backreferences break the same way. Because this is in the shared preset, [^1] navigation is broken in chat, memory summaries, and artifacts. Please add target-aware fragment remapping after sanitization (or otherwise make the clobber strategy compatible with generated footnotes) and cover the forward and backreference links in a regression test.
…rs single-prefixed Review follow-ups: - The scoped slug attacher holds one GithubSlugger, but streamdown caches the unified processor by plugin name, so the instance survived across parses and repeated renders of the same heading grew -1/-2 suffixes (the artifact-anchor e2e could not find the id on re-render). The transformer now resets the slugger per tree, as rehype-slug does; a regression test renders identical artifact markdown twice. - remark-rehype emits GFM footnote anchors already clobber-prefixed (user-content-fn-1); the sanitize step prefixed those ids again while their hrefs stayed single-prefixed, breaking footnote navigation in every chain built on the shared preset. A new rehypeClobberFragments step runs right after sanitize: double-prefixed ids are normalized back to one prefix, and unprefixed fragment hrefs are translated to the prefixed form (already-prefixed and external links untouched). The artifact slug now inserts after this step; covered by a footnote regression test on the shared render path. Unit suite 1036 passing; artifact-preview e2e verified locally (9/9, including the heading-anchor scroll test).
|
Both issues fixed in cc38bd4 — and thank you for the processor-caching insight, it explained the e2e failure exactly: P1: the scoped slug transformer now calls P2: added a Unit suite 1036 passing; eslint/tsc/prettier clean. |
Keep the redesigned memory facts list from this PR. Main's Streamdown sanitization on memory summaries does not apply because this branch no longer renders summaries via SafeStreamdown. Auto-merged chat-page (ThreadSubagentBatches), i18n, and mock-api branch coverage from bytedance#4983/bytedance#4998/bytedance#4987.
Streamdown 2.5 replaces its entire default rehype chain
[rehype-raw, rehype-sanitize, rehype-harden] with whatever array the
caller passes via the rehypePlugins prop. Every custom chain in this
repo therefore rendered LLM/stored markdown without any sanitization:
artifact-file-detail.tsx) parse raw HTML via rehypeRaw, so a
generated .md artifact could inject <style>/<iframe>/on* handlers
into the workspace DOM (stored XSS; only javascript: anchors were
blocked by the ArtifactLink component).
shared preset without component overrides, so a hostile
in stored memory content rendered as a
clickable anchor.
Fix strategy:
as a direct dependency and re-insert a [rehypeSanitize, schema] step
in the shared preset (core/streamdown/plugins.ts). It runs after
rehypeRaw (raw HTML must be parsed into hast before it can be
cleaned) and before rehypeKatex/rehypeSlug (their output is trusted
and would otherwise be filtered or clobbered) - the same
raw -> sanitize -> math ordering streamdown itself uses.
base of streamdown's own sanitize schema) so legitimate authored
artifact HTML (tables, details, images, alignment/size attributes)
keeps working while script/iframe/style, on* handlers and
non-allow-listed URL schemes (javascript:, data:, ...) are dropped.
The only extensions are tel: hrefs and the math-inline/math-display
class markers remark-math emits and rehype-katex detects.
sanitize's id clobbering (id="x" -> id="user-content-x") cannot break
the heading anchors it creates.
defense in depth, matching the chat rendering path.
Unit tests feed a hostile payload (,
, <script>, <iframe>, <style>, ontoggle) through both
render paths and assert no executable/clickable equivalent survives,
plus regression guards for heading anchors, legitimate HTML and KaTeX
math rendering.