From 2bbfe71df478b96085700e6881f31b662487a546 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 5 Jun 2026 12:22:30 +0200 Subject: [PATCH 1/2] fix(deps): add AAD bearer fallback and HTML sign-in detection in download_ado_file (closes #1671) - When ADO_APM_PAT is absent, resolve an AAD bearer token via AuthResolver.resolve() (which internally uses az-cli); inject it as Authorization: Bearer . PAT path is first and unchanged. - Detect ADO's silent HTTP 200 + text/html sign-in-page response; raise an actionable RuntimeError (via build_error_context) instead of writing the HTML to disk. Detection fires on both the primary and the main/master fallback requests. - Auth-boundary lint (scripts/lint-auth-signals.sh Rule A) is satisfied: download_strategies.py routes through AuthResolver, never imports the bearer provider directly. - Regression tests added: HTML-200 raises, bearer header set when no PAT, PAT path unchanged, resolver bypassed when PAT present, fallback-ref HTML detection, mutation-break assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/deps/download_strategies.py | 47 +++++++- .../deps/test_download_strategies_phase3.py | 107 ++++++++++++++++++ 2 files changed, 151 insertions(+), 3 deletions(-) diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index 239f17561..cb07ad23e 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,57 @@ 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 and raise an actionable error before any bytes are + returned to the caller. + """ + content_type = response.headers.get("Content-Type", "") + 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 +587,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..47c6a45dc 100644 --- a/tests/unit/deps/test_download_strategies_phase3.py +++ b/tests/unit/deps/test_download_strategies_phase3.py @@ -889,11 +889,118 @@ 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) + # Must raise, never return html bytes as if they were file content + with pytest.raises(RuntimeError): + result = d.download_ado_file(self._dep(), "apm.yml") + # If we reach here, the bug is present: HTML was returned as content + assert result != html_body, "Bug: HTML sign-in page 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") + # --------------------------------------------------------------------------- # download_gitlab_file From 101bf4e6ef671d9a4acfc1d656d4608e4c0ea64b Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 5 Jun 2026 12:40:16 +0200 Subject: [PATCH 2/2] fix(deps): guard _check_html_signin on status 200, normalize Content-Type, fix test assertions - Add early return in _check_html_signin when response.status_code != 200 so 404/403 responses with text/html bodies fall through to raise_for_status and the existing 404-fallback / 401-403 error paths (addresses Copilot review and panel blocking finding: misleading 'sign-in page' error on non-200 responses). - Lowercase Content-Type before substring match per RFC 7230 case-insensitivity (addresses Copilot review and panel blocking finding: bypass possible with 'Text/HTML' or 'TEXT/HTML'). - Restructure test_html_200_response_writes_no_content to place assertions outside the pytest.raises context manager so they execute in the non-raising (buggy) path (addresses Copilot review and test-coverage-expert: dead assertions). - Add test_404_with_html_content_type_does_not_trigger_sign_in_detection to lock in the status-code guard: 404+text/html must reach the main/master fallback. - Add test_bearer_fallback_skipped_when_scheme_not_bearer to confirm non-bearer scheme tokens are not injected as Bearer headers. - Add CHANGELOG entry for #1671 fix. All folds: addresses panel CEO follow-ups FU-1 (status-code guard), FU-2 (case-normalization), FU-3 (dead assertions), FU-4 (CHANGELOG), FU-5 (404+html regression trap); also addresses Copilot inline comments id:3362036952 and id:3362036988. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 ++ src/apm_cli/deps/download_strategies.py | 10 +++- .../deps/test_download_strategies_phase3.py | 57 +++++++++++++++++-- 3 files changed, 64 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 795425092..9447e0663 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 keeps format-transformed rule files (`.claude/rules`, `.cursor/rules`, `.windsurf/rules`) tracked in `managed_files` and rewrites them when the source instruction changes, instead of mis-classifying them as diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index cb07ad23e..2c7bdbf78 100644 --- a/src/apm_cli/deps/download_strategies.py +++ b/src/apm_cli/deps/download_strategies.py @@ -543,10 +543,14 @@ def _check_html_signin(response) -> None: 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 and raise an actionable error before any bytes are - returned to the caller. + 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. """ - content_type = response.headers.get("Content-Type", "") + 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}. " diff --git a/tests/unit/deps/test_download_strategies_phase3.py b/tests/unit/deps/test_download_strategies_phase3.py index 47c6a45dc..211e91b28 100644 --- a/tests/unit/deps/test_download_strategies_phase3.py +++ b/tests/unit/deps/test_download_strategies_phase3.py @@ -930,11 +930,17 @@ def test_html_200_response_writes_no_content(self) -> None: resp = _fake_response(200, html_body, headers={"Content-Type": "text/html"}) host._resilient_get.return_value = resp d = DownloadDelegate(host) - # Must raise, never return html bytes as if they were file content - with pytest.raises(RuntimeError): + # 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") - # If we reach here, the bug is present: HTML was returned as content - assert result != html_body, "Bug: HTML sign-in page returned as file content" + 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.""" @@ -1001,6 +1007,49 @@ def test_html_detection_on_fallback_ref_response(self) -> None: 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", {}) + assert "Authorization" not in headers + # --------------------------------------------------------------------------- # download_gitlab_file