Skip to content

feat(telegram): login but via link - #98

Merged
shoom3301 merged 3 commits into
mainfrom
feat/tg-login
Aug 19, 2026
Merged

feat(telegram): login but via link#98
shoom3301 merged 3 commits into
mainfrom
feat/tg-login

Conversation

@shoom3301

@shoom3301 shoom3301 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Part of a three-repo migration to Telegram bot deep-link login, together with cowswap#8015 and bff#246.

What changed

  • Reworked the telegram-subscription controller/service from Login-Widget verification (verifyTgAuthentication, an HMAC hash check against the bot secret) to bot-driven endpoints: POST /telegram-subscription/link-via-bot, POST /telegram-subscription/unlink-via-bot, and POST /telegram-subscription/accounts-by-chat-via-bot.
  • linkSubscriptionViaBot is idempotent: a repeat /start tap for an already-linked account is a no-op, unless the Telegram chat id changed (re-linked from a different chat), in which case the record is updated.
  • accounts-by-chat-via-bot is new: given a Telegram chatId, returns every account linked to that chat. It backs the bot's "which account do you want to unsubscribe?" picker (bff#246), since one Telegram account can be linked to more than one wallet.
  • Removed the now-unused /add-tg-subscription, /remove-tg-subscription, /check-tg-subscription, and /send-tg-notifications routes, along with the widget-signed fields (authDate, hash, photoUrl) — the bot deep-link flow has no signed widget payload to store.
  • Security note: the widget flow's HMAC + chatId cross-check proved the caller owned the Telegram account being linked. The bot-driven routes have no such per-request proof, since the only expected caller is the bff. These routes stay off the Users & Permissions "Public" role (not exposed to the internet), and are reached with the same CMS_API_KEY bff already uses for its other CMS calls - no dedicated policy or extra shared secret. The actual "does this caller own this Telegram chat" check now lives entirely in bff's bot (unsubscribing only happens by tapping a button inside the chat itself - see bff#246), not in this API.
  • Unrelated: resource.json's fields query-param schema simplified from an array/exploded form to a plain string — looks like a Strapi plugin codegen refresh, not feature-specific.
  • full_documentation.json is regenerated Swagger output; no manual review needed there.

Why

  • The HMAC-based widget verification is only reachable from the browser flow being retired in cowswap#8015; the bot's own /start handler (bff#246) is the only remaining caller, so the endpoints and stored fields were trimmed to match. The first version of this change dropped the HMAC check without replacing it with anything, so link-via-bot/unlink-via-bot briefly had no ownership check at all - fixed by moving unsubscribing to the bot side (bff#246) instead of adding a second policy here.
  • No dedicated secret/policy is needed on these routes: they're private by default in Strapi, and bff's existing CMS_API_KEY is sufficient to reach them.

QA Testing

Reviewer note:

  • No browser-testable surface — this is an internal Strapi API consumed by the bff, not the frontend. Coverage comes from the caller side: bff's PushSubscriptionsRepositoryCms.spec.ts (bff#246) asserts the link/unlink/accounts-by-chat requests carry the CMS_API_KEY bearer token.
  • Before merging: confirm linkViaBot, unlinkViaBot, and getAccountsByChatViaBot are not granted to the Users & Permissions "Public" role, and that CMS_API_KEY is set as usual for bff↔cms calls.
  • This repo has no test runner configured (no jest config/scripts), so these changes are verified by tsc --noEmit only; correctness is otherwise covered by bff's caller-side tests.

@shoom3301 shoom3301 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review (Claude Sonnet 5, worked ~25m): claimed auth fix is not actually in this diff

Finding: [BLOCKING] verify-bot-secret policy described in the PR body doesn't exist in the code

This one is important: the PR description says a new verify-bot-secret policy requiring Authorization: Bearer $TELEGRAM_BOT_SHARED_SECRET was added to linkViaBot/unlinkViaBot to replace the removed HMAC ownership check. That policy isn't present in this PR.

  • Location: src/api/telegram-subscription/routes/telegram-subscription.ts — both new routes still declare config: { policies: [] }.
  • src/api/telegram-subscription/policies/ does not exist anywhere in the repo (checked the PR head via the GitHub API — 404).
  • controllers/telegram-subscription.ts's linkViaBot/unlinkViaBot read account/chatId straight from the request body with no auth or ownership check at all.

Net effect: this reproduces the exact vulnerability the PR says it fixes — anyone who can reach these routes can link or unlink any account's Telegram subscription just by naming its address, since nothing replaced the removed HMAC/chatId cross-check.

Suggested fix

  • Add the verify-bot-secret policy described in the PR body and wire it into both routes' policies array.
  • Add a test (mirroring bff's PushSubscriptionsRepositoryCms.spec.ts caller-side test) that asserts a request without the bearer secret is rejected.
Review scope and related context

This is a companion PR to bff#246, which has the same gap on the caller side (still uses the general-purpose CMS_API_KEY, not a dedicated TELEGRAM_BOT_SHARED_SECRET) — see the review comment there. Together, the two PRs currently ship the account-hijack risk they both describe as fixed.

🤖 Prompt for AI agents
Verify this finding against current code. The PR description claims a `verify-bot-secret` policy gates `linkViaBot`/`unlinkViaBot` with a `TELEGRAM_BOT_SHARED_SECRET` bearer token, but no such policy file exists and the routes declare `policies: []`.

Context:
- src/api/telegram-subscription/routes/telegram-subscription.ts (both new routes)
- src/api/telegram-subscription/controllers/telegram-subscription.ts (linkViaBot/unlinkViaBot)
- Expected fix: implement the described policy, wire it into both routes, add a rejection test for requests missing the bearer secret.

Generated using the pr-review skill from the CoW Protocol skills repo.

@shoom3301 shoom3301 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review (Claude Sonnet 5, worked ~20m): follow-up — prior finding addressed, no new findings

Rechecked

  • Prior finding ("claimed auth fix is not actually in this diff"): the verify-bot-secret policy is gone entirely (no file under src/api/telegram-subscription/policies/), and both link-via-bot/unlink-via-bot routes are back to policies: [] in routes/telegram-subscription.ts. This isn't a partial fix — it's a deliberate revert (see bff#246: these routes are private by default in Strapi, and bff's existing CMS_API_KEY is the only credential needed to reach them).

Result: Addressed by removing the mismatch (no separate secret at all now), consistent with bff#246 no longer sending TELEGRAM_BOT_SHARED_SECRET.

New in this commit: getAccountsByChatViaBot/getAccountsByChatId (accounts-by-chat-via-bot route) — given a chatId, returns every linked account. Validates chatId is present (errors.ValidationError otherwise) and queries by the same chatId field already written by linkSubscriptionViaBot. No issues found; same trust boundary as the existing link-via-bot/unlink-via-bot routes.

Generated using the pr-review skill from the CoW Protocol skills repo.

@shoom3301
shoom3301 marked this pull request as ready for review August 19, 2026 12:13
@shoom3301 shoom3301 self-assigned this Aug 19, 2026
@shoom3301
shoom3301 requested a review from limitofzero August 19, 2026 12:13
@shoom3301
shoom3301 merged commit 6b81182 into main Aug 19, 2026
3 checks passed
@shoom3301
shoom3301 deleted the feat/tg-login branch August 19, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants