🔒 Aisle Security Analysis

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

1. 🟡 Secrets audit may read models.json outside stateDir via configurable agentDir (unrestricted file access + potential DoS)

Description

runSecretsAudit() now scans models.json files discovered by listAgentModelsJsonPaths(). For agent IDs coming from config, it uses resolveAgentDir(config, agentId) which accepts agents.list[].agentDir from config and resolves it to an absolute path (via resolveUserPath) without constraining it to the OpenClaw stateDir.

As a result:

• Unintended file access: If a config sets agentDir to an arbitrary location, the audit will attempt to read <agentDir>/models.json even if it is outside the intended state directory.
  • This can be abused when an attacker can influence the config and filesystem (e.g., place a symlink at <agentDir>/models.json pointing to another file). The audit will follow the symlink and read the target.
• Denial-of-service risk: readJsonObjectIfExists() uses fs.readFileSync() and JSON.parse() with no file-size/type checks. A very large file or special file at that path can cause high memory/CPU usage or blocking reads.
• Information disclosure in reports: Findings include the full file path for the scanned file, which can leak sensitive host path information in audit output/logs.

Vulnerable code (path construction from config-controlled agentDir):

const agentDir = resolveAgentDir(config, agentId);
paths.add(path.join(resolveUserPath(agentDir), "models.json"));

Sinks:

const raw = fs.readFileSync(filePath, "utf8");
const parsed = JSON.parse(raw);

Recommendation

Harden models.json discovery and reading:

1. Constrain scans to the intended state directory (recommended for an audit tool).
   • Ignore agents.list[].agentDir overrides when scanning for models.json, and instead derive paths strictly from the provided stateDir:

const stateRoot = resolveUserPath(stateDir);
for (const agentId of listAgentIds(config)) {
  const id = normalizeAgentId(agentId);
  paths.add(path.join(stateRoot, "agents", id, "agent", "models.json"));
}

2. If agentDir overrides must be supported, enforce an allowlist root and resolve symlinks:

const allowedRoot = path.join(resolveUserPath(stateDir), "agents") + path.sep;
const candidate = fs.realpathSync.native(path.join(resolveUserPath(agentDir), "models.json"));
if (!candidate.startsWith(allowedRoot)) return; // skip

3. Prevent DoS / unsafe file types before reading:
   • lstatSync/statSync to ensure it is a regular file
   • reject symlinks (or require realpath checks)
   • cap file size (e.g., 1–5 MB) before readFileSync

const st = fs.statSync(candidate);
if (!st.isFile() || st.size > 5 * 1024 * 1024) return;

4. Consider redacting or shortening scanned paths in user-facing audit output (or gate full paths behind a --verbose flag).

2. 🟡 Symlink/Hardlink following allows arbitrary file overwrite/chmod when writing models.json

Description

The ensureOpenClawModelsJson() routine writes models.json and enforces permissions using path-based filesystem operations (fs.writeFile and fs.chmod) without defending against symlink/hardlink attacks.

Key points:

• Attacker-controlled path: the target directory can be influenced via:
  • OPENCLAW_AGENT_DIR / PI_CODING_AGENT_DIR (see resolveOpenClawAgentDir()), and
  • direct agentDirOverride passed by various callers.
• Symlink following: Node’s fs.writeFile(path, ...) and fs.chmod(path, ...) follow symlinks by default.
• TOCTOU window: the code does readRawFile(targetPath) and later writeFile(targetPath, ...) / chmod(targetPath, ...). An attacker who can write inside agentDir can replace models.json with a symlink (or a hardlink where allowed) between these operations.
• Impact: if OpenClaw runs with elevated privileges (or is pointed at a directory writable by an untrusted user), this can be used to:
  • overwrite arbitrary files with JSON content, and/or
  • change permissions of arbitrary files to 0600 (including on the “no content change” path).

Vulnerable code:

await fs.writeFile(targetPath, next, { mode: 0o600 });
await ensureModelsFileMode(targetPath);

​// ...
async function ensureModelsFileMode(pathname: string): Promise<void> {
  await fs.chmod(pathname, 0o600).catch(() => {
    ​// best-effort
  });
}

Recommendation

Harden file writes/permission changes against symlink/hardlink attacks and make the write atomic.

Recommended pattern:

1. Ensure agentDir exists with correct permissions.
2. Write to a random temp file in the same directory, set mode on the temp file (via fchmod if possible), then rename() it over models.json.
3. For the “no content change” path, avoid chmod(path). Instead open the file with O_NOFOLLOW and use fchmod() on the returned FD/handle; also reject non-regular files and (optionally) nlink > 1.

Example (POSIX-oriented):

import { constants as c } from "node:fs";
import { randomUUID } from "node:crypto";

await fs.mkdir(agentDir, { recursive: true, mode: 0o700 });

const tmpPath = path.join(agentDir, `.models.json.${process.pid}.${randomUUID()}.tmp`);
const mode = 0o600;

​// Write temp with O_EXCL + (when supported) O_NOFOLLOW
const flags = c.O_WRONLY | c.O_CREAT | c.O_EXCL | (c.O_NOFOLLOW ?? 0);
const handle = await fs.open(tmpPath, flags, mode);
try {
  await handle.writeFile(next, "utf8");
  await handle.chmod(mode).catch(() => {});
} finally {
  await handle.close();
}

​// rename replaces an existing symlink *itself* rather than following it
await fs.rename(tmpPath, targetPath);

Alternatively, reuse existing repository helpers such as writeTextAtomic() / writeJsonAtomic() (from src/infra/json-files.ts) and add a safe-mode repair path that uses open(..., O_NOFOLLOW) + fchmod rather than chmod(path).

Also consider explicitly refusing to operate when lstat(targetPath).isSymbolicLink() or stat.nlink > 1 (hardlink) is detected, and avoid best-effort chmod on paths that cannot be verified safely.

3. 🔵 Secrets audit may miss plaintext secrets in models.json due to overly broad env-var marker detection

Description

