Skip to content

fix(mcp): scope oauth callback ports per provider - #34280

Closed
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix-34260-mcp-oauth-scoped-port
Closed

fix(mcp): scope oauth callback ports per provider#34280
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix-34260-mcp-oauth-scoped-port

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • bind MCP OAuth redirect/callback handlers to each provider's resolved port instead of the mutable module global
  • keep the legacy _oauth_port for compatibility, but stop using it as the live dispatch source once the provider is built
  • add regression tests proving two providers keep distinct redirect hints and callback bind ports even after the global is overwritten

Testing

  • uv run --frozen pytest -q -o addopts='' tests/tools/test_mcp_oauth.py -k "per_provider or RedirectHandlerSshHint or WaitForCallbackNoBlocking or configure_callback_port or build_oauth_auth_preserves_server_url_path"
  • uv run --frozen pytest -q -o addopts='' tests/tools/test_mcp_oauth_manager.py tests/tools/test_mcp_oauth.py -k "manager or per_provider or port_stored_globally or returns_oauth_provider or scope_passed_through or pre_registered_client_id_stored"
  • uv run --frozen ruff check tools/mcp_oauth.py tools/mcp_oauth_manager.py tests/tools/test_mcp_oauth.py
  • git diff --check

Refs #34260

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth labels May 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the per-provider callback-port issue. The current-main premise is still valid: both construction paths overwrite _oauth_port and install shared handlers (tools/mcp_oauth.py:937-946; tools/mcp_oauth_manager.py:544-555).

Problems

  • This May patch predates 755194ffe9a117c70a81505ecf4cade1fbc5af26. Current main's handlers reject non-interactive authorization before browser launch (tools/mcp_oauth.py:557) and before callback binding (tools/mcp_oauth.py:643). The new factory closures in this diff duplicate the handler bodies without those checks, then replace the provider callbacks with the unguarded closures. A direct salvage would regress the [Bug]: Headless MCP OAuth blocks gateway startup with stale cached tokens #57836 boundary contract.
  • The added regressions cover build_oauth_auth, but not the changed manager callback wiring. Current manager coverage at tests/tools/test_mcp_oauth_manager.py:240-255 only verifies the provider subclass.

Suggested changes

  • Preserve the two current guard checks inside the port-bound factories and test that the bound callbacks neither open a browser nor bind HTTPServer in non-interactive mode.
  • Add a two-provider manager-path regression alongside the direct-construction regression.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
teknium1 added a commit that referenced this pull request Jul 16, 2026
…cross ports

_wait_for_callback still read the legacy module-level _oauth_port, so
with two concurrent OAuth flows, flow A's callback wait bound flow B's
port while A's redirect URI pointed at A's port — the callback-side
half of the cross-flow collision that #65622 fixed on the redirect
side. _make_callback_waiter(port) closes over each flow's resolved
port; both provider construction sites (build_oauth_auth and
MCPOAuthManager._build_provider) now wire per-flow waiters. The legacy
_wait_for_callback delegates for backwards compatibility.

Direction credit to @LeonSGP43 (#34280) and the #34260 analysis.
teknium1 added a commit that referenced this pull request Jul 16, 2026
…cross ports

_wait_for_callback still read the legacy module-level _oauth_port, so
with two concurrent OAuth flows, flow A's callback wait bound flow B's
port while A's redirect URI pointed at A's port — the callback-side
half of the cross-flow collision that #65622 fixed on the redirect
side. _make_callback_waiter(port) closes over each flow's resolved
port; both provider construction sites (build_oauth_auth and
MCPOAuthManager._build_provider) now wire per-flow waiters. The legacy
_wait_for_callback delegates for backwards compatibility.

Direction credit to @LeonSGP43 (#34280) and the #34260 analysis.
@teknium1

Copy link
Copy Markdown
Contributor

Resolved on main via PR #65664 (commit d34cc40), which implements the same per-flow callback-waiter direction your PR and your #34260 analysis called for — _make_callback_waiter(port) closures at both provider construction sites, with the legacy _wait_for_callback kept as a compat shim. Your branch predated the reserved-socket TOCTOU pool and the #57836 non-interactive guard so it couldn't be cherry-picked cleanly, but the direction credit is yours in the merged PR body. #34260 is closed by the merge. Thanks @LeonSGP43 — that issue writeup was the best map of this bug cluster anyone produced.

@teknium1 teknium1 closed this Jul 16, 2026
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…cross ports

_wait_for_callback still read the legacy module-level _oauth_port, so
with two concurrent OAuth flows, flow A's callback wait bound flow B's
port while A's redirect URI pointed at A's port — the callback-side
half of the cross-flow collision that NousResearch#65622 fixed on the redirect
side. _make_callback_waiter(port) closes over each flow's resolved
port; both provider construction sites (build_oauth_auth and
MCPOAuthManager._build_provider) now wire per-flow waiters. The legacy
_wait_for_callback delegates for backwards compatibility.

Direction credit to @LeonSGP43 (NousResearch#34280) and the NousResearch#34260 analysis.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…cross ports

_wait_for_callback still read the legacy module-level _oauth_port, so
with two concurrent OAuth flows, flow A's callback wait bound flow B's
port while A's redirect URI pointed at A's port — the callback-side
half of the cross-flow collision that NousResearch#65622 fixed on the redirect
side. _make_callback_waiter(port) closes over each flow's resolved
port; both provider construction sites (build_oauth_auth and
MCPOAuthManager._build_provider) now wire per-flow waiters. The legacy
_wait_for_callback delegates for backwards compatibility.

Direction credit to @LeonSGP43 (NousResearch#34280) and the NousResearch#34260 analysis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants