Skip to content

fix(mcp): reuse cached oauth redirect port on re-auth - #49249

Closed
x9x9x9x9x9x91 wants to merge 1 commit into
NousResearch:mainfrom
x9x9x9x9x9x91:fix/mcp-oauth-reuse-cached-port
Closed

fix(mcp): reuse cached oauth redirect port on re-auth#49249
x9x9x9x9x9x91 wants to merge 1 commit into
NousResearch:mainfrom
x9x9x9x9x9x91:fix/mcp-oauth-reuse-cached-port

Conversation

@x9x9x9x9x9x91

Copy link
Copy Markdown
Contributor

Problem

On MCP OAuth re-authentication, the callback server binds a new random free port each time (_find_free_port()), while reusing the stored dynamic-client-registration client_id. Many providers pin the redirect URI(s) to the values registered at first auth and reject a callback on a different port (invalid_redirect_uri / no matching registered URI), so re-auth against a cached client registration can fail even though the client_id is still valid.

Fix

When a cached client registration exists, reuse the redirect port parsed from the stored registration instead of picking a fresh random one:

  • Adds _cached_redirect_port(storage) — returns the loopback callback port from the cached client registration's redirect URI, or None if absent/unparseable.
  • The callback server and redirect_uri then share the previously-registered port, so the callback matches a registered URI on re-auth.
  • Falls back to a fresh free port when there is no cached registration (first-auth behavior unchanged).

Test

Extends tests/tools/test_mcp_oauth.py to cover the cached-port reuse path and the no-cache fallback. 69 passed.

Note: this touches the same mcp_oauth.py area as the recent OAuth hardening (#2552); rebased clean on current main.

@alt-glitch alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jun 19, 2026

@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 tracing both OAuth provider-construction paths. The current-main premise is valid: tools/mcp_oauth.py:830 and tools/mcp_oauth_manager.py:544 each allocate a fresh port for redirect_port: 0.

Problems

  • tools/mcp_oauth.py:165 accepts localhost, but _build_client_metadata() always reconstructs http://127.0.0.1:<port>/callback (tools/mcp_oauth.py:849). A cached localhost registration therefore still gets a different redirect URI. Preserve the cached authority or limit this reuse path to 127.0.0.1, with a regression test.
  • The new fallback helper should also make malformed cache shapes and invalid URI ports fall through to fresh-port allocation; its data.get(...) and parsed.port accesses are outside the guarded parse operation. Add focused negative tests.

Suggested changes

  • Add localhost and malformed-cache coverage alongside the existing cached-127.0.0.1 test.

Automated hermes-sweeper review.

Comment thread tools/mcp_oauth.py
continue
if (
parsed.scheme == "http"
and parsed.hostname in {"127.0.0.1", "localhost"}

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.

This accepts a cached localhost URI, but the later metadata builder always emits http://127.0.0.1:<port>/callback. That still changes the registered URI on re-auth. Please preserve the cached authority or restrict this branch to canonical 127.0.0.1, and add a localhost regression test.

Comment thread tools/mcp_oauth.py
if not data:
return None

for uri in data.get("redirect_uris") or []:

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 validate the decoded client-info shape and keep URI port extraction inside the malformed-data fallback path. The helper promises None for unusable registrations, but these accesses can raise instead of reaching _find_free_port().

@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

Merged on main via PR #65718 with your commit cherry-picked and authorship preserved — thanks @x9x9x9x9x9x91! The salvage composed your cached-port precedence with two changes that landed after your branch: the reserved-socket TOCTOU pool (#65622) now only engages for truly-fresh ephemeral picks, and _cached_redirect_port also parses the localhost URI form for redirect_host users. Closing since the branch base predated the OAuth rework.

@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 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.

3 participants