fix: escape shinychat's raw-HTML element names in markdown content - #287
Open
cpsievert wants to merge 11 commits into
Open
fix: escape shinychat's raw-HTML element names in markdown content#287cpsievert wants to merge 11 commits into
cpsievert wants to merge 11 commits into
Conversation
The markdown pipeline's safety argument is that content becomes React elements via toJsxRuntime rather than innerHTML, so raw HTML in model output is inert. Three entries in the tag-to-component map break that: `shinychat-raw-html` reaches `el.innerHTML`, and the two tool elements carry `icon`, `footer`, `tool-name`, and `value` attributes that reach `dangerouslySetInnerHTML`. Those elements are only ever built by `split_html_islands()` and the tool-card tagifier, which run when an app passes htmltools UI rather than a string -- and that content is always labelled content_type "html". So an assistant *markdown* message naming them was never legitimate, and rendering it as markup let model output execute script in the app's origin. That is reachable without any forged input or shared bookmark: an app whose model sees untrusted data (retrieval, web-fetching tools) can be steered into emitting the tag. Escape the three names on the markdown-parsed branch so they render as literal text. This has to happen on the client: the server streams deltas, so a tag name split across two chunks passes any per-chunk filter and is reassembled live in the browser. The client escapes the accumulated block, after reassembly. Content the server built as HTML is untouched, so islands, tool cards, and Shiny bindings render exactly as before. Two existing tests fed tool elements through markdown-typed segments, a payload neither server produces; they now use the html content type that R and Python actually send. Does not address hostile content arriving *as* content_type "html" through a forged `_messages` snapshot -- that route is tracked separately.
chat_ui(messages =) renders tag content into a <shinychat-raw-html> island but never labelled it, so the client defaulted to markdown and (since f0b97e0) escaped the island name into visible text. htmlwidgets in that position also never got bound, because RawHTML's bindAll() only runs on the html branch.
ChatMessage already computes content_type='html' for non-str content; the chat_ui() tag just never forwarded it, so the client defaulted to markdown and escaped the <shinychat-raw-html> island into visible text.
Also folds the chat_ui() news entries in alongside the XSS fix, and keeps the UNRELEASED changelog to a single "Bug fixes" section.
2 tasks
2 tasks
…lied output_markdown_stream() (R and Python) let content_type default to "markdown" even when content was actual UI (pre_process_ui()/ split_html_islands() output), which wraps it in a <shinychat-raw-html> island. That mislabeling was silently masked before this branch: the markdown pipeline's raw-HTML passthrough still parsed and rendered the island correctly despite the wrong content_type. Once markdown-rendered content escapes shinychat's reserved element names, the same mislabeling turns into real breakage -- caught by CI on this branch (test_stream_shiny_ui.py), the same way chat_ui(messages=) was. Same fix as chat_ui(): force content_type to "html" whenever content isn't a plain string, regardless of what the caller passed or defaulted to.
…arkdown
is.character() alone matches htmltools::HTML() (class c("html", "character")),
so an HTML()-marked message string was being treated as plain markdown --
mislabeled content_type, same class of bug as the chat_ui() Tag/TagList
fix earlier on this branch. Mirrors chat_set_greeting()'s existing three-way
branch (plain string / HTML() string / Tag-rendered) instead of introducing a
second, inconsistent test for the same thing.
Caught by Copilot's review on #287.
cpsievert
added a commit
that referenced
this pull request
Aug 5, 2026
…arkdown
is.character() alone matches htmltools::HTML() (class c("html", "character")),
so an HTML()-marked message string was being treated as plain markdown --
mislabeled content_type, same class of bug as the chat_ui() Tag/TagList
fix earlier on this branch. Mirrors chat_set_greeting()'s existing three-way
branch (plain string / HTML() string / Tag-rendered) instead of introducing a
second, inconsistent test for the same thing.
Caught by Copilot's review on #287.
cpsievert
changed the title
Rewrite the three bug-fix bullets so they read as a single, plain description of the user-visible behavior change, rather than exposing the vulnerability language and the incidental history of a regression that was introduced and fixed within this same branch.
… Tag/TagList
chat_ui()'s message loop, chat_ui()'s greeting handling, chat_set_greeting(),
and output_markdown_stream() each special-cased a raw HTML() string as a
"fast path" that skipped pre_process_ui() and rendered it unwrapped. That
matched Tag/TagList content on the label (content_type = "html") but not on
behavior: only content wrapped in <shinychat-raw-html> reaches RawHTML's
literal innerHTML assignment and its Shiny bindAll() call, so Shiny bindings
inside a hand-authored HTML() string were silently never bound.
Route HTML()-classed content through the same pre_process_ui() branch as
Tag/TagList everywhere it was special-cased, collapsing what had become a
3-way is.character()/inherits("html") branch back down to the 2-way split
already used (correctly) by chat_append() and the greeting stream chunk
loop. content_type is now only a caller's choice for a genuine plain
string; HTML()-classed content is always "html", same as Tag/TagList.
output_markdown_stream()'s content_type parameter default changes from
"markdown" to NULL so a caller's explicit choice (for a plain string) can
be told apart from not having specified one.
…e as chat_ui()/chat_set_greeting() chat_append_message() classified content with a forward allowlist of specific classes (shiny.tag, shiny.tag.list, html, htmlwidget, shinychat_tool_card). Content that pre_process_ui() still resolves and wraps in a <shinychat-raw-html> island via as.tags() -- e.g. a bare list of tag children -- but that doesn't match one of those classes fell through to content_type "markdown", so the client's reserved-element escaping turned the island's own wrapper into visible text. Switch to the same inverse-exclusion check used everywhere else on this branch: is.character(content) && !inherits(content, "html").
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.
Summary
<shinychat-raw-html>,<shiny-tool-request>, or<shiny-tool-result>was rendered as those live custom elements, which write straight toinnerHTML/dangerouslySetInnerHTML. No bookmark, no forged input, no feature flag — just model output (e.g. a RAG document, a tool result fetching an untrusted page) repeating one of those names verbatim, which is the standard indirect-prompt-injection shape. Those names are now escaped whenever content renders through the markdown branch, so they display as literal text instead of instantiating a live element.content_type = "html"— never in markdown. Making that boundary a hard one surfaced a family of R/Python bugs where content that is HTML (htmltools tags,TagLists,HTML()strings, htmlwidgets) was getting labeled"markdown"instead — inchat_ui(messages=),chat_set_greeting()/chat_ui()'s greeting,output_markdown_stream(), andchat_append_message(). Previously this was harmless: the mislabeled wrapper still parsed fine either way, so it just rendered correctly by accident. With the escaping fix in place, the same mislabeling would instead make widgets in initial messages, greetings, and streamed/appended content silently stop rendering. All four are fixed to always label such content"html"regardless of what the caller passed or defaulted to.htmltools::HTML()strings) had a deeper bug once inspected: it was being labeled"html"but rendered through a fast path that skipped the wrapper entirely, so Shiny bindings inside a hand-authoredHTML()string never actually bound. Routed through the same path asTag/TagListcontent so behavior matches the label.Test plan
npx vitest run(js) — 710 passingdevtools::test()(R) — 639 passinguv run pytest(py) — 142 passing