Skip to content

fix(mcp-oauth): allow configurable redirect_uri for MCP OAuth flows - #47755

Closed
flewe wants to merge 2 commits into
NousResearch:mainfrom
flewe:fix/mcp-oauth-redirect-uri
Closed

fix(mcp-oauth): allow configurable redirect_uri for MCP OAuth flows#47755
flewe wants to merge 2 commits into
NousResearch:mainfrom
flewe:fix/mcp-oauth-redirect-uri

Conversation

@flewe

@flewe flewe commented Jun 17, 2026

Copy link
Copy Markdown

Allow the OAuth redirect_uri to be configured via the MCP server config,
removing the hardcoded http://127.0.0.1:<port>/callback.
This is needed when the callback must go through a proxy (e.g. Tailscale Funnel
exposing a public HTTPS URL that forwards to localhost).

@alt-glitch alt-glitch added type/feature New feature or request tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have labels Jun 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: implements the configurable OAuth redirect_uri requested in #29299 (HTTPS/proxy callback support). Note #21482 plumbs a redirect host override (127.0.0.1/localhost/::1) — a narrower, different mechanism than this full redirect_uri override. Not a duplicate; the full-URI override is novel (main still hardcodes the loopback callback).

… hint

The PR added a configurable `redirect_uri` (proxy/Funnel callbacks) but
shipped without tests, and the loopback SSH-tunnel hint stayed hardcoded —
actively misleading the exact proxy user the feature targets.

- Extract `_resolve_redirect_uri(cfg, port)` so the client-metadata and
  pre-registration paths derive an identical callback (a mismatch makes the
  authorization server reject the redirect).
- Make `_redirect_handler` redirect_uri-aware: a configured proxy callback
  reaches this machine on its own, so it no longer prints the `ssh -N -L`
  loopback guidance. Wired via `functools.partial` — no new global state.
- Document `redirect_uri` in the config block.
- 14 new tests (red/green TDD): helper resolution + empty-string fallback,
  metadata + pre-registration for configured/default, AnyUrl normalization,
  no-client_id skip, client_secret combo, and both SSH-hint branches.

ruff clean · 81 passed (tests/tools/test_mcp_oauth.py) · ty baseline unchanged

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@burka

burka commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Hardened this in ce21f5b (red/green TDD, +14 tests):

  • DRY: extracted _resolve_redirect_uri(cfg, port) so the client-metadata and pre-registration paths derive an identical callback — a mismatch gets the redirect rejected.
  • Fixed a misleading hint: the loopback ssh -N -L guidance was hardcoded and fired even with a proxy redirect_uri — i.e. for the exact user this PR is for. _redirect_handler is now redirect_uri-aware (wired via functools.partial, no new global) and prints an accurate "forwards to this machine, no tunnel needed" note.
  • Tests: helper resolution + empty-string fallback, metadata & pre-registration for configured/default, AnyUrl normalization, no-client_id skip, client_secret combo, both SSH-hint branches.
  • Docs: redirect_uri added to the config block.

ruff clean · 81 passed · ty baseline unchanged.

samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Jun 24, 2026
…able MCP OAuth redirect_uri)

Nimmt NousResearch#47755 in UPSTREAM_PRS auf — macht die
OAuth-redirect_uri pro MCP-Server konfigurierbar (oauth.redirect_uri),
statt hardcoded http://127.0.0.1:<port>/callback. Ermöglicht den
headless Browser-Login über öffentliche Redirect-URL + Proxy (HAProxy →
Loopback) ohne SSH/Paste. Hintergrund/Entscheid: docs/10 im Doku-Repo.

Test-Merge der Skript-Sequenz (main + PR-Head) lokal: konfliktfrei.
Noch nicht ausgeführt/gepusht — Skript-Lauf folgt separat.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Jun 24, 2026
samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Jun 25, 2026
Add an opt-in `oauth.redirect_bind` config key so the OAuth callback
HTTPServer can bind a routable address instead of loopback. This lets a
reverse proxy on a *different* host forward the callback to the listener
(our HAProxy topology, where the proxy cannot reach the LXC loopback).

Defaults to 127.0.0.1 -> fully backwards compatible. Complements the
configurable redirect_uri (NousResearch#47755): redirect_uri advertises the public
URL, redirect_bind makes the listener reachable by the proxy. Intended to
run behind a firewall (port reachable only via the proxy), mirroring the
dashboard --insecure bind.

- _oauth_bind_host module global (default loopback)
- _configure_callback_port resolves it from oauth.redirect_bind, set
  before the free-port probe so the probe runs on the chosen interface
- _find_free_port(host) probes the chosen interface
- _wait_for_callback binds (_oauth_bind_host, _oauth_port)

Tests: config resolution, loopback default + empty fallback, host-aware
port probe. NOT run locally (no Python runtime in this env) -> rely on
fork CI / host.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Jun 25, 2026
…Bind)

Eigener Fork-Patch (kein Upstream-PR): feat/mcp-oauth-redirect-bind.
Komplementär zu NousResearch#47755 (redirect_uri). Default Loopback, opt-in.
Siehe docs/11 im Doku-Repo. Upstream-PR-Kandidat (Track 2, Issue-first).
@douglasg14b

