Description

The new hasSessionWriteLockTimeout() helper recursively traverses error, cause, and reason properties of arbitrary error-like objects without a maximum depth limit.

• Input err is unknown and can be any object (including provider/SDK error objects that may embed nested objects derived from remote responses)
• The function performs recursive descent on three properties per node (error, cause, reason)
• There is cycle detection (seen), but no depth/step limit, so an attacker (or untrusted upstream) that can influence the shape of an error object could create a very deep chain and trigger:
  • stack exhaustion (RangeError: Maximum call stack size exceeded)
  • excessive CPU time from large traversals

This helper is called from hasTimeoutHint(), isTimeoutError(), and resolveFailoverClassificationFromErrorInternal(), which are likely executed on error paths that can be triggered repeatedly, amplifying the DoS impact.

Vulnerable code:

return (
  hasSessionWriteLockTimeout(candidate.error, seen) ||
  hasSessionWriteLockTimeout(candidate.cause, seen) ||
  hasSessionWriteLockTimeout(candidate.reason, seen)
);

Recommendation

Add a maximum traversal depth / node budget similar to MAX_FAILOVER_CAUSE_DEPTH, and/or implement an iterative traversal to avoid stack exhaustion.

Example fix (depth-limited + iterative):

const MAX_SESSION_LOCK_CAUSE_DEPTH = 25;

function hasSessionWriteLockTimeout(err: unknown): boolean {
  const seen = new Set<object>();
  const stack: Array<{ value: unknown; depth: number }> = [{ value: err, depth: 0 }];

  while (stack.length) {
    const { value, depth } = stack.pop()!;
    if (isSessionWriteLockTimeoutError(value)) return true;
    if (!value || typeof value !== "object") continue;
    if (seen.has(value)) continue;
    if (depth >= MAX_SESSION_LOCK_CAUSE_DEPTH) continue;
    seen.add(value);

    const c = value as { error?: unknown; cause?: unknown; reason?: unknown };
    if (c.error !== undefined) stack.push({ value: c.error, depth: depth + 1 });
    if (c.cause !== undefined) stack.push({ value: c.cause, depth: depth + 1 });
    if (c.reason !== undefined) stack.push({ value: c.reason, depth: depth + 1 });
  }
  return false;
}

This prevents stack overflow and bounds worst-case work even with attacker-controlled nesting.

Description

isSessionWriteLockTimeoutError() trusts a magic string error code and returns true for any object with a matching code, not just real SessionWriteLockTimeoutError instances.

This is security-relevant because failover-error.ts uses hasSessionWriteLockTimeout() to suppress timeout detection and failover classification:

• hasTimeoutHint() / isTimeoutError() return false if a session-write-lock timeout is detected
• resolveFailoverClassificationFromErrorInternal() returns null (no failover) when hasSessionLock is true and there is no “explicit failover metadata”

Since failover classification consumes generic error objects and extracts .code recursively (getErrorCode() uses findErrorProperty() over error/cause), an error originating from outside this module (e.g., provider/network error payloads that include a code field) can be made to look like a session-write-lock timeout simply by setting code to OPENCLAW_SESSION_WRITE_LOCK_TIMEOUT. This can prevent retries/failover and degrade availability (routing manipulation / DoS on fallback).

Vulnerable code:

export function isSessionWriteLockTimeoutError(err: unknown): boolean {
  return (
    err instanceof SessionWriteLockTimeoutError ||
    Boolean(
      err &&
      typeof err === "object" &&
      (err as { code?: unknown }).code === SESSION_WRITE_LOCK_TIMEOUT_CODE,
    )
  );
}

Recommendation

Do not treat untrusted/plain objects as SessionWriteLockTimeoutError based solely on a magic string.

Options:

1. Strictest (preferred): only accept real instances (or ones that can be validated strongly):

export function isSessionWriteLockTimeoutError(err: unknown): err is SessionWriteLockTimeoutError {
  return err instanceof SessionWriteLockTimeoutError;
}

2. If cross-boundary serialization is required, add strong branding and validate multiple fields:

• check name === "SessionWriteLockTimeoutError"
• require numeric timeoutMs, string owner, string lockPath
• optionally include a non-guessable brand (e.g., a Symbol.for(...) property set only locally) or wrap/translate external errors into a dedicated internal error type rather than trusting .code.

Also consider scoping the suppression logic in failover-error.ts so only known-local lock-timeout errors (created by your locking code) suppress failover/timeout classification, rather than any nested error object containing a matching code.

Description

SessionWriteLockTimeoutError embeds the absolute lockPath and owner (PID) directly in the exception message. This message is later propagated through generic error formatting (e.g., formatErrorMessage()), which is used for diagnostics/telemetry and potentially other user-visible surfaces.

Impact:

• Information disclosure: reveals internal filesystem layout (absolute session/lock path) and process IDs.
• Log forging / injection: POSIX filenames can contain newlines/control characters; because lockPath is derived from sessionFile (via path.basename(sessionFile)), an attacker who can influence sessionFile can inject newlines into logs/telemetry entries that include err.message.

Vulnerable code:

super(
  `session file locked (timeout ${params.timeoutMs}ms): ${params.owner} ${params.lockPath}`,
);

Recommendation

Avoid putting sensitive/internal identifiers (absolute paths, PIDs) into the human-readable error message. Keep them as structured fields for debugging, and sanitize/escape before logging.

Suggested change:

export class SessionWriteLockTimeoutError extends Error {
  readonly code = SESSION_WRITE_LOCK_TIMEOUT_CODE;
  readonly timeoutMs: number;
  readonly owner: string;
  readonly lockPath: string;

  constructor(params: { timeoutMs: number; owner: string; lockPath: string }) {
    super(`session file locked (timeout ${params.timeoutMs}ms)`);
    this.name = "SessionWriteLockTimeoutError";
    this.timeoutMs = params.timeoutMs;
    this.owner = params.owner;
    this.lockPath = params.lockPath;
  }
}

When logging, log structured metadata and escape control characters:

const safePath = lockPath.replace(/[\r\n\t]/g, " ");
log.warn({ code, owner, lockPath: safePath }, "session file locked");

If any of these errors can reach HTTP/UI responses, map them to a generic user-safe message and omit internal paths/PIDs.