[ROB-3999] Fix oauth client secret post 200 (HolmesGPT#2106)

The two-attempt token exchange
After the user clicks "Allow" in the browser, Slack redirects to Holmes
with an authorization code. Holmes calls exchange_code_for_tokens to
swap that code for an access_token.

Attempt 1 — HTTP Basic Auth (unchanged):


auth = httpx.BasicAuth(client_id, client_secret)
resp = httpx.post(token_url, data=data, auth=auth, ...)
Holmes sends client_id + client_secret in the Authorization header.

For Slack, this returns:


HTTP/2 200
{"ok": false, "error": "bad_client_secret"}
That's the trap: Slack returns 200 OK with an error body because its
token endpoint only accepts client_secret_post, not Basic Auth.

Attempt 2 — POST-body fallback (the fix):

Before:


if client_secret and not resp.is_success:
    # retry with secret in body
This never fired for Slack, because resp.is_success was True.

After:


def _has_access_token(r):
    try:
        return "access_token" in r.json()
    except Exception:
        return False

if client_secret and (not resp.is_success or not
_has_access_token(resp)):
    data["client_secret"] = client_secret
resp = httpx.post(token_url, data=data, ...) # no auth=, secret in body
The new clause not _has_access_token(resp) treats a 200 response without
an access_token field as a soft failure. For Slack's 200-with-error
pattern, this fires the body retry — Holmes re-POSTs with client_secret
inside the form-encoded body. Slack accepts that and responds:


{"ok": true, "access_token": "xoxp-…", "scope": "...", "team": {...}}
The function then validates "access_token" in token_data and returns the
dict.

Why this is safe for IdPs that worked before
IdPs that accept Basic Auth (e.g. Notion) still succeed on attempt 1 —
the response contains access_token, _has_access_token returns True, no
retry happens.
IdPs that return 4xx on Basic Auth (e.g. Supabase) still trigger the
retry via the original not resp.is_success branch.
The change only adds a third trigger for the retry; no existing IdP
loses any behavior.
What you'd see end-to-end (verified live earlier in this session)
CLI/cluster opens browser → user authorizes → callback delivers code.
exchange_code_for_tokens POSTs with Basic Auth → Slack returns 200 +
bad_client_secret.
New gate detects no access_token → POSTs again with secret in body →
Slack returns 200 + xoxp-… token.
Holmes caches the token and uses it as Authorization: Bearer xoxp-…
against https://mcp.slack.com/mcp, then calls tools/list → tools come
back → Holmes can invoke slack_read_user_profile, slack_read_file, etc.
The two regression tests in tests/test_mcp_oauth.py exercise both
Slack-style outcomes (success on body retry; failure if body retry also
returns no token), so the behavior is locked in.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* OAuth token exchange now properly handles successful HTTP responses
that lack access tokens, automatically retrying with alternative
credential delivery methods—improving compatibility with certain
identity providers.
* Enhanced error diagnostics for OAuth tool loading failures to better
identify configuration mismatches or workspace/token inconsistencies.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: Avi-Robusta

holmes/core/oauth_config.py

@@ -76,8 +76,17 @@ def exchange_code_for_tokens(
     except httpx.HTTPError as e:
         raise OAuthTokenExchangeError(0, f"Token request to {token_url} failed: {e}") from e
 
-    # If Basic Auth failed, retry with client_secret in POST body
-    if client_secret and not resp.is_success:
+    # Retry with client_secret in POST body if Basic Auth failed, OR if the IdP
+    # returned 200 with a non-token body (e.g. Slack responds HTTP 200 +
+    # {"ok": false, "error": "bad_client_secret"} when it only accepts
+    # client_secret_post).
+    def _has_access_token(r: httpx.Response) -> bool:
+        try:
+            return "access_token" in r.json()
+        except Exception:
+            return False
+
+    if client_secret and (not resp.is_success or not _has_access_token(resp)):
         data["client_secret"] = client_secret
         try:
             resp = httpx.post(

holmes/core/tools_utils/oauth_tool_connector.py

@@ -131,6 +131,7 @@ def load_tools_for_user(
                     "Failed to load OAuth tools for user %s on toolset %s: %s",
                     user_id, toolset.name, self._extract_error_message(e),
                 )
+                self._log_token_config_mismatch(user_id, toolset)
             return []
 
     def store_user_tools(self, user_id: str, toolset_name: str, tools: List[Tool]) -> None:
@@ -227,6 +228,60 @@ def _extract_error_message(exc: BaseException) -> str:
             current = current.exceptions[0]
         return str(current)
 
+    @staticmethod
+    def _log_token_config_mismatch(user_id: str, toolset: Any) -> None:
+        """On MCP failure, log if the stored token was issued under a different
+        client_id / token_url than the toolset's current OAuth config.
+
+        Catches config drift that would otherwise be invisible: rotated
+        client_id, two toolsets sharing a provider URL with different client
+        configs, or a stale cached token after credentials were changed.
+        Also surfaces the workspace/team the token belongs to for servers
+        that gate access per workspace (e.g. Slack), so users can spot
+        wrong-workspace auth.
+        """
+        try:
+            oauth_cfg = getattr(getattr(toolset, "_mcp_config", None), "oauth", None)
+            if oauth_cfg is None:
+                return
+            store = getattr(_get_token_manager(), "_store", None)
+            if store is None:
+                return
+            provider = getattr(oauth_cfg, "authorization_url", None) or "unknown"
+            try:
+                tok = store.get_token(provider, user_id=user_id)
+            except TypeError:
+                tok = store.get_token(provider)
+            if not isinstance(tok, dict):
+                return
+
+            cfg_client_id = getattr(oauth_cfg, "client_id", None)
+            cfg_token_url = getattr(oauth_cfg, "token_url", None)
+            tok_client_id = tok.get("client_id")
+            tok_token_url = tok.get("token_url")
+            tok_team = tok.get("team")
+
+            mismatches = []
+            if cfg_client_id and tok_client_id and cfg_client_id != tok_client_id:
+                mismatches.append(f"client_id: stored={tok_client_id} config={cfg_client_id}")
+            if cfg_token_url and tok_token_url and cfg_token_url != tok_token_url:
+                mismatches.append(f"token_url: stored={tok_token_url} config={cfg_token_url}")
+
+            if mismatches:
+                logger.warning(
+                    "MCP token/config mismatch for user %s on toolset %s — %s. "
+                    "The cached token was issued under a different OAuth config; re-authenticate.",
+                    user_id, toolset.name, "; ".join(mismatches),
+                )
+            elif tok_team:
+                logger.warning(
+                    "MCP failure for user %s on toolset %s — token was issued in workspace %s. "
+                    "If the server gates access per workspace, the user may have authenticated in the wrong one.",
+                    user_id, toolset.name, tok_team,
+                )
+        except Exception:
+            logger.debug("Failed to log token/config mismatch", exc_info=True)
+
     @staticmethod
     def _evict_expired_token(user_id: str, toolset: Any) -> None:
         """Remove an expired/revoked token from cache and disk (CLI only).

tests/test_mcp_oauth.py

@@ -32,7 +32,9 @@
     OAuthDecisionCode,
     OAuthEndpoints,
     OAuthExchangeManager,
+    OAuthTokenExchangeError,
     _get_exchange_manager,
+    exchange_code_for_tokens,
     parse_oauth_decision,
 )
 from holmes.core.oauth_utils import (
@@ -370,6 +372,73 @@ def test_missing_exchange_does_not_crash(self):
         _get_exchange_manager().complete_exchange("nonexistent-id", oauth_code, None)
         # Should log error but not raise
 
+    def test_client_secret_post_falls_back_when_idp_returns_200_on_error(self):
+        """Regression: Slack (and other client_secret_post-only IdPs) return HTTP 200
+        with {"ok": false, "error": ...} when the secret is sent via Basic Auth.
+
+        The exchange must detect a 200 response without an access_token and retry
+        with client_secret in the POST body.
+        """
+        basic_auth_resp = MagicMock()
+        basic_auth_resp.status_code = 200
+        basic_auth_resp.is_success = True
+        basic_auth_resp.json.return_value = {"ok": False, "error": "bad_client_secret"}
+
+        body_auth_resp = MagicMock()
+        body_auth_resp.status_code = 200
+        body_auth_resp.is_success = True
+        body_auth_resp.json.return_value = {
+            "ok": True,
+            "access_token": "xoxp-slack-token",
+            "token_type": "Bearer",
+        }
+
+        with patch(
+            "holmes.core.oauth_config.httpx.post",
+            side_effect=[basic_auth_resp, body_auth_resp],
+        ) as mock_post:
+            token_data = exchange_code_for_tokens(
+                token_url="https://slack.com/api/oauth.v2.user.access",
+                code="slack-auth-code",
+                redirect_uri="http://frontend/callback",
+                client_id="slack-client-id",
+                code_verifier="slack-pkce-verifier",
+                client_secret="slack-client-secret",
+            )
+
+        assert token_data["access_token"] == "xoxp-slack-token"
+        assert mock_post.call_count == 2
+
+        first_call_kwargs = mock_post.call_args_list[0].kwargs
+        assert isinstance(first_call_kwargs.get("auth"), httpx.BasicAuth)
+
+        second_call_kwargs = mock_post.call_args_list[1].kwargs
+        assert second_call_kwargs.get("auth") is None
+        assert second_call_kwargs["data"]["client_secret"] == "slack-client-secret"
+
+    def test_client_secret_post_raises_when_body_retry_also_fails(self):
+        """If even the POST-body retry returns 200 without access_token, the function
+        must still raise OAuthTokenExchangeError (no silent success)."""
+        bad_resp = MagicMock()
+        bad_resp.status_code = 200
+        bad_resp.is_success = True
+        bad_resp.text = '{"ok": false, "error": "bad_client_secret"}'
+        bad_resp.json.return_value = {"ok": False, "error": "bad_client_secret"}
+
+        with patch(
+            "holmes.core.oauth_config.httpx.post",
+            side_effect=[bad_resp, bad_resp],
+        ):
+            with pytest.raises(OAuthTokenExchangeError) as excinfo:
+                exchange_code_for_tokens(
+                    token_url="https://slack.com/api/oauth.v2.user.access",
+                    code="slack-auth-code",
+                    redirect_uri="http://frontend/callback",
+                    client_id="slack-client-id",
+                    client_secret="slack-client-secret",
+                )
+            assert "access_token" in str(excinfo.value.detail)
+
 
 class TestParseOAuthDecision:
     def test_valid_oauth_decision(self):