diff --git a/Cargo.lock b/Cargo.lock index 781c6f4..d5dc2cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4894,6 +4894,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "tokio-stream", + "url", "windows-sys 0.61.2", ] diff --git a/Cargo.toml b/Cargo.toml index a03bdd9..1456a12 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,6 +75,10 @@ serde_json = "1.0.150" thiserror = "2.0.18" tokio = { version = "1.52.3", features = ["full"] } tokio-stream = "0.1.18" +# Chat link policy: parse + validate clickable URL candidates (scheme/host/ +# userinfo checks in `sanitize::is_safe_web_url`). Already in the tree +# transitively via iroh — this only promotes it to a direct dependency. +url = "2.5" # --- Platform-specific dependencies ----------------------------------------- # Audio and the native file-picker backends differ per OS. Everything else in the diff --git a/docs/chat-hardening-plan.md b/docs/chat-hardening-plan.md index e057aba..a306276 100644 --- a/docs/chat-hardening-plan.md +++ b/docs/chat-hardening-plan.md @@ -1,15 +1,18 @@ # Chat hardening — ephemeral implementation plan -**Status (2026-07-18):** Phases 1–3 COMPLETE. Phase 1 = shared text policy in +**Status (2026-07-18):** Phases 1–4 COMPLETE. Phase 1 = shared text policy in `src/sanitize.rs`, ceilings enforced at UI input, sign point, and gossip ingress. Phase 2 = roster-bound authorship (`src/core/chatroster.rs`), replay dedup + rate limits (`ChatIngressGate` in `src/network/gossip.rs`). Phase 3 = attachment cache/serve-store budgets, downscaled previews, auto-fetch byte/request budgets -(`src/core/fetchbudget.rs`), exact transfers, bounded local reads. All gates -green each phase. Phases 4–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. +(`src/core/fetchbudget.rs`), exact transfers, bounded local reads. Phase 4 = +parsed-URL link policy (`is_safe_web_url`/`link_ranges` in `src/sanitize.rs`, +`url` crate), 8-link cap, cached link ranges in `ChatEntry`, 512 KiB history +text budget, chat-body bidi-override strip (closes S14). All gates green each +phase. Phase 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. ## Goal @@ -257,31 +260,34 @@ unbounded memory, bandwidth, decoder, or task pressure. **Target:** keep clickable links without making malformed/deceptive input or many small spans an unnecessary UI/launcher surface. -- [ ] Make `url` a direct dependency (already present transitively) and validate +- [x] Make `url` a direct dependency (already present transitively) and validate link candidates with `url::Url`. -- [ ] A clickable URL must have an `http` or `https` scheme and a valid host. -- [ ] Treat URLs containing username/password syntax as plain text, or require an +- [x] A clickable URL must have an `http` or `https` scheme and a valid host. +- [x] Treat URLs containing username/password syntax as plain text, or require an explicit confirmation that shows the parsed destination host. Prefer plain text for the first implementation. -- [ ] Preserve the existing defense-in-depth validation in `AppMessage::OpenUrl`; +- [x] Preserve the existing defense-in-depth validation in `AppMessage::OpenUrl`; replace prefix checks with the shared parsed-URL policy. -- [ ] Cap clickable candidates at eight per message. Remaining content stays +- [x] Cap clickable candidates at eight per message. Remaining content stays selectable plain text and must still round-trip exactly. -- [ ] Refactor linkification to return borrowed ranges/offsets or cache link ranges +- [x] Refactor linkification to return borrowed ranges/offsets or cache link ranges in `ChatEntry`, avoiding allocation and rescanning on every redraw. -- [ ] Bound retained history by total sanitized text bytes as well as 300 entries. +- [x] Bound retained history by total sanitized text bytes as well as 300 entries. Eviction must keep attachment bookkeeping coherent and should not invalidate an open Save/Play operation. -- [ ] Do not add metadata fetching, remote images, Markdown, or link previews. +- [x] Do not add metadata fetching, remote images, Markdown, or link previews. +- [x] (Folded in from S14, per the security handoff) Strip bidi + overrides/isolates from the chat BODY in `sanitize_chat`, keeping the other + expressive format characters (ZWJ/ZWNJ/LRM/RLM). ### Phase 4 tests -- [ ] Valid HTTP/HTTPS, malformed host, empty host, mixed case, Unicode path/query, +- [x] Valid HTTP/HTTPS, malformed host, empty host, mixed case, Unicode path/query, punctuation, credentials/userinfo, and non-web schemes. -- [ ] Eight-link boundary and many-link adversarial input. -- [ ] Segment/range reconstruction exactly reproduces the sanitized message. -- [ ] Entry-count and total-text-budget history eviction. -- [ ] Opener policy cannot launch a non-web scheme even if called directly. +- [x] Eight-link boundary and many-link adversarial input. +- [x] Segment/range reconstruction exactly reproduces the sanitized message. +- [x] Entry-count and total-text-budget history eviction. +- [x] Opener policy cannot launch a non-web scheme even if called directly. ## Phase 5 — Honest local send status @@ -484,3 +490,31 @@ The plan is complete when: explicit length compare; short transfers get the explicit `len == declared_size` check. Music fetches ride `fetch_blob`, so they inherit exactness for free. +- **2026-07-18 (Phase 4):** The S14 chat-body half (bidi strip) landed here per + the security handoff: `sanitize_chat` strips ONLY bidi overrides/isolates + (U+202A–202E, U+2066–2069) — the characters that can visually reorder a + rendered line — while ZWJ/ZWNJ (emoji sequences, joining scripts) and the + LRM/RLM direction *marks* (which cannot reorder) are kept. Labels/filenames + keep the stricter full-format-strip. +- **2026-07-18 (Phase 4):** A link's href is the exact displayed slice of the + message — validation is parse-only, no normalization on open — so what the + user sees IS the argv the opener receives. Consequence: WHATWG slash + collapsing means `http:///path` parses to host `path` (as in browsers) and is + accepted; the empty-host rejects are `http://` and friends that fail parsing. +- **2026-07-18 (Phase 4):** URLs with userinfo syntax went the plan-preferred + plain-text route (no confirmation dialog). A candidate that fails the policy + leaves its WHOLE whitespace-delimited run as plain text without re-scanning + the interior — `http://a@http://b.com` yields zero links, by design. +- **2026-07-18 (Phase 4):** Scheme detection became ASCII-case-insensitive + (`Http://…` from sentence auto-capitalization now linkifies); the policy + check is unaffected since `url` normalizes scheme/host case during parsing. +- **2026-07-18 (Phase 4):** Cached ranges in `ChatEntry.links`, filled inside + `push_chat` (the single history choke point), were chosen over + borrowed-return-per-redraw: redraws now slice cached char-boundary ranges, + and only link spans allocate (their href String). +- **2026-07-18 (Phase 4):** History byte-budget eviction (512 KiB, alongside + the 300-entry cap) deliberately does NOT touch the attachment byte cache: + that cache is bounded by its own Phase 3 budgets, and leaving it alone means + an open Save/Play on an evicted line keeps its bytes-in-hand (the save + dialog falls back to the generic "download" name). The just-pushed entry is + never evicted; a single message's 8 KiB ceiling cannot exceed the budget. diff --git a/src/app/mod.rs b/src/app/mod.rs index 1187bde..e40a29c 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -319,6 +319,11 @@ struct ChatEntry { /// `AppState.attachments` keyed by `(author, attachment.id)`; the entry only /// holds the descriptor so history stays cheap. attachment: Option, + /// Byte ranges of `text`'s validated clickable links, computed ONCE by + /// [`push_chat`] (construction sites leave it empty) so each redraw slices + /// cached ranges instead of rescanning and re-validating every message. + /// At most [`crate::sanitize::CHAT_MSG_MAX_LINKS`] entries. + links: Vec>, } /// Fetch state of a chat attachment's bytes (session-only). Absence from the @@ -564,6 +569,12 @@ fn build_preview_handle(bytes: &[u8]) -> Option { /// Cap on retained chat history so a long call can't grow it without bound. const CHAT_HISTORY_MAX: usize = 300; +/// Cap on TOTAL retained sanitized chat text bytes, enforced alongside +/// [`CHAT_HISTORY_MAX`] (300 ceiling-length messages would otherwise retain +/// ~2.4 MiB and make every redraw/selection walk it). The entry cap bounds the +/// common case; this bounds a max-length-message flood (chat-hardening Phase 4). +const CHAT_HISTORY_MAX_TEXT_BYTES: usize = 512 * 1024; + /// Which room-screen divider a drag is resizing. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum DividerKind { @@ -2157,6 +2168,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { mine: false, from: Some(from), attachment, + links: Vec::new(), }, ); } @@ -3082,6 +3094,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { mine: true, from: Some(state.self_id.clone()), attachment: None, + links: Vec::new(), }, ); let _ = state.controller.send(CoreCommand::SendChat(text)); @@ -3166,6 +3179,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { mine: true, from: Some(state.self_id.clone()), attachment: Some(att.clone()), + links: Vec::new(), }, ); let _ = state.controller.send(CoreCommand::SendChatFile { @@ -3483,16 +3497,19 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { } } AppMessage::OpenUrl(url) => { - // Defence in depth: only ever hand http(s) URLs to the opener. The - // link span's href came from `linkify`, which only emits http/https, - // but re-check here so this can't be widened into launching arbitrary - // schemes/args. Each opener receives the URL as a single argv entry - // (no shell), so there's no injection surface: + // Defence in depth: only ever hand policy-clean web URLs to the + // opener. The link span's href came from `link_ranges`, which only + // emits candidates passing `is_safe_web_url`, but re-check the SAME + // parsed policy here (http/https scheme + real host + no userinfo) + // so this handler can't be widened into launching arbitrary + // schemes/args even if called directly. Each opener receives the + // URL as a single argv entry (no shell), so there's no injection + // surface: // - Unix: `xdg-open `. // - Windows: `rundll32 url.dll,FileProtocolHandler ` — opens the // default browser without going through `cmd`/`start`, which would // otherwise re-parse `&` in query strings. - if url.starts_with("http://") || url.starts_with("https://") { + if crate::sanitize::is_safe_web_url(&url) { let spawned = { #[cfg(unix)] { @@ -3798,14 +3815,27 @@ fn short_id(id: &str) -> String { id.chars().take(8).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) { +/// Append a chat line, computing its cached link ranges, then trimming the +/// oldest entries while history exceeds EITHER the entry cap or the total +/// text-byte budget so a long call (or a max-length-message flood) can't grow +/// the buffer without bound. The just-pushed entry is never evicted (a single +/// message's 8 KiB ceiling sits far below the byte budget). Eviction only drops +/// `ChatEntry` values: the attachment byte cache in `AppState.attachments` is +/// bounded by its own budgets and is deliberately NOT touched here, so an open +/// Save/Play on an evicted line keeps its bytes-in-hand (the save dialog then +/// falls back to the "download" default name). +fn push_chat(messages: &mut Vec, mut entry: ChatEntry) { + entry.links = crate::sanitize::link_ranges(&entry.text); messages.push(entry); - if messages.len() > CHAT_HISTORY_MAX { - let overflow = messages.len() - CHAT_HISTORY_MAX; - messages.drain(..overflow); + let mut total: usize = messages.iter().map(|m| m.text.len()).sum(); + let mut overflow = 0; + while messages.len() - overflow > CHAT_HISTORY_MAX + || (total > CHAT_HISTORY_MAX_TEXT_BYTES && messages.len() - overflow > 1) + { + total -= messages[overflow].text.len(); + overflow += 1; } + messages.drain(..overflow); } /// Pick the default save-dialog filename for an attachment, matched by the FULL @@ -7275,12 +7305,18 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { let name_color = if m.mine { color_green } else { color_lavender }; // Split the (already-sanitized) message into text + URL spans so // links render clickable and open in the system browser (A13). - let spans: Vec<_> = crate::sanitize::linkify(&m.text) + // The link ranges were validated + cached at push time, so this + // only slices borrowed text — no rescanning per redraw. + let spans: Vec<_> = crate::sanitize::segments(&m.text, &m.links) .into_iter() - .map(|seg| match seg { - crate::sanitize::Segment::Text(t) => span(t).size(13).color(color_text), - crate::sanitize::Segment::Link(u) => { - span(u.clone()).size(13).color(color_blue).link(u) + .map(|(piece, is_link)| { + if is_link { + span(piece) + .size(13) + .color(color_blue) + .link(piece.to_string()) + } else { + span(piece).size(13).color(color_text) } }) .collect(); @@ -9479,6 +9515,7 @@ mod tests { kind: crate::files::AttachmentKind::File, id: shared_id, }), + links: Vec::new(), }; // Attacker's line is FIRST in history, so a bare-id scan would pick it. let messages = vec![mk(attacker, "evil.sh"), mk(victim, "report.pdf")]; @@ -9525,6 +9562,7 @@ mod tests { mine: false, from: Some(peer.to_string()), attachment: None, + links: Vec::new(), }); state.chat_input = "draft".to_string(); let att_key = (peer, attachment_id); @@ -10433,6 +10471,7 @@ mod tests { mine: true, from: None, attachment: None, + links: Vec::new(), }; push_chat(&mut messages, entry); assert_eq!(messages.len(), 1); @@ -10454,6 +10493,7 @@ mod tests { mine: i % 2 == 0, from: None, attachment: None, + links: Vec::new(), }, ); } @@ -10484,6 +10524,7 @@ mod tests { mine: i % 2 == 0, from: None, attachment: None, + links: Vec::new(), }, ); } @@ -10501,4 +10542,60 @@ mod tests { format!("Msg{}", total_pushes - 1) ); } + + #[test] + fn test_push_chat_evicts_on_total_text_bytes() { + use super::{CHAT_HISTORY_MAX_TEXT_BYTES, ChatEntry, push_chat}; + // Ceiling-length messages (8 KiB each) hit the 512 KiB byte budget at 64 + // entries — far below the 300-entry cap. + let big = "x".repeat(crate::sanitize::CHAT_MSG_MAX_BYTES); + let mut messages = Vec::new(); + for i in 0..80 { + push_chat( + &mut messages, + ChatEntry { + name: format!("User{}", i), + text: big.clone(), + mine: false, + from: None, + attachment: None, + links: Vec::new(), + }, + ); + let total: usize = messages.iter().map(|m| m.text.len()).sum(); + assert!(total <= CHAT_HISTORY_MAX_TEXT_BYTES, "over budget at {i}"); + } + assert_eq!( + messages.len(), + CHAT_HISTORY_MAX_TEXT_BYTES / crate::sanitize::CHAT_MSG_MAX_BYTES + ); + // Oldest evicted, newest kept. + assert_eq!(messages[0].name, "User16"); + assert_eq!(messages[messages.len() - 1].name, "User79"); + } + + #[test] + fn test_push_chat_caches_validated_link_ranges() { + use super::{ChatEntry, push_chat}; + let mut messages = Vec::new(); + push_chat( + &mut messages, + ChatEntry { + name: "Alice".to_string(), + text: "see https://ok.org and javascript:alert(1) too".to_string(), + mine: false, + from: None, + attachment: None, + links: Vec::new(), + }, + ); + // Ranges are filled at push time (the construction-site value is + // ignored) and slicing them yields exactly the validated link. + let m = &messages[0]; + assert_eq!(m.links.len(), 1); + assert_eq!(&m.text[m.links[0].clone()], "https://ok.org"); + let pieces = crate::sanitize::segments(&m.text, &m.links); + let rebuilt: String = pieces.iter().map(|(s, _)| *s).collect(); + assert_eq!(rebuilt, m.text); + } } diff --git a/src/sanitize.rs b/src/sanitize.rs index c5500d2..50d5a3d 100644 --- a/src/sanitize.rs +++ b/src/sanitize.rs @@ -12,6 +12,18 @@ /// the roster readable. pub const NAME_MAX_CHARS: usize = 48; +/// Bidirectional override/isolate format characters (`General_Category=Cf`, NOT +/// caught by [`char::is_control`]) that can visually reorder surrounding text. +/// Stripped even from expressive chat bodies (security finding S14): unlike the +/// benign zero-width joiners/marks, these let a sender make rendered text read +/// differently from what was actually sent. +pub(crate) fn is_bidi_override_char(c: char) -> bool { + matches!(c, + '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO (bidi overrides) + | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI (bidi isolates) + ) +} + /// Unicode *format* characters (`General_Category=Cf`) that can spoof or garble a /// rendered name even though they are NOT caught by [`char::is_control`]: /// bidirectional overrides/isolates (text-direction spoofing) and @@ -19,13 +31,12 @@ pub const NAME_MAX_CHARS: usize = 48; /// explicitly so the sanitizer stays dependency-free (std exposes no category /// query). Stripped outright rather than replaced. pub(crate) fn is_spoofing_format_char(c: char) -> bool { - matches!(c, - '\u{200B}'..='\u{200F}' // zero-width space, ZWNJ, ZWJ, LRM, RLM - | '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO (bidi overrides) - | '\u{2060}'..='\u{2064}' // word joiner .. invisible plus - | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI (bidi isolates) - | '\u{FEFF}' // BOM / zero-width no-break space - ) + is_bidi_override_char(c) + || matches!(c, + '\u{200B}'..='\u{200F}' // zero-width space, ZWNJ, ZWJ, LRM, RLM + | '\u{2060}'..='\u{2064}' // word joiner .. invisible plus + | '\u{FEFF}' // BOM / zero-width no-break space + ) } /// Max characters kept for a broadcast game-presence label after sanitizing @@ -86,18 +97,24 @@ 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. +/// payload). Single pass: bidi overrides/isolates are stripped outright (S14 — +/// they can visually reorder the rendered line), 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 the OTHER format +/// characters (ZWJ/ZWNJ/LRM/RLM etc.) that the short-label sanitizers strip — +/// chat is expressive text, not a label, and those are needed for emoji +/// sequences and joining scripts. 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() { + if is_bidi_override_char(c) { + continue; + } let c = if c.is_control() { ' ' } else { c }; if c.is_whitespace() { // Trim: only mark a separator once visible text exists; a trailing @@ -155,14 +172,10 @@ pub fn admit_chat_text(raw: &str, has_attachment: bool) -> Option { (!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 { - /// Plain text to render as-is. - Text(String), - /// A detected URL to render as a clickable link (also its href). - Link(String), -} +/// Max clickable links rendered per chat message. Later URL candidates stay +/// selectable plain text — bounds both the span count a message can force the +/// renderer to build and the opener targets one line can carry. +pub const CHAT_MSG_MAX_LINKS: usize = 8; /// Trailing characters commonly adjacent to a URL in prose that should NOT be /// part of the link (so "see http://x.com." or "(http://x.com)" linkify cleanly). @@ -173,42 +186,85 @@ fn is_url_trailing_punct(c: char) -> bool { ) } -/// Find the byte index of the earliest `http://` or `https://` scheme in `s`, +/// Find the byte index of the earliest `http://` or `https://` scheme in `s` +/// (ASCII-case-insensitive, so a sentence-capitalized "Http://…" still counts), /// scanning only on char boundaries so slicing is always safe. fn find_scheme(s: &str) -> Option { s.char_indices().find_map(|(i, _)| { let tail = &s[i..]; - (tail.starts_with("http://") || tail.starts_with("https://")).then_some(i) + let matches_prefix = |p: &str| { + tail.get(..p.len()) + .is_some_and(|t| t.eq_ignore_ascii_case(p)) + }; + (matches_prefix("http://") || matches_prefix("https://")).then_some(i) }) } -/// Split an (already chat-sanitized) message into plain-text and URL [`Segment`]s -/// for rendering. **Conservative on purpose:** only `http://` / `https://` runs -/// are treated as links, each ending at the first whitespace, with trailing prose -/// punctuation peeled back into the following text. Concatenating every segment's -/// inner string reproduces the input exactly (no characters added or dropped), so -/// it's purely a presentational split. Linkify AFTER sanitizing so control/format -/// chars are already gone (the URL can't smuggle them). Pure → unit-testable. -pub fn linkify(input: &str) -> Vec { +/// The clickable-link policy, shared by link detection ([`link_ranges`]) and the +/// opener's defence-in-depth re-check (`AppMessage::OpenUrl`): the candidate must +/// parse as a URL with an `http`/`https` scheme, a non-empty host, and NO +/// username/password syntax (`http://user@host` reads as a credential but is a +/// classic destination-spoof — such text stays plain, never clickable). +pub fn is_safe_web_url(s: &str) -> bool { + let Ok(u) = url::Url::parse(s) else { + return false; + }; + matches!(u.scheme(), "http" | "https") + && u.host_str().is_some_and(|h| !h.is_empty()) + && u.username().is_empty() + && u.password().is_none() +} + +/// Detect clickable links in an (already chat-sanitized) message, returning the +/// byte range of each — computed ONCE when a message enters history and cached +/// on its entry, so redraws slice instead of rescanning. **Conservative on +/// purpose:** only `http://` / `https://` runs count, each ending at the first +/// whitespace with trailing prose punctuation peeled off, and only candidates +/// passing [`is_safe_web_url`] become links — a failing candidate's whole +/// whitespace-delimited run stays plain text (its interior is not re-scanned). +/// At most [`CHAT_MSG_MAX_LINKS`] ranges; ranges are ascending, non-overlapping, +/// and always on char boundaries. The href is exactly the displayed slice, so +/// what the user sees IS what the opener receives. +pub fn link_ranges(text: &str) -> Vec> { let mut out = Vec::new(); - let mut rest = input; - while !rest.is_empty() { - let Some(start) = find_scheme(rest) else { - out.push(Segment::Text(rest.to_string())); + let mut base = 0usize; + while out.len() < CHAT_MSG_MAX_LINKS { + let Some(start) = find_scheme(&text[base..]) else { break; }; - if start > 0 { - out.push(Segment::Text(rest[..start].to_string())); + let run_start = base + start; + let run = &text[run_start..]; + let run_end = run.find(char::is_whitespace).unwrap_or(run.len()); + // Peel trailing punctuation back out of the candidate; a run is at least + // the 7-byte scheme long, so `base` always advances. + let candidate = run[..run_end].trim_end_matches(is_url_trailing_punct); + if is_safe_web_url(candidate) { + out.push(run_start..run_start + candidate.len()); + base = run_start + candidate.len(); + } else { + base = run_start + run_end; } - let after = &rest[start..]; - let end = after.find(char::is_whitespace).unwrap_or(after.len()); - let candidate = &after[..end]; - // Peel trailing punctuation back out of the link. - let url = candidate.trim_end_matches(is_url_trailing_punct); - out.push(Segment::Link(url.to_string())); - // Continue past just the URL; any peeled punctuation + the rest (incl. the - // whitespace) is reconsidered as ordinary text on the next iteration. - rest = &after[url.len()..]; + } + out +} + +/// Split `text` into `(slice, is_link)` pieces from cached [`link_ranges`] +/// output. Concatenating the slices reproduces `text` exactly (purely a +/// presentational split — no characters added or dropped). Borrows, so a redraw +/// allocates nothing for plain text. `ranges` must come from [`link_ranges`] on +/// this same `text` (ascending, non-overlapping, char-boundary ranges). +pub fn segments<'a>(text: &'a str, ranges: &[std::ops::Range]) -> Vec<(&'a str, bool)> { + let mut out = Vec::new(); + let mut pos = 0usize; + for r in ranges { + if r.start > pos { + out.push((&text[pos..r.start], false)); + } + out.push((&text[r.clone()], true)); + pos = r.end; + } + if pos < text.len() { + out.push((&text[pos..], false)); } out } @@ -416,93 +472,80 @@ mod tests { ); } - // --- linkify ----------------------------------------------------------- + #[test] + fn chat_strips_bidi_overrides_but_keeps_benign_format_chars() { + // Overrides and isolates are removed outright (S14) … + assert_eq!(sanitize_chat("pay \u{202E}gpj.exe now"), "pay gpj.exe now"); + assert_eq!(sanitize_chat("a\u{2066}b\u{2069}c"), "abc"); + assert_eq!( + sanitize_chat("\u{202A}\u{202B}\u{202C}\u{202D}\u{202E}"), + "" + ); + // … while the expressive format characters chat promises to keep — ZWJ + // (emoji sequences), ZWNJ (joining scripts), LRM/RLM (bidi *marks*, which + // cannot reorder text) — survive. + for kept in ['\u{200D}', '\u{200C}', '\u{200E}', '\u{200F}'] { + let msg = format!("a{kept}b"); + assert_eq!(sanitize_chat(&msg), msg, "stripped benign {kept:?}"); + } + } - /// Concatenating every segment's inner text must reproduce the input exactly. - fn reassemble(segs: &[Segment]) -> String { - segs.iter() - .map(|s| match s { - Segment::Text(t) | Segment::Link(t) => t.as_str(), - }) + // --- link policy --------------------------------------------------------- + + /// Concatenating every segment's slice must reproduce the input exactly. + fn reassemble(text: &str) -> String { + segments(text, &link_ranges(text)) + .iter() + .map(|(s, _)| *s) + .collect() + } + + /// The link slices of a message, in order. + fn links(text: &str) -> Vec<&str> { + segments(text, &link_ranges(text)) + .into_iter() + .filter_map(|(s, is_link)| is_link.then_some(s)) .collect() } #[test] - fn linkify_plain_text_has_no_links() { - let segs = linkify("just a normal message, nothing here"); - assert_eq!( - segs, - vec![Segment::Text("just a normal message, nothing here".into())] - ); + fn url_policy_accepts_only_wellformed_web_urls() { + for ok in [ + "http://example.com", + "https://a.test/path?q=1&w=2", + "HTTP://EXAMPLE.COM", // mixed case scheme+host + "https://x.com:8443/p", // explicit port + "https://d.com/路径?q=世界#frag", // unicode path/query/fragment + // WHATWG parsing (what browsers do) collapses the extra slash into + // host "path" — a valid, if odd, destination; not an empty host. + "http:///path", + ] { + assert!(is_safe_web_url(ok), "rejected {ok:?}"); + } + for bad in [ + "", + "example.com", // no scheme + "http://", // empty host + "ftp://x.com", // non-web scheme + "file:///etc/passwd", // no host, wrong scheme + "javascript:alert(1)", // opener must never see this + "http://user@good.com", // userinfo → destination spoof risk + "http://user:pw@good.com", // credentials + "http://exa mple.com", // malformed host + ] { + assert!(!is_safe_web_url(bad), "accepted {bad:?}"); + } } #[test] - fn linkify_detects_http_and_https() { + fn link_ranges_detects_http_and_https_with_exact_roundtrip() { + assert_eq!(links("see http://example.com now"), ["http://example.com"]); assert_eq!( - linkify("see http://example.com now"), - vec![ - Segment::Text("see ".into()), - Segment::Link("http://example.com".into()), - Segment::Text(" now".into()), - ] + links("a http://one.com b https://two.com c"), + ["http://one.com", "https://two.com"] ); - assert_eq!( - linkify("https://a.test/path?q=1"), - vec![Segment::Link("https://a.test/path?q=1".into())] - ); - } - - #[test] - fn linkify_peels_trailing_punctuation() { - // Sentence-final period is not part of the link. - assert_eq!( - linkify("go to https://x.com."), - vec![ - Segment::Text("go to ".into()), - Segment::Link("https://x.com".into()), - Segment::Text(".".into()), - ] - ); - // Parenthesized URL. - assert_eq!( - linkify("(https://x.com)"), - vec![ - Segment::Text("(".into()), - Segment::Link("https://x.com".into()), - Segment::Text(")".into()), - ] - ); - } - - #[test] - fn linkify_handles_multiple_urls() { - let segs = linkify("a http://one.com b https://two.com c"); - assert_eq!( - segs, - vec![ - Segment::Text("a ".into()), - Segment::Link("http://one.com".into()), - Segment::Text(" b ".into()), - Segment::Link("https://two.com".into()), - Segment::Text(" c".into()), - ] - ); - } - - #[test] - fn linkify_only_matches_http_schemes() { - // Non-web schemes and bare domains are NOT linkified (conservative). - let segs = linkify("email me@x.com or ftp://x.com or visit x.com"); - assert_eq!( - segs, - vec![Segment::Text( - "email me@x.com or ftp://x.com or visit x.com".into() - )] - ); - } - - #[test] - fn linkify_preserves_input_exactly() { + // Sentence-capitalized scheme still detected; href = the displayed slice. + assert_eq!(links("go to Http://example.com"), ["Http://example.com"]); for msg in [ "", "no urls at all", @@ -510,12 +553,67 @@ mod tests { "pre http://a.com/x?y=z&w=1 mid https://b.org/p, end!", "weird))) http://c.com]]] tail", "unicode 世界 http://d.com/路径 more 世界", + "bad http:// and http://user@x.com around https://ok.org here", ] { - assert_eq!( - reassemble(&linkify(msg)), - msg, - "roundtrip failed for {msg:?}" - ); + assert_eq!(reassemble(msg), msg, "roundtrip failed for {msg:?}"); } } + + #[test] + fn link_ranges_peels_trailing_punctuation() { + assert_eq!(links("go to https://x.com."), ["https://x.com"]); + assert_eq!(links("(https://x.com)"), ["https://x.com"]); + } + + #[test] + fn link_ranges_leaves_invalid_candidates_as_plain_text() { + // Non-web schemes and bare domains never linkify (conservative). + assert_eq!( + links("email me@x.com or ftp://x.com or visit x.com"), + [] as [&str; 0] + ); + // A malformed/deceptive candidate stays text WITHOUT eating a later + // valid link. + assert_eq!(links("http:// then https://ok.org"), ["https://ok.org"]); + assert_eq!( + links("http://user:pw@evil.com vs https://good.com"), + ["https://good.com"] + ); + // An invalid run's interior is not re-scanned for nested schemes. + assert_eq!(links("http://a@http://b.com"), [] as [&str; 0]); + } + + #[test] + fn link_ranges_caps_clickable_links_per_message() { + let many = (0..CHAT_MSG_MAX_LINKS + 4) + .map(|i| format!("https://site{i}.test")) + .collect::>() + .join(" "); + let ranges = link_ranges(&many); + assert_eq!(ranges.len(), CHAT_MSG_MAX_LINKS); + // The 9th+ URLs remain, but as plain selectable text. + assert_eq!(reassemble(&many), many); + let l = links(&many); + assert_eq!(l.last(), Some(&"https://site7.test")); + // Exactly at the cap: all clickable. + let at_cap = (0..CHAT_MSG_MAX_LINKS) + .map(|i| format!("https://site{i}.test")) + .collect::>() + .join(" "); + assert_eq!(link_ranges(&at_cap).len(), CHAT_MSG_MAX_LINKS); + } + + #[test] + fn link_ranges_survives_adversarial_many_link_input() { + // A ceiling-length message packed with minimal URLs: bounded output, + // exact reconstruction, and every range on char boundaries. + let flood = "http://a.io ".repeat(CHAT_MSG_MAX_BYTES / 12 + 1); + let msg = sanitize_chat(&flood); + let ranges = link_ranges(&msg); + assert_eq!(ranges.len(), CHAT_MSG_MAX_LINKS); + for r in &ranges { + assert!(msg.is_char_boundary(r.start) && msg.is_char_boundary(r.end)); + } + assert_eq!(reassemble(&msg), msg); + } }