From 242188b7b14cd52556026f9274a43f6233e9fc95 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 12:42:06 +0100 Subject: [PATCH] refactor: unify boundary-safe reads for bootstrap and includes --- src/agents/sandbox/fs-bridge.ts | 24 ++- src/agents/workspace.bootstrap-cache.test.ts | 28 +++ ...rkspace.load-extra-bootstrap-files.test.ts | 41 ++++- src/agents/workspace.test.ts | 32 ++++ src/agents/workspace.ts | 161 ++++++++++++------ src/config/includes.ts | 43 +---- .../bundled/bootstrap-extra-files/handler.ts | 16 +- src/infra/boundary-file-read.ts | 129 ++++++++++++++ 8 files changed, 374 insertions(+), 100 deletions(-) create mode 100644 src/infra/boundary-file-read.ts diff --git a/src/agents/sandbox/fs-bridge.ts b/src/agents/sandbox/fs-bridge.ts index 23ebcce51b1..3c92297663a 100644 --- a/src/agents/sandbox/fs-bridge.ts +++ b/src/agents/sandbox/fs-bridge.ts @@ -1,8 +1,6 @@ -import { - assertNoPathAliasEscape, - PATH_ALIAS_POLICIES, - type PathAliasPolicy, -} from "../../infra/path-alias-guards.js"; +import fs from "node:fs"; +import { openBoundaryFile } from "../../infra/boundary-file-read.js"; +import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../../infra/path-alias-guards.js"; import { execDockerRaw, type ExecDockerRawResult } from "./docker.js"; import { buildSandboxFsMounts, @@ -24,6 +22,7 @@ type PathSafetyOptions = { action: string; aliasPolicy?: PathAliasPolicy; requireWritable?: boolean; + allowMissingTarget?: boolean; }; export type SandboxResolvedPath = { @@ -254,12 +253,23 @@ class SandboxFsBridgeImpl implements SandboxFsBridge { ); } - await assertNoPathAliasEscape({ + const guarded = await openBoundaryFile({ absolutePath: target.hostPath, rootPath: lexicalMount.hostRoot, boundaryLabel: "sandbox mount root", - policy: options.aliasPolicy, + aliasPolicy: options.aliasPolicy, }); + if (!guarded.ok) { + if (guarded.reason !== "path" || options.allowMissingTarget === false) { + throw guarded.error instanceof Error + ? guarded.error + : new Error( + `Sandbox boundary checks failed; cannot ${options.action}: ${target.containerPath}`, + ); + } + } else { + fs.closeSync(guarded.fd); + } const canonicalContainerPath = await this.resolveCanonicalContainerPath({ containerPath: target.containerPath, diff --git a/src/agents/workspace.bootstrap-cache.test.ts b/src/agents/workspace.bootstrap-cache.test.ts index a41bafe4a96..6d5300feba1 100644 --- a/src/agents/workspace.bootstrap-cache.test.ts +++ b/src/agents/workspace.bootstrap-cache.test.ts @@ -74,6 +74,34 @@ describe("workspace bootstrap file caching", () => { expectAgentsContent(agentsFile2, content2); }); + it("invalidates cache when inode changes with same mtime", async () => { + if (process.platform === "win32") { + return; + } + const content1 = "# old-content"; + const content2 = "# new-content"; + const filePath = path.join(workspaceDir, DEFAULT_AGENTS_FILENAME); + const tempPath = path.join(workspaceDir, ".AGENTS.tmp"); + + await writeWorkspaceFile({ + dir: workspaceDir, + name: DEFAULT_AGENTS_FILENAME, + content: content1, + }); + const originalStat = await fs.stat(filePath); + + const agentsFile1 = await loadAgentsFile(workspaceDir); + expectAgentsContent(agentsFile1, content1); + + await fs.writeFile(tempPath, content2, "utf-8"); + await fs.utimes(tempPath, originalStat.atime, originalStat.mtime); + await fs.rename(tempPath, filePath); + await fs.utimes(filePath, originalStat.atime, originalStat.mtime); + + const agentsFile2 = await loadAgentsFile(workspaceDir); + expectAgentsContent(agentsFile2, content2); + }); + it("handles file deletion gracefully", async () => { const content = "# Some content"; const filePath = path.join(workspaceDir, DEFAULT_AGENTS_FILENAME); diff --git a/src/agents/workspace.load-extra-bootstrap-files.test.ts b/src/agents/workspace.load-extra-bootstrap-files.test.ts index 0a478524aef..a10d0c727b4 100644 --- a/src/agents/workspace.load-extra-bootstrap-files.test.ts +++ b/src/agents/workspace.load-extra-bootstrap-files.test.ts @@ -2,7 +2,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { loadExtraBootstrapFiles } from "./workspace.js"; +import { loadExtraBootstrapFiles, loadExtraBootstrapFilesWithDiagnostics } from "./workspace.js"; describe("loadExtraBootstrapFiles", () => { let fixtureRoot = ""; @@ -69,4 +69,43 @@ describe("loadExtraBootstrapFiles", () => { expect(files[0]?.name).toBe("AGENTS.md"); expect(files[0]?.content).toBe("linked agents"); }); + + it("rejects hardlinked aliases to files outside workspace", async () => { + if (process.platform === "win32") { + return; + } + + const rootDir = await createWorkspaceDir("hardlink"); + const workspaceDir = path.join(rootDir, "workspace"); + const outsideDir = path.join(rootDir, "outside"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + const outsideFile = path.join(outsideDir, "AGENTS.md"); + const linkedFile = path.join(workspaceDir, "AGENTS.md"); + await fs.writeFile(outsideFile, "outside", "utf-8"); + try { + await fs.link(outsideFile, linkedFile); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const files = await loadExtraBootstrapFiles(workspaceDir, ["AGENTS.md"]); + expect(files).toHaveLength(0); + }); + + it("skips oversized bootstrap files and reports diagnostics", async () => { + const workspaceDir = await createWorkspaceDir("oversized"); + const payload = "x".repeat(2 * 1024 * 1024 + 1); + await fs.writeFile(path.join(workspaceDir, "AGENTS.md"), payload, "utf-8"); + + const { files, diagnostics } = await loadExtraBootstrapFilesWithDiagnostics(workspaceDir, [ + "AGENTS.md", + ]); + + expect(files).toHaveLength(0); + expect(diagnostics.some((d) => d.reason === "security")).toBe(true); + }); }); diff --git a/src/agents/workspace.test.ts b/src/agents/workspace.test.ts index 0c854178917..3f080077fa9 100644 --- a/src/agents/workspace.test.ts +++ b/src/agents/workspace.test.ts @@ -1,4 +1,5 @@ import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; import { makeTempWorkspace, writeWorkspaceFile } from "../test-helpers/workspace.js"; @@ -142,6 +143,37 @@ describe("loadWorkspaceBootstrapFiles", () => { const files = await loadWorkspaceBootstrapFiles(tempDir); expect(getMemoryEntries(files)).toHaveLength(0); }); + + it("treats hardlinked bootstrap aliases as missing", async () => { + if (process.platform === "win32") { + return; + } + const rootDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-workspace-hardlink-")); + try { + const workspaceDir = path.join(rootDir, "workspace"); + const outsideDir = path.join(rootDir, "outside"); + await fs.mkdir(workspaceDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + const outsideFile = path.join(outsideDir, DEFAULT_AGENTS_FILENAME); + const linkPath = path.join(workspaceDir, DEFAULT_AGENTS_FILENAME); + await fs.writeFile(outsideFile, "outside", "utf-8"); + try { + await fs.link(outsideFile, linkPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + const files = await loadWorkspaceBootstrapFiles(workspaceDir); + const agents = files.find((file) => file.name === DEFAULT_AGENTS_FILENAME); + expect(agents?.missing).toBe(true); + expect(agents?.content).toBeUndefined(); + } finally { + await fs.rm(rootDir, { recursive: true, force: true }); + } + }); }); describe("filterBootstrapFilesForSession", () => { diff --git a/src/agents/workspace.ts b/src/agents/workspace.ts index dbef9c6517d..89b788f1e02 100644 --- a/src/agents/workspace.ts +++ b/src/agents/workspace.ts @@ -1,6 +1,8 @@ +import syncFs from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { openBoundaryFile } from "../infra/boundary-file-read.js"; import { resolveRequiredHomeDir } from "../infra/home-dir.js"; import { runCommandWithTimeout } from "../process/exec.js"; import { isCronSessionKey, isSubagentSessionKey } from "../routing/session-key.js"; @@ -35,33 +37,53 @@ const WORKSPACE_STATE_VERSION = 1; const workspaceTemplateCache = new Map>(); let gitAvailabilityPromise: Promise | null = null; +const MAX_WORKSPACE_BOOTSTRAP_FILE_BYTES = 2 * 1024 * 1024; -// File content cache with mtime invalidation to avoid redundant reads -const workspaceFileCache = new Map(); +// File content cache keyed by stable file identity to avoid stale reads. +const workspaceFileCache = new Map(); /** - * Read file with caching based on mtime. Returns cached content if file - * hasn't changed, otherwise reads from disk and updates cache. + * Read workspace files via boundary-safe open and cache by inode/dev/size/mtime identity. */ -async function readFileWithCache(filePath: string): Promise { +type WorkspaceGuardedReadResult = + | { ok: true; content: string } + | { ok: false; reason: "path" | "validation" | "io"; error?: unknown }; + +function workspaceFileIdentity(stat: syncFs.Stats, canonicalPath: string): string { + return `${canonicalPath}|${stat.dev}:${stat.ino}:${stat.size}:${stat.mtimeMs}`; +} + +async function readWorkspaceFileWithGuards(params: { + filePath: string; + workspaceDir: string; +}): Promise { + const opened = await openBoundaryFile({ + absolutePath: params.filePath, + rootPath: params.workspaceDir, + boundaryLabel: "workspace root", + maxBytes: MAX_WORKSPACE_BOOTSTRAP_FILE_BYTES, + }); + if (!opened.ok) { + workspaceFileCache.delete(params.filePath); + return opened; + } + + const identity = workspaceFileIdentity(opened.stat, opened.path); + const cached = workspaceFileCache.get(params.filePath); + if (cached && cached.identity === identity) { + syncFs.closeSync(opened.fd); + return { ok: true, content: cached.content }; + } + try { - const stats = await fs.stat(filePath); - const mtimeMs = stats.mtimeMs; - const cached = workspaceFileCache.get(filePath); - - // Return cached content if mtime matches - if (cached && cached.mtimeMs === mtimeMs) { - return cached.content; - } - - // Read from disk and update cache - const content = await fs.readFile(filePath, "utf-8"); - workspaceFileCache.set(filePath, { content, mtimeMs }); - return content; + const content = syncFs.readFileSync(opened.fd, "utf-8"); + workspaceFileCache.set(params.filePath, { content, identity }); + return { ok: true, content }; } catch (error) { - // Remove from cache if file doesn't exist or is unreadable - workspaceFileCache.delete(filePath); - throw error; + workspaceFileCache.delete(params.filePath); + return { ok: false, reason: "io", error }; + } finally { + syncFs.closeSync(opened.fd); } } @@ -125,6 +147,18 @@ export type WorkspaceBootstrapFile = { missing: boolean; }; +export type ExtraBootstrapLoadDiagnosticCode = + | "invalid-bootstrap-filename" + | "missing" + | "security" + | "io"; + +export type ExtraBootstrapLoadDiagnostic = { + path: string; + reason: ExtraBootstrapLoadDiagnosticCode; + detail: string; +}; + type WorkspaceOnboardingState = { version: typeof WORKSPACE_STATE_VERSION; bootstrapSeededAt?: string; @@ -479,15 +513,18 @@ export async function loadWorkspaceBootstrapFiles(dir: string): Promise { + const loaded = await loadExtraBootstrapFilesWithDiagnostics(dir, extraPatterns); + return loaded.files; +} + +export async function loadExtraBootstrapFilesWithDiagnostics( + dir: string, + extraPatterns: string[], +): Promise<{ + files: WorkspaceBootstrapFile[]; + diagnostics: ExtraBootstrapLoadDiagnostic[]; +}> { if (!extraPatterns.length) { - return []; + return { files: [], diagnostics: [] }; } const resolvedDir = resolveUserPath(dir); - let realResolvedDir = resolvedDir; - try { - realResolvedDir = await fs.realpath(resolvedDir); - } catch { - // Keep lexical root if realpath fails. - } // Resolve glob patterns into concrete file paths const resolvedPaths = new Set(); @@ -545,37 +587,46 @@ export async function loadExtraBootstrapFiles( } } - const result: WorkspaceBootstrapFile[] = []; + const files: WorkspaceBootstrapFile[] = []; + const diagnostics: ExtraBootstrapLoadDiagnostic[] = []; for (const relPath of resolvedPaths) { const filePath = path.resolve(resolvedDir, relPath); - // Guard against path traversal — resolved path must stay within workspace - if (!filePath.startsWith(resolvedDir + path.sep) && filePath !== resolvedDir) { + // Only load files whose basename is a recognized bootstrap filename + const baseName = path.basename(relPath); + if (!VALID_BOOTSTRAP_NAMES.has(baseName)) { + diagnostics.push({ + path: filePath, + reason: "invalid-bootstrap-filename", + detail: `unsupported bootstrap basename: ${baseName}`, + }); continue; } - try { - // Resolve symlinks and verify the real path is still within workspace - const realFilePath = await fs.realpath(filePath); - if ( - !realFilePath.startsWith(realResolvedDir + path.sep) && - realFilePath !== realResolvedDir - ) { - continue; - } - // Only load files whose basename is a recognized bootstrap filename - const baseName = path.basename(relPath); - if (!VALID_BOOTSTRAP_NAMES.has(baseName)) { - continue; - } - const content = await readFileWithCache(realFilePath); - result.push({ + const loaded = await readWorkspaceFileWithGuards({ + filePath, + workspaceDir: resolvedDir, + }); + if (loaded.ok) { + files.push({ name: baseName as WorkspaceBootstrapFileName, path: filePath, - content, + content: loaded.content, missing: false, }); - } catch { - // Silently skip missing extra files + continue; } + + const reason: ExtraBootstrapLoadDiagnosticCode = + loaded.reason === "path" ? "missing" : loaded.reason === "validation" ? "security" : "io"; + diagnostics.push({ + path: filePath, + reason, + detail: + loaded.error instanceof Error + ? loaded.error.message + : typeof loaded.error === "string" + ? loaded.error + : reason, + }); } - return result; + return { files, diagnostics }; } diff --git a/src/config/includes.ts b/src/config/includes.ts index 0d87a2ae378..9486aabdf1f 100644 --- a/src/config/includes.ts +++ b/src/config/includes.ts @@ -13,7 +13,7 @@ import fs from "node:fs"; import path from "node:path"; import JSON5 from "json5"; -import { openVerifiedFileSync } from "../infra/safe-open-sync.js"; +import { canUseBoundaryFileOpen, openBoundaryFileSync } from "../infra/boundary-file-read.js"; import { isPathInside } from "../security/scan-paths.js"; import { isPlainObject } from "../utils.js"; import { isBlockedObjectKey } from "./prototype-keys.js"; @@ -286,46 +286,19 @@ function safeRealpath(target: string): string { } } -function canUseVerifiedFileOpen(ioFs: typeof fs): boolean { - return ( - typeof ioFs.openSync === "function" && - typeof ioFs.closeSync === "function" && - typeof ioFs.fstatSync === "function" && - typeof ioFs.lstatSync === "function" && - typeof ioFs.realpathSync === "function" && - typeof ioFs.readFileSync === "function" && - typeof ioFs.constants === "object" && - ioFs.constants !== null - ); -} - export function readConfigIncludeFileWithGuards(params: IncludeFileReadParams): string { const ioFs = params.ioFs ?? fs; const maxBytes = params.maxBytes ?? MAX_INCLUDE_FILE_BYTES; - let realPath = params.resolvedPath; - try { - realPath = ioFs.realpathSync(params.resolvedPath); - if (!isPathInside(params.rootRealDir, realPath)) { - throw new ConfigIncludeError( - `Include path resolves outside config directory (symlink): ${params.includePath}`, - params.includePath, - ); - } - } catch (err) { - if (err instanceof ConfigIncludeError) { - throw err; - } - // File may not exist yet; read path will report a precise error below. - } - - if (!canUseVerifiedFileOpen(ioFs)) { + if (!canUseBoundaryFileOpen(ioFs)) { return ioFs.readFileSync(params.resolvedPath, "utf-8"); } - const opened = openVerifiedFileSync({ - filePath: params.resolvedPath, - resolvedPath: realPath, - rejectHardlinks: true, + const opened = openBoundaryFileSync({ + absolutePath: params.resolvedPath, + rootPath: params.rootRealDir, + rootRealPath: params.rootRealDir, + boundaryLabel: "config directory", + skipLexicalRootCheck: true, maxBytes, ioFs, }); diff --git a/src/hooks/bundled/bootstrap-extra-files/handler.ts b/src/hooks/bundled/bootstrap-extra-files/handler.ts index 2f015b280fc..bc708b62d07 100644 --- a/src/hooks/bundled/bootstrap-extra-files/handler.ts +++ b/src/hooks/bundled/bootstrap-extra-files/handler.ts @@ -1,6 +1,6 @@ import { filterBootstrapFilesForSession, - loadExtraBootstrapFiles, + loadExtraBootstrapFilesWithDiagnostics, } from "../../../agents/workspace.js"; import { createSubsystemLogger } from "../../../logging/subsystem.js"; import { resolveHookConfig } from "../../config.js"; @@ -45,7 +45,19 @@ const bootstrapExtraFilesHook: HookHandler = async (event) => { } try { - const extras = await loadExtraBootstrapFiles(context.workspaceDir, patterns); + const { files: extras, diagnostics } = await loadExtraBootstrapFilesWithDiagnostics( + context.workspaceDir, + patterns, + ); + if (diagnostics.length > 0) { + log.debug("skipped extra bootstrap candidates", { + skipped: diagnostics.length, + reasons: diagnostics.reduce>((counts, item) => { + counts[item.reason] = (counts[item.reason] ?? 0) + 1; + return counts; + }, {}), + }); + } if (extras.length === 0) { return; } diff --git a/src/infra/boundary-file-read.ts b/src/infra/boundary-file-read.ts new file mode 100644 index 00000000000..a8d416e0310 --- /dev/null +++ b/src/infra/boundary-file-read.ts @@ -0,0 +1,129 @@ +import fs from "node:fs"; +import path from "node:path"; +import { assertNoPathAliasEscape, type PathAliasPolicy } from "./path-alias-guards.js"; +import { isNotFoundPathError, isPathInside } from "./path-guards.js"; +import { openVerifiedFileSync, type SafeOpenSyncFailureReason } from "./safe-open-sync.js"; + +type BoundaryReadFs = Pick< + typeof fs, + | "closeSync" + | "constants" + | "fstatSync" + | "lstatSync" + | "openSync" + | "readFileSync" + | "realpathSync" +>; + +export type BoundaryFileOpenFailureReason = SafeOpenSyncFailureReason | "validation"; + +export type BoundaryFileOpenResult = + | { ok: true; path: string; fd: number; stat: fs.Stats; rootRealPath: string } + | { ok: false; reason: BoundaryFileOpenFailureReason; error?: unknown }; + +export type OpenBoundaryFileSyncParams = { + absolutePath: string; + rootPath: string; + boundaryLabel: string; + rootRealPath?: string; + maxBytes?: number; + rejectHardlinks?: boolean; + skipLexicalRootCheck?: boolean; + ioFs?: BoundaryReadFs; +}; + +export type OpenBoundaryFileParams = OpenBoundaryFileSyncParams & { + aliasPolicy?: PathAliasPolicy; +}; + +function safeRealpathSync(ioFs: Pick, value: string): string { + try { + return path.resolve(ioFs.realpathSync(value)); + } catch { + return path.resolve(value); + } +} + +export function canUseBoundaryFileOpen(ioFs: typeof fs): boolean { + return ( + typeof ioFs.openSync === "function" && + typeof ioFs.closeSync === "function" && + typeof ioFs.fstatSync === "function" && + typeof ioFs.lstatSync === "function" && + typeof ioFs.realpathSync === "function" && + typeof ioFs.readFileSync === "function" && + typeof ioFs.constants === "object" && + ioFs.constants !== null + ); +} + +export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): BoundaryFileOpenResult { + const ioFs = params.ioFs ?? fs; + const absolutePath = path.resolve(params.absolutePath); + const rootPath = path.resolve(params.rootPath); + const rootRealPath = params.rootRealPath + ? path.resolve(params.rootRealPath) + : safeRealpathSync(ioFs, rootPath); + + if (!params.skipLexicalRootCheck && !isPathInside(rootPath, absolutePath)) { + return { + ok: false, + reason: "validation", + error: new Error(`Path escapes ${params.boundaryLabel}: ${absolutePath} (root: ${rootPath})`), + }; + } + + let resolvedPath = absolutePath; + try { + const candidateRealPath = path.resolve(ioFs.realpathSync(absolutePath)); + if (!isPathInside(rootRealPath, candidateRealPath)) { + return { + ok: false, + reason: "validation", + error: new Error( + `Path resolves outside ${params.boundaryLabel}: ${absolutePath} (root: ${rootRealPath})`, + ), + }; + } + resolvedPath = candidateRealPath; + } catch (error) { + if (!isNotFoundPathError(error)) { + // Keep resolvedPath as lexical path; openVerifiedFileSync below will produce + // a canonical error classification for missing/unreadable targets. + } + } + + const opened = openVerifiedFileSync({ + filePath: absolutePath, + resolvedPath, + rejectHardlinks: params.rejectHardlinks ?? true, + maxBytes: params.maxBytes, + ioFs, + }); + if (!opened.ok) { + return opened; + } + return { + ok: true, + path: opened.path, + fd: opened.fd, + stat: opened.stat, + rootRealPath, + }; +} + +export async function openBoundaryFile( + params: OpenBoundaryFileParams, +): Promise { + try { + await assertNoPathAliasEscape({ + absolutePath: params.absolutePath, + rootPath: params.rootPath, + boundaryLabel: params.boundaryLabel, + policy: params.aliasPolicy, + }); + } catch (error) { + return { ok: false, reason: "validation", error }; + } + return openBoundaryFileSync(params); +}