diff --git a/docs/tools/plugin.md b/docs/tools/plugin.md index adfca3a2c9e..6cb1f0cb476 100644 --- a/docs/tools/plugin.md +++ b/docs/tools/plugin.md @@ -128,6 +128,13 @@ visible plugin without importing runtime code or repairing dependencies. See [Plugin dependency resolution](/plugins/dependency-resolution) for the install-time lifecycle. +For npm installs, mutable selectors such as `latest` or a dist-tag are resolved +before installation and then pinned to the exact verified version in OpenClaw's +managed npm root. After npm finishes, OpenClaw verifies the installed +`package-lock.json` entry still matches the resolved version and integrity. If +npm writes different package metadata, the install fails and the managed package +is rolled back instead of accepting a different plugin artifact. + Source checkouts are pnpm workspaces. If you clone OpenClaw to hack on bundled plugins, run `pnpm install`; OpenClaw then loads bundled plugins from `extensions/` so edits and package-local dependencies are used directly. diff --git a/src/infra/safe-package-install.test.ts b/src/infra/safe-package-install.test.ts index b01458c922f..98caedccf43 100644 --- a/src/infra/safe-package-install.test.ts +++ b/src/infra/safe-package-install.test.ts @@ -68,4 +68,21 @@ describe("safe npm install helpers", () => { npm_config_yes: "true", }); }); + + it("allows package-lock-enabled installs to write lockfiles", () => { + expect( + createSafeNpmInstallEnv( + { + PATH: "/usr/bin:/bin", + npm_config_save: "false", + }, + { + packageLock: true, + }, + ), + ).toMatchObject({ + npm_config_package_lock: "true", + npm_config_save: "true", + }); + }); }); diff --git a/src/infra/safe-package-install.ts b/src/infra/safe-package-install.ts index 84be37c2602..72f318981d3 100644 --- a/src/infra/safe-package-install.ts +++ b/src/infra/safe-package-install.ts @@ -28,6 +28,7 @@ export function createSafeNpmInstallEnv( npm_config_fund: "false", npm_config_ignore_scripts: "true", npm_config_package_lock: options.packageLock === true ? "true" : "false", + ...(options.packageLock === true ? { npm_config_save: "true" } : {}), ...(options.ignoreWorkspaces ? { npm_config_workspaces: "false" } : {}), ...(options.legacyPeerDeps ? { npm_config_legacy_peer_deps: "true" } : {}), }; diff --git a/src/plugins/install.npm-spec.e2e.test.ts b/src/plugins/install.npm-spec.e2e.test.ts new file mode 100644 index 00000000000..20d06fafcf7 --- /dev/null +++ b/src/plugins/install.npm-spec.e2e.test.ts @@ -0,0 +1,225 @@ +import { execFileSync } from "node:child_process"; +import crypto from "node:crypto"; +import fs from "node:fs/promises"; +import http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { installPluginFromNpmSpec } from "./install.js"; + +type PackedVersion = { + archive: Buffer; + integrity: string; + shasum: string; + tarballName: string; + version: string; +}; + +const tempDirs: string[] = []; +const servers: http.Server[] = []; +const envKeys = ["NPM_CONFIG_REGISTRY", "npm_config_registry"] as const; +const originalEnv = Object.fromEntries(envKeys.map((key) => [key, process.env[key]])); + +afterEach(async () => { + for (const server of servers.splice(0)) { + await new Promise((resolve) => server.close(() => resolve())); + } + for (const key of envKeys) { + const original = originalEnv[key]; + if (original === undefined) { + delete process.env[key]; + } else { + process.env[key] = original; + } + } + await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); +}); + +async function makeTempDir(label: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), `openclaw-${label}-`)); + tempDirs.push(dir); + return dir; +} + +async function packPlugin(params: { + packageName: string; + pluginId: string; + version: string; + rootDir: string; +}): Promise { + const packageDir = path.join(params.rootDir, `package-${params.version}`); + await fs.mkdir(path.join(packageDir, "dist"), { recursive: true }); + await fs.writeFile( + path.join(packageDir, "package.json"), + `${JSON.stringify( + { + name: params.packageName, + version: params.version, + type: "module", + openclaw: { extensions: ["./dist/index.js"] }, + }, + null, + 2, + )}\n`, + "utf8", + ); + await fs.writeFile( + path.join(packageDir, "openclaw.plugin.json"), + `${JSON.stringify( + { + id: params.pluginId, + name: params.pluginId, + configSchema: { type: "object" }, + }, + null, + 2, + )}\n`, + "utf8", + ); + await fs.writeFile(path.join(packageDir, "dist", "index.js"), "export {};\n", "utf8"); + + const packOutput = execFileSync( + "npm", + ["pack", "--json", "--ignore-scripts", "--pack-destination", params.rootDir], + { cwd: packageDir, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }, + ); + const parsed = JSON.parse(packOutput) as Array<{ filename: string }>; + const tarballName = parsed[0]?.filename; + if (!tarballName) { + throw new Error(`npm pack did not return a tarball for ${params.packageName}`); + } + const archive = await fs.readFile(path.join(params.rootDir, tarballName)); + return { + archive, + integrity: `sha512-${crypto.createHash("sha512").update(archive).digest("base64")}`, + shasum: crypto.createHash("sha1").update(archive).digest("hex"), + tarballName, + version: params.version, + }; +} + +async function startMutableRegistry(params: { + packageName: string; + initialLatest: string; + laterLatest: string; + versions: PackedVersion[]; +}): Promise { + let latestVersion = params.initialLatest; + let metadataRequests = 0; + const versions = new Map(params.versions.map((entry) => [entry.version, entry])); + const encodedPackageName = encodeURIComponent(params.packageName).replace("%40", "@"); + + const server = http.createServer((request, response) => { + const url = new URL(request.url ?? "/", "http://127.0.0.1"); + const baseUrl = `http://127.0.0.1:${(server.address() as { port: number }).port}`; + if (request.method !== "GET") { + response.writeHead(405, { "content-type": "text/plain" }); + response.end("method not allowed"); + return; + } + + if (url.pathname === `/${encodedPackageName}`) { + metadataRequests += 1; + const metadataLatest = latestVersion; + if (metadataRequests === 1) { + latestVersion = params.laterLatest; + } + response.writeHead(200, { "content-type": "application/json" }); + response.end( + `${JSON.stringify({ + name: params.packageName, + "dist-tags": { latest: metadataLatest }, + versions: Object.fromEntries( + [...versions.entries()].map(([version, entry]) => [ + version, + { + name: params.packageName, + version, + dist: { + integrity: entry.integrity, + shasum: entry.shasum, + tarball: `${baseUrl}/${encodedPackageName}/-/${entry.tarballName}`, + }, + }, + ]), + ), + })}\n`, + ); + return; + } + + const tarballPrefix = `/${encodedPackageName}/-/`; + if (url.pathname.startsWith(tarballPrefix)) { + const entry = [...versions.values()].find((candidate) => + url.pathname.endsWith(`/${candidate.tarballName}`), + ); + if (entry) { + response.writeHead(200, { + "content-length": String(entry.archive.length), + "content-type": "application/octet-stream", + }); + response.end(entry.archive); + return; + } + } + + response.writeHead(404, { "content-type": "text/plain" }); + response.end(`not found: ${url.pathname}`); + }); + + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + servers.push(server); + return `http://127.0.0.1:${(server.address() as { port: number }).port}`; +} + +describe("installPluginFromNpmSpec e2e", () => { + it("pins a mutable npm tag to the version resolved before install", async () => { + const rootDir = await makeTempDir("npm-plugin-e2e"); + const npmRoot = path.join(rootDir, "managed-npm"); + const packageName = `mutable-plugin-${crypto.randomUUID().replace(/-/g, "").slice(0, 12)}`; + const pluginId = packageName; + const versions = [ + await packPlugin({ packageName, pluginId, version: "1.0.0", rootDir }), + await packPlugin({ packageName, pluginId, version: "2.0.0", rootDir }), + ]; + const registry = await startMutableRegistry({ + packageName, + initialLatest: "1.0.0", + laterLatest: "2.0.0", + versions, + }); + process.env.NPM_CONFIG_REGISTRY = registry; + process.env.npm_config_registry = registry; + + const result = await installPluginFromNpmSpec({ + spec: `${packageName}@latest`, + npmDir: npmRoot, + logger: { info: () => {}, warn: () => {} }, + timeoutMs: 120_000, + }); + + if (!result.ok) { + throw new Error(result.error); + } + expect(result.ok).toBe(true); + expect(result.npmResolution?.version).toBe("1.0.0"); + + const manifest = JSON.parse(await fs.readFile(path.join(npmRoot, "package.json"), "utf8")) as { + dependencies?: Record; + }; + expect(manifest.dependencies?.[packageName]).toBe("1.0.0"); + + const installedManifest = JSON.parse( + await fs.readFile(path.join(result.targetDir, "package.json"), "utf8"), + ) as { version?: string }; + expect(installedManifest.version).toBe("1.0.0"); + + const lock = JSON.parse(await fs.readFile(path.join(npmRoot, "package-lock.json"), "utf8")) as { + packages?: Record; + }; + expect(lock.packages?.[`node_modules/${packageName}`]).toMatchObject({ + integrity: versions[0]?.integrity, + version: "1.0.0", + }); + }); +}); diff --git a/src/plugins/install.npm-spec.test.ts b/src/plugins/install.npm-spec.test.ts index 2cd5f2ef952..29e5ff8b3d6 100644 --- a/src/plugins/install.npm-spec.test.ts +++ b/src/plugins/install.npm-spec.test.ts @@ -39,6 +39,9 @@ function expectNpmInstallIntoRoot(params: { calls: unknown[][]; npmRoot: string (call) => Array.isArray(call[0]) && call[0][0] === "npm" && call[0][1] === "install", ); expect(installCalls).toHaveLength(1); + expect(installCalls[0]?.[1]).toMatchObject({ + cwd: params.npmRoot, + }); expect(installCalls[0]?.[0]).toEqual([ "npm", "install", @@ -48,7 +51,7 @@ function expectNpmInstallIntoRoot(params: { calls: unknown[][]; npmRoot: string "--no-audit", "--no-fund", "--prefix", - params.npmRoot, + ".", ]); } @@ -133,6 +136,7 @@ type MockNpmPackage = { expectedDependencySpec?: string; installedVersion?: string; installedIntegrity?: string; + skipLockfileEntry?: boolean; }; function writeNpmRootPackageLock(params: { @@ -146,6 +150,9 @@ function writeNpmRootPackageLock(params: { }, }; for (const pkg of params.packages) { + if (pkg.skipLockfileEntry) { + continue; + } lockPackages[`node_modules/${pkg.packageName}`] = { version: pkg.installedVersion ?? pkg.version, integrity: pkg.installedIntegrity ?? pkg.integrity ?? "sha512-plugin-test", @@ -173,76 +180,82 @@ function mockNpmViewAndInstall(params: { expectedDependencySpec?: string; installedVersion?: string; installedIntegrity?: string; + skipLockfileEntry?: boolean; }) { mockNpmViewAndInstallMany([params]); } function mockNpmViewAndInstallMany(packages: MockNpmPackage[]) { const packagesByName = new Map(packages.map((pkg) => [pkg.packageName, pkg])); - runCommandWithTimeoutMock.mockImplementation(async (argv: string[]) => { - const viewPackage = packages.find( - (pkg) => JSON.stringify(argv) === JSON.stringify(npmViewArgv(pkg.spec)), - ); - if (viewPackage) { - return successfulSpawn( - JSON.stringify({ - name: viewPackage.packageName, - version: viewPackage.version, - dist: { - integrity: viewPackage.integrity ?? "sha512-plugin-test", - shasum: viewPackage.shasum ?? "pluginshasum", - }, - }), + runCommandWithTimeoutMock.mockImplementation( + async (argv: string[], options?: { cwd?: string }) => { + const viewPackage = packages.find( + (pkg) => JSON.stringify(argv) === JSON.stringify(npmViewArgv(pkg.spec)), ); - } - if (argv[0] === "npm" && argv[1] === "install") { - const prefixIndex = argv.indexOf("--prefix"); - const npmRoot = prefixIndex >= 0 ? argv[prefixIndex + 1] : undefined; - if (!npmRoot) { - throw new Error(`unexpected npm install command: ${argv.join(" ")}`); + if (viewPackage) { + return successfulSpawn( + JSON.stringify({ + name: viewPackage.packageName, + version: viewPackage.version, + dist: { + integrity: viewPackage.integrity ?? "sha512-plugin-test", + shasum: viewPackage.shasum ?? "pluginshasum", + }, + }), + ); } - const manifest = JSON.parse(fs.readFileSync(path.join(npmRoot, "package.json"), "utf8")) as { - dependencies?: Record; - }; - const installedPackages: MockNpmPackage[] = []; - for (const packageName of Object.keys(manifest.dependencies ?? {})) { - const pkg = packagesByName.get(packageName); - if (!pkg) { - throw new Error(`unexpected managed npm dependency: ${packageName}`); + if (argv[0] === "npm" && argv[1] === "install") { + const prefixIndex = argv.indexOf("--prefix"); + const prefixValue = prefixIndex >= 0 ? argv[prefixIndex + 1] : undefined; + const npmRoot = prefixValue === "." ? options?.cwd : prefixValue; + if (!npmRoot) { + throw new Error(`unexpected npm install command: ${argv.join(" ")}`); } - const dependencySpec = manifest.dependencies?.[packageName]; - if (pkg.expectedDependencySpec && dependencySpec !== pkg.expectedDependencySpec) { - throw new Error( - `expected managed npm dependency ${packageName}@${pkg.expectedDependencySpec}, got ${dependencySpec ?? ""}`, - ); + const manifest = JSON.parse( + fs.readFileSync(path.join(npmRoot, "package.json"), "utf8"), + ) as { + dependencies?: Record; + }; + const installedPackages: MockNpmPackage[] = []; + for (const packageName of Object.keys(manifest.dependencies ?? {})) { + const pkg = packagesByName.get(packageName); + if (!pkg) { + throw new Error(`unexpected managed npm dependency: ${packageName}`); + } + const dependencySpec = manifest.dependencies?.[packageName]; + if (pkg.expectedDependencySpec && dependencySpec !== pkg.expectedDependencySpec) { + throw new Error( + `expected managed npm dependency ${packageName}@${pkg.expectedDependencySpec}, got ${dependencySpec ?? ""}`, + ); + } + writeInstalledNpmPlugin({ + ...pkg, + version: pkg.installedVersion ?? pkg.version, + }); + installedPackages.push(pkg); } - writeInstalledNpmPlugin({ - ...pkg, - version: pkg.installedVersion ?? pkg.version, + writeNpmRootPackageLock({ + npmRoot, + dependencies: manifest.dependencies ?? {}, + packages: installedPackages, }); - installedPackages.push(pkg); + return successfulSpawn(); } - writeNpmRootPackageLock({ - npmRoot, - dependencies: manifest.dependencies ?? {}, - packages: installedPackages, - }); - return successfulSpawn(); - } - if (argv[0] === "npm" && argv[1] === "uninstall") { - const packageName = argv.at(-1); - const pkg = packageName ? packagesByName.get(packageName) : undefined; - if (!pkg) { - throw new Error(`unexpected npm uninstall package: ${packageName ?? ""}`); + if (argv[0] === "npm" && argv[1] === "uninstall") { + const packageName = argv.at(-1); + const pkg = packageName ? packagesByName.get(packageName) : undefined; + if (!pkg) { + throw new Error(`unexpected npm uninstall package: ${packageName ?? ""}`); + } + fs.rmSync(path.join(pkg.npmRoot, "node_modules", pkg.packageName), { + recursive: true, + force: true, + }); + return successfulSpawn(); } - fs.rmSync(path.join(pkg.npmRoot, "node_modules", pkg.packageName), { - recursive: true, - force: true, - }); - return successfulSpawn(); - } - throw new Error(`unexpected command: ${argv.join(" ")}`); - }); + throw new Error(`unexpected command: ${argv.join(" ")}`); + }, + ); } afterAll(() => { @@ -350,6 +363,61 @@ describe("installPluginFromNpmSpec", () => { expect(fs.existsSync(path.join(npmRoot, "node_modules", "drift-plugin"))).toBe(false); }); + it("rejects npm installs when the installed version drifts from verified metadata", async () => { + const npmRoot = path.join(suiteTempRootTracker.makeTempDir(), "npm"); + mockNpmViewAndInstall({ + spec: "version-drift-plugin@latest", + packageName: "version-drift-plugin", + version: "1.0.0", + pluginId: "version-drift-plugin", + installedVersion: "1.0.1", + npmRoot, + expectedDependencySpec: "1.0.0", + }); + + const result = await installPluginFromNpmSpec({ + spec: "version-drift-plugin@latest", + npmDir: npmRoot, + logger: { info: () => {}, warn: () => {} }, + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain("version 1.0.1"); + expect(result.error).toContain("expected 1.0.0"); + expect(fs.existsSync(path.join(npmRoot, "node_modules", "version-drift-plugin"))).toBe(false); + }); + + it("rejects npm installs when package-lock omits the installed plugin", async () => { + const npmRoot = path.join(suiteTempRootTracker.makeTempDir(), "npm"); + mockNpmViewAndInstall({ + spec: "missing-lock-plugin@latest", + packageName: "missing-lock-plugin", + version: "1.0.0", + pluginId: "missing-lock-plugin", + npmRoot, + expectedDependencySpec: "1.0.0", + skipLockfileEntry: true, + }); + + const result = await installPluginFromNpmSpec({ + spec: "missing-lock-plugin@latest", + npmDir: npmRoot, + logger: { info: () => {}, warn: () => {} }, + }); + + expect(result.ok).toBe(false); + if (result.ok) { + return; + } + expect(result.error).toContain( + "npm install did not record package-lock metadata for missing-lock-plugin", + ); + expect(fs.existsSync(path.join(npmRoot, "node_modules", "missing-lock-plugin"))).toBe(false); + }); + it("rejects npm installs with blocked hoisted transitive dependencies", async () => { const stateDir = suiteTempRootTracker.makeTempDir(); const npmRoot = path.join(stateDir, "npm"); diff --git a/src/plugins/install.ts b/src/plugins/install.ts index 2d28827578d..7f90bb458b3 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -223,10 +223,11 @@ async function rollbackManagedNpmPluginInstall(params: { "--no-audit", "--no-fund", "--prefix", - params.npmRoot, + ".", params.packageName, ], { + cwd: params.npmRoot, timeoutMs: Math.max(params.timeoutMs, 300_000), env: createSafeNpmInstallEnv(process.env, { packageLock: true, quiet: true }), }, @@ -1243,9 +1244,10 @@ export async function installPluginFromNpmSpec( noFund: true, }), "--prefix", - npmRoot, + ".", ], { + cwd: npmRoot, timeoutMs: Math.max(timeoutMs, 300_000), env: createSafeNpmInstallEnv(process.env, { packageLock: true, quiet: true }), }, diff --git a/src/plugins/uninstall.test.ts b/src/plugins/uninstall.test.ts index a8829a6e127..a7a8543353b 100644 --- a/src/plugins/uninstall.test.ts +++ b/src/plugins/uninstall.test.ts @@ -978,10 +978,11 @@ describe("uninstallPlugin", () => { "--no-audit", "--no-fund", "--prefix", - npmRoot, + ".", "@openclaw/kitchen-sink", ], expect.objectContaining({ + cwd: npmRoot, timeoutMs: 300_000, env: expect.objectContaining({ NPM_CONFIG_IGNORE_SCRIPTS: "true", diff --git a/src/plugins/uninstall.ts b/src/plugins/uninstall.ts index 9f737592e3e..561c7d8453d 100644 --- a/src/plugins/uninstall.ts +++ b/src/plugins/uninstall.ts @@ -598,10 +598,11 @@ export async function applyPluginUninstallDirectoryRemoval( "--no-audit", "--no-fund", "--prefix", - removal.cleanup.npmRoot, + ".", removal.cleanup.packageName, ], { + cwd: removal.cleanup.npmRoot, timeoutMs: 300_000, env: createSafeNpmInstallEnv(process.env, { packageLock: true, quiet: true }), },