feat(notifications): add Telegram as a notification delivery channel - #1523
Open
srtab wants to merge 21 commits into
Open
feat(notifications): add Telegram as a notification delivery channel#1523srtab wants to merge 21 commits into
srtab wants to merge 21 commits into
Conversation
The best-effort sendMessage guard had no test: the autouse mock always answered 200, so deleting _reply left the suite green. Gate that mock on an own_replies marker so a test can register its own failure, and add the outage, happy-path and missing-token cases. Resolve the client inside the try as well, since reading the encrypted bot token can raise after the binding write has committed.
…e webhook on save
…dress-filter test
…cile dead end The Bot API takes the token in the URL path, which sentry_sdk's parse_url does not sanitize — so every httpx breadcrumb and span recorded a full-control credential, and a chained httpx frame carried it into tracebacks. TGClient.post now raises TelegramTransportError from None (dropping the frames whose locals hold the URL) and the Sentry config redacts the token from breadcrumbs, spans and event payloads. The new error stays a plain Exception so the delivery retry ladder still engages. The reconcile cron returned early whenever the URL matched, so a stored webhook secret that had diverged from Telegram's 401'd every update forever with no path back. A reported last_error_message now falls through to sync_telegram, which re-asserts the stored secret. An unverified binding rendered Disconnect only, leaving the documented reconnect unreachable; the connect control is now shared as a partial and rendered beside Disconnect, still behind the connect_ready gate. Also: guard the 2xx json() parse as transient, cap every fixed renderer row so a pathological repo_id cannot push a message past 4096, bound the decoded user pk to a signed 64-bit range before it reaches a query, percent-encode bot_username into the deep link, and move webhook_url() inside the tick's try. Adds a test that notifications/telegram/ imports nothing from notification internals, which is what keeps a later extraction a move rather than a rewrite.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The router carried no default auth, so the next JSON route added to it would have been public by omission. Declare auth=django_auth, matching nav_router; the Telegram callback already opted out with auth=None, which ninja honours because inheritance only applies when auth is NOT_SET.
…e handshake test Review fixes for the Telegram notification channel. Transport. `_tg_post` and `_extract_tg_error` called `.get` on whatever `response.json()` returned, so a proxy or captive portal answering 2xx (or 4xx) with valid-but-non-object JSON raised `AttributeError` instead of the transient `TelegramTransportError` the comment beside it promised — escaping the permanent/transient split into whichever broad handler was upstream. Both now guard on `isinstance(body, dict)`. Exception hierarchy. Retryable failures were a bare `RuntimeError`, which no caller could distinguish from a genuine bug, so six sites fell back to `except Exception`. Adds a `TelegramError` root and a named `TelegramTransientError`, and narrows the catches in `config.py`, `core/forms.py` and the reconcile cron. A bug in our own code no longer reports to the admin as a Telegram outage. Log levels. The reconcile cron logged every `getWebhookInfo` failure with `logger.exception`, so a Telegram outage minted a Sentry error event four times an hour for its whole duration — the pattern `_load_server_tools` exists to avoid, and inconsistent with `config.py`, which warns for the same exceptions. Anticipated failures now warn; only unexpected ones keep the traceback. `NoReverseMatch` from the documented string coupling to the callback route is split out too, since nothing else would ever report it. Unreachable chats. `is_blocked_error` matched only "bot was blocked by the user", so "user is deactivated" and "chat not found" — equally permanent — left the binding verified. The user's channel kept showing "Verified" with no reconnect control while notifications silently stopped. Renamed to `is_unreachable_chat_error` and widened; a message-level rejection such as "message is too long" still leaves the binding alone. Plain-text fallback. It exists so a new event type delivers before its renderer ships, but concatenated an unbounded `body` with no cap, so an over-length sendMessage was a 400 filed as permanent — losing the notification it existed to deliver. Now capped at `TG_MAX_CHARS`. Webhook route. The module docstring promised "2xx for everything except a bad secret" and nothing enforced it: a `DecryptionError` from the secret read after a key rotation, or a DB error inside dispatch, became a 500 — the response Telegram punishes by disabling the webhook. The dispatch is now guarded and the secret read fails closed, both logging at ERROR so the swallow stays visible. `/start` refusals. Three distinct causes returned one "link expired" message and logged nothing at all, so a SECRET_KEY rotation failed every handshake invisibly and a deactivated user was told to retry forever. Each is now logged distinctly, and a deactivated account gets its own message. Handshake coverage. The Connect view and the webhook each resolve binding state independently, and no test fed one to the other — every `/start` test minted via the harness's own helper. Replacing the view's mint with a state-blind `mint_token(pk, address="", verified_at="")` passed all 458 notification tests while permanently breaking reconnect-after-block. Adds the end-to-end test, in both the fresh and already-bound states. Also: `sync_telegram` warned "the bot token was removed" on a fresh install saving the group with nothing entered, which is the most likely first interaction — now gated on a stored webhook secret, the flag that says a webhook was once registered. `config.save()` is scoped to the two columns it changes so it cannot revert a concurrent edit made during the `getMe` round trip. `DisconnectTelegramView` goes through a new `unbind_user` instead of writing `UserChannelBinding` directly, so `telegram_bindings` is again the only door. Three comments corrected: `core` has two `notifications` edges rather than none, a 32-byte MAC would also fit the 64-character budget, and the email channel does not read `TONE_EMOJI`. Documents `DAIV_TELEGRAM_BOT_USERNAME` and `DAIV_TELEGRAM_WEBHOOK_SECRET`, which were env-lockable but unlisted.
…231725 # Conflicts: # daiv/notifications/tasks.py
…bing
Deduplication pass over the Telegram channel work. Almost all of it is
behaviour-preserving; the exceptions are called out below.
Renderer registries. Rocket Chat and Telegram each carried a byte-identical
`_registry` dict with its own `register_renderer`/`get_renderer` pair, differing
only in the channel name inside the duplicate-registration error. Both now hold
a `RendererRegistry[R]`, which takes that name as its one argument. One instance
per channel rather than one shared map: a renderer registered for Rocket Chat
must never satisfy a Telegram lookup.
Renderer helpers. The token and cost formatting was forked per channel, so the
labels and the em-dash placeholder could drift independently. `_usage_value` and
`_cost_value` move to `BaseRenderer`, and the three labels become shared
constants — being constants, they no longer go through the Telegram row
builder's per-label truncation. `_compose_text` was duplicated verbatim in both
channels behind a comment claiming the copy was load-bearing for a test patch
target; it is not, and it is now `renderers.base.compose_plain_text`.
Connect gating. `UserChannelsView._connect_ready` switched on `channel_type` to
special-case Telegram, and the template hardcoded Telegram's blocked-reason
string in an `{% else %}` that any other channel would also reach. Both become
`NotificationChannel` hooks, so a channel answers for itself and the template
renders whatever it returns.
Disconnect view. `DeleteRocketChatBindingView` and `DisconnectTelegramView`
differed only in the `channel_type` they filtered on. One
`ChannelDisconnectView` takes it through `as_view` per URL.
Bot API client. `_tg_post` took its `TGClient` as a parameter and is now the
`call` method on it. It also posted through the module-level `httpx.post`, which
builds a client per call; a process-wide pooled client keeps a TLS handshake and
a CA-bundle parse off every Bot API request. Nothing per-token belongs on that
client, since the token rides in the URL path.
Sentry hooks. Three identical one-line wrappers collapse to one `_scrub_hook`.
`scrub_telegram_token` now substring-tests for the API host before running its
regex: `scrub_payload` reaches it for every string in every breadcrumb, and
almost none are Bot API URLs.
Dead code. Drops `binding_state`, a one-line pass-through to
`binding_state_for_pk`, and `unbind_user`, whose only caller was the deleted
Telegram disconnect view.
CodeQL. `_secret_is_valid` logged `_SECRET_HEADER` when the header was absent.
The value is the fixed header name rather than a secret, but CodeQL reads any
log of a SECRET-named constant as a cleartext credential leak, and failed this
PR's scanning check at high severity for it. The message now names the header
inline. The constant already carried a `noqa: S105` for ruff's version of the
same false positive.
…231725 # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds Telegram as a third notification delivery channel alongside email and Rocket Chat, with per-event rich rendering, a deep-link handshake, and a webhook that leaves room for an interactive bot later.
Implements
docs/superpowers/plans/2026-08-21-telegram-notification-channel.mdagainstdocs/superpowers/specs/2026-08-21-telegram-notification-channel-design.md.Architecture
A self-contained bot transport (
daiv/notifications/telegram/) that imports no notification internals, plus aNotificationChanneland a django-ninja webhook route on the notifications side that read and writeUserChannelBinding. The dependency arrow points one way, so extracting adaiv/telegram/app later is a move rather than a rewrite —tests/unit_tests/notifications/test_telegram_boundary.pywalks the package's imports (includingTYPE_CHECKINGblocks) to keep that true.Registration happens both on config save and from a reconcile cron, so env-only deployments converge without anyone visiting the dashboard.
What an operator sees
Administrators enable Telegram under Dashboard → Configuration → Telegram with a bot token from @Botfather; the bot username is derived via
getMerather than typed, and the webhook secret is generated once. Users link their chat from/accounts/channels/with a Connect button that hands off to the bot via a 10-minute HMAC deep-link token. Blocking the bot, sending/stop, or selecting Disconnect all unlink the chat.Connecting requires DAIV to be reachable from the internet over HTTPS (delivery does not), and one bot token serves exactly one instance — Telegram allows a single webhook per bot, so the reconcile cron warns loudly and names any foreign URL it finds.
Notable design points
validate_gitlab_webhook, which returnsTruewhen no secret is configured. The route answers401for a bad secret and2xxfor everything else, because Telegram retries non-2xx and eventually disables the webhook.422. Update schemas are lenient by construction.startbudget; the HMAC folds the caller's current binding state, so any successful/startinvalidates every earlier token.parse_mode=HTML, never MarkdownV2. Every interpolated value is escaped, and truncation always lands in escaped text so a cut can never halve a tag or an entity — Telegram answers unbalanced HTML with a 400 that the channel files as permanent.403 bot was blocked by the userflip unverifies the binding so later notifications record as skipped rather than burning three retries each. The string match is acknowledged fragile; if the wording drifts the flip stops firing and the delivery is a permanent FAILED, never a retry storm.BaseRendererwith no behaviour change — that task's existing 37-test suite changed by exactly one line (a patch target), which is the evidence it stayed behaviour-identical.Security
The Telegram Bot API requires the token in the URL path, and
sentry_sdk's URL sanitiser strips query strings and userinfo but not paths. Left alone, every Bot API call would have shipped a full-control credential to Sentry via breadcrumbs, spans and traceback frame locals — a token that can re-point the webhook and intercept handshakes. Closed on both carriers: transport failures raisefrom Noneso httpx frames never enter the captured chain, anddaiv/daiv/settings/components/sentry.pyscrubsapi.telegram.org/bot…URLs from breadcrumbs, events and transactions. Verified end-to-end against the real SDK with a control that demonstrates the pre-fix path does leak.Also hardened along the way: the webhook secret comparison encodes to UTF-8 bytes before
hmac.compare_digest, which otherwise raisesTypeErroron a non-ASCII header and returned 500 instead of the documented 401 to an unauthenticated caller.Migrations
Two, both additive with no data migration and no backfill:
notifications/0008_telegram_channel_type— twoAlterFieldchoice updates (a no-op at the DB level on PostgreSQL)core/0016_siteconfiguration_telegram— four nullable columnsEnabling Telegram does not backfill:
redrive_missing_notifications_cron_taskkeys onNotificationrows rather than delivery rows, so the first Telegram message is the next notify-worthy run.Testing
make test→ 4818 passed (baseline 4781 onmain), 90.6% coverage. Tests exercise real behaviour rather than mocks throughout —httpx_mockat the transport boundary, the real ORM, and the Django test client against the real route.makemigrations --checkclean;make lint-typingat its existing diagnostic baseline with no new error class.Known follow-ups
Filed separately rather than widened into this branch:
IntegrityErrorinside the unauthenticated webhook handler, so it needs retry handling and a concurrency test.auth=django_auth(it carries only the deliberately public webhook today).sentry_sdk.init, so the fix above cannot be silently deleted.validate_gitlab_webhookhas the same non-ASCIIhmac.compare_digestexposure fixed here for Telegram.Out of scope
Webhook-secret rotation, a
getUpdatespolling fallback, honouring429'sretry_after, a throttle on the webhook route, backfilling deliveries onto pre-existing notifications, revoking a live link token, and any interactive bot command beyond/startand/stop. Each was considered and deliberately deferred; the module layout leaves room for the last one.