fix: address review — unused imports, host normalization, provider+scheme validation, security hardening

- Remove unused imports (os, field, Any) in config.py
- Normalize hosts during load (strip + lowercase)
- Validate token/token_env are non-empty strings during load
- Validate provider+scheme compatibility during load
- Fix extra_headers order: auth headers applied last, cannot be overridden
- Remove unused 'tried' variable in http.py
- Warn (once) on malformed auth.json instead of silent fallback
- URL-encode OAuth2 client credentials body in azure_devops.py
- Update 403 message to mention auth.json configuration
- Fix registry leak in test_register_duplicate (try/finally)
- Fix import style consistency in test_authentication.py
- Add azure-cli and azure-ad token acquisition tests (mock subprocess/urlopen)
- Add autouse fixture to isolate upgrade tests from real auth.json
- 1829 tests passing

Author: mnriem

src/specify_cli/__init__.py

@@ -1737,7 +1737,7 @@ def _fetch_latest_release_tag() -> tuple[str | None, str | None]:
     except urllib.error.HTTPError as e:
         # Order matters: HTTPError is a subclass of URLError.
         if e.code == 403:
-            return None, "rate limited (try setting GH_TOKEN or GITHUB_TOKEN)"
+            return None, "rate limited (try setting GH_TOKEN and configuring ~/.specify/auth.json)"
         return None, f"HTTP {e.code}"
     except (urllib.error.URLError, OSError):
         return None, "offline or timeout"

src/specify_cli/authentication/azure_devops.py

@@ -95,12 +95,13 @@ def _acquire_via_client_credentials(entry: AuthConfigEntry) -> str | None:
             f"https://login.microsoftonline.com/{entry.tenant_id}"
             "/oauth2/v2.0/token"
         )
-        body = (
-            f"grant_type=client_credentials"
-            f"&client_id={entry.client_id}"
-            f"&client_secret={client_secret}"
-            f"&scope={_ADO_RESOURCE_ID}/.default"
-        ).encode("utf-8")
+        from urllib.parse import urlencode
+        body = urlencode({
+            "grant_type": "client_credentials",
+            "client_id": entry.client_id,
+            "client_secret": client_secret,
+            "scope": f"{_ADO_RESOURCE_ID}/.default",
+        }).encode("utf-8")
 
         req = urllib.request.Request(
             url,

src/specify_cli/authentication/config.py

@@ -8,12 +8,10 @@
 from __future__ import annotations
 
 import json
-import os
 import stat
-from dataclasses import dataclass, field
+from dataclasses import dataclass
 from fnmatch import fnmatch
 from pathlib import Path
-from typing import Any
 from urllib.parse import urlparse
 
 
@@ -94,8 +92,10 @@ def load_auth_config(
         hosts = entry_raw.get("hosts")
         if not isinstance(hosts, list) or not hosts:
             raise ValueError(f"providers[{i}]: 'hosts' must be a non-empty array")
-        if not all(isinstance(h, str) and h for h in hosts):
+        if not all(isinstance(h, str) and h.strip() for h in hosts):
             raise ValueError(f"providers[{i}]: each host must be a non-empty string")
+        # Normalize hosts: strip whitespace and lowercase
+        hosts = [h.strip().lower() for h in hosts]
 
         provider = entry_raw.get("provider", "")
         if not isinstance(provider, str) or not provider:
@@ -113,6 +113,21 @@ def load_auth_config(
         token = entry_raw.get("token")
         token_env = entry_raw.get("token_env")
 
+        # Validate token/token_env types
+        if token is not None and (not isinstance(token, str) or not token.strip()):
+            raise ValueError(f"providers[{i}]: 'token' must be a non-empty string")
+        if token_env is not None and (not isinstance(token_env, str) or not token_env.strip()):
+            raise ValueError(f"providers[{i}]: 'token_env' must be a non-empty string")
+
+        # Validate provider+scheme compatibility
+        from . import get_provider as _get_provider
+        _prov = _get_provider(provider)
+        if _prov is not None and auth not in _prov.supported_auth_schemes:
+            raise ValueError(
+                f"providers[{i}]: provider {provider!r} does not support "
+                f"auth scheme {auth!r}; supported: {list(_prov.supported_auth_schemes)}"
+            )
+
         # Validate token source based on auth scheme
         if auth in ("bearer", "basic-pat"):
             if not token and not token_env:

src/specify_cli/authentication/http.py

@@ -29,7 +29,14 @@ def _load_config() -> list[AuthConfigEntry]:
         return _config_override
     try:
         return load_auth_config()
-    except (ValueError, OSError):
+    except (ValueError, OSError) as exc:
+        import warnings
+        warnings.warn(
+            f"Failed to load ~/.specify/auth.json: {exc}. "
+            "All requests will be unauthenticated.",
+            UserWarning,
+            stacklevel=2,
+        )
         return []
 
 
@@ -67,6 +74,9 @@ def build_request(url: str, extra_headers: dict[str, str] | None = None) -> urll
     Returns a plain request when no entry matches or the file doesn't exist.
     """
     headers: dict[str, str] = {}
+    if extra_headers:
+        headers.update(extra_headers)
+    # Auth headers applied last — cannot be overridden by extra_headers
     entries = find_entries_for_url(url, _load_config())
     for entry in entries:
         provider = get_provider(entry.provider)
@@ -76,8 +86,6 @@ def build_request(url: str, extra_headers: dict[str, str] | None = None) -> urll
         if token:
             headers.update(provider.auth_headers(token, entry.auth))
             break
-    if extra_headers:
-        headers.update(extra_headers)
     return urllib.request.Request(url, headers=headers)
 
 
@@ -95,21 +103,21 @@ def open_url(url: str, timeout: int = 10, extra_headers: dict[str, str] | None =
     entries = find_entries_for_url(url, _load_config())
 
     def _make_req(auth_headers: dict[str, str]) -> urllib.request.Request:
-        merged = {**auth_headers}
+        merged = {}
         if extra_headers:
             merged.update(extra_headers)
+        # Auth headers applied last — cannot be overridden by extra_headers
+        merged.update(auth_headers)
         return urllib.request.Request(url, headers=merged)
 
     # Try each matching entry
-    tried = 0
     for entry in entries:
         provider = get_provider(entry.provider)
         if provider is None:
             continue
         token = provider.resolve_token(entry)
         if not token:
             continue
-        tried += 1
 
         req = _make_req(provider.auth_headers(token, entry.auth))
         opener = urllib.request.build_opener(_StripAuthOnRedirect(entry.hosts))

tests/test_authentication.py

@@ -303,10 +303,12 @@ def test_register_duplicate_raises_key_error(self):
         class _UniqueStub(_StubProvider):
             key = "__test_duplicate__"
 
-        _register(_UniqueStub())
-        with pytest.raises(KeyError, match="already registered"):
+        try:
             _register(_UniqueStub())
-        AUTH_REGISTRY.pop("__test_duplicate__", None)
+            with pytest.raises(KeyError, match="already registered"):
+                _register(_UniqueStub())
+        finally:
+            AUTH_REGISTRY.pop("__test_duplicate__", None)
 
     def test_register_empty_key_raises_value_error(self):
         class _EmptyKey(_StubProvider):
@@ -406,6 +408,76 @@ def test_supported_schemes(self):
         assert "azure-cli" in schemes
         assert "azure-ad" in schemes
 
+    def test_resolve_token_azure_cli_success(self):
+        """azure-cli acquires token via az CLI."""
+        from unittest.mock import patch, MagicMock
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-cli",
+        )
+        result = MagicMock()
+        result.returncode = 0
+        result.stdout = '{"accessToken": "cli-acquired-token"}'
+        with patch("specify_cli.authentication.azure_devops.subprocess.run", return_value=result):
+            assert AzureDevOpsAuth().resolve_token(entry) == "cli-acquired-token"
+
+    def test_resolve_token_azure_cli_failure_returns_none(self):
+        """azure-cli returns None when az CLI fails."""
+        from unittest.mock import patch, MagicMock
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-cli",
+        )
+        result = MagicMock()
+        result.returncode = 1
+        result.stdout = ""
+        with patch("specify_cli.authentication.azure_devops.subprocess.run", return_value=result):
+            assert AzureDevOpsAuth().resolve_token(entry) is None
+
+    def test_resolve_token_azure_cli_not_installed_returns_none(self):
+        """azure-cli returns None when az is not installed."""
+        from unittest.mock import patch
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-cli",
+        )
+        with patch("specify_cli.authentication.azure_devops.subprocess.run", side_effect=OSError("not found")):
+            assert AzureDevOpsAuth().resolve_token(entry) is None
+
+    def test_resolve_token_azure_ad_success(self, monkeypatch):
+        """azure-ad acquires token via OAuth2 client credentials."""
+        from unittest.mock import patch, MagicMock
+        monkeypatch.setenv("MY_SECRET", "secret-value")
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-ad",
+            tenant_id="tid", client_id="cid", client_secret_env="MY_SECRET",
+        )
+        mock_resp = MagicMock()
+        mock_resp.read.return_value = b'{"access_token": "ad-acquired-token"}'
+        mock_resp.__enter__ = lambda s: s
+        mock_resp.__exit__ = MagicMock(return_value=False)
+        with patch("urllib.request.urlopen", return_value=mock_resp):
+            assert AzureDevOpsAuth().resolve_token(entry) == "ad-acquired-token"
+
+    def test_resolve_token_azure_ad_missing_secret_returns_none(self, monkeypatch):
+        """azure-ad returns None when client secret env var is missing."""
+        monkeypatch.delenv("MY_SECRET", raising=False)
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-ad",
+            tenant_id="tid", client_id="cid", client_secret_env="MY_SECRET",
+        )
+        assert AzureDevOpsAuth().resolve_token(entry) is None
+
+    def test_resolve_token_azure_ad_network_error_returns_none(self, monkeypatch):
+        """azure-ad returns None on network errors."""
+        import urllib.error
+        from unittest.mock import patch
+        monkeypatch.setenv("MY_SECRET", "secret-value")
+        entry = AuthConfigEntry(
+            hosts=("dev.azure.com",), provider="azure-devops", auth="azure-ad",
+            tenant_id="tid", client_id="cid", client_secret_env="MY_SECRET",
+        )
+        with patch("urllib.request.urlopen",
+                    side_effect=urllib.error.URLError("connection refused")):
+            assert AzureDevOpsAuth().resolve_token(entry) is None
+
 
 # ---------------------------------------------------------------------------
 # open_url / build_request — positive tests
@@ -414,7 +486,7 @@ def test_supported_schemes(self):
 
 class TestAuthenticatedHttp:
     def _set_config(self, monkeypatch, entries):
-        import specify_cli.authentication.http as _mod
+        from specify_cli.authentication import http as _mod
         monkeypatch.setattr(_mod, "_config_override", entries)
 
     def test_build_request_attaches_auth_for_matching_host(self, monkeypatch):
@@ -515,7 +587,7 @@ def fake_side_effect(req, timeout=None):
 
 class TestAuthenticatedHttpNegative:
     def _set_config(self, monkeypatch, entries):
-        import specify_cli.authentication.http as _mod
+        from specify_cli.authentication import http as _mod
         monkeypatch.setattr(_mod, "_config_override", entries)
 
     def test_500_raises_immediately(self, monkeypatch):
@@ -601,7 +673,7 @@ def test_redirect_outside_hosts_strips_auth(self):
 
 class TestFetchLatestReleaseTagDelegation:
     def _set_config(self, monkeypatch, entries):
-        import specify_cli.authentication.http as _mod
+        from specify_cli.authentication import http as _mod
         monkeypatch.setattr(_mod, "_config_override", entries)
 
     def _capture_request(self):

tests/test_upgrade.py

@@ -24,6 +24,13 @@
     app,
 )
 
+
+@pytest.fixture(autouse=True)
+def _isolate_auth_config(monkeypatch):
+    """Ensure tests never read the real ~/.specify/auth.json."""
+    from specify_cli.authentication import http as _mod
+    monkeypatch.setattr(_mod, "_config_override", [])
+
 from tests.conftest import strip_ansi
 
 runner = CliRunner()
@@ -223,7 +230,7 @@ def test_403_maps_to_rate_limited(self):
         ):
             tag, reason = _fetch_latest_release_tag()
         assert tag is None
-        assert reason == "rate limited (try setting GH_TOKEN or GITHUB_TOKEN)"
+        assert reason == "rate limited (try setting GH_TOKEN and configuring ~/.specify/auth.json)"
 
     @pytest.mark.parametrize("code", [404, 500, 502])
     def test_other_http_uses_code_string(self, code):
@@ -247,7 +254,7 @@ def test_generic_exception_propagates(self):
 
 _FAILURE_CASES = [
     ("offline or timeout", urllib.error.URLError("down")),
-    ("rate limited (try setting GH_TOKEN or GITHUB_TOKEN)", _http_error(403)),
+    ("rate limited (try setting GH_TOKEN and configuring ~/.specify/auth.json)", _http_error(403)),
     ("HTTP 500", _http_error(500)),
 ]
 
@@ -263,10 +270,10 @@ def test_failure_prints_installed_plus_one_line_reason(
             result = runner.invoke(app, ["self", "check"])
         output = strip_ansi(result.output)
         assert "Installed: 0.7.4" in output
-        if expected_reason == "rate limited (try setting GH_TOKEN or GITHUB_TOKEN)":
+        if expected_reason == "rate limited (try setting GH_TOKEN and configuring ~/.specify/auth.json)":
             assert "Could not check latest release: rate limited" in output
             assert "GH_TOKEN" in output
-            assert "GITHUB_TOKEN" in output
+            assert "auth.json" in output
         else:
             assert f"Could not check latest release: {expected_reason}" in output