fix(mcp-oauth): port mismatch breaks browser redirect, path traversal, shared handler state by 0xbyt4 · Pull Request #2521 · NousResearch/hermes-agent

0xbyt4 · 2026-03-22T16:33:10Z

Summary

Three bugs in the new MCP OAuth 2.1 PKCE implementation (tools/mcp_oauth.py) that shipped in #2456.

Bug 1 — CRITICAL: OAuth browser redirect never works

build_oauth_auth() calls _find_free_port() at line 209 to build the redirect_uri registered with the authorization server. When the callback arrives, _wait_for_callback() calls _find_free_port() again at line 153, getting a different ephemeral port, and starts the HTTP server on that new port.

Result: browser redirects to port A, server listens on port B — connection refused, 120s timeout, falls back to manual code entry every time.

Fix: Store the port from build_oauth_auth() in module-level _oauth_port and reuse it in _wait_for_callback().

Bug 2 — MEDIUM: Path traversal via unsanitized server_name

HermesTokenStorage uses server_name directly in filenames:

self._base_dir() / f"{self._server_name}.json"

A name like ../../.ssh/config writes token files outside ~/.hermes/mcp-tokens/:

>>> HermesTokenStorage("../../.ssh/config")._tokens_path()
/Users/user/.ssh/config.json

Fix: Sanitize server_name with regex (same pattern used in other modules).

Bug 3 — MEDIUM: Class-level state causes data races

_CallbackHandler stores auth_code and state as class attributes shared across all instances. Concurrent OAuth flows overwrite each other's results.

Fix: Replace class with a factory function _make_callback_handler() that returns a handler class + closure-scoped result dict, isolating each flow.

Verification

All three bugs reproduced and verified fixed with bash:

# Bug 1: Port mismatch
>>> _oauth_port = 12345  # set by build_oauth_auth
>>> _wait_for_callback()  # now uses 12345, not a new port

# Bug 2: Path traversal
>>> HermesTokenStorage("../../.ssh/config")._tokens_path()
# Before: /Users/user/.ssh/config.json
# After:  /Users/user/.hermes/mcp-tokens/ssh-config.json

# Bug 3: Shared state
>>> _, r1 = _make_callback_handler()
>>> _, r2 = _make_callback_handler()
>>> r1["auth_code"] = "A"; r2["auth_code"] = "B"
>>> r1["auth_code"]  # "A" (was "B" before fix)

Test plan

17 tests pass (10 existing + 7 new)
Path traversal: ../../.ssh/config stays within mcp-tokens/
Special characters sanitized (@, :, /)
Normal server names preserved
Concurrent handler result dicts are independent
Handler writes to own result dict, not class-level
build_oauth_auth stores port in _oauth_port
No external files import changed functions (fully isolated)

…uth flow Three bugs in the new MCP OAuth 2.1 PKCE implementation: 1. CRITICAL: OAuth redirect port mismatch — build_oauth_auth() calls _find_free_port() to register the redirect_uri, but _wait_for_callback() calls _find_free_port() again getting a DIFFERENT port. Browser redirects to port A, server listens on port B — callback never arrives, 120s timeout. Fix: share the port via module-level _oauth_port variable. 2. MEDIUM: Path traversal via unsanitized server_name — HermesTokenStorage uses server_name directly in filenames. A name like "../../.ssh/config" writes token files outside ~/.hermes/mcp-tokens/. Fix: sanitize server_name with the same regex pattern used elsewhere. 3. MEDIUM: Class-level auth_code/state on _CallbackHandler causes data races if concurrent OAuth flows run. Second callback overwrites first. Fix: factory function _make_callback_handler() returns a handler class with a closure-scoped result dict, isolating each flow.

…port sharing 7 new tests covering: - Path traversal blocked (../../.ssh/config stays in mcp-tokens/) - Dots/slashes sanitized and resolved within base dir - Normal server names preserved - Special characters sanitized (@, :, /) - Concurrent handler result dicts are independent - Handler writes to its own result dict, not class-level - build_oauth_auth stores port in module-level _oauth_port

When the MCP background loop is stopped and closed, httpx/httpcore async transports held by streamablehttp_client fire __del__ finalizers that call call_soon() on the now-dead loop. asyncio catches the resulting RuntimeError and routes it to the loop's exception handler, which by default prints 'Unhandled exception in event loop: Event loop is closed' on stderr. Fix: install a custom exception handler on the MCP event loop at creation time that silently drops RuntimeError('Event loop is closed') — a benign shutdown race — and forwards all other exceptions to the default handler. Root cause: Python's GC runs transport __del__ finalizers after the loop is closed. The transports try to schedule cleanup via call_soon(), which raises because the loop is already dead. The connection teardown is a no-op at this point so the error is safe to suppress. Matches the pattern used in model_tools.py (ab6abc2) and auxiliary_client.py (stale async client cache invalidation) for the same class of async shutdown races. Fixes NousResearch#2521

