fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards by lennney · Pull Request #54325 · NousResearch/hermes-agent

lennney · 2026-06-28T15:52:46Z

What does this PR do?

Fixes #34260, preserves #57836 fail-fast contract.

Eliminates the module-level _oauth_port global that caused TOCTOU race and concurrent-flow port collision. Introduces OAuthCallbackServer (bind-first class) but defers binding until the interactive guard passes, so gateway/cron/background contexts never consume a port for an unusable cached token.

This PR addresses review feedback from @teknium1 (hermes-sweeper) which identified two regressions in the original bind-first approach.

Related Issue

Fixes #34260

Type of Change

Bug fix (non-breaking change that fixes an issue)

Changes Made

Non-interactive fail-fast guards restored (`tools/mcp_oauth.py`)

_redirect_handler() regains _raise_if_non_interactive() -- rejects before printing URL
_wait_for_callback() regains _raise_if_non_interactive() -- rejects before binding OAuthCallbackServer
Tests TestNonInteractiveFailFastAtCallbackBoundary (6 tests) restored -- verify no listener is bound in non-interactive contexts

Port resolution separated from server binding

_configure_callback_port() now only resolves the port (returns int) -- no HTTPServer created
OAuthCallbackServer is created inside _wait_for_callback() after the non-interactive guard passes
The legacy _oauth_port global is still set as a bridge for existing consumers

Server lifecycle management

_wait_for_callback() creates OAuthCallbackServer, calls server.start(), then await server.wait() inside a try block, with server.close() in finally
OAuthCallbackServer.close() is always called on success, failure, or timeout
The server is not stored in cfg dict or retained via callback partials after the flow completes

OAuthCallbackServer (kept from original PR)

Binds HTTPServer at construction (when called inside _wait_for_callback)
Persistent: loops handle_request() until callback or timeout
Graceful shutdown via threading.Event() stop signal
Path filtering: only /callback processed, others get 404

Callers updated

build_oauth_auth (tools/mcp_oauth.py): passes port via functools.partial instead of server
_build_provider (tools/mcp_oauth_manager.py): same -- passes port via partial
_redirect_handler: accepts optional port param

Honcho OAuth: display-guard added (`plugins/memory/honcho/oauth_flow.py`)

Same bug class as [Bug]: Headless MCP OAuth blocks gateway startup with stale cached tokens #57836: authorize_via_loopback() now checks _can_display_browser() before binding port :8765
Headless/SSH environments fail fast with actionable error instead of blocking 300s
CLI callers with custom open_url are unaffected

Files changed

tools/mcp_oauth.py -- core change
tools/mcp_oauth_manager.py -- caller update
tests/tools/test_mcp_oauth.py -- restored + migrated tests
plugins/memory/honcho/oauth_flow.py -- display guard

How to Test

Run the MCP OAuth test suite:

pytest tests/tools/test_mcp_oauth.py -v

Verify all 84 tests pass:
- Restored TestNonInteractiveFailFastAtCallbackBoundary (6 tests) -- verify fail-fast before binding
- All _wait_for_callback tests migrated from server= to port= param

Run Honcho OAuth tests:

pytest tests/honcho_plugin/test_oauth_flow.py -v

Manual: configure an OAuth MCP server, run hermes mcp login <server> with cached-but-expired tokens -- verify fail-fast in non-interactive mode, success in interactive mode

Security

The fail-fast contract (#57836) is now enforced at three layers:

Build-time: _is_interactive() and not storage.has_cached_tokens() -- no cached tokens in non-interactive env
Redirect boundary: _redirect_handler -- cached-but-unusable token caught before URL printed
Callback boundary: _wait_for_callback -- cached-but-unusable token caught before port bound

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/tools/test_mcp_oauth.py and all tests pass (84/84)
I've added tests for my changes (restored 6 boundary tests, migrated 8 server tests)
I've tested on my platform: Ubuntu 24.04 (Linux)

Documentation & Housekeeping

I've updated relevant documentation -- docstrings on new classes, methods, and restored guards
I've considered cross-platform impact (Windows, macOS) -- all changes use stdlib, no platform-specific code
I've considered security -- the fail-fast contract ([Bug]: Headless MCP OAuth blocks gateway startup with stale cached tokens #57836) is restored and verified by tests

Introduce OAuthCallbackServer that binds HTTPServer at construction, eliminating the TOCTOU gap between port discovery and actual server startup. Features: - Bind-first: server is live before _configure_callback_port returns - Path filtering: only /callback processed, other paths get 404 - Persistent: handle_request() loop until callback or timeout - Clean shutdown via close() 3 new tests: bind+connect, thread lifecycle, close+port release. Refs: #34260

alt-glitch · 2026-06-28T16:07:16Z

This was generated by AI during triage.

Competing fix cluster for the _oauth_port module-level global (TOCTOU / concurrent-flow port collision, all tracking #34260):

fix(mcp): scope oauth callback ports per provider #34280 (earliest open) and Bind MCP OAuth handlers per provider #44607 scope the callback port per provider via factory closures, keeping the legacy global.
This PR (fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards #54325) takes a different architectural route: it eliminates the global entirely with a bind-first OAuthCallbackServer (binds HTTPServer at construction, before the flow starts), and is explicitly PR 1/2.
Related: OAuth redirect_handler/callback_handler use shared globals, causing cross-server state pollution #44588, OAuth: ephemeral HTTPServer per flow causes OSError Address already in use #44590 (ephemeral-server TIME_WAIT addr-in-use), fix(mcp): report a clear error when the OAuth callback port is in use #50786 (busy-port error message).

Same root cause, genuinely different mechanism — flagging for a maintainer to pick the closure-scoping vs global-elimination approach. Not marking any as duplicate.

lennney · 2026-06-28T16:12:24Z

Thanks for the triage, @alt-glitch.

A quick note on the architectural choice between the two approaches:

The closure-scoping approach (#34280) keeps the legacy _oauth_port global and scopes it per-provider — which addresses the concurrent-flow collision but leaves the TOCTOU gap between port discovery and actual server bind.

This PR (#54325) follows the bind-first pattern described in the issue's "Suggested fix direction" section, modeled on Claude Code's auth.ts. The key properties:

No TOCTOU: OAuthCallbackServer binds at construction, before the OAuth flow starts. The port is never released between discovery and use.
Path filtering: only /callback is processed; stray requests (/favicon.ico, browser preflight) don't consume the handler slot.
Persistent server: loops handle_request() until callback or timeout (not single-request).
Paste retry (PR2): invalid pastes let the user retry instead of silently failing.

Happy to adjust scope or split further if the maintainer prefers a smaller diff. The OAuthCallbackServer class is self-contained and could be introduced incrementally if needed.

…ckServer Instead of finding a port with _find_free_port() (bind→close→remember), creates and starts an OAuthCallbackServer immediately. The server is bound and serving before the function returns, eliminating the TOCTOU gap. Stores server instance in cfg['_callback_server']. Refs: #34260

…OCTOU Rewrite _wait_for_callback to accept an optional OAuthCallbackServer parameter. When provided, polls the pre-bound server instead of creating a new HTTPServer on the port — eliminating the TOCTOU gap where port could be stolen between _configure_callback_port and _wait_for_callback. Changes: - _wait_for_callback(server=...) polls server.wait() + paste race - _redirect_handler accepts port param (falls back to _oauth_port) - build_oauth_auth uses functools.partial for both handlers - _build_provider uses functools.partial for both handlers - Legacy _oauth_port path preserved for backward compat Refs: #34260

…ckServer Verify: - /callback?code=... returns 200 and sets auth_code - /favicon.ico returns 404, server stays alive for next request - Existing paste integration and skip tests adapt to new server param Refs: #34260

- _paste_callback_reader: loop with up to 5 retries on invalid input - Remove _oauth_port module-level global (replaced by OAuthCallbackServer) - Remove _make_callback_handler (replaced by OAuthCallbackServer._make_handler) - Remove legacy path in _wait_for_callback (server param now required) - _redirect_handler: use port parameter directly, no fallback Tests will be updated by subagent — known breakage in paste/skip tests. Refs: #34260

…ndling - Remove _make_callback_handler dead function (replaced by OAuthCallbackServer) - Remove stale global _oauth_port statement from _configure_callback_port - Add ValueError to _serve_loop exception handler (prevents unhandled thread exception when close() interrupts handle_request()) Review: APPROVED after fixes

…kServer Replace _make_callback_handler() calls (deleted in PR2) with OAuthCallbackServer._make_handler() instances. Each test properly cleans up the server via .close().

The _serve_loop previously continued looping for up to 300s after close() called server_close(), because handle_request() exceptions were caught and the deadline had not expired. The thread.join(2.0) often raced past the 1.0s handle_request() timeout, causing test_close_stops_server to fail flakily. Add threading.Event() as a stop signal: close() sets it before server_close(), and _serve_loop checks it in the loop condition. Increase join timeout to 5s for safety margin.

lennney · 2026-07-07T02:23:07Z

Removed workspace files (.codegraph/.gitignore, docs/plans/) from the changeset. The branch has been force-pushed with only 3 source files remaining in the diff.

teknium1

Thanks for pursuing the bind-first approach; current main still has the global-port/TOCTOU premise described in #34260.

Problems

PR head binds at tools/mcp_oauth.py:787-790 during provider construction, but removes current main’s non-interactive redirect/callback guards (tools/mcp_oauth.py:547-560, 633-647, commit 755194ffe). A cached-but-unusable token can therefore bind a listener in gateway, cron, or background discovery, regressing the current fail-fast contract covered by tests/tools/test_mcp_oauth.py:597-680.
OAuthCallbackServer.close() has no production caller. The provider retains the server via callback partials (tools/mcp_oauth.py:905-906), while _wait_for_callback() awaits it without a finally (tools/mcp_oauth.py:654) and _serve_loop() does not close the socket on completion (tools/mcp_oauth.py:528-538).

Suggested changes

Retain the current non-interactive guards and create/bind the per-flow server only after crossing that interactive boundary.
Give the server an explicit lifecycle: close it on every callback outcome and provider eviction, with manager-path tests for non-interactive cached credentials and configured-port reuse.

Automated hermes-sweeper review.

teknium1 · 2026-07-15T07:02:01Z

-    cfg["_resolved_port"] = port
-    _oauth_port = port  # legacy consumer: _wait_for_callback reads this
-    return port
+    server = OAuthCallbackServer(port=requested)


This now binds during provider construction, before current main's non-interactive redirect/callback guards. MCPOAuthManager.get_or_build_provider() is called from tools/mcp_tool.py:2365; cached-but-unusable tokens would again bind a listener in gateway/cron/background contexts, regressing commit 755194ffe. Preserve that guard and defer binding until the interactive authorization path.

teknium1 · 2026-07-15T07:02:01Z

+            threading.Thread(
+                target=_paste_callback_reader, args=(server._result,), daemon=True
+            ).start()
+        result = await server.wait()


Please close the callback server in a finally. No production path calls OAuthCallbackServer.close(): the server is retained by the callback partials at lines 905-906, and _serve_loop() exits without server_close(). This can leave an explicit redirect_port occupied after success, failure, or timeout.

@teknium1

…erver lifecycle PR review by @teknium1 identified two regressions in the bind-first approach: 1. Port binding during provider construction bypasses non-interactive guards - _configure_callback_port() now resolves port only (no OAuthCallbackServer bind) - _redirect_handler and _wait_for_callback regain _raise_if_non_interactive() checks - Server (OAuthCallbackServer) is created only after the interactive guard passes - Gateway/cron/background contexts fail fast before consuming a port 2. OAuthCallbackServer.close() had no caller — server leaked on success/failure/timeout - _wait_for_callback now creates server internally with finally: server.close() - Clean lifecycle: bind → start → try/wait → finally/close Also removes cfg['_callback_server'] (no longer stored) — port is passed via partial. Updates tests: restore TestNonInteractiveFailFastAtCallbackBoundary, migrate all _wait_for_callback(server=server) callers to _wait_for_callback(port=port).

authorize_via_loopback() now checks _can_display_browser() before binding a port on :8765. If open_url defaults to webbrowser.open (the dashboard path) but no display/desktop is available, fails fast with an actionable error instead of blocking for 300s. Same bug class as #57836 (MCP OAuth non-interactive guard) adapted for the Honcho flow — always user-initiated, so the guard is environmental (no display) rather than stdin-interactivity based. CLI callers that pass a custom open_url (which prints the URL + webbrowser) are unaffected.

alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jun 28, 2026

lennney marked this pull request as ready for review June 28, 2026 16:45

alt-glitch mentioned this pull request Jun 30, 2026

fix(mcp-oauth): graceful fallback when OAuth callback port is already bound #55741

Closed

13 tasks

lennney and others added 7 commits July 7, 2026 10:22

test(mcp-oauth): migrate TestCallbackHandlerIsolation to OAuthCallbac…

1a191a9

…kServer Replace _make_callback_handler() calls (deleted in PR2) with OAuthCallbackServer._make_handler() instances. Each test properly cleans up the server via .close().

lennney force-pushed the fix/mcp-oauth-port-global branch from 0fcd2f8 to 425d54c Compare July 7, 2026 02:23

alt-glitch mentioned this pull request Jul 14, 2026

fix(mcp): replace global _oauth_port with per-provider closures #44685

Closed

3 tasks

teknium1 reviewed Jul 15, 2026

View reviewed changes

lennney added 2 commits July 15, 2026 15:28

lennney changed the title ~~fix(mcp-oauth): eliminate _oauth_port global, bind callback server first~~ fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards Jul 15, 2026

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 15, 2026

lennney closed this by deleting the head repository Jul 16, 2026

Navigation Menu

Uh oh!

fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards - #54325

fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards#54325
lennney wants to merge 10 commits into
NousResearch:mainfrom
lennney:fix/mcp-oauth-port-global

lennney commented Jun 28, 2026 •

edited

Loading

Uh oh!

alt-glitch commented Jun 28, 2026

Uh oh!

lennney commented Jun 28, 2026

Uh oh!

lennney commented Jul 7, 2026

Uh oh!

teknium1 left a comment

Uh oh!

teknium1 Jul 15, 2026

Uh oh!

teknium1 Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

lennney commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Related Issue

Type of Change

Changes Made

Non-interactive fail-fast guards restored (tools/mcp_oauth.py)

Port resolution separated from server binding

Server lifecycle management

OAuthCallbackServer (kept from original PR)

Callers updated

Honcho OAuth: display-guard added (plugins/memory/honcho/oauth_flow.py)

Files changed

How to Test

Security

Checklist

Code

Documentation & Housekeeping

Uh oh!

alt-glitch commented Jun 28, 2026

Uh oh!

lennney commented Jun 28, 2026

Uh oh!

lennney commented Jul 7, 2026

Uh oh!

teknium1 left a comment

Choose a reason for hiding this comment

Problems

Suggested changes

Uh oh!

teknium1 Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

teknium1 Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lennney commented Jun 28, 2026 •

edited

Loading

Non-interactive fail-fast guards restored (`tools/mcp_oauth.py`)

Honcho OAuth: display-guard added (`plugins/memory/honcho/oauth_flow.py`)