refactor: unify boundary-safe reads for bootstrap and includes

This commit is contained in:
Peter Steinberger
2026-02-26 12:42:06 +01:00
parent 199ef9f8ea
commit 242188b7b1
8 changed files with 374 additions and 100 deletions

View File

@@ -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,

View File

@@ -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);

View File

@@ -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);
});
});

View File

@@ -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", () => {

View File

@@ -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<string, Promise<string>>();
let gitAvailabilityPromise: Promise<boolean> | 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<string, { content: string; mtimeMs: number }>();
// File content cache keyed by stable file identity to avoid stale reads.
const workspaceFileCache = new Map<string, { content: string; identity: string }>();
/**
* 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<string> {
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<WorkspaceGuardedReadResult> {
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<Workspac
const result: WorkspaceBootstrapFile[] = [];
for (const entry of entries) {
try {
const content = await readFileWithCache(entry.filePath);
const loaded = await readWorkspaceFileWithGuards({
filePath: entry.filePath,
workspaceDir: resolvedDir,
});
if (loaded.ok) {
result.push({
name: entry.name,
path: entry.filePath,
content,
content: loaded.content,
missing: false,
});
} catch {
} else {
result.push({ name: entry.name, path: entry.filePath, missing: true });
}
}
@@ -516,16 +553,21 @@ export async function loadExtraBootstrapFiles(
dir: string,
extraPatterns: string[],
): Promise<WorkspaceBootstrapFile[]> {
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<string>();
@@ -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 };
}

View File

@@ -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,
});

View File

@@ -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<Record<string, number>>((counts, item) => {
counts[item.reason] = (counts[item.reason] ?? 0) + 1;
return counts;
}, {}),
});
}
if (extras.length === 0) {
return;
}

View File

@@ -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<typeof fs, "realpathSync">, 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<BoundaryFileOpenResult> {
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);
}