fix(callback-url): single-use, conversation-bound callback tokens with 7-day TTL (#194)

[patrick-chinchill]

Summary

Ports the callback-token half of upstream's conversation-boundary hardening. Callback-URL button tokens (value = "__cb:<token>") are now:

• bound to the button that minted them (actionId) and to the conversation they were posted in (scope: {id, type});
• single-use: resolving takes a 10 s per-token lock, then deletes the record;
• shorter-lived: the TTL drops from 30 days to 7.

The callback POST now reports the stored actionId. The token swap also keeps every button field except callback_url.

Breaking for in-flight tokens. See Consumer impact below.

Upstream commits mapped

Upstream	Ported here
b7c9316b fix(chat): tighten conversation boundaries (vercel/chat#875, chat@4.40.0)	Callback-token part only: record shape, 7-day TTL, locked resolve with strict validation and actionId + scope match, delete, release_lock in finally, thread / channel / DM-fallback / channel-edit scopes, Chat passes {action_id, channel_id, thread_id} and POSTs the stored actionId. The rest of that commit is in #195, #229/#230 and #235.
4a0b5c0c feat(cards): button tooltips and card width hint (vercel/chat#895, chat@4.40.0)	Button copy only: {k: v for k, v in el.items() if k != "callback_url"} plus the encoded value. The tooltip / width fields belong to #202.

Changes

• src/chat_sdk/callback_url.py:
  • CALLBACK_TTL_MS is now 7 days. New constant CALLBACK_LOCK_TTL_MS = 10_000.
  • New dataclasses CallbackScope(id, type) and CallbackContext(action_id, channel_id=None, thread_id=None).
  • ResolvedCallback gains keyword-only action_id and scope.
  • process_card_callback_urls(card, state, scope) takes a required scope.
  • resolve_callback_url(token, state, context=None): returns None if the lock is held. Otherwise it gets the record and validates it with isinstance, never truthiness. It then matches actionId and the scope id, deletes the record, and releases the lock in finally. A None context never matches, and a mismatch leaves the record in place.
• thread.py / channel.py:
  • _process_callback_urls(postable, scope=None) defaults to {thread.id, "thread"} for a thread and {channel.id, "channel"} for a channel.
  • post_ephemeral mints only on the path that delivers. The native path uses the default scope. The DM fallback mints after open_dm, bound to {adapter.channel_id_from_thread_id(dm_thread_id), "channel"}. When neither path runs, no token is minted.
  • Channel SentMessage.edit binds to {channel.id, "channel"} (divergence, below).
• chat.py _handle_action_event builds the CallbackContext (channel_id comes from channel_id_from_thread_id(event.thread_id) when a thread id is present) and sends the stored actionId in the POST.

Tests

Ported from upstream (chat@4.41.1):

• callback-url.test.ts:
  • "keeps every other button field when replacing the callback URL" (uses Button(tooltip=) from [4.41/C10] Cards & modals core: Chart, Table options, Button tooltip, Card width, DateInput/NumberInput, select change events #202).
  • "rejects legacy unbound callback records"
  • "allows a callback token to be consumed only once"
  • "rejects callback tokens outside their action and thread"
  • "rejects callback tokens outside their channel"
  • The existing processCardCallbackUrls tests now pass a scope, and "resolves stored callback with URL and original value" also asserts the scope and that the record was deleted.
• thread.test.ts: "should bind fallback DM callbacks to the DM channel" (slack:DU456). "should encode callbackUrl when posting a card" now asserts the thread scope.
• channel.test.ts: "should bind fallback DM callbacks to the DM channel" (slack:DU1), ported explicitly. "should encode callbackUrl tokens when posting a card" now asserts {id: "slack:C123", type: "channel"}.
• chat.test.ts: the testtoken123, tok999, tok555 and bad-token fixtures gain actionId and scope. tests/integration/test_replay_callback_url.py is updated the same way (slack:C00FAKECHAN1, as upstream).

Python-specific:

• TestResolveCallbackUrlValidation: parametrized malformed records are rejected and not deleted. An empty-string actionId passes, as upstream. The snake_case original_value key is no longer read. A None context never matches.
• TestResolveCallbackUrlLocking:
  • asyncio.gather of two resolves on MemoryStateAdapter yields exactly one result. get yields inside the critical section, so the test fails without the lock.
  • A held lock returns None without reading.
  • The lock is taken on the record key with a 10 000 ms TTL.
  • release_lock is awaited when get raises.
• TestProcessCardCallbackUrlsStoredRecord: pins the exact camelCase record and the 7-day TTL. originalValue is omitted, never None.
• TestActionsCallbackTokenBinding (test_chat_faithful.py): the resolver receives the click context and the POST carries the stored actionId; a repeat click dispatches the raw value with no second POST; a click from another thread neither resolves nor consumes the token.
• post_ephemeral with no native path and no fallback_to_dm never awaits state.set.
• Channel edit round trip with the real Slack id functions and _handle_block_actions (channel and DM, synthetic slack:C…: post id and post-[4.41/SL4] Slack inbound normalization: self-mention decode, content is_mention, mrkdwn fixes, bot author ids, socket retries #209 slack:C…:<ts> post id).
• ChannelImpl.post_ephemeral with no delivery path mints nothing (Channel twin of the Thread test); the channel DM-fallback test asserts exactly one token.

Fidelity

• Strict at the pin (chat@4.31.0): 733/733.
• Target report (--report-target, chat@4.41.1):

  Delta vs committed report (HEAD): missing 282 -> 276 (-6)
    packages/chat/src/callback-url.test.ts: 5 -> 0 (-5)
    packages/chat/src/thread.test.ts: 18 -> 17 (-1)
  

  After merging origin/main (256 missing on main), scripts/fidelity_target.json was regenerated rather than hand-merged: 250 missing (main 256 -> 250, -6).

Divergences

Two behavioral divergences, which is the budget from docs/UPSTREAM_SYNC.md. Both are in the non-parity table.

Channel edit callback scope (commit diverge(channel), widened in the convergence pass). Upstream binds a channel SentMessage.edit to {threadId, "thread"}, where threadId is the id the adapter reported for the post. Python binds every channel edit to {channel.id, "channel"}, the scope the original post used. The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream too), Python Slack reports the synthetic slack:C…: until #209 while clicks carry the message ts, a Slack DM click carries no ts even after #209 makes the post report one, and a chained edit drops the override until #195. Every click on the message derives the channel id, so edited buttons POST regardless of #209's merge order.

• It is no broader than the original post.
• Regression tests: the real-Slack round trip (test_edited_slack_channel_card_resolves_for_the_real_click, 4 cases), the real-Teams-id round trip (test_edited_teams_channel_card_resolves_for_a_click_in_that_channel) and test_chained_edit_keeps_callback_tokens_resolvable.
• Upstream issue: to be filed (Teams / Google Chat edited channel cards never POST).

The lease fence (Astra round 2). After deleting a matched record, resolve_callback_url calls extend_lock(lock, CALLBACK_LOCK_TTL_MS). If that fails, the 10 s lease lapsed mid-consume, and the call returns None (fail closed). Upstream returns the record regardless, so a get or delete that stalls past the lease lets a second click take the expired lock, resolve the same token, and POST again. extend_lock checks token ownership and expiry in every backend, and Chat already relies on it for lock heartbeats.

• Documented in the non-parity table and in CHANGELOG, with a code breadcrumb.
• Regression test: test_lost_lease_fails_closed_instead_of_double_consuming.
• Landed in d856645 under a fix(callback-url) prefix; relabeled by the empty diverge(callback-url) commit (no force-push). The squash title/body should carry both diverge(channel) and diverge(callback-url). Upstream issue: to be filed (a stalled consume past the lease double-POSTs).

Two Python-surface notes:

• test_handles_legacy_string_format is kept, not deleted (the issue said to delete it). "handles legacy string format" is still in callback-url.test.ts at the strict pin (chat@4.31.0), so deleting the def fails --strict (732/733). It now asserts the new behavior, where the rejection test does not: a legacy string record resolves nothing even with a context that would match, and it is not deleted. It can be folded into the rejection test when the pin moves ([4.41/C11] Bump fidelity pin + UPSTREAM_PARITY to chat@4.41.1, record non-parity rows, cut 0.4.41 #203).
• ResolvedCallback.action_id / scope are keyword-only, so existing positional ResolvedCallback(url, original_value) construction still binds the same fields.

docs/UPSTREAM_SYNC.md gains a "Callback-URL tokens (chat@4.40, #194)" section. It records the legacy-record rejection and the dropped original_value fallback, which no Python release ever wrote, and it has the per-adapter channel_id_from_thread_id check.

Verify-first results

• Downstream: chinchill-api pins v0.4.31.1. A grep of its Python sources finds callback_url only in OAuth redirect code (mcp_servers.py, integrations.py, integration_oauth_service.py). There is no card Button(callback_url=), resolve_callback_url or __cb: use, so nothing depends on repeat clicks or on the 30-day TTL.

• channel_id_from_thread_id vs ChannelImpl.id: thread.channel derives its id with the same function in every adapter (derive_channel_id). chat.channel(id) matches whenever id is canonical:

  • Slack slack:C…
  • Discord discord:{guild}:{channel}
  • Google Chat gchat:spaces/…
  • GitHub github:owner/repo
  • Linear linear:{issueId}
  • Telegram telegram:{chatId}
  • Teams: the thread id re-encoded with ;messageid= stripped
  • WhatsApp, Messenger and Twilio (after [4.41/TW1] Twilio: isolate locks and channels per conversation, restrict authenticated media downloads #235): the thread id itself

  No adapter diverges.

Known thread-scope click-id mismatches (Astra rounds 1–3)

The adapter reports a click thread_id that differs from the thread id the card was posted under. The click still runs handlers with the raw __cb: value, but nothing is POSTed. Recorded in docs/UPSTREAM_SYNC.md and CHANGELOG:

• Cards thread.posted into a Google Chat DM thread (…:dm). This is upstream behavior at chat@4.41.1, because handleCardClick omits the :dm suffix. Changing the Google Chat click thread id would be an adapter divergence that changes handler thread ids, so it is left out of this core PR. The post_ephemeral DM fallback is unaffected, because it binds to the DM channel.

The former Slack channel-edit gap (synthetic slack:C…: post id, until #209) is closed by the widened channel-edit divergence above, which also covers Slack DMs after #209, Teams, Google Chat and chained edits.

Consumer impact

Breaking for callback_url users (CHANGELOG entry under "Unreleased (4.41 wave)", marked Breaking):

• Tokens minted before the upgrade stop resolving. The click runs handlers with the raw __cb:… value and nothing is POSTed.
• A repeat click no longer POSTs.
• Tokens expire after 7 days.
• The POST actionId is now the minted button's id.
• The record is deleted before the POST, so a failed POST is not retried, as upstream.

There is no impact on Slack/Teams streaming, and none on chinchill, which does not use card callback URLs.

Validation

ruff check, ruff format --check, audit_test_quality.py (0 hard failures), verify_test_fidelity.py --check-docs and --strict at 4.31.0 (733/733) all pass, and pytest reports 5929 passed, 24 skipped (after merging origin/main). pyrefly check reports 0 errors.

Merge gate

Review findings (2 reviewers, 5 items):

• Fixed: Slack channel edit regression (edited slack:C… card buttons never POST until [4.41/SL4] Slack inbound normalization: self-mention decode, content is_mention, mrkdwn fixes, bot author ids, socket retries #209). Channel SentMessage.edit now always binds to the channel. Failing-first: test_edited_slack_channel_card_resolves_for_the_real_click (real SlackAdapter ids + _handle_block_actions) fails on the previous HEAD.
• Fixed: post-[4.41/SL4] Slack inbound normalization: self-mention decode, content is_mention, mrkdwn fixes, bot author ids, socket retries #209 Slack DM channel edits would stop POSTing. Same fix; the DM cases are in the same parametrized test. CHANGELOG / UPSTREAM_SYNC wording updated (the "until [4.41/SL4] Slack inbound normalization: self-mention decode, content is_mention, mrkdwn fixes, bot author ids, socket retries #209" known gap is gone).
• Fixed: missing Channel post_ephemeral no-delivery-path test. Added test_post_ephemeral_without_delivery_path_mints_no_callback_token to test_channel_faithful.py plus a one-token assertion in the DM-fallback test; both kill the pre-mint mutation.
• Fixed (process), reported twice: lease-fence divergence landed under a fix(...) prefix. An empty diverge(callback-url) commit relabels d856645; the squash message must carry both diverge(...) labels. Non-parity rows now say the upstream issues are to be filed (not filed from this automation).

Main merge: origin/main merged; docs/UPSTREAM_SYNC.md conflict resolved (both sections kept; #202's "interim callback-URL gap (until #194)" bullet updated since this PR closes it), CHANGELOG #202 tooltip note updated, test_callback_url.py now uses the real Button(tooltip=), fidelity_target.json regenerated (256 -> 250).

gpt-6-astra: 1 round on 2b50c8f: "No actionable regressions found." (clean).

Bots: CodeRabbit skipped (draft, then rate limited); no gemini or inline review comments.

CI on 2b50c8f: Tests 3.12/3.13, Lint & Type Check (ruff, audit, pin drift, strict fidelity, pyrefly), CodeQL: all green.

Closes #194
Part of #184

[coderabbitai]

Warning

Review limit reached

Next included review available in 20 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: cce4a870-a1e6-4c65-9e06-1e60be6abacc

📥 Commits

Reviewing files that changed from the base of the PR and between 9ab938f and 84bfd56.

📒 Files selected for processing (12)

• CHANGELOG.md
• docs/UPSTREAM_SYNC.md
• scripts/fidelity_target.json
• src/chat_sdk/callback_url.py
• src/chat_sdk/channel.py
• src/chat_sdk/chat.py
• src/chat_sdk/thread.py
• tests/integration/test_replay_callback_url.py
• tests/test_callback_url.py
• tests/test_channel_faithful.py
• tests/test_chat_faithful.py
• tests/test_thread_faithful.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[patrick-chinchill]

Merge gate: CI green on 84bfd56 (Lint & Type Check, test (3.12), test (3.13), CodeQL / Analyze (python, actions)); local Codex review (gpt-6-astra, xhigh, --base origin/main) on 2b50c8f: "No actionable regressions found. The full test suite passed (5,929 passed, 24 skipped), and lint and type checks passed."; 5 astra rounds (rounds 1-3 findings fixed, round 4 and the converge re-review clean). 84bfd56 only merges origin/main (#221, #250): no conflicts, Teams-only files with no callback-token interaction, so no fresh review was needed. Local full validation passed: 6023 passed, strict fidelity OK, pyrefly 0 errors, fidelity_target unchanged (missing 250 -> 250). CodeRabbit is rate-limited and no other bot findings are open. Merging with --admin (Protect Main requires a code-owner approval).