From 87f1ced2275c0a0a09f5749a3b919c8a2a1303c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 30 May 2026 02:04:12 +0000 Subject: [PATCH] fix(slack): timing-safe socket-token comparison (port 9824d33 slice) Port the Slack slice of upstream 9824d33 (PR #441 adapter-hardening pass): the forwarded Socket Mode events receiver validates the `x-slack-socket-token` bearer against `socket_forwarding_secret`. Encode both operands to UTF-8 bytes before `hmac.compare_digest`, mirroring upstream `timingSafeStringEqual`'s `Buffer.from(x, "utf8")` comparison. This keeps the constant-time guarantee, returns False on length mismatch (the secret length is fixed at config time, so not a leak), and avoids the `TypeError` that `str` `compare_digest` raises on non-ASCII tokens (which would have surfaced as a crash rather than a clean 401). Per CLAUDE.md Port Rule: `==` for signatures -> hmac.compare_digest. Tests: spy on `hmac.compare_digest` to prove the timing-safe path is exercised with byte operands (load-bearing against a regression to `==`), plus a non-ASCII token is rejected with 401 rather than crashing. Existing valid/invalid/missing-secret cases retained. 1 of 4 slices of the 9824d33 security pass (gchat/github/linear slices ship as separate PRs). Refs #98. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj --- src/chat_sdk/adapters/slack/adapter.py | 8 ++- tests/test_slack_socket_mode.py | 79 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 7cb2f1a..d1168ee 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -1235,8 +1235,14 @@ async def handle_webhook(self, request: Any, options: WebhookOptions | None = No # success. socket_token = headers.get("x-slack-socket-token") or headers.get("X-Slack-Socket-Token") if socket_token: + # Constant-time bearer comparison (upstream timingSafeStringEqual, + # 9824d33 / PR #441). Encode both operands to UTF-8 bytes so + # ``compare_digest`` mirrors upstream's ``Buffer.from(x, "utf8")`` + # comparison: it returns False on length mismatch (the secret's + # length is fixed at config time, so that is not a leak) and never + # raises on non-ASCII tokens the way str comparison would. if not self._socket_forwarding_secret or not hmac.compare_digest( - socket_token, self._socket_forwarding_secret + socket_token.encode(), self._socket_forwarding_secret.encode() ): self._logger.warn("Invalid socket forwarding token") return {"body": "Invalid socket token", "status": 401} diff --git a/tests/test_slack_socket_mode.py b/tests/test_slack_socket_mode.py index df6d176..789bddc 100644 --- a/tests/test_slack_socket_mode.py +++ b/tests/test_slack_socket_mode.py @@ -343,6 +343,85 @@ async def test_webhook_socket_token_rejected_when_no_secret_configured(self): result = await adapter.handle_webhook(request) assert result["status"] == 401 + async def test_webhook_socket_token_uses_timing_safe_compare(self, monkeypatch): + """The socket-token check MUST route through ``hmac.compare_digest``. + + Upstream switched this comparison to a constant-time check + (timingSafeStringEqual, 9824d33 / PR #441). This is the load-bearing + guard against a regression to a plain ``==`` / ``!=`` comparison: + we spy on ``hmac.compare_digest`` as referenced by the adapter module + and assert it is invoked for the socket token, with both operands + encoded to bytes (mirroring upstream's ``Buffer.from(x, "utf8")``). + """ + import time as _time + + from chat_sdk.adapters.slack import adapter as adapter_mod + + calls: list[tuple[Any, Any]] = [] + real_compare = adapter_mod.hmac.compare_digest + + def _spy_compare(a, b): + calls.append((a, b)) + return real_compare(a, b) + + monkeypatch.setattr(adapter_mod.hmac, "compare_digest", _spy_compare) + + adapter = _make_socket_adapter(socket_forwarding_secret="fwd-secret") + chat = _make_mock_chat() + adapter._chat = chat + forwarded = { + "type": "socket_event", + "eventType": "events_api", + "body": { + "type": "event_callback", + "event": { + "type": "message", + "channel": "C123", + "ts": "1.0", + "user": "U1", + "text": "hi", + "team": "T1", + }, + "team_id": "T1", + }, + "timestamp": int(_time.time()), + } + request = _FakeRequest( + json.dumps(forwarded), + headers={ + "content-type": "application/json", + "x-slack-socket-token": "fwd-secret", + }, + ) + result = await adapter.handle_webhook(request) + + # Valid token => accepted, AND the timing-safe path was exercised + # with byte-encoded operands. + assert result["status"] == 200 + assert (b"fwd-secret", b"fwd-secret") in calls, ( + f"socket token must be compared via hmac.compare_digest on bytes, not == ; recorded calls: {calls!r}" + ) + + async def test_webhook_socket_token_non_ascii_rejected_not_crash(self): + """A non-ASCII socket token must be rejected (401), never crash. + + ``hmac.compare_digest`` raises ``TypeError`` on non-ASCII ``str`` + operands; encoding to bytes (the upstream behavior) avoids that. This + pins the ``.encode()`` so a regression to str comparison surfaces as a + crash here rather than silently in production. + """ + adapter = _make_socket_adapter(socket_forwarding_secret="fwd-secret") + adapter._chat = _make_mock_chat() + request = _FakeRequest( + json.dumps({"type": "socket_event", "eventType": "events_api", "body": {}}), + headers={ + "content-type": "application/json", + "x-slack-socket-token": "café-not-the-secret", + }, + ) + result = await adapter.handle_webhook(request) + assert result["status"] == 401 + # --------------------------------------------------------------------------- # _route_socket_event dispatch