From 5ad121cc331c795cbe78ee102c6a93423abe8e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Tue, 12 May 2026 21:01:28 +0800 Subject: [PATCH 1/3] fix: marketplace install auth host on *.ghe.com (closes #1285) Marketplaces registered on `*.ghe.com` (GHE Cloud) resolved plugin install auth against `github.com` instead of the registered enterprise host. The marketplace resolver emitted a canonical without the host prefix (`owner/repo/path`); `DependencyReference.parse` defaults missing hosts to `github.com`, mis-routing every downstream auth lookup -- and poisoning the resulting `apm.yml` entry with the wrong `git:` URL. `_marketplace_host_needs_explicit_git_path` conflated two orthogonal concerns. GitHub-family hosts (github.com + *.ghe.com) correctly skip the GitLab-style structured-ref path because they have no nested-group ambiguity -- but the same gate also dropped the host hint needed by `DependencyReference.parse` to recover the correct enterprise host. `github.com` survived because parse defaults to it; `*.ghe.com` broke. Backfill the host onto the canonical for in-marketplace sources only, scoped to GitHub-family enterprise hosts. Idempotent against dict sources that already carry a host-qualified `repo`. Cross-repo sources without explicit host qualification stay out of scope -- they are a separate manifest-author concern and inferring host there would change existing semantics. Round-trip stable through `DependencyReference.parse` / `to_canonical()`, so `apm.yml` lockfile entries now record the correct enterprise `git:` URL instead of the (silently wrong) `https://github.com/...` URL produced before the fix. --- src/apm_cli/marketplace/resolver.py | 41 ++++- .../marketplace/test_marketplace_resolver.py | 174 ++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index e5bd57b50..120b396d3 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,28 @@ 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. + """ + 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] + 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 +571,20 @@ 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}" + # ---- 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..bd79f5906 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -794,6 +794,180 @@ 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_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.""" From eebb0d278df73d8f8518c55d38992bc5ee9f8387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Tue, 12 May 2026 21:34:11 +0800 Subject: [PATCH 2/3] fix: skip URL/SSH form canonical in GHE host backfill (#1285) Manifests that put a full URL or SSH SCP shorthand in a dict source's `repo` field reach `_needs_canonical_host_prefix` via `_resolve_github_source` (which validates only that `/` is present) and `_is_in_marketplace_source` (whose normalizer accepts URL/SSH paths). Before this guard the prefix step produced malformed canonicals like `corp.ghe.com/https://corp.ghe.com/...` that `DependencyReference.parse` rejects with `ValueError` -- a regression versus the pre-fix tolerance where parse recovered host from the URL form natively. Detect URL/SSH form by `:` in the first slash-split segment of the canonical (both `https:` and `git@host:owner` carry a colon; bare owner names and bare host shorthand do not) and return False, leaving the canonical untouched. Downstream parse already handles both URL and SSH forms natively for routing, so no host backfill is needed. --- src/apm_cli/marketplace/resolver.py | 9 ++++ .../marketplace/test_marketplace_resolver.py | 51 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index 120b396d3..e083b58a9 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -197,11 +197,20 @@ def _needs_canonical_host_prefix(canonical: str, host: str) -> bool: 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. + + 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() diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index bd79f5906..bdf8640e2 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -948,6 +948,57 @@ def test_version_spec_override_preserves_host_prefix( 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): From 9596af76c8069ed1067d484430fc983d6d9ac21a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Wed, 13 May 2026 16:33:06 +0800 Subject: [PATCH 3/3] docs: changelog + debug trace + GHES docstring for #1285 follow-up Three non-behavioral additions in response to PR review feedback: - CHANGELOG `[Unreleased] Fixed` entry so enterprise teams who pinned workarounds for the silent github.com auth fallback can discover the fix on upgrade (they will not, otherwise -- this is the highest-value follow-up surfaced by the review panel). - `logger.debug` on the host backfill path. The catch-all `Resolved %s@%s -> %s` already shows the prefixed canonical at the end of resolution, but does not explain WHY the prefix is there. Distinguishing "source emitted host-qualified" from "resolver backfilled host" matters for operators tracing future #1285-class auth-routing issues with `--verbose`. - Docstring paragraph on `_needs_canonical_host_prefix` explaining why GHES (`GITHUB_HOST` self-managed) is not handled here -- GHES takes the structured `dep_ref` path upstream and never reaches this helper. Prevents a future developer from extending the helper's condition set in a way that conflicts with the upstream structured-ref path. Cross-repo silent mis-route warning and the integration-test regression trap suggested by the review panel are tracked as separate follow-up issues, intentionally not bundled here: the cross-repo warning needs install-time error-handler placement (a resolver-time always-on warning would false-positive on intentional github.com cross-host references), and the integration test needs fixture/marker infrastructure that inflates this PR's scope without changing the fix's correctness. --- CHANGELOG.md | 1 + src/apm_cli/marketplace/resolver.py | 13 +++++++++++++ 2 files changed, 14 insertions(+) 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 e083b58a9..f62dbe4fc 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -198,6 +198,13 @@ def _needs_canonical_host_prefix(canonical: str, host: str) -> bool: 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 @@ -593,6 +600,12 @@ def _emit_warning(msg: str) -> None: 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