fix: harden tunnels and code quality checks

crates/shelldeck-core/src/config/platform_attention.rs

@@ -2743,7 +2743,7 @@ mod tests {
         Snapshot {
             mode: String,
             outcome: String,
-            snapshot: CorpusSnapshot,
+            snapshot: Box<CorpusSnapshot>,
         },
         Refusal {
             category: String,

crates/shelldeck-core/src/config/platform_review/worktree.rs

@@ -1005,10 +1005,10 @@ mod tests {
         .expect("exact mapping")
     }
 
-    fn lane<'a>(
-        projection: &'a ReviewWorktreeProjection,
+    fn lane(
+        projection: &ReviewWorktreeProjection,
         lane: ReviewWorktreeLane,
-    ) -> &'a [ReviewWorktreeFile] {
+    ) -> &[ReviewWorktreeFile] {
         &projection
             .lanes
             .iter()

crates/shelldeck-ssh/src/tunnel.rs

@@ -90,9 +90,15 @@ impl TunnelManager {
         remote_host: String,
         remote_port: u16,
     ) -> crate::Result<Uuid> {
-        if !Self::check_port_available(local_port).await {
-            return Err(SshError::PortInUse(local_port));
-        }
+        // Bind before reporting success. The previous availability probe then
+        // spawned a second bind, leaving a TOCTOU window where the method
+        // returned Ok even though no listener was ever created.
+        let listener = TcpListener::bind(("127.0.0.1", local_port))
+            .await
+            .map_err(|error| match error.kind() {
+                std::io::ErrorKind::AddrInUse => SshError::PortInUse(local_port),
+                _ => SshError::Io(error),
+            })?;
 
         let id = Uuid::new_v4();
         let status = Arc::new(ParkingMutex::new(TunnelStatus::Active));
@@ -105,15 +111,6 @@ impl TunnelManager {
         let bytes_received_clone = bytes_received.clone();
 
         tokio::spawn(async move {
-            let listener = match TcpListener::bind(format!("127.0.0.1:{}", local_port)).await {
-                Ok(l) => l,
-                Err(e) => {
-                    tracing::error!("Failed to bind local port {}: {}", local_port, e);
-                    *status_clone.lock() = TunnelStatus::Error;
-                    return;
-                }
-            };
-
             tracing::info!(
                 "Local forward: 127.0.0.1:{} -> {}:{}",
                 local_port,
@@ -304,9 +301,16 @@ impl TunnelManager {
         local_host: String,
         local_port: u16,
     ) -> crate::Result<Uuid> {
-        if !Self::check_port_available(local_port).await {
-            return Err(SshError::PortInUse(local_port));
-        }
+        let bind_addr = format!("{}:{}", local_host, local_port);
+        // As with local forwards, ownership of the bound socket is the
+        // readiness signal. Returning only after this succeeds makes Ok mean
+        // that clients can connect immediately and removes a bind race.
+        let listener = TcpListener::bind(&bind_addr)
+            .await
+            .map_err(|error| match error.kind() {
+                std::io::ErrorKind::AddrInUse => SshError::PortInUse(local_port),
+                _ => SshError::Io(error),
+            })?;
 
         let id = Uuid::new_v4();
         let status = Arc::new(ParkingMutex::new(TunnelStatus::Active));
@@ -319,16 +323,6 @@ impl TunnelManager {
         let bytes_received_clone = bytes_received.clone();
 
         tokio::spawn(async move {
-            let bind_addr = format!("{}:{}", local_host, local_port);
-            let listener = match TcpListener::bind(&bind_addr).await {
-                Ok(l) => l,
-                Err(e) => {
-                    tracing::error!("Failed to bind SOCKS5 listener on {}: {}", bind_addr, e);
-                    *status_clone.lock() = TunnelStatus::Error;
-                    return;
-                }
-            };
-
             tracing::info!("Dynamic forward: SOCKS5 proxy on {}", bind_addr);
 
             let mut connections = tokio::task::JoinSet::new();
@@ -762,7 +756,8 @@ mod tests {
     use russh::keys::{ssh_key::Algorithm, PrivateKey};
     use russh::server::{self, Auth, Msg, Session};
     use russh::Channel;
-    use std::sync::Arc;
+    use std::collections::HashSet;
+    use std::sync::{Arc, LazyLock, Mutex as StdMutex};
     use std::time::Duration;
     use tokio::io::{AsyncReadExt, AsyncWriteExt};
     use tokio::net::{TcpListener, TcpStream};
@@ -963,24 +958,29 @@ mod tests {
         (Arc::new(Mutex::new(handle)), request_rx, server_task)
     }
 
+    static CLAIMED_TEST_PORTS: LazyLock<StdMutex<HashSet<u16>>> =
+        LazyLock::new(|| StdMutex::new(HashSet::new()));
+
     async fn unused_local_port() -> u16 {
-        let listener = TcpListener::bind("127.0.0.1:0")
-            .await
-            .expect("reserve an ephemeral port");
-        listener.local_addr().expect("read local address").port()
+        loop {
+            let listener = TcpListener::bind("127.0.0.1:0")
+                .await
+                .expect("reserve an ephemeral port");
+            let port = listener.local_addr().expect("read local address").port();
+            if CLAIMED_TEST_PORTS
+                .lock()
+                .expect("claimed test ports lock")
+                .insert(port)
+            {
+                return port;
+            }
+        }
     }
 
-    async fn connect_when_ready(port: u16) -> TcpStream {
-        timeout(Duration::from_secs(2), async move {
-            loop {
-                match TcpStream::connect(("127.0.0.1", port)).await {
-                    Ok(stream) => return stream,
-                    Err(_) => sleep(Duration::from_millis(5)).await,
-                }
-            }
-        })
-        .await
-        .expect("tunnel listener did not become ready")
+    async fn connect_to_tunnel(port: u16) -> TcpStream {
+        TcpStream::connect(("127.0.0.1", port))
+            .await
+            .expect("tunnel listener must be bound before start returns")
     }
 
     async fn wait_until_stopped(manager: &TunnelManager, id: uuid::Uuid) {
@@ -1023,7 +1023,7 @@ mod tests {
             .await
             .expect("start local forward");
 
-        let mut client = connect_when_ready(local_port).await;
+        let mut client = connect_to_tunnel(local_port).await;
         client.write_all(b"shelldeck").await.expect("write tunnel");
         let mut echoed = [0_u8; 9];
         client
@@ -1165,7 +1165,7 @@ mod tests {
             .await
             .expect("start SOCKS5 forward");
 
-        let mut client = connect_when_ready(local_port).await;
+        let mut client = connect_to_tunnel(local_port).await;
         negotiate_no_auth(&mut client).await;
         let host = b"echo.internal";
         let mut connect = vec![0x05, 0x01, 0x00, 0x03, host.len() as u8];
@@ -1202,7 +1202,7 @@ mod tests {
         assert_eq!(&echoed, b"proxy");
 
         for command in [0x02_u8, 0x03_u8] {
-            let mut rejected = connect_when_ready(local_port).await;
+            let mut rejected = connect_to_tunnel(local_port).await;
             negotiate_no_auth(&mut rejected).await;
             rejected
                 .write_all(&[0x05, command, 0x00, 0x01, 127, 0, 0, 1, 0, 80])
@@ -1239,7 +1239,7 @@ mod tests {
 
     // SDTEST-561, SDTEST-563
     #[tokio::test]
-    async fn port_availability_and_prebound_start_failure_are_reported() {
+    async fn port_availability_and_prebound_start_failures_are_reported() {
         let listener = TcpListener::bind("127.0.0.1:0")
             .await
             .expect("bind occupied test port");
@@ -1252,10 +1252,21 @@ mod tests {
         let (handle, _requests, server_task) = start_echo_server().await;
         let mut manager = TunnelManager::new();
         let error = manager
-            .start_local_forward(handle, occupied_port, "echo.internal".to_owned(), 4242)
+            .start_local_forward(
+                handle.clone(),
+                occupied_port,
+                "echo.internal".to_owned(),
+                4242,
+            )
             .await
             .expect_err("prebound local port must fail");
         assert!(matches!(error, crate::SshError::PortInUse(port) if port == occupied_port));
+
+        let error = manager
+            .start_socks_forward(handle, "127.0.0.1".to_owned(), occupied_port)
+            .await
+            .expect_err("prebound SOCKS5 port must fail");
+        assert!(matches!(error, crate::SshError::PortInUse(port) if port == occupied_port));
         assert!(manager.tunnels().is_empty());
 
         server_task.abort();
@@ -1281,8 +1292,8 @@ mod tests {
             .start_local_forward(handle, second_port, "second.internal".to_owned(), 1002)
             .await
             .expect("start second tunnel");
-        let mut first = connect_when_ready(first_port).await;
-        let mut second = connect_when_ready(second_port).await;
+        let mut first = connect_to_tunnel(first_port).await;
+        let mut second = connect_to_tunnel(second_port).await;
         first.write_all(b"a").await.expect("write first tunnel");
         second.write_all(b"b").await.expect("write second tunnel");
         let mut byte = [0_u8; 1];

crates/shelldeck-ui/src/workspace/user_home.rs

@@ -278,8 +278,10 @@ impl Workspace {
         Option<manage_sites::ManagedSiteInfo>,
         Vec<manage_sites::ManagedSiteInfo>,
     ) {
-        let payload = self.site_directory.clone().unwrap_or_default();
-        let active_id = self.app_config.cloud_sync.active_site_id.clone();
+        let Some(payload) = self.site_directory.as_ref() else {
+            return (None, Vec::new());
+        };
+        let active_id = self.app_config.cloud_sync.active_site_id.as_deref();
         let conn_site_ids: std::collections::HashSet<String> = self
             .connections
             .iter()
@@ -302,17 +304,16 @@ impl Workspace {
             })
             .cloned()
             .collect();
-        sites.sort_by(|a, b| {
-            let a_conn = conn_site_ids.contains(&a.site_id);
-            let b_conn = conn_site_ids.contains(&b.site_id);
-            b_conn.cmp(&a_conn).then(
-                a.display_label()
-                    .to_lowercase()
-                    .cmp(&b.display_label().to_lowercase()),
+        // Cache the normalized label once per site. `sort_by` used to rebuild
+        // both lowercase strings for every comparison (O(n log n) temporary
+        // allocations) each time the Sites tab rendered.
+        sites.sort_by_cached_key(|site| {
+            (
+                !conn_site_ids.contains(&site.site_id),
+                site.display_label().to_lowercase(),
             )
         });
         let active = active_id
-            .as_deref()
             .and_then(|id| sites.iter().position(|s| s.site_id == id))
             .map(|idx| sites.remove(idx));
         (active, sites)
@@ -714,7 +715,6 @@ impl Workspace {
             .issues_list
             .iter()
             .filter(|issue| self.is_user_visible_issue(issue))
-            .cloned()
             .collect::<Vec<_>>();
         let open_requests = my_requests
             .iter()
@@ -1631,7 +1631,8 @@ impl Workspace {
         cx: &mut Context<Self>,
     ) -> impl IntoElement {
         let account = self.app_config.account.clone().unwrap_or_default();
-        let payload = self.site_directory.clone().unwrap_or_default();
+        let payload = self.site_directory.as_ref();
+        let total_sites = payload.map_or(0, |directory| directory.sites.len());
         let role_tokens = user_role_tokens(&account);
         let primary_role = primary_user_role(&role_tokens)
             .filter(|role| *role != "user")
@@ -1652,7 +1653,15 @@ impl Workspace {
         ];
         let area_buttons: Vec<manage_sites::ManageArea> = preferred
             .iter()
-            .filter_map(|k| payload.areas.iter().find(|a| a.key == *k).cloned())
+            .filter_map(|key| {
+                payload.and_then(|directory| {
+                    directory
+                        .areas
+                        .iter()
+                        .find(|area| area.key == *key)
+                        .cloned()
+                })
+            })
             .collect();
 
         // Header card.
@@ -1759,11 +1768,10 @@ impl Workspace {
                     )
             });
 
-        // Sites: filter by search, sort (conn-bearing first, then alpha),
-        // split into (active-card, others-for-virt-list). Recomputed inside
-        // the `uniform_list` processor as well — cheap enough on 300 sites
-        // (< 1ms) and keeps the model authoritative.
+        // Sites: filter by search, sort (conn-bearing first, then alpha), and
+        // split into (active-card, others-for-virt-list) once per render.
         let (active_site, others_sites) = self.partition_user_sites(cx);
+        let others_sites = Rc::new(others_sites);
         let others_count = others_sites.len();
         let site_search_query = self
             .user_sites_search_state
@@ -1772,7 +1780,7 @@ impl Workspace {
             .trim()
             .to_string();
         let empty_state = user_sites_empty_state(
-            payload.sites.len(),
+            total_sites,
             usize::from(active_site.is_some()) + others_count,
             &site_search_query,
         );
@@ -1913,15 +1921,15 @@ impl Workspace {
             } else {
                 SITE_ROW_H
             };
+            let visible_sites = Rc::clone(&others_sites);
             list = list.child(
                 uniform_list(
                     "user-home-sites-virt",
                     others_count,
                     cx.processor(move |this, range: Range<usize>, _window, cx| {
-                        let (_, others) = this.partition_user_sites(cx);
                         let mut items: Vec<AnyElement> = Vec::new();
                         for i in range {
-                            if let Some(site) = others.get(i) {
+                            if let Some(site) = visible_sites.get(i) {
                                 items.push(
                                     this.render_compact_site_row(site, compact_user_home, cx)
                                         .into_any_element(),
@@ -1978,7 +1986,7 @@ impl Workspace {
                                     .child(t!("user.sites.title").to_string()),
                             ),
                     );
-                if payload.sites.len() > 5 {
+                if total_sites > 5 {
                     let entity = cx.entity();
                     row = row.child(
                         div().w(px(260.0)).child(

deny.toml

@@ -7,8 +7,11 @@ targets = [
 all-features = true
 
 [advisories]
-unmaintained = "workspace"
+unmaintained = "all"
 unsound = "all"
+# Keep the target-agnostic cargo-audit baseline mirrored below. cargo-deny's
+# configured desktop graph does not encounter the WASM-only `instant` edge.
+unused-ignored-advisory = "allow"
 ignore = [
     { id = "RUSTSEC-2023-0071", reason = "rsa is transitive through russh's default rsa feature; the advisory records no patched release at all, and dropping the feature would remove RSA SSH key support" },
     { id = "RUSTSEC-2024-0384", reason = "instant is transitive through futures-lite 1 in adabraka_util 0.5.1, a crates.io sibling of the vendored GPUI that we do not fork" },

docs/testing/tests-ssh.md

@@ -102,9 +102,9 @@ Existing: **4 protocol/lifecycle tests.**
 | ID | Location | SDUC | Status | Notes |
 |---|---|---|---|---|
 | SDTEST-560 | `shelldeck-core::port_forward::zero_is_rejected` + `all_non_zero_ports_are_accepted` | SDUC-049 | Green | Already covered by SDTEST-030; `u16` makes overflow unrepresentable and 0 is rejected. |
-| SDTEST-561 | `tunnel.rs::port_availability_and_prebound_start_failure_are_reported` | SDUC-049 | Green | Reserves a real loopback port and verifies a released ephemeral port. |
+| SDTEST-561 | `tunnel.rs::port_availability_and_prebound_start_failures_are_reported` | SDUC-049 | Green | Reserves a real loopback port and verifies a released ephemeral port. |
 | SDTEST-562 | `tunnel.rs::local_forward_echoes_tracks_bytes_and_drains_on_stop` | SDUC-049 | Green | Real `russh` direct-tcpip over an in-memory transport; loopback listener forwards bytes both ways. |
-| SDTEST-563 | `tunnel.rs::port_availability_and_prebound_start_failure_are_reported` | SDUC-049 | Green | Pre-bound port returns `SshError::PortInUse` without registering a tunnel. |
+| SDTEST-563 | `tunnel.rs::port_availability_and_prebound_start_failures_are_reported` | SDUC-049 | Green | Local and SOCKS pre-bound ports synchronously return `SshError::PortInUse` without registering a tunnel. |
 | SDTEST-564 | `tunnel.rs::local_forward_echoes_tracks_bytes_and_drains_on_stop` | SDUC-052 | Green | Caught detached copy tasks: stop now aborts and joins accepted connections before publishing `Stopped`. |
 | SDTEST-565 | `tunnel.rs::remote_forward_requests_the_port_and_routes_connections_to_the_local_target` | SDUC-050, SDUC-052 | Green | Real reverse forward end to end: the in-memory server records `tcpip_forward("0.0.0.0", 8443)`, then plays the remote side by opening a forwarded-tcpip channel back. Bytes reach a loopback echo target and return; counters are directional; stopping the tunnel closes the already-routed connection. |
 | SDTEST-566 | `tunnel.rs::socks5_connect_echoes_and_rejects_bind_and_udp_associate` | SDUC-051 | Green | Raw SOCKS5 no-auth + domain CONNECT reaches direct-tcpip and echoes; BIND/UDP-associate return command-not-supported without opening a channel. |