Skip to content

security: Telegram menu callback handlers don't enforce intent ownership #151

Open

Description

@coderabbitai

Note: this is a focused, expanded write-up of finding #1 in issue #91 ("security: missing authorization and input validation in Telegram menu and preferences API"). #91 remains the umbrella ticket covering three other independent findings (preferences API auth, callbackHandler non-null asserts). When picking up the work, decide whether to close this and resolve under #91 or close finding 1 in #91 in favour of this — but don't fix in two places.

Summary

Telegram menu callback handlers accept an untrusted intentId from the callback payload without first verifying that the requesting Telegram user owns the referenced intent/card. A malicious user can intercept their own callback, swap the intentId for another user's, and resend it to the bot.

Affected handlers (as of PR #148)

Callback action Handler Impact
menu_cancel_confirm:<intentId> showCancelConfirm Reads another user's intent details (subject, query, status, budget)
menu_cancel_do:<intentId> doCancelIntent → expireIntent Cancels another user's intent
menu_card_cancel:<intentId> doCancelCard → cancelCard Cancels another user's virtual card

Root cause

  • showCancelConfirm calls prisma.purchaseIntent.findUnique({ where: { id: intentId } }) with no userId filter.
  • expireIntent (in src/orchestrator/intentService.ts) accepts only intentId and never checks ownership.
  • getPaymentProvider().cancelCard(intentId) likewise has no ownership check.

The Telegram UI only surfaces these buttons for the user's own intents (showCancelList filters by userId), but the enforcement is client-side only.

Intent IDs are UUIDs — but not fully secret

UUIDs are hard to guess by brute force; however, they can be leaked via referral codes, audit exports, user-to-user sharing, or logs. The threat is realistic for a targeted attack.

Recommended fix

  1. Move the getUserByChatId lookup above the cancel dispatch blocks so user.id is available before any cancel action executes.
  2. Thread userId into showCancelConfirm, doCancelIntent, and doCancelCard.
  3. Add userId to downstream DB queries/mutations:
    • prisma.purchaseIntent.findUnique({ where: { id: intentId, userId } })
    • expireIntent(intentId, userId) with ownership assertion inside the service
    • cancelCard ownership check (depends on payment provider implementation)
  4. Defense in depth — also enforce at the service layer, not only at the Telegram handler. Add an assertOwnership(intentId, userId) (or equivalent guard) at the entry of expireIntent and the payment provider's cancelCard so any future caller path (HTTP API, internal scripts, new bots) inherits the check for free.
  5. Return a generic ⚠️ Intent not found. if the ownership check fails (avoid leaking whether the intent exists).
  6. Log every rejected cross-user attempt at warn with structured fields { chatId, telegramUserId, intentId, ownerUserId } so attempts are visible in observability and can feed alerting.

Design decision: enforcement can live at the handler layer, the service layer, or both — recommendation above is "both". Worth confirming explicitly in the fix PR.

Acceptance criteria

  • getUserByChatId resolved before any of the three cancel branches dispatch
  • userId threaded into showCancelConfirm / doCancelIntent / doCancelCard
  • DB lookups in showCancelConfirm constrained by { id: intentId, userId }
  • Service-layer ownership guard added to expireIntent and cancelCard (defense in depth)
  • Generic "not found" message on ownership mismatch — does not leak existence
  • Rejected cross-user attempts logged at warn with { chatId, telegramUserId, intentId, ownerUserId }
  • Integration tests for cross-user scenarios:
    • cross-user menu_cancel_confirm → "Intent not found", no DB read leak
    • cross-user menu_cancel_do → no state change on victim's intent
    • cross-user menu_card_cancel → no state change on victim's card

Out of scope (here)

  • Rate limiting on rejected attempts (separate ticket if/when needed)
  • A broader audit of every other menu_* action — this ticket is scoped to the three cancel paths above. A follow-up should review menu callback authorization holistically.

References

/cc @JonasBaeumer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions