Skip to content

Bind MCP OAuth handlers per provider - #44607

Closed
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:her-275-44588-oauth-flow-closures
Closed

Bind MCP OAuth handlers per provider#44607
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:her-275-44588-oauth-flow-closures

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • bind MCP OAuth redirect and callback handlers to each provider's resolved callback port
  • keep the legacy module-level wrappers for backward compatibility
  • add focused regressions for the factory closures and manager wiring

Closes #44588

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — reviewed this PR's diff (per-provider OAuth handler closures for MCP).

What I checked:

  • _make_redirect_handler(callback_port) and _make_wait_for_callback(callback_port) correctly close over the port at factory creation time, replacing the mutable module-level _oauth_port
  • Back-compat wrappers _redirect_handler and _wait_for_callback delegate to the factory with _oauth_port as fallback
  • build_oauth_auth now passes cfg["_resolved_port"] to the factories instead of relying on the module global
  • _build_provider in mcp_oauth_manager.py imports the new factory functions
  • Tests verify closed-over port is used (not the module-level _oauth_port), and that the manager passes per-server handlers to the provider

Verdict: Clean. The closure-over-port pattern correctly prevents concurrent OAuth flows from interfering with each other's SSH hint messages and callback listeners.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools duplicate This issue or pull request already exists labels Jun 12, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #34280 (earlier open PR, same approach) — bind MCP OAuth redirect/callback handlers per provider via factory closures instead of module-level globals. Same root cause as #34260/#5344. Closes #44588.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the mutable callback-port race with per-provider closures. Current main still has that race: _configure_callback_port() writes _oauth_port at tools/mcp_oauth.py:828-832, and both construction paths pass global-reading handlers (tools/mcp_oauth.py:945-946; tools/mcp_oauth_manager.py:544-555).

Problems

  • The new redirect factory starts at tools/mcp_oauth.py:402 without current main's _raise_if_non_interactive(...) boundary guard (tools/mcp_oauth.py:557).
  • The callback factory binds at tools/mcp_oauth.py:464 without current main's pre-bind guard (tools/mcp_oauth.py:643). Those protections were added in 755194ffe9a117c70a81505ecf4cade1fbc5af26; applying this June snapshot would regress background OAuth fail-fast behavior.

Suggested changes

  • Salvage the closures into the current guarded handlers, keeping both guards inside the factory-produced handlers.
  • Update the closure tests to assert no URL/browser/listener activity for non-interactive flows, alongside the closed-over-port assertions.

Automated hermes-sweeper review.

Comment thread tools/mcp_oauth.py
file=sys.stderr,
def _make_redirect_handler(callback_port: int | None):
"""Return a redirect handler bound to one OAuth flow's callback port."""
async def _handler(authorization_url: str) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please preserve current main's _raise_if_non_interactive(...) guard inside this factory before URL output or browser launch. The equivalent current handler guards at tools/mcp_oauth.py:557 (commit 755194ffe); omitting it reintroduces background OAuth flows that cannot be completed.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Resolved on main via PR #65622, which merged the same per-provider closure approach (salvaged from #44872, with #5345 by @caseyg as the earliest submission — all credited). Thanks for the contribution @LeonSGP43!

@teknium1 teknium1 closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

OAuth redirect_handler/callback_handler use shared globals, causing cross-server state pollution

4 participants