fix(security): harden fs-safe copy writes

This commit is contained in:
Peter Steinberger
2026-03-07 19:10:19 +00:00
parent 6bfae2714f
commit 74ecdec9ba
2 changed files with 99 additions and 4 deletions

View File

@@ -300,6 +300,66 @@ describe("fs-safe", () => {
},
);
it("does not truncate existing target when atomic copy rename fails", async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const sourceDir = await tempDirs.make("openclaw-fs-safe-source-");
const sourcePath = path.join(sourceDir, "in.txt");
const targetPath = path.join(root, "nested", "copied.txt");
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(sourcePath, "copy-new");
await fs.writeFile(targetPath, "copy-existing");
const renameSpy = vi
.spyOn(fs, "rename")
.mockRejectedValue(Object.assign(new Error("rename blocked"), { code: "EACCES" }));
try {
await expect(
copyFileWithinRoot({
sourcePath,
rootDir: root,
relativePath: "nested/copied.txt",
}),
).rejects.toMatchObject({ code: "EACCES" });
} finally {
renameSpy.mockRestore();
}
await expect(fs.readFile(targetPath, "utf8")).resolves.toBe("copy-existing");
});
it.runIf(process.platform !== "win32")(
"rejects when a hardlink appears after atomic copy rename",
async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const sourceDir = await tempDirs.make("openclaw-fs-safe-source-");
const sourcePath = path.join(sourceDir, "copy-source.txt");
const targetPath = path.join(root, "nested", "copied.txt");
const aliasPath = path.join(root, "nested", "alias.txt");
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(sourcePath, "copy-new");
await fs.writeFile(targetPath, "copy-existing");
const realRename = fs.rename.bind(fs);
let linked = false;
const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (...args) => {
await realRename(...args);
if (!linked) {
linked = true;
await fs.link(String(args[1]), aliasPath);
}
});
try {
await expect(
copyFileWithinRoot({
sourcePath,
rootDir: root,
relativePath: "nested/copied.txt",
}),
).rejects.toMatchObject({ code: "invalid-path" });
} finally {
renameSpy.mockRestore();
}
await expect(fs.readFile(aliasPath, "utf8")).resolves.toBe("copy-new");
},
);
it("copies a file within root safely", async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const sourceDir = await tempDirs.make("openclaw-fs-safe-source-");

View File

@@ -554,32 +554,67 @@ export async function copyFileWithinRoot(params: {
let target: SafeWritableOpenResult | null = null;
let sourceClosedByStream = false;
let targetClosedByStream = false;
let targetClosedByUs = false;
let tempHandle: FileHandle | null = null;
let tempPath: string | null = null;
let tempClosedByStream = false;
try {
target = await openWritableFileWithinRoot({
rootDir: params.rootDir,
relativePath: params.relativePath,
mkdir: params.mkdir,
truncateExisting: false,
});
const destinationPath = target.openedRealPath;
const targetMode = target.openedStat.mode & 0o777;
await target.handle.close().catch(() => {});
targetClosedByUs = true;
tempPath = buildAtomicWriteTempPath(destinationPath);
tempHandle = await fs.open(tempPath, OPEN_WRITE_CREATE_FLAGS, targetMode || 0o600);
const sourceStream = source.handle.createReadStream();
const targetStream = target.handle.createWriteStream();
const targetStream = tempHandle.createWriteStream();
sourceStream.once("close", () => {
sourceClosedByStream = true;
});
targetStream.once("close", () => {
targetClosedByStream = true;
tempClosedByStream = true;
});
await pipeline(sourceStream, targetStream);
const writtenStat = await fs.stat(tempPath);
if (!tempClosedByStream) {
await tempHandle.close().catch(() => {});
tempClosedByStream = true;
}
tempHandle = null;
await fs.rename(tempPath, destinationPath);
tempPath = null;
try {
await verifyAtomicWriteResult({
rootDir: params.rootDir,
targetPath: destinationPath,
expectedStat: writtenStat,
});
} catch (err) {
emitWriteBoundaryWarning(`post-copy verification failed: ${String(err)}`);
throw err;
}
} catch (err) {
if (target?.createdForWrite) {
await fs.rm(target.openedRealPath, { force: true }).catch(() => {});
}
throw err;
} finally {
if (tempPath) {
await fs.rm(tempPath, { force: true }).catch(() => {});
}
if (!sourceClosedByStream) {
await source.handle.close().catch(() => {});
}
if (target && !targetClosedByStream) {
if (tempHandle && !tempClosedByStream) {
await tempHandle.close().catch(() => {});
}
if (target && !targetClosedByUs) {
await target.handle.close().catch(() => {});
}
}