diff --git a/CHANGELOG.md b/CHANGELOG.md index d92081b6006..4711346d5e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Telegram/Security: require numeric Telegram sender IDs for allowlist authorization (reject `@username` principals) and warn in `openclaw security audit` when legacy configs contain usernames. Thanks @vincentkoc. - Security/Skills: harden archive extraction for download-installed skills to prevent path traversal outside the target directory. Thanks @markmusson. - Security/Media: stream and bound URL-backed input media fetches to prevent memory exhaustion from oversized responses. Thanks @vincentkoc. - Security/Signal: harden signal-cli archive extraction during install to prevent path traversal outside the install root. diff --git a/docs/channels/groups.md b/docs/channels/groups.md index d2497148b2c..1b3fb0394a3 100644 --- a/docs/channels/groups.md +++ b/docs/channels/groups.md @@ -138,7 +138,7 @@ Control how group/room messages are handled per channel: }, telegram: { groupPolicy: "disabled", - groupAllowFrom: ["123456789", "@username"], + groupAllowFrom: ["123456789"], // numeric Telegram user id (wizard can resolve @username) }, signal: { groupPolicy: "disabled", diff --git a/docs/channels/telegram.md b/docs/channels/telegram.md index 25e46a9dca5..46d44bd9b4b 100644 --- a/docs/channels/telegram.md +++ b/docs/channels/telegram.md @@ -112,7 +112,8 @@ Token resolution order is account-aware. In practice, config values win over env - `open` (requires `allowFrom` to include `"*"`) - `disabled` - `channels.telegram.allowFrom` accepts numeric IDs and usernames. `telegram:` / `tg:` prefixes are accepted and normalized. + `channels.telegram.allowFrom` accepts numeric Telegram user IDs. `telegram:` / `tg:` prefixes are accepted and normalized. + The onboarding wizard accepts `@username` input and resolves it to numeric IDs. ### Finding your Telegram user ID @@ -681,9 +682,9 @@ Primary reference: - `channels.telegram.botToken`: bot token (BotFather). - `channels.telegram.tokenFile`: read token from file path. - `channels.telegram.dmPolicy`: `pairing | allowlist | open | disabled` (default: pairing). -- `channels.telegram.allowFrom`: DM allowlist (ids/usernames). `open` requires `"*"`. +- `channels.telegram.allowFrom`: DM allowlist (numeric Telegram user IDs). `open` requires `"*"`. - `channels.telegram.groupPolicy`: `open | allowlist | disabled` (default: allowlist). -- `channels.telegram.groupAllowFrom`: group sender allowlist (ids/usernames). +- `channels.telegram.groupAllowFrom`: group sender allowlist (numeric Telegram user IDs). - `channels.telegram.groups`: per-group defaults + allowlist (use `"*"` for global defaults). - `channels.telegram.groups..groupPolicy`: per-group override for groupPolicy (`open | allowlist | disabled`). - `channels.telegram.groups..requireMention`: mention gating default. diff --git a/docs/help/faq.md b/docs/help/faq.md index 60b27eb04d2..9dbfbca7ceb 100644 --- a/docs/help/faq.md +++ b/docs/help/faq.md @@ -794,7 +794,9 @@ without WhatsApp/Telegram. ### Telegram what goes in allowFrom -`channels.telegram.allowFrom` is **the human sender's Telegram user ID** (numeric, recommended) or `@username`. It is not the bot username. +`channels.telegram.allowFrom` is **the human sender's Telegram user ID** (numeric). It is not the bot username. + +The onboarding wizard accepts `@username` input and resolves it to a numeric ID, but OpenClaw authorization uses numeric IDs only. Safer (no third-party bot): diff --git a/src/channels/plugins/onboarding/telegram.ts b/src/channels/plugins/onboarding/telegram.ts index d84a1aded51..f5916ef4515 100644 --- a/src/channels/plugins/onboarding/telegram.ts +++ b/src/channels/plugins/onboarding/telegram.ts @@ -115,7 +115,7 @@ async function promptTelegramAllowFrom(params: { let resolvedIds: string[] = []; while (resolvedIds.length === 0) { const entry = await prompter.text({ - message: "Telegram allowFrom (username or user id)", + message: "Telegram allowFrom (numeric sender id; @username resolves to id)", placeholder: "@username", initialValue: existingAllowFrom[0] ? String(existingAllowFrom[0]) : undefined, validate: (value) => (String(value ?? "").trim() ? undefined : "Required"), diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index 9207cab0d60..d62346787e7 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -14,6 +14,18 @@ function normalizeAllowFromList(list: Array | undefined | null) return list.map((v) => String(v).trim()).filter(Boolean); } +function normalizeTelegramAllowFromEntry(raw: unknown): string { + const base = typeof raw === "string" ? raw : typeof raw === "number" ? String(raw) : ""; + return base + .trim() + .replace(/^(telegram|tg):/i, "") + .trim(); +} + +function isNumericTelegramUserId(raw: string): boolean { + return /^\d+$/.test(raw); +} + function classifyChannelWarningSeverity(message: string): SecurityAuditSeverity { const s = message.toLowerCase(); if ( @@ -353,10 +365,39 @@ export async function collectChannelSecurityFindings(params: { const storeAllowFrom = await readChannelAllowFromStore("telegram").catch(() => []); const storeHasWildcard = storeAllowFrom.some((v) => String(v).trim() === "*"); + const invalidTelegramAllowFromEntries = new Set(); + for (const entry of storeAllowFrom) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (!isNumericTelegramUserId(normalized)) { + invalidTelegramAllowFromEntries.add(normalized); + } + } const groupAllowFrom = Array.isArray(telegramCfg.groupAllowFrom) ? telegramCfg.groupAllowFrom : []; const groupAllowFromHasWildcard = groupAllowFrom.some((v) => String(v).trim() === "*"); + for (const entry of groupAllowFrom) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (!isNumericTelegramUserId(normalized)) { + invalidTelegramAllowFromEntries.add(normalized); + } + } + const dmAllowFrom = Array.isArray(telegramCfg.allowFrom) ? telegramCfg.allowFrom : []; + for (const entry of dmAllowFrom) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (!isNumericTelegramUserId(normalized)) { + invalidTelegramAllowFromEntries.add(normalized); + } + } const anyGroupOverride = Boolean( groups && Object.values(groups).some((value) => { @@ -366,6 +407,15 @@ export async function collectChannelSecurityFindings(params: { const group = value as Record; const allowFrom = Array.isArray(group.allowFrom) ? group.allowFrom : []; if (allowFrom.length > 0) { + for (const entry of allowFrom) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (!isNumericTelegramUserId(normalized)) { + invalidTelegramAllowFromEntries.add(normalized); + } + } return true; } const topics = group.topics; @@ -378,6 +428,15 @@ export async function collectChannelSecurityFindings(params: { } const topic = topicValue as Record; const topicAllow = Array.isArray(topic.allowFrom) ? topic.allowFrom : []; + for (const entry of topicAllow) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (!isNumericTelegramUserId(normalized)) { + invalidTelegramAllowFromEntries.add(normalized); + } + } return topicAllow.length > 0; }); }), @@ -386,6 +445,24 @@ export async function collectChannelSecurityFindings(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 onboarding to resolve), then re-run the audit.", + }); + } + if (storeHasWildcard || groupAllowFromHasWildcard) { findings.push({ checkId: "channels.telegram.groups.allowFrom.wildcard", @@ -394,7 +471,7 @@ export async function collectChannelSecurityFindings(params: { detail: 'Telegram group sender allowlist contains "*", which allows any group member to run /… commands and control directives.', remediation: - 'Remove "*" from channels.telegram.groupAllowFrom and pairing store; prefer explicit user ids/usernames.', + 'Remove "*" from channels.telegram.groupAllowFrom and pairing store; prefer explicit numeric Telegram user IDs.', }); continue; } diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index a040ff4a19a..caad90b1ef7 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -1099,6 +1099,50 @@ describe("security audit", () => { } }); + it("warns when Telegram allowFrom entries are non-numeric (legacy @username configs)", async () => { + const prevStateDir = process.env.OPENCLAW_STATE_DIR; + const tmp = await fs.mkdtemp( + path.join(os.tmpdir(), "openclaw-security-audit-telegram-invalid-allowfrom-"), + ); + process.env.OPENCLAW_STATE_DIR = tmp; + await fs.mkdir(path.join(tmp, "credentials"), { recursive: true, mode: 0o700 }); + try { + const cfg: OpenClawConfig = { + channels: { + telegram: { + enabled: true, + botToken: "t", + groupPolicy: "allowlist", + groupAllowFrom: ["@TrustedOperator"], + groups: { "-100123": {} }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [telegramPlugin], + }); + + expect(res.findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: "channels.telegram.allowFrom.invalid_entries", + severity: "warn", + }), + ]), + ); + } finally { + if (prevStateDir == null) { + delete process.env.OPENCLAW_STATE_DIR; + } else { + process.env.OPENCLAW_STATE_DIR = prevStateDir; + } + } + }); + it("adds a warning when deep probe fails", async () => { const cfg: OpenClawConfig = { gateway: { mode: "local" } }; diff --git a/src/telegram/bot-access.ts b/src/telegram/bot-access.ts index f3ac93b4cda..05a2034c7d6 100644 --- a/src/telegram/bot-access.ts +++ b/src/telegram/bot-access.ts @@ -2,12 +2,34 @@ import type { AllowlistMatch } from "../channels/allowlist-match.js"; export type NormalizedAllowFrom = { entries: string[]; - entriesLower: string[]; hasWildcard: boolean; hasEntries: boolean; + invalidEntries: string[]; }; -export type AllowFromMatch = AllowlistMatch<"wildcard" | "id" | "username">; +export type AllowFromMatch = AllowlistMatch<"wildcard" | "id">; + +const warnedInvalidEntries = new Set(); + +function warnInvalidAllowFromEntries(entries: string[]) { + if (process.env.VITEST || process.env.NODE_ENV === "test") { + return; + } + for (const entry of entries) { + if (warnedInvalidEntries.has(entry)) { + continue; + } + warnedInvalidEntries.add(entry); + console.warn( + [ + "[telegram] Invalid allowFrom entry:", + JSON.stringify(entry), + "- allowFrom/groupAllowFrom authorization requires numeric Telegram sender IDs only.", + 'If you had "@username" entries, re-run onboarding (it resolves @username to IDs) or replace them manually.', + ].join(" "), + ); + } +} export const normalizeAllowFrom = (list?: Array): NormalizedAllowFrom => { const entries = (list ?? []).map((value) => String(value).trim()).filter(Boolean); @@ -15,12 +37,16 @@ export const normalizeAllowFrom = (list?: Array): NormalizedAll const normalized = entries .filter((value) => value !== "*") .map((value) => value.replace(/^(telegram|tg):/i, "")); - const normalizedLower = normalized.map((value) => value.toLowerCase()); + const invalidEntries = normalized.filter((value) => !/^\d+$/.test(value)); + if (invalidEntries.length > 0) { + warnInvalidAllowFromEntries([...new Set(invalidEntries)]); + } + const ids = normalized.filter((value) => /^\d+$/.test(value)); return { - entries: normalized, - entriesLower: normalizedLower, + entries: ids, hasWildcard, hasEntries: entries.length > 0, + invalidEntries, }; }; @@ -48,7 +74,7 @@ export const isSenderAllowed = (params: { senderId?: string; senderUsername?: string; }) => { - const { allow, senderId, senderUsername } = params; + const { allow, senderId } = params; if (!allow.hasEntries) { return true; } @@ -58,11 +84,7 @@ export const isSenderAllowed = (params: { if (senderId && allow.entries.includes(senderId)) { return true; } - const username = senderUsername?.toLowerCase(); - if (!username) { - return false; - } - return allow.entriesLower.some((entry) => entry === username || entry === `@${username}`); + return false; }; export const resolveSenderAllowMatch = (params: { @@ -70,7 +92,7 @@ export const resolveSenderAllowMatch = (params: { senderId?: string; senderUsername?: string; }): AllowFromMatch => { - const { allow, senderId, senderUsername } = params; + const { allow, senderId } = params; if (allow.hasWildcard) { return { allowed: true, matchKey: "*", matchSource: "wildcard" }; } @@ -80,15 +102,5 @@ export const resolveSenderAllowMatch = (params: { if (senderId && allow.entries.includes(senderId)) { return { allowed: true, matchKey: senderId, matchSource: "id" }; } - const username = senderUsername?.toLowerCase(); - if (!username) { - return { allowed: false }; - } - const entry = allow.entriesLower.find( - (candidate) => candidate === username || candidate === `@${username}`, - ); - if (entry) { - return { allowed: true, matchKey: entry, matchSource: "username" }; - } return { allowed: false }; }; diff --git a/src/telegram/bot.create-telegram-bot.blocks-all-group-messages-grouppolicy-is.test.ts b/src/telegram/bot.create-telegram-bot.blocks-all-group-messages-grouppolicy-is.test.ts index 0436c03ce1f..cf10939453c 100644 --- a/src/telegram/bot.create-telegram-bot.blocks-all-group-messages-grouppolicy-is.test.ts +++ b/src/telegram/bot.create-telegram-bot.blocks-all-group-messages-grouppolicy-is.test.ts @@ -248,7 +248,7 @@ describe("createTelegramBot", () => { expect(replySpy).toHaveBeenCalledTimes(1); }); - it("allows group messages from senders in allowFrom (by username) when groupPolicy is 'allowlist'", async () => { + it("blocks group messages when allowFrom is configured with @username entries (numeric IDs required)", async () => { onSpy.mockReset(); const replySpy = replyModule.__replySpy as unknown as ReturnType; replySpy.mockReset(); @@ -276,7 +276,7 @@ describe("createTelegramBot", () => { getFile: async () => ({ download: async () => new Uint8Array() }), }); - expect(replySpy).toHaveBeenCalledTimes(1); + expect(replySpy).toHaveBeenCalledTimes(0); }); it("allows group messages from telegram:-prefixed allowFrom entries when groupPolicy is 'allowlist'", async () => { onSpy.mockReset(); diff --git a/src/telegram/bot.create-telegram-bot.matches-usernames-case-insensitively-grouppolicy-is.test.ts b/src/telegram/bot.create-telegram-bot.matches-usernames-case-insensitively-grouppolicy-is.test.ts index a7d6a444f9d..9dc4662a7af 100644 --- a/src/telegram/bot.create-telegram-bot.matches-usernames-case-insensitively-grouppolicy-is.test.ts +++ b/src/telegram/bot.create-telegram-bot.matches-usernames-case-insensitively-grouppolicy-is.test.ts @@ -159,7 +159,7 @@ describe("createTelegramBot", () => { // groupPolicy tests - it("matches usernames case-insensitively when groupPolicy is 'allowlist'", async () => { + it("blocks @username allowFrom entries when groupPolicy is 'allowlist' (numeric IDs required)", async () => { onSpy.mockReset(); const replySpy = replyModule.__replySpy as unknown as ReturnType; replySpy.mockReset(); @@ -187,7 +187,7 @@ describe("createTelegramBot", () => { getFile: async () => ({ download: async () => new Uint8Array() }), }); - expect(replySpy).toHaveBeenCalledTimes(1); + expect(replySpy).toHaveBeenCalledTimes(0); }); it("allows direct messages regardless of groupPolicy", async () => { onSpy.mockReset();