From a7f0ae9c07ab0f2514840df678be36f994d56386 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Mon, 20 Apr 2026 20:13:44 -0400 Subject: [PATCH] fix: tighten read-only plugin triage paths --- src/channels/plugins/read-only.ts | 42 +++------- src/infra/provider-usage.auth.plugin.test.ts | 24 ++++++ src/infra/provider-usage.auth.ts | 28 +++---- src/plugins/channel-plugin-ids.test.ts | 27 +++++++ src/plugins/channel-plugin-ids.ts | 56 ++++++++++++- .../runtime/runtime-registry-loader.ts | 1 + .../audit-plugin-readonly-scope.test.ts | 81 +++++++++++++++++++ src/security/audit.ts | 36 ++++++--- 8 files changed, 240 insertions(+), 55 deletions(-) create mode 100644 src/security/audit-plugin-readonly-scope.test.ts diff --git a/src/channels/plugins/read-only.ts b/src/channels/plugins/read-only.ts index 652dcd269c7..1b1f01c39df 100644 --- a/src/channels/plugins/read-only.ts +++ b/src/channels/plugins/read-only.ts @@ -1,12 +1,14 @@ import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../../agents/agent-scope.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; -import { resolveDiscoverableScopedChannelPluginIds } from "../../plugins/channel-plugin-ids.js"; +import { + listConfiguredChannelIdsForPluginScope, + resolveDiscoverableScopedChannelPluginIds, +} from "../../plugins/channel-plugin-ids.js"; import { loadOpenClawPlugins } from "../../plugins/loader.js"; import { loadPluginManifestRegistry, type PluginManifestRecord, } from "../../plugins/manifest-registry.js"; -import { listPotentialConfiguredChannelIds } from "../config-presence.js"; import { getBundledChannelSetupPlugin } from "./bundled.js"; import { listChannelPlugins } from "./registry.js"; import type { ChannelPlugin } from "./types.plugin.js"; @@ -48,11 +50,6 @@ function addChannelPlugins( } } -function hasNonEmptyEnvValue(env: NodeJS.ProcessEnv, key: string): boolean { - const value = env[key]; - return typeof value === "string" && value.trim().length > 0; -} - function resolveReadOnlyWorkspaceDir( cfg: OpenClawConfig, options: ReadOnlyChannelPluginOptions, @@ -74,22 +71,6 @@ function listExternalChannelManifestRecords(params: { }).plugins.filter((plugin) => plugin.origin !== "bundled" && plugin.channels.length > 0); } -function listExternalEnvConfiguredChannelIds(params: { - records: readonly PluginManifestRecord[]; - env: NodeJS.ProcessEnv; -}): string[] { - const channelIds = new Set(); - for (const record of params.records) { - for (const channelId of record.channels) { - const envVars = record.channelEnvVars?.[channelId] ?? []; - if (envVars.some((envVar) => hasNonEmptyEnvValue(params.env, envVar))) { - channelIds.add(channelId); - } - } - } - return [...channelIds].toSorted((left, right) => left.localeCompare(right)); -} - function resolveExternalReadOnlyChannelPluginIds(params: { cfg: OpenClawConfig; activationSourceConfig?: OpenClawConfig; @@ -148,15 +129,16 @@ export function listReadOnlyChannelPluginsForConfig( cache: options.cache, }); const configuredChannelIds = [ - ...new Set([ - ...listPotentialConfiguredChannelIds(cfg, env, { - includePersistedAuthState: options.includePersistedAuthState, - }), - ...listExternalEnvConfiguredChannelIds({ - records: externalManifestRecords, + ...new Set( + listConfiguredChannelIdsForPluginScope({ + config: cfg, + workspaceDir, env, + cache: options.cache, + includePersistedAuthState: options.includePersistedAuthState, + manifestRecords: externalManifestRecords, }), - ]), + ), ]; const byId = new Map(); diff --git a/src/infra/provider-usage.auth.plugin.test.ts b/src/infra/provider-usage.auth.plugin.test.ts index 0b5da3a19a8..bda61836e72 100644 --- a/src/infra/provider-usage.auth.plugin.test.ts +++ b/src/infra/provider-usage.auth.plugin.test.ts @@ -69,4 +69,28 @@ describe("resolveProviderAuths plugin boundary", () => { expect(resolveProviderUsageAuthWithPluginMock).not.toHaveBeenCalled(); expect(ensureAuthProfileStoreMock).not.toHaveBeenCalled(); }); + + it("skips plugin usage auth per provider when only another provider has direct credentials", async () => { + await expect( + resolveProviderAuths({ + providers: ["anthropic", "zai"], + skipPluginAuthWithoutCredentialSource: true, + env: { + ANTHROPIC_API_KEY: "sk-ant", + }, + }), + ).resolves.toEqual([ + { + provider: "anthropic", + token: "sk-ant", + }, + ]); + + expect(resolveProviderUsageAuthWithPluginMock).toHaveBeenCalledTimes(1); + expect(resolveProviderUsageAuthWithPluginMock).toHaveBeenCalledWith( + expect.objectContaining({ + provider: "anthropic", + }), + ); + }); }); diff --git a/src/infra/provider-usage.auth.ts b/src/infra/provider-usage.auth.ts index 801323a4004..6df11a2e09b 100644 --- a/src/infra/provider-usage.auth.ts +++ b/src/infra/provider-usage.auth.ts @@ -242,25 +242,25 @@ export async function resolveProviderAuths(params: { env: params.env ?? process.env, agentDir: params.agentDir, }; - const hasDirectCredentialSource = params.providers.some((provider) => - Boolean( + const hasAuthProfileStoreSource = hasAnyAuthProfileStoreSource(params.agentDir); + const auths: ProviderAuth[] = []; + + for (const provider of params.providers) { + const hasDirectCredentialSource = Boolean( resolveProviderApiKeyFromConfig({ state: { ...stateBase, allowAuthProfileStore: false }, providerIds: [provider], }), - ), - ); - const allowAuthProfileStore = - !params.skipPluginAuthWithoutCredentialSource || - hasDirectCredentialSource || - hasAnyAuthProfileStoreSource(params.agentDir); - const state: UsageAuthState = { - ...stateBase, - allowAuthProfileStore, - }; - const auths: ProviderAuth[] = []; + ); + const allowAuthProfileStore = + !params.skipPluginAuthWithoutCredentialSource || + hasDirectCredentialSource || + hasAuthProfileStoreSource; + const state: UsageAuthState = { + ...stateBase, + allowAuthProfileStore, + }; - for (const provider of params.providers) { if (!params.skipPluginAuthWithoutCredentialSource || allowAuthProfileStore) { const pluginAuth = await resolveProviderUsageAuthViaPlugin({ state, diff --git a/src/plugins/channel-plugin-ids.test.ts b/src/plugins/channel-plugin-ids.test.ts index c66237a1302..14f827662a4 100644 --- a/src/plugins/channel-plugin-ids.test.ts +++ b/src/plugins/channel-plugin-ids.test.ts @@ -102,6 +102,17 @@ function createManifestRegistryFixture() { providers: [], cliBackends: [], }, + { + id: "external-env-channel-plugin", + channels: ["external-env-channel"], + channelEnvVars: { + "external-env-channel": ["EXTERNAL_ENV_CHANNEL_TOKEN"], + }, + origin: "config", + enabledByDefault: undefined, + providers: [], + cliBackends: [], + }, { id: "voice-call", channels: [], @@ -586,6 +597,22 @@ describe("resolveConfiguredChannelPluginIds", () => { ).toEqual([]); }); + it("includes trusted external channel owners configured only by manifest env vars", () => { + expect( + resolveConfiguredChannelPluginIds({ + config: { + plugins: { + allow: ["external-env-channel-plugin"], + }, + } as OpenClawConfig, + workspaceDir: "/tmp", + env: { + EXTERNAL_ENV_CHANNEL_TOKEN: "token", + } as NodeJS.ProcessEnv, + }), + ).toEqual(["external-env-channel-plugin"]); + }); + it("blocks bundled activation owners when explicitly disabled", () => { expect( resolveConfiguredChannelPluginIds({ diff --git a/src/plugins/channel-plugin-ids.ts b/src/plugins/channel-plugin-ids.ts index e6d6c9898c7..7eab9d4ebc3 100644 --- a/src/plugins/channel-plugin-ids.ts +++ b/src/plugins/channel-plugin-ids.ts @@ -61,6 +61,56 @@ function normalizeChannelIds(channelIds: Iterable): string[] { ).toSorted((left, right) => left.localeCompare(right)); } +function hasNonEmptyEnvValue(env: NodeJS.ProcessEnv, key: string): boolean { + const value = env[key]; + return typeof value === "string" && value.trim().length > 0; +} + +function listEnvConfiguredManifestChannelIds(params: { + records: readonly PluginManifestRecord[]; + env: NodeJS.ProcessEnv; +}): string[] { + const channelIds = new Set(); + for (const record of params.records) { + for (const channelId of record.channels) { + const envVars = record.channelEnvVars?.[channelId] ?? []; + if (envVars.some((envVar) => hasNonEmptyEnvValue(params.env, envVar))) { + channelIds.add(channelId); + } + } + } + return [...channelIds].toSorted((left, right) => left.localeCompare(right)); +} + +export function listConfiguredChannelIdsForPluginScope(params: { + config: OpenClawConfig; + workspaceDir?: string; + env: NodeJS.ProcessEnv; + cache?: boolean; + includePersistedAuthState?: boolean; + manifestRecords?: readonly PluginManifestRecord[]; +}): string[] { + const records = + params.manifestRecords ?? + loadPluginManifestRegistry({ + config: params.config, + workspaceDir: params.workspaceDir, + env: params.env, + cache: params.cache, + }).plugins; + return [ + ...new Set([ + ...listPotentialConfiguredChannelIds(params.config, params.env, { + includePersistedAuthState: params.includePersistedAuthState, + }), + ...listEnvConfiguredManifestChannelIds({ + records, + env: params.env, + }), + ]), + ].toSorted((left, right) => left.localeCompare(right)); +} + function isChannelPluginEligibleForScopedOwnership(params: { plugin: PluginManifestRecord; normalizedConfig: ReturnType; @@ -222,7 +272,11 @@ export function resolveConfiguredChannelPluginIds(params: { env: NodeJS.ProcessEnv; }): string[] { const configuredChannelIds = new Set( - listPotentialConfiguredChannelIds(params.config, params.env).map((id) => id.trim()), + listConfiguredChannelIdsForPluginScope({ + config: params.config, + workspaceDir: params.workspaceDir, + env: params.env, + }).map((id) => id.trim()), ); if (configuredChannelIds.size === 0) { return []; diff --git a/src/plugins/runtime/runtime-registry-loader.ts b/src/plugins/runtime/runtime-registry-loader.ts index 811b072b751..8be63200466 100644 --- a/src/plugins/runtime/runtime-registry-loader.ts +++ b/src/plugins/runtime/runtime-registry-loader.ts @@ -78,6 +78,7 @@ export function ensurePluginRegistryLoaded(options?: { config?: OpenClawConfig; activationSourceConfig?: OpenClawConfig; env?: NodeJS.ProcessEnv; + workspaceDir?: string; onlyPluginIds?: string[]; }): void { const scope = options?.scope ?? "all"; diff --git a/src/security/audit-plugin-readonly-scope.test.ts b/src/security/audit-plugin-readonly-scope.test.ts new file mode 100644 index 00000000000..fcab0d3704d --- /dev/null +++ b/src/security/audit-plugin-readonly-scope.test.ts @@ -0,0 +1,81 @@ +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +const applyPluginAutoEnableMock = vi.hoisted(() => vi.fn()); +const loadPluginMetadataRegistrySnapshotMock = vi.hoisted(() => vi.fn()); +const resolveConfiguredChannelPluginIdsMock = vi.hoisted(() => vi.fn()); + +vi.mock("../config/plugin-auto-enable.js", () => ({ + applyPluginAutoEnable: (...args: unknown[]) => applyPluginAutoEnableMock(...args), +})); + +vi.mock("../plugins/channel-plugin-ids.js", () => ({ + resolveConfiguredChannelPluginIds: (...args: unknown[]) => + resolveConfiguredChannelPluginIdsMock(...args), +})); + +vi.mock("../plugins/runtime/metadata-registry-loader.js", () => ({ + loadPluginMetadataRegistrySnapshot: (...args: unknown[]) => + loadPluginMetadataRegistrySnapshotMock(...args), +})); + +let runSecurityAudit: typeof import("./audit.js").runSecurityAudit; + +describe("security audit read-only plugin scope", () => { + beforeAll(async () => { + ({ runSecurityAudit } = await import("./audit.js")); + }); + + beforeEach(() => { + applyPluginAutoEnableMock.mockReset(); + loadPluginMetadataRegistrySnapshotMock.mockReset(); + resolveConfiguredChannelPluginIdsMock.mockReset(); + applyPluginAutoEnableMock.mockImplementation((params: { config: unknown }) => ({ + config: params.config, + changes: [], + autoEnabledReasons: {}, + })); + loadPluginMetadataRegistrySnapshotMock.mockReturnValue({ + securityAuditCollectors: [], + }); + resolveConfiguredChannelPluginIdsMock.mockReturnValue([]); + }); + + it("removes configured channel owner plugin ids before loading audit collectors", async () => { + const sourceConfig = { + plugins: { + allow: ["external-channel-plugin", "audit-plugin"], + }, + }; + applyPluginAutoEnableMock.mockReturnValue({ + config: sourceConfig, + changes: [], + autoEnabledReasons: { + "external-channel-plugin": ["channel:external"], + "audit-plugin": ["explicit"], + }, + }); + resolveConfiguredChannelPluginIdsMock.mockReturnValue(["external-channel-plugin"]); + + await runSecurityAudit({ + config: sourceConfig, + sourceConfig, + env: {} as NodeJS.ProcessEnv, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [], + }); + + expect(resolveConfiguredChannelPluginIdsMock).toHaveBeenCalledWith( + expect.objectContaining({ + config: sourceConfig, + activationSourceConfig: sourceConfig, + env: {}, + }), + ); + expect(loadPluginMetadataRegistrySnapshotMock).toHaveBeenCalledWith( + expect.objectContaining({ + onlyPluginIds: ["audit-plugin"], + }), + ); + }); +}); diff --git a/src/security/audit.ts b/src/security/audit.ts index 27a4d8b5483..1bb8267114c 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -1,9 +1,7 @@ import path from "node:path"; +import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../agents/agent-scope.js"; import { resolveSandboxConfigForAgent } from "../agents/sandbox/config.js"; -import { - hasPotentialConfiguredChannels, - listPotentialConfiguredChannelIds, -} from "../channels/config-presence.js"; +import { hasPotentialConfiguredChannels } from "../channels/config-presence.js"; import type { listChannelPlugins } from "../channels/plugins/index.js"; import type { ConfigFileSnapshot, OpenClawConfig } from "../config/config.js"; import { resolveConfigPath, resolveStateDir } from "../config/paths.js"; @@ -16,6 +14,7 @@ import { } from "../infra/exec-safe-bin-runtime-policy.js"; import { listRiskyConfiguredSafeBins } from "../infra/exec-safe-bin-semantics.js"; import { normalizeTrustedSafeBinDirs } from "../infra/exec-safe-bin-trust.js"; +import { resolveConfiguredChannelPluginIds } from "../plugins/channel-plugin-ids.js"; import { getActivePluginRegistry } from "../plugins/runtime.js"; import { DEFAULT_AGENT_ID } from "../routing/session-key.js"; import { asNullableRecord } from "../shared/record-coerce.js"; @@ -73,6 +72,8 @@ export type SecurityAuditOptions = { codeSafetySummaryCache?: Map>; /** Optional explicit auth for deep gateway probe. */ deepProbeAuth?: { token?: string; password?: string }; + /** Override workspace used for workspace plugin discovery. */ + workspaceDir?: string; /** Dependency injection for tests. */ probeGatewayFn?: ProbeGatewayFn; }; @@ -95,6 +96,7 @@ type AuditExecutionContext = { configSnapshot: ConfigFileSnapshot | null; codeSafetySummaryCache: Map>; deepProbeAuth?: { token?: string; password?: string }; + workspaceDir?: string; }; let channelPluginsModulePromise: Promise | undefined; @@ -356,11 +358,13 @@ async function collectPluginSecurityAuditFindings( } } if (context.includeChannelSecurity && context.plugins !== undefined) { - for (const channelId of listPotentialConfiguredChannelIds( - context.sourceConfig, - context.env, - )) { - requestedPluginIds.delete(channelId); + for (const pluginId of resolveConfiguredChannelPluginIds({ + config: autoEnabled.config, + activationSourceConfig: context.sourceConfig, + workspaceDir: context.workspaceDir, + env: context.env, + })) { + requestedPluginIds.delete(pluginId); } } if (requestedPluginIds.size === 0) { @@ -372,6 +376,7 @@ async function collectPluginSecurityAuditFindings( config: autoEnabled.config, activationSourceConfig: context.sourceConfig, env: context.env, + workspaceDir: context.workspaceDir, onlyPluginIds: [...requestedPluginIds], }); collectors = snapshot.securityAuditCollectors ?? []; @@ -894,6 +899,8 @@ async function createAuditExecutionContext( const deepTimeoutMs = Math.max(250, opts.deepTimeoutMs ?? 5000); const stateDir = opts.stateDir ?? resolveStateDir(env); const configPath = opts.configPath ?? resolveConfigPath(env, stateDir); + const workspaceDir = + opts.workspaceDir ?? resolveAgentWorkspaceDir(cfg, resolveDefaultAgentId(cfg)); const { readConfigSnapshotForAudit } = await loadAuditNonDeepModule(); const configSnapshot = includeFilesystem ? opts.configSnapshot !== undefined @@ -915,6 +922,7 @@ async function createAuditExecutionContext( execDockerRawFn: opts.execDockerRawFn, probeGatewayFn: opts.probeGatewayFn, plugins: opts.plugins, + workspaceDir, configSnapshot, codeSafetySummaryCache: opts.codeSafetySummaryCache ?? new Map>(), deepProbeAuth: opts.deepProbeAuth, @@ -997,13 +1005,21 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise 0); if (shouldAuditChannelSecurity) { if (context.plugins === undefined) { (await loadPluginRegistryLoaderModule()).ensurePluginRegistryLoaded({ scope: "configured-channels", config: cfg, activationSourceConfig: context.sourceConfig, + workspaceDir: context.workspaceDir, env, }); }