From 561152099038d939076249d5a0446e4c4746e28b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:57:37 -0700 Subject: [PATCH 01/15] fix(connectors): name a permanent Telegram delivery failure on the connector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Row D. A permanent send failure changed nothing: the integration stayed `connected` with `errorMessage: null`, no Activity row was written, and the relay kept running — so a user who blocked the bot saw replies vanish while the page said the connector was fine. Classify at the call site, by the chat the send targeted (wren 73777). Only the four sends to the connector's own bound chat may flip — telegramBridge :204, :285, :406 and decisionCardReconcileService :101 — plus the bind confirmation in the webhook route, whose permanent failure undoes the bind it just wrote. - `telegramService.sendMessage` now returns Telegram's own `errorCode` and `description`. They were read only to be logged, so every failure arrived as axios's "Request failed with status code 400" and a 403 was indistinguishable from a transient one. The service still decides nothing. - `noteBoundChatDeliveryFailure` refuses any chat that is not the connector's bound one, so an inbound update's failure cannot mislabel the owner's connector (vera 73761) even if a receive-side site is wired to it later. - Permanent is a 403, or a 400 whose description says the chat is gone. A 401 is the shared bot token and any other 400 is content — a pod named `A ` 400s every confirmation with "can't parse entities", and that must not undo a working bind. Handled at the source: `escapeHtml` moves into telegramService beside the `parse_mode: 'HTML'` it exists for and is imported back by the bridge, replacing a copy rather than adding one. - The flip is one update matched on {_id, config.chatId}: status `error`, named `errorMessage`, chatId unset — which stops the relay and re-allows a connect code — and an Activity row only if it matched. The bind clears `errorMessage` so reconnecting clears the named reason. Tests: classification truth table, each non-permanent shape, the inbound-chat refusal, a transient failure end to end, and the re-bind race. The pass-through test exists because without the two fields the whole feature is a silent no-op. --- .../connectorDeliveryFailureService.test.js | 154 ++++++++++++++++++ ...egramService.deliveryFailureFields.test.js | 58 +++++++ backend/routes/webhooks/telegram.ts | 22 ++- .../connectorDeliveryFailureService.ts | 125 ++++++++++++++ .../services/decisionCardReconcileService.ts | 8 +- backend/services/telegramBridgeService.ts | 25 ++- backend/services/telegramService.ts | 32 +++- 7 files changed, 409 insertions(+), 15 deletions(-) create mode 100644 backend/__tests__/unit/services/connectorDeliveryFailureService.test.js create mode 100644 backend/__tests__/unit/services/telegramService.deliveryFailureFields.test.js create mode 100644 backend/services/connectorDeliveryFailureService.ts diff --git a/backend/__tests__/unit/services/connectorDeliveryFailureService.test.js b/backend/__tests__/unit/services/connectorDeliveryFailureService.test.js new file mode 100644 index 0000000000..1dc570bbfa --- /dev/null +++ b/backend/__tests__/unit/services/connectorDeliveryFailureService.test.js @@ -0,0 +1,154 @@ +// Row D: a permanent delivery failure must name itself on the connector, and +// only the connector that owns the chat may be flipped. +// +// The cases that matter are the ones where flipping would be WRONG: an inbound +// chat's failure (vera 73761), a content 400 (wren 73778), a 401 from the shared +// bot token, and a connector re-bound between the send and the classification. +// Each is asserted against the STORED document as well as the return value, +// because a service that returns false and writes anyway would satisfy the +// return value alone. +const { MongoMemoryServer } = require('mongodb-memory-server'); +const mongoose = require('mongoose'); + +let mongod; +let Integration; +let Activity; +let deliveryFailures; + +const POD = new mongoose.Types.ObjectId(); + +const makeIntegration = (overrides = {}) => Integration.create({ + name: 'Telegram', + type: 'telegram', + podId: POD, + createdBy: new mongoose.Types.ObjectId(), + status: 'connected', + config: { chatId: '55501' }, + ...overrides, +}); + +const permanent403 = { + success: false, + errorCode: 403, + description: 'Forbidden: bot was blocked by the user', +}; + +const storedIntegration = async (id) => Integration.findById(id).lean(); + +beforeAll(async () => { + mongod = await MongoMemoryServer.create(); + await mongoose.connect(mongod.getUri()); + Integration = require('../../../models/Integration'); + Activity = require('../../../models/Activity'); + deliveryFailures = require('../../../services/connectorDeliveryFailureService'); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongod.stop(); +}); + +afterEach(async () => { + await Integration.deleteMany({}); + await Activity.deleteMany({}); +}); + +describe('classification', () => { + it('is permanent for a 403 and for a 400 whose description says the chat is gone', () => { + expect(deliveryFailures.classifyTelegramDeliveryFailure({ success: false, errorCode: 403 })) + .toBe(deliveryFailures.TELEGRAM_BLOCKED_REASON); + expect(deliveryFailures.classifyTelegramDeliveryFailure({ + success: false, + errorCode: 400, + description: 'Bad Request: chat not found', + })).toBe(deliveryFailures.TELEGRAM_CHAT_GONE_REASON); + expect(deliveryFailures.classifyTelegramDeliveryFailure({ + success: false, + errorCode: 400, + description: 'Chat Not Found', + })).toBe(deliveryFailures.TELEGRAM_CHAT_GONE_REASON); + }); + + it('is not permanent for content 400s, our own token, rate limits, or a success', () => { + [ + // The regression that made the escape load-bearing: a pod named `A ` + // produces exactly this 400, and it must not undo a working connector. + { success: false, errorCode: 400, description: "Bad Request: can't parse entities: unsupported start tag" }, + { success: false, errorCode: 400, description: 'Bad Request: message to reply not found' }, + { success: false, errorCode: 401, description: 'Unauthorized' }, + { success: false, errorCode: 429, description: 'Too Many Requests: retry after 3' }, + { success: false, errorCode: 500, description: 'Internal Server Error' }, + { success: true }, + null, + undefined, + ].forEach((result) => { + expect(deliveryFailures.classifyTelegramDeliveryFailure(result)).toBeNull(); + }); + }); +}); + +describe('which chat may flip', () => { + it('flips the connector when its own bound chat refuses delivery, and names the reason', async () => { + const integration = await makeIntegration(); + + const flipped = await deliveryFailures.noteBoundChatDeliveryFailure(integration, '55501', permanent403); + expect(flipped).toBe(true); + + const stored = await storedIntegration(integration._id); + expect(stored.status).toBe('error'); + expect(stored.errorMessage).toBe(deliveryFailures.TELEGRAM_BLOCKED_REASON); + // Unset, which is what stops the relay and re-allows a connect code. + expect(stored.config.chatId).toBeUndefined(); + + const rows = await Activity.find({ action: 'connector_delivery_failed' }).lean(); + expect(rows).toHaveLength(1); + expect(String(rows[0].podId)).toBe(String(POD)); + expect(rows[0].content).toContain(deliveryFailures.TELEGRAM_BLOCKED_REASON); + }); + + it('refuses a chat the connector does not own — the inbound case — and writes nothing', async () => { + const integration = await makeIntegration(); + + const flipped = await deliveryFailures.noteBoundChatDeliveryFailure(integration, '99999', permanent403); + expect(flipped).toBe(false); + + const stored = await storedIntegration(integration._id); + expect(stored.status).toBe('connected'); + expect(stored.config.chatId).toBe('55501'); + expect(await Activity.countDocuments({})).toBe(0); + }); + + it('leaves a transient failure alone, end to end', async () => { + const integration = await makeIntegration(); + + const flipped = await deliveryFailures.noteBoundChatDeliveryFailure(integration, '55501', { + success: false, + errorCode: 429, + description: 'Too Many Requests: retry after 3', + }); + expect(flipped).toBe(false); + + const stored = await storedIntegration(integration._id); + expect(stored.status).toBe('connected'); + expect(stored.config.chatId).toBe('55501'); + expect(stored.errorMessage == null).toBe(true); + expect(await Activity.countDocuments({})).toBe(0); + }); + + it('writes nothing when the connector was re-bound between the send and the classification', async () => { + const integration = await makeIntegration(); + await Integration.updateOne({ _id: integration._id }, { $set: { 'config.chatId': '77777' } }); + + const flipped = await deliveryFailures.flipConnectorOnPermanentDeliveryFailure({ + integrationId: integration._id, + failedChatId: '55501', + reason: deliveryFailures.TELEGRAM_BLOCKED_REASON, + }); + expect(flipped).toBe(false); + + const stored = await storedIntegration(integration._id); + expect(stored.status).toBe('connected'); + expect(stored.config.chatId).toBe('77777'); + expect(await Activity.countDocuments({})).toBe(0); + }); +}); diff --git a/backend/__tests__/unit/services/telegramService.deliveryFailureFields.test.js b/backend/__tests__/unit/services/telegramService.deliveryFailureFields.test.js new file mode 100644 index 0000000000..3478073630 --- /dev/null +++ b/backend/__tests__/unit/services/telegramService.deliveryFailureFields.test.js @@ -0,0 +1,58 @@ +// Row D rests on `sendMessage` handing Telegram's own error_code and description +// to its caller. Until this change they were read only to be logged, and every +// failure arrived as axios's "Request failed with status code 400" — so a +// classifier would have had nothing to classify and a permanent 403 would have +// looked exactly like a transient one. This is the test that keeps the feature +// from silently becoming a no-op. +const axios = require('axios'); + +jest.mock('axios', () => ({ post: jest.fn() })); + +const telegramService = require('../../../services/telegramService'); + +let consoleError; + +beforeEach(() => { + jest.clearAllMocks(); + consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + consoleError.mockRestore(); +}); + +describe('sendMessage passes the failure detail through', () => { + it('returns error_code and description so a call site can classify', async () => { + axios.post.mockRejectedValueOnce({ + response: { data: { ok: false, error_code: 400, description: 'Bad Request: chat not found' } }, + message: 'Request failed with status code 400', + }); + + const result = await telegramService.sendMessage('token', '55501', 'hi'); + + expect(result.success).toBe(false); + expect(result.errorCode).toBe(400); + expect(result.description).toBe('Bad Request: chat not found'); + // The old field stays: existing callers read it and nothing here decides. + expect(result.error).toBe('Request failed with status code 400'); + }); + + it('returns neither field when the failure carried no Telegram body', async () => { + axios.post.mockRejectedValueOnce({ message: 'socket hang up' }); + + const result = await telegramService.sendMessage('token', '55501', 'hi'); + + expect(result.success).toBe(false); + expect(result.errorCode).toBeUndefined(); + expect(result.description).toBeUndefined(); + }); +}); + +describe('escapeHtml', () => { + it('escapes the characters that would otherwise 400 an HTML send', () => { + // The bind confirmation names the pod inside parse_mode HTML, so a pod + // called `A ` used to fail the whole send with "can't parse entities". + expect(telegramService.escapeHtml('A & "c"')).toBe('A <b> & "c"'); + expect(telegramService.escapeHtml('plain')).toBe('plain'); + }); +}); diff --git a/backend/routes/webhooks/telegram.ts b/backend/routes/webhooks/telegram.ts index bb5ba4817e..de2ea2aa6c 100644 --- a/backend/routes/webhooks/telegram.ts +++ b/backend/routes/webhooks/telegram.ts @@ -8,6 +8,8 @@ const registry = require('../../integrations'); const IntegrationSummaryService = require('../../services/integrationSummaryService'); const AgentEventService = require('../../services/agentEventService'); const telegramService = require('../../services/telegramService'); +const { escapeHtml } = telegramService; +const deliveryFailures = require('../../services/connectorDeliveryFailureService'); const { isConnectCodeExpired, registerEnableAttempt } = require('../../services/telegramConnectCode'); const { claimDelivery: claimWebhookDelivery, @@ -159,8 +161,13 @@ const handleEnableCommand = async (chat: any, code: any) => { return; } - await Integration.findByIdAndUpdate(integration._id, { + // `errorMessage: null` because the working bind is the other state of the + // field the failure writes, and only a bind knows the connector works again + // (wren 73779). `new: true` is what the confirmation below classifies against: + // the chat id it compares is the one this update just stored. + const bound = await Integration.findByIdAndUpdate(integration._id, { status: 'connected', + errorMessage: null, $set: { 'config.chatId': chatId, 'config.chatTitle': chatTitle, @@ -171,18 +178,25 @@ const handleEnableCommand = async (chat: any, code: any) => { 'config.connectCode': '', 'config.connectCodeExpiresAt': '', }, - }); + }, { new: true }); const pod = await Pod.findById(integration.podId).lean(); const podName = pod?.name || 'your pod'; - await telegramService.sendMessage( + // Escaped because parse_mode is HTML: a pod named `A ` made Telegram reject + // this send with 400 "can't parse entities", which is a content failure and + // must never undo a bind (wren 73778). + const confirmation = await telegramService.sendMessage( botToken, chatId, - `✅ Connected this chat to ${podName} in Commonly.\n` + `✅ Connected this chat to ${escapeHtml(podName)} in Commonly.\n` + 'Agent messages from the pod will appear here. Too chatty? Send ' + '/mode attention to only get what needs you. /help lists the rest.', ); + // The one receive-side send that may flip: it targets the chat that was just + // bound, so a permanent failure means this bind is unusable. Undoing it clears + // the chat id, which is what lets the user mint a fresh code and reconnect. + await deliveryFailures.noteBoundChatDeliveryFailure(bound, chatId, confirmation); }; const handleSummaryCommand = async (chat: any, integration: any) => { diff --git a/backend/services/connectorDeliveryFailureService.ts b/backend/services/connectorDeliveryFailureService.ts new file mode 100644 index 0000000000..f0a83ee46c --- /dev/null +++ b/backend/services/connectorDeliveryFailureService.ts @@ -0,0 +1,125 @@ +import Integration from '../models/Integration'; +import Activity from '../models/Activity'; + +// Row D: a permanent delivery failure used to change nothing at all — the +// integration stayed `connected`, `errorMessage` stayed null, no Activity row +// was written and the relay kept running, so replies vanished while the page +// said the connector was fine. The fix is to classify the failure at the call +// site that sent it and flip the connector only for the chats it owns. +// +// Classification lives at the call site, not inside sendMessage (wren 73777): +// a Telegram chat id has two provenances — the connector's own bound chat, and +// the chat an inbound update came from. A 400/403 on the second says nothing +// about the first: a stranger who holds a connect code, binds, then blocks the +// bot would otherwise mark someone's working connector as needing attention +// (vera 73761). `noteBoundChatDeliveryFailure` can only ever act on the bound +// chat, so that wiring mistake is refused rather than merely discouraged. + +// The page shows a cause, not a number: "the bot was blocked" is actionable, +// "403" is not. +const TELEGRAM_BLOCKED_REASON = 'Telegram stopped delivering: the bot was blocked or removed from this chat.'; +const TELEGRAM_CHAT_GONE_REASON = 'Telegram stopped delivering: this chat no longer exists.'; + +interface DeliveryResult { + success?: boolean; + errorCode?: number; + description?: string; +} + +/** + * Telegram's own error_code/description → a named reason, or null when the + * failure says nothing about this connector. + * + * 403 blocked, kicked, or the bot was stopped → permanent + * 400 description "chat not found" → permanent (the chat is gone) + * 401 our shared bot token, not this connector → never + * 400 anything else — an unescaped entity, a bad offset, a bad offset in the + * reply target. Content, not reachability: a pod named `A ` used to 400 + * every confirmation, and that must not undo a working bind (wren 73778). + * 429/5xx, and anything we did not classify → never + */ +const classifyTelegramDeliveryFailure = (result?: DeliveryResult | null): string | null => { + if (!result || result.success === true) return null; + if (result.errorCode === 403) return TELEGRAM_BLOCKED_REASON; + if (result.errorCode === 400 && /chat not found/i.test(String(result.description || ''))) { + return TELEGRAM_CHAT_GONE_REASON; + } + return null; +}; + +interface FlipInput { + integrationId: unknown; + failedChatId: unknown; + reason: string; +} + +/** + * One matched update, then an Activity row only if it matched (wren 73779). + * + * The match on `config.chatId` is the race guard: if the connector was + * re-bound between the send and this call, the update matches nothing and no + * row is written — we would be reporting a failure for a chat it no longer + * owns. Unsetting the chat id is also what stops the relay (no relay predicate + * reads `status`) and what allows a reconnect (the connect-code route 409s + * while a chat id is present). + */ +const flipConnectorOnPermanentDeliveryFailure = async ({ + integrationId, + failedChatId, + reason, +}: FlipInput): Promise => { + const matched = await Integration.findOneAndUpdate( + { _id: integrationId, 'config.chatId': String(failedChatId) }, + { + $set: { status: 'error', errorMessage: reason }, + $unset: { 'config.chatId': '' }, + }, + { new: true }, + ); + if (!matched) return false; + + await Activity.create({ + type: 'pod_event', + actor: { id: null, name: 'Commonly', type: 'system', verified: true }, + action: 'connector_delivery_failed', + content: `${reason} Relaying is stopped until this connector is reconnected.`, + podId: matched.podId, + sourceType: 'event', + }); + return true; +}; + +/** + * Call this only from a send that targeted the connector's own bound chat. + * + * `sentToChatId` is compared against the stored one on purpose: the four sites + * that may flip all send to `integration.config.chatId`, and the receive-side + * replies in the Telegram webhook send to the chat the update came from. Passing + * an inbound chat here is the griefing lever, and it returns false instead of + * flipping someone else's working connector. + */ +const noteBoundChatDeliveryFailure = async ( + integration: { _id?: unknown; config?: { chatId?: unknown } } | null | undefined, + sentToChatId: unknown, + result?: DeliveryResult | null, +): Promise => { + const boundChatId = integration?.config?.chatId; + if (!boundChatId || !sentToChatId || String(boundChatId) !== String(sentToChatId)) return false; + + const reason = classifyTelegramDeliveryFailure(result); + if (!reason) return false; + + return flipConnectorOnPermanentDeliveryFailure({ + integrationId: integration?._id, + failedChatId: boundChatId, + reason, + }); +}; + +export { + classifyTelegramDeliveryFailure, + flipConnectorOnPermanentDeliveryFailure, + noteBoundChatDeliveryFailure, + TELEGRAM_BLOCKED_REASON, + TELEGRAM_CHAT_GONE_REASON, +}; diff --git a/backend/services/decisionCardReconcileService.ts b/backend/services/decisionCardReconcileService.ts index c9b3377dfd..87dd811b1c 100644 --- a/backend/services/decisionCardReconcileService.ts +++ b/backend/services/decisionCardReconcileService.ts @@ -10,6 +10,7 @@ const Pod = require('../models/Pod'); const isPodMember = require('../utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const telegramSend = require('./telegramService'); +const deliveryFailures = require('./connectorDeliveryFailureService'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const SlackApi = require('./slackApi'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require @@ -104,7 +105,12 @@ const sendClosingLine = async ( text, { replyToMessageId: card.tgMessageId, plainText: true }, ); - if (!sent?.success) throw new Error('Telegram ruling confirmation was not sent'); + if (!sent?.success) { + // Bound chat: the closing line goes to the connector's own chat, so a + // permanent failure is the connector's, not an inbound sender's. + await deliveryFailures.noteBoundChatDeliveryFailure(integration, integration.config?.chatId, sent); + throw new Error('Telegram ruling confirmation was not sent'); + } return; } const token = await connectorSecrets.get(String(integration.config?.botTokenRef)); diff --git a/backend/services/telegramBridgeService.ts b/backend/services/telegramBridgeService.ts index 40bd412f67..5af9b46f14 100644 --- a/backend/services/telegramBridgeService.ts +++ b/backend/services/telegramBridgeService.ts @@ -24,6 +24,7 @@ const IntegrationModel = require('../models/Integration'); const isPodMember = require('../utils/isPodMember'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const telegramSend = require('./telegramService'); +const deliveryFailures = require('./connectorDeliveryFailureService'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require const { shouldEscalate } = require('./connectorRelayPolicy'); // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require @@ -63,10 +64,10 @@ interface TelegramIntegrationDoc { }; } -const escapeHtml = (raw: string): string => String(raw) - .replace(/&/g, '&') - .replace(//g, '>'); +// This escape moved into telegramService, beside the `parse_mode: 'HTML'` it +// exists for, and is imported back: one Telegram escape for the whole backend +// rather than a copy per module (wren 73790). +const { escapeHtml } = telegramSend; export { shouldEscalate }; @@ -201,7 +202,9 @@ const replyNoActivePod = async (integration: TelegramIntegrationDoc): Promise` makes Telegram reject the whole send with 400 "can't parse +// entities", and the bind confirmation that carries the pod name must never be +// lost that way (wren 73778). Lives beside the parse_mode it exists for so the +// route and the bridge share one implementation instead of a fourth copy. +const escapeHtml = (raw: string): string => String(raw) + .replace(/&/g, '&') + .replace(//g, '>'); + async function sendMessage(botToken: string, chatId: string | number, text: string, options?: { replyToMessageId?: string; plainText?: boolean; @@ -34,13 +52,21 @@ async function sendMessage(botToken: string, chatId: string | number, text: stri messageId: response.data?.result?.message_id, }; } catch (error) { - const err = error as { response?: { data: unknown }; message: string }; + const err = error as { + response?: { data?: { error_code?: number; description?: string } }; + message: string; + }; console.error( 'Error sending Telegram message:', err.response?.data || err.message, ); - return { success: false, error: err.message }; + return { + success: false, + error: err.message, + errorCode: err.response?.data?.error_code, + description: err.response?.data?.description, + }; } } -export { sendMessage }; +export { sendMessage, escapeHtml }; From c256bdbfa9cf81df8cfb1f557d3c37301fdc8009 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:02:31 -0700 Subject: [PATCH 02/15] fix(connectors): escape every value the Telegram route interpolates into HTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vera 73812: the bind confirmation was not the only site. `telegramService` sets `parse_mode: 'HTML'`, and this route interpolated three more values straight into it: :315 the pod summary title and body (generated from pod messages) :372 the pod name again, in /status :374 `config.leadAgentUsername`, a value the agent is named by All four now go through `escapeHtml`, imported from `telegramService` (already required here) rather than added as a fifth copy of the helper. This is not only an injection fix. Telegram HTML accepts ``, so an unescaped name renders a clickable link inside a message the user reads as the bot's own words; and a stray `<` returns 400 "can't parse entities" — the content-400 class the new classifier is required NOT to flip the connector on. Escaping removes that class at its source instead of asking the classifier to keep sorting it out. Witnesses: the four sites are asserted through the real route, with values that would render as markup if unescaped. Two harness fixes were needed and are disclosed: - the test's `telegramService` mock was `{ sendMessage }` only, so the newly imported `escapeHtml` was undefined and the pre-existing `/commonly-enable` test went red with a 500. `sendMessage` stays stubbed (it is the network); `escapeHtml` is now the real one, because escaping is behaviour this route owes. - the bind's `findByIdAndUpdate` assertion pins the `{ new: true }` third argument: the confirmation's classification reads back the chat id that update stores, so without it a permanent failure of the confirmation could never undo the bind it just wrote. --- .../unit/routes/telegram.webhook.test.js | 89 ++++++++++++++++++- backend/routes/webhooks/telegram.ts | 8 +- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/backend/__tests__/unit/routes/telegram.webhook.test.js b/backend/__tests__/unit/routes/telegram.webhook.test.js index cd61a404b8..797e2f506e 100644 --- a/backend/__tests__/unit/routes/telegram.webhook.test.js +++ b/backend/__tests__/unit/routes/telegram.webhook.test.js @@ -6,7 +6,13 @@ jest.mock('../../../models/Pod'); jest.mock('../../../models/Summary', () => ({ findOne: jest.fn() })); jest.mock('../../../services/integrationSummaryService', () => ({ createSummary: jest.fn() })); jest.mock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); -jest.mock('../../../services/telegramService', () => ({ sendMessage: jest.fn() })); +// `sendMessage` is stubbed because it is the network; `escapeHtml` must stay +// real, because escaping is behaviour this route is responsible for and a mock +// that dropped it turned every escaped send into a 500 (vera 73812). +jest.mock('../../../services/telegramService', () => ({ + ...jest.requireActual('../../../services/telegramService'), + sendMessage: jest.fn(), +})); jest.mock('../../../integrations', () => ({ get: jest.fn() })); jest.mock('../../../services/telegramBridgeService', () => ({ relayTelegramMessageToPod: jest.fn(), @@ -82,7 +88,14 @@ describe('Telegram webhook routes', () => { 'config.chatId': '42', 'config.chatType': 'group', }), + // The bind is the event that clears a named failure, and only a bind + // knows the connector works again (wren 73779). + errorMessage: null, }), + // `new: true` is read back by the confirmation's classification: without + // it the doc in hand still carries the PRE-bind chat id, and a permanent + // failure of that confirmation could never undo the bind it just wrote. + { new: true }, ); expect(telegramService.sendMessage).toHaveBeenCalledWith( 'bot-token', @@ -91,6 +104,34 @@ describe('Telegram webhook routes', () => { ); }); + it('escapes a pod name in the bind confirmation, which Telegram parses as HTML', async () => { + const integration = { + _id: 'integration-1', + podId: 'pod-1', + config: { connectCode: 'abc123', connectCodeExpiresAt: new Date(Date.now() + 60000) }, + }; + Integration.findOne = jest.fn().mockResolvedValueOnce(integration).mockResolvedValueOnce(null); + Integration.findByIdAndUpdate = jest.fn().mockResolvedValue({ config: { chatId: '42' } }); + Pod.findById.mockReturnValue({ + lean: jest.fn().mockResolvedValue({ name: 'R&D ops' }), + }); + + const res = await request(app) + .post('/api/webhooks/telegram') + .send({ + message: { + text: '/commonly-enable abc123', + chat: { id: 42, title: 'Test Chat', type: 'group' }, + from: { id: 7, first_name: 'Sam' }, + }, + }); + + expect(res.status).toBe(200); + const text = telegramService.sendMessage.mock.calls[0][2]; + expect(text).toContain('Connected this chat to R&D <b>ops</b>'); + expect(text).not.toContain('ops'); + }); + it('posts integration summary on /summary', async () => { const integration = { _id: 'integration-1', @@ -310,6 +351,52 @@ describe('bridge command surface (/mode /status /mute /help)', () => { ); }); + it('/status escapes the pod name and the lead agent handle it reports', async () => { + Integration.findOne = jest.fn().mockResolvedValue({ + ...linked(), + config: { + ...linked().config, + leadAgentUsername: 'lead', + }, + }); + Pod.findById.mockReturnValue({ + lean: jest.fn().mockResolvedValue({ name: 'R&D chat' }), + }); + + const res = await post('/status'); + + expect(res.status).toBe(200); + const text = telegramService.sendMessage.mock.calls[0][2]; + // Telegram HTML accepts , so an unescaped name renders a clickable + // link inside a message the user reads as the bot's own words. + expect(text).toContain('R&D <b>chat</b>'); + expect(text).not.toContain('chat'); + expect(text).toContain('<a href="https://evil.example">lead</a>'); + expect(text).not.toContain(''); + }); + + it('/tldr escapes the summary title and body, which are generated from pod text', async () => { + Integration.findOne = jest.fn().mockResolvedValue(linked()); + Summary.findOne.mockReturnValue({ + sort: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue({ + title: 'Sprint ', + content: 'Agent said ship it & left', + }), + }), + }); + + const res = await post('/tldr'); + + expect(res.status).toBe(200); + const text = telegramService.sendMessage.mock.calls[0][2]; + expect(text).toContain('<script>alert(1)</script>'); + expect(text).not.toContain('