fix(security): escape model-authored raw HTML islands - #360
Open
gadenbuie wants to merge 4 commits into
Open
Conversation
gadenbuie
marked this pull request as ready for review
August 27, 2026 19:18
Model-authored markdown containing a forged <shiny-tool-result value-type="html"> could spawn a real tool card whose value attribute reached innerHTML — stored, zero-click XSS when combined with custom-display (reachable via prompt injection). Two layers are now closed: - Routing: ROUTABLE_CONTENT_TYPES drops "markdown". Only html-typed blocks (server-authored Tag output; both Python and R label Tag content "html" while model prose is "markdown") are scanned for tool elements. Verified no legitimate flow delivers tool markup in a markdown-typed block, including history/bookmark restore. - Component map: chatTagToComponentMap's tool bridges were a fallback that resolved forged elements even when routing skipped them. It is now split by trust: html-typed chat blocks keep the full map, while markdown/thinking (model-authored) content gets untrustedChatTagToComponentMap, which renders both tool tags as escaped, inert text via a shared EscapedIsland component (extracted from MarkdownStream). Greetings keep the full map (server-authored). Adds regression tests covering the custom-display, expanded, framed, full-screen, and collapsed spoof variants, asserting no tool card, no forged element, and no decoded payload reach the DOM. Test fixtures that unrealistically carried tool markup in markdown-typed blocks were corrected to "html" (or split into ordered prose + tool blocks).
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.
Closes #287.
Summary
This PR prevents model-authored Markdown from creating or escaping into shinychat's privileged raw-HTML islands. It also adds explicit content provenance to
MarkdownStream, so trusted server-rendered HTML and untrusted text can safely coexist in a single streamed value, including mixed values such as:The fix is implemented at three layers:
rehypeRawparses them.innerHTML.MarkdownStreamcarries explicit leaf-level trust over the wire and applies a component-map backstop to every untrusted segment.The JavaScript bundle is rebuilt and copied into both the Python and R packages.
Background and vulnerability
shinychat supports server-authored interactive HTML by wrapping it in a custom element:
The React renderer maps that element to
RawHTML, which serializes the element's children and assigns them throughinnerHTML. This is intentional for trusted Shiny/htmltools output, but it makes the custom element a security boundary: model output must not be able to instantiate one.Previously, assistant Markdown passed through
rehypeRawand then through the same component mapping used for trusted server HTML. A model could therefore emit a raw-island element, or use parser edge cases around its boundaries, and route attacker-controlled markup intoRawHTML.A straightforward regex replacement is not a sufficient boundary here. HTML tag recognition is permissive and stateful: names are ASCII-case-insensitive; attributes may be malformed but still tokenized;
>can occur inside quoted values; apparent tags inside comments, attributes, raw-text elements, and RCDATA are inert; and self-closing syntax has tokenizer-specific behavior. Attempts to encode all of this in increasingly broad regexes are difficult to audit and can introduce catastrophic backtracking.Rendering changes
Untrusted assistant Markdown
markdownProcessornow bracketsrehypeRawwith a disguise/escape plugin pair.Before parsing,
rehypeDisguiseReservedIslandsrewrites emitted raw-island tokens to marked<template>elements.<template>is used because parse5 gives it stable tree-construction behavior even when the island contains block children that would otherwise be hoisted or reparented.For example, an untrusted island is conceptually transformed as follows:
After
rehypeRaw,rehypeEscapeReservedIslandsidentifies only templates carrying the private marker, reconstructs the original island serialization, and replaces the element with a HAST text node. React therefore displays the attempted markup literally; it never sees ashiny-chat-raw-htmlelement and cannot dispatch it toRawHTML.Both
<shiny-chat-raw-html>and the compatibility spelling<shinychat-raw-html>are covered.Trusted Markdown
A separate
trustedMarkdownProcessorretains raw-island support for content whose provenance is explicitly server-authored. This preserves existing Shiny/htmltools functionality instead of globally disabling HTML islands.HTML content
The HTML processor remains intentionally minimal and does not run the Markdown disguise/escape pair. For untrusted MarkdownStream HTML segments, the component map overrides both island tags with
EscapedIsland, which serializes the HAST node as visible text. This is a defense-in-depth backstop on the Markdown path and the primary island protection on the HTML path.Chat message HTML blocks have a different existing contract: they are trusted, server-authored output and intentionally retain the live raw-island mapping. Model responses use the Markdown processor. Supporting untrusted HTML blocks in chat would require adding provenance to the chat block wire format; inferring it in the client would weaken the trust boundary.
Tokenizer-aware scanner
rewriteTagsHtml()replaces the previous start/self-close regexes and provides the implementation behindrewriteEndTagsHtml(). It performs one forward-only pass and records non-overlapping replacement ranges.The scanner mirrors the HTML tokenizer states relevant to this boundary:
/>self-closing syntaxStart replacements preserve the original attribute bytes. A tokenizer-recognized self-closing custom element is normalized to an explicit open/close pair because HTML ignores the self-closing flag on non-void elements; without normalization, the replacement
<template/>could consume following content.The scan never restarts from a failed candidate or rescans a suffix. Its runtime is O(input length), including repeated malformed candidates and long attribute lists.
The same scanner now handles the existing
<shiny-aside>to<template>rewrite. This removes the second tag-recognition implementation and keeps raw-island and aside parsing behavior aligned.Trust on the wire
MarkdownStreamno longer infers whether a rendered string is safe. Every streamed content message carries:trusted: whether the content originated as trusted server HTML/Tag contentsegment_start: whether the chunk begins a new authored leafThe client stores ordered
{text, trusted}segments and only coalesces a continuation into the previous segment when its trust value matches andsegment_startis false. Missing or malformed provenance fails closed to untrusted.Initial content uses a structured
content-segmentspayload with the same contract, rather than flattening mixed content into one string and one trust bit.Python and R classify content at the leaf level:
HTML()/Tag content is rendered server-side and trustedLeaf-level classification is necessary to prevent trust laundering. Classifying an entire TagList as trusted because it contains one Tag would make an adjacent model-authored string eligible for
RawHTML. With this implementation, the mixed example above renders an<h2>from the string and a live trusted<div>from the Tag, without extending trust across the sibling boundary.Why this shape
The processor-level escape establishes a secure default for assistant Markdown: callers do not need to remember a special component map to prevent raw islands. The MarkdownStream component-map override remains valuable because it independently protects the HTML path and limits the impact of a future Markdown parser edge case.
Trust is explicit at the transport boundary because the browser cannot reliably recover provenance from a flattened HTML string. Carrying provenance per leaf is the minimum information needed to preserve mixed Markdown/HTML composition safely.
The tokenizer scanner is purpose-built rather than regex-based because correctness depends on tokenization context, and because a monotonic state machine provides an auditable linear-time bound for adversarial model output.
Follow-up: consistent trust handling for tool markup (d0db972)
Review of this branch surfaced a related gap in how tool markup (
<shiny-tool-request>/<shiny-tool-result>) is instantiated. The tool router ran on both markdown- and html-typed content and was gated only by message role, so tool elements written into assistant Markdown text were turned into live tool cards — includingvalue-type="html"payloads, which render throughRawHTML. Since assistant Markdown is model output, tool markup should never originate there. This change makes tool-card instantiation follow the same trust boundary as raw-HTML islands:ROUTABLE_CONTENT_TYPESno longer includes"markdown". Both the Python and R servers label Tag-authored content (which is how tool cards are always built) ascontent_type="html", while model prose arrives as"markdown". We audited the paths that could plausibly deliver tool markup as markdown — custommessage_content/contents_shinychat()normalizers, transform hooks, and history/bookmark restore — and confirmed none do: legitimate tool markup is always html-typed end to end.chatTagToComponentMap's tool bridges previously resolved<shiny-tool-*>elements wherever the router left them, including in Markdown content. Html-typed chat blocks (server-authored) keep the full map as a fallback for unrouted elements; markdown and thinking content now use a variant that renders both tool tags as literal, inert text via a sharedEscapedIslandcomponent (extracted fromMarkdownStream). Greetings keep the full map since they are server-authored.custom-display,expanded,framed,full-screen, and collapsed variants, asserting it stays inert text and never becomes a tool card. Existing test fixtures that placed tool markup in markdown-typed blocks — a shape real server output never produces — were corrected to"html"or split into ordered prose and tool blocks.We also considered the more general alternative: threading the per-segment trust provenance this PR adds for MarkdownStream through the whole chat message/block model, and gating tool routing on trusted runs rather than on content type. We chose not to take that on here. Content type already coincides with trust on every real flow, so the gate is equivalent today at a fraction of the complexity; the harder part of the general design is history restore, where a flattened transcript arrives without provenance and trust would have to be re-established from server-side records rather than from stored markup. If a future feature needs untrusted html-typed chat blocks, that provenance work becomes the right foundation and can build on this PR's wire format.