Description

The gateway shutdown/restart loop now allows restart drains to wait indefinitely when gateway.reload.deferralTimeoutMs is absent or non-positive.

• resolveRestartDrainTimeoutMs() returns undefined unless a positive timeout is explicitly configured.
• When the timeout is undefined, the code calls waitForActiveTasks(undefined) and waitForActiveEmbeddedRuns(undefined), both of which can wait indefinitely for in-flight work captured at call time.
• The forceExitTimer watchdog is not armed during the drain phase for indefinite restarts (it is only armed later, right before server.close()), so a stuck/long-running task or embedded run can prevent the restart forever.

This creates an availability risk where an external actor who can cause long-running/stuck tasks (e.g., via requests that trigger work on the command queue or embedded runs) can indefinitely delay restarts/reloads, potentially blocking security updates or recovery actions.

Vulnerable code (restart drain with undefined timeout, no watchdog during drain):

const restartDrainTimeoutMs = isRestart ? resolveRestartDrainTimeoutMs() : 0;
...
if (activeTasks > 0 || activeRuns > 0) {
  const [tasksDrain, runsDrain] = await Promise.all([
    activeTasks > 0 ? waitForActiveTasks(restartDrainTimeoutMs) : Promise.resolve({ drained: true }),
    activeRuns > 0 ? waitForActiveEmbeddedRuns(restartDrainTimeoutMs) : Promise.resolve({ drained: true }),
  ]);
}
...
​// For indefinite restart, watchdog is only armed *after* draining
armCloseForceExitTimerForIndefiniteRestart();
await server?.close(...);

Recommendation

Reintroduce a bounded default for restart draining and/or ensure a watchdog is armed before entering the drain.

Options:

1. Keep the previous safe default (e.g., 5 minutes) when config is absent:

const timeoutMs = loadConfig().gateway?.reload?.deferralTimeoutMs;
const restartDrainTimeoutMs =
  typeof timeoutMs === "number" && Number.isFinite(timeoutMs) && timeoutMs >= 0
    ? timeoutMs
    : DEFAULT_RESTART_DRAIN_TIMEOUT_MS;

2. If supporting indefinite drain is desired, still arm a hard upper bound watchdog during the drain (distinct from the server.close watchdog), so a stuck task cannot block restarts forever:

​// Always arm a force-exit watchdog for restart, even if drain timeout is undefined.
armForceExitTimer(SUPERVISOR_STOP_TIMEOUT_MS - 5_000);

Also consider making drain behavior resilient to untrusted/remote-triggerable work by:

• restricting which tasks/runs can block restart
• adding cancellation/abort mechanisms for tasks
• emitting restart anyway after a maximum elapsed time

Description

deferGatewayRestartUntilIdle() changed from a bounded wait (previously default 5 minutes) to an indefinite wait unless a positive maxWaitMs is explicitly provided.

• maxWaitMs is set to undefined when opts.maxWaitMs is absent or non-positive.
• The polling loop only triggers onTimeout and emits the restart signal when maxWaitMs is defined.
• As a result, if pending work never drains (e.g., stuck tasks, hung embedded runs, or a workload that keeps pending > 0), the restart signal may never be emitted.

This can be abused (or triggered accidentally) to indefinitely block restarts initiated via the restart coordinator (e.g., config watcher or RPC path), impacting availability and the ability to apply updates.

Vulnerable code:

const maxWaitMs =
  typeof opts.maxWaitMs === "number" && Number.isFinite(opts.maxWaitMs) && opts.maxWaitMs > 0
    ? Math.max(pollMs, Math.floor(opts.maxWaitMs))
    : undefined;
...
if (maxWaitMs !== undefined && elapsedMs >= maxWaitMs) {
  ...
  void emitPreparedGatewayRestart(opts.emitHooks);
}

Recommendation

Maintain a finite default maximum wait for restart deferral, even when maxWaitMs is not provided, to ensure the system eventually makes progress.

For example, restore the previous default (5 minutes) unless explicitly configured:

const DEFAULT_DEFERRAL_MAX_WAIT_MS = 300_000;
const raw = opts.maxWaitMs;
const maxWaitMs =
  typeof raw === "number" && Number.isFinite(raw) && raw >= 0
    ? Math.max(pollMs, Math.floor(raw))
    : DEFAULT_DEFERRAL_MAX_WAIT_MS;

If supporting indefinite deferral is necessary, add a separate hard-stop watchdog and/or an operator override (force restart) to prevent untrusted workloads from blocking restarts indefinitely.

