Skip to content

fix(mcp-oauth): per-server callback ports + reuse registered redirect URI - #5345

Closed
caseyg wants to merge 1 commit into
NousResearch:mainfrom
caseyg:fix/mcp-oauth-port-collision
Closed

fix(mcp-oauth): per-server callback ports + reuse registered redirect URI#5345
caseyg wants to merge 1 commit into
NousResearch:mainfrom
caseyg:fix/mcp-oauth-port-collision

Conversation

@caseyg

@caseyg caseyg commented Apr 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

  • Replace shared _oauth_port global with per-server ports via closure, fixing "Address already in use" when multiple MCP servers do OAuth concurrently
  • Reuse the port from existing client registrations so redirect URIs match across restarts, fixing "invalid_grant: Invalid redirect URI" errors

Fixes #5344

Context

PR #2552 fixed the original port mismatch (build vs callback using different ports) by introducing a module-level _oauth_port global. However, when multiple OAuth-backed MCP servers initialize concurrently (common on startup), each build_oauth_auth() call overwrites the global — all flows compete for the same port.

Additionally, each restart picked a fresh random port, but the OAuth provider (e.g. Notion) still had the old port in the registered client's redirect URI, causing token exchange failures.

Test plan

  • Configure 2+ OAuth MCP servers (e.g. Notion, Todoist, Clay)
  • Delete cached tokens to force re-auth: rm ~/.hermes/mcp-tokens/*.json
  • Start Hermes — verify no "Address already in use" errors, all OAuth flows complete
  • Restart Hermes — verify no "Invalid redirect URI" errors, cached tokens + client registrations work

🤖 Generated with Claude Code

@caseyg @claude
fix(mcp-oauth): per-server callback ports + reuse registered redirect…
cf28dbf 
… URI

Two bugs in the OAuth flow caused reliable failures with multiple MCP servers:

1. All OAuth flows shared a single global `_oauth_port`, so concurrent
   flows (common on startup) all tried to bind the same port — every
   flow after the first got "Address already in use".

2. Each restart picked a new random port, but the OAuth client was
   already registered with the provider using the old port's redirect
   URI, causing "invalid_grant: Invalid redirect URI" on token exchange.

Fix: replace the shared global with per-server ports via closure
(`_make_wait_for_callback(port)`), and reuse the port from an existing
client registration before falling back to a new random port.

Fixes NousResearch#5344

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jvg123 jvg123 mentioned this pull request Apr 6, 2026
Closed
7 tasks
jvg123 added a commit to jvg123/hermes-agent that referenced this pull request Apr 6, 2026
@jvg123
fix(tools): fix MCP OAuth token serialization and cached token refresh
8aa72ec 
Two fixes for MCP OAuth in tools/mcp_oauth.py:

1. AnyUrl serialization: model_dump(mode='json') in set_tokens() and
   set_client_info() to handle Pydantic AnyUrl objects that aren't
   JSON-serializable by default. Without this, token storage fails on
   first OAuth connection with TypeError.

2. Cached token refresh (critical): get_tokens() now checks if the
   access token has expired by comparing file mtime + expires_in against
   current time. If expired, clears access_token but keeps refresh_token,
   so the MCP SDK uses the refresh flow instead of sending an expired
   token, getting 401, and falling through to full browser auth.

   Without this fix, any non-interactive deployment (gateway, Docker,
   cron) loses MCP connectivity after ~1 hour and cannot recover, even
   with a valid refresh token.

Note: redirect URI port reuse is addressed separately in NousResearch#5345.

Tests added for both fixes.
@alt-glitch alt-glitch mentioned this pull request Apr 27, 2026
Closed
@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 labels May 1, 2026
@alt-glitch alt-glitch mentioned this pull request May 1, 2026
Closed
@alt-glitch alt-glitch mentioned this pull request May 20, 2026
Closed
1 task
@alt-glitch alt-glitch mentioned this pull request Jun 12, 2026
Closed
9 tasks
@teknium1

teknium1 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Thanks for identifying the shared-port and stale-registration failure modes. The premise is still present on current main, but this patch needs adaptation to the MCP OAuth manager path.

Problems

  • Runtime MCP setup calls get_manager().get_or_build_provider() in tools/mcp_tool.py:2364-2367, while this PR changes only legacy build_oauth_auth(). The manager still calls _configure_callback_port() and injects global-dependent handlers at tools/mcp_oauth_manager.py:544-555.
  • _configure_callback_port() still assigns the module-global _oauth_port (tools/mcp_oauth.py:828-832), _wait_for_callback() binds that global (tools/mcp_oauth.py:655), and _redirect_handler() reads it for SSH guidance (tools/mcp_oauth.py:575-590). A callback-only closure would leave the runtime path and redirect-side state leak unfixed.
  • Current tests retain a legacy-global assertion at tests/tools/test_mcp_oauth.py:409-423; no test covers two manager-built providers or cached registration port reuse.

Suggested changes

  • Thread one resolved port through manager-built client metadata, redirect handler, and callback handler; use persisted client info only when no explicit redirect_port is configured.
  • Add manager-path regression coverage for concurrent distinct-server flows and restart-time registered-port reuse.

This is an automated hermes-sweeper review.

@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 12, 2026
@alt-glitch alt-glitch mentioned this pull request Jul 14, 2026
Closed
3 tasks
@teknium1 teknium1 mentioned this pull request Jul 16, 2026
Merged
@teknium1

teknium1 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Resolved on main via PR #65622. Your PR was the EARLIEST submission of the per-provider closure approach for the _oauth_port cross-server pollution — submitted back in April, well before the cluster that followed (#44607, #44685, #44872). We salvaged #44872's implementation because its branch was the most current with tests, but you called the right fix first and are credited in the merged PR body. Thanks @caseyg!

@teknium1 teknium1 closed this Jul 16, 2026
@teknium1 teknium1 mentioned this pull request Jul 16, 2026
Closed
@caseyg

caseyg commented Jul 16, 2026

Copy link
Copy Markdown
Author

That's so cool. Thanks for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/auth Authentication, OAuth, credential pools 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MCP OAuth: port collision on concurrent auth + redirect URI mismatch on restart

3 participants

@caseyg @teknium1 @alt-glitch