Skip to content

fix(mcp): plumb OAuth redirect host config - #21482

Open
nocturnum91 wants to merge 1 commit into
NousResearch:mainfrom
nocturnum91:fix/mcp-oauth-redirect-config-plumbing
Open

fix(mcp): plumb OAuth redirect host config#21482
nocturnum91 wants to merge 1 commit into
NousResearch:mainfrom
nocturnum91:fix/mcp-oauth-redirect-config-plumbing

Conversation

@nocturnum91

@nocturnum91 nocturnum91 commented May 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds MCP OAuth redirect-host plumbing so configured loopback redirect hosts are reflected consistently in redirect URI generation and callback listener binding.

Related Issue

N/A.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Add oauth.redirect_host plumbing for MCP OAuth redirect URIs.
  • Restrict redirect hosts to loopback values only (127.0.0.1, localhost, ::1, [::1]).
  • Keep listener behavior aligned with the selected redirect host, including IPv6 loopback handling.
  • Bind generated redirect URIs and callback listener behavior to the same validated config value.
  • Add regression coverage for valid and invalid redirect-host configurations.

How to Test

  1. Run MCP OAuth redirect-host focused tests.
  2. Verify loopback-only validation for IPv4, localhost, and IPv6 forms.
  3. Verify generated redirect URIs and listener binding stay consistent.
  4. Run a read-only Codex review of the final branch delta.

Verification performed during refresh:

  • Focused MCP OAuth redirect-host tests passed locally.
  • Read-only Codex review: VERDICT: CLEAN.
  • Branch was rebased onto current main and updated with --force-with-lease.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Note: I ran focused/regression tests locally for the changed MCP OAuth redirect-host behavior and am relying on GitHub Actions for the full matrix.

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A — this PR does not add a skill.

Screenshots / Logs

N/A. Relevant verification is in local focused test output, Codex review logs, and GitHub Actions checks.

@alt-glitch alt-glitch added type/feature New feature or request P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools labels May 7, 2026
@nocturnum91
nocturnum91 force-pushed the fix/mcp-oauth-redirect-config-plumbing branch 10 times, most recently from d841856 to 306d715 Compare May 16, 2026 14:04
@nocturnum91
nocturnum91 force-pushed the fix/mcp-oauth-redirect-config-plumbing branch 6 times, most recently from 160064c to 275ba2f Compare May 19, 2026 12:31
@nocturnum91
fix(mcp): plumb OAuth redirect host config
fd6c270 
Add loopback-only redirect_host handling for MCP OAuth callback URIs, including IPv6/localhost listener support and provider-local callback binding.

Rebuild cached providers when OAuth config changes and invalidate stale dynamic client registrations whose redirect URIs no longer match.

Tests: uv run --extra dev pytest tests/tools/test_mcp_oauth.py tests/tools/test_mcp_oauth_metadata.py -q
@nocturnum91
nocturnum91 force-pushed the fix/mcp-oauth-redirect-config-plumbing branch from 275ba2f to fd6c270 Compare May 19, 2026 14:12
@alt-glitch alt-glitch mentioned this pull request May 20, 2026
Closed
1 task
This was referenced May 21, 2026
Closed
Open
@alt-glitch alt-glitch mentioned this pull request Jun 17, 2026
Closed
@alt-glitch alt-glitch mentioned this pull request Jun 25, 2026
Open
@alt-glitch alt-glitch mentioned this pull request Jul 10, 2026
Closed
teknium1
teknium1 reviewed Jul 13, 2026
View reviewed changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing the redirect-host mismatch; current main still hard-codes 127.0.0.1 in the metadata, pre-registered-client, and listener paths (tools/mcp_oauth.py:655,849,876).

Problems

  • The new handlers at tools/mcp_oauth.py:594 and :687 do not retain current main's non-interactive fail-fast guards at tools/mcp_oauth.py:557 and :643 (from 755194ffe). Salvage must preserve those guards before printing an authorization URL or binding listeners.
  • localhost is forced to bind both AF_INET and AF_INET6 (tools/mcp_oauth.py:155), then any failed family aborts the flow (:720-735). This breaks IPv4-only localhost hosts even when their resolver and browser use IPv4.
  • The callback replacement omits main's interactive paste-back fallback (tools/mcp_oauth.py:667-683), which the MCP guide documents at website/docs/user-guide/features/mcp.md:225-230.

Suggested changes

  • Integrate the host/URI plumbing with the current guard and paste-back contracts intact.
  • Select supported loopback families for localhost and add IPv4-only coverage.
  • Document oauth.redirect_host and its accepted loopback values.

Automated hermes-sweeper review.

Comment thread tools/mcp_oauth.py
into BOTH loopback families and bind both. Literal addresses
(``127.0.0.1`` / ``::1``) keep their single-family behavior.
"""
if bind_host == "localhost":

@teknium1 teknium1 Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

localhost is expanded to AF_INET6 unconditionally, while the later bind loop fails the entire flow if either family is unavailable. An IPv4-only host can resolve and use localhost successfully over IPv4; choose available resolved loopback families rather than making IPv6 mandatory.

Comment thread tools/mcp_oauth.py

Opens the browser automatically when possible; always prints the URL
as a fallback for headless/SSH/gateway environments.
async def _redirect_handler_impl(

@teknium1 teknium1 Jul 13, 2026

Copy link
Copy Markdown
Collaborator

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(...) before this implementation prints an authorization URL or opens a browser. 755194ffe added that boundary guard for cached-but-unusable tokens in gateway/cron/background contexts.

Comment thread tools/mcp_oauth.py

# The callback server is already running (started in build_oauth_auth).
# We just need to poll for the result.
async def _wait_for_callback_impl(

@teknium1 teknium1 Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Carry forward the current interactive _paste_callback_reader race into this provider-local implementation. Main currently supports SSH users pasting the redirect URL when no tunnel is available, and the MCP guide documents that workflow.

@teknium1 teknium1 added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 13, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@alt-glitch alt-glitch mentioned this pull request Jul 13, 2026
Closed
@judywheeler8525-arch judywheeler8525-arch mentioned this pull request Sep 1, 2026
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teknium1 teknium1 teknium1 left review comments

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-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/mcp MCP client and OAuth type/feature New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nocturnum91 @teknium1 @alt-glitch