From cd3fb8cd2181749f827661361392ea91946021f8 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Fri, 24 Jul 2026 23:58:11 -0700 Subject: [PATCH 1/7] fix(remote-ssh): close sessions on disconnect and fix reconnect serialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping a russh `Handle` closes the socket without telling the peer, so the server keeps the session until its own timeout. With ProxyJump that is N+1 abandoned sessions per disconnect, one per hop, and they accumulate against the server's MaxSessions. Send SSH_MSG_DISCONNECT before dropping, target first then hops in reverse so each goodbye still has a channel to travel over. Applied to disconnect, disconnect_all, reconnect, re-connect under a live id, container listing and test_connection — the last two build sessions that are never registered anywhere, so nothing else could ever have closed them. Bounded at 1s per handle: the call queues onto a session loop that may be wedged on an unwritable socket, and this runs on app shutdown. Automatic reconnect also skips handles another task still holds, so config drift cannot kill an exec or PTY that is mid-flight. Reconnect serialization was broken in a way the review understated: a successful reconnect installs a *new* `alive` Arc, so the flag captured before taking the lock stays false forever and every queued waiter reconnects again — the exact stampede the lock exists to prevent. Move the locks out of ActiveConnection into a map keyed by connection id (the entry they lived in is replaced wholesale) and re-read liveness from the map under the lock. Also: - Attribute a failed connection to the exact hop with a transparent StagedError wrapper rather than matching message text, which blames the wrong host when two hops share a user@host:port label. Deliberately not an anyhow context: that would replace the message the user sees. - Refuse keyboard-interactive multi-hop without a per-hop IdentityFile instead of replaying one host's OTP at the next, and warn — in the log and in the connection dialog — when a bastion will receive the target's password. - Clean up orphaned scratch files left by an abruptly closed channel. Pid files are swept by liveness (`kill -0`), never age: a supervised command may run for hours and its pid file is how cancellation finds it. - Reach one more generation with `pkill -P` when `setsid` is unavailable and the process-group signal cannot work. - Say what to do in the local-container sshd validation error. --- .../src/remote_ssh/manager.rs | 563 ++++++++++++++---- .../ssh-remote/SSHConnectionDialog.scss | 4 + .../ssh-remote/SSHConnectionDialog.tsx | 11 + src/web-ui/src/locales/en-US/common.json | 1 + src/web-ui/src/locales/zh-CN/common.json | 1 + src/web-ui/src/locales/zh-TW/common.json | 1 + 6 files changed, 478 insertions(+), 103 deletions(-) diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 20272d53b..05dd8f010 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -209,9 +209,6 @@ struct ActiveConnection { /// Allows `is_connected` and SFTP/exec entry points to detect a dead session /// without waiting for the next failed I/O. alive: Arc, - /// Per-connection lock to serialize transparent reconnect attempts and - /// avoid stampedes when multiple SFTP/exec calls hit a dead session at once. - reconnect_lock: Arc>, } struct EstablishedSession { @@ -222,6 +219,53 @@ struct EstablishedSession { effective_config: SSHConnectionConfig, } +/// Which stage of the connection chain an error came from. +/// +/// `test_connection` needs this to attribute a failure exactly; matching on +/// message text cannot, because two jump hosts may share a `user@host:port` +/// label and the first match then always wins. +/// +/// Deliberately *not* an `anyhow` context: a context replaces the error's +/// `Display`, so tagging this way would turn every SSH error the user sees into +/// "connection stage 'target'". This wrapper is transparent instead — `Display` +/// and `source` both delegate, so `{}`, `{:#}` and `chain()` all read exactly as +/// they did before the tag existed. +#[derive(Debug)] +struct StagedError { + stage: String, + inner: anyhow::Error, +} + +impl std::fmt::Display for StagedError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.inner, formatter) + } +} + +impl std::error::Error for StagedError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.inner.source() + } +} + +/// Per-handle ceiling for the goodbye packet. Generous for a local channel +/// send, short enough that a chain of dead hops cannot stall shutdown. +const SSH_DISCONNECT_TIMEOUT: Duration = Duration::from_secs(1); + +fn tag_failed_stage(stage: &str, error: anyhow::Error) -> anyhow::Error { + anyhow::Error::new(StagedError { + stage: stage.to_string(), + inner: error, + }) +} + +/// Outermost stage tag in an error's chain, if any. +fn failed_stage_of(error: &anyhow::Error) -> Option<&str> { + error + .downcast_ref::() + .map(|staged| staged.stage.as_str()) +} + /// SSH client handler with host key verification struct SSHHandler { /// Expected host key (if connecting to known host) @@ -853,7 +897,8 @@ fn validate_container_config(container: &ContainerWorkspaceConfig) -> anyhow::Re } if container.local && matches!(container.access, ContainerAccess::Sshd) { anyhow::bail!( - "A local container with sshd must be configured as an SSH endpoint, not local Docker" + "A local Docker container cannot use sshd access; choose docker-exec or auto instead. \ + To reach a container over SSH, configure it as a remote SSH host." ); } Ok(()) @@ -1002,6 +1047,54 @@ fn workspace_command(config: &SSHConnectionConfig, command: &str, tty: bool) -> } } +/// Shell prelude that removes orphaned upload temporaries. +/// +/// The `trap cleanup EXIT` in the command below covers an orderly exit, but a +/// dropped SSH channel (network failure, laptop lid) kills the remote shell +/// without ever running it, so a long-lived host slowly accumulates orphans. +/// +/// A day is the bound because the only thing distinguishing an orphan from a +/// live upload is age, and the cost of guessing wrong is a corrupted transfer +/// in another session. No real upload runs for 24 hours; plenty run for one. +fn stale_upload_sweep(quoted_dir: &str) -> String { + format!( + "if command -v find >/dev/null 2>&1; then \ + find {quoted_dir} -maxdepth 1 -name '.bitfun-upload-*.tmp' -mmin +1440 \ + -exec rm -f -- {{}} \\; 2>/dev/null || true; \ + fi; " + ) +} + +/// Shell prelude that removes pid files whose process is gone. +/// +/// Liveness rather than age: a supervised command may legitimately run for +/// hours, and deleting its pid file would silently break cancellation for that +/// session — the pid file is how interrupt/kill finds the process. `kill -0` +/// answers the question exactly, so nothing live is ever touched. +/// +/// The empty-file case is the one exception. `supervised_container_command` +/// creates the file before writing the pid into it, so an empty file may belong +/// to a command that is starting right now; those are only removed once they +/// are far too old for that to be true. +fn stale_pid_file_sweep() -> String { + "for stale_pid_file in /tmp/.bitfun-exec-*.pid; do \ + [ -e \"$stale_pid_file\" ] || continue; \ + if [ ! -s \"$stale_pid_file\" ]; then \ + if command -v find >/dev/null 2>&1; then \ + find \"$stale_pid_file\" -mmin +60 -exec rm -f -- {} \\; 2>/dev/null || true; \ + fi; \ + continue; \ + fi; \ + stale_pid=$(cat \"$stale_pid_file\" 2>/dev/null || true); \ + case \"$stale_pid\" in \ + ''|*[!0-9]*) rm -f -- \"$stale_pid_file\" 2>/dev/null || true; continue;; \ + esac; \ + kill -0 \"$stale_pid\" 2>/dev/null || rm -f -- \"$stale_pid_file\" 2>/dev/null || true; \ + done; \ + unset stale_pid_file stale_pid 2>/dev/null || true; " + .to_string() +} + fn supervised_container_command( container: &ContainerWorkspaceConfig, command: &str, @@ -1011,6 +1104,14 @@ fn supervised_container_command( (wrapped, pid_file) } +/// Wrap `command` so it can be tracked and signalled from a separate channel. +/// +/// `setsid` gives the command its own process group, which is what makes the +/// `kill -- -$pid` group signal reach the whole tree. Where it is missing the +/// child stays in this shell's group, so a group signal would either fail or +/// hit the supervisor itself; both `terminate_child` here and +/// [`container_signal_command`] therefore fall back to `pkill -P` plus a direct +/// `kill`, which reaches one generation instead of all of them. fn supervised_container_command_with_pid_file( container: &ContainerWorkspaceConfig, command: &str, @@ -1019,17 +1120,21 @@ fn supervised_container_command_with_pid_file( let quoted_pid_file = crate::remote_ssh::shell::quote_arg(&pid_file); let quoted_command = crate::remote_ssh::shell::quote_arg(command); let quoted_shell = crate::remote_ssh::shell::quote_arg(&container.shell); + let sweep = stale_pid_file_sweep(); format!( - "pid_file={quoted_pid_file}; \ + "{sweep}\ + pid_file={quoted_pid_file}; \ child=; \ tracking=1; \ (umask 077; : > \"$pid_file\") 2>/dev/null || tracking=0; \ remove_pid_file() {{ rm -f -- \"$pid_file\" 2>/dev/null || true; }}; \ terminate_child() {{ \ [ -n \"$child\" ] || return 0; \ - kill -TERM -- \"-$child\" 2>/dev/null \ - || kill -TERM \"$child\" 2>/dev/null \ - || true; \ + if kill -TERM -- \"-$child\" 2>/dev/null; then return 0; fi; \ + if command -v pkill >/dev/null 2>&1; then \ + pkill -TERM -P \"$child\" 2>/dev/null || true; \ + fi; \ + kill -TERM \"$child\" 2>/dev/null || true; \ }}; \ trap remove_pid_file EXIT; \ trap 'terminate_child; exit 143' HUP TERM; \ @@ -1065,9 +1170,12 @@ fn container_signal_command( [ -s \"$pid_file\" ] || exit 75; \ pid=$(cat \"$pid_file\" 2>/dev/null) || exit 75; \ case \"$pid\" in ''|*[!0-9]*) exit 75;; esac; \ - kill -{signal_name} -- \"-$pid\" 2>/dev/null \ - || kill -{signal_name} \"$pid\" 2>/dev/null \ - || true" + if kill -{signal_name} -- \"-$pid\" 2>/dev/null; then :; else \ + if command -v pkill >/dev/null 2>&1; then \ + pkill -{signal_name} -P \"$pid\" 2>/dev/null || true; \ + fi; \ + kill -{signal_name} \"$pid\" 2>/dev/null || true; \ + fi" ) } @@ -1349,6 +1457,12 @@ fn parse_container_file_output( #[derive(Clone)] pub struct SSHConnectionManager { connections: Arc>>, + /// Reconnect serialization, keyed by connection id and deliberately held + /// *outside* `ActiveConnection`. Reconnect replaces the map entry wholesale, + /// so a lock stored inside it would be orphaned mid-flight: waiters would + /// hold a mutex nobody else takes and re-run the reconnect they were queued + /// behind. + reconnect_locks: Arc>>>>, saved_connections: Arc>>, config_path: std::path::PathBuf, /// Known hosts storage @@ -1369,6 +1483,7 @@ impl SSHConnectionManager { let password_vault = std::sync::Arc::new(SSHPasswordVault::new(data_dir)); Self { connections: Arc::new(tokio::sync::RwLock::new(HashMap::new())), + reconnect_locks: Arc::new(tokio::sync::RwLock::new(HashMap::new())), saved_connections: Arc::new(tokio::sync::RwLock::new(Vec::new())), config_path, known_hosts: Arc::new(tokio::sync::RwLock::new(HashMap::new())), @@ -1852,23 +1967,32 @@ impl SSHConnectionManager { ) .await .context("Could not connect to the Docker host")?; - let handle = established - .handle - .ok_or_else(|| anyhow!("Docker host SSH handle is unavailable"))?; - let command = format!( - "{} ps -a --format {}", - crate::remote_ssh::shell::quote_arg(&container.docker_path), - crate::remote_ssh::shell::quote_arg(format) - ); - let result = Self::execute_command_internal( - &handle, - &command, - SSHCommandOptions { - timeout_ms: Some(config.options.connect_timeout_secs.max(1) * 1000), - cancellation_token: None, - }, - ) - .await?; + // This session is never published to the connections map, so nothing + // else will ever close it: every exit below has to run through the + // shutdown, or listing containers leaks one server-side session per + // hop each time the connection dialog refreshes. + let outcome = match established.handle.as_ref() { + Some(handle) => { + let command = format!( + "{} ps -a --format {}", + crate::remote_ssh::shell::quote_arg(&container.docker_path), + crate::remote_ssh::shell::quote_arg(format) + ); + Self::execute_command_internal( + handle, + &command, + SSHCommandOptions { + timeout_ms: Some(config.options.connect_timeout_secs.max(1) * 1000), + cancellation_token: None, + }, + ) + .await + } + None => Err(anyhow!("Docker host SSH handle is unavailable")), + }; + Self::shutdown_established_session(established).await; + + let result = outcome?; if result.exit_code != 0 { anyhow::bail!( "Docker container listing failed on SSH host: {}", @@ -1949,28 +2073,43 @@ impl SSHConnectionManager { for stage in &mut stages { stage.success = true; } + let server_info = established.server_info.clone(); + let resolved_container_access = established + .effective_config + .container + .as_ref() + .map(|container| container.access.clone()); + // A test connection is never registered, so this is the only + // place that can close it. Without this the dialog's "Test" + // button leaks one server-side session per hop, per click. + Self::shutdown_established_session(established).await; ConnectionTestReport { success: true, stages, - server_info: established.server_info, - resolved_container_access: established - .effective_config - .container - .map(|container| container.access), + server_info, + resolved_container_access, } } Err(error) => { let error_text = error.to_string(); + let reported_stage = failed_stage_of(&error); let failing_index = stages .iter() - .position(|stage| { - (stage.id.starts_with("jump-") && error_text.contains(&stage.label)) - || (stage.id == "container" - && (error_text.to_ascii_lowercase().contains("docker container") - || error_text.to_ascii_lowercase().contains("container sshd"))) - || (stage.id == "docker-host" - && error_text.to_ascii_lowercase().contains("docker") - && !error_text.to_ascii_lowercase().contains("container")) + .position(|stage| match reported_stage { + // The chain marker is authoritative: it survives two hops + // sharing a label, which text matching does not. + Some(reported) => stage.id == reported, + None => { + (stage.id.starts_with("jump-") && error_text.contains(&stage.label)) + || (stage.id == "container" + && (error_text.to_ascii_lowercase().contains("docker container") + || error_text + .to_ascii_lowercase() + .contains("container sshd"))) + || (stage.id == "docker-host" + && error_text.to_ascii_lowercase().contains("docker") + && !error_text.to_ascii_lowercase().contains("container")) + } }) .unwrap_or_else(|| stages.len().saturating_sub(1)); for stage in stages.iter_mut().take(failing_index) { @@ -2313,21 +2452,27 @@ impl SSHConnectionManager { let connection_id = config.id.clone(); let server_info = established.server_info.clone(); - let mut guard = self.connections.write().await; - guard.insert( - connection_id.clone(), - ActiveConnection { - handle: established.handle.map(Arc::new), - jump_handles: established.jump_handles.into_iter().map(Arc::new).collect(), - config, - effective_config: established.effective_config, - server_info: server_info.clone(), - sftp_session: Arc::new(tokio::sync::RwLock::new(None)), - server_key: None, - alive: established.alive, - reconnect_lock: Arc::new(tokio::sync::Mutex::new(())), - }, - ); + let replaced = { + let mut guard = self.connections.write().await; + guard.insert( + connection_id.clone(), + ActiveConnection { + handle: established.handle.map(Arc::new), + jump_handles: established.jump_handles.into_iter().map(Arc::new).collect(), + config, + effective_config: established.effective_config, + server_info: server_info.clone(), + sftp_session: Arc::new(tokio::sync::RwLock::new(None)), + server_key: None, + alive: established.alive, + }, + ) + }; + // Reconnecting under an id that is still live would otherwise strand the + // previous session (plus one per jump host) on the server. + if let Some(previous) = replaced { + Self::shutdown_active_connection(previous).await; + } Ok(SSHConnectionResult { success: true, @@ -2357,6 +2502,10 @@ impl SSHConnectionManager { return Ok(session); } } + // Left untagged on purpose: this one call can fail as either stage + // (missing `docker` binary vs. a container that will not start), and + // the message-based fallback in `test_connection` already tells them + // apart. A blanket marker would make it always report "container". let server_info = self.probe_local_container(config, timeout_secs).await?; return Ok(EstablishedSession { handle: None, @@ -2367,10 +2516,15 @@ impl SSHConnectionManager { }); } - let jumps = self.resolve_proxy_jump_chain(config).await?; + let jumps = self + .resolve_proxy_jump_chain(config) + .await + .map_err(|error| tag_failed_stage("configuration", error))?; if jumps.is_empty() { - let (handle, alive, mut server_info) = - self.establish_direct_session(config, timeout_secs).await?; + let (handle, alive, mut server_info) = self + .establish_direct_session(config, timeout_secs) + .await + .map_err(|error| tag_failed_stage("target", error))?; if config .container .as_ref() @@ -2393,7 +2547,8 @@ impl SSHConnectionManager { server_info = self .probe_remote_container(&handle, config, timeout_secs) .await - .map(Some)?; + .map(Some) + .map_err(|error| tag_failed_stage("container", error))?; } return Ok(EstablishedSession { handle: Some(handle), @@ -2423,7 +2578,8 @@ impl SSHConnectionManager { "Jump 1 ({}) connection or authentication failed", connection_label(first) ) - })?; + }) + .map_err(|error| tag_failed_stage("jump-1", error))?; let mut jump_handles = vec![first_handle]; for (index, hop) in jumps.iter().enumerate().skip(1) { @@ -2440,7 +2596,8 @@ impl SSHConnectionManager { connection_label(hop), index ) - })?; + }) + .map_err(|error| tag_failed_stage(&format!("jump-{}", index + 1), error))?; let (handle, _, _) = self .establish_stream_session( hop, @@ -2455,7 +2612,8 @@ impl SSHConnectionManager { index + 1, connection_label(hop) ) - })?; + }) + .map_err(|error| tag_failed_stage(&format!("jump-{}", index + 1), error))?; jump_handles.push(handle); } @@ -2471,7 +2629,8 @@ impl SSHConnectionManager { connection_label(config), jump_handles.len() ) - })?; + }) + .map_err(|error| tag_failed_stage("target", error))?; let (handle, alive, mut server_info) = self .establish_stream_session( config, @@ -2485,7 +2644,8 @@ impl SSHConnectionManager { "Final target {} SSH handshake or authentication failed", connection_label(config) ) - })?; + }) + .map_err(|error| tag_failed_stage("target", error))?; if config .container .as_ref() @@ -2725,14 +2885,28 @@ impl SSHConnectionManager { .as_ref() .and_then(|entry| entry.certificate_file.clone()), } - } else if matches!( - &config.auth, - SSHAuthMethod::Password { .. } | SSHAuthMethod::KeyboardInteractive { .. } - ) { - // Explicit runtime challenge responses may also be needed by a - // bastion. Per-hop users still come from the jump token/config; - // keys and certificates remain independently configurable in - // each Host block. + } else if matches!(&config.auth, SSHAuthMethod::KeyboardInteractive { .. }) { + // Keyboard-interactive answers are one host's challenge/response + // exchange — an OTP accepted by the bastion is not an OTP for the + // next hop. Replaying the same vector would fail at hop 2 with an + // opaque auth error, so say what is actually missing instead. + anyhow::bail!( + "ProxyJump entry '{}' has no IdentityFile in ~/.ssh/config, and \ + keyboard-interactive responses cannot be reused across hops. \ + Add an IdentityFile for this jump host (or load its key into the \ + SSH agent) and try again.", + value + ); + } else if matches!(&config.auth, SSHAuthMethod::Password { .. }) { + // Falling back to the target's password is the only thing left + // when a bastion has no key configured, but it does hand that + // password to every hop — the UI warns about this. + log::warn!( + "ProxyJump entry '{}' has no IdentityFile; reusing the target's password \ + for this hop. Configure a per-host IdentityFile to avoid sending the \ + password to the bastion.", + value + ); config.auth.clone() } else { SSHAuthMethod::Agent { @@ -3193,17 +3367,114 @@ impl SSHConnectionManager { Ok(result) } + /// Per-connection reconnect mutex, created on first use. + async fn reconnect_lock_for(&self, connection_id: &str) -> Arc> { + if let Some(lock) = self.reconnect_locks.read().await.get(connection_id) { + return lock.clone(); + } + self.reconnect_locks + .write() + .await + .entry(connection_id.to_string()) + .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))) + .clone() + } + + /// Send SSH_MSG_DISCONNECT before letting a handle drop. + /// + /// Dropping a russh `Handle` only closes the socket. The server has no way + /// to tell that from a network partition, so it keeps the session — and its + /// PTYs, forwardings and `MaxSessions` slot — until its own timeout fires. + async fn shutdown_handle(handle: &Handle) { + // Bounded: `disconnect` queues onto the session loop's channel, and a + // loop wedged on an unwritable socket would never drain it. This runs on + // app shutdown and on every reconnect, neither of which may block on a + // peer that has stopped reading. A missed goodbye packet costs one + // server-side session until its timeout — the status quo — while a hang + // here costs the whole shutdown. + match tokio::time::timeout( + SSH_DISCONNECT_TIMEOUT, + handle.disconnect(russh::Disconnect::ByApplication, "client closing", "en"), + ) + .await + { + Ok(Ok(())) => {} + Ok(Err(error)) => { + log::debug!("SSH disconnect message could not be sent: {}", error) + } + Err(_) => log::debug!( + "SSH disconnect message timed out after {:?}; dropping the transport", + SSH_DISCONNECT_TIMEOUT + ), + } + } + + /// Tear down a whole hop chain, target first. + /// + /// Order matters with ProxyJump: every hop carries the next one's stream, so + /// closing hop 1 first would cut the channel the later disconnects still + /// have to travel over and turn an orderly shutdown back into N abandoned + /// sessions. + async fn shutdown_handle_chain(handles: Vec<&Handle>) { + for handle in handles { + Self::shutdown_handle(handle).await; + } + } + + /// Ordered handle chain of an `ActiveConnection`: target, then jump hosts + /// from the last hop backwards. + fn active_handle_chain(connection: &ActiveConnection) -> Vec<&Handle> { + let mut chain: Vec<&Handle> = Vec::with_capacity( + connection.jump_handles.len() + usize::from(connection.handle.is_some()), + ); + chain.extend(connection.handle.as_deref()); + chain.extend(connection.jump_handles.iter().rev().map(Arc::as_ref)); + chain + } + + /// Same ordering for a session that was never published to the map. + fn established_handle_chain(session: &EstablishedSession) -> Vec<&Handle> { + let mut chain: Vec<&Handle> = + Vec::with_capacity(session.jump_handles.len() + usize::from(session.handle.is_some())); + chain.extend(session.handle.as_ref()); + chain.extend(session.jump_handles.iter().rev()); + chain + } + + async fn shutdown_active_connection(connection: ActiveConnection) { + Self::shutdown_handle_chain(Self::active_handle_chain(&connection)).await; + } + + async fn shutdown_established_session(session: EstablishedSession) { + Self::shutdown_handle_chain(Self::established_handle_chain(&session)).await; + } + /// Disconnect from a server pub async fn disconnect(&self, connection_id: &str) -> anyhow::Result<()> { - let mut guard = self.connections.write().await; - guard.remove(connection_id); + // Drop the map lock before the network round-trip: the disconnect below + // awaits, and holding the write lock across it would block every other + // connection's exec/SFTP entry point. + let removed = { + let mut guard = self.connections.write().await; + guard.remove(connection_id) + }; + self.reconnect_locks.write().await.remove(connection_id); + if let Some(connection) = removed { + Self::shutdown_active_connection(connection).await; + } Ok(()) } /// Disconnect all connections pub async fn disconnect_all(&self) { - let mut guard = self.connections.write().await; - guard.clear(); + let removed: Vec = { + let mut guard = self.connections.write().await; + guard.drain().map(|(_, connection)| connection).collect() + }; + self.reconnect_locks.write().await.clear(); + for connection in removed { + Self::shutdown_active_connection(connection).await; + } } /// Check if connected. @@ -3332,20 +3603,13 @@ impl SSHConnectionManager { .load_connection_config_from_saved(connection_id) .await?; - let (alive_flag, reconnect_lock, active_config) = { + let reconnect_lock = self.reconnect_lock_for(connection_id).await; + let (alive_flag, active_config) = { let guard = self.connections.read().await; if let Some(conn) = guard.get(connection_id) { - ( - conn.alive.clone(), - conn.reconnect_lock.clone(), - Some(conn.config.clone()), - ) + (conn.alive.clone(), Some(conn.config.clone())) } else { - ( - Arc::new(AtomicBool::new(false)), - Arc::new(tokio::sync::Mutex::new(())), - None, - ) + (Arc::new(AtomicBool::new(false)), None) } }; @@ -3375,18 +3639,25 @@ impl SSHConnectionManager { // Serialize concurrent reconnect attempts for the same connection. let _guard = reconnect_lock.lock().await; - // Re-check under lock; another task may have already restored the session. - if alive_flag.load(Ordering::SeqCst) { - // Re-check config drift under lock as well. - if let Some(ref saved) = saved_config { - let guard = self.connections.read().await; - if let Some(conn) = guard.get(connection_id) { - if saved.connection_params_equal(&conn.config) { - return Ok(()); + // Re-check under lock; another task may have already restored the + // session. The flag has to be re-read from the map, not from the Arc + // captured above: a successful reconnect installs a *new* `alive` Arc, + // leaving the captured one false forever. Trusting it would make every + // waiter queued behind the lock reconnect all over again — the exact + // stampede this lock exists to prevent. + { + let guard = self.connections.read().await; + if let Some(conn) = guard.get(connection_id) { + if conn.alive.load(Ordering::SeqCst) { + match saved_config { + Some(ref saved) if saved.connection_params_equal(&conn.config) => { + return Ok(()) + } + None => return Ok(()), + // Still drifted: fall through and reconnect. + Some(_) => {} } } - } else { - return Ok(()); } } @@ -3450,9 +3721,15 @@ impl SSHConnectionManager { // Replace the handle, update the config to the latest saved version, // and clear the cached SFTP session so subsequent operations open a // fresh channel on the new transport. - { + // Handles the reconnect displaces still have to be told goodbye: the + // session is usually dead, but "usually" covers neither config drift + // (where the old session is perfectly healthy) nor a half-open link the + // server still counts against its session limit. + let (stale_handle, stale_jump_handles) = { let mut guard = self.connections.write().await; if let Some(conn) = guard.get_mut(connection_id) { + let stale_handle = conn.handle.take(); + let stale_jump_handles = std::mem::take(&mut conn.jump_handles); conn.handle = established.handle.map(Arc::new); conn.jump_handles = established.jump_handles.into_iter().map(Arc::new).collect(); conn.config = config; @@ -3463,8 +3740,9 @@ impl SSHConnectionManager { } let mut sftp_guard = conn.sftp_session.write().await; *sftp_guard = None; + (stale_handle, stale_jump_handles) } else { - guard.insert( + let replaced = guard.insert( connection_id.to_string(), ActiveConnection { handle: established.handle.map(Arc::new), @@ -3475,11 +3753,31 @@ impl SSHConnectionManager { sftp_session: Arc::new(tokio::sync::RwLock::new(None)), server_key: None, alive: established.alive, - reconnect_lock: Arc::new(tokio::sync::Mutex::new(())), }, ); + match replaced { + Some(previous) => (previous.handle, previous.jump_handles), + None => (None, Vec::new()), + } } - } + }; + // Skip any handle another task still holds. Reconnect is automatic and + // can fire on config drift while the *old* session is perfectly healthy + // and carrying an in-flight exec or PTY; sending a goodbye there would + // kill that command. Those handles simply drop when their user finishes, + // which is the pre-existing behaviour. An explicit `disconnect()` has no + // such guard — ending in-flight work is what the user asked for. + let unused = |handle: &&Arc>| Arc::strong_count(handle) == 1; + let mut stale_chain: Vec<&Handle> = Vec::new(); + stale_chain.extend(stale_handle.iter().filter(unused).map(Arc::as_ref)); + stale_chain.extend( + stale_jump_handles + .iter() + .rev() + .filter(|handle| Arc::strong_count(handle) == 1) + .map(Arc::as_ref), + ); + Self::shutdown_handle_chain(stale_chain).await; log::info!("SSH session {} reconnected successfully", connection_id); Ok(()) @@ -3940,9 +4238,12 @@ impl SSHConnectionManager { ); let quoted_temporary = crate::remote_ssh::shell::quote_arg(&temporary); let quoted_path = crate::remote_ssh::shell::quote_arg(&path); + let quoted_parent = crate::remote_ssh::shell::quote_arg(parent); let expected_size = content.len(); + let sweep = stale_upload_sweep("ed_parent); let command = format!( - "tmp={quoted_temporary}; target={quoted_path}; expected={expected_size}; \ + "{sweep} \ + tmp={quoted_temporary}; target={quoted_path}; expected={expected_size}; \ cleanup() {{ rm -f -- \"$tmp\"; }}; trap cleanup EXIT HUP INT TERM; \ umask 077; status=0; cat > \"$tmp\" || status=$?; \ if [ \"$status\" -eq 0 ]; then \ @@ -4596,6 +4897,10 @@ impl SSHConnectionManager { .container .as_ref() .expect("docker exec connection must have container config"); + // ` -lc ` looks like a shell inside a shell, but the + // outer one execs the inner as the final simple command of `-c`, so + // one process ends up on the PTY — with the login profile already + // sourced, which a bare `docker exec -it … sh` would skip. let command = docker_exec_host_command(container, &container.shell, true); channel .exec(false, command) @@ -5024,6 +5329,58 @@ mod tests { ); } + #[test] + #[cfg(unix)] + fn pid_file_sweep_spares_processes_that_are_still_running() { + // The whole point of the pid file is cancellation, so removing one that + // is still in use silently breaks interrupt/kill for a command that may + // legitimately run for hours. Liveness, never age. + let live = format!("/tmp/.bitfun-exec-live-{}.pid", uuid::Uuid::new_v4()); + let dead = format!("/tmp/.bitfun-exec-dead-{}.pid", uuid::Uuid::new_v4()); + std::fs::write(&live, std::process::id().to_string()).expect("write live pid file"); + // Reaped immediately, so its pid is guaranteed not to be running. + let mut corpse = std::process::Command::new("true") + .spawn() + .expect("spawn short-lived process"); + let dead_pid = corpse.id(); + corpse.wait().expect("reap"); + std::fs::write(&dead, dead_pid.to_string()).expect("write dead pid file"); + + let output = std::process::Command::new("sh") + .args(["-lc", &stale_pid_file_sweep()]) + .output() + .expect("run sweep"); + + assert!(output.status.success()); + let live_exists = std::path::Path::new(&live).exists(); + let dead_exists = std::path::Path::new(&dead).exists(); + let _ = std::fs::remove_file(&live); + let _ = std::fs::remove_file(&dead); + assert!(live_exists, "a running command's pid file must survive"); + assert!(!dead_exists, "an orphaned pid file must be removed"); + } + + #[test] + fn stage_tagging_is_readable_without_changing_the_error_text() { + let original = anyhow!("Jump 1 (deploy@bastion:22) connection or authentication failed") + .context("Could not connect to the Docker host"); + let expected_display = format!("{original}"); + let expected_alternate = format!("{original:#}"); + + let tagged = tag_failed_stage("jump-1", original); + + // The tag exists only for attribution. An anyhow *context* would have + // replaced the message the user sees with "connection stage 'jump-1'". + assert_eq!(failed_stage_of(&tagged), Some("jump-1")); + assert_eq!(format!("{tagged}"), expected_display); + assert_eq!(format!("{tagged:#}"), expected_alternate); + + // Still findable once a caller layers its own context on top. + let wrapped = tagged.context("Workspace connection failed"); + assert_eq!(failed_stage_of(&wrapped), Some("jump-1")); + assert_eq!(format!("{wrapped}"), "Workspace connection failed"); + } + #[test] fn supervised_container_command_tracks_and_signals_the_container_process_group() { let container = ContainerWorkspaceConfig { diff --git a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.scss b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.scss index 061814139..9733d24e3 100644 --- a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.scss +++ b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.scss @@ -387,6 +387,10 @@ font-size: 11px; line-height: 1.45; color: var(--color-text-muted); + + &--warning { + color: var(--color-warning); + } } &__browse-key { diff --git a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.tsx b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.tsx index 593eeb3b2..485afea8f 100644 --- a/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.tsx +++ b/src/web-ui/src/features/ssh-remote/SSHConnectionDialog.tsx @@ -1227,6 +1227,17 @@ export const SSHConnectionDialog: React.FC = ({
{t('ssh.remote.proxyJumpHint')}
+ {/* + A hop without its own IdentityFile falls back to this + connection's credentials, so the bastion sees the + target's password. Worth saying out loud before it is + sent, not after. + */} + {formData.proxyJump.trim() && formData.authType === 'password' && ( +
+ {t('ssh.remote.proxyJumpPasswordWarning')} +
+ )}
diff --git a/src/web-ui/src/locales/en-US/common.json b/src/web-ui/src/locales/en-US/common.json index cb2b8eda6..f30f8bfc3 100644 --- a/src/web-ui/src/locales/en-US/common.json +++ b/src/web-ui/src/locales/en-US/common.json @@ -1312,6 +1312,7 @@ "proxyJump": "Jump hosts (ProxyJump)", "proxyJumpPlaceholder": "jump1,jump2", "proxyJumpHint": "Comma-separated SSH config aliases or [user@]host[:port]. Each hop can use its own User and IdentityFile from ~/.ssh/config.", + "proxyJumpPasswordWarning": "A jump host with no IdentityFile in ~/.ssh/config will be sent this password. Give each hop its own key if you do not fully trust it.", "via": "via", "containerName": "Container name or ID", "containerNamePlaceholder": "my-dev-container", diff --git a/src/web-ui/src/locales/zh-CN/common.json b/src/web-ui/src/locales/zh-CN/common.json index 001efcb6c..bf6a9c36b 100644 --- a/src/web-ui/src/locales/zh-CN/common.json +++ b/src/web-ui/src/locales/zh-CN/common.json @@ -1312,6 +1312,7 @@ "proxyJump": "跳板链（ProxyJump）", "proxyJumpPlaceholder": "jump1,jump2", "proxyJumpHint": "使用逗号分隔 SSH 配置别名或 [user@]host[:port]。每一跳可从 ~/.ssh/config 使用独立的 User 和 IdentityFile。", + "proxyJumpPasswordWarning": "若跳板机在 ~/.ssh/config 中没有配置 IdentityFile，该密码会被发送给它。如果不完全信任该跳板机，请为每一跳单独配置密钥。", "via": "经由", "containerName": "容器名称或 ID", "containerNamePlaceholder": "my-dev-container", diff --git a/src/web-ui/src/locales/zh-TW/common.json b/src/web-ui/src/locales/zh-TW/common.json index 12b3ac8f2..60eb07858 100644 --- a/src/web-ui/src/locales/zh-TW/common.json +++ b/src/web-ui/src/locales/zh-TW/common.json @@ -1312,6 +1312,7 @@ "proxyJump": "跳板鏈（ProxyJump）", "proxyJumpPlaceholder": "jump1,jump2", "proxyJumpHint": "使用逗號分隔 SSH 設定別名或 [user@]host[:port]。每一跳可從 ~/.ssh/config 使用獨立的 User 和 IdentityFile。", + "proxyJumpPasswordWarning": "若跳板機在 ~/.ssh/config 中沒有設定 IdentityFile，該密碼會被傳送給它。如果不完全信任該跳板機，請為每一跳單獨設定金鑰。", "via": "經由", "containerName": "容器名稱或 ID", "containerNamePlaceholder": "my-dev-container", From 129cbdb5a45e0376b6094d70a5fd07f0efcbf0f3 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Fri, 24 Jul 2026 23:58:27 -0700 Subject: [PATCH 2/7] fix(relay): stop mis-detecting China and verify the Docker install script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Region detection let a heuristic override a definitive answer. A resolved country of US/DE/JP fell through to the connectivity probe, which reports "CN" whenever mirrors.aliyun.com is reachable and github.com is not — and mirrors.aliyun.com answers from anywhere while GitHub has outages, rate limits and corporate blocks. One transient GitHub failure was enough to rewrite a Frankfurt VPS's apt sources to Aliyun, point Docker at docker.1ms.run and route GitHub clones through a third-party proxy. Treat any resolved country code as authoritative; run the heuristics only when there is no answer at all. In CN mode the Docker install script defaulted to a jsDelivr copy of docker/docker-install@master — a floating ref, edge-cached for hours, fetched over a third-party CDN and then executed as root with no verification at all. Require either a pinned BITFUN_DOCKER_INSTALL_SHA256 or byte-identical content from a second independent origin. Fails closed, with BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED=1 as the explicit opt-out; the Aliyun docker-ce path is GPG-verified and already covers Debian/Ubuntu/RHEL. The host apt list was written with a hardcoded `https://`. Minimal cloud images use `http://` sources precisely because ca-certificates is not installed, and mirror init runs before anything is installed — so the rewrite broke the very apt-get update that would have installed the CA bundle, with a TLS error that says nothing about mirrors. Pick the scheme from what the host can actually verify; package signatures are what protect apt either way. Also: - Probe apt-get update after rewriting sources and restore the originals on failure, so a bad mirror is reported as a bad mirror instead of surfacing three steps later as an unattributed error. - Give unsupported distros the same BitFun-owned-list-plus-rename treatment, so restore_apt can actually roll them back; an in-place sed left the host pinned with only a timestamped backup to find by hand. Never overwrite an existing .bitfun-disabled — on a re-run that would replace the real upstream sources with already-mirrored content. - Clean up docker.list/docker.asc on every failure path, refuse an empty GPG key, and stop returning success after a failed install — which made the caller skip its fallback and fail later at `systemctl enable --now docker`. - Prefer HTTPS for country lookups; the answer decides which mirrors a host gets, so a tamperable plain-HTTP body should not be the first choice. - Skip re-detection when the mode is already resolved in this shell. - Drop the redundant [registries.*] cargo table and git-fetch-with-cli, which the relay builder has no `git` to honour. --- src/apps/relay-server/Dockerfile | 14 +- src/apps/relay-server/mirror.sh | 348 +++++++++++++++++++++++++++---- 2 files changed, 311 insertions(+), 51 deletions(-) diff --git a/src/apps/relay-server/Dockerfile b/src/apps/relay-server/Dockerfile index b3bafb000..f0d185445 100644 --- a/src/apps/relay-server/Dockerfile +++ b/src/apps/relay-server/Dockerfile @@ -44,18 +44,20 @@ RUN set -eux; \ /etc/apt/sources.list.d/debian.sources; \ fi; \ mkdir -p /usr/local/cargo; \ + # `[source.*]` alone is what redirects crates.io. A matching + # `[registries.*]` table would only matter for crates that name the + # registry explicitly, which none here do — and an unused named registry + # is something Cargo may start warning about. + # + # No `git-fetch-with-cli` either: `git` is not installed in this builder, + # so it would turn any future git dependency into a CN-only build failure + # while global builds kept working. printf '%s\n' \ '[source.crates-io]' \ 'replace-with = "bitfun-rsproxy-sparse"' \ '' \ '[source.bitfun-rsproxy-sparse]' \ "registry = \"${BITFUN_CARGO_SPARSE_URL}\"" \ - '' \ - '[registries.bitfun-rsproxy-sparse]' \ - "index = \"${BITFUN_CARGO_SPARSE_URL}\"" \ - '' \ - '[net]' \ - 'git-fetch-with-cli = true' \ > /usr/local/cargo/config.toml; \ fi diff --git a/src/apps/relay-server/mirror.sh b/src/apps/relay-server/mirror.sh index 7bb6a7d9d..47cbf5833 100644 --- a/src/apps/relay-server/mirror.sh +++ b/src/apps/relay-server/mirror.sh @@ -147,25 +147,37 @@ bitfun_mirror_country_via_bash_tcp() { return 1 } +# Country lookups, HTTPS first. +# +# The answer decides whether this host gets Chinese apt/Docker/GitHub mirrors, +# so an on-path attacker who can rewrite a plain-HTTP body can choose that for +# us. The HTTP endpoints are kept only as a last resort for hosts where TLS is +# unavailable, and the mode they produce is announced as unverified. bitfun_mirror_detect_country() { - local code="" - code="$(bitfun_mirror_http_body "https://ipinfo.io/country" 3 | tr -d '[:space:]' | tr '[:lower:]' '[:upper:]')" - if [ "${#code}" -eq 2 ]; then - echo "$code" - return 0 - fi + local code="" endpoint + for endpoint in \ + "https://ipinfo.io/country" \ + "https://ifconfig.co/country-iso" \ + "https://api.country.is/"; do + code="$(bitfun_mirror_http_body "$endpoint" 3 \ + | tr -d '[:space:]' \ + | tr '[:lower:]' '[:upper:]' \ + | sed -n 's/.*"COUNTRY":"\([A-Z][A-Z]\)".*/\1/p;s/^\([A-Z][A-Z]\)$/\1/p' \ + | head -n 1)" + if [ "${#code}" -eq 2 ]; then + echo "$code" + return 0 + fi + done code="$(bitfun_mirror_http_body "http://ip-api.com/line/?fields=countryCode" 3 | tr -d '[:space:]' | tr '[:lower:]' '[:upper:]')" if [ "${#code}" -eq 2 ]; then - echo "$code" - return 0 - fi - code="$(bitfun_mirror_http_body "https://ifconfig.co/country-iso" 3 | tr -d '[:space:]' | tr '[:lower:]' '[:upper:]')" - if [ "${#code}" -eq 2 ]; then + echo ">>> Region detect: only the unauthenticated HTTP lookup answered" >&2 echo "$code" return 0 fi code="$(bitfun_mirror_country_via_bash_tcp 2>/dev/null || true)" if [ "${#code}" -eq 2 ]; then + echo ">>> Region detect: only the unauthenticated HTTP lookup answered" >&2 echo "$code" return 0 fi @@ -216,6 +228,21 @@ bitfun_mirror_resolve_mode() { ;; esac + # Already resolved in this shell (or exported by the caller). Detection costs + # up to four HTTP lookups plus two 4s probes, and mirror.sh is initialised at + # several points of one deploy; re-running it there is both slow and unstable, + # because a network blip mid-deploy could flip the mode between steps. + case "${BITFUN_MIRROR_MODE:-}" in + cn) + export BITFUN_USE_CN_MIRROR=1 + return 0 + ;; + global) + export BITFUN_USE_CN_MIRROR=0 + return 0 + ;; + esac + local country="" country="$(bitfun_mirror_detect_country || true)" if [ "$country" = "CN" ]; then @@ -225,6 +252,18 @@ bitfun_mirror_resolve_mode() { return 0 fi + # A resolved country code is a direct answer, so stop here. The heuristics + # below exist for the case where there is no answer at all; letting them + # override one turns a transient GitHub outage on a Frankfurt VPS into + # rewritten apt sources, a Chinese Docker registry and GitHub traffic through + # a third-party proxy — because mirrors.aliyun.com answers from anywhere. + if [ -n "$country" ]; then + echo ">>> Region detect: public IP country=${country} → global mirrors" + export BITFUN_MIRROR_MODE=global + export BITFUN_USE_CN_MIRROR=0 + return 0 + fi + if bitfun_mirror_timezone_suggests_cn; then echo ">>> Region detect: timezone suggests mainland China → China mirrors" export BITFUN_MIRROR_MODE=cn @@ -239,11 +278,7 @@ bitfun_mirror_resolve_mode() { return 0 fi - if [ -n "$country" ]; then - echo ">>> Region detect: public IP country=${country} → global mirrors" - else - echo ">>> Region detect: inconclusive → global mirrors" - fi + echo ">>> Region detect: inconclusive → global mirrors" export BITFUN_MIRROR_MODE=global export BITFUN_USE_CN_MIRROR=0 return 0 @@ -351,8 +386,40 @@ bitfun_mirror_file_cksum() { || bitfun_mirror_priv cksum "$path" 2>/dev/null | awk '{print $1 " " $2}' } +# Scheme for the apt mirror we are about to write. +# +# `bitfun_mirror_init` runs before anything is installed, and minimal cloud +# images ship without `ca-certificates` — that is precisely why their stock +# sources are `http://`. Writing `https://` there breaks the very `apt-get +# update` that would install the CA bundle, and it fails with a TLS error that +# says nothing about mirrors. Package signatures are what protect apt, so http +# costs integrity nothing; use https only when the host can actually verify it. +bitfun_mirror_apt_scheme() { + if [ -n "${BITFUN_APT_SCHEME:-}" ]; then + printf '%s' "$BITFUN_APT_SCHEME" + return 0 + fi + if [ -s /etc/ssl/certs/ca-certificates.crt ] \ + || [ -s /etc/pki/tls/certs/ca-bundle.crt ]; then + if [ -f /usr/lib/apt/methods/https ] || [ -f /usr/libexec/apt/methods/https ]; then + printf 'https' + return 0 + fi + # Modern apt has https built into the http method; only older releases ship + # a separate binary, so treat its absence as conclusive only there. + if ! command -v apt-get >/dev/null 2>&1 \ + || apt-get --version 2>/dev/null | grep -Eq 'apt 2\.|apt 1\.[6-9]'; then + printf 'https' + return 0 + fi + fi + printf 'http' +} + bitfun_mirror_apply_apt_debian_family() { local mirror="${BITFUN_APT_MIRROR:-mirrors.aliyun.com}" + local scheme + scheme="$(bitfun_mirror_apt_scheme)" local id="" version_codename="" id_like="" # shellcheck disable=SC1091 . /etc/os-release 2>/dev/null || true @@ -403,35 +470,57 @@ bitfun_mirror_apply_apt_debian_family() { ubuntu) cat >"$tmp" <"$tmp" <>> apt mirror: skip (no /etc/apt/sources.list to rewrite)" + return 0 + fi + { + echo "# Managed by BitFun relay deploy (China mirrors). Safe to delete to revert." sed -e "s|deb.debian.org/debian|${mirror}/debian|g" \ -e "s|security.debian.org/debian-security|${mirror}/debian-security|g" \ -e "s|archive.ubuntu.com/ubuntu|${mirror}/ubuntu|g" \ -e "s|security.ubuntu.com/ubuntu|${mirror}/ubuntu|g" \ - /etc/apt/sources.list >"$rewritten" - bitfun_mirror_priv cp "$rewritten" /etc/apt/sources.list - rm -f "$rewritten" + /etc/apt/sources.list + } >"$tmp" + bitfun_mirror_priv cp "$tmp" "$list_file" + rm -f "$tmp" + bitfun_mirror_backup_file /etc/apt/sources.list + if [ -e /etc/apt/sources.list.bitfun-disabled ]; then + # A previous deploy already saved the real upstream sources here. + # Overwriting it would destroy the only copy `restore_apt` can put back, + # so drop the current file (already mirrored, and preserved as a + # timestamped backup above) instead of promoting it to "the original". + bitfun_mirror_priv rm -f /etc/apt/sources.list 2>/dev/null || true + elif ! bitfun_mirror_priv mv /etc/apt/sources.list /etc/apt/sources.list.bitfun-disabled; then + # Could not disable the original, so both lists would be active and the + # overseas hosts would still be tried. Undo instead of half-applying. + bitfun_mirror_priv rm -f "$list_file" 2>/dev/null || true + echo ">>> apt mirror: could not disable /etc/apt/sources.list; left untouched" >&2 + return 1 fi - echo ">>> apt mirror: rewrote common upstream hosts → ${mirror}" + echo ">>> apt mirror: rewrote common upstream hosts → ${mirror} (${list_file})" return 0 ;; esac @@ -452,6 +541,20 @@ EOF echo ">>> apt mirror: enabled ${list_file} → ${mirror}" } +# Bounded `apt-get update` against whatever sources are currently active. +bitfun_mirror_probe_apt() { + local timeout_prefix="" + if command -v timeout >/dev/null 2>&1; then + timeout_prefix="timeout 180" + fi + # shellcheck disable=SC2086 + bitfun_mirror_priv $timeout_prefix apt-get \ + -o Acquire::Retries=1 \ + -o Acquire::http::Timeout=15 \ + -o Acquire::https::Timeout=15 \ + update >/dev/null 2>&1 +} + bitfun_mirror_apply_apt() { if ! command -v apt-get >/dev/null 2>&1; then return 0 @@ -459,7 +562,25 @@ bitfun_mirror_apply_apt() { if [ ! -f /etc/os-release ]; then return 0 fi - bitfun_mirror_apply_apt_debian_family || echo ">>> apt mirror: apply failed (continuing)" >&2 + if ! bitfun_mirror_apply_apt_debian_family; then + echo ">>> apt mirror: apply failed (continuing)" >&2 + return 0 + fi + + # A write that succeeds still proves nothing about the mirror. Without this + # probe the first symptom is an `apt-get update` failure several steps later, + # with nothing tying it back to the sources BitFun swapped in. + if bitfun_mirror_probe_apt; then + return 0 + fi + echo ">>> apt mirror: ${BITFUN_APT_MIRROR:-mirrors.aliyun.com} did not answer a test \ +apt-get update; restoring the previous sources" >&2 + bitfun_mirror_restore_apt || true + if ! bitfun_mirror_probe_apt; then + echo ">>> apt mirror: apt-get update still fails after restoring the original \ +sources, so the failure is not the mirror" >&2 + fi + return 0 } bitfun_mirror_write_docker_daemon_json() { @@ -895,6 +1016,16 @@ bitfun_mirror_apply_host() { bitfun_mirror_chown_to_home_owner "$HOME/.bitfun" "$HOME/.bitfun/mirror-mode" } +# Undo a partially applied Aliyun docker-ce repository. +# +# The caller falls back to get.docker.com when this install fails, and that path +# runs its own `apt-get update` — which would pick up a half-written docker.list +# or an unusable docker.asc and fail on those instead, hiding the real error. +bitfun_mirror_cleanup_docker_aliyun_apt() { + bitfun_mirror_priv rm -f /etc/apt/sources.list.d/docker.list /etc/apt/keyrings/docker.asc \ + 2>/dev/null || true +} + # Install Docker Engine from Aliyun docker-ce (CN). Returns 0 on success. bitfun_mirror_install_docker_aliyun() { if ! command -v apt-get >/dev/null 2>&1 && ! command -v dnf >/dev/null 2>&1 && ! command -v yum >/dev/null 2>&1; then @@ -924,16 +1055,50 @@ bitfun_mirror_install_docker_aliyun() { ;; esac [ -n "$version_codename" ] || return 1 - bitfun_mirror_priv apt-get update -y - bitfun_mirror_priv apt-get install -y ca-certificates curl - bitfun_mirror_priv install -m 0755 -d /etc/apt/keyrings - curl -fsSL --retry 3 "https://mirrors.aliyun.com/docker-ce/linux/${docker_ce_distro}/gpg" \ - | bitfun_mirror_priv tee /etc/apt/keyrings/docker.asc >/dev/null - bitfun_mirror_priv chmod a+r /etc/apt/keyrings/docker.asc + + bitfun_mirror_priv apt-get update -y || true + bitfun_mirror_priv apt-get install -y ca-certificates curl || { + echo ">>> Aliyun docker-ce: could not install ca-certificates/curl" >&2 + return 1 + } + bitfun_mirror_priv install -m 0755 -d /etc/apt/keyrings || return 1 + + local key_tmp + key_tmp="$(mktemp)" + if ! curl -fsSL --retry 3 "https://mirrors.aliyun.com/docker-ce/linux/${docker_ce_distro}/gpg" \ + -o "$key_tmp" || [ ! -s "$key_tmp" ]; then + # An empty or missing key silently produces a repository apt can never + # verify, so stop before it is written anywhere. + rm -f "$key_tmp" + echo ">>> Aliyun docker-ce: GPG key download failed or was empty" >&2 + return 1 + fi + bitfun_mirror_priv cp "$key_tmp" /etc/apt/keyrings/docker.asc || { + rm -f "$key_tmp" + bitfun_mirror_cleanup_docker_aliyun_apt + return 1 + } + rm -f "$key_tmp" + bitfun_mirror_priv chmod a+r /etc/apt/keyrings/docker.asc || true echo "deb [arch=${arch} signed-by=/etc/apt/keyrings/docker.asc] https://mirrors.aliyun.com/docker-ce/linux/${docker_ce_distro} ${version_codename} stable" \ - | bitfun_mirror_priv tee /etc/apt/sources.list.d/docker.list >/dev/null - bitfun_mirror_priv apt-get update -y - bitfun_mirror_priv apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin + | bitfun_mirror_priv tee /etc/apt/sources.list.d/docker.list >/dev/null || { + bitfun_mirror_cleanup_docker_aliyun_apt + return 1 + } + if ! bitfun_mirror_priv apt-get update -y; then + bitfun_mirror_cleanup_docker_aliyun_apt + echo ">>> Aliyun docker-ce: apt-get update failed for the docker-ce repository" >&2 + return 1 + fi + # `return 0` unconditionally here would report success on a failed install, + # and the caller would skip its fallback and fail later at `systemctl enable + # --now docker` with an error that says nothing about apt. + if ! bitfun_mirror_priv apt-get install -y docker-ce docker-ce-cli containerd.io \ + docker-buildx-plugin docker-compose-plugin; then + bitfun_mirror_cleanup_docker_aliyun_apt + echo ">>> Aliyun docker-ce: package installation failed" >&2 + return 1 + fi return 0 fi @@ -948,18 +1113,111 @@ enabled=1 gpgcheck=1 gpgkey=https://mirrors.aliyun.com/docker-ce/linux/centos/gpg EOF - bitfun_mirror_priv "$pkg" install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin + if ! bitfun_mirror_priv "$pkg" install -y docker-ce docker-ce-cli containerd.io \ + docker-buildx-plugin docker-compose-plugin; then + bitfun_mirror_priv rm -f /etc/yum.repos.d/docker-ce.repo 2>/dev/null || true + echo ">>> Aliyun docker-ce: package installation failed" >&2 + return 1 + fi return 0 fi return 1 } -# Download get.docker.com script to $1 using CN-aware URL. +bitfun_mirror_sha256_of() { + if command -v sha256sum >/dev/null 2>&1; then + sha256sum "$1" 2>/dev/null | awk '{print $1}' + elif command -v shasum >/dev/null 2>&1; then + shasum -a 256 "$1" 2>/dev/null | awk '{print $1}' + else + return 1 + fi +} + +# Download the Docker install script to $1 using the CN-aware URL, and prove it +# is the upstream script before anyone runs it. +# +# The CN default is a jsDelivr copy of `docker/docker-install@master`: a floating +# ref, edge-cached for hours, fetched over a third-party CDN — and the result is +# executed as root. Nothing about that path is authenticated, so a compromise +# anywhere along it is a root shell on every host deployed while it lasts. +# +# Two ways out, in order: +# * `BITFUN_DOCKER_INSTALL_SHA256` — an operator-pinned digest, checked exactly. +# * cross-origin agreement — the same script fetched from a second, independent +# origin must hash identically. One compromised mirror is then not enough. +# +# Neither is available on a host that can only reach the CN CDN, so +# `BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED=1` exists as a deliberate, logged +# opt-out. It is not the default: the Aliyun docker-ce path above is GPG-verified +# and covers Debian/Ubuntu/RHEL, so failing closed here costs little. bitfun_mirror_fetch_docker_install_script() { local dest="$1" local url="${BITFUN_DOCKER_GET_URL:-https://get.docker.com}" + local expected="${BITFUN_DOCKER_INSTALL_SHA256:-}" + local actual="" reference="" reference_url="" verified=0 + echo ">>> Fetching Docker install script: ${url}" - curl -fsSL --retry 3 "$url" -o "$dest" + curl -fsSL --retry 3 "$url" -o "$dest" || return 1 + + actual="$(bitfun_mirror_sha256_of "$dest" || true)" + if [ -z "$actual" ]; then + echo ">>> Docker install script: no sha256 tool available; cannot verify" >&2 + elif [ -n "$expected" ]; then + if [ "$actual" = "$expected" ]; then + verified=1 + else + echo ">>> Docker install script: sha256 ${actual} does not match the pinned \ +${expected}; refusing to run it" >&2 + rm -f "$dest" + return 1 + fi + else + local candidate + for candidate in \ + "https://get.docker.com" \ + "https://raw.githubusercontent.com/docker/docker-install/master/install.sh"; do + [ "$candidate" = "$url" ] && continue + if curl -fsSL --retry 1 --max-time 30 "$candidate" -o "${dest}.crosscheck" 2>/dev/null; then + reference="$(bitfun_mirror_sha256_of "${dest}.crosscheck" || true)" + rm -f "${dest}.crosscheck" + if [ -n "$reference" ] && [ "$reference" = "$actual" ]; then + verified=1 + reference_url="$candidate" + break + fi + if [ -n "$reference" ]; then + echo ">>> Docker install script: ${candidate} serves different bytes \ +(${reference} vs ${actual}); refusing to run it" >&2 + rm -f "$dest" + return 1 + fi + fi + rm -f "${dest}.crosscheck" + done + fi + + if [ "$verified" -eq 1 ]; then + if [ -n "$reference_url" ]; then + echo ">>> Docker install script verified against ${reference_url} (sha256 ${actual})" + else + echo ">>> Docker install script matches the pinned sha256" + fi + return 0 + fi + + if [ "${BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED:-0}" = "1" ]; then + echo ">>> WARNING: running an unverified Docker install script from ${url} \ +because BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED=1 (sha256 ${actual:-unknown})" >&2 + return 0 + fi + + echo ">>> Docker install script from ${url} could not be verified against an \ +independent origin. Install Docker with the distribution's own packages, set \ +BITFUN_DOCKER_INSTALL_SHA256=${actual:-} to pin this exact script, or set \ +BITFUN_DOCKER_INSTALL_ALLOW_UNVERIFIED=1 to accept the risk." >&2 + rm -f "$dest" + return 1 } bitfun_mirror_init() { From 77f2d176e04ce55e3def7055a1bc51e73e789465 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Fri, 24 Jul 2026 23:58:39 -0700 Subject: [PATCH 3/7] fix(release): bound updater memory, surface failures, and verify mirrored assets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The archive is read fully into memory with no ceiling, and the checksum can only be verified once the whole body has arrived — so a hostile or misconfigured origin can stream until the process is OOM-killed without any integrity check ever running. Cap it. Automatic updates install from a detached child whose output goes to /dev/null, so a repeatedly failing update was indistinguishable from one that never ran. The child now records why it gave up and the next interactive launch reports it. InstallLock used check-then-write, leaving a window in which two `bitfun update` processes both see no lock — inside the very protection against interleaved binary swaps. Use create_new, clearing only a stale lock. The openbitfun sync script mirrored release assets without ever running the checksum it mirrors alongside them. Verifying an archive against a sidecar from the same host proves nothing if both were copied from a bad download, so a corrupt archive would be published and every downstream install would fail against a checksum that matched. Check before publishing the manifest, and delete a mismatched archive so the next run re-fetches instead of skipping it. Scoped to the manifest's own assets: the directory also holds Desktop packages whose sidecar format this script does not own. Also: - Report progress during long downloads instead of showing nothing for minutes. - Attach rollback failures to the error. `let _ = fs::rename(...)` hid the one case that matters — the difference between "the update did not apply" and "there is no working bitfun on this machine any more". - Resolve the systemd user unit through dirs::config_dir() so XDG_CONFIG_HOME is honoured, still checking ~/.config so a unit from an older install is found. --- scripts/openbitfun-release-sync.sh | 50 ++++++ src/apps/cli/src/self_update.rs | 240 +++++++++++++++++++++++++---- 2 files changed, 260 insertions(+), 30 deletions(-) diff --git a/scripts/openbitfun-release-sync.sh b/scripts/openbitfun-release-sync.sh index da4bd680a..8c66881d8 100755 --- a/scripts/openbitfun-release-sync.sh +++ b/scripts/openbitfun-release-sync.sh @@ -89,6 +89,46 @@ download_asset() { fi } +# Check the mirrored Linux archives against the `.sha256` sidecars mirrored with +# them. Reads the filename list on stdin, one per line. +# +# Scoped to the manifest's own assets rather than globbing `*.sha256`: the same +# directory also holds the Desktop packages, whose sidecars this script does not +# own and must not assume the format of. Deleting a good archive because a +# foreign sidecar was laid out differently would be worse than not checking. +# +# A mismatch means this run pulled a bad copy from GitHub, so the archive is +# removed: `download_asset` skips files that already exist, and leaving a corrupt +# one in place would make every later run treat it as done. Removing it lets the +# next run re-fetch and self-heal. +verify_mirrored_checksums() { + local filename sidecar archive expected actual failed=0 + while IFS= read -r filename; do + [ -n "$filename" ] || continue + case "$filename" in + *.sha256) ;; + *) continue ;; + esac + sidecar="${VERSION_DIR}/${filename}" + archive="${sidecar%.sha256}" + if [ ! -f "$sidecar" ] || [ ! -f "$archive" ]; then + continue + fi + expected="$(awk '{print $1; exit}' "$sidecar")" + actual="$(sha256sum "$archive" | awk '{print $1}')" + if [ "$expected" != "$actual" ]; then + log "ERROR: checksum mismatch for $(basename "$archive"): expected $expected, got $actual" + rm -f "$archive" + failed=1 + fi + done + if [ "$failed" -ne 0 ]; then + log "ERROR: refusing to publish a manifest for unverified assets" + return 1 + fi + return 0 +} + # Fetch linux-binaries.json into $LINUX_MANIFEST_TMP, retrying transient # failures. Sets LINUX_MANIFEST_STATE to one of: # ok — downloaded @@ -166,6 +206,16 @@ PY download_asset "$url" "${VERSION_DIR}/${filename}" || exit 1 done <<< "$LINUX_ASSET_LIST" + # Verify before publishing the manifest that points at these bytes. + # + # Clients check an archive against the checksum served next to it. When both + # come from here, mirroring a corrupted archive alongside its original + # checksum is the one failure that verification cannot catch — every + # downstream install would then fail, or worse, succeed on bad bytes if the + # corruption also reached the sidecar. Checking here is the only place the + # two copies can still be compared. + cut -f2 <<< "$LINUX_ASSET_LIST" | verify_mirrored_checksums || exit 1 + "$PYTHON" - "$LINUX_MANIFEST_TMP" "${VERSION_DIR}/linux-binaries.json" \ "$OPENBITFUN_BASE_URL" <<'PY' import json, sys diff --git a/src/apps/cli/src/self_update.rs b/src/apps/cli/src/self_update.rs index 3788d158b..d0256b5be 100644 --- a/src/apps/cli/src/self_update.rs +++ b/src/apps/cli/src/self_update.rs @@ -40,6 +40,14 @@ const READ_TIMEOUT: Duration = Duration::from_secs(30); const MANIFEST_TIMEOUT: Duration = Duration::from_secs(20); /// Ceiling for the whole startup-path check, which precedes the first paint. const AUTO_CHECK_BUDGET: Duration = Duration::from_secs(10); +/// Hard ceiling on an archive held in memory. The checksum can only be verified +/// once the whole body has arrived, so without a cap a hostile or misconfigured +/// origin can stream until the process is OOM-killed and no integrity check ever +/// runs. Official CLI archives are tens of megabytes; this leaves ample room. +const MAX_ARCHIVE_BYTES: usize = 512 * 1024 * 1024; +/// How often a long download reports progress. Silence for minutes is +/// indistinguishable from a hang. +const PROGRESS_INTERVAL: Duration = Duration::from_secs(5); #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] @@ -81,7 +89,16 @@ pub(crate) async fn run_manual(check_only: bool) -> Result { } else { Some(InstallLock::acquire()?) }; - let outcome = update_from_configured_sources(check_only).await?; + let result = update_from_configured_sources(check_only).await; + // A background install writes to /dev/null, so this file is the only way its + // failure ever reaches the user — the next launch reports it. + if is_background_install() { + match &result { + Err(error) => record_background_failure(&format!("{error:#}")), + Ok(_) => clear_background_failure(), + } + } + let outcome = result?; match outcome { UpdateOutcome::Current => { println!("BitFun CLI is up to date ({}).", env!("CARGO_PKG_VERSION")) @@ -105,7 +122,14 @@ pub(crate) async fn run_manual(check_only: bool) -> Result { /// must never sit behind a transfer whose duration is set by the user's /// bandwidth; the previous inline download could hold the TUI for minutes. pub(crate) async fn maybe_run_automatic() { - if !automatic_update_is_eligible() || !automatic_check_is_due() { + if !automatic_update_is_eligible() { + return; + } + // Reported before the due-time gate, so a failure is not sat on for the rest + // of the six-hour interval. Previously an automatic update could fail every + // time and the user would only ever see it as "the version never changes". + report_background_failure(); + if !automatic_check_is_due() { return; } mark_automatic_check(); @@ -152,6 +176,7 @@ fn spawn_detached_install() -> Result { let exe = std::env::current_exe().context("resolve current BitFun CLI executable")?; Command::new(exe) .arg("update") + .env(BACKGROUND_INSTALL_ENV, "1") .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -160,6 +185,53 @@ fn spawn_detached_install() -> Result { Ok(true) } +/// Marks the detached child so it knows to leave a failure behind for the next +/// interactive launch to report. +const BACKGROUND_INSTALL_ENV: &str = "BITFUN_CLI_BACKGROUND_UPDATE"; + +fn is_background_install() -> bool { + std::env::var_os(BACKGROUND_INSTALL_ENV).is_some() +} + +fn background_failure_path() -> Option { + crate::config::CliConfig::config_dir() + .ok() + .map(|dir| dir.join("update-last-error")) +} + +fn record_background_failure(message: &str) { + let Some(path) = background_failure_path() else { + return; + }; + if let Some(parent) = path.parent() { + let _ = fs::create_dir_all(parent); + } + let _ = fs::write(path, message); +} + +fn clear_background_failure() { + if let Some(path) = background_failure_path() { + let _ = fs::remove_file(path); + } +} + +/// Print (once) the reason the last background install gave up. +fn report_background_failure() { + let Some(path) = background_failure_path() else { + return; + }; + let Ok(message) = fs::read_to_string(&path) else { + return; + }; + let _ = fs::remove_file(&path); + let message = message.trim(); + if message.is_empty() { + return; + } + eprintln!("The last background BitFun CLI update failed: {message}"); + eprintln!("Run `bitfun update` to retry."); +} + /// Guards against two `bitfun update` runs swapping the binaries at once. struct InstallLock { path: PathBuf, @@ -190,16 +262,39 @@ impl InstallLock { fn acquire() -> Result { let path = Self::path().ok_or_else(|| anyhow!("cannot resolve the CLI update lock location"))?; - if Self::is_held() { - return Err(anyhow!( - "another BitFun CLI update is already running ({})", - path.display() - )); + if let Some(parent) = path.parent() { + let _ = fs::create_dir_all(parent); + } + // `create_new` is the whole point: a check-then-write leaves a window in + // which two `bitfun update` processes both see no lock, and interleaving + // their backup/stage/swap renames can leave no working binary at all. + // Only a stale lock is cleared, and only then is the create retried. + match fs::OpenOptions::new().write(true).create_new(true).open(&path) { + Ok(mut file) => { + use std::io::Write as _; + let _ = write!(file, "{}", std::process::id()); + Ok(Self { path }) + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + if Self::is_held() { + return Err(anyhow!( + "another BitFun CLI update is already running ({})", + path.display() + )); + } + fs::remove_file(&path) + .with_context(|| format!("clear stale lock {}", path.display()))?; + let mut file = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&path) + .with_context(|| format!("create {}", path.display()))?; + use std::io::Write as _; + let _ = write!(file, "{}", std::process::id()); + Ok(Self { path }) + } + Err(error) => Err(error).with_context(|| format!("create {}", path.display())), } - let _ = fs::remove_file(&path); - fs::write(&path, std::process::id().to_string()) - .with_context(|| format!("create {}", path.display()))?; - Ok(Self { path }) } } @@ -630,15 +725,32 @@ async fn stream_with_stall_guard( buffer: &mut Vec, url: &str, ) -> Result<()> { + // `Content-Length` is the server's claim, not a promise, so it only shapes + // the progress line; the hard limit below is what actually bounds memory. + let expected_total = response + .content_length() + .map(|remaining| remaining as usize + buffer.len()); let mut stream = response.bytes_stream(); let mut window_start = Instant::now(); let mut window_bytes: u64 = 0; + let mut last_report = Instant::now(); while let Some(chunk) = stream.next().await { let chunk = chunk.with_context(|| format!("read {url}"))?; + if buffer.len() + chunk.len() > MAX_ARCHIVE_BYTES { + return Err(anyhow!( + "release archive exceeds the {} MB ceiling; refusing to keep reading", + MAX_ARCHIVE_BYTES / (1024 * 1024) + )); + } buffer.extend_from_slice(&chunk); window_bytes += chunk.len() as u64; + if last_report.elapsed() >= PROGRESS_INTERVAL { + report_progress(buffer.len(), expected_total); + last_report = Instant::now(); + } + let elapsed = window_start.elapsed(); if elapsed >= STALL_WINDOW { let rate = window_bytes / elapsed.as_secs().max(1); @@ -656,6 +768,19 @@ async fn stream_with_stall_guard( Ok(()) } +fn report_progress(downloaded: usize, expected_total: Option) { + let megabytes = |bytes: usize| bytes as f64 / (1024.0 * 1024.0); + match expected_total { + Some(total) if total > 0 => eprintln!( + " downloaded {:.1} MB of {:.1} MB ({}%)", + megabytes(downloaded), + megabytes(total), + downloaded.saturating_mul(100) / total + ), + _ => eprintln!(" downloaded {:.1} MB", megabytes(downloaded)), + } +} + /// Download `url`, resuming with a Range request when a previous source left a /// partial body. Every source serves an identical artifact, so partial progress /// carries across sources; a bad resume is caught by the checksum. @@ -794,28 +919,74 @@ fn install_archive(archive: &[u8], current_exe: &Path) -> Result<()> { let primary_backup = stage.path().join("previous-bitfun"); let legacy_backup = stage.path().join("previous-bitfun-cli"); + + // Rollback runs while something has already gone wrong, so its own failures + // are the ones that matter most: they are the difference between "the update + // did not apply" and "there is no working `bitfun` on this machine any + // more". Swallowing them leaves the user with a broken install and no clue. + let mut rollback_failures: Vec = Vec::new(); + let restore = |from: &Path, to: &Path, rollback_failures: &mut Vec| { + if let Err(error) = fs::rename(from, to) { + rollback_failures.push(format!("restore {}: {error}", to.display())); + } + }; + let rollback_error = |error: std::io::Error, step: &str, failures: Vec| { + let base = anyhow!(error).context(step.to_string()); + if failures.is_empty() { + return base; + } + base.context(format!( + "the previous CLI could NOT be put back ({}); reinstall BitFun manually", + failures.join("; ") + )) + }; + fs::rename(current_exe, &primary_backup).context("back up current bitfun")?; if let Err(error) = fs::rename(&legacy_target, &legacy_backup) { - let _ = fs::rename(&primary_backup, current_exe); - return Err(error).context("back up current bitfun-cli"); + restore(&primary_backup, current_exe, &mut rollback_failures); + return Err(rollback_error( + error, + "back up current bitfun-cli", + rollback_failures, + )); } if let Err(error) = fs::rename(&staged_primary, current_exe) { - let _ = fs::rename(&legacy_backup, &legacy_target); - let _ = fs::rename(&primary_backup, current_exe); - return Err(error).context("install updated bitfun"); + restore(&legacy_backup, &legacy_target, &mut rollback_failures); + restore(&primary_backup, current_exe, &mut rollback_failures); + return Err(rollback_error( + error, + "install updated bitfun", + rollback_failures, + )); } if let Err(error) = fs::rename(&staged_legacy, &legacy_target) { - let _ = fs::remove_file(current_exe); - let _ = fs::rename(&legacy_backup, &legacy_target); - let _ = fs::rename(&primary_backup, current_exe); - return Err(error).context("install updated bitfun-cli"); + if let Err(remove_error) = fs::remove_file(current_exe) { + rollback_failures.push(format!("remove {}: {remove_error}", current_exe.display())); + } + restore(&legacy_backup, &legacy_target, &mut rollback_failures); + restore(&primary_backup, current_exe, &mut rollback_failures); + return Err(rollback_error( + error, + "install updated bitfun-cli", + rollback_failures, + )); } if let Err(error) = validate_entrypoint_pair(current_exe, &legacy_target) { - let _ = fs::remove_file(current_exe); - let _ = fs::remove_file(&legacy_target); - let _ = fs::rename(&legacy_backup, &legacy_target); - let _ = fs::rename(&primary_backup, current_exe); - return Err(error).context("validate installed CLI update"); + for path in [current_exe, legacy_target.as_path()] { + if let Err(remove_error) = fs::remove_file(path) { + rollback_failures.push(format!("remove {}: {remove_error}", path.display())); + } + } + restore(&legacy_backup, &legacy_target, &mut rollback_failures); + restore(&primary_backup, current_exe, &mut rollback_failures); + let failed = error.context("validate installed CLI update"); + if rollback_failures.is_empty() { + return Err(failed); + } + return Err(failed.context(format!( + "the previous CLI could NOT be put back ({}); reinstall BitFun manually", + rollback_failures.join("; ") + ))); } Ok(()) } @@ -931,11 +1102,20 @@ fn mark_automatic_check() { } fn restart_managed_daemon() { - let Some(home) = dirs::home_dir() else { - return; - }; - let unit = home.join(".config/systemd/user/bitfun-cli-daemon.service"); - if unit.is_file() { + // `dirs::config_dir()` honours `XDG_CONFIG_HOME`, which is where systemd + // --user actually looks. Hard-coding `~/.config` missed the unit on any host + // that relocates it, and the daemon then kept running the old binary. + // + // Both are checked because an install predating this could have written the + // unit to `~/.config` on a host that sets `XDG_CONFIG_HOME` elsewhere; a + // `try-restart` for a unit systemd does not know is a harmless no-op, while + // missing one leaves a stale daemon behind. + let candidates = [dirs::config_dir(), dirs::home_dir().map(|it| it.join(".config"))]; + let installed = candidates + .iter() + .flatten() + .any(|dir| dir.join("systemd/user/bitfun-cli-daemon.service").is_file()); + if installed { let _ = Command::new("systemctl") .args(["--user", "try-restart", "bitfun-cli-daemon.service"]) .status(); From ea13f89f0f8c2382c68dcffd352075492f9271d7 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Fri, 24 Jul 2026 23:58:48 -0700 Subject: [PATCH 4/7] perf(web): resolve mouse glow inside the frame and stop the crossing flicker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleViewportChange is registered for scroll with capture: true, so it fires for every scroll event from every scrollable descendant — well over 60 a second on a trackpad — and each call synchronously ran document.elementFromPoint, which forces a layout. The rAF was throttled; the layout-forcing work was not. Flag the work in the handler and do the hit test once, in the frame callback. Crossing between adjacent surfaces hard-hid the overlay: pointerout/pointermove deactivated synchronously, then the next frame showed the new surface, leaving one frame with no glow on every card-to-card move. Let the frame decide instead — it hides only when no surface is found. findSurface read getComputedStyle twice per element: once for the ignore/resize guard over the whole path, once in the surface search. One pass, one style read, identical semantics. Also: - Emit after initialize() reads the stored preference, so a subscriber that attached first is not left showing the default — a toggle reading "on" while the glow is off. - Drop hue-rotate(4deg) from the light-theme filter chain. Four degrees is invisible once saturate has clipped the blue channel, so it was pure cost on an element repainted every pointer frame. saturate/brightness stay: they are what the light-theme look is, and no color-mix of the accent token reproduces them. --- .../mouse-glow/MouseGlowEffect.scss | 11 +- .../mouse-glow/core/MouseGlowService.test.ts | 59 ++++++++- .../mouse-glow/core/MouseGlowService.ts | 113 ++++++++++++------ 3 files changed, 142 insertions(+), 41 deletions(-) diff --git a/src/web-ui/src/infrastructure/mouse-glow/MouseGlowEffect.scss b/src/web-ui/src/infrastructure/mouse-glow/MouseGlowEffect.scss index a06beac53..21f6c8e5c 100644 --- a/src/web-ui/src/infrastructure/mouse-glow/MouseGlowEffect.scss +++ b/src/web-ui/src/infrastructure/mouse-glow/MouseGlowEffect.scss @@ -80,8 +80,17 @@ transparent ); + /** + * Every filter here re-rasterizes an element whose background is rewritten on + * each pointer frame, so the chain is kept as short as the look allows. + * + * `saturate`/`brightness` are load-bearing: on a light background the plain + * accent mix reads as washed-out grey-blue, and no `color-mix` of the accent + * token reproduces the boosted result. `hue-rotate(4deg)` did not survive that + * test — a four-degree shift is invisible once `saturate` has already clipped + * the blue channel, so it was pure cost. + */ filter: - hue-rotate(4deg) saturate(2.65) brightness(1.2) drop-shadow(0 0 8px var(--mouse-glow-shadow)); diff --git a/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.test.ts b/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.test.ts index 3f2a84432..929e096d9 100644 --- a/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.test.ts +++ b/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.test.ts @@ -112,7 +112,7 @@ describe('MouseGlowService', () => { surface.remove(); }); - it('clears the previous glow immediately when the pointer leaves its surface', () => { + it('keeps the glow up until the frame resolves where the pointer went', () => { const surface = document.createElement('div'); surface.setAttribute('data-mouse-glow-surface', ''); surface.getBoundingClientRect = () => ({ @@ -146,12 +146,69 @@ describe('MouseGlowService', () => { clientY: 68, })); + // Deliberately still visible: hiding here and showing the next surface one + // frame later blanked the glow for a frame on every card-to-card crossing. + expect(overlay?.hasAttribute('data-active')).toBe(true); + + nextFrame?.(0); + expect(overlay?.hasAttribute('data-active')).toBe(false); expect(overlay?.hidden).toBe(true); surface.remove(); plainElement.remove(); }); + it('moves the glow between adjacent surfaces without a hidden frame', () => { + const makeSurface = (left: number): HTMLElement => { + const element = document.createElement('div'); + element.setAttribute('data-mouse-glow-surface', ''); + element.getBoundingClientRect = () => ({ + bottom: 128, + height: 80, + left, + right: left + 200, + top: 48, + width: 200, + x: left, + y: 48, + toJSON: () => ({}), + }); + return element; + }; + const first = makeSurface(20); + const second = makeSurface(240); + document.body.append(first, second); + service.initialize(); + + first.dispatchEvent(new MouseEvent('pointermove', { + bubbles: true, + clientX: 72, + clientY: 68, + })); + nextFrame?.(0); + + const overlay = document.getElementById('bitfun-mouse-glow-overlay'); + expect(overlay?.hasAttribute('data-active')).toBe(true); + + first.dispatchEvent(new PointerEvent('pointerout', { + bubbles: true, + relatedTarget: second, + })); + expect(overlay?.hasAttribute('data-active')).toBe(true); + + second.dispatchEvent(new MouseEvent('pointermove', { + bubbles: true, + clientX: 300, + clientY: 68, + })); + nextFrame?.(0); + + expect(overlay?.hasAttribute('data-active')).toBe(true); + expect(overlay?.hidden).toBe(false); + first.remove(); + second.remove(); + }); + it('clears the glow when the pointer enters an iframe', () => { const surface = document.createElement('div'); surface.setAttribute('data-mouse-glow-surface', ''); diff --git a/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.ts b/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.ts index 4e25a2914..0a8ee1612 100644 --- a/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.ts +++ b/src/web-ui/src/infrastructure/mouse-glow/core/MouseGlowService.ts @@ -40,6 +40,8 @@ export class MouseGlowService { private pointerY = 0; private pendingElements: HTMLElement[] | null = null; private pendingSurface: HTMLElement | null = null; + /// Set by scroll/resize, consumed by the next frame. See `handleViewportChange`. + private resolveFromPointerPosition = false; private activeSurface: HTMLElement | null = null; private overlay: HTMLDivElement | null = null; private reducedMotionQuery: MediaQueryList | null = null; @@ -51,11 +53,18 @@ export class MouseGlowService { } this.initialized = true; + const previousEnabled = this.enabled; this.enabled = this.readStoredPreference(); this.reducedMotionQuery = window.matchMedia?.('(prefers-reduced-motion: reduce)') ?? null; this.overlay = this.ensureOverlay(); this.applyEnabledState(); + // Subscribers that attached before the stored preference was read are still + // showing the default. Without this the settings toggle can sit on "on" + // while the glow is off, for as long as nothing else emits. + if (previousEnabled !== this.enabled) { + this.emit(); + } window.addEventListener('pointermove', this.handlePointerMove, { passive: true }); window.addEventListener('pointerout', this.handlePointerOut, { passive: true }); window.addEventListener('resize', this.handleViewportChange, { passive: true }); @@ -115,13 +124,12 @@ export class MouseGlowService { this.pointerX = event.clientX; this.pointerY = event.clientY; const path = event.composedPath?.() ?? []; - const elements = path.filter( + // Leaving the old surface visible until the frame resolves the new one: + // hiding here first made the glow disappear for a frame every time the + // pointer crossed from one card to the one next to it. + this.pendingElements = path.filter( (item): item is HTMLElement => item instanceof HTMLElement ); - if (this.activeSurface && !elements.includes(this.activeSurface)) { - this.deactivateSurface(); - } - this.pendingElements = elements; this.pendingSurface = null; this.scheduleFrame(); }; @@ -133,11 +141,19 @@ export class MouseGlowService { return; } - if ( - !(relatedTarget instanceof Node) - || (this.activeSurface && !this.activeSurface.contains(relatedTarget)) - ) { + if (!(relatedTarget instanceof Node)) { this.deactivateSurface(); + return; + } + + if (this.activeSurface && !this.activeSurface.contains(relatedTarget)) { + // Same reason as above — resolve where the pointer went instead of + // blanking the overlay and waiting for the next `pointermove`. + this.pendingElements = relatedTarget instanceof Element + ? this.getAncestorElements(relatedTarget) + : null; + this.pendingSurface = null; + this.scheduleFrame(); } }; @@ -146,11 +162,11 @@ export class MouseGlowService { return; } - const pointerTarget = document.elementFromPoint?.(this.pointerX, this.pointerY) ?? null; - this.pendingElements = pointerTarget - ? this.getAncestorElements(pointerTarget) - : null; - this.pendingSurface = pointerTarget ? null : this.activeSurface; + // Only flag the work. `elementFromPoint` forces a layout, and this listener + // is registered with `capture: true`, so it runs for every scroll event from + // every scrollable descendant — well over 60 a second on a trackpad. Doing + // the hit test in the frame callback collapses that back to once per frame. + this.resolveFromPointerPosition = true; this.scheduleFrame(); }; @@ -189,6 +205,7 @@ export class MouseGlowService { } this.pendingElements = null; this.pendingSurface = null; + this.resolveFromPointerPosition = false; this.deactivateSurface(); } @@ -210,10 +227,21 @@ export class MouseGlowService { this.frameId = window.requestAnimationFrame(() => { this.frameId = null; + if (this.resolveFromPointerPosition) { + this.resolveFromPointerPosition = false; + // Post-scroll layout, so this is the authoritative answer for where the + // pointer now is — it supersedes anything a `pointermove` queued. + const pointerTarget = document.elementFromPoint?.(this.pointerX, this.pointerY) ?? null; + this.pendingElements = pointerTarget + ? this.getAncestorElements(pointerTarget) + : null; + this.pendingSurface = pointerTarget ? null : this.activeSurface; + } const surface = this.pendingElements ? this.findSurface(this.pendingElements) : this.pendingSurface; this.pendingElements = null; + this.pendingSurface = null; this.updateOverlay(surface); }); } @@ -339,30 +367,39 @@ export class MouseGlowService { } private findSurface(elements: HTMLElement[]): HTMLElement | null { - if ( - elements.some(element => - element.hasAttribute('data-mouse-glow-ignore') - || this.isResizeInteractionElement(element) - ) - ) { - return null; - } - - // The event path is ordered from the deepest target outward, so the first - // matching visual boundary is also the most specific one under the pointer. - return elements.find(element => { - if (this.isDividerSurface(element)) { - return true; + // Single pass, one `getComputedStyle` per element. The resize/ignore guard + // has to see the whole path while the surface search stops at the first + // match, but both need the same style object — reading it in two separate + // passes doubled the style work on every frame. + let surface: HTMLElement | null = null; + + for (const element of elements) { + const style = window.getComputedStyle(element); + if (element.hasAttribute('data-mouse-glow-ignore') || this.isResizeCursor(style)) { + return null; } - if (element.hasAttribute('data-mouse-glow-surface')) { - return true; + if (surface) { + continue; } - const hasSemanticClass = this.hasSemanticSurfaceClass(element); - return this.isAutomaticSurface(element, hasSemanticClass); - }) ?? null; + // The event path is ordered from the deepest target outward, so the first + // matching visual boundary is also the most specific one under the pointer. + if ( + this.isDividerSurface(element, style) + || element.hasAttribute('data-mouse-glow-surface') + || this.isAutomaticSurface(element, style, this.hasSemanticSurfaceClass(element)) + ) { + surface = element; + } + } + + return surface; } - private isAutomaticSurface(element: HTMLElement, hasSemanticClass: boolean): boolean { + private isAutomaticSurface( + element: HTMLElement, + style: CSSStyleDeclaration, + hasSemanticClass: boolean, + ): boolean { if ( element === document.body || element === this.overlay @@ -376,7 +413,6 @@ export class MouseGlowService { return false; } - const style = window.getComputedStyle(element); if (!this.isVisibleElement(style)) { return false; } @@ -396,7 +432,7 @@ export class MouseGlowService { return hasSemanticClass && hasRoundedCorners && hasBackground; } - private isDividerSurface(element: HTMLElement): boolean { + private isDividerSurface(element: HTMLElement, style: CSSStyleDeclaration): boolean { if ( element === document.body || element === this.overlay @@ -406,7 +442,6 @@ export class MouseGlowService { } const rect = element.getBoundingClientRect(); - const style = window.getComputedStyle(element); if (!this.isVisibleElement(style)) { return false; } @@ -508,8 +543,8 @@ export class MouseGlowService { return style.boxShadow !== 'none' && /\binset\b/i.test(style.boxShadow); } - private isResizeInteractionElement(element: HTMLElement): boolean { - return /(?:^|-)resize$/i.test(window.getComputedStyle(element).cursor); + private isResizeCursor(style: CSSStyleDeclaration): boolean { + return /(?:^|-)resize$/i.test(style.cursor); } private findOverlayHost(surface: HTMLElement): HTMLElement { From 57de5ecedfa3b8a888adb8f239cb630d9ea34da4 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Fri, 24 Jul 2026 23:58:59 -0700 Subject: [PATCH 5/7] fix(web): ref-count the modal scroll lock and stop redundant search re-layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Modals stack — a dialog opening a confirmation on top of itself is routine — and each cleared body overflow on its own, so closing the inner one re-enabled background scroll while the outer was still open. The 180ms exit delay widened the window. Ref-count the lock across instances. The centralized interactive-surface rule re-asserted its property list with !important, which forced all eight properties onto components that had chosen a longer duration for one of them, and made `transition: none` unattainable — so an element being dragged animated its way to the pointer. Drop it and let the zero-specificity :where() default lose to component-owned transitions, which is the contract it should have had; [data-motion='none'] remains the opt-out. Search navigation re-armed a rAF on every successful frame once any collapsed content had been expanded, because the flag that recorded the expansion was never reset. The loop only stopped at the 24-attempt ceiling, so ~22 frames re-ran a full DOM tree-walk, re-highlight, ancestor getComputedStyle scan and scroll recompute to reproduce a position that had already converged. Re-arm only while the resolved position is still moving. Also build the folded search text from the same per-character lowercasing the offsets are built from. Whole-string toLowerCase is not guaranteed to agree in length, and any divergence leaves holes in the offset array — the lookup then returns undefined and a real match is silently dropped. --- src/web-ui/src/app/styles/motion.scss | 17 +++++---- .../components/Modal/Modal.tsx | 36 +++++++++++++++---- .../components/modern/VirtualMessageList.tsx | 18 +++++++++- .../components/modern/flowChatSearchDom.ts | 16 ++++++--- 4 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/web-ui/src/app/styles/motion.scss b/src/web-ui/src/app/styles/motion.scss index fc2dd5380..855792b76 100644 --- a/src/web-ui/src/app/styles/motion.scss +++ b/src/web-ui/src/app/styles/motion.scss @@ -79,6 +79,14 @@ * * Individual transform properties compose with component-owned `transform` * (icons, drag handles, toggles) instead of replacing it. + * + * Override contract: this is a zero-specificity `:where()` default, so any + * component that declares its own `transition` owns it completely. That is + * deliberate. The property list used to be re-asserted with `!important`, + * which forced all eight properties onto components that had chosen a longer + * duration for one of them — and made `transition: none` unattainable, so an + * element being dragged animated its way to the pointer. Opt out entirely + * with `[data-motion='none']`. */ :where( button, @@ -104,15 +112,6 @@ color var(--motion-fast) var(--easing-standard), box-shadow var(--motion-fast) var(--easing-standard), opacity var(--motion-fast) var(--easing-standard); - transition-property: - translate, - scale, - transform, - background-color, - border-color, - color, - box-shadow, - opacity !important; } @media (hover: hover) and (pointer: fine) { diff --git a/src/web-ui/src/component-library/components/Modal/Modal.tsx b/src/web-ui/src/component-library/components/Modal/Modal.tsx index b6b9cfdb3..ae2b86d44 100644 --- a/src/web-ui/src/component-library/components/Modal/Modal.tsx +++ b/src/web-ui/src/component-library/components/Modal/Modal.tsx @@ -10,6 +10,32 @@ import './Modal.scss'; // Keep in sync with modal-overlay-exit/modal-dialog-exit in Modal.scss. const MODAL_EXIT_DURATION_MS = 180; +/** + * Ref-counted `body` scroll lock, shared by every Modal instance. + * + * Modals stack — a dialog opening a confirmation on top of itself is routine — + * and each one clearing `overflow` on its own would unlock the page while the + * others are still open. The 180 ms exit delay widens that window: the inner + * modal's cleanup lands well after it has visually gone. + */ +let bodyScrollLockCount = 0; + +function lockBodyScroll(): () => void { + bodyScrollLockCount += 1; + document.body.style.overflow = 'hidden'; + let released = false; + return () => { + if (released) { + return; + } + released = true; + bodyScrollLockCount = Math.max(0, bodyScrollLockCount - 1); + if (bodyScrollLockCount === 0) { + document.body.style.overflow = ''; + } + }; +} + export interface ModalProps { isOpen: boolean; onClose: () => void; @@ -89,15 +115,11 @@ export const Modal: React.FC = ({ }, [isOpen, isPresent]); useEffect(() => { - if (isOpen || isPresent) { - document.body.style.overflow = 'hidden'; - } else { - document.body.style.overflow = ''; + if (!isOpen && !isPresent) { + return; } - return () => { - document.body.style.overflow = ''; - }; + return lockBodyScroll(); }, [isOpen, isPresent]); useEffect(() => { diff --git a/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.tsx b/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.tsx index 871b003c4..196f75e53 100644 --- a/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.tsx +++ b/src/web-ui/src/flow_chat/components/modern/VirtualMessageList.tsx @@ -3639,6 +3639,13 @@ const VirtualMessageListSession = forwardRef { if (searchNavigationRequestIdRef.current !== requestId) { return; @@ -3773,7 +3780,16 @@ const VirtualMessageListSession = forwardRef Date: Fri, 24 Jul 2026 23:59:10 -0700 Subject: [PATCH 6/7] fix(desktop): narrow remote tool detection and tidy sleep-prevention lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remote_tool_missing matched any message containing the tool name and "not found", so `tar: link: Not found in archive` was reported as "Remote server does not have 'tar' installed" — telling the user to install something they already have while hiding the real cause. Restrict to shell-level phrasing. get_prevent_sleep_enabled started a thread and took an OS inhibitor as a side effect of a read, blocking on D-Bus to do it. spawn_config_listener already owns runtime state at startup and on every config update, so the getter can just read. Its worker round-trip is now bounded: keepawake is a blocking D-Bus call to logind with no timeout of its own, and a hung session manager would hold the mutex forever, wedging every later toggle including the one that turns the feature off. Also: - Reject `..` in the parent of a remote archive path, not only in the file name; the parent is cd-ed into before the command runs. - Give `git rev-parse` on a date filter a timeout, and log when a value resolves to the current second — approxidate never rejects input, so a typo silently becomes a filter that matches nothing. - Extract the prevent-sleep rollback so it can be tested, and cover the failed save, failed rollback and ConfigReloaded paths. - Record why recursion_limit = "256" stays. Measured rather than assumed: at 128 the build fails on Vec>: Send, ~15 frames of which are inside h2 itself. Box::pin cannot collapse third-party depth; only erasing a mid-chain future to Pin> would, at the cost of an allocation and dynamic dispatch on the dialog-turn path. --- src/apps/desktop/src/api/commands.rs | 59 ++++- src/apps/desktop/src/lib.rs | 15 ++ src/apps/desktop/src/sleep_prevention.rs | 206 +++++++++++++++--- .../services-integrations/src/git/service.rs | 38 +++- .../services-integrations/src/git/utils.rs | 63 ++++++ 5 files changed, 350 insertions(+), 31 deletions(-) diff --git a/src/apps/desktop/src/api/commands.rs b/src/apps/desktop/src/api/commands.rs index 6fd23fb7b..7d6b84f56 100644 --- a/src/apps/desktop/src/api/commands.rs +++ b/src/apps/desktop/src/api/commands.rs @@ -3206,6 +3206,22 @@ fn split_remote_archive_path(path: &str) -> Result<(String, String), String> { return Err(format!("Cannot determine file name of '{}'", path)); } + // The parent is `cd`-ed into before the archive command runs, so traversal + // has to be rejected here too, not only in the file name. + // + // Only `..`. A `.` component is not traversal and `./name` has always been + // accepted — the resolved parent for a bare `name` is literally "." — so + // rejecting it would break paths that work today for no security gain. + // + // Checked against the input rather than the resolved parent, which is + // synthesized for relative paths. + if trimmed.split('/').rev().skip(1).any(|component| component == "..") { + return Err(format!( + "Remote path '{}' must not contain '..' components", + path + )); + } + Ok((parent, base_name)) } @@ -3217,11 +3233,22 @@ fn join_remote_path(parent: &str, name: &str) -> String { } } +/// Whether a remote command failed because the tool itself is absent. +/// +/// Only shell-level phrasing counts. A tool that ran and then complained is not +/// a missing tool: `tar: link: Not found in archive` mentions both "tar" and +/// "not found", and treating that as absence tells the user to install +/// something they already have while hiding the real cause — a corrupt archive. fn remote_tool_missing(message: &str, tool: &str) -> bool { let lower = message.to_lowercase(); + let tool = tool.to_lowercase(); + // POSIX shells: `sh: 1: tar: not found`, `bash: tar: command not found`, + // `zsh: command not found: tar`, busybox `tar: applet not found`. lower.contains("command not found") || lower.contains("not installed") - || (lower.contains(&tool.to_lowercase()) && lower.contains("not found")) + || lower.contains("applet not found") + || lower.contains(&format!("{tool}: not found")) + || lower.contains(&format!("{tool}: no such file or directory")) } fn build_remote_compress_command( @@ -4027,6 +4054,36 @@ mod archive_tests { join_remote_path("/home/developer", "project.zip"), "/home/developer/project.zip" ); + assert!( + split_remote_archive_path("/home/../etc/project.zip").is_err(), + "traversal in a parent component must be rejected, not just in the file name" + ); + // `.` is not traversal, and these forms worked before the guard existed. + assert_eq!( + split_remote_archive_path("./project.zip").expect("dot-relative path"), + (".".to_string(), "project.zip".to_string()) + ); + assert_eq!( + split_remote_archive_path("/home/./project.zip").expect("dot component"), + ("/home/.".to_string(), "project.zip".to_string()) + ); + } + + #[test] + fn tool_output_mentioning_not_found_is_not_a_missing_tool() { + assert!(remote_tool_missing("sh: 1: tar: not found", "tar")); + assert!(remote_tool_missing("bash: tar: command not found", "tar")); + assert!(remote_tool_missing("tar: applet not found", "tar")); + assert!(remote_tool_missing("zsh: command not found: zip", "zip")); + // tar ran fine; the archive is what is broken. + assert!(!remote_tool_missing( + "tar: link: Not found in archive", + "tar" + )); + assert!(!remote_tool_missing( + "unzip: cannot find zipfile directory in one of project.zip, not found", + "zip" + )); } #[test] diff --git a/src/apps/desktop/src/lib.rs b/src/apps/desktop/src/lib.rs index aa48ac52d..15653ad82 100644 --- a/src/apps/desktop/src/lib.rs +++ b/src/apps/desktop/src/lib.rs @@ -4,6 +4,21 @@ //! //! The reqwest HTTP/2 and MCP transport type graph exceeds rustc's default //! trait-evaluation recursion budget when desktop tasks require `Send`. +//! +//! Concretely, dropping the limit back to 128 fails with `overflow evaluating +//! the requirement Vec>>: Send`. +//! The chain runs ~15 frames through h2's own internals (`Slab` → `Buffer` → +//! `Recv` → `Actions` → `Inner` → `Arc>` → `RecvStream` → hyper's +//! `Incoming`), into the MCP remote transport, then out through roughly ten +//! nested `async fn` bodies from `agentic::coordination::scheduler` to the +//! `tokio::spawn` in `api::remote_connect_api`. +//! +//! Most of that depth is in third-party types, so `Box::pin`-ing one of our own +//! futures does not collapse it; only erasing a mid-chain future to +//! `Pin>` would, at the cost of an allocation and dynamic +//! dispatch on the dialog-turn path. Raising the budget is the mechanism rustc +//! itself suggests, costs nothing at runtime, and is re-checked whenever this +//! attribute is touched. pub mod api; pub mod computer_use; diff --git a/src/apps/desktop/src/sleep_prevention.rs b/src/apps/desktop/src/sleep_prevention.rs index 471d1f89b..b5a08957f 100644 --- a/src/apps/desktop/src/sleep_prevention.rs +++ b/src/apps/desktop/src/sleep_prevention.rs @@ -13,6 +13,10 @@ use tauri::{AppHandle, Manager, State}; use crate::api::app_state::AppState; const PREVENT_SLEEP_CONFIG_PATH: &str = "app.prevent_sleep"; +/// Ceiling for one worker round-trip. Acquiring an OS inhibitor is a local call +/// on every platform; anything slower than this is a stuck session manager, not +/// a slow one. +const WORKER_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); enum SleepPreventionRequest { SetEnabled { @@ -47,10 +51,23 @@ impl SleepPreventionState { return Err("Sleep-prevention worker is unavailable".to_string()); } - let outcome = result - .await - .map_err(|_| "Sleep-prevention worker stopped unexpectedly".to_string()) - .and_then(|outcome| outcome); + // Bounded on purpose. The worker's `keepawake` call is a blocking D-Bus + // round-trip to logind on Linux, with no timeout of its own; a hung or + // slow session manager would otherwise hold this mutex forever and wedge + // every later toggle, including the one that turns the feature off. + let outcome = match tokio::time::timeout(WORKER_REQUEST_TIMEOUT, result).await { + Ok(received) => received + .map_err(|_| "Sleep-prevention worker stopped unexpectedly".to_string()) + .and_then(|outcome| outcome), + Err(_) => { + // The thread is still stuck inside the OS call, so it cannot be + // reused; dropping the sender retires it once that call returns. + Err(format!( + "Sleep-prevention request timed out after {} seconds", + WORKER_REQUEST_TIMEOUT.as_secs() + )) + } + }; // An inactive or failed worker owns no useful resources. Dropping the // final sender lets the thread exit instead of keeping one alive for @@ -153,6 +170,51 @@ async fn sync_from_config(config_service: &ConfigService, sleep_prevention: &Sle } } +/// Events that can change `app.prevent_sleep`. +/// +/// `ConfigReloaded` covers a config import or an external edit: the preference +/// can flip without any command running, and without re-reading it the runtime +/// state would silently disagree with the saved one until the next restart. +fn config_event_requires_sync(event: &ConfigUpdateEvent) -> bool { + matches!( + event, + ConfigUpdateEvent::AppUpdated | ConfigUpdateEvent::ConfigReloaded + ) +} + +/// Apply the new runtime state, then persist it, undoing the runtime change if +/// persisting fails. +/// +/// Order matters: applying first means a rejected inhibitor never reaches disk, +/// and rolling back means a failed save never leaves the running app in a state +/// the config does not describe. Written against injected closures so the +/// rollback branch can be tested without a failing filesystem. +async fn apply_then_persist( + previous: bool, + enabled: bool, + apply: Apply, + persist: Persist, +) -> Result<(), String> +where + Apply: Fn(bool) -> ApplyFut, + ApplyFut: std::future::Future>, + Persist: FnOnce() -> PersistFut, + PersistFut: std::future::Future>, +{ + apply(enabled).await?; + + let Err(error) = persist().await else { + return Ok(()); + }; + if let Err(rollback_error) = apply(previous).await { + return Err(format!( + "Failed to save prevent-sleep preference: {}; runtime rollback also failed: {}", + error, rollback_error + )); + } + Err(format!("Failed to save prevent-sleep preference: {}", error)) +} + /// Applies the saved preference at startup and after config imports/reloads. pub fn spawn_config_listener(app: AppHandle) { let app_state: State<'_, AppState> = app.state(); @@ -170,7 +232,7 @@ pub fn spawn_config_listener(app: AppHandle) { loop { match receiver.recv().await { - Ok(ConfigUpdateEvent::AppUpdated) | Ok(ConfigUpdateEvent::ConfigReloaded) => { + Ok(event) if config_event_requires_sync(&event) => { sync_from_config(&config_service, &sleep_prevention).await; } Ok(_) => {} @@ -194,6 +256,10 @@ pub fn spawn_config_listener(app: AppHandle) { #[serde(rename_all = "camelCase")] pub struct GetPreventSleepEnabledRequest {} +/// Reads the preference. Deliberately free of side effects: the runtime state is +/// owned by [`spawn_config_listener`], which applies it at startup and on every +/// config update. Re-applying it from a getter meant reading the setting could +/// start a thread and take an OS inhibitor — and, on Linux, block on D-Bus. #[tauri::command] pub async fn get_prevent_sleep_enabled( app_state: State<'_, AppState>, @@ -201,9 +267,8 @@ pub async fn get_prevent_sleep_enabled( request: GetPreventSleepEnabledRequest, ) -> Result { let _ = request; - let enabled = configured_enabled(&app_state.config_service).await?; - sleep_prevention.set_enabled(enabled).await?; - Ok(enabled) + let _ = sleep_prevention; + configured_enabled(&app_state.config_service).await } #[derive(Debug, Deserialize)] @@ -219,24 +284,19 @@ pub async fn set_prevent_sleep_enabled( request: SetPreventSleepEnabledRequest, ) -> Result<(), String> { let previous = configured_enabled(&app_state.config_service).await?; - sleep_prevention.set_enabled(request.enabled).await?; - - if let Err(error) = app_state - .config_service - .set_config(PREVENT_SLEEP_CONFIG_PATH, request.enabled) - .await - { - if let Err(rollback_error) = sleep_prevention.set_enabled(previous).await { - return Err(format!( - "Failed to save prevent-sleep preference: {}; runtime rollback also failed: {}", - error, rollback_error - )); - } - return Err(format!( - "Failed to save prevent-sleep preference: {}", - error - )); - } + apply_then_persist( + previous, + request.enabled, + |enabled| sleep_prevention.set_enabled(enabled), + || async { + app_state + .config_service + .set_config(PREVENT_SLEEP_CONFIG_PATH, request.enabled) + .await + .map_err(|error| error.to_string()) + }, + ) + .await?; crate::api::remote_connect_api::notify_settings_changed(); Ok(()) @@ -244,7 +304,11 @@ pub async fn set_prevent_sleep_enabled( #[cfg(test)] mod tests { - use super::{start_worker, SleepPreventionState}; + use super::{ + apply_then_persist, config_event_requires_sync, start_worker, SleepPreventionState, + }; + use bitfun_core::service::config::ConfigUpdateEvent; + use std::sync::{Arc, Mutex}; #[tokio::test] async fn inactive_state_does_not_start_a_worker() { @@ -266,4 +330,92 @@ mod tests { assert!(state.worker.lock().await.is_none()); } + + #[tokio::test] + async fn a_failed_save_puts_the_runtime_state_back() { + let applied = Arc::new(Mutex::new(Vec::new())); + let recorder = Arc::clone(&applied); + + let error = apply_then_persist( + false, + true, + move |enabled| { + let recorder = Arc::clone(&recorder); + async move { + recorder.lock().unwrap().push(enabled); + Ok(()) + } + }, + || async { Err("disk is full".to_string()) }, + ) + .await + .expect_err("a failed save must surface as an error"); + + assert!(error.contains("disk is full"), "unexpected error: {error}"); + assert!( + !error.contains("rollback also failed"), + "rollback succeeded, so it must not be reported as failed: {error}" + ); + assert_eq!( + *applied.lock().unwrap(), + vec![true, false], + "the new state must be applied, then reverted to the previous one" + ); + } + + #[tokio::test] + async fn a_failed_save_and_a_failed_rollback_report_both() { + let error = apply_then_persist( + false, + true, + |enabled| async move { + if enabled { + Ok(()) + } else { + Err("inhibitor stuck".to_string()) + } + }, + || async { Err("disk is full".to_string()) }, + ) + .await + .expect_err("a failed save must surface as an error"); + + assert!(error.contains("disk is full"), "unexpected error: {error}"); + assert!( + error.contains("inhibitor stuck"), + "a failed rollback must be reported too: {error}" + ); + } + + #[tokio::test] + async fn a_successful_save_does_not_roll_back() { + let applied = Arc::new(Mutex::new(Vec::new())); + let recorder = Arc::clone(&applied); + + apply_then_persist( + false, + true, + move |enabled| { + let recorder = Arc::clone(&recorder); + async move { + recorder.lock().unwrap().push(enabled); + Ok(()) + } + }, + || async { Ok(()) }, + ) + .await + .expect("saving succeeded"); + + assert_eq!(*applied.lock().unwrap(), vec![true]); + } + + #[test] + fn config_reload_re_reads_the_preference() { + assert!(config_event_requires_sync(&ConfigUpdateEvent::ConfigReloaded)); + assert!(config_event_requires_sync(&ConfigUpdateEvent::AppUpdated)); + assert!(!config_event_requires_sync( + &ConfigUpdateEvent::ModelConfigurationUpdated + )); + } } diff --git a/src/crates/services/services-integrations/src/git/service.rs b/src/crates/services/services-integrations/src/git/service.rs index 93ac03f28..9d8871c6d 100644 --- a/src/crates/services/services-integrations/src/git/service.rs +++ b/src/crates/services/services-integrations/src/git/service.rs @@ -27,11 +27,24 @@ fn review_path_has_parent_traversal(path: &str, windows: bool) -> bool { } } +/// `git rev-parse` on a date string is a pure local parse, so anything slower +/// than this is a stuck process, not a slow one. +const APPROXIDATE_TIMEOUT: Duration = Duration::from_secs(5); + +/// Resolve a `since`/`until` value through Git's own approxidate parser. +/// +/// Note the semantics this inherits: approxidate never rejects input. A typo +/// like `--since=lst week` parses as "now", producing a filter that matches +/// almost nothing and an empty, successful result. That is Git's documented +/// behaviour and callers are matched to it deliberately, so unparseable input is +/// logged rather than turned into an error — but it is why an empty commit list +/// is worth double-checking against the filter that produced it. fn resolve_git_approxidate(repo_path: &Path, option: &str, value: &str) -> Result { let argument = format!("{option}={value}"); - let output = execute_git_command_sync( + let output = execute_git_command_sync_with_timeout( repo_path.to_string_lossy().as_ref(), &["rev-parse", argument.as_str()], + APPROXIDATE_TIMEOUT, ) .map_err(|error| { GitError::CommandFailed(format!( @@ -48,11 +61,30 @@ fn resolve_git_approxidate(repo_path: &Path, option: &str, value: &str) -> Resul output.trim() )) })?; - timestamp.parse::().map_err(|error| { + let timestamp = timestamp.parse::().map_err(|error| { GitError::CommandFailed(format!( "Git returned an invalid timestamp for '{argument}': {error}" )) - }) + })?; + + // Approxidate falls back to "now" for anything it cannot read, so a value + // that lands on the current second is the one signal that the input was + // probably a typo. Not an error — "now" and "today" are legitimate — but + // worth a line when the query then comes back empty. + let now = chrono::Utc::now().timestamp(); + if (now - timestamp).abs() <= 1 + && !matches!( + value.trim().to_ascii_lowercase().as_str(), + "now" | "today" | "" + ) + { + log::warn!( + "Git date filter '{argument}' resolved to the current time; \ + approxidate could not parse it and the result will be nearly empty" + ); + } + + Ok(timestamp) } impl GitService { diff --git a/src/crates/services/services-integrations/src/git/utils.rs b/src/crates/services/services-integrations/src/git/utils.rs index 7d4031a93..fd75496a2 100644 --- a/src/crates/services/services-integrations/src/git/utils.rs +++ b/src/crates/services/services-integrations/src/git/utils.rs @@ -376,6 +376,69 @@ pub fn execute_git_command_sync_raw( }) } +/// Executes a Git command synchronously, giving up after `timeout`. +/// +/// Only for commands whose output is a handful of bytes: stdout and stderr are +/// piped and drained after exit, so a command that filled a pipe buffer while +/// still running would deadlock against this poll loop rather than time out. +/// +/// On expiry the child is killed and reaped, so nothing is left behind to hold +/// the repository lock or a worker thread. +pub fn execute_git_command_sync_with_timeout( + repo_path: &str, + args: &[&str], + timeout: std::time::Duration, +) -> Result { + use std::process::Stdio; + + let mut child = process_manager::create_command("git") + .current_dir(repo_path) + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|e| GitError::CommandFailed(format!("Failed to execute git command: {}", e)))?; + + let deadline = std::time::Instant::now() + timeout; + loop { + match child.try_wait() { + Ok(Some(_)) => break, + Ok(None) => {} + Err(e) => { + return Err(GitError::CommandFailed(format!( + "Failed to wait for git command: {}", + e + ))) + } + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + return Err(GitError::CommandFailed(format!( + "git {} timed out after {:?}", + args.join(" "), + timeout + ))); + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + + let output = child + .wait_with_output() + .map_err(|e| GitError::CommandFailed(format!("Failed to read git output: {}", e)))?; + if output.status.success() { + return Ok(String::from_utf8_lossy(&output.stdout).to_string()); + } + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); + Err(GitError::CommandFailed(if stderr.is_empty() { + stdout + } else { + stderr + })) +} + /// Executes a Git command synchronously. pub fn execute_git_command_sync(repo_path: &str, args: &[&str]) -> Result { let result = execute_git_command_sync_raw(repo_path, args)?; From 6f094f91091eb5c259686dcf8467b4cdf835bc01 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Sat, 25 Jul 2026 00:11:31 -0700 Subject: [PATCH 7/7] test(cli): retry the compat launcher exec past a Linux ETXTBSY race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing flake, unrelated to the rest of this PR; it happened to lose the coin toss on this run's ubuntu job. The three tests in this file run as threads of one process. `legacy_reports_a_missing_primary_without_recursing` copies the launcher to a tempdir and execs it; while `fs::copy` has the destination open for writing, a sibling thread forking for its own `Command` gives the child an inherited write descriptor, and `execve` of that path then fails with ETXTBSY until the child clears it. The descriptor is CLOEXEC, so the window is tiny — invisible until a loaded runner widens it. The writer is already closed by the time `fs::copy` returns and the inherited copy is out of the caller's hands, so retrying is the fix available here. --- src/apps/cli/tests/compat_entrypoint.rs | 29 +++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/apps/cli/tests/compat_entrypoint.rs b/src/apps/cli/tests/compat_entrypoint.rs index 69ed26021..48f986917 100644 --- a/src/apps/cli/tests/compat_entrypoint.rs +++ b/src/apps/cli/tests/compat_entrypoint.rs @@ -1,7 +1,30 @@ -use std::process::Command; +use std::process::{Command, Output}; const DEPRECATION: &str = "Warning: `bitfun-cli` is deprecated; use `bitfun` instead."; +/// Run a just-written executable, retrying while Linux reports `ETXTBSY`. +/// +/// The tests in this file run as threads of one process. When one copies a +/// binary, the destination is briefly open for writing; if a sibling thread +/// forks for its own `Command` in that window, the child inherits the write +/// descriptor, and an `execve` of that file fails with "Text file busy" until +/// the child clears it. The descriptor is `CLOEXEC`, so the window is short and +/// the race is invisible until a loaded CI runner widens it. +/// +/// Retrying is the fix available to a caller: the writer is already closed by +/// the time `fs::copy` returns, and the inherited copy is out of our hands. +fn run_freshly_written(command: &mut Command) -> std::io::Result { + for _ in 0..50 { + match command.output() { + Err(error) if error.kind() == std::io::ErrorKind::ExecutableFileBusy => { + std::thread::sleep(std::time::Duration::from_millis(20)); + } + result => return result, + } + } + command.output() +} + #[test] fn legacy_version_matches_primary_and_warns_only_on_stderr() { let primary = Command::new(env!("CARGO_BIN_EXE_bitfun")) @@ -46,9 +69,7 @@ fn legacy_reports_a_missing_primary_without_recursing() { let copied = temp.path().join(file_name); std::fs::copy(env!("CARGO_BIN_EXE_bitfun-cli"), &copied) .expect("copy deprecated launcher without primary sibling"); - let output = Command::new(copied) - .arg("--version") - .output() + let output = run_freshly_written(Command::new(copied).arg("--version")) .expect("run isolated deprecated launcher"); let stderr = String::from_utf8_lossy(&output.stderr);