🔒 Aisle Security Analysis

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

1. 🟡 Symlink/TOCTOU arbitrary file overwrite & chmod via models.json persistence path

Description

The ensureOpenClawModelsJson() persistence hardening introduces/retains unsafe filesystem operations on a path that can be influenced via configuration/environment (agentDirOverride parameter and OPENCLAW_AGENT_DIR / PI_CODING_AGENT_DIR).

If an attacker can influence agentDir (or can create/replace files inside agentDir, e.g., if it is placed in a writable directory), they can exploit symlink following to affect arbitrary files:

• targetPath is computed as path.join(agentDir, "models.json") with no restriction that agentDir is within a trusted state directory.
• fs.writeFile(targetPath, ...) follows symlinks, so a pre-created models.json -> /path/to/victim symlink can cause overwriting arbitrary files.
• Newly added ensureModelsFileMode() calls fs.chmod(targetPath, 0o600) even on the no-content-change path, which can change permissions of the symlink target as well (best-effort, errors ignored), enabling targeted DoS or permission clobbering.
• No lstat/realpath checks, no O_NOFOLLOW/exclusive create, and no atomic-temp-write + rename pattern are used.

Vulnerable code:

const agentDir = agentDirOverride?.trim() ? agentDirOverride.trim() : resolveOpenClawAgentDir();
const targetPath = path.join(agentDir, "models.json");
...
await fs.writeFile(targetPath, next, { mode: 0o600 });
await fs.chmod(targetPath, 0o600).catch(() => {});

Impact depends on deployment, but in any scenario where untrusted input can affect agentDir or untrusted users can write within that directory, this becomes a classic symlink/hardlink attack surface (CWE-59).

Recommendation

Harden models.json writes against symlink/hardlink attacks and restrict the writable location.

1. Restrict/validate agentDir

• Prefer not to accept arbitrary agentDirOverride from any untrusted context.
• Enforce that agentDir resolves under a trusted base directory (e.g., resolveStateDir()/agents/...).

2. Refuse symlinks and enforce regular file semantics

• Before chmod/write, lstat() the path and reject if it is a symlink.
• Also consider verifying the parent directory is not symlinked (realpath on parent) and has safe ownership/permissions.

3. Use atomic write with a temp file + rename

• Write to models.json.tmp.<pid> with secure flags/mode, fsync, then rename.

Example (best-effort illustration):

import { constants as FS } from "node:fs";

async function safeWriteModelsJson(targetPath: string, content: string) {
  ​// Refuse symlink targets
  try {
    const st = await fs.lstat(targetPath);
    if (st.isSymbolicLink()) {
      throw new Error("Refusing to write models.json via symlink");
    }
  } catch {
    ​// ok if missing
  }

  const dir = path.dirname(targetPath);
  await fs.mkdir(dir, { recursive: true, mode: 0o700 });

  const tmpPath = `${targetPath}.tmp.${process.pid}`;
  const fh = await fs.open(tmpPath, FS.O_CREAT | FS.O_TRUNC | FS.O_WRONLY, 0o600);
  try {
    await fh.writeFile(content, "utf8");
    await fh.sync();
  } finally {
    await fh.close();
  }
  await fs.rename(tmpPath, targetPath);
}

4. Avoid chmod-on-unchanged-content for untrusted paths

• Only repair mode after verifying the file is a regular file under a trusted directory; otherwise skip.

Analyzed PR: #38955 at commit 056a7a2

_{Last updated on: 2026-03-07T18:02:41Z}

Greptile Summary

This PR delivers a comprehensive end-to-end fix for the SecretRef plaintext-persistence cluster. The implementation correctly:

• Reorders normalize-before-merge in ensureOpenClawModelsJson, ensuring secretRefManagedProviders is populated before merge logic decides whether to preserve stale keys
• Wires source config snapshots through both the agent and models command paths for proper provenance tracking
• Replaces the previous broad env-var-shape marker heuristic with a bounded-set marker registry (explicit markers + known env names), verified by regression tests confirming uppercase real keys are treated as credentials
• Guards request paths against marker values being emitted as real credentials via sanitizeModelHeaders({ stripSecretRefMarkers: true }) in the registry read path
• Extends secrets audit/apply coverage to models.json API keys and sensitive provider headers
• Hardens models.json writes with deterministic serialization, 0600 file mode enforcement, and per-path write locking with finally block release guarantee

The security-critical paths (normalize-before-merge ordering, SecretRef-managed key preservation, marker guards, file mode enforcement, write locking) are correctly implemented with comprehensive test coverage. The code is safe to merge.

Confidence Score: 5/5

• This PR is safe to merge. It comprehensively closes the plaintext-persistence security surface with correct implementation of all hardening measures and no critical logic errors detected.
• The security-critical paths are correctly implemented: normalize-before-merge ordering ensures SecretRef-managed provider key preservation works correctly, marker detection uses a bounded known-env registry (not broad regex patterns), request paths are guarded against marker leakage, file mode enforcement is deterministic, and write locking with try/finally ensures atomicity. Comprehensive test coverage validates all these hardening measures. No functional or security issues found in code review.
• No files require special attention.

_{Last reviewed commit: 146e82f}

@greptile-apps Review

@mdlmarkham Hi! Maintainer here. Please stop. Do not release a code review bot on this project. It is not needed nor helpful.

@greptile-apps Review

@codex Review

@greptile-apps Review

@codex Review