Hotfix/client update reinstall hardening

clients/openframe-client/src/lib.rs

@@ -761,8 +761,9 @@ impl Client {
         self.script_schedule_execution_listener.start().await?;
         info!("Script schedule execution listener started");
 
-        // Start tool run manager
-        self.tool_run_manager.run().await?;
+        if let Err(e) = self.tool_run_manager.run().await {
+            error!("Failed to start tool run manager: {:#}", e);
+        }
 
         // Start mesh self-heal watcher (re-fetch .msh + bounce agent if held on a stale MeshID).
         self.mesh_self_heal_service.run().await?;

clients/openframe-client/src/platform/preferences_writer.rs

@@ -19,8 +19,11 @@ pub fn write<'a>(
     }
 
     for (key, value) in &prefs {
-        let status = Command::new("sudo")
+        let status = Command::new("launchctl")
             .args([
+                "asuser",
+                &user.uid.to_string(),
+                "sudo",
                 "-u",
                 &user.username,
                 "defaults",
@@ -32,8 +35,25 @@ pub fn write<'a>(
             .status()
             .with_context(|| format!("Failed to write preference '{}'", key))?;
 
-        if !status.success() {
-            anyhow::bail!("defaults write failed for '{}': exit {}", key, status);
+        if status.success() {
+            continue;
+        }
+
+        let fallback = Command::new("sudo")
+            .args([
+                "-u",
+                &user.username,
+                "defaults",
+                "write",
+                bundle_id,
+                key,
+                value,
+            ])
+            .status()
+            .with_context(|| format!("Failed to write preference '{}'", key))?;
+
+        if !fallback.success() {
+            anyhow::bail!("defaults write failed for '{}': {}", key, fallback);
         }
     }
 

clients/openframe-client/src/platform/tool_updater/gui_app.rs

@@ -1,13 +1,13 @@
 use anyhow::{Context, Result};
 use async_trait::async_trait;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use tracing::{error, info, warn};
 
 use super::{ToolUpdater, ToolUpdaterDeps, UpdateContext};
 use crate::models::{DownloadConfiguration, Installation, InstalledTool};
 use crate::platform::preferences_writer::{args_to_pairs, write as write_preferences};
-use crate::platform::remove_app_bundle;
 use crate::platform::user_session::{get_console_user, launch_as_user};
+use crate::platform::DirectoryManager;
 
 pub struct GuiAppToolUpdater {
     deps: ToolUpdaterDeps,
@@ -17,6 +17,46 @@ impl GuiAppToolUpdater {
     pub fn new(deps: ToolUpdaterDeps) -> Self {
         Self { deps }
     }
+
+    fn backup_path_for(bundle: &Path) -> PathBuf {
+        let name = bundle
+            .file_name()
+            .map(|n| n.to_string_lossy().to_string())
+            .unwrap_or_else(|| "bundle".to_string());
+        bundle.with_file_name(format!(".{name}.update-backup"))
+    }
+
+    async fn move_bundle_aside(
+        executable_path: &str,
+        tool_agent_id: &str,
+    ) -> Result<Option<PathBuf>> {
+        let Some(bundle) = DirectoryManager::find_app_bundle_path(Path::new(executable_path))
+        else {
+            warn!(tool_id = %tool_agent_id, "No .app bundle in {} — updating without a backup", executable_path);
+            return Ok(None);
+        };
+        let backup = Self::backup_path_for(&bundle);
+
+        if !bundle.exists() {
+            if backup.exists() {
+                info!(tool_id = %tool_agent_id, "Adopting the backup left by an interrupted update");
+                return Ok(Some(backup));
+            }
+            return Ok(None);
+        }
+        if backup.exists() {
+            tokio::fs::remove_dir_all(&backup).await.ok();
+        }
+        tokio::fs::rename(&bundle, &backup).await.with_context(|| {
+            format!(
+                "Failed to move {} aside to {}",
+                bundle.display(),
+                backup.display()
+            )
+        })?;
+        info!(tool_id = %tool_agent_id, "Old app bundle kept at {}", backup.display());
+        Ok(Some(backup))
+    }
 }
 
 #[async_trait]
@@ -34,8 +74,15 @@ impl ToolUpdater for GuiAppToolUpdater {
 
         tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;
 
+        let backup_path = match &tool.installation {
+            Installation::GuiApp {
+                executable_path, ..
+            } => Self::move_bundle_aside(executable_path, tool_agent_id).await?,
+            _ => None,
+        };
+
         Ok(UpdateContext {
-            backup_path: None,
+            backup_path,
             needs_restart: true,
         })
     }
@@ -49,20 +96,13 @@ impl ToolUpdater for GuiAppToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Applying GuiApp update");
 
-        let Installation::GuiApp {
-            executable_path,
-            bundle_id,
-        } = &tool.installation
-        else {
+        let Installation::GuiApp { bundle_id, .. } = &tool.installation else {
             anyhow::bail!(
                 "Expected GuiApp installation type for tool: {}",
                 tool_agent_id
             );
         };
 
-        info!(tool_id = %tool_agent_id, "Removing old app bundle");
-        remove_app_bundle(executable_path).await?;
-
         let applications_dir = PathBuf::from("/Applications");
 
         info!(tool_id = %tool_agent_id, "Downloading and installing new version from: {}", config.link);
@@ -93,6 +133,14 @@ impl ToolUpdater for GuiAppToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Finalizing GuiApp update");
 
+        if let Some(backup) = &ctx.backup_path {
+            if backup.exists() {
+                if let Err(e) = tokio::fs::remove_dir_all(backup).await {
+                    warn!(tool_id = %tool_agent_id, "Failed to remove the old app bundle {}: {:#}", backup.display(), e);
+                }
+            }
+        }
+
         if !ctx.needs_restart {
             info!(tool_id = %tool_agent_id, "Restart not requested, skipping");
             return Ok(());
@@ -154,10 +202,37 @@ impl ToolUpdater for GuiAppToolUpdater {
         Ok(())
     }
 
-    async fn rollback(&self, tool: &InstalledTool, _ctx: &UpdateContext) -> Result<()> {
+    async fn rollback(&self, tool: &InstalledTool, ctx: &UpdateContext) -> Result<()> {
         let tool_agent_id = &tool.tool_agent_id;
-        warn!(tool_id = %tool_agent_id,
-              "Rollback requested for GuiApp but no backup available. User should reinstall from server.");
+
+        let Some(backup) = &ctx.backup_path else {
+            warn!(tool_id = %tool_agent_id,
+                  "Rollback requested for GuiApp but no backup was taken. User should reinstall from server.");
+            return Ok(());
+        };
+
+        let Installation::GuiApp {
+            executable_path, ..
+        } = &tool.installation
+        else {
+            anyhow::bail!("Expected GuiApp installation type for tool: {tool_agent_id}");
+        };
+        let Some(bundle) = DirectoryManager::find_app_bundle_path(Path::new(executable_path))
+        else {
+            anyhow::bail!("Could not resolve the .app bundle path for {tool_agent_id}");
+        };
+
+        if bundle.exists() {
+            tokio::fs::remove_dir_all(&bundle).await.ok();
+        }
+        tokio::fs::rename(backup, &bundle).await.with_context(|| {
+            format!(
+                "Failed to restore the previous app bundle from {}",
+                backup.display()
+            )
+        })?;
+
+        info!(tool_id = %tool_agent_id, "Restored the previous app bundle: {}", bundle.display());
         Ok(())
     }
 }

clients/openframe-client/src/platform/tool_updater/standard.rs

@@ -31,7 +31,10 @@ impl ToolUpdater for StandardToolUpdater {
             .await
             .with_context(|| format!("Failed to stop tool: {}", tool_agent_id))?;
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         clear_aside_binary(&agent_path, tool_agent_id).await;
         log_update_survivors(&self.deps, tool).await;
 
@@ -52,7 +55,10 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Applying Standard tool update");
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         download_and_write_binary(&self.deps, config, &agent_path, tool_agent_id).await?;
         Ok(None)
     }
@@ -74,7 +80,10 @@ impl ToolUpdater for StandardToolUpdater {
         let tool_agent_id = &tool.tool_agent_id;
         info!(tool_id = %tool_agent_id, "Rolling back Standard tool update");
 
-        let agent_path = self.deps.directory_manager.get_agent_path(tool_agent_id);
+        let agent_path = self
+            .deps
+            .directory_manager
+            .get_tool_executable_path(&tool.tool_agent_id, tool.installation.executable_path());
         restore_from_backup(ctx.backup_path.as_ref(), &agent_path, tool_agent_id).await
     }
 }

clients/openframe-client/src/service.rs

@@ -83,14 +83,22 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
     };
 
     // Report that the service is running
-    let _ = set_service_status(&status_handle, ServiceState::Running);
+    let _ = set_service_status(
+        &status_handle,
+        ServiceState::Running,
+        ServiceExitCode::Win32(0),
+    );
 
     // Create a Tokio runtime and run the service core
     let rt = match Runtime::new() {
         Ok(runtime) => runtime,
         Err(e) => {
             eprintln!("Failed to create Tokio runtime: {:?}", e);
-            let _ = set_service_status(&status_handle, ServiceState::Stopped);
+            let _ = set_service_status(
+                &status_handle,
+                ServiceState::Stopped,
+                ServiceExitCode::ServiceSpecific(1),
+            );
             return;
         }
     };
@@ -114,17 +122,29 @@ fn windows_service_main(_args: Vec<std::ffi::OsString>) {
     });
 
     if let Err(e) = result {
-        eprintln!("Service core failed: {:?}", e);
-        let _ = set_service_status(&status_handle, ServiceState::Stopped);
+        error!("Service core failed: {:#}", e);
+        let _ = set_service_status(
+            &status_handle,
+            ServiceState::Stopped,
+            ServiceExitCode::ServiceSpecific(1),
+        );
     } else {
         info!("Service stopped gracefully");
-        let _ = set_service_status(&status_handle, ServiceState::Stopped);
+        let _ = set_service_status(
+            &status_handle,
+            ServiceState::Stopped,
+            ServiceExitCode::Win32(0),
+        );
     }
 }
 
 /// Helper function to set service status
 #[cfg(windows)]
-fn set_service_status(status_handle: &ServiceStatusHandle, state: ServiceState) -> Result<()> {
+fn set_service_status(
+    status_handle: &ServiceStatusHandle,
+    state: ServiceState,
+    exit_code: ServiceExitCode,
+) -> Result<()> {
     let status = ServiceStatus {
         service_type: ServiceType::OWN_PROCESS,
         current_state: state,
@@ -133,7 +153,7 @@ fn set_service_status(status_handle: &ServiceStatusHandle, state: ServiceState)
         } else {
             ServiceControlAccept::empty()
         },
-        exit_code: ServiceExitCode::Win32(0),
+        exit_code,
         checkpoint: 0,
         wait_hint: std::time::Duration::from_secs(5),
         process_id: None,

clients/openframe-client/src/services/github_download_service.rs

@@ -8,9 +8,32 @@ use bytes::Bytes;
 use reqwest::Client;
 use std::io::Cursor;
 use std::path::Path;
+#[cfg(target_os = "macos")]
+use std::path::{Component, PathBuf};
 use tokio::time::Duration;
 use tracing::{info, warn};
 
+/// `Path::join` with an absolute entry replaces the base, and `..` is resolved by the OS.
+#[cfg(target_os = "macos")]
+fn safe_join(target_dir: &Path, entry_path: &Path) -> Result<PathBuf> {
+    for component in entry_path.components() {
+        match component {
+            Component::Normal(_) | Component::CurDir => {}
+            _ => {
+                return Err(anyhow!(
+                    "Refusing archive entry with an unsafe path: {}",
+                    entry_path.display()
+                ))
+            }
+        }
+    }
+    Ok(target_dir.join(entry_path))
+}
+
+#[cfg(all(test, target_os = "macos"))]
+#[path = "github_download_service_tests.rs"]
+mod tests;
+
 #[derive(Clone)]
 pub struct GithubDownloadService {
     http_client: Client,
@@ -409,7 +432,7 @@ impl GithubDownloadService {
         for entry_result in archive.entries().context("Failed to read tar entries")? {
             let mut entry = entry_result.context("Failed to read tar entry")?;
             let path = entry.path().context("Failed to get entry path")?;
-            let dest_path = target_dir.join(&path);
+            let dest_path = safe_join(target_dir, &path)?;
 
             if entry.header().entry_type().is_dir() {
                 fs::create_dir_all(&dest_path).with_context(|| {