fix: upgrade http base URLs to https for OAuth request

AGENTS.md

@@ -50,9 +50,9 @@ Run each package's suite in a **separate** `pytest` invocation, as nox does —
 
 ### Type Checking
 ```bash
-nox -s basedpyright(bfabric)
-nox -s basedpyright(bfabric_scripts)
-nox -s basedpyright(bfabric_app_runner)
+nox -s "basedpyright(package='bfabric')"
+nox -s "basedpyright(package='bfabric_scripts')"
+nox -s "basedpyright(package='bfabric_app_runner')"
 ```
 
 ### Linting and formatting

bfabric/docs/changelog.md

@@ -9,6 +9,10 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them
 
 ## \[Unreleased\]
 
+### Fixed
+
+- OAuth requests to an instance recorded with an `http://` base URL are sent over `https://` instead of failing on the server's redirect (loopback hosts excepted). `bfabric.oauth.https_base_url` exposes the upgrade.
+
 ### Added
 
 - `upload_files` accepts `on_hash_progress`, called while each file's MD5 is computed; `bfabric-cli workunit upload` shows it as a progress bar.

bfabric/src/bfabric/oauth/__init__.py

@@ -3,7 +3,7 @@
 # Importing this module pulls authlib, joserfc and httpx; ``import bfabric`` alone does not.
 from bfabric.oauth._credential_provider import OAuthCredentialProvider
 from bfabric.oauth._device_code import device_code_login
