From 1f8985c254efc6bc1cbe7992d0fd05359cfd862f Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 03:17:17 -0700 Subject: [PATCH 1/6] fix(callback-url): single-use, conversation-bound callback tokens with 7-day TTL (#194) Port the callback-token half of upstream b7c9316b (vercel/chat#875, chat@4.40.0) and the button copy from 4a0b5c0c (vercel/chat#895): - store {actionId, url, originalValue?, scope: {id, type}}; TTL 30d -> 7d - resolve_callback_url takes a CallbackContext, locks the token for 10s, validates strictly, matches actionId + scope, deletes, releases in finally - thread/channel posts bind to their own scope; the post_ephemeral DM fallback mints after open_dm, bound to the DM channel; channel SentMessage.edit binds to the message's thread - Chat POSTs the stored actionId - token swap keeps every button field except callback_url Breaking for in-flight tokens: legacy records stop resolving, repeat clicks no longer POST. Closes #194 --- CHANGELOG.md | 8 + docs/UPSTREAM_SYNC.md | 59 +++ scripts/fidelity_target.json | 42 +- src/chat_sdk/callback_url.py | 150 +++++-- src/chat_sdk/channel.py | 28 +- src/chat_sdk/chat.py | 20 +- src/chat_sdk/thread.py | 24 +- tests/integration/test_replay_callback_url.py | 7 +- tests/test_callback_url.py | 367 +++++++++++++++++- tests/test_channel_faithful.py | 58 +++ tests/test_chat_faithful.py | 90 ++++- tests/test_thread_faithful.py | 41 ++ 12 files changed, 805 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fdbb7a78..098861ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,14 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - `--report-target` prints its delta against the report committed at `HEAD` (via `git show`), so a second run before committing no longer compares against the first run's output and prints `+0`. - `--check-docs` is case-insensitive and also catches `--branch=`, `-b`, line-continued and markdown-decorated pins. - `scripts/fidelity_target.json` is committed as the authoritative wave-wide missing list at `chat@4.41.1`: 282 missing of 1036 (130 in strict-tier files, 152 in target-tier files), including 16 `.each` templates. +- **Breaking (security) — callback-URL button tokens are single-use, bound to their button and conversation, and expire after 7 days** (#194). Ports the callback-token part of upstream `b7c9316b` (vercel/chat#875, chat@4.40.0) and the button copy from `4a0b5c0c` (vercel/chat#895, chat@4.40.0). + - **Pre-upgrade tokens stop resolving.** A token is now stored as `{actionId, url, originalValue?, scope: {id, type}}`. Records written before the upgrade (`{url, originalValue}` or a bare URL string) are rejected: clicking such a button runs `on_action` handlers with the raw `__cb:…` value and POSTs nothing. The resolver also no longer reads a snake_case `original_value` key. + - **A repeat click no longer POSTs.** Resolving a token deletes it, under a 10-second per-token state lock. A second click, or one that lands while the first holds the lock, dispatches the raw `__cb:…` value without a POST. The record is deleted before the POST, so a failed POST is not retried. + - **Tokens expire after 7 days** (was 30). + - **The POST `actionId` is now the minted button's id**, read from the stored record, instead of the incoming event's `action_id`. + - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts and schedules bind to the channel, and a channel `SentMessage.edit` binds to the message's thread. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. + - The token swap now keeps every button field except `callback_url` (it used to copy a fixed whitelist), so new fields such as `tooltip` (#202) survive. + - API: `process_card_callback_urls(card, state, scope)` takes a required `CallbackScope`. `resolve_callback_url(token, state, context=None)` takes a `CallbackContext` (a `None` context never matches). `ResolvedCallback` gains keyword-only `action_id` and `scope`. New constant: `CALLBACK_LOCK_TTL_MS = 10_000`. - **BREAKING (security) — Telegram: webhook verification is required by default; repeated updates are deduplicated** (#224; upstream vercel/chat#858, #799, #813). - Telegram webhook deployments without `TELEGRAM_WEBHOOK_SECRET_TOKEN` / `secret_token` now **fail to start or return 401**: `mode="webhook"` raises `ValidationError` in the constructor, `mode="auto"` raises from `initialize()` when it resolves to webhook mode (so `Chat` initialization fails and is retried on every webhook until the config is fixed), and `handle_webhook` returns 401 `"Webhook verification required"` before reading the body. Previously the adapter logged a warning and dispatched every update, including `callback_query` button actions. - **Escape hatch:** `TelegramAdapterConfig(allow_unverified_webhooks=True)` or `TELEGRAM_ALLOW_UNVERIFIED_WEBHOOKS=true` (only the exact string `"true"` counts; an explicit `allow_unverified_webhooks=False` wins over the env var; a non-`bool` value such as `"false"` raises `ValidationError` rather than being treated as truthy). Polling mode needs neither. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 311963b0..3be4a01a 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -346,6 +346,65 @@ Regression coverage: `tests/test_twilio_adapter.py::TestThreadIds` (`test_uses_the_full_dm_thread_id_as_its_channel_id`, `test_isolates_concurrent_recipients_with_thread_scoped_locks`). +### Callback-URL tokens (chat@4.40, #194) + +Parity, not a divergence. The callback-token part of upstream `b7c9316b` +(vercel/chat#875, chat@4.40.0) plus the button copy from `4a0b5c0c` +(vercel/chat#895): + +- **Stored record.** `chat:callback:` now holds + `{"actionId", "url", "originalValue"?, "scope": {"id", "type"}}`. The keys + stay camelCase so either SDK can resolve the other's tokens. + `originalValue` is omitted when the button has no value. The TTL is 7 days + (was 30). +- **Scope.** Thread posts, schedules and edits bind to + `{thread.id, "thread"}`. Channel posts and schedules bind to + `{channel.id, "channel"}`, and a channel `SentMessage.edit` binds to + `{thread_id, "thread"}`. The `post_ephemeral` DM fallback mints tokens only + after `open_dm`, bound to + `{adapter.channel_id_from_thread_id(dm_thread_id), "channel"}`. When neither + the native path nor the DM fallback runs, no token is minted. +- **Resolve.** `resolve_callback_url(token, state, context)` acquires + `acquire_lock("chat:callback:", 10_000)` and returns `None` if the + lock is taken. Otherwise it validates the record, requires `actionId` and + the scope id to match the click's `CallbackContext`, deletes the record, and + releases the lock in `finally`. The scope id is `channel_id` for a channel + scope and `thread_id` for a thread scope. The action's POST body carries the + stored `actionId`. +- **Legacy records are rejected.** A bare URL string (the "legacy string + format" the old resolver accepted), a pre-upgrade `{url, originalValue}` + record, or any other object without `actionId` / `scope` no longer resolves. Such a click + dispatches the raw `__cb:` value and nothing is POSTed, as upstream. + Validation uses `isinstance` checks, not truthiness, so an empty-string + `actionId` still passes, and a present `originalValue` must be a `str`. +- **Dropped Python-only fallback.** The resolver used to fall back to a + snake_case `original_value` key. No Python release wrote that key, and + upstream never read it, so it is gone. Only `originalValue` is read. +- **Button copy.** The token swap copies every button key except + `callback_url` (it used to copy a whitelist), so fields such as `tooltip` + (#202) survive. + +Channel-scoped tokens resolve only when `adapter.channel_id_from_thread_id` +of the clicked message's thread equals the `ChannelImpl.id` that minted the +token. In every adapter, `thread.channel` derives its id with that same +function (`derive_channel_id`). For a `chat.channel(id)` handle the ids match +when `id` is canonical for the adapter (Slack `slack:C123`, Discord +`discord:{guild}:{channel}`, Google Chat `gchat:spaces/X`, GitHub +`github:owner/repo`, Linear `linear:{issueId}`, Telegram `telegram:{chatId}`, +Teams: the thread id re-encoded with `;messageid=…` stripped from the +conversation id; WhatsApp, Messenger and +Twilio (#235): the thread id itself). + +**Breaking for `callback_url` users:** tokens minted before the upgrade stop +resolving, a repeat click no longer POSTs, tokens expire after 7 days, and the +POST's `actionId` is the minted button's id. The record is deleted before the +POST, so a failed POST is not retried, as upstream. Regression coverage: +`tests/test_callback_url.py` (including the Python-specific +`TestResolveCallbackUrlValidation` / `TestResolveCallbackUrlLocking`), +`tests/test_chat_faithful.py::TestActionsCallbackTokenBinding`, and the +`should bind fallback DM callbacks to the DM channel` ports in +`tests/test_thread_faithful.py` and `tests/test_channel_faithful.py`. + ## What to Port vs What to Adapt ### Port 1:1 diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index 4dfd6f60..eb5091c7 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -5,10 +5,10 @@ "totals": { "ts_tests": 1036, "each_templates": 16, - "matched_exact": 671, - "matched_fuzzy": 83, - "missing": 282, - "extra": 362 + "matched_exact": 678, + "matched_fuzzy": 82, + "missing": 276, + "extra": 377 }, "absent_ts_files": [], "files": { @@ -21,7 +21,7 @@ "matched_exact": 132, "matched_fuzzy": 12, "missing_count": 33, - "extra_count": 13, + "extra_count": 16, "missing": [ ["Chat", "should call onNewMention for newline-separated GitHub bot mentions"], ["Chat", "should not call onNewMention for concatenated GitHub bot mention text"], @@ -78,10 +78,10 @@ "python_exists": true, "ts_tests": 159, "each_templates": 1, - "matched_exact": 133, + "matched_exact": 134, "matched_fuzzy": 8, - "missing_count": 18, - "extra_count": 28, + "missing_count": 17, + "extra_count": 29, "missing": [ ["post with Plan", "should auto-complete all in_progress tasks when switching back to default addTask"], ["post with Plan", "should target the most recent in_progress task when updating without id"], @@ -99,8 +99,7 @@ ["reply()", "converts JSX cards before delegating"], ["reply()", "buffers streams into one markdown reply"], ["reply()", "resolves a message id against messages the thread knows"], - ["reply()", "leaves replyTo undefined for an unknown message id"], - ["callbackUrl processing", "should bind fallback DM callbacks to the DM channel"] + ["reply()", "leaves replyTo undefined for an unknown message id"] ], "fuzzy_matches": [ ["should pass stream options from Slack current message context via $label", "test_should_pass_stream_options_from_current_message_context"], @@ -119,14 +118,13 @@ "python_exists": true, "ts_tests": 67, "each_templates": 0, - "matched_exact": 65, - "matched_fuzzy": 2, + "matched_exact": 66, + "matched_fuzzy": 1, "missing_count": 0, - "extra_count": 4, + "extra_count": 6, "missing": [], "fuzzy_matches": [ - ["should resolve a JSX Card and its children before scheduling", "test_should_convert_jsx_card_elements_to_cardelement"], - ["should bind fallback DM callbacks to the DM channel", "test_should_leave_channel_unchanged_when_chat_rebind_lookup_fails"] + ["should resolve a JSX Card and its children before scheduling", "test_should_convert_jsx_card_elements_to_cardelement"] ] }, "packages/chat/src/markdown.test.ts": { @@ -301,17 +299,11 @@ "python_exists": true, "ts_tests": 21, "each_templates": 0, - "matched_exact": 16, + "matched_exact": 21, "matched_fuzzy": 0, - "missing_count": 5, - "extra_count": 1, - "missing": [ - ["processCardCallbackUrls", "keeps every other button field when replacing the callback URL"], - ["resolveCallbackUrl", "rejects legacy unbound callback records"], - ["resolveCallbackUrl", "allows a callback token to be consumed only once"], - ["resolveCallbackUrl", "rejects callback tokens outside their action and thread"], - ["resolveCallbackUrl", "rejects callback tokens outside their channel"] - ], + "missing_count": 0, + "extra_count": 10, + "missing": [], "fuzzy_matches": [] }, "packages/chat/src/thread-history.test.ts": { diff --git a/src/chat_sdk/callback_url.py b/src/chat_sdk/callback_url.py index c573928d..26702ae9 100644 --- a/src/chat_sdk/callback_url.py +++ b/src/chat_sdk/callback_url.py @@ -1,6 +1,6 @@ """Callback URL handling for buttons and modals. -Python port of callback-url.ts (vercel/chat#454). +Python port of callback-url.ts (vercel/chat#454, vercel/chat#875). When a button (or modal) carries a ``callback_url``, the SDK stores the URL in the state adapter under a short random token at post time and rewrites @@ -9,14 +9,20 @@ the original value for handlers, and POSTs the action payload to the stored URL. Modal callback URLs are stored in the modal context and POSTed on submit. + +Since chat@4.40.0 (vercel/chat#875) a button token is bound to the button +that minted it (``actionId``) and to the conversation it was posted in +(``scope``: a thread or a channel). A click resolves the token only when +both match, and a resolved token is deleted, so each token is consumed at +most once. """ from __future__ import annotations import json import secrets -from dataclasses import dataclass -from typing import Any +from dataclasses import dataclass, field +from typing import Any, Literal from chat_sdk.cards import ActionsElement, ButtonElement, CardChild, CardElement from chat_sdk.errors import ChatError @@ -24,7 +30,8 @@ CALLBACK_TOKEN_PREFIX = "__cb:" CALLBACK_CACHE_KEY_PREFIX = "chat:callback:" -CALLBACK_TTL_MS = 30 * 24 * 60 * 60 * 1000 # 30 days +CALLBACK_TTL_MS = 7 * 24 * 60 * 60 * 1000 # 7 days +CALLBACK_LOCK_TTL_MS = 10_000 # --------------------------------------------------------------------------- @@ -39,12 +46,38 @@ class DecodedCallbackValue: callback_token: str | None +@dataclass(frozen=True) +class CallbackScope: + """The conversation a callback token is bound to. + + Stored as ``{"id", "type"}``. A ``"thread"`` scope matches the clicked + message's thread id; a ``"channel"`` scope matches the channel id the + adapter derives from it (``channel_id_from_thread_id``). + """ + + id: str + type: Literal["channel", "thread"] + + +@dataclass(frozen=True) +class CallbackContext: + """Where a button click happened; matched against a stored callback.""" + + action_id: str + channel_id: str | None = None + thread_id: str | None = None + + @dataclass(frozen=True) class ResolvedCallback: - """A stored callback resolved from the state adapter.""" + """A stored callback, resolved and consumed from the state adapter.""" url: str original_value: str | None = None + # Keyword-only so positional ``ResolvedCallback(url, original_value)`` + # keeps binding the same fields as before these were added. + action_id: str = field(kw_only=True) + scope: CallbackScope = field(kw_only=True) @dataclass(frozen=True) @@ -87,6 +120,7 @@ def _generate_token() -> str: async def _process_actions_element( actions: ActionsElement, state_adapter: StateAdapter, + scope: CallbackScope, ) -> ActionsElement: children: list[Any] = [] for el in actions.get("children", []): @@ -95,25 +129,21 @@ async def _process_actions_element( continue token = _generate_token() - # Stored shape matches the TS SDK (`{url, originalValue}`) so state - # written by either SDK resolves in both. `originalValue` is omitted - # (not None) when the button has no value — hazard #7. - stored: dict[str, Any] = {"url": el["callback_url"]} + # Stored shape matches the TS SDK (`{actionId, url, originalValue?, + # scope: {id, type}}`) so state written by either SDK resolves in + # both. `originalValue` is omitted (not None) when the button has no + # value — hazard #7. + stored: dict[str, Any] = {"actionId": el["id"], "url": el["callback_url"]} original_value = el.get("value") if original_value is not None: stored["originalValue"] = original_value + stored["scope"] = {"id": scope.id, "type": scope.type} await state_adapter.set(f"{CALLBACK_CACHE_KEY_PREFIX}{token}", stored, CALLBACK_TTL_MS) - # Rebuild the button without `callback_url` (mirrors upstream's - # explicit-key copy; keys absent on the source stay absent). - processed: ButtonElement = {"type": "button", "id": el["id"], "label": el["label"]} - if "style" in el: - processed["style"] = el["style"] - if "disabled" in el: - processed["disabled"] = el["disabled"] + # Keep every other button field so new ones (like tooltip) are not + # silently dropped; only the callback URL is replaced by the token. + processed: ButtonElement = {k: v for k, v in el.items() if k != "callback_url"} # type: ignore[assignment] processed["value"] = encode_callback_value(token) - if "action_type" in el: - processed["action_type"] = el["action_type"] children.append(processed) return {"type": "actions", "children": children} @@ -134,13 +164,14 @@ def _has_callback_buttons(children: list[CardChild]) -> bool: async def _process_children( children: list[CardChild], state_adapter: StateAdapter, + scope: CallbackScope, ) -> list[CardChild]: result: list[CardChild] = [] for child in children: if isinstance(child, dict) and child.get("type") == "actions": - result.append(await _process_actions_element(child, state_adapter)) # type: ignore[arg-type] + result.append(await _process_actions_element(child, state_adapter, scope)) # type: ignore[arg-type] elif isinstance(child, dict) and child.get("type") == "section" and "children" in child: - result.append({**child, "children": await _process_children(child["children"], state_adapter)}) # type: ignore[misc] + result.append({**child, "children": await _process_children(child["children"], state_adapter, scope)}) # type: ignore[misc] else: result.append(child) return result @@ -149,16 +180,18 @@ async def _process_children( async def process_card_callback_urls( card: CardElement, state_adapter: StateAdapter, + scope: CallbackScope, ) -> CardElement: """Replace ``callback_url`` buttons with encoded token values. + Each minted token is bound to its button's ``id`` and to ``scope``. Returns the *same* card object when no button carries a callback URL; otherwise returns a new card (the original is never mutated). """ if not _has_callback_buttons(card.get("children", [])): return card - return {**card, "children": await _process_children(card.get("children", []), state_adapter)} + return {**card, "children": await _process_children(card.get("children", []), state_adapter, scope)} # --------------------------------------------------------------------------- @@ -166,19 +199,78 @@ async def process_card_callback_urls( # --------------------------------------------------------------------------- +def _parse_stored_callback(stored: Any) -> ResolvedCallback | None: + """Validate a stored record strictly; ``None`` for any other shape. + + Mirrors upstream's ``typeof`` checks (never truthiness): a legacy + bare-string record, or one without ``actionId`` / ``scope``, is + rejected. An empty-string ``actionId`` is still a string and passes. + """ + if not isinstance(stored, dict): + return None + action_id = stored.get("actionId") + url = stored.get("url") + if not isinstance(action_id, str) or not isinstance(url, str): + return None + original_value = stored.get("originalValue") + if "originalValue" in stored and not isinstance(original_value, str): + return None + scope = stored.get("scope") + if not isinstance(scope, dict): + return None + scope_id = scope.get("id") + if not isinstance(scope_id, str): + return None + raw_type = scope.get("type") + scope_type: Literal["channel", "thread"] + if raw_type == "channel": + scope_type = "channel" + elif raw_type == "thread": + scope_type = "thread" + else: + return None + return ResolvedCallback( + url=url, + original_value=original_value, + action_id=action_id, + scope=CallbackScope(id=scope_id, type=scope_type), + ) + + async def resolve_callback_url( token: str, state_adapter: StateAdapter, + context: CallbackContext | None = None, ) -> ResolvedCallback | None: - """Look up a stored callback by token. Returns ``None`` when unknown.""" - stored = await state_adapter.get(f"{CALLBACK_CACHE_KEY_PREFIX}{token}") - if not stored: + """Resolve and consume a stored button callback. + + Returns ``None`` when the token is unknown or malformed, when a + concurrent click holds the token's lock, or when the record does not + match ``context`` (the clicked ``action_id`` and, depending on the + stored scope, ``channel_id`` or ``thread_id``). A ``None`` context + never matches. A matching record is deleted before returning, so each + token resolves at most once; a mismatch leaves the record in place. + """ + key = f"{CALLBACK_CACHE_KEY_PREFIX}{token}" + # Lock keys live in their own namespace in every state backend, so + # locking the value key does not touch the stored record. + lock = await state_adapter.acquire_lock(key, CALLBACK_LOCK_TTL_MS) + if lock is None: return None - if isinstance(stored, str): - # Legacy format: the URL was stored as a bare string. - return ResolvedCallback(url=stored) - original_value = stored["originalValue"] if "originalValue" in stored else stored.get("original_value") - return ResolvedCallback(url=stored.get("url"), original_value=original_value) + + try: + resolved = _parse_stored_callback(await state_adapter.get(key)) + if resolved is None or context is None: + return None + + scope_id = context.channel_id if resolved.scope.type == "channel" else context.thread_id + if resolved.action_id != context.action_id or resolved.scope.id != scope_id: + return None + + await state_adapter.delete(key) + return resolved + finally: + await state_adapter.release_lock(lock) async def _fetch(url: str, *, method: str, headers: dict[str, str], body: str) -> tuple[int, str]: diff --git a/src/chat_sdk/channel.py b/src/chat_sdk/channel.py index 3bf57130..0a426f82 100644 --- a/src/chat_sdk/channel.py +++ b/src/chat_sdk/channel.py @@ -12,7 +12,7 @@ from datetime import datetime, timezone from typing import Any -from chat_sdk.callback_url import process_card_callback_urls +from chat_sdk.callback_url import CallbackScope, process_card_callback_urls from chat_sdk.errors import ChatNotImplementedError from chat_sdk.plan import is_postable_object, post_postable_object from chat_sdk.thread import ( @@ -340,9 +340,10 @@ async def post_ephemeral( """Post an ephemeral message visible only to the specified user.""" user_id = user if isinstance(user, str) else user.user_id - message = await self._process_callback_urls(message) # type: ignore[assignment] - + # Callback tokens are minted per delivery path so each is bound to + # the conversation the card actually lands in (vercel/chat#875). if hasattr(self.adapter, "post_ephemeral") and self.adapter.post_ephemeral: # type: ignore[union-attr] + message = await self._process_callback_urls(message) # type: ignore[assignment] return await self.adapter.post_ephemeral(self._id, user_id, message) # type: ignore[union-attr] if not options.fallback_to_dm: @@ -350,6 +351,10 @@ async def post_ephemeral( if hasattr(self.adapter, "open_dm") and self.adapter.open_dm: # type: ignore[union-attr] dm_thread_id: str = await self.adapter.open_dm(user_id) # type: ignore[union-attr] + message = await self._process_callback_urls( # type: ignore[assignment] + message, + CallbackScope(id=self.adapter.channel_id_from_thread_id(dm_thread_id), type="channel"), + ) result: RawMessage = await self.adapter.post_message(dm_thread_id, message) return EphemeralMessage( id=result.id, @@ -379,20 +384,27 @@ async def schedule( async def _process_callback_urls( self, postable: str | AdapterPostableMessage, + scope: CallbackScope | None = None, ) -> str | AdapterPostableMessage: - """Encode ``callback_url`` buttons in outgoing cards (vercel/chat#454).""" + """Encode ``callback_url`` buttons in outgoing cards (vercel/chat#454). + + Tokens are bound to ``scope``, which defaults to this channel. + """ if isinstance(postable, str): return postable + if scope is None: + scope = CallbackScope(id=self._id, type="channel") + if isinstance(postable, dict) and postable.get("type") == "card": - return await process_card_callback_urls(postable, self._state_adapter) + return await process_card_callback_urls(postable, self._state_adapter, scope) if ( isinstance(postable, PostableCard) and isinstance(postable.card, dict) and postable.card.get("type") == "card" ): - processed = await process_card_callback_urls(postable.card, self._state_adapter) + processed = await process_card_callback_urls(postable.card, self._state_adapter, scope) if processed is not postable.card: return replace(postable, card=processed) @@ -548,7 +560,9 @@ def _create_sent_message( plain_text, formatted, attachments = _extract_message_content(postable) async def _edit(new_content: Any) -> SentMessage: - new_content = await channel_impl._process_callback_urls(new_content) + new_content = await channel_impl._process_callback_urls( + new_content, CallbackScope(id=thread_id, type="thread") + ) await adapter.edit_message(thread_id, message_id, new_content) return channel_impl._create_sent_message(message_id, new_content) diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 5e20de46..cdf18f1f 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -19,6 +19,7 @@ from typing import Any from chat_sdk.callback_url import ( + CallbackContext, decode_callback_value, post_to_callback_url, resolve_callback_url, @@ -1434,12 +1435,24 @@ async def _handle_action_event(self, event: ActionEvent) -> None: # Decode a callback token (`__cb:`) planted at post time by # process_card_callback_urls. When one resolves, handlers see the # button's original value and the action payload is POSTed to the - # stored callback URL concurrently with the handlers. + # stored callback URL concurrently with the handlers. A token only + # resolves for the button and conversation that minted it, and only + # once (vercel/chat#875); otherwise handlers see the raw value and + # nothing is POSTed. callback_token = decode_callback_value(event.value).callback_token resolved = None if callback_token: - resolved = await resolve_callback_url(callback_token, self._state_adapter) + channel_id = event.adapter.channel_id_from_thread_id(event.thread_id) if event.thread_id else None + resolved = await resolve_callback_url( + callback_token, + self._state_adapter, + CallbackContext( + action_id=event.action_id, + channel_id=channel_id, + thread_id=event.thread_id, + ), + ) action_value = resolved.original_value if resolved is not None else event.value @@ -1448,7 +1461,8 @@ async def _handle_action_event(self, event: ActionEvent) -> None: callback_url = resolved.url # Wire payload: camelCase keys, optional keys omitted (not None) # to mirror upstream's JSON.stringify semantics — hazard #7. - payload: dict[str, Any] = {"type": "action", "actionId": event.action_id} + # `actionId` is the minted button's id, as stored with the token. + payload: dict[str, Any] = {"type": "action", "actionId": resolved.action_id} if resolved.original_value is not None: payload["value"] = resolved.original_value payload["user"] = {"id": event.user.user_id, "name": event.user.user_name} diff --git a/src/chat_sdk/thread.py b/src/chat_sdk/thread.py index 182e7829..99ee85ae 100644 --- a/src/chat_sdk/thread.py +++ b/src/chat_sdk/thread.py @@ -14,7 +14,7 @@ from datetime import datetime, timezone from typing import TYPE_CHECKING, Any, Protocol, cast, runtime_checkable -from chat_sdk.callback_url import process_card_callback_urls +from chat_sdk.callback_url import CallbackScope, process_card_callback_urls from chat_sdk.errors import ChatNotImplementedError from chat_sdk.logger import Logger from chat_sdk.plan import is_postable_object, post_postable_object @@ -613,10 +613,11 @@ async def post_ephemeral( """ user_id = user if isinstance(user, str) else user.user_id - message = await self._process_callback_urls(message) # type: ignore[assignment] - + # Callback tokens are minted per delivery path so each is bound to + # the conversation the card actually lands in (vercel/chat#875). # Try native ephemeral if hasattr(self.adapter, "post_ephemeral") and self.adapter.post_ephemeral: # type: ignore[union-attr] + message = await self._process_callback_urls(message) # type: ignore[assignment] return await self.adapter.post_ephemeral(self._id, user_id, message) # type: ignore[union-attr] if not options.fallback_to_dm: @@ -625,6 +626,10 @@ async def post_ephemeral( # Fallback: send via DM if hasattr(self.adapter, "open_dm") and self.adapter.open_dm: # type: ignore[union-attr] dm_thread_id: str = await self.adapter.open_dm(user_id) # type: ignore[union-attr] + message = await self._process_callback_urls( # type: ignore[assignment] + message, + CallbackScope(id=self.adapter.channel_id_from_thread_id(dm_thread_id), type="channel"), + ) result: RawMessage = await self.adapter.post_message(dm_thread_id, message) return EphemeralMessage( id=result.id, @@ -638,20 +643,27 @@ async def post_ephemeral( async def _process_callback_urls( self, postable: str | AdapterPostableMessage, + scope: CallbackScope | None = None, ) -> str | AdapterPostableMessage: - """Encode ``callback_url`` buttons in outgoing cards (vercel/chat#454).""" + """Encode ``callback_url`` buttons in outgoing cards (vercel/chat#454). + + Tokens are bound to ``scope``, which defaults to this thread. + """ if isinstance(postable, str): return postable + if scope is None: + scope = CallbackScope(id=self._id, type="thread") + if isinstance(postable, dict) and postable.get("type") == "card": - return await process_card_callback_urls(postable, self._state_adapter) + return await process_card_callback_urls(postable, self._state_adapter, scope) if ( isinstance(postable, PostableCard) and isinstance(postable.card, dict) and postable.card.get("type") == "card" ): - processed = await process_card_callback_urls(postable.card, self._state_adapter) + processed = await process_card_callback_urls(postable.card, self._state_adapter, scope) if processed is not postable.card: return replace(postable, card=processed) diff --git a/tests/integration/test_replay_callback_url.py b/tests/integration/test_replay_callback_url.py index 2a0edc0b..711985ce 100644 --- a/tests/integration/test_replay_callback_url.py +++ b/tests/integration/test_replay_callback_url.py @@ -153,7 +153,12 @@ async def on_mention(thread, message, context): # done at post time. await ctx.state.set( f"chat:callback:{CALLBACK_TOKEN}", - {"url": CALLBACK_BUTTON_URL, "originalValue": "order-99"}, + { + "actionId": "approve", + "url": CALLBACK_BUTTON_URL, + "originalValue": "order-99", + "scope": {"id": "slack:C00FAKECHAN1", "type": "channel"}, + }, ) # Synthesize a block_actions payload with the SDK's encoded diff --git a/tests/test_callback_url.py b/tests/test_callback_url.py index 6a4a683c..02be2f14 100644 --- a/tests/test_callback_url.py +++ b/tests/test_callback_url.py @@ -1,4 +1,4 @@ -"""Faithful translation of callback-url.test.ts (17 tests). +"""Faithful translation of callback-url.test.ts (21 tests at chat@4.41.1). Each ``it("...")`` block from the TypeScript test suite is translated to a corresponding ``async def test_...`` method, preserving the same @@ -7,17 +7,27 @@ TS stubs the global ``fetch``; Python patches the ``_fetch`` seam in ``chat_sdk.callback_url`` (the lazy aiohttp wrapper). +Python-specific coverage for the token lock / consume path lives in +``TestResolveCallbackUrlLocking`` at the bottom of the resolve section. + TS file: packages/chat/src/callback-url.test.ts """ from __future__ import annotations +import asyncio import copy import json import re from unittest.mock import AsyncMock, call, patch +import pytest + from chat_sdk.callback_url import ( + CALLBACK_LOCK_TTL_MS, + CALLBACK_TTL_MS, + CallbackContext, + CallbackScope, decode_callback_value, encode_callback_value, post_to_callback_url, @@ -25,6 +35,7 @@ resolve_callback_url, ) from chat_sdk.cards import Actions, Button, Card, CardText, Section +from chat_sdk.state.memory import MemoryStateAdapter from chat_sdk.testing import MockStateAdapter, create_mock_state CALLBACK_TOKEN_PATTERN = re.compile(r"^__cb:[a-f0-9]{16}$") @@ -71,6 +82,9 @@ def test_roundtrips_encodedecode(self): # =========================================================================== +CHANNEL_SCOPE = CallbackScope(id="slack:C1", type="channel") + + class TestProcessCardCallbackUrls: """describe("processCardCallbackUrls")""" @@ -88,7 +102,7 @@ async def test_returns_card_unchanged_when_no_buttons_have_callbackurl(self): ], ) - result = await process_card_callback_urls(card, state) + result = await process_card_callback_urls(card, state, CHANNEL_SCOPE) assert result is card # it("encodes callbackUrl into button value and stores in state") @@ -109,7 +123,7 @@ async def test_encodes_callbackurl_into_button_value_and_stores_in_state(self): ], ) - result = await process_card_callback_urls(card, state) + result = await process_card_callback_urls(card, state, CallbackScope(id="slack:C1:1.1", type="thread")) actions = next(c for c in result["children"] if c["type"] == "actions") button = actions["children"][0] @@ -120,9 +134,14 @@ async def test_encodes_callbackurl_into_button_value_and_stores_in_state(self): decoded = decode_callback_value(button["value"]) assert decoded.callback_token is not None - resolved = await resolve_callback_url(decoded.callback_token, state) + resolved = await resolve_callback_url( + decoded.callback_token, + state, + CallbackContext(action_id="approve", thread_id="slack:C1:1.1"), + ) assert resolved is not None assert resolved.url == "https://example.com/webhook/123" + assert resolved.scope == CallbackScope(id="slack:C1:1.1", type="thread") # it("stores original value in state alongside callback URL") async def test_stores_original_value_in_state_alongside_callback_url(self): @@ -143,13 +162,17 @@ async def test_stores_original_value_in_state_alongside_callback_url(self): ], ) - result = await process_card_callback_urls(card, state) + result = await process_card_callback_urls(card, state, CHANNEL_SCOPE) button = next(c for c in result["children"] if c["type"] == "actions")["children"][0] assert CALLBACK_TOKEN_PATTERN.match(button["value"]) decoded = decode_callback_value(button["value"]) - resolved = await resolve_callback_url(decoded.callback_token or "", state) + resolved = await resolve_callback_url( + decoded.callback_token or "", + state, + CallbackContext(action_id="btn", channel_id="slack:C1"), + ) assert resolved is not None assert resolved.url == "https://hook.example.com" assert resolved.original_value == "item-99" @@ -173,7 +196,7 @@ async def test_only_processes_buttons_with_callbackurl_leaves_others_untouched(s ], ) - result = await process_card_callback_urls(card, state) + result = await process_card_callback_urls(card, state, CHANNEL_SCOPE) actions = next(c for c in result["children"] if c["type"] == "actions") normal_btn = actions["children"][0] callback_btn = actions["children"][1] @@ -181,6 +204,38 @@ async def test_only_processes_buttons_with_callbackurl_leaves_others_untouched(s assert normal_btn["value"] == "keep" assert CALLBACK_PREFIX_PATTERN.match(callback_btn["value"]) + # it("keeps every other button field when replacing the callback URL") + async def test_keeps_every_other_button_field_when_replacing_the_callback_url(self): + state = self._state() + button_in = Button( + id="approve", + label="Approve", + style="primary", + disabled=True, + action_type="modal", + callback_url="https://example.com/hook", + ) + # `Button(tooltip=...)` arrives with #202; a raw key proves unknown + # fields survive the token swap. + button_in["tooltip"] = "Approve the request" # type: ignore[typeddict-unknown-key] + card = Card(title="Test", children=[Actions([button_in])]) + + result = await process_card_callback_urls(card, state, CHANNEL_SCOPE) + actions = next(c for c in result["children"] if c["type"] == "actions") + button = actions["children"][0] + + assert {k: v for k, v in button.items() if k != "value"} == { + "type": "button", + "id": "approve", + "label": "Approve", + "style": "primary", + "disabled": True, + "action_type": "modal", + "tooltip": "Approve the request", + } + assert "callback_url" not in button + assert CALLBACK_PREFIX_PATTERN.match(button["value"]) + # it("processes buttons nested inside sections") async def test_processes_buttons_nested_inside_sections(self): state = self._state() @@ -204,7 +259,7 @@ async def test_processes_buttons_nested_inside_sections(self): ], ) - result = await process_card_callback_urls(card, state) + result = await process_card_callback_urls(card, state, CHANNEL_SCOPE) section = next(c for c in result["children"] if c["type"] == "section") actions = next(c for c in section["children"] if c["type"] == "actions") button = actions["children"][0] @@ -214,7 +269,11 @@ async def test_processes_buttons_nested_inside_sections(self): assert "callback_url" not in button decoded = decode_callback_value(button["value"]) - resolved = await resolve_callback_url(decoded.callback_token or "", state) + resolved = await resolve_callback_url( + decoded.callback_token or "", + state, + CallbackContext(action_id="nested-btn", channel_id="slack:C1"), + ) assert resolved is not None assert resolved.url == "https://example.com/nested" @@ -237,10 +296,55 @@ async def test_does_not_mutate_the_original_card(self): ) original = copy.deepcopy(card) - await process_card_callback_urls(card, state) + await process_card_callback_urls(card, state, CHANNEL_SCOPE) assert card == original +class TestProcessCardCallbackUrlsStoredRecord: + """Python-specific: the exact record written to state (cross-SDK shape).""" + + async def test_stores_camelcase_record_with_scope_and_seven_day_ttl(self): + state = create_mock_state() + state.set = AsyncMock(wraps=state.set) # type: ignore[method-assign] + card = Card( + children=[ + Actions( + [ + Button(id="with-value", label="A", value="v1", callback_url="https://example.com/a"), + Button(id="no-value", label="B", callback_url="https://example.com/b"), + ] + ) + ] + ) + + result = await process_card_callback_urls(card, state, CallbackScope(id="slack:C9:9.9", type="thread")) + + tokens = [decode_callback_value(b["value"]).callback_token for b in result["children"][0]["children"]] + assert state.set.await_args_list == [ + call( + f"chat:callback:{tokens[0]}", + { + "actionId": "with-value", + "url": "https://example.com/a", + "originalValue": "v1", + "scope": {"id": "slack:C9:9.9", "type": "thread"}, + }, + CALLBACK_TTL_MS, + ), + # `originalValue` is omitted, never written as None. + call( + f"chat:callback:{tokens[1]}", + { + "actionId": "no-value", + "url": "https://example.com/b", + "scope": {"id": "slack:C9:9.9", "type": "thread"}, + }, + CALLBACK_TTL_MS, + ), + ] + assert CALLBACK_TTL_MS == 7 * 24 * 60 * 60 * 1000 + + # =========================================================================== # resolveCallbackUrl # =========================================================================== @@ -260,22 +364,255 @@ async def test_resolves_stored_callback_with_url_and_original_value(self): state = create_mock_state() await state.set( "chat:callback:test-token", - {"url": "https://example.com/hook", "originalValue": "item-42"}, + { + "actionId": "approve", + "url": "https://example.com/hook", + "originalValue": "item-42", + "scope": {"id": "slack:C1:1.1", "type": "thread"}, + }, + ) + result = await resolve_callback_url( + "test-token", + state, + CallbackContext(action_id="approve", thread_id="slack:C1:1.1"), ) - result = await resolve_callback_url("test-token", state) assert result is not None assert result.url == "https://example.com/hook" assert result.original_value == "item-42" + assert result.scope == CallbackScope(id="slack:C1:1.1", type="thread") + assert await state.get("chat:callback:test-token") is None + + # it("rejects legacy unbound callback records") + async def test_rejects_legacy_unbound_callback_records(self): + state = create_mock_state() + await state.set("chat:callback:legacy-token", "https://example.com/hook") + result = await resolve_callback_url("legacy-token", state, CallbackContext(action_id="approve")) + assert result is None - # it("handles legacy string format") + # it("handles legacy string format") — chat@4.31.0 title, behavior now + # reversed. Kept so strict fidelity at the 4.31.0 pin stays green until + # the pin moves to 4.41.1 (#203), where upstream replaced it with the + # test above. Asserts what the rejection above does not: even a context + # that would match any record resolves nothing, and the legacy record is + # left for its TTL rather than deleted. async def test_handles_legacy_string_format(self): state = create_mock_state() await state.set("chat:callback:legacy-token", "https://example.com/hook") - result = await resolve_callback_url("legacy-token", state) + state.delete = AsyncMock(wraps=state.delete) # type: ignore[method-assign] + + result = await resolve_callback_url( + "legacy-token", + state, + CallbackContext(action_id="approve", channel_id="slack:C1", thread_id="slack:C1:1.1"), + ) + + assert result is None + state.delete.assert_not_awaited() + assert await state.get("chat:callback:legacy-token") == "https://example.com/hook" + assert state._locks == {} + + # it("allows a callback token to be consumed only once") + async def test_allows_a_callback_token_to_be_consumed_only_once(self): + state = create_mock_state() + await state.set( + "chat:callback:single-use", + { + "actionId": "approve", + "scope": {"id": "slack:C1", "type": "channel"}, + "url": "https://example.com/hook", + }, + ) + context = CallbackContext(action_id="approve", channel_id="slack:C1") + + first = await resolve_callback_url("single-use", state, context) + assert first is not None + assert first.url == "https://example.com/hook" + assert await resolve_callback_url("single-use", state, context) is None + + # it("rejects callback tokens outside their action and thread") + async def test_rejects_callback_tokens_outside_their_action_and_thread(self): + state = create_mock_state() + await state.set( + "chat:callback:bound-token", + { + "actionId": "approve", + "scope": {"id": "slack:C1:1.1", "type": "thread"}, + "url": "https://example.com/hook", + }, + ) + + assert ( + await resolve_callback_url( + "bound-token", + state, + CallbackContext(action_id="deny", thread_id="slack:C1:1.1"), + ) + is None + ) + assert ( + await resolve_callback_url( + "bound-token", + state, + CallbackContext(action_id="approve", thread_id="slack:C1:2.2"), + ) + is None + ) + assert await state.get("chat:callback:bound-token") is not None + + # it("rejects callback tokens outside their channel") + async def test_rejects_callback_tokens_outside_their_channel(self): + state = create_mock_state() + await state.set( + "chat:callback:channel-token", + { + "actionId": "approve", + "scope": {"id": "slack:C1", "type": "channel"}, + "url": "https://example.com/hook", + }, + ) + + assert ( + await resolve_callback_url( + "channel-token", + state, + CallbackContext(action_id="approve", channel_id="slack:C2", thread_id="slack:C2:2.2"), + ) + is None + ) + resolved = await resolve_callback_url( + "channel-token", + state, + CallbackContext(action_id="approve", channel_id="slack:C1", thread_id="slack:C1:2.2"), + ) + assert resolved is not None + assert resolved.url == "https://example.com/hook" + + +# Python-specific: strict record validation (isinstance, never truthiness). +_VALID_RECORD = { + "actionId": "approve", + "url": "https://example.com/hook", + "scope": {"id": "slack:C1", "type": "channel"}, +} + + +class TestResolveCallbackUrlValidation: + """Python-specific: record shapes upstream's ``typeof`` checks reject.""" + + @pytest.mark.parametrize( + "record", + [ + pytest.param({**_VALID_RECORD, "actionId": None}, id="actionId-none"), + pytest.param({k: v for k, v in _VALID_RECORD.items() if k != "actionId"}, id="actionId-missing"), + pytest.param({**_VALID_RECORD, "url": 42}, id="url-not-str"), + pytest.param({**_VALID_RECORD, "originalValue": None}, id="originalValue-none"), + pytest.param({**_VALID_RECORD, "originalValue": 7}, id="originalValue-not-str"), + pytest.param({k: v for k, v in _VALID_RECORD.items() if k != "scope"}, id="scope-missing"), + pytest.param({**_VALID_RECORD, "scope": "slack:C1"}, id="scope-not-dict"), + pytest.param({**_VALID_RECORD, "scope": {"id": 1, "type": "channel"}}, id="scope-id-not-str"), + pytest.param({**_VALID_RECORD, "scope": {"id": "slack:C1", "type": "dm"}}, id="scope-type-unknown"), + pytest.param(["https://example.com/hook"], id="list"), + ], + ) + async def test_rejects_malformed_records_without_deleting(self, record): + state = create_mock_state() + state.cache["chat:callback:tok"] = record + + result = await resolve_callback_url("tok", state, CallbackContext(action_id="approve", channel_id="slack:C1")) + + assert result is None + assert state.cache["chat:callback:tok"] == record + + async def test_accepts_empty_string_action_id_like_upstream(self): + state = create_mock_state() + state.cache["chat:callback:tok"] = {**_VALID_RECORD, "actionId": ""} + + result = await resolve_callback_url("tok", state, CallbackContext(action_id="", channel_id="slack:C1")) + + assert result is not None + assert result.action_id == "" + assert result.original_value is None + + async def test_ignores_snake_case_original_value_key(self): + # The pre-4.41 Python-only `original_value` fallback read is gone: + # only the cross-SDK camelCase `originalValue` key is honored. + state = create_mock_state() + state.cache["chat:callback:tok"] = {**_VALID_RECORD, "original_value": "legacy"} + + result = await resolve_callback_url("tok", state, CallbackContext(action_id="approve", channel_id="slack:C1")) + assert result is not None - assert result.url == "https://example.com/hook" assert result.original_value is None + async def test_none_context_never_matches(self): + state = create_mock_state() + state.cache["chat:callback:tok"] = dict(_VALID_RECORD) + + assert await resolve_callback_url("tok", state) is None + assert "chat:callback:tok" in state.cache + + +class TestResolveCallbackUrlLocking: + """Python-specific: the consume path is serialized by a per-token lock.""" + + async def test_concurrent_resolves_yield_exactly_one_result(self): + state = MemoryStateAdapter() + await state.connect() + await state.set("chat:callback:race", dict(_VALID_RECORD)) + context = CallbackContext(action_id="approve", channel_id="slack:C1") + real_get = state.get + + async def yielding_get(key: str): + # Yield inside the critical section so the second click runs + # while the first still holds the token's lock. + await asyncio.sleep(0) + return await real_get(key) + + state.get = yielding_get # type: ignore[method-assign] + + results = await asyncio.gather( + resolve_callback_url("race", state, context), + resolve_callback_url("race", state, context), + ) + + assert sum(r is not None for r in results) == 1 + assert await state.get("chat:callback:race") is None + + async def test_returns_none_without_reading_when_lock_is_held(self): + state = create_mock_state() + state.cache["chat:callback:held"] = dict(_VALID_RECORD) + held = await state.acquire_lock("chat:callback:held", CALLBACK_LOCK_TTL_MS) + assert held is not None + state.get = AsyncMock(wraps=state.get) # type: ignore[method-assign] + + result = await resolve_callback_url("held", state, CallbackContext(action_id="approve", channel_id="slack:C1")) + + assert result is None + state.get.assert_not_awaited() + assert state.cache["chat:callback:held"] == _VALID_RECORD + + async def test_locks_the_record_key_with_a_ten_second_ttl(self): + state = create_mock_state() + state.cache["chat:callback:tok"] = dict(_VALID_RECORD) + + await resolve_callback_url("tok", state, CallbackContext(action_id="approve", channel_id="slack:C1")) + + assert state._acquire_lock_calls == [("chat:callback:tok", 10_000)] + # Released afterwards, so a later click is not locked out. + assert state._locks == {} + + async def test_releases_lock_when_get_raises(self): + state = create_mock_state() + state.get = AsyncMock(side_effect=RuntimeError("state down")) # type: ignore[method-assign] + state.release_lock = AsyncMock(wraps=state.release_lock) # type: ignore[method-assign] + + with pytest.raises(RuntimeError, match="state down"): + await resolve_callback_url("tok", state, CallbackContext(action_id="approve", channel_id="slack:C1")) + + state.release_lock.assert_awaited_once() + assert state.release_lock.await_args.args[0].thread_id == "chat:callback:tok" + assert state._locks == {} + # =========================================================================== # postToCallbackUrl diff --git a/tests/test_channel_faithful.py b/tests/test_channel_faithful.py index d14b1f2c..1428a72d 100644 --- a/tests/test_channel_faithful.py +++ b/tests/test_channel_faithful.py @@ -1446,6 +1446,7 @@ async def test_should_encode_callbackurl_tokens_when_posting_a_card(self): stored = await state.get(f"chat:callback:{decoded.callback_token}") assert stored is not None assert stored["url"] == "https://example.com/hook" + assert stored["scope"] == {"id": "slack:C123", "type": "channel"} # it("should encode callbackUrl when posting via postEphemeral") @pytest.mark.asyncio @@ -1484,6 +1485,39 @@ async def test_should_encode_callbackurl_when_posting_via_postephemeral(self): assert stored is not None assert stored["url"] == "https://example.com/eph" + # it("should bind fallback DM callbacks to the DM channel") + @pytest.mark.asyncio + async def test_should_bind_fallback_dm_callbacks_to_the_dm_channel(self): + adapter = create_mock_adapter() + state = create_mock_state() + channel = _make_channel(adapter, state) + + await channel.post_ephemeral( + "U1", + Card( + children=[ + Actions( + [ + Button( + id="ack", + label="Ack", + callback_url="https://example.com/dm", + ) + ] + ), + ] + ), + PostEphemeralOptions(fallback_to_dm=True), + ) + + dm_thread_id, sent_card = adapter._post_calls[0] + assert dm_thread_id == "slack:DU1:" + button = sent_card["children"][0]["children"][0] + callback_token = decode_callback_value(button["value"]).callback_token + stored = await state.get(f"chat:callback:{callback_token}") + assert stored is not None + assert stored["scope"] == {"id": "slack:DU1", "type": "channel"} + # it("should encode callbackUrl when scheduling") @pytest.mark.asyncio async def test_should_encode_callbackurl_when_scheduling(self): @@ -1555,6 +1589,30 @@ async def test_should_encode_callbackurl_when_editing_a_sent_card(self): assert stored is not None assert stored["url"] == "https://example.com/edit" + # Python-specific: an edited channel message binds its tokens to the + # thread the adapter reported for the post, not to the channel. + @pytest.mark.asyncio + async def test_edit_binds_callback_tokens_to_the_posted_thread(self): + adapter = create_mock_adapter() + state = create_mock_state() + + async def post_into_thread(channel_id: str, message: Any) -> RawMessage: + return RawMessage(id="msg-1", thread_id="slack:C123:1700.1", raw={}) + + adapter.post_channel_message = post_into_thread # type: ignore[assignment] + channel = _make_channel(adapter, state) + + sent = await channel.post("Hello") + await sent.edit(Card(children=[Actions([Button(id="redo", label="Redo", callback_url="https://e.com/x")])])) + + edit_thread_id, _, edited_card = adapter._edit_calls[0] + assert edit_thread_id == "slack:C123:1700.1" + callback_token = decode_callback_value(edited_card["children"][0]["children"][0]["value"]).callback_token + stored = await state.get(f"chat:callback:{callback_token}") + assert stored is not None + assert stored["actionId"] == "redo" + assert stored["scope"] == {"id": "slack:C123:1700.1", "type": "thread"} + # it("should pass plain string posts through unchanged") @pytest.mark.asyncio async def test_should_pass_plain_string_posts_through_unchanged(self): diff --git a/tests/test_chat_faithful.py b/tests/test_chat_faithful.py index 07a05a72..64cccc5b 100644 --- a/tests/test_chat_faithful.py +++ b/tests/test_chat_faithful.py @@ -17,6 +17,7 @@ import pytest +from chat_sdk.callback_url import CallbackContext, CallbackScope, ResolvedCallback from chat_sdk.chat import Chat from chat_sdk.emoji import get_emoji from chat_sdk.errors import ChatError, LockError @@ -4222,8 +4223,10 @@ async def _handler(event): chat.on_action("approve", _handler) state.cache["chat:callback:testtoken123"] = { + "actionId": "approve", "url": "https://example.com/webhook/hook1", "originalValue": "order-789", + "scope": {"id": "slack:C123", "type": "channel"}, } event = _make_action_event(adapter, action_id="approve", value="__cb:testtoken123") @@ -4256,7 +4259,11 @@ async def test_should_decode_callbackurl_token_with_no_original_value(self): chat.on_action(lambda event: received.append(event)) - state.cache["chat:callback:tok999"] = {"url": "https://example.com/webhook/hook2"} + state.cache["chat:callback:tok999"] = { + "actionId": "deny", + "url": "https://example.com/webhook/hook2", + "scope": {"id": "slack:C123:1234.5678", "type": "thread"}, + } event = _make_action_event(adapter, action_id="deny", value="__cb:tok999") @@ -4301,7 +4308,11 @@ async def _specific(event): chat.on_action("approve", _specific) - state.cache["chat:callback:tok555"] = {"url": "https://example.com/webhook/hook3"} + state.cache["chat:callback:tok555"] = { + "actionId": "approve", + "url": "https://example.com/webhook/hook3", + "scope": {"id": "slack:C123:1234.5678", "type": "thread"}, + } event = _make_action_event(adapter, action_id="approve", value="__cb:tok555") @@ -4313,6 +4324,75 @@ async def _specific(event): assert mock_fetch.await_count == 1 +class TestActionsCallbackTokenBinding: + """Python-specific: token binding and consumption through process_action.""" + + async def test_resolves_with_click_context_and_posts_the_stored_action_id(self): + chat, adapter, state = await _init_chat() + chat.on_action(lambda event: None) + resolved = ResolvedCallback( + url="https://example.com/webhook/ctx", + original_value="v", + action_id="minted-id", + scope=CallbackScope(id="slack:C123", type="channel"), + ) + event = _make_action_event(adapter, action_id="approve", value="__cb:ctxtoken") + + with ( + patch("chat_sdk.chat.resolve_callback_url", new=AsyncMock(return_value=resolved)) as mock_resolve, + patch("chat_sdk.callback_url._fetch", new=AsyncMock(return_value=(200, "ok"))) as mock_fetch, + ): + await _process_action_and_wait(chat, event) + + assert mock_resolve.await_args.args[0] == "ctxtoken" + assert mock_resolve.await_args.args[2] == CallbackContext( + action_id="approve", + channel_id="slack:C123", + thread_id="slack:C123:1234.5678", + ) + body = json.loads(mock_fetch.await_args.kwargs["body"]) + assert body["actionId"] == "minted-id" + + async def test_repeat_click_dispatches_raw_value_without_posting(self): + chat, adapter, state = await _init_chat() + received: list[ActionEvent] = [] + chat.on_action(lambda event: received.append(event)) + state.cache["chat:callback:once"] = { + "actionId": "approve", + "url": "https://example.com/webhook/once", + "originalValue": "order-1", + "scope": {"id": "slack:C123:1234.5678", "type": "thread"}, + } + + with patch("chat_sdk.callback_url._fetch", new=AsyncMock(return_value=(200, "ok"))) as mock_fetch: + await _process_action_and_wait(chat, _make_action_event(adapter, value="__cb:once")) + await _process_action_and_wait(chat, _make_action_event(adapter, value="__cb:once")) + + assert [e.value for e in received] == ["order-1", "__cb:once"] + assert mock_fetch.await_count == 1 + assert "chat:callback:once" not in state.cache + + async def test_click_in_another_thread_does_not_resolve_or_consume(self): + chat, adapter, state = await _init_chat() + received: list[ActionEvent] = [] + chat.on_action(lambda event: received.append(event)) + record = { + "actionId": "approve", + "url": "https://example.com/webhook/bound", + "scope": {"id": "slack:C123:1234.5678", "type": "thread"}, + } + state.cache["chat:callback:bound"] = dict(record) + + with patch("chat_sdk.callback_url._fetch", new=AsyncMock(return_value=(200, "ok"))) as mock_fetch: + await _process_action_and_wait( + chat, _make_action_event(adapter, value="__cb:bound", thread_id="slack:C999:1.1") + ) + + assert [e.value for e in received] == ["__cb:bound"] + assert mock_fetch.await_count == 0 + assert state.cache["chat:callback:bound"] == record + + # ============================================================================ # 25. Modal callbackUrl handling (vercel/chat#454) # ============================================================================ @@ -4558,7 +4638,11 @@ async def _handler(event): chat.on_action("approve", _handler) - state.cache["chat:callback:bad-token"] = {"url": "https://example.com/webhook/will-fail"} + state.cache["chat:callback:bad-token"] = { + "actionId": "approve", + "url": "https://example.com/webhook/will-fail", + "scope": {"id": "slack:C123:1234.5678", "type": "thread"}, + } event = _make_action_event(adapter, action_id="approve", value="__cb:bad-token") diff --git a/tests/test_thread_faithful.py b/tests/test_thread_faithful.py index f79ffb5a..0355a2df 100644 --- a/tests/test_thread_faithful.py +++ b/tests/test_thread_faithful.py @@ -3809,6 +3809,7 @@ async def test_should_encode_callbackurl_when_posting_a_card(self): stored = await state.get(f"chat:callback:{callback_token}") assert stored is not None assert stored["url"] == "https://example.com/post-hook" + assert stored["scope"] == {"id": "slack:C123:1234.5678", "type": "thread"} # it("should encode callbackUrl when posting via postEphemeral with native support") @pytest.mark.asyncio @@ -3841,6 +3842,46 @@ async def test_should_encode_callbackurl_when_posting_via_postephemeral_with_nat assert stored is not None assert stored["url"] == "https://example.com/eph" + # it("should bind fallback DM callbacks to the DM channel") + @pytest.mark.asyncio + async def test_should_bind_fallback_dm_callbacks_to_the_dm_channel(self): + adapter = create_mock_adapter() + state = create_mock_state() + thread = _make_thread(adapter, state) + + await thread.post_ephemeral( + "U456", + _make_card_with_callback("https://example.com/dm"), + PostEphemeralOptions(fallback_to_dm=True), + ) + + dm_thread_id, sent_card = adapter._post_calls[0] + assert dm_thread_id == "slack:DU456:" + button = sent_card["children"][0]["children"][0] + callback_token = decode_callback_value(button["value"]).callback_token + stored = await state.get(f"chat:callback:{callback_token}") + assert stored is not None + assert stored["scope"] == {"id": "slack:DU456", "type": "channel"} + + # Python-specific: with no native ephemeral and no DM fallback, nothing + # is delivered, so no callback token may be minted. + @pytest.mark.asyncio + async def test_post_ephemeral_without_delivery_path_mints_no_callback_token(self): + adapter = create_mock_adapter() + state = create_mock_state() + state.set = AsyncMock(wraps=state.set) # type: ignore[method-assign] + thread = _make_thread(adapter, state) + + result = await thread.post_ephemeral( + "U456", + _make_card_with_callback("https://example.com/none"), + PostEphemeralOptions(fallback_to_dm=False), + ) + + assert result is None + state.set.assert_not_awaited() + assert self._callback_keys(state) == [] + # it("should encode callbackUrl when scheduling a card") @pytest.mark.asyncio async def test_should_encode_callbackurl_when_scheduling_a_card(self): From c66f042509dc3efb9f0f8b3c781e09996c55272b Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 03:23:01 -0700 Subject: [PATCH 2/6] docs(callback-url): record thread-scope click-id mismatches (Slack until #209, Google Chat upstream parity) (#194) --- docs/UPSTREAM_SYNC.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 3be4a01a..34ce09e4 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -395,6 +395,27 @@ Teams: the thread id re-encoded with `;messageid=…` stripped from the conversation id; WhatsApp, Messenger and Twilio (#235): the thread id itself). +Thread-scoped tokens resolve only when the adapter's click `thread_id` +equals the thread id the card was posted or edited under. There are three +known mismatches. All are upstream behavior at chat@4.41.1 or are fixed by +another wave issue, so none is a divergence here. In each case the click +still runs `on_action` handlers with the raw `__cb:…` value, but nothing is +POSTed: + +- **Slack channel `SentMessage.edit`, until #209.** Python's + `post_channel_message` still returns the synthetic `slack:C…:` thread id, + but a click reports `slack:C…:`. Upstream `92530dd3` + (vercel/chat#720, chat@4.35.0) makes the post return `slack:C…:`, and + #209 ports it. `main` is not released mid-wave, so no consumer sees the gap. +- **Google Chat channel `SentMessage.edit`.** `post_channel_message` returns + the channel id as the thread id, upstream included, so an edited channel + card is bound to `gchat:spaces/X` as a thread. +- **Google Chat cards posted to a DM thread** (`gchat:spaces/X:dm`) by + `thread.post`. `_handle_card_click` encodes the clicked message's thread + name without the `:dm` suffix, as upstream `handleCardClick` does. The + `post_ephemeral` DM fallback is unaffected, because it binds to the DM + *channel*, which both ids share. + **Breaking for `callback_url` users:** tokens minted before the upgrade stop resolving, a repeat click no longer POSTs, tokens expire after 7 days, and the POST's `actionId` is the minted button's id. The record is deleted before the From d8566451067cf1c539715d575e6ea3b80ec94e48 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 03:31:31 -0700 Subject: [PATCH 3/6] fix(callback-url): fail closed when the token lease lapses mid-consume (#194) Python-specific hardening (divergence from upstream, documented in docs/UPSTREAM_SYNC.md): after deleting a matched record, resolve_callback_url confirms with extend_lock that its 10s lease never lapsed, so a stalled state call cannot let a second click also resolve and POST. Also records the chained channel-edit gap owned by #195. --- CHANGELOG.md | 2 ++ docs/UPSTREAM_SYNC.md | 13 ++++++++++--- scripts/fidelity_target.json | 4 ++-- src/chat_sdk/callback_url.py | 11 ++++++++++- tests/test_callback_url.py | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 57 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 098861ff..e0495a7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **The POST `actionId` is now the minted button's id**, read from the stored record, instead of the incoming event's `action_id`. - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts and schedules bind to the channel, and a channel `SentMessage.edit` binds to the message's thread. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. - The token swap now keeps every button field except `callback_url` (it used to copy a fixed whitelist), so new fields such as `tooltip` (#202) survive. + - **Python-specific (divergence from upstream):** after deleting a matched record, the resolver checks with `extend_lock` that its 10-second lease never lapsed, and returns `None` if it did. Upstream returns the record regardless, so a state call that stalls past the lease can let a second click also resolve and POST. See `docs/UPSTREAM_SYNC.md`. + - Known gaps, all upstream behavior or owned by other wave issues (see `docs/UPSTREAM_SYNC.md`): until #209, an edited Slack channel message's buttons never POST, because `post_channel_message` returns the synthetic `slack:C…:` thread id. Until #195, a second chained channel edit binds its buttons to the channel id as a thread. On Google Chat, edited channel messages and cards posted into a DM thread never POST (upstream parity). - API: `process_card_callback_urls(card, state, scope)` takes a required `CallbackScope`. `resolve_callback_url(token, state, context=None)` takes a `CallbackContext` (a `None` context never matches). `ResolvedCallback` gains keyword-only `action_id` and `scope`. New constant: `CALLBACK_LOCK_TTL_MS = 10_000`. - **BREAKING (security) — Telegram: webhook verification is required by default; repeated updates are deduplicated** (#224; upstream vercel/chat#858, #799, #813). - Telegram webhook deployments without `TELEGRAM_WEBHOOK_SECRET_TOKEN` / `secret_token` now **fail to start or return 401**: `mode="webhook"` raises `ValidationError` in the constructor, `mode="auto"` raises from `initialize()` when it resolves to webhook mode (so `Chat` initialization fails and is retried on every webhook until the config is fixed), and `handle_webhook` returns 401 `"Webhook verification required"` before reading the body. Previously the adapter logged a warning and dispatched every update, including `callback_query` button actions. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 34ce09e4..5d17271c 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -348,7 +348,8 @@ Regression coverage: `tests/test_twilio_adapter.py::TestThreadIds` ### Callback-URL tokens (chat@4.40, #194) -Parity, not a divergence. The callback-token part of upstream `b7c9316b` +Parity, except for one Python-only hardening: the lease fence in the +non-parity table (*Callback-token lease fence*). The callback-token part of upstream `b7c9316b` (vercel/chat#875, chat@4.40.0) plus the button copy from `4a0b5c0c` (vercel/chat#895): @@ -367,7 +368,8 @@ Parity, not a divergence. The callback-token part of upstream `b7c9316b` - **Resolve.** `resolve_callback_url(token, state, context)` acquires `acquire_lock("chat:callback:", 10_000)` and returns `None` if the lock is taken. Otherwise it validates the record, requires `actionId` and - the scope id to match the click's `CallbackContext`, deletes the record, and + the scope id to match the click's `CallbackContext`, deletes the record, + checks with `extend_lock` that the lease never lapsed (Python-only), and releases the lock in `finally`. The scope id is `channel_id` for a channel scope and `thread_id` for a thread scope. The action's POST body carries the stored `actionId`. @@ -396,7 +398,7 @@ conversation id; WhatsApp, Messenger and Twilio (#235): the thread id itself). Thread-scoped tokens resolve only when the adapter's click `thread_id` -equals the thread id the card was posted or edited under. There are three +equals the thread id the card was posted or edited under. There are four known mismatches. All are upstream behavior at chat@4.41.1 or are fixed by another wave issue, so none is a divergence here. In each case the click still runs `on_action` handlers with the raw `__cb:…` value, but nothing is @@ -407,6 +409,10 @@ POSTed: but a click reports `slack:C…:`. Upstream `92530dd3` (vercel/chat#720, chat@4.35.0) makes the post return `slack:C…:`, and #209 ports it. `main` is not released mid-wave, so no consumer sees the gap. +- **Chained channel edits (`sent = await sent.edit(...)` twice), until + #195.** The `SentMessage` returned by a channel edit drops the thread-id + override, so the second edit binds to the channel id as a thread. Upstream + `16ea171e` (vercel/chat#848) passes the thread id through, and #195 ports it. - **Google Chat channel `SentMessage.edit`.** `post_channel_message` returns the channel id as the thread id, upstream included, so an edited channel card is bound to `gchat:spaces/X` as a thread. @@ -835,6 +841,7 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | +| Callback-token lease fence (4.41 wave, #194) | 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` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Markdown parser | Subset of CommonMark (no setext headings, indented code, HTML, escaped chars, backtick spans >1) | Full CommonMark via remark | See [DECISIONS.md](DECISIONS.md#why-hand-rolled-markdown-parser) | | `_remend` streaming repair | Parity-based emphasis closing | `remend` npm package | Simplified; handles common cases | | `walkAst` | Deep-copies the tree (immutable) | Mutates the tree in place | Python convention; safer | diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index eb5091c7..a962a5fd 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -8,7 +8,7 @@ "matched_exact": 678, "matched_fuzzy": 82, "missing": 276, - "extra": 377 + "extra": 378 }, "absent_ts_files": [], "files": { @@ -302,7 +302,7 @@ "matched_exact": 21, "matched_fuzzy": 0, "missing_count": 0, - "extra_count": 10, + "extra_count": 11, "missing": [], "fuzzy_matches": [] }, diff --git a/src/chat_sdk/callback_url.py b/src/chat_sdk/callback_url.py index 26702ae9..a6f5de1e 100644 --- a/src/chat_sdk/callback_url.py +++ b/src/chat_sdk/callback_url.py @@ -249,7 +249,9 @@ async def resolve_callback_url( match ``context`` (the clicked ``action_id`` and, depending on the stored scope, ``channel_id`` or ``thread_id``). A ``None`` context never matches. A matching record is deleted before returning, so each - token resolves at most once; a mismatch leaves the record in place. + token resolves at most once; a mismatch leaves the record in place. If + the lock lease lapsed while the record was being consumed, the result is + ``None`` (fail closed) so a concurrent click cannot also POST. """ key = f"{CALLBACK_CACHE_KEY_PREFIX}{token}" # Lock keys live in their own namespace in every state backend, so @@ -268,6 +270,13 @@ async def resolve_callback_url( return None await state_adapter.delete(key) + # Python-specific fence (divergence from upstream — see + # docs/UPSTREAM_SYNC.md): if a stalled state call outlived the lock + # lease, another click may have taken the lock and consumed the same + # record. `extend_lock` succeeds only while our lease never lapsed, + # so a lost lease fails closed instead of POSTing a second time. + if not await state_adapter.extend_lock(lock, CALLBACK_LOCK_TTL_MS): + return None return resolved finally: await state_adapter.release_lock(lock) diff --git a/tests/test_callback_url.py b/tests/test_callback_url.py index 02be2f14..961cc08f 100644 --- a/tests/test_callback_url.py +++ b/tests/test_callback_url.py @@ -28,6 +28,7 @@ CALLBACK_TTL_MS, CallbackContext, CallbackScope, + ResolvedCallback, decode_callback_value, encode_callback_value, post_to_callback_url, @@ -578,6 +579,38 @@ async def yielding_get(key: str): assert sum(r is not None for r in results) == 1 assert await state.get("chat:callback:race") is None + async def test_lost_lease_fails_closed_instead_of_double_consuming(self): + """Divergence from upstream (docs/UPSTREAM_SYNC.md): upstream returns + the record even when its 10 s lease lapsed mid-consume, so a second + click that took the expired lock also resolves and both POST. + """ + state = MemoryStateAdapter() + await state.connect() + await state.set("chat:callback:stall", dict(_VALID_RECORD)) + context = CallbackContext(action_id="approve", channel_id="slack:C1") + real_delete = state.delete + stalled = False + second: list[ResolvedCallback | None] = [] + + async def stalled_delete(key: str) -> None: + nonlocal stalled + if not stalled: + stalled = True + # The lease lapses while this delete is in flight, and a + # second click takes the lock and consumes the same record. + await state.force_release_lock(key) + second.append(await resolve_callback_url("stall", state, context)) + await real_delete(key) + + state.delete = stalled_delete # type: ignore[method-assign] + + first = await resolve_callback_url("stall", state, context) + + assert first is None + assert second[0] is not None + assert second[0].url == "https://example.com/hook" + assert await state.get("chat:callback:stall") is None + async def test_returns_none_without_reading_when_lock_is_held(self): state = create_mock_state() state.cache["chat:callback:held"] = dict(_VALID_RECORD) From b4ab70b5b9b6939f3a7b4ecc75f2f1a00f802c27 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 03:37:53 -0700 Subject: [PATCH 4/6] diverge(channel): bind edited channel cards to the channel when no thread was reported (#194) Teams and Google Chat post_channel_message return the channel id as the thread id (upstream too), and a chained edit drops the override until #195, so upstream's {thread_id, "thread"} edit scope can never match a click. Bind those edits to the channel, as the original post did. Documented in the non-parity table; regression tests cover a real-Teams-id round trip and chained edits. --- CHANGELOG.md | 3 +- docs/UPSTREAM_SYNC.md | 35 ++++++++++--------- scripts/fidelity_target.json | 4 +-- src/chat_sdk/channel.py | 15 +++++++-- tests/test_channel_faithful.py | 61 ++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0495a7a..67fcd85b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,8 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts and schedules bind to the channel, and a channel `SentMessage.edit` binds to the message's thread. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. - The token swap now keeps every button field except `callback_url` (it used to copy a fixed whitelist), so new fields such as `tooltip` (#202) survive. - **Python-specific (divergence from upstream):** after deleting a matched record, the resolver checks with `extend_lock` that its 10-second lease never lapsed, and returns `None` if it did. Upstream returns the record regardless, so a state call that stalls past the lease can let a second click also resolve and POST. See `docs/UPSTREAM_SYNC.md`. - - Known gaps, all upstream behavior or owned by other wave issues (see `docs/UPSTREAM_SYNC.md`): until #209, an edited Slack channel message's buttons never POST, because `post_channel_message` returns the synthetic `slack:C…:` thread id. Until #195, a second chained channel edit binds its buttons to the channel id as a thread. On Google Chat, edited channel messages and cards posted into a DM thread never POST (upstream parity). + - **Python-specific (divergence from upstream):** a channel `SentMessage.edit` binds its tokens to the channel, not to `{channel id, "thread"}`, when the adapter reported no thread of its own for the post. This covers Teams and Google Chat channel posts and chained edits. Upstream's thread scope can never match a click there, so those edited buttons would never POST. + - Known gaps (see `docs/UPSTREAM_SYNC.md`): until #209, an edited Slack channel message's buttons never POST, because `post_channel_message` returns the synthetic `slack:C…:` thread id. Google Chat cards `thread.post`ed into a DM thread never POST (upstream parity: the card click omits the `:dm` suffix). - API: `process_card_callback_urls(card, state, scope)` takes a required `CallbackScope`. `resolve_callback_url(token, state, context=None)` takes a `CallbackContext` (a `None` context never matches). `ResolvedCallback` gains keyword-only `action_id` and `scope`. New constant: `CALLBACK_LOCK_TTL_MS = 10_000`. - **BREAKING (security) — Telegram: webhook verification is required by default; repeated updates are deduplicated** (#224; upstream vercel/chat#858, #799, #813). - Telegram webhook deployments without `TELEGRAM_WEBHOOK_SECRET_TOKEN` / `secret_token` now **fail to start or return 401**: `mode="webhook"` raises `ValidationError` in the constructor, `mode="auto"` raises from `initialize()` when it resolves to webhook mode (so `Chat` initialization fails and is retried on every webhook until the config is fixed), and `handle_webhook` returns 401 `"Webhook verification required"` before reading the body. Previously the adapter logged a warning and dispatched every update, including `callback_query` button actions. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 5d17271c..cbc207be 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -348,8 +348,9 @@ Regression coverage: `tests/test_twilio_adapter.py::TestThreadIds` ### Callback-URL tokens (chat@4.40, #194) -Parity, except for one Python-only hardening: the lease fence in the -non-parity table (*Callback-token lease fence*). The callback-token part of upstream `b7c9316b` +Parity, with two Python-only divergences recorded in the non-parity table: +*Callback-token lease fence* and *Channel edit callback scope*. The +callback-token part of upstream `b7c9316b` (vercel/chat#875, chat@4.40.0) plus the button copy from `4a0b5c0c` (vercel/chat#895): @@ -360,8 +361,9 @@ non-parity table (*Callback-token lease fence*). The callback-token part of upst (was 30). - **Scope.** Thread posts, schedules and edits bind to `{thread.id, "thread"}`. Channel posts and schedules bind to - `{channel.id, "channel"}`, and a channel `SentMessage.edit` binds to - `{thread_id, "thread"}`. The `post_ephemeral` DM fallback mints tokens only + `{channel.id, "channel"}`. A channel `SentMessage.edit` binds to + `{thread_id, "thread"}` when the post reported its own thread id, and + otherwise to `{channel.id, "channel"}` (a Python-only divergence). The `post_ephemeral` DM fallback mints tokens only after `open_dm`, bound to `{adapter.channel_id_from_thread_id(dm_thread_id), "channel"}`. When neither the native path nor the DM fallback runs, no token is minted. @@ -398,30 +400,30 @@ conversation id; WhatsApp, Messenger and Twilio (#235): the thread id itself). Thread-scoped tokens resolve only when the adapter's click `thread_id` -equals the thread id the card was posted or edited under. There are four -known mismatches. All are upstream behavior at chat@4.41.1 or are fixed by -another wave issue, so none is a divergence here. In each case the click -still runs `on_action` handlers with the raw `__cb:…` value, but nothing is -POSTed: +equals the thread id the card was posted or edited under. Two known +mismatches remain. Neither is a divergence: one is fixed by another wave +issue and the other is upstream behavior at chat@4.41.1. In both cases the +click still runs `on_action` handlers with the raw `__cb:…` value, but +nothing is POSTed: - **Slack channel `SentMessage.edit`, until #209.** Python's `post_channel_message` still returns the synthetic `slack:C…:` thread id, but a click reports `slack:C…:`. Upstream `92530dd3` (vercel/chat#720, chat@4.35.0) makes the post return `slack:C…:`, and #209 ports it. `main` is not released mid-wave, so no consumer sees the gap. -- **Chained channel edits (`sent = await sent.edit(...)` twice), until - #195.** The `SentMessage` returned by a channel edit drops the thread-id - override, so the second edit binds to the channel id as a thread. Upstream - `16ea171e` (vercel/chat#848) passes the thread id through, and #195 ports it. -- **Google Chat channel `SentMessage.edit`.** `post_channel_message` returns - the channel id as the thread id, upstream included, so an edited channel - card is bound to `gchat:spaces/X` as a thread. - **Google Chat cards posted to a DM thread** (`gchat:spaces/X:dm`) by `thread.post`. `_handle_card_click` encodes the clicked message's thread name without the `:dm` suffix, as upstream `handleCardClick` does. The `post_ephemeral` DM fallback is unaffected, because it binds to the DM *channel*, which both ids share. +The channel-edit divergence covers the edits where a thread scope could never +match a click: +- Teams and Google Chat edits, whose `post_channel_message` returns the channel + id as the thread id (upstream too); +- chained edits (`sent = await sent.edit(...)` twice), whose returned + `SentMessage` drops the thread-id override until #195 ports `16ea171e`. + **Breaking for `callback_url` users:** tokens minted before the upgrade stop resolving, a repeat click no longer POSTs, tokens expire after 7 days, and the POST's `actionId` is the minted button's id. The record is deleted before the @@ -841,6 +843,7 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | +| Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{thread_id, "thread"}` only when `thread_id` (the id the adapter reported for the post) differs from the channel id. Otherwise it binds to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` always binds to `{threadId, "thread"}`, where `threadId` is the reported id or the channel id | Teams and Google Chat `postChannelMessage` return the channel id as `threadId` (upstream as well), and so does a `SentMessage` returned by an edit until #195 ports `16ea171e`. A thread scope with the channel id never equals a click's thread id (Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), so upstream's edited buttons never POST there. Binding to the channel is no broader than the original post. Adapters whose thread id is the channel id (WhatsApp, Messenger, Twilio) resolve under either scope. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. | | Callback-token lease fence (4.41 wave, #194) | 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` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Markdown parser | Subset of CommonMark (no setext headings, indented code, HTML, escaped chars, backtick spans >1) | Full CommonMark via remark | See [DECISIONS.md](DECISIONS.md#why-hand-rolled-markdown-parser) | | `_remend` streaming repair | Parity-based emphasis closing | `remend` npm package | Simplified; handles common cases | diff --git a/scripts/fidelity_target.json b/scripts/fidelity_target.json index a962a5fd..bb627d2f 100644 --- a/scripts/fidelity_target.json +++ b/scripts/fidelity_target.json @@ -8,7 +8,7 @@ "matched_exact": 678, "matched_fuzzy": 82, "missing": 276, - "extra": 378 + "extra": 380 }, "absent_ts_files": [], "files": { @@ -121,7 +121,7 @@ "matched_exact": 66, "matched_fuzzy": 1, "missing_count": 0, - "extra_count": 6, + "extra_count": 8, "missing": [], "fuzzy_matches": [ ["should resolve a JSX Card and its children before scheduling", "test_should_convert_jsx_card_elements_to_cardelement"] diff --git a/src/chat_sdk/channel.py b/src/chat_sdk/channel.py index 0a426f82..f88a628f 100644 --- a/src/chat_sdk/channel.py +++ b/src/chat_sdk/channel.py @@ -559,10 +559,19 @@ def _create_sent_message( plain_text, formatted, attachments = _extract_message_content(postable) + # Upstream binds edited-card tokens to `{thread_id, "thread"}`. When the + # adapter reported no thread of its own for the post (Teams and Google + # Chat return the channel id; a chained edit drops the override), that + # id never equals a click's thread id, so bind to the channel as the + # original post did. Divergence from upstream — see docs/UPSTREAM_SYNC.md + edit_scope = ( + CallbackScope(id=thread_id, type="thread") + if thread_id != channel_impl._id + else CallbackScope(id=channel_impl._id, type="channel") + ) + async def _edit(new_content: Any) -> SentMessage: - new_content = await channel_impl._process_callback_urls( - new_content, CallbackScope(id=thread_id, type="thread") - ) + new_content = await channel_impl._process_callback_urls(new_content, edit_scope) await adapter.edit_message(thread_id, message_id, new_content) return channel_impl._create_sent_message(message_id, new_content) diff --git a/tests/test_channel_faithful.py b/tests/test_channel_faithful.py index 1428a72d..a45be593 100644 --- a/tests/test_channel_faithful.py +++ b/tests/test_channel_faithful.py @@ -1613,6 +1613,67 @@ async def post_into_thread(channel_id: str, message: Any) -> RawMessage: assert stored["actionId"] == "redo" assert stored["scope"] == {"id": "slack:C123:1700.1", "type": "thread"} + # Python-specific divergence (docs/UPSTREAM_SYNC.md): when the adapter + # reports the channel id as the post's thread id (Teams, Google Chat), + # upstream's `{channel_id, "thread"}` edit scope can never match a click. + # Round trip with the real Teams id functions and the replayed click shape. + @pytest.mark.asyncio + async def test_edited_teams_channel_card_resolves_for_a_click_in_that_channel(self): + pytest.importorskip("microsoft_teams") + from chat_sdk.adapters.teams.adapter import TeamsAdapter + from chat_sdk.adapters.teams.types import TeamsAdapterConfig, TeamsThreadId + from chat_sdk.callback_url import CallbackContext, resolve_callback_url + + adapter = TeamsAdapter(TeamsAdapterConfig(app_id="test-app-id", app_password="test-password")) + service_url = "https://smba.trafficmanager.net/teams/" + channel_id = adapter.encode_thread_id( + TeamsThreadId(conversation_id="19:abc@thread.tacv2", service_url=service_url) + ) + # Real `post_channel_message` returns `thread_id=channel_id`. + adapter.post_channel_message = AsyncMock( # type: ignore[method-assign] + return_value=RawMessage(id="1767297849909", thread_id=channel_id, raw={}) + ) + adapter.edit_message = AsyncMock(return_value=None) # type: ignore[method-assign] + state = create_mock_state() + channel = ChannelImpl(_ChannelImplConfigWithAdapter(id=channel_id, adapter=adapter, state_adapter=state)) + + sent = await channel.post("Hello") + await sent.edit(Card(children=[Actions([Button(id="redo", label="Redo", callback_url="https://e.com/x")])])) + + edited_card = adapter.edit_message.await_args.args[2] + token = decode_callback_value(edited_card["children"][0]["children"][0]["value"]).callback_token + assert token is not None + # A Teams card click reports the message's thread (`;messageid=`). + click_thread_id = adapter.encode_thread_id( + TeamsThreadId(conversation_id="19:abc@thread.tacv2;messageid=1767297849909", service_url=service_url) + ) + resolved = await resolve_callback_url( + token, + state, + CallbackContext( + action_id="redo", + channel_id=adapter.channel_id_from_thread_id(click_thread_id), + thread_id=click_thread_id, + ), + ) + assert resolved is not None + assert resolved.scope.type == "channel" + assert resolved.scope.id == channel_id + + @pytest.mark.asyncio + async def test_chained_edit_keeps_callback_tokens_resolvable(self): + channel, adapter, state, post_calls = self._make_tracked_channel() + + sent = await channel.post("Hello") + edited = await sent.edit("First edit") + await edited.edit(Card(children=[Actions([Button(id="redo", label="Redo", callback_url="https://e.com/x")])])) + + edited_card = adapter._edit_calls[1][2] + token = decode_callback_value(edited_card["children"][0]["children"][0]["value"]).callback_token + stored = await state.get(f"chat:callback:{token}") + assert stored is not None + assert stored["scope"] == {"id": "slack:C123", "type": "channel"} + # it("should pass plain string posts through unchanged") @pytest.mark.asyncio async def test_should_pass_plain_string_posts_through_unchanged(self): From c99e76e45635102795699a0f289f7b0e5dbd9a9b Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 04:05:18 -0700 Subject: [PATCH 5/6] diverge(channel): bind every channel SentMessage.edit callback token to the channel (#194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported thread id for a channel post never matches a click on Slack today (synthetic slack:C…:) nor on Slack DMs after #209 (DM clicks carry no ts), so the conditional thread scope left edited buttons unable to POST. Channel scope matches every click on the message and is no broader than the original post's scope. Also: ChannelImpl.post_ephemeral no-delivery-path test (mints nothing), single-token assertion on the DM-fallback test, and upstream-issue notes on both #194 divergences. The lease fence landed in d856645 under a fix() prefix; it is a divergence: diverge(callback-url) — see docs/UPSTREAM_SYNC.md. --- CHANGELOG.md | 6 +-- docs/UPSTREAM_SYNC.md | 38 ++++++------- src/chat_sdk/channel.py | 19 ++++--- tests/test_channel_faithful.py | 98 ++++++++++++++++++++++++++++------ 4 files changed, 113 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67fcd85b..e85e6af8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,11 @@ Sync wave from `chat@4.31.0` to `chat@4.41.1` (tracking #184). `UPSTREAM_PARITY` - **A repeat click no longer POSTs.** Resolving a token deletes it, under a 10-second per-token state lock. A second click, or one that lands while the first holds the lock, dispatches the raw `__cb:…` value without a POST. The record is deleted before the POST, so a failed POST is not retried. - **Tokens expire after 7 days** (was 30). - **The POST `actionId` is now the minted button's id**, read from the stored record, instead of the incoming event's `action_id`. - - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts and schedules bind to the channel, and a channel `SentMessage.edit` binds to the message's thread. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. + - Tokens resolve only for the button that minted them (`actionId`) and in the conversation they were posted to. Thread posts, schedules and edits bind to the thread. Channel posts, schedules and channel `SentMessage.edit`s bind to the channel. A `post_ephemeral` DM fallback binds to the DM channel, and with neither native ephemeral nor a DM fallback no token is minted. A click whose action or conversation does not match leaves the record in place. - The token swap now keeps every button field except `callback_url` (it used to copy a fixed whitelist), so new fields such as `tooltip` (#202) survive. - **Python-specific (divergence from upstream):** after deleting a matched record, the resolver checks with `extend_lock` that its 10-second lease never lapsed, and returns `None` if it did. Upstream returns the record regardless, so a state call that stalls past the lease can let a second click also resolve and POST. See `docs/UPSTREAM_SYNC.md`. - - **Python-specific (divergence from upstream):** a channel `SentMessage.edit` binds its tokens to the channel, not to `{channel id, "thread"}`, when the adapter reported no thread of its own for the post. This covers Teams and Google Chat channel posts and chained edits. Upstream's thread scope can never match a click there, so those edited buttons would never POST. - - Known gaps (see `docs/UPSTREAM_SYNC.md`): until #209, an edited Slack channel message's buttons never POST, because `post_channel_message` returns the synthetic `slack:C…:` thread id. Google Chat cards `thread.post`ed into a DM thread never POST (upstream parity: the card click omits the `:dm` suffix). + - **Python-specific (divergence from upstream):** a channel `SentMessage.edit` binds its tokens to the channel (the scope the original `channel.post` used), not to `{reported thread id, "thread"}`. The thread id an adapter reports for a channel post often never equals a click's thread id: Teams and Google Chat report the channel id, Slack reports the synthetic `slack:C…:` (a click carries the message ts, and a Slack DM click carries no ts even once #209 makes the post report one), and a chained edit drops the reported id. Upstream's thread scope would leave those edited buttons never POSTing. + - Known gap (see `docs/UPSTREAM_SYNC.md`): Google Chat cards `thread.post`ed into a DM thread never POST (upstream parity: the card click omits the `:dm` suffix). - API: `process_card_callback_urls(card, state, scope)` takes a required `CallbackScope`. `resolve_callback_url(token, state, context=None)` takes a `CallbackContext` (a `None` context never matches). `ResolvedCallback` gains keyword-only `action_id` and `scope`. New constant: `CALLBACK_LOCK_TTL_MS = 10_000`. - **BREAKING (security) — Telegram: webhook verification is required by default; repeated updates are deduplicated** (#224; upstream vercel/chat#858, #799, #813). - Telegram webhook deployments without `TELEGRAM_WEBHOOK_SECRET_TOKEN` / `secret_token` now **fail to start or return 401**: `mode="webhook"` raises `ValidationError` in the constructor, `mode="auto"` raises from `initialize()` when it resolves to webhook mode (so `Chat` initialization fails and is retried on every webhook until the config is fixed), and `handle_webhook` returns 401 `"Webhook verification required"` before reading the body. Previously the adapter logged a warning and dispatched every update, including `callback_query` button actions. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index cbc207be..41b9cee4 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -361,9 +361,9 @@ callback-token part of upstream `b7c9316b` (was 30). - **Scope.** Thread posts, schedules and edits bind to `{thread.id, "thread"}`. Channel posts and schedules bind to - `{channel.id, "channel"}`. A channel `SentMessage.edit` binds to - `{thread_id, "thread"}` when the post reported its own thread id, and - otherwise to `{channel.id, "channel"}` (a Python-only divergence). The `post_ephemeral` DM fallback mints tokens only + `{channel.id, "channel"}`. A channel `SentMessage.edit` also binds to + `{channel.id, "channel"}` (a Python-only divergence; upstream binds it to + the reported thread id). The `post_ephemeral` DM fallback mints tokens only after `open_dm`, bound to `{adapter.channel_id_from_thread_id(dm_thread_id), "channel"}`. When neither the native path nor the DM fallback runs, no token is minted. @@ -400,30 +400,32 @@ conversation id; WhatsApp, Messenger and Twilio (#235): the thread id itself). Thread-scoped tokens resolve only when the adapter's click `thread_id` -equals the thread id the card was posted or edited under. Two known -mismatches remain. Neither is a divergence: one is fixed by another wave -issue and the other is upstream behavior at chat@4.41.1. In both cases the +equals the thread id the card was posted or edited under. One known mismatch +remains, and it is upstream behavior at chat@4.41.1, not a divergence: the click still runs `on_action` handlers with the raw `__cb:…` value, but -nothing is POSTed: +nothing is POSTed. -- **Slack channel `SentMessage.edit`, until #209.** Python's - `post_channel_message` still returns the synthetic `slack:C…:` thread id, - but a click reports `slack:C…:`. Upstream `92530dd3` - (vercel/chat#720, chat@4.35.0) makes the post return `slack:C…:`, and - #209 ports it. `main` is not released mid-wave, so no consumer sees the gap. - **Google Chat cards posted to a DM thread** (`gchat:spaces/X:dm`) by `thread.post`. `_handle_card_click` encodes the clicked message's thread name without the `:dm` suffix, as upstream `handleCardClick` does. The `post_ephemeral` DM fallback is unaffected, because it binds to the DM *channel*, which both ids share. -The channel-edit divergence covers the edits where a thread scope could never -match a click: -- Teams and Google Chat edits, whose `post_channel_message` returns the channel - id as the thread id (upstream too); +The channel-edit divergence exists because the thread id reported for a +channel post often never equals a click's thread id: +- Teams and Google Chat `post_channel_message` return the channel id as the + thread id (upstream too); +- Slack `post_channel_message` returns the synthetic `slack:C…:` until #209 + ports upstream `92530dd3` (vercel/chat#720), while a click reports + `slack:C…:`. After #209 a Slack *DM* post reports + `slack:D…:`, but a DM click reports `slack:D…:` (Python's DM + `_handle_block_actions` divergence), so a thread scope would still miss; - chained edits (`sent = await sent.edit(...)` twice), whose returned `SentMessage` drops the thread-id override until #195 ports `16ea171e`. +Binding to the channel resolves all of these, independent of #209's merge +order, because every click on the message derives the same channel id. + **Breaking for `callback_url` users:** tokens minted before the upgrade stop resolving, a repeat click no longer POSTs, tokens expire after 7 days, and the POST's `actionId` is the minted button's id. The record is deleted before the @@ -843,8 +845,8 @@ stay explicit instead of being rediscovered in code review. | Area | Python behavior | TS behavior | Rationale | |------|----------------|-------------|-----------| | JSX Card/Modal elements | Not supported; tests skipped | `Card()` returns JSX element | Python has no JSX runtime | -| Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{thread_id, "thread"}` only when `thread_id` (the id the adapter reported for the post) differs from the channel id. Otherwise it binds to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` always binds to `{threadId, "thread"}`, where `threadId` is the reported id or the channel id | Teams and Google Chat `postChannelMessage` return the channel id as `threadId` (upstream as well), and so does a `SentMessage` returned by an edit until #195 ports `16ea171e`. A thread scope with the channel id never equals a click's thread id (Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), so upstream's edited buttons never POST there. Binding to the channel is no broader than the original post. Adapters whose thread id is the channel id (WhatsApp, Messenger, Twilio) resolve under either scope. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. | -| Callback-token lease fence (4.41 wave, #194) | 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` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | +| Channel edit callback scope (4.41 wave, #194) | A channel `SentMessage.edit` binds new callback tokens to `{channel.id, "channel"}`, the scope the original `channel.post` used | `createSentMessage(...).edit` binds to `{threadId, "thread"}`, where `threadId` is the id the adapter reported for the post (or the channel id) | The reported thread id often never equals a click's thread id: Teams and Google Chat report the channel id (upstream as well; Teams clicks carry `;messageid=`, Google Chat clicks carry the thread name), Python's 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 ports `16ea171e`. Upstream's edited buttons never POST in those cases. Every click on the message derives the channel id, and the channel scope is no broader than the original post's. Regression tests: `tests/test_channel_faithful.py::TestCallbackUrlProcessing::test_edited_slack_channel_card_resolves_for_the_real_click` (real Slack id functions and `_handle_block_actions`, channel and DM, before and after #209), `::test_edited_teams_channel_card_resolves_for_a_click_in_that_channel` (real Teams id functions) and `::test_chained_edit_keeps_callback_tokens_resolvable`. To be filed as an upstream issue against vercel/chat (Teams and Google Chat edited channel cards never POST); upstream Slack is unaffected, since its `postChannelMessage` and DM clicks both carry the message ts. | +| Callback-token lease fence (4.41 wave, #194) | 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` instead of the record (fail closed: no POST, raw `__cb:` value to handlers) | `resolveCallbackUrl` returns the record after `delete` regardless of lease state, so if a `get`/`delete` stalls past the lease, a second click that takes the expired lock also resolves it and both POST | Keeps the single-use contract under state-backend stalls. `extend_lock` checks token ownership and expiry in every backend (Memory, Redis script, Postgres `WHERE token = $4 AND expires_at > now()`), and `Chat` already relies on it for lock heartbeats. The cost is one extra state call per resolved click. A stalled consume that loses its lease also burns the token without a POST. To be filed as an upstream issue against vercel/chat (a stalled `get`/`delete` past the 10 s lease lets a second click double-POST). Regression test: `tests/test_callback_url.py::TestResolveCallbackUrlLocking::test_lost_lease_fails_closed_instead_of_double_consuming`. | | Markdown parser | Subset of CommonMark (no setext headings, indented code, HTML, escaped chars, backtick spans >1) | Full CommonMark via remark | See [DECISIONS.md](DECISIONS.md#why-hand-rolled-markdown-parser) | | `_remend` streaming repair | Parity-based emphasis closing | `remend` npm package | Simplified; handles common cases | | `walkAst` | Deep-copies the tree (immutable) | Mutates the tree in place | Python convention; safer | diff --git a/src/chat_sdk/channel.py b/src/chat_sdk/channel.py index f88a628f..ac5c5b59 100644 --- a/src/chat_sdk/channel.py +++ b/src/chat_sdk/channel.py @@ -559,16 +559,15 @@ def _create_sent_message( plain_text, formatted, attachments = _extract_message_content(postable) - # Upstream binds edited-card tokens to `{thread_id, "thread"}`. When the - # adapter reported no thread of its own for the post (Teams and Google - # Chat return the channel id; a chained edit drops the override), that - # id never equals a click's thread id, so bind to the channel as the - # original post did. Divergence from upstream — see docs/UPSTREAM_SYNC.md - edit_scope = ( - CallbackScope(id=thread_id, type="thread") - if thread_id != channel_impl._id - else CallbackScope(id=channel_impl._id, type="channel") - ) + # Upstream binds edited-card tokens to `{thread_id, "thread"}`. Python + # binds them to the channel, the scope the original `channel.post` + # used, because the thread id reported for a channel post often never + # equals a click's thread id: Teams and Google Chat report the channel + # id; Slack reports `slack:C…:` while a channel click carries the + # message ts and a DM click carries none; a chained edit drops the + # override. Every click on this message derives this channel id. + # Divergence from upstream — see docs/UPSTREAM_SYNC.md. + edit_scope = CallbackScope(id=channel_impl._id, type="channel") async def _edit(new_content: Any) -> SentMessage: new_content = await channel_impl._process_callback_urls(new_content, edit_scope) diff --git a/tests/test_channel_faithful.py b/tests/test_channel_faithful.py index a45be593..bc16b8f4 100644 --- a/tests/test_channel_faithful.py +++ b/tests/test_channel_faithful.py @@ -17,7 +17,7 @@ import pytest -from chat_sdk.callback_url import decode_callback_value +from chat_sdk.callback_url import CallbackScope, decode_callback_value from chat_sdk.cards import Actions, Button, Card from chat_sdk.channel import ChannelImpl, _ChannelImplConfigWithAdapter, derive_channel_id from chat_sdk.errors import ChatNotImplementedError @@ -1517,6 +1517,26 @@ async def test_should_bind_fallback_dm_callbacks_to_the_dm_channel(self): stored = await state.get(f"chat:callback:{callback_token}") assert stored is not None assert stored["scope"] == {"id": "slack:DU1", "type": "channel"} + assert len(self._callback_keys(state)) == 1 + + # Python-specific: tokens are minted per delivery path, so a channel + # ephemeral with no native path and no DM fallback mints nothing. + @pytest.mark.asyncio + async def test_post_ephemeral_without_delivery_path_mints_no_callback_token(self): + adapter = create_mock_adapter() + state = create_mock_state() + state.set = AsyncMock(wraps=state.set) # type: ignore[method-assign] + channel = _make_channel(adapter, state) + + result = await channel.post_ephemeral( + "U1", + Card(children=[Actions([Button(id="ack", label="Ack", callback_url="https://example.com/none")])]), + PostEphemeralOptions(fallback_to_dm=False), + ) + + assert result is None + state.set.assert_not_awaited() + assert self._callback_keys(state) == [] # it("should encode callbackUrl when scheduling") @pytest.mark.asyncio @@ -1589,29 +1609,73 @@ async def test_should_encode_callbackurl_when_editing_a_sent_card(self): assert stored is not None assert stored["url"] == "https://example.com/edit" - # Python-specific: an edited channel message binds its tokens to the - # thread the adapter reported for the post, not to the channel. + # Python-specific divergence (docs/UPSTREAM_SYNC.md): an edited channel + # card binds its tokens to the channel, not to the reported thread id. + # Round trip with the real Slack id functions and the real block_actions + # click, both for the synthetic `slack:C…:` post id Python reports today + # and for the `slack:C…:` id upstream 92530dd3 reports (#209). A DM + # click reports no ts, so a thread scope would miss it either way. @pytest.mark.asyncio - async def test_edit_binds_callback_tokens_to_the_posted_thread(self): - adapter = create_mock_adapter() - state = create_mock_state() + @pytest.mark.parametrize( + ("channel_id", "post_thread_id"), + [ + ("slack:C123", "slack:C123:"), + ("slack:C123", "slack:C123:1700.1"), + ("slack:D123", "slack:D123:"), + ("slack:D123", "slack:D123:1700.1"), + ], + ) + async def test_edited_slack_channel_card_resolves_for_the_real_click(self, channel_id: str, post_thread_id: str): + from chat_sdk.adapters.slack.adapter import SlackAdapter + from chat_sdk.adapters.slack.types import SlackAdapterConfig + from chat_sdk.callback_url import CallbackContext, resolve_callback_url - async def post_into_thread(channel_id: str, message: Any) -> RawMessage: - return RawMessage(id="msg-1", thread_id="slack:C123:1700.1", raw={}) + adapter = SlackAdapter(SlackAdapterConfig(bot_token="xoxb-test", signing_secret="s")) + clicks: list[Any] = [] - adapter.post_channel_message = post_into_thread # type: ignore[assignment] - channel = _make_channel(adapter, state) + class _ClickSink: + def process_action(self, event: Any, options: Any = None) -> None: + clicks.append(event) + + adapter._chat = _ClickSink() # type: ignore[assignment] + adapter.post_channel_message = AsyncMock( # type: ignore[method-assign] + return_value=RawMessage(id="1700.1", thread_id=post_thread_id, raw={}) + ) + adapter.edit_message = AsyncMock(return_value=None) # type: ignore[method-assign] + state = create_mock_state() + channel = ChannelImpl(_ChannelImplConfigWithAdapter(id=channel_id, adapter=adapter, state_adapter=state)) sent = await channel.post("Hello") await sent.edit(Card(children=[Actions([Button(id="redo", label="Redo", callback_url="https://e.com/x")])])) - edit_thread_id, _, edited_card = adapter._edit_calls[0] - assert edit_thread_id == "slack:C123:1700.1" - callback_token = decode_callback_value(edited_card["children"][0]["children"][0]["value"]).callback_token - stored = await state.get(f"chat:callback:{callback_token}") - assert stored is not None - assert stored["actionId"] == "redo" - assert stored["scope"] == {"id": "slack:C123:1700.1", "type": "thread"} + edit_thread_id, _, edited_card = adapter.edit_message.await_args.args + assert edit_thread_id == post_thread_id + token = decode_callback_value(edited_card["children"][0]["children"][0]["value"]).callback_token + assert token is not None + + raw_channel = channel_id.split(":")[1] + adapter._handle_block_actions( + { + "type": "block_actions", + "channel": {"id": raw_channel}, + "container": {"type": "message", "channel_id": raw_channel, "message_ts": "1700.1"}, + "message": {"ts": "1700.1"}, + "user": {"id": "U1", "username": "u"}, + "actions": [{"action_id": "redo", "value": f"__cb:{token}"}], + } + ) + click = clicks[0] + resolved = await resolve_callback_url( + token, + state, + CallbackContext( + action_id=click.action_id, + channel_id=adapter.channel_id_from_thread_id(click.thread_id), + thread_id=click.thread_id, + ), + ) + assert resolved is not None + assert resolved.scope == CallbackScope(id=channel_id, type="channel") # Python-specific divergence (docs/UPSTREAM_SYNC.md): when the adapter # reports the channel id as the post's thread id (Teams, Google Chat), From 56b6206cb5385271b889192cf8d93a1b975cf3db Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Wed, 30 Sep 2026 04:05:22 -0700 Subject: [PATCH 6/6] diverge(callback-url): fail closed when the token lease lapses mid-consume (#194) Relabels d856645, which landed this Python-only divergence under a fix() prefix (force-push is disallowed). Non-parity row: 'Callback-token lease fence' in docs/UPSTREAM_SYNC.md; upstream issue to be filed.