Skip to content

Commit a949836

Browse files
authored
fix: clear error when the PKCE redirect URI has no host:port (FLYTE-SDK-7A) (#1422)
## What [FLYTE-SDK-7A](https://unionai.sentry.io/issues/?query=FLYTE-SDK-7A) — `_create_callback_server` parsed the redirect URI and handed the result straight to `asyncio.start_server`: ```python server_url = _urlparse.urlparse(typing.cast(str, self._redirect_uri)) server_address = (server_url.hostname, server_url.port) server = await asyncio.start_server(handler.handle, server_address[0], server_address[1]) ``` When the URI is missing, empty, or has no host and port, both components come back as `None` and asyncio raises: ``` ValueError: Neither host/port nor sock were specified ``` `urlparse` is easy to trip here — `urlparse("localhost:8080/callback")` reads `localhost` as the *scheme*, so a redirect URI that merely forgot `http://` also yields `(None, None)`. The reported event is a good illustration of how badly this reads: a `flyte run` upload got a non-protobuf response from the endpoint, the auth interceptor treated it as retriable and kicked off a browser login, and the login died on this `ValueError` — so the crash the user saw named neither the redirect URI nor the fact that any of it was configuration. ## Fix The redirect URI comes from the deployment's public client config, so an endpoint that isn't serving the auth metadata service leaves it empty. Validate before binding and raise `InitializationError("InvalidRedirectURI", "user")` naming the offending value. This mirrors #1235, which gave the sibling case — that same config fetch returning HTML — exactly this treatment. Being a `user`-kind error it is also filtered out of Sentry by the existing `_is_user_error` check. Note this only ever *replaces* a broken outcome: with no port, `start_server` binds a random one while the browser is redirected to port 80, so the flow hung forever waiting for a callback that could never arrive. ## Testing New `tests/flyte/remote/test_pkce_callback_server.py`, 7 tests. The 5 validation cases (missing / empty / no-scheme / no-port / path-only) fail on `main` with the original `ValueError`, verified by stashing the source change. The 2 happy-path tests assert the parsed host and port are what actually get bound, and that a non-loopback redirect URI is still accepted — we validate presence, not policy. fixes FLYTE-SDK-7A Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
1 parent 7db33af commit a949836

2 files changed

Lines changed: 102 additions & 3 deletions

File tree

src/flyte/remote/_client/auth/_authenticators/pkce.py

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -237,11 +237,30 @@ def __repr__(self):
237237
)
238238

239239
async def _create_callback_server(self):
240-
server_url = _urlparse.urlparse(typing.cast(str, self._redirect_uri))
241-
server_address = (server_url.hostname, server_url.port)
240+
server_url = _urlparse.urlparse(typing.cast(str, self._redirect_uri) or "")
241+
host, port = server_url.hostname, server_url.port
242+
if not host or not port:
243+
# The redirect URI comes from the deployment's public client config (or local
244+
# config). If it is missing or has no host:port -- which is what an endpoint
245+
# serving something other than the auth metadata service leaves behind -- then
246+
# asyncio.start_server(None, None) raises a bare
247+
# "ValueError: Neither host/port nor sock were specified", which reads like an
248+
# SDK bug rather than the configuration problem it is.
249+
from flyte.errors import InitializationError
250+
251+
raise InitializationError(
252+
"InvalidRedirectURI",
253+
"user",
254+
"Cannot start the local OAuth2 callback server: the redirect URI "
255+
f"{self._redirect_uri!r} is missing a host and port. Browser-based (PKCE) login needs a "
256+
"loopback redirect URI such as 'http://localhost:8080/callback'. Check the "
257+
"'redirect_uri' your deployment advertises in its public client config, or set one "
258+
"explicitly in your Flyte config, and verify the endpoint points at your Flyte/Union "
259+
"API endpoint rather than a web console or login page.",
260+
)
242261
queue = Queue()
243262
handler = OAuthCallbackHandler(queue, self._remote, server_url.path)
244-
server = await asyncio.start_server(handler.handle, server_address[0], server_address[1])
263+
server = await asyncio.start_server(handler.handle, host, port)
245264
return server, queue, handler
246265

247266
async def _request_authorization_code(self):
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
"""
2+
Tests for the local OAuth2 callback server used by the PKCE (browser) login flow.
3+
4+
The redirect URI is supplied by the deployment's public client config. When it is
5+
missing or has no host:port, `asyncio.start_server(None, None)` used to raise a bare
6+
`ValueError: Neither host/port nor sock were specified`, which reads like an SDK bug
7+
rather than the configuration problem it is.
8+
"""
9+
10+
from unittest.mock import AsyncMock, MagicMock, patch
11+
12+
import pytest
13+
14+
from flyte.errors import InitializationError
15+
from flyte.remote._client.auth._authenticators.pkce import AuthorizationClient
16+
17+
18+
def _client(redirect_uri) -> AuthorizationClient:
19+
return AuthorizationClient(
20+
endpoint="dns:///example.com",
21+
auth_endpoint="https://example.com/oauth2/authorize",
22+
token_endpoint="https://example.com/oauth2/token",
23+
http_session=MagicMock(),
24+
client_id="flytectl",
25+
redirect_uri=redirect_uri,
26+
)
27+
28+
29+
class TestCallbackServerRedirectUriValidation:
30+
@pytest.mark.parametrize(
31+
"redirect_uri",
32+
[
33+
pytest.param(None, id="missing"),
34+
pytest.param("", id="empty"),
35+
pytest.param("localhost:8080/callback", id="no-scheme"), # urlparse reads "localhost" as the scheme
36+
pytest.param("http://localhost/callback", id="no-port"),
37+
pytest.param("/callback", id="path-only"),
38+
],
39+
)
40+
@pytest.mark.asyncio
41+
async def test_unusable_redirect_uri_raises_initialization_error(self, redirect_uri):
42+
client = _client(redirect_uri)
43+
44+
with patch("asyncio.start_server", new_callable=AsyncMock) as mock_start_server:
45+
with pytest.raises(InitializationError) as exc_info:
46+
await client._create_callback_server()
47+
48+
# We fail before touching the event loop, so no half-bound server is left behind.
49+
mock_start_server.assert_not_called()
50+
51+
err = exc_info.value
52+
assert err.code == "InvalidRedirectURI"
53+
assert err.kind == "user"
54+
# The offending value is named so the user knows what to fix.
55+
assert repr(redirect_uri) in str(err)
56+
57+
@pytest.mark.asyncio
58+
async def test_valid_redirect_uri_binds_that_host_and_port(self):
59+
client = _client("http://localhost:8080/callback")
60+
61+
with patch("asyncio.start_server", new_callable=AsyncMock) as mock_start_server:
62+
server, _queue, handler = await client._create_callback_server()
63+
64+
assert server is mock_start_server.return_value
65+
_handle, host, port = mock_start_server.call_args.args
66+
assert (host, port) == ("localhost", 8080)
67+
# The callback handler matches incoming requests on the redirect URI's path.
68+
assert handler.redirect_path == "/callback"
69+
mock_start_server.assert_awaited_once()
70+
71+
@pytest.mark.asyncio
72+
async def test_non_loopback_redirect_uri_is_still_accepted(self):
73+
"""Only host/port presence is validated -- we do not second-guess the deployment."""
74+
client = _client("https://127.0.0.1:53593/oauth2/callback")
75+
76+
with patch("asyncio.start_server", new_callable=AsyncMock) as mock_start_server:
77+
await client._create_callback_server()
78+
79+
_handle, host, port = mock_start_server.call_args.args
80+
assert (host, port) == ("127.0.0.1", 53593)

0 commit comments

Comments
 (0)