-from bfabric.oauth._endpoints import token_url
+from bfabric.oauth._endpoints import https_base_url, token_url
 from bfabric.oauth._pkce import AuthorizationRequest, exchange_code, graphical_browser_available, pkce_login
 from bfabric.oauth._registration import (
     delete_client,
@@ -27,6 +27,7 @@
     "device_code_login",
     "exchange_code",
     "graphical_browser_available",
+    "https_base_url",
     "pkce_login",
     "read_client",
     "register_client",

bfabric/src/bfabric/oauth/_device_code.py

@@ -10,7 +10,7 @@
 from loguru import logger
 
 from bfabric.errors import BfabricOAuthError, raise_if_unavailable
-from bfabric.oauth._endpoints import token_url
+from bfabric.oauth._endpoints import https_base_url, token_url
 
 if TYPE_CHECKING:
     from bfabric.config.base_url import BaseUrl
@@ -31,7 +31,7 @@ def _request_device_code(
     logger.debug("Requesting device code from {}", base_url)
     with raise_if_unavailable(base_url):
         response = httpx.post(
-            f"{base_url}/rest/oauth/device_authorization",
+            f"{https_base_url(base_url)}/rest/oauth/device_authorization",
             data={
                 "client_id": client_id,
                 "scope": scope,

bfabric/src/bfabric/oauth/_endpoints.py

@@ -6,11 +6,27 @@
 
 from __future__ import annotations
 
-from urllib.parse import urlencode
+from urllib.parse import urlencode, urlsplit, urlunsplit
 
 from bfabric.config.base_url import BaseUrl
 
 
+_LOOPBACK_HOSTS = frozenset({"localhost", "127.0.0.1", "::1"})
+
+
+def https_base_url(base_url: str) -> str:
+    """*base_url* with ``http`` upgraded to ``https``, except for a loopback host (local development).
+
+    OAuth requests carry codes, secrets and refresh tokens, so they must not go out in the clear. A
+    config that still records ``http://`` would otherwise send the first request unencrypted and then
+    fail, because the server answers with a redirect that ``httpx`` does not follow.
+    """
+    parts = urlsplit(base_url)
+    if parts.scheme != "http" or parts.hostname in _LOOPBACK_HOSTS:
+        return base_url
+    return urlunsplit(parts._replace(scheme="https"))
+
+
 def authorize_url(
     base_url: str,
     *,
@@ -36,9 +52,9 @@ def authorize_url(
             "scope": scope,
         }
     )
-    return f"{BaseUrl(base_url)}/rest/oauth/authorize?{query}"
+    return f"{https_base_url(BaseUrl(base_url))}/rest/oauth/authorize?{query}"
 
 
 def token_url(base_url: str) -> str:
     """The token endpoint that issues, exchanges and refreshes tokens."""
-    return f"{BaseUrl(base_url)}/rest/oauth/token"
+    return f"{https_base_url(BaseUrl(base_url))}/rest/oauth/token"

bfabric/src/bfabric/oauth/_registration.py

@@ -8,6 +8,7 @@
 from loguru import logger
 
 from bfabric.errors import raise_if_unavailable
+from bfabric.oauth._endpoints import https_base_url
 
 if TYPE_CHECKING:
     from bfabric.bfabric import Bfabric
@@ -60,7 +61,7 @@ def register_client(
     :param grant_types: Explicit list of grant types to request (overrides the default)
     :returns: Registration response containing ``client_id``, ``client_secret``, etc.
     """
-    url = f"{base_url}/rest/oauth/register"
+    url = f"{https_base_url(base_url)}/rest/oauth/register"
     resolved_grant_types = grant_types if grant_types is not None else _default_grant_types(service_user)
     body: dict[str, object] = {
         "client_name": client_name,

bfabric/src/bfabric/oauth/_token_exchange.py

@@ -15,7 +15,7 @@
 
 from bfabric.errors import raise_if_unavailable
 
-from bfabric.oauth._endpoints import token_url
+from bfabric.oauth._endpoints import https_base_url, token_url
 from bfabric.oauth._url_token import UrlTokenContext
 
 if TYPE_CHECKING:
@@ -66,7 +66,7 @@ def introspect_token(
 
     :raises httpx.HTTPStatusError: On non-2xx responses
     """
-    url = f"{base_url}/rest/oauth/introspect"
+    url = f"{https_base_url(base_url)}/rest/oauth/introspect"
     logger.debug("Introspecting token at {}", url)
     with raise_if_unavailable(base_url):
         response = httpx.post(

bfabric/src/bfabric/oauth/_url_token.py

@@ -13,6 +13,7 @@
 
 from bfabric.config.base_url import BaseUrl
 from bfabric.errors import raise_if_unavailable
+from bfabric.oauth._endpoints import https_base_url
 from pydantic import BaseModel, ConfigDict, Field
 
 
@@ -73,7 +74,7 @@ def _fetch_jwks(base_url: BaseUrl) -> dict[str, object]:
                 return jwks
 
     logger.debug("Fetching JWKS from {}", base_url)
-    url = f"{base_url}/rest/oauth/jwks"
+    url = f"{https_base_url(base_url)}/rest/oauth/jwks"
     with raise_if_unavailable(base_url):
         response = httpx.get(url, timeout=30)
     _ = response.raise_for_status()

bfabric_scripts/docs/changelog.md

@@ -1,5 +1,9 @@
 ## \[Unreleased\]
 
+### Fixed
+
+- `bfabric-cli login` and `auth device-code` no longer fail on an environment recorded with an `http://` base URL; they use `https://` and save the corrected URL.
+
 ### Changed
 
 - `bfabric-cli login` switches to the device-code flow over SSH, without a display, or with only a text browser, unless `--no-browser` is given. `bfabric-cli workunit upload` shows a progress bar while hashing, including an overall total and ETA, hashes 4 files at once (`--hash-workers`), and reuses checksums of unchanged files from earlier runs (`--no-hash-cache` to disable). `--chunk-size` (MiB) sets the upload chunk size; it defaults to 32 instead of 4, which is faster on fast links.

bfabric_scripts/src/bfabric_scripts/cli/login/_common.py

@@ -18,6 +18,7 @@
 from bfabric_scripts.cli.interactive import confirm, is_interactive, select_choice, select_or_input, text_input
 from bfabric_scripts.cli.login._constants import SCOPE_PRESETS, SCOPE_PRESETS_BY_NAME
 from bfabric.config import BaseUrl
+from bfabric.oauth import https_base_url
 from bfabric_scripts.cli.login._urls import KNOWN_INSTANCES, normalize_base_url
 
 if TYPE_CHECKING:
@@ -69,7 +70,21 @@ def _pick_or_type(message: str, labels: dict[str, str], prompt: str) -> str | No
 
 
 def resolve_base_url(base_url: str | None, env: EnvironmentConfig | None) -> BaseUrl | None:
-    """Resolve the instance URL: explicit, else the environment's recorded one, else a picker."""
+    """Resolve the instance URL: explicit, else the environment's recorded one, else a picker.
+
+    A plain ``http`` URL is upgraded to ``https`` (loopback excepted), so a login never sends its OAuth
+    requests unencrypted and the environment is saved with the corrected URL.
+    """
+    resolved = _resolve_raw_base_url(base_url, env)
+    if resolved is None:
+        return None
+    secure = BaseUrl(https_base_url(resolved))
+    if secure != resolved:
+        print(f"Using {secure} instead of {resolved}: OAuth requires https.", file=sys.stderr)
+    return secure
+
+
+def _resolve_raw_base_url(base_url: str | None, env: EnvironmentConfig | None) -> BaseUrl | None:
     if base_url is not None:
         return normalize_base_url(base_url)
     if env is not None:

tests/bfabric/oauth/test_device_code.py

@@ -41,6 +41,14 @@ def test_posts_correct_params(self, mocker):
         )
         assert result == mock_response.json.return_value
 
+    def test_upgrades_a_plain_http_base_url(self, mocker):
+        """A recorded ``http://`` URL would otherwise be sent in the clear, then fail on the server's redirect."""
+        mock_post = mocker.patch("bfabric.oauth._device_code.httpx.post")
+
+        _request_device_code("http://example.com/bfabric", client_id="c", scope="api:read")
+
+        assert mock_post.call_args.args[0] == "https://example.com/bfabric/rest/oauth/device_authorization"
+
     def test_raises_on_http_error(self, mocker):
         mock_response = mocker.MagicMock()
         mock_response.raise_for_status.side_effect = httpx.HTTPStatusError(

tests/bfabric/oauth/test_endpoints.py

@@ -3,7 +3,8 @@
 import pytest
 
 from bfabric.config.base_url import BaseUrl
-from bfabric.oauth import token_url
+from bfabric.oauth import https_base_url, token_url
+from bfabric.oauth._endpoints import authorize_url
 
 
 class TestTokenUrl:
@@ -17,3 +18,34 @@ def test_validates_the_base_url(self):
         # Only that the validation happens -- what BaseUrl accepts is tested where BaseUrl lives.
         with pytest.raises(ValueError):
             token_url("https://example.com/notbfabric")
+
+
+class TestHttpsBaseUrl:
+    def test_upgrades_http(self):
+        assert https_base_url("http://fgcz-bfabric.uzh.ch/bfabric") == "https://fgcz-bfabric.uzh.ch/bfabric"
+
+    def test_keeps_the_port(self):
+        assert https_base_url("http://example.com:8080/bfabric") == "https://example.com:8080/bfabric"
+
+    def test_leaves_https_alone(self):
+        assert https_base_url("https://example.com/bfabric") == "https://example.com/bfabric"
+
+    @pytest.mark.parametrize(
+        "url", ["http://localhost:8080/bfabric", "http://127.0.0.1:8000/bfabric", "http://[::1]:8000/bfabric"]
+    )
+    def test_leaves_loopback_alone_for_local_development(self, url):
+        assert https_base_url(url) == url
+
+    def test_token_url_uses_https(self):
+        assert token_url("http://example.com/bfabric") == "https://example.com/bfabric/rest/oauth/token"
+
+    def test_authorize_url_uses_https(self):
+        url = authorize_url(
+            "http://example.com/bfabric",
+            client_id="c",
+            redirect_uri="http://127.0.0.1:1/callback",
+            code_challenge="x",
+            state="s",
+            scope="api:read",
+        )
+        assert url.startswith("https://example.com/bfabric/rest/oauth/authorize?")

tests/bfabric_scripts/cli/login/test_login_common.py

@@ -186,6 +186,17 @@ def test_falls_back_to_the_recorded_base_url(self):
         env = EnvironmentConfig.model_validate({"base_url": "https://recorded.example.com/bfabric"})
         assert resolve_base_url(None, env) == "https://recorded.example.com/bfabric"
 
+    def test_recorded_http_url_is_upgraded_to_https(self, capsys):
+        env = EnvironmentConfig.model_validate({"base_url": "http://recorded.example.com/bfabric"})
+        assert resolve_base_url(None, env) == "https://recorded.example.com/bfabric"
+        assert "https" in capsys.readouterr().err
+
+    def test_explicit_http_url_is_upgraded_to_https(self):
+        assert resolve_base_url("http://typed.example.com/bfabric", None) == "https://typed.example.com/bfabric"
+
+    def test_explicit_loopback_http_url_is_kept(self):
+        assert resolve_base_url("http://localhost:8000/bfabric", None) == "http://localhost:8000/bfabric"
+
     def test_explicit_value_wins_over_the_recorded_one(self):
         env = EnvironmentConfig.model_validate({"base_url": "https://recorded.example.com/bfabric"})
         assert resolve_base_url("https://typed.example.com/bfabric", env) == "https://typed.example.com/bfabric"