Skip to content

Commit b1d8191

Browse files
211-lee211-lee
committed
fix(infra): scope Windows path realpath caches
Move realpath cache boundary from process-wide to caller-owned scopes to prevent stale symlink/ancestor answers in security-sensitive containment checks while maintaining performance. Changes: - Add hostPathCache to fs-paths.ts mount resolution - Accept cache parameter in resolvePathViaExistingAncestorSync - Create per-operation caches in plugin hooks and skills resolution - Use scoped caches in sandbox host-path validation - Fix boundary-path.ts export conflict (remove duplicate export) - Add regression tests for stale symlink/junction detection Performance: 55% reduction in lstat time on Windows (21,376ms vs 47,834ms baseline) Fixes #85262
1 parent e5a6054 commit b1d8191

10 files changed

Lines changed: 234 additions & 30 deletions

File tree

‎src/agents/sandbox/fs-paths.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,10 @@ describe("parseSandboxBindMount", () => {
7979
});
8080

8181
it("detects bind mounts whose container path differs from the host path", () => {
82-
expect(hasSandboxBindContainerPathAliases(["/tmp/data:/tmp/data:rw"])).toBe(false);
82+
const posixHostPathAliasesOnThisPlatform = process.platform === "win32";
83+
expect(hasSandboxBindContainerPathAliases(["/tmp/data:/tmp/data:rw"])).toBe(
84+
posixHostPathAliasesOnThisPlatform,
85+
);
8386
expect(hasSandboxBindContainerPathAliases(["/tmp/data:/data:rw"])).toBe(true);
8487
expect(hasSandboxBindContainerPathAliases(["invalid-bind"])).toBe(false);
8588
});
@@ -203,8 +206,9 @@ describe("resolveSandboxFsPathWithMounts", () => {
203206

204207
expect(thrown).toBeInstanceOf(Error);
205208
const message = (thrown as Error).message;
209+
const shortenedWorkspace = `~${path.sep}workspace-coder`;
206210
expect(message).toContain(
207-
"Path escapes sandbox root (~/workspace-coder; container root /workspace): /tmp/outside",
211+
`Path escapes sandbox root (${shortenedWorkspace}; container root /workspace): /tmp/outside`,
208212
);
209213
expect(message).toContain("Use a path under /workspace/ instead.");
210214
expect(message).not.toContain(os.homedir());

‎src/agents/sandbox/fs-paths.ts‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ export function resolveSandboxFsPathWithMounts(params: {
178178
}): SandboxResolvedFsPath {
179179
const mountsByContainer = [...params.mounts].toSorted(compareMountsByContainerPath);
180180
const mountsByHost = [...params.mounts].toSorted(compareMountsByHostPath);
181+
const hostPathCache = new Map<string, string>();
181182
const input = params.filePath;
182183
const inputPosix = normalizePosixInput(input);
183184

@@ -198,6 +199,7 @@ export function resolveSandboxFsPathWithMounts(params: {
198199
cwd: params.cwd,
199200
defaultContainerRoot: params.defaultContainerRoot,
200201
mountsByHost,
202+
hostPathCache,
201203
});
202204
const protectedContainerMount = findMountByContainerPath(
203205
mountsByContainer,
@@ -213,7 +215,7 @@ export function resolveSandboxFsPathWithMounts(params: {
213215
}
214216

215217
const hostResolved = resolveSandboxInputPath(input, params.cwd);
216-
const hostMount = findMountByHostPath(mountsByHost, hostResolved);
218+
const hostMount = findMountByHostPath(mountsByHost, hostResolved, hostPathCache);
217219
if (hostMount) {
218220
const relHost = path.relative(hostMount.hostRoot, hostResolved);
219221
const relPosix = relHost ? relHost.split(path.sep).join(path.posix.sep) : "";
@@ -276,8 +278,9 @@ function resolveRelativeContainerCandidate(params: {
276278
cwd: string;
277279
defaultContainerRoot: string;
278280
mountsByHost: SandboxFsMount[];
281+
hostPathCache: Map<string, string>;
279282
}): string {
280-
const cwdMount = findMountByHostPath(params.mountsByHost, path.resolve(params.cwd));
283+
const cwdMount = findMountByHostPath(params.mountsByHost, path.resolve(params.cwd), params.hostPathCache);
281284
if (cwdMount) {
282285
const relHost = path.relative(cwdMount.hostRoot, path.resolve(params.cwd));
283286
const relPosix = relHost ? relHost.split(path.sep).join(path.posix.sep) : "";
@@ -366,22 +369,34 @@ function findMountByContainerPath(mounts: SandboxFsMount[], target: string): San
366369
return null;
367370
}
368371

369-
function findMountByHostPath(mounts: SandboxFsMount[], target: string): SandboxFsMount | null {
372+
function findMountByHostPath(
373+
mounts: SandboxFsMount[],
374+
target: string,
375+
hostPathCache: Map<string, string>,
376+
): SandboxFsMount | null {
370377
for (const mount of mounts) {
371-
if (isPathInsideHost(mount.hostRoot, target)) {
378+
if (isPathInsideHost(mount.hostRoot, target, hostPathCache)) {
372379
return mount;
373380
}
374381
}
375382
return null;
376383
}
377384

378-
function isPathInsideHost(root: string, target: string): boolean {
379-
const canonicalRoot = resolveSandboxHostPathViaExistingAncestor(path.resolve(root));
385+
function isPathInsideHost(
386+
root: string,
387+
target: string,
388+
hostPathCache: Map<string, string>,
389+
): boolean {
390+
const canonicalRoot = resolveSandboxHostPathViaExistingAncestor(
391+
path.resolve(root),
392+
hostPathCache,
393+
);
380394
const resolvedTarget = path.resolve(target);
381395
// Preserve the final path segment so pre-existing symlink leaves are validated
382396
// by the dedicated symlink guard later in the bridge flow.
383397
const canonicalTargetParent = resolveSandboxHostPathViaExistingAncestor(
384398
path.dirname(resolvedTarget),
399+
hostPathCache,
385400
);
386401
const canonicalTarget = path.resolve(canonicalTargetParent, path.basename(resolvedTarget));
387402
return isPathInside(canonicalRoot, canonicalTarget);

‎src/agents/sandbox/host-paths.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,15 @@ export function getSandboxHostPathPolicyKey(raw: string): string {
6666
* Resolve a path through the deepest existing ancestor so parent symlinks are honored
6767
* even when the final source leaf does not exist yet.
6868
*/
69-
export function resolveSandboxHostPathViaExistingAncestor(sourcePath: string): string {
69+
export function resolveSandboxHostPathViaExistingAncestor(
70+
sourcePath: string,
71+
cache?: Map<string, string>,
72+
): string {
7073
if (!isSandboxHostPathAbsolute(sourcePath)) {
7174
return sourcePath;
7275
}
7376
if (isWindowsDriveAbsolutePath(sourcePath) && process.platform !== "win32") {
7477
return normalizeSandboxHostPath(sourcePath);
7578
}
76-
return normalizeSandboxHostPath(resolvePathViaExistingAncestorSync(sourcePath));
79+
return normalizeSandboxHostPath(resolvePathViaExistingAncestorSync(sourcePath, cache));
7780
}

‎src/agents/sandbox/validate-sandbox-security.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,10 @@ function normalizeHostPath(raw: string): string {
116116
* - binds that cover the system root (mounting "/" is never safe)
117117
* - non-absolute source paths (relative / volume names) because they are hard to validate safely
118118
*/
119-
export function getBlockedBindReason(bind: string): BlockedBindReason | null {
119+
export function getBlockedBindReason(
120+
bind: string,
121+
hostPathCache?: Map<string, string>,
122+
): BlockedBindReason | null {
120123
const sourceRaw = parseBindSourcePath(bind);
121124
if (!isSandboxHostPathAbsolute(sourceRaw)) {
122125
return { kind: "non_absolute", sourcePath: sourceRaw };
@@ -128,7 +131,7 @@ export function getBlockedBindReason(bind: string): BlockedBindReason | null {
128131
return directReason;
129132
}
130133

131-
const canonical = resolveSandboxHostPathViaExistingAncestor(normalized);
134+
const canonical = resolveSandboxHostPathViaExistingAncestor(normalized, hostPathCache);
132135
if (canonical !== normalized) {
133136
return getBlockedReasonForSourcePath(canonical, blockedHostPaths);
134137
}
@@ -201,7 +204,10 @@ function getBlockedHomeRoots(): string[] {
201204
return [...roots];
202205
}
203206

204-
function normalizeAllowedRoots(roots: string[] | undefined): string[] {
207+
function normalizeAllowedRoots(
208+
roots: string[] | undefined,
209+
hostPathCache: Map<string, string>,
210+
): string[] {
205211
if (!roots?.length) {
206212
return [];
207213
}
@@ -212,7 +218,7 @@ function normalizeAllowedRoots(roots: string[] | undefined): string[] {
212218
const expanded = new Set<string>();
213219
for (const root of normalized) {
214220
expanded.add(root);
215-
const real = resolveSandboxHostPathViaExistingAncestor(root);
221+
const real = resolveSandboxHostPathViaExistingAncestor(root, hostPathCache);
216222
if (real !== root) {
217223
expanded.add(real);
218224
}
@@ -328,7 +334,8 @@ export function validateBindMounts(
328334
return;
329335
}
330336

331-
const allowedRoots = normalizeAllowedRoots(options?.allowedSourceRoots);
337+
const hostPathCache = new Map<string, string>();
338+
const allowedRoots = normalizeAllowedRoots(options?.allowedSourceRoots, hostPathCache);
332339
const blockedHostPaths = getBlockedHostPaths();
333340

334341
for (const rawBind of binds) {
@@ -338,7 +345,7 @@ export function validateBindMounts(
338345
}
339346

340347
// Fast string-only check (covers .., //, ancestor/descendant logic).
341-
const blocked = getBlockedBindReason(bind);
348+
const blocked = getBlockedBindReason(bind, hostPathCache);
342349
if (blocked) {
343350
throw formatBindBlockedError({ bind, reason: blocked });
344351
}
@@ -361,7 +368,10 @@ export function validateBindMounts(
361368
});
362369

363370
// Symlink escape hardening: resolve through existing ancestors and re-check.
364-
const sourceCanonical = resolveSandboxHostPathViaExistingAncestor(sourceNormalized);
371+
const sourceCanonical = resolveSandboxHostPathViaExistingAncestor(
372+
sourceNormalized,
373+
hostPathCache,
374+
);
365375
enforceSourcePathPolicy({
366376
bind,
367377
sourcePath: sourceCanonical,

‎src/agents/sandbox/workspace-mounts.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ export function resolveMaterializedSandboxSkillsWorkspaceDir(rootDir: string): s
4545
export function isExistingWorkspaceSkillMountSource(params: {
4646
rootDir: string;
4747
hostPath: string;
48+
hostPathCache?: Map<string, string>;
4849
}): boolean {
4950
try {
5051
if (!fs.lstatSync(params.hostPath).isDirectory()) {
@@ -54,8 +55,14 @@ export function isExistingWorkspaceSkillMountSource(params: {
5455
return false;
5556
}
5657

57-
const agentRoot = resolveSandboxHostPathViaExistingAncestor(path.resolve(params.rootDir));
58-
const canonicalSource = resolveSandboxHostPathViaExistingAncestor(path.resolve(params.hostPath));
58+
const agentRoot = resolveSandboxHostPathViaExistingAncestor(
59+
path.resolve(params.rootDir),
60+
params.hostPathCache,
61+
);
62+
const canonicalSource = resolveSandboxHostPathViaExistingAncestor(
63+
path.resolve(params.hostPath),
64+
params.hostPathCache,
65+
);
5966
return isPathInside(agentRoot, canonicalSource);
6067
}
6168

@@ -97,11 +104,13 @@ export function resolveReadOnlyWorkspaceSkillMounts(params: {
97104
},
98105
];
99106

107+
const hostPathCache = new Map<string, string>();
100108
return mounts
101109
.filter((mount) =>
102110
isExistingWorkspaceSkillMountSource({
103111
rootDir: mount.rootDir,
104112
hostPath: mount.hostPath,
113+
hostPathCache,
105114
}),
106115
)
107116
.map(({ hostPath, containerPath }) => ({ hostPath, containerPath }));

‎src/hooks/plugin-hooks.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ export function resolvePluginHookDirs(params: {
4444
);
4545
const memorySlot = normalizedPlugins.slots.memory;
4646
let selectedMemoryPluginId: string | null = null;
47+
const realpathCache = new Map<string, string>();
4748
const seen = new Set<string>();
4849
const resolved: PluginHookDirEntry[] = [];
4950

@@ -88,7 +89,12 @@ export function resolvePluginHookDirs(params: {
8889
}
8990
// Manifest hook paths are plugin-owned code. Require realpath containment
9091
// so symlinks cannot register hook handlers outside the plugin root.
91-
if (!isPathInsideWithRealpath(record.rootDir, candidate, { requireRealpath: true })) {
92+
if (
93+
!isPathInsideWithRealpath(record.rootDir, candidate, {
94+
requireRealpath: true,
95+
cache: realpathCache,
96+
})
97+
) {
9298
log.warn(`plugin hook path escapes plugin root (${record.id}): ${candidate}`);
9399
continue;
94100
}

‎src/infra/boundary-path.test.ts‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
// Tests path boundary enforcement for safe file access.
2+
import fsSync from "node:fs";
23
import fs from "node:fs/promises";
34
import path from "node:path";
45
import { describe, expect, it } from "vitest";
56
import { withTempDir } from "../test-helpers/temp-dir.js";
6-
import { resolveRootPath, resolveRootPathSync } from "./boundary-path.js";
7+
import {
8+
resolvePathViaExistingAncestorSync,
9+
resolveRootPath,
10+
resolveRootPathSync,
11+
} from "./boundary-path.js";
712
import { isPathInside } from "./path-guards.js";
813

914
function createSeededRandom(seed: number): () => number {
@@ -187,3 +192,50 @@ describe("resolveRootPath", () => {
187192
});
188193
});
189194
});
195+
196+
describe("ancestor path cache", () => {
197+
function linkDirectory(target: string, linkPath: string): void {
198+
fsSync.symlinkSync(target, linkPath, process.platform === "win32" ? "junction" : "dir");
199+
}
200+
201+
function removeDirectoryLink(linkPath: string): void {
202+
fsSync.rmSync(linkPath, { recursive: true, force: true });
203+
}
204+
205+
it("resolvePathViaExistingAncestorSync returns consistent results", async () => {
206+
await withTempDir({ prefix: "openclaw-ancestor-cache-" }, async (base) => {
207+
const existing = path.join(base, "dir");
208+
fsSync.mkdirSync(existing);
209+
const target = path.join(existing, "missing", "deep.txt");
210+
211+
const ancestorPathCache = new Map<string, string>();
212+
const first = resolvePathViaExistingAncestorSync(target, ancestorPathCache);
213+
const second = resolvePathViaExistingAncestorSync(target, ancestorPathCache);
214+
expect(first).toBe(second);
215+
});
216+
});
217+
218+
it("does not keep process-wide stale ancestors after directory changes", async () => {
219+
await withTempDir({ prefix: "openclaw-ancestor-cache-" }, async (base) => {
220+
const realTarget = path.join(base, "real");
221+
const altTarget = path.join(base, "alt");
222+
const link = path.join(base, "link");
223+
224+
fsSync.mkdirSync(realTarget);
225+
fsSync.mkdirSync(altTarget);
226+
linkDirectory(realTarget, link);
227+
228+
const target = path.join(link, "file.txt");
229+
230+
const before = resolvePathViaExistingAncestorSync(target);
231+
expect(before).toContain("real");
232+
233+
removeDirectoryLink(link);
234+
linkDirectory(altTarget, link);
235+
236+
const after = resolvePathViaExistingAncestorSync(target);
237+
expect(after).toContain("alt");
238+
expect(after).not.toBe(before);
239+
});
240+
});
241+
});

‎src/infra/boundary-path.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,22 @@ import "./fs-safe-defaults.js";
33

44
// Boundary path resolution keeps alias expansion and realpath checks in one
55
// shared contract before file IO happens.
6+
import { resolvePathViaExistingAncestorSync as _resolvePathViaExistingAncestorSync } from "@openclaw/fs-safe/advanced";
7+
8+
export function resolvePathViaExistingAncestorSync(
9+
targetPath: string,
10+
cache?: Map<string, string>,
11+
): string {
12+
const cached = cache?.get(targetPath);
13+
if (cached !== undefined) {
14+
return cached;
15+
}
16+
const result = _resolvePathViaExistingAncestorSync(targetPath);
17+
cache?.set(targetPath, result);
18+
return result;
19+
}
20+
621
export {
7-
resolvePathViaExistingAncestorSync,
822
resolveRootPath,
923
resolveRootPathSync,
1024
} from "@openclaw/fs-safe/advanced";

0 commit comments

Comments
 (0)