…te (salvage #2521) (#2552) * fix(mcp-oauth): port mismatch, path traversal, and shared state in OAuth flow Three bugs in the new MCP OAuth 2.1 PKCE implementation: 1. CRITICAL: OAuth redirect port mismatch — build_oauth_auth() calls _find_free_port() to register the redirect_uri, but _wait_for_callback() calls _find_free_port() again getting a DIFFERENT port. Browser redirects to port A, server listens on port B — callback never arrives, 120s timeout. Fix: share the port via module-level _oauth_port variable. 2. MEDIUM: Path traversal via unsanitized server_name — HermesTokenStorage uses server_name directly in filenames. A name like "../../.ssh/config" writes token files outside ~/.hermes/mcp-tokens/. Fix: sanitize server_name with the same regex pattern used elsewhere. 3. MEDIUM: Class-level auth_code/state on _CallbackHandler causes data races if concurrent OAuth flows run. Second callback overwrites first. Fix: factory function _make_callback_handler() returns a handler class with a closure-scoped result dict, isolating each flow. * test: add tests for MCP OAuth path traversal, handler isolation, and port sharing 7 new tests covering: - Path traversal blocked (../../.ssh/config stays in mcp-tokens/) - Dots/slashes sanitized and resolved within base dir - Normal server names preserved - Special characters sanitized (@, :, /) - Concurrent handler result dicts are independent - Handler writes to its own result dict, not class-level - build_oauth_auth stores port in module-level _oauth_port --------- Co-authored-by: 0xbyt4 <35742124+0xbyt4@users.noreply.github.com>

teknium1 · 2026-03-22T22:02:32Z

Merged via PR #2552. Your substantive commits were cherry-picked onto current main with authorship preserved. All three fixes (port mismatch, path traversal, handler isolation) are in along with your 7 new tests. Thanks for the solid contribution!

…te (salvage NousResearch#2521) (NousResearch#2552) * fix(mcp-oauth): port mismatch, path traversal, and shared state in OAuth flow Three bugs in the new MCP OAuth 2.1 PKCE implementation: 1. CRITICAL: OAuth redirect port mismatch — build_oauth_auth() calls _find_free_port() to register the redirect_uri, but _wait_for_callback() calls _find_free_port() again getting a DIFFERENT port. Browser redirects to port A, server listens on port B — callback never arrives, 120s timeout. Fix: share the port via module-level _oauth_port variable. 2. MEDIUM: Path traversal via unsanitized server_name — HermesTokenStorage uses server_name directly in filenames. A name like "../../.ssh/config" writes token files outside ~/.hermes/mcp-tokens/. Fix: sanitize server_name with the same regex pattern used elsewhere. 3. MEDIUM: Class-level auth_code/state on _CallbackHandler causes data races if concurrent OAuth flows run. Second callback overwrites first. Fix: factory function _make_callback_handler() returns a handler class with a closure-scoped result dict, isolating each flow. * test: add tests for MCP OAuth path traversal, handler isolation, and port sharing 7 new tests covering: - Path traversal blocked (../../.ssh/config stays in mcp-tokens/) - Dots/slashes sanitized and resolved within base dir - Normal server names preserved - Special characters sanitized (@, :, /) - Concurrent handler result dicts are independent - Handler writes to its own result dict, not class-level - build_oauth_auth stores port in module-level _oauth_port --------- Co-authored-by: 0xbyt4 <35742124+0xbyt4@users.noreply.github.com>

0xbyt4 added 2 commits March 22, 2026 19:28

teknium1 mentioned this pull request Mar 22, 2026

fix(mcp-oauth): port mismatch, path traversal, and shared handler state (salvage #2521) #2552

Merged

teknium1 closed this Mar 22, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp-oauth): port mismatch breaks browser redirect, path traversal, shared handler state#2521

fix(mcp-oauth): port mismatch breaks browser redirect, path traversal, shared handler state#2521
0xbyt4 wants to merge 2 commits into
NousResearch:mainfrom
0xbyt4:fix/mcp-oauth-bugs

0xbyt4 commented Mar 22, 2026

Uh oh!

teknium1 commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

0xbyt4 commented Mar 22, 2026

Summary

Bug 1 — CRITICAL: OAuth browser redirect never works

Bug 2 — MEDIUM: Path traversal via unsanitized server_name

Bug 3 — MEDIUM: Class-level state causes data races

Verification

Test plan

Uh oh!

teknium1 commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants