diff --git a/CHANGELOG.md b/CHANGELOG.md index e64564802..b8606c180 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index 239f17561..2c7bdbf78 100644 --- a/src/apm_cli/deps/download_strategies.py +++ b/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: diff --git a/tests/unit/deps/test_download_strategies_phase3.py b/tests/unit/deps/test_download_strategies_phase3.py index d01544c74..211e91b28 100644 --- a/tests/unit/deps/test_download_strategies_phase3.py +++ b/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"Azure DevOps Services | Sign In" + 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"Sign In" + 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"Sign In", + 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"Not Found", 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", {})