fix(plugins): enforce own install record lookups (#102872)

* fix(plugins): enforce own install record lookups

Co-authored-by: Alix-007 <li.long15@xydigit.com>

* test(plugins): use valid prototype-key package fixtures

---------

Co-authored-by: Alix-007 <li.long15@xydigit.com>
This commit is contained in:
Peter Steinberger 2026-07-09 15:54:21 +01:00 committed by GitHub
parent 4bf348bec8
commit a99ecff78a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 204 additions and 22 deletions

View file

@ -105,7 +105,7 @@ export async function runPluginUninstallCommand(
runtime.exit(1);
return;
}
const hasInstall = pluginId in (cfg.plugins?.installs ?? {});
const hasInstall = Object.hasOwn(cfg.plugins?.installs ?? {}, pluginId);
const preview: string[] = [];
if (plan.actions.entry) {

View file

@ -1,7 +1,11 @@
// Plugin update selection tests cover CLI plugin update target selection.
import { describe, expect, it } from "vitest";
import type { HookInstallRecord } from "../config/types.hooks.js";
import type { PluginInstallRecord } from "../config/types.plugins.js";
import { resolvePluginUpdateSelection } from "./plugins-update-selection.js";
import {
resolveHookPackUpdateSelection,
resolvePluginUpdateSelection,
} from "./plugins-update-selection.js";
function createNpmInstall(params: {
spec: string;
@ -16,6 +20,19 @@ function createNpmInstall(params: {
};
}
function createNpmHookInstall(params: {
spec: string;
installPath?: string;
resolvedName?: string;
}): HookInstallRecord {
return {
source: "npm",
spec: params.spec,
installPath: params.installPath ?? "/tmp/hook-pack",
...(params.resolvedName ? { resolvedName: params.resolvedName } : {}),
};
}
describe("resolvePluginUpdateSelection", () => {
it("maps an explicit unscoped npm dist-tag update to the tracked plugin id", () => {
expect(
@ -113,4 +130,52 @@ describe("resolvePluginUpdateSelection", () => {
},
});
});
it("maps prototype-named npm packages by own install records", () => {
expect(
resolvePluginUpdateSelection({
installs: {
"tracked-constructor": createNpmInstall({
spec: "constructor",
resolvedName: "constructor",
}),
},
rawId: "constructor",
}),
).toEqual({
pluginIds: ["tracked-constructor"],
specOverrides: {
"tracked-constructor": "constructor",
},
});
});
});
describe("resolveHookPackUpdateSelection", () => {
it("does not treat inherited prototype keys as installed hook ids", () => {
expect(
resolveHookPackUpdateSelection({
installs: {},
rawId: "constructor",
}),
).toEqual({
hookIds: [],
});
});
it("keeps own prototype-named hook ids selectable", () => {
expect(
resolveHookPackUpdateSelection({
installs: {
constructor: createNpmHookInstall({
spec: "openclaw-hooks-constructor",
resolvedName: "openclaw-hooks-constructor",
}),
},
rawId: "constructor",
}),
).toEqual({
hookIds: ["constructor"],
});
});
});

View file

@ -20,7 +20,7 @@ export function resolvePluginUpdateSelection(params: {
return { pluginIds: [] };
}
if (params.rawId in params.installs) {
if (Object.hasOwn(params.installs, params.rawId)) {
return { pluginIds: [params.rawId] };
}
@ -67,7 +67,7 @@ export function resolveHookPackUpdateSelection(params: {
if (!params.rawId) {
return { hookIds: [] };
}
if (params.rawId in params.installs) {
if (Object.hasOwn(params.installs, params.rawId)) {
return { hookIds: [params.rawId] };
}

View file

@ -743,6 +743,36 @@ describe("uninstallPlugin", () => {
}
});
it("does not treat inherited prototype names as installed plugins", async () => {
const result = await uninstallPlugin({
config: createPluginConfig({ entries: {}, installs: {} }),
pluginId: "constructor",
deleteFiles: false,
});
expect(result).toEqual({ ok: false, error: "Plugin not found: constructor" });
});
it("uninstalls own prototype-named plugin records", async () => {
const result = await uninstallPlugin({
config: createPluginConfig({
entries: {
constructor: { enabled: true },
},
installs: {
constructor: createNpmInstallRecord("constructor"),
},
}),
pluginId: "constructor",
deleteFiles: false,
});
const successfulResult = expectSuccessfulUninstall(result);
expect(successfulResult.config.plugins).toBeUndefined();
expect(successfulResult.actions.entry).toBe(true);
expect(successfulResult.actions.install).toBe(true);
});
it("cleans stale policy references even when plugin code and install records are gone", async () => {
const result = await uninstallPlugin({
config: createPluginConfig({

View file

@ -392,7 +392,7 @@ export function removePluginFromConfig(
// Remove from entries
let entries = pluginsConfig.entries;
if (entries && pluginId in entries) {
if (entries && Object.hasOwn(entries, pluginId)) {
const { [pluginId]: _, ...rest } = entries;
entries = Object.keys(rest).length > 0 ? rest : undefined;
actions.entry = true;
@ -400,8 +400,9 @@ export function removePluginFromConfig(
// Remove from installs
let installs = pluginsConfig.installs;
const installRecord = installs?.[pluginId];
if (installs && pluginId in installs) {
const hasInstallRecord = Object.hasOwn(installs ?? {}, pluginId);
const installRecord = hasInstallRecord ? installs?.[pluginId] : undefined;
if (installs && hasInstallRecord) {
const { [pluginId]: _, ...rest } = installs;
installs = Object.keys(rest).length > 0 ? rest : undefined;
actions.install = true;
@ -498,7 +499,6 @@ export function removePluginFromConfig(
// Remove channel config owned by this installed plugin.
// Built-in channels have no install record, so keep their config untouched.
const hasInstallRecord = Object.hasOwn(cfg.plugins?.installs ?? {}, pluginId);
let channels = cfg.channels as Record<string, unknown> | undefined;
if (hasInstallRecord && channels) {
for (const key of resolveUninstallChannelConfigKeys(pluginId, opts)) {
@ -539,9 +539,11 @@ export type UninstallPluginParams = {
export function planPluginUninstall(params: UninstallPluginParams): PluginUninstallPlanResult {
const { config, pluginId, channelIds, deleteFiles = true, extensionsDir } = params;
const hasEntry = pluginId in (config.plugins?.entries ?? {});
const hasInstall = pluginId in (config.plugins?.installs ?? {});
const installRecord = config.plugins?.installs?.[pluginId];
const entries = config.plugins?.entries ?? {};
const installs = config.plugins?.installs ?? {};
const hasEntry = Object.hasOwn(entries, pluginId);
const hasInstall = Object.hasOwn(installs, pluginId);
const installRecord = hasInstall ? installs[pluginId] : undefined;
const isLinked = isLinkedPathInstallRecord(installRecord);
// Remove from config

View file

@ -555,6 +555,26 @@ describe("updateNpmInstalledPlugins", () => {
}
});
it("does not treat inherited prototype names as install records", async () => {
const config: OpenClawConfig = { plugins: { installs: {} } };
const result = await updateNpmInstalledPlugins({
config,
pluginIds: ["constructor"],
});
expect(installPluginFromNpmSpecMock).not.toHaveBeenCalled();
expect(result.changed).toBe(false);
expect(result.config).toBe(config);
expect(result.outcomes).toEqual([
{
pluginId: "constructor",
status: "skipped",
message: 'No install record for "constructor".',
},
]);
});
it.each([
{
name: "skips integrity drift checks for unpinned npm specs during dry-run updates",
@ -5377,6 +5397,56 @@ describe("syncPluginsForUpdateChannel", () => {
});
});
it.each(["constructor", "__proto__"])(
"migrates already-externalized records to prototype-named plugin id %s",
async (targetPluginId) => {
const legacyPluginId = `legacy-${targetPluginId}`;
const npmPackageName = `openclaw-plugin-${targetPluginId}`;
resolveBundledPluginSourcesMock.mockReturnValue(new Map());
const result = await syncPluginsForUpdateChannel({
channel: "stable",
externalizedBundledPluginBridges: [
{
bundledPluginId: legacyPluginId,
pluginId: targetPluginId,
npmSpec: npmPackageName,
channelIds: [],
},
],
config: {
plugins: {
entries: {
[legacyPluginId]: { enabled: true },
},
installs: {
[legacyPluginId]: {
source: "npm",
spec: npmPackageName,
installPath: `/tmp/${targetPluginId}`,
},
},
},
},
});
expect(installPluginFromNpmSpecMock).not.toHaveBeenCalled();
expect(result.changed).toBe(true);
expect(Object.hasOwn(result.config.plugins?.entries ?? {}, targetPluginId)).toBe(true);
expect(Object.getPrototypeOf(result.config.plugins?.entries ?? {})).toBe(Object.prototype);
expect(result.config.plugins?.entries?.[targetPluginId]).toEqual({ enabled: true });
expect(Object.hasOwn(result.config.plugins?.installs ?? {}, targetPluginId)).toBe(true);
expect(Object.getPrototypeOf(result.config.plugins?.installs ?? {})).toBe(Object.prototype);
expectRecordFields(result.config.plugins?.installs?.[targetPluginId], {
source: "npm",
spec: npmPackageName,
installPath: `/tmp/${targetPluginId}`,
});
expect(result.config.plugins?.entries?.[legacyPluginId]).toBeUndefined();
expect(result.config.plugins?.installs?.[legacyPluginId]).toBeUndefined();
},
);
it("removes stale bundled load paths for already-externalized npm installs", async () => {
resolveBundledPluginSourcesMock.mockReturnValue(new Map());

View file

@ -733,6 +733,9 @@ function resolveBridgeInstallRecord(params: {
bridge: ExternalizedBundledPluginBridge;
}): { pluginId: string; record: PluginInstallRecord } | undefined {
for (const pluginId of getExternalizedBundledPluginLookupIds(params.bridge)) {
if (!Object.hasOwn(params.installs, pluginId)) {
continue;
}
const record = params.installs[pluginId];
if (record) {
return { pluginId, record };
@ -1169,10 +1172,16 @@ function migratePluginConfigId(cfg: OpenClawConfig, fromId: string, toId: string
const installs = plugins.installs;
if (installs && Object.hasOwn(installs, fromId)) {
const record = installs[fromId];
const nextInstalls = { ...installs };
const record = nextInstalls[fromId];
if (record && !(toId in nextInstalls)) {
nextInstalls[toId] = record;
if (record && !Object.hasOwn(installs, toId)) {
// Plugin ids are record keys; define data properties so "__proto__" cannot invoke its setter.
Object.defineProperty(nextInstalls, toId, {
configurable: true,
enumerable: true,
value: record,
writable: true,
});
}
delete nextInstalls[fromId];
ensureNextPlugins().installs = nextInstalls;
@ -1180,15 +1189,21 @@ function migratePluginConfigId(cfg: OpenClawConfig, fromId: string, toId: string
const entries = plugins.entries;
if (entries && Object.hasOwn(entries, fromId)) {
const entry = entries[fromId];
const existingEntry = Object.hasOwn(entries, toId) ? entries[toId] : undefined;
const nextEntries = { ...entries };
const entry = nextEntries[fromId];
if (entry) {
nextEntries[toId] = nextEntries[toId]
? {
...entry,
...nextEntries[toId],
}
: entry;
Object.defineProperty(nextEntries, toId, {
configurable: true,
enumerable: true,
value: existingEntry
? {
...entry,
...existingEntry,
}
: entry,
writable: true,
});
}
delete nextEntries[fromId];
ensureNextPlugins().entries = nextEntries;
@ -1447,7 +1462,7 @@ export async function updateNpmInstalledPlugins(params: {
continue;
}
const record = installs[pluginId];
const record = Object.hasOwn(installs, pluginId) ? installs[pluginId] : undefined;
if (!record) {
outcomes.push({
pluginId,