Description

The gateway restart/stop decision on SIGTERM is now influenced by a local file (gateway-restart-intent.json) stored under the state directory, which is resolved from OPENCLAW_STATE_DIR.

Because resolveStateDir(env) trusts env.OPENCLAW_STATE_DIR and consumeGatewayRestartIntentSync() does not validate file/directory ownership or permissions, an attacker who can write to the resolved state directory can:

• Force behavior changes/DoS: make a SIGTERM be treated as a "restart" (draining) rather than a "stop" by pre-placing a valid intent file.
• Persistently influence behavior: clearGatewayRestartIntentSync() refuses to unlink files with nlink > 1, so a hard-linked intent file cannot be removed by the consumer and can continue to affect subsequent SIGTERM handling.
• In environments where the gateway/CLI is launched with elevated privileges and attacker-controlled environment (e.g., sudo -E, misconfigured service env), OPENCLAW_STATE_DIR could redirect writes to unexpected locations.

Vulnerable code (intent path derived from env; no ownership/permission checks; hardlink prevents clearing):

function resolveGatewayRestartIntentPath(env: NodeJS.ProcessEnv = process.env): string {
  return path.join(resolveStateDir(env), GATEWAY_RESTART_INTENT_FILENAME);
}

const stat = fs.lstatSync(intentPath);
if (!stat.isFile() || stat.size > GATEWAY_RESTART_INTENT_MAX_BYTES) {
  return false;
}
raw = fs.readFileSync(intentPath, "utf8");
...
clearGatewayRestartIntentSync(env);

const stat = fs.lstatSync(intentPath);
if (!stat.isFile() || stat.nlink > 1) {
  return false;
}
fs.unlinkSync(intentPath);

This is primarily a local integrity issue whose impact depends on how OPENCLAW_STATE_DIR is configured and whether the state dir is writable by other users (or the process runs with elevated privileges under an attacker-influenced environment).

Recommendation

Harden the trust boundary around the restart-intent control file.

1. Constrain/validate the state directory when running in privileged contexts (or always):

• Refuse to honor OPENCLAW_STATE_DIR when process.getuid?.() === 0 (or when running as a system service), or only allow it to point under an expected base directory.
• Ensure the state directory is not group/world-writable and is owned by the current uid.

2. Validate file ownership & mode before trusting content:

• Require stat.uid === process.getuid() (when available)
• Require (stat.mode & 0o077) === 0 (no group/world access)

3. Make clearing robust:

• Consider unlinking regardless of nlink (or at least treat nlink>1 as suspicious and ignore without using it).

Example hardening (POSIX-only checks guarded for portability):

const stat = fs.lstatSync(intentPath);
if (!stat.isFile() || stat.size > MAX_BYTES) return false;
if (typeof process.getuid === 'function') {
  if (stat.uid !== process.getuid()) return false;
  if ((stat.mode & 0o077) !== 0) return false;
}

Also consider opening/reading using openSync and (where supported) O_NOFOLLOW to reduce filesystem trickery risk.

1. src/cli/daemon-cli/lifecycle-core.ts, line 172-183 (link)

   wroteRestartIntent flag set before the actual write

   wroteRestartIntent is set to true based on params.serviceNoun === "Gateway" before writeGatewayRestartIntentSync() is called. Because writeGatewayRestartIntentSync silently swallows its own errors (it logs a warning internally but never throws), if the write fails the flag is still true. On a subsequent service.restart failure, clearGatewayRestartIntentSync is called to clean up a file that was never written — a no-op in practice but potentially misleading. Consider capturing the return value of writeGatewayRestartIntentSync or renaming to shouldWriteRestartIntent to reflect that it's a precondition, not a past-tense record of success.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/cli/daemon-cli/lifecycle-core.ts
   Line: 172-183
   
   Comment:
   **`wroteRestartIntent` flag set before the actual write**
   
   `wroteRestartIntent` is set to `true` based on `params.serviceNoun === "Gateway"` before `writeGatewayRestartIntentSync()` is called. Because `writeGatewayRestartIntentSync` silently swallows its own errors (it logs a warning internally but never throws), if the write fails the flag is still `true`. On a subsequent `service.restart` failure, `clearGatewayRestartIntentSync` is called to clean up a file that was never written — a no-op in practice but potentially misleading. Consider capturing the return value of `writeGatewayRestartIntentSync` or renaming to `shouldWriteRestartIntent` to reflect that it's a precondition, not a past-tense record of success.
   
   How can I resolve this? If you propose a fix, please make it concise.