feat(telegram): login but via link - #98
Conversation
shoom3301
left a comment
There was a problem hiding this comment.
⚠️ 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 declareconfig: { 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'slinkViaBot/unlinkViaBotreadaccount/chatIdstraight 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-secretpolicy described in the PR body and wire it into both routes'policiesarray. - Add a test (mirroring bff's
PushSubscriptionsRepositoryCms.spec.tscaller-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
left a comment
There was a problem hiding this comment.
✅ 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-secretpolicy is gone entirely (no file undersrc/api/telegram-subscription/policies/), and bothlink-via-bot/unlink-via-botroutes are back topolicies: []inroutes/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 existingCMS_API_KEYis 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.
Part of a three-repo migration to Telegram bot deep-link login, together with cowswap#8015 and bff#246.
What changed
telegram-subscriptioncontroller/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, andPOST /telegram-subscription/accounts-by-chat-via-bot.linkSubscriptionViaBotis idempotent: a repeat/starttap 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-botis new: given a TelegramchatId, 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./add-tg-subscription,/remove-tg-subscription,/check-tg-subscription, and/send-tg-notificationsroutes, along with the widget-signed fields (authDate,hash,photoUrl) — the bot deep-link flow has no signed widget payload to store.CMS_API_KEYbff 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.resource.json'sfieldsquery-param schema simplified from an array/exploded form to a plain string — looks like a Strapi plugin codegen refresh, not feature-specific.full_documentation.jsonis regenerated Swagger output; no manual review needed there.Why
/starthandler (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, solink-via-bot/unlink-via-botbriefly had no ownership check at all - fixed by moving unsubscribing to the bot side (bff#246) instead of adding a second policy here.CMS_API_KEYis sufficient to reach them.QA Testing
Reviewer note:
bff'sPushSubscriptionsRepositoryCms.spec.ts(bff#246) asserts the link/unlink/accounts-by-chat requests carry theCMS_API_KEYbearer token.linkViaBot,unlinkViaBot, andgetAccountsByChatViaBotare not granted to the Users & Permissions "Public" role, and thatCMS_API_KEYis set as usual for bff↔cms calls.tsc --noEmitonly; correctness is otherwise covered by bff's caller-side tests.