Skip to content

fix(mcp): harden headless OAuth callback lifecycle - #93066

Open
oskarchad wants to merge 1 commit into
NousResearch:mainfrom
oskarchad:fix/mcp-headless-oauth-upstream
Open

fix(mcp): harden headless OAuth callback lifecycle#93066
oskarchad wants to merge 1 commit into
NousResearch:mainfrom
oskarchad:fix/mcp-headless-oauth-upstream

Conversation

@oskarchad

Copy link
Copy Markdown

What does this PR do?

Hardens the explicit MCP OAuth login path used on headless hosts.

A non-TTY paste channel could turn EOF into reconnect-driven authorization retries, while the loopback callback listener and blocking TTY reader could outlive a completed flow. Concurrent HTTP/paste callbacks could also overwrite the first result, and callback/state errors exposed OAuth query values in logs.

This change keeps one login attempt single-flight, preserves the original loopback callback in non-TTY mode, deterministically shuts down and joins the listener, atomically latches the first terminal result, makes the TTY reader cancellable, and redacts callback/state values in both OAuth provider paths.

Related: #57836, #73997. I also reviewed the partial overlap in #74027 and the much broader #84963; neither covers this complete minimal callback/TTY/result/redaction contract.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tools/mcp_oauth.py: non-TTY gating, bounded listener teardown, first-result latch, cancellable paste reader, and callback/state log redaction.
  • tools/mcp_oauth_manager.py: the same state-mismatch redaction on the cached-provider path.
  • tests/tools/test_mcp_oauth_single_attempt.py: end-to-end regressions for the headless, loopback, TTY, teardown, latch, and redaction cases.

How to Test

HERMES_PYTHON=/tmp/hermes-upstream-pr-venv/bin/python scripts/run_tests.sh \
  tests/tools/test_mcp_oauth.py \
  tests/tools/test_mcp_oauth_single_attempt.py \
  tests/tools/test_mcp_oauth_user_agent.py \
  tests/tools/test_mcp_oauth_metadata.py \
  tests/tools/test_mcp_oauth_manager.py \
  tests/tools/test_mcp_oauth_integration.py \
  tests/tools/test_mcp_oauth_cold_load_expiry.py \
  tests/tools/test_mcp_oauth_bidirectional.py \
  tests/tools/test_mcp_dashboard_oauth.py \
  tests/hermes_cli/test_mcp_dashboard_oauth.py \
  tests/tools/test_mcp_failure_classification.py -q

Result on Linux 6.8: 134 passed. TDD check against current main production files: the new regression file fails 8 cases for the expected EOF/listener/latch/TTY/redaction reasons; restored branch passes all 10 cases.

ruff check on all changed Python files and git diff --check origin/main...HEAD also pass.

Checklist

  • I've read the Contributing Guide
  • My commit message follows Conventional Commits
  • I searched open and closed PRs/issues for overlap
  • This PR contains only this OAuth callback lifecycle fix
  • I've added regression tests
  • Tested on Linux 6.8
  • Documentation/config/schema changes are N/A

Provenance: this is a byte-identical replay of the locally reviewed cumulative patch from aa3491815a... through 5bafded1e7... onto upstream f293e7206b... (diff SHA-256 1709c6d1e7d188a46d1d8d7d7805bc7727668c07a8a96febec51aa7f179ccd1a).

@alt-glitch 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 Aug 23, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Thorough lifecycle hardening with genuinely good tests (real sockets, pty-based TTY cases, credential-redaction assertions, stalled-client timeout). The threaded callback server + bounded request timeouts + first-result latching close real races. Suggestions:

  1. tools/mcp_oauth.py:857-861log_message now drops the request line entirely. That fixes the code/state leak but also discards useful diagnostics (path, status). Logging the path with the query stripped (self.path.split('?', 1)[0]) keeps signal without exposing credentials.

  2. Duplicated redaction override: _perform_authorization is copy-pasted into both provider classes (tools/mcp_oauth.py:1365-1375, tools/mcp_oauth_manager.py:159-169), matching on the SDK's English message prefix "State parameter mismatch:". Two copies plus a brittle string match means an SDK rewording silently disables redaction in both places at once. Consider one shared helper/mixin and matching more loosely (e.g., "state" in str(exc).lower() and "mismatch" in str(exc).lower()).

  3. tools/mcp_oauth.py:501-524_try_set_callback_result's lock-free fallback duplicates decision logic for legacy dicts; since every current caller now includes _lock, consider making the lock mandatory so there's exactly one path to reason about.

  4. Windows paste reader (_read_windows_paste_line) raising KeyboardInterrupt from a non-main thread is caught by _paste_callback_reader's except clause — fine — but \x1a (Ctrl+Z) returning "" conflates EOF with empty input; harmless today given both paths bail, just noting the semantic overlap.

The port-release assertions after every flow are exactly the regression guard this area needed.

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: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