mirror of
https://github.com/openclaw/openclaw.git
synced 2026-10-03 09:39:25 +00:00
fix(memory): reject reads outside a replaced authorized directory (#156222)
Related: #154370 ## What Problem This Solves Fixes memory reads returning contents outside an authorized directory when that directory is replaced between path admission and the actual read. ## User Impact Memory reads and transient retries stay bound to the admitted filesystem root. Existing contained workspace aliases, strict extra-directory rules, hardlinks, literal filenames, Markdown/glob restrictions, file-size behavior, and missing-file versus I/O-error results remain supported. ## Why This Change Was Made The memory reader now retains the existing fs-safe Root through the content read and retry, replacing separate path checks followed by an unrelated absolute-path read. This removes duplicate containment helpers without changing configuration, stored formats, or public memory APIs. The existing `memory_get` and remote-worker routes continue to call the same reader. ## Evidence - Two real directory-substitution cases returned outside contents on the original implementation and now refuse the substituted data. Existing compatibility controls also passed on the original source. - Final focused proof passed 40 cases in 73.022 seconds. The changed reader suite passed all 15 cases with `node scripts/run-vitest.mjs packages/memory-host-sdk/src/host/read-file.test.ts --maxWorkers=1 --reporter=verbose` in 59.985 seconds wall time. Cold preparation differs between runs; no speed comparison is claimed. - Coverage includes contained workspace aliases, rejected extra-root aliases, hardlinks, literal `~`, Markdown/glob admission, transient `EAGAIN` retries, propagated `EIO`, and missing/error distinctions. - All selected check components passed across the initial runs and targeted lint corrections, including core and all 25 test type graphs, dead exports, formatting, and the remaining guards. The final ten-command recovery run passed in 583.501 seconds; the original failing check command is not represented as a pass. - Earlier review findings about retry and I/O propagation were fixed. The final review's Markdown-widening claim was rejected after source inspection: `matchesDirectory` still requires `.md`, its directory branch requires that predicate, and the retained `note.txt` rejection case passes. Raw review findings were preserved; there are no accepted actionable findings. The final lint edit only omitted an identical default `void` type argument. - Proof used synthetic local files on macOS. No Windows or live-Gateway execution is claimed.
This commit is contained in:
Peter Steinberger
GitHub
parent
f2d7a0d210
commit
6619a0e5eb
3 changed files with 230 additions and 110 deletions
|
|
@ -1,11 +1,7 @@
|
|||
// Retain fs-safe's process configuration for host-side memory file operations.
|
||||
export { root } from "@openclaw/fs-safe/root";
|
||||
export { isPathInside, isPathInsideWithRealpath } from "@openclaw/fs-safe/path";
|
||||
export {
|
||||
assertNoSymlinkParents,
|
||||
readRegularFile,
|
||||
statRegularFile,
|
||||
} from "@openclaw/fs-safe/advanced";
|
||||
export { isPathInside } from "@openclaw/fs-safe/path";
|
||||
export { readRegularFile, statRegularFile } from "@openclaw/fs-safe/advanced";
|
||||
export { walkDirectory, type WalkDirectoryEntry } from "@openclaw/fs-safe/walk";
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -3,8 +3,13 @@ import fsSync from "node:fs";
|
|||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { createDeferred } from "../../../../test/helpers/promise.js";
|
||||
import { useAutoCleanupTempDirTracker } from "../../../../test/helpers/temp-dir.js";
|
||||
import { readAgentMemoryFile, readMemoryFile } from "./read-file.js";
|
||||
import * as memoryReadRetry from "./read-retry.js";
|
||||
|
||||
const tempDirs = useAutoCleanupTempDirTracker(afterEach);
|
||||
|
||||
async function createDirectorySymlink(target: string, linkPath: string): Promise<boolean> {
|
||||
try {
|
||||
|
|
@ -20,6 +25,150 @@ async function createDirectorySymlink(target: string, linkPath: string): Promise
|
|||
}
|
||||
|
||||
describe("readMemoryFile", () => {
|
||||
it("follows contained workspace parent aliases while keeping extra directories strict", async () => {
|
||||
const directory = tempDirs.make("memory-read-parent-alias-");
|
||||
const workspaceDir = path.join(directory, "workspace");
|
||||
const notes = path.join(workspaceDir, "notes");
|
||||
await fs.mkdir(path.join(workspaceDir, "memory"), { recursive: true });
|
||||
await fs.mkdir(notes);
|
||||
await fs.writeFile(path.join(notes, "note.md"), "linked notes");
|
||||
await fs.symlink(notes, path.join(workspaceDir, "memory", "alias"), "junction");
|
||||
const relPath = "memory/alias/note.md";
|
||||
await expect(readMemoryFile({ workspaceDir, relPath })).resolves.toMatchObject({
|
||||
status: "ok",
|
||||
text: "linked notes",
|
||||
path: relPath,
|
||||
});
|
||||
await expect(
|
||||
readMemoryFile({
|
||||
workspaceDir: path.join(directory, "other-workspace"),
|
||||
extraPaths: [workspaceDir],
|
||||
relPath: path.join(workspaceDir, relPath),
|
||||
}),
|
||||
).rejects.toMatchObject({ code: "MEMORY_PATH_NOT_ALLOWED" });
|
||||
});
|
||||
|
||||
it.each(["EAGAIN", "EIO"])(
|
||||
"preserves read-time %s handling for workspace memory",
|
||||
async (code) => {
|
||||
const workspaceDir = tempDirs.make("memory-read-operational-");
|
||||
const relPath = "memory/note.md";
|
||||
const absolutePath = path.join(workspaceDir, relPath);
|
||||
await fs.mkdir(path.dirname(absolutePath));
|
||||
await fs.writeFile(absolutePath, "memory contents");
|
||||
const failure = Object.assign(new Error(`${code}: read metadata unavailable`), { code });
|
||||
const faultSeen = createDeferred();
|
||||
let reading = false;
|
||||
let injected = false;
|
||||
const retry = memoryReadRetry.retryTransientMemoryRead;
|
||||
const retrySpy = vi
|
||||
.spyOn(memoryReadRetry, "retryTransientMemoryRead")
|
||||
.mockImplementation((read, label) =>
|
||||
retry(async () => {
|
||||
reading = true;
|
||||
return await read();
|
||||
}, label),
|
||||
);
|
||||
const lstat = fsSync.lstatSync;
|
||||
const statSpy = vi.spyOn(fsSync, "lstatSync").mockImplementation((...args) => {
|
||||
if (reading && !injected && path.resolve(String(args[0])) === absolutePath) {
|
||||
injected = true;
|
||||
faultSeen.resolve();
|
||||
throw failure;
|
||||
}
|
||||
return lstat(...args);
|
||||
});
|
||||
vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] });
|
||||
try {
|
||||
const result = readMemoryFile({ workspaceDir, relPath }).then(
|
||||
(value) => ({ status: "fulfilled" as const, value }),
|
||||
(error: unknown) => ({ status: "rejected" as const, error }),
|
||||
);
|
||||
await Promise.race([faultSeen.promise, result]);
|
||||
await vi.runAllTimersAsync();
|
||||
const outcome = await result;
|
||||
if (code === "EAGAIN") {
|
||||
expect(outcome).toEqual({
|
||||
status: "fulfilled",
|
||||
value: { status: "ok", text: "memory contents", path: relPath, from: 1, lines: 1 },
|
||||
});
|
||||
} else {
|
||||
expect(outcome).toEqual({ status: "rejected", error: failure });
|
||||
}
|
||||
expect(injected).toBe(true);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
statSpy.mockRestore();
|
||||
retrySpy.mockRestore();
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
it.each(["workspace", "extra directory"])(
|
||||
"retains the authorized %s when its pathname is replaced before reading",
|
||||
async (source) => {
|
||||
const directory = tempDirs.make("memory-read-root-replacement-");
|
||||
const workspaceDir = path.join(directory, "workspace");
|
||||
const authorized = source === "workspace" ? workspaceDir : path.join(directory, "extra");
|
||||
const outside = path.join(directory, "outside");
|
||||
const moved = path.join(directory, "moved");
|
||||
const filename = source === "workspace" ? "memory/note.md" : "note.md";
|
||||
await fs.mkdir(workspaceDir);
|
||||
await fs.mkdir(path.dirname(path.join(authorized, filename)), { recursive: true });
|
||||
await fs.mkdir(path.dirname(path.join(outside, filename)), { recursive: true });
|
||||
await fs.writeFile(path.join(authorized, filename), "authorized contents");
|
||||
await fs.writeFile(path.join(outside, filename), "outside contents");
|
||||
const retry = memoryReadRetry.retryTransientMemoryRead;
|
||||
const spy = vi
|
||||
.spyOn(memoryReadRetry, "retryTransientMemoryRead")
|
||||
.mockImplementation(async (read, label) => {
|
||||
await fs.rename(authorized, moved);
|
||||
await fs.symlink(outside, authorized, "junction");
|
||||
return await retry(read, label);
|
||||
});
|
||||
try {
|
||||
const absolutePath = path.join(authorized, filename);
|
||||
await expect(
|
||||
readMemoryFile({
|
||||
workspaceDir,
|
||||
extraPaths: source === "workspace" ? [] : [authorized],
|
||||
relPath: absolutePath,
|
||||
}),
|
||||
).resolves.toEqual({
|
||||
status: "not_found",
|
||||
text: "",
|
||||
path: path.relative(workspaceDir, absolutePath).replace(/\\/g, "/"),
|
||||
});
|
||||
expect(await fs.realpath(authorized)).toBe(await fs.realpath(outside));
|
||||
expect(await fs.readFile(path.join(moved, filename), "utf8")).toBe("authorized contents");
|
||||
expect(await fs.readFile(path.join(outside, filename), "utf8")).toBe("outside contents");
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
it("reads an allowed hardlinked memory file larger than the default Root byte limit", async () => {
|
||||
const directory = tempDirs.make("memory-read-large-hardlink-");
|
||||
const workspaceDir = path.join(directory, "workspace");
|
||||
await fs.mkdir(path.join(workspaceDir, "memory"), { recursive: true });
|
||||
const source = path.join(directory, "source.md");
|
||||
await fs.writeFile(
|
||||
source,
|
||||
Buffer.concat([Buffer.from("first line\n"), Buffer.alloc(17 * 1024 * 1024, 120)]),
|
||||
);
|
||||
await fs.link(source, path.join(workspaceDir, "memory", "large.md"));
|
||||
await expect(
|
||||
readMemoryFile({ workspaceDir, relPath: "memory/large.md", lines: 1 }),
|
||||
).resolves.toMatchObject({
|
||||
status: "ok",
|
||||
text: "first line\n\n[More content available. Use from=2 to continue.]",
|
||||
path: "memory/large.md",
|
||||
lines: 1,
|
||||
nextFrom: 2,
|
||||
});
|
||||
});
|
||||
|
||||
it("returns not found for absent extra paths and rejects non-directory parents", async () => {
|
||||
const tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), "memory-read-file-"));
|
||||
try {
|
||||
|
|
@ -201,7 +350,7 @@ describe("readMemoryFile", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it.each(["runbooks", "..notes", "...notes"])(
|
||||
it.each(["runbooks", "..notes", "...notes", "~"])(
|
||||
"enforces %s glob patterns through agent reads",
|
||||
async (directory) => {
|
||||
const tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), "memory-read-file-"));
|
||||
|
|
|
|||
|
|
@ -7,15 +7,7 @@ import {
|
|||
type OpenClawConfig,
|
||||
} from "./config-utils.js";
|
||||
import { isExplicitExtraMarkdownFilePath } from "./explicit-extra-markdown.js";
|
||||
import {
|
||||
assertNoSymlinkParents,
|
||||
isFileMissingError,
|
||||
isPathInside,
|
||||
isPathInsideWithRealpath,
|
||||
readRegularFile,
|
||||
root,
|
||||
statRegularFile,
|
||||
} from "./fs-utils.js";
|
||||
import { isFileMissingError, isPathInside, root } from "./fs-utils.js";
|
||||
import {
|
||||
isMemoryPath,
|
||||
matchesExtraMemoryPathEntry,
|
||||
|
|
@ -38,36 +30,6 @@ function memoryPathNotAllowed(): Error {
|
|||
});
|
||||
}
|
||||
|
||||
/** Check that an absolute path stays inside an allowed extra directory without symlink escapes. */
|
||||
async function isAllowedAdditionalDirectoryPath(
|
||||
additionalPath: string,
|
||||
absPath: string,
|
||||
): Promise<boolean> {
|
||||
if (!isPathInside(additionalPath, absPath)) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
await assertNoSymlinkParents({ rootDir: additionalPath, targetPath: absPath });
|
||||
} catch (err) {
|
||||
if (err instanceof Error && "code" in err && !isFileMissingError(err)) {
|
||||
throw err;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
if (!isPathInsideWithRealpath(additionalPath, absPath)) {
|
||||
try {
|
||||
await fs.lstat(absPath);
|
||||
} catch (err) {
|
||||
if (isFileMissingError(err)) {
|
||||
return true;
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/** Return true when a file vanished after path validation but before content read. */
|
||||
function isFileDisappearedDuringReadError(err: unknown): boolean {
|
||||
return (
|
||||
|
|
@ -101,9 +63,72 @@ export async function readMemoryFile(params: {
|
|||
const relPath = path.relative(params.workspaceDir, absPath).replace(/\\/g, "/");
|
||||
const inWorkspace = relPath.length > 0 && !relPath.startsWith("..") && !path.isAbsolute(relPath);
|
||||
const allowedWorkspace = inWorkspace && isMemoryPath(relPath);
|
||||
let allowedAdditional: false | "directory" | "file" = false;
|
||||
const notFound = (): MemoryReadResult => ({ status: "not_found", text: "", path: relPath });
|
||||
const readFromRoot = async (directory: string): Promise<MemoryReadResult> => {
|
||||
const filesystem = await root(directory, {
|
||||
hardlinks: "allow",
|
||||
maxBytes: Infinity,
|
||||
symlinks: allowedWorkspace ? "follow-parents-within-root" : "reject",
|
||||
});
|
||||
let content: string;
|
||||
try {
|
||||
content = (
|
||||
await retryTransientMemoryRead(async () => {
|
||||
try {
|
||||
return await filesystem.read(`./${path.relative(directory, absPath)}`);
|
||||
} catch (err) {
|
||||
// Keep read-time I/O errors visible to the existing retry predicate.
|
||||
if (
|
||||
err instanceof Error &&
|
||||
"code" in err &&
|
||||
err.code === "outside-workspace" &&
|
||||
err.cause instanceof Error &&
|
||||
"code" in err.cause &&
|
||||
!isFileMissingError(err.cause) &&
|
||||
err.cause.code !== "ELOOP"
|
||||
) {
|
||||
throw err.cause;
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}, `read memory file ${absPath}`)
|
||||
).buffer.toString("utf-8");
|
||||
} catch (err) {
|
||||
const code = err && typeof err === "object" && "code" in err ? err.code : undefined;
|
||||
if (code === "not-file") {
|
||||
throw new Error("path must be a regular file", { cause: err });
|
||||
}
|
||||
// Missing leaves return not_found; non-directory extra-path parents are not authorized.
|
||||
if (code !== "ENOTDIR" && isFileDisappearedDuringReadError(err)) {
|
||||
return notFound();
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
return buildMemoryReadResult({
|
||||
content,
|
||||
relPath,
|
||||
from: params.from,
|
||||
lines: params.lines,
|
||||
defaultLines: params.defaultLines ?? DEFAULT_MEMORY_READ_LINES,
|
||||
maxChars: params.maxChars,
|
||||
suggestReadFallback: allowedWorkspace,
|
||||
});
|
||||
};
|
||||
if (allowedWorkspace) {
|
||||
if (!absPath.endsWith(".md")) {
|
||||
throw memoryPathNotAllowed();
|
||||
}
|
||||
try {
|
||||
return await readFromRoot(params.workspaceDir);
|
||||
} catch (err) {
|
||||
if (isFileMissingError(err)) {
|
||||
return notFound();
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
let additionalPathError: Error | undefined;
|
||||
if (!allowedWorkspace && (params.extraPaths?.length ?? 0) > 0) {
|
||||
if ((params.extraPaths?.length ?? 0) > 0) {
|
||||
const additionalPaths = normalizeExtraMemoryPathEntries(params.workspaceDir, params.extraPaths);
|
||||
for (const additionalPath of additionalPaths) {
|
||||
const matchesFile =
|
||||
|
|
@ -120,77 +145,27 @@ export async function readMemoryFile(params: {
|
|||
if (stat.isSymbolicLink()) {
|
||||
continue;
|
||||
}
|
||||
if (stat.isDirectory()) {
|
||||
if (
|
||||
matchesDirectory &&
|
||||
(await isAllowedAdditionalDirectoryPath(additionalPath.path, absPath))
|
||||
) {
|
||||
const candidateStat = await fs.lstat(absPath).catch(() => null);
|
||||
if (candidateStat?.isSymbolicLink()) {
|
||||
continue;
|
||||
}
|
||||
allowedAdditional = "directory";
|
||||
break;
|
||||
}
|
||||
continue;
|
||||
if (stat.isDirectory() && matchesDirectory) {
|
||||
return await readFromRoot(additionalPath.path);
|
||||
}
|
||||
if (stat.isFile() && matchesFile) {
|
||||
allowedAdditional = "file";
|
||||
break;
|
||||
return await readFromRoot(path.dirname(additionalPath.path));
|
||||
}
|
||||
} catch (err) {
|
||||
if (err instanceof Error && !isFileMissingError(err)) {
|
||||
const code = err && typeof err === "object" && "code" in err ? err.code : undefined;
|
||||
if (
|
||||
err instanceof Error &&
|
||||
!isFileMissingError(err) &&
|
||||
code !== "symlink" &&
|
||||
code !== "outside-workspace"
|
||||
) {
|
||||
// Another configured root may still authorize this file.
|
||||
additionalPathError ??= err;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!allowedWorkspace && !allowedAdditional) {
|
||||
throw additionalPathError ?? memoryPathNotAllowed();
|
||||
}
|
||||
if (!absPath.endsWith(".md") && allowedAdditional !== "file") {
|
||||
throw memoryPathNotAllowed();
|
||||
}
|
||||
if (allowedWorkspace) {
|
||||
try {
|
||||
// Workspace reads use the safe fs root so symlink escapes are rejected before file IO.
|
||||
const workspaceRoot = await root(params.workspaceDir);
|
||||
await workspaceRoot.resolve(relPath);
|
||||
} catch (err) {
|
||||
if (isFileMissingError(err)) {
|
||||
return { status: "not_found", text: "", path: relPath };
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
const statResult = await statRegularFile(absPath);
|
||||
if (statResult.missing) {
|
||||
return { status: "not_found", text: "", path: relPath };
|
||||
}
|
||||
let content: string;
|
||||
try {
|
||||
content = (
|
||||
await retryTransientMemoryRead(
|
||||
() => readRegularFile({ filePath: absPath }),
|
||||
`read memory file ${absPath}`,
|
||||
)
|
||||
).buffer.toString("utf-8");
|
||||
} catch (err) {
|
||||
if (isFileDisappearedDuringReadError(err)) {
|
||||
return { status: "not_found", text: "", path: relPath };
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
return buildMemoryReadResult({
|
||||
content,
|
||||
relPath,
|
||||
from: params.from,
|
||||
lines: params.lines,
|
||||
defaultLines: params.defaultLines ?? DEFAULT_MEMORY_READ_LINES,
|
||||
maxChars: params.maxChars,
|
||||
suggestReadFallback: allowedWorkspace,
|
||||
});
|
||||
throw additionalPathError ?? memoryPathNotAllowed();
|
||||
}
|
||||
|
||||
/** Resolve agent memory config and read one memory file for that agent. */
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue