From 41acc3e21afdcfd21748e01ef5ef7ff7047d462b Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Sat, 1 Aug 2026 00:09:59 +0200 Subject: [PATCH] fix(service): unescape plist XML, surface both launchctl streams, clear status fields when not installed Three review findings on task 4: - read_plist_fields now parses the plist properly via the plist crate (already a transitive dep of service-manager, promoted to direct) instead of slicing raw XML, so a PATH or program path containing & or < no longer round-trips as literal &/< through cli::service::render_status. - launchctl's error path now includes both stdout and stderr, trimmed and joined only on non-empty parts, so a failure never surfaces as "launchctl load failed: " with nothing after the colon. - status() now returns identical None fields (program, path_env, snapshotted, pid) whether the plist is absent or the plist exists but the crate reports NotInstalled (e.g. written but never loaded, or booted out of band) - extracted via a shared not_installed_status/build_status split so both paths run the same code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EGntTHCW3sEPy1VBRopNNp --- Cargo.lock | 1 + Cargo.toml | 1 + crates/xy/Cargo.toml | 1 + crates/xy/src/service.rs | 195 +++++++++++++++++++++++++++++++-------- 4 files changed, 161 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c1381c2..7dad1b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1348,6 +1348,7 @@ dependencies = [ "etcetera", "humantime", "nix", + "plist", "serde", "serde_json", "service-manager", diff --git a/Cargo.toml b/Cargo.toml index fc03f27..aeaeb82 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,3 +34,4 @@ async-trait = "0.1" tempfile = "3" tokio-test = "0.4" service-manager = "0.11" +plist = "1" diff --git a/crates/xy/Cargo.toml b/crates/xy/Cargo.toml index dd72002..20c0fee 100644 --- a/crates/xy/Cargo.toml +++ b/crates/xy/Cargo.toml @@ -31,6 +31,7 @@ etcetera.workspace = true nix.workspace = true humantime.workspace = true service-manager = { workspace = true } +plist = { workspace = true } [dev-dependencies] tempfile.workspace = true diff --git a/crates/xy/src/service.rs b/crates/xy/src/service.rs index 937b570..ac2e663 100644 --- a/crates/xy/src/service.rs +++ b/crates/xy/src/service.rs @@ -139,33 +139,27 @@ pub(crate) fn read_pid(pidfile: &Path) -> Option { .ok() } -pub(crate) fn status(label: &str, pidfile: &Path) -> Result { - let plist = plist_path_for(label)?; +fn not_installed_status(label: &str, plist: PathBuf) -> AgentStatus { + AgentStatus { + label: label.to_string(), + plist, + state: AgentState::NotInstalled, + program: None, + path_env: None, + snapshotted: None, + pid: None, + } +} - if !plist.exists() { - return Ok(AgentStatus { - label: label.to_string(), - plist, - state: AgentState::NotInstalled, - program: None, - path_env: None, - snapshotted: None, - pid: None, - }); +fn build_status(label: &str, plist: PathBuf, pidfile: &Path, state: AgentState) -> AgentStatus { + if state == AgentState::NotInstalled { + return not_installed_status(label, plist); } let snapshotted = std::fs::metadata(&plist) .and_then(|meta| meta.modified()) .ok(); - let parsed: ServiceLabel = label.parse().context("parse service label")?; - - let state = match manager().status(ServiceStatusCtx { label: parsed })? { - ServiceStatus::Running => AgentState::Running, - ServiceStatus::Stopped(_) => AgentState::Stopped, - ServiceStatus::NotInstalled => AgentState::NotInstalled, - }; - let pid = if state == AgentState::Running { read_pid(pidfile) } else { @@ -174,7 +168,7 @@ pub(crate) fn status(label: &str, pidfile: &Path) -> Result { let (program, path_env) = read_plist_fields(&plist); - Ok(AgentStatus { + AgentStatus { label: label.to_string(), plist, state, @@ -182,26 +176,46 @@ pub(crate) fn status(label: &str, pidfile: &Path) -> Result { path_env, snapshotted, pid, - }) + } +} + +pub(crate) fn status(label: &str, pidfile: &Path) -> Result { + let plist = plist_path_for(label)?; + + if !plist.exists() { + return Ok(not_installed_status(label, plist)); + } + + let parsed: ServiceLabel = label.parse().context("parse service label")?; + + let state = match manager().status(ServiceStatusCtx { label: parsed })? { + ServiceStatus::Running => AgentState::Running, + ServiceStatus::Stopped(_) => AgentState::Stopped, + ServiceStatus::NotInstalled => AgentState::NotInstalled, + }; + + Ok(build_status(label, plist, pidfile, state)) } fn read_plist_fields(plist: &Path) -> (Option, Option) { - let Ok(contents) = std::fs::read_to_string(plist) else { + let Ok(value) = plist::Value::from_file(plist) else { return (None, None); }; - let program = contents - .split("ProgramArguments") - .nth(1) - .and_then(|rest| rest.split("").nth(1)) - .and_then(|rest| rest.split("").next()) + let dict = value.as_dictionary(); + + let program = dict + .and_then(|dict| dict.get("ProgramArguments")) + .and_then(plist::Value::as_array) + .and_then(|args| args.first()) + .and_then(plist::Value::as_string) .map(PathBuf::from); - let path_env = contents - .split("PATH") - .nth(1) - .and_then(|rest| rest.split("").nth(1)) - .and_then(|rest| rest.split("").next()) + let path_env = dict + .and_then(|dict| dict.get("EnvironmentVariables")) + .and_then(plist::Value::as_dictionary) + .and_then(|env| env.get("PATH")) + .and_then(plist::Value::as_string) .map(str::to_string); (program, path_env) @@ -215,15 +229,24 @@ fn launchctl(verb: &str, plist: &Path) -> Result<()> { .with_context(|| format!("run launchctl {verb}"))?; if !output.status.success() { - anyhow::bail!( - "launchctl {verb} failed: {}", - String::from_utf8_lossy(&output.stderr).trim() - ); + return Err(launchctl_error(verb, &output.stdout, &output.stderr)); } Ok(()) } +fn launchctl_error(verb: &str, stdout: &[u8], stderr: &[u8]) -> anyhow::Error { + let stdout = String::from_utf8_lossy(stdout); + let stderr = String::from_utf8_lossy(stderr); + + let detail: Vec<&str> = [stdout.trim(), stderr.trim()] + .into_iter() + .filter(|part| !part.is_empty()) + .collect(); + + anyhow::anyhow!("launchctl {verb} failed: {}", detail.join("; ")) +} + pub(crate) fn start(label: &str) -> Result<()> { launchctl("load", &plist_path_for(label)?) } @@ -350,4 +373,102 @@ mod tests { assert_eq!(read_pid(&missing), None); assert_eq!(read_pid(&garbage), None); } + + fn write_agent_plist(path: &Path, program: &str, path_env: Option<&str>) { + let mut dict = plist::Dictionary::new(); + + dict.insert( + "ProgramArguments".to_string(), + plist::Value::Array(vec![plist::Value::String(program.to_string())]), + ); + + if let Some(path_env) = path_env { + let mut env = plist::Dictionary::new(); + env.insert( + "PATH".to_string(), + plist::Value::String(path_env.to_string()), + ); + + dict.insert( + "EnvironmentVariables".to_string(), + plist::Value::Dictionary(env), + ); + } + + plist::Value::Dictionary(dict).to_file_xml(path).unwrap(); + } + + #[test] + fn read_plist_fields_unescapes_xml_entities() { + let tmp = tempfile::tempdir().unwrap(); + let plist_path = tmp.path().join("agent.plist"); + + write_agent_plist( + &plist_path, + "/usr/local/bin/xy", + Some("/usr/bin:/opt/a&b:/opt/"), + ); + + let (program, path_env) = read_plist_fields(&plist_path); + + assert_eq!(program, Some(PathBuf::from("/usr/local/bin/xy"))); + assert_eq!(path_env, Some("/usr/bin:/opt/a&b:/opt/".to_string())); + } + + #[test] + fn read_plist_fields_on_malformed_plist_returns_none_without_panicking() { + let tmp = tempfile::tempdir().unwrap(); + let plist_path = tmp.path().join("broken.plist"); + + std::fs::write(&plist_path, "ProgramArgum").unwrap(); + + let (program, path_env) = read_plist_fields(&plist_path); + + assert!(program.is_none()); + assert!(path_env.is_none()); + } + + #[test] + fn launchctl_error_includes_both_streams_when_populated() { + let err = launchctl_error( + "unload", + b"out text", + b"Unload failed: 5: Input/output error", + ); + + let message = err.to_string(); + + assert!(message.contains("out text")); + assert!(message.contains("Unload failed: 5: Input/output error")); + } + + #[test] + fn launchctl_error_omits_dangling_separator_when_stdout_empty() { + let err = launchctl_error("load", b"", b"boom"); + + assert_eq!(err.to_string(), "launchctl load failed: boom"); + } + + #[test] + fn not_installed_state_clears_all_optional_fields_even_when_plist_exists() { + let tmp = tempfile::tempdir().unwrap(); + let plist_path = tmp.path().join("agent.plist"); + + write_agent_plist(&plist_path, "/usr/local/bin/xy", Some("/usr/bin")); + + let pidfile = tmp.path().join("xy.pid"); + std::fs::write(&pidfile, "4821").unwrap(); + + let status = build_status( + "se.aceofba.xy-ghost", + plist_path, + &pidfile, + AgentState::NotInstalled, + ); + + assert!(status.program.is_none()); + assert!(status.path_env.is_none()); + assert!(status.snapshotted.is_none()); + assert!(status.pid.is_none()); + } }