mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-01 12:02:05 +00:00
fix(mcp): skip preflight content-type probe for OAuth servers
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
This commit is contained in:
parent
9d919daf44
commit
d05cc8f4d6
2 changed files with 84 additions and 6 deletions
|
|
@ -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 = {}
|
||||
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue