fix: TOCTOU port race between _find_free_port() and HTTPServer bind - #22161
fix: TOCTOU port race between _find_free_port() and HTTPServer bind#22161amathxbt wants to merge 1 commit into
Conversation
|
Reproduction import socket, threading
from tools.mcp_oauth import _find_free_port
# Simulate the TOCTOU window: grab the port immediately after _find_free_port returns
port = _find_free_port()
s = socket.socket()
s.bind(('127.0.0.1', port)) # Steal the port
# Now _wait_for_callback tries to bind the same port and gets OSError
# The error message says 'Complete auth in browser' — completely misleading
s.close()Root cause: Fix: Added |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real callback-port race. The current implementation still selects a port via _find_free_port() (tools/mcp_oauth.py:151-155, called at :830) and binds the callback listener only later (:655), so the premise remains valid.
Problems
- The submitted diff does not add or wire
_bind_free_socket(): its callback hunk only changes theOSErrordiagnostic at PRtools/mcp_oauth.py:444-450. The selection-to-bind race therefore remains. - PR
tools/mcp_oauth.py:449recommendsredirect_port: 0, although that is already the default race-prone path, and the message ends with the incomplete phrase "or ensure". - No regression test covers a competing bind. Current tests at
tests/tools/test_mcp_oauth.py:223-232only check returned port values.
Suggested changes
- Rework the callback lifecycle so a listener/reservation owns the port through callback handling, then derive the redirect URI from that live port.
- Add a deterministic occupied-port regression test and an explicit-port conflict diagnostic test.
- Remove unrelated unused
_port_excbindings in the diff.
Automated hermes-sweeper review.
| except OSError: | ||
| # Port already in use — the server from build_oauth_auth is running. | ||
| # Fall back to polling the server started by build_oauth_auth. | ||
| except OSError as _port_exc: |
There was a problem hiding this comment.
This changes only the diagnostic. The diff neither defines nor uses _bind_free_socket(), so _find_free_port() still releases the selected port before the HTTPServer bind and the reported race remains.
| "OAuth callback timed out — could not bind callback port. " | ||
| "Complete the authorization in a browser first, then retry." | ||
| "OAuth callback server could not bind port (TOCTOU race or port conflict). " | ||
| "Use redirect_port: 0 (default) to auto-select a free port, or ensure" |
There was a problem hiding this comment.
redirect_port: 0 is already the default path and is the path that calls _find_free_port(), so it cannot resolve this race. The message also ends with the incomplete phrase "or ensure".
| try: | ||
| os.chmod(path.parent, 0o700) | ||
| except OSError: | ||
| except OSError as _port_exc: |
There was a problem hiding this comment.
This unrelated exception binding is unused and does not affect callback-port handling; please leave unrelated exception handlers unchanged.
_find_free_port() closed its probe socket before HTTPServer re-bound the port minutes later, leaving a window where another process could steal it (#22161 by @amathxbt). _reserve_callback_port() now keeps the selected socket bound (bounded FIFO pool) until _wait_for_callback adopts it via bind_and_activate=False. Also sets allow_reuse_address BEFORE binding — the cherry-picked #44872 set it after the constructor had already bound, where it is a no-op. Also updates the three #57836 non-interactive-guard tests to the closure-factory API from #44872.
|
Fixed on main via PR #65622 (commit 95a0f9c), credited to you in the commit message and PR body — thanks @amathxbt for the TOCTOU diagnosis and repro! The merged fix implements your keep-the-socket-bound direction: |
_find_free_port() closed its probe socket before HTTPServer re-bound the port minutes later, leaving a window where another process could steal it (NousResearch#22161 by @amathxbt). _reserve_callback_port() now keeps the selected socket bound (bounded FIFO pool) until _wait_for_callback adopts it via bind_and_activate=False. Also sets allow_reuse_address BEFORE binding — the cherry-picked NousResearch#44872 set it after the constructor had already bound, where it is a no-op. Also updates the three NousResearch#57836 non-interactive-guard tests to the closure-factory API from NousResearch#44872.
_find_free_port() closed its probe socket before HTTPServer re-bound the port minutes later, leaving a window where another process could steal it (NousResearch#22161 by @amathxbt). _reserve_callback_port() now keeps the selected socket bound (bounded FIFO pool) until _wait_for_callback adopts it via bind_and_activate=False. Also sets allow_reuse_address BEFORE binding — the cherry-picked NousResearch#44872 set it after the constructor had already bound, where it is a no-op. Also updates the three NousResearch#57836 non-interactive-guard tests to the closure-factory API from NousResearch#44872.
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>
…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>
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under heavy concurrency an ephemeral-reservation churn could close a parked pinned socket before _wait_for_callback adopted it, silently reopening the port-stealing window the pin exists to prevent (#22161). Eviction now skips the pinned range; it is already bounded by _CIMD_PORTS. Follow-up to the #84050 salvage.
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under heavy concurrency an ephemeral-reservation churn could close a parked pinned socket before _wait_for_callback adopted it, silently reopening the port-stealing window the pin exists to prevent (#22161). Eviction now skips the pinned range; it is already bounded by _CIMD_PORTS. Follow-up to the #84050 salvage.
The _MAX_RESERVED_SOCKETS cap applied to pinned CIMD sockets too, so under heavy concurrency an ephemeral-reservation churn could close a parked pinned socket before _wait_for_callback adopted it, silently reopening the port-stealing window the pin exists to prevent (NousResearch#22161). Eviction now skips the pinned range; it is already bounded by _CIMD_PORTS. Follow-up to the NousResearch#84050 salvage.
Bug
_find_free_port()intools/mcp_oauth.pyopens a socket, reads the port number, then closes the socket before returning. Between the close and the subsequentHTTPServer()bind, another process can grab the same port.Impact
In environments with high port churn (containers, CI runners, concurrent Hermes instances), the callback server intermittently fails to bind, producing an unhelpful error that tells the user to complete auth in a browser rather than diagnosing the port conflict.
Fix
Add
_bind_free_socket()which returns the socket still open withSO_REUSEADDRset. Update_wait_for_callbackto use it and update theOSErrorhandler to emit a clear diagnostic message naming the conflicting port.