From 3b640726d7df7bc63ec94eb0dd3b52eba61b64b3 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 26 Jun 2026 17:00:09 -0400 Subject: [PATCH] =?UTF-8?q?fix(security):=20address=20Codex=20F-02/F-12=20?= =?UTF-8?q?audit=20=E2=80=94=20save-filename=20alias=20+=20doc=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex audit of 381e00b (f02-f12-audit-2026-06-26) found no P1/P2 regression and verified the F-12 key round-trip invariant sound (iroh EndpointId Display/FromStr are exact inverses). Acting on the one real P3 + nits: - P3: save_attachment_task picked the dialog's DEFAULT FILENAME by bare attachment id, so a peer reusing a victim's id could mislabel the save with another sender's name/extension (bytes were already author-keyed and correct — this was a metadata residual, not content aliasing). Extracted a pure `attachment_default_name` that matches the full (author, id) key, like find_attachment_source. +1 unit test (closes the audit's coverage gap). - Doc nits: refreshed the stale `attachment_data` reference on ChatEntry, the "keyed by attachment id" note on spawn_attachment_fetch, and a duplicated doc block above find_attachment_source. DEFERRED (user decision pending): the P3 judgement call — pending_plays / invalid_audio / clip playing_id stay bare-id keyed, so duplicate-id audio rows share play/seek/invalid state (cosmetic; bytes played are still author-keyed and correct). Fully closing it means threading AttachmentKey through the clip player. 424 lib tests, clippy --all-targets clean, release build green. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 70 ++++++++++++++++++++++++++++++++++++------------- src/core/mod.rs | 3 ++- 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 322ea83..3bc8727 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -123,8 +123,8 @@ struct ChatEntry { /// for any future system-generated lines. from: Option, /// Optional file attachment descriptor. The bytes (if fetched) live in - /// `AppState.attachment_data` keyed by `attachment.id`; the entry only holds - /// the descriptor so history stays cheap. + /// `AppState.attachments` keyed by `(author, attachment.id)`; the entry only + /// holds the descriptor so history stays cheap. attachment: Option, } @@ -2272,9 +2272,22 @@ 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. +/// Pick the default save-dialog filename for an attachment, matched by the FULL +/// `(author, id)` key — not the bare id — so a peer reusing another sender's id +/// can't supply the filename (and extension) for a different line (Tier C F-12 +/// metadata residual). Falls back to "download" if the line is gone. Matches own +/// and received lines alike (our own `from` = `self_id` parses to the key author). +fn attachment_default_name(messages: &[ChatEntry], key: AttachmentKey) -> String { + messages + .iter() + .find_map(|m| { + let att = m.attachment.as_ref()?; + let from = m.from.as_ref()?.parse::().ok()?; + (att.id == key.1 && from == key.0).then(|| att.name.clone()) + }) + .unwrap_or_else(|| "download".to_string()) +} + /// 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 @@ -2327,16 +2340,7 @@ fn save_attachment_task(state: &AppState, key: AttachmentKey) -> Task for Icon { #[cfg(test)] mod tests { use super::{ - format_duration, initial_window_position, reconnect_attempt_chime, reconnected_chime, - set_peer_gate_config, set_peer_volume_config, AppConfig, AppState, AttachmentCache, - AttachmentState, ChatEntry, GateMeter, METER_MAX, + attachment_default_name, format_duration, initial_window_position, reconnect_attempt_chime, + reconnected_chime, set_peer_gate_config, set_peer_volume_config, AppConfig, AppState, + AttachmentCache, AttachmentState, ChatEntry, GateMeter, METER_MAX, }; use iroh::SecretKey; @@ -5984,6 +5988,36 @@ mod tests { assert!(cache.get(&k).is_some()); } + #[test] + fn attachment_default_name_matches_full_key_not_bare_id() { + // Two chat lines carry the SAME attachment id but come from different + // senders. The save-dialog default filename must be the one belonging to + // the clicked (author, id) — not whichever line happens to match the bare + // id first (Tier C F-12 metadata residual). + let victim = SecretKey::generate().public(); + let attacker = SecretKey::generate().public(); + let shared_id = [7u8; 32]; + let mk = |from: iroh::EndpointId, fname: &str| ChatEntry { + name: "Peer".to_string(), + text: String::new(), + mine: false, + from: Some(from.to_string()), + attachment: Some(crate::files::ChatAttachment { + name: fname.to_string(), + size: 3, + kind: crate::files::AttachmentKind::File, + id: shared_id, + }), + }; + // 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")]; + assert_eq!(attachment_default_name(&messages, (victim, shared_id)), "report.pdf"); + assert_eq!(attachment_default_name(&messages, (attacker, shared_id)), "evil.sh"); + // Unknown line → safe fallback. + let unknown = SecretKey::generate().public(); + assert_eq!(attachment_default_name(&messages, (unknown, shared_id)), "download"); + } + #[test] fn reset_room_state_clears_all_room_scoped_state() { let mut state = AppState::default(); diff --git a/src/core/mod.rs b/src/core/mod.rs index 930c264..42b647d 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -805,7 +805,8 @@ fn should_auto_fetch(is_image: bool, author_in_roster: bool, already_inflight: b /// Fetch a chat attachment's bytes from `from` over the file plane in a detached /// task, then report the result to the UI via [`UiEvent::AttachmentReady`] / -/// [`UiEvent::AttachmentFailed`] keyed by the attachment id. For images +/// [`UiEvent::AttachmentFailed`], tagged with `from` so the UI keys the bytes by +/// `(author, id)` and can't alias a same-id attachment from another sender. For images /// (`is_image`) the bytes are defensively re-validated (decodable + within pixel /// limits) before being handed to the renderer; an "Image" that doesn't decode is /// reported as a failure rather than rendered. `guard` is `Some` for bounded