From 9fecb88ebeaa29b9e0de8457d28f784bf8b94e32 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 00:38:00 +0000 Subject: [PATCH 1/2] fix(slack): prefer webhook_verifier over signing_secret (vercel/chat#468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING: flips precedence to match upstream's reversal in vercel/chat#468 (commit 0f0c203, chat@4.29.0). When both webhook_verifier and signing_secret are configured, webhook_verifier now wins; SLACK_SIGNING_SECRET env var is also ignored when a verifier is set, so an env-configured deployment can't silently shadow a verifier the caller wired up. This reverses the precedence the Python port shipped in 0.4.27 (PR #87), which preferred signing_secret to track upstream's intent at that time — upstream has since reversed itself and this port follows. The stale comment at adapter.py:385 ("matches upstream") is updated accordingly. Part of the 0.4.29 sync wave; tracking issue #98. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- CHANGELOG.md | 8 ++++ docs/UPSTREAM_SYNC.md | 2 +- src/chat_sdk/adapters/slack/adapter.py | 40 +++++++++------- src/chat_sdk/adapters/slack/types.py | 11 +++-- .../test_slack_dynamic_token_and_verifier.py | 48 +++++++++++++++++-- 5 files changed, 84 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ead5f66..f516d8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## Unreleased + +Part of the 0.4.29 sync wave (tracking issue #98). + +### Behavior changes (Slack) + +- **`webhook_verifier` now takes precedence over `signing_secret`** (#108, vercel/chat#468, commit `0f0c203`). When a Slack adapter is constructed with both `webhook_verifier` and `signing_secret` (or with `webhook_verifier` while `SLACK_SIGNING_SECRET` is set in the env), the verifier wins and the signing-secret path is dropped entirely. This **reverses** the precedence the Python port shipped in 0.4.27 (PR #87), which preferred `signing_secret` to match upstream's intent at that time. Upstream reversed itself in vercel/chat#468 (`chat@4.29.0`) so an env-configured `SLACK_SIGNING_SECRET` could not silently shadow a verifier the caller wired up; this port now follows. **Migration:** if you relied on a configured `signing_secret` overriding `webhook_verifier`, drop the `webhook_verifier` from your `SlackAdapterConfig` (or, if you wired the verifier in deliberately, your signing-secret path is now correctly inert and you can remove it). The built-in HMAC + 5-minute timestamp tolerance only applies on the signing-secret path; verifier implementers remain responsible for replay protection (`slack/types.py` SECURITY contract). + ## 0.4.27 (2026-05-28) Synced to upstream `vercel/chat@4.27.0` (release commit `f55378a`, Apr 30 2026). Highlights: Slack Socket Mode + dynamic bot-token resolver, Teams native DM streaming, `chat.get_user()` across all 8 adapters, Telegram MarkdownV2 rendering, and a sweep of adapter bug fixes. Sets `UPSTREAM_PARITY = "4.27.0"`. diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 9dd621d..a5404da 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -624,7 +624,7 @@ stay explicit instead of being rediscovered in code review. | `RawMessage.text` override field — **transitional** | New optional `text: str \| None = None` field on `RawMessage` (`src/chat_sdk/types.py`). When set by an adapter, `Thread._handle_stream` MUST prefer it over its own local accumulator when constructing the recorded `SentMessage` body / message-history entry. `None` falls back to the local accumulator (backward-compatible default for adapters that don't need the override). Set by `_stream_via_emit` so the recorded message matches what Teams actually shipped, even when chunks were buffered into the throttle window and cancellation skipped the flush emit. | Upstream's `RawMessage` (`packages/chat/src/types.ts`) is `{ id; raw; threadId }` only. Cancellation-text reconciliation lives inside `@microsoft/teams.apps`'s `IStreamer.emit` (the npm SDK owns the buffer and never surfaces a buffered-but-unsent suffix to `chat`). | Direct consequence of the hand-rolled Teams native streaming row above. Without the override, the SDK's local accumulator (which captures every chunk yielded to the adapter, including chunks coalesced into the throttle window) would diverge from what Teams actually accepted whenever a session is canceled with buffered text pending — recording text the user never saw. Disappears alongside the hand-rolled wire format once we migrate to `microsoft-teams-apps` (Python). Regression coverage: `tests/test_thread_faithful.py::test_should_prefer_raw_message_text_override_over_local_accumulator` (would fail if someone "fixes" Thread.stream back to upstream's local-accumulator-only behavior) and `tests/test_teams_native_streaming.py::test_canceled_stream_sets_raw_message_text_override`. | | Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | | `SlackAdapter.current_token` / `current_token_async` / `current_client` | Public accessors that return the request-context-bound token and a preconfigured `AsyncWebClient`. `current_token` (sync `@property`) reads the cache; `current_token_async` (async method) invokes the resolver on demand for callable `bot_token` configs used outside `handle_webhook`. | Not exposed (`getToken()` is private on the TS `SlackAdapter`) | Python-only addition (issue #47). Downstream code that calls Slack Web APIs from inside a handler — email resolution, user profile fetches, reaction bookkeeping — otherwise depends on underscore-prefixed helpers. The async variant is required because the sync `current_token` cannot drive an async resolver (see `bot_token` resolver invocation site row). | -| `SlackAdapterConfig.webhook_verifier` | Optional `Callable[[request, body], bool \| str \| None \| Awaitable[...]]` that fully replaces signing-secret HMAC verification. Lets callers integrate platform-managed verification (e.g. Slack Enterprise Grid edge proxies, KMS-signed payloads, test harness escape hatches). Mutually exclusive with `signing_secret` — explicit `signing_secret` wins when both are passed. | Not exposed — TS only honors `SLACK_SIGNING_SECRET` HMAC verification (`signRequest` / `verifyRequestSignature`) | Python-only extension. The contract is documented as a SECURITY surface in `slack/types.py` (`SlackWebhookVerifier`): returning truthy passes the request, falsy/None rejects 401, and a `str` substitutes the request body before dispatch. Closes the same gap that drives `bot_token` resolvers (custom secret-manager integration) for the signing-secret half of the auth model. | +| `SlackAdapterConfig.webhook_verifier` | Optional `Callable[[request, body], bool \| str \| None \| Awaitable[...]]` that fully replaces signing-secret HMAC verification. Lets callers integrate platform-managed verification (e.g. Slack Enterprise Grid edge proxies, KMS-signed payloads, test harness escape hatches). `webhook_verifier` takes precedence over both `signing_secret` (config) and the `SLACK_SIGNING_SECRET` env var — when set, both are ignored. | Upstream has its own `webhookVerifier` field on `SlackAdapterConfig` and matches this precedence direction after vercel/chat#468 (commit `0f0c203`, chat@4.29.0). | Behavior parity restored in 0.4.29 sync wave. The original Python port (PR #87, 0.4.27) preferred `signing_secret` to match upstream's intent at that time; upstream reversed itself in #468 so an env-configured `SLACK_SIGNING_SECRET` could not silently shadow a verifier the caller wired up. This port follows. The contract is documented as a SECURITY surface in `slack/types.py` (`SlackWebhookVerifier`): returning truthy passes the request, falsy/None rejects 401, and a `str` substitutes the request body before dispatch. | | Slack `bot_token` resolver invocation site | Resolved once at `handle_webhook` entry into a per-request `ContextVar`; sync `_get_token` reads it for the rest of the request. Public adapter methods (`post_message`, `add_reaction`, `upload_files`, etc.) DON'T re-resolve — calling them outside `handle_webhook` (cron jobs, background tasks) with a callable `bot_token` raises `AuthenticationError` until the caller awaits `current_token_async()` first | TS `getToken` is async and resolves on EVERY API call site, so cron/background usage just works | Python keeps `_get_token` sync to preserve the existing pre-resolver public API and to avoid threading `await` through every adapter call site. The trade-off is that callable-`bot_token` usage outside the webhook flow needs an explicit `await adapter.current_token_async()` (or `await adapter._resolve_default_token()`) before the first sync-token-consuming call. Static-string `bot_token` is unaffected (cache primed at construction). | | Slack `bot_token` resolver caching scope | Single resolution per request, cached in `_resolved_default_token` `ContextVar` for the rest of that request | Provider invoked on every API call within a single request | Within-request caching enables the sync `_get_token` path. Functionally equivalent for rotation (TTL >> request lifetime); diverges only if the resolver is itself sensitive to per-call freshness (rare). | | `ConcurrencyConfig.max_concurrent` | Enforced via `asyncio.Semaphore` in the `"concurrent"` strategy path; rejects non-integer or `<= 0` values, and rejects any non-`None` `max_concurrent` paired with a non-`"concurrent"` strategy | Accepted into the config type with docstring "Default: Infinity" but never read (3 writes, 0 reads) | Silent correctness bug upstream — consumers setting `max_concurrent=N` with `strategy="concurrent"` reasonably expect an N-way bound on in-flight handlers. We honor the documented contract via a semaphore and fail-fast on misconfiguration so it's never silent. `max_concurrent=None` stays compatible with every strategy (unbounded default). | diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 4de18ce..6d7bfc8 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -276,9 +276,11 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: mode = config.mode or "webhook" - # An explicit ``webhook_verifier`` opts out of the SLACK_SIGNING_SECRET - # env fallback — otherwise an env-configured deployment would silently - # shadow the verifier the caller intended to use. + # ``webhook_verifier`` takes precedence over ``signing_secret`` (config) + # and the ``SLACK_SIGNING_SECRET`` env var. When the caller wires up a + # verifier we ignore both so an env-configured deployment can't silently + # shadow it (mirrors upstream vercel/chat#468, which reversed the + # original direction the Python port shipped in PR #87). # # Use explicit ``is not None`` checks rather than truthiness fallbacks # (per docs/UPSTREAM_SYNC.md hazard #1): an explicit empty string for @@ -317,13 +319,17 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: "slack", "webhook_verifier must be callable.", ) - signing_secret = ( - config.signing_secret - if config.signing_secret is not None - else (None if webhook_verifier is not None else os.environ.get("SLACK_SIGNING_SECRET")) - ) - if signing_secret == "": - signing_secret = None + if webhook_verifier is not None: + # Verifier wins: drop both the config ``signing_secret`` and the + # ``SLACK_SIGNING_SECRET`` env fallback. Mirrors upstream + # vercel/chat#468 (``webhookVerifier`` ?? undefined : (...)). + signing_secret: str | None = None + else: + signing_secret = ( + config.signing_secret if config.signing_secret is not None else os.environ.get("SLACK_SIGNING_SECRET") + ) + if signing_secret == "": + signing_secret = None # ``signing_secret`` is required in webhook mode (unless a # ``webhook_verifier`` replaces the built-in HMAC check). Socket mode # legitimately runs without a signing secret because Slack does not @@ -381,9 +387,9 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: self._name = "slack" self._signing_secret: str | None = signing_secret - # ``webhook_verifier`` is honored only when ``signing_secret`` is not set — - # signing_secret takes precedence when both are provided (matches upstream). - self._webhook_verifier: SlackWebhookVerifier | None = None if signing_secret else webhook_verifier + # ``webhook_verifier`` takes precedence; ``signing_secret`` is only used + # when no verifier is configured (matches upstream vercel/chat#468). + self._webhook_verifier: SlackWebhookVerifier | None = webhook_verifier # Resolver returning the default (single-workspace) bot token. ``None`` in # multi-workspace mode where the token is resolved per-team from the # InstallationStore. Single-workspace mode with a static string still @@ -1276,9 +1282,11 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No if self._mode == "socket": return {"body": "Webhooks are disabled in socket mode", "status": 405} - # Verify the request — custom verifier takes priority when configured - # without a signing_secret. The verifier may also return a string that - # replaces the body for downstream parsing (e.g. canonicalization). + # Verify the request — when a custom ``webhook_verifier`` is configured + # it takes precedence over ``signing_secret`` / ``SLACK_SIGNING_SECRET`` + # (matches upstream vercel/chat#468). The verifier may also return a + # string that replaces the body for downstream parsing (e.g. + # canonicalization). if self._webhook_verifier is not None: try: verifier_result = self._webhook_verifier(request, body) diff --git a/src/chat_sdk/adapters/slack/types.py b/src/chat_sdk/adapters/slack/types.py index f41328f..42c2a35 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/src/chat_sdk/adapters/slack/types.py @@ -85,15 +85,16 @@ class SlackAdapterConfig: # not required in socket mode (Slack does not sign socket events). mode: SlackAdapterMode = "webhook" # Signing secret for webhook verification. Defaults to SLACK_SIGNING_SECRET env var, - # *unless* ``webhook_verifier`` is provided — passing an explicit verifier opts - # out of the env fallback so a deployment-set ``SLACK_SIGNING_SECRET`` can't - # silently shadow the verifier. Required in webhook mode; optional in socket mode. + # *unless* ``webhook_verifier`` is provided — an explicit verifier takes + # precedence over both this field and the ``SLACK_SIGNING_SECRET`` env var, + # so an env-configured deployment can't silently shadow the verifier the + # caller wired up. Required in webhook mode; optional in socket mode. signing_secret: str | None = None # Custom webhook verifier. When provided, replaces the built-in HMAC + timestamp # check. See :data:`SlackWebhookVerifier` for the SECURITY contract — the # implementer is responsible for constant-time comparison and replay protection. - # When both ``signing_secret`` and ``webhook_verifier`` are set, ``signing_secret`` - # takes precedence. + # ``webhook_verifier`` takes precedence over ``signing_secret`` and the + # ``SLACK_SIGNING_SECRET`` env var; when it is set, those are ignored. webhook_verifier: SlackWebhookVerifier | None = None # Shared secret for authenticating events forwarded from a separate # socket-mode listener via HTTP POST. Auto-detected from diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index ce25fee..0fbf91e 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -385,7 +385,16 @@ def test_webhook_verifier_alone_is_sufficient(self): if old is not None: os.environ["SLACK_SIGNING_SECRET"] = old - def test_signing_secret_takes_precedence_over_verifier(self): + def test_webhook_verifier_takes_precedence_over_signing_secret(self): + """When both are set, ``webhook_verifier`` wins. + + Port of upstream vercel/chat#468 (commit ``0f0c203``): the original + Python port (PR #87) and upstream both used to prefer + ``signing_secret``; upstream has since reversed itself and this port + follows. Callers wiring up a verifier no longer have it silently + shadowed by a present ``signing_secret`` (or ``SLACK_SIGNING_SECRET`` + env var; see :meth:`test_verifier_opts_out_of_env_signing_secret`). + """ verifier_called: list[int] = [] def verifier(req: Any, body: str) -> bool: @@ -399,8 +408,10 @@ def verifier(req: Any, body: str) -> bool: webhook_verifier=verifier, ) ) - # Internal: webhook_verifier was suppressed because signing_secret won. - assert adapter._webhook_verifier is None + # The verifier wins; the explicit signing_secret is dropped so the + # built-in HMAC path is never taken. + assert adapter._webhook_verifier is verifier + assert adapter._signing_secret is None def test_verifier_opts_out_of_env_signing_secret(self): """An explicit verifier suppresses the SLACK_SIGNING_SECRET env fallback. @@ -791,6 +802,37 @@ def verifier(req: Any, body: str) -> str: assert response["status"] == 200 assert json.loads(response["body"]) == {"challenge": "canonical"} + @pytest.mark.asyncio + async def test_verifier_runs_even_when_signing_secret_is_configured(self): + """``webhook_verifier`` wins over a configured ``signing_secret``. + + Port of upstream vercel/chat#468 — the upstream test was previously + titled "prefers signingSecret over webhookVerifier" and asserted + ``verifier`` was *not* called when both were set. Upstream reversed + that behavior; this Python port mirrors the new direction: with no + signing headers (which would fail the built-in HMAC check), the + verifier still runs and the request succeeds. + """ + verifier_calls: list[tuple[Any, str]] = [] + + def verifier(req: Any, body: str) -> bool: + verifier_calls.append((req, body)) + return True + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token="xoxb-x", + webhook_verifier=verifier, + ) + ) + body = json.dumps({"type": "url_verification", "challenge": "test-challenge"}) + # No signing headers — only the verifier should run. + response = await adapter.handle_webhook(_FakeRequest(body, {"content-type": "application/json"})) + assert response["status"] == 200 + assert json.loads(response["body"]) == {"challenge": "test-challenge"} + assert len(verifier_calls) == 1 + @pytest.mark.asyncio async def test_verifier_path_does_not_invoke_default_signature_check(self): """When verifier is configured, the built-in HMAC + timestamp check is skipped. From e80faeeec559258fbc7b4103c1d1de4e89b4a0da Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 00:56:36 +0000 Subject: [PATCH 2/2] fix(slack): refresh stale construction-cascade comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge review of the verifier-precedence flip caught that the multi-line comment at adapter.py:285-293 still described the pre-flip 'normalize empty-string to None then validate' cascade. After the flip, empty-string signing_secret is rejected outright by an explicit guard below, so the normalize-cascade rationale is dead code documentation. Replace with a 3-line summary pointing at the actual mechanism. Same class of bug PR #87 originally shipped with (stale 'matches upstream' comment) — caught here by the review pattern instead of letting it sit. --- src/chat_sdk/adapters/slack/adapter.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 6d7bfc8..b0b0756 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -282,15 +282,9 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # shadow it (mirrors upstream vercel/chat#468, which reversed the # original direction the Python port shipped in PR #87). # - # Use explicit ``is not None`` checks rather than truthiness fallbacks - # (per docs/UPSTREAM_SYNC.md hazard #1): an explicit empty string for - # ``signing_secret`` should fail validation, not silently fall through - # to ``SLACK_SIGNING_SECRET`` from the environment. *But* an empty - # string is itself unusable downstream (``_verify_signature`` would - # short-circuit with ``if not self._signing_secret`` and reject every - # webhook with 401), so after the cascade we normalize ``""`` to - # ``None`` to surface the misconfiguration here at init rather than - # silently failing on every request. + # Empty-string ``signing_secret`` is rejected outright below; + # empty ``SLACK_SIGNING_SECRET`` env values are normalized to ``None`` + # so they can't masquerade as a configured secret. webhook_verifier = config.webhook_verifier # Reject an explicit empty-string ``signing_secret`` at construction — # even when a ``webhook_verifier`` is set. An explicit ``""`` is a