fix(slack): prefer webhook_verifier over signing_secret (vercel/chat#468)

CHANGELOG.md

@@ -14,6 +14,10 @@ Each substantive commit lands as its own PR (matching the cadence used
 during the 4.27 sync: #83, #85, #86, #87, #88, #89, #90, #91, #92, #99,
 #101, #103, #104, #105). Tracking issue: #98.
 
+### Behavior changes (Slack)
+
+- **`webhook_verifier` now takes precedence over `signing_secret`** (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).
+
 ### Sync scope (37 substantive upstream commits between `f55378a..chat@4.29.0`)
 
 #### Core (`packages/chat`)

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). |

src/chat_sdk/adapters/slack/adapter.py

@@ -276,19 +276,15 @@ 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
-        # ``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
@@ -317,13 +313,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 +381,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 +1276,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)

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

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.