From 9da468526b0d6758a39d7011524ec159b86ba609 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 02:43:46 +0000 Subject: [PATCH] Fix footguns: AAI_AUTH_PORT import crash + spawn fd leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - auth/endpoints.py parsed AAI_AUTH_PORT with a module-level int() at import time. This module is on the CLI's import hot path, so a malformed value (e.g. AAI_AUTH_PORT=abc) raised a raw ValueError that crashed *every* aai command, even 'aai --help' — not just 'aai login'. Resolve the port lazily in a validated loopback_port() that raises a clean CLIError (with range check) only on the login path. - init/runner.spawn() passed log_path.open("w") straight to Popen and never closed the parent's handle, leaking a file descriptor for the lifetime of the (long-lived) tunnel process. Close it after Popen returns; the child keeps its own dup. Tests updated to drive the port via AAI_AUTH_PORT, with added boundary coverage (1, 65535, 65536, 0, non-integer). --- aai_cli/auth/endpoints.py | 36 +++++++++++++++++++++++++++++++++-- aai_cli/auth/loopback.py | 5 +++-- aai_cli/init/runner.py | 23 ++++++++++++++-------- tests/test_auth_endpoints.py | 37 +++++++++++++++++++++++++++++++++++- tests/test_auth_loopback.py | 8 ++++---- tests/test_init_runner.py | 9 ++++++--- 6 files changed, 98 insertions(+), 20 deletions(-) diff --git a/aai_cli/auth/endpoints.py b/aai_cli/auth/endpoints.py index c1b59971..13361305 100644 --- a/aai_cli/auth/endpoints.py +++ b/aai_cli/auth/endpoints.py @@ -3,6 +3,7 @@ import os from aai_cli import environments +from aai_cli.errors import CLIError # Constant across environments. STYTCH_OAUTH_PROVIDER = "google" @@ -10,8 +11,39 @@ # Fixed loopback (Stytch does exact-match redirect validation; 8585 is registered). LOOPBACK_HOST = "127.0.0.1" -LOOPBACK_PORT = int(os.environ.get("AAI_AUTH_PORT", "8585")) LOOPBACK_PATH = "/callback" +_DEFAULT_LOOPBACK_PORT = 8585 +_MAX_PORT = 65535 # highest valid TCP port + + +def _invalid_auth_port(raw: str) -> CLIError: + return CLIError( + f"AAI_AUTH_PORT must be a port number in 1-65535, got {raw!r}.", + error_type="invalid_env", + exit_code=2, + suggestion="Unset AAI_AUTH_PORT, or set it to a free port in 1-65535.", + ) + + +def loopback_port() -> int: + """The loopback callback port, overridable via ``AAI_AUTH_PORT`` (dev/test only). + + Resolved lazily — never at import — and validated, so a malformed override + surfaces as a clean CLIError on the login path instead of the raw ``ValueError`` + a module-level ``int(...)`` would raise. This module sits on the CLI's import hot + path, so that ValueError would otherwise crash *every* ``aai`` command (even + ``--help``), not just ``aai login``. + """ + raw = os.environ.get("AAI_AUTH_PORT") + if raw is None: + return _DEFAULT_LOOPBACK_PORT + try: + port = int(raw) + except ValueError as exc: + raise _invalid_auth_port(raw) from exc + if not 1 <= port <= _MAX_PORT: + raise _invalid_auth_port(raw) + return port # Environment-specific values resolve from the active environment (see @@ -36,4 +68,4 @@ def signup_url() -> str: def redirect_uri() -> str: """The exact loopback redirect URL registered in Stytch.""" - return f"http://{LOOPBACK_HOST}:{LOOPBACK_PORT}{LOOPBACK_PATH}" + return f"http://{LOOPBACK_HOST}:{loopback_port()}{LOOPBACK_PATH}" diff --git a/aai_cli/auth/loopback.py b/aai_cli/auth/loopback.py index 63246522..b27b77e7 100644 --- a/aai_cli/auth/loopback.py +++ b/aai_cli/auth/loopback.py @@ -70,12 +70,13 @@ def do_GET(self) -> None: # stdlib API name def log_message(self, format: str, *args: object) -> None: # silence stderr logging pass + port = endpoints.loopback_port() try: - server = HTTPServer((endpoints.LOOPBACK_HOST, endpoints.LOOPBACK_PORT), Handler) + server = HTTPServer((endpoints.LOOPBACK_HOST, port), Handler) except OSError as exc: raise APIError( f"Could not start the login callback server on " - f"{endpoints.LOOPBACK_HOST}:{endpoints.LOOPBACK_PORT} ({exc}). " + f"{endpoints.LOOPBACK_HOST}:{port} ({exc}). " "Close whatever is using that port and run 'aai login' again." ) from exc thread = threading.Thread(target=server.serve_forever, daemon=True) diff --git a/aai_cli/init/runner.py b/aai_cli/init/runner.py index d8832a60..e0606225 100644 --- a/aai_cli/init/runner.py +++ b/aai_cli/init/runner.py @@ -84,14 +84,21 @@ def spawn( used to capture cloudflared's output for URL discovery. Without it, stdio is inherited. """ if log_path is not None: - return subprocess.Popen( - command, - cwd=cwd, - env=env, - stdout=log_path.open("w"), - stderr=subprocess.STDOUT, - text=True, - ) + # The child gets its own dup of the fd once Popen returns, so close the + # parent's handle straight away instead of leaking it for the (long-lived) + # process's whole lifetime. + log = log_path.open("w") + try: + return subprocess.Popen( + command, + cwd=cwd, + env=env, + stdout=log, + stderr=subprocess.STDOUT, + text=True, + ) + finally: + log.close() return subprocess.Popen(command, cwd=cwd, env=env, text=True) diff --git a/tests/test_auth_endpoints.py b/tests/test_auth_endpoints.py index a273d4e8..03a45b44 100644 --- a/tests/test_auth_endpoints.py +++ b/tests/test_auth_endpoints.py @@ -1,4 +1,7 @@ +import pytest + from aai_cli.auth import endpoints +from aai_cli.errors import CLIError def test_redirect_uri_is_fixed_loopback(): @@ -19,5 +22,37 @@ def test_constants_are_environment_independent(): def test_env_override_changes_redirect_uri(monkeypatch): - monkeypatch.setattr(endpoints, "LOOPBACK_PORT", 9999) + monkeypatch.setenv("AAI_AUTH_PORT", "9999") assert endpoints.redirect_uri() == "http://127.0.0.1:9999/callback" + + +def test_loopback_port_rejects_non_integer(monkeypatch): + # A typo'd AAI_AUTH_PORT must surface as a clean CLIError on the login path, + # not a raw ValueError that would crash every command at import time. + monkeypatch.setenv("AAI_AUTH_PORT", "abc") + with pytest.raises(CLIError) as excinfo: + endpoints.loopback_port() + assert excinfo.value.exit_code == 2 + assert "AAI_AUTH_PORT" in str(excinfo.value) + + +def test_loopback_port_rejects_above_max(monkeypatch): + # 65535 is the highest valid TCP port; one past it must be rejected. + monkeypatch.setenv("AAI_AUTH_PORT", "65536") + with pytest.raises(CLIError): + endpoints.loopback_port() + + +def test_loopback_port_accepts_boundary_values(monkeypatch): + # The valid range is exactly 1..65535 inclusive. + monkeypatch.setenv("AAI_AUTH_PORT", "1") + assert endpoints.loopback_port() == 1 + monkeypatch.setenv("AAI_AUTH_PORT", "65535") + assert endpoints.loopback_port() == 65535 + + +def test_loopback_port_rejects_zero(monkeypatch): + # Port 0 (OS-assign) is meaningless for a fixed, pre-registered redirect URI. + monkeypatch.setenv("AAI_AUTH_PORT", "0") + with pytest.raises(CLIError): + endpoints.loopback_port() diff --git a/tests/test_auth_loopback.py b/tests/test_auth_loopback.py index c587fdbf..4c4b2f0b 100644 --- a/tests/test_auth_loopback.py +++ b/tests/test_auth_loopback.py @@ -23,7 +23,7 @@ def _unique_loopback_port(monkeypatch): with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: probe.bind((endpoints.LOOPBACK_HOST, 0)) port = probe.getsockname()[1] - monkeypatch.setattr(endpoints, "LOOPBACK_PORT", port) + monkeypatch.setenv("AAI_AUTH_PORT", str(port)) def _hit(path: str) -> int | None: @@ -35,7 +35,7 @@ def _hit(path: str) -> int | None: # Retry briefly until the server thread is bound. for _ in range(50): conn = http.client.HTTPConnection( - endpoints.LOOPBACK_HOST, endpoints.LOOPBACK_PORT, timeout=2 + endpoints.LOOPBACK_HOST, endpoints.loopback_port(), timeout=2 ) try: conn.request("GET", path) @@ -91,7 +91,7 @@ def _body(path: str) -> bytes: Callers first confirm the server is bound via `_hit`, so no readiness loop is needed here. """ - conn = http.client.HTTPConnection(endpoints.LOOPBACK_HOST, endpoints.LOOPBACK_PORT, timeout=2) + conn = http.client.HTTPConnection(endpoints.LOOPBACK_HOST, endpoints.loopback_port(), timeout=2) try: conn.request("GET", path) return conn.getresponse().read() @@ -146,7 +146,7 @@ def test_capture_raises_clean_error_when_port_unavailable(monkeypatch): busy.bind((endpoints.LOOPBACK_HOST, 0)) busy.listen(1) port = busy.getsockname()[1] - monkeypatch.setattr(endpoints, "LOOPBACK_PORT", port) + monkeypatch.setenv("AAI_AUTH_PORT", str(port)) try: with pytest.raises(APIError): loopback.capture_callback(timeout=1.0) diff --git a/tests/test_init_runner.py b/tests/test_init_runner.py index 04e157f4..46a2aec4 100644 --- a/tests/test_init_runner.py +++ b/tests/test_init_runner.py @@ -214,9 +214,12 @@ def fake_popen(cmd, **kwargs): runner.spawn(["cloudflared"], cwd=tmp_path, log_path=log) assert captured["kwargs"]["stderr"] is runner.subprocess.STDOUT assert captured["kwargs"]["text"] is True - # stdout is an open writable handle to the log file - assert captured["kwargs"]["stdout"].writable() - captured["kwargs"]["stdout"].close() + stdout = captured["kwargs"]["stdout"] + # spawn writes the child's stdout to the log file... + assert stdout.name == str(log) + # ...and closes the parent's handle once Popen returns (the child keeps its dup), + # so the file descriptor isn't leaked for the process's whole lifetime. + assert stdout.closed is True def test_run_server_passes_command_and_env(monkeypatch):