From 2d53ad5cb66e5d781a7fa4ad906118fb45a2a1a7 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 24 Apr 2026 15:42:46 -0700 Subject: [PATCH] fix(channels): harden manifest read-only metadata --- src/channels/plugins/read-only.test.ts | 103 ++++++++++++++++++++++++- src/channels/plugins/read-only.ts | 80 +++++++++++++++---- src/infra/channel-summary.ts | 3 +- 3 files changed, 169 insertions(+), 17 deletions(-) diff --git a/src/channels/plugins/read-only.test.ts b/src/channels/plugins/read-only.test.ts index 35dec06a03a..52c7eb81f7f 100644 --- a/src/channels/plugins/read-only.test.ts +++ b/src/channels/plugins/read-only.test.ts @@ -18,6 +18,8 @@ function writeExternalSetupChannelPlugin( channelId?: string; manifestChannelIds?: string[]; manifestChannelConfig?: boolean; + manifestChannelDescription?: string; + manifestChannelLabel?: string; setupRequiresRuntime?: boolean; setupChannelId?: string; } = {}, @@ -80,8 +82,8 @@ function writeExternalSetupChannelPlugin( sensitive: true, }, }, - label: "External Chat Manifest", - description: "manifest config", + label: options.manifestChannelLabel ?? "External Chat Manifest", + description: options.manifestChannelDescription ?? "manifest config", preferOver: ["legacy-external-chat"], }, ]), @@ -569,6 +571,103 @@ describe("listReadOnlyChannelPluginsForConfig", () => { expect(fs.existsSync(fullMarker)).toBe(false); }); + it("sanitizes terminal control sequences from manifest channel metadata", () => { + const { pluginDir } = writeExternalSetupChannelPlugin({ + setupEntry: false, + pluginId: "external-chat-plugin", + channelId: "external-chat", + manifestChannelConfig: true, + manifestChannelLabel: "External\u001b[31m Chat\u001b[0m", + manifestChannelDescription: "manifest\u001b[2K config", + }); + const plugins = listReadOnlyChannelPluginsForConfig( + { + channels: { + "external-chat": { token: "configured" }, + }, + plugins: { + load: { paths: [pluginDir] }, + allow: ["external-chat-plugin"], + }, + } as never, + { + env: { ...process.env }, + includePersistedAuthState: false, + }, + ); + + const plugin = plugins.find((entry) => entry.id === "external-chat"); + expect(plugin?.meta.label).toBe("External Chat"); + expect(plugin?.meta.selectionLabel).toBe("External Chat"); + expect(plugin?.meta.blurb).toBe("manifest config"); + }); + + it("ignores manifest channel configs with unsafe channel ids", () => { + const unsafeChannelId = "__proto__"; + const { pluginDir, fullMarker, setupMarker } = writeExternalSetupChannelPlugin({ + setupEntry: false, + pluginId: "external-chat-plugin", + channelId: unsafeChannelId, + manifestChannelIds: [unsafeChannelId], + manifestChannelConfig: true, + }); + const plugins = listReadOnlyChannelPluginsForConfig( + { + channels: Object.fromEntries([[unsafeChannelId, { token: "configured" }]]), + plugins: { + load: { paths: [pluginDir] }, + allow: ["external-chat-plugin"], + }, + } as never, + { + env: { ...process.env }, + includePersistedAuthState: false, + }, + ); + + expect(plugins.some((entry) => entry.id === unsafeChannelId)).toBe(false); + expect(fs.existsSync(setupMarker)).toBe(false); + expect(fs.existsSync(fullMarker)).toBe(false); + }); + + it("uses own normalized account ids for manifest channel account config", () => { + const { pluginDir } = writeExternalSetupChannelPlugin({ + setupEntry: false, + pluginId: "external-chat-plugin", + channelId: "external-chat", + manifestChannelConfig: true, + }); + const inheritedAccounts = Object.create({ + inherited: { token: "prototype-token" }, + }) as Record; + inheritedAccounts.default = { token: "default-token" }; + inheritedAccounts.named = { token: "named-token" }; + const cfg = { + channels: { + "external-chat": { + accounts: inheritedAccounts, + }, + }, + plugins: { + load: { paths: [pluginDir] }, + allow: ["external-chat-plugin"], + }, + } as never; + const plugin = listReadOnlyChannelPluginsForConfig(cfg, { + env: { ...process.env }, + includePersistedAuthState: false, + }).find((entry) => entry.id === "external-chat"); + + expect(plugin?.config.listAccountIds(cfg)).toEqual(["default", "named"]); + expect(plugin?.config.resolveAccount(cfg, "__proto__")).toMatchObject({ + accountId: "default", + config: { token: "default-token" }, + }); + expect(plugin?.config.resolveAccount(cfg, "inherited")).not.toMatchObject({ + config: { token: "prototype-token" }, + }); + }); + it("keeps setup-entry precedence when channel config descriptors are not runtime cutoffs", () => { const { pluginDir, fullMarker, setupMarker } = writeExternalSetupChannelPlugin({ pluginId: "external-chat-plugin", diff --git a/src/channels/plugins/read-only.ts b/src/channels/plugins/read-only.ts index dcc86e2c1bb..86d64c2bef9 100644 --- a/src/channels/plugins/read-only.ts +++ b/src/channels/plugins/read-only.ts @@ -1,5 +1,6 @@ import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../../agents/agent-scope.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; +import { isBlockedObjectKey } from "../../infra/prototype-keys.js"; import { hasExplicitChannelConfig, listConfiguredChannelIdsForReadOnlyScope, @@ -10,11 +11,14 @@ import { loadPluginManifestRegistry, type PluginManifestRecord, } from "../../plugins/manifest-registry.js"; -import { DEFAULT_ACCOUNT_ID } from "../../routing/session-key.js"; +import { DEFAULT_ACCOUNT_ID, normalizeAccountId } from "../../routing/session-key.js"; +import { sanitizeForLog } from "../../terminal/ansi.js"; import { getBundledChannelSetupPlugin } from "./bundled.js"; import { listChannelPlugins } from "./registry.js"; import type { ChannelPlugin } from "./types.plugin.js"; +const SAFE_MANIFEST_CHANNEL_ID_PATTERN = /^[a-z0-9][a-z0-9_-]{0,63}$/i; + type ReadOnlyChannelPluginOptions = { env?: NodeJS.ProcessEnv; workspaceDir?: string; @@ -28,6 +32,7 @@ type ReadOnlyChannelPluginResolution = { configuredChannelIds: string[]; missingConfiguredChannelIds: string[]; }; +type ManifestChannelConfigRecord = NonNullable[string]; function addChannelPlugins( byId: Map, @@ -66,6 +71,21 @@ function rebindChannelScopedString( return value; } +function isSafeManifestChannelId(channelId: string): boolean { + return SAFE_MANIFEST_CHANNEL_ID_PATTERN.test(channelId) && !isBlockedObjectKey(channelId); +} + +function readOwnRecordValue(record: Record, key: string): unknown { + if (isBlockedObjectKey(key) || !Object.prototype.hasOwnProperty.call(record, key)) { + return undefined; + } + return record[key]; +} + +function normalizeManifestText(value: string | undefined, fallback: string): string { + return sanitizeForLog(value?.trim() || fallback).trim(); +} + function rebindChannelConfig( cfg: OpenClawConfig, sourceChannelId: string, @@ -113,11 +133,14 @@ function restoreReboundChannelConfig(params: { } function getChannelConfigRecord(cfg: OpenClawConfig, channelId: string): Record { + if (!isSafeManifestChannelId(channelId)) { + return {}; + } const channels = cfg.channels; if (!channels || typeof channels !== "object" || Array.isArray(channels)) { return {}; } - const entry = (channels as Record)[channelId]; + const entry = readOwnRecordValue(channels as Record, channelId); return entry && typeof entry === "object" && !Array.isArray(entry) ? (entry as Record) : {}; @@ -127,7 +150,14 @@ function listManifestChannelAccountIds(cfg: OpenClawConfig, channelId: string): const channelConfig = getChannelConfigRecord(cfg, channelId); const accounts = channelConfig.accounts; if (accounts && typeof accounts === "object" && !Array.isArray(accounts)) { - return Object.keys(accounts).toSorted((left, right) => left.localeCompare(right)); + return [ + ...new Set( + Object.keys(accounts) + .filter((accountId) => !isBlockedObjectKey(accountId)) + .map((accountId) => normalizeAccountId(accountId)) + .filter((accountId) => !isBlockedObjectKey(accountId)), + ), + ].toSorted((left, right) => left.localeCompare(right)); } return hasExplicitChannelConfig({ config: cfg, channelId }) ? [DEFAULT_ACCOUNT_ID] : []; } @@ -138,10 +168,13 @@ function resolveManifestChannelAccountConfig(params: { accountId?: string | null; }): Record { const channelConfig = getChannelConfigRecord(params.cfg, params.channelId); - const resolvedAccountId = params.accountId?.trim() || DEFAULT_ACCOUNT_ID; + const resolvedAccountId = normalizeAccountId(params.accountId); const accounts = channelConfig.accounts; if (accounts && typeof accounts === "object" && !Array.isArray(accounts)) { - const accountConfig = (accounts as Record)[resolvedAccountId]; + const accountConfig = readOwnRecordValue( + accounts as Record, + resolvedAccountId, + ); if (accountConfig && typeof accountConfig === "object" && !Array.isArray(accountConfig)) { return accountConfig as Record; } @@ -153,19 +186,31 @@ function buildManifestChannelPlugin(params: { record: PluginManifestRecord; channelId: string; }): ChannelPlugin | undefined { - const channelConfig = params.record.channelConfigs?.[params.channelId]; - if (!channelConfig) { + if (!isSafeManifestChannelId(params.channelId)) { return undefined; } - const label = channelConfig.label?.trim() || params.record.name || params.channelId; - const blurb = channelConfig.description?.trim() || params.record.description || ""; + const channelConfigValue = params.record.channelConfigs + ? readOwnRecordValue(params.record.channelConfigs as Record, params.channelId) + : undefined; + if ( + !channelConfigValue || + typeof channelConfigValue !== "object" || + Array.isArray(channelConfigValue) + ) { + return undefined; + } + const channelConfig = channelConfigValue as ManifestChannelConfigRecord; + const label = + normalizeManifestText(channelConfig.label, params.record.name || params.channelId) || + params.channelId; + const blurb = normalizeManifestText(channelConfig.description, params.record.description || ""); return { id: params.channelId, meta: { id: params.channelId, label, selectionLabel: label, - docsPath: `/channels/${params.channelId}`, + docsPath: `/channels/${encodeURIComponent(params.channelId)}`, blurb, ...(channelConfig.preferOver?.length ? { preferOver: channelConfig.preferOver } : {}), }, @@ -179,7 +224,7 @@ function buildManifestChannelPlugin(params: { listAccountIds: (cfg) => listManifestChannelAccountIds(cfg, params.channelId), defaultAccountId: () => DEFAULT_ACCOUNT_ID, resolveAccount: (cfg, accountId) => ({ - accountId: accountId?.trim() || DEFAULT_ACCOUNT_ID, + accountId: normalizeAccountId(accountId), config: resolveManifestChannelAccountConfig({ cfg, channelId: params.channelId, @@ -340,7 +385,9 @@ function addSetupChannelPlugins( }, ): void { for (const setup of setups) { - const ownedMissingChannelIds = options.ownedMissingChannelIdsByPluginId.get(setup.pluginId); + const ownedMissingChannelIds = options.ownedMissingChannelIdsByPluginId + .get(setup.pluginId) + ?.filter(isSafeManifestChannelId); if (!ownedMissingChannelIds || ownedMissingChannelIds.length === 0) { continue; } @@ -361,7 +408,9 @@ function addSetupChannelPlugins( ); continue; } - const ownedChannelIds = options.ownedChannelIdsByPluginId.get(setup.pluginId) ?? []; + const ownedChannelIds = (options.ownedChannelIdsByPluginId.get(setup.pluginId) ?? []).filter( + isSafeManifestChannelId, + ); if (setup.plugin.id !== setup.pluginId && !ownedChannelIds.includes(setup.plugin.id)) { continue; } @@ -395,6 +444,9 @@ function addManifestChannelPlugins( continue; } for (const channelId of record.channels) { + if (!isSafeManifestChannelId(channelId)) { + continue; + } if (!channelIds.has(channelId)) { continue; } @@ -487,7 +539,7 @@ export function resolveReadOnlyChannelPluginsForConfig( manifestRecords, }), ), - ]; + ].filter(isSafeManifestChannelId); const byId = new Map(); addChannelPlugins(byId, listChannelPlugins()); diff --git a/src/infra/channel-summary.ts b/src/infra/channel-summary.ts index f5812d5b198..0063f1fc53f 100644 --- a/src/infra/channel-summary.ts +++ b/src/infra/channel-summary.ts @@ -9,6 +9,7 @@ import type { ChannelPlugin } from "../channels/plugins/types.plugin.js"; import type { ChannelAccountSnapshot } from "../channels/plugins/types.public.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { DEFAULT_ACCOUNT_ID } from "../routing/session-key.js"; +import { sanitizeForLog } from "../terminal/ansi.js"; import { theme } from "../terminal/theme.js"; import { formatTimeAgo } from "./format-time/format-relative.ts"; @@ -199,7 +200,7 @@ export async function buildChannelSummary( : status === "not linked" || status === "auth stabilizing" ? theme.error : theme.muted; - const baseLabel = plugin.meta.label ?? plugin.id; + const baseLabel = sanitizeForLog(plugin.meta.label ?? plugin.id).trim() || plugin.id; let line = `${baseLabel}: ${status}`; const authAgeMs =