From a493dfcaf694f63f0ced304cd04d995f951f54e6 Mon Sep 17 00:00:00 2001 From: pm25coder Date: Sat, 3 Oct 2026 04:22:07 +0800 Subject: [PATCH 1/2] fix(pkce): keep the redirect port bound until the callback server owns it `_find_free_port()` bound a socket, read the port and closed it again, so between picking the port and the callback server binding it, any process on the machine could claim it. The browser's redirect then landed on a port nothing was listening on and the flow failed with a connection refused (`devagent auth` / any MCP OAuth login on a busy machine). The flow now binds the socket once and passes it to the server, which adopts it instead of binding a port of its own: - `_bind_callback_socket()` binds loopback on an ephemeral port and keeps the socket -- the port is never free in between. - `_start_callback_server(sock)` builds the `HTTPServer` with `bind_and_activate=False`, assigns the already-bound socket and calls `server_activate()` to listen on it. The redirect URI is built from `sock.getsockname()`, so the address the browser is sent to is the socket the server is serving on. - the socket is closed when the flow ends (`server_close()` in the `finally`), so the port is not left bound for the rest of the process. Measured on this host, with the old helper and the new one side by side: ``` helper: _find_free_port (bind, read, close), port=64311 another process can take the port: True <- the window this fixes helper: _bind_callback_socket (socket held open), port=64310 another process can take the port: False ``` 7 tests in `tests/test_pkce_callback_port.py`, all offline (loopback only): another socket cannot take the port; the adopted socket actually serves the callback (code and error arms); closing the server releases the port; the redirect URI the flow advertises is the port it is listening on (the end-to-end arm, standing in for the browser); and the port is released when the flow returns. Full suite on this host: green apart from `test_phase17.py::TestFixtureProject:: test_data_store_tests_pass_despite_bug`, a pre-existing Windows GBK-locale failure that also fails on unmodified main. ruff clean. Fixes #61 --- CHANGELOG.md | 4 + devagent/mcp/auth/pkce.py | 41 +++++++--- tests/test_pkce_callback_port.py | 134 +++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 tests/test_pkce_callback_port.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a14c58..9da352c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `_gemini` now forwards the tool definitions and converts `FunctionCall` parts back into `ToolCallRequest`; the reply is read from `candidates[0].content.parts` instead of `resp.text`, which raises when the reply has no text part (a pure function call has none) - gemini's `Schema` proto rejects unknown keywords rather than ignoring them (`ValueError: Unknown field for Schema: `), so tool schemas are reduced to the keywords the proto carries — ten of the twenty-three shipped tool schemas include a JSON-Schema `default`, which would have raised on the first tool-using call - 13 tests in `tests/test_gemini_tool_calling.py`, offline: the SDK's own request builder runs for real and only the transport client is faked +**The OAuth redirect port is no longer released between picking it and listening on it** +- `_find_free_port()` bound a socket, read the port and closed it again, so any process on the machine could claim that port before the callback server bound it — the browser's redirect then landed on a port nothing was listening on and the flow died with a connection refused +- the flow now binds the socket once (`_bind_callback_socket`) and hands it to the server, which adopts it instead of binding a port of its own; the socket is closed when the flow ends rather than staying bound for the rest of the process +- 6 tests in `tests/test_pkce_callback_port.py`, including the end-to-end arm that follows the redirect the flow advertises ### Added diff --git a/devagent/mcp/auth/pkce.py b/devagent/mcp/auth/pkce.py index 14818ca..595f5fa 100644 --- a/devagent/mcp/auth/pkce.py +++ b/devagent/mcp/auth/pkce.py @@ -26,6 +26,7 @@ import http.server import json import secrets +import socket import threading import urllib.parse import urllib.request @@ -87,19 +88,39 @@ def log_message(self, fmt: str, *args: object) -> None: return _Handler -def _start_callback_server(port: int) -> tuple[http.server.HTTPServer, _CallbackResult]: +def _start_callback_server(sock: socket.socket) -> tuple[http.server.HTTPServer, _CallbackResult]: + """Serve on *sock*, which must already be bound to the redirect address. + + The socket is handed over rather than looked up again: the port is never free + between picking it and listening on it, so another process cannot claim it in + that window and leave the browser's redirect with nothing to connect to. + """ result = _CallbackResult() - server = http.server.HTTPServer(("127.0.0.1", port), _make_handler(result)) + server = http.server.HTTPServer( + sock.getsockname(), _make_handler(result), bind_and_activate=False + ) + server.socket = sock + server.server_activate() # listen() on the socket we already hold + server.server_name = "127.0.0.1" + server.server_port = sock.getsockname()[1] thread = threading.Thread(target=server.serve_forever, daemon=True) thread.start() return server, result -def _find_free_port() -> int: - import socket - with socket.socket() as s: - s.bind(("127.0.0.1", 0)) - return s.getsockname()[1] +def _bind_callback_socket() -> socket.socket: + """Bind a loopback socket on an ephemeral port and keep it open. + + The caller reads the port off this socket for the redirect URI and passes the + same socket to :func:`_start_callback_server` once the browser is on its way. + """ + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + sock.bind(("127.0.0.1", 0)) + except OSError: + sock.close() + raise + return sock # --------------------------------------------------------------------------- @@ -152,7 +173,8 @@ def pkce_authorize( """ verifier = generate_code_verifier() challenge = compute_code_challenge(verifier) - port = _find_free_port() + callback_sock = _bind_callback_socket() + port = callback_sock.getsockname()[1] redirect_uri = f"http://127.0.0.1:{port}/callback" params = { @@ -166,7 +188,7 @@ def pkce_authorize( } auth_url = f"{authorization_url}?{urllib.parse.urlencode(params)}" - server, result = _start_callback_server(port) + server, result = _start_callback_server(callback_sock) try: webbrowser.open(auth_url) if not result.done.wait(timeout=timeout): @@ -176,6 +198,7 @@ def pkce_authorize( return _exchange_code(token_url, client_id, result.code, verifier, redirect_uri) finally: server.shutdown() + server.server_close() def refresh_token_flow( diff --git a/tests/test_pkce_callback_port.py b/tests/test_pkce_callback_port.py new file mode 100644 index 0000000..92bd6d3 --- /dev/null +++ b/tests/test_pkce_callback_port.py @@ -0,0 +1,134 @@ +"""The OAuth redirect port must not be free between picking it and listening on it. + +``_find_free_port`` bound a socket, read the port and closed it again, so anything on the +machine could claim that port before the callback server bound it -- the redirect from the +browser then landed on a port nothing was listening on. The flow now binds the socket once +and hands it to the server, so the port is never released in between. + +Offline: loopback sockets only, no browser and no network beyond 127.0.0.1. +""" + +from __future__ import annotations + +import socket +import urllib.parse +import urllib.request + +import pytest + +from devagent.mcp.auth import pkce + + +def _probe_bind(port: int) -> None: + """Try to take *port* the way an unrelated process would.""" + probe = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + probe.bind(("127.0.0.1", port)) + finally: + probe.close() + + +class TestThePortIsHeld: + def test_another_socket_cannot_take_the_port(self) -> None: + sock = pkce._bind_callback_socket() + try: + port = sock.getsockname()[1] + with pytest.raises(OSError): + _probe_bind(port) + finally: + sock.close() + + def test_the_callback_server_serves_on_the_same_socket(self) -> None: + sock = pkce._bind_callback_socket() + port = sock.getsockname()[1] + server, result = pkce._start_callback_server(sock) + try: + with urllib.request.urlopen( + f"http://127.0.0.1:{port}/callback?code=abc", timeout=5 + ) as resp: + body = resp.read() + status = resp.status + assert status == 200 + assert b"Authorization complete" in body + assert result.code == "abc" + assert result.error == "" + assert result.done.is_set() + finally: + server.shutdown() + server.server_close() + + def test_an_oauth_error_is_captured_too(self) -> None: + sock = pkce._bind_callback_socket() + port = sock.getsockname()[1] + server, result = pkce._start_callback_server(sock) + try: + urllib.request.urlopen( + f"http://127.0.0.1:{port}/callback?error=access_denied", timeout=5 + ).read() + assert result.error == "access_denied" + assert result.code == "" + finally: + server.shutdown() + server.server_close() + + def test_closing_the_server_releases_the_port(self) -> None: + sock = pkce._bind_callback_socket() + port = sock.getsockname()[1] + server, _ = pkce._start_callback_server(sock) + server.shutdown() + server.server_close() + + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.bind(("127.0.0.1", port)) # must not raise + + +class TestTheFlowUsesThatSocket: + def test_the_redirect_uri_points_at_the_listening_port(self, monkeypatch) -> None: + """The browser's redirect has to reach the server the flow just started.""" + seen: dict[str, str] = {} + + def fake_open(url: str) -> None: + query = dict(urllib.parse.parse_qsl(urllib.parse.urlparse(url).query)) + seen["redirect_uri"] = query["redirect_uri"] + # stand in for the browser following the provider's redirect + urllib.request.urlopen(f"{query['redirect_uri']}?code=CODE123", timeout=5).read() + + def fake_exchange(token_url, client_id, code, verifier, redirect_uri): + return {"access_token": "t", "code": code, "redirect_uri": redirect_uri} + + monkeypatch.setattr(pkce.webbrowser, "open", fake_open) + monkeypatch.setattr(pkce, "_exchange_code", fake_exchange) + + tokens = pkce.pkce_authorize( + authorization_url="https://auth.example.com/authorize", + token_url="https://auth.example.com/token", + client_id="devagent", + scopes=["mcp.read"], + timeout=10, + ) + + assert tokens["code"] == "CODE123" + assert tokens["redirect_uri"] == seen["redirect_uri"] + assert seen["redirect_uri"].startswith("http://127.0.0.1:") + + def test_the_port_is_released_when_the_flow_ends(self, monkeypatch) -> None: + held: dict[str, int] = {} + + def fake_open(url: str) -> None: + query = dict(urllib.parse.parse_qsl(urllib.parse.urlparse(url).query)) + held["port"] = int(urllib.parse.urlparse(query["redirect_uri"]).port) + urllib.request.urlopen(f"{query['redirect_uri']}?code=C", timeout=5).read() + + monkeypatch.setattr(pkce.webbrowser, "open", fake_open) + monkeypatch.setattr(pkce, "_exchange_code", lambda *a, **k: {"access_token": "t"}) + + pkce.pkce_authorize( + authorization_url="https://auth.example.com/authorize", + token_url="https://auth.example.com/token", + client_id="devagent", + scopes=["mcp.read"], + timeout=10, + ) + + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.bind(("127.0.0.1", held["port"])) # must not raise From 2e21951af0317b9ec11ce160b06e881ab8704990 Mon Sep 17 00:00:00 2001 From: pm25coder Date: Sat, 3 Oct 2026 07:27:07 +0800 Subject: [PATCH 2/2] test(pkce): assert nothing is listening, not that the port can be rebound The flow test that checks the callback port is free once `pkce_authorize` returns makes a real request to the server first, and the fake browser never closed its end. On Linux the server side of that connection keeps the port in `FIN_WAIT_2`, so the probe's `bind()` fails: first with `EADDRINUSE` (red on both Linux legs, green on Windows, which allows the rebind), and still failing with `SO_REUSEADDR` set, because `SO_REUSEADDR` forgives a connection in `TIME_WAIT`, not one that was never closed. The test now asserts the fact it is about: once the flow returns, nothing is listening on the callback port, checked by connecting and expecting a refusal. A refused connect needs no listener and does not depend on the old connection's state, on either OS. The bind probe stays for the held case, where a live listener must make it fail -- checked: it returns False with the server up, so the assertion is not vacuous. The fake browser closes the response it reads, standing in for a browser closing the redirect page, so the server is not left in `FIN_WAIT_2` either. Follow-up to b3b6b52 (the original fix) and 769c90b (a first attempt that added `SO_REUSEADDR` and did not turn the Linux legs green). b3b6b52's body also said "7 tests"; the file has six. Local: 6 passed; probe control: False while listening, True after `server_close` and with a lingering connection and no listener. --- tests/test_pkce_callback_port.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/tests/test_pkce_callback_port.py b/tests/test_pkce_callback_port.py index 92bd6d3..31df3c8 100644 --- a/tests/test_pkce_callback_port.py +++ b/tests/test_pkce_callback_port.py @@ -20,7 +20,7 @@ def _probe_bind(port: int) -> None: - """Try to take *port* the way an unrelated process would.""" + """Try to take *port* the way an unrelated process would, without round trips.""" probe = socket.socket(socket.AF_INET, socket.SOCK_STREAM) try: probe.bind(("127.0.0.1", port)) @@ -28,6 +28,24 @@ def _probe_bind(port: int) -> None: probe.close() +def _nothing_is_listening(port: int) -> bool: + """Whether a fresh connection to *port* is refused, i.e. no server holds it. + + A refused connection is the fact under test -- once the flow returns (or once + ``server_close()`` has run) nothing may be listening on the callback port. + Probing with a ``bind`` measures something else, and on Linux it measures it + wrongly: the port is also unbindable -- even with ``SO_REUSEADDR`` -- while the + connection the last run accepted lingers (the client never closed its end), and + that is not the server still holding the port. A connect to a port with no + listener is refused whatever state the old connection is in, on either OS. + """ + try: + with socket.create_connection(("127.0.0.1", port), timeout=2): + return False + except OSError: + return True + + class TestThePortIsHeld: def test_another_socket_cannot_take_the_port(self) -> None: sock = pkce._bind_callback_socket() @@ -78,8 +96,7 @@ def test_closing_the_server_releases_the_port(self) -> None: server.shutdown() server.server_close() - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: - probe.bind(("127.0.0.1", port)) # must not raise + assert _nothing_is_listening(port) class TestTheFlowUsesThatSocket: @@ -91,7 +108,8 @@ def fake_open(url: str) -> None: query = dict(urllib.parse.parse_qsl(urllib.parse.urlparse(url).query)) seen["redirect_uri"] = query["redirect_uri"] # stand in for the browser following the provider's redirect - urllib.request.urlopen(f"{query['redirect_uri']}?code=CODE123", timeout=5).read() + with urllib.request.urlopen(f"{query['redirect_uri']}?code=CODE123", timeout=5) as resp: + resp.read() def fake_exchange(token_url, client_id, code, verifier, redirect_uri): return {"access_token": "t", "code": code, "redirect_uri": redirect_uri} @@ -117,7 +135,8 @@ def test_the_port_is_released_when_the_flow_ends(self, monkeypatch) -> None: def fake_open(url: str) -> None: query = dict(urllib.parse.parse_qsl(urllib.parse.urlparse(url).query)) held["port"] = int(urllib.parse.urlparse(query["redirect_uri"]).port) - urllib.request.urlopen(f"{query['redirect_uri']}?code=C", timeout=5).read() + with urllib.request.urlopen(f"{query['redirect_uri']}?code=C", timeout=5) as resp: + resp.read() monkeypatch.setattr(pkce.webbrowser, "open", fake_open) monkeypatch.setattr(pkce, "_exchange_code", lambda *a, **k: {"access_token": "t"}) @@ -130,5 +149,4 @@ def fake_open(url: str) -> None: timeout=10, ) - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: - probe.bind(("127.0.0.1", held["port"])) # must not raise + assert _nothing_is_listening(held["port"])