fix: marketplace add authenticates for private repos#688
fix: marketplace add authenticates for private repos#688jacobokeeffe-ow wants to merge 1 commit intomicrosoft:mainfrom
Conversation
jacobokeeffe-ow
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
|
@microsoft-github-policy-service agree company="Oliver Wyman" |
|
@sergio-sisternes-epam apologies for the formatting changes but when I run I might be mistaken but I don't see |
sergio-sisternes-epam
left a comment
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Clean fix -- the 6-line change in _do_fetch() correctly distinguishes "private repo hiding existence" from "file not found" by raising on unauthenticated 404 to trigger try_with_fallback auth retry. Good test coverage with 4 regression tests.
Re: formatting -- we don't enforce black in CI, so the reformatting here is fine (contained to files you touched). We should update the contributing guide to clarify this. Thanks for flagging it!
Note: CI hasn't run yet (fork PR). Would be good to get Build & Test triggered before merge.
|
thanks @sergio-sisternes-epam. I think you might need to give approval in order the get the github action to run? https://github.com/microsoft/apm/actions/runs/24339327316 : "This workflow is awaiting approval from a maintainer" |
_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.
jacobokeeffe-ow
force-pushed
the
fix/marketplace-add-private-repo-auth
branch
from
a25b049 to
91e36d2
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes apm marketplace add failing against private GitHub repositories by ensuring unauthenticated 404s trigger an authenticated retry, matching GitHub’s “private repo returns 404” behavior.
Changes:
- Update GitHub contents fetch logic to raise on unauthenticated 404s so auth fallback engages.
- Add regression tests covering private-repo auth fallback and preserving public-repo behavior.
- Reformat several tests for readability (multi-line patches, literals, and mocks).
Show a summary per file
| File | Description |
|---|---|
| tests/unit/marketplace/test_marketplace_client.py | Adds regression tests for private repo auth fallback and refactors existing test formatting. |
| src/apm_cli/marketplace/client.py | Adjusts 404 handling in _do_fetch to raise on unauthenticated 404 to trigger auth fallback. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 4
| 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 |
There was a problem hiding this comment.
Raising on unauthenticated 404s can break the documented contract that 404 => None when no credentials are available (e.g., public repo where the candidate path genuinely doesn’t exist). This can also break _auto_detect_path probing because a missing first candidate would raise instead of returning None to continue to the next path. A concrete fix is to translate an ultimate 404 exception (after try_with_fallback has exhausted available credentials) back into None in _fetch_file (e.g., catch requests.HTTPError, inspect exc.response.status_code == 404, and return None), while still allowing the initial unauthenticated 404 to raise to trigger the authenticated retry.
| 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() |
There was a problem hiding this comment.
The PR description and new tests describe an unauthenticated-first flow (unauth 404, then authenticated retry) to preserve rate-limit optimization for public repos, but the current implementation details shown elsewhere indicate try_with_fallback(..., unauth_first=False) (auth-first). Since this 404-raise logic only helps if an unauthenticated attempt happens before an authenticated one, please align the actual try_with_fallback call behavior with the PR description/tests (either switch to unauth-first + this exception strategy, or update the description/tests if auth-first is the intended behavior).
| 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") |
There was a problem hiding this comment.
This test claims to validate the unauthenticated path, but it doesn’t assert that the outgoing request omitted the Authorization header. In environments where credentials may be discovered from non-env sources (e.g., other token providers not patched here), the test could still pass while exercising an authenticated request. Strengthen the test by asserting on the captured headers for the requests.get call(s) (e.g., ensure Authorization is absent) and/or patch the credential resolution entry-point used by the resolver in this path so the test deterministically covers the unauthenticated branch.
| resp.raise_for_status.side_effect = ( | ||
| requests.HTTPError(response=resp) if status_code >= 400 else None | ||
| ) |
There was a problem hiding this comment.
For successful responses (status_code < 400), setting raise_for_status.side_effect = None still leaves raise_for_status() returning a MagicMock, which diverges from requests.Response.raise_for_status() returning None. Consider setting resp.raise_for_status.return_value = None for the non-error case (and using side_effect only for the error case) to better mimic real behavior and reduce confusion in future tests.
| resp.raise_for_status.side_effect = ( | |
| requests.HTTPError(response=resp) if status_code >= 400 else None | |
| ) | |
| if status_code >= 400: | |
| resp.raise_for_status.side_effect = requests.HTTPError(response=resp) | |
| else: | |
| resp.raise_for_status.return_value = None |
|
Hi @jacobokeeffe-ow -- thank you for this contribution, and especially for the exceptional root cause analysis in #669. Your detailed investigation of the While this PR was in review, we merged #701 which addresses the same bug via your suggested fix #2 ( I'm also closing #669 with credit to your root cause analysis, which was the foundation for the fix. Your contributions here have been genuinely valuable to the project. We'd love to keep collaborating with you on future issues -- feel free to pick up anything that catches your eye: https://github.com/microsoft/apm/issues Thanks again for your time and thoroughness! |
Description
Fix
apm marketplace addfailing to fetchmarketplace.jsonfrom private repositories, even when valid authentication tokens are available.Fixes #669
Type of change
Root cause
_do_fetch()inclient.pyreturnedNoneon HTTP 404 regardless of whether a token was present. Sincetry_with_fallback(unauth_first=True)only escalates to authenticated retry on exceptions (notNonereturns), the auth fallback never fired for private repos. GitHub returns 404 (not 403) for private repos to avoid leaking existence, making the unauthenticated 404 indistinguishable from "file not found."Fix
When
_do_fetch()receives a 404 without a token, it now raises viaresp.raise_for_status()so thattry_with_fallbackcatches the exception and retries with authentication. When a 404 is received with a token, it still returnsNone(the file genuinely doesn't exist).This preserves the rate-limit optimization for public repos (unauthenticated requests succeed on first try) while correctly handling private repos.
Testing
4 new tests in
TestPrivateRepoAuth:test_private_repo_unauthenticated_404_retries_with_token— core regression testtest_authenticated_404_returns_none— genuinely missing files still returnNonetest_public_repo_unauthenticated_success— no regression for public repostest_auto_detect_private_repo_finds_marketplace_json— end-to-end through_auto_detect_pathAlso verified against a real private marketplace repo — the fix correctly resolves
marketplace.jsonand returns both plugins.