From 381e00bc0efe2e6f5ed6e472e128b41dd44d9e24 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 26 Jun 2026 15:58:26 -0400 Subject: [PATCH] fix(security): bound + author-key chat attachment cache (Tier C F-02/F-12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chat-attachment result cache (`attachment_data` + `image_handle_cache`) was keyed by attachment id alone and only cleared on room-leave, so an authenticated insider could (F-02) stream distinct attachments to grow it without bound, and (F-12) reuse a victim's attachment id to alias displayed/ saved bytes — the id is attacker-chosen, so a signature only proves keypair ownership, not a distinct human. F-12: thread the author (`from: EndpointId`) back through the `AttachmentReady`/`AttachmentFailed` core→UI events (the fetch task already holds it) and key all attachment result state on `(author, id)`: - new `AttachmentKey = (EndpointId, AttachmentId)`; - `attachment_data` + `image_handle_cache` fold into one `AttachmentCache`; - `pending_saves` and the `SaveAttachment`/`PlayAudio` messages re-keyed, so the save/fetch dispatch can't be redirected to the wrong sender's line; - `find_attachment_source` now matches author AND id; - the render path resolves each line's key from `ChatEntry.from`. F-02: `AttachmentCache` is bounded (`ATTACHMENT_CACHE_CAP = 64`) with insertion-order eviction. True LRU is impossible because iced's `view` borrows `&self` and so can't reorder on a render read; the generous cap means a normal session never evicts and the newest (on-screen) entries are always retained — only an abusive stream hits the bound. Deliberately id-keyed (cosmetic only, documented): the clip player's `playing_id`, `pending_plays`, `invalid_audio` — they're coupled to the id-keyed clip player, and the bytes actually played come from the author-keyed cache, so content is always correct. No gossip/wire/protocol change (UiEvent is in-process), no new deps. +6 unit tests (cache eviction, replace-keeps-position, same-id/distinct-author non-aliasing, is_ready/handle/clear, cap-zero clamp). 423 lib tests, clippy --all-targets clean, release build green. TESTS-GREEN-ONLY. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 377 +++++++++++++++++++++++++++++++------------ src/core/messages.rs | 10 +- src/core/mod.rs | 5 +- 3 files changed, 287 insertions(+), 105 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 46febab..322ea83 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -23,7 +23,7 @@ use iced::{ Point, Rectangle, Renderer, Size, }; use iroh::EndpointId; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::sync::{Arc, OnceLock}; use tokio::sync::Mutex; @@ -137,6 +137,91 @@ enum AttachmentState { Failed(String), } +/// Identifies one fetched attachment by BOTH the authoring peer and the +/// attachment id. The id is attacker-chosen, so a malicious peer can reuse a +/// victim's id to alias displayed/saved content; keying on the author too means +/// each chat line resolves only its own sender's bytes (Tier C F-12). +type AttachmentKey = (EndpointId, crate::files::AttachmentId); + +/// Cap on retained attachment results so an insider streaming distinct +/// attachments can't grow the cache without bound (Tier C F-02). Sized well +/// above any realistic on-screen image working set. +const ATTACHMENT_CACHE_CAP: usize = 64; + +/// One cached attachment result: its fetch state plus, for ready images, the +/// pre-built iced image handle (built once on arrival, not per redraw — the +/// e917c53 flicker fix). `handle` is `None` for files and failures. +#[derive(Debug, Clone)] +struct AttachmentEntry { + state: AttachmentState, + handle: Option, +} + +/// Bounded store of fetched chat-attachment results, keyed by [`AttachmentKey`]. +/// +/// Eviction is insertion-order (oldest first), NOT true LRU: iced's `view` +/// borrows `&self`, so the render read path cannot reorder an access-ordered +/// cache. With a generous cap the newest entries — the ones actually on screen — +/// are always retained, so a normal session never evicts; only an abusive stream +/// of distinct attachments hits the bound (Tier C F-02). +/// +/// Construct via [`AttachmentCache::new`] — there is deliberately no `Default`, +/// because a zero cap would make `insert` evict endlessly. +#[derive(Debug)] +struct AttachmentCache { + entries: HashMap, + /// Keys in insertion order; the front is the eviction candidate. Holds + /// exactly the present keys (one entry each), so it is bounded by `cap`. + order: VecDeque, + cap: usize, +} + +impl AttachmentCache { + fn new(cap: usize) -> Self { + Self { entries: HashMap::new(), order: VecDeque::new(), cap: cap.max(1) } + } + + /// Insert or replace an entry. A brand-new key evicts the oldest entries + /// until there is room; replacing an existing key keeps its position (and so + /// its age), only updating the value. + fn insert(&mut self, key: AttachmentKey, state: AttachmentState, handle: Option) { + if !self.entries.contains_key(&key) { + while self.entries.len() >= self.cap { + match self.order.pop_front() { + Some(old) => { + self.entries.remove(&old); + } + None => break, + } + } + self.order.push_back(key); + } + self.entries.insert(key, AttachmentEntry { state, handle }); + } + + fn get(&self, key: &AttachmentKey) -> Option<&AttachmentState> { + self.entries.get(key).map(|e| &e.state) + } + + fn handle(&self, key: &AttachmentKey) -> Option<&iced::widget::image::Handle> { + self.entries.get(key).and_then(|e| e.handle.as_ref()) + } + + fn is_ready(&self, key: &AttachmentKey) -> bool { + matches!(self.entries.get(key), Some(AttachmentEntry { state: AttachmentState::Ready(_), .. })) + } + + fn clear(&mut self) { + self.entries.clear(); + self.order.clear(); + } + + #[cfg(test)] + fn len(&self) -> usize { + self.entries.len() + } +} + /// Cap on retained chat history so a long call can't grow it without bound. const CHAT_HISTORY_MAX: usize = 300; @@ -289,12 +374,15 @@ pub enum AppMessage { PickAttachmentFile, /// Result of the attach picker: (filename, bytes), or None if cancelled. AttachmentFilePicked(Option<(String, Vec)>), - /// Save (downloading first if needed) a received attachment to disk. - SaveAttachment(crate::files::AttachmentId), + /// Save (downloading first if needed) a received attachment to disk. Carries + /// the full `(author, id)` key so the correct sender's bytes are fetched and + /// saved even if another peer reused the same attachment id (Tier C F-12). + SaveAttachment(AttachmentKey), /// Result of the async save dialog: a status line to show, or None if cancelled. AttachmentSaved(Option), - /// Fetch (if needed) and start an inline audio attachment. - PlayAudio(crate::files::AttachmentId), + /// Fetch (if needed) and start an inline audio attachment. Carries the full + /// `(author, id)` key (see [`AppMessage::SaveAttachment`]). + PlayAudio(AttachmentKey), PauseAudio, ResumeAudio, SeekAudio(crate::files::AttachmentId, f32), @@ -451,15 +539,14 @@ pub struct AppState { /// Room text-chat history (newest last) and the pending input line. chat_messages: Vec, chat_input: String, - /// Fetched/failed state for chat attachments, keyed by attachment id. - /// Session-only (cleared on leave); never persisted. - attachment_data: HashMap, - /// Cached iced image handles for ready image attachments, keyed by id, so we - /// don't re-upload to the GPU every redraw (the e917c53 avatar flicker fix). - image_handle_cache: HashMap, - /// Attachment ids the user asked to save before the bytes arrived; when the - /// fetch completes a save dialog is opened for them. - pending_saves: std::collections::HashSet, + /// Fetched/failed bytes + cached image handles for chat attachments, keyed by + /// `(author, id)` and bounded. Session-only (cleared on leave); never + /// persisted. (Tier C F-02 bound + F-12 author keying.) + attachments: AttachmentCache, + /// Attachments the user asked to save before the bytes arrived; when the + /// fetch completes a save dialog is opened for them. Keyed by `(author, id)` + /// so a same-id attachment from a different sender can't trigger the save. + pending_saves: HashSet, /// Clip ids waiting for the existing attachment fetch path to return bytes. pending_plays: HashSet, /// Filename-hinted audio whose fetched bytes or decoder validation failed; @@ -536,8 +623,7 @@ impl AppState { self.locally_muted.clear(); self.chat_messages.clear(); self.chat_input.clear(); - self.attachment_data.clear(); - self.image_handle_cache.clear(); + self.attachments.clear(); self.pending_saves.clear(); self.pending_plays.clear(); self.invalid_audio.clear(); @@ -658,9 +744,8 @@ impl Default for AppState { recording: false, recording_started: None, chat_messages: Vec::new(), - attachment_data: HashMap::new(), - image_handle_cache: HashMap::new(), - pending_saves: std::collections::HashSet::new(), + attachments: AttachmentCache::new(ATTACHMENT_CACHE_CAP), + pending_saves: HashSet::new(), pending_plays: HashSet::new(), invalid_audio: HashSet::new(), clip_player, @@ -1186,31 +1271,31 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { }); } } - UiEvent::AttachmentReady { id, data } => { - // Bytes arrived. For images we can cache the iced handle now - // (built once, not per redraw). If the user was waiting to save - // this file, the save dialog is opened from update() below by - // checking pending_saves — done lazily so this arm stays simple. - if crate::files::validate_image_bytes(&data).is_some() { - state.image_handle_cache.insert( - id, - iced::widget::image::Handle::from_bytes(data.clone()), - ); - } - let needs_save = state.pending_saves.remove(&id); + UiEvent::AttachmentReady { from, id, data } => { + // Bytes arrived for this specific (author, id). For images we + // can cache the iced handle now (built once, not per redraw). + // If the user was waiting to save this file, the save dialog + // is opened from update() below by checking pending_saves — + // done lazily so this arm stays simple. + let key = (from, id); + let handle = crate::files::validate_image_bytes(&data) + .is_some() + .then(|| iced::widget::image::Handle::from_bytes(data.clone())); + let needs_save = state.pending_saves.remove(&key); let needs_play = state.pending_plays.remove(&id); - state.attachment_data.insert(id, AttachmentState::Ready(data)); + state.attachments.insert(key, AttachmentState::Ready(data), handle); if needs_play { - play_ready_audio(state, id); + play_ready_audio(state, key); } if needs_save { - return save_attachment_task(state, id); + return save_attachment_task(state, key); } } - UiEvent::AttachmentFailed { id, error } => { - state.pending_saves.remove(&id); + UiEvent::AttachmentFailed { from, id, error } => { + let key = (from, id); + state.pending_saves.remove(&key); state.pending_plays.remove(&id); - state.attachment_data.insert(id, AttachmentState::Failed(error.clone())); + state.attachments.insert(key, AttachmentState::Failed(error.clone()), None); state.status_message = format!("Attachment failed: {error}"); } UiEvent::ScreenShareStarted => { @@ -1870,17 +1955,18 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { id, }; // Keep our own bytes locally so we see our own attachment inline - // immediately (others fetch it off the file plane). - if kind == crate::files::AttachmentKind::Image - && crate::files::validate_image_bytes(&bytes).is_some() - { + // immediately (others fetch it off the file plane). Cache under + // our own (author, id) key so the render path — which keys the + // line by `from` = self_id — finds them. + if let Ok(self_eid) = state.self_id.parse::() { + let key = (self_eid, id); + let handle = (kind == crate::files::AttachmentKind::Image + && crate::files::validate_image_bytes(&bytes).is_some()) + .then(|| iced::widget::image::Handle::from_bytes(bytes.clone())); state - .image_handle_cache - .insert(id, iced::widget::image::Handle::from_bytes(bytes.clone())); + .attachments + .insert(key, AttachmentState::Ready(bytes.clone()), handle); } - state - .attachment_data - .insert(id, AttachmentState::Ready(bytes.clone())); push_chat( &mut state.chat_messages, ChatEntry { @@ -1898,21 +1984,18 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { }); } } - AppMessage::SaveAttachment(id) => { + AppMessage::SaveAttachment(key) => { // If we already have the bytes, save now; otherwise fetch from the - // sender and save when AttachmentReady arrives (pending_saves). - if matches!(state.attachment_data.get(&id), Some(AttachmentState::Ready(_))) { - return save_attachment_task(state, id); - } else if let Some((from, att)) = find_attachment_source(state, id) { - if let Ok(eid) = from.parse::() { - state.pending_saves.insert(id); - state.status_message = format!("Downloading {}…", att.name); - let _ = state - .controller - .send(CoreCommand::FetchAttachment { from: eid, attachment: att }); - } else { - state.status_message = "Can't download: unknown sender.".to_string(); - } + // sender and save when AttachmentReady arrives (pending_saves). The + // key's author half is the exact sender of the clicked line. + if state.attachments.is_ready(&key) { + return save_attachment_task(state, key); + } else if let Some(att) = find_attachment_source(state, key) { + state.pending_saves.insert(key); + state.status_message = format!("Downloading {}…", att.name); + let _ = state + .controller + .send(CoreCommand::FetchAttachment { from: key.0, attachment: att }); } } AppMessage::AttachmentSaved(msg) => { @@ -1920,21 +2003,18 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { state.status_message = m; } } - AppMessage::PlayAudio(id) => { - if matches!(state.attachment_data.get(&id), Some(AttachmentState::Ready(_))) { - play_ready_audio(state, id); - } else if let Some((from, att)) = find_attachment_source(state, id) { - if let Ok(eid) = from.parse::() { - // Repeated clicks while the transfer is pending must not - // launch duplicate fetches. - if state.pending_plays.insert(id) { - state.status_message = format!("Loading {}…", att.name); - let _ = state - .controller - .send(CoreCommand::FetchAttachment { from: eid, attachment: att }); - } - } else { - state.status_message = "Can't play: unknown sender.".to_string(); + AppMessage::PlayAudio(key) => { + if state.attachments.is_ready(&key) { + play_ready_audio(state, key); + } else if let Some(att) = find_attachment_source(state, key) { + // Repeated clicks while the transfer is pending must not launch + // duplicate fetches. (pending_plays is id-keyed — it's coupled to + // the id-keyed clip player; same-id collisions are cosmetic.) + if state.pending_plays.insert(key.1) { + state.status_message = format!("Loading {}…", att.name); + let _ = state + .controller + .send(CoreCommand::FetchAttachment { from: key.0, attachment: att }); } } } @@ -2195,14 +2275,19 @@ fn push_chat(messages: &mut Vec, entry: ChatEntry) { /// Find the sender id + descriptor for a received attachment by its id, so a /// fetch can be addressed. Returns `None` for our own attachments or an unknown /// id. +/// Find the chat-attachment descriptor for an exact `(author, id)` key among +/// received (non-own) messages. Matching on the author too — not just the id — +/// means a peer reusing another sender's id can't redirect the fetch to the +/// wrong line (Tier C F-12). fn find_attachment_source( state: &AppState, - id: crate::files::AttachmentId, -) -> Option<(String, crate::files::ChatAttachment)> { + key: AttachmentKey, +) -> Option { state.chat_messages.iter().find_map(|m| { let att = m.attachment.as_ref()?; - if att.id == id && !m.mine { - Some((m.from.clone()?, att.clone())) + let from = m.from.as_ref()?; + if att.id == key.1 && !m.mine && from.parse::().ok() == Some(key.0) { + Some(att.clone()) } else { None } @@ -2211,13 +2296,15 @@ fn find_attachment_source( /// Validate cached bytes and hand them to the independent clip player. A false /// filename hint falls back to the generic file chip without reaching rodio. -fn play_ready_audio(state: &mut AppState, id: crate::files::AttachmentId) { - let Some(AttachmentState::Ready(data)) = state.attachment_data.get(&id) else { +fn play_ready_audio(state: &mut AppState, key: AttachmentKey) { + let Some(AttachmentState::Ready(data)) = state.attachments.get(&key) else { return; }; + let id = key.1; if crate::files::is_probably_audio(data) { + let bytes = data.clone(); state.invalid_audio.remove(&id); - state.clip_player.play(id, data.clone()); + state.clip_player.play(id, bytes); } else { state.invalid_audio.insert(id); state.status_message = "This attachment is not valid supported audio.".to_string(); @@ -2235,8 +2322,8 @@ fn play_ready_audio(state: &mut AppState, id: crate::files::AttachmentId) { /// the Linux `xdg-desktop-portal`/GTK backend wedges the dialog — Save/Cancel /// stop responding. The file *picker* paths already use the async variant; this /// is the one save path that must match. -fn save_attachment_task(state: &AppState, id: crate::files::AttachmentId) -> Task { - let Some(AttachmentState::Ready(data)) = state.attachment_data.get(&id) else { +fn save_attachment_task(state: &AppState, key: AttachmentKey) -> Task { + let Some(AttachmentState::Ready(data)) = state.attachments.get(&key) else { return Task::none(); }; let data = data.clone(); @@ -2246,7 +2333,7 @@ fn save_attachment_task(state: &AppState, id: crate::files::AttachmentId) -> Tas .find_map(|m| { m.attachment .as_ref() - .filter(|a| a.id == id) + .filter(|a| a.id == key.1) .map(|a| a.name.clone()) }) .unwrap_or_else(|| "download".to_string()); @@ -4382,7 +4469,14 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { ); // Attachment row (indented under the message), if any. if let Some(att) = &m.attachment { - let data = state.attachment_data.get(&att.id); + // This line's cache key is (its author, the attachment id). + // `None` only for a system line or an unparseable author. + let key: Option = m + .from + .as_ref() + .and_then(|f| f.parse::().ok()) + .map(|eid| (eid, att.id)); + let data = key.as_ref().and_then(|k| state.attachments.get(k)); let elem: Element<'_, AppMessage> = if let Some(AttachmentState::Failed(e)) = data { text(format!("⚠ {} — {e}", att.name)) @@ -4390,7 +4484,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { .color(color_red) .into() } else if att.kind == crate::files::AttachmentKind::Image { - match state.image_handle_cache.get(&att.id) { + match key.as_ref().and_then(|k| state.attachments.handle(k)) { Some(handle) => iced::widget::image(handle.clone()) .width(iced::Length::Fixed(260.0)) .into(), @@ -4419,7 +4513,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { button(text("Pause").size(12)).on_press(AppMessage::PauseAudio) } else { button(text("Play").size(12)) - .on_press(AppMessage::PlayAudio(att.id)) + .on_press_maybe(key.map(AppMessage::PlayAudio)) } .style(b_style( color_blue, @@ -4447,7 +4541,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { "Download" }) .size(12)) - .on_press(AppMessage::SaveAttachment(att.id)) + .on_press_maybe(key.map(AppMessage::SaveAttachment)) .style(b_style( color_surface, color_overlay, @@ -4493,7 +4587,7 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { .size(12) .color(color_text), button(text(btn_label).size(12)) - .on_press(AppMessage::SaveAttachment(att.id)) + .on_press_maybe(key.map(AppMessage::SaveAttachment)) .style(b_style( color_blue, color_lavender, @@ -5800,11 +5894,96 @@ impl Program for Icon { mod tests { use super::{ format_duration, initial_window_position, reconnect_attempt_chime, reconnected_chime, - set_peer_gate_config, set_peer_volume_config, AppConfig, AppState, AttachmentState, - ChatEntry, GateMeter, METER_MAX, + set_peer_gate_config, set_peer_volume_config, AppConfig, AppState, AttachmentCache, + AttachmentState, ChatEntry, GateMeter, METER_MAX, }; use iroh::SecretKey; + /// Two distinct (peer, id) keys for cache tests; `id` may be shared to model + /// a malicious peer reusing a victim's attachment id (Tier C F-12). + fn peer_key(id: [u8; 32]) -> super::AttachmentKey { + (SecretKey::generate().public(), id) + } + + #[test] + fn attachment_cache_evicts_oldest_when_full() { + let mut cache = AttachmentCache::new(2); + let k1 = peer_key([1u8; 32]); + let k2 = peer_key([2u8; 32]); + let k3 = peer_key([3u8; 32]); + cache.insert(k1, AttachmentState::Ready(vec![1]), None); + cache.insert(k2, AttachmentState::Ready(vec![2]), None); + assert_eq!(cache.len(), 2); + // Inserting a third NEW key evicts the oldest (k1), not the newest. + cache.insert(k3, AttachmentState::Ready(vec![3]), None); + assert_eq!(cache.len(), 2); + assert!(cache.get(&k1).is_none(), "oldest should be evicted"); + assert!(cache.get(&k2).is_some()); + assert!(cache.get(&k3).is_some()); + } + + #[test] + fn attachment_cache_replace_keeps_position_and_count() { + let mut cache = AttachmentCache::new(2); + let k1 = peer_key([1u8; 32]); + let k2 = peer_key([2u8; 32]); + cache.insert(k1, AttachmentState::Failed("pending".into()), None); + cache.insert(k2, AttachmentState::Ready(vec![2]), None); + // Replacing k1 (Failed -> Ready) must NOT bump it to newest; it stays the + // eviction candidate, and the count is unchanged (no order leak). + cache.insert(k1, AttachmentState::Ready(vec![1]), None); + assert_eq!(cache.len(), 2); + let k3 = peer_key([3u8; 32]); + cache.insert(k3, AttachmentState::Ready(vec![3]), None); + assert!(cache.get(&k1).is_none(), "replaced entry kept its old age"); + assert!(cache.get(&k2).is_some()); + assert!(cache.get(&k3).is_some()); + } + + #[test] + fn attachment_cache_same_id_distinct_authors_do_not_alias() { + // The F-12 core property: two peers sending the SAME attachment id keep + // separate bytes — one never overwrites or aliases the other. + let mut cache = AttachmentCache::new(8); + let shared_id = [7u8; 32]; + let victim = peer_key(shared_id); + let attacker = peer_key(shared_id); + cache.insert(victim, AttachmentState::Ready(vec![1, 1, 1]), None); + cache.insert(attacker, AttachmentState::Ready(vec![9, 9, 9]), None); + assert_eq!(cache.len(), 2); + assert!(matches!(cache.get(&victim), Some(AttachmentState::Ready(b)) if b == &[1, 1, 1])); + assert!(matches!(cache.get(&attacker), Some(AttachmentState::Ready(b)) if b == &[9, 9, 9])); + } + + #[test] + fn attachment_cache_is_ready_and_handle_and_clear() { + let mut cache = AttachmentCache::new(4); + let k = peer_key([5u8; 32]); + cache.insert( + k, + AttachmentState::Ready(vec![1]), + Some(iced::widget::image::Handle::from_bytes(vec![1])), + ); + assert!(cache.is_ready(&k)); + assert!(cache.handle(&k).is_some()); + let failed = peer_key([6u8; 32]); + cache.insert(failed, AttachmentState::Failed("nope".into()), None); + assert!(!cache.is_ready(&failed)); + assert!(cache.handle(&failed).is_none()); + cache.clear(); + assert_eq!(cache.len(), 0); + assert!(cache.get(&k).is_none()); + } + + #[test] + fn attachment_cache_cap_zero_is_clamped_to_one() { + let mut cache = AttachmentCache::new(0); + let k = peer_key([1u8; 32]); + cache.insert(k, AttachmentState::Ready(vec![1]), None); + assert_eq!(cache.len(), 1); + assert!(cache.get(&k).is_some()); + } + #[test] fn reset_room_state_clears_all_room_scoped_state() { let mut state = AppState::default(); @@ -5830,12 +6009,13 @@ mod tests { attachment: None, }); state.chat_input = "draft".to_string(); - state.attachment_data.insert(attachment_id, AttachmentState::Ready(vec![1])); - state.image_handle_cache.insert( - attachment_id, - iced::widget::image::Handle::from_bytes(vec![1]), + let att_key = (peer, attachment_id); + state.attachments.insert( + att_key, + AttachmentState::Ready(vec![1]), + Some(iced::widget::image::Handle::from_bytes(vec![1])), ); - state.pending_saves.insert(attachment_id); + state.pending_saves.insert(att_key); state.pending_plays.insert(attachment_id); state.invalid_audio.insert(attachment_id); state.connecting.insert(peer); @@ -5854,8 +6034,7 @@ mod tests { assert!(state.locally_muted.is_empty()); assert!(state.chat_messages.is_empty()); assert!(state.chat_input.is_empty()); - assert!(state.attachment_data.is_empty()); - assert!(state.image_handle_cache.is_empty()); + assert!(state.attachments.len() == 0); assert!(state.pending_saves.is_empty()); assert!(state.pending_plays.is_empty()); assert!(state.invalid_audio.is_empty()); diff --git a/src/core/messages.rs b/src/core/messages.rs index 17c90a0..293c3d4 100644 --- a/src/core/messages.rs +++ b/src/core/messages.rs @@ -133,11 +133,13 @@ pub enum UiEvent { /// string, used to key their avatar (W4). ChatMessage { from: String, name: String, text: String, attachment: Option }, /// An attachment's bytes are now available (auto-fetched for images, or - /// fetched on demand for files). Keyed by attachment id so the UI can match - /// it to the chat entry. - AttachmentReady { id: crate::files::AttachmentId, data: Vec }, + /// fetched on demand for files). Keyed by `(from, id)`: the id is + /// attacker-chosen, so a malicious peer can reuse a victim's id — the author + /// disambiguates whose bytes these are and stops content aliasing (Tier C + /// F-12). + AttachmentReady { from: EndpointId, id: crate::files::AttachmentId, data: Vec }, /// An attachment fetch failed (sender gone, too large, decode error, etc.). - AttachmentFailed { id: crate::files::AttachmentId, error: String }, + AttachmentFailed { from: EndpointId, id: crate::files::AttachmentId, error: String }, /// Our own screen share started; the UI flips the Share button to "Stop". ScreenShareStarted, /// Our own screen share stopped (or failed to start). diff --git a/src/core/mod.rs b/src/core/mod.rs index 2f329e6..930c264 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -827,6 +827,7 @@ fn spawn_attachment_fetch( if is_image && crate::files::validate_image_bytes(&data).is_none() { let _ = ui_tx .send(UiEvent::AttachmentFailed { + from, id: att.id, error: "received image failed to decode".to_string(), }) @@ -834,12 +835,12 @@ fn spawn_attachment_fetch( return; } let _ = ui_tx - .send(UiEvent::AttachmentReady { id: att.id, data }) + .send(UiEvent::AttachmentReady { from, id: att.id, data }) .await; } Err(e) => { let _ = ui_tx - .send(UiEvent::AttachmentFailed { id: att.id, error: e.to_string() }) + .send(UiEvent::AttachmentFailed { from, id: att.id, error: e.to_string() }) .await; } }