fix(cli): escape attacker-controlled nicknames before terminal output - #633
Merged
Conversation
Member nicknames are attacker-controlled (any room member sets their own) but were printed to the terminal unescaped at several sites outside the deputy commands, letting a hostile nickname inject ANSI terminal escape sequences into an operator's session. Reuse the existing `display_nickname` escape helper (already used on the deputy surface) at every terminal print site: reply quotes (`reply_prefix_display`), reaction/deletion/message human-format output in api.rs, `message list`'s human branch, and `dm list`'s thread header. Each of these sites shares its raw nickname value with a JSON output field or (for replies) a value persisted into `ReplyContentV1` — those raw values are left untouched, since escaping them would corrupt the data for a bridge/consumer or for future readers. [AI-assisted - Claude]
…on rendering The initial fix missed a second injection path: render_mentions_for_terminal substitutes a member's current nickname (or the wire token's own attacker-supplied snapshot name, for an unknown member) into message and reply text with no escaping, and that rendered text reaches the same human println sites the first commit fixed for the direct author/nickname case. Split render_plaintext (common/src/mention.rs) into a thin wrapper plus render_plaintext_transformed, which runs a transform closure over whichever name gets substituted -- resolved live nickname OR the snapshot fallback -- before splicing it in. Add render_mentions_for_terminal_escaped and message_display_text_for_terminal as terminal-only siblings that pass display_nickname as the transform, and a preview_for_terminal field on ReplyContextDisplay::Quote for the reply-preview case. Every JSON/ persistence-facing counterpart is left untouched and stays byte-identical. common/ is a feature-gated client-only module here (mentions feature), so this needs no delegate/contract WASM migration entry. [AI-assisted - Claude]
…urce here
Per review: cli/src/commands/message.rs and cli/src/commands/dm.rs each have
exactly one #[cfg(test)] mod tests block, at the end, with no earlier
#[cfg(test)] item -- which is why the plain split_once("mod tests") /
find("mod tests") cut used by their new pin tests is safe there. Note it
inline so a future edit adding an earlier #[cfg(test)] item doesn't silently
weaken the pin.
[AI-assisted - Claude]
…, rename raw helper, close wasted work - Bump river-core (workspace version) 0.1.19 -> 0.1.20 and update cli/Cargo.toml's pin to match. river-core 0.1.19 is already published on crates.io and does not contain render_plaintext_transformed, so riverctl's publish would otherwise resolve against a river-core build missing that function. - Add deputies::escape_nickname_inline: display_nickname's escaping with the outer quote pair trimmed, for splicing an escaped name into the MIDDLE of other text (an @mention substituted into a message/reply preview) rather than presenting it as a standalone column. The column-forgery argument for quoting does not transfer inline -- the attacker already controls every surrounding byte there -- so quoting bought no security and cost legibility on every ordinary mention. render_mentions_for_terminal_escaped now uses this instead of display_nickname. - Rename render_mentions_for_terminal -> render_mentions_raw: the old name actively contradicted what it does (it is the UNescaped variant feeding JSON/persistence). Add a source-scrape pin (human_output_arms_never_call_a_ raw_content_or_mention_helper, plus an extension of message.rs's existing pin) asserting no OutputFormat::Human arm calls a raw helper directly -- this exact miss is what this PR is about, twice. - output_message no longer computes the raw `content` before the match when only the Json arm reads it; moved the binding into that arm so the Human arm doesn't decrypt and mention-render the body a second time for nothing. [AI-assisted - Claude]
riverctl 0.2.11 is already published on crates.io, and the release workflow's publish_if_needed step skips an already-published version -- so a riverctl-v0.2.11 tag would run green and publish nothing, and this fix would never reach a user. Bump to 0.2.12. Also fix a comment in output_message left stale by the earlier commit that moved the raw `content` binding into the Json arm. [AI-assisted - Claude]
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.
Problem
riverctlprints member nicknames to the terminal unescaped at several sites outside the deputy commands. Nicknames are attacker-controlled — any room member sets their own — so an unescaped nickname can inject ANSI terminal escape sequences into an operator's terminal (e.g. to rewrite prior output, hide text via a bare\r, or sound a bell).unseal_nickname_display(cli/src/api.rs) is a plain, unfilteredString::from_utf8_lossywith no escaping, and was called raw at the human-output sites inapi.rs(reply quoting, reaction/deletion/message printing),commands/message.rs(message list), andcommands/dm.rs(dm list).A second injection path exists via
@mentionrendering: the mention-substitution renderer (cli/src/api.rs) substitutes a member's current nickname (or the wire token's own attacker-supplied snapshot name, when the member is unknown) into message/reply text with no escaping, and that text feeds the same human println sites. One attacker suffices — set your own nickname to a hostile payload and@mentionyourself in a message or a reply.An escape helper already existed and was already used correctly on the deputy surface:
display_nickname(cli/src/deputies.rs), which quotes the nickname and escapes it via{:?}(covers control, format, and separator Unicode categories, not a hand-maintained range list).Scope statement: this PR closes the nickname-injection surface (direct nickname printing and
@mentionsubstitution). Message bodies, DM bodies, reaction emoji, and room names/descriptions are ALSO attacker-controlled and printed raw, but that is a different vulnerability class — you can't{:?}-escape a message body without wrecking readability, so it needs a different fix shape. Deliberately not addressed here; Ian is deciding how that gets handled.Approach
Reuse
display_nickname's escape table at every identified terminal print site rather than inventing a second escaping scheme.Went site by site because several sites share the same raw value with a JSON output field or a value persisted to contract state, and escaping those would corrupt the data:
Escaped (terminal-only output) — direct nickname sites:
api.rs::reply_prefix_display— the[reply to X: ...]terminal quote prefix (a column/label site — kept quoted viadisplay_nickname).api.rs::output_reaction_change/output_deletion— escape a deriveddisplay_nameused only in the humanprintln!; the sharednicknamevariable feeding the JSON"nickname"field is untouched.api.rs::output_message(human branch) — this branch makes its own separateunseal_nickname_displaycall distinct from the JSON branch's, so it's escaped inline.commands/message.rs(message list, human branch) — same shape asoutput_message.commands/dm.rs(dm list, human thread-header) — the sharednicknameslookup map is read by both the human and JSON branches; escaping happens at the print site, not at the map.Escaped (terminal-only output) —
@mentionrendering:common/src/mention.rs::render_plaintextis split into a thin wrapper plusrender_plaintext_transformed, which runs atransformclosure over whichever name is substituted (resolved live nickname OR the token's own snapshot fallback) before splicing it in — the fallback matters because it's just as attacker-controlled, and the parser (unlike the outgoing encoder'ssanitize_name) applies no charset restriction on read.api.rs::render_mentions_for_terminal_escapedis the terminal-only sibling of the raw renderer (renamedrender_mentions_raw— see below), passing a NEW helper,deputies::escape_nickname_inline, as the transform.escape_nickname_inlinevsdisplay_nickname: an inline@mentionsits in the MIDDLE of other text, not a standalone column, so the column-forgery argument that justifiesdisplay_nickname's quoting doesn't transfer — inside a message body the attacker already controls every surrounding byte and can fake a row as plain prose with no mention at all. Quoting every ordinary mention (hey @"Alice" can you review?) would cost legibility for zero extra security.escape_nickname_inlinereuses the exact same escape table (display_nickname's{:?}output with the outer quote pair trimmed), so it isn't a second hand-maintained scheme.api.rs::message_display_text_for_terminalis the terminal-only sibling ofmessage_display_text_with_secrets, sharing a common raw-body helper, that renders mentions through the escaped path. Wired intooutput_message's human branch andcommands/message.rs'smessage listhuman branch.ReplyContextDisplay::Quotegained apreview_for_terminalfield (mention-escaped) alongside the existingpreview(raw, unchanged, used byreply_to_jsonand the persistedReplyContentV1);reply_prefix_displaynow readspreview_for_terminal.preview_for_terminaltruncates AFTER escaping — this is intentionally unchanged and safe:truncate_reply_previewtakes 50 chars from text whose control bytes are already literal\u{...}sequences, so a mid-sequence cut yields harmless text like...\u{1and can never re-materialize a control byte. Accepted cosmetic residual, not something this PR fixes.Naming / robustness cleanup (from review):
render_mentions_for_terminaldespite feeding JSON/persistence — actively misleading. Renamed torender_mentions_raw.human_output_arms_never_call_a_raw_content_or_mention_helper(plus an extension ofmessage.rs's existing pin), asserting noOutputFormat::Humanarm inapi.rs/message.rscalls a raw helper (render_mentions_raw/message_display_text_with_secrets) directly. This PR exists because that exact mistake happened twice; the pin is there so a third miss fails CI instead of shipping.output_messagecomputed the raw, unescapedcontentunconditionally before branching on format, but the Human arm only readscontent_for_terminal. Moved thecontentbinding into theJsonarm, the only place that still reads it.Deliberately left raw:
api.rs(reply-send path)target_author_name— persisted intoReplyContentV1.target_author_name/ contract state. Escaping here would corrupt the stored value for every future reader.reply_to_json'spreview/author, and the JSON arms ofoutput_reaction_change,output_deletion,output_message,message list, anddm list— JSON's own string escaping already makes these safe, and a second escaping pass would corrupt the value for a bridge/consumer.cli/src/deputies.rs(deputy commands) andcommands/member.rs(member list) were already correct before this PR and are unchanged.commands/identity.rs(identity whoami) prints a member's ownmember_inforecord read from network room state (storage::self_identity_fromreadsroom_info.state.member_info.canonical(member_id)) — but the contract binds a non-owner'smember_infoto that member's own verifying key (member_info.rs,verify_signature_with_key(&member.member.member_vk)), so nobody else can author it. Out of scope for this attacker-controlled-other-party issue.Version bumps (required for this to actually ship)
river-core(workspace version) 0.1.19 → 0.1.20.render_plaintext_transformedis new public API inriver-core.river-core0.1.19 is already published on crates.io;cli/Cargo.tomlpinsriver-core = { version = "=0.1.19", path = "../common", ... }, andcargo publishstrips thepath, resolving against the published crate — which lacks the new function. Not caught by CI; only bites at publish time. Updated the=0.1.19pin to=0.1.20to match.riverctl(cli/Cargo.toml) 0.2.11 → 0.2.12. crates.io's max published riverctl version is also0.2.11, and the release workflow'spublish_if_neededstep skips an already-published version — so ariverctl-v0.2.11release tag would run fully green and publish nothing. This fix would never reach a user.check-delegate-migration/check-room-contract-migrationpass on this PR, but only because they diff the COMMITTED.wasmfiles between base and head, and this PR touches no.wasmfile — that is NOT evidence the version bump is WASM-neutral. It is not: rustc's-C metadataincludes the crate version, and this repo has already measured that exact effect (ui/src/components/app/chat_delegate.rs, the V30/Room summary is ~29 KB because it embeds 64-byte Ed25519 signatures where a u32 hash would do #571legacy_set_fingerprint_is_stable_across_codegen_changesnote: a river-core version bump alone re-keyedchat_delegate.wasmeven though the functional change that motivated the bump was dead-code-eliminated from the delegate). Shipping a version-bump-only PR without an immediate WASM rebuild has precedent here (b2654d1b,f3f6907e,f72b5a6c), so this is not a blocker — but the next PR that legitimately rebuildschat_delegate.wasm/room_contract.wasminherits a re-key it did not cause, and will need alegacy_delegates.tomlentry, acommon/legacy_room_contracts.tomlentry, and an updatedlegacy_set_fingerprintpin (currentlyc43e66ee147e3739).Testing
api.rs::reply_author_ansi_escapes_are_escaped_for_terminal_and_raw_in_json— a nickname containing ANSI CSI (\x1b[2J), a bare CR, and a bell must render escaped (quoted) throughreply_prefix_display, and remain byte-identical (raw) throughreply_to_json.api.rs::terminal_nickname_sites_escape_human_output_and_leave_json_raw,commands/message.rs::message_list_escapes_nickname_for_human_and_keeps_json_raw,commands/dm.rs::dm_list_escapes_nickname_for_human_and_keeps_json_raw— source-scrape pins confirming each human branch escapes viadisplay_nicknameand each JSON branch stays raw.api.rs::human_output_arms_never_call_a_raw_content_or_mention_helper— brace-matched source-scrape (reusing the existingproduction_source/end_of_iteminfrastructure) asserting noOutputFormat::Humanarm inapi.rscalls a raw helper directly.common/src/mention.rs::render_plaintext_transformed_applies_to_resolved_and_fallback_names_alike— the transform hook fires for both the resolved and snapshot-fallback branches.api.rs::render_escaped_escapes_the_resolved_live_nickname/render_escaped_escapes_the_snapshot_fallback_name_too— bothrender_mentions_for_terminal_escapedbranches escape (unquoted, inline form) a hostile payload; the fallback case is built by hand, not viaencode_mention, since that helper's ownsanitize_namewould launder the payload before it reaches the parser.api.rs::message_display_text_for_terminal_escapes_mentioned_nickname_json_stays_rawandreply_preview_escapes_mentioned_nickname_for_terminal_json_stays_raw— end-to-end through the actual functions wired intooutput_message/message list/reply_prefix_display, confirming the human path escapes a mentioned member's hostile nickname while the JSON/persistence path stays byte-identical.deputies.rs::escape_nickname_inline_matches_display_nickname_minus_quotes— direct unit coverage: same escape table asdisplay_nickname, no added quoting, ordinary names round-trip unchanged, empty-name edge case doesn't panic.human_output_arms_never_call_a_raw_content_or_mention_helperpin and the un-quoting change.unverifiable_quotes_are_omitted_from_json_and_neutral_in_texttest, whose expected string pinned the old unescaped/unquoted terminal format.cargo test -p riverctl --lib: 347 passed, 0 failed.cargo test -p river-core --features ecies,ecies-randomized,migration,mentions --lib: 253 passed, 0 failed.cargo fmt --checkclean on both crates.check-delegate-migration,check-room-contract-migration,check-pointer-freshness, andcheck-wasm-syncall pass — see the "Version bumps" section above for why that's expected but not proof of WASM-neutrality.Closes #474
[AI-assisted - Claude]