From d0952147fa359c5d9fc1777ee5f43d4b3f3dec48 Mon Sep 17 00:00:00 2001 From: Zortos <65777760+zortos293@users.noreply.github.com> Date: Sun, 20 Sep 2026 00:54:55 +0200 Subject: [PATCH 1/2] fix(updates): quote MSI INSTALL_ROOT to avoid msiexec 1639 and clear terminal transactions Pass INSTALL_ROOT as raw PROPERTY=value with quotes only around the value so msiexec accepts paths with spaces instead of failing with 1639 and a help dialog. Add a verbose msiexec log (/l*v) and surface its path in failure messages; reject quote/control characters in MSI paths with a clear error. Report terminal helper outcomes (succeeded/rolled-back/failed) for one launch only, then clear the persisted active transaction so a stale failure dialog does not reappear on every restart. Also drop unreadable active-apply.json at startup/check/download/apply boundaries so one corrupt file cannot block future updates. --- docs/core-protocol.md | 6 +- .../opennow-core/src/update_apply/managed.rs | 107 +++++++++++++++++- native/opennow-core/src/updater.rs | 106 ++++++++++++++--- 3 files changed, 199 insertions(+), 20 deletions(-) diff --git a/docs/core-protocol.md b/docs/core-protocol.md index 63dd302d7..99d853342 100644 --- a/docs/core-protocol.md +++ b/docs/core-protocol.md @@ -912,7 +912,11 @@ counts as success. On a later launch, the core reconciles these managed outcomes against the native package registration and its running version. A known live installer keeps `managed-pending` active. A completed installer resolves to `succeeded` or -`failed`, and the core clears the persisted active transaction. An MSI reboot +`failed`, and the core clears the persisted active transaction. Terminal helper +outcomes (`succeeded`, `rolled-back`, `failed`) are likewise reported for that +launch only; the core clears the persisted transaction after the first +reconciliation so the same failure dialog does not reappear on every restart. +An MSI reboot warning remains until Windows' per-boot sequence number changes. Sessions remain available, but another update must wait for the required reboot so it cannot overlap pending Windows file replacements. Reopening the app before diff --git a/native/opennow-core/src/update_apply/managed.rs b/native/opennow-core/src/update_apply/managed.rs index 5ad3b2606..12c28d888 100644 --- a/native/opennow-core/src/update_apply/managed.rs +++ b/native/opennow-core/src/update_apply/managed.rs @@ -159,6 +159,17 @@ mod windows { Ok(value.strip_prefix("\\\\?\\").unwrap_or(value).to_owned()) } + pub(super) fn install_root_argument(target: &Path) -> Result { + let path = argument_path(target)?; + if path.contains('"') || path.contains('\n') || path.contains('\r') { + return Err("Windows Installer requires an installation path without quotes".to_owned()); + } + if path.is_empty() || path.len() > 32767 { + return Err("Windows Installer requires a bounded installation path".to_owned()); + } + Ok(format!("INSTALL_ROOT=\"{path}\"")) + } + struct Handle(u32); impl Drop for Handle { fn drop(&mut self) { @@ -379,6 +390,25 @@ pub(super) fn windows_managed(target: &Path) -> Result { Ok(false) } +#[cfg(not(windows))] +fn msi_install_root_argument(target: &Path) -> Result { + let value = target + .to_str() + .ok_or("Windows Installer requires a Unicode installation path")?; + let path = if let Some(unc) = value.strip_prefix("\\\\?\\UNC\\") { + format!("\\\\{unc}") + } else { + value.strip_prefix("\\\\?\\").unwrap_or(value).to_owned() + }; + if path.contains('"') || path.contains('\n') || path.contains('\r') { + return Err("Windows Installer requires an installation path without quotes".to_owned()); + } + if path.is_empty() || path.len() > 32767 { + return Err("Windows Installer requires a bounded installation path".to_owned()); + } + Ok(format!("INSTALL_ROOT=\"{path}\"")) +} + pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> { let identity = plan .managed_identity @@ -404,17 +434,40 @@ pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> { let mut command = Command::new(Path::new(&system).join("System32/msiexec.exe")); command.arg("/i"); #[cfg(windows)] - command.arg(windows::argument_path(&plan.package)?); + { + let package = windows::argument_path(&plan.package)?; + if package.contains('"') { + return Err( + "Windows Installer requires a package path without quotes".to_owned() + ); + } + command.arg(package); + } #[cfg(not(windows))] command.arg(&plan.package); command.args(["/passive", "/norestart", "REBOOT=ReallySuppress"]); + // msiexec parses the raw command line for PROPERTY=value tokens. A Rust + // Command::arg containing spaces would be quoted as a whole + // ("INSTALL_ROOT=C:\Program Files\..."), which msiexec rejects with 1639 + // and a help dialog. Emit INSTALL_ROOT="..." with quotes only around the + // value, matching the installer contract test. #[cfg(windows)] - command.arg(format!( - "INSTALL_ROOT={}", - windows::argument_path(&plan.target)? - )); + { + use std::os::windows::process::CommandExt; + command.raw_arg(windows::install_root_argument(&plan.target)?); + let log = windows::argument_path(&directory.join("msiexec.log"))?; + if log.contains('"') { + return Err("Windows Installer requires a log path without quotes".to_owned()); + } + command.arg("/l*v"); + command.arg(log); + } #[cfg(not(windows))] - command.arg(format!("INSTALL_ROOT={}", plan.target.display())); + { + command.arg(msi_install_root_argument(&plan.target)?); + command.arg("/l*v"); + command.arg(directory.join("msiexec.log")); + } command } _ => return Err("Not a managed package update".to_owned()), @@ -456,6 +509,12 @@ pub(super) fn install(plan: &Plan, directory: &Path) -> Result<(), String> { } if code != 0 { let mut message = installer_failure_message(plan.kind, code); + if plan.kind == InstallKind::WindowsMsi { + let log = directory.join("msiexec.log"); + if log.is_file() { + message.push_str(&format!("; installer log: {}", log.display())); + } + } if prepare(plan.kind, &plan.package, &plan.target, &plan.version).is_ok() && super::canonical_file(&plan.application_executable).is_ok() { @@ -510,6 +569,9 @@ fn installer_failure_message(kind: InstallKind, code: i32) -> String { (InstallKind::WindowsMsi, 1602) => { "Windows Installer was cancelled. Try installing the update again and complete the installer prompts" } + (InstallKind::WindowsMsi, 1639) => { + "Windows Installer rejected the update command line. Try installing the update again; the installer log beside the update outcome has details" + } _ => "Native package manager failed", }; format!("{reason} (exit code {code}); update completion is not confirmed") @@ -599,6 +661,11 @@ mod tests { 1602, "Windows Installer was cancelled", ), + ( + InstallKind::WindowsMsi, + 1639, + "rejected the update command line", + ), ] { let message = installer_failure_message(kind, code); assert!(message.contains(explanation), "{message}"); @@ -611,6 +678,7 @@ mod tests { fn installer_failure_codes_are_interpreted_only_for_their_package_manager() { for (kind, code) in [ (InstallKind::DebianPackage, 1602), + (InstallKind::DebianPackage, 1639), (InstallKind::WindowsMsi, 126), (InstallKind::WindowsMsi, 127), (InstallKind::DebianPackage, 1), @@ -625,6 +693,33 @@ mod tests { } } + #[test] + fn msi_install_root_quotes_only_the_value_for_msiexec() { + #[cfg(windows)] + let argument = windows::install_root_argument(Path::new( + r"C:\Program Files\OpenNOW Nightly", + )) + .unwrap(); + #[cfg(not(windows))] + let argument = msi_install_root_argument(Path::new( + r"C:\Program Files\OpenNOW Nightly", + )) + .unwrap(); + assert_eq!( + argument, + r#"INSTALL_ROOT="C:\Program Files\OpenNOW Nightly""# + ); + #[cfg(windows)] + let stripped = windows::install_root_argument(Path::new(r"\\?\C:\OpenNOW")).unwrap(); + #[cfg(not(windows))] + let stripped = msi_install_root_argument(Path::new(r"\\?\C:\OpenNOW")).unwrap(); + assert_eq!(stripped, r#"INSTALL_ROOT="C:\OpenNOW""#); + #[cfg(windows)] + assert!(windows::install_root_argument(Path::new("C:\\evil\"quote")).is_err()); + #[cfg(not(windows))] + assert!(msi_install_root_argument(Path::new("C:\\evil\"quote")).is_err()); + } + #[cfg(target_os = "linux")] #[test] fn deb_ownership_matches_canonical_directories_but_rejects_file_symlinks() { diff --git a/native/opennow-core/src/updater.rs b/native/opennow-core/src/updater.rs index 7748557f7..17051271f 100644 --- a/native/opennow-core/src/updater.rs +++ b/native/opennow-core/src/updater.rs @@ -95,11 +95,17 @@ impl UpdaterService { "idle", "Ready to check GitHub Releases".to_owned(), ), - Err(error) => ( - None, - "failed", - format!("Could not recover update status: {error}"), - ), + Err(error) => { + // A corrupt transaction must not block every future launch. Report it + // once for this session, then drop the unreadable persistence so the + // next startup begins clean. Diagnostics remain in the message. + let _ = fs::remove_file(staging_dir.join("active-apply.json")); + ( + None, + "failed", + format!("Could not recover update status: {error}"), + ) + } } }; Ok(Self { @@ -175,6 +181,7 @@ impl UpdaterService { state.transaction = None; state.message = "Checking GitHub Releases…".to_owned(); } + let _ = fs::remove_file(self.staging_dir.join("active-apply.json")); let releases = self.fetch_releases(RELEASES_URL); let releases = match releases { Ok(releases) => releases, @@ -300,6 +307,7 @@ impl UpdaterService { state.message = format!("Downloading OpenNOW {}…", available.version); available }; + let _ = fs::remove_file(self.staging_dir.join("active-apply.json")); match self.download_verified(&available) { Ok(downloaded) => { let mut state = self.state.lock().expect("updater state poisoned"); @@ -339,6 +347,7 @@ impl UpdaterService { "Verifying and preparing a complete replacement before shutdown.".to_owned(); state.transaction = None; } + let _ = fs::remove_file(self.staging_dir.join("active-apply.json")); let result = (|| { verify_downloaded_file(&downloaded)?; let application = std::env::var_os("OPENNOW_APP_EXECUTABLE") @@ -371,6 +380,7 @@ impl UpdaterService { state.status = "failed"; state.message = format!("Update preparation failed; OpenNOW remains open: {error}"); state.transaction = None; + let _ = fs::remove_file(self.staging_dir.join("active-apply.json")); return Err(error); } Ok(self.state()) @@ -564,11 +574,15 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) { state.message = "The update helper outcome is missing or does not match the prepared version." .to_owned(); + // Terminal: report once for this launch, then drop persistence so the next + // startup does not re-show the same failure dialog. + clear_finished_transaction(state, staging_dir); return; } Err(error) => { state.status = "failed"; state.message = error; + clear_finished_transaction(state, staging_dir); return; } }; @@ -605,14 +619,7 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) { state.message = format!("Could not recover native update completion: {error}"); } } - state.transaction = None; - match fs::remove_file(staging_dir.join("active-apply.json")) { - Ok(()) => (), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => (), - Err(error) => state.message.push_str(&format!( - "; could not remove the completed update transaction: {error}" - )), - } + clear_finished_transaction(state, staging_dir); return; } if matches!( @@ -628,11 +635,13 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) { Ok(false) => { state.status = "failed"; state.message = "The update helper stopped before completing the transaction. OpenNOW will not close automatically.".to_owned(); + clear_finished_transaction(state, staging_dir); return; } Err(error) => { state.status = "failed"; state.message = format!("Could not confirm update helper ownership: {error}"); + clear_finished_transaction(state, staging_dir); return; } } @@ -649,6 +658,26 @@ fn reconcile_transaction(state: &mut State, staging_dir: &Path) { OutcomeStatus::RebootRequired => "reboot-required", }; state.message = outcome.message; + if matches!( + outcome.status, + OutcomeStatus::Completed | OutcomeStatus::RolledBack | OutcomeStatus::Failed + ) { + // Terminal helper outcomes are reported for this launch only. Clearing the + // persisted transaction prevents the same failure dialog on every restart, + // matching the managed-recovery behavior documented in core-protocol.md. + clear_finished_transaction(state, staging_dir); + } +} + +fn clear_finished_transaction(state: &mut State, staging_dir: &Path) { + state.transaction = None; + match fs::remove_file(staging_dir.join("active-apply.json")) { + Ok(()) => (), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => (), + Err(error) => state.message.push_str(&format!( + "; could not remove the completed update transaction: {error}" + )), + } } fn restore_downloaded_status(state: &mut State) { @@ -1211,6 +1240,9 @@ mod tests { update_apply::OutcomeStatus::Installing, update_apply::OutcomeStatus::AwaitingStartup, ] { + // Terminal failures clear persistence after the first report, so each + // stale-helper case needs fresh persistence to exercise the same path. + save_prepared_update(&updates, &prepared).unwrap(); fs::write( &prepared.outcome_path, serde_json::to_vec(&update_apply::UpdateOutcome { @@ -1228,6 +1260,49 @@ mod tests { assert_eq!(updater.state()["status"], "failed"); assert_eq!(updater.state()["exitRequired"], false); assert!(!updater.installation_pending()); + // The failure is reported for this launch only; the next startup begins + // clean instead of re-showing the same dialog. + assert!(!updates.join("active-apply.json").exists()); + } + } + + #[test] + fn terminal_helper_outcomes_are_reported_once_then_cleared() { + for (outcome, expected) in [ + (update_apply::OutcomeStatus::Failed, "failed"), + (update_apply::OutcomeStatus::RolledBack, "rolled-back"), + (update_apply::OutcomeStatus::Completed, "succeeded"), + ] { + let directory = tempfile::tempdir().unwrap(); + let updates = directory.path().join("updates"); + fs::create_dir(&updates).unwrap(); + let prepared = update_apply::PreparedUpdate { + plan_path: directory.path().join("plan.json"), + outcome_path: directory.path().join("outcome.json"), + version: "1.1.0".to_owned(), + }; + save_prepared_update(&updates, &prepared).unwrap(); + fs::write( + &prepared.outcome_path, + serde_json::to_vec(&update_apply::UpdateOutcome { + schema_version: 1, + version: prepared.version.clone(), + status: outcome, + message: "Terminal helper result".to_owned(), + installed_version: None, + restarted_process: None, + }) + .unwrap(), + ) + .unwrap(); + let updater = UpdaterService::new(directory.path()).unwrap(); + assert_eq!(updater.state()["status"], expected); + assert!(!updater.installation_pending()); + assert!(!updates.join("active-apply.json").exists()); + let restarted = UpdaterService::new(directory.path()).unwrap(); + assert_eq!(restarted.state()["status"], "idle"); + assert_eq!(restarted.state()["canCheck"], true); + assert!(!restarted.installation_pending()); } } @@ -1244,6 +1319,11 @@ mod tests { assert_eq!(updater.state()["status"], "failed"); assert_eq!(updater.state()["exitRequired"], false); assert_eq!(updater.state()["canCheck"], true); + // Corrupt persistence is dropped after the first report so the next launch + // starts clean instead of failing forever. + assert!(!directory.path().join("updates/active-apply.json").exists()); + let restarted = UpdaterService::new(directory.path()).unwrap(); + assert_eq!(restarted.state()["status"], "idle"); } #[test] From e13d89f95b796ae4d0e7b0529667aded149a4fa5 Mon Sep 17 00:00:00 2001 From: Zortos <65777760+zortos293@users.noreply.github.com> Date: Sun, 20 Sep 2026 01:02:30 +0200 Subject: [PATCH 2/2] fix(updates): apply rustfmt to MSI install-root helper and test --- native/opennow-core/src/update_apply/managed.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/native/opennow-core/src/update_apply/managed.rs b/native/opennow-core/src/update_apply/managed.rs index 12c28d888..2fdfb30b1 100644 --- a/native/opennow-core/src/update_apply/managed.rs +++ b/native/opennow-core/src/update_apply/managed.rs @@ -162,7 +162,9 @@ mod windows { pub(super) fn install_root_argument(target: &Path) -> Result { let path = argument_path(target)?; if path.contains('"') || path.contains('\n') || path.contains('\r') { - return Err("Windows Installer requires an installation path without quotes".to_owned()); + return Err( + "Windows Installer requires an installation path without quotes".to_owned(), + ); } if path.is_empty() || path.len() > 32767 { return Err("Windows Installer requires a bounded installation path".to_owned()); @@ -696,15 +698,11 @@ mod tests { #[test] fn msi_install_root_quotes_only_the_value_for_msiexec() { #[cfg(windows)] - let argument = windows::install_root_argument(Path::new( - r"C:\Program Files\OpenNOW Nightly", - )) - .unwrap(); + let argument = + windows::install_root_argument(Path::new(r"C:\Program Files\OpenNOW Nightly")).unwrap(); #[cfg(not(windows))] - let argument = msi_install_root_argument(Path::new( - r"C:\Program Files\OpenNOW Nightly", - )) - .unwrap(); + let argument = + msi_install_root_argument(Path::new(r"C:\Program Files\OpenNOW Nightly")).unwrap(); assert_eq!( argument, r#"INSTALL_ROOT="C:\Program Files\OpenNOW Nightly""#