From e302353d6187ceda2f4ee844e928c93690788553 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 1 May 2026 14:07:13 -0700 Subject: [PATCH] fix(plugins): harden managed plugin install lifecycle --- CHANGELOG.md | 1 + src/plugins/git-install.test.ts | 123 ++++++++++++++++++++++++--- src/plugins/git-install.ts | 63 ++++++++++++-- src/plugins/install.npm-spec.test.ts | 30 +++++++ src/plugins/install.ts | 16 +++- src/plugins/uninstall.test.ts | 67 +++++++++++++++ src/plugins/uninstall.ts | 39 ++++++++- 7 files changed, 316 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 537015db342..732c6f38417 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai - Gateway/config: report failed backup restores as failed in logs and config observe audit records instead of marking them valid. (#70515) Thanks @davidangularme. - Compaction: use the active session model fallback chain for implicit summarization failures without persisting fallback model selection, so Azure content-filter 400s can recover. Fixes #64960. (#74470) Thanks @jalehman and @OpenCodeEngineer. - Gateway/config: allow `gateway config.patch` to update documented subagent thinking defaults. Fixes #75764. (#75802) Thanks @kAIborg24. +- Plugins/CLI: keep git plugin install paths credential-free, preserve existing git checkouts until replacement succeeds, honor duplicate npm install mode, and remove managed git repos on uninstall. Thanks @vincentkoc. ## 2026.4.30 diff --git a/src/plugins/git-install.test.ts b/src/plugins/git-install.test.ts index 9d1d10698fa..a63a7d8485e 100644 --- a/src/plugins/git-install.test.ts +++ b/src/plugins/git-install.test.ts @@ -1,4 +1,9 @@ +import { createHash } from "node:crypto"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it, vi, beforeEach } from "vitest"; +import { redactSensitiveUrlLikeString } from "../shared/net/redact-sensitive-url.js"; const runCommandWithTimeoutMock = vi.fn(); const installPluginFromInstalledPackageDirMock = vi.fn(); @@ -20,6 +25,14 @@ vi.resetModules(); const { installPluginFromGitSpec, parseGitPluginSpec } = await import("./git-install.js"); +function expectedGitRepoDir(params: { gitDir: string; normalizedSpec: string }): string { + const hash = createHash("sha256") + .update(redactSensitiveUrlLikeString(params.normalizedSpec)) + .digest("hex") + .slice(0, 16); + return path.join(params.gitDir, `git-${hash}`, "repo"); +} + describe("parseGitPluginSpec", () => { it("normalizes GitHub shorthand and ref selectors", () => { expect(parseGitPluginSpec("git:github.com/acme/demo@v1.2.3")).toMatchObject({ @@ -55,13 +68,18 @@ describe("installPluginFromGitSpec", () => { .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }) .mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" }) .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }); - installPluginFromInstalledPackageDirMock.mockResolvedValue({ - ok: true, - pluginId: "demo", - targetDir: "/tmp/git-root/repo", - version: "1.2.3", - extensions: ["index.js"], - }); + installPluginFromInstalledPackageDirMock.mockImplementation( + async (params: { packageDir: string }) => { + await fs.mkdir(params.packageDir, { recursive: true }); + return { + ok: true, + pluginId: "demo", + targetDir: params.packageDir, + version: "1.2.3", + extensions: ["index.js"], + }; + }, + ); const result = await installPluginFromGitSpec({ spec: "git:github.com/acme/demo@v1.2.3", @@ -115,13 +133,18 @@ describe("installPluginFromGitSpec", () => { .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }) .mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" }) .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }); - installPluginFromInstalledPackageDirMock.mockResolvedValue({ - ok: true, - pluginId: "demo", - targetDir: "/tmp/git-root/repo", - version: "1.2.3", - extensions: ["index.js"], - }); + installPluginFromInstalledPackageDirMock.mockImplementation( + async (params: { packageDir: string }) => { + await fs.mkdir(params.packageDir, { recursive: true }); + return { + ok: true, + pluginId: "demo", + targetDir: params.packageDir, + version: "1.2.3", + extensions: ["index.js"], + }; + }, + ); await installPluginFromGitSpec({ spec: "git:github.com/acme/demo" }); @@ -134,4 +157,76 @@ describe("installPluginFromGitSpec", () => { expect.stringContaining("/repo"), ]); }); + + it("uses a credential-free managed repo path for authenticated git URLs", async () => { + const gitDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-git-install-path-")); + try { + runCommandWithTimeoutMock + .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }) + .mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" }) + .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }); + installPluginFromInstalledPackageDirMock.mockImplementation( + async (params: { packageDir: string }) => { + await fs.mkdir(params.packageDir, { recursive: true }); + return { + ok: true, + pluginId: "demo", + targetDir: params.packageDir, + version: "1.2.3", + extensions: ["index.js"], + }; + }, + ); + + const result = await installPluginFromGitSpec({ + spec: "git:https://token@github.com/acme/demo.git", + gitDir, + }); + + expect(result.ok).toBe(true); + if (!result.ok) { + throw new Error(result.error); + } + expect(result.targetDir).toBe( + expectedGitRepoDir({ + gitDir, + normalizedSpec: "git:https://token@github.com/acme/demo.git", + }), + ); + expect(result.targetDir).not.toContain("token"); + expect(result.targetDir).not.toContain("github.com"); + } finally { + await fs.rm(gitDir, { recursive: true, force: true }); + } + }); + + it("keeps the existing managed repo when replacement install fails", async () => { + const gitDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-git-install-preserve-")); + const normalizedSpec = "git:https://github.com/acme/demo.git"; + const existingRepoDir = expectedGitRepoDir({ gitDir, normalizedSpec }); + const markerPath = path.join(existingRepoDir, "existing.txt"); + try { + await fs.mkdir(existingRepoDir, { recursive: true }); + await fs.writeFile(markerPath, "keep"); + runCommandWithTimeoutMock + .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }) + .mockResolvedValueOnce({ code: 0, stdout: "abc123\n", stderr: "" }) + .mockResolvedValueOnce({ code: 1, stdout: "", stderr: "npm failed" }); + + const result = await installPluginFromGitSpec({ + spec: "git:https://github.com/acme/demo.git", + gitDir, + mode: "update", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("npm install failed"); + } + await expect(fs.readFile(markerPath, "utf8")).resolves.toBe("keep"); + expect(installPluginFromInstalledPackageDirMock).not.toHaveBeenCalled(); + } finally { + await fs.rm(gitDir, { recursive: true, force: true }); + } + }); }); diff --git a/src/plugins/git-install.ts b/src/plugins/git-install.ts index 93009c94610..1bb5482cdf0 100644 --- a/src/plugins/git-install.ts +++ b/src/plugins/git-install.ts @@ -1,6 +1,6 @@ +import { createHash } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; -import { safePathSegmentHashed } from "../infra/install-safe-path.js"; import { withTempDir } from "../infra/install-source-utils.js"; import { createSafeNpmInstallArgs, @@ -183,7 +183,50 @@ function resolveGitInstallRepoDir(params: { source: ParsedGitPluginSpec; }): string { const gitRoot = params.gitDir ? resolveUserPath(params.gitDir) : resolveDefaultPluginGitDir(); - return path.join(gitRoot, safePathSegmentHashed(params.source.normalizedSpec), "repo"); + const redactedSpec = redactSensitiveUrlLikeString(params.source.normalizedSpec); + const hash = createHash("sha256").update(redactedSpec).digest("hex").slice(0, 16); + return path.join(gitRoot, `git-${hash}`, "repo"); +} + +async function replaceManagedGitRepo(params: { + stagedRepoDir: string; + persistentRepoDir: string; +}): Promise<{ ok: true } | { ok: false; error: string }> { + const parentDir = path.dirname(params.persistentRepoDir); + const backupDir = path.join(parentDir, `.repo-backup-${process.pid}-${Date.now()}`); + let backupCreated = false; + + try { + await fs.mkdir(parentDir, { recursive: true }); + try { + await fs.rename(params.persistentRepoDir, backupDir); + backupCreated = true; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + throw err; + } + } + + try { + await fs.rename(params.stagedRepoDir, params.persistentRepoDir); + } catch (err) { + if (backupCreated) { + await fs.rename(backupDir, params.persistentRepoDir); + backupCreated = false; + } + throw err; + } + + if (backupCreated) { + await fs.rm(backupDir, { recursive: true, force: true }); + } + return { ok: true }; + } catch (err) { + return { + ok: false, + error: `failed to replace managed git plugin repository: ${String(err)}`, + }; + } } function formatGitCommandFailure(params: { @@ -244,11 +287,7 @@ export async function installPluginFromGitSpec( const persistentRepoDir = resolveGitInstallRepoDir({ gitDir: params.gitDir, source: parsed }); return await withTempDir("openclaw-git-plugin-", async (tmpDir) => { - const repoDir = params.dryRun ? path.join(tmpDir, "repo") : persistentRepoDir; - if (!params.dryRun) { - await fs.rm(repoDir, { recursive: true, force: true }); - await fs.mkdir(path.dirname(repoDir), { recursive: true }); - } + const repoDir = path.join(tmpDir, "repo"); params.logger?.info?.( `Cloning ${sanitizeForLog(redactSensitiveUrlLikeString(parsed.label))}...`, ); @@ -330,9 +369,19 @@ export async function installPluginFromGitSpec( if (!result.ok) { return result; } + if (!params.dryRun) { + const replaceResult = await replaceManagedGitRepo({ + stagedRepoDir: repoDir, + persistentRepoDir, + }); + if (!replaceResult.ok) { + return replaceResult; + } + } return { ...result, + targetDir: params.dryRun ? result.targetDir : persistentRepoDir, git: { url: parsed.url, ref: parsed.ref, diff --git a/src/plugins/install.npm-spec.test.ts b/src/plugins/install.npm-spec.test.ts index 2b01bdd27f5..fca7a432dda 100644 --- a/src/plugins/install.npm-spec.test.ts +++ b/src/plugins/install.npm-spec.test.ts @@ -269,6 +269,36 @@ describe("installPluginFromNpmSpec", () => { } }); + it("rejects duplicate npm installs unless update mode is requested", async () => { + const stateDir = suiteTempRootTracker.makeTempDir(); + const npmRoot = path.join(stateDir, "npm"); + const installRoot = path.join(npmRoot, "node_modules", "@openclaw", "voice-call"); + fs.mkdirSync(installRoot, { recursive: true }); + mockNpmViewMetadataResult(runCommandWithTimeoutMock, { + name: "@openclaw/voice-call", + version: "0.0.1", + integrity: "sha512-plugin-test", + shasum: "pluginshasum", + }); + + const result = await installPluginFromNpmSpec({ + spec: "@openclaw/voice-call@0.0.1", + npmDir: npmRoot, + mode: "install", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("plugin already exists"); + expect(result.error).toContain(installRoot); + } + expect( + runCommandWithTimeoutMock.mock.calls.some( + (call) => Array.isArray(call[0]) && call[0][0] === "npm" && call[0][1] === "install", + ), + ).toBe(false); + }); + it("aborts when integrity drift callback rejects the fetched artifact", async () => { mockNpmViewMetadataResult(runCommandWithTimeoutMock, { name: "@openclaw/voice-call", diff --git a/src/plugins/install.ts b/src/plugins/install.ts index bc5216e68ad..a37d85ccec2 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -1082,7 +1082,7 @@ export async function installPluginFromNpmSpec( }, ): Promise { const runtime = await loadPluginInstallRuntime(); - const { logger, timeoutMs, dryRun } = runtime.resolveTimedInstallModeOptions( + const { logger, timeoutMs, mode, dryRun } = runtime.resolveTimedInstallModeOptions( params, defaultLogger, ); @@ -1148,6 +1148,19 @@ export async function installPluginFromNpmSpec( const npmRoot = params.npmDir ? resolveUserPath(params.npmDir) : resolveDefaultPluginNpmDir(); const installRoot = path.join(npmRoot, "node_modules", parsedSpec.name); + const effectiveMode = await resolveEffectiveInstallMode({ + runtime, + requestedMode: mode, + targetPath: installRoot, + }); + const availability = await ensureInstallTargetAvailableForMode({ + runtime, + targetPath: installRoot, + mode: effectiveMode, + }); + if (!availability.ok) { + return availability; + } if (dryRun) { return { ok: true, @@ -1192,6 +1205,7 @@ export async function installPluginFromNpmSpec( dependencyTreeRootDir: npmRoot, logger, expectedPluginId, + mode: effectiveMode, installPolicyRequest: { kind: "plugin-npm", requestedSpecifier: spec, diff --git a/src/plugins/uninstall.test.ts b/src/plugins/uninstall.test.ts index 3e7035447ff..1b689cf568a 100644 --- a/src/plugins/uninstall.test.ts +++ b/src/plugins/uninstall.test.ts @@ -115,6 +115,16 @@ function createNpmInstallRecord(pluginId = "my-plugin", installPath?: string): P }; } +function createGitInstallRecord(pluginId = "my-plugin", installPath?: string): PluginInstallRecord { + return { + source: "git", + spec: `git:https://github.com/acme/${pluginId}.git`, + gitUrl: `https://github.com/acme/${pluginId}.git`, + gitCommit: "abc123", + ...(installPath ? { installPath } : {}), + }; +} + function createPathInstallRecord( installPath = "/path/to/plugin", sourcePath = installPath, @@ -905,6 +915,31 @@ describe("uninstallPlugin", () => { }); await expect(fs.access(installPath)).rejects.toThrow(); }); + + it("deletes managed git install repos outside the extensions directory", async () => { + const stateDir = path.join(tempDir, "state"); + const extensionsDir = path.join(stateDir, "extensions"); + const installPath = path.join(stateDir, "git", "git-abc123", "repo"); + await fs.mkdir(installPath, { recursive: true }); + await fs.writeFile(path.join(installPath, "index.js"), "// git plugin"); + + const result = await uninstallPlugin({ + config: createPluginConfig({ + entries: createSinglePluginEntries(), + installs: { + "my-plugin": createGitInstallRecord("my-plugin", installPath), + }, + }), + pluginId: "my-plugin", + deleteFiles: true, + extensionsDir, + }); + + expectSuccessfulUninstallActions(result, { + directory: true, + }); + await expect(fs.access(installPath)).rejects.toThrow(); + }); }); describe("resolveUninstallDirectoryTarget", () => { @@ -993,6 +1028,38 @@ describe("resolveUninstallDirectoryTarget", () => { ).toBe(installPath); }); + it("uses configured installPath when git installed it under the managed git root", () => { + const stateDir = path.join(os.tmpdir(), "openclaw-uninstall-safe"); + const extensionsDir = path.join(stateDir, "extensions"); + const installPath = path.join(stateDir, "git", "git-abc123", "repo"); + + expect( + resolveUninstallDirectoryTarget({ + pluginId: "my-plugin", + hasInstall: true, + installRecord: createGitInstallRecord("my-plugin", installPath), + extensionsDir, + }), + ).toBe(installPath); + }); + + it("does not trust git install paths outside the managed git root", () => { + const stateDir = path.join(os.tmpdir(), "openclaw-uninstall-safe"); + const extensionsDir = path.join(stateDir, "extensions"); + + expect( + resolveUninstallDirectoryTarget({ + pluginId: "my-plugin", + hasInstall: true, + installRecord: createGitInstallRecord( + "my-plugin", + path.join(os.tmpdir(), "git", "git-abc123", "repo"), + ), + extensionsDir, + }), + ).toBe(resolvePluginInstallDir("my-plugin", extensionsDir)); + }); + it("does not trust npm install paths outside the managed npm root", () => { const stateDir = path.join(os.tmpdir(), "openclaw-uninstall-safe"); const extensionsDir = path.join(stateDir, "extensions"); diff --git a/src/plugins/uninstall.ts b/src/plugins/uninstall.ts index ce3e1e09fc1..fc7f2dadd6c 100644 --- a/src/plugins/uninstall.ts +++ b/src/plugins/uninstall.ts @@ -4,7 +4,11 @@ import path from "node:path"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import type { PluginInstallRecord } from "../config/types.plugins.js"; import { formatErrorMessage } from "../infra/errors.js"; -import { resolveDefaultPluginNpmDir, resolvePluginInstallDir } from "./install-paths.js"; +import { + resolveDefaultPluginGitDir, + resolveDefaultPluginNpmDir, + resolvePluginInstallDir, +} from "./install-paths.js"; import { defaultSlotIdForKey } from "./slots.js"; export type UninstallActions = { @@ -121,6 +125,13 @@ export function resolveUninstallDirectoryTarget(params: { if (npmManagedPath) { return npmManagedPath; } + const gitManagedPath = resolveGitManagedInstallPath({ + installRecord: params.installRecord, + extensionsDir: params.extensionsDir, + }); + if (gitManagedPath) { + return gitManagedPath; + } let defaultPath: string; try { @@ -182,6 +193,32 @@ function resolveNpmManagedInstallPath(params: { return null; } +function resolveGitManagedInstallPath(params: { + installRecord?: PluginInstallRecord; + extensionsDir?: string; +}): string | null { + const installPath = params.installRecord?.installPath?.trim(); + if (params.installRecord?.source !== "git" || !installPath) { + return null; + } + + const gitRoots = new Set(); + if (params.extensionsDir) { + gitRoots.add(path.join(path.dirname(path.resolve(params.extensionsDir)), "git")); + } + gitRoots.add(resolveDefaultPluginGitDir()); + + for (const gitRoot of gitRoots) { + if ( + isPathInsideOrEqual(gitRoot, installPath) && + resolveComparablePath(gitRoot) !== resolveComparablePath(installPath) + ) { + return installPath; + } + } + return null; +} + function resolveRecordedManagedInstallPath(params: { pluginId: string; installPath: string;