From d05cc8f4d6b588800c6882a667dd5272e61da0a2 Mon Sep 17 00:00:00 2001 From: homelab-ha-agent Date: Sat, 20 Jun 2026 19:40:54 -0400 Subject: [PATCH] fix(mcp): skip preflight content-type probe for OAuth servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OAuth-protected MCP servers (e.g. Hospitable) return 200 text/html on an unauthenticated HEAD probe — a login/landing page the server cannot substitute for a real MCP response without a Bearer token. The preflight cannot distinguish this from a misconfigured URL, so it raises NonMcpEndpointError before the OAuth browser flow has a chance to run. Add `and self._auth_type != "oauth"` to the preflight condition in MCPServerTask.run(). The probe is inapplicable to OAuth servers: their URL legitimacy is established by .well-known/oauth-protected-resource during the OAuth handshake, not by a GET content-type check. Concrete repro: Hospitable (https://mcp.hospitable.com/mcp) returns `200 text/html` to an unauthenticated httpx HEAD. Without the guard: ✗ NonMcpEndpointError at `hermes mcp test` With the guard: ✓ Connected (1487ms) — 63 tools discovered Relation to open PRs: - #37598 adds a POST probe fallback for POST-only non-OAuth servers (e.g. DocuSeal), but only passes when POST returns 2xx + MCP content-type. Hospitable returns 401 on the POST probe (Bearer challenge), so #37598 does not cover this case. - #49463 extends the POST probe to also pass on non-2xx auth challenges (making it OAuth-aware), but is labeled duplicate of #37598 and may not land independently. This fix is complementary: it handles OAuth servers with zero extra round-trips rather than adding a POST probe step. Tests: - test_oauth_server_html_response_raises_without_skip: documents that _preflight_content_type raises NonMcpEndpointError for 200 text/html (the underlying issue), with an OAuth-server docstring. - test_run_skips_preflight_for_oauth: verifies that run() does NOT invoke _preflight_content_type when auth_type=="oauth", using class-level monkeypatching so the gate is exercised without a live MCP transport. 23 passed tests/tools/test_mcp_preflight_content_type.py --- .../tools/test_mcp_preflight_content_type.py | 78 +++++++++++++++++++ tools/mcp_tool.py | 12 +-- 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/tests/tools/test_mcp_preflight_content_type.py b/tests/tools/test_mcp_preflight_content_type.py index 312aa48dfc9..191ca69d7c7 100644 --- a/tests/tools/test_mcp_preflight_content_type.py +++ b/tests/tools/test_mcp_preflight_content_type.py @@ -7,6 +7,16 @@ production probe must run on its own httpx client outside the MCP SDK's anyio task group, and a faithful test must exercise that actual method so the content-type allow-list, HEAD->GET fallback, and best-effort pass-through are all covered as shipped. + +OAuth note +---------- +``MCPServerTask.run()`` skips the preflight entirely when ``auth_type=="oauth"`` +(see ``test_run_skips_preflight_for_oauth``). OAuth-protected MCP servers +return ``200 text/html`` (a login/landing page) on an unauthenticated probe, +which ``_preflight_content_type`` correctly rejects — the probe cannot tell +whether the page is a valid OAuth endpoint or a misconfigured URL. The right +validator for OAuth servers is ``.well-known/oauth-protected-resource``, which +the OAuth handshake consults automatically. """ from __future__ import annotations @@ -206,6 +216,74 @@ def test_head_501_falls_back_to_get_and_passes_json(): # ssl_verify / client_cert forwarding to the probe client # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# OAuth server: why the run() guard is needed +# --------------------------------------------------------------------------- + +def test_oauth_server_html_response_raises_without_skip(): + """_preflight_content_type raises NonMcpEndpointError for 200 text/html. + + This documents the failure mode that the ``self._auth_type != "oauth"`` + guard in ``MCPServerTask.run()`` prevents. An OAuth-protected MCP server + returns a login/landing page on an unauthenticated HEAD probe — identical + to a misconfigured URL from the preflight's point of view — because it + cannot serve a meaningful MCP response without a Bearer token. + + Real-world example: Hospitable's MCP server + (``https://mcp.hospitable.com/mcp``) returns ``200 text/html`` to an + unauthenticated httpx HEAD request. With the guard removed, connecting + via ``hermes mcp add/login`` raises ``NonMcpEndpointError`` before the + OAuth browser flow can begin. With the guard in place, 63 tools are + discovered and the server connects successfully. + """ + task = _make_task("hospitable") + # HEAD returns 200 text/html — what Hospitable sends without a token. + with _serve(_handler(status=200, content_type="text/html; charset=UTF-8")) as base: + with pytest.raises(NonMcpEndpointError) as exc_info: + asyncio.run(task._preflight_content_type(f"{base}/mcp", timeout=5.0)) + assert "hospitable" in str(exc_info.value) + + +def test_run_skips_preflight_for_oauth(monkeypatch): + """MCPServerTask.run() must not call _preflight_content_type for OAuth servers. + + The ``self._auth_type != "oauth"`` guard in the preflight condition ensures + that OAuth-protected servers never hit ``NonMcpEndpointError`` from the + unauthenticated GET probe. The probe is inapplicable to OAuth servers: + their identity is established by the OAuth metadata discovery + (``.well-known/oauth-protected-resource``), not by a GET content-type check. + """ + import tools.mcp_tool as _mcp + + preflight_calls: list[str] = [] + + async def _inner(): + # Patch at the class level: replacement receives (self, url, **kwargs). + async def _fake_preflight(self, url, **kwargs): + preflight_calls.append(url) + + async def _fake_run_http(self, config): + # Abort immediately after the preflight gate — we only want to + # verify the gate, not exercise the real transport. + raise asyncio.CancelledError() + + # Bypass URL validation so the test doesn't need a live network. + monkeypatch.setattr(_mcp, "_validate_remote_mcp_url", lambda n, u: None) + monkeypatch.setattr(_mcp.MCPServerTask, "_preflight_content_type", _fake_preflight) + monkeypatch.setattr(_mcp.MCPServerTask, "_run_http", _fake_run_http) + + task = _mcp.MCPServerTask("hospitable-test") + with pytest.raises(asyncio.CancelledError): + await task.run({"url": "https://mcp.hospitable.com/mcp", "auth": "oauth"}) + + asyncio.run(_inner()) + assert preflight_calls == [], ( + "_preflight_content_type must not be called for OAuth servers; " + "without the guard the OAuth flow is blocked by the 200 text/html " + "landing page the server returns to an unauthenticated probe" + ) + + def test_ssl_verify_and_cert_forwarded(monkeypatch): captured: dict = {} diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 6b73fac7ad4..c125db62a11 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -2307,12 +2307,12 @@ class MCPServerTask: # before surfacing an opaque CancelledError. Probing here — once, # outside the SDK task group — fails fast and non-retryably with # an actionable message, mirroring the URL-validation path above. - # Skip the probe when _ready is already set: that only happens - # after a prior successful connect, so this run() invocation is a - # reconnect (OAuth recovery / manual refresh). The endpoint was - # already validated once; re-probing burns a redundant network - # round-trip against a known-good server on every reconnect. - if config.get("transport") != "sse" and not self._ready.is_set(): + # Skip the probe when _ready is already set (reconnect after a + # prior successful connect) — the endpoint was validated once, + # re-probing is a redundant round-trip. Also skip for OAuth servers: + # without a cached token the endpoint returns HTML or 401, which + # would incorrectly block the OAuth flow before it can run. + if config.get("transport") != "sse" and not self._ready.is_set() and self._auth_type != "oauth": try: _probe_headers = dict(config.get("headers") or {}) await self._preflight_content_type(