diff --git a/CHANGELOG.md b/CHANGELOG.md index a7892a33e51..7457b1b954e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ Docs: https://docs.openclaw.ai - Channels/Discord: let text-only configs drop the `GuildVoiceStates` gateway intent and expose a bounded `/gateway/bot` metadata timeout with rate-limited fallback logs, reducing idle CPU and warning floods. Fixes #73709 and #73585. Thanks @sanchezm86 and @trac3r00. - Agents/sessions: mark same-turn `sessions_send` and A2A reply prompts with an inter-session `isUser=false` envelope before they reach the model, so foreign session output no longer lands as bare active user text. Fixes #73702; refs #73698, #73609, #73595, and #73622. Thanks @alvelda. - Outbound/security: strip known internal runtime scaffolding such as `` and `` at the final channel delivery boundary and keep Discord output on targeted tag stripping, so degraded harness replies cannot leak those tags to users. Fixes #73595. Thanks @gabrielexito-stack and @martingarramon. +- Security/Telegram: load Telegram security adapters in read-only audit/doctor, audit malformed Telegram DM `allowFrom` entries even when groups are disabled, and keep allowlist DM audits from counting stale pairing-store senders, so public/shared-DM risk checks stay accurate. Refs #73698. Thanks @xace1825. - CLI/plugins: use plugin metadata snapshots for install slot selection and add opt-in plugin lifecycle timing traces, so plugin install avoids runtime-loading the plugin registry for metadata-only decisions. Thanks @shakkernerd. - fix(plugins): restrict bundled plugin dir resolution to trusted package roots. (#73275) Thanks @pgondhi987. - fix(security): prevent workspace PATH injection via service env and trash helpers. (#73264) Thanks @pgondhi987. diff --git a/extensions/telegram/src/security-audit.test.ts b/extensions/telegram/src/security-audit.test.ts index f389d1f4b57..7f5eebbdffb 100644 --- a/extensions/telegram/src/security-audit.test.ts +++ b/extensions/telegram/src/security-audit.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../runtime-api.js"; import type { ResolvedTelegramAccount } from "./accounts.js"; import { collectTelegramSecurityAuditFindings } from "./security-audit.js"; @@ -32,6 +32,11 @@ function getTelegramConfig(cfg: OpenClawConfig) { } describe("Telegram security audit findings", () => { + beforeEach(() => { + readChannelAllowFromStoreMock.mockReset(); + readChannelAllowFromStoreMock.mockResolvedValue([]); + }); + it("flags group commands without a sender allowlist", async () => { const cfg: OpenClawConfig = { channels: { @@ -44,7 +49,6 @@ describe("Telegram security audit findings", () => { }, }; - readChannelAllowFromStoreMock.mockResolvedValue([]); const findings = await collectTelegramSecurityAuditFindings({ cfg, account: createTelegramAccount(getTelegramConfig(cfg)), @@ -74,7 +78,6 @@ describe("Telegram security audit findings", () => { }, }; - readChannelAllowFromStoreMock.mockResolvedValue([]); const findings = await collectTelegramSecurityAuditFindings({ cfg, account: createTelegramAccount(getTelegramConfig(cfg)), @@ -90,4 +93,61 @@ describe("Telegram security audit findings", () => { ]), ); }); + + it("warns about invalid DM allowFrom entries even when groups are not enabled", async () => { + const cfg: OpenClawConfig = { + channels: { + telegram: { + enabled: true, + botToken: "t", + dmPolicy: "allowlist", + allowFrom: ["@TrustedOperator"], + groupPolicy: "allowlist", + }, + }, + }; + + const findings = await collectTelegramSecurityAuditFindings({ + cfg, + account: createTelegramAccount(getTelegramConfig(cfg)), + accountId: "default", + }); + + expect(findings).toEqual([ + expect.objectContaining({ + checkId: "channels.telegram.allowFrom.invalid_entries", + severity: "warn", + }), + ]); + expect(readChannelAllowFromStoreMock).not.toHaveBeenCalled(); + }); + + it("warns about invalid DM allowFrom entries when text commands are disabled", async () => { + const cfg: OpenClawConfig = { + commands: { text: false }, + channels: { + telegram: { + enabled: true, + botToken: "t", + dmPolicy: "allowlist", + allowFrom: ["@TrustedOperator"], + groupPolicy: "allowlist", + }, + }, + }; + + const findings = await collectTelegramSecurityAuditFindings({ + cfg, + account: createTelegramAccount(getTelegramConfig(cfg)), + accountId: "default", + }); + + expect(findings).toEqual([ + expect.objectContaining({ + checkId: "channels.telegram.allowFrom.invalid_entries", + severity: "warn", + }), + ]); + expect(readChannelAllowFromStoreMock).not.toHaveBeenCalled(); + }); }); diff --git a/extensions/telegram/src/security-audit.ts b/extensions/telegram/src/security-audit.ts index b3191fc6dca..174e82f9757 100644 --- a/extensions/telegram/src/security-audit.ts +++ b/extensions/telegram/src/security-audit.ts @@ -24,6 +24,36 @@ function collectInvalidTelegramAllowFromEntries(params: { entries: unknown; targ } } +function appendInvalidTelegramAllowFromFinding( + findings: Array<{ + checkId: string; + severity: "info" | "warn" | "critical"; + title: string; + detail: string; + remediation?: string; + }>, + invalidTelegramAllowFromEntries: Set, +) { + if (invalidTelegramAllowFromEntries.size === 0) { + return; + } + const examples = Array.from(invalidTelegramAllowFromEntries).slice(0, 5); + const more = + invalidTelegramAllowFromEntries.size > examples.length + ? ` (+${invalidTelegramAllowFromEntries.size - examples.length} more)` + : ""; + findings.push({ + checkId: "channels.telegram.allowFrom.invalid_entries", + severity: "warn", + title: "Telegram allowlist contains non-numeric entries", + detail: + "Telegram sender authorization requires numeric Telegram user IDs. " + + `Found non-numeric allowFrom entries: ${examples.join(", ")}${more}.`, + remediation: + "Replace @username entries with numeric Telegram user IDs (use setup to resolve), then re-run the audit.", + }); +} + export async function collectTelegramSecurityAuditFindings(params: { cfg: OpenClawConfig; accountId?: string | null; @@ -36,13 +66,20 @@ export async function collectTelegramSecurityAuditFindings(params: { detail: string; remediation?: string; }> = []; - if (params.cfg.commands?.text === false) { - return findings; - } const telegramCfg = params.account.config ?? {}; const accountId = normalizeOptionalString(params.accountId) ?? params.account.accountId ?? "default"; + const invalidTelegramAllowFromEntries = new Set(); + collectInvalidTelegramAllowFromEntries({ + entries: Array.isArray(telegramCfg.allowFrom) ? telegramCfg.allowFrom : [], + target: invalidTelegramAllowFromEntries, + }); + if (params.cfg.commands?.text === false) { + appendInvalidTelegramAllowFromFinding(findings, invalidTelegramAllowFromEntries); + return findings; + } + const defaultGroupPolicy = params.cfg.channels?.defaults?.groupPolicy; const groupPolicy = (telegramCfg.groupPolicy as string | undefined) ?? defaultGroupPolicy ?? "allowlist"; @@ -51,6 +88,7 @@ export async function collectTelegramSecurityAuditFindings(params: { const groupAccessPossible = groupPolicy === "open" || (groupPolicy === "allowlist" && groupsConfigured); if (!groupAccessPossible) { + appendInvalidTelegramAllowFromFinding(findings, invalidTelegramAllowFromEntries); return findings; } @@ -60,7 +98,6 @@ export async function collectTelegramSecurityAuditFindings(params: { const storeHasWildcard = storeAllowFrom.some( (value) => (normalizeOptionalString(value) ?? "") === "*", ); - const invalidTelegramAllowFromEntries = new Set(); collectInvalidTelegramAllowFromEntries({ entries: storeAllowFrom, target: invalidTelegramAllowFromEntries, @@ -75,10 +112,6 @@ export async function collectTelegramSecurityAuditFindings(params: { entries: groupAllowFrom, target: invalidTelegramAllowFromEntries, }); - collectInvalidTelegramAllowFromEntries({ - entries: Array.isArray(telegramCfg.allowFrom) ? telegramCfg.allowFrom : [], - target: invalidTelegramAllowFromEntries, - }); let anyGroupOverride = false; if (groups) { @@ -119,23 +152,7 @@ export async function collectTelegramSecurityAuditFindings(params: { const hasAnySenderAllowlist = storeAllowFrom.length > 0 || groupAllowFrom.length > 0 || anyGroupOverride; - if (invalidTelegramAllowFromEntries.size > 0) { - const examples = Array.from(invalidTelegramAllowFromEntries).slice(0, 5); - const more = - invalidTelegramAllowFromEntries.size > examples.length - ? ` (+${invalidTelegramAllowFromEntries.size - examples.length} more)` - : ""; - findings.push({ - checkId: "channels.telegram.allowFrom.invalid_entries", - severity: "warn", - title: "Telegram allowlist contains non-numeric entries", - detail: - "Telegram sender authorization requires numeric Telegram user IDs. " + - `Found non-numeric allowFrom entries: ${examples.join(", ")}${more}.`, - remediation: - "Replace @username entries with numeric Telegram user IDs (use setup to resolve), then re-run the audit.", - }); - } + appendInvalidTelegramAllowFromFinding(findings, invalidTelegramAllowFromEntries); if (storeHasWildcard || groupAllowFromHasWildcard) { findings.push({ diff --git a/src/commands/doctor-security.test.ts b/src/commands/doctor-security.test.ts index 24fd49db866..6701daf84e4 100644 --- a/src/commands/doctor-security.test.ts +++ b/src/commands/doctor-security.test.ts @@ -202,7 +202,7 @@ describe("noteSecurityWarnings gateway exposure", () => { await noteSecurityWarnings(cfg); expect(listReadOnlyChannelPluginsForConfigMock).toHaveBeenCalledWith(cfg, { includePersistedAuthState: true, - includeSetupRuntimeFallback: false, + includeSetupRuntimeFallback: true, }); const message = lastMessage(); expect(message).toContain('config set session.dmScope "per-channel-peer"'); @@ -465,7 +465,7 @@ describe("noteSecurityWarnings gateway exposure", () => { {}, { includePersistedAuthState: true, - includeSetupRuntimeFallback: false, + includeSetupRuntimeFallback: true, }, ); const message = lastMessage(); diff --git a/src/commands/doctor-security.ts b/src/commands/doctor-security.ts index 2a6f46a8b4a..05e2a28d062 100644 --- a/src/commands/doctor-security.ts +++ b/src/commands/doctor-security.ts @@ -269,6 +269,7 @@ export async function noteSecurityWarnings(cfg: OpenClawConfig) { provider: params.provider, accountId: params.accountId, allowFrom: params.allowFrom, + dmPolicy, normalizeEntry: params.normalizeEntry, }); const dmScope = cfg.session?.dmScope ?? "main"; @@ -306,7 +307,7 @@ export async function noteSecurityWarnings(cfg: OpenClawConfig) { for (const plugin of listReadOnlyChannelPluginsForConfig(cfg, { includePersistedAuthState: true, - includeSetupRuntimeFallback: false, + includeSetupRuntimeFallback: true, })) { if (!plugin.security) { continue; diff --git a/src/security/audit-channel-dm-policy.test.ts b/src/security/audit-channel-dm-policy.test.ts index 162d3eb6174..30bb1442210 100644 --- a/src/security/audit-channel-dm-policy.test.ts +++ b/src/security/audit-channel-dm-policy.test.ts @@ -54,4 +54,58 @@ describe("security audit channel dm policy", () => { ]), ); }); + + it("flags public DMs and shared main-session scope together", async () => { + const cfg: OpenClawConfig = { + session: { dmScope: "main" }, + channels: { telegram: { enabled: true } }, + }; + const plugins: ChannelPlugin[] = [ + { + id: "telegram", + meta: { + id: "telegram", + label: "Telegram", + selectionLabel: "Telegram", + docsPath: "/channels/telegram", + blurb: "Test", + }, + capabilities: { chatTypes: ["direct"] }, + config: { + listAccountIds: () => ["default"], + inspectAccount: () => ({ enabled: true, configured: true }), + resolveAccount: () => ({}), + isEnabled: () => true, + isConfigured: () => true, + }, + security: { + resolveDmPolicy: () => ({ + policy: "open", + allowFrom: ["*"], + policyPath: "channels.telegram.dmPolicy", + allowFromPath: "channels.telegram.", + approveHint: "approve", + }), + }, + }, + ]; + + const findings = await collectChannelSecurityFindings({ + cfg, + plugins, + }); + + expect(findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: "channels.telegram.dm.open", + severity: "critical", + }), + expect.objectContaining({ + checkId: "channels.telegram.dm.scope_main_multiuser", + severity: "warn", + }), + ]), + ); + }); }); diff --git a/src/security/audit-channel-readonly-setup-fallback.test.ts b/src/security/audit-channel-readonly-setup-fallback.test.ts new file mode 100644 index 00000000000..71f6cf08cbb --- /dev/null +++ b/src/security/audit-channel-readonly-setup-fallback.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it, vi } from "vitest"; +import type { ChannelPlugin } from "../channels/plugins/types.plugin.js"; +import type { OpenClawConfig } from "../config/config.js"; + +const { listReadOnlyChannelPluginsForConfigMock, hasConfiguredChannelsForReadOnlyScopeMock } = + vi.hoisted(() => ({ + listReadOnlyChannelPluginsForConfigMock: vi.fn(), + hasConfiguredChannelsForReadOnlyScopeMock: vi.fn(), + })); + +vi.mock("../channels/plugins/read-only.js", () => ({ + listReadOnlyChannelPluginsForConfig: (...args: unknown[]) => + listReadOnlyChannelPluginsForConfigMock(...args), +})); + +vi.mock("../plugins/channel-plugin-ids.js", () => ({ + hasConfiguredChannelsForReadOnlyScope: (...args: unknown[]) => + hasConfiguredChannelsForReadOnlyScopeMock(...args), + resolveConfiguredChannelPluginIds: () => [], +})); + +const { runSecurityAudit } = await import("./audit.js"); + +describe("security audit channel read-only setup fallback", () => { + it("uses setup fallback plugins so bundled channel security adapters are audited", async () => { + const plugin = { + id: "telegram", + meta: { + id: "telegram", + label: "Telegram", + selectionLabel: "Telegram", + docsPath: "/channels/telegram", + blurb: "Test", + }, + capabilities: { chatTypes: ["direct"] }, + config: { + listAccountIds: () => ["default"], + inspectAccount: () => ({ enabled: true, configured: true }), + resolveAccount: () => ({}), + isEnabled: () => true, + isConfigured: () => true, + }, + security: { + resolveDmPolicy: () => ({ + policy: "open", + allowFrom: ["*"], + policyPath: "channels.telegram.dmPolicy", + allowFromPath: "channels.telegram.", + approveHint: "approve", + }), + }, + } satisfies ChannelPlugin; + const cfg = { + session: { dmScope: "main" }, + channels: { telegram: { enabled: true } }, + } satisfies OpenClawConfig; + + hasConfiguredChannelsForReadOnlyScopeMock.mockReturnValue(true); + listReadOnlyChannelPluginsForConfigMock.mockReturnValue([plugin]); + + const report = await runSecurityAudit({ + config: cfg, + sourceConfig: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + loadPluginSecurityCollectors: false, + }); + + expect(listReadOnlyChannelPluginsForConfigMock).toHaveBeenCalledWith( + cfg, + expect.objectContaining({ + includePersistedAuthState: true, + includeSetupRuntimeFallback: true, + }), + ); + expect(report.findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ checkId: "channels.telegram.dm.open" }), + expect.objectContaining({ checkId: "channels.telegram.dm.scope_main_multiuser" }), + ]), + ); + }); +}); diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index 8feb38df275..4e585bce8fc 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -210,6 +210,7 @@ export async function collectChannelSecurityFindings(params: { provider: input.provider, accountId: input.accountId, allowFrom: input.allowFrom, + dmPolicy: input.dmPolicy, normalizeEntry: input.normalizeEntry, }); const dmScope = params.cfg.session?.dmScope ?? "main"; diff --git a/src/security/audit.ts b/src/security/audit.ts index 311a323dc62..57e0a3894a3 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -1059,7 +1059,7 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise { expect(state.isMultiUserDm).toBe(false); }); + it("does not count pairing-store senders for allowlist DM policy", async () => { + let called = false; + const state = await resolveDmAllowState({ + provider: "demo-channel-c" as never, + accountId: "default", + dmPolicy: "allowlist", + allowFrom: ["owner"], + readStore: async (_provider, _accountId) => { + called = true; + return ["paired-user"]; + }, + }); + + expect(called).toBe(false); + expect(state.allowCount).toBe(1); + expect(state.isMultiUserDm).toBe(false); + }); + it.each([ { name: "dmPolicy is allowlist", diff --git a/src/security/dm-policy-shared.ts b/src/security/dm-policy-shared.ts index 79c9e93e705..430607b72a4 100644 --- a/src/security/dm-policy-shared.ts +++ b/src/security/dm-policy-shared.ts @@ -295,6 +295,7 @@ export async function resolveDmAllowState(params: { provider: ChannelId; accountId: string; allowFrom?: Array | null; + dmPolicy?: string | null; normalizeEntry?: (raw: string) => string; readStore?: (provider: ChannelId, accountId: string) => Promise; }): Promise<{ @@ -310,6 +311,7 @@ export async function resolveDmAllowState(params: { const storeAllowFrom = await readStoreAllowFromForDmPolicy({ provider: params.provider, accountId: params.accountId, + dmPolicy: params.dmPolicy, readStore: params.readStore, }); const normalizeEntry = params.normalizeEntry ?? ((value: string) => value);