Skip to content

fix: marketplace install auth host on *.ghe.com (closes #1285)#1292

Merged
sergio-sisternes-epam merged 3 commits into
microsoft:mainfrom
edenfunf:fix/marketplace-ghe-host-prefix-1285
May 13, 2026
Merged

fix: marketplace install auth host on *.ghe.com (closes #1285)#1292
sergio-sisternes-epam merged 3 commits into
microsoft:mainfrom
edenfunf:fix/marketplace-ghe-host-prefix-1285

Conversation

@edenfunf
Copy link
Copy Markdown
Contributor

TL;DR

For marketplaces registered on *.ghe.com (GHE Cloud) hosts, plugin install resolved auth against github.com instead of the registered enterprise host. This change backfills the marketplace host on the resolver's canonical string so DependencyReference.parse recovers the correct host downstream. Closes #1285.

Problem

apm install <plugin>@<marketplace> against a marketplace registered with --host corp.ghe.com failed with an authentication error. The verbose trace showed:

Resolving my-plugin@my-marketplace via marketplace...
Resolved to: myorg/my-marketplace/plugins/my-plugin        <-- missing corp.ghe.com/ prefix
Auth resolved: host=github.com, org=myorg, ...             <-- wrong host
[i] plugins/my-plugin/apm.yml@main (Authentication failed for myorg/my-marketplace ...)

DependencyReference.parse defaults missing hosts to github.com, so every *.ghe.com marketplace mis-routes auth. The same plugin installed via fully-qualified path worked correctly:

apm install corp.ghe.com/myorg/my-marketplace/plugins/my-plugin

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.yml entry with git: https://github.com/... even though the marketplace was on corp.ghe.com, poisoning the lockfile with the wrong origin.

Root cause

resolve_marketplace_plugin uses _marketplace_host_needs_explicit_git_path(source.host) to decide whether to build a structured DependencyReference (explicit git: URL + path:). That helper returns False for any GitHub-family host (github.com + *.ghe.com) because shorthand owner/repo[/path] is unambiguous on those hosts -- no GitLab-style nested-group ambiguity.

The flaw: that single condition conflated two orthogonal concerns.

  • Clone-path semantics: do we need explicit git: + path: to disambiguate the clone target? No for GitHub family. ✓
  • Auth-host semantics: does the canonical carry enough info for DependencyReference.parse to recover the correct host? No for *.ghe.com; yes for github.com because it is the documented parse default.

Both concerns were gated on the same return value, so *.ghe.com correctly 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 in resolve_marketplace_plugin adds five lines after the existing structured-ref block:

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}"

Three guards justify the scope:

  1. dep_ref is None -- the structured-ref path (GitLab family, self-managed FQDN) already carries host via to_canonical(). Don't double-handle.
  2. _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.
  3. _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-qualified repo.

dependency_reference remains None for GitHub-family hosts -- the clone-path shorthand semantics are preserved, only the auth-routing string is corrected.

Tests

New class TestResolveMarketplacePluginGHECloud in tests/unit/marketplace/test_marketplace_resolver.py (7 cases). Each test locks one branch of the helper or one invariant the fix must preserve:

Test Locks
test_relative_source_carries_host_in_canonical Primary bug case; parse round-trip recovers corp.ghe.com
test_dict_github_source_bare_repo_carries_host Dict-source variant of the same in-marketplace fix
test_dict_github_source_host_qualified_repo_not_double_prefixed Idempotent guard against double-prefix
test_dict_github_source_mixed_case_host_qualified_not_double_prefixed Idempotent guard is case-insensitive
test_cross_repo_source_not_prefixed Cross-repo references explicitly out of scope
test_version_spec_override_preserves_host_prefix Ref override stacks on top of host-prefixed canonical
test_github_com_canonical_remains_bare Regression: github.com path unchanged

Validation

End-to-end equivalence check against real AuthResolver (mock marketplace fetch only):

Pre-fix bare canonical This PR Workaround FQDN install
canonical myorg/my-marketplace/plugins/my-plugin corp.ghe.com/myorg/my-marketplace/plugins/my-plugin corp.ghe.com/myorg/my-marketplace/plugins/my-plugin
AuthResolver target host github.com corp.ghe.com corp.ghe.com
HostInfo.kind github ghe_cloud ghe_cloud
clone URL https://github.com/myorg/my-marketplace https://corp.ghe.com/myorg/my-marketplace https://corp.ghe.com/myorg/my-marketplace
apm.yml entry git https://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

uv run pytest tests/unit/marketplace/test_marketplace_resolver.py -x

For a manual reproduction:

  1. Register a marketplace on a *.ghe.com host: apm marketplace add --host corp.ghe.com myorg/my-marketplace
  2. Run apm install my-plugin@my-marketplace --verbose
  3. Before this change: Resolved to: myorg/my-marketplace/plugins/my-plugin and Auth resolved: host=github.com (auth fails).
  4. After this change: Resolved to: corp.ghe.com/myorg/my-marketplace/plugins/my-plugin and Auth resolved: host=corp.ghe.com (auth succeeds).

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.
Copilot AI review requested due to automatic review settings May 12, 2026 13:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in resolve_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.

Comment thread src/apm_cli/marketplace/resolver.py
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.
@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label May 13, 2026
@github-actions
Copy link
Copy Markdown

APM Review Panel: ship_with_followups

PR #1292 fixes a silent enterprise auth mis-route that blocked *.ghe.com marketplace installs -- a targeted, trust-positive fix with clean security posture and strong unit coverage.

cc @edenfunf -- a fresh advisory pass is ready for your review.

This PR closes a real, user-reported enterprise regression (#1285): marketplace plugins sourced from *.ghe.com hosts were having their canonical silently left bare, causing DependencyReference.parse to default to github.com credentials and fail auth against the enterprise host. Auth-expert and supply-chain-security both confirm the fix is auth-correct and that the host injected into canonical is sourced from the trusted MarketplaceSource registry config -- never from manifest content. The triple gate (dep_ref is None + _is_in_marketplace_source + is_github_hostname excluding github.com) eliminates token cross-contamination risk. Nine unit tests including real DependencyReference.parse boundary assertions give HIGH confidence on the happy path.

Panel signals converge tightly on two follow-up clusters: (1) CHANGELOG + doc surface -- oss-growth-hacker and doc-writer both flag the missing [Unreleased] Fixed entry independently; this is the single highest-value non-code item since enterprise teams who pinned workarounds for #1285 will never discover the fix without it. (2) Cross-repo GHE plugin silent mis-route -- python-architect, devx-ux, and supply-chain-security all independently identify that cross-repo plugins on a *.ghe.com marketplace still emit a bare canonical and silently fall back to github.com auth. All three agree this is pre-existing behaviour, out of scope for this PR, but the absence of any user-facing warning makes it an invisible failure mode that will generate repeat support tickets. A single logger.warning (or structured install-time check) would close the UX gap without changing behaviour. The test-coverage-expert finding of a missing integration-with-fixtures regression trap is evidence-backed (outcome: missing, principles: secure-by-default + governed-by-policy) and ranks above opinion-only recommended findings; this is the right follow-up issue to open, not a blocker for a fix that already has strong unit coverage.

The auth-expert docstring gap (why GHES hosts are intentionally excluded from _needs_canonical_host_prefix) is a low-cost clarification that will prevent future developers from re-introducing the same class of bug. The cli-logging-expert finding of a missing logger.debug on the backfill path directly serves the --verbose diagnostic surface that operators will use when debugging future #1285-class issues.

Aligned with: Secure by Default -- fix ensures enterprise credentials are selected by default for *.ghe.com marketplace sources without any user config change. Portable by Manifest -- transparent to manifest authors, no apm.yml or plugin manifest changes required. Governed by Policy -- auth routing follows the AuthResolver credential hierarchy scoped correctly to the enterprise host; the missing integration-with-fixtures test means this policy contract is not yet machine-verified end-to-end.

Growth signal. This is a quiet enterprise unlock. GHE admins who hit #1285 searched release notes and found nothing -- a CHANGELOG entry with the *.ghe.com host pattern is the single highest-ROI item in this PR's follow-up list. Consider a dedicated "Enterprise" subsection under [Unreleased] as GHE fixes accumulate; it signals intentional enterprise investment to evaluators scanning the changelog before adoption decisions.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 1 Minimal, well-scoped fix; guard logic is correct; one unaddressed cross-repo GHE edge case worth noting; test suite is thorough.
CLI Logging Expert 0 1 1 Backfill path is silent while every comparable resolver branch emits logger.debug; add one debug line so --verbose traces auth routing decisions for *.ghe.com.
DevX UX Expert 0 2 1 Happy-path GHE fix is zero-config and transparent; cross-repo GHE plugins still silently mis-route auth with no actionable error surface.
Supply Chain Security Expert 0 1 1 Fix correctly routes enterprise host auth; host sourced from trusted MarketplaceSource, not manifest content; one residual auth-routing gap for cross-repo plugins.
OSS Growth Hacker 0 1 2 GHE marketplace auth fix is a high-value enterprise unlock -- but missing CHANGELOG entry means enterprise teams won't discover it on upgrade.
Auth Expert 0 1 1 Fix is auth-correct: *.ghe.com canonical host backfill routes enterprise token; GHES/cross-host guards are sound; no token cross-contamination risk.
Doc Writer 0 2 1 CHANGELOG missing #1285 entry; consumer auth doc has no mention of *.ghe.com marketplace auth fallback bug; module docstring is accurate.
Test Coverage Expert 0 1 0 9 unit tests cover helper + backfill thoroughly; integration-tier regression trap for GHE auth routing is missing (floor: integration-with-fixtures for auth resolution).

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [OSS Growth Hacker + Doc Writer] Add CHANGELOG [Unreleased] Fixed entry for [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285 -- Convergent signal from two personas. Enterprise teams who pinned workarounds for [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285 will never discover this fix without a changelog entry. Suggested copy: "apm marketplace install on *.ghe.com hosts now uses enterprise credentials instead of silently falling back to github.com auth (closes [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285)". Zero-cost, high-enterprise-trust ROI.
  2. [Test Coverage Expert] Add integration-with-fixtures regression trap for GHE marketplace auth routing -- Evidence-backed missing test (outcome: missing) on a secure-by-default + governed-by-policy surface. The full flow apm install -> resolve_marketplace_plugin -> canonical -> DependencyReference.parse -> auth credential lookup is not exercised by any test in tests/integration/. Suggested file: tests/integration/marketplace/test_ghe_marketplace_install_e2e.py.
  3. [Python Architect + DevX UX Expert + Supply Chain Security Expert] Emit logger.warning for cross-repo GHE plugins that will silently mis-route auth -- Three independent panelists flag this pre-existing gap. When _is_in_marketplace_source returns False for a *.ghe.com marketplace and the canonical is bare owner/repo, the downstream 401 is invisible to the user. A single warning with a host-qualify hint converts a silent failure into an actionable error and prevents repeat [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285-class support tickets.
  4. [CLI Logging Expert] Add logger.debug on the host-prefix backfill path -- The backfill block is the exact scenario operators will trace with --verbose when debugging *.ghe.com auth failures. Every comparable resolver branch emits a debug line. Suggested: logger.debug("Backfilled enterprise host '%s' onto canonical for %s@%s (auth routing #1285)", source.host, plugin_name, marketplace_name).
  5. [Auth Expert] Add docstring note explaining why GHES (GITHUB_HOST) hosts are intentionally excluded from _needs_canonical_host_prefix -- GHES hosts correctly take the dep_ref path, but a developer reading the function will not know this without an explicit note. Prevents future developers from re-introducing the same class of auth-routing bug.

Architecture

classDiagram
    direction LR
    class resolve_marketplace_plugin {
        <<EntryPoint>>
        +plugin_name str
        +marketplace_name str
        +version_spec str
        +auth_resolver AuthResolver
        +returns MarketplacePluginResolution
    }
    class MarketplacePluginResolution {
        <<ValueObject>>
        +canonical str
        +plugin MarketplacePlugin
        +dependency_reference DependencyReference
    }
    class MarketplaceSource {
        <<ValueObject>>
        +host str
        +owner str
        +repo str
        +branch str
    }
    class MarketplacePlugin {
        <<ValueObject>>
        +name str
        +source str
        +version str
    }
    class DependencyReference {
        <<ValueObject>>
        +host str
        +repo_url str
        +virtual_path str
        +reference str
        +parse(canonical) DependencyReference
        +to_canonical() str
    }
    class _needs_canonical_host_prefix {
        <<Pure>>
        +canonical str
        +host str
        +returns bool
    }
    class _is_in_marketplace_source {
        <<Pure>>
        +plugin MarketplacePlugin
        +source MarketplaceSource
        +returns bool
    }
    class is_github_hostname {
        <<Pure>>
        +hostname str
        +returns bool
    }
    resolve_marketplace_plugin *-- MarketplacePluginResolution : produces
    resolve_marketplace_plugin ..> MarketplaceSource : reads
    resolve_marketplace_plugin ..> MarketplacePlugin : reads
    resolve_marketplace_plugin ..> _is_in_marketplace_source : calls
    resolve_marketplace_plugin ..> _needs_canonical_host_prefix : calls
    resolve_marketplace_plugin ..> DependencyReference : delegates parse
    _needs_canonical_host_prefix ..> is_github_hostname : delegates
    MarketplacePluginResolution o-- DependencyReference : optional
    class _needs_canonical_host_prefix:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A(["apm install plugin@marketplace"]) --> B["resolve_marketplace_plugin()"]
    B --> C["fetch_or_cache() -- fetch marketplace manifest"]
    C --> D["resolve_plugin_source() -- build bare canonical: owner/repo/path"]
    D --> E{"_marketplace_host_needs_explicit_git_path(source.host)?"}
    E -->|"Yes - GitLab/generic"| F["dep_ref set via _gitlab_in_marketplace_dependency_reference()"]
    E -->|"No - github.com or *.ghe.com"| G["dep_ref = None, canonical stays bare"]
    F --> H{"dep_ref is None?"}
    G --> H
    H -->|"No - GitLab path"| I["skip host backfill"]
    H -->|"Yes"| J{"_is_in_marketplace_source() AND _needs_canonical_host_prefix()?"}
    J -->|"False: github.com, cross-repo, or already host-prefixed"| K["canonical unchanged"]
    J -->|"True: *.ghe.com in-marketplace -- FIX #1285"| L["canonical = source.host/canonical"]
    I --> M{"version_spec override?"}
    K --> M
    L --> M
    M -->|"Yes"| N["canonical = base#version_spec"]
    M -->|"No"| O["MarketplacePluginResolution(canonical, plugin, dep_ref)"]
    N --> O
    O --> P["DependencyReference.parse(canonical) -- routes auth to host in canonical"]
    P --> Q(["Auth at correct GHE host"])
Loading

Recommendation

Ship this PR. The fix is auth-correct, security-positive, and transparent to users -- auth-expert and supply-chain-security both confirm no token cross-contamination risk and a correctly scoped trust boundary. Unit test quality is HIGH with real DependencyReference.parse boundary assertions. The five follow-ups are all non-blocking improvements: the CHANGELOG entry is the highest-urgency item (open a follow-up commit or PR in the same release window); the integration-with-fixtures test should be tracked as a follow-up issue with the secure-by-default label; the cross-repo warning, debug log, and docstring clarification can land in the same minor release. None of these gaps were introduced by this PR, and none represent regressions from the pre-PR state.


Full per-persona findings

Python Architect

  • [recommended] Cross-repo GHE plugins silently misroute auth and the limitation is only documented in a test comment at src/apm_cli/marketplace/resolver.py
    When a *.ghe.com marketplace lists a cross-repo plugin, _is_in_marketplace_source returns False and _needs_canonical_host_prefix is never reached. The bare canonical propagates and DependencyReference.parse defaults to github.com, causing the same auth-routing failure this PR fixes for in-marketplace sources. The test test_cross_repo_source_not_prefixed documents this as intentional but the limitation is invisible to users.
    Suggested: Add a logger.debug or logger.warning in resolve_marketplace_plugin when source.host is a *.ghe.com host and a cross-repo dict source is detected without a host-qualified repo field.

  • [nit] Guard order in _needs_canonical_host_prefix could surface the idempotency check before the colon check at src/apm_cli/marketplace/resolver.py
    The first_segment.lower() != h.lower() idempotency check is the most semantically important guard; currently it is last, after the : check. Reordering is a no-op at runtime but reads top-to-bottom more clearly.

CLI Logging Expert

  • [recommended] Missing logger.debug for host-prefix backfill path at src/apm_cli/marketplace/resolver.py
    The immediately adjacent raw-ref override block emits logger.debug with plugin/marketplace/ref context. The backfill block is the exact scenario operators will need to trace when debugging *.ghe.com auth failures -- yet it is completely silent.
    Suggested: After canonical = f"{source.host}/{canonical}" add: logger.debug("Backfilled enterprise host '%s' onto canonical for %s@%s (auth routing #1285)", source.host, plugin_name, marketplace_name)

  • [nit] warning_handler is not threaded to _needs_canonical_host_prefix callers -- purely cosmetic, no action required.

DevX UX Expert

  • [recommended] Cross-repo GHE plugin: silent auth mis-route with no recovery hint at src/apm_cli/marketplace/resolver.py
    When a GHE marketplace user lists a cross-repo plugin without host qualification, DependencyReference.parse defaults to github.com, auth fails, and the user receives no signal. A one-line warning when _is_in_marketplace_source returns False but source.host is a *.ghe.com host would surface a concrete next action.
    Suggested: Text: "Plugin X source Y is outside the marketplace repository; if it lives on {host}, qualify the repo field as {host}/{repo} for correct authentication."

  • [recommended] No validation or error annotation when canonical is still auth-unresolvable at install time at src/apm_cli/marketplace/resolver.py
    If for any reason the backfill is skipped, the downstream failure is a generic clone/auth error. A structured install-time check comparing dep.host vs source.host would give users one copy-pasteable correction instead of a raw git auth failure.

  • [nit] _needs_canonical_host_prefix docstring is longer than the function body -- accurate but slow to scan; consider a one-line summary + inline comments at key guards.

Supply Chain Security Expert

  • [recommended] Cross-repo plugins on *.ghe.com marketplaces still mis-route auth at src/apm_cli/marketplace/resolver.py
    A cross-repo plugin hosted on the same *.ghe.com instance will still emit a bare canonical, so DependencyReference.parse defaults to github.com credentials. Pre-existing behaviour, not introduced by this PR.

  • [nit] is_github_hostname note about "any FQDN" could mislead future readers -- a one-liner distinguishing *.ghe.com (GHE Cloud) from GHES (arbitrary FQDN) would close the reading gap at src/apm_cli/utils/github_host.py.

OSS Growth Hacker

Auth Expert

  • [recommended] Docstring should clarify why GHES (GITHUB_HOST) hosts are intentionally excluded from _needs_canonical_host_prefix at src/apm_cli/marketplace/resolver.py
    GHES hosts return False from is_github_hostname, which is correct because they already take the dep_ref path. A one-sentence note closes the reading gap and prevents future developers from treating this as a missing case.

  • [nit] String-source shortcut in _is_in_marketplace_source bypasses host validation -- isinstance(s, str) -> return True unconditionally, so a mis-authored manifest entry could receive an unexpected enterprise host prefix. Pre-existing, outside this PR's scope.

Doc Writer

  • [recommended] CHANGELOG [Unreleased] missing entry for [BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285 at CHANGELOG.md
    The [Unreleased] Fixed section has entries but none for this user-visible bug fix.

  • [recommended] consumer/authentication.md has no mention of *.ghe.com marketplace-specific auth routing at docs/src/content/docs/consumer/authentication.md
    The doc covers *.ghe.com auth for direct installs but not marketplace-sourced installs. A user who hits this bug will consult this page and remain confused.

  • [nit] Module docstring: issue reference (#1285) in RST docstring is non-standard and won't render as a hyperlink -- consider dropping or using a .. versionchanged:: directive.

Test Coverage Expert

  • [recommended] No integration-with-fixtures regression trap for GHE marketplace auth routing at tests/integration/marketplace/test_ghe_marketplace_install_e2e.py
    The bug fixed here is user-reported ([BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host #1285). The full flow apm install -> resolve_marketplace_plugin -> canonical -> DependencyReference.parse -> auth credential lookup is not exercised by any test in tests/integration/. Grepped for resolve_marketplace_plugin, MarketplaceSource.*ghe, ghe.*install -- zero hits. The 9 unit tests are HIGH quality (real DependencyReference.parse boundary assertions) but do not certify the end-to-end auth routing promise.
    Proof (missing at integration-with-fixtures): tests/integration/marketplace/test_ghe_marketplace_install_e2e.py -- proves: apm install with a *.ghe.com marketplace source resolves credentials at the enterprise host, not github.com [secure-by-default, governed-by-policy]

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1292 · ● 2.9M ·

…low-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 microsoft#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.
@edenfunf edenfunf force-pushed the fix/marketplace-ghe-host-prefix-1285 branch from f3bee6d to 9596af7 Compare May 13, 2026 08:47
@sergio-sisternes-epam sergio-sisternes-epam added this pull request to the merge queue May 13, 2026
Merged via the queue into microsoft:main with commit 09f5854 May 13, 2026
9 checks passed
abhinavgautam01 pushed a commit to abhinavgautam01/apm that referenced this pull request May 14, 2026
…ting (closes microsoft#1304) (microsoft#1312)

* test(integration): regression trap for *.ghe.com marketplace auth routing (closes microsoft#1304)

Adds tests/integration/test_ghe_marketplace_install_e2e.py exercising
the full install pipeline from resolve_marketplace_plugin through
DependencyReference.parse to AuthResolver.resolve_for_dep. The unit
tests in tests/unit/marketplace/ cover the resolver layer directly
but stop at the canonical string; the review panel for PR microsoft#1292
(closes microsoft#1285) flagged the absence of integration-tier coverage on
this auth-routing contract as a secure-by-default + governed-by-policy
test floor.

Five test cases:

- Three parametrized cases (relative source, dict github bare repo,
  dict github host-qualified repo) assert the full chain lands on the
  enterprise host: canonical carries the prefix, parse recovers it,
  AuthContext.host_info.host == "corp.ghe.com" with kind "ghe_cloud".
- A github.com marketplace regression case locks the pre-existing
  default-host behaviour so the fix did not silently change it.
- A regression trap for the cross-repo silent mis-route tracked in
  microsoft#1305: asserts the current (buggy) behaviour with a docstring
  pointing at the issue so the future fix has an explicit before/after
  diff to assert against.

Stubs at two seams only:

- get_marketplace_by_name / fetch_or_cache: skip marketplace registry
  + manifest network I/O. MarketplaceSource is trust-boundary config,
  not manifest content (the auth-expert confirmed this distinction
  during PR microsoft#1292 review).
- AuthResolver._resolve_token: skip env/gh-cli/credential-helper I/O
  so the test is deterministic and runs without tokens. host_info on
  the returned AuthContext is still real (built by classify_host) --
  that is the routing contract under test.

* test(integration): split GHE marketplace test by what each case actually traps

Local regression-trap verification (reverting _needs_canonical_host_prefix
to ``return False`` and re-running) revealed that one of the three
parametrized cases -- ``dict-host-qualified-repo`` -- passes regardless
of whether the fix is enabled. The manifest's ``repo`` field carries the
host through ``_resolve_github_source`` so the canonical reaches the
prefix step already host-qualified; the fix's idempotent guard makes it
a no-op there.

That case is therefore an idempotency lock, not a microsoft#1285 regression trap.
Bundling it inside ``test_ghe_marketplace_routes_auth_at_enterprise_host``
with the genuinely-trapping ``relative-source`` and ``dict-bare-repo``
cases implied a trapping contract it does not deliver.

Restructure to be honest about contracts:

- ``test_ghe_marketplace_backfills_host_on_bare_canonical`` (parametrized,
  2 cases) -- the actual microsoft#1285 regression trap. Verified by toggling the
  fix off: both cases fail at all three layers (canonical, parse host,
  AuthContext).
- ``test_ghe_marketplace_host_qualified_dict_source_routes_idempotently``
  -- separate test, named for what it locks (idempotent guard + correct
  routing on the already-host-qualified path). Documented as NOT a trap.

The github.com regression check and the microsoft#1305 cross-repo trap are
unchanged in scope; they were already correctly framed as non-microsoft#1285
contracts. Total test count stays at 5; verdict surface is the same.
edenfunf added a commit to edenfunf/apm that referenced this pull request May 14, 2026
Address PR microsoft#1319 review panel findings and the github-advanced-security
``Incomplete URL substring sanitization`` flag without changing the fix's
core design.

logger.info -> logger.warning
=============================

The PR microsoft#1292 review panel's top-five follow-up microsoft#3 (the seed for microsoft#1305)
explicitly recommended ``logger.warning`` for this exact diagnostic:

  "A single ``logger.warning`` (or structured install-time check) would
  close the UX gap... converts a silent failure into an actionable error
  and prevents repeat microsoft#1285-class support tickets."

The original PR microsoft#1319 used ``logger.info`` on the rationale that
``CommandLogger.info`` is documented for "persistent advisory context...
must survive quiet-mode suppression". The current panel's three-persona
convergence (Python Architect + CLI Logging Expert + DevX UX) is that
``info`` is still visually ambient at default verbosity -- an operator
scanning a red ``[x]`` line will not register an adjacent ``[i]`` as
the recovery action. ``warning`` renders ``[!]`` and matches the
traffic-light convention the codebase uses elsewhere. As a side benefit,
``warning`` is implemented on both ``CommandLogger`` and
``NullCommandLogger`` (``info`` is not on the latter), so the message
shape is now safe against any future caller variant.

Remove the ``Hint:`` prefix; the ``[!]`` symbol carries the advisory
signal on its own. Inline the resolved enterprise hostname into the
"registered on" clause so the test assertions can anchor on contextual
prose (e.g. ``"registered on 'corp.ghe.com'"``) instead of bare
hostname substrings -- which silences the CodeQL flag without weakening
what the assertion verifies.

Auth-expert second clause
=========================

The original hint read as if the misconfigured case was the only
explanation for a validation failure: "If you meant the enterprise host,
set the plugin's repo field to corp.ghe.com/...". A legitimate
``github.com`` cross-host dep that fails for a transient reason
(rate-limit, network, expired PAT) would read that hint and add an
enterprise host prefix that breaks a working config.

Append the auth-expert recommended second clause:

  "If this is intentionally a github.com dependency, verify your
  github.com credentials and that the repository is accessible."

Both clauses are explicitly conditional, so neither path is misdirected.
The original issue's "two intents" framing assumed validation success
vs 401; this clause covers the third path (validation failure on a
legitimate dep) that was not in the issue spec but is real.

Integration trap + new e2e test
===============================

``test_cross_repo_locks_known_silent_misroute`` in
``tests/integration/test_ghe_marketplace_install_e2e.py`` was authored
by PR microsoft#1292 specifically to give "the future microsoft#1305 fix an explicit
before/after diff to assert against". The microsoft#1305 fix deliberately
preserves the resolver-level routing (bare cross-repo -> github.com is
correct for legitimate cross-host deps) and adds a sentinel + an
install-time hint instead. Update the test's docstring to reflect this,
keep the routing-preservation assertions, and add three new sentinel
assertions so the metadata the install command consumes is locked at
the integration tier.

Add ``TestCrossRepoMisconfigHintIntegration`` with two scenarios:

- ``test_cross_repo_hint_emitted_on_validation_failure``: drives the
  real ``_resolve_package_references`` + ``InstallLogger`` through
  ``capsys`` and asserts the warning-level hint contains the
  plugin@marketplace identity, the enterprise host anchored to its
  "registered on" clause, the bare repo, the host-qualified fix value,
  and the auth-expert second clause.
- ``test_legitimate_cross_host_validation_passes_no_hint``: locks the
  no-pollution contract for the legitimate cross-host path that
  validates successfully.

This matches the convention PR microsoft#1292 established with PR microsoft#1312 (microsoft#1304
closer): panel-flagged ``outcome: missing`` integration findings on
secure-by-default surfaces should land an integration-tier trap, not
just unit coverage.

CHANGELOG
=========

Add the ``[Unreleased] Fixed`` entry naming GHE enterprise marketplace
explicitly so enterprise teams scanning the changelog for cross-repo
misconfiguration symptoms recognize the fix on upgrade. Mirrors the
PR microsoft#1292 entry style.

Out of scope
============

The supply-chain finding (cross-repo bare where the same owner/repo
exists on github.com with attacker-staged content) is a real
dependency-confusion vector but is not the diagnostic-surface problem
microsoft#1305 targets; tracked as a separate follow-up issue. The doc-writer
finding referenced ``docs/manifest-schema.md`` which does not exist in
this repository; documentation additions deferred to a focused docs PR.
danielmeppiel added a commit to edenfunf/apm that referenced this pull request May 14, 2026
) (microsoft#1319)

* fix: hint to host-qualify cross-repo on *.ghe.com (closes microsoft#1305)

PR microsoft#1292 fixed the silent ``github.com`` auth fallback for **in-marketplace**
plugin sources on ``*.ghe.com`` marketplaces but deliberately scoped its host
backfill via ``_is_in_marketplace_source`` to avoid changing routing for
cross-repo dict sources. A bare cross-repo ``repo: owner/proj`` on an enterprise
marketplace still legitimately means two different things -- a real
``github.com`` open-source dep, or a misconfigured same-host entry that should
have been ``corp.ghe.com/owner/proj`` -- and the resolver cannot disambiguate
them. The silent mis-route survives for the second intent: the canonical stays
bare, ``DependencyReference.parse`` defaults the host to ``github.com``, and
the install path reports the generic
``not accessible or doesn't exist -- run with --verbose for auth details``
with zero pointer at the marketplace's enterprise host.

Surface the diagnostic at the install-time validation-failure boundary, not
at resolver time. The legitimate cross-host case validates successfully and
never sees a hint; the misconfigured case fails validation and gets an
actionable host-qualified suggestion. The resolver-time always-on warning
the PR microsoft#1292 review panel rejected -- which would false-positive on the
legitimate case and train operators to ignore -- is avoided.

Approach
========

Resolver attaches a typed ``CrossRepoMisconfigRisk`` sentinel to
``MarketplacePluginResolution`` when **all** of:

- ``dependency_reference`` is ``None`` (GitHub-family virtual-shorthand path;
  GitLab-class and self-managed FQDN marketplaces build a structured ref
  upstream and sidestep the bug)
- ``plugin.source`` is a dict whose normalized type is ``github`` -- via the
  existing ``_coerce_dict_plugin_type`` (covers ``type``/``kind``/``source``
  synonyms plus the inferred-github fallback). Cross-repo ``gitlab`` /
  ``git-subdir`` dict sources on enterprise marketplaces hit the same
  auth-routing bug but the "host-qualify with marketplace host" remediation
  only matches operator intent for the GitHub family.
- the source is NOT an in-marketplace reference (PR microsoft#1292's domain)
- ``_needs_canonical_host_prefix`` agrees the canonical is bare and the host
  is GitHub-family enterprise (``*.ghe.com``; idempotent against already
  host-qualified, URL, and SSH forms)
- the ``repo`` field is a non-empty ``owner/repo`` shorthand

The helper is pure -- no logging, no canonical mutation. Resolver behavior is
unchanged; only the resolution object carries one extra optional field.

Install command records the risk in a per-call ``_misconfig_risks`` dict
**before** validation runs. The existing ``_marketplace_provenance`` map only
gets written on validation success and cannot be relied on at the failure
boundary. When ``_validate_package_exists`` returns ``False`` (which is how
the GitHub-family auth failure surfaces -- ``AuthResolver.try_with_fallback``
collapses 401/404/network into a single ``False``, no typed ``AuthenticationError``),
the validation-fail branch emits the hint inline via ``logger.info`` so the
operator can correct ``marketplace.json`` without rerunning under ``--verbose``
to decode the auth trace.

Why this layer, not ``AuthenticationError``
===========================================

The two ``raise AuthenticationError`` sites in the install pipeline are both
gated to non-GitHub hosts (ADO / self-managed): ``pipeline.py`` preflight skips
``is_github_hostname(host)`` early; ``validation.py`` requires the
``is_ado_auth_failure_signal`` stderr pattern. The github.com fallback path
goes through ``try_with_fallback`` which returns ``False`` on failure, and the
caller records ``(canonical, reason)`` into ``invalid_outcomes``. Decorating
``AuthenticationError`` would be a dead-code hook for this bug -- the typed
exception never fires on the github.com path. The validation-fail branch is
the actual choke point.

Scope and tradeoffs
===================

- 404 typo on the cross-repo ``repo`` field and network failures will also
  trigger the hint; the wording leads with the routing fact ("resolved to
  'github.com'") and the suggestion is conditional ("If you meant the
  enterprise host"), so the false-positive remains advisory rather than
  misleading. Distinguishing 401 from 404/network here would require
  threading HTTP status out of ``try_with_fallback`` -- a much broader
  cross-cutting change.
- The silent-success-on-wrong-host case (cross-repo bare where the same
  ``owner/repo`` happens to exist on github.com with different content)
  cannot be detected without changing the routing semantics PR microsoft#1292
  preserved. This is acknowledged out of scope in the issue.

Tests
=====

``TestCrossRepoMisconfigRisk`` (resolver, 14 cases) locks the truth table for
sentinel attach / no-attach across the dict-type synonyms (``type``, ``kind``,
``source``, inferred-github), host-qualified / URL / SSH / no-slash defensive
guards, the gitlab / git-subdir exclusion, and pure ``github.com`` marketplace
non-pollution.

``TestResolvePackageReferencesCrossRepoMisconfigHint`` (install, 4 cases)
locks the hint emission contract: hint fires only when a risk-bearing
marketplace resolution subsequently fails validation; the legitimate
cross-host path that validates successfully emits no hint; in-marketplace
and plain owner/repo failures emit no hint.

Both test suites were toggle-verified -- removing the resolver helper call or
the install-side emission block makes the corresponding positive case fail.

* review: warning-level hint, anchored test substrings, integration trap

Address PR microsoft#1319 review panel findings and the github-advanced-security
``Incomplete URL substring sanitization`` flag without changing the fix's
core design.

logger.info -> logger.warning
=============================

The PR microsoft#1292 review panel's top-five follow-up microsoft#3 (the seed for microsoft#1305)
explicitly recommended ``logger.warning`` for this exact diagnostic:

  "A single ``logger.warning`` (or structured install-time check) would
  close the UX gap... converts a silent failure into an actionable error
  and prevents repeat microsoft#1285-class support tickets."

The original PR microsoft#1319 used ``logger.info`` on the rationale that
``CommandLogger.info`` is documented for "persistent advisory context...
must survive quiet-mode suppression". The current panel's three-persona
convergence (Python Architect + CLI Logging Expert + DevX UX) is that
``info`` is still visually ambient at default verbosity -- an operator
scanning a red ``[x]`` line will not register an adjacent ``[i]`` as
the recovery action. ``warning`` renders ``[!]`` and matches the
traffic-light convention the codebase uses elsewhere. As a side benefit,
``warning`` is implemented on both ``CommandLogger`` and
``NullCommandLogger`` (``info`` is not on the latter), so the message
shape is now safe against any future caller variant.

Remove the ``Hint:`` prefix; the ``[!]`` symbol carries the advisory
signal on its own. Inline the resolved enterprise hostname into the
"registered on" clause so the test assertions can anchor on contextual
prose (e.g. ``"registered on 'corp.ghe.com'"``) instead of bare
hostname substrings -- which silences the CodeQL flag without weakening
what the assertion verifies.

Auth-expert second clause
=========================

The original hint read as if the misconfigured case was the only
explanation for a validation failure: "If you meant the enterprise host,
set the plugin's repo field to corp.ghe.com/...". A legitimate
``github.com`` cross-host dep that fails for a transient reason
(rate-limit, network, expired PAT) would read that hint and add an
enterprise host prefix that breaks a working config.

Append the auth-expert recommended second clause:

  "If this is intentionally a github.com dependency, verify your
  github.com credentials and that the repository is accessible."

Both clauses are explicitly conditional, so neither path is misdirected.
The original issue's "two intents" framing assumed validation success
vs 401; this clause covers the third path (validation failure on a
legitimate dep) that was not in the issue spec but is real.

Integration trap + new e2e test
===============================

``test_cross_repo_locks_known_silent_misroute`` in
``tests/integration/test_ghe_marketplace_install_e2e.py`` was authored
by PR microsoft#1292 specifically to give "the future microsoft#1305 fix an explicit
before/after diff to assert against". The microsoft#1305 fix deliberately
preserves the resolver-level routing (bare cross-repo -> github.com is
correct for legitimate cross-host deps) and adds a sentinel + an
install-time hint instead. Update the test's docstring to reflect this,
keep the routing-preservation assertions, and add three new sentinel
assertions so the metadata the install command consumes is locked at
the integration tier.

Add ``TestCrossRepoMisconfigHintIntegration`` with two scenarios:

- ``test_cross_repo_hint_emitted_on_validation_failure``: drives the
  real ``_resolve_package_references`` + ``InstallLogger`` through
  ``capsys`` and asserts the warning-level hint contains the
  plugin@marketplace identity, the enterprise host anchored to its
  "registered on" clause, the bare repo, the host-qualified fix value,
  and the auth-expert second clause.
- ``test_legitimate_cross_host_validation_passes_no_hint``: locks the
  no-pollution contract for the legitimate cross-host path that
  validates successfully.

This matches the convention PR microsoft#1292 established with PR microsoft#1312 (microsoft#1304
closer): panel-flagged ``outcome: missing`` integration findings on
secure-by-default surfaces should land an integration-tier trap, not
just unit coverage.

CHANGELOG
=========

Add the ``[Unreleased] Fixed`` entry naming GHE enterprise marketplace
explicitly so enterprise teams scanning the changelog for cross-repo
misconfiguration symptoms recognize the fix on upgrade. Mirrors the
PR microsoft#1292 entry style.

Out of scope
============

The supply-chain finding (cross-repo bare where the same owner/repo
exists on github.com with attacker-staged content) is a real
dependency-confusion vector but is not the diagnostic-surface problem
microsoft#1305 targets; tracked as a separate follow-up issue. The doc-writer
finding referenced ``docs/manifest-schema.md`` which does not exist in
this repository; documentation additions deferred to a focused docs PR.

* style: ruff format new test files

---------

Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Marketplace install fails for *.ghe.com hosts — auth resolves against github.com instead of registered host

4 participants