fix: marketplace install auth host on *.ghe.com (closes #1285)#1292
Open
edenfunf wants to merge 2 commits into
Open
fix: marketplace install auth host on *.ghe.com (closes #1285)#1292edenfunf wants to merge 2 commits into
edenfunf wants to merge 2 commits into
Conversation
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes marketplace plugin installs for GHE Cloud (*.ghe.com) hosts by ensuring the resolved canonical reference carries the enterprise host prefix, so downstream DependencyReference.parse() (and auth routing) does not default to github.com (closes #1285).
Changes:
- Add
_needs_canonical_host_prefix()and a scoped host-prefix backfill inresolve_marketplace_plugin()for GitHub-family enterprise hosts. - Add a focused unit test suite covering GHE Cloud canonical host prefixing, idempotency, cross-repo non-prefixing, and version ref override behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/apm_cli/marketplace/resolver.py |
Adds helper + conditional canonical host-prefix backfill for *.ghe.com marketplace resolutions. |
tests/unit/marketplace/test_marketplace_resolver.py |
Adds unit tests validating correct canonical host behavior for GHE Cloud marketplaces and key regressions. |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
For marketplaces registered on
*.ghe.com(GHE Cloud) hosts, plugin install resolved auth againstgithub.cominstead of the registered enterprise host. This change backfills the marketplace host on the resolver's canonical string soDependencyReference.parserecovers the correct host downstream. Closes #1285.Problem
apm install <plugin>@<marketplace>against a marketplace registered with--host corp.ghe.comfailed with an authentication error. The verbose trace showed:DependencyReference.parsedefaults missing hosts togithub.com, so every*.ghe.commarketplace mis-routes auth. The same plugin installed via fully-qualified path worked correctly:i.e. the rest of the install pipeline already handles GHE Cloud correctly; only the marketplace → canonical step dropped the host.
A second, quieter symptom: the bare canonical also produced an
apm.ymlentry withgit: https://github.com/...even though the marketplace was oncorp.ghe.com, poisoning the lockfile with the wrong origin.Root cause
resolve_marketplace_pluginuses_marketplace_host_needs_explicit_git_path(source.host)to decide whether to build a structuredDependencyReference(explicitgit:URL +path:). That helper returnsFalsefor any GitHub-family host (github.com+*.ghe.com) because shorthandowner/repo[/path]is unambiguous on those hosts -- no GitLab-style nested-group ambiguity.The flaw: that single condition conflated two orthogonal concerns.
git:+path:to disambiguate the clone target? No for GitHub family. ✓DependencyReference.parseto recover the correct host? No for*.ghe.com; yes forgithub.combecause it is the documentedparsedefault.Both concerns were gated on the same return value, so
*.ghe.comcorrectly skipped the structured-ref path but incorrectly also dropped the host hint.Approach
Backfill the host onto the canonical post-hoc, scoped narrowly. The structured-ref path for GitLab-family hosts is left untouched -- this only fills the gap for the GitHub-family enterprise branch.
A new pure helper
_needs_canonical_host_prefix(canonical, host)answers a single question; the call site inresolve_marketplace_pluginadds five lines after the existing structured-ref block:Three guards justify the scope:
dep_ref is None-- the structured-ref path (GitLab family, self-managed FQDN) already carries host viato_canonical(). Don't double-handle._is_in_marketplace_source(plugin, source)-- only sources whose host is unambiguously the registered marketplace host get backfilled. Cross-repo dict sources without explicit host qualification are deliberately untouched; treating them as on-host would silently change routing for existing manifests._needs_canonical_host_prefix(canonical, host)-- narrows to GitHub-family enterprise hosts (is_github_hostname(host) and host != "github.com") and is idempotent (case-insensitive) against dict sources that already carry a host-qualifiedrepo.dependency_referenceremainsNonefor GitHub-family hosts -- the clone-path shorthand semantics are preserved, only the auth-routing string is corrected.Tests
New class
TestResolveMarketplacePluginGHECloudintests/unit/marketplace/test_marketplace_resolver.py(7 cases). Each test locks one branch of the helper or one invariant the fix must preserve:test_relative_source_carries_host_in_canonicalcorp.ghe.comtest_dict_github_source_bare_repo_carries_hosttest_dict_github_source_host_qualified_repo_not_double_prefixedtest_dict_github_source_mixed_case_host_qualified_not_double_prefixedtest_cross_repo_source_not_prefixedtest_version_spec_override_preserves_host_prefixtest_github_com_canonical_remains_bareValidation
End-to-end equivalence check against real
AuthResolver(mock marketplace fetch only):myorg/my-marketplace/plugins/my-plugincorp.ghe.com/myorg/my-marketplace/plugins/my-plugincorp.ghe.com/myorg/my-marketplace/plugins/my-plugingithub.comcorp.ghe.comcorp.ghe.comgithubghe_cloudghe_cloudhttps://github.com/myorg/my-marketplacehttps://corp.ghe.com/myorg/my-marketplacehttps://corp.ghe.com/myorg/my-marketplacehttps://github.com/...https://corp.ghe.com/...https://corp.ghe.com/...After this change the marketplace path produces byte-identical resolver / auth / lockfile output to the documented workaround (
apm install corp.ghe.com/owner/repo/path).How to test
For a manual reproduction:
*.ghe.comhost:apm marketplace add --host corp.ghe.com myorg/my-marketplaceapm install my-plugin@my-marketplace --verboseResolved to: myorg/my-marketplace/plugins/my-pluginandAuth resolved: host=github.com(auth fails).Resolved to: corp.ghe.com/myorg/my-marketplace/plugins/my-pluginandAuth resolved: host=corp.ghe.com(auth succeeds).