🔒 Aisle Security Analysis

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

1. 🟡 Blocking synchronous filesystem discovery on gateway request path enables DoS via resource exhaustion

Description

loadCombinedSessionStoreForGateway() now performs synchronous, unbounded session-store discovery across all agent roots on every call.

This becomes a denial-of-service risk because:

• Multiple gateway endpoints call loadCombinedSessionStoreForGateway() in request handlers (e.g., sessions.list, sessions.resolve, sessions.usage).
• The function calls resolveAllAgentSessionStoreTargetsSync(cfg), which does synchronous readdirSync + lstatSync + realpathSync work across agent directories and then loads each discovered sessions.json.
• An attacker who can trigger these endpoints repeatedly (even if authenticated) can force repeated event-loop blocking work.
• A local attacker (or any actor able to write to the state directory) can amplify cost by creating thousands of agent subdirectories under an agents root, causing the discovery loop to scale linearly with directory count.

Vulnerable code:

const targets = resolveAllAgentSessionStoreTargetsSync(cfg);
for (const target of targets) {
  const store = loadSessionStore(target.storePath);
  for (const [key, entry] of Object.entries(store)) {
    ​// merge
  }
}

Impact:

• Event-loop blocking (sync FS calls + JSON loads) → increased latency / request timeouts.
• Resource exhaustion proportional to number/size of stores discovered → process becomes unresponsive.

Recommendation

Avoid synchronous discovery/loading on the request path and add caching + bounds.

Recommended mitigation (example): cache discovered targets for a short TTL and refresh asynchronously.

​// module-level cache
let cachedTargets: { targets: SessionStoreTarget[]; expiresAt: number } | null = null;
const TARGETS_TTL_MS = 5_000;

async function getTargetsCached(cfg: OpenClawConfig): Promise<SessionStoreTarget[]> {
  const now = Date.now();
  if (cachedTargets && cachedTargets.expiresAt > now) return cachedTargets.targets;
  const targets = await resolveAllAgentSessionStoreTargets(cfg);
  cachedTargets = { targets, expiresAt: now + TARGETS_TTL_MS };
  return targets;
}

export async function loadCombinedSessionStoreForGatewayAsync(cfg: OpenClawConfig) {
  const targets = await getTargetsCached(cfg);
  ​// load stores (ideally async) and merge
}

Additional hardening options:

• Enforce a maximum number of discovered agent directories/stores (with warning + truncation).
• Perform discovery once at startup and refresh in the background.
• Gate broad discovery behind a config flag when running in server mode.
• Consider moving store loading off the hot path (e.g., incremental index).

2. 🟡 TOCTOU + hardlink bypass in session-store discovery validation (possible arbitrary file read)

Description

Session-store discovery attempts to keep discovered sessions.json within an agents/ root by checking lstat() (reject symlinks) and comparing realpath() results. However:

• TOCTOU race: validation is performed (lstat/realpath/within-root) and then the validated path string is returned. The file is later opened by path (e.g. fs.readFileSync(storePath) in loadSessionStore) without O_NOFOLLOW/identity checks. A local attacker with write access to the discovered sessions.json directory can swap the validated regular file for a symlink after validation but before read, causing the process to read a different file.
• Hardlink bypass: the validation rejects symlinks but does not reject hardlinks (nlink > 1). realpath() does not “resolve” hardlinks, so a hardlinked sessions.json can point to content from an arbitrary file on the same filesystem. If the OpenClaw process runs with higher privileges than the attacker, this can become an arbitrary file read / information disclosure primitive (the gateway can serve parsed store content to clients).

Vulnerable code (validation step):

const stat = fsSync.lstatSync(storePath);
if (stat.isSymbolicLink() || !stat.isFile()) {
  return undefined;
}
const realStorePath = fsSync.realpathSync(storePath);
const realAgentsRoot = params.realAgentsRoot ?? fsSync.realpathSync(params.agentsRoot);
return isWithinRoot(realStorePath, realAgentsRoot) ? realStorePath : undefined;

Because the validation does not bind the check to the subsequent read, and does not reject hardlinks, it is possible for a discovered store to reference unexpected content.

Recommendation

Bind validation to the actual file opened, and reject hardlinks.

Option A (recommended): when reading sessions.json, open it safely using the existing safe-open utilities (they already support O_NOFOLLOW, hardlink rejection, and identity checks):

import { openVerifiedFileSync } from "../infra/safe-open-sync.js";

const opened = openVerifiedFileSync({
  filePath: storePath,
  rejectPathSymlink: true,
  rejectHardlinks: true,
  maxBytes: /* reasonable max size */,
  allowedType: "file",
});
if (!opened.ok) return undefined;
try {
  const raw = fs.readFileSync(opened.fd, "utf8");
  ​// parse JSON...
} finally {
  fs.closeSync(opened.fd);
}

Option B: if discovery must return a string path, then at minimum re-validate immediately before reading (including nlink checks) and consider using fs.openSync with O_NOFOLLOW where available.

Additionally, consider adding a maximum allowed size for sessions.json reads to reduce DoS risk from discovered roots.

Analyzed PR: #44176 at commit 52ebbf5

_{Last updated on: 2026-03-12T18:19:39Z}

初歩確認

CI 通過後にレビュー予定。変更規模 (+517/-171, 14 files) を考慮し、以下を確認予定:

• session-store discovery の新しいロジックが既存の挙動と後方互換か
• 追加されたテストが edge case をカバーしているか

Greptile Review / Aisle 結果も参照します。

初歩確認

CI 通過後にレビュー予定。変更規模 (+517/-171, 14 files) を考慮し、以下を確認予定:

• session-store discovery の新しいロジックが既存の挙動と後方互換か
• 追加されたテストが edge case をカバーしているか

Greptile Review / Aisle 結果も参照します。

stop running automated checks and spamming comments/PRs; final warning

Greptile Summary

This PR hardens custom session-store discovery across ACP enumeration, gateway combined-store loading, and run-id fallback lookup by extracting a shared resolveAllAgentSessionStoreTargets helper that discovers disk-only and retired agent stores under both the default state tree and any configured custom template root. The refactoring is clean, backward-compatible, and well-tested for the new discovery paths.

Key changes:

• New src/config/sessions/targets.ts centralizes resolveSessionStoreTargets, resolveAllAgentSessionStoreTargets (async), and resolveAllAgentSessionStoreTargetsSync with proper env propagation.
• resolveStorePath in paths.ts now threads a caller-supplied env through all home-dir helpers instead of hard-coding process.env.
• New resolveAgentsDirFromSessionStorePath extracts the agents/ root from a store path so discovery can scan the custom root without re-parsing the template.
• loadCombinedSessionStoreForGateway and resolveSessionKeyForRun now use the shared discovery, picking up disk-only/retired agents under custom store roots.
• listAcpSessionEntries (ACP startup reconciliation) similarly delegates to the shared helper.
• Test gap: src/config/sessions/targets.test.ts covers resolveAllAgentSessionStoreTargets thoroughly, but resolveSessionStoreTargets error/validation cases (unknown agent rejection, conflicting --agent/--all-agents selectors, deduplication of shared-path stores) that previously lived in session-store-targets.test.ts were removed and not replaced anywhere.

Confidence Score: 4/5

• Safe to merge; the fix is well-scoped and backward-compatible, with the only concern being reduced unit-test coverage for resolveSessionStoreTargets error paths.
• The core logic is correct: discovery correctly unions configured and disk-scanned targets, deduplicates by path, and preserves actual on-disk paths. All new paths are regression-tested with real filesystem fixtures. The main deduction is the removal of unit tests for resolveSessionStoreTargets error cases (unknown agent, conflicting selectors, deduplication) without adding equivalent tests in the new targets.test.ts.
• src/commands/session-store-targets.test.ts — error-path tests for resolveSessionStoreTargets were dropped and should be added to src/config/sessions/targets.test.ts.

This is a comment left during a code review.
Path: src/commands/session-store-targets.test.ts
Line: 235-272

Comment:
**Error-path tests dropped without replacement in `targets.test.ts`**

The refactor removed four specific test cases that validated important invariants of `resolveSessionStoreTargets`:

- "dedupes shared store paths for --all-agents"
- "rejects unknown agent ids"
- "rejects conflicting selectors" (two assertions: `--agent`+`--all-agents` and `--store`+`--all-agents`)
- "resolves all configured agent stores"

The replacement test only verifies the delegation call-through. However, `src/config/sessions/targets.test.ts` exclusively tests `resolveAllAgentSessionStoreTargets` — it has zero tests for `resolveSessionStoreTargets` itself. The error-throwing paths (`hasAgent && allAgents`, `opts.store && allAgents/hasAgent`, unknown-agent guard) and deduplication logic are now untested. Since those guard clauses are still present in `targets.ts`, they should be exercised at the new location.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: e18feb6}

Thanks for the review. I checked these concerns against SECURITY.md and also hardened the implementation on top of that.

From a security-triage standpoint, I do not think the reported scenarios are in-scope vulnerabilities under OpenClaw's stated trust model:

• SECURITY.md explicitly does not treat a shared gateway as a per-user adversarial isolation boundary.
• Authenticated gateway callers are trusted operators in that model.
• Reports that depend on writing attacker-controlled files or symlinks under trusted local state such as ~/.openclaw are out of scope.

That applies to the symlinked-store, retired-store visibility, normalized-dir collision, and run-id poisoning scenarios as written in the comment.

That said, the underlying hardening opportunities were still worthwhile, so I pushed defense-in-depth changes in the PR head:

• discovered sessions.json files are now skipped if they are symlinks
• discovered stores are validated to remain inside the resolved agent root after realpath
• discovered directory names that collapse into the implicit default main agent id are skipped unless they actually are main
• duplicate sessionId run lookups are now deterministic and ambiguity-safe instead of taking the first arbitrary match

This was added in the current PR head commit:
4f2970d6ccaba621240bce3f7f3b427f80e3074b

Targeted verification passed:
pnpm vitest run src/config/sessions/targets.test.ts src/gateway/server-session-key.test.ts src/acp/runtime/session-meta.test.ts src/gateway/session-utils.test.ts

So my conclusion is:

• the original comment does not establish an in-scope security issue under SECURITY.md
• the code is now stricter anyway, which removes the main defense-in-depth concerns that were worth addressing

Merged via squash.

• Prepared head SHA: 52ebbf5188b47386f2a78ac4715993bc082e911b
• Merge commit: 46f0bfc55b587eaa276a59bbdb3fbceef445f0bf

Thanks @gumadeiras!