fix(telegram): require webhook verification by default, dedupe repeated updates (#224)

[patrick-chinchill]

Summary

Telegram webhook mode used to fail open. With no secret token it logged one warning and then dispatched every update, callback_query button actions included. This PR ports upstream's fail-closed default and update_id redelivery dedupe.

What changed

• TelegramAdapterConfig.allow_unverified_webhooks: bool | None: new field. It resolves from TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS only for the exact string "true". An explicit config value, including False, wins over the env var. A non-bool value such as "false" raises ValidationError. The secret_token docstring now says the secret is required in webhook mode.
• Fail closed:
  • The constructor raises ValidationError for mode="webhook" when there is neither a secret nor the opt-out.
  • initialize() raises the same error when mode="auto" resolves to webhook.
  • handle_webhook returns 401 "Webhook verification required" as its first statement, before it reads the body.
  • The opt-out warning is now "Telegram webhook verification is explicitly disabled" and is still logged once.
• Dedupe: once the _chat check passes, each integral update_id is claimed with set_if_not_exists("telegram:webhook-update:{sha256(bot_user_id)}:{update_id}", True, 86_400_000).
  • A duplicate returns 200 with no dispatch.
  • A state failure or an unresolvable bot identity returns 503 with no dispatch.
  • "Integral" follows Number.isInteger: an integral JSON float (7.0, 7e0) is normalised to 7 and shares that key.
  • A missing or non-integral id (including bool and fractional floats) is dispatched without a claim, as upstream does.
• _ensure_bot_identity() runs one shared getMe as an asyncio.Task.
  • Waiters await it through asyncio.shield, so cancelling one webhook doesn't cancel the lookup for the others.
  • The stored task is cleared on success and on failure, so a failed startup getMe is retried on the next webhook.
  • initialize calls it and keeps its warn-on-failure behaviour.
• secret_token now resolves with ?? semantics (is not None). An explicit "" no longer silently falls back to the env secret.
• Docs:
  • docs/SECURITY.md: replaced the "silent skip" limitation with the new required-by-default behaviour.
  • docs/UPSTREAM_SYNC.md: new row that records the Python-surface adaptations and the one deliberate divergence.
  • CHANGELOG.md: added an "Unreleased (4.41 wave)" entry flagged BREAKING (security).

Upstream mapping

Upstream	Ported
c4a359e7 fix(telegram): require webhook verification by default (vercel/chat#858, chat@4.39.0)	config flag and env var, constructor/initialize errors, early 401, warning text, claim for every accepted update
1d2b78d9 Deduplicate repeated Telegram webhook updates (#799, chat@4.38.0)	update_id claim with 24h TTL; duplicate → 200, failure → 503
7a1150ce Add Vercel Connect support to Telegram (#813, chat@4.38.0)	only ensureBotIdentity and the sha256(bot_user_id) scope; the async token resolver is #189

Tests ported (tests/test_telegram_webhook.py)

• createTelegramAdapter: requires verification in webhook mode, allows explicit unverified webhook mode, allows polling mode without webhook verification.
• constructor env var resolution: resolves allowUnverifiedWebhooks from the env var, rejects TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS=false in webhook mode.
• TelegramAdapter:
  • deduplicates sequential and concurrent webhook updates
  • dispatches distinct and missing update IDs
  • deduplicates explicitly allowed unverified updates
  • scopes claims by bot identity
  • returns 503 without dispatch when the state fails
  • rejects unverified callback queries before dispatch
  • auto mode requires verification when a webhook is registered
• bot token resolver, scope assertions only, with static tokens: stable bot-identity scope (telegram:webhook-update:{sha256("999")}:1, TTL 86_400_000), the scope stays the same across token rotation and instances, and identity resolution is retried on a later webhook.

Python-specific tests:

• Only the exact string "true" opts out (checked with "1", "True", "TRUE", "yes", " true" and "").
• An explicit False wins over env "true".
• A non-bool allow_unverified_webhooks ("false", "true", 0, 1) raises ValidationError.
• An explicit secret_token="" doesn't pick up the env secret.
• update_id values True, False, "1", 1.5 and None are dispatched without a claim.
• update_id values 7, 7.0 and 7e0 share one claim key, so there is one dispatch.
• An authenticated non-object body ([1]) gets 200 and doesn't raise.
• An unresolvable identity returns 503 with no dispatch and no claim, and the failure isn't cached.
• Concurrent identity waiters share one getMe.
• Cancelling one identity waiter leaves the other resolved.
• initialize in polling mode needs no verification.

All state and telegram_fetch mocks are AsyncMock. The tests don't sleep; they use only asyncio.sleep(0) yields and events. The new verification and dedupe test classes clear TELEGRAM_* env vars, so developer shell exports can't flip the results. I checked that the bool guard, asyncio.shield, integral-float, non-bool opt-out and non-object-body tests fail when their guards are removed.

Tests that break by design, now updated:

• test_telegram_api.py::test_webhook_warns_no_verification is replaced by test_webhook_rejects_before_reading_body_without_verification. It expects 401, checks the body is never read, and checks there is no dispatch and no claim. It clears the env fallbacks.
• The Telegram fixture replay now uses mode="webhook", allow_unverified_webhooks=True, as upstream replay-telegram.test.ts does. It also mocks getMe (previously it hit the network) and the async claim.
• Other handle_webhook callers use allow_unverified_webhooks=True or a mocked claim and scope.

packages/adapter-telegram/src/index.test.ts is not fidelity-mapped (#78), so the strict fidelity count is unchanged.

Divergences

Python-surface adaptations, recorded in docs/UPSTREAM_SYNC.md:

• The error text uses snake_case option names.
• The shared getMe is a shielded asyncio.Task.
• update_id claiming mirrors Number.isInteger. bool is excluded (Python's bool is an int subclass), and integral floats are normalised to int.

One deliberate divergence: a non-bool allow_unverified_webhooks raises ValidationError. Upstream's TS boolean type rules such values out at compile time, and bool("false") would otherwise silently fail open.

The earlier or fallback for secret_token was itself a divergence; this PR removes it.

Out of scope, as the issue says:

• the polling checkpoint on the same scope ([4.41/TG3] Telegram polling & media: await handlers before advancing offset, media groups, multi-file uploads #227, which can reuse _ensure_bot_identity)
• the Vercel Connect token resolver ([4.41/DEF1] Deferred upstream 4.32–4.41 features (demand-gated backlog) #189)
• Postgres set_if_not_exists reclaiming expired rows ([4.41/ST1] Postgres state: reclaim expired set_if_not_exists rows, migration-managed schemas #240), which cross-instance dedupe on Postgres depends on

Consumer impact

BREAKING (security) for Telegram webhook users without a secret. mode="webhook" fails at construction. mode="auto" with a registered webhook fails in initialize(). Chat._ensure_initialized propagates that error and retries it on every webhook, so nothing is processed until the config is fixed; this is the intended fail-closed behaviour. The escape hatch is allow_unverified_webhooks=True or TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS=true. Polling users aren't affected. There's no impact on Slack or Teams.

Validation

Passing: ruff check, ruff format --check, audit (0 hard failures), strict fidelity at chat@4.31.0 (732/732), and every Telegram, fixture-replay and critical test (533 passed; they also pass with TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS=true exported). The full pytest run gives 5182 passed, 13 skipped and 3 failed. The 3 failures are Teams tests that don't involve Telegram (see Review).

Review

Addressed in 0c9032a. Every new test fails against the pre-fix adapter.

Fixed

• Integral-float update_id not claimed (reported by both reviewers; adapter.py dedupe guard). In JS, Number.isInteger(7.0) is true and ${7.0} is "7". The new _integral_update_id helper normalises 7.0 and 7e0 to int, so they share the ...:7 claim key. bool values and fractional floats are still not claimed. 1.0 moved out of the not-claimed parametrize list (replaced by 1.5). New test: test_integral_float_update_ids_share_the_integer_claim_key, which sends 7, 7.0 and 7e0: 1 dispatch, 3 claims on the same key. The UPSTREAM_SYNC row wording is corrected.
• allow_unverified_webhooks="false" fails open (bool() coercion). A non-bool value now raises ValidationError. This is recorded as a deliberate Python divergence in UPSTREAM_SYNC and the CHANGELOG. New test: test_non_bool_allow_unverified_webhooks_is_rejected[false|true|0|1].
• Authenticated non-object body ([1]) raised AttributeError from the update.get(...) call in the process_update failure log. The log now uses the already-guarded raw update_id. New test: test_authenticated_non_object_body_is_acknowledged_without_raising.
• Env-sensitive tests (reported by both reviewers). test_webhook_rejects_before_reading_body_without_verification now clears TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS and TELEGRAM_WEBHOOK_SECRET_TOKEN. Both new dedupe test classes have an autouse fixture that clears TELEGRAM_*. The 16 slash-routing tests that are sensitive to TELEGRAM_WEBHOOK_SECRET_TOKEN fail the same way on main and are left alone, to stay in scope.

Declined: none.

CI failures not caused by this PR: test (3.12) and test (3.13) fail on the same 3 Teams tests, before and after the review commit:

• TestOutboundServiceUrlRouting::test_edit_message_retargets_real_activities_client: fake_update() got an unexpected keyword argument 'service_url'
• TestCreateStreamer::test_creates_streamer_for_valid_dm_activity and test_returns_none_when_create_stream_raises: 'App' object has no attribute 'activity_sender'

The cause is microsoft-teams-apps 2.1.0 API drift. That version is already pinned in main's uv.lock, which this PR does not touch. As far as I can find, no open issue tracks this yet.

Update: resolved by #251 (Teams SDK capped <2.1), now merged into this branch; the full suite is green (see Merge gate).

Merge gate

Final HEAD: 04f9db1. The branch is merged with origin/main at 639885a, which includes #251, #235, #234 and #245. #245's log-hygiene work doesn't touch Telegram, and the Telegram logs this PR adds carry metadata only (updateId, error, botUserId, userName).

gpt-6-astra review: 5 rounds. Final verdict (round 5, on 04f9db1):

No actionable regressions found against the specified merge base. The full test suite passed (5,289 passed, 13 skipped), and lint checks passed for the changed Python files.

Fixed

• Round 1 [P2], dc6a5d1: the getMe username was lost on re-initialize. _ensure_bot_identity returns early once the scope is cached, so a second initialize() (for example Chat.shutdown() then Chat.initialize()) re-applied Chat.user_name and never restored the Telegram username. After that, @real_bot mentions and /ping@real_bot stopped routing. The resolved username is now cached and re-applied. Upstream ensureBotIdentity in chat@4.41.1 has the same bug, so the fix is recorded as a deliberate Python divergence in docs/UPSTREAM_SYNC.md. New test: test_reinitialize_keeps_the_get_me_username_over_the_chat_name. It fails without the fix.
• Round 1 [P2], dc6a5d1: allow_unverified_webhooks shifted positional config arguments. It was inserted as the first dataclass field, so TelegramAdapterConfig(None, "token") bound "token" to api_base_url. It is now the last field. New test: test_positional_config_arguments_keep_their_pre_opt_out_binding. It fails without the fix.
• Round 2 [P3], 24e74ff: asyncio.ensure_future for the shared getMe task broke the repo rule in AGENTS.md. It now uses asyncio.get_running_loop().create_task(...), in the adapter and in the identity-waiter tests.
• Round 3 on 24e74ff: clean. Round 4 on the merge of main, 7e4e41c: clean. Round 5 on the second merge of main, 04f9db1: clean. Neither merge changed Telegram code; the only conflicts were in the CHANGELOG, resolved to one wave heading with all bullets kept.

Rebutted: none.

Bot comments: CodeRabbit posted only a "Review limit reached" notice, which it updates on each push. It left no review content and no inline comments. There are no gemini-code-assist comments and no review threads to address.

CI on 04f9db1: all green. Lint & Type Check, test (3.12), test (3.13), Analyze (actions), Analyze (python) and CodeQL pass. The CodeRabbit status also passes, with the note "rate limited". Local full validation is green: ruff check, ruff format --check, the audit (0 hard failures), strict fidelity at chat@4.31.0, and pytest (5289 passed, 13 skipped).

Closes #224
Part of #184

[coderabbitai]

Warning

Review limit reached

Next included review available in 38 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 866563ab-2e7a-4005-bac7-32c6467da838

📥 Commits

Reviewing files that changed from the base of the PR and between b4e83d1 and 714911d.

📒 Files selected for processing (8)

• CHANGELOG.md
• docs/SECURITY.md
• docs/UPSTREAM_SYNC.md
• src/chat_sdk/adapters/telegram/adapter.py
• src/chat_sdk/adapters/telegram/types.py
• tests/test_fixture_replay.py
• tests/test_telegram_api.py
• tests/test_telegram_webhook.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[patrick-chinchill]

Merge gate: CI green on 714911d (Lint & Type Check, test (3.12), test (3.13), Analyze (actions), Analyze (python), CodeQL; CodeRabbit status pass/rate-limited); local Codex review (gpt-6-astra, xhigh, --base origin/main) on 714911d: clean — "No actionable regressions found relative to the specified merge base. Validation passed: 5,445 tests passed, 13 skipped; targeted lint and formatting checks passed, and type checking reported zero errors."; 6 astra rounds total (1 fresh round after merging main at b4e83d1 with #222/#205/#231 — clean merge, no conflicts; prior 5 rounds converged on 04f9db1); no bot review content (CodeRabbit rate-limited only, no review threads). Local full validation green (ruff, format, audit 0 hard failures, strict fidelity @ chat@4.31.0, pytest 5445 passed/13 skipped). Merging with --admin (Protect Main requires a code-owner approval).