From fb61ae15fcf960937dfecdb77eecfac96ddbc669 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 8 Aug 2026 00:48:59 -0700 Subject: [PATCH] fix(daemon): skip unreadable foreign LaunchDaemon plists in ownership scan (#120481) * fix(daemon): skip unreadable foreign LaunchDaemon plists in ownership scan inspectSystemLaunchDaemonOwnership raised SystemLaunchDaemonOwnershipError when any /Library/LaunchDaemons plist was unreadable, such as third-party VPN helper plists with mode 600. This broke gateway install/repair and launchd.integration.e2e on affected Macs. Unreadable plists are foreign by contract, while loaded same-label daemons remain caught by the bracketing launchctl probes. * fix(ci): regenerate plugin-sdk api baseline after 267268f646c * test(doctor): scope gateway-bind persistence test to its writer seam Culprit: 267268f646c. * docs(daemon): document unreadable plist tradeoff --- .../.generated/plugin-sdk-api-baseline.sha256 | 2 +- ...nfig-flow.gateway-bind-persistence.test.ts | 39 +++++---------- src/daemon/launchd-system.test.ts | 50 +++++++++++++++---- src/daemon/launchd-system.ts | 17 ++++++- 4 files changed, 70 insertions(+), 38 deletions(-) diff --git a/docs/.generated/plugin-sdk-api-baseline.sha256 b/docs/.generated/plugin-sdk-api-baseline.sha256 index 7191324f3ab3..00b4a65aff35 100644 --- a/docs/.generated/plugin-sdk-api-baseline.sha256 +++ b/docs/.generated/plugin-sdk-api-baseline.sha256 @@ -122,7 +122,7 @@ aa8a411ad37c1d1143b67376bf2d20255b9eedff61d80815f42e4f8ed7bd8e58 module/secret- 57dcb1462d4c4f9a98d934c4ca975b163d704758af9821a64001ff3ac05637c3 module/secret-input-runtime dc0ee07d392a85c218939000b28c0138f139215da00f5592b34a68ba8e29a25d module/secret-ref-runtime f97549081955e412d8eb64070c64bb5921bb1324744d824db05767d3adcce403 module/security-runtime -5b1e30fe228e0d8c74c11d6ad2d9aed3df80cd3ea4ba2dd63c30cc5f084e93c9 module/session-catalog +8f57eaf07620598f0a62cf603d450d9a9da2d4ea7a867f13aadc1212019e5eb9 module/session-catalog 50f5e344f98c27570b7a30e32a906b612e2383d21f102e88cd93e1d5425a6de9 module/session-discussion f112bdabc51ba8659b37d0a6f6a32a2b1d471e5b49b56e108bf750ec55a7ea71 module/session-store-runtime 36affbe151431a6141664b6838e20f2d121ff210d57a3c1b4b41a8818b5c81d8 module/setup diff --git a/src/commands/doctor-config-flow.gateway-bind-persistence.test.ts b/src/commands/doctor-config-flow.gateway-bind-persistence.test.ts index 2191a6597f00..394c0ec3a59c 100644 --- a/src/commands/doctor-config-flow.gateway-bind-persistence.test.ts +++ b/src/commands/doctor-config-flow.gateway-bind-persistence.test.ts @@ -1,20 +1,14 @@ // Verifies Doctor persists legacy gateway bind repairs through the real config writer. import fs from "node:fs/promises"; -import { afterEach, describe, expect, it, vi } from "vitest"; -import { readConfigFileSnapshot } from "../config/config.js"; +import { describe, expect, it, vi } from "vitest"; import { withTempHome, writeOpenClawConfig } from "../config/test-helpers.js"; import { runInitialConfigWriteHealth } from "../flows/doctor-health-contribution-runners.config.js"; import type { DoctorHealthFlowContext } from "../flows/doctor-health-contribution-types.js"; import type { RuntimeEnv } from "../runtime.js"; -import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js"; -import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js"; import { createDoctorPrompter, type DoctorOptions } from "./doctor-prompter.js"; +import { migrateLegacyConfig } from "./doctor/shared/legacy-config-migrate.js"; describe("Doctor gateway bind persistence", () => { - afterEach(() => { - closeOpenClawStateDatabaseForTest(); - }); - it.each([ ["localhost", "loopback"], ["0.0.0.0", "lan"], @@ -30,36 +24,27 @@ describe("Doctor gateway bind persistence", () => { }; const options: DoctorOptions = { nonInteractive: true, repair: true }; const prompter = createDoctorPrompter({ runtime, options }); - const configResult = await loadAndMaybeMigrateDoctorConfig({ - options, - confirm: (params) => prompter.confirm(params), - runtime, - prompter, - }); + const migration = migrateLegacyConfig({ gateway: { mode: "local", bind: legacyBind } }); + expect(migration.config).not.toBeNull(); + const cfg = migration.config!; + const configResult = { cfg, shouldWriteConfig: true }; const ctx: DoctorHealthFlowContext = { runtime, options, prompter, configResult, - cfg: configResult.cfg, - cfgForPersistence: structuredClone(configResult.cfg), - sourceConfigValid: configResult.sourceConfigValid ?? true, + cfg, + cfgForPersistence: structuredClone(cfg), + sourceConfigValid: true, configPath, stateDirExistedAtStart: true, - ...(configResult.runWithPluginMetadataSnapshot - ? { runWithPluginMetadataSnapshot: configResult.runWithPluginMetadataSnapshot } - : {}), - ...(configResult.invalidatePluginMetadataSnapshot - ? { invalidatePluginMetadataSnapshot: configResult.invalidatePluginMetadataSnapshot } - : {}), }; await runInitialConfigWriteHealth(ctx); - const snapshot = await readConfigFileSnapshot(); - expect(snapshot.valid).toBe(true); - expect(snapshot.config.gateway?.bind).toBe(canonicalBind); - expect(await fs.readFile(configPath, "utf-8")).not.toContain(`"bind": "${legacyBind}"`); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")); + expect(persisted.gateway?.bind).toBe(canonicalBind); + expect(persisted.gateway?.bind).not.toBe(legacyBind); }); }); }); diff --git a/src/daemon/launchd-system.test.ts b/src/daemon/launchd-system.test.ts index cc236bb6b066..bc697f6ae02b 100644 --- a/src/daemon/launchd-system.test.ts +++ b/src/daemon/launchd-system.test.ts @@ -1,6 +1,6 @@ // System launchd ownership tests cover loaded, installed, and unverifiable states. import { execFileSync } from "node:child_process"; -import { chmodSync, mkdirSync, writeFileSync } from "node:fs"; +import { chmodSync, constants, mkdirSync, writeFileSync } from "node:fs"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; @@ -20,9 +20,10 @@ function fsError(code: string, target: string): NodeJS.ErrnoException { vi.mock("node:fs/promises", () => { const mocked = { - access: vi.fn(async (target: string) => { + constants, + access: vi.fn(async (target: string, mode?: number) => { const code = state.accessErrors.get(target); - if (code) { + if (code && mode === constants.R_OK) { throw fsError(code, target); } if (!state.files.has(target)) { @@ -80,7 +81,10 @@ import { const tempDirs = useAutoCleanupTempDirTracker(afterEach); const hostPlatform = process.platform; -function runRenderedProbe(plistName: string, plistMode: "unlabeled" | "same-label" | "malformed") { +function runRenderedProbe( + plistName: string, + plistMode: "unlabeled" | "same-label" | "malformed" | "unreadable", +) { const root = tempDirs.make("openclaw-launchd-probe-"); const daemonsDir = path.join(root, "daemons"); const binDir = path.join(root, "bin"); @@ -93,6 +97,12 @@ function runRenderedProbe(plistName: string, plistMode: "unlabeled" | "same-labe `#!/bin/sh mode=$1 for last; do :; done +case "$last" in + *unreadable*) + printf '%s\\n' "Operation not permitted" >&2 + exit 1 + ;; +esac fixture=$(cat "$last") if [ "$mode" = "-extract" ]; then if [ "$fixture" = "same-label" ]; then @@ -117,6 +127,9 @@ exit 1 ); chmodSync(plutilShim, 0o700); chmodSync(launchctlShim, 0o700); + if (plistMode === "unreadable") { + chmodSync(path.join(daemonsDir, plistName), 0o000); + } const script = renderSystemLaunchDaemonOwnershipShellProbe("ai.openclaw.gateway") .replaceAll("/Library/LaunchDaemons", daemonsDir) .replaceAll("/usr/bin/plutil", plutilShim) @@ -260,17 +273,34 @@ describe("system LaunchDaemon ownership", () => { expect(execLaunchctl).toHaveBeenCalledTimes(2); }); - it("fails closed on an unreadable noncanonical vendor plist", async () => { + it("skips an unreadable foreign plist", async () => { const unrelated = "/Library/LaunchDaemons/com.vendor.locked.plist"; state.files.set(unrelated, ""); state.accessErrors.set(unrelated, "EACCES"); state.plutilErrors.set(unrelated, "Operation not permitted"); await expect(inspectSystemLaunchDaemonOwnership("ai.openclaw.gateway")).resolves.toEqual({ - status: "unverifiable", + status: "absent", serviceTarget: "system/ai.openclaw.gateway", - operation: "filesystem", - detail: `${unrelated}: EACCES: ${unrelated}`, + }); + await expect( + assertNoSystemLaunchDaemonOwnership("ai.openclaw.gateway"), + ).resolves.toBeUndefined(); + }); + + it("detects a readable owner after an unreadable foreign plist", async () => { + const unrelated = "/Library/LaunchDaemons/com.vendor.locked.plist"; + const owner = "/Library/LaunchDaemons/vendor-openclaw.plist"; + state.files.set(unrelated, ""); + state.accessErrors.set(unrelated, "EACCES"); + state.plutilErrors.set(unrelated, "Operation not permitted"); + state.files.set(owner, ""); + state.plutilValues.set(owner, { Label: "ai.openclaw.gateway" }); + + await expect(inspectSystemLaunchDaemonOwnership("ai.openclaw.gateway")).resolves.toEqual({ + status: "installed", + serviceTarget: "system/ai.openclaw.gateway", + plistPath: owner, }); }); @@ -332,6 +362,7 @@ describe("system LaunchDaemon ownership", () => { "/usr/bin/find \"$openclaw_system_launchd_dir\" -mindepth 1 -maxdepth 1 -name '*.plist' -print0", ); expect(script).toContain("while IFS= read -r -d '' openclaw_system_launchd_plist"); + expect(script).toContain('[ ! -r "$openclaw_system_launchd_plist" ]'); expect(script).toContain('[ ! -x "$openclaw_system_launchd_dir" ]'); expect(script).not.toContain('"$openclaw_system_launchd_dir"/*.plist'); expect(script).toContain( @@ -340,8 +371,9 @@ describe("system LaunchDaemon ownership", () => { expect(script).not.toContain("|| true"); }); - it("executes the rendered probe across unlabeled, same-label, and malformed plists", () => { + it("executes the rendered probe across readable and unreadable plists", () => { expect(runRenderedProbe("com.google.keystone.daemon.plist", "unlabeled")).toBe(""); + expect(runRenderedProbe("com.vendor.unreadable.plist", "unreadable")).toBe(""); expect(runRenderedProbe("vendor-openclaw.plist", "same-label")).toContain( "vendor-openclaw.plist", ); diff --git a/src/daemon/launchd-system.ts b/src/daemon/launchd-system.ts index 9769e9df2458..695440a5be8d 100644 --- a/src/daemon/launchd-system.ts +++ b/src/daemon/launchd-system.ts @@ -71,6 +71,11 @@ if [ -z "$openclaw_system_launchd_conflict" ]; then if openclaw_system_launchd_entries=$(/usr/bin/mktemp "\${TMPDIR:-/tmp}/openclaw-launchd-scan.XXXXXX" 2>&1); then if /usr/bin/find "$openclaw_system_launchd_dir" -mindepth 1 -maxdepth 1 -name '*.plist' -print0 >"$openclaw_system_launchd_entries"; then while IFS= read -r -d '' openclaw_system_launchd_plist; do + # Unreadable plists are treated as foreign: loaded same-label daemons are caught by the + # bracketing launchctl probes; an unloaded unreadable same-label plist is an accepted operator-created edge (#120481). + if [ ! -r "$openclaw_system_launchd_plist" ]; then + continue + fi if openclaw_system_launchd_plist_label=$(/usr/bin/plutil -extract Label raw -o - -- "$openclaw_system_launchd_plist" 2>&1); then if [ "$openclaw_system_launchd_plist_label" != "$openclaw_system_launchd_label" ]; then continue @@ -115,6 +120,7 @@ type LaunchDaemonPlistLabelResult = | { status: "ok"; label: string } | { status: "unlabeled" } | { status: "missing" } + | { status: "unreadable" } | { status: "unverifiable"; detail: string }; /** Reads the top-level Label through the native parser for XML and binary plists. */ @@ -141,11 +147,15 @@ export async function readLaunchDaemonPlistLabel( } } try { - await fs.access(plistPath); + await fs.access(plistPath, fs.constants.R_OK); } catch (error) { if (isMissingPathError(error)) { return { status: "missing" }; } + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "EACCES" || code === "EPERM") { + return { status: "unreadable" }; + } return { status: "unverifiable", detail: formatUnknownError(error) }; } return { @@ -178,6 +188,11 @@ async function findInstalledSystemLaunchDaemon( if (result.status === "ok" && result.label === label) { return { status: "installed", plistPath }; } + // Unreadable plists are treated as foreign: loaded same-label daemons are caught by the + // bracketing launchctl probes; an unloaded unreadable same-label plist is an accepted operator-created edge (#120481). + if (result.status === "unreadable") { + continue; + } if (result.status === "unverifiable") { return { status: "unverifiable", detail: `${plistPath}: ${result.detail}` }; }