diff --git a/CHANGELOG.md b/CHANGELOG.md index f16280684..a6a04aa6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Override `Path.home()` itself in the root test conftest so the 46 remaining Windows `RuntimeError: Could not determine home directory` failures on xdist worker `gw2` cannot recur regardless of which conftest the worker imports first; per-test `monkeypatch.setenv("HOME", ...)` continues to work because the override consults env vars before falling back to the hermetic tmp dir. (#1272) - Retry the `apm mcp search` and `apm mcp show` integration tests on the documented "Could not reach MCP registry" transient (with backoff and a final skip) so a brief `api.mcp.github.com` outage no longer red-marks the Windows integration job. (#1274) - Also wrap `Path.expanduser()` in the root test conftest so the `windows-2025-vs2026` runner cannot raise `RuntimeError("Could not determine home directory.")` from `ntpath.expanduser` when production code (e.g. `install.package_resolution.user_scope_rejection_reason`) calls `Path("~/pkg").expanduser()`. Falls back to the hermetic tmp dir; assertions about `~/pkg` being absolute still hold. (#1276) +- `apm install` from marketplaces registered on `*.ghe.com` (GHE Cloud) hosts now routes auth at the registered enterprise host instead of silently defaulting to `github.com` and failing with 401; the marketplace resolver backfills the enterprise host onto the canonical so downstream `DependencyReference.parse` recovers it, and the resulting `apm.yml` entry records the correct enterprise `git:` URL instead of `https://github.com/...`. (#1292) ## [0.13.0] - 2026-05-11 diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index e5bd57b50..f62dbe4fc 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -14,7 +14,10 @@ :class:`~apm_cli.models.dependency.reference.DependencyReference` built like explicit ``git:`` + ``path:``; clone target is only the registered marketplace project; the plugin directory is ``virtual_path``. -``github.com`` / ``*.ghe.com`` keep shorthand (no structured ref). :func:`resolve_marketplace_plugin` returns +``github.com`` and ``*.ghe.com`` keep shorthand (no structured ref); ``*.ghe.com`` +canonicals additionally carry a host prefix so downstream auth resolves at the +enterprise host instead of falling back to ``github.com`` (#1285). +:func:`resolve_marketplace_plugin` returns :class:`MarketplacePluginResolution`, which iterates as ``(canonical, plugin)`` so existing ``canonical, plugin = resolve_marketplace_plugin(...)`` call sites keep working; consumers that need the structured ref use ``result.dependency_reference``. @@ -180,6 +183,44 @@ def _marketplace_host_needs_explicit_git_path(host: str) -> bool: return not is_github_hostname(h) +def _needs_canonical_host_prefix(canonical: str, host: str) -> bool: + """True when a GitHub-family enterprise host must be prefixed to ``canonical``. + + GitHub-family hosts (``github.com`` + ``*.ghe.com``) keep virtual shorthand -- + ``resolve_plugin_source`` emits a bare ``owner/repo[/path]`` canonical because + there is no nested-group ambiguity to disambiguate. ``DependencyReference.parse`` + defaults missing hosts to ``github.com``, which is correct for ``github.com`` but + silently mis-routes auth for every ``*.ghe.com`` marketplace. + + Returns True only for enterprise GitHub hosts (``*.ghe.com``) so the caller can + backfill the host while preserving shorthand semantics. Idempotent: when the + canonical already starts with ``host`` (case-insensitive) -- as happens when the + manifest's dict source carries a host-qualified ``repo`` -- this returns False + so the prefix is not duplicated. + + GHES (GitHub Enterprise Server, configured via ``GITHUB_HOST``) is not handled + here. Those hosts return True from ``_marketplace_host_needs_explicit_git_path`` + (neither GitHub-family nor ADO) so ``resolve_marketplace_plugin`` builds a + structured ``dep_ref`` upstream and this helper is never reached. The + ``is_github_hostname`` check below is defense-in-depth that would also reject + them if a future change ever bypassed the upstream guard. + + Also returns False when ``canonical`` is in URL form (``https://...``) or SSH + SCP shorthand (``git@host:owner/repo``). Manifests that put a full URL in the + ``repo`` field reach this point via ``_resolve_github_source`` (which only + requires a ``/``); detecting those by ``":"`` in the first slash-split segment + avoids producing malformed ``host/https://...`` canonicals. Those forms already + carry a host and ``DependencyReference.parse`` resolves them natively. + """ + h = (host or "").strip() + if not h or not is_github_hostname(h) or h.lower() == "github.com": + return False + first_segment = canonical.split("/", 1)[0] + if ":" in first_segment: + return False + return first_segment.lower() != h.lower() + + def _marketplace_https_git_url(source: MarketplaceSource) -> str: """HTTPS clone URL for the registered marketplace project (same project as ``marketplace.json``).""" segments = [p for p in f"{source.owner}/{source.repo}".split("/") if p] @@ -546,6 +587,26 @@ def _emit_warning(msg: str) -> None: ) canonical = dep_ref.to_canonical() + # ---- Backfill host on canonical for GitHub-family enterprise hosts ---- + # ``*.ghe.com`` marketplaces keep virtual shorthand (no structured ``dep_ref``) + # because there is no nested-group ambiguity to disambiguate, but the bare + # canonical drops the host that ``DependencyReference.parse`` needs to route auth + # at the enterprise host instead of falling back to ``github.com``. Backfill the + # host so the canonical self-routes, scoped to in-marketplace sources where the + # host is unambiguously the registered marketplace host (#1285). + if ( + dep_ref is None + and _is_in_marketplace_source(plugin, source) + and _needs_canonical_host_prefix(canonical, source.host) + ): + canonical = f"{source.host}/{canonical}" + logger.debug( + "Backfilled marketplace host '%s' onto canonical for %s@%s (auth routing #1285)", + source.host, + plugin_name, + marketplace_name, + ) + # ---- Raw ref override ---- # When version_spec is provided it is treated as a raw git ref that # overrides whatever ref came from the marketplace source field. diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index fd5c33e48..bdf8640e2 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -794,6 +794,231 @@ def test_kind_key_github_dict_in_marketplace_gets_structured_ref( assert dep.repo_url == "epm-ease/ai-apm-registry" +class TestResolveMarketplacePluginGHECloud: + """GHE Cloud (``*.ghe.com``) marketplaces must carry host in canonical (issue #1285). + + GitHub-family hosts keep virtual shorthand (no ``dependency_reference``) because + they have no GitLab-style nested-group ambiguity. ``github.com`` is the + ``DependencyReference.parse`` default so a bare ``owner/repo`` canonical + self-routes; ``*.ghe.com`` is not, so canonical must carry the host forward or + downstream auth lands on ``github.com`` with the wrong credentials. + """ + + @pytest.fixture + def ghe_marketplace_source(self) -> MarketplaceSource: + return MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="corp.ghe.com", + branch="main", + ) + + @staticmethod + def _manifest_with_plugin(plugin: MarketplacePlugin) -> MarketplaceManifest: + return MarketplaceManifest( + name="my-marketplace", + plugins=(plugin,), + plugin_root="", + ) + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_relative_source_carries_host_in_canonical( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") + + assert result.canonical == "corp.ghe.com/myorg/my-marketplace/plugins/my-plugin" + # GHE keeps shorthand semantics -- no structured dep_ref, only canonical + assert result.dependency_reference is None + # The whole point: parse must recover the GHE host instead of defaulting to github.com + dep = DependencyReference.parse(result.canonical) + assert dep.host == "corp.ghe.com" + assert dep.repo_url == "myorg/my-marketplace" + assert dep.virtual_path == "plugins/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_dict_github_source_bare_repo_carries_host( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """Dict ``github`` source whose bare ``repo`` matches the marketplace project.""" + plugin = MarketplacePlugin( + name="dict-bare", + source={ + "type": "github", + "repo": "myorg/my-marketplace", + "path": "plugins/my-plugin", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("dict-bare", "my-marketplace") + assert result.canonical == "corp.ghe.com/myorg/my-marketplace/plugins/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_dict_github_source_host_qualified_repo_not_double_prefixed( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """Idempotent: dict source already carrying the host in ``repo`` keeps a single prefix.""" + plugin = MarketplacePlugin( + name="dict-qualified", + source={ + "type": "github", + "repo": "corp.ghe.com/myorg/my-marketplace", + "path": "plugins/my-plugin", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("dict-qualified", "my-marketplace") + assert result.canonical == "corp.ghe.com/myorg/my-marketplace/plugins/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_dict_github_source_mixed_case_host_qualified_not_double_prefixed( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """Case-insensitive idempotent check: manifests may write host in any case.""" + plugin = MarketplacePlugin( + name="dict-mixed", + source={ + "type": "github", + "repo": "Corp.GHE.com/myorg/my-marketplace", + "path": "plugins/my-plugin", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("dict-mixed", "my-marketplace") + # Manifest-supplied case is preserved; no second prefix is added + assert result.canonical == "Corp.GHE.com/myorg/my-marketplace/plugins/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_source_not_prefixed(self, mock_get, mock_fetch, ghe_marketplace_source): + """Cross-repo dict source is out of scope; canonical is left to the existing parse default. + + Bare unqualified ``repo`` pointing to a different project carries no signal that + it lives on the marketplace host; treating it as such would silently change the + host routing of every cross-repo plugin entry. Manifest authors must + host-qualify cross-host references explicitly. + """ + plugin = MarketplacePlugin( + name="cross-repo", + source={ + "type": "github", + "repo": "anotherorg/anothertool", + "path": "plugins/my-plugin", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("cross-repo", "my-marketplace") + assert result.canonical == "anotherorg/anothertool/plugins/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_version_spec_override_preserves_host_prefix( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """``version_spec`` raw-ref override stacks correctly on a host-prefixed canonical.""" + plugin = MarketplacePlugin(name="ref-overridden", source="./plugins/ref-overridden") + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin( + "ref-overridden", "my-marketplace", version_spec="release-2.0" + ) + assert ( + result.canonical + == "corp.ghe.com/myorg/my-marketplace/plugins/ref-overridden#release-2.0" + ) + dep = DependencyReference.parse(result.canonical) + assert dep.host == "corp.ghe.com" + assert dep.reference == "release-2.0" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_url_form_repo_not_prefixed(self, mock_get, mock_fetch, ghe_marketplace_source): + """Dict source whose ``repo`` is a full ``https://`` URL must not be host-prefixed. + + ``_resolve_github_source`` only validates ``/`` in ``repo`` so a full URL slips + through and produces a canonical that already carries a scheme + host. Prefixing + again would yield a malformed ``corp.ghe.com/https://...`` string that + ``DependencyReference.parse`` rejects with ValueError. + """ + plugin = MarketplacePlugin( + name="url-form", + source={ + "type": "github", + "repo": "https://corp.ghe.com/myorg/my-marketplace", + "path": "plugins/url-form", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("url-form", "my-marketplace") + assert result.canonical == "https://corp.ghe.com/myorg/my-marketplace/plugins/url-form" + # Downstream parse still recovers the GHE host from the URL form natively. + dep = DependencyReference.parse(result.canonical) + assert dep.host == "corp.ghe.com" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_ssh_form_repo_not_prefixed(self, mock_get, mock_fetch, ghe_marketplace_source): + """Dict source whose ``repo`` is an SSH SCP shorthand must not be host-prefixed. + + Same class as the URL-form regression: ``git@host:owner/repo`` carries its own + host and an SCP-style ``:`` path separator that breaks a naive ``/`` split. + """ + plugin = MarketplacePlugin( + name="ssh-form", + source={ + "type": "github", + "repo": "git@corp.ghe.com:myorg/my-marketplace", + "path": "plugins/ssh-form", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("ssh-form", "my-marketplace") + assert result.canonical == "git@corp.ghe.com:myorg/my-marketplace/plugins/ssh-form" + dep = DependencyReference.parse(result.canonical) + assert dep.host == "corp.ghe.com" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_github_com_canonical_remains_bare(self, mock_get, mock_fetch): + """Regression: github.com marketplace canonical stays bare (parse default applies).""" + gh_source = MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="github.com", + branch="main", + ) + plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") + mock_get.return_value = gh_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") + assert result.canonical == "myorg/my-marketplace/plugins/my-plugin" + assert result.dependency_reference is None + + class TestGitLabShorthandParseVsStructuredRef: """``DependencyReference.parse`` on a long FQDN does not split monorepo paths on GitLab hosts."""