fix(security): harden fs-safe copy writes

steipete · steipete · commit 74ecdec9bab0 · 2026-03-07T19:10:27.000Z
diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts
@@ -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-");
diff --git a/src/infra/fs-safe.ts b/src/infra/fs-safe.ts
@@ -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(() => {});
     }
   }
