fix(security): harden install base drift cleanup

2026-03-07 22:44:16 +00:00 · 2026-03-07 19:22:56 +00:00
parent c06014d50c
commit d72734946a
2 changed files with 220 additions and 21 deletions
--- a/src/infra/install-package-dir.test.ts
+++ b/src/infra/install-package-dir.test.ts
@@ -11,6 +11,58 @@ async function listMatchingDirs(root: string, prefix: string): Promise<string[]>
    .map((entry) => entry.name);
 }

+function normalizeDarwinTmpPath(filePath: string): string {
+  return process.platform === "darwin" && filePath.startsWith("/private/var/")
+    ? filePath.slice("/private".length)
+    : filePath;
+}
+
+async function rebindInstallBasePath(params: {
+  installBaseDir: string;
+  preservedDir: string;
+  outsideTarget: string;
+}): Promise<void> {
+  await fs.rename(params.installBaseDir, params.preservedDir);
+  await fs.symlink(
+    params.outsideTarget,
+    params.installBaseDir,
+    process.platform === "win32" ? "junction" : undefined,
+  );
+}
+
+async function withInstallBaseReboundOnRealpathCall<T>(params: {
+  installBaseDir: string;
+  preservedDir: string;
+  outsideTarget: string;
+  rebindAtCall: number;
+  run: () => Promise<T>;
+}): Promise<T> {
+  const installBasePath = normalizeDarwinTmpPath(path.resolve(params.installBaseDir));
+  const realRealpath = fs.realpath.bind(fs);
+  let installBaseRealpathCalls = 0;
+  const realpathSpy = vi
+    .spyOn(fs, "realpath")
+    .mockImplementation(async (...args: Parameters<typeof fs.realpath>) => {
+      const filePath = normalizeDarwinTmpPath(path.resolve(String(args[0])));
+      if (filePath === installBasePath) {
+        installBaseRealpathCalls += 1;
+        if (installBaseRealpathCalls === params.rebindAtCall) {
+          await rebindInstallBasePath({
+            installBaseDir: params.installBaseDir,
+            preservedDir: params.preservedDir,
+            outsideTarget: params.outsideTarget,
+          });
+        }
+      }
+      return await realRealpath(...args);
+    });
+  try {
+    return await params.run();
+  } finally {
+    realpathSpy.mockRestore();
+  }
+}
+
 describe("installPackageDir", () => {
  let fixtureRoot = "";

@@ -103,4 +155,97 @@ describe("installPackageDir", () => {
    const backupRoot = path.join(installBaseDir, ".openclaw-install-backups");
    await expect(fs.readdir(backupRoot)).resolves.toHaveLength(0);
  });
+
+  it("aborts without outside writes when the install base is rebound before publish", async () => {
+    fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-package-dir-"));
+    const sourceDir = path.join(fixtureRoot, "source");
+    const installBaseDir = path.join(fixtureRoot, "plugins");
+    const preservedInstallRoot = path.join(fixtureRoot, "plugins-preserved");
+    const outsideInstallRoot = path.join(fixtureRoot, "outside-plugins");
+    const targetDir = path.join(installBaseDir, "demo");
+    await fs.mkdir(sourceDir, { recursive: true });
+    await fs.mkdir(installBaseDir, { recursive: true });
+    await fs.mkdir(outsideInstallRoot, { recursive: true });
+    await fs.writeFile(path.join(sourceDir, "marker.txt"), "new");
+
+    const warnings: string[] = [];
+    await withInstallBaseReboundOnRealpathCall({
+      installBaseDir,
+      preservedDir: preservedInstallRoot,
+      outsideTarget: outsideInstallRoot,
+      rebindAtCall: 3,
+      run: async () => {
+        await expect(
+          installPackageDir({
+            sourceDir,
+            targetDir,
+            mode: "install",
+            timeoutMs: 1_000,
+            copyErrorPrefix: "failed to copy plugin",
+            hasDeps: false,
+            depsLogMessage: "Installing deps…",
+            logger: { warn: (message) => warnings.push(message) },
+          }),
+        ).resolves.toEqual({
+          ok: false,
+          error: "failed to copy plugin: Error: install base directory changed during install",
+        });
+      },
+    });
+
+    await expect(
+      fs.stat(path.join(outsideInstallRoot, "demo", "marker.txt")),
+    ).rejects.toMatchObject({
+      code: "ENOENT",
+    });
+    expect(warnings).toContain(
+      "Install base directory changed during install; aborting staged publish.",
+    );
+  });
+
+  it("warns and leaves the backup in place when the install base changes before backup cleanup", async () => {
+    fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-package-dir-"));
+    const sourceDir = path.join(fixtureRoot, "source");
+    const installBaseDir = path.join(fixtureRoot, "plugins");
+    const preservedInstallRoot = path.join(fixtureRoot, "plugins-preserved");
+    const outsideInstallRoot = path.join(fixtureRoot, "outside-plugins");
+    const targetDir = path.join(installBaseDir, "demo");
+    await fs.mkdir(sourceDir, { recursive: true });
+    await fs.mkdir(installBaseDir, { recursive: true });
+    await fs.mkdir(outsideInstallRoot, { recursive: true });
+    await fs.mkdir(path.join(installBaseDir, "demo"), { recursive: true });
+    await fs.writeFile(path.join(installBaseDir, "demo", "marker.txt"), "old");
+    await fs.writeFile(path.join(sourceDir, "marker.txt"), "new");
+
+    const warnings: string[] = [];
+    const result = await withInstallBaseReboundOnRealpathCall({
+      installBaseDir,
+      preservedDir: preservedInstallRoot,
+      outsideTarget: outsideInstallRoot,
+      rebindAtCall: 7,
+      run: async () =>
+        await installPackageDir({
+          sourceDir,
+          targetDir,
+          mode: "update",
+          timeoutMs: 1_000,
+          copyErrorPrefix: "failed to copy plugin",
+          hasDeps: false,
+          depsLogMessage: "Installing deps…",
+          logger: { warn: (message) => warnings.push(message) },
+        }),
+    });
+
+    expect(result).toEqual({ ok: true });
+    expect(warnings).toContain(
+      "Install base directory changed before backup cleanup; leaving backup in place.",
+    );
+    await expect(
+      fs.stat(path.join(outsideInstallRoot, "demo", "marker.txt")),
+    ).rejects.toMatchObject({
+      code: "ENOENT",
+    });
+    const backupRoot = path.join(preservedInstallRoot, ".openclaw-install-backups");
+    await expect(fs.readdir(backupRoot)).resolves.toHaveLength(1);
+  });
 });
--- a/src/infra/install-package-dir.ts
+++ b/src/infra/install-package-dir.ts
@@ -4,6 +4,12 @@ import { runCommandWithTimeout } from "../process/exec.js";
 import { fileExists } from "./archive.js";
 import { assertCanonicalPathWithinBase } from "./install-safe-path.js";

+const INSTALL_BASE_CHANGED_ERROR_MESSAGE = "install base directory changed during install";
+const INSTALL_BASE_CHANGED_ABORT_WARNING =
+  "Install base directory changed during install; aborting staged publish.";
+const INSTALL_BASE_CHANGED_BACKUP_WARNING =
+  "Install base directory changed before backup cleanup; leaving backup in place.";
+
 function isObjectRecord(value: unknown): value is Record<string, unknown> {
  return Boolean(value) && typeof value === "object" && !Array.isArray(value);
 }
@@ -68,26 +74,54 @@ function isRelativePathInsideBase(relativePath: string): boolean {
  );
 }

+function isInstallBaseChangedError(error: unknown): boolean {
+  return error instanceof Error && error.message === INSTALL_BASE_CHANGED_ERROR_MESSAGE;
+}
+
 async function assertInstallBaseStable(params: {
  installBaseDir: string;
  expectedRealPath: string;
 }): Promise<void> {
  const baseLstat = await fs.lstat(params.installBaseDir);
  if (!baseLstat.isDirectory() || baseLstat.isSymbolicLink()) {
-    throw new Error("install base directory changed during install");
+    throw new Error(INSTALL_BASE_CHANGED_ERROR_MESSAGE);
  }
  const currentRealPath = await fs.realpath(params.installBaseDir);
  if (currentRealPath !== params.expectedRealPath) {
-    throw new Error("install base directory changed during install");
+    throw new Error(INSTALL_BASE_CHANGED_ERROR_MESSAGE);
  }
 }

+async function cleanupInstallTempDir(dirPath: string | null): Promise<void> {
+  if (!dirPath) {
+    return;
+  }
+  await fs.rm(dirPath, { recursive: true, force: true }).catch(() => undefined);
+}
+
+async function resolveInstallPublishTarget(params: {
+  installBaseDir: string;
+  targetDir: string;
+}): Promise<{ installBaseRealPath: string; canonicalTargetDir: string }> {
+  const installBaseResolved = path.resolve(params.installBaseDir);
+  const targetResolved = path.resolve(params.targetDir);
+  const targetRelativePath = path.relative(installBaseResolved, targetResolved);
+  if (!isRelativePathInsideBase(targetRelativePath)) {
+    throw new Error("invalid install target path");
+  }
+  const installBaseRealPath = await fs.realpath(params.installBaseDir);
+  return {
+    installBaseRealPath,
+    canonicalTargetDir: path.join(installBaseRealPath, targetRelativePath),
+  };
+}
+
 export async function installPackageDir(params: {
  sourceDir: string;
  targetDir: string;
  mode: "install" | "update";
  timeoutMs: number;
-  logger?: { info?: (message: string) => void };
+  logger?: { info?: (message: string) => void; warn?: (message: string) => void };
  copyErrorPrefix: string;
  hasDeps: boolean;
  depsLogMessage: string;
@@ -100,22 +134,29 @@ export async function installPackageDir(params: {
    installBaseDir,
    candidatePaths: [params.targetDir],
  });
-  const installBaseResolved = path.resolve(installBaseDir);
-  const targetResolved = path.resolve(params.targetDir);
-  const targetRelativePath = path.relative(installBaseResolved, targetResolved);
-  if (!isRelativePathInsideBase(targetRelativePath)) {
-    return { ok: false, error: `${params.copyErrorPrefix}: Error: invalid install target path` };
+  let installBaseRealPath: string;
+  let canonicalTargetDir: string;
+  try {
+    ({ installBaseRealPath, canonicalTargetDir } = await resolveInstallPublishTarget({
+      installBaseDir,
+      targetDir: params.targetDir,
+    }));
+  } catch (err) {
+    return { ok: false, error: `${params.copyErrorPrefix}: ${String(err)}` };
  }
-  const installBaseRealPath = await fs.realpath(installBaseDir);
-  const canonicalTargetDir = path.join(installBaseRealPath, targetRelativePath);

  let stageDir: string | null = null;
  let backupDir: string | null = null;
-  const fail = async (error: string) => {
-    await restoreBackup();
-    if (stageDir) {
-      await fs.rm(stageDir, { recursive: true, force: true }).catch(() => undefined);
-      stageDir = null;
+  const fail = async (error: string, cause?: unknown) => {
+    const installBaseChanged = isInstallBaseChangedError(cause);
+    if (installBaseChanged) {
+      params.logger?.warn?.(INSTALL_BASE_CHANGED_ABORT_WARNING);
+    } else {
+      await restoreBackup();
+      if (stageDir) {
+        await cleanupInstallTempDir(stageDir);
+        stageDir = null;
+      }
    }
    return { ok: false as const, error };
  };
@@ -135,13 +176,13 @@ export async function installPackageDir(params: {
    stageDir = await fs.mkdtemp(path.join(installBaseRealPath, ".openclaw-install-stage-"));
    await fs.cp(params.sourceDir, stageDir, { recursive: true });
  } catch (err) {
-    return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
+    return await fail(`${params.copyErrorPrefix}: ${String(err)}`, err);
  }

  try {
    await params.afterCopy?.(stageDir);
  } catch (err) {
-    return await fail(`post-copy validation failed: ${String(err)}`);
+    return await fail(`post-copy validation failed: ${String(err)}`, err);
  }

  if (params.hasDeps) {
@@ -174,7 +215,7 @@ export async function installPackageDir(params: {
      });
      await fs.rename(canonicalTargetDir, backupDir);
    } catch (err) {
-      return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
+      return await fail(`${params.copyErrorPrefix}: ${String(err)}`, err);
    }
  }

@@ -186,14 +227,27 @@ export async function installPackageDir(params: {
    await fs.rename(stageDir, canonicalTargetDir);
    stageDir = null;
  } catch (err) {
-    return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
+    return await fail(`${params.copyErrorPrefix}: ${String(err)}`, err);
  }

+  if (backupDir) {
+    try {
+      await assertInstallBaseStable({
+        installBaseDir,
+        expectedRealPath: installBaseRealPath,
+      });
+    } catch (err) {
+      if (isInstallBaseChangedError(err)) {
+        params.logger?.warn?.(INSTALL_BASE_CHANGED_BACKUP_WARNING);
+      }
+      backupDir = null;
+    }
+  }
  if (backupDir) {
    await fs.rm(backupDir, { recursive: true, force: true }).catch(() => undefined);
  }
  if (stageDir) {
-    await fs.rm(stageDir, { recursive: true, force: true }).catch(() => undefined);
+    await cleanupInstallTempDir(stageDir);
  }

  return { ok: true };
@@ -204,7 +258,7 @@ export async function installPackageDirWithManifestDeps(params: {
  targetDir: string;
  mode: "install" | "update";
  timeoutMs: number;
-  logger?: { info?: (message: string) => void };
+  logger?: { info?: (message: string) => void; warn?: (message: string) => void };
  copyErrorPrefix: string;
  depsLogMessage: string;
  manifestDependencies?: Record<string, unknown>;