🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

1. 🔵 Terminal/log injection via unsanitized port diagnostic errors in gateway restart health checks

Description

renderGatewayPortHealthDiagnostics() interpolates snapshot.portUsage.errors directly into user-visible output without sanitizing control characters/ANSI escapes.

• snapshot.portUsage.errors originates from inspectPortUsage() (which captures raw stderr/stdout and exception strings from system tools like lsof, ss, netstat, wmic) and also from catch (err) { errors: [String(err)] }.
• These error strings may contain newlines (\n/\r) and potentially terminal escape sequences (ANSI SGR, OSC-8 hyperlinks, etc.).
• When printed to the terminal (or emitted in JSON warnings), this can enable log forging / terminal escape injection (CWE-117), e.g. a local attacker controlling a process name/command line that appears in diagnostics, or influencing tool output, could inject misleading lines or terminal control sequences.

Vulnerable code:

if (snapshot.portUsage.errors?.length) {
  lines.push(`Port diagnostics errors: ${snapshot.portUsage.errors.join("; ")}`);
}

Recommendation

Sanitize untrusted diagnostic text before rendering to terminal/log output.

Use existing sanitizers (e.g. sanitizeTerminalText or sanitizeForLog) on each error string (and ideally also on listener commandLine/command fields when formatting port diagnostics).

Example fix:

import { sanitizeTerminalText } from "../../terminal/safe-text.js";

if (snapshot.portUsage.errors?.length) {
  const safeErrors = snapshot.portUsage.errors.map((e) => sanitizeTerminalText(String(e)));
  lines.push(`Port diagnostics errors: ${safeErrors.join("; ")}`);
}

If multi-line error details are valuable, consider splitting into multiple sanitized lines rather than embedding raw newlines into a single log line.

2. 🔵 Untrusted env/config can select port used to signal unmanaged gateway processes (local DoS)

Description

The new unmanaged stop/restart fallback can send signals to processes based on a port value derived from environment/config, which can be influenced by untrusted context.

In resolveGatewayLifecyclePort():

• The port is resolved from the service's ExecStart args (--port ...) or from resolveGatewayPort(config, mergedEnv).
• mergedEnv is built from process.env plus any service-defined environment.
• resolveGatewayPort() honors OPENCLAW_GATEWAY_PORT (and config gateway.port).

When the service manager is not loaded, runDaemonStop() / runDaemonRestart() will use that resolved port to find listener PIDs and then send signals (SIGTERM/SIGUSR1).

Because the CLI loads dotenv from the current working directory (see infra/dotenv.ts, dotenv.config() default), an attacker who can control the directory where a victim runs openclaw gateway stop/restart can drop a .env file setting e.g. OPENCLAW_GATEWAY_PORT and/or OPENCLAW_CONFIG_PATH (pointing to an attacker-controlled config) to steer which port is targeted. This enables a local denial-of-service against openclaw processes listening on an attacker-chosen port whenever the service is not loaded.

Vulnerable logic (port resolution feeding process signaling) is introduced by the new unmanaged stop/restart behavior.

Recommendation

Treat the unmanaged stop/restart path as a safety-critical operation and avoid resolving the target port from ambient/untrusted configuration.

Recommended changes:

1. Require an explicit port for unmanaged signaling, or default to the compiled-in default port only.
2. Do not honor OPENCLAW_GATEWAY_PORT / config overrides for unmanaged kill/restart unless the user explicitly opts in.
3. (Optional) Add a confirmation/warning when targeting a non-default port.

Example hardening approach:

import { DEFAULT_GATEWAY_PORT } from "../../config/paths.js";
import { parsePort } from "./shared.js"; // or wherever parsePort is exported

function resolveUnmanagedSignalPort(opts: { port?: string }): number {
  ​// Only trust an explicit CLI option (not dotenv/config/service env) for signaling.
  return parsePort(opts.port) ?? DEFAULT_GATEWAY_PORT;
}

Then use this value for stopGatewayWithoutServiceManager() / restartGatewayWithoutServiceManager() instead of resolveGatewayLifecyclePort().

If you need to preserve configurability, consider only trusting service-managed configuration (the installed unit’s --port) and ignoring cwd-loaded dotenv/config for these lifecycle actions.

3. 🔵 Unverified PID signaling when service manager not loaded (port-based process.kill)

Description

The new unmanaged gateway stop/restart paths signal processes based on PIDs discovered by looking up listeners on a TCP port, without strong verification that the target process is actually the intended OpenClaw gateway instance.

In src/cli/daemon-cli/lifecycle.ts:

• PIDs are derived from findGatewayPidsOnPortSync(port) and only filtered for pid > 0.
• stopGatewayWithoutServiceManager() then sends SIGTERM to every PID found.
• restartGatewayWithoutServiceManager() sends SIGUSR1 to the single PID found.

Security impact:

• Process identity is not verified (no check of executable path, full cmdline, uid, or a gateway-specific marker). Anything that ends up in findGatewayPidsOnPortSync() results can be signaled.
• TOCTOU / PID reuse risk: between port inspection and process.kill, the original process can exit and the PID can be reused, causing signals to be delivered to an unintended process.
• If the CLI is run with elevated privileges (e.g., via sudo), this becomes a stronger local denial-of-service primitive because it can signal other users’ processes.

Vulnerable code:

for (const pid of pids) {
  process.kill(pid, "SIGTERM");
}
...
process.kill(pids[0], "SIGUSR1");

Recommendation

Harden unmanaged stop/restart by validating process identity immediately before signaling.

Suggested defenses (can be combined):

1. Verify the listener is actually the gateway using stronger attributes:

   • command line contains an expected marker (e.g., openclaw-gateway), and/or
   • executable path matches the expected installation path, and/or
   • the listener uid matches the current uid (unless explicitly allowed).

2. Re-check that the PID is still the listener on that port right before signaling (mitigates PID reuse).

3. Handle ESRCH/EPERM safely: treat ESRCH as already-stopped; report EPERM with a clear message.

Example approach (pseudo-code):

import { inspectPortUsage } from "../../infra/ports-inspect.js";

async function safeSignalGatewayByPort(port: number, signal: NodeJS.Signals) {
  const usage = await inspectPortUsage(port);
  const candidates = usage.listeners
    .filter(l => typeof l.pid === "number")
    .filter(l => (l.commandLine ?? l.command ?? "").toLowerCase().includes("openclaw-gateway"));

  ​// optionally also enforce same-user:
  ​// .filter(l => l.user === os.userInfo().username)

  if (candidates.length !== 1) throw new Error("expected exactly one gateway listener");

  const pid = candidates[0].pid!;
  ​// Re-verify PID still listening (repeat inspectPortUsage or /proc check) before kill
  process.kill(pid, signal);
}

Also consider requiring an explicit --force flag before killing unmanaged processes, and/or printing details (pid + cmdline + user) for interactive confirmation when not using --json.

Analyzed PR: #39355 at commit 54e0861

_{Last updated on: 2026-03-08T02:27:32Z}

Latest run failed. Keeping previous successful results. Trace ID: 019ccb38d4c2db0bc2a616402606c682.

_{Last updated on: 2026-03-08T02:28:15Z}

Greptile Summary

This PR adds an unmanaged-listener fallback to openclaw gateway stop and openclaw gateway restart so that container/supervisor deployments no longer receive a service disabled no-op when no service-manager unit is registered.

The implementation is clean and well-scoped:

• The onNotLoaded hook is opt-in and wrapped in try/catch at call sites, ensuring exceptions from process.kill() surface as structured error messages
• PID deduplication via Set before signalling is a good defensive measure
• The managed paths (launchd/systemd/schtasks) remain completely untouched
• Restart properly fails with a diagnostic when multiple listeners are found, avoiding guessing
• Test coverage covers the key new behaviors: signal delivery, JSON response handling, and post-restart health checks

Confidence Score: 4/5

• Safe to merge. Changes are narrowly scoped to the unmanaged-listener fallback path, with no modifications to managed-service (launchd/systemd/schtasks) code paths.
• The PR is well-designed with clean separation of concerns: unmanaged paths are opt-in via the onNotLoaded callback, proper error handling via try/catch, and full preservation of existing health checks and validation. The implementation defensively handles edge cases (PID dedup, explicit failure on multi-listener restarts). Test coverage is solid for the new behaviors. The core concern around logical consistency in post-restart health checks is mitigated by platform-specific guards (includeUnknownListenersAsStale is Windows-only) and the acknowledged low practical risk. Score is 4/5 rather than 5/5 purely due to the one untested code path (stale-PID recovery in unmanaged restart on Windows), though this path is unlikely to execute in practice given Windows limitations with SIGUSR1.
• No files require special attention. All changes follow established patterns and preserve backward compatibility.

_{Last reviewed commit: 042d212}

Addressed the unmanaged restart post-check issue.

What changed:

• unmanaged onNotLoaded restart no longer flows into the service-managed waitForGatewayHealthyRestart() path
• added a separate listener/probe-only wait path in src/cli/daemon-cli/restart-health.ts
• kept stale-PID cleanup and service.restart() retry logic scoped to service-managed restarts only
• added regression coverage proving unmanaged restart uses the new path and does not call the service-managed health/retry flow

Verification:

• pnpm vitest src/cli/daemon-cli/lifecycle.test.ts src/cli/daemon-cli/lifecycle-core.test.ts
• pnpm check

Validated both follow-up findings on the current branch state.

1. Undefined loadConfig() fallback: valid

• the unmanaged stop/restart catch path had regressed to an out-of-scope loadConfig() reference after the rebase
• fixed by routing those fallbacks back through readBestEffortConfig() and an env-only final fallback

2. Port-derived PID signaling too loose: valid enough to harden

• the fallback was still trusting findGatewayPidsOnPortSync(port) too much before signaling
• tightened it by re-reading live process argv before every unmanaged SIGTERM/SIGUSR1 and only signaling processes that still look like an OpenClaw gateway process
• added regression coverage that non-gateway argv on the same candidate PID is skipped instead of being signaled

Verification:

• pnpm vitest src/cli/daemon-cli/lifecycle.test.ts src/cli/daemon-cli/lifecycle-core.test.ts
• pnpm build
• pnpm check

Follow-up on the latest review batch:

• The two low findings against commit 042d212 are stale on current PR head; both were already fixed earlier on this branch.
• I also addressed the unmanaged restart false-positive case: when the running gateway reports commands.restart=false, the CLI now fails before sending unmanaged SIGUSR1 instead of treating a still-healthy port as proof of restart.

Verification:

• pnpm vitest src/cli/daemon-cli/lifecycle.test.ts src/cli/daemon-cli/lifecycle-core.test.ts
• pnpm build
• pnpm check

Landed in main: bf9c362