From 91e36d2d01df2b96e3f6cc4005663ee0dd76ad23 Mon Sep 17 00:00:00 2001 From: Jacob O'Keeffe Date: Mon, 13 Apr 2026 11:47:26 +0100 Subject: [PATCH] fix: marketplace add authenticates for private repos (#669) _do_fetch() returned None on unauthenticated 404, preventing try_with_fallback from retrying with a token. GitHub returns 404 (not 403) for private repos to hide existence, so the auth escalation never fired. Now raise on 404 when no token is present so the fallback retries authenticated. Authenticated 404 still returns None (file genuinely missing). Public repos are unaffected. --- src/apm_cli/marketplace/client.py | 21 +- .../marketplace/test_marketplace_client.py | 303 ++++++++++++------ 2 files changed, 228 insertions(+), 96 deletions(-) diff --git a/src/apm_cli/marketplace/client.py b/src/apm_cli/marketplace/client.py index 6fba4edf..2cae8069 100644 --- a/src/apm_cli/marketplace/client.py +++ b/src/apm_cli/marketplace/client.py @@ -18,7 +18,12 @@ import requests from .errors import MarketplaceFetchError -from .models import MarketplaceManifest, MarketplacePlugin, MarketplaceSource, parse_marketplace_json +from .models import ( + MarketplaceManifest, + MarketplacePlugin, + MarketplaceSource, + parse_marketplace_json, +) from .registry import get_registered_marketplaces logger = logging.getLogger(__name__) @@ -173,7 +178,9 @@ def _try_proxy_fetch( except (json.JSONDecodeError, ValueError): logger.debug( "Proxy returned non-JSON for %s/%s %s", - source.owner, source.repo, file_path, + source.owner, + source.repo, + file_path, ) return None @@ -213,7 +220,9 @@ def _fetch_file( if cfg is not None and cfg.enforce_only: logger.debug( "PROXY_REGISTRY_ONLY blocks direct GitHub fetch for %s/%s %s", - source.owner, source.repo, file_path, + source.owner, + source.repo, + file_path, ) return None @@ -229,6 +238,12 @@ def _do_fetch(token, _git_env): headers["Authorization"] = f"token {token}" resp = requests.get(url, headers=headers, timeout=30) if resp.status_code == 404: + if not token: + # Unauthenticated 404 is ambiguous: could be a genuinely + # missing file *or* a private repo hiding its existence. + # Raise so that ``try_with_fallback`` retries with a token. + resp.raise_for_status() + # Authenticated 404 means the file genuinely does not exist. return None resp.raise_for_status() return resp.json() diff --git a/tests/unit/marketplace/test_marketplace_client.py b/tests/unit/marketplace/test_marketplace_client.py index cfa053bf..878ee558 100644 --- a/tests/unit/marketplace/test_marketplace_client.py +++ b/tests/unit/marketplace/test_marketplace_client.py @@ -5,6 +5,7 @@ from unittest.mock import MagicMock, patch import pytest +import requests from apm_cli.marketplace.errors import MarketplaceFetchError from apm_cli.marketplace.models import MarketplaceSource @@ -16,7 +17,9 @@ def _isolate_cache(tmp_path, monkeypatch): """Point cache and config to temp directories.""" config_dir = str(tmp_path / ".apm") monkeypatch.setattr("apm_cli.config.CONFIG_DIR", config_dir) - monkeypatch.setattr("apm_cli.config.CONFIG_FILE", str(tmp_path / ".apm" / "config.json")) + monkeypatch.setattr( + "apm_cli.config.CONFIG_FILE", str(tmp_path / ".apm" / "config.json") + ) monkeypatch.setattr("apm_cli.config._config_cache", None) monkeypatch.setattr("apm_cli.marketplace.registry._registry_cache", None) yield @@ -85,7 +88,9 @@ def test_fetch_from_network(self, tmp_path): } mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = raw_data - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) manifest = client_mod.fetch_marketplace( source, force_refresh=True, auth_resolver=mock_resolver @@ -113,7 +118,9 @@ def test_force_refresh_bypasses_cache(self, tmp_path): new_data = {"name": "Fresh", "plugins": [{"name": "new", "repository": "o/r"}]} mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = new_data - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) manifest = client_mod.fetch_marketplace( source, force_refresh=True, auth_resolver=mock_resolver @@ -133,18 +140,20 @@ def test_stale_while_revalidate(self, tmp_path): # Network fetch will fail mock_resolver = MagicMock() mock_resolver.try_with_fallback.side_effect = Exception("Network error") - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - - manifest = client_mod.fetch_marketplace( - source, auth_resolver=mock_resolver + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" ) + + manifest = client_mod.fetch_marketplace(source, auth_resolver=mock_resolver) assert manifest.name == "Stale" # Falls back to stale cache def test_no_cache_no_network_raises(self, tmp_path): source = _make_source() mock_resolver = MagicMock() mock_resolver.try_with_fallback.side_effect = Exception("Network error") - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) with pytest.raises(MarketplaceFetchError): client_mod.fetch_marketplace( @@ -164,7 +173,9 @@ def mock_fetch(host, op, org=None, unauth_first=False): return {"name": "Test", "plugins": []} mock_resolver.try_with_fallback.side_effect = mock_fetch - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) path = client_mod._auto_detect_path(source, auth_resolver=mock_resolver) assert path == "marketplace.json" @@ -183,7 +194,9 @@ def mock_fetch(host, op, org=None, unauth_first=False): return {"name": "Test", "plugins": []} mock_resolver.try_with_fallback.side_effect = mock_fetch - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) path = client_mod._auto_detect_path(source, auth_resolver=mock_resolver) assert path == ".github/plugin/marketplace.json" @@ -192,7 +205,9 @@ def test_not_found_anywhere(self, tmp_path): source = _make_source() mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = None - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) path = client_mod._auto_detect_path(source, auth_resolver=mock_resolver) assert path is None @@ -201,7 +216,10 @@ def test_not_found_anywhere(self, tmp_path): class TestProxyAwareFetch: """Proxy-aware marketplace fetch via Artifactory Archive Entry Download.""" - _MARKETPLACE_JSON = {"name": "Test", "plugins": [{"name": "p1", "repository": "o/r"}]} + _MARKETPLACE_JSON = { + "name": "Test", + "plugins": [{"name": "p1", "repository": "o/r"}], + } def _make_cfg(self, enforce_only=False): cfg = MagicMock() @@ -217,8 +235,15 @@ def test_proxy_fetch_success(self): source = _make_source() cfg = self._make_cfg() raw = json.dumps(self._MARKETPLACE_JSON).encode() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=raw) as mock_fetch: + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + return_value=raw, + ) as mock_fetch, + ): result = client_mod._fetch_file(source, "marketplace.json") assert result == self._MARKETPLACE_JSON @@ -237,12 +262,23 @@ def test_proxy_none_falls_through_to_github(self): """Proxy returns None, no enforce_only -- falls through to GitHub API.""" source = _make_source() cfg = self._make_cfg(enforce_only=False) - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None): + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + return_value=None, + ), + ): mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = self._MARKETPLACE_JSON - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) + result = client_mod._fetch_file( + source, "marketplace.json", auth_resolver=mock_resolver + ) assert result == self._MARKETPLACE_JSON mock_resolver.try_with_fallback.assert_called_once() @@ -251,10 +287,19 @@ def test_proxy_only_blocks_github_fallback(self): """Proxy returns None + enforce_only -- returns None, no GitHub call.""" source = _make_source() cfg = self._make_cfg(enforce_only=True) - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None): + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + return_value=None, + ), + ): mock_resolver = MagicMock() - result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + result = client_mod._fetch_file( + source, "marketplace.json", auth_resolver=mock_resolver + ) assert result is None mock_resolver.try_with_fallback.assert_not_called() @@ -262,11 +307,17 @@ def test_proxy_only_blocks_github_fallback(self): def test_no_proxy_uses_github(self): """No proxy configured -- standard GitHub API path.""" source = _make_source() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None): + with patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None + ): mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = self._MARKETPLACE_JSON - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) + result = client_mod._fetch_file( + source, "marketplace.json", auth_resolver=mock_resolver + ) assert result == self._MARKETPLACE_JSON @@ -274,12 +325,23 @@ def test_proxy_non_json_falls_through(self): """Proxy returns non-JSON bytes -- treated as failure, falls to GitHub.""" source = _make_source() cfg = self._make_cfg(enforce_only=False) - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=b"\x89PNG binary"): + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + return_value=b"\x89PNG binary", + ), + ): mock_resolver = MagicMock() mock_resolver.try_with_fallback.return_value = self._MARKETPLACE_JSON - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + mock_resolver.classify_host.return_value = MagicMock( + api_base="https://api.github.com" + ) + result = client_mod._fetch_file( + source, "marketplace.json", auth_resolver=mock_resolver + ) assert result == self._MARKETPLACE_JSON @@ -295,8 +357,15 @@ def mock_entry(*args, **kwargs): return None # first candidate not found return json.dumps(self._MARKETPLACE_JSON).encode() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", side_effect=mock_entry): + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + side_effect=mock_entry, + ), + ): path = client_mod._auto_detect_path(source) assert path == ".github/plugin/marketplace.json" @@ -306,8 +375,15 @@ def test_fetch_marketplace_via_proxy_end_to_end(self): source = _make_source() cfg = self._make_cfg() raw = json.dumps(self._MARKETPLACE_JSON).encode() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg), \ - patch("apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=raw): + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=cfg + ), + patch( + "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", + return_value=raw, + ), + ): manifest = client_mod.fetch_marketplace(source, force_refresh=True) assert manifest.name == "Test" @@ -315,83 +391,122 @@ def test_fetch_marketplace_via_proxy_end_to_end(self): assert manifest.plugins[0].name == "p1" -@patch("apm_cli.marketplace.client._try_proxy_fetch", return_value=None) class TestPrivateRepoAuth: - """Verify unauth_first=False so private repos get credentials before unauthenticated fallback. + """Regression tests for private-repo marketplace auth (GitHub #669). - GitHub returns 404 (not 403) for unauthenticated requests to private repos. - With unauth_first=True the old code would try unauthenticated first, receive a 404, and - silently treat the repo as non-existent. The fix sets unauth_first=False so the token - is used on the first attempt. + ``_do_fetch`` must raise on unauthenticated 404 so that + ``try_with_fallback`` escalates to an authenticated retry. + An authenticated 404 should still return ``None`` (file not found). """ - _MARKETPLACE_JSON = {"name": "Private Plugins", "plugins": []} + _MARKETPLACE_JSON = { + "name": "Test", + "plugins": [{"name": "p1", "repository": "o/r"}], + } + + def _make_mock_response(self, status_code, json_data=None): + resp = MagicMock() + resp.status_code = status_code + resp.json.return_value = json_data + resp.raise_for_status.side_effect = ( + requests.HTTPError(response=resp) if status_code >= 400 else None + ) + return resp - def test_fetch_file_private_repo_auth_first(self, _proxy): - """_fetch_file passes unauth_first=False so private repos are reached via auth first.""" + def test_private_repo_unauthenticated_404_retries_with_token(self, monkeypatch): + """Unauthenticated 404 on a private repo should trigger auth fallback + and succeed when the token grants access.""" source = _make_source() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None): - mock_resolver = MagicMock() - mock_resolver.try_with_fallback.return_value = self._MARKETPLACE_JSON - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") + monkeypatch.setenv("GITHUB_TOKEN", "ghp_test_token") - result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + call_count = [0] + + def mock_get(url, headers=None, timeout=None): + call_count[0] += 1 + if headers and "Authorization" in headers: + # Authenticated request -> success + return self._make_mock_response(200, self._MARKETPLACE_JSON) + # Unauthenticated request -> 404 (private repo) + return self._make_mock_response(404) + + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None + ), + patch("apm_cli.marketplace.client.requests.get", side_effect=mock_get), + ): + result = client_mod._fetch_file(source, "marketplace.json") assert result == self._MARKETPLACE_JSON - mock_resolver.try_with_fallback.assert_called_once() - _, call_kwargs = mock_resolver.try_with_fallback.call_args - assert call_kwargs.get("unauth_first") is False, ( - "unauth_first must be False -- private repos respond 404 to unauthenticated requests" + assert call_count[0] == 2, ( + "Should have made 2 requests (unauth 404, then auth success)" ) - def test_fetch_file_no_proxy_passes_unauth_first_false(self, _proxy): - """With no proxy, try_with_fallback is explicitly called with unauth_first=False (not True).""" + def test_authenticated_404_returns_none(self, monkeypatch): + """When the token is valid but the file genuinely doesn't exist, + _fetch_file should return None (not raise).""" source = _make_source() - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None): - mock_resolver = MagicMock() - # Simulate private repo returning None (404) for unauthenticated; would succeed with auth - mock_resolver.try_with_fallback.return_value = None - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - - client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_test_token") + + def mock_get(url, headers=None, timeout=None): + # Both unauth and auth return 404 -- file really doesn't exist + return self._make_mock_response(404) + + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None + ), + patch("apm_cli.marketplace.client.requests.get", side_effect=mock_get), + ): + result = client_mod._fetch_file(source, "marketplace.json") - mock_resolver.try_with_fallback.assert_called_once() - call_kwargs = mock_resolver.try_with_fallback.call_args.kwargs - assert "unauth_first" in call_kwargs, ( - "unauth_first kwarg must be passed explicitly to try_with_fallback" - ) - assert call_kwargs["unauth_first"] is False, ( - f"Expected unauth_first=False, got {call_kwargs['unauth_first']!r}" - ) + assert result is None - def test_auto_detect_private_repo_succeeds_with_auth(self, _proxy): - """_auto_detect_path finds a private repo's manifest via auth on the third candidate path.""" + def test_public_repo_unauthenticated_success(self, monkeypatch): + """Public repos should still work without auth (no regression).""" source = _make_source() - call_count = [0] + # No token set -- ensure unauthenticated path works for public repos + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_APM_PAT", raising=False) + + def mock_get(url, headers=None, timeout=None): + return self._make_mock_response(200, self._MARKETPLACE_JSON) + + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None + ), + patch("apm_cli.marketplace.client.requests.get", side_effect=mock_get), + patch( + "apm_cli.core.auth.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), + ): + result = client_mod._fetch_file(source, "marketplace.json") - def mock_try_with_fallback(host, op, org=None, unauth_first=False): - call_count[0] += 1 - if call_count[0] < 3: - # marketplace.json and .github/plugin/marketplace.json: 404 on private repo - return None - # .claude-plugin/marketplace.json: found with auth - return self._MARKETPLACE_JSON + assert result == self._MARKETPLACE_JSON - mock_resolver = MagicMock() - mock_resolver.try_with_fallback.side_effect = mock_try_with_fallback - mock_resolver.classify_host.return_value = MagicMock(api_base="https://api.github.com") - - with patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None): - path = client_mod._auto_detect_path(source, auth_resolver=mock_resolver) - - assert path == ".claude-plugin/marketplace.json" - # All three candidates were probed before finding it on the third - assert mock_resolver.try_with_fallback.call_count == 3 - # Every probe used unauth_first=False (auth credentials always tried first) - for call in mock_resolver.try_with_fallback.call_args_list: - assert call.kwargs.get("unauth_first") is False, ( - f"Expected unauth_first=False for all probes, got {call.kwargs!r}" - ) + def test_auto_detect_private_repo_finds_marketplace_json(self, monkeypatch): + """_auto_detect_path should find marketplace.json in a private repo + after auth fallback.""" + source = _make_source() + monkeypatch.setenv("GITHUB_TOKEN", "ghp_test_token") + + def mock_get(url, headers=None, timeout=None): + if headers and "Authorization" in headers: + return self._make_mock_response(200, self._MARKETPLACE_JSON) + return self._make_mock_response(404) + + with ( + patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None + ), + patch("apm_cli.marketplace.client.requests.get", side_effect=mock_get), + ): + path = client_mod._auto_detect_path(source) + + assert path == "marketplace.json" class TestCacheKey: @@ -402,7 +517,9 @@ def test_github_default_unchanged(self): assert client_mod._cache_key(source) == "skills" def test_non_default_host_includes_host(self): - source = MarketplaceSource(name="skills", owner="o", repo="r", host="ghes.corp.com") + source = MarketplaceSource( + name="skills", owner="o", repo="r", host="ghes.corp.com" + ) key = client_mod._cache_key(source) assert key.startswith("ghes.corp.com") or key.startswith("ghes_corp_com") assert key.endswith("skills")