From f2f7b166e6e2539ae2b6db57a3705916e4bdafcb Mon Sep 17 00:00:00 2001 From: pedrokarim Date: Thu, 10 Sep 2026 09:40:19 +0200 Subject: [PATCH] fix: harden tunnels and code quality checks - Bind local and SOCKS forward listeners before start returns, so Ok means the port is accepting connections and a pre-bound port reports PortInUse synchronously instead of failing inside the spawned task. - Make tunnel tests connect immediately and claim unique ephemeral ports, removing parallel flakiness. - Fix clippy --all-targets findings in platform attention and review worktree tests. - Partition User home sites once per render with cached sort keys and borrow the site directory and request list instead of cloning them. - Check unmaintained RustSec advisories across the whole dependency graph in deny.toml. --- .../src/config/platform_attention.rs | 2 +- .../src/config/platform_review/worktree.rs | 6 +- crates/shelldeck-ssh/src/tunnel.rs | 107 ++++++++++-------- .../shelldeck-ui/src/workspace/user_home.rs | 50 ++++---- deny.toml | 5 +- docs/testing/tests-ssh.md | 4 +- 6 files changed, 98 insertions(+), 76 deletions(-) diff --git a/crates/shelldeck-core/src/config/platform_attention.rs b/crates/shelldeck-core/src/config/platform_attention.rs index fb7a3f5d..e6ad3101 100644 --- a/crates/shelldeck-core/src/config/platform_attention.rs +++ b/crates/shelldeck-core/src/config/platform_attention.rs @@ -2743,7 +2743,7 @@ mod tests { Snapshot { mode: String, outcome: String, - snapshot: CorpusSnapshot, + snapshot: Box, }, Refusal { category: String, diff --git a/crates/shelldeck-core/src/config/platform_review/worktree.rs b/crates/shelldeck-core/src/config/platform_review/worktree.rs index 79ba257e..d56398bb 100644 --- a/crates/shelldeck-core/src/config/platform_review/worktree.rs +++ b/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() diff --git a/crates/shelldeck-ssh/src/tunnel.rs b/crates/shelldeck-ssh/src/tunnel.rs index bb0938e1..c614d1de 100644 --- a/crates/shelldeck-ssh/src/tunnel.rs +++ b/crates/shelldeck-ssh/src/tunnel.rs @@ -90,9 +90,15 @@ impl TunnelManager { remote_host: String, remote_port: u16, ) -> crate::Result { - 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 { - 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>> = + 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]; diff --git a/crates/shelldeck-ui/src/workspace/user_home.rs b/crates/shelldeck-ui/src/workspace/user_home.rs index 8a5c8dcc..a67cd02e 100644 --- a/crates/shelldeck-ui/src/workspace/user_home.rs +++ b/crates/shelldeck-ui/src/workspace/user_home.rs @@ -278,8 +278,10 @@ impl Workspace { Option, Vec, ) { - 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 = 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::>(); let open_requests = my_requests .iter() @@ -1631,7 +1631,8 @@ impl Workspace { cx: &mut Context, ) -> 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 = 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, _window, cx| { - let (_, others) = this.partition_user_sites(cx); let mut items: Vec = 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( diff --git a/deny.toml b/deny.toml index bea74793..b158b5fa 100644 --- a/deny.toml +++ b/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" }, diff --git a/docs/testing/tests-ssh.md b/docs/testing/tests-ssh.md index 0ef6bc90..87da0649 100644 --- a/docs/testing/tests-ssh.md +++ b/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. |