Python: fail fast on MCP streamable HTTP 401/403#13458
Python: fail fast on MCP streamable HTTP 401/403#13458VedantMadane wants to merge 4 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
VedantMadane
force-pushed
the
fix/mcp-401-403
branch
from
cca6e0b to
32d5f51
Compare
This prevents connect() from hanging indefinitely when session creation or initialization fails. Part of addressing microsoft#13414.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✗ Correctness
The
ready_event.set()additions in_inner_connectare correct and fix a real deadlock whereconnect()would hang forever if session creation or initialization failed. However, the newMCPStreamableHttpPlugin.__aenter__preflight check has two significant issues: (1) it breaks at least three existing tests that useMCPStreamableHttpPluginwithout mockinghttpx, and (2) a plain HTTP GET to an MCP streamable HTTP endpoint is not guaranteed to return 200 — MCP uses POST for JSON-RPC, and a GET may return 405 Method Not Allowed or other non-200 codes — causingraise_for_status()to incorrectly reject perfectly valid servers.
✗ Security Reliability
The PR adds two fixes: (1)
ready_event.set()calls before raising in_inner_connectexception handlers to prevent deadlocks when session creation/initialization fails — these are clear reliability improvements. (2) A preflight HTTP GET check inMCPStreamableHttpPlugin.__aenter__to detect 401/403 early. The preflight has a reliability concern: it usesresponse.raise_for_status()which will reject servers that return non-2xx for a bare GET (e.g., 405 Method Not Allowed), even though the actual MCP protocol POST would succeed. Additionally, the preflight constructs its ownhttpx.AsyncClientusing onlyself.headers, but doesn't forward_client_kwargs— so if users pass authentication via theauthparameter (httpx.Auth) or a customhttpx_client_factory, the preflight will fail with 401/403 even though the actual connection would work. Theready_event.set()fixes and the httpx dependency (already a transitive dep of mcp) are sound.
✗ Test Coverage
The PR adds two changes: (1) a preflight HTTP auth check in
MCPStreamableHttpPlugin.__aenter__and (2)ready_event.set()calls in two additional exception handlers in_inner_connect. While two new tests cover the preflight check for 401/403 and 200 success, three existing tests that useMCPStreamableHttpPlugin(test_mcp_plugin_session_not_initialize,test_mcp_plugin_session_initialized,test_with_kwargs_streamablehttp) do not mockhttpx.AsyncClientand will now attempt real HTTP requests, causing them to fail. Additionally, theready_event.set()bug fix in the session-creation and session-initialization failure paths has zero test coverage, and the preflightraise_for_status()path for non-401/403 HTTP errors (e.g., 500) is not tested either.
✗ Design Approach
This PR contains two separate changes bundled together. The
ready_event.set()additions in_inner_connectare a legitimate deadlock fix — without them, ifClientSession.__aenter__orsession.initialize()raise,connect()hangs forever waiting on an event that never gets set. However, the preflight GET request added toMCPStreamableHttpPlugin.__aenter__is a fundamentally flawed design:streamablehttp_client(and its successorstreamable_http_client) communicates via HTTP POST, not GET. A 200 response to a GET on the MCP endpoint provides no guarantee that the subsequent POST-based MCP handshake will succeed with the same auth headers. Worse,response.raise_for_status()inside a broadexcept Exceptionconverts transient failures (503, 429) and wrong-URL failures (404, 405) into misleadingKernelPluginInvalidConfigurationErrormessages. It also introduces a TOCTOU race (auth valid at preflight may expire before actual connection), adds a redundant round-trip, and only applies toMCPStreamableHttpPluginwhileMCPSSEPluginhas the same auth-error surface. The right approach is to let auth errors fromstreamablehttp_clientpropagate through_inner_connect's existing catch blocks (already fixed by theready_event.set()changes), and optionally inspect the exception type/message there to give more specific guidance — no extra HTTP request needed.
Flagged Issues
- The preflight GET in
__aenter__breaks existing teststest_mcp_plugin_session_not_initialize(parametrized with MCPStreamableHttpPlugin),test_mcp_plugin_session_initialized(parametrized with MCPStreamableHttpPlugin), andtest_with_kwargs_streamablehttp— none of which mockhttpx.AsyncClient, so the preflight will attempt real network requests and fail. - The preflight uses GET, but MCP streamable HTTP communicates via POST. A GET to the MCP endpoint tests a different handler/route: a valid MCP server may return 404 or 405 for GET while working perfectly for POST, and a server returning 200 for GET may still reject POST with 401/403. Combined with
raise_for_status()inside a broadexcept Exception, ALL non-2xx responses — including transient errors (503, 429) and method-not-allowed (405) — are converted intoKernelPluginInvalidConfigurationError('check your configuration'), which is incorrect and actively misleading. - The preflight
httpx.AsyncClientonly receivesself.headersbut does not forward theauthkwarg from_client_kwargs. Users authenticating viaauth=httpx.BasicAuth(...)will always hit spurious 401/403 on the preflight, blocking a valid connection. - The
ready_event.set()additions in the session-creation and session-initialization failure handlers (lines 320 and 328) have no test coverage. These are important deadlock-prevention fixes and should have dedicated tests exercising each failure path. - The preflight introduces a TOCTOU race: a token valid at preflight time may expire before the actual MCP connection, so the check provides no real guarantee and the caller still fails at the true connection point — which is already handled by
_inner_connect's catch blocks (now correctly fixed byready_event.set()).
Suggestions
- Consider landing the
ready_event.set()deadlock fixes independently of the preflight feature — those alone resolve the hang that presumably motivated this PR, and auth errors will now propagate correctly through_inner_connect. - Consider removing the
__aenter__preflight override entirely. The_inner_connectpathway already surfaces all connection/auth failures; if better auth error messaging is desired, catch auth-specific exceptions (e.g.,httpx.HTTPStatusErrorwith 401/403) inside_inner_connect's existing try/except blocks and raiseKernelPluginInvalidConfigurationErrorwith targeted messaging — no extra HTTP request needed. - If a preflight check is retained, only gate on 401/403 status codes (the stated purpose) and do not call
raise_for_status(). Also skip the preflight whenself.sessionis already provided, since no transport check is needed in that case. - Forward relevant kwargs (at minimum
auth) from_client_kwargsto the preflight httpx client to ensure consistent authentication behavior. - If a health-check preflight is genuinely desired in future, it should use the same HTTP method (POST) and headers as the real MCP handshake, and should not re-wrap transient HTTP errors as configuration errors.
- Add tests for the
ready_event.set()failure paths (mockClientSession(...)to raise andsession.initialize()to raise) and for the preflightraise_for_status()code path (e.g., simulate a 500 response).
Automated review by moonbox3's agents
| @@ -760,6 +763,27 @@ def get_mcp_client(self) -> _AsyncGeneratorContextManager[Any, None]: | |||
| args.update(self._client_kwargs) | |||
| return streamablehttp_client(**args) | |||
|
|
|||
There was a problem hiding this comment.
This preflight GET uses the wrong HTTP method (streamablehttp_client uses POST) and runs unconditionally — even when self.session is already provided, breaking existing tests that supply a mock session without mocking httpx.AsyncClient. Consider removing the preflight entirely (auth errors now propagate via _inner_connect thanks to the ready_event.set() fixes) or at minimum guarding with if not self.session:.
| async def __aenter__(self) -> Self: | |
| """Fail fast on authentication/authorization errors before connecting.""" | |
| if not self.session: | |
| timeout = self.timeout if self.timeout is not None else 30 | |
| try: | |
| async with httpx.AsyncClient(timeout=timeout, headers=self.headers) as client: | |
| response = await client.get(self.url) | |
| if response.status_code in (httpx.codes.UNAUTHORIZED, httpx.codes.FORBIDDEN): | |
| raise KernelPluginInvalidConfigurationError( | |
| f"Failed to connect to the MCP server: received HTTP {response.status_code} (unauthorized/forbidden)." | |
| ) | |
| except KernelPluginInvalidConfigurationError: | |
| raise | |
| except Exception as ex: # pragma: no cover - guarded for unexpected failures | |
| raise KernelPluginInvalidConfigurationError( | |
| "Failed to connect to the MCP server. Please check your configuration." | |
| ) from ex |
| ) | ||
| # Raise for other HTTP errors to surface configuration/network issues early. |
There was a problem hiding this comment.
The preflight httpx.AsyncClient only receives self.headers, but users can pass auth=httpx.BasicAuth(...) via **kwargs (stored in _client_kwargs). The preflight won't include that auth, causing spurious 401/403 failures. Forward the auth kwarg (and potentially other relevant kwargs) to the preflight client.
| ) | |
| # Raise for other HTTP errors to surface configuration/network issues early. | |
| client_kwargs: dict[str, Any] = {"timeout": timeout, "headers": self.headers} | |
| if "auth" in self._client_kwargs: | |
| client_kwargs["auth"] = self._client_kwargs["auth"] | |
| try: | |
| async with httpx.AsyncClient(**client_kwargs) as client: |
| response.raise_for_status() | ||
| except KernelPluginInvalidConfigurationError: | ||
| raise | ||
| except Exception as ex: # pragma: no cover - guarded for unexpected failures |
There was a problem hiding this comment.
raise_for_status() rejects any non-2xx response (405, 404, 503, 429, etc.). Since the MCP spec does not mandate 200 OK for a plain GET on a streamable HTTP endpoint, this produces false negatives for valid MCP servers and misleadingly wraps transient errors as configuration errors. The preflight should only gate on 401/403 (its stated purpose).
| except Exception as ex: # pragma: no cover - guarded for unexpected failures | |
| # Only block on authentication errors; other HTTP status codes | |
| # will be surfaced during the actual MCP connection. |
| ) | ||
| except Exception as ex: | ||
| await self._exit_stack.aclose() | ||
| ready_event.set() |
There was a problem hiding this comment.
This ready_event.set() fix prevents connect() from hanging when session creation fails. Add a test that mocks ClientSession(...) to raise and asserts that KernelPluginInvalidConfigurationError is raised (rather than a hang).
| await session.initialize() | ||
| except Exception as ex: | ||
| await self._exit_stack.aclose() | ||
| ready_event.set() |
There was a problem hiding this comment.
Same as above — this ready_event.set() fix for initialization failure also needs a test. Mock session.initialize() to raise and assert proper error propagation.
| @patch("semantic_kernel.connectors.mcp.streamablehttp_client") | ||
| async def test_streamable_http_raises_on_401_403(mock_streamable_client, mock_session, monkeypatch, status_code): | ||
| # Avoid real network: preflight should catch unauthorized/forbidden before streamable client is used. | ||
| monkeypatch.setattr("semantic_kernel.connectors.mcp.httpx.AsyncClient", lambda **_: _DummyHttpxClient(status_code)) |
There was a problem hiding this comment.
The monkeypatch fixture must appear before the @patch parameters in the function signature. With @patch decorators, mock arguments are injected in reverse decorator order (bottom-up), so the correct order is (mock_session, mock_streamable_client, monkeypatch, status_code). As written, mock_streamable_client receives the ClientSession mock and vice versa, swapping their intended assignments.
| @patch("semantic_kernel.connectors.mcp.streamablehttp_client") | |
| async def test_streamable_http_raises_on_401_403(mock_streamable_client, mock_session, monkeypatch, status_code): | |
| # Avoid real network: preflight should catch unauthorized/forbidden before streamable client is used. | |
| monkeypatch.setattr("semantic_kernel.connectors.mcp.httpx.AsyncClient", lambda **_: _DummyHttpxClient(status_code)) | |
| async def test_streamable_http_raises_on_401_403(mock_session, mock_streamable_client, monkeypatch, status_code): |
| monkeypatch.setattr("semantic_kernel.connectors.mcp.httpx.AsyncClient", lambda **_: _DummyHttpxClient(status_code)) | ||
|
|
||
| with pytest.raises(KernelPluginInvalidConfigurationError): | ||
| async with MCPStreamableHttpPlugin( |
There was a problem hiding this comment.
Good coverage for 401/403, but the raise_for_status() path for other server errors (e.g., 500, 502) is not tested. Add a parametrized case with a 5xx status code to verify behavior.
|
Addressed the automated review feedback:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Python MCP Streamable HTTP connector to surface connection/authentication problems earlier by adding an HTTP preflight during context-manager entry and by preventing connect() from hanging when session setup fails.
Changes:
- Added a preflight
httpxrequest inMCPStreamableHttpPlugin.__aenter__to fail early on HTTP 401/403. - Ensured
ready_eventis set when session creation/initialization fails to avoidconnect()waiting forever. - Added unit tests for 401/403 behavior, success behavior, skipping preflight when an existing session is provided, and non-hanging failure cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| python/semantic_kernel/connectors/mcp.py | Adds httpx preflight in __aenter__ and unblocks connect() by setting ready_event on more failure paths. |
| python/tests/unit/connectors/mcp/test_mcp.py | Adds unit tests and a dummy httpx.AsyncClient to validate preflight behavior and non-hanging failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| timeout = self.timeout if self.timeout is not None else 30 | ||
| client_kwargs: dict[str, Any] = {"timeout": timeout, "headers": self.headers} | ||
| if "auth" in self._client_kwargs: | ||
| client_kwargs["auth"] = self._client_kwargs["auth"] | ||
| try: | ||
| async with httpx.AsyncClient(**client_kwargs) as client: | ||
| response = await client.get(self.url) |
There was a problem hiding this comment.
The preflight timeout is passed directly into httpx.AsyncClient. In this repo, MCPStreamableHttpPlugin is used with timeout=timedelta(...) (e.g., the OAuth sample), which httpx does not accept as a timeout value. Consider normalizing self.timeout to a value compatible with httpx (e.g., convert timedelta to seconds or build an httpx.Timeout) before constructing the client so preflight doesn’t break existing callers.
| response = await client.get(self.url) | ||
| if response.status_code in (httpx.codes.UNAUTHORIZED, httpx.codes.FORBIDDEN): | ||
| raise KernelPluginInvalidConfigurationError( | ||
| f"Failed to connect to the MCP server: received HTTP {response.status_code} (unauthorized/forbidden)." | ||
| ) |
There was a problem hiding this comment.
The PR description says non-success HTTP responses should be raised early, but the preflight currently only errors on 401/403 and silently proceeds on other error codes (e.g., 404/500). Consider treating any non-2xx response as a configuration/connectivity error (while still providing a tailored message for 401/403).
| response = await client.get(self.url) | ||
| if response.status_code in (httpx.codes.UNAUTHORIZED, httpx.codes.FORBIDDEN): | ||
| raise KernelPluginInvalidConfigurationError( | ||
| f"Failed to connect to the MCP server: received HTTP {response.status_code} (unauthorized/forbidden)." | ||
| ) |
There was a problem hiding this comment.
The preflight uses client.get(self.url), which by default reads the full response body. For a “streamable” endpoint this can block until timeout if the server keeps the response open. Consider issuing the request in streaming mode (so you only read headers/status) and closing the response immediately after checking status_code, to avoid unnecessary latency/hangs during preflight.
| response = await client.get(self.url) | |
| if response.status_code in (httpx.codes.UNAUTHORIZED, httpx.codes.FORBIDDEN): | |
| raise KernelPluginInvalidConfigurationError( | |
| f"Failed to connect to the MCP server: received HTTP {response.status_code} (unauthorized/forbidden)." | |
| ) | |
| async with client.stream("GET", self.url) as response: | |
| if response.status_code in (httpx.codes.UNAUTHORIZED, httpx.codes.FORBIDDEN): | |
| raise KernelPluginInvalidConfigurationError( | |
| f"Failed to connect to the MCP server: received HTTP {response.status_code} (unauthorized/forbidden)." | |
| ) |
| ) | ||
| ) | ||
| except Exception as ex: | ||
| await self._exit_stack.aclose() | ||
| ready_event.set() | ||
| raise KernelPluginInvalidConfigurationError( | ||
| "Failed to create a session. Please check your configuration." | ||
| ) from ex | ||
| try: | ||
| await session.initialize() | ||
| except Exception as ex: | ||
| await self._exit_stack.aclose() | ||
| ready_event.set() | ||
| raise KernelPluginInvalidConfigurationError( | ||
| "Failed to initialize session. Please check your configuration." | ||
| ) from ex |
There was a problem hiding this comment.
Setting ready_event on session creation/initialization failure prevents connect() from hanging, but it also lets connect()/__aenter__ return successfully even though _inner_connect has already failed. That can cause the async with body to run with plugin.session unset and only raise later when close() awaits the failed task. Consider having connect() detect when _current_task has completed with an exception (e.g., if self._current_task.done(): await self._current_task) after ready_event.wait(), so failures surface immediately on enter.
| @patch("semantic_kernel.connectors.mcp.streamablehttp_client") | ||
| async def test_streamable_http_session_create_failure_does_not_hang(mock_streamable_client): | ||
| """ready_event.set() prevents connect() from hanging when session creation fails.""" | ||
| mock_streamable_client.side_effect = RuntimeError("connection refused") | ||
|
|
||
| with pytest.raises(KernelPluginInvalidConfigurationError, match="create a session"): | ||
| async with MCPStreamableHttpPlugin( | ||
| name="TestMCPPlugin", | ||
| description="Test MCP Plugin", | ||
| url="http://localhost:8080/mcp", | ||
| ): | ||
| pass |
There was a problem hiding this comment.
This test sets streamablehttp_client.side_effect to raise, which fails during transport/client creation and should map to the connector error message about failing to connect to the MCP server (not “create a session”). The match="create a session" assertion is likely incorrect and may cause a false negative.
| mock_generator.__aenter__.return_value = (mock_read, mock_write, mock_callback) | ||
| mock_generator.__aexit__.return_value = (mock_read, mock_write, mock_callback) | ||
| mock_streamable_client.return_value = mock_generator | ||
| mock_session.return_value.initialize = AsyncMock(side_effect=RuntimeError("init failed")) |
There was a problem hiding this comment.
session.initialize() is awaited on the entered ClientSession instance (the object returned from ClientSession().__aenter__() via AsyncExitStack.enter_async_context). This test sets initialize on mock_session.return_value, which may not be the same object, making the failure injection brittle. Prefer setting the side_effect on mock_session.return_value.__aenter__.return_value.initialize to ensure the initialization path actually raises.
| mock_session.return_value.initialize = AsyncMock(side_effect=RuntimeError("init failed")) | |
| mock_session.return_value.__aenter__.return_value.initialize = AsyncMock( | |
| side_effect=RuntimeError("init failed") | |
| ) |
- Guard preflight check with `if not self.session` so existing-session callers skip the HTTP probe entirely - Forward `auth` from `_client_kwargs` to the preflight httpx client - Remove `raise_for_status()` — only gate on 401/403 as originally intended; other status codes propagate via the MCP connection itself - Add tests for `ready_event.set()` fixes (session creation failure and `session.initialize()` failure no longer hang) - Add test verifying preflight is skipped when a session is provided
Summary
This PR improves the Python MCP Streamable HTTP connector to fail fast on authentication / authorization issues.
MCPStreamableHttpPlugin.__aenter__and raisesKernelPluginInvalidConfigurationErroron HTTP 401/403.Why
When credentials or permissions are misconfigured, the server returns 401/403 and the plugin should error clearly and immediately, instead of failing later during session setup.
Tests
httpx.AsyncClient:401/403raisesKernelPluginInvalidConfigurationError200proceeds successfullyRelated
microsoft/semantic-kernel#13414