diff --git a/CHANGELOG.md b/CHANGELOG.md index fda0657c..4e3e33e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ All notable changes to OCM are documented here. ### Fixed +- Accept reformatted and binary macOS LaunchAgent plists for the same OCM store while still rejecting foreign or invalid owners. Thanks @TheAngryPit (#147, #149). - Keep npm-owned executable updates with npm, prevent temporary npx caches from owning background services, and preserve process identity during npm-launched gateway refreshes. Protect managed and symlinked installer destinations even diff --git a/Cargo.lock b/Cargo.lock index 26be9c9b..22e0921a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -571,6 +571,7 @@ dependencies = [ "indicatif", "json5", "libc", + "plist", "rusqlite", "semver", "serde", @@ -607,6 +608,18 @@ version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f6b464fbc74e149a392436b17d523f769e057cb6877f6a5c4618bc6f11800548" +[[package]] +name = "plist" +version = "1.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2896bade328c13f7042a297ea5ac5b0951f6cf989dea5f32c2fd98da398195cb" +dependencies = [ + "base64", + "indexmap", + "quick-xml", + "time", +] + [[package]] name = "portable-atomic" version = "1.15.0" @@ -637,6 +650,15 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "quick-xml" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41b1177fdf999d2321d3fb46ff47159d9c1fb9ad66a4879f8c50a0b504615e9b" +dependencies = [ + "memchr", +] + [[package]] name = "quote" version = "1.0.47" diff --git a/Cargo.toml b/Cargo.toml index c764364c..cb2dacd9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ path = "src/main.rs" [dependencies] serde = { version = "1.0", features = ["derive"] } serde_json = { version = "1.0", features = ["preserve_order"] } +plist = { version = "1.10", default-features = false } json5 = "1.3" serde_yaml = "0.9" sha2 = "0.11" diff --git a/docs/USAGE.md b/docs/USAGE.md index 056fc38e..9414fec1 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -966,6 +966,11 @@ Background services: Windows service support is not implemented yet. +On macOS, service ownership is read from `EnvironmentVariables.OCM_HOME` in +the LaunchAgent plist. Plist-aware editors can reformat the definition or +convert it to binary without changing ownership. A different, missing, or +invalid owner still prevents OCM from replacing or controlling that service. + ## Safety notes `ocm` keeps safety checks around destructive actions. diff --git a/src/service/platform.rs b/src/service/platform.rs index 4a43ba62..81c635da 100644 --- a/src/service/platform.rs +++ b/src/service/platform.rs @@ -292,27 +292,47 @@ pub(crate) fn managed_service_owner_matches( ocm_home: &str, env: &BTreeMap, ) -> Result { - let raw = fs::read_to_string(definition_path).map_err(|error| { + let raw = fs::read(definition_path).map_err(|error| { format!( "failed to read existing service definition {}: {error}", display_path(definition_path) ) })?; - let owner_markers = match service_manager_kind(env) { - ServiceManagerKind::Launchd => vec![format!( - "OCM_HOME\n {}", - plist_escape(ocm_home) - )], - ServiceManagerKind::SystemdUser => vec![ - format!("Environment=\"OCM_HOME={}\"", systemd_escape(ocm_home)), - format!( - "Environment=\"OCM_HOME={}\"", - systemd_legacy_escape(ocm_home) - ), - ], - ServiceManagerKind::Unsupported => return Ok(true), - }; - Ok(owner_markers.iter().any(|marker| raw.contains(marker))) + match service_manager_kind(env) { + ServiceManagerKind::Launchd => { + let value = plist::Value::from_reader(std::io::Cursor::new(&raw)).map_err(|error| { + format!( + "failed to parse existing service definition {}: {error}", + display_path(definition_path) + ) + })?; + Ok(value + .as_dictionary() + .and_then(|dict| dict.get("EnvironmentVariables")) + .and_then(plist::Value::as_dictionary) + .and_then(|dict| dict.get("OCM_HOME")) + .and_then(plist::Value::as_string) + == Some(ocm_home)) + } + ServiceManagerKind::SystemdUser => { + let raw = std::str::from_utf8(&raw).map_err(|error| { + format!( + "failed to read existing service definition {}: {error}", + display_path(definition_path) + ) + })?; + Ok([ + format!("Environment=\"OCM_HOME={}\"", systemd_escape(ocm_home)), + format!( + "Environment=\"OCM_HOME={}\"", + systemd_legacy_escape(ocm_home) + ), + ] + .iter() + .any(|marker| raw.contains(marker))) + } + ServiceManagerKind::Unsupported => Ok(true), + } } pub(crate) fn validate_managed_service_executable( @@ -1725,6 +1745,70 @@ mod tests { fs::remove_dir_all(&root).unwrap(); } + #[test] + fn launchd_service_owner_accepts_reformatted_plist() { + let root = tempfile::tempdir().unwrap(); + let env = BTreeMap::from([( + "OCM_INTERNAL_SERVICE_MANAGER".to_string(), + "launchd".to_string(), + )]); + let definition = ManagedServiceDefinition { + label: OCM_SERVICE_LABEL.to_string(), + description: "owner fixture".to_string(), + definition_path: root.path().join("owner.plist"), + program_arguments: vec!["/bin/true".to_string()], + working_directory: root.path().to_path_buf(), + stdout_path: root.path().join("stdout.log"), + stderr_path: root.path().join("stderr.log"), + environment: BTreeMap::from([( + "OCM_HOME".to_string(), + "/tmp/store & data".to_string(), + )]), + }; + write_managed_service_definition(&definition, &env).unwrap(); + let original = fs::read_to_string(&definition.definition_path).unwrap(); + let reformatted = original.replace(" ", "\t"); + assert_ne!(original, reformatted); + fs::write(&definition.definition_path, &reformatted).unwrap(); + super::validate_managed_service_owner(&definition, &env).unwrap(); + assert_eq!( + fs::read_to_string(&definition.definition_path).unwrap(), + reformatted + ); + let value = plist::Value::from_reader_xml(reformatted.as_bytes()).unwrap(); + value.to_file_binary(&definition.definition_path).unwrap(); + super::validate_managed_service_owner(&definition, &env).unwrap(); + + // Only EnvironmentVariables.OCM_HOME owns the service. A matching string + // elsewhere (including comments) must never authorize another store. + for body in [ + "EnvironmentVariablesOCM_HOME/tmp/other", + "OCM_HOME/tmp/store & data", + "EnvironmentVariablesOCM_HOME7", + "EnvironmentVariables/tmp/store & data", + "", + "", + ] { + let fixture = format!("{body}"); + fs::write(&definition.definition_path, &fixture).unwrap(); + let error = write_managed_service_definition(&definition, &env).unwrap_err(); + assert!( + error.contains("already bound to a different OCM_HOME"), + "{error}" + ); + assert_eq!( + fs::read_to_string(&definition.definition_path).unwrap(), + fixture + ); + } + fs::write(&definition.definition_path, "not a plist").unwrap(); + let error = super::validate_managed_service_owner(&definition, &env).unwrap_err(); + assert!( + error.contains("failed to parse existing service definition"), + "{error}" + ); + } + #[test] fn stable_service_identity_rejects_a_different_store_owner() { let unique = SystemTime::now() diff --git a/tests/daemon_runtime_tests.rs b/tests/daemon_runtime_tests.rs index 422cd40c..800ac223 100644 --- a/tests/daemon_runtime_tests.rs +++ b/tests/daemon_runtime_tests.rs @@ -1794,6 +1794,7 @@ fn daemon_defers_a_saved_service_start_until_source_watch_releases_the_env() { let source_was_started = launcher_marker.exists(); drop(source_watch); let after = wait_for_runtime_children(&runtime_path, 2, Some("demo"), Duration::from_secs(10)); + let source_was_resumed = wait_for_file(&launcher_marker, Duration::from_secs(5)); stop_process(&mut daemon); assert!(during.is_some(), "unwatched sibling should remain runnable"); @@ -1803,7 +1804,10 @@ fn daemon_defers_a_saved_service_start_until_source_watch_releases_the_env() { after.is_some(), "service did not resume after watch released" ); - assert!(launcher_marker.exists()); + assert!( + source_was_resumed, + "resumed child did not write its startup marker" + ); } #[test] diff --git a/tests/service_command_tests.rs b/tests/service_command_tests.rs index 3647dd7b..3ae7d71c 100644 --- a/tests/service_command_tests.rs +++ b/tests/service_command_tests.rs @@ -715,6 +715,53 @@ fn service_stop_keeps_the_daemon_while_a_sibling_env_is_running() { assert!(managed_service_definition_path(&env, &cwd, "ocm").exists()); } +#[test] +fn service_start_accepts_reformatted_same_store_plist() { + let root = TestDir::new("service-start-reformatted-owner"); + let cwd = root.child("workspace"); + fs::create_dir_all(&cwd).unwrap(); + let env = launchd_env(&root); + setup_launcher_env(&cwd, &env); + let started = run_ocm(&cwd, &env, &["service", "start", "demo"]); + assert!(started.status.success(), "{}", stderr(&started)); + + let path = managed_service_definition_path(&env, &cwd, "ocm"); + let original = fs::read_to_string(&path).unwrap(); + let reformatted = original.replace(" ", "\t"); + assert_ne!(original, reformatted); + fs::write(&path, &reformatted).unwrap(); + #[cfg(target_os = "macos")] + { + let edited = Command::new("/usr/libexec/PlistBuddy") + .args(["-c", "Set :Comment reformatted-owner-fixture"]) + .arg(&path) + .output() + .unwrap(); + assert!(edited.status.success(), "{}", stderr(&edited)); + } + let restarted = run_ocm(&cwd, &env, &["service", "start", "demo"]); + assert!(restarted.status.success(), "{}", stderr(&restarted)); + + let mut foreign_env = env.clone(); + foreign_env.insert( + "OCM_HOME".to_string(), + path_string(&root.child("foreign-store")), + ); + setup_launcher_env(&cwd, &foreign_env); + let before = fs::read(&path).unwrap(); + let rejected = run_ocm(&cwd, &foreign_env, &["service", "start", "demo"]); + assert!(!rejected.status.success()); + assert!( + stderr(&rejected).contains("already bound to a different OCM_HOME"), + "{}", + stderr(&rejected) + ); + assert_eq!(fs::read(&path).unwrap(), before); + + let stopped = run_ocm(&cwd, &env, &["service", "stop", "demo"]); + assert!(stopped.status.success(), "{}", stderr(&stopped)); +} + #[test] fn service_start_rejects_a_daemon_owned_by_another_store() { let root = TestDir::new("service-start-other-store-owner"); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 488c6fa8..132e83d4 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -411,7 +411,7 @@ pub fn npm_fixture(package: &Path) -> Option<(PathBuf, PathBuf)> { ); let binary = payload.join(format!("vendor/{target}/bin/ocm")); fs::create_dir_all(binary.parent().unwrap()).unwrap(); - fs::hard_link(ocm_test_binary_path(), &binary).unwrap(); + fs::copy(ocm_test_binary_path(), &binary).unwrap(); let entrypoint = package.join("bin/ocm.cjs"); write_executable_script(&entrypoint, include_str!("../../npm/ocm.cjs")); Some((entrypoint, binary)) diff --git a/tests/upgrade_command_tests.rs b/tests/upgrade_command_tests.rs index 09e8e8f3..1a1a47f8 100644 --- a/tests/upgrade_command_tests.rs +++ b/tests/upgrade_command_tests.rs @@ -1184,15 +1184,8 @@ fn upgrade_rolls_back_when_gateway_rpc_is_not_ready() { let cwd = root.child("workspace"); fs::create_dir_all(&cwd).unwrap(); - let health_server = - TestHttpServer::serve_bytes_times("/health", "application/json", br#"{"ok":true}"#, 8); - let health_url = health_server.url(); - let health_port = health_url - .split(':') - .nth(2) - .and_then(|value| value.split('/').next()) - .and_then(|value| value.parse::().ok()) - .unwrap(); + let (health_port, health_requests, health_stop, health_handle) = + spawn_converging_health_server(); let old_tarball = openclaw_package_tarball(&recording_openclaw_script("2026.3.24"), "2026.3.24"); @@ -1356,6 +1349,7 @@ fn upgrade_rolls_back_when_gateway_rpc_is_not_ready() { let upgrade = run_ocm(&cwd, &env, &["upgrade", "demo"]); observer_done.store(true, Ordering::Relaxed); let (stop_count, start_count, target_entered_backoff) = restart_observer.join().unwrap(); + stop_converging_health_server(health_port, &health_stop, health_handle); assert!(!upgrade.status.success(), "{}", stdout(&upgrade)); let output = stdout(&upgrade); @@ -1365,7 +1359,7 @@ fn upgrade_rolls_back_when_gateway_rpc_is_not_ready() { output.contains("post-upgrade gateway readiness failed: gateway RPC is not ready"), "{output}" ); - assert!(!health_server.requests().is_empty()); + assert!(health_requests.load(Ordering::SeqCst) > 0); assert!(target_entered_backoff); assert!(stop_count >= 2, "stop_count={stop_count}"); assert!(start_count >= 2, "start_count={start_count}");