From e249683ac944c5398f84c2e1bb2f9247c3efa3f1 Mon Sep 17 00:00:00 2001 From: yahya-nusseibeh <254622510+yahya-nusseibeh@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:16:01 +0200 Subject: [PATCH] horizon-server update on Windows: swap the current junction safely point() removed the `current` junction before making the new one, so a failed mklink left the server with no current release, and std quotes an argument only when it holds a space, so a folder like D:\mc&co\ reached cmd unquoted and & split the command. - The new junction is made first as current.switching; the old one is removed and the new one renamed into place; if that rename fails the old junction is put back. - mklink runs as `cmd /D /S /C "mklink /J "link" "target""` with raw_arg, both paths quoted; paths holding % or " (which keep a meaning inside quotes) are refused with a message. - Test (Windows only): real junctions in a folder named with `&`: current moves v1 -> v2 and reads each release's file, the old release is untouched, no current.switching is left, and a release whose path holds % is refused with current unchanged. Compiled, linted (clippy -D warnings) and tested on GitHub's windows-latest runner: all 146 horizon-server tests, the two junction tests among them. The Unix build and tests pass too. Split from #9129. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SJ3stK8iXVDXqoi7midcqd --- crates/horizon-server/src/update.rs | 98 ++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/crates/horizon-server/src/update.rs b/crates/horizon-server/src/update.rs index 4dc0a54364..cd7a839b33 100644 --- a/crates/horizon-server/src/update.rs +++ b/crates/horizon-server/src/update.rs @@ -215,18 +215,52 @@ fn point(link: &Path, target: &Path) -> std::io::Result<()> { std::fs::rename(&fresh, link) } -/// Makes `link` a junction to `target` (install.ps1's links: they need no admin rights). +/// Makes `link` a junction to `target` (install.ps1's links: they need no admin rights). The new +/// junction is made first, beside it, so a failure leaves `current` as it was; it then takes the old +/// one's place, and if that last step fails the old junction is put back. #[cfg(windows)] fn point(link: &Path, target: &Path) -> std::io::Result<()> { - if link.exists() || link.symlink_metadata().is_ok() { - std::fs::remove_dir(link)?; + let fresh = link.with_extension("switching"); + if fresh.symlink_metadata().is_ok() { + std::fs::remove_dir(&fresh)?; } + junction(&fresh, target)?; + let old = std::fs::read_link(link).ok(); + if link.symlink_metadata().is_ok() { + // A junction: removing it removes the link, never the release it points at. + if let Err(e) = std::fs::remove_dir(link) { + let _ = std::fs::remove_dir(&fresh); + return Err(e); + } + } + std::fs::rename(&fresh, link).inspect_err(|_| { + let _ = std::fs::remove_dir(&fresh); + if let Some(old) = &old { + let _ = junction(link, old); + } + }) +} + +/// `mklink /J link target` through cmd, with both paths quoted as cmd reads them: unquoted, a folder +/// named with `&` or `^` (and no space, so std doesn't quote it) would split the command. `%` and `"` +/// keep a meaning inside quotes, so paths holding them are refused. +#[cfg(windows)] +fn junction(link: &Path, target: &Path) -> std::io::Result<()> { + use std::os::windows::process::CommandExt; + let quoted = |p: &Path| -> std::io::Result { + let text = p.to_str().ok_or_else(|| { + std::io::Error::other(format!("{}: not a path cmd can take", p.display())) + })?; + if text.contains(['%', '"']) { + return Err(std::io::Error::other(format!( + "{text}: a folder whose path holds % or \" can't be linked; move the server elsewhere" + ))); + } + Ok(format!("\"{text}\"")) + }; + let line = mklink_line("ed(link)?, "ed(target)?); let status = crate::process::command("cmd") - .arg("/C") - .arg("mklink") - .arg("/J") - .arg(link) - .arg(target) + .raw_arg(line) .stdout(std::process::Stdio::null()) .status()?; if status.success() { @@ -238,6 +272,13 @@ fn point(link: &Path, target: &Path) -> std::io::Result<()> { } } +/// cmd's arguments for a junction, both paths already quoted: `/D` skips AutoRun, and `/S /C "..."` +/// makes cmd take the outer quotes off and run the rest as written. +#[cfg_attr(not(windows), allow(dead_code))] +fn mklink_line(link: &str, target: &str) -> String { + format!("/D /S /C \"mklink /J {link} {target}\"") +} + /// A release on GitHub: its name here and the archive for this computer. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Release { @@ -908,6 +949,47 @@ mod tests { Install { root } } + /// Real junctions, in a folder whose name cmd would split on unquoted (`&`, no space so std doesn't + /// quote it): `current` moves from one release to the other and nothing else changes. + #[cfg(windows)] + #[test] + fn junctions_switch_in_a_folder_cmd_would_split() { + let root = std::env::temp_dir().join(format!( + "horizon-server-junction-mc&co-{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&root); + for tag in ["v1", "v2", "v%3"] { + let release = root.join("releases").join(tag); + std::fs::create_dir_all(&release).unwrap(); + std::fs::write(release.join(exe("horizon-server")), tag).unwrap(); + } + let install = Install { root: root.clone() }; + let running = root.join("current").join(exe("horizon-server")); + install.switch("v1").unwrap(); + assert_eq!(std::fs::read_to_string(&running).unwrap(), "v1"); + install.switch("v2").unwrap(); + assert_eq!(std::fs::read_to_string(&running).unwrap(), "v2"); + assert!(root.join("current.switching").symlink_metadata().is_err()); + // Replacing the junction never touched the release it pointed at. + let v1 = root.join("releases").join("v1").join(exe("horizon-server")); + assert_eq!(std::fs::read_to_string(v1).unwrap(), "v1"); + // `%` keeps a meaning inside cmd's quotes: refused, and `current` stays where it was. + let refused = install.switch("v%3").unwrap_err(); + assert!(refused.contains('%'), "{refused}"); + assert_eq!(std::fs::read_to_string(&running).unwrap(), "v2"); + assert!(root.join("current.switching").symlink_metadata().is_err()); + std::fs::remove_dir_all(&root).unwrap(); + } + + #[test] + fn a_junctions_paths_are_quoted_for_cmd() { + assert_eq!( + mklink_line(r#""D:\mc&co\current""#, r#""D:\mc&co\releases\v0.2.95""#), + r#"/D /S /C "mklink /J "D:\mc&co\current" "D:\mc&co\releases\v0.2.95"""# + ); + } + #[test] fn tags_compare_by_number() { assert!(newer("v2026.10.19", "v2026.10.18.27"));