chat: parsed-URL link policy, cached link ranges, history byte budget (Phase 4)
CI / check (push) Failing after 3m10s
CI / check (push) Failing after 3m10s
Phase 4 of docs/chat-hardening-plan.md — URL and rendering resilience. Closes the chat-body half of S14 (bidi override strip). - sanitize: new is_safe_web_url shared link policy (url crate, promoted to a direct dependency): http/https scheme + non-empty host + no userinfo; candidates failing it stay plain text (their whole whitespace run, interior not re-scanned). Scheme detection is now ASCII-case-insensitive. - sanitize: linkify() -> link_ranges()/segments(): validated byte ranges computed once, exact-roundtrip slicing, at most CHAT_MSG_MAX_LINKS (8) clickable links per message; the rest stays selectable plain text. - sanitize_chat: strips bidi overrides/isolates (U+202A-202E, U+2066-2069) from message bodies while keeping ZWJ/ZWNJ/LRM/RLM (S14 chat-body half). - app: ChatEntry caches its link ranges (filled in push_chat), so redraws slice instead of rescanning/re-validating; only link spans allocate. - app: chat history now also bounded by 512 KiB total sanitized text (CHAT_HISTORY_MAX_TEXT_BYTES) alongside the 300-entry cap; the attachment byte cache is deliberately untouched by history eviction (own budgets). - app: AppMessage::OpenUrl re-checks the same parsed policy (defence in depth) instead of prefix checks - non-web schemes can never reach the opener even if the handler is invoked directly. 571 lib tests green (+3 net); clippy -D warnings + fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+114
-17
@@ -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<crate::files::ChatAttachment>,
|
||||
/// 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<std::ops::Range<usize>>,
|
||||
}
|
||||
|
||||
/// Fetch state of a chat attachment's bytes (session-only). Absence from the
|
||||
@@ -564,6 +569,12 @@ fn build_preview_handle(bytes: &[u8]) -> Option<PreviewHandle> {
|
||||
/// 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<AppMessage> {
|
||||
mine: false,
|
||||
from: Some(from),
|
||||
attachment,
|
||||
links: Vec::new(),
|
||||
},
|
||||
);
|
||||
}
|
||||
@@ -3082,6 +3094,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task<AppMessage> {
|
||||
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<AppMessage> {
|
||||
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> {
|
||||
}
|
||||
}
|
||||
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 <url>`.
|
||||
// - Windows: `rundll32 url.dll,FileProtocolHandler <url>` — 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<ChatEntry>, 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<ChatEntry>, 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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user