From 5927148ee4e86e34429418e2ea246fb2db6e2d53 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 17 Jul 2026 18:36:21 -0400 Subject: [PATCH] chat: enforce shared text policy at UI, sign point, and gossip ingress Chat-hardening plan Phase 1. The chat body policy (2,000-char + 8 KiB ceilings, single-pass control/whitespace normalization) moves from the UI layer into src/sanitize.rs and is now enforced at every trust boundary: cap_chat_input bounds the live input (oversized paste), the gossip sign point re-sanitizes so non-UI callers can't bypass policy, and gossip ingress rejects oversized raw text before sanitizing (admit_chat_text) and drops messages with neither visible text nor an attachment. The incoming chat author label now uses the strict name sanitizer until Phase 2 roster-binds it. +8 tests (532 lib green). Co-Authored-By: Claude Fable 5 --- docs/chat-hardening-plan.md | 44 +++++--- src/app/mod.rs | 70 +++++------- src/network/gossip.rs | 23 ++++ src/sanitize.rs | 205 +++++++++++++++++++++++++++++++++- src/widget/selectable_text.rs | 2 +- 5 files changed, 285 insertions(+), 59 deletions(-) diff --git a/docs/chat-hardening-plan.md b/docs/chat-hardening-plan.md index d37bcee..ea51f05 100644 --- a/docs/chat-hardening-plan.md +++ b/docs/chat-hardening-plan.md @@ -1,6 +1,8 @@ # Chat hardening — ephemeral implementation plan -**Status (2026-07-15):** PLANNED, not started. This is a temporary scope +**Status (2026-07-17):** Phase 1 COMPLETE (shared text policy in `src/sanitize.rs`, +ceilings enforced at UI input, sign point, and gossip ingress; all gates green). +Phases 2–5 not started. This is a temporary scope contract for hardening the existing room chat. Update the checkboxes and decision log as work lands, then delete this file when the work is complete. Do not add link previews as part of this effort. @@ -88,39 +90,39 @@ tests. Values are starting points, not a compatibility contract. **Target:** downstream layers never receive or retain an unexpectedly large or unsafe chat string. -- [ ] Move chat constants and `sanitize_chat` from `src/app/mod.rs` into +- [x] Move chat constants and `sanitize_chat` from `src/app/mod.rs` into `src/sanitize.rs` (or a narrowly scoped shared chat-policy module if that keeps the API clearer). -- [ ] Implement a single-pass sanitizer that: +- [x] Implement a single-pass sanitizer that: - maps control characters to spaces; - collapses whitespace and trims ends; - enforces both the character and UTF-8 byte ceilings without splitting a scalar; - returns empty for content with no visible text. -- [ ] Add `cap_chat_input` for live editing. It must preserve the user's current +- [x] Add `cap_chat_input` for live editing. It must preserve the user's current whitespace while enforcing character and byte ceilings; normalization remains a submit/ingress operation so typing does not visibly jump. -- [ ] Apply `cap_chat_input` in `AppMessage::ChatInputChanged`, covering keyboard, +- [x] Apply `cap_chat_input` in `AppMessage::ChatInputChanged`, covering keyboard, clipboard, primary-selection, and context-menu paste paths through the controlled input widget. -- [ ] Sanitize outgoing text immediately before local echo and `CoreCommand` send. -- [ ] Sanitize again before `GossipMessage::Chat` is signed, so a future non-UI +- [x] Sanitize outgoing text immediately before local echo and `CoreCommand` send. +- [x] Sanitize again before `GossipMessage::Chat` is signed, so a future non-UI caller cannot bypass policy. -- [ ] At gossip ingress, reject raw chat text over the byte ceiling before doing +- [x] At gossip ingress, reject raw chat text over the byte ceiling before doing downstream sanitization; sanitize accepted text before creating `RoomEvent`. -- [ ] Keep attachment-only messages when the sanitized caption is empty; drop a +- [x] Keep attachment-only messages when the sanitized caption is empty; drop a chat with neither visible text nor a valid attachment. -- [ ] Stop sanitizing an incoming chat `name` with the body sanitizer. Phase 2 +- [x] Stop sanitizing an incoming chat `name` with the body sanitizer. Phase 2 replaces it with the roster-bound name. ### Phase 1 tests -- [ ] ASCII, multibyte Unicode, emoji, whitespace, NUL/CR/LF/TAB/ESC, empty input. -- [ ] Exact character and byte boundaries, including a four-byte scalar at the +- [x] ASCII, multibyte Unicode, emoji, whitespace, NUL/CR/LF/TAB/ESC, empty input. +- [x] Exact character and byte boundaries, including a four-byte scalar at the cutoff. -- [ ] Oversized paste never makes `state.chat_input` exceed either ceiling. -- [ ] Outgoing, incoming, and direct core/network paths converge on the same +- [x] Oversized paste never makes `state.chat_input` exceed either ceiling. +- [x] Outgoing, incoming, and direct core/network paths converge on the same normalized result. -- [ ] Empty captions are retained only when a valid attachment remains. +- [x] Empty captions are retained only when a valid attachment remains. ## Phase 2 — Admission, identity binding, replay, and spam control @@ -383,3 +385,15 @@ The plan is complete when: privacy-oriented design. - **2026-07-15:** Initial scope keeps all wire formats stable; hardening is local admission, validation, resource accounting, and honest UI state. +- **2026-07-17 (Phase 1):** The 8 KiB byte ceiling deliberately cannot bind on + *sanitized* output (2,000 scalars × 4 bytes = 8,000 ≤ 8,192), so inside + `sanitize_chat`/`cap_chat_input` it is defense in depth; its operative role is + the raw-ingress reject in `admit_chat_text`. +- **2026-07-17 (Phase 1):** Interim until Phase 2's roster binding: the incoming + chat `name` now goes through the strict `sanitize_name` label sanitizer at the + UI edge (was the body sanitizer), so author labels already get bidi/zero-width + stripping and the 48-char label cap. +- **2026-07-17 (Phase 1):** `send_chat` at the gossip sign point silently no-ops + (Ok) on an empty-after-sanitize body with no attachment rather than erroring; + the UI already prevents this case, and Phase 5's send-status work is where + send-path feedback gets designed. diff --git a/src/app/mod.rs b/src/app/mod.rs index 8b1b641..9075a5b 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1996,12 +1996,18 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { attachment, } => { // Incoming peer content is untrusted — sanitize name + text. + // Both were already sanitized at gossip ingress; this UI-edge + // pass is defense in depth (both sanitizers are idempotent). // (The attachment filename was already sanitized in core.) - let text = sanitize_chat(&text); + let text = crate::sanitize::sanitize_chat(&text); // Keep the message if it has visible text OR an attachment (an // image with no caption is still a real message). if !text.is_empty() || attachment.is_some() { - let name = sanitize_chat(&name); + // The author label is a spoof-sensitive short label, not + // body text — use the strict name sanitizer (Phase 2 of + // the chat-hardening plan replaces the wire name with the + // roster-bound name entirely). + let name = crate::sanitize::sanitize_name(&name); push_chat( &mut state.chat_messages, ChatEntry { @@ -2578,7 +2584,10 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { .send(CoreCommand::SetRecording(!state.recording)); } AppMessage::ChatInputChanged(val) => { - state.chat_input = val; + // Cap (never normalize) while editing, so an oversized paste can't + // grow the retained input past the chat ceilings but the visible + // text doesn't jump under the user's cursor. + state.chat_input = crate::sanitize::cap_chat_input(val); } AppMessage::DividerDragged(kind, delta) => { // Apply live; the final position is persisted on drag release (the @@ -2906,7 +2915,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { state.playlist_drawer_open = !state.playlist_drawer_open; } AppMessage::ChatSubmit => { - let text = sanitize_chat(&state.chat_input); + let text = crate::sanitize::sanitize_chat(&state.chat_input); if !text.is_empty() { // Local echo (gossip suppresses our own author, so it won't come back). push_chat( @@ -3580,24 +3589,6 @@ fn short_id(id: &str) -> String { id.chars().take(8).collect() } -/// Max characters kept for a single chat message after sanitizing. -const CHAT_MSG_MAX_CHARS: usize = 2000; - -/// Sanitize a chat string for display, applied to BOTH our outgoing text and -/// incoming text from peers (peer input is untrusted — a buggy/malicious sender -/// could include control characters or an enormous payload). Drops control -/// characters (ANSI escapes, NUL, stray CR/LF/TAB), collapses any whitespace run -/// to a single space, trims the ends, and caps the length. Returns "" for input -/// that is empty after cleaning (the caller skips empty messages). -fn sanitize_chat(input: &str) -> String { - let no_control: String = input - .chars() - .map(|c| if c.is_control() { ' ' } else { c }) - .collect(); - let collapsed = no_control.split_whitespace().collect::>().join(" "); - collapsed.chars().take(CHAT_MSG_MAX_CHARS).collect() -} - /// Append a chat line, trimming the oldest once history exceeds the cap so a long /// call can't grow the buffer without bound. fn push_chat(messages: &mut Vec, entry: ChatEntry) { @@ -9839,27 +9830,24 @@ mod tests { assert_eq!(clamp_chat_height(window_h, window_h), window_h - 300.0); } - #[test] - fn sanitize_chat_strips_control_chars_and_collapses_whitespace() { - use super::sanitize_chat; - // Plain text is unchanged. - assert_eq!(sanitize_chat("hello world"), "hello world"); - // Leading/trailing whitespace trimmed; interior runs collapsed. - assert_eq!(sanitize_chat(" hi there "), "hi there"); - // Control chars (NUL, CR, LF, TAB, ANSI ESC) become spaces, then collapse. - // The ESC of an ANSI sequence is stripped; the printable "[31m" remains inert text. - assert_eq!(sanitize_chat("a\u{0}b\r\nc\td\u{1b}[31m"), "a b c d [31m"); - // An all-control / all-whitespace message sanitizes to empty. - assert_eq!(sanitize_chat("\u{0}\r\n\t "), ""); - // Unicode/emoji text is preserved. - assert_eq!(sanitize_chat("héllo 🎙 世界"), "héllo 🎙 世界"); - } + // (sanitize_chat's own behavior is tested in `crate::sanitize`, where the + // chat text policy now lives.) #[test] - fn sanitize_chat_caps_length() { - use super::{CHAT_MSG_MAX_CHARS, sanitize_chat}; - let long = "x".repeat(CHAT_MSG_MAX_CHARS + 500); - assert_eq!(sanitize_chat(&long).chars().count(), CHAT_MSG_MAX_CHARS); + fn oversized_paste_never_exceeds_chat_ceilings_in_state() { + use crate::sanitize::{CHAT_MSG_MAX_BYTES, CHAT_MSG_MAX_CHARS}; + let mut state = AppState::default(); + // A paste far past both ceilings, through the real update handler + // (keyboard, clipboard, primary-selection, and context-menu paste all + // land on ChatInputChanged via the controlled input widget). + let huge = "🎮x ".repeat(CHAT_MSG_MAX_CHARS); + let _ = update(&mut state, AppMessage::ChatInputChanged(huge)); + assert!(state.chat_input.chars().count() <= CHAT_MSG_MAX_CHARS); + assert!(state.chat_input.len() <= CHAT_MSG_MAX_BYTES); + // An in-bounds draft is stored untouched — no normalization mid-edit. + let draft = " spaced draft \t".to_string(); + let _ = update(&mut state, AppMessage::ChatInputChanged(draft.clone())); + assert_eq!(state.chat_input, draft); } #[test] diff --git a/src/network/gossip.rs b/src/network/gossip.rs index e11b769..22bb948 100644 --- a/src/network/gossip.rs +++ b/src/network/gossip.rs @@ -819,6 +819,21 @@ impl RoomState for IrohGossipState { a.name = crate::files::sanitize_filename(&a.name); Some(a) }); + // Chat text policy at INGRESS: reject raw text + // over the byte ceiling before spending any + // sanitize work on it (a compliant sender + // sanitizes before signing), and drop a message + // with neither visible text nor an attachment. + let Some(text) = crate::sanitize::admit_chat_text( + &text, + attachment.is_some(), + ) else { + crate::log_msg(&format!( + "Dropped out-of-policy chat from author={:?} (oversized or empty)", + payload.author + )); + continue; + }; let _ = event_tx .send(RoomEvent::ChatMessage { from: payload.author, @@ -966,6 +981,14 @@ impl RoomState for IrohGossipState { text: String, attachment: Option, ) -> Result<(), NetError> { + // Enforce the chat text policy at the SIGN point, not only in the UI, so + // a future non-UI caller can't sign an out-of-policy body (chat-hardening + // plan Phase 1). Idempotent over the UI's own sanitize pass. + let text = crate::sanitize::sanitize_chat(&text); + if text.is_empty() && attachment.is_none() { + // Nothing visible to send — not an error, just nothing to do. + return Ok(()); + } let name = { let guard = self.self_state.lock().unwrap(); match guard.as_ref() { diff --git a/src/sanitize.rs b/src/sanitize.rs index 3002070..49a4660 100644 --- a/src/sanitize.rs +++ b/src/sanitize.rs @@ -3,8 +3,9 @@ //! Peer display names ride the gossip presence plane (`PeerState.name`), which is //! untrusted and spoofable, yet they're rendered directly in the roster. This //! module cleans a name at the gossip ingest point so every downstream consumer -//! gets a safe value (security finding S4). Chat text has its own sanitizer in -//! the UI layer (`app::sanitize_chat`). +//! gets a safe value (security finding S4). Chat text policy ([`sanitize_chat`], +//! [`cap_chat_input`], [`admit_chat_text`]) also lives here so the UI, the gossip +//! sign point, and the gossip ingress all enforce the same ceilings. /// Max characters kept for a peer's display name after sanitizing. Names are /// short labels, so a tight cap both prevents UI/layout/memory abuse and keeps @@ -73,6 +74,87 @@ pub fn sanitize_game_label(input: &str) -> String { out } +/// Max characters kept for a single chat message after sanitizing. +pub const CHAT_MSG_MAX_CHARS: usize = 2000; + +/// Max UTF-8 bytes kept for a single chat message, enforced alongside +/// [`CHAT_MSG_MAX_CHARS`] (2,000 four-byte scalars would otherwise reach 8,000 +/// bytes). This is also the ingress bound: signed peers never produce more, so +/// raw incoming text above it is rejected outright (see [`admit_chat_text`]). +pub const CHAT_MSG_MAX_BYTES: usize = 8 * 1024; + +/// Sanitize a chat message body, applied to BOTH our outgoing text (before local +/// echo, and again at the gossip sign point) and incoming peer text (untrusted — +/// a buggy/malicious sender could include control characters or an enormous +/// payload). Single pass: control characters become spaces, any whitespace run +/// collapses to a single space, the ends are trimmed, and both the character and +/// UTF-8 byte ceilings are enforced without ever splitting a scalar. Message +/// bodies deliberately keep Unicode format characters (ZWJ/ZWNJ etc.) that the +/// short-label sanitizers strip — chat is expressive text, not a label. Returns +/// `""` for input with no visible text (callers drop empty messages). Idempotent, +/// so layered application converges on the same result. +pub fn sanitize_chat(input: &str) -> String { + let mut out = String::new(); + let mut chars = 0usize; + let mut pending_space = false; + for c in input.chars() { + let c = if c.is_control() { ' ' } else { c }; + if c.is_whitespace() { + // Trim: only mark a separator once visible text exists; a trailing + // run is never emitted because the space lands with the NEXT char. + pending_space = !out.is_empty(); + continue; + } + let sep = usize::from(pending_space); + if chars + sep + 1 > CHAT_MSG_MAX_CHARS + || out.len() + sep + c.len_utf8() > CHAT_MSG_MAX_BYTES + { + break; + } + if pending_space { + out.push(' '); + chars += 1; + pending_space = false; + } + out.push(c); + chars += 1; + } + out +} + +/// Cap the LIVE chat-input text (typing, clipboard/primary-selection paste, +/// context-menu paste) at the chat ceilings. Unlike [`sanitize_chat`] this +/// preserves the user's whitespace exactly — normalization stays a submit-time +/// operation so the visible text never jumps while editing — and only truncates, +/// always on a scalar boundary. Returns the input unchanged when within bounds. +pub fn cap_chat_input(input: String) -> String { + if input.len() <= CHAT_MSG_MAX_BYTES && input.chars().count() <= CHAT_MSG_MAX_CHARS { + return input; + } + let mut out = String::new(); + for (chars, c) in input.chars().enumerate() { + if chars >= CHAT_MSG_MAX_CHARS || out.len() + c.len_utf8() > CHAT_MSG_MAX_BYTES { + break; + } + out.push(c); + } + out +} + +/// Gossip-ingress admission for an untrusted incoming chat body. `None` drops +/// the message: raw text over the byte ceiling is rejected BEFORE any +/// sanitization work (a compliant sender sanitizes before signing, so oversized +/// text is a protocol violation, not something to repair), and a message with +/// neither visible text nor an attachment carries nothing to show. Otherwise +/// yields the sanitized (possibly empty, attachment-only) body to forward. +pub fn admit_chat_text(raw: &str, has_attachment: bool) -> Option { + if raw.len() > CHAT_MSG_MAX_BYTES { + return None; + } + let text = sanitize_chat(raw); + (!text.is_empty() || has_attachment).then_some(text) +} + /// A piece of a chat message after URL detection: literal text or a link. #[derive(Debug, PartialEq, Eq, Clone)] pub enum Segment { @@ -215,6 +297,125 @@ mod tests { assert_eq!(sanitize_game_label("\u{0}\r\n\t "), ""); } + // --- chat body policy --------------------------------------------------- + + #[test] + fn chat_keeps_ordinary_text_and_unicode() { + assert_eq!(sanitize_chat("hello world"), "hello world"); + assert_eq!(sanitize_chat("héllo 🎙 世界"), "héllo 🎙 世界"); + // Bodies keep format characters that label sanitizers strip: a ZWJ emoji + // family sequence survives intact. + let family = "👨\u{200D}👩\u{200D}👧"; + assert_eq!(sanitize_chat(family), family); + } + + #[test] + fn chat_strips_control_chars_and_collapses_whitespace() { + assert_eq!(sanitize_chat(" hi there "), "hi there"); + assert_eq!(sanitize_chat("a\u{0}b\r\nc\td\u{1b}[31m"), "a b c d [31m"); + assert_eq!(sanitize_chat("\u{0}\r\n\t "), ""); + assert_eq!(sanitize_chat(""), ""); + } + + #[test] + fn chat_caps_chars_at_exact_boundary_without_trailing_space() { + let long = "x".repeat(CHAT_MSG_MAX_CHARS + 500); + assert_eq!(sanitize_chat(&long).chars().count(), CHAT_MSG_MAX_CHARS); + assert_eq!( + sanitize_chat(&"x".repeat(CHAT_MSG_MAX_CHARS)) + .chars() + .count(), + CHAT_MSG_MAX_CHARS + ); + // Truncation never leaves a dangling separator: with "word " units the + // cut lands mid-run, and the output still ends on visible text. + let words = "word ".repeat(1000); + let out = sanitize_chat(&words); + assert!(out.chars().count() <= CHAT_MSG_MAX_CHARS); + assert!(!out.ends_with(' ')); + } + + #[test] + fn chat_ceilings_never_split_a_scalar() { + // Four-byte scalars: the char cap bites first (2,000 × 4 = 8,000 bytes, + // inside the byte ceiling by design) and the last emoji is kept whole. + let emoji = "🎮".repeat(CHAT_MSG_MAX_CHARS + 100); + let out = sanitize_chat(&emoji); + assert_eq!(out.chars().count(), CHAT_MSG_MAX_CHARS); + assert!(out.len() <= CHAT_MSG_MAX_BYTES); + assert!(out.chars().all(|c| c == '🎮')); + // Three-byte scalars at the char boundary. + let cjk = "世".repeat(CHAT_MSG_MAX_CHARS + 1); + let out = sanitize_chat(&cjk); + assert_eq!(out.chars().count(), CHAT_MSG_MAX_CHARS); + assert!(out.is_char_boundary(out.len())); + } + + #[test] + fn chat_sanitize_is_idempotent() { + for input in [ + "plain text", + " spaced \t out\r\n text ", + "unicode 🎙 世界 👨\u{200D}👩\u{200D}👧", + &"word ".repeat(1000), + &"🎮".repeat(CHAT_MSG_MAX_CHARS + 100), + ] { + let once = sanitize_chat(input); + assert_eq!(sanitize_chat(&once), once, "not idempotent for {input:?}"); + } + } + + #[test] + fn cap_chat_input_preserves_whitespace_within_bounds() { + // In-bounds input comes back byte-identical — no normalization while + // the user is still editing. + let draft = " hello world \t ".to_string(); + assert_eq!(cap_chat_input(draft.clone()), draft); + } + + #[test] + fn cap_chat_input_truncates_oversized_paste_on_scalar_boundary() { + let paste = "x".repeat(CHAT_MSG_MAX_CHARS + 5000); + let out = cap_chat_input(paste); + assert_eq!(out.chars().count(), CHAT_MSG_MAX_CHARS); + let emoji_paste = "🎮".repeat(CHAT_MSG_MAX_CHARS + 100); + let out = cap_chat_input(emoji_paste); + assert_eq!(out.chars().count(), CHAT_MSG_MAX_CHARS); + assert!(out.len() <= CHAT_MSG_MAX_BYTES); + assert!(out.chars().all(|c| c == '🎮')); + } + + #[test] + fn admit_rejects_oversized_raw_bytes_before_sanitizing() { + // One byte over the ceiling → rejected outright, attachment or not. + let over = "x".repeat(CHAT_MSG_MAX_BYTES + 1); + assert_eq!(admit_chat_text(&over, false), None); + assert_eq!(admit_chat_text(&over, true), None); + // Exactly at the ceiling → admitted (then sanitized/capped). + let at = "x".repeat(CHAT_MSG_MAX_BYTES); + let admitted = admit_chat_text(&at, false).expect("at-ceiling text admitted"); + assert_eq!(admitted.chars().count(), CHAT_MSG_MAX_CHARS); + // Multibyte raw over the ceiling → rejected. + let cjk_over = "世".repeat(CHAT_MSG_MAX_BYTES / 3 + 1); + assert!(cjk_over.len() > CHAT_MSG_MAX_BYTES); + assert_eq!(admit_chat_text(&cjk_over, false), None); + } + + #[test] + fn admit_keeps_attachment_only_messages_and_drops_truly_empty_ones() { + // No visible text + no attachment → nothing to show, dropped. + assert_eq!(admit_chat_text("", false), None); + assert_eq!(admit_chat_text("\u{0}\r\n\t ", false), None); + // Same bodies WITH an attachment → kept as an empty caption. + assert_eq!(admit_chat_text("", true), Some(String::new())); + assert_eq!(admit_chat_text("\u{0}\r\n\t ", true), Some(String::new())); + // Normal text converges on the same result as direct sanitization. + assert_eq!( + admit_chat_text(" hi there ", false), + Some(sanitize_chat(" hi there ")) + ); + } + // --- linkify ----------------------------------------------------------- /// Concatenating every segment's inner text must reproduce the input exactly. diff --git a/src/widget/selectable_text.rs b/src/widget/selectable_text.rs index b3600fa..d49d91a 100644 --- a/src/widget/selectable_text.rs +++ b/src/widget/selectable_text.rs @@ -21,7 +21,7 @@ const HIT_SEARCH_STEPS: usize = 24; // `Hit::CharOffset(cursor.index)`, and cosmic-text's `cursor.index` is a byte // offset WITHIN its buffer line — it discards the line number. That equals the // global byte offset only when the text is a single logical line. Chat bodies -// satisfy this because `app::sanitize_chat` turns every control char (incl. `\n` +// satisfy this because `sanitize::sanitize_chat` turns every control char (incl. `\n` // and `\r`) into a space and collapses whitespace, so a stored message can never // contain a newline. If that sanitizer ever starts preserving newlines, this // widget's per-line offsets would stop being global and selection/copy across