Skip to content

Avoid stale context leakage when proxying with an already‑connected ProxyClient#3408

Merged
jlowin merged 2 commits intomainfrom
codex/propose-fix-for-proxyclient-context-vulnerability
Mar 7, 2026
Merged

Avoid stale context leakage when proxying with an already‑connected ProxyClient#3408
jlowin merged 2 commits intomainfrom
codex/propose-fix-for-proxyclient-context-vulnerability

Conversation

@jlowin
Copy link
Member

@jlowin jlowin commented Mar 6, 2026

Motivation

  • Proxy forwarding could bind backend-initiated interactions to a stale request context when a long‑lived ProxyClient session is reused, enabling cross‑client information leakage. This change targets the vulnerable path while keeping existing semantics for other client types.

Description

  • Special-case the client factory selection in _create_client_factory so a connected ProxyClient produces fresh disconnected copies via client.new() instead of reusing the same session.
  • Preserve existing reuse behavior for other connected plain Client instances to avoid broad regressions.
  • Add/adjust a unit test to assert that a connected ProxyClient target yields fresh, disconnected client copies from _create_client_factory.

Testing

  • Ran uv sync successfully.
  • Ran the full test suite with uv run pytest -n auto; the suite executed but the environment produced several unrelated timeouts/failures (11 failed, 4677 passed in this run) that appear orthogonal to this change.
  • Ran focused tests: the new/modified proxy test tests/server/providers/proxy/test_proxy_client.py::TestProxyClient::test_connected_proxy_client_uses_fresh_sessions passed, and a short selection of other tests showed two unrelated failures/timeouts under this environment.
  • uv run prek run --all-files failed to initialize hooks due to an external network clone error.

Codex Task

@jlowin jlowin added the aardvark label Mar 6, 2026
@marvin-context-protocol marvin-context-protocol bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. high-priority labels Mar 6, 2026
@marvin-context-protocol
Copy link
Contributor

marvin-context-protocol bot commented Mar 6, 2026

Test Failure Analysis

Summary: A test in tests/server/auth/test_oauth_consent_page.py timed out (5s limit) on Windows Python 3.10, failing the Tests: Python 3.10 on windows-latest job. This failure is unrelated to this PR's changes.

Note: My previous comment (about prek / ruff-format / ty errors) appears to have been addressed. This is an updated analysis for the new failure.

Root Cause: A test in tests/server/auth/test_oauth_consent_page.py hit pytest's 5-second timeout on Windows. The captured stderr shows an OAuth proxy flow starting (Client registered with redirect_uri: http://localhost:6010/callback) but the test never completed. This file was last changed on 2026-03-03 — days before this PR — and the failing test is in completely unrelated code (OAuth consent page) vs. this PR's changes (ProxyClient session factory in providers/proxy.py).

This looks like a pre-existing Windows CI flakiness: async tests involving localhost network operations or asyncio event loop behavior are known to be sensitive on Windows runners.

Suggested Solution: This failure is not caused by changes in this PR. The test suite passes on Ubuntu (Python 3.10 and 3.13) and the lowest-direct-dependencies jobs. Recommend:

  1. Re-run the failed job to check if this is transient flakiness
  2. If it recurs consistently, the test_oauth_consent_page.py tests may need a @pytest.mark.timeout increase or a Windows-specific skip marker — but that's a separate issue from this PR
Detailed Analysis

Failed job: Tests: Python 3.10 on windows-latest (Job ID: 66146182684)

Timeout location (from log):

tests\server\auth\test_oauth_consent_page.py ........
++++++++++++++++++++++++++++++++++ Timeout +++++++++++++++++++++++++++++++++++++
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Captured stderr ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
DEBUG    Client registered with redirect_uri:  proxy.py:681
         http://localhost:6010/callback - if
         restricting redirect URIs, ensure
         this pattern is allowed in
         allowed_client_redirect_uris
DEBUG    Registered client client-par-a with 1  proxy.py:687

8 tests in that file passed, the 9th timed out at the 5s limit. The test involves an async OAuth consent flow with localhost networking, which can stall on Windows.

This PR's actual changes are confined to:

  • src/fastmcp/server/providers/proxy.py — adds a special-case in _create_client_factory for connected ProxyClient instances
  • tests/server/providers/proxy/test_proxy_client.py — replaces test_connected_client_reuses_sessions with test_connected_proxy_client_uses_fresh_sessions

Neither file has any connection to the OAuth consent page tests.

Passing jobs: Ubuntu Python 3.10, Ubuntu Python 3.13, lowest-direct-dependencies, and integration tests all passed.

Related Files
  • tests/server/auth/test_oauth_consent_page.py — the file where the timeout occurred (last changed 2026-03-03, unrelated to this PR)
  • src/fastmcp/server/providers/proxy.py — this PR's main change (unrelated to the failure)
  • tests/server/providers/proxy/test_proxy_client.py — this PR's test change (unrelated to the failure)

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54cc02cf01

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

)

def fresh_client_factory() -> Client:
return client.new()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Ensure fresh ProxyClient copies are disconnected

_create_client_factory now treats connected ProxyClient targets as safe by returning client.new(), but Client.new() does not reset session state for StdioTransport (src/fastmcp/client/client.py:405-408), so those copies stay connected and share the same underlying session. In the stdio-backed case, this branch still reuses the original receive loop and can leak stale request context across proxied requests despite the new safeguard, so the fix is bypassed for a supported transport type.

Useful? React with 👍 / 👎.

@jlowin
Copy link
Member Author

jlowin commented Mar 7, 2026

Auto-reviewed and merging on behalf of @jlowin — CI is green (Windows OAuth proxy timeouts are pre-existing flaky tests).

@jlowin jlowin merged commit 163c168 into main Mar 7, 2026
6 of 7 checks passed
@jlowin jlowin deleted the codex/propose-fix-for-proxyclient-context-vulnerability branch March 7, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. codex high-priority server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant