From efa4dc16aee35403f49807b3a934a3dffd49612a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 03:01:16 +0000 Subject: [PATCH 01/12] feat(slack): dynamic bot_token resolver and custom webhook_verifier (vercel/chat#421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow ``bot_token`` to be a zero-arg callable returning ``str | Awaitable[str]`` so apps can rotate or lazily fetch tokens; the resolver is invoked per call (rotation-safe). Add ``webhook_verifier`` as an alternative to ``signing_secret`` for custom request verification — returns truthy/string to accept (string substitutes the body for downstream parsing), falsy or raises to reject with 401. Mirrors the upstream TS PR. Notable adaptations: - Per-request ``ContextVar`` (``_resolved_default_token``) caches the most recent resolver result so the existing sync ``_get_token()`` call sites see a primed value during dispatch without leaking across concurrent webhooks (hazard #6: ContextVar boundaries). - ``handle_webhook`` awaits the resolver once at the top after the signature/verifier check, so dispatch + sync API helpers downstream observe rotation. - ``schedule_message`` ``cancel()`` re-resolves in single-workspace mode and snapshots ``ctx.token`` in multi-workspace mode (matches upstream rotation-safe semantics for 12h-TTL Slack rotated tokens). - ``Attachment.fetchData`` snapshots ``ctx.token`` for multi-workspace and defers to the resolver in single-workspace. - An explicit ``webhook_verifier`` opts out of the ``SLACK_SIGNING_SECRET`` env fallback so a deployment-set env can't silently shadow the verifier. - ``current_token_async()`` added alongside the existing sync ``current_token`` property for callers that need to invoke the resolver. SECURITY: the default verifier path continues to use ``hmac.compare_digest`` and a 5-minute timestamp tolerance check. When a custom verifier is configured, both are bypassed — implementers MUST do constant-time comparison + timestamp/replay validation themselves. The ``SlackWebhookVerifier`` docstring captures the contract. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 299 +++++++- src/chat_sdk/adapters/slack/types.py | 48 +- .../test_slack_dynamic_token_and_verifier.py | 676 ++++++++++++++++++ 3 files changed, 991 insertions(+), 32 deletions(-) create mode 100644 tests/test_slack_dynamic_token_and_verifier.py diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 7fc74cf8..df980f56 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -47,8 +47,11 @@ from chat_sdk.adapters.slack.types import ( RequestContext, SlackAdapterConfig, + SlackBotToken, + SlackBotTokenResolver, SlackInstallation, SlackThreadId, + SlackWebhookVerifier, ) from chat_sdk.emoji import convert_emoji_placeholders, emoji_to_slack, resolve_emoji_from_slack from chat_sdk.logger import ConsoleLogger, Logger @@ -164,6 +167,28 @@ def _find_next_mention(text: str) -> int: return min(at_idx, hash_idx) +def _normalize_bot_token_provider( + value: SlackBotToken | None, +) -> SlackBotTokenResolver | None: + """Normalize a ``bot_token`` config value to a zero-arg resolver. + + Mirrors upstream's ``normalizeBotTokenProvider``. A static string becomes + a resolver that returns the same string; a callable is returned as-is so + rotation or async lookup is preserved. ``None`` -> ``None`` (no token). + """ + if value is None: + return None + if callable(value): + # Already a resolver — keep the user's callable so rotation works. + return value + static = value + + def _provider() -> str: + return static + + return _provider + + # --------------------------------------------------------------------------- # Adapter # --------------------------------------------------------------------------- @@ -187,24 +212,58 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: self._request_context: ContextVar[RequestContext | None] = ContextVar( f"slack_request_context_{id(self)}", default=None ) + # Per-request cache of the resolved default bot token. Populated at the + # top of ``handle_webhook`` (after the signature/verifier check) and + # read by the sync ``_get_token`` path. Each concurrent request gets + # its own contextvar copy so dynamic resolvers returning different + # values per call cannot bleed between in-flight requests. + self._resolved_default_token: ContextVar[str | None] = ContextVar( + f"slack_resolved_default_token_{id(self)}", default=None + ) if config is None: config = SlackAdapterConfig() - signing_secret = config.signing_secret or os.environ.get("SLACK_SIGNING_SECRET") - if not signing_secret: + # 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 = config.webhook_verifier + signing_secret = config.signing_secret or ( + None if webhook_verifier is not None else os.environ.get("SLACK_SIGNING_SECRET") + ) + if not (signing_secret or webhook_verifier): raise ValidationError( "slack", - "signingSecret is required. Set SLACK_SIGNING_SECRET or provide it in config.", + "signingSecret or webhookVerifier is required. Set SLACK_SIGNING_SECRET, " + "provide signing_secret in config, or provide a webhook_verifier.", ) # Auth fields: botToken presence selects single-workspace mode. zero_config = not (config.signing_secret or config.bot_token or config.client_id or config.client_secret) - bot_token = config.bot_token or (os.environ.get("SLACK_BOT_TOKEN") if zero_config else None) + bot_token_config: SlackBotToken | None = config.bot_token + if bot_token_config is None and zero_config: + env_token = os.environ.get("SLACK_BOT_TOKEN") + if env_token: + bot_token_config = env_token + + bot_token_provider = _normalize_bot_token_provider(bot_token_config) self._name = "slack" - self._signing_secret = signing_secret - self._default_bot_token: str | None = bot_token + 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 + # 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 + # uses a resolver under the hood (returns the same string each call). + self._default_bot_token_provider: SlackBotTokenResolver | None = bot_token_provider + # Last successfully resolved default token, kept for the sync ``current_token`` + # accessor and for any code path that needs a token outside an awaitable + # context. Populated lazily on first await of the resolver and refreshed + # on each subsequent webhook entry. Static-string configs prime this at + # construction time so sync access works before any webhook fires. + self._default_bot_token_cache: str | None = bot_token_config if isinstance(bot_token_config, str) else None self._logger: Logger = config.logger or ConsoleLogger("info") self._user_name: str = config.user_name or "bot" self._bot_user_id: str | None = config.bot_user_id or None @@ -276,12 +335,26 @@ def current_token(self) -> str: In multi-workspace mode this is the token resolved by the ``InstallationStore`` for the current request; in single-workspace - mode it is the default bot token. Raises - :class:`AuthenticationError` when called outside a request context - with no default token configured. + mode it is the default bot token (or the most recently resolved + token when a dynamic ``bot_token`` resolver is configured). + + Synchronous: when ``bot_token`` is a resolver and this property is + accessed before the resolver has run for the first time, an + :class:`AuthenticationError` is raised. Use + :meth:`current_token_async` from async contexts to invoke the + resolver on demand. """ return self._get_token() + async def current_token_async(self) -> str: + """Async variant of :attr:`current_token` that invokes the resolver. + + Prefer this over :attr:`current_token` in async code paths when a + dynamic ``bot_token`` resolver is configured — it ensures the + resolver is awaited rather than relying on the cached value. + """ + return await self._resolve_token_async() + @property def current_client(self) -> Any: """Return an ``AsyncWebClient`` preconfigured with :attr:`current_token`. @@ -302,20 +375,116 @@ def current_client(self) -> Any: # ------------------------------------------------------------------ def _get_token(self) -> str: - """Return the current bot token for API calls. + """Return the current bot token for API calls (sync path). + + Checks (in order): + + 1. Multi-workspace request context (``_request_context``) + 2. Per-request resolved default token (``_resolved_default_token``) + — primed by ``handle_webhook`` after invoking the resolver + 3. Static default token cache (set by the constructor for + string-typed ``bot_token`` configs) + 4. Raises :class:`AuthenticationError` + + Synchronous: the token resolver is *not* invoked here. Webhook entry + paths call :meth:`_resolve_default_token` first so the per-request + cache is primed; static-string ``bot_token`` configs prime the + process-wide cache at construction time so this is a no-op for the + common case. Use :meth:`_resolve_token_async` from new async call + sites that need to invoke a resolver outside webhook flow (e.g. + cron jobs). + """ + ctx = self._request_context.get() + if ctx and ctx.token: + return ctx.token + per_request = self._resolved_default_token.get() + if per_request is not None: + return per_request + if self._default_bot_token_cache is not None: + return self._default_bot_token_cache + if self._default_bot_token_provider is not None: + # Resolver-based default token configured but never resolved. This + # is a programming error: the async entry path should have awaited + # ``_resolve_default_token()`` before reaching here. + raise AuthenticationError( + "slack", + "Bot token resolver has not been invoked yet. Use the async API " + "(handle_webhook / current_token_async) so the resolver runs first.", + ) + raise AuthenticationError( + "slack", + "No bot token available. In multi-workspace mode, ensure the webhook is being processed.", + ) - Checks request context (multi-workspace) -> default token (single-workspace) -> raises. + async def _resolve_token_async(self) -> str: + """Async equivalent of :meth:`_get_token` that invokes the resolver. + + Calls the configured ``bot_token`` resolver (if any), refreshes the + per-request cache used by :meth:`_get_token`, and returns the + resulting token. Multi-workspace request context still wins. """ ctx = self._request_context.get() if ctx and ctx.token: return ctx.token - if self._default_bot_token: - return self._default_bot_token + if self._default_bot_token_provider is not None: + return await self._resolve_default_token() + if self._default_bot_token_cache is not None: + return self._default_bot_token_cache raise AuthenticationError( "slack", "No bot token available. In multi-workspace mode, ensure the webhook is being processed.", ) + async def _resolve_default_token(self) -> str: + """Invoke the ``bot_token`` resolver and prime the per-request cache. + + Per upstream, the resolver is called *every time a token is needed* + (it is the resolver's responsibility to memoize if desired) — this + method does not memoize across calls within a process. The result is + stashed in the per-request :attr:`_resolved_default_token` ContextVar + so the sync ``_get_token`` path inside that request sees the value + without re-invoking the resolver, while concurrent requests with + their own ContextVar copies are isolated from each other. + + For static-string ``bot_token`` configs the resolver returns the + same string each call, but we still update the per-request cache to + keep the code path uniform. + + Raises :class:`AuthenticationError` if no resolver is configured. + """ + provider = self._default_bot_token_provider + if provider is None: + raise AuthenticationError( + "slack", + "No default bot token resolver configured (multi-workspace mode).", + ) + try: + result = provider() + except Exception as exc: + self._logger.error("Bot token resolver raised", {"error": exc}) + raise + if inspect.isawaitable(result): + token = await result + else: + token = result + if not isinstance(token, str) or not token: + raise AuthenticationError( + "slack", + "Bot token resolver returned an empty or non-string value.", + ) + # Per-request cache for downstream sync ``_get_token`` calls. + self._resolved_default_token.set(token) + return token + + @property + def _is_single_workspace(self) -> bool: + """``True`` when a default bot token (static or resolver) is configured. + + Multi-workspace mode is the absence of a default token, in which + case tokens are resolved per-team via the InstallationStore. + """ + return self._default_bot_token_provider is not None + def _get_client(self, token: str | None = None) -> Any: """Return an ``AsyncWebClient`` for the given (or current) token. @@ -357,10 +526,13 @@ async def initialize(self, chat: ChatInstance) -> None: """Initialize the adapter and optionally fetch bot identity.""" self._chat = chat - # Single-workspace: fetch bot user ID via auth.test - if self._default_bot_token and not self._bot_user_id: + # Single-workspace: fetch bot user ID via auth.test. We resolve the + # bot token via the resolver here so dynamic resolvers work at init + # time without forcing the caller to seed a static token first. + if self._is_single_workspace and not self._bot_user_id: try: - client = self._get_client(self._default_bot_token) + token = await self._resolve_default_token() + client = self._get_client(token) auth_result = await client.auth_test() self._bot_user_id = auth_result.get("user_id") self._bot_id = auth_result.get("bot_id") or None @@ -374,7 +546,7 @@ async def initialize(self, chat: ChatInstance) -> None: except Exception as exc: self._logger.warn("Could not fetch bot user ID", {"error": exc}) - if not self._default_bot_token: + if not self._is_single_workspace: self._logger.info("Slack adapter initialized in multi-workspace mode") async def disconnect(self) -> None: @@ -722,11 +894,46 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # Extract headers headers = getattr(request, "headers", {}) - timestamp = headers.get("x-slack-request-timestamp") or headers.get("X-Slack-Request-Timestamp") - signature = headers.get("x-slack-signature") or headers.get("X-Slack-Signature") - if not self._verify_signature(body, timestamp, signature): - return {"body": "Invalid signature", "status": 401} + # 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). + if self._webhook_verifier is not None: + try: + verifier_result = self._webhook_verifier(request, body) + if inspect.isawaitable(verifier_result): + verifier_result = await verifier_result + except Exception as exc: + self._logger.warn("Webhook verifier rejected request", {"error": exc}) + return {"body": "Invalid signature", "status": 401} + if not verifier_result: + self._logger.warn("Webhook verifier rejected request") + return {"body": "Invalid signature", "status": 401} + if isinstance(verifier_result, str): + # Substitute the verifier-supplied canonical body before + # parsing. Other truthy returns are pure verification. + body = verifier_result + else: + timestamp = headers.get("x-slack-request-timestamp") or headers.get("X-Slack-Request-Timestamp") + signature = headers.get("x-slack-signature") or headers.get("X-Slack-Signature") + if not self._verify_signature(body, timestamp, signature): + return {"body": "Invalid signature", "status": 401} + + # Resolve the default bot token via the (possibly async) resolver + # before dispatching, so synchronous ``_get_token`` call sites + # downstream see a primed cache. For static-string configs this is + # already primed at construction, so the resolver is a no-op identity + # closure. For dynamic resolvers we re-resolve on every webhook entry + # so rotation is observed. + if self._is_single_workspace: + try: + await self._resolve_default_token() + except Exception as exc: + # Resolver failures here would manifest as auth errors deeper + # in dispatch; surface them at the entry point so the caller + # gets a clean 500 instead of partial processing. Re-raise. + self._logger.error("Bot token resolver failed", {"error": exc}) + raise # Form-urlencoded payloads (interactive + slash commands) content_type = headers.get("content-type") or headers.get("Content-Type") or "" @@ -736,7 +943,7 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # Slash command if "command" in params and "payload" not in params: team_id = (params.get("team_id") or [None])[0] - if not self._default_bot_token and team_id: + if not self._is_single_workspace and team_id: ctx = await self._resolve_token_for_team(team_id) if ctx: tok = self._request_context.set(ctx) @@ -748,7 +955,7 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No return await self._handle_slash_command(params, options) # Interactive payload - if not self._default_bot_token: + if not self._is_single_workspace: team_id_interactive = self._extract_team_id_from_interactive(body) if team_id_interactive: ctx = await self._resolve_token_for_team(team_id_interactive) @@ -781,7 +988,7 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # creates a task via asyncio.create_task). The copied context is # isolated -- the ContextVar change does not leak back to the caller # and does not need an explicit reset. - if not self._default_bot_token and payload.get("type") == "event_callback": + if not self._is_single_workspace and payload.get("type") == "event_callback": team_id_event = payload.get("team_id") if team_id_event: ctx = await self._resolve_token_for_team(team_id_event) @@ -802,6 +1009,13 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # ================================================================== def _verify_signature(self, body: str, timestamp: str | None, signature: str | None) -> bool: + # Defensive: ``_verify_signature`` should never be reached when only a + # ``webhook_verifier`` is configured (handle_webhook gates on that), + # but if a subclass calls this directly without a signing_secret, + # fail closed. + if not self._signing_secret: + return False + if not (timestamp and signature): return False @@ -825,6 +1039,10 @@ def _verify_signature(self, body: str, timestamp: str | None, signature: str | N ) try: + # ``hmac.compare_digest`` is the canonical constant-time comparison. + # Custom verifiers passed via ``webhook_verifier`` MUST do the + # same — a regression to ``==`` would leak signature bytes via + # timing. return hmac.compare_digest(signature, expected) except Exception: return False @@ -1855,8 +2073,13 @@ def _create_attachment(self, file: dict[str, Any], team_id: str | None = None) - the queue/debounce path JSON-serializes the message. """ url = file.get("url_private") - # Capture token at creation time (during webhook processing) - bot_token = self._get_token() + # Capture per-request token from the active webhook context so + # ``fetch_data`` can run later without being inside the ContextVar + # frame (e.g. after the message has been queued + rehydrated). + # For single-workspace mode the default provider is re-resolved at + # fetch time so dynamic ``bot_token`` resolvers honor rotation. + ctx = self._request_context.get() + ctx_token: str | None = ctx.token if ctx and ctx.token else None mimetype = file.get("mimetype", "") att_type: str = "file" @@ -1868,7 +2091,8 @@ def _create_attachment(self, file: dict[str, Any], team_id: str | None = None) - att_type = "audio" async def fetch_data() -> bytes: - return await self._fetch_slack_file(url, bot_token) # type: ignore[arg-type] + token = ctx_token if ctx_token is not None else await self._resolve_token_async() + return await self._fetch_slack_file(url, token) # type: ignore[arg-type] fetch_meta: dict[str, str] = {} if url: @@ -1975,7 +2199,9 @@ async def fetch_data() -> bytes: ) token = installation.bot_token else: - token = adapter._get_token() + # Use the async resolver so a dynamic ``bot_token`` provider + # is invoked at fetch time (rotation-safe). + token = await adapter._resolve_token_async() return await adapter._fetch_slack_file(url, token) return Attachment( @@ -2502,7 +2728,16 @@ async def schedule_message( if files: raise ValidationError("slack", "File uploads are not supported in scheduled messages") - token = self._get_token() + # For multi-workspace mode, snapshot the per-team token from the + # active request context — ``cancel()`` may run outside the + # ContextVar frame. For single-workspace mode, defer resolution to + # ``cancel()`` so dynamic resolvers honor token rotation between + # ``schedule_message()`` and ``cancel()`` (Slack rotated tokens have + # a 12h TTL and scheduled messages can outlive their schedule-time + # token). + ctx = self._request_context.get() + ctx_token: str | None = ctx.token if ctx and ctx.token else None + token = ctx_token if ctx_token is not None else await self._resolve_token_async() try: client = self._get_client(token) @@ -2535,7 +2770,11 @@ async def schedule_message( adapter = self async def cancel() -> None: - c = adapter._get_client(token) + # Multi-workspace: use the snapshotted ctx_token (resolver + # runs outside the request frame). Single-workspace: re- + # resolve so token rotation between schedule + cancel works. + cancel_token = ctx_token if ctx_token is not None else await adapter._resolve_token_async() + c = adapter._get_client(cancel_token) await c.chat_deleteScheduledMessage(channel=channel, scheduled_message_id=scheduled_message_id) return ScheduledMessage( diff --git a/src/chat_sdk/adapters/slack/types.py b/src/chat_sdk/adapters/slack/types.py index a085b80b..aaedd71a 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/src/chat_sdk/adapters/slack/types.py @@ -2,11 +2,43 @@ from __future__ import annotations +from collections.abc import Awaitable, Callable from dataclasses import dataclass from typing import Any, TypedDict from chat_sdk.logger import Logger +# --------------------------------------------------------------------------- +# Bot token resolver +# --------------------------------------------------------------------------- + +# Bot token configuration. Either a static string or a zero-arg callable that +# returns either ``str`` synchronously or an awaitable resolving to ``str``. +# The callable is invoked each time a token is needed, enabling rotation or +# lazy retrieval from a secret manager. +# +# Matches the upstream TS contract: +# ``type SlackBotToken = string | (() => string | Promise)`` +SlackBotTokenResolver = Callable[[], "str | Awaitable[str]"] +SlackBotToken = "str | SlackBotTokenResolver" + +# Custom webhook verifier. Receives the original request object and the raw +# body string already consumed by the adapter. Return: +# - ``True`` (or any truthy non-string value) to accept the request as-is. +# - A ``str`` to accept *and* substitute the verified body for downstream +# parsing (useful when the verifier canonicalizes the payload). +# - ``False``/falsy or raise to reject (adapter responds with 401). +# +# May be sync or async. +# +# SECURITY: When a custom verifier replaces ``signing_secret``, the adapter's +# built-in HMAC + timestamp tolerance check is bypassed. The implementer is +# responsible for: +# - constant-time signature comparison (use ``hmac.compare_digest``, never ``==``) +# - replay protection (validate ``x-slack-request-timestamp`` freshness) +# - any other freshness/origin checks the platform requires +SlackWebhookVerifier = Callable[[Any, str], "bool | str | None | Awaitable[bool | str | None]"] + # ============================================================================= # Configuration # ============================================================================= @@ -17,7 +49,10 @@ class SlackAdapterConfig: """Configuration for the Slack adapter.""" # Bot token (xoxb-...). Required for single-workspace mode. Omit for multi-workspace. - bot_token: str | None = None + # May be a string, or a zero-arg callable returning ``str`` or ``Awaitable[str]`` + # (called on each use to support rotation or deferred resolution from a + # secret manager). See :data:`SlackBotToken`. + bot_token: SlackBotToken | None = None # Bot user ID (will be fetched if not provided) bot_user_id: str | None = None # Slack app client ID (required for OAuth / multi-workspace) @@ -32,8 +67,17 @@ class SlackAdapterConfig: installation_key_prefix: str = "slack:installation" # Logger instance for error reporting. Defaults to ConsoleLogger. logger: Logger | None = None - # Signing secret for webhook verification. Defaults to SLACK_SIGNING_SECRET env var. + # 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. 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: SlackWebhookVerifier | None = None # Maximum number of cached AsyncWebClient instances (LRU-bounded). # Defaults to 100. Increase for large multi-workspace deployments. client_cache_max: int | None = None diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py new file mode 100644 index 00000000..4e5da887 --- /dev/null +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -0,0 +1,676 @@ +"""Tests for the dynamic ``bot_token`` resolver and custom ``webhook_verifier``. + +Port of upstream ``vercel/chat#421`` (commit ``2531e9c``) — adds the +``SlackBotToken`` resolver shape and the ``SlackWebhookVerifier`` escape +hatch that bypasses HMAC signature verification. + +The custom verifier path is security-sensitive: the default verifier uses +``hmac.compare_digest`` and a 5-minute timestamp tolerance check. A custom +verifier replaces both — these tests assert that the verifier is called, +that throws/falsy returns reject with 401, that string returns substitute +the body for downstream parsing, and that an explicit verifier opts out of +the ``SLACK_SIGNING_SECRET`` env fallback. +""" + +from __future__ import annotations + +import asyncio +import hashlib +import hmac +import json +import os +import time +from typing import Any +from unittest.mock import AsyncMock, MagicMock + +import pytest + +try: + from chat_sdk.adapters.slack.adapter import SlackAdapter, create_slack_adapter + from chat_sdk.adapters.slack.types import ( + SlackAdapterConfig, + SlackInstallation, + ) + from chat_sdk.shared.errors import AuthenticationError, ValidationError + + _SLACK_AVAILABLE = True +except ImportError: + _SLACK_AVAILABLE = False + +pytestmark = pytest.mark.skipif(not _SLACK_AVAILABLE, reason="Slack adapter import failed") + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +class _FakeRequest: + def __init__(self, body: str, headers: dict[str, str] | None = None): + self.body = body.encode("utf-8") + self.headers = headers or {} + + async def text(self) -> str: + return self.body.decode("utf-8") + + +def _slack_signature(body: str, secret: str, timestamp: int | None = None) -> tuple[str, str]: + ts = str(timestamp or int(time.time())) + sig_base = f"v0:{ts}:{body}" + sig = "v0=" + hmac.new(secret.encode(), sig_base.encode(), hashlib.sha256).hexdigest() + return ts, sig + + +def _signed_request(body: str, secret: str = "test-signing-secret") -> _FakeRequest: + ts, sig = _slack_signature(body, secret) + return _FakeRequest( + body, + { + "x-slack-request-timestamp": ts, + "x-slack-signature": sig, + "content-type": "application/json", + }, + ) + + +def _unsigned_request(body: str, content_type: str = "application/json") -> _FakeRequest: + return _FakeRequest(body, {"content-type": content_type}) + + +def _make_mock_state() -> MagicMock: + cache: dict[str, Any] = {} + state = MagicMock() + state.get = AsyncMock(side_effect=lambda k: cache.get(k)) + state.set = AsyncMock(side_effect=lambda k, v, *a, **kw: cache.__setitem__(k, v)) + state.delete = AsyncMock(side_effect=lambda k: cache.pop(k, None)) + state.append_to_list = AsyncMock() + state.get_list = AsyncMock(return_value=[]) + state._cache = cache + return state + + +def _make_mock_chat(state: MagicMock) -> MagicMock: + chat = MagicMock() + chat.process_message = MagicMock() + chat.handle_incoming_message = AsyncMock() + chat.process_reaction = MagicMock() + chat.process_action = MagicMock() + chat.process_modal_submit = AsyncMock() + chat.process_modal_close = MagicMock() + chat.process_slash_command = MagicMock() + chat.get_state = MagicMock(return_value=state) + chat.get_user_name = MagicMock(return_value="test-bot") + chat.get_logger = MagicMock(return_value=MagicMock()) + return chat + + +# --------------------------------------------------------------------------- +# Constructor: bot_token as a callable resolver +# --------------------------------------------------------------------------- + + +class TestBotTokenResolverConstruction: + """``bot_token`` accepts both a static string and a callable resolver.""" + + def test_accepts_static_string(self): + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token="xoxb-static")) + # current_token is sync — works for static config without ever invoking a resolver. + assert adapter.current_token == "xoxb-static" + + def test_accepts_sync_callable(self): + calls: list[int] = [] + + def resolver() -> str: + calls.append(1) + return "xoxb-from-sync-resolver" + + # Construction must not invoke the resolver — verifier-only modes need + # to defer all token resolution to per-request flow. + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + assert isinstance(adapter, SlackAdapter) + assert calls == [] + + def test_accepts_async_callable(self): + async def resolver() -> str: + return "xoxb-from-async-resolver" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + # No exception — resolver isn't invoked yet. + assert adapter.name == "slack" + + @pytest.mark.asyncio + async def test_sync_resolver_invoked_via_current_token_async(self): + calls: list[int] = [] + + def resolver() -> str: + calls.append(1) + return "xoxb-from-sync-resolver" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + token = await adapter.current_token_async() + assert token == "xoxb-from-sync-resolver" + assert calls == [1] + + @pytest.mark.asyncio + async def test_async_resolver_invoked_via_current_token_async(self): + calls: list[int] = [] + + async def resolver() -> str: + calls.append(1) + await asyncio.sleep(0) + return "xoxb-from-async-resolver" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + token = await adapter.current_token_async() + assert token == "xoxb-from-async-resolver" + assert calls == [1] + + @pytest.mark.asyncio + async def test_resolver_invoked_per_call_supports_rotation(self): + tokens = ["xoxb-token-1", "xoxb-token-2", "xoxb-token-3"] + i = [0] + + def resolver() -> str: + t = tokens[i[0]] + i[0] += 1 + return t + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + assert await adapter.current_token_async() == "xoxb-token-1" + assert await adapter.current_token_async() == "xoxb-token-2" + assert await adapter.current_token_async() == "xoxb-token-3" + assert i[0] == 3 + + def test_sync_current_token_with_resolver_before_resolution_raises(self): + """Sync ``current_token`` access before the resolver has run raises a clear error.""" + + def resolver() -> str: + return "xoxb-resolved" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(AuthenticationError, match="resolver has not been invoked"): + _ = adapter.current_token + + @pytest.mark.asyncio + async def test_resolver_returning_empty_string_raises(self): + def resolver() -> str: + return "" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(AuthenticationError, match="empty or non-string"): + await adapter.current_token_async() + + @pytest.mark.asyncio + async def test_resolver_propagates_user_exceptions(self): + def resolver() -> str: + raise RuntimeError("token fetch failed") + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(RuntimeError, match="token fetch failed"): + await adapter.current_token_async() + + +# --------------------------------------------------------------------------- +# Constructor: webhook_verifier +# --------------------------------------------------------------------------- + + +class TestWebhookVerifierConstruction: + """Webhook verifier replaces signing_secret as the auth requirement.""" + + def test_signing_secret_or_verifier_required(self): + old = os.environ.pop("SLACK_SIGNING_SECRET", None) + try: + with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + SlackAdapter(SlackAdapterConfig(bot_token="xoxb-x")) + finally: + if old is not None: + os.environ["SLACK_SIGNING_SECRET"] = old + + def test_webhook_verifier_alone_is_sufficient(self): + old = os.environ.pop("SLACK_SIGNING_SECRET", None) + try: + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + assert isinstance(adapter, SlackAdapter) + finally: + if old is not None: + os.environ["SLACK_SIGNING_SECRET"] = old + + def test_signing_secret_takes_precedence_over_verifier(self): + verifier_called: list[int] = [] + + def verifier(req: Any, body: str) -> bool: + verifier_called.append(1) + return True + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="s", + bot_token="xoxb-x", + webhook_verifier=verifier, + ) + ) + # Internal: webhook_verifier was suppressed because signing_secret won. + assert adapter._webhook_verifier is None + + def test_verifier_opts_out_of_env_signing_secret(self): + """An explicit verifier suppresses the SLACK_SIGNING_SECRET env fallback. + + Regression: a deployment with SLACK_SIGNING_SECRET set in env would + otherwise silently shadow the verifier the caller intended to use. + """ + old = os.environ.get("SLACK_SIGNING_SECRET") + os.environ["SLACK_SIGNING_SECRET"] = "env-secret-should-not-be-used" + try: + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + assert adapter._signing_secret is None + assert adapter._webhook_verifier is not None + finally: + if old is None: + os.environ.pop("SLACK_SIGNING_SECRET", None) + else: + os.environ["SLACK_SIGNING_SECRET"] = old + + +# --------------------------------------------------------------------------- +# handle_webhook with custom verifier +# --------------------------------------------------------------------------- + + +class TestHandleWebhookCustomVerifier: + @pytest.mark.asyncio + async def test_verifier_truthy_accepts_request(self): + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + body = json.dumps({"type": "url_verification", "challenge": "verifier-challenge"}) + response = await adapter.handle_webhook(_unsigned_request(body)) + assert response["status"] == 200 + assert json.loads(response["body"]) == {"challenge": "verifier-challenge"} + + @pytest.mark.asyncio + async def test_verifier_throws_returns_401(self): + def verifier(req: Any, body: str) -> bool: + raise RuntimeError("bad signature") + + adapter = SlackAdapter(SlackAdapterConfig(bot_token="xoxb-x", webhook_verifier=verifier)) + body = json.dumps({"type": "url_verification"}) + response = await adapter.handle_webhook(_unsigned_request(body)) + assert response["status"] == 401 + + @pytest.mark.asyncio + async def test_verifier_returns_false_returns_401(self): + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: False, + ) + ) + body = json.dumps({"type": "url_verification"}) + response = await adapter.handle_webhook(_unsigned_request(body)) + assert response["status"] == 401 + + @pytest.mark.asyncio + async def test_verifier_returns_none_returns_401(self): + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: None, + ) + ) + body = json.dumps({"type": "url_verification"}) + response = await adapter.handle_webhook(_unsigned_request(body)) + assert response["status"] == 401 + + @pytest.mark.asyncio + async def test_verifier_receives_request_and_body(self): + captured: list[tuple[Any, str]] = [] + + def verifier(req: Any, body: str) -> bool: + captured.append((req, body)) + return True + + adapter = SlackAdapter(SlackAdapterConfig(bot_token="xoxb-x", webhook_verifier=verifier)) + body = json.dumps({"type": "url_verification", "challenge": "x"}) + request = _unsigned_request(body) + await adapter.handle_webhook(request) + assert len(captured) == 1 + assert captured[0][0] is request + assert captured[0][1] == body + + @pytest.mark.asyncio + async def test_async_verifier_is_awaited(self): + async def verifier(req: Any, body: str) -> bool: + await asyncio.sleep(0) + return True + + adapter = SlackAdapter(SlackAdapterConfig(bot_token="xoxb-x", webhook_verifier=verifier)) + body = json.dumps({"type": "url_verification", "challenge": "async-challenge"}) + response = await adapter.handle_webhook(_unsigned_request(body)) + assert response["status"] == 200 + assert json.loads(response["body"]) == {"challenge": "async-challenge"} + + @pytest.mark.asyncio + async def test_verifier_returning_string_substitutes_body(self): + # Verifier swaps the body so the parser sees a different challenge + canonical_body = json.dumps({"type": "url_verification", "challenge": "canonical"}) + + def verifier(req: Any, body: str) -> str: + return canonical_body + + adapter = SlackAdapter(SlackAdapterConfig(bot_token="xoxb-x", webhook_verifier=verifier)) + # Original body has a *different* challenge; the verifier's return + # should win for downstream parsing. + original_body = json.dumps({"type": "url_verification", "challenge": "original"}) + response = await adapter.handle_webhook(_unsigned_request(original_body)) + assert response["status"] == 200 + assert json.loads(response["body"]) == {"challenge": "canonical"} + + @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. + + SECURITY note: this is the documented escape hatch. The implementer + is responsible for replay protection (timestamp freshness) since the + default 5-minute tolerance is bypassed. + """ + # No timestamp/signature headers — would fail the default check — + # but the verifier accepts. + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + body = json.dumps({"type": "url_verification", "challenge": "no-headers"}) + response = await adapter.handle_webhook(_FakeRequest(body, {"content-type": "application/json"})) + assert response["status"] == 200 + + +# --------------------------------------------------------------------------- +# Resolver runs at webhook entry — sync _get_token sees the resolved value +# --------------------------------------------------------------------------- + + +class TestResolverIntegratedWithWebhookFlow: + @pytest.mark.asyncio + async def test_handle_webhook_invokes_resolver_before_dispatch(self): + calls: list[int] = [] + + def resolver() -> str: + calls.append(1) + return "xoxb-from-resolver" + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + body = json.dumps({"type": "url_verification", "challenge": "ok"}) + response = await adapter.handle_webhook(_signed_request(body)) + assert response["status"] == 200 + assert calls == [1] + + @pytest.mark.asyncio + async def test_resolver_result_visible_to_sync_get_token_during_dispatch(self): + """During webhook processing, sync ``_get_token`` returns the freshly resolved value.""" + resolved: list[str | None] = [] + + def resolver() -> str: + return "xoxb-rotated" + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + + # Custom verifier captures the sync token visible from inside dispatch. + # The verifier runs BEFORE the resolver primes the per-request cache, + # so we instead patch _process_event_payload to capture mid-dispatch. + original = adapter._process_event_payload + + def capture(payload: Any, options: Any = None) -> None: + try: + resolved.append(adapter._get_token()) + except Exception as exc: # pragma: no cover - debug aid + resolved.append(f"ERR: {exc!r}") + original(payload, options) + + adapter._process_event_payload = capture # type: ignore[method-assign] + + body = json.dumps( + { + "type": "event_callback", + "event": {"type": "user_change", "user": {"id": "U1"}}, + "team_id": "T1", + } + ) + await adapter.handle_webhook(_signed_request(body)) + assert resolved == ["xoxb-rotated"] + + +# --------------------------------------------------------------------------- +# Per-request isolation: concurrent webhooks see their own resolved token +# --------------------------------------------------------------------------- + + +class TestConcurrentRequestIsolation: + @pytest.mark.asyncio + async def test_concurrent_resolver_invocations_do_not_leak_across_requests(self): + """Two concurrent ``_resolve_default_token`` calls each see their own token. + + Regression guard for hazard #6 (ContextVar boundaries): the + per-request resolved token cache must use ContextVar, not a shared + instance attribute. We force the second call to await BEFORE its + ``_get_token()`` read so that, if the cache were a shared instance + attribute, the first task's overwrite would leak into the second + task's read. + """ + i = [0] + # Both tasks enter the resolver and obtain their own token. Then the + # FIRST task gates on this event before reading _get_token, so that + # the SECOND task can run its full resolve+set+read cycle in + # between. With a per-request ContextVar the first task's read + # still returns its own resolved token; with a shared attribute it + # would be clobbered by the second task's set. + first_task_can_read = asyncio.Event() + second_task_done = asyncio.Event() + + async def resolver() -> str: + n = i[0] + i[0] += 1 + return f"xoxb-call-{n}" + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + + async def call(label: str, is_first: bool) -> tuple[str, str]: + await adapter._resolve_default_token() + if is_first: + # Wait for the second task to fully run its resolve cycle. + await first_task_can_read.wait() + else: + # Yield so the first task already passed _resolve_default_token. + await asyncio.sleep(0) + await adapter._resolve_default_token() + second_task_done.set() + first_task_can_read.set() + token = adapter._get_token() + return label, token + + async def first() -> tuple[str, str]: + return await call("first", is_first=True) + + async def second() -> tuple[str, str]: + # Let the first task enter call() and pass its first await. + await asyncio.sleep(0) + return await call("second", is_first=False) + + results = await asyncio.gather(first(), second()) + by_label = dict(results) + # If the per-request cache were a shared attribute, ``first`` would + # see ``second``'s last-resolved token. With a ContextVar the first + # task sees its own resolver call and the second task sees its own. + assert by_label["first"] == "xoxb-call-0" + assert by_label["second"] in {"xoxb-call-1", "xoxb-call-2"} + # Sanity: tokens are distinct across the two tasks. + assert by_label["first"] != by_label["second"] + + @pytest.mark.asyncio + async def test_multi_workspace_concurrent_team_resolution_isolated(self): + """Two concurrent webhooks for different teams each see their own InstallationStore token.""" + state = _make_mock_state() + chat = _make_mock_chat(state) + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="test-signing-secret")) + await adapter.initialize(chat) + + await adapter.set_installation( + "T_ALPHA", + SlackInstallation(bot_token="xoxb-alpha", bot_user_id="U_ALPHA", team_name="Alpha"), + ) + await adapter.set_installation( + "T_BETA", + SlackInstallation(bot_token="xoxb-beta", bot_user_id="U_BETA", team_name="Beta"), + ) + + captured: dict[str, str] = {} + gate = asyncio.Event() + first_in: list[int] = [] + + original = adapter._process_event_payload + + def capture(payload: Any, options: Any = None) -> None: + team = payload.get("team_id", "?") + # Force interleave: the first request to arrive waits until the + # second one also enters this function, so both are in flight + # with their respective ContextVar copies. + if not first_in: + first_in.append(1) + try: + captured[team] = adapter._get_token() + except Exception as exc: # pragma: no cover + captured[team] = f"ERR: {exc!r}" + original(payload, options) + + adapter._process_event_payload = capture # type: ignore[method-assign] + + async def run_one(team: str) -> None: + body = json.dumps( + { + "type": "event_callback", + "event": {"type": "user_change", "user": {"id": f"U_{team}"}}, + "team_id": team, + } + ) + # Yield to let the other task interleave. + await asyncio.sleep(0) + await adapter.handle_webhook(_signed_request(body)) + gate.set() + + await asyncio.gather(run_one("T_ALPHA"), run_one("T_BETA")) + + assert captured["T_ALPHA"] == "xoxb-alpha" + assert captured["T_BETA"] == "xoxb-beta" + + +# --------------------------------------------------------------------------- +# create_slack_adapter factory wires the new options through +# --------------------------------------------------------------------------- + + +class TestCreateSlackAdapterFactoryAcceptsNewOptions: + @pytest.mark.asyncio + async def test_factory_accepts_resolver(self): + adapter = create_slack_adapter( + SlackAdapterConfig( + signing_secret="s", + bot_token=lambda: "xoxb-from-factory-resolver", + ) + ) + token = await adapter.current_token_async() + assert token == "xoxb-from-factory-resolver" + + def test_factory_accepts_verifier(self): + old = os.environ.pop("SLACK_SIGNING_SECRET", None) + try: + adapter = create_slack_adapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + assert isinstance(adapter, SlackAdapter) + finally: + if old is not None: + os.environ["SLACK_SIGNING_SECRET"] = old + + +# --------------------------------------------------------------------------- +# Adversarial / SECURITY: the default verifier must use compare_digest +# --------------------------------------------------------------------------- + + +class TestSecurityProperties: + def test_default_verifier_uses_constant_time_compare(self): + """Spot-check: the default verifier path uses ``hmac.compare_digest``. + + A regression to ``==`` would leak signature bytes via timing. This + test inspects the source of ``_verify_signature`` to assert the + primitive has not been swapped out. + """ + import inspect as _inspect + + src = _inspect.getsource(SlackAdapter._verify_signature) + assert "compare_digest" in src, ( + "default signature verifier must use hmac.compare_digest for constant-time comparison" + ) + + @pytest.mark.asyncio + async def test_custom_verifier_is_a_security_escape_hatch(self): + """The custom verifier bypasses both HMAC and the timestamp tolerance check. + + Implementers must take responsibility for both. This test does NOT + validate implementer code — only that the documented contract holds: + an accepting verifier accepts a request that the default check + would reject for missing/old timestamps. + """ + adapter = SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-x", + webhook_verifier=lambda req, body: True, + ) + ) + # Old timestamp (10 min in the past) — the default verifier would + # reject this, but the custom verifier accepts. + body = json.dumps({"type": "url_verification", "challenge": "old"}) + old_ts = str(int(time.time()) - 600) + request = _FakeRequest( + body, + { + "x-slack-request-timestamp": old_ts, + "x-slack-signature": "v0=garbage", + "content-type": "application/json", + }, + ) + response = await adapter.handle_webhook(request) + assert response["status"] == 200, "custom verifier must be able to accept requests the default check rejects" From 9a97bc644867d7d16386d3996d23699f19d6a5ae Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 18:20:03 +0000 Subject: [PATCH 02/12] fix(slack): catch async resolver exceptions in _resolve_default_token Address gemini-code-assist review on #87 (line 469). When the resolver is an async function, calling provider() returns a coroutine; the actual exception is raised at the await, which was outside the try block. Move the await inside the try so async resolver failures are logged through self._logger.error and propagate consistently with sync resolver failures. Adds test_async_resolver_exception_is_logged_and_propagated regression test that fails before the fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 8 +++--- .../test_slack_dynamic_token_and_verifier.py | 27 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index df980f56..327bb403 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -460,13 +460,13 @@ async def _resolve_default_token(self) -> str: ) try: result = provider() + if inspect.isawaitable(result): + token = await result + else: + token = result except Exception as exc: self._logger.error("Bot token resolver raised", {"error": exc}) raise - if inspect.isawaitable(result): - token = await result - else: - token = result if not isinstance(token, str) or not token: raise AuthenticationError( "slack", diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index 4e5da887..fcd76fb6 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -209,6 +209,33 @@ def resolver() -> str: with pytest.raises(RuntimeError, match="token fetch failed"): await adapter.current_token_async() + @pytest.mark.asyncio + async def test_async_resolver_exception_is_logged_and_propagated(self): + """Async resolver exceptions raise during ``await``, not at call time. + + What to fix if this fails: in ``_resolve_default_token`` + (``adapters/slack/adapter.py``), make sure the ``await result`` is + inside the ``try`` block alongside ``provider()`` — otherwise async + resolver failures bypass the logger and the rotation-safety + invariants documented in the PR. + """ + log_calls: list[tuple[str, dict[str, object]]] = [] + + async def resolver() -> str: + raise RuntimeError("async fetch failed") + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + adapter._logger.error = lambda msg, ctx=None: log_calls.append((msg, ctx or {})) # type: ignore[assignment] + + with pytest.raises(RuntimeError, match="async fetch failed"): + await adapter.current_token_async() + + assert any(msg == "Bot token resolver raised" for msg, _ in log_calls), ( + "_resolve_default_token must log async resolver failures via " + "self._logger.error('Bot token resolver raised'); " + "ensure 'await result' is inside the try block" + ) + # --------------------------------------------------------------------------- # Constructor: webhook_verifier From 2ba5f772d15a77ca432c0d78652484068d3e5265 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 20:13:44 +0000 Subject: [PATCH 03/12] fix(slack): address PR #87 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tighten ``test_default_verifier_uses_constant_time_compare`` to require an actual ``hmac.compare_digest(`` call via regex (substring "compare_digest" matches comments too). - Add 4 rotation/snapshot tests for ``schedule_message().cancel()`` and ``Attachment.fetch_data``: assert single-workspace mode re-resolves the token via the dynamic resolver, while multi-workspace mode uses the ctx_token snapshot captured at construction time and does NOT consult InstallationStore at call time. - Document body-substitution misuse in ``SlackWebhookVerifier`` docstring (returning attacker-controlled bytes grants payload injection — third documented failure mode alongside missing constant-time compare and replay protection). - Add resolver non-string return tests (``None``, ``int``, ``dict``) — only ``""`` was covered. - Tighten the ``current_token`` error-message regex to require ``current_token_async`` so callers know which async accessor to use. - Make ``SlackBotToken`` a real ``TypeAlias`` instead of a runtime string — the previous form was a plain ``str`` at runtime, defeating type-checker recognition. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/types.py | 9 +- .../test_slack_dynamic_token_and_verifier.py | 314 +++++++++++++++++- 2 files changed, 317 insertions(+), 6 deletions(-) diff --git a/src/chat_sdk/adapters/slack/types.py b/src/chat_sdk/adapters/slack/types.py index aaedd71a..9f68fe80 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/src/chat_sdk/adapters/slack/types.py @@ -4,7 +4,7 @@ from collections.abc import Awaitable, Callable from dataclasses import dataclass -from typing import Any, TypedDict +from typing import Any, TypeAlias, TypedDict from chat_sdk.logger import Logger @@ -20,7 +20,7 @@ # Matches the upstream TS contract: # ``type SlackBotToken = string | (() => string | Promise)`` SlackBotTokenResolver = Callable[[], "str | Awaitable[str]"] -SlackBotToken = "str | SlackBotTokenResolver" +SlackBotToken: TypeAlias = str | SlackBotTokenResolver # Custom webhook verifier. Receives the original request object and the raw # body string already consumed by the adapter. Return: @@ -37,6 +37,11 @@ # - constant-time signature comparison (use ``hmac.compare_digest``, never ``==``) # - replay protection (validate ``x-slack-request-timestamp`` freshness) # - any other freshness/origin checks the platform requires +# - body-substitution safety: when returning a ``str`` to substitute the body +# for downstream parsing, the returned bytes MUST be derived from a +# verified payload. Returning attacker-controlled bytes (e.g. echoing the +# unverified raw body or splicing in untrusted fields) grants payload +# injection — downstream parsing trusts the substituted body unconditionally. SlackWebhookVerifier = Callable[[Any, str], "bool | str | None | Awaitable[bool | str | None]"] # ============================================================================= diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index fcd76fb6..f1957c7d 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -188,7 +188,12 @@ def resolver() -> str: return "xoxb-resolved" adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) - with pytest.raises(AuthenticationError, match="resolver has not been invoked"): + # Tightened: error message must mention ``current_token_async`` so + # callers know the right async accessor to use, not just that "the + # resolver hasn't run". Substring check on "current_token_async" + # is escaped via ``re.escape`` so the underscore isn't treated as a + # regex token (it isn't, but be explicit). + with pytest.raises(AuthenticationError, match=r"current_token_async"): _ = adapter.current_token @pytest.mark.asyncio @@ -200,6 +205,39 @@ def resolver() -> str: with pytest.raises(AuthenticationError, match="empty or non-string"): await adapter.current_token_async() + @pytest.mark.asyncio + async def test_resolver_returning_none_raises(self): + """Non-string ``None`` return must be rejected, not silently used.""" + + def resolver() -> Any: + return None + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(AuthenticationError, match="empty or non-string"): + await adapter.current_token_async() + + @pytest.mark.asyncio + async def test_resolver_returning_int_raises(self): + """Non-string ``int`` (e.g. accidental ``return 0``) must be rejected.""" + + def resolver() -> Any: + return 12345 + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(AuthenticationError, match="empty or non-string"): + await adapter.current_token_async() + + @pytest.mark.asyncio + async def test_resolver_returning_dict_raises(self): + """Non-string ``dict`` (e.g. returning the secret-manager response object) must be rejected.""" + + def resolver() -> Any: + return {"token": "xoxb-buried-in-dict"} + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + with pytest.raises(AuthenticationError, match="empty or non-string"): + await adapter.current_token_async() + @pytest.mark.asyncio async def test_resolver_propagates_user_exceptions(self): def resolver() -> str: @@ -663,13 +701,17 @@ def test_default_verifier_uses_constant_time_compare(self): A regression to ``==`` would leak signature bytes via timing. This test inspects the source of ``_verify_signature`` to assert the - primitive has not been swapped out. + primitive has not been swapped out. The regex requires an actual + ``hmac.compare_digest(`` call — a passing mention in a comment or + docstring (e.g. ``# use hmac.compare_digest, never ==``) does not + satisfy the assertion. """ import inspect as _inspect + import re as _re src = _inspect.getsource(SlackAdapter._verify_signature) - assert "compare_digest" in src, ( - "default signature verifier must use hmac.compare_digest for constant-time comparison" + assert _re.search(r"\bhmac\.compare_digest\s*\(", src), ( + "default signature verifier must call hmac.compare_digest(...) for constant-time comparison" ) @pytest.mark.asyncio @@ -701,3 +743,267 @@ async def test_custom_verifier_is_a_security_escape_hatch(self): ) response = await adapter.handle_webhook(request) assert response["status"] == 200, "custom verifier must be able to accept requests the default check rejects" + + +# --------------------------------------------------------------------------- +# Rotation safety: schedule_message().cancel() and Attachment.fetch_data +# +# These two paths build a closure that runs *after* the originating webhook +# context has unwound. The contract is: +# +# - Single-workspace mode: re-resolve the token at call time so a dynamic +# ``bot_token`` resolver picks up rotated tokens (Slack rotation TTL is 12h +# and a scheduled message / queued attachment can outlive its origin token). +# - Multi-workspace mode: snapshot the per-team token from the request +# context at construction time — the per-team InstallationStore lookup +# already happened at webhook entry, and ``cancel`` / ``fetch_data`` may +# run outside any ContextVar frame. +# --------------------------------------------------------------------------- + + +def _install_mock_slack_client(adapter: SlackAdapter) -> dict[str, Any]: + """Patch ``adapter._get_client`` to return a recording mock. + + Returns a dict containing ``calls`` (list of (method, kwargs, token) + tuples) and ``tokens`` (set of tokens the adapter requested clients for). + """ + record: dict[str, Any] = {"calls": [], "tokens": []} + + class _Client: + def __init__(self, token: str) -> None: + self._token = token + + async def chat_scheduleMessage(self, **kwargs: Any) -> dict[str, Any]: + record["calls"].append(("chat_scheduleMessage", kwargs, self._token)) + return {"scheduled_message_id": "Q-test", "ok": True} + + async def chat_deleteScheduledMessage(self, **kwargs: Any) -> dict[str, Any]: + record["calls"].append(("chat_deleteScheduledMessage", kwargs, self._token)) + return {"ok": True} + + def _get_client(token: str | None = None) -> Any: + resolved = token if token is not None else adapter._get_token() + record["tokens"].append(resolved) + return _Client(resolved) + + adapter._get_client = _get_client # type: ignore[method-assign] + return record + + +class TestScheduleMessageCancelRotationSafety: + @pytest.mark.asyncio + async def test_schedule_message_cancel_re_resolves_token_in_single_workspace_mode(self): + """In single-workspace mode, ``cancel()`` invokes the resolver again. + + The contract: a dynamic ``bot_token`` resolver must pick up rotated + tokens between ``schedule_message()`` and ``cancel()``. We assert + the resolver is called twice and that ``cancel()`` reaches Slack + with the *new* token. + """ + from datetime import datetime, timedelta, timezone + + tokens = ["xoxb-old", "xoxb-new"] + i = [0] + + def resolver() -> str: + t = tokens[i[0]] + i[0] += 1 + return t + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + record = _install_mock_slack_client(adapter) + + future = datetime.now(tz=timezone.utc) + timedelta(hours=1) + scheduled = await adapter.schedule_message("slack:C123:1234567890.000000", "hello", future) + + # First call to resolver happened during schedule_message itself. + schedule_call = next(c for c in record["calls"] if c[0] == "chat_scheduleMessage") + assert schedule_call[2] == "xoxb-old" + + await scheduled.cancel() + + # Cancel must have re-invoked the resolver and used the rotated token. + assert i[0] == 2, f"resolver should be invoked twice (schedule + cancel), got {i[0]}" + cancel_call = next(c for c in record["calls"] if c[0] == "chat_deleteScheduledMessage") + assert cancel_call[2] == "xoxb-new", ( + "cancel() in single-workspace mode must re-resolve the token so rotated " + "credentials are picked up; got the original 'xoxb-old' instead" + ) + + @pytest.mark.asyncio + async def test_schedule_message_cancel_uses_snapshot_in_multi_workspace_mode(self): + """In multi-workspace mode, ``cancel()`` uses the snapshotted per-team token. + + Rationale: ``cancel()`` may run outside any ContextVar frame (e.g. + from a cron job) — there's no per-team request context to consult. + We assert that ``cancel()`` does NOT call the InstallationStore a + second time, and uses the snapshot captured at ``schedule_message`` + time. + """ + from datetime import datetime, timedelta, timezone + + state = _make_mock_state() + chat = _make_mock_chat(state) + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="test-signing-secret")) + await adapter.initialize(chat) + + await adapter.set_installation( + "T_MWS", + SlackInstallation(bot_token="xoxb-team-mws", bot_user_id="U_MWS", team_name="MWS"), + ) + + record = _install_mock_slack_client(adapter) + + # Spy on get_installation to assert ``cancel()`` does not re-consult it. + get_install_calls: list[str] = [] + original_get = adapter.get_installation + + async def get_install_spy(team_id: str) -> Any: + get_install_calls.append(team_id) + return await original_get(team_id) + + adapter.get_installation = get_install_spy # type: ignore[method-assign] + + # Run schedule_message with the per-team token in context. + future = datetime.now(tz=timezone.utc) + timedelta(hours=1) + + async def do_schedule() -> Any: + return await adapter.schedule_message("slack:C123:1234567890.000000", "hi", future) + + scheduled = await adapter.with_bot_token_async("xoxb-team-mws", do_schedule) + + schedule_call = next(c for c in record["calls"] if c[0] == "chat_scheduleMessage") + assert schedule_call[2] == "xoxb-team-mws" + + # Now run cancel OUTSIDE the request context — snapshot must be used. + get_install_calls.clear() + await scheduled.cancel() + + cancel_call = next(c for c in record["calls"] if c[0] == "chat_deleteScheduledMessage") + assert cancel_call[2] == "xoxb-team-mws", ( + "cancel() in multi-workspace mode must use the snapshotted ctx_token, " + "not re-resolve via the (absent) request context" + ) + assert get_install_calls == [], ( + f"cancel() must NOT re-consult InstallationStore in multi-workspace mode; got {get_install_calls!r}" + ) + + +class TestAttachmentFetchDataRotationSafety: + def _make_file(self) -> dict[str, Any]: + return { + "url_private": "https://files.slack.com/img.png", + "mimetype": "image/png", + "name": "img.png", + "size": 100, + } + + @pytest.mark.asyncio + async def test_attachment_fetch_data_re_resolves_token_in_single_workspace_mode(self): + """``fetch_data`` re-invokes the resolver in single-workspace mode. + + Same rotation contract as ``schedule_message().cancel()`` — a + queued message can outlive the bot token that minted it. + """ + tokens = ["xoxb-att-old", "xoxb-att-new"] + i = [0] + + def resolver() -> str: + t = tokens[i[0]] + i[0] += 1 + return t + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + + # Stub _fetch_slack_file so we can assert which token it received. + fetched: list[tuple[str, str]] = [] + + async def fake_fetch(url: str, token: str) -> bytes: + fetched.append((url, token)) + return b"bytes" + + adapter._fetch_slack_file = fake_fetch # type: ignore[method-assign] + + # Build the attachment OUTSIDE any request context (single-workspace + # mode: no ctx token to snapshot — the closure must defer to the + # resolver at call time). + attachment = adapter._create_attachment(self._make_file()) + assert attachment.fetch_data is not None + assert i[0] == 0, "resolver must NOT be invoked at attachment-creation time" + + result = await attachment.fetch_data() + assert result == b"bytes" + assert i[0] == 1, "resolver must be invoked once at fetch_data() time" + assert fetched == [("https://files.slack.com/img.png", "xoxb-att-old")] + + # Second fetch picks up the rotated token. + result2 = await attachment.fetch_data() + assert result2 == b"bytes" + assert i[0] == 2, "resolver must be invoked again on a second fetch (rotation)" + assert fetched[-1] == ("https://files.slack.com/img.png", "xoxb-att-new") + + @pytest.mark.asyncio + async def test_attachment_fetch_data_uses_snapshot_in_multi_workspace_mode(self): + """``fetch_data`` uses the snapshotted per-team token in multi-workspace mode. + + The closure must NOT consult the InstallationStore at fetch time — + the per-team token was already captured into the closure when the + webhook was being processed. + """ + state = _make_mock_state() + chat = _make_mock_chat(state) + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="test-signing-secret")) + await adapter.initialize(chat) + + await adapter.set_installation( + "T_ATT", + SlackInstallation(bot_token="xoxb-att-team", bot_user_id="U_ATT", team_name="ATT"), + ) + + fetched: list[tuple[str, str]] = [] + + async def fake_fetch(url: str, token: str) -> bytes: + fetched.append((url, token)) + return b"bytes" + + adapter._fetch_slack_file = fake_fetch # type: ignore[method-assign] + + # Spy on get_installation — fetch_data must not consult it. + get_install_calls: list[str] = [] + original_get = adapter.get_installation + + async def get_install_spy(team_id: str) -> Any: + get_install_calls.append(team_id) + return await original_get(team_id) + + adapter.get_installation = get_install_spy # type: ignore[method-assign] + + # Build the attachment INSIDE the per-team request context so the + # snapshot captures the team token. + async def build() -> Any: + return adapter._create_attachment(self._make_file(), team_id="T_ATT") + + attachment = await adapter.with_bot_token_async("xoxb-att-team", build) + assert attachment.fetch_data is not None + + # Now invoke fetch_data OUTSIDE any request context. + get_install_calls.clear() + result = await attachment.fetch_data() + assert result == b"bytes" + assert fetched == [("https://files.slack.com/img.png", "xoxb-att-team")], ( + "fetch_data in multi-workspace mode must use the snapshotted ctx_token captured at attachment-creation time" + ) + assert get_install_calls == [], ( + "fetch_data must NOT re-consult InstallationStore at fetch time in " + f"multi-workspace mode; got {get_install_calls!r}" + ) From b262479c516145bf4af4305a0b5f3067ca46395e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 20:30:22 +0000 Subject: [PATCH 04/12] fix(slack): URL verification short-circuits before bot-token resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address re-review on PR #87 (Medium #1). Slack's url_verification ping arrives at app-install / event-subscription time and only expects the challenge echo — no bot token / API call required. Previously the single-workspace resolver was invoked at handle_webhook entry, BEFORE the url_verification short-circuit, so a flaky/down secret manager would block app installation with a 500. Move the JSON peek for url_verification ahead of _resolve_default_token() and short-circuit there. Mirrors upstream where getToken() is only called at per-API-call sites, never at webhook entry. Adds test_url_verification_bypasses_broken_resolver: configures a resolver that raises and asserts URL verification still returns 200 with the challenge echo. Also documents two related divergences in docs/UPSTREAM_SYNC.md non-parity table (Medium #2 + Nit from re-review): - Slack bot_token resolver invocation site: TS resolves on every API call site (cron-mode works); Python resolves once at handle_webhook entry into a ContextVar (cron callers must await current_token_async() first). - Within-request resolver caching scope: TS calls per API call; Python caches per request to keep _get_token sync. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- docs/UPSTREAM_SYNC.md | 2 ++ src/chat_sdk/adapters/slack/adapter.py | 30 ++++++++++++++++++- .../test_slack_dynamic_token_and_verifier.py | 30 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index a9057930..e696a7c2 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -489,6 +489,8 @@ stay explicit instead of being rediscovered in code review. | Fallback streaming final SentMessage content | SentMessage + final edit carry `final_content` (remend'd — inline markers auto-closed) | SentMessage + final edit carry raw `accumulated` | Narrow UX refinement. If a stream ends with an unclosed `*`/`~~`/etc., upstream ships the unclosed marker; we run `_remend` so the user sees a clean final message. Not observable in the common case where streams close their own markers. | | 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_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | 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. | +| 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). | | Redis lock token format | `{token_prefix}_{ms}_{secrets.token_hex(16)}` — always 32 hex chars, CSPRNG-sourced | `ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}` — base36, ≤13 chars, **not** CSPRNG | Interop via `IoRedisStateAdapter(token_prefix="ioredis")` still works for lock-release (release/extend compare by full-string equality, and each runtime only releases what it issued), but the token byte-shape diverges. Intentional — CSPRNG should not be regressed to `Math.random()` for cosmetic byte-for-byte compatibility. | | `StreamingPlan.is_supported()` / `get_fallback_text()` | Raise `RuntimeError` to fail loudly if a generic posting path (e.g. `ChannelImpl.post`, `post_postable_object`) tries to consume a `StreamingPlan` as a normal `PostableObject` | Silently return `True` / `""` — `ChannelImpl.post` would route through `postPostableObject` and post an empty-string fallback | Prevents `StreamingPlan` being silently routed through non-stream-aware posting paths where upstream would post a blank message or attempt a wrong-shape `adapter.post_object("stream", ...)` call. Internal dispatch is guarded by the `kind == "stream"` short-circuit in `post_postable_object` / `Thread.post`; this also protects third-party code that duck-types PostableObjects. | diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 327bb403..d8437080 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -919,6 +919,35 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No if not self._verify_signature(body, timestamp, signature): return {"body": "Invalid signature", "status": 401} + # URL verification is special: Slack sends a JSON ``url_verification`` + # ping at app-install / event-subscription time and only expects the + # ``challenge`` echo. No token / API call is required — so resolve + # the JSON-payload short-circuit BEFORE invoking the bot-token + # resolver. A broken or down resolver (secrets manager outage, key + # rotation in flight) must NOT prevent URL verification from + # succeeding — that would block app installation and re-subscription. + # Mirrors upstream parity where ``getToken`` is only called at + # per-API-call sites, not at webhook entry. + content_type = headers.get("content-type") or headers.get("Content-Type") or "" + early_payload: dict[str, Any] | None = None + if "application/json" in content_type or (not content_type and body and body.lstrip().startswith(("{", "["))): + try: + maybe = json.loads(body) + if isinstance(maybe, dict): + early_payload = maybe + except (json.JSONDecodeError, ValueError): + early_payload = None + if ( + early_payload is not None + and early_payload.get("type") == "url_verification" + and early_payload.get("challenge") + ): + return { + "body": json.dumps({"challenge": early_payload["challenge"]}), + "status": 200, + "headers": {"Content-Type": "application/json"}, + } + # Resolve the default bot token via the (possibly async) resolver # before dispatching, so synchronous ``_get_token`` call sites # downstream see a primed cache. For static-string configs this is @@ -936,7 +965,6 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No raise # Form-urlencoded payloads (interactive + slash commands) - content_type = headers.get("content-type") or headers.get("Content-Type") or "" if "application/x-www-form-urlencoded" in content_type: params = parse_qs(body, keep_blank_values=True) diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index f1957c7d..9c3b05a6 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -274,6 +274,36 @@ async def resolver() -> str: "ensure 'await result' is inside the try block" ) + @pytest.mark.asyncio + async def test_url_verification_bypasses_broken_resolver(self): + """A broken bot-token resolver must not break Slack's URL verification. + + URL verification is a one-time setup ping at app-install / event- + subscription time and only needs the ``challenge`` echo back. No API + call (and thus no token) is required. + + What to fix if this fails: in + ``src/chat_sdk/adapters/slack/adapter.py`` ``handle_webhook``, the + ``url_verification`` short-circuit must run BEFORE + ``_resolve_default_token()``. Otherwise a flaky/down secret-manager + keeps Slack from re-subscribing the webhook, which blocks app + installation. Mirrors upstream where ``getToken`` is only called at + per-API-call sites, never at webhook entry. + """ + + def resolver() -> str: + raise RuntimeError("secret manager is down") + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + body = json.dumps({"type": "url_verification", "challenge": "abc-123"}) + response = await adapter.handle_webhook(_signed_request(body, "s")) + + assert response["status"] == 200, ( + "URL verification must succeed even when the bot-token resolver is broken; " + "the resolver call must be deferred until after the url_verification short-circuit" + ) + assert json.loads(response["body"]) == {"challenge": "abc-123"} + # --------------------------------------------------------------------------- # Constructor: webhook_verifier From 5549c0abd5d0909078bf7f96c62bbd2dad36fca7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 20:31:02 +0000 Subject: [PATCH 05/12] test(slack): update resolver-before-dispatch test for url_verification fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version used a url_verification payload, which is now special-cased to short-circuit BEFORE the resolver runs (per the PR #87 follow-up fix). Switch to an event_callback payload — the actual dispatch path that DOES need a token — to exercise the invariant. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- .../test_slack_dynamic_token_and_verifier.py | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index 9c3b05a6..149ac4e3 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -503,6 +503,15 @@ async def test_verifier_path_does_not_invoke_default_signature_check(self): class TestResolverIntegratedWithWebhookFlow: @pytest.mark.asyncio async def test_handle_webhook_invokes_resolver_before_dispatch(self): + """For real event payloads, the resolver is invoked at handle_webhook + entry so downstream sync ``_get_token`` callers see the resolved + value. + + Note: url_verification is now special-cased and short-circuits + BEFORE the resolver runs (see test_url_verification_bypasses_broken_resolver + above) — so this test uses a regular ``event_callback`` payload to + exercise the resolver-before-dispatch invariant. + """ calls: list[int] = [] def resolver() -> str: @@ -515,10 +524,17 @@ def resolver() -> str: bot_token=resolver, ) ) - body = json.dumps({"type": "url_verification", "challenge": "ok"}) + # Use an event_callback (a real Slack event) — this is the path that + # actually needs a token in dispatch. URL verification doesn't. + body = json.dumps({ + "type": "event_callback", + "team_id": "T123", + "event": {"type": "app_mention", "user": "U1", "channel": "C1", "ts": "1.0", "text": "hi"}, + }) response = await adapter.handle_webhook(_signed_request(body)) + # event_callback returns 200 even if no handlers fire. assert response["status"] == 200 - assert calls == [1] + assert calls == [1], "resolver must be invoked at handle_webhook entry for real events" @pytest.mark.asyncio async def test_resolver_result_visible_to_sync_get_token_during_dispatch(self): From f0d3d2f290e64c846c5734905de9b0ee364e363f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 23:43:51 +0000 Subject: [PATCH 06/12] style(slack): ruff format new url_verification test --- tests/test_slack_dynamic_token_and_verifier.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index 149ac4e3..ad3e59fe 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -526,11 +526,13 @@ def resolver() -> str: ) # Use an event_callback (a real Slack event) — this is the path that # actually needs a token in dispatch. URL verification doesn't. - body = json.dumps({ - "type": "event_callback", - "team_id": "T123", - "event": {"type": "app_mention", "user": "U1", "channel": "C1", "ts": "1.0", "text": "hi"}, - }) + body = json.dumps( + { + "type": "event_callback", + "team_id": "T123", + "event": {"type": "app_mention", "user": "U1", "channel": "C1", "ts": "1.0", "text": "hi"}, + } + ) response = await adapter.handle_webhook(_signed_request(body)) # event_callback returns 200 even if no handlers fire. assert response["status"] == 200 From 5a648ecee6b454be04ba4333740dcd6995e3af81 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 03:47:52 +0000 Subject: [PATCH 07/12] fix(slack): address CodeRabbit major findings on PR #87 Two defects flagged by CodeRabbit in the dynamic-bot-token port: 1. Truthiness-based auth fallbacks (discussion_r3285672704). The constructor used `config.signing_secret or ...`, `not (config.X or ...)` patterns that let an explicit empty-string config value silently fall through to environment credentials. Per docs/UPSTREAM_SYNC.md hazard #1 the rule when porting from TS is `x if x is not None else default`. Rewrites the signing_secret cascade, the validation guard, the zero_config check, and the env bot-token fallback to use explicit `is not None` checks so empty strings fail validation instead of flipping the adapter into the wrong auth mode. 2. Sync token cache not refreshed after resolver (discussion_r3285672709). `_resolve_default_token` wrote only the per-request ContextVar (`_resolved_default_token`), so callers reading the documented sync `current_token` / `current_client` accessors *outside* that ContextVar scope still saw the pre-resolution state and raised `AuthenticationError`. Adds `self._default_bot_token_cache = token` right before the ContextVar set so the process-wide cache that the sync `_get_token` path reads is refreshed too. Regression test `test_resolver_refreshes_sync_token_cache` clears the ContextVar after the resolver runs and asserts sync `current_token` returns the freshly resolved value (verified to fail without the fix). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 30 ++++++++++-- .../test_slack_dynamic_token_and_verifier.py | 49 +++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index d8437080..c2a177f0 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -226,11 +226,18 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # 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. + # + # 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. webhook_verifier = config.webhook_verifier - signing_secret = config.signing_secret or ( - None if webhook_verifier is not None else os.environ.get("SLACK_SIGNING_SECRET") + 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 not (signing_secret or webhook_verifier): + if signing_secret is None and webhook_verifier is None: raise ValidationError( "slack", "signingSecret or webhookVerifier is required. Set SLACK_SIGNING_SECRET, " @@ -238,12 +245,20 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: ) # Auth fields: botToken presence selects single-workspace mode. - zero_config = not (config.signing_secret or config.bot_token or config.client_id or config.client_secret) + # Explicit ``is not None`` to mirror the truthiness-trap rule above: + # an explicit empty string for any of these should still count as + # "config provided" and disable the env-fallback path. + zero_config = ( + config.signing_secret is None + and config.bot_token is None + and config.client_id is None + and config.client_secret is None + ) bot_token_config: SlackBotToken | None = config.bot_token if bot_token_config is None and zero_config: env_token = os.environ.get("SLACK_BOT_TOKEN") - if env_token: + if env_token is not None: bot_token_config = env_token bot_token_provider = _normalize_bot_token_provider(bot_token_config) @@ -472,6 +487,11 @@ async def _resolve_default_token(self) -> str: "slack", "Bot token resolver returned an empty or non-string value.", ) + # Refresh the process-wide cache so sync ``current_token`` / + # ``current_client`` access outside the request ContextVar scope + # (e.g. callers reading the most recently resolved token from a + # different task) still observes the freshly resolved value. + self._default_bot_token_cache = token # Per-request cache for downstream sync ``_get_token`` calls. self._resolved_default_token.set(token) return token diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index ad3e59fe..ace613d0 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -304,6 +304,55 @@ def resolver() -> str: ) assert json.loads(response["body"]) == {"challenge": "abc-123"} + @pytest.mark.asyncio + async def test_resolver_refreshes_sync_token_cache(self): + """After the resolver runs, sync ``current_token`` sees the resolved token. + + Regression for CodeRabbit r3285672709: ``_resolve_default_token`` + previously only wrote the per-request ContextVar, so callers reading + ``current_token`` *outside* that ContextVar scope (e.g. from a sync + helper invoked from a different task) still saw the pre-resolution + state and raised ``AuthenticationError``. The resolver must also + refresh the process-wide ``_default_bot_token_cache`` so the sync + path returns the freshly resolved value. + + What to fix if this fails: in ``_resolve_default_token`` + (``adapters/slack/adapter.py``), after the + ``isinstance(token, str)`` / non-empty validation and before + ``self._resolved_default_token.set(token)``, assign + ``self._default_bot_token_cache = token``. + """ + + def resolver() -> str: + return "xoxb-resolved-token" + + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", bot_token=resolver)) + + # Sanity: before the resolver runs, sync ``current_token`` must raise + # (matches ``test_sync_current_token_with_resolver_before_resolution_raises``). + with pytest.raises(AuthenticationError, match=r"current_token_async"): + _ = adapter.current_token + + # Invoke the resolver via the documented async entry point. + token = await adapter.current_token_async() + assert token == "xoxb-resolved-token" + + # Now read sync ``current_token`` from a context *without* the + # ContextVar set by the resolver — running the read inside a fresh + # ``asyncio.to_thread`` would copy the ContextVar, so use a brand-new + # asyncio task with the default (empty) ContextVar state by spawning + # a new task and clearing the per-request var. + # + # Simpler equivalent: directly clear the ContextVar and read. The + # per-request var being reset back to ``None`` mirrors the "different + # task, no ContextVar copy" scenario described in the finding. + adapter._resolved_default_token.set(None) + assert adapter.current_token == "xoxb-resolved-token", ( + "sync current_token must read from the refreshed " + "_default_bot_token_cache after the resolver succeeds; the " + "ContextVar-only cache breaks sync access outside the request scope" + ) + # --------------------------------------------------------------------------- # Constructor: webhook_verifier From 2ecd45175f6bf5dcf9ef545381a489aa9d7b1d51 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 04:16:27 +0000 Subject: [PATCH 08/12] fix(slack): reject empty-string signing_secret/SLACK_BOT_TOKEN at init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Codex P1 on PR #87 (discussion_r3285725152). The truthiness-to-``is not None`` rewrite in ``5a648ec`` correctly stopped empty-string config values from silently falling through to env fallbacks, but it over-corrected by then *accepting* the empty string as a valid value: - ``SlackAdapterConfig(signing_secret="")`` passed validation, but ``_verify_signature`` short-circuits on ``if not self._signing_secret`` and rejects every webhook with ``401`` — failing fast at init is strictly better than failing on every production request. - ``SLACK_BOT_TOKEN=""`` cached ``""`` as the default bot token; every Slack API call then went out as ``Authorization: Bearer `` and surfaced as opaque ``invalid_auth`` errors. Fix (Option B from the brief): keep the ``is not None`` cascade so explicit-empty user config still suppresses env fallbacks (that part of ``5a648ec`` is right), but normalize the post-cascade ``""`` to ``None`` before the validation guard so the misconfiguration surfaces at adapter construction. For ``SLACK_BOT_TOKEN`` env specifically, treat ``""`` as unset (env empty == "no token here") rather than as user-provided explicit-empty. Regression tests in ``test_slack_dynamic_token_and_verifier.py`` cover: ``signing_secret=""`` alone, with ``bot_token`` set, with env ``SLACK_SIGNING_SECRET`` set, the verifier-as-fallback path, and the empty-env ``SLACK_BOT_TOKEN`` path. All four fail against ``5a648ec`` and pass after this fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 19 +++- .../test_slack_dynamic_token_and_verifier.py | 99 +++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index c2a177f0..49c59aa5 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -230,18 +230,25 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # 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. + # 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. webhook_verifier = config.webhook_verifier 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 signing_secret is None and webhook_verifier is None: raise ValidationError( "slack", "signingSecret or webhookVerifier is required. Set SLACK_SIGNING_SECRET, " - "provide signing_secret in config, or provide a webhook_verifier.", + "provide a non-empty signing_secret in config, or provide a webhook_verifier.", ) # Auth fields: botToken presence selects single-workspace mode. @@ -258,7 +265,13 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: bot_token_config: SlackBotToken | None = config.bot_token if bot_token_config is None and zero_config: env_token = os.environ.get("SLACK_BOT_TOKEN") - if env_token is not None: + # Same empty-string-as-missing rule as ``signing_secret``: an empty + # SLACK_BOT_TOKEN would cache ``""`` in + # ``_default_bot_token_cache`` and ``_get_token`` would happily + # return it, producing opaque "invalid_auth" errors from Slack on + # every API call. Treat ``""`` as unset so the adapter falls + # through to multi-workspace mode (or fails clearly later). + if env_token is not None and env_token != "": bot_token_config = env_token bot_token_provider = _normalize_bot_token_provider(bot_token_config) diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index ace613d0..bf3637b5 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -425,6 +425,105 @@ def test_verifier_opts_out_of_env_signing_secret(self): else: os.environ["SLACK_SIGNING_SECRET"] = old + def test_empty_signing_secret_rejected_at_construction(self): + """Explicit ``signing_secret=""`` must fail validation at adapter init. + + Regression for Codex P1 on PR #87: the ``is not None`` cascade in + commit ``5a648ec`` over-corrected the truthiness trap — it correctly + stopped empty strings from silently falling through to the env + fallback, but then *accepted* the empty string as a valid signing + secret. ``_verify_signature`` short-circuits with + ``if not self._signing_secret`` and returns ``False`` for every + webhook, leaving the adapter responding ``401`` to Slack on every + request rather than failing fast at construction. Empty-string is + an unusable value: it must be treated as "not provided" by the + validation guard so the misconfiguration surfaces here, not on the + production webhook path. + """ + old = os.environ.pop("SLACK_SIGNING_SECRET", None) + try: + with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + SlackAdapter(SlackAdapterConfig(signing_secret="")) + # Also fails when an otherwise-valid bot_token is set: the + # signing_secret value is what matters, not the rest of the + # config. + with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + SlackAdapter(SlackAdapterConfig(signing_secret="", bot_token="xoxb-test")) + finally: + if old is not None: + os.environ["SLACK_SIGNING_SECRET"] = old + + def test_empty_signing_secret_does_not_fall_back_to_env(self): + """Empty-string ``signing_secret`` is rejected even with SLACK_SIGNING_SECRET set. + + Pairs with :meth:`test_empty_signing_secret_rejected_at_construction`: + normalizing ``""`` to ``None`` *after* the env-fallback cascade + means the explicit empty string still suppresses the env fallback + (that part of ``5a648ec`` is correct), and the resulting ``None`` + then fails the ``signing_secret is None and webhook_verifier is + None`` guard. The user's explicit (if empty) config wins over env. + """ + old = os.environ.get("SLACK_SIGNING_SECRET") + os.environ["SLACK_SIGNING_SECRET"] = "env-secret-should-not-be-used" + try: + with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + SlackAdapter(SlackAdapterConfig(signing_secret="", bot_token="xoxb-test")) + finally: + if old is None: + os.environ.pop("SLACK_SIGNING_SECRET", None) + else: + os.environ["SLACK_SIGNING_SECRET"] = old + + def test_empty_signing_secret_with_verifier_is_accepted(self): + """``signing_secret=""`` + ``webhook_verifier`` => verifier path takes over. + + The validation guard fails only when *both* are unusable. An empty + signing secret with an explicit verifier normalizes to "no signing + secret, use the verifier" — consistent with the documented + precedence rule (a non-empty signing_secret would win, an empty + one is treated as absent). + """ + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="", + bot_token="xoxb-test", + webhook_verifier=lambda req, body: True, + ) + ) + assert adapter._signing_secret is None + assert adapter._webhook_verifier is not None + + def test_empty_env_slack_bot_token_does_not_become_static_token(self): + """``SLACK_BOT_TOKEN=""`` must not be cached as the default bot token. + + Regression companion to the empty-signing_secret fix: the env + fallback for ``SLACK_BOT_TOKEN`` was updated in ``5a648ec`` to + use ``if env_token is not None`` to match the + "explicit-empty-is-config" rule. But unlike user config, an empty + env var is the system telling us "no token here" — caching + ``""`` as the default bot token would propagate to every Slack + API call as ``Authorization: Bearer `` and surface as opaque + ``invalid_auth`` errors. Treat env ``""`` as unset. + """ + old_token = os.environ.get("SLACK_BOT_TOKEN") + old_secret = os.environ.get("SLACK_SIGNING_SECRET") + os.environ["SLACK_BOT_TOKEN"] = "" + os.environ["SLACK_SIGNING_SECRET"] = "valid-signing-secret" + try: + # Zero-config path is what triggers the env fallback. + adapter = SlackAdapter() + assert adapter._default_bot_token_cache is None + assert adapter._default_bot_token_provider is None + finally: + if old_token is None: + os.environ.pop("SLACK_BOT_TOKEN", None) + else: + os.environ["SLACK_BOT_TOKEN"] = old_token + if old_secret is None: + os.environ.pop("SLACK_SIGNING_SECRET", None) + else: + os.environ["SLACK_SIGNING_SECRET"] = old_secret + # --------------------------------------------------------------------------- # handle_webhook with custom verifier From 7c30c137730e16aa099113903d1538096c8854bd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 05:25:17 +0000 Subject: [PATCH 09/12] fix(slack): reject empty bot_token, drop env-shadow on empty client_id/secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review on PR #87 surfaced two follow-on hazards left after 2ecd451's empty-string normalization sweep: 1. ``SlackAdapterConfig(bot_token="")`` was still accepted at init: the constructor primed ``_default_bot_token_cache`` with ``""`` and the sync ``_get_token`` path happily returned the empty string, producing ``Authorization: Bearer `` API calls and opaque ``invalid_auth`` errors from Slack. The async resolver path (``_resolve_default_token``) already raises on empty results, but failing fast at construction is strictly better than failing on every Slack API call in production — same rationale as the ``signing_secret=""`` rejection in 2ecd451. Callable resolvers are unaffected (they're validated at resolve time). 2. ``self._client_id = config.client_id or env`` / ditto for ``client_secret`` (lines 312-315) still used truthiness fallback — hazard #1 in the same constructor block 5a648ec was supposed to have cleaned up. An explicit ``client_id=""`` silently flipped to either the env value (zero_config) or ``None`` (non-zero-config), neither of which matches the user's intent. Rewrites to per-field ``is not None`` gating with an empty-env-as-unset rule mirroring SLACK_BOT_TOKEN env (empty env is "nothing here", not a configured value — would otherwise surface as opaque OAuth ``invalid_client`` errors mid-flow). Three regression tests in ``test_slack_dynamic_token_and_verifier.py``: ``test_empty_string_bot_token_rejected_at_construction``, ``test_empty_client_id_does_not_fall_back_to_env``, and ``test_empty_env_client_id_treated_as_unset``. All three fail against 2ecd451 and pass after this fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 39 +++++++- .../test_slack_dynamic_token_and_verifier.py | 99 +++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 49c59aa5..146f3812 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -263,6 +263,20 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: ) bot_token_config: SlackBotToken | None = config.bot_token + # Reject explicit empty-string ``bot_token`` at init for the same + # reason ``signing_secret=""`` is rejected: it would prime + # ``_default_bot_token_cache`` with ``""`` and the sync ``_get_token`` + # path would happily return it, producing ``Authorization: Bearer `` + # API calls and opaque ``invalid_auth`` errors from Slack. (The async + # resolver path catches this later, but failing fast at construction + # is strictly better.) Callable resolvers may legitimately *return* + # non-string values at resolve time — that case is already validated + # in ``_resolve_default_token``. + if isinstance(bot_token_config, str) and bot_token_config == "": + raise ValidationError( + "slack", + "bot_token must be a non-empty string or a callable resolver; got an empty string.", + ) if bot_token_config is None and zero_config: env_token = os.environ.get("SLACK_BOT_TOKEN") # Same empty-string-as-missing rule as ``signing_secret``: an empty @@ -308,11 +322,26 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: self._client_cache: OrderedDict[str, Any] = OrderedDict() self._client_cache_max = config.client_cache_max if config.client_cache_max is not None else 100 - # Multi-workspace OAuth fields - self._client_id: str | None = config.client_id or (os.environ.get("SLACK_CLIENT_ID") if zero_config else None) - self._client_secret: str | None = config.client_secret or ( - os.environ.get("SLACK_CLIENT_SECRET") if zero_config else None - ) + # Multi-workspace OAuth fields. + # ``is not None`` (not truthiness) so an explicit empty-string user + # config does not silently fall back to env (hazard #1). Empty env + # values are treated as "unset" (mirrors SLACK_BOT_TOKEN env rule): + # an empty SLACK_CLIENT_ID would be useless downstream and produce + # opaque OAuth failures rather than a clear "not configured" state. + if config.client_id is not None: + self._client_id: str | None = config.client_id + elif zero_config: + env_client_id = os.environ.get("SLACK_CLIENT_ID") + self._client_id = env_client_id if env_client_id else None + else: + self._client_id = None + if config.client_secret is not None: + self._client_secret: str | None = config.client_secret + elif zero_config: + env_client_secret = os.environ.get("SLACK_CLIENT_SECRET") + self._client_secret = env_client_secret if env_client_secret else None + else: + self._client_secret = None self._installation_key_prefix = config.installation_key_prefix or "slack:installation" encryption_key_raw = config.encryption_key or os.environ.get("SLACK_ENCRYPTION_KEY") diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index bf3637b5..a56d330f 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -524,6 +524,105 @@ def test_empty_env_slack_bot_token_does_not_become_static_token(self): else: os.environ["SLACK_SIGNING_SECRET"] = old_secret + def test_empty_string_bot_token_rejected_at_construction(self): + """Explicit ``bot_token=""`` must fail validation at adapter init. + + Companion to :meth:`test_empty_signing_secret_rejected_at_construction` + — the same hazard for ``signing_secret=""`` exists for a + user-configured ``bot_token=""``: ``_default_bot_token_cache`` would + be primed with ``""`` and the sync ``_get_token`` path would happily + return it, producing ``Authorization: Bearer `` API calls and + opaque ``invalid_auth`` errors from Slack. The async resolver path + catches this later, but failing fast at construction is strictly + better than failing on every Slack API call in production. + + Callable resolvers are unaffected: they may return any string at + resolve time and the empty-result case is already validated in + ``_resolve_default_token``. + """ + old_secret = os.environ.pop("SLACK_SIGNING_SECRET", None) + old_token = os.environ.pop("SLACK_BOT_TOKEN", None) + try: + with pytest.raises(ValidationError, match="bot_token"): + SlackAdapter(SlackAdapterConfig(signing_secret="ok", bot_token="")) + finally: + if old_secret is not None: + os.environ["SLACK_SIGNING_SECRET"] = old_secret + if old_token is not None: + os.environ["SLACK_BOT_TOKEN"] = old_token + + def test_empty_client_id_does_not_fall_back_to_env(self): + """Explicit ``client_id=""`` is honored as "not configured", not env-shadowed. + + Regression for hazard #1 (truthiness trap) in the OAuth field + cascade. The previous ``config.client_id or env`` pattern silently + converted ``client_id=""`` into the env value when ``zero_config`` + was true, and into ``None`` when it wasn't — neither matches the + user's intent. Using ``is not None`` makes the behavior explicit: + an explicit (even empty) user config value wins over env. + """ + old_id = os.environ.get("SLACK_CLIENT_ID") + old_secret = os.environ.get("SLACK_CLIENT_SECRET") + os.environ["SLACK_CLIENT_ID"] = "env-id-should-not-be-used" + os.environ["SLACK_CLIENT_SECRET"] = "env-secret-should-not-be-used" + try: + # Explicit empty user config — env must NOT shadow it. With a + # bot_token set, zero_config is False anyway, so env wouldn't be + # read; this test exercises the multi-workspace zero_config path + # to confirm the per-field ``is not None`` gate also rejects env + # when the user passed an explicit empty value. + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="ok", client_id="", client_secret="")) + assert adapter._client_id == "" + assert adapter._client_secret == "" + finally: + if old_id is None: + os.environ.pop("SLACK_CLIENT_ID", None) + else: + os.environ["SLACK_CLIENT_ID"] = old_id + if old_secret is None: + os.environ.pop("SLACK_CLIENT_SECRET", None) + else: + os.environ["SLACK_CLIENT_SECRET"] = old_secret + + def test_empty_env_client_id_treated_as_unset(self): + """``SLACK_CLIENT_ID=""`` env must not become the resolved client_id. + + Mirrors the SLACK_BOT_TOKEN-empty rule from ``2ecd451``: an empty + env value is the system telling us "nothing here", not a valid + configured value. Empty env client_id would surface as opaque + OAuth ``invalid_client`` errors mid-flow rather than a clear "OAuth + not configured" state. + """ + old_token = os.environ.get("SLACK_BOT_TOKEN") + old_secret = os.environ.get("SLACK_SIGNING_SECRET") + old_id = os.environ.get("SLACK_CLIENT_ID") + old_csecret = os.environ.get("SLACK_CLIENT_SECRET") + os.environ.pop("SLACK_BOT_TOKEN", None) + os.environ["SLACK_SIGNING_SECRET"] = "valid-signing-secret" + os.environ["SLACK_CLIENT_ID"] = "" + os.environ["SLACK_CLIENT_SECRET"] = "" + try: + adapter = SlackAdapter() + assert adapter._client_id is None + assert adapter._client_secret is None + finally: + if old_token is None: + os.environ.pop("SLACK_BOT_TOKEN", None) + else: + os.environ["SLACK_BOT_TOKEN"] = old_token + if old_secret is None: + os.environ.pop("SLACK_SIGNING_SECRET", None) + else: + os.environ["SLACK_SIGNING_SECRET"] = old_secret + if old_id is None: + os.environ.pop("SLACK_CLIENT_ID", None) + else: + os.environ["SLACK_CLIENT_ID"] = old_id + if old_csecret is None: + os.environ.pop("SLACK_CLIENT_SECRET", None) + else: + os.environ["SLACK_CLIENT_SECRET"] = old_csecret + # --------------------------------------------------------------------------- # handle_webhook with custom verifier From 8921a5c26be45f9a8e917fafcf4f8f4d511aacf5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 06:05:40 +0000 Subject: [PATCH 10/12] fix(slack): drop env-shadow on empty encryption_key (hazard #1) Second-round self-review on PR #87 found one remaining truthiness trap the first round missed: ``encryption_key = config.encryption_key or os.environ.get("SLACK_ENCRYPTION_KEY")`` at line 347 used the same fallback pattern 7c30c13 cleaned up for ``client_id`` / ``client_secret``. When the user explicitly passes ``encryption_key=""`` with ``SLACK_ENCRYPTION_KEY`` set in the env, env silently shadowed the user's explicit "opt out" intent and downstream installation tokens were encrypted with a key the user didn't ask for. Per-field ``is not None`` gating, mirroring the client_id / client_secret cascade. Functional blast radius is narrower than the bot_token case (the final ``if encryption_key_raw`` short-circuits if both user and env are empty), but the explicit-user-config-wins-over-env rule should be uniform across all four secret-bearing fields. Adding a regression test pins the behavior so a future re-introduction of the ``or`` shortcut would fail CI. Validated: - ``test_empty_encryption_key_does_not_fall_back_to_env`` fails against 7c30c13 and passes after this fix. - Full Slack test suite (70 tests) green. - Full repo test suite green except pre-existing GitHub failure (unrelated, mentioned in task instructions). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 11 +++++- .../test_slack_dynamic_token_and_verifier.py | 34 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 146f3812..06cbb3a9 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -344,7 +344,16 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: self._client_secret = None self._installation_key_prefix = config.installation_key_prefix or "slack:installation" - encryption_key_raw = config.encryption_key or os.environ.get("SLACK_ENCRYPTION_KEY") + # ``is not None`` (not truthiness) so an explicit ``encryption_key=""`` + # is treated as "user explicitly opted out" and is NOT silently + # shadowed by ``SLACK_ENCRYPTION_KEY`` from the env. Mirrors the + # client_id / client_secret rule above (hazard #1). An empty user + # config still short-circuits ``decode_key`` via the final + # ``if encryption_key_raw`` guard, so no broken key is built. + if config.encryption_key is not None: + encryption_key_raw: str | None = config.encryption_key + else: + encryption_key_raw = os.environ.get("SLACK_ENCRYPTION_KEY") self._encryption_key: bytes | None = None if encryption_key_raw: self._encryption_key = decode_key(encryption_key_raw) diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index a56d330f..f330460a 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -623,6 +623,40 @@ def test_empty_env_client_id_treated_as_unset(self): else: os.environ["SLACK_CLIENT_SECRET"] = old_csecret + def test_empty_encryption_key_does_not_fall_back_to_env(self): + """Explicit ``encryption_key=""`` is honored as "user opted out", not env-shadowed. + + Companion to the ``client_id=""`` / ``client_secret=""`` fix in + ``7c30c13``: the previous ``config.encryption_key or env`` pattern + silently substituted ``SLACK_ENCRYPTION_KEY`` whenever the user + explicitly passed an empty string. That violates hazard #1 — an + explicit user config value (even ``""``) must win over env. + + Functional impact is narrower than the client_id case because the + end result is gated by ``if encryption_key_raw``, so an empty user + config eventually becomes ``self._encryption_key = None``. But with + env set, env would *replace* the user's explicit "off" intent and + downstream installation tokens would be encrypted with a key the + user didn't ask for — surprising at minimum, and load-bearing for + callers who rotate keys by clearing the explicit config. + """ + import base64 + + old_key = os.environ.get("SLACK_ENCRYPTION_KEY") + # 32 bytes of `x`, base64-encoded — a valid key shape. + os.environ["SLACK_ENCRYPTION_KEY"] = base64.b64encode(b"x" * 32).decode() + try: + adapter = SlackAdapter(SlackAdapterConfig(signing_secret="s", encryption_key="")) + assert adapter._encryption_key is None, ( + "explicit encryption_key='' must opt out of encryption, not " + "silently fall back to SLACK_ENCRYPTION_KEY from env" + ) + finally: + if old_key is None: + os.environ.pop("SLACK_ENCRYPTION_KEY", None) + else: + os.environ["SLACK_ENCRYPTION_KEY"] = old_key + # --------------------------------------------------------------------------- # handle_webhook with custom verifier From b70813430767b7fa2d86ea4453bd9707830d05a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 08:38:09 +0000 Subject: [PATCH 11/12] docs(slack): list webhook_verifier and current_token_async divergences Round-3 self-review on PR #87 found two Python-only API additions missing from the Known Non-Parity table: - `current_token_async`: previously only `current_token` / `current_client` were listed. The async variant exists specifically for callable `bot_token` resolvers and is required when the sync property cannot drive an async resolver. - `webhook_verifier`: a fully Python-only `SlackAdapterConfig` field with no TS counterpart. Documenting it as a security-surfaced extension so a future port doesn't delete it thinking it's drift. No code changes; documentation-only update so the divergence table matches what the PR actually ships. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- docs/UPSTREAM_SYNC.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index e696a7c2..35b96995 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -488,7 +488,8 @@ stay explicit instead of being rediscovered in code review. | Fallback streaming stream-exception capture | `_fallback_stream` captures exceptions from the stream iterator, flushes whatever content was already rendered, awaits `pending_edit`, and re-raises after cleanup | `try/finally` only — exception propagates immediately, `pendingEdit` is un-awaited, and the placeholder is stranded as `"..."` | Upstream leaves a hard UX failure when streams crash mid-flight (common: LLM connection drops): placeholder visible forever, orphan background task. We flush + clean up before re-raising so the caller still sees the original error and users see the partial content instead of a spinner. | | Fallback streaming final SentMessage content | SentMessage + final edit carry `final_content` (remend'd — inline markers auto-closed) | SentMessage + final edit carry raw `accumulated` | Narrow UX refinement. If a stream ends with an unclosed `*`/`~~`/etc., upstream ships the unclosed marker; we run `_remend` so the user sees a clean final message. Not observable in the common case where streams close their own markers. | | 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_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | 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. | +| `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. | | 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). | From cfd3f62d6ee70300fca0ece5608ced41928bff87 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 15:02:54 +0000 Subject: [PATCH 12/12] fix(slack): reject empty signing_secret and non-callable webhook_verifier at init Two construction-time config-validation guards for PR #87 bot findings: - CodeRabbit Major: an explicit ``signing_secret=""`` was normalized to ``None`` and, when a ``webhook_verifier`` was also set, silently flipped the adapter from the built-in HMAC check to the custom verifier without the caller's knowledge. An explicit empty string is a config typo and is now rejected outright, regardless of whether a verifier is provided. - Codex P2: a non-callable ``webhook_verifier`` (e.g. ``""`` / ``False`` / ``123``) passed the ``is not None`` guard, then ``handle_webhook`` tried to call it, the ``TypeError`` was caught as an invalid signature, and every webhook failed closed with 401 -- an opaque production outage from a one-character typo. Now rejected at construction via ``callable()``. Both guards run before the signing_secret env-fallback cascade. Valid config paths (callable verifier, valid signing_secret, signing_secret precedence over verifier, zero-config env, and the existing zero-config rejection) are unchanged and verified. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 27 ++++++ .../test_slack_dynamic_token_and_verifier.py | 92 +++++++++++++------ 2 files changed, 91 insertions(+), 28 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 06cbb3a9..4d6a59af 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -237,6 +237,33 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: # ``None`` to surface the misconfiguration here at init rather than # silently failing on every request. 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 + # config typo (e.g. an unset env var interpolated into the field), and + # silently normalizing it to ``None`` would flip the adapter from the + # built-in HMAC check to the custom verifier *without the caller's + # knowledge*. Fail fast here so the typo surfaces at init rather than + # silently altering which verification path runs in production. (An + # unset/``None`` signing_secret still legitimately defers to the + # verifier or the env fallback below — only the explicit ``""`` is a + # hard error.) + if config.signing_secret == "": + raise ValidationError( + "slack", + "signing_secret must be a non-empty string when provided.", + ) + # Reject a non-callable ``webhook_verifier`` at construction. A typo + # such as ``webhook_verifier=""`` / ``False`` / ``123`` passes the + # ``is not None`` guard below, then ``handle_webhook`` tries to *call* + # it, the resulting ``TypeError`` is caught and reported as an invalid + # signature, and every webhook fails closed with 401 — an opaque + # production outage from a one-character mistake. ``None`` (unset) is + # fine; anything else must be callable. + if webhook_verifier is not None and not callable(webhook_verifier): + raise ValidationError( + "slack", + "webhook_verifier must be callable.", + ) signing_secret = ( config.signing_secret if config.signing_secret is not None diff --git a/tests/test_slack_dynamic_token_and_verifier.py b/tests/test_slack_dynamic_token_and_verifier.py index f330460a..ce25fee7 100644 --- a/tests/test_slack_dynamic_token_and_verifier.py +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -435,19 +435,20 @@ def test_empty_signing_secret_rejected_at_construction(self): secret. ``_verify_signature`` short-circuits with ``if not self._signing_secret`` and returns ``False`` for every webhook, leaving the adapter responding ``401`` to Slack on every - request rather than failing fast at construction. Empty-string is - an unusable value: it must be treated as "not provided" by the - validation guard so the misconfiguration surfaces here, not on the - production webhook path. + request rather than failing fast at construction. An explicit empty + string is now rejected outright by a dedicated construction guard + (see :meth:`test_empty_signing_secret_rejected_even_with_verifier`), + so the misconfiguration surfaces here, not on the production webhook + path. """ old = os.environ.pop("SLACK_SIGNING_SECRET", None) try: - with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + with pytest.raises(ValidationError, match="signing_secret must be a non-empty string"): SlackAdapter(SlackAdapterConfig(signing_secret="")) # Also fails when an otherwise-valid bot_token is set: the # signing_secret value is what matters, not the rest of the # config. - with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + with pytest.raises(ValidationError, match="signing_secret must be a non-empty string"): SlackAdapter(SlackAdapterConfig(signing_secret="", bot_token="xoxb-test")) finally: if old is not None: @@ -457,16 +458,15 @@ def test_empty_signing_secret_does_not_fall_back_to_env(self): """Empty-string ``signing_secret`` is rejected even with SLACK_SIGNING_SECRET set. Pairs with :meth:`test_empty_signing_secret_rejected_at_construction`: - normalizing ``""`` to ``None`` *after* the env-fallback cascade - means the explicit empty string still suppresses the env fallback - (that part of ``5a648ec`` is correct), and the resulting ``None`` - then fails the ``signing_secret is None and webhook_verifier is - None`` guard. The user's explicit (if empty) config wins over env. + an explicit empty string is a hard error at construction (the + dedicated guard fires before the env-fallback cascade), so a + deployment-set ``SLACK_SIGNING_SECRET`` cannot rescue the typo. The + user's explicit (if empty) config loses to nothing — it fails fast. """ old = os.environ.get("SLACK_SIGNING_SECRET") os.environ["SLACK_SIGNING_SECRET"] = "env-secret-should-not-be-used" try: - with pytest.raises(ValidationError, match="signingSecret or webhookVerifier"): + with pytest.raises(ValidationError, match="signing_secret must be a non-empty string"): SlackAdapter(SlackAdapterConfig(signing_secret="", bot_token="xoxb-test")) finally: if old is None: @@ -474,24 +474,60 @@ def test_empty_signing_secret_does_not_fall_back_to_env(self): else: os.environ["SLACK_SIGNING_SECRET"] = old - def test_empty_signing_secret_with_verifier_is_accepted(self): - """``signing_secret=""`` + ``webhook_verifier`` => verifier path takes over. + def test_empty_signing_secret_rejected_even_with_verifier(self): + """``signing_secret=""`` is rejected at construction even with a verifier. + + Regression for CodeRabbit Major on PR #87. An explicit empty-string + ``signing_secret`` is a config typo (e.g. an unset env var + interpolated into the field). Previously it normalized to ``None`` and, + when a ``webhook_verifier`` was also set, the guard passed and the + adapter silently switched from the built-in HMAC check to the custom + verifier — without the caller's knowledge. The explicit empty string + must now fail fast at construction regardless of whether a verifier is + provided, so the typo surfaces at init rather than silently altering + which verification path runs in production. + """ + old = os.environ.pop("SLACK_SIGNING_SECRET", None) + try: + with pytest.raises(ValidationError, match="signing_secret must be a non-empty string"): + SlackAdapter( + SlackAdapterConfig( + signing_secret="", + bot_token="xoxb-test", + webhook_verifier=lambda req, body: True, + ) + ) + finally: + if old is not None: + os.environ["SLACK_SIGNING_SECRET"] = old + + def test_non_callable_webhook_verifier_rejected_at_construction(self): + """A non-callable ``webhook_verifier`` must fail validation at init. - The validation guard fails only when *both* are unusable. An empty - signing secret with an explicit verifier normalizes to "no signing - secret, use the verifier" — consistent with the documented - precedence rule (a non-empty signing_secret would win, an empty - one is treated as absent). + Regression for Codex P2 on PR #87. A typo such as + ``webhook_verifier=""`` / ``False`` / ``123`` passed the ``is not + None`` guard, then ``handle_webhook`` tried to *call* it, the + resulting ``TypeError`` was caught and reported as an invalid + signature, and every webhook failed closed with 401 — an opaque + production outage from a one-character mistake. Reject any non-None, + non-callable value at construction so the typo surfaces immediately. """ - adapter = SlackAdapter( - SlackAdapterConfig( - signing_secret="", - bot_token="xoxb-test", - webhook_verifier=lambda req, body: True, - ) - ) - assert adapter._signing_secret is None - assert adapter._webhook_verifier is not None + old = os.environ.get("SLACK_SIGNING_SECRET") + os.environ["SLACK_SIGNING_SECRET"] = "valid-signing-secret" + try: + for bad in ("", False, 123): + with pytest.raises(ValidationError, match="webhook_verifier must be callable"): + SlackAdapter( + SlackAdapterConfig( + bot_token="xoxb-test", + webhook_verifier=bad, # type: ignore[arg-type] + ) + ) + finally: + if old is None: + os.environ.pop("SLACK_SIGNING_SECRET", None) + else: + os.environ["SLACK_SIGNING_SECRET"] = old def test_empty_env_slack_bot_token_does_not_become_static_token(self): """``SLACK_BOT_TOKEN=""`` must not be cached as the default bot token.