Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 81 additions & 4 deletions bin/cli/tray/autostart.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,64 @@ export function isAutostartEnabled() {
return false;
}

/**
* Pure parse of `launchctl list <label>` output: true only when the agent's
* "PID" line matches the given process id. launchctl prints the running PID for
* a managed Aqua agent; a loaded-but-not-running agent omits the key entirely.
*
* Exported for unit testing — the darwin branch can't execute on the Linux CI.
*/
export function parseAgentSelfFromLaunchctl(output, pid) {
if (!output) return false;
const match = output.match(/"PID"\s*=\s*(\d+)/);
return !!(match && parseInt(match[1], 10) === pid);
}
Comment on lines +214 to +218

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential false positives (for example, if a path or argument in the launchctl output contains the substring "PID" = <number>), we should anchor the regex to the start of the line using ^ and the multiline flag /m. This ensures we only match the actual "PID" key of the launchd service.

Suggested change
export function parseAgentSelfFromLaunchctl(output, pid) {
if (!output) return false;
const match = output.match(/"PID"\s*=\s*(\d+)/);
return !!(match && parseInt(match[1], 10) === pid);
}
export function parseAgentSelfFromLaunchctl(output, pid) {
if (!output) return false;
const match = output.match(/^\s*"PID"\s*=\s*(\d+)/m);
return !!(match && parseInt(match[1], 10) === pid);
}


/**
* True when `runList()` (which should run `launchctl list <label>`) succeeds,
* i.e. launchd actually recognizes the agent. A plist sitting on disk that
* launchd never loaded must NOT count as enabled — checking file existence
* alone makes the tray menu lie ("✓ Enabled" while launchd has the agent in a
* failed state or never loaded it).
*
* `runList` is injectable so the parse/branch logic is testable without a real
* launchctl on the (Linux) CI runner.
*/
export function isLaunchdAgentLoaded(runList) {
try {
runList();
return true;
} catch {
return false;
}
}

/**
* Returns true when the current Node process IS the running instance launchd is
* managing under our agent label.
*
* `launchctl unload`/`load -w` for a user-domain agent sends SIGTERM to the
* running process. When the running OmniRoute cli was itself spawned by the
* autostart launchd agent (autostart was enabled, then the machine rebooted,
* then the user clicked the tray "Disable Autostart" item), an unload would
* kill the very process executing the click handler — the tray icon would
* silently disappear instead of the menu label flipping. Enable/disable skip
* launchctl in that case; the on-disk plist change is what matters for the next
* login, and launchd already has us loaded under our own PID.
*/
function isAgentSelfMac() {
try {
const output = execFileSync("launchctl", ["list", APP_LABEL], {
encoding: "utf8",
stdio: ["ignore", "pipe", "ignore"],
timeout: 3000,
});
return parseAgentSelfFromLaunchctl(output, process.pid);
} catch {
return false;
}
}

function enableMac() {
const plistDir = join(homedir(), "Library", "LaunchAgents");
mkdirSync(plistDir, { recursive: true });
Expand All @@ -225,6 +283,10 @@ function enableMac() {
<key>KeepAlive</key><false/>
</dict></plist>`;
writeFileSync(plistPath, plist, { mode: 0o644 });
// If we're already the running agent, launchctl load/unload would SIGTERM us.
// The plist is updated on disk and launchd already has us loaded under our own
// PID — nothing more to do for the current session.
if (isAgentSelfMac()) return existsSync(plistPath);
try {
execSync("launchctl load -w " + JSON.stringify(plistPath), { stdio: "ignore" });
} catch {}
Comment on lines 290 to 292

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using execFileSync instead of execSync with shell string concatenation is safer and more robust because it avoids shell parsing and potential shell injection/evaluation issues (e.g., if the user's home directory path contains special shell characters). It is also consistent with how launchctl list is executed elsewhere in this file.

Suggested change
try {
execSync("launchctl load -w " + JSON.stringify(plistPath), { stdio: "ignore" });
} catch {}
try {
execFileSync("launchctl", ["load", "-w", plistPath], { stdio: "ignore" });
} catch {}

Expand All @@ -233,17 +295,32 @@ function enableMac() {

function disableMac() {
const plistPath = join(homedir(), "Library", "LaunchAgents", `${APP_LABEL}.plist`);
try {
execSync("launchctl unload -w " + JSON.stringify(plistPath), { stdio: "ignore" });
} catch {}
// Don't kill ourselves: when the current process is the running agent,
// `launchctl unload` sends SIGTERM and a user clicking "Disable Autostart"
// from the tray would lose the tray icon instead of just flipping the label.
// Removing the plist file is enough to stop the agent at the next login.
if (!isAgentSelfMac()) {
try {
execSync("launchctl unload -w " + JSON.stringify(plistPath), { stdio: "ignore" });
} catch {}
Comment on lines +303 to +305

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using execFileSync instead of execSync with shell string concatenation is safer and more robust because it avoids shell parsing and potential shell injection/evaluation issues. It is also consistent with how launchctl list is executed elsewhere in this file.

Suggested change
try {
execSync("launchctl unload -w " + JSON.stringify(plistPath), { stdio: "ignore" });
} catch {}
try {
execFileSync("launchctl", ["unload", "-w", plistPath], { stdio: "ignore" });
} catch {}

}
try {
unlinkSync(plistPath);
} catch {}
return !existsSync(plistPath);
}

function isEnabledMac() {
return existsSync(join(homedir(), "Library", "LaunchAgents", `${APP_LABEL}.plist`));
const plistPath = join(homedir(), "Library", "LaunchAgents", `${APP_LABEL}.plist`);
if (!existsSync(plistPath)) return false;
// The plist file existing is not enough — launchd must actually recognize the
// agent, otherwise the tray menu reports "Enabled" for an inert/failed agent.
return isLaunchdAgentLoaded(() =>
execFileSync("launchctl", ["list", APP_LABEL], {
stdio: ["ignore", "ignore", "ignore"],
timeout: 3000,
})
);
}

function enableWin() {
Expand Down
91 changes: 91 additions & 0 deletions tests/unit/cli/autostart-macos-launchctl.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import test from "node:test";
import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import { join } from "node:path";
import {
parseAgentSelfFromLaunchctl,
isLaunchdAgentLoaded,
} from "../../../bin/cli/tray/autostart.mjs";

// ---------------------------------------------------------------------------
// parseAgentSelfFromLaunchctl — pure parse of `launchctl list <label>` output.
// True only when the agent's "PID" line matches the current process PID, so the
// enable()/disable() macOS paths can skip launchctl unload/load when the running
// process IS the autostart agent (the unload would self-SIGTERM the tray).
// ---------------------------------------------------------------------------

test("parseAgentSelfFromLaunchctl returns true when PID matches", () => {
const output = `{
\t"StandardOutPath" = "/tmp/omniroute.out.log";
\t"PID" = 4242;
\t"Label" = "com.omniroute.autostart";
}`;
assert.equal(parseAgentSelfFromLaunchctl(output, 4242), true);
});

test("parseAgentSelfFromLaunchctl returns false when PID differs", () => {
const output = `{
\t"PID" = 9999;
\t"Label" = "com.omniroute.autostart";
}`;
assert.equal(parseAgentSelfFromLaunchctl(output, 4242), false);
});

test("parseAgentSelfFromLaunchctl returns false when no PID present", () => {
// Loaded-but-not-running agent: launchctl omits the PID key.
const output = `{
\t"Label" = "com.omniroute.autostart";
\t"LastExitStatus" = 0;
}`;
assert.equal(parseAgentSelfFromLaunchctl(output, 4242), false);
});

test("parseAgentSelfFromLaunchctl tolerates whitespace variations", () => {
assert.equal(parseAgentSelfFromLaunchctl('"PID"=4242;', 4242), true);
assert.equal(parseAgentSelfFromLaunchctl('"PID" = 4242 ;', 4242), true);
});

test("parseAgentSelfFromLaunchctl returns false on empty/garbage", () => {
assert.equal(parseAgentSelfFromLaunchctl("", 4242), false);
assert.equal(parseAgentSelfFromLaunchctl("not launchctl output", 4242), false);
});

// ---------------------------------------------------------------------------
// isLaunchdAgentLoaded — true only when `launchctl list <label>` succeeds.
// This is what closes the false-"Enabled" gap: a plist on disk that launchd
// does not actually recognize must report NOT enabled.
// ---------------------------------------------------------------------------

test("isLaunchdAgentLoaded returns true when launchctl list succeeds", () => {
const ran = isLaunchdAgentLoaded(() => {
/* launchctl exited 0 — agent recognized */
});
assert.equal(ran, true);
});

test("isLaunchdAgentLoaded returns false when launchctl list throws", () => {
const ran = isLaunchdAgentLoaded(() => {
throw new Error("Could not find service");
});
assert.equal(ran, false);
});

// ---------------------------------------------------------------------------
// Source-level guard: isEnabledMac() must verify with launchctl, not just the
// plist file existence. Mirrors the repo's existing Linux source-assertion test
// (the darwin branch can't execute on the Linux CI runner).
// ---------------------------------------------------------------------------

test("isEnabledMac verifies launchd registration, not just plist existence", () => {
const source = readFileSync(join(process.cwd(), "bin/cli/tray/autostart.mjs"), "utf8");
// The verification helper is wired into the darwin enabled-check.
assert.match(source, /isLaunchdAgentLoaded/);
// launchctl list is queried with the app label, no shell interpolation.
assert.match(source, /launchctl",\s*\["list",\s*APP_LABEL\]/);
});

test("enable/disable macOS skip launchctl when the current process is the agent", () => {
const source = readFileSync(join(process.cwd(), "bin/cli/tray/autostart.mjs"), "utf8");
assert.match(source, /isAgentSelfMac/);
assert.match(source, /parseAgentSelfFromLaunchctl/);
});
Loading