secrets audit now scans generated agents/*/agent/models.json for plaintext provider credentials, but it suppresses findings when values look like markers.

The new marker logic treats any ALL-CAPS/underscore/digit string (/^[A-Z][A-Z0-9_]*$/) as an env-var marker by default. In collectModelsJsonSecrets(), this causes a false negative when a real API key/token happens to match that pattern (e.g., base32/uppercase secrets, or other providers’ uppercase credential formats):

• Input: providers.<id>.apiKey read from models.json (untrusted file content at rest)
• Decision point: isNonSecretApiKeyMarker(apiKey) returns true for env-var-shaped strings
• Effect: audit skips PLAINTEXT_FOUND, leaving a plaintext secret at rest undetected

Additionally, header values are skipped when they start with secretref-env: without validating that the suffix is a legitimate env-var name, which can also suppress findings for malformed/abused values.

Vulnerable logic:

} else if (isNonEmptyString(apiKey) && !isNonSecretApiKeyMarker(apiKey)) {
  addFinding(... "PLAINTEXT_FOUND" ...)
}
...
if (isSecretRefHeaderValueMarker(headerValue)) {
  continue;
}

Recommendation

Make audit’s suppression rules strict and evidence-based, so it avoids intended markers without creating blind spots:

1. Do not treat arbitrary env-var-shaped strings as non-secret markers during audit. Instead, only suppress when the value is:

   • A known fixed marker token (e.g. minimax-oauth, qwen-oauth, ollama-local, secretref-managed, AWS SDK marker names), or
   • An env var name that is known to be used as a SecretRef (derived from config/auth profile refs), or optionally present in .env.

2. Validate secretref-env: header markers by ensuring the suffix is a valid env var name (e.g. isValidEnvSecretRefId()), otherwise treat it as plaintext.

Example hardening (illustrative):

import { isValidEnvSecretRefId } from "../config/types.secrets.js";
import { isEnvVarNameMarker, isAwsSdkAuthMarker, NON_ENV_SECRETREF_MARKER } from "../agents/model-auth-markers.js";

function isAuditSafeMarkerApiKey(value: string, knownEnvRefIds: Set<string>): boolean {
  const v = value.trim();
  if (!v) return false;
  ​// Only fixed tokens / known markers
  if (v === NON_ENV_SECRETREF_MARKER || isAwsSdkAuthMarker(v) || v === "minimax-oauth" || v === "qwen-oauth" || v === "ollama-local") {
    return true;
  }
  ​// Only treat env var name as marker if it is actually referenced
  return isEnvVarNameMarker(v) && knownEnvRefIds.has(v);
}

function isAuditSafeSecretRefEnvHeaderMarker(value: string): boolean {
  const v = value.trim();
  if (!v.startsWith("secretref-env:")) return false;
  const suffix = v.slice("secretref-env:".length);
  return isValidEnvSecretRefId(suffix);
}

If you want to preserve the current UX (storing env var names in models.json), consider adding a disambiguating prefix for env-var markers (e.g. $OPENAI_API_KEY), which removes collision with uppercase secrets.

Analyzed PR: #38470 at commit 4e0dfa1

_{Last updated on: 2026-03-07T03:11:52Z}

Greptile Summary

This PR systematically hardens the SecretRef-to-models.json persistence pipeline across 53 files: non-env SecretRef values are now replaced with non-secret marker strings before being written to disk, env-SecretRef providers store an env-var-name reference instead, and the secrets audit command gains a new scan of generated models.json files. Supporting changes include per-path write locking for models.json, a source-config snapshot plumbing path (so the resolved runtime config is never used as the persistence input), and header-level SecretRef support in collectors and runtime sanitization.

Key areas of the change:

• src/agents/model-auth-markers.ts – new module centralising all non-secret marker constants and predicates used across persistence, merge, runtime, and audit paths.
• src/agents/models-config.ts – write path overhauled: normalize-before-merge ordering, per-path mutex (withModelsJsonWriteLock), 0600 file-mode hardening on both write and no-change paths, and source-snapshot routing via resolveModelsConfigInput.
• src/agents/models-config.providers.ts – resolveApiKeyFromProfiles now returns a typed provenance record (ProfileApiKeyResolution) enabling downstream callers to distinguish plaintext from env/non-env SecretRef sources; header SecretRef normalization added.
• src/secrets/audit.ts – collectModelsJsonSecrets added, covering unresolved SecretRef objects and plaintext residues in provider apiKey and sensitive header fields.
• One logic gap found: in mergeWithExistingProviderSecrets (models-config.ts ~line 168), the guard !secretRefManagedProviders.has(key) correctly skips preservation for providers that are currently SecretRef-managed, but does not prevent a stale NON_ENV_SECRETREF_MARKER string already present in the file from being preserved when a provider transitions from SecretRef-managed to plaintext. The stale marker would override the new plaintext value and cause runtime auth failures. See the inline comment for a suggested fix using isNonSecretApiKeyMarker.

Confidence Score: 3/5

• Safe to merge for new SecretRef configurations, but the SecretRef-to-plaintext transition edge case can cause runtime auth failures in merge mode until fixed.
• The broad set of changes is well-structured and the core SecretRef-hardening goals are met for all new and steady-state configurations. However, the merge logic in mergeWithExistingProviderSecrets does not guard against preserving a stale non-env-ref marker from an existing models.json when the user switches a provider back to plaintext credentials. Any user who previously had a non-env SecretRef provider and now removes the SecretRef from their config would silently receive broken API credentials on the next models.json regeneration in merge mode. The issue is localised to one predicate check and would not affect plain-plaintext or brand-new SecretRef deployments.
• src/agents/models-config.ts — specifically the mergeWithExistingProviderSecrets function around the preserved.apiKey assignment.

_{Last reviewed commit: 4e0dfa1}

Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch.

Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch.