From edf92f1cb039d710a536dcd81fc61af9623ec713 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 19 Feb 2026 14:36:53 +0000 Subject: [PATCH] refactor: share npm integrity drift handling --- src/hooks/install.ts | 44 +++++--------- src/infra/npm-integrity.test.ts | 103 ++++++++++++++++++++++++++++++++ src/infra/npm-integrity.ts | 93 ++++++++++++++++++++++++++++ src/plugins/install.ts | 44 +++++--------- src/plugins/update.ts | 84 +++++++++++++++----------- 5 files changed, 274 insertions(+), 94 deletions(-) create mode 100644 src/infra/npm-integrity.test.ts create mode 100644 src/infra/npm-integrity.ts diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 2eb3553363..8fe4ce6fb0 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -17,6 +17,7 @@ import { resolveArchiveSourcePath, withTempDir, } from "../infra/install-source-utils.js"; +import { resolveNpmIntegrityDriftWithDefaultMessage } from "../infra/npm-integrity.js"; import { validateRegistryNpmSpec } from "../infra/npm-registry-spec.js"; import { isPathInside, isPathInsideWithRealpath } from "../security/scan-paths.js"; import { CONFIG_DIR, resolveUserPath } from "../utils.js"; @@ -430,36 +431,21 @@ export async function installHooksFromNpmSpec(params: { resolvedAt: new Date().toISOString(), }; - let integrityDrift: NpmIntegrityDrift | undefined; - if ( - params.expectedIntegrity && - npmResolution.integrity && - params.expectedIntegrity !== npmResolution.integrity - ) { - integrityDrift = { - expectedIntegrity: params.expectedIntegrity, - actualIntegrity: npmResolution.integrity, + const driftResult = await resolveNpmIntegrityDriftWithDefaultMessage({ + spec, + expectedIntegrity: params.expectedIntegrity, + resolution: npmResolution, + onIntegrityDrift: params.onIntegrityDrift, + warn: (message) => { + logger.warn?.(message); + }, + }); + const integrityDrift = driftResult.integrityDrift; + if (driftResult.error) { + return { + ok: false, + error: driftResult.error, }; - const driftPayload: HookNpmIntegrityDriftParams = { - spec, - expectedIntegrity: integrityDrift.expectedIntegrity, - actualIntegrity: integrityDrift.actualIntegrity, - resolution: npmResolution, - }; - let proceed = true; - if (params.onIntegrityDrift) { - proceed = await params.onIntegrityDrift(driftPayload); - } else { - logger.warn?.( - `Integrity drift detected for ${driftPayload.resolution.resolvedSpec ?? driftPayload.spec}: expected ${driftPayload.expectedIntegrity}, got ${driftPayload.actualIntegrity}`, - ); - } - if (!proceed) { - return { - ok: false, - error: `aborted: npm package integrity drift detected for ${driftPayload.resolution.resolvedSpec ?? driftPayload.spec}`, - }; - } } const installResult = await installHooksFromArchive({ diff --git a/src/infra/npm-integrity.test.ts b/src/infra/npm-integrity.test.ts new file mode 100644 index 0000000000..e7e40b4641 --- /dev/null +++ b/src/infra/npm-integrity.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it, vi } from "vitest"; +import { + resolveNpmIntegrityDrift, + resolveNpmIntegrityDriftWithDefaultMessage, +} from "./npm-integrity.js"; + +describe("resolveNpmIntegrityDrift", () => { + it("returns proceed=true when integrity is missing or unchanged", async () => { + await expect( + resolveNpmIntegrityDrift({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-same", + resolution: { integrity: "sha512-same", resolvedAt: "2026-01-01T00:00:00.000Z" }, + createPayload: () => "unused", + }), + ).resolves.toEqual({ proceed: true }); + + await expect( + resolveNpmIntegrityDrift({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-same", + resolution: { resolvedAt: "2026-01-01T00:00:00.000Z" }, + createPayload: () => "unused", + }), + ).resolves.toEqual({ proceed: true }); + }); + + it("uses callback on integrity drift", async () => { + const onIntegrityDrift = vi.fn(async () => false); + const result = await resolveNpmIntegrityDrift({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-old", + resolution: { + integrity: "sha512-new", + resolvedAt: "2026-01-01T00:00:00.000Z", + }, + createPayload: ({ expectedIntegrity, actualIntegrity }) => ({ + expectedIntegrity, + actualIntegrity, + }), + onIntegrityDrift, + }); + + expect(onIntegrityDrift).toHaveBeenCalledWith({ + expectedIntegrity: "sha512-old", + actualIntegrity: "sha512-new", + }); + expect(result.proceed).toBe(false); + expect(result.integrityDrift).toEqual({ + expectedIntegrity: "sha512-old", + actualIntegrity: "sha512-new", + }); + }); + + it("warns by default when no callback is provided", async () => { + const warn = vi.fn(); + const result = await resolveNpmIntegrityDrift({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-old", + resolution: { + integrity: "sha512-new", + resolvedAt: "2026-01-01T00:00:00.000Z", + }, + createPayload: ({ spec }) => ({ spec }), + warn, + }); + + expect(warn).toHaveBeenCalledWith({ spec: "@openclaw/test@1.0.0" }); + expect(result.proceed).toBe(true); + }); + + it("formats default warning and abort error messages", async () => { + const warn = vi.fn(); + const warningResult = await resolveNpmIntegrityDriftWithDefaultMessage({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-old", + resolution: { + integrity: "sha512-new", + resolvedSpec: "@openclaw/test@1.0.0", + resolvedAt: "2026-01-01T00:00:00.000Z", + }, + warn, + }); + expect(warningResult.error).toBeUndefined(); + expect(warn).toHaveBeenCalledWith( + "Integrity drift detected for @openclaw/test@1.0.0: expected sha512-old, got sha512-new", + ); + + const abortResult = await resolveNpmIntegrityDriftWithDefaultMessage({ + spec: "@openclaw/test@1.0.0", + expectedIntegrity: "sha512-old", + resolution: { + integrity: "sha512-new", + resolvedSpec: "@openclaw/test@1.0.0", + resolvedAt: "2026-01-01T00:00:00.000Z", + }, + onIntegrityDrift: async () => false, + }); + expect(abortResult.error).toBe( + "aborted: npm package integrity drift detected for @openclaw/test@1.0.0", + ); + }); +}); diff --git a/src/infra/npm-integrity.ts b/src/infra/npm-integrity.ts new file mode 100644 index 0000000000..b26c9d0a19 --- /dev/null +++ b/src/infra/npm-integrity.ts @@ -0,0 +1,93 @@ +import type { NpmIntegrityDrift, NpmSpecResolution } from "./install-source-utils.js"; + +export type NpmIntegrityDriftPayload = { + spec: string; + expectedIntegrity: string; + actualIntegrity: string; + resolution: NpmSpecResolution; +}; + +type ResolveNpmIntegrityDriftParams = { + spec: string; + expectedIntegrity?: string; + resolution: NpmSpecResolution; + createPayload: (params: { + spec: string; + expectedIntegrity: string; + actualIntegrity: string; + resolution: NpmSpecResolution; + }) => TPayload; + onIntegrityDrift?: (payload: TPayload) => boolean | Promise; + warn?: (payload: TPayload) => void; +}; + +export type ResolveNpmIntegrityDriftResult = { + integrityDrift?: NpmIntegrityDrift; + proceed: boolean; + payload?: TPayload; +}; + +export async function resolveNpmIntegrityDrift( + params: ResolveNpmIntegrityDriftParams, +): Promise> { + if (!params.expectedIntegrity || !params.resolution.integrity) { + return { proceed: true }; + } + if (params.expectedIntegrity === params.resolution.integrity) { + return { proceed: true }; + } + + const integrityDrift: NpmIntegrityDrift = { + expectedIntegrity: params.expectedIntegrity, + actualIntegrity: params.resolution.integrity, + }; + const payload = params.createPayload({ + spec: params.spec, + expectedIntegrity: integrityDrift.expectedIntegrity, + actualIntegrity: integrityDrift.actualIntegrity, + resolution: params.resolution, + }); + + let proceed = true; + if (params.onIntegrityDrift) { + proceed = await params.onIntegrityDrift(payload); + } else { + params.warn?.(payload); + } + + return { integrityDrift, proceed, payload }; +} + +type ResolveNpmIntegrityDriftWithDefaultMessageParams = { + spec: string; + expectedIntegrity?: string; + resolution: NpmSpecResolution; + onIntegrityDrift?: (payload: NpmIntegrityDriftPayload) => boolean | Promise; + warn?: (message: string) => void; +}; + +export async function resolveNpmIntegrityDriftWithDefaultMessage( + params: ResolveNpmIntegrityDriftWithDefaultMessageParams, +): Promise<{ integrityDrift?: NpmIntegrityDrift; error?: string }> { + const driftResult = await resolveNpmIntegrityDrift({ + spec: params.spec, + expectedIntegrity: params.expectedIntegrity, + resolution: params.resolution, + createPayload: (drift) => ({ ...drift }), + onIntegrityDrift: params.onIntegrityDrift, + warn: (driftPayload) => { + params.warn?.( + `Integrity drift detected for ${driftPayload.resolution.resolvedSpec ?? driftPayload.spec}: expected ${driftPayload.expectedIntegrity}, got ${driftPayload.actualIntegrity}`, + ); + }, + }); + + if (!driftResult.proceed && driftResult.payload) { + return { + integrityDrift: driftResult.integrityDrift, + error: `aborted: npm package integrity drift detected for ${driftResult.payload.resolution.resolvedSpec ?? driftResult.payload.spec}`, + }; + } + + return { integrityDrift: driftResult.integrityDrift }; +} diff --git a/src/plugins/install.ts b/src/plugins/install.ts index 32e33bb9f3..c357f4c1a2 100644 --- a/src/plugins/install.ts +++ b/src/plugins/install.ts @@ -21,6 +21,7 @@ import { resolveArchiveSourcePath, withTempDir, } from "../infra/install-source-utils.js"; +import { resolveNpmIntegrityDriftWithDefaultMessage } from "../infra/npm-integrity.js"; import { validateRegistryNpmSpec } from "../infra/npm-registry-spec.js"; import { extensionUsesSkippedScannerPath, isPathInside } from "../security/scan-paths.js"; import * as skillScanner from "../security/skill-scanner.js"; @@ -458,36 +459,21 @@ export async function installPluginFromNpmSpec(params: { resolvedAt: new Date().toISOString(), }; - let integrityDrift: NpmIntegrityDrift | undefined; - if ( - params.expectedIntegrity && - npmResolution.integrity && - params.expectedIntegrity !== npmResolution.integrity - ) { - integrityDrift = { - expectedIntegrity: params.expectedIntegrity, - actualIntegrity: npmResolution.integrity, + const driftResult = await resolveNpmIntegrityDriftWithDefaultMessage({ + spec, + expectedIntegrity: params.expectedIntegrity, + resolution: npmResolution, + onIntegrityDrift: params.onIntegrityDrift, + warn: (message) => { + logger.warn?.(message); + }, + }); + const integrityDrift = driftResult.integrityDrift; + if (driftResult.error) { + return { + ok: false, + error: driftResult.error, }; - const driftPayload: PluginNpmIntegrityDriftParams = { - spec, - expectedIntegrity: integrityDrift.expectedIntegrity, - actualIntegrity: integrityDrift.actualIntegrity, - resolution: npmResolution, - }; - let proceed = true; - if (params.onIntegrityDrift) { - proceed = await params.onIntegrityDrift(driftPayload); - } else { - logger.warn?.( - `Integrity drift detected for ${driftPayload.resolution.resolvedSpec ?? driftPayload.spec}: expected ${driftPayload.expectedIntegrity}, got ${driftPayload.actualIntegrity}`, - ); - } - if (!proceed) { - return { - ok: false, - error: `aborted: npm package integrity drift detected for ${driftPayload.resolution.resolvedSpec ?? driftPayload.spec}`, - }; - } } const installResult = await installPluginFromArchive({ diff --git a/src/plugins/update.ts b/src/plugins/update.ts index 628ff84995..288aa15275 100644 --- a/src/plugins/update.ts +++ b/src/plugins/update.ts @@ -58,6 +58,16 @@ type BundledPluginSource = { npmSpec?: string; }; +type InstallIntegrityDrift = { + spec: string; + expectedIntegrity: string; + actualIntegrity: string; + resolution: { + resolvedSpec?: string; + version?: string; + }; +}; + async function readInstalledPackageVersion(dir: string): Promise { try { const raw = await fs.readFile(`${dir}/package.json`, "utf-8"); @@ -147,6 +157,32 @@ function buildLoadPathHelpers(existing: string[]) { }; } +function createPluginUpdateIntegrityDriftHandler(params: { + pluginId: string; + dryRun: boolean; + logger: PluginUpdateLogger; + onIntegrityDrift?: (params: PluginUpdateIntegrityDriftParams) => boolean | Promise; +}) { + return async (drift: InstallIntegrityDrift) => { + const payload: PluginUpdateIntegrityDriftParams = { + pluginId: params.pluginId, + spec: drift.spec, + expectedIntegrity: drift.expectedIntegrity, + actualIntegrity: drift.actualIntegrity, + resolvedSpec: drift.resolution.resolvedSpec, + resolvedVersion: drift.resolution.version, + dryRun: params.dryRun, + }; + if (params.onIntegrityDrift) { + return await params.onIntegrityDrift(payload); + } + params.logger.warn?.( + `Integrity drift for "${params.pluginId}" (${payload.resolvedSpec ?? payload.spec}): expected ${payload.expectedIntegrity}, got ${payload.actualIntegrity}`, + ); + return true; + }; +} + export async function updateNpmInstalledPlugins(params: { config: OpenClawConfig; logger?: PluginUpdateLogger; @@ -222,24 +258,12 @@ export async function updateNpmInstalledPlugins(params: { dryRun: true, expectedPluginId: pluginId, expectedIntegrity: record.integrity, - onIntegrityDrift: async (drift) => { - const payload: PluginUpdateIntegrityDriftParams = { - pluginId, - spec: drift.spec, - expectedIntegrity: drift.expectedIntegrity, - actualIntegrity: drift.actualIntegrity, - resolvedSpec: drift.resolution.resolvedSpec, - resolvedVersion: drift.resolution.version, - dryRun: true, - }; - if (params.onIntegrityDrift) { - return await params.onIntegrityDrift(payload); - } - logger.warn?.( - `Integrity drift for "${pluginId}" (${payload.resolvedSpec ?? payload.spec}): expected ${payload.expectedIntegrity}, got ${payload.actualIntegrity}`, - ); - return true; - }, + onIntegrityDrift: createPluginUpdateIntegrityDriftHandler({ + pluginId, + dryRun: true, + logger, + onIntegrityDrift: params.onIntegrityDrift, + }), logger, }); } catch (err) { @@ -288,24 +312,12 @@ export async function updateNpmInstalledPlugins(params: { mode: "update", expectedPluginId: pluginId, expectedIntegrity: record.integrity, - onIntegrityDrift: async (drift) => { - const payload: PluginUpdateIntegrityDriftParams = { - pluginId, - spec: drift.spec, - expectedIntegrity: drift.expectedIntegrity, - actualIntegrity: drift.actualIntegrity, - resolvedSpec: drift.resolution.resolvedSpec, - resolvedVersion: drift.resolution.version, - dryRun: false, - }; - if (params.onIntegrityDrift) { - return await params.onIntegrityDrift(payload); - } - logger.warn?.( - `Integrity drift for "${pluginId}" (${payload.resolvedSpec ?? payload.spec}): expected ${payload.expectedIntegrity}, got ${payload.actualIntegrity}`, - ); - return true; - }, + onIntegrityDrift: createPluginUpdateIntegrityDriftHandler({ + pluginId, + dryRun: false, + logger, + onIntegrityDrift: params.onIntegrityDrift, + }), logger, }); } catch (err) {