diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a25c0aa8d9..8f8987b18f3 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/infra/install-package-dir.test.ts b/src/infra/install-package-dir.test.ts new file mode 100644 index 00000000000..9dcd532cdc4 --- /dev/null +++ b/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 { + 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) => { + 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); + }); +}); diff --git a/src/infra/install-package-dir.ts b/src/infra/install-package-dir.ts index 5c5527000cf..d2364558f5c 100644 --- a/src/infra/install-package-dir.ts +++ b/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 { + 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; + afterCopy?: (installedDir: string) => void | Promise; }): 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; - afterCopy?: () => void | Promise; + afterCopy?: (installedDir: string) => void | Promise; }): Promise<{ ok: true } | { ok: false; error: string }> { return installPackageDir({ ...params, diff --git a/src/plugins/install.ts b/src/plugins/install.ts index 6860568cd74..e6e107877cf 100644 --- a/src/plugins/install.ts +++ b/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; } diff --git a/src/test-utils/exec-assertions.ts b/src/test-utils/exec-assertions.ts index def16cdfa05..99907a1ad74 100644 --- a/src/test-utils/exec-assertions.ts +++ b/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: { diff --git a/src/test-utils/npm-spec-install-test-helpers.ts b/src/test-utils/npm-spec-install-test-helpers.ts index 9ef8e29404e..bebff88ba45 100644 --- a/src/test-utils/npm-spec-install-test-helpers.ts +++ b/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, }); }