From 6d0bf99076368f3de145211b91da1fab8c2d0720 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Sun, 31 May 2026 15:10:22 -0400 Subject: [PATCH] fix(friends): five robustness bugs in the friends/control plane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found in a bug audit of the just-merged friends-list feature. No crashes or security holes, but five real state/correctness bugs: - Host child dying on its own left the share campaign running, so it kept pushing a now-dead ticket to friends (retrying offline ones forever) and leaked share_status/met/share_code. The unexpected-exit path now captures the stderr error, then routes through the full stop_host() teardown (notably stop_share). (gui/mod.rs pump_host_events) - on_friend_request downgraded an already-Accepted friend back to PendingIncoming when they re-sent a request (e.g. after losing their store). It now stays Accepted and re-confirms. (friends.rs) - on_friend_accept advanced *any* known peer to Accepted, including a PendingIncoming one — a peer could mark itself accepted without the local user's consent. Now only a PendingOutgoing request we sent is honoured. (friends.rs) - A ShareCode redelivered by an ACK-loss retry fired a duplicate desktop notification. push_notice now reports whether the code is new/changed and only then toasts. (gui/mod.rs) - An inbound control message could be delayed up to IO_TIMEOUT on a degraded link because handle() awaited the sender's close before forwarding it. Forward to the UI first, then await close so the ACK still flushes. (control.rs) Adds two friends-store transition tests (accept ignores a pending-incoming peer; request doesn't downgrade an accepted friend). 47 gui / 8 headless tests pass, clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 --- src/common/control.rs | 11 ++++--- src/common/friends.rs | 71 ++++++++++++++++++++++++++++++++----------- src/gui/mod.rs | 54 +++++++++++++++++++++----------- 3 files changed, 97 insertions(+), 39 deletions(-) diff --git a/src/common/control.rs b/src/common/control.rs index e6d360a..a53b66a 100644 --- a/src/common/control.rs +++ b/src/common/control.rs @@ -166,13 +166,16 @@ async fn handle(incoming: Incoming, tx: &mpsc::Sender) -> Result<()> { .await .context("timed out reading control message")??; - // Wait (briefly) for the sender's close so our ACK flushes before the - // connection is dropped at the end of this scope. - let _ = tokio::time::timeout(IO_TIMEOUT, conn.closed()).await; - + // Hand the message up first, so it reaches the UI promptly even when the + // sender is slow to close (a degraded link could otherwise delay a friend + // request / pushed code by up to IO_TIMEOUT). tx.send(Inbound { from, msg }) .await .map_err(|_| anyhow::anyhow!("control: receiver dropped"))?; + + // Then wait (briefly) for the sender's close so our ACK has flushed before + // the connection is dropped at the end of this scope. + let _ = tokio::time::timeout(IO_TIMEOUT, conn.closed()).await; Ok(()) } diff --git a/src/common/friends.rs b/src/common/friends.rs index f95c20d..5e0a81c 100644 --- a/src/common/friends.rs +++ b/src/common/friends.rs @@ -147,35 +147,45 @@ impl FriendStore { self.friends.len() != before } - /// Apply an inbound friend request. Returns `true` if it *completes a mutual - /// match* — we'd already sent them one, so they're now [`Accepted`] and the - /// caller should reply with a `FriendAccept`. Otherwise it's recorded as + /// Apply an inbound friend request. Returns `true` if the friendship is now + /// settled at [`Accepted`] and the caller should reply with a `FriendAccept` + /// — either because we'd already sent them a request (a mutual match) or + /// because they're an existing friend re-announcing (we never downgrade an + /// [`Accepted`] friend back to pending; a peer who lost their store and + /// re-adds us just gets re-confirmed). Otherwise it's recorded as /// [`PendingIncoming`] for the user to act on and `false` is returned. /// /// [`Accepted`]: FriendState::Accepted /// [`PendingIncoming`]: FriendState::PendingIncoming pub fn on_friend_request(&mut self, id: EndpointId, name: String) -> bool { + match self.find(&id).map(|f| f.state) { + Some(FriendState::PendingOutgoing | FriendState::Accepted) => { + self.upsert(id, name, FriendState::Accepted); + true + } + _ => { + self.upsert(id, name, FriendState::PendingIncoming); + false + } + } + } + + /// Apply an inbound acceptance of a request we sent. Returns `true` only if + /// it advanced one of *our* outgoing requests to [`Accepted`]. An accept for + /// any other state is ignored: a stranger's, or one for a peer still in + /// [`PendingIncoming`] (their request, awaiting our decision) — honouring the + /// latter would let a peer mark itself accepted without the local user's + /// consent. + /// + /// [`Accepted`]: FriendState::Accepted + /// [`PendingIncoming`]: FriendState::PendingIncoming + pub fn on_friend_accept(&mut self, id: EndpointId, name: String) -> bool { if matches!( self.find(&id).map(|f| f.state), Some(FriendState::PendingOutgoing) ) { self.upsert(id, name, FriendState::Accepted); true - } else { - self.upsert(id, name, FriendState::PendingIncoming); - false - } - } - - /// Apply an inbound acceptance of a request we sent. Returns `true` if it - /// advanced a friendship to [`Accepted`] (i.e. we actually knew this peer); - /// an accept from a stranger is ignored. - /// - /// [`Accepted`]: FriendState::Accepted - pub fn on_friend_accept(&mut self, id: EndpointId, name: String) -> bool { - if self.find(&id).is_some() { - self.upsert(id, name, FriendState::Accepted); - true } else { false } @@ -293,4 +303,29 @@ mod tests { assert!(!store.on_friend_accept(stranger, "Nope".into())); assert!(store.find(&stranger).is_none()); } + + #[test] + fn accept_does_not_advance_a_pending_incoming_peer() { + // They asked us and we haven't decided yet; an unsolicited FriendAccept + // from them must not auto-accept on our behalf (consent bypass). + let mut store = FriendStore::default(); + let id = sample_id(); + store.upsert(id, "Theirs".into(), FriendState::PendingIncoming); + assert!(!store.on_friend_accept(id, "Theirs".into())); + assert_eq!(store.find(&id).unwrap().state, FriendState::PendingIncoming); + } + + #[test] + fn request_does_not_downgrade_an_accepted_friend() { + // A current friend re-sending a request (e.g. after losing their store) + // must stay accepted; the call signals a re-confirm rather than a + // downgrade to pending. + let mut store = FriendStore::default(); + let id = sample_id(); + store.upsert(id, "Pal".into(), FriendState::Accepted); + let settled = store.on_friend_request(id, "Pal (reinstalled)".into()); + assert!(settled); + assert_eq!(store.find(&id).unwrap().state, FriendState::Accepted); + assert_eq!(store.find(&id).unwrap().name, "Pal (reinstalled)"); + } } diff --git a/src/gui/mod.rs b/src/gui/mod.rs index 406de59..ca31404 100644 --- a/src/gui/mod.rs +++ b/src/gui/mod.rs @@ -1145,11 +1145,14 @@ impl PixelPassApp { f.name = name.clone(); store_changed = true; } - self.push_notice(from, name.clone(), ticket); - notify( - "PixelPass — a friend is sharing", - format!("{name} is sharing their screen. Open PixelPass to watch."), - ); + // Only toast for a new/changed code — an ACK-loss retry + // redelivers the same code and shouldn't fire again. + if self.push_notice(from, name.clone(), ticket) { + notify( + "PixelPass — a friend is sharing", + format!("{name} is sharing their screen. Open PixelPass to watch."), + ); + } } else { tracing::warn!(from = %from, "presence: ignoring ShareCode from a non-friend"); } @@ -1197,13 +1200,20 @@ impl PixelPassApp { } /// Record a share code a friend pushed us, replacing any prior notice from - /// the same friend (their previous code is stale once they re-host). - fn push_notice(&mut self, from: iroh::EndpointId, name: String, code: String) { + /// the same friend (their previous code is stale once they re-host). Returns + /// `true` if this is a new notice or a *different* code than we already had + /// from them — i.e. worth a fresh desktop notification. A duplicate delivery + /// (an ACK-loss retry redelivering the same code) updates in place and + /// returns `false`, so it doesn't fire a second toast. + fn push_notice(&mut self, from: iroh::EndpointId, name: String, code: String) -> bool { if let Some(n) = self.notices.iter_mut().find(|n| n.from == from) { + let changed = n.code != code; n.name = name; n.code = code; + changed } else { self.notices.push(ShareNotice { from, name, code }); + true } } @@ -2318,19 +2328,29 @@ impl PixelPassApp { self.apply_host_event(ev); } - if let Some(p) = &mut self.host.proc - && !p.is_alive() - { - if self.host.ticket.is_none() { - let tail = p.stderr_tail(); - self.host.error = Some(if tail.trim().is_empty() { + let dead = self.host.proc.as_mut().is_some_and(|p| !p.is_alive()); + if dead { + // If it never reached a ticket, capture why (from the stderr tail) + // before tearing down. Then run the *full* Stop cleanup — most + // importantly stop_share, so a host that died on its own stops + // pushing its now-dead code to friends. Without this the campaign + // would keep retrying offline friends with a stale ticket for the + // life of the GUI, and share_status/met/share_code would leak. + let error = self.host.ticket.is_none().then(|| { + let tail = self + .host + .proc + .as_mut() + .map(|p| p.stderr_tail()) + .unwrap_or_default(); + if tail.trim().is_empty() { "Host exited before it could start.".to_string() } else { format!("Host exited before it could start:\n{tail}") - }); - } - self.host.proc = None; - self.host.capturing = false; + } + }); + self.stop_host(); + self.host.error = error; } }