Skip to content

fix: dedupe tg subscriptions with a chat id - #102

Merged
azebuado merged 1 commit into
mainfrom
fix/telegram-subscription
Sep 30, 2026
Merged

azebuado merged 1 commit into
mainfrom
fix/telegram-subscription

Conversation

@azebuado

Copy link
Copy Markdown
Contributor

Summary

  • Require chat id for tg subscriptions

Must be deployed after cowprotocol/bff#261

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 Opus 4.8, worked ~3m): 1 non-blocking note; no blockers

Focused security fix; the eviction and unlink-scoping changes are correct. One note on the "dedupe" guarantee.

Finding: [NON-BLOCKING] Dedup is best-effort — concurrent redemptions can still duplicate

  • Location: src/api/telegram-subscription/services/telegram-subscription.ts — linkSubscriptionViaBot
  • The dedup is a find-then-create with no DB-level uniqueness on (account, chatId). Two near-simultaneous /start redemptions from the same chat can both pass the alreadyLinked check and insert duplicate rows. Not a security issue (same chat, no eviction), but the PR title says "dedupe", so worth noting the guarantee is app-level only.
  • Deliberate simple-version choice — flagging for the record, not requesting a change.

Suggested fix (optional, if hardening later)

  • Add a composite unique index on (account, chat_id) via a Knex migration, and catch the unique-violation in create to keep it idempotent. Ship those two together.
  • No regression test accompanies this security fix; the repo has no test harness, so non-blocking, but a small guard against reintroducing the eviction would be worthwhile if a runner is ever added.
Review scope and related context

No prior review comments to dedupe against. Verified independently:

  • Eviction fixed: a different chatId now adds a row instead of overwriting the incumbent; same chat re-link is an idempotent no-op.
  • Unlink scoped to the calling chat's rows only; removeSubscriptions (the delete-all primitive) removed with no remaining references in src/ or lib/.
  • Deploy order correct: controller now hard-requires chatId, so BFF (#261) must deploy first — already stated in the PR body.
  • Existing rows are safe: schema unchanged, database/migrations/ empty, no lifecycle hook — nothing runs at deploy. Not fixed by this PR: already-evicted victims (their old chatId was overwritten pre-fix and is unrecoverable).
🤖 Prompt for AI agents
Verify against current code. Fix only if still valid; keep the change minimal.

Context:
- src/api/telegram-subscription/services/telegram-subscription.ts, linkSubscriptionViaBot
- find-then-create dedup has a TOCTOU window: no unique constraint on (account, chatId), so concurrent /start redemptions from the same chat can insert duplicate rows.
- If hardening: add a Knex migration for a composite unique index on (account, chat_id) in database/migrations/*.js (must be .js, idempotent/IF NOT EXISTS, self-healing dedupe of existing dups, with a down()), and catch the unique-violation in create() to stay idempotent.

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

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.

Intentional decision to not change the db.

@azebuado
azebuado merged commit 9e1491e into main Sep 30, 2026
3 of 4 checks passed
@azebuado
azebuado deleted the fix/telegram-subscription branch September 30, 2026 14:54
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