diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index f8ac33a..7f63122 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -493,7 +493,10 @@ stay explicit instead of being rediscovered in code review. | Teams native streaming final-send when first chunk's `id` was empty (DMs) | `_close_stream_session` sends the final `message` activity whenever `text` is non-empty, even if `stream_id` is `None` (Bot Framework REST response returned `{"id": ""}` on the first chunk). The final activity omits `streamId` from `channelData` rather than serializing `None`. | Upstream's `streamViaEmit` awaits the `chunk` event for the first activity's `id`; if Teams returns an empty id, `messageId` becomes `""` and the SDK's auto-close emits the final activity through `IStreamer` regardless | Without the final `message` activity, the Teams client's streaming UI keeps spinning until the platform times the session out — a stuck-loading-state UX failure with no user workaround. We mirror upstream's looser check (text non-empty → ship the final) so the streaming indicator clears even when the Bot Framework REST response surface returns an empty `id`. | | `RawMessage.text` override field — **transitional** | New optional `text: str \| None = None` field on `RawMessage` (`src/chat_sdk/types.py`). When set by an adapter, `Thread._handle_stream` MUST prefer it over its own local accumulator when constructing the recorded `SentMessage` body / message-history entry. `None` falls back to the local accumulator (backward-compatible default for adapters that don't need the override). Set by `_stream_via_emit` so the recorded message matches what Teams actually shipped, even when chunks were buffered into the throttle window and cancellation skipped the flush emit. | Upstream's `RawMessage` (`packages/chat/src/types.ts`) is `{ id; raw; threadId }` only. Cancellation-text reconciliation lives inside `@microsoft/teams.apps`'s `IStreamer.emit` (the npm SDK owns the buffer and never surfaces a buffered-but-unsent suffix to `chat`). | Direct consequence of the hand-rolled Teams native streaming row above. Without the override, the SDK's local accumulator (which captures every chunk yielded to the adapter, including chunks coalesced into the throttle window) would diverge from what Teams actually accepted whenever a session is canceled with buffered text pending — recording text the user never saw. Disappears alongside the hand-rolled wire format once we migrate to `microsoft-teams-apps` (Python). Regression coverage: `tests/test_thread_faithful.py::test_should_prefer_raw_message_text_override_over_local_accumulator` (would fail if someone "fixes" Thread.stream back to upstream's local-accumulator-only behavior) and `tests/test_teams_native_streaming.py::test_canceled_stream_sets_raw_message_text_override`. | | Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | -| `SlackAdapter.current_token` / `current_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). | | `ConcurrencyConfig.max_concurrent` slot scope | **Single global `asyncio.Semaphore`** — caps total in-flight handlers across all threads to `max_concurrent` | **Per-thread slot map** — `acquireConcurrentSlot(threadId, maxConcurrent)` keys the in-flight counter by `threadId`, so each thread has its own N-way bound | When upstream caught up (vercel/chat#419) it implemented per-thread slots; the Python port shipped earlier with a global semaphore and the slot-scope distinction wasn't visible in the original divergence row. Result: a deployment with `max_concurrent=2` and 100 active threads serializes everything globally on Python (peak in-flight = 2 across all threads) but allows 200 concurrent handlers on TS (2 per thread × 100). The `chat.test.ts > should track slots per thread independently` fidelity entry is `pytest.mark.skip`-ped in `tests/test_chat_faithful.py` until the implementation is restructured to a `dict[thread_id, asyncio.Semaphore]` (with cleanup-on-empty to avoid unbounded growth). Tracked as a follow-up. | | 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. | diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 98d19ec..75044bc 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -49,8 +49,11 @@ RequestContext, SlackAdapterConfig, SlackAdapterMode, + 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 @@ -166,6 +169,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 # --------------------------------------------------------------------------- @@ -189,18 +214,149 @@ 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() mode = config.mode or "webhook" - signing_secret = config.signing_secret or os.environ.get("SLACK_SIGNING_SECRET") - if not signing_secret and mode == "webhook": + + # An explicit ``webhook_verifier`` opts out of the SLACK_SIGNING_SECRET + # env fallback — otherwise an env-configured deployment would silently + # shadow the verifier the caller intended to use. + # + # Use explicit ``is not None`` checks rather than truthiness fallbacks + # (per docs/UPSTREAM_SYNC.md hazard #1): an explicit empty string for + # ``signing_secret`` should fail validation, not silently fall through + # to ``SLACK_SIGNING_SECRET`` from the environment. *But* an empty + # string is itself unusable downstream (``_verify_signature`` would + # short-circuit with ``if not self._signing_secret`` and reject every + # webhook with 401), so after the cascade we normalize ``""`` to + # ``None`` to surface the misconfiguration here at init rather than + # silently failing on every request. + 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 + else (None if webhook_verifier is not None else os.environ.get("SLACK_SIGNING_SECRET")) + ) + if signing_secret == "": + signing_secret = None + # ``signing_secret`` is required in webhook mode (unless a + # ``webhook_verifier`` replaces the built-in HMAC check). Socket mode + # legitimately runs without a signing secret because Slack does not + # sign events delivered over the WebSocket — only HTTP-forwarded events + # need a separate ``socket_forwarding_secret`` bearer check. + if mode == "webhook" and signing_secret is None and webhook_verifier is None: + raise ValidationError( + "slack", + "signingSecret or webhookVerifier is required for webhook mode. Set " + "SLACK_SIGNING_SECRET, provide a non-empty signing_secret in config, " + "or provide a webhook_verifier.", + ) + + # Auth fields: botToken presence selects single-workspace mode. + # 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. ``app_token`` + # also participates (so socket-mode-only configs disable env fallbacks + # for the other secrets the same way bot-token-only configs do). + 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 + and config.app_token is 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", - "signingSecret is required for webhook mode. Set SLACK_SIGNING_SECRET or provide it in config.", + "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 + # 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) - app_token = config.app_token or os.environ.get("SLACK_APP_TOKEN") + self._name = "slack" + self._signing_secret: str | None = signing_secret + # ``webhook_verifier`` is honored only when ``signing_secret`` is not set — + # signing_secret takes precedence when both are provided (matches upstream). + self._webhook_verifier: SlackWebhookVerifier | None = None if signing_secret else webhook_verifier + # 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 + + # ------------------------------------------------------------------ + # Socket mode wiring (PR #86). Resolved AFTER the bot-token resolver + # is set up so the eventual socket client can read the resolved token + # via ``_get_token`` / ``current_token_async``. ``app_token`` is the + # long-lived app-level secret used to open the WebSocket; the bot + # token (potentially resolver-backed) is still used for API calls. + # ------------------------------------------------------------------ + app_token = config.app_token if config.app_token is not None else os.environ.get("SLACK_APP_TOKEN") + if app_token == "": + app_token = None if mode == "socket": if not app_token: raise ValidationError( @@ -217,17 +373,6 @@ def __init__(self, config: SlackAdapterConfig | None = None) -> None: "Bot tokens (xoxb-) are not valid for socket mode.", ) - # 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 or config.app_token - ) - - bot_token = config.bot_token or (os.environ.get("SLACK_BOT_TOKEN") if zero_config else None) - - self._name = "slack" - self._signing_secret: str | None = signing_secret - self._default_bot_token: str | None = bot_token - # Socket mode state self._mode: SlackAdapterMode = mode self._app_token: str | None = app_token @@ -272,14 +417,38 @@ 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") + # ``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) @@ -337,12 +506,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`. @@ -363,20 +546,121 @@ 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.", + ) + + async def _resolve_token_async(self) -> str: + """Async equivalent of :meth:`_get_token` that invokes the resolver. - Checks request context (multi-workspace) -> default token (single-workspace) -> raises. + 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() + 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 not isinstance(token, str) or not token: + raise AuthenticationError( + "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 + + @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. @@ -418,10 +702,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 @@ -435,7 +722,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") if self._mode == "socket": @@ -797,8 +1084,13 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # Forwarded socket-mode events bypass Slack signature verification — # they're authenticated by a shared bearer secret instead. This lets a # separate process run the WebSocket and POST events back to the - # webhook endpoint over HTTP. Hazard #12: refuse if no secret is - # configured rather than treating an empty header match as success. + # webhook endpoint over HTTP. Handled BEFORE any signature / verifier / + # resolver work because the auth model is entirely different: an + # ``x-slack-socket-token`` request never carries a Slack signature + # and shouldn't pay the resolver-call cost (or surface resolver + # failures) when its bearer is invalid. Hazard #12: refuse if no + # secret is configured rather than treating an empty header match as + # success. socket_token = headers.get("x-slack-socket-token") or headers.get("X-Slack-Socket-Token") if socket_token: if not self._socket_forwarding_secret or not hmac.compare_digest( @@ -842,21 +1134,83 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No if self._mode == "socket": return {"body": "Webhooks are disabled in socket mode", "status": 405} - 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") + # 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} + + # 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"}, + } - 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 "" if "application/x-www-form-urlencoded" in content_type: params = parse_qs(body, keep_blank_values=True) # 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) @@ -868,7 +1222,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) @@ -901,7 +1255,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) @@ -922,11 +1276,17 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # ================================================================== def _verify_signature(self, body: str, timestamp: str | None, signature: str | None) -> bool: - # Refuse rather than HMAC against an empty key. This matters in socket - # mode where ``signing_secret`` is optional — without this guard a + # 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. This also covers the socket-mode case where + # ``signing_secret`` is legitimately ``None`` — without this guard a # caller could call ``handle_webhook`` while in socket mode and # silently pass verification with an empty secret. - if not (timestamp and signature and self._signing_secret): + if not self._signing_secret: + return False + + if not (timestamp and signature): return False # Check timestamp is recent (within 5 minutes) @@ -949,6 +1309,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 @@ -1735,7 +2099,7 @@ def _log_exc(t: asyncio.Task[Any]) -> None: # by handlers (hazard #6). team_id_event = payload.get("team_id") try: - if not self._default_bot_token and team_id_event: + if not self._is_single_workspace and team_id_event: ctx = await self._resolve_token_for_team(team_id_event) if ctx is None: self._logger.warn( @@ -1769,7 +2133,7 @@ def _log_exc(t: asyncio.Task[Any]) -> None: async def run_slash() -> None: team_id_slash = (params.get("team_id") or [None])[0] - if not self._default_bot_token and team_id_slash: + if not self._is_single_workspace and team_id_slash: ctx = await self._resolve_token_for_team(team_id_slash) if ctx is None: self._logger.warn("Could not resolve token for slash command") @@ -1790,7 +2154,7 @@ async def run_slash() -> None: # Multi-workspace: scope token resolution to the dispatch. team_ref = body.get("team") team_id_interactive = team_ref.get("id") if isinstance(team_ref, dict) else body.get("team_id") - if not self._default_bot_token and team_id_interactive: + if not self._is_single_workspace and team_id_interactive: ctx = await self._resolve_token_for_team(team_id_interactive) if ctx is None: self._logger.warn("Could not resolve token for interactive payload") @@ -2503,8 +2867,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" @@ -2516,7 +2885,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: @@ -2623,7 +2993,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( @@ -3150,7 +3522,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) @@ -3183,7 +3564,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 2f3fd21..f41328f 100644 --- a/src/chat_sdk/adapters/slack/types.py +++ b/src/chat_sdk/adapters/slack/types.py @@ -2,11 +2,48 @@ from __future__ import annotations +from collections.abc import Awaitable, Callable from dataclasses import dataclass -from typing import Any, Literal, TypedDict +from typing import Any, Literal, TypeAlias, 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: TypeAlias = 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 +# - 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]"] + # Connection mode for the Slack adapter. ``"webhook"`` (default) consumes # events via signed HTTP POSTs from Slack. ``"socket"`` opens a long-lived # WebSocket via Slack's Socket Mode and ACKs each event over the socket. @@ -24,7 +61,10 @@ class SlackAdapterConfig: # App-level token (xapp-...). Required when ``mode == "socket"``. app_token: str | None = None # 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) @@ -44,9 +84,17 @@ class SlackAdapterConfig: # ``initialize()`` and dispatches events over it. ``signing_secret`` is # not required in socket mode (Slack does not sign socket events). mode: SlackAdapterMode = "webhook" - # Signing secret for webhook verification. Defaults to SLACK_SIGNING_SECRET env var. - # Required in webhook mode; optional in socket mode. + # Signing secret for webhook verification. Defaults to SLACK_SIGNING_SECRET env var, + # *unless* ``webhook_verifier`` is provided — passing an explicit verifier opts + # out of the env fallback so a deployment-set ``SLACK_SIGNING_SECRET`` can't + # silently shadow the verifier. Required in webhook mode; optional in socket mode. 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 # Shared secret for authenticating events forwarded from a separate # socket-mode listener via HTTP POST. Auto-detected from # SLACK_SOCKET_FORWARDING_SECRET. Falls back to ``app_token`` if not set 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 0000000..ce25fee --- /dev/null +++ b/tests/test_slack_dynamic_token_and_verifier.py @@ -0,0 +1,1374 @@ +"""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)) + # 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 + 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_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: + 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() + + @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" + ) + + @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"} + + @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 +# --------------------------------------------------------------------------- + + +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 + + 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. 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="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="signing_secret must be a non-empty string"): + 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`: + 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="signing_secret must be a non-empty string"): + 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_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. + + 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. + """ + 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. + + 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 + + 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 + + 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 +# --------------------------------------------------------------------------- + + +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): + """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: + calls.append(1) + return "xoxb-from-resolver" + + adapter = SlackAdapter( + SlackAdapterConfig( + signing_secret="test-signing-secret", + bot_token=resolver, + ) + ) + # 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], "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): + """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. 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 _re.search(r"\bhmac\.compare_digest\s*\(", src), ( + "default signature verifier must call 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" + + +# --------------------------------------------------------------------------- +# 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}" + ) diff --git a/tests/test_slack_socket_mode.py b/tests/test_slack_socket_mode.py index 10d5e3a..df6d176 100644 --- a/tests/test_slack_socket_mode.py +++ b/tests/test_slack_socket_mode.py @@ -197,7 +197,12 @@ def test_webhook_mode_still_requires_signing_secret(self): # satisfied by SLACK_SIGNING_SECRET in the dev shell. prev = os.environ.pop("SLACK_SIGNING_SECRET", None) try: - with pytest.raises(ValidationError, match="signingSecret is required"): + # PR #87 widened the message to "signingSecret or webhookVerifier + # is required" once a custom verifier became a valid alternative + # to the env-configured signing secret. The spirit of the check + # — webhook mode without any verification mechanism is rejected — + # is unchanged. + with pytest.raises(ValidationError, match="signingSecret"): SlackAdapter(SlackAdapterConfig(bot_token="xoxb-test")) finally: if prev is not None: