fix(deps): add AAD bearer fallback and HTML sign-in detection in download_ado_file (closes #1671)

CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `apm install` now falls back to an AAD bearer token (via `az login`) when no
+  `ADO_APM_PAT` is configured for Azure DevOps file downloads, and fail-closes
+  when ADO returns an interactive HTML sign-in page with HTTP 200 instead of
+  writing corrupt HTML to disk. (by @danielmeppiel, closes #1671)
 - `apm install` now splits FQDN monorepo subpath shorthand on GitHub
   Enterprise Server hosts. With `GITHUB_HOST` set, a dependency string like
   `ghe.example.com/org/repo/packages/skill` resolves to `git: org/repo` plus

src/apm_cli/deps/download_strategies.py

@@ -495,8 +495,6 @@ def download_ado_file(
         Returns:
             bytes: File content
         """
-        import base64
-
         # Validate required ADO fields before proceeding
         if not all([dep_ref.ado_organization, dep_ref.ado_project, dep_ref.ado_repo]):
             raise ValueError(
@@ -516,15 +514,61 @@ def download_ado_file(
             host,
         )
 
-        # Set up authentication headers - ADO uses Basic auth with PAT
+        # Set up authentication headers.
+        # PAT path is first and unchanged; bearer is strictly the fallback
+        # when no PAT is present.  Bearer acquisition is routed through
+        # AuthResolver.resolve() so this module stays inside the auth-protocol
+        # boundary (scripts/lint-auth-signals.sh Rule A); auth.py's resolver
+        # handles the AAD bearer lookup internally.
         headers: dict[str, str] = {}
         if self._host.ado_token:
             # ADO uses Basic auth: username can be empty, password is the PAT
             auth = base64.b64encode(f":{self._host.ado_token}".encode()).decode()
             headers["Authorization"] = f"Basic {auth}"
+        else:
+            # No PAT: ask the resolver for an AAD bearer token.  If az-cli is
+            # available and the user is signed in, AuthResolver._resolve_token()
+            # returns a bearer token and auth_scheme="bearer" transparently.
+            auth_ctx = self._host.auth_resolver.resolve(
+                host,
+                dep_ref.ado_organization,
+                port=dep_ref.port,
+            )
+            if auth_ctx.token and auth_ctx.auth_scheme == "bearer":
+                headers["Authorization"] = f"Bearer {auth_ctx.token}"
+
+        def _check_html_signin(response) -> None:
+            """Fail-closed when ADO returns an interactive sign-in HTML page.
+
+            Azure DevOps responds with HTTP 200 + text/html when auth is
+            missing or insufficient instead of a 401.  Writing that HTML to
+            disk produces a corrupt file (the #1671 bug).  Detect it by
+            Content-Type only on 200 responses so 404/403 error pages with
+            text/html bodies still fall through to raise_for_status and the
+            existing 404-fallback / 401-403 error paths.  Content-Type is
+            lowercased before comparison per RFC 7230 case-insensitivity.
+            """
+            if response.status_code != 200:
+                return
+            content_type = response.headers.get("Content-Type", "").lower()
+            if "text/html" in content_type:
+                error_msg = (
+                    f"Azure DevOps returned a sign-in page for {dep_ref.repo_url}. "
+                    "The server responded with HTML instead of the requested file, "
+                    "which means authentication is missing or insufficient. "
+                )
+                error_msg += self._host.auth_resolver.build_error_context(
+                    host,
+                    "download",
+                    org=dep_ref.ado_organization if dep_ref else None,
+                    port=dep_ref.port if dep_ref else None,
+                    dep_url=dep_ref.repo_url if dep_ref else None,
+                )
+                raise RuntimeError(error_msg)
 
         try:
             response = self._host._resilient_get(api_url, headers=headers, timeout=30)
+            _check_html_signin(response)
             response.raise_for_status()
             return response.content
         except requests.exceptions.HTTPError as e:
@@ -547,6 +591,7 @@ def download_ado_file(
 
                 try:
                     response = self._host._resilient_get(fallback_url, headers=headers, timeout=30)
+                    _check_html_signin(response)
                     response.raise_for_status()
                     return response.content
                 except requests.exceptions.HTTPError as fallback_err:

tests/unit/deps/test_download_strategies_phase3.py

@@ -889,6 +889,162 @@ def test_no_ado_token_no_auth_header(self) -> None:
         d = DownloadDelegate(host)
         resp = _fake_response(200, b"ok")
         host._resilient_get.return_value = resp
+        # Simulate auth_resolver.resolve() returning no token (no PAT, no bearer)
+        ctx = MagicMock()
+        ctx.token = None
+        ctx.auth_scheme = "basic"
+        host.auth_resolver.resolve.return_value = ctx
+        d.download_ado_file(self._dep(), "apm.yml")
+        call_kwargs = host._resilient_get.call_args[1]
+        headers = call_kwargs.get("headers", {})
+        assert "Authorization" not in headers
+
+    # ------------------------------------------------------------------
+    # Regression tests for #1671: HTML sign-in page + bearer fallback
+    # ------------------------------------------------------------------
+
+    def test_html_200_response_raises_actionable_error(self) -> None:
+        """Regression #1671: ADO returns 200 + text/html when unauth; must fail-closed."""
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = None
+        ctx.auth_scheme = "basic"
+        host.auth_resolver.resolve.return_value = ctx
+        host.auth_resolver.build_error_context.return_value = "Set ADO_APM_PAT or run 'az login'."
+        html_body = b"<!DOCTYPE html><html><head><title>Azure DevOps Services | Sign In</title></head></html>"
+        resp = _fake_response(200, html_body, headers={"Content-Type": "text/html; charset=utf-8"})
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
+        with pytest.raises(RuntimeError, match="sign-in page"):
+            d.download_ado_file(self._dep(), "apm.yml")
+
+    def test_html_200_response_writes_no_content(self) -> None:
+        """Mutation break: HTML sign-in detection must prevent content from being returned."""
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = None
+        ctx.auth_scheme = "basic"
+        host.auth_resolver.resolve.return_value = ctx
+        host.auth_resolver.build_error_context.return_value = "Set ADO_APM_PAT."
+        html_body = b"<html><body>Sign In</body></html>"
+        resp = _fake_response(200, html_body, headers={"Content-Type": "text/html"})
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
+        # Assertions are placed OUTSIDE the pytest.raises block so they execute
+        # in the non-raising path; if the guard is removed, download_ado_file
+        # returns html_body and the assert below catches the regression.
+        raised = False
+        result = None
+        try:
+            result = d.download_ado_file(self._dep(), "apm.yml")
+        except RuntimeError:
+            raised = True
+        assert raised, "Expected RuntimeError for HTML sign-in page; guard may have been removed"
+        assert result is None, "Bug: HTML sign-in page was returned as file content"
+
+    def test_bearer_fallback_used_when_no_pat(self) -> None:
+        """Regression #1671: bearer token from auth_resolver used when no PAT present."""
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = "aad-bearer-jwt"
+        ctx.auth_scheme = "bearer"
+        host.auth_resolver.resolve.return_value = ctx
+        resp = _fake_response(
+            200, b"agent content", headers={"Content-Type": "application/octet-stream"}
+        )
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
+        result = d.download_ado_file(self._dep(), "apm.yml")
+        assert result == b"agent content"
+        call_kwargs = host._resilient_get.call_args[1]
+        headers = call_kwargs.get("headers", {})
+        assert headers.get("Authorization") == "Bearer aad-bearer-jwt"
+
+    def test_pat_path_unchanged_when_pat_present(self) -> None:
+        """PAT must remain the primary auth path; bearer is strictly the fallback."""
+        host = _make_host(ado_token="my-secret-pat")
+        resp = _fake_response(
+            200, b"file bytes", headers={"Content-Type": "application/octet-stream"}
+        )
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
+        result = d.download_ado_file(self._dep(), "apm.yml")
+        assert result == b"file bytes"
+        call_kwargs = host._resilient_get.call_args[1]
+        headers = call_kwargs.get("headers", {})
+        # PAT is used as Basic auth, bearer is NOT used
+        assert "Authorization" in headers
+        expected_auth = base64.b64encode(b":my-secret-pat").decode()
+        assert expected_auth in headers["Authorization"]
+        # auth_resolver.resolve() must NOT be called when PAT is present
+        host.auth_resolver.resolve.assert_not_called()
+
+    def test_bearer_auth_resolver_not_called_when_pat_present(self) -> None:
+        """Mutation break: resolver.resolve() is bypassed entirely when ado_token is set."""
+        host = _make_host(ado_token="pat-value")
+        resp = _fake_response(200, b"data", headers={"Content-Type": "application/json"})
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
+        d.download_ado_file(self._dep(), "apm.yml")
+        host.auth_resolver.resolve.assert_not_called()
+
+    def test_html_detection_on_fallback_ref_response(self) -> None:
+        """HTML sign-in detection fires on the main/master fallback response too."""
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = None
+        ctx.auth_scheme = "basic"
+        host.auth_resolver.resolve.return_value = ctx
+        host.auth_resolver.build_error_context.return_value = "Set ADO_APM_PAT."
+        fail_resp = _fake_response(404, b"")
+        html_resp = _fake_response(
+            200,
+            b"<html>Sign In</html>",
+            headers={"Content-Type": "text/html"},
+        )
+        host._resilient_get.side_effect = [fail_resp, html_resp]
+        d = DownloadDelegate(host)
+        with pytest.raises(RuntimeError, match="sign-in page"):
+            d.download_ado_file(self._dep(), "apm.yml", ref="main")
+
+    def test_404_with_html_content_type_does_not_trigger_sign_in_detection(self) -> None:
+        """Regression lock: 404 response with text/html body must NOT fire the HTML sign-in guard.
+
+        ADO can return 404 pages as text/html.  _check_html_signin must only fire
+        on 200 responses so 404s fall through to raise_for_status and then to the
+        main/master ref fallback path rather than raising a misleading 'sign-in page'
+        error.  This test proves the status-code guard added in the HTML detection fix.
+        """
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = None
+        ctx.auth_scheme = "basic"
+        host.auth_resolver.resolve.return_value = ctx
+        # First call: 404 with text/html (an ADO error page)
+        # Second call (fallback): 200 with real content
+        html_404 = _fake_response(
+            404, b"<html>Not Found</html>", headers={"Content-Type": "text/html"}
+        )
+        ok_resp = _fake_response(
+            200, b"file-bytes", headers={"Content-Type": "application/octet-stream"}
+        )
+        host._resilient_get.side_effect = [html_404, ok_resp]
+        d = DownloadDelegate(host)
+        # Should succeed via the main->master fallback, NOT raise "sign-in page"
+        result = d.download_ado_file(self._dep(), "apm.yml", ref="main")
+        assert result == b"file-bytes"
+        assert host._resilient_get.call_count == 2
+
+    def test_bearer_fallback_skipped_when_scheme_not_bearer(self) -> None:
+        """When auth_resolver returns a non-bearer scheme, no Authorization header is injected."""
+        host = _make_host(ado_token=None)
+        ctx = MagicMock()
+        ctx.token = "some-token"
+        ctx.auth_scheme = "basic"  # non-bearer scheme -> must not inject as Bearer
+        host.auth_resolver.resolve.return_value = ctx
+        resp = _fake_response(200, b"content", headers={"Content-Type": "application/octet-stream"})
+        host._resilient_get.return_value = resp
+        d = DownloadDelegate(host)
         d.download_ado_file(self._dep(), "apm.yml")
         call_kwargs = host._resilient_get.call_args[1]
         headers = call_kwargs.get("headers", {})