[codex] Fix client auth timeout and expiry handling

src/labapi/client.py

@@ -128,6 +128,18 @@ def init_poolmanager(self, *args: Any, **kwargs: Any):
         super().init_poolmanager(*args, **kwargs, ssl_context=context)  # pyright: ignore[reportUnknownMemberType]
 
 
+def _close_auth_driver(driver: Any) -> None:
+    """Close an auth browser driver without masking authentication results."""
+    try:
+        driver.quit()
+    except Exception as exc:
+        warnings.warn(
+            f"Failed to close authentication browser driver: {exc}",
+            RuntimeWarning,
+            stacklevel=2,
+        )
+
+
 class _AuthResponseCollector:
     """Context manager for binding and waiting on a loopback auth callback."""
 
@@ -143,6 +155,8 @@ def __init__(
         self._client = client
         self._port = port
         self._callback_path = callback_path or f"/auth/{token_urlsafe(24)}/"
+        if timeout is not None and timeout <= 0:
+            raise ValueError("timeout must be positive or None")
         self._timeout = timeout
         self._error: str | None = None
         self._email: str | None = None
@@ -223,7 +237,16 @@ def wait(self) -> User:
         deadline = None if self._timeout is None else monotonic() + self._timeout
 
         while True:
-            self._httpd.timeout = deadline if deadline is None else min(deadline, 0.5)
+            if deadline is None:
+                self._httpd.timeout = None
+            else:
+                remaining = deadline - monotonic()
+                if remaining <= 0:
+                    raise AuthenticationError(
+                        "Timed out waiting for the authentication callback"
+                    )
+                self._httpd.timeout = min(remaining, 0.5)
+
             self._httpd.handle_request()
 
             if self._error is not None:
@@ -232,16 +255,6 @@ def wait(self) -> User:
             if self._auth_code and self._email:
                 return self._client.login(self._email, self._auth_code)
 
-            if deadline is not None:
-                remaining = deadline - monotonic()
-                if remaining <= 0:
-                    raise AuthenticationError(
-                        "Timed out waiting for the authentication callback"
-                    )
-                self._httpd.timeout = min(remaining, 0.5)
-            else:
-                self._httpd.timeout = None
-
 
 class Client:
     """A client for the LabArchives API.
@@ -693,7 +706,7 @@ def default_authenticate(
                 return auth_response_collector.wait()
             finally:
                 if driver is not None:
-                    driver.quit()
+                    _close_auth_driver(driver)
 
     def collect_auth_response(
         self,
@@ -753,7 +766,8 @@ def construct_url(
                                  actual method name differs from the URI path.
         :returns: The fully constructed and signed URL.
         :raises ValueError: If ``api_method_uri`` does not contain any non-empty
-                            path segments after normalization.
+                            path segments after normalization, or if
+                            ``expires_in`` is explicitly expired.
         """
         if isinstance(api_method_uri, str):
             api_method_uri = api_method_uri.split("/")
@@ -786,7 +800,7 @@ def construct_url(
         # LabArchives API does not use fragments in API requests
         url = urlunsplit((scheme, netloc, path, urlencode(query), ""))
 
-        if expires_in:
+        if expires_in is not None:
             return self._sign_url(url, api_method, expires_in)
         return self._sign_url(url, api_method)
 
@@ -825,14 +839,19 @@ def _sign_url(
                            `timedelta` object or a specific `datetime` object. Defaults
                            to 60 seconds from the current time.
         :returns: The fully signed URL.
+        :raises ValueError: If ``expires_in`` is not a future expiry.
         """
         scheme, netloc, path, querystring, _f = urlsplit(url)
         query = dict(parse_qsl(querystring))
 
         if isinstance(expires_in, timedelta):
+            if expires_in <= timedelta(0):
+                raise ValueError("expires_in must be a positive duration")
             expiry = round((datetime.now() + expires_in).timestamp() * 1000)
         else:
             expiry = round(expires_in.timestamp() * 1000)
+            if expiry <= round(datetime.now().timestamp() * 1000):
+                raise ValueError("expires_in must be a future datetime")
         sig = self._signature(api_method, expiry)
 
         query["akid"] = self._akid

tests/test_client.py

@@ -243,17 +243,48 @@ def test_client_construct_url_defaults_to_sixty_second_expiry(self):
     def test_client_construct_url_accepts_absolute_datetime_expiry(self):
         """Test construct_url preserves explicit absolute expiration datetimes."""
         client = Client("https://api.test.com", "test_akid", "test_password")
-        absolute_expiry = datetime(2026, 4, 1, 12, 5, 0)
+        fixed_now = datetime(2026, 4, 1, 12, 0, 0)
+        absolute_expiry = fixed_now + timedelta(minutes=5)
 
-        signed_url = client.construct_url(
-            "users/get_info",
-            {"uid": "123"},
-            expires_in=absolute_expiry,
-        )
+        with patch("labapi.client.datetime") as mock_datetime:
+            mock_datetime.now.return_value = fixed_now
+            signed_url = client.construct_url(
+                "users/get_info",
+                {"uid": "123"},
+                expires_in=absolute_expiry,
+            )
 
         expires = dict(parse_qsl(urlsplit(signed_url).query))["expires"]
         assert int(expires) == round(absolute_expiry.timestamp() * 1000)
 
+    @pytest.mark.parametrize("expires_in", [timedelta(0), timedelta(seconds=-1)])
+    def test_client_construct_url_rejects_non_positive_duration_expiry(
+        self, expires_in: timedelta
+    ):
+        """Test explicit non-positive duration expiries are rejected."""
+        client = Client("https://api.test.com", "test_akid", "test_password")
+
+        with pytest.raises(ValueError, match="positive duration"):
+            client.construct_url(
+                "users/get_info",
+                {"uid": "123"},
+                expires_in=expires_in,
+            )
+
+    def test_client_construct_url_rejects_expired_absolute_datetime(self):
+        """Test explicit expired absolute datetime expiries are rejected."""
+        client = Client("https://api.test.com", "test_akid", "test_password")
+        fixed_now = datetime(2026, 4, 1, 12, 0, 0)
+
+        with patch("labapi.client.datetime") as mock_datetime:
+            mock_datetime.now.return_value = fixed_now
+            with pytest.raises(ValueError, match="future datetime"):
+                client.construct_url(
+                    "users/get_info",
+                    {"uid": "123"},
+                    expires_in=fixed_now,
+                )
+
     def test_client_api_get_parses_raw_response_bytes(self):
         """Test Client.api_get parses response.content rather than re-encoded text."""
         client = Client("https://api.test.com", "test_akid", "test_password")
@@ -646,6 +677,47 @@ def test_default_authenticate_binds_listener_before_printing_auth_url(self):
         assert events.index("bind") < events.index("print")
         assert events.index("print") < events.index("wait")
 
+    def test_default_authenticate_warns_when_browser_cleanup_fails(self):
+        """Test browser cleanup failures do not mask successful authentication."""
+        client = Client("https://api.test.com", "test_akid", "test_password")
+        auth_response_collector = MagicMock()
+        auth_response_collector.__enter__.return_value = auth_response_collector
+        auth_response_collector.wait.return_value = Mock(spec=User)
+        driver = MagicMock()
+        driver.quit.side_effect = RuntimeError("quit failed")
+        webdriver = MagicMock()
+        webdriver.Chrome.return_value = driver
+
+        with (
+            patch.object(
+                client,
+                "generate_auth_url",
+                return_value="https://auth.test/url",
+            ),
+            patch.object(
+                client,
+                "collect_auth_response",
+                return_value=auth_response_collector,
+            ),
+            patch("labapi.client.token_urlsafe", return_value="test-token"),
+            patch("labapi.client.detect_default_browser", return_value="chrome"),
+            patch.dict(
+                "sys.modules",
+                {
+                    "selenium": MagicMock(webdriver=webdriver),
+                    "selenium.webdriver": webdriver,
+                },
+            ),
+            pytest.warns(
+                RuntimeWarning,
+                match="Failed to close authentication browser driver",
+            ),
+        ):
+            result = client.default_authenticate()
+
+        assert result is auth_response_collector.wait.return_value
+        driver.quit.assert_called_once_with()
+
     def test_collect_auth_response_binds_loopback_callback_listener(self):
         """Test auth callback collector binds the expected loopback listener."""
         client = Client("https://api.test.com", "test_akid", "test_password")
@@ -714,6 +786,39 @@ def test_collect_auth_response_requires_context_manager(self):
         ):
             auth_response_collector.wait()
 
+    @pytest.mark.parametrize("timeout", [0.0, -1.0])
+    def test_collect_auth_response_rejects_non_positive_timeout(self, timeout: float):
+        """Test callback collectors reject non-positive timeout values."""
+        client = Client("https://api.test.com", "test_akid", "test_password")
+
+        with pytest.raises(ValueError, match="timeout must be positive"):
+            client.collect_auth_response(timeout=timeout)
+
+    def test_collect_auth_response_sets_timeout_from_remaining_time(self):
+        """Test callback wait uses remaining timeout duration before blocking."""
+        client = Client("https://api.test.com", "test_akid", "test_password")
+        observed_timeouts: list[float | None] = []
+
+        class FakeTCPServer:
+            def __init__(self, _server_address, _handler_cls):
+                self.timeout: float | None = None
+
+            def handle_request(self):
+                observed_timeouts.append(self.timeout)
+
+            def server_close(self):
+                pass
+
+        with (
+            patch("labapi.client.TCPServer", FakeTCPServer),
+            patch("labapi.client.monotonic", side_effect=[100.0, 100.09, 100.11]),
+            client.collect_auth_response(timeout=0.1) as auth_response_collector,
+            pytest.raises(AuthenticationError, match="Timed out"),
+        ):
+            auth_response_collector.wait()
+
+        assert observed_timeouts == [pytest.approx(0.01)]
+
     def test_collect_auth_response_ignores_invalid_callbacks_until_valid(self):
         """Test auth callback collector ignores stray requests until the expected callback arrives."""
         client = Client("https://api.test.com", "test_akid", "test_password")