fix(pkg-r): faithfully restore displayed chat UI from bookmark, not re-derived turns - #279
fix(pkg-r): faithfully restore displayed chat UI from bookmark, not re-derived turns#279cpsievert wants to merge 22 commits into
Conversation
…bookmarks Wires bookmark_save_ui()/bookmark_restore_ui() into chat_restore()'s onBookmark/onRestore handlers so bookmarked sessions restore the exact browser-displayed UI (server store), falling back to turn-derived UI for URL bookmarks or when no snapshot was captured.
memCompress(x, "gzip") writes RFC 1950 (zlib) data, and memDecompress's default type = "unknown" only reliably detects that format starting in R 4.4.0 (libdeflate). On R 4.2/4.3, auto-detection silently falls back to "none", corrupting bookmark and UI-snapshot restoration. Passing type = "gzip" explicitly removes the dependency on auto-detection.
…ponse flags decode_ui_snapshot() now returns NULL instead of erroring on a corrupted bookmark payload, letting chat_restore() fall back to turn-derived UI as documented rather than aborting the whole restore. Also removes the flag-based bookmark-on-response machinery (chat_update_bookmark, has/set_session_bookmark_on_response), which became dead code once chat_restore() switched to triggering on the client's _messages echo instead of stream completion.
cancel_bookmark_on_response fired automatically whenever the browser's _messages echo happened to end in an assistant message -- including echoes produced by populating existing UI at session start (initial turn replay via client_set_ui(), or bookmark_restore_ui()'s replay), not just genuine live responses. A two-flag replay guard alone isn't enough: replaying N existing messages produces N separate growing echoes, and a single-use suppression flag only catches the first. Add has_user_submitted, gating the response-bookmark trigger on a real user submission having occurred in this session, alongside the is_replaying_ui/suppress_next_bookmark pair (mirroring HistoryController's is_replaying/suppress_next_save) which still guard a submission racing the tail of a startup replay. Also: trim a duplicated rationale comment in client_state.R, and add two deferred test assertions (empty-list fallback, first replayed message) from the earlier per-task review.
c60e073 to
03b1b57
Compare
gadenbuie
left a comment
gadenbuie
left a comment
There was a problem hiding this comment.
Thanks for closing the restore/transform gap left by #272 — the snapshot codec and the reuse of restore_history_message() are clean, and the has_user_submitted reasoning is well documented.
A few things I'd like to resolve before merge. The blocking one is a trust-boundary problem: a server bookmark now persists the browser-controlled _messages snapshot (including htmlDeps) and replays it verbatim into any session that opens the bookmark URL, which is a stored-script vector. The response-bookmark guard ordering and the malformed-snapshot fallback also need a look, and the new tests don't yet exercise the observer where the guard bug would surface.
| }) | ||
|
|
||
| return(prom) | ||
| state$values[[paste0(id, "_ui")]] <- encode_ui_snapshot( |
There was a problem hiding this comment.
[Blocking] Persists browser-controlled UI metadata that replays as script.
bookmark_save_ui() serializes the raw browser-reported _messages snapshot, and on restore restore_chat_ui() → restore_history_message() forwards its htmlDeps unchanged (chat.R:816), where the client turns them into <script> elements appended to document.head.
Because a server bookmark is shareable via its _state_id_ URL, an attacker can submit a forged _messages value in their own session, let chat_restore() mint the bookmark, and share the URL — the recipient's session then loads attacker-selected dependency scripts / raw HTML in the app origin during restore.
The _messages input handler (messages_input_value()) normalizes field names and validates attachments, but it does not establish a trust boundary for htmlDeps or html segments. Please reconstruct/allowlist dependencies from server-owned state (or otherwise ensure replayed HTML + deps cannot execute client-supplied code) rather than persisting them from an untrusted client input. Worth an adversarial test proving a forged snapshot cannot load a script.
There was a problem hiding this comment.
Both halves fixed, same shape of fix, same input boundary — and reworked once
more since the first pass. Now on #288 (44d71933, b68e11e9, 0fa88f24).
Deps, in 44d71933: send_chat_action() records every dependency it
sends, keyed by name@version. At the _messages input boundary each
reported dependency is replaced by the server's own copy of it, and anything
the server never sent is dropped. Nothing downstream — bookmark state, history
store, replay — ever sees a client-supplied dependency object again.
Html segment content, in b68e11e9/0fa88f24: the raw-html sink you
flagged (RawHTML.tsx:26, el.innerHTML = html) needed the same trust
boundary as deps, but there's no server-owned dependency object to substitute
here — the displayed string is the thing worth preserving, so it can't just
be swapped for a canonical copy.
The first pass (b68e11e9) recorded a hash for every prefix of a streaming
html run, because it couldn't predict where the client would close a segment.
On a second look that turned out to be defending a path that doesn't exist:
buildMessagesSnapshot() on the client filters out anything still streaming,
so the browser only ever reports a settled message — never an intermediate
prefix. 0fa88f24 replaces the prefix-hashing with a ledger that performs the
same merge the client's chunk reducer performs (append vs. replace,
content-type boundaries, chunk_start/chunk/chunk_end) and records the one
resulting string when the message settles. Same trust boundary, no more
O(n²) hashing, and a test now pins the invariant directly: an in-flight
prefix is asserted not trusted, only the settled merge is.
messages_input_value() degrades any reported content_type: "html" segment
that doesn't match the ledger to "markdown" — content untouched, so it
renders as inert literal text (the markdown branch escapes
shinychat-raw-html/shiny-tool-request/shiny-tool-result as of #287).
Also ported the same boundary to Python (_html_trust.py), which had the
identical gap — messages_input_value() there took content_type and
htmlDeps from the client verbatim into StoredMessage and on into the
history store. Wasn't in scope for your original comment since this PR never
touched Python, but it's the same sink, so it's part of #288 now.
Same adversarial test shape as before, in both languages: forge a _messages
value with attacker html, sanitize it through the input handler, feed it into
restore_history_message() (the primitive a history-conversation switch and
bookmark restore both call per stored message) into a fresh session, and
assert "html" never appears among the segment types — or a dependency
object — that reach the render/send path there. Added an end-to-end R/Python
pair that drives a real chat_append()/append_message() call through the
actual send path and feeds the wire content back through the input handler
the way the browser would, to check the ledger agrees with genuine server
output and not just with my model of it.
Two more bugs found while getting the invariant solid enough to build the
registry on, now on different PRs: chat_ui(messages = list(div(...))) was
rendering its <shinychat-raw-html> wrapper as literal text instead of the
actual content — fixed in #287, travels with the escaping fix rather than
the trust boundary. And chat_ui(messages =) content was being included in
the browser's reported snapshot even though it's static page markup, so a
bookmark restore rendered it twice — fixed in #290, since it only matters
once the restore feature exists to do the double-rendering.
Still open, deliberately not closed here: a forger with a trusted html
segment can still reorder it, duplicate it, or attach it to a different
message. Every byte reaching the sink is still server-authored, so this is
transcript spoofing, not script injection — closing it needs positional or
role binding per segment, which felt like its own PR.
| label = "on_response_do_bookmark", | ||
| ignoreInit = TRUE, | ||
| { | ||
| if (!has_user_submitted) { |
There was a problem hiding this comment.
[Required] Response-bookmark guard order can eat the first real bookmark.
The guard checks !has_user_submitted before suppress_next_bookmark. During the whole replay window has_user_submitted is FALSE, so we return() on the first line every time and never consume suppress_next_bookmark during replay. The stale flag leaks into the first genuine interaction.
Whether it does harm depends on echo ordering: if the first post-submit _messages echo ends in "user" it is consumed harmlessly, but if the user+assistant echoes coalesce into one update ending in "assistant", suppress eats that one → no bookmark for the first assistant reply, which is exactly the miss this PR set out to prevent.
HistoryController$on_response (chat_history.R:84-90) checks suppress_next_save first with no has_user_submitted gate, so its flag is consumed by the post-replay echo as designed — this ordering is inverted relative to that. Per your own comment at :159-169, has_user_submitted is sufficient on its own; if so, dropping suppress_next_bookmark (and likely is_replaying_ui, never TRUE once has_user_submitted is) removes the misfire. Otherwise, please reorder so suppress is consumed during replay and document the scenario where has_user_submitted alone is insufficient.
There was a problem hiding this comment.
Now on #290 (b07e645c).
Your read is right, including that has_user_submitted is sufficient on its
own. is_replaying_ui is only ever TRUE during startup or restore, which is
strictly before the first submission, so both it and suppress_next_bookmark
were unreachable-but-harmful. Both are gone, along with the onFlushed
bookkeeping that maintained them.
Confirmed the failure mode before fixing it: reintroducing the old ordering
fails three of the new observer tests, including the coalesced-echo case.
One thing worth flagging directly, since it's still open and not something
your comment covered: has_user_submitted only guards the startup replay
window. If an app pairs chat_restore() with chat_enable_history(),
switching conversations replays N messages and produces N _messages echoes
after the user has already submitted once this session — each ending in an
assistant message mints its own bookmark. Not proposing a fix for this PR;
flagging it as a follow-up (also noted in #290's description).
| type = "gzip", | ||
| asChar = TRUE | ||
| ) | ||
| jsonlite::unserializeJSON(json) |
There was a problem hiding this comment.
[Required] Syntactically valid but malformed snapshots still abort restore.
decode_ui_snapshot() catches base64/gzip/JSON errors but does not validate the decoded value, so a well-formed-but-wrong payload slips through. E.g. decode_ui_snapshot(encode_ui_snapshot(1L)) returns 1L, and replay then fails on message$role rather than taking the documented turn-derived fallback. Please validate the transcript container and each message before replay, and route validation failures into the fallback. (Python's _restore_bookmark_message() is a useful parity target.)
Relatedly, error = function(e) NULL swallows genuine corruption silently — a rlang::warn() in the catch would make a lost faithful-restore diagnosable instead of a silent downgrade to turn-derived UI.
There was a problem hiding this comment.
Split across #290 (7feac78b → simplified by 475c4601) and #289 (b37f7fa5).
R side, #290: decode_ui_snapshot() initially grew a set of structural
predicates validating the transcript container and each message (role,
segments, and the optional attachments/htmlDeps fields) before returning
it, so decode_ui_snapshot(encode_ui_snapshot(1L)) would take the
turn-derived fallback instead of failing on message$role. On review that
felt like validating a shape shinychat itself writes into its own bookmark
state dir — the only realistic producer of a wrong shape is version skew (a
bookmark URL outlives the shinychat that wrote it). 475c4601 replaces the
predicates with a version envelope on the snapshot, matching what
client_get_state() already does for the turns payload: one check instead of
a structural walk, same fallback, same warning.
Both failure modes (unreadable envelope, corrupted payload) still
rlang::warn() — agreed a silent downgrade is worse than a noisy one.
Python side, now its own PR (#289) rather than folded in here:
_restore_bookmark_message() was raising on a malformed message, which
aborts the whole restore loop and drops every message after the bad one too
(Shiny's generic on_restore error handling just shows a banner, it doesn't
resume the loop). Python has no turns-derived fallback to route into the way
R does, so the proportionate fix there is per-message: warn and skip just the
bad message, restoring everything else. Test seeds one malformed message
between two valid ones and asserts both valid ones still restore.
| @@ -0,0 +1,197 @@ | |||
| test_that("encode/decode UI snapshot round-trips a simple message", { | |||
There was a problem hiding this comment.
[Required] Response-bookmark state machine is untested.
Current tests cover the codecs and restore_history_message() replay, but never register chat_restore() and drive _user_input / _messages through a reactive session — which is exactly why the guard-ordering issue (the !has_user_submitted check running before suppress_next_bookmark) would not be caught. Please add observer-level coverage: startup/restore replay does not bookmark; a user-only snapshot does not response-bookmark; the first settled assistant response bookmarks exactly once; both bookmark_on_input settings; callback teardown.
There was a problem hiding this comment.
Now on #290 (b07e645c + pkg-r/tests/testthat/test-chat_restore.R).
All of it observer-level through shiny::testServer() with doBookmark()
counted: startup/restore replay doesn't bookmark, a user-only snapshot
doesn't, the first settled assistant reply bookmarks exactly once, both
bookmark_on_input settings, bookmark_on_response = FALSE, and teardown.
The one that would have caught the guard bug drives the coalesced post-submit
echo specifically, since that's the only ordering where the miss is visible.
| return(NULL) | ||
| } | ||
| json <- jsonlite::serializeJSON(messages) | ||
| base64enc::base64encode(memCompress(json, "gzip")) |
There was a problem hiding this comment.
[Optional] Duplicated serialize→gzip→base64 pipeline.
The serializeJSON → memCompress(gzip) → base64encode pair (and its decode inverse) now lives here and in client_get_state()/client_set_state() (:44-45, :68-72). This is the exact pair the cross-R-version fix (ec897ab) had to touch twice; a shared gzip_b64_encode/decode helper would localize the next such fix.
There was a problem hiding this comment.
Now on #290 (2891dd4e).
One gzip_b64_encode()/gzip_b64_decode(), with the serializeJSON rationale
and the explicit type = "gzip" note (the thing an earlier commit on this
branch had to fix twice) documented once at the helpers.
The browser's `<id>_messages` report was carried into saved history and bookmark state verbatim, dependency objects included, and replayed with `send_chat_action(html_deps =)` on restore -- which reaches `Shiny.renderDependenciesAsync()` and injects the dependency's script/head content into the page. Because a server bookmark is shareable via its `_state_id_` URL, an attacker could forge a `_messages` value in their own session, let `chat_restore()` mint the bookmark, and share the URL to load attacker-chosen scripts into the recipient's session. `send_chat_action()` now records every dependency it sends, keyed by name+version, and the `_messages` input handler treats the client's report as nothing more than a set of identities: each reported dependency is replaced by the server's own copy of it, and anything the server never sent is dropped. Sanitizing at the input boundary covers the history store as well as bookmark state. Hostile `html` segment content is a separate, unaddressed vector with the same reach; tracked separately.
The guard checked `!has_user_submitted` before `suppress_next_bookmark`, so it returned on the first line for the whole replay window and never consumed the suppression flag. The stale flag then leaked into the first genuine interaction: when the post-submit `_messages` echo coalesced the user and assistant messages into one update ending in "assistant", suppress ate exactly the bookmark this PR set out to guarantee. `has_user_submitted` is sufficient on its own -- every replay window falls strictly before the first submission -- so `suppress_next_bookmark` and `is_replaying_ui` are gone, along with the onFlushed bookkeeping that maintained them. Adds observer-level coverage that drives `_user_input`/`_messages` through a reactive session: startup replay does not bookmark, a user-only snapshot does not, the first settled reply bookmarks exactly once under both `bookmark_on_input` settings, and teardown stops both observers. All three failure modes reproduce against the old ordering.
The serialize -> gzip -> base64 pair and its inverse lived in three places, two of which the cross-R-version fix in ec897ab had to touch separately. Now one `gzip_b64_encode()`/`gzip_b64_decode()`, with the rationale for serializeJSON and for the explicit `type = "gzip"` documented once at the helpers. Also drops the `_ignore()` stub, since the codec is a real use of base64enc.
`decode_ui_snapshot()` caught base64/gzip/JSON errors but not well-formed-but-wrong payloads, so e.g. a bare integer reached replay and failed on `message$role` instead of taking the documented turn-derived fallback. It now validates the transcript container and each message, and routes failures into that fallback. Both failure modes warn: a silent downgrade from faithful restore to re-derived UI is otherwise undiagnosable.
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.
chat_ui(messages =) content is static app markup that the browser re-renders on every page load, including a restored one. Reporting it back meant a bookmark persisted it, so restore rendered it twice -- verified as three saved messages coming back as six -- and it put content in the snapshot that the server never sent.
Hashes every prefix of a consecutive html run, so whichever boundary the client picks when it merges adjacent same-type chunks into one segment, the string it reports back is in the registry. Recording only; the validation that uses it lands next.
record_sent_html_content() read segment fields with $, which partial- matches "content" onto "content_type" for a segment missing content. That let a malformed segment shaped list(content_type = "html") get treated as html content "html" and permanently trusted. Switch to [[ ]] throughout so only an exact field name matches, and add a regression test for the malformed-segment case.
A reported content_type: 'html' segment now has to match something the server actually sent this session; a miss degrades that one segment to markdown, where the client escapes shinychat's raw-HTML element names. Closes the last reach of a forged _messages report: bookmark state is shareable via its _state_id_ URL, and RawHTML assigns html content straight to innerHTML.
|
Splitting this into four PRs rather than merging it as one — it mixed an
|
Why
#272 made the browser the authoritative source for what a chat currently
displays, so the server always saves the exact rendered snapshot the user
saw (transformations included) rather than re-deriving it from LLM turns.
chat_restore()never got the other half of that: on restore, it stillcalled
client_set_ui(), which regenerates the UI straight from theclient's turns. Any transformation applied between receiving a messageand displaying it (custom rendering, markdown post-processing, etc.) was
silently dropped on restore, even though the browser's own snapshot -
the thing #272 introduced - had it the whole time. Bookmark restoration
and live chat state also disagreed about what "the chat" looked like.
There was a second, related gap: the "bookmark on response" trigger fired
when the assistant's stream completed, but the browser echoes its settled
message snapshot back to the server in a later round trip. Bookmarking
on stream completion could persist a snapshot missing the reply that had
just finished, since the browser hadn't reported it yet.
What changed
chat_restore()now captures the browser's reported message snapshotinto the bookmark's
state$values(server bookmark store only - thegzip+base64 payload isn't URL-bookmark friendly) and replays that
snapshot on restore, preserving display-only transformations exactly
as shown.
client_set_ui()path forbookmarkStore = "url"and for bookmarks saved before this snapshotexisted, so nothing breaks for existing bookmarks.
_messagesecho (once it reports a settled transcript ending in anassistant message), instead of stream completion - mirroring the same
fix feat: make the browser the source of truth for chat messages #272 already made for history saves, applied here to bookmarks.
population (a fresh session's pre-seeded turns, or a restore itself) -
only a genuine user-submitted exchange should mint an automatic
bookmark.
decompression, and removes bookmark-on-response code made fully dead
by the trigger change above.
encode_ui_snapshot()/decode_ui_snapshot()(reusing theserializeJSON-> gzip -> base64 pipeline already used for clientstate) plus regression tests covering round-tripping, htmlDeps/
attachments preservation, and the fallback path for pre-snapshot
bookmarks.
Test plan
and turn-derived fallback (
pkg-r/tests/testthat/test-chat_restore.R)devtools::test()/devtools::check()inpkg-r/— 599 passed, 0 failures