fix(security): stage installs before publish

CHANGELOG.md

@@ -68,6 +68,7 @@ Docs: https://docs.openclaw.ai
 - Gateway/remote WS break-glass hostname support: honor `OPENCLAW_ALLOW_INSECURE_PRIVATE_WS=1` for `ws://` hostname URLs (not only private IP literals) across onboarding validation and runtime gateway connection checks, while still rejecting public IP literals and non-unicast IPv6 endpoints. (#36930) Thanks @manju-rn.
 - Routing/binding lookup scalability: pre-index route bindings by channel/account and avoid full binding-list rescans on channel-account cache rollover, preventing multi-second `resolveAgentRoute` stalls in large binding configurations. (#36915) Thanks @songchenghao.
 - Browser/session cleanup: track browser tabs opened by session-scoped browser tool runs and close tracked tabs during `sessions.reset`/`sessions.delete` runtime cleanup, preventing orphaned tabs and unbounded browser memory growth after session teardown. (#36666) Thanks @Harnoor6693.
+- Plugin/hook install rollback hardening: stage installs under the canonical install base, validate and run dependency installs before publish, and restore updates by rename instead of deleting the target path, reducing partial-replace and symlink-rebind risk during install failures.
 - Slack/local file upload allowlist parity: propagate `mediaLocalRoots` through the Slack send action pipeline so workspace-rooted attachments pass `assertLocalMediaAllowed` checks while non-allowlisted paths remain blocked. (synthesis: #36656; overlap considered from #36516, #36496, #36493, #36484, #32648, #30888) Thanks @2233admin.
 - Agents/compaction safeguard pre-check: skip embedded compaction before entering the Pi SDK when a session has no real conversation messages, avoiding unnecessary LLM API calls on idle sessions. (#36451) thanks @Sid-Qin.
 - Config/schema cache key stability: build merged schema cache keys with incremental hashing to avoid large single-string serialization and prevent `RangeError: Invalid string length` on high-cardinality plugin/channel metadata. (#36603) Thanks @powermaster888.

src/infra/install-package-dir.test.ts

@@ -0,0 +1,106 @@
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { installPackageDir } from "./install-package-dir.js";
+
+async function listMatchingDirs(root: string, prefix: string): Promise<string[]> {
+  const entries = await fs.readdir(root, { withFileTypes: true });
+  return entries
+    .filter((entry) => entry.isDirectory() && entry.name.startsWith(prefix))
+    .map((entry) => entry.name);
+}
+
+describe("installPackageDir", () => {
+  let fixtureRoot = "";
+
+  afterEach(async () => {
+    vi.restoreAllMocks();
+    if (fixtureRoot) {
+      await fs.rm(fixtureRoot, { recursive: true, force: true });
+      fixtureRoot = "";
+    }
+  });
+
+  it("keeps the existing install in place when staged validation fails", async () => {
+    fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-package-dir-"));
+    const installBaseDir = path.join(fixtureRoot, "plugins");
+    const sourceDir = path.join(fixtureRoot, "source");
+    const targetDir = path.join(installBaseDir, "demo");
+    await fs.mkdir(sourceDir, { recursive: true });
+    await fs.mkdir(targetDir, { recursive: true });
+    await fs.writeFile(path.join(sourceDir, "marker.txt"), "new");
+    await fs.writeFile(path.join(targetDir, "marker.txt"), "old");
+
+    const result = await installPackageDir({
+      sourceDir,
+      targetDir,
+      mode: "update",
+      timeoutMs: 1_000,
+      copyErrorPrefix: "failed to copy plugin",
+      hasDeps: false,
+      depsLogMessage: "Installing deps…",
+      afterCopy: async (installedDir) => {
+        expect(installedDir).not.toBe(targetDir);
+        await expect(fs.readFile(path.join(installedDir, "marker.txt"), "utf8")).resolves.toBe(
+          "new",
+        );
+        throw new Error("validation boom");
+      },
+    });
+
+    expect(result).toEqual({
+      ok: false,
+      error: "post-copy validation failed: Error: validation boom",
+    });
+    await expect(fs.readFile(path.join(targetDir, "marker.txt"), "utf8")).resolves.toBe("old");
+    await expect(
+      listMatchingDirs(installBaseDir, ".openclaw-install-stage-"),
+    ).resolves.toHaveLength(0);
+    await expect(
+      listMatchingDirs(installBaseDir, ".openclaw-install-backups"),
+    ).resolves.toHaveLength(0);
+  });
+
+  it("restores the original install if publish rename fails", async () => {
+    fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-install-package-dir-"));
+    const installBaseDir = path.join(fixtureRoot, "plugins");
+    const sourceDir = path.join(fixtureRoot, "source");
+    const targetDir = path.join(installBaseDir, "demo");
+    await fs.mkdir(sourceDir, { recursive: true });
+    await fs.mkdir(targetDir, { recursive: true });
+    await fs.writeFile(path.join(sourceDir, "marker.txt"), "new");
+    await fs.writeFile(path.join(targetDir, "marker.txt"), "old");
+
+    const realRename = fs.rename.bind(fs);
+    let renameCalls = 0;
+    vi.spyOn(fs, "rename").mockImplementation(async (...args: Parameters<typeof fs.rename>) => {
+      renameCalls += 1;
+      if (renameCalls === 2) {
+        throw new Error("publish boom");
+      }
+      return await realRename(...args);
+    });
+
+    const result = await installPackageDir({
+      sourceDir,
+      targetDir,
+      mode: "update",
+      timeoutMs: 1_000,
+      copyErrorPrefix: "failed to copy plugin",
+      hasDeps: false,
+      depsLogMessage: "Installing deps…",
+    });
+
+    expect(result).toEqual({
+      ok: false,
+      error: "failed to copy plugin: Error: publish boom",
+    });
+    await expect(fs.readFile(path.join(targetDir, "marker.txt"), "utf8")).resolves.toBe("old");
+    await expect(
+      listMatchingDirs(installBaseDir, ".openclaw-install-stage-"),
+    ).resolves.toHaveLength(0);
+    const backupRoot = path.join(installBaseDir, ".openclaw-install-backups");
+    await expect(fs.readdir(backupRoot)).resolves.toHaveLength(0);
+  });
+});

src/infra/install-package-dir.ts

@@ -62,6 +62,26 @@ async function assertInstallBoundaryPaths(params: {
   }
 }
 
+function isRelativePathInsideBase(relativePath: string): boolean {
+  return (
+    Boolean(relativePath) && relativePath !== ".." && !relativePath.startsWith(`..${path.sep}`)
+  );
+}
+
+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");
+  }
+  const currentRealPath = await fs.realpath(params.installBaseDir);
+  if (currentRealPath !== params.expectedRealPath) {
+    throw new Error("install base directory changed during install");
+  }
+}
+
 export async function installPackageDir(params: {
   sourceDir: string;
   targetDir: string;
@@ -71,7 +91,7 @@ export async function installPackageDir(params: {
   copyErrorPrefix: string;
   hasDeps: boolean;
   depsLogMessage: string;
-  afterCopy?: () => void | Promise<void>;
+  afterCopy?: (installedDir: string) => void | Promise<void>;
 }): Promise<{ ok: true } | { ok: false; error: string }> {
   params.logger?.info?.(`Installing to ${params.targetDir}…`);
   const installBaseDir = path.dirname(params.targetDir);
@@ -80,70 +100,101 @@ export async function installPackageDir(params: {
     installBaseDir,
     candidatePaths: [params.targetDir],
   });
-  let backupDir: string | null = null;
-  if (params.mode === "update" && (await fileExists(params.targetDir))) {
-    const backupRoot = path.join(path.dirname(params.targetDir), ".openclaw-install-backups");
-    backupDir = path.join(backupRoot, `${path.basename(params.targetDir)}-${Date.now()}`);
-    await fs.mkdir(backupRoot, { recursive: true });
-    await assertInstallBoundaryPaths({
-      installBaseDir,
-      candidatePaths: [backupDir],
-    });
-    await fs.rename(params.targetDir, backupDir);
+  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` };
   }
+  const installBaseRealPath = await fs.realpath(installBaseDir);
+  const canonicalTargetDir = path.join(installBaseRealPath, targetRelativePath);
 
-  const rollback = async () => {
+  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;
+    }
+    return { ok: false as const, error };
+  };
+  const restoreBackup = async () => {
     if (!backupDir) {
       return;
     }
-    await assertInstallBoundaryPaths({
-      installBaseDir,
-      candidatePaths: [params.targetDir, backupDir],
-    });
-    await fs.rm(params.targetDir, { recursive: true, force: true }).catch(() => undefined);
-    await fs.rename(backupDir, params.targetDir).catch(() => undefined);
+    await fs.rename(backupDir, canonicalTargetDir).catch(() => undefined);
+    backupDir = null;
   };
 
   try {
     await assertInstallBoundaryPaths({
-      installBaseDir,
-      candidatePaths: [params.targetDir],
+      installBaseDir: installBaseRealPath,
+      candidatePaths: [canonicalTargetDir],
     });
-    await fs.cp(params.sourceDir, params.targetDir, { recursive: true });
+    stageDir = await fs.mkdtemp(path.join(installBaseRealPath, ".openclaw-install-stage-"));
+    await fs.cp(params.sourceDir, stageDir, { recursive: true });
   } catch (err) {
-    await rollback();
-    return { ok: false, error: `${params.copyErrorPrefix}: ${String(err)}` };
+    return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
   }
 
   try {
-    await params.afterCopy?.();
+    await params.afterCopy?.(stageDir);
   } catch (err) {
-    await rollback();
-    return { ok: false, error: `post-copy validation failed: ${String(err)}` };
+    return await fail(`post-copy validation failed: ${String(err)}`);
   }
 
   if (params.hasDeps) {
-    await sanitizeManifestForNpmInstall(params.targetDir);
+    await sanitizeManifestForNpmInstall(stageDir);
     params.logger?.info?.(params.depsLogMessage);
     const npmRes = await runCommandWithTimeout(
       ["npm", "install", "--omit=dev", "--omit=peer", "--silent", "--ignore-scripts"],
       {
         timeoutMs: Math.max(params.timeoutMs, 300_000),
-        cwd: params.targetDir,
+        cwd: stageDir,
       },
     );
     if (npmRes.code !== 0) {
-      await rollback();
-      return {
-        ok: false,
-        error: `npm install failed: ${npmRes.stderr.trim() || npmRes.stdout.trim()}`,
-      };
+      return await fail(`npm install failed: ${npmRes.stderr.trim() || npmRes.stdout.trim()}`);
     }
   }
 
+  if (params.mode === "update" && (await fileExists(canonicalTargetDir))) {
+    const backupRoot = path.join(installBaseRealPath, ".openclaw-install-backups");
+    backupDir = path.join(backupRoot, `${path.basename(canonicalTargetDir)}-${Date.now()}`);
+    try {
+      await fs.mkdir(backupRoot, { recursive: true });
+      await assertInstallBoundaryPaths({
+        installBaseDir: installBaseRealPath,
+        candidatePaths: [backupDir],
+      });
+      await assertInstallBaseStable({
+        installBaseDir,
+        expectedRealPath: installBaseRealPath,
+      });
+      await fs.rename(canonicalTargetDir, backupDir);
+    } catch (err) {
+      return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
+    }
+  }
+
+  try {
+    await assertInstallBaseStable({
+      installBaseDir,
+      expectedRealPath: installBaseRealPath,
+    });
+    await fs.rename(stageDir, canonicalTargetDir);
+    stageDir = null;
+  } catch (err) {
+    return await fail(`${params.copyErrorPrefix}: ${String(err)}`);
+  }
+
   if (backupDir) {
     await fs.rm(backupDir, { recursive: true, force: true }).catch(() => undefined);
   }
+  if (stageDir) {
+    await fs.rm(stageDir, { recursive: true, force: true }).catch(() => undefined);
+  }
 
   return { ok: true };
 }
@@ -157,7 +208,7 @@ export async function installPackageDirWithManifestDeps(params: {
   copyErrorPrefix: string;
   depsLogMessage: string;
   manifestDependencies?: Record<string, unknown>;
-  afterCopy?: () => void | Promise<void>;
+  afterCopy?: (installedDir: string) => void | Promise<void>;
 }): Promise<{ ok: true } | { ok: false; error: string }> {
   return installPackageDir({
     ...params,

src/plugins/install.ts

@@ -349,10 +349,10 @@ async function installPluginFromPackageDir(
     copyErrorPrefix: "failed to copy plugin",
     hasDeps,
     depsLogMessage: "Installing plugin dependencies…",
-    afterCopy: async () => {
+    afterCopy: async (installedDir) => {
       for (const entry of extensions) {
-        const resolvedEntry = path.resolve(targetDir, entry);
-        if (!isPathInside(targetDir, resolvedEntry)) {
+        const resolvedEntry = path.resolve(installedDir, entry);
+        if (!isPathInside(installedDir, resolvedEntry)) {
           logger.warn?.(`extension entry escapes plugin directory: ${entry}`);
           continue;
         }

src/test-utils/exec-assertions.ts

@@ -1,8 +1,15 @@
+import path from "node:path";
 import { expect } from "vitest";
 
+function normalizeDarwinTmpPath(filePath: string): string {
+  return process.platform === "darwin" && filePath.startsWith("/private/var/")
+    ? filePath.slice("/private".length)
+    : filePath;
+}
+
 export function expectSingleNpmInstallIgnoreScriptsCall(params: {
   calls: Array<[unknown, { cwd?: string } | undefined]>;
-  expectedCwd: string;
+  expectedTargetDir: string;
 }) {
   const npmCalls = params.calls.filter((call) => Array.isArray(call[0]) && call[0][0] === "npm");
   expect(npmCalls.length).toBe(1);
@@ -19,7 +26,11 @@ export function expectSingleNpmInstallIgnoreScriptsCall(params: {
     "--silent",
     "--ignore-scripts",
   ]);
-  expect(opts?.cwd).toBe(params.expectedCwd);
+  expect(opts?.cwd).toBeTruthy();
+  const cwd = normalizeDarwinTmpPath(String(opts?.cwd));
+  const expectedTargetDir = normalizeDarwinTmpPath(params.expectedTargetDir);
+  expect(path.dirname(cwd)).toBe(path.dirname(expectedTargetDir));
+  expect(path.basename(cwd)).toMatch(/^\.openclaw-install-stage-/);
 }
 
 export function expectSingleNpmPackIgnoreScriptsCall(params: {

src/test-utils/npm-spec-install-test-helpers.ts

@@ -112,6 +112,6 @@ export async function expectInstallUsesIgnoreScripts(params: {
   }
   expectSingleNpmInstallIgnoreScriptsCall({
     calls: params.run.mock.calls as Array<[unknown, { cwd?: string } | undefined]>,
-    expectedCwd: result.targetDir,
+    expectedTargetDir: result.targetDir,
   });
 }