Copy link
Copy Markdown

This is very much needed for things like Slack MCP, which requires localhost instead of 127.0.0.1.

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

Thank you — configurable redirect URIs address a verified current-main limitation: tools/mcp_oauth.py:849 and tools/mcp_oauth.py:876 hardcode the loopback callback in both OAuth metadata and pre-registered client state.

Problems

  • The new functools.partial is only added to build_oauth_auth (tools/mcp_oauth.py, PR right side around line 805). Normal MCP connections use get_manager().get_or_build_provider() from tools/mcp_tool.py:2364-2367; tools/mcp_oauth_manager.py:548-556 constructs that provider independently and still passes _redirect_handler directly. The new proxy-aware SSH guidance therefore does not run on the production path.
  • No user-facing MCP documentation is included. website/docs/reference/mcp-config-reference.md:276-292 documents auth: oauth but not the new oauth.redirect_uri configuration or its same-host proxy requirement.

Suggested changes

  • Mirror the redirect-handler binding in MCPOAuthManager._build_provider and cover that manager path in tests.
  • Add the configuration and proxy-forwarding requirements to the MCP configuration reference.

Automated hermes-sweeper review.

Comment thread tools/mcp_oauth.py
client_metadata=client_metadata,
storage=storage,
redirect_handler=_redirect_handler,
redirect_handler=functools.partial(

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 binding only changes the legacy build_oauth_auth path. Runtime MCP connections use MCPOAuthManager._build_provider (tools/mcp_oauth_manager.py:548-556), which still passes _redirect_handler directly; mirror this partial there and add a manager-path regression test.

@teknium1 teknium1 added 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 via PR #65610 with your commits cherry-picked onto current main and your authorship preserved in git log — thanks for the clean implementation and the thorough test coverage! One adaptation during salvage: main landed a per-provider closure refactor for the OAuth handlers (#44588/#44590) after your branch point, so the redirect_uri parameter now rides on the _make_redirect_handler(port, redirect_uri=...) closure factory instead of a functools.partial — behavior is identical, and your SSH-hint fix and tests carried over. Also folded in a redirect_host option from #63889 through your _resolve_redirect_uri so both compose.

@teknium1 teknium1 closed this Jul 16, 2026
samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Aug 12, 2026
Upstream hat NousResearch#47755 (configurable redirect_uri) via NousResearch#65610 gemerged und dabei
umgebaut: _make_redirect_handler-Closure statt functools.partial (NousResearch#44588/NousResearch#44590),
_resolve_redirect_uri mit neuer redirect_host-Praezedenz (NousResearch#63889), plus eine
TOCTOU-Portreservierung (NousResearch#22161).

Konflikte in tools/mcp_oauth.py und tests/tools/test_mcp_oauth.py zugunsten der
Upstream-Seite aufgeloest; unser oauth.redirect_bind neu darauf aufgesetzt:

- _oauth_bind_host global (Default Loopback) — jetzt explizit abgegrenzt gegen
  upstreams redirect_host (das nur die *angekuendigte* URI umschreibt)
- _configure_callback_port loest redirect_bind auf, VOR der Portwahl
- _reserve_callback_port bindet auf _oauth_bind_host (NEUE zweite Bindstelle aus
  NousResearch#22161 — sonst waeren Reservierung und HTTPServer uneinig)
- HTTPServer bindet (_oauth_bind_host, port)
- _find_free_port(host=None) faellt auf _oauth_bind_host zurueck

Tests als TestRedirectBind neu aufgesetzt (die alte Datei war upstream
klassenbasiert reorganisiert), inkl. Regressionstest fuer die Reservierung.
5 passed. Der eine Fehlschlag in test_mcp_oauth.py
(test_build_oauth_auth_preserves_server_url_path) ist vorbestehend und
umgebungsbedingt — er faellt auf unveraendertem upstream/main identisch aus
(mcp-SDK im Devcontainer nicht installiert).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
samuelpulfer pushed a commit to immeditech/hermes-agent that referenced this pull request Aug 12, 2026
…ed (NousResearch#65610)

Damit ist immeditech-main = upstream/main + nur noch unsere eigenen Patches
(oauth.redirect_bind, HERMES_UPDATE_BRANCH, Devcontainer/Sync-Infra).

- 47755 aus UPSTREAM_PRS raus; Begruendung im Skript-Kommentar festgehalten,
  inkl. Hinweis, dass upstream beim Salvage umgebaut hat
- leeres Array unter 'set -u' abgesichert (bash 3.2 auf macOS wuerde sonst
  bei "${UPSTREAM_PRS[@]}" abbrechen) + Hinweisausgabe
- docs/immeditech-fork.md: kuratierte PRs alle erledigt (Tabelle mit Ausgang),
  neuer Abschnitt 'Eigene Patches' als vollstaendiges Delta gegen upstream,
  Warnung wegen der zweiten Bindstelle aus NousResearch#22161
- agent/secret_sources/__init__.py auf Upstream-Stand zurueck (uebrig war nur
  eine Backtick-Kosmetik aus der NousResearch#42300-Entfernung)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 P3 Low — cosmetic, nice to have 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/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants