Skip to content

fix(pkce): keep the redirect port bound until the callback server owns it - #94

Open
pm25coder wants to merge 2 commits into
knight22-21:mainfrom
pm25coder:fix/pkce-callback-port
Open

pm25coder wants to merge 2 commits into
knight22-21:mainfrom
pm25coder:fix/pkce-callback-port

Conversation

@pm25coder

Copy link
Copy Markdown
Contributor

Problem

_find_free_port() bound a socket, read the port, and closed it again. Between that close and the callback server binding the port, 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 (and any MCP OAuth login) is exposed on a machine with normal socket churn.

Measured on this host, old helper and 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

Change

The socket is bound once and handed to the server, so the port is never free in between (issue option A — no retry loop needed):

  • _bind_callback_socket() binds loopback on an ephemeral port and keeps the socket.
  • _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 serves on.
  • the socket is closed when the flow ends (server_close() in the finally) — previously the listening socket stayed bound for the rest of the process.

_find_free_port() had no other caller and is removed.

Tests

7 tests in tests/test_pkce_callback_port.py, all offline (loopback only, no browser):

  • another socket cannot take the port (the reservation, stated directly);
  • 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 following the redirect);
  • the port is released when the flow returns.
uv run --python 3.13 --with pytest --with pytest-asyncio python -m pytest tests/ -q
809 passed, 1 skipped, 1 failed

The failure is test_phase17.py::TestFixtureProject::test_data_store_tests_pass_despite_bug, a pre-existing Windows GBK-locale UnicodeDecodeError that fails on unmodified main too. ruff clean.

Fixes #61.

@pm25coder
pm25coder force-pushed the fix/pkce-callback-port branch from 769c90b to 5489e3e Compare October 2, 2026 23:32
@knight22-21

Copy link
Copy Markdown
Owner

Thanks for the fix! A PR merged just before this one touched \CHANGELOG.md,
so there's a conflict now. Could you rebase onto \main\ and force-push?

\
git fetch upstream
git rebase upstream/main
git push --force-with-lease
\\

Everything else looks good — CI passed on all test and lint checks.

…s 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 knight22-21#61
…ound

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.
@pm25coder
pm25coder force-pushed the fix/pkce-callback-port branch from 5489e3e to 2e21951 Compare October 19, 2026 10:46
@pm25coder

Copy link
Copy Markdown
Contributor Author

Rebased onto main (5345a78) and force-pushed. The only conflict was CHANGELOG.md — both entries sit under ### Fixed, so I kept both, yours (Gemini tool definitions) first and mine after it.

Re-running the test file turned up something in my own entry: it claimed "7 tests" for tests/test_pkce_callback_port.py, and the file has 6 — it had 6 in the original branch too, so the number was wrong from the start. Corrected it in the same commit rather than leave a count that doesn't match the file.

Local after the rebase: 6 passed in tests/test_pkce_callback_port.py, 16 passed / 1 skipped for tests/ -k "pkce or oauth". CI should settle the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PKCE OAuth _find_free_port() has TOCTOU race between port discovery and server bind

2 participants