Skip to content

Commit 0746507

Browse files
committed
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.
1 parent 22ebb35 commit 0746507

4 files changed

Lines changed: 656 additions & 1 deletion

File tree

src/apm_cli/commands/install.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,13 @@ def _resolve_package_references(
349349
invalid_outcomes = [] # (package, reason) tuples
350350
_marketplace_provenance = {} # canonical -> {discovered_via, marketplace_plugin_name}
351351
_apm_yml_entries = {} # canonical -> apm.yml entry (str or dict for HTTP deps)
352+
# #1305: canonical -> (marketplace_name, plugin_name, CrossRepoMisconfigRisk)
353+
# for cross-repo dict ``type: github`` sources on enterprise marketplaces
354+
# whose bare ``repo`` would mis-route auth at ``github.com``. Recorded
355+
# before validation runs so the validation-fail branch can emit an
356+
# actionable hint -- ``_marketplace_provenance`` is only written on
357+
# validation success and cannot be relied on at the failure boundary.
358+
_misconfig_risks = {}
352359
validated_packages = []
353360
dependencies_changed = False
354361

@@ -403,6 +410,13 @@ def warning_handler(msg):
403410
}
404411
package = canonical_str
405412
marketplace_dep_ref = getattr(resolution, "dependency_reference", None)
413+
_risk = getattr(resolution, "cross_repo_misconfig_risk", None)
414+
if _risk is not None:
415+
_misconfig_risks[canonical_str] = (
416+
marketplace_name,
417+
plugin_name,
418+
_risk,
419+
)
406420
except Exception as mkt_err:
407421
reason = str(mkt_err)
408422
invalid_outcomes.append((package, reason))
@@ -521,6 +535,24 @@ def warning_handler(msg):
521535
invalid_outcomes.append((package, reason))
522536
if logger:
523537
logger.validation_fail(package, reason)
538+
# #1305: when a cross-repo dict ``type: github`` source on an
539+
# enterprise marketplace fails validation, the failure is most
540+
# likely the silent auth mis-route (bare canonical fell back to
541+
# ``github.com``). Surface the host-qualify hint inline so the
542+
# operator can correct ``marketplace.json`` without rerunning
543+
# under ``--verbose`` to decode the auth trace. The legitimate
544+
# cross-host case validates successfully and never reaches here.
545+
_risk_entry = _misconfig_risks.get(package)
546+
if _risk_entry is not None and logger:
547+
_mp_name, _plugin_name, _risk = _risk_entry
548+
logger.info(
549+
f"Hint: '{_plugin_name}@{_mp_name}' is registered on "
550+
f"'{_risk.marketplace_host}', but the plugin's bare "
551+
f"`repo: {_risk.bare_repo_field}` resolved to "
552+
"'github.com'. If you meant the enterprise host, set "
553+
"the plugin's `repo` field to "
554+
f"'{_risk.suggested_qualified_repo}' in marketplace.json."
555+
)
524556

525557
return (
526558
valid_outcomes,

src/apm_cli/marketplace/resolver.py

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,29 @@
4747
_SEMVER_RANGE_CHARS = re.compile(r"[~^<>=!]")
4848

4949

50+
@dataclass(frozen=True)
51+
class CrossRepoMisconfigRisk:
52+
"""Signal that a cross-repo dict ``type: github`` source on an enterprise
53+
GitHub-family marketplace resolved to a bare canonical (#1305).
54+
55+
Attached to :class:`MarketplacePluginResolution` when the marketplace is on
56+
``*.ghe.com`` and the plugin's dict source declares a bare ``owner/repo``
57+
that does not match the marketplace project. The resolver deliberately
58+
leaves these canonicals bare (PR #1292 scoped its host backfill to
59+
in-marketplace sources), so ``DependencyReference.parse`` defaults the host
60+
to ``github.com``. Two intents share this syntax -- a legitimate cross-host
61+
``github.com`` open-source dep, or a misconfigured same-host entry that
62+
should have been ``corp.ghe.com/owner/repo`` -- and the resolver cannot
63+
distinguish them. The install command consults this sentinel when the
64+
package fails validation so an actionable hint surfaces only at the
65+
failure boundary, never on the legitimate path.
66+
"""
67+
68+
marketplace_host: str
69+
bare_repo_field: str
70+
suggested_qualified_repo: str
71+
72+
5073
@dataclass
5174
class MarketplacePluginResolution:
5275
"""Outcome of :func:`resolve_marketplace_plugin`.
@@ -57,11 +80,15 @@ class MarketplacePluginResolution:
5780
subdirectory plugins), install logic should prefer it over
5881
:meth:`~apm_cli.models.dependency.reference.DependencyReference.parse`
5982
on :attr:`canonical` to avoid mis-parsing nested paths as GitLab project segments.
83+
:attr:`cross_repo_misconfig_risk` is non-``None`` only for the #1305
84+
cross-repo bare-on-enterprise pattern; consumers emit it as a hint when the
85+
package subsequently fails validation.
6086
"""
6187

6288
canonical: str
6389
plugin: MarketplacePlugin
6490
dependency_reference: DependencyReference | None = None
91+
cross_repo_misconfig_risk: CrossRepoMisconfigRisk | None = None
6592

6693
def __iter__(self) -> Iterator[str | MarketplacePlugin]:
6794
yield self.canonical
@@ -221,6 +248,55 @@ def _needs_canonical_host_prefix(canonical: str, host: str) -> bool:
221248
return first_segment.lower() != h.lower()
222249

223250

251+
def _compute_cross_repo_misconfig_risk(
252+
plugin: MarketplacePlugin,
253+
source: MarketplaceSource,
254+
canonical: str,
255+
dep_ref: DependencyReference | None,
256+
) -> CrossRepoMisconfigRisk | None:
257+
"""Identify the #1305 misconfiguration: cross-repo dict ``type: github``
258+
source with bare ``repo`` on an enterprise GitHub-family marketplace.
259+
260+
Returns a :class:`CrossRepoMisconfigRisk` when **all** of:
261+
262+
- ``dep_ref`` is ``None`` (GitHub-family virtual-shorthand path; GitLab and
263+
self-managed FQDNs build a structured ref upstream and sidestep the bug)
264+
- ``plugin.source`` is a dict whose normalized type is ``github`` (other
265+
dict types -- ``gitlab``, ``git-subdir`` -- hit the same auth-routing
266+
bug but the "host-qualify with marketplace host" remediation only
267+
matches operator intent for the GitHub family)
268+
- the source is **not** an in-marketplace reference (PR #1292 already
269+
backfills the host for those)
270+
- ``_needs_canonical_host_prefix`` agrees the canonical is bare and the
271+
host is GitHub-family enterprise (``*.ghe.com``; idempotent against
272+
already host-qualified, URL, and SSH forms)
273+
- the ``repo`` field is a non-empty ``owner/repo`` shorthand
274+
275+
Otherwise returns ``None``. Pure -- no logging, no side effects.
276+
"""
277+
if dep_ref is not None:
278+
return None
279+
if not isinstance(plugin.source, dict):
280+
return None
281+
if _coerce_dict_plugin_type(plugin.source) != "github":
282+
return None
283+
if _is_in_marketplace_source(plugin, source):
284+
return None
285+
if not _needs_canonical_host_prefix(canonical, source.host):
286+
return None
287+
repo_field = plugin.source.get("repo", "")
288+
if not isinstance(repo_field, str):
289+
return None
290+
bare = repo_field.strip().lstrip("/")
291+
if "/" not in bare:
292+
return None
293+
return CrossRepoMisconfigRisk(
294+
marketplace_host=source.host,
295+
bare_repo_field=bare,
296+
suggested_qualified_repo=f"{source.host}/{bare}",
297+
)
298+
299+
224300
def _marketplace_https_git_url(source: MarketplaceSource) -> str:
225301
"""HTTPS clone URL for the registered marketplace project (same project as ``marketplace.json``)."""
226302
segments = [p for p in f"{source.owner}/{source.repo}".split("/") if p]
@@ -607,6 +683,19 @@ def _emit_warning(msg: str) -> None:
607683
marketplace_name,
608684
)
609685

686+
# ---- Cross-repo misconfig sentinel (#1305) ----
687+
# PR #1292's host backfill only covers in-marketplace sources. A cross-repo
688+
# dict ``type: github`` source with a bare ``repo`` on an enterprise
689+
# marketplace cannot be safely backfilled here -- the bare syntax also
690+
# legitimately means "a github.com open-source dep from this enterprise
691+
# marketplace" -- so the canonical stays bare and downstream auth routes at
692+
# github.com. Attach a sentinel so the install command can emit an
693+
# actionable hint ONLY when the package subsequently fails validation; the
694+
# legitimate cross-host path validates fine and never sees the hint.
695+
cross_repo_misconfig_risk = _compute_cross_repo_misconfig_risk(
696+
plugin, source, canonical, dep_ref
697+
)
698+
610699
# ---- Raw ref override ----
611700
# When version_spec is provided it is treated as a raw git ref that
612701
# overrides whatever ref came from the marketplace source field.
@@ -674,5 +763,8 @@ def _emit_warning(msg: str) -> None:
674763
logger.debug("Shadow detection failed", exc_info=True)
675764

676765
return MarketplacePluginResolution(
677-
canonical=canonical, plugin=plugin, dependency_reference=dep_ref
766+
canonical=canonical,
767+
plugin=plugin,
768+
dependency_reference=dep_ref,
769+
cross_repo_misconfig_risk=cross_repo_misconfig_risk,
678770
)

tests/unit/commands/test_install_resolve_refs.py

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,3 +280,205 @@ def test_generated_entry_round_trips_via_from_apm_yml(self, tmp_path):
280280
assert ref.repo_url == "epm-ease/apm-registry"
281281
assert ref.virtual_path == "agents/ai-run-ba-flow"
282282
assert ref.is_virtual is True
283+
284+
285+
# ---------------------------------------------------------------------------
286+
# #1305 -- cross-repo bare-on-enterprise hint surfaces on validation failure
287+
# ---------------------------------------------------------------------------
288+
289+
290+
class TestResolvePackageReferencesCrossRepoMisconfigHint:
291+
"""When a marketplace resolution attaches a ``CrossRepoMisconfigRisk``
292+
sentinel and the package later fails validation, an actionable hint must
293+
be emitted via the logger. The legitimate cross-host path validates
294+
successfully and never reaches the hint branch."""
295+
296+
@staticmethod
297+
def _resolution_with_risk():
298+
from apm_cli.marketplace.models import MarketplacePlugin
299+
from apm_cli.marketplace.resolver import (
300+
CrossRepoMisconfigRisk,
301+
MarketplacePluginResolution,
302+
)
303+
304+
plugin = MarketplacePlugin(
305+
name="shared-tool",
306+
source={
307+
"type": "github",
308+
"repo": "platform-team/shared-tool",
309+
"path": "plugins/shared",
310+
},
311+
)
312+
return MarketplacePluginResolution(
313+
canonical="platform-team/shared-tool/plugins/shared",
314+
plugin=plugin,
315+
dependency_reference=None,
316+
cross_repo_misconfig_risk=CrossRepoMisconfigRisk(
317+
marketplace_host="corp.ghe.com",
318+
bare_repo_field="platform-team/shared-tool",
319+
suggested_qualified_repo=(
320+
"corp.ghe.com/platform-team/shared-tool"
321+
),
322+
),
323+
)
324+
325+
@staticmethod
326+
def _resolution_without_risk():
327+
from apm_cli.marketplace.models import MarketplacePlugin
328+
from apm_cli.marketplace.resolver import MarketplacePluginResolution
329+
330+
plugin = MarketplacePlugin(
331+
name="shared-tool",
332+
source="./plugins/shared",
333+
)
334+
return MarketplacePluginResolution(
335+
canonical="myorg/my-marketplace/plugins/shared",
336+
plugin=plugin,
337+
dependency_reference=None,
338+
cross_repo_misconfig_risk=None,
339+
)
340+
341+
@patch("apm_cli.commands.install._validate_package_exists", return_value=False)
342+
@patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin")
343+
@patch("apm_cli.marketplace.resolver.parse_marketplace_ref")
344+
@patch("apm_cli.commands.install.DependencyReference")
345+
def test_hint_emitted_on_validation_failure_with_risk(
346+
self,
347+
mock_dep_cls,
348+
mock_parse_ref,
349+
mock_resolve_mkt,
350+
mock_validate,
351+
):
352+
"""Risk-bearing resolution that fails validation emits the hint."""
353+
mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None)
354+
mock_resolve_mkt.return_value = self._resolution_with_risk()
355+
ref = _make_dep_ref(
356+
"platform-team/shared-tool/plugins/shared",
357+
"github.com/platform-team/shared-tool/plugins/shared",
358+
)
359+
mock_dep_cls.parse.return_value = ref
360+
mock_dep_cls.is_local_path.return_value = False
361+
_disable_gitlab_direct_probe(mock_dep_cls)
362+
363+
logger = MagicMock()
364+
logger.verbose = False
365+
366+
_resolve_package_references(
367+
["shared-tool@my-marketplace"],
368+
[],
369+
set(),
370+
logger=logger,
371+
)
372+
373+
# The hint must be emitted exactly once and include the
374+
# actionable host-qualified suggestion.
375+
info_calls = [c for c in logger.info.call_args_list]
376+
assert len(info_calls) == 1
377+
emitted = info_calls[0].args[0]
378+
assert "shared-tool@my-marketplace" in emitted
379+
assert "corp.ghe.com" in emitted
380+
assert "platform-team/shared-tool" in emitted
381+
assert (
382+
"corp.ghe.com/platform-team/shared-tool" in emitted
383+
)
384+
385+
@patch("apm_cli.commands.install._validate_package_exists", return_value=True)
386+
@patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin")
387+
@patch("apm_cli.marketplace.resolver.parse_marketplace_ref")
388+
@patch("apm_cli.commands.install.DependencyReference")
389+
def test_hint_not_emitted_when_validation_passes_even_with_risk(
390+
self,
391+
mock_dep_cls,
392+
mock_parse_ref,
393+
mock_resolve_mkt,
394+
mock_validate,
395+
):
396+
"""The legitimate cross-host path (validation succeeds) must NOT
397+
emit the hint -- this is the entire reason the hint lives at the
398+
failure boundary instead of at resolver time."""
399+
mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None)
400+
mock_resolve_mkt.return_value = self._resolution_with_risk()
401+
ref = _make_dep_ref(
402+
"platform-team/shared-tool/plugins/shared",
403+
"github.com/platform-team/shared-tool/plugins/shared",
404+
)
405+
mock_dep_cls.parse.return_value = ref
406+
mock_dep_cls.is_local_path.return_value = False
407+
_disable_gitlab_direct_probe(mock_dep_cls)
408+
409+
logger = MagicMock()
410+
logger.verbose = False
411+
412+
_resolve_package_references(
413+
["shared-tool@my-marketplace"],
414+
[],
415+
set(),
416+
logger=logger,
417+
)
418+
419+
assert logger.info.call_args_list == []
420+
421+
@patch("apm_cli.commands.install._validate_package_exists", return_value=False)
422+
@patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin")
423+
@patch("apm_cli.marketplace.resolver.parse_marketplace_ref")
424+
@patch("apm_cli.commands.install.DependencyReference")
425+
def test_no_hint_when_resolution_has_no_risk(
426+
self,
427+
mock_dep_cls,
428+
mock_parse_ref,
429+
mock_resolve_mkt,
430+
mock_validate,
431+
):
432+
"""In-marketplace / non-enterprise resolutions carry no risk
433+
sentinel; validation-fail must NOT print a hint."""
434+
mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None)
435+
mock_resolve_mkt.return_value = self._resolution_without_risk()
436+
ref = _make_dep_ref(
437+
"myorg/my-marketplace/plugins/shared",
438+
"github.com/myorg/my-marketplace/plugins/shared",
439+
)
440+
mock_dep_cls.parse.return_value = ref
441+
mock_dep_cls.is_local_path.return_value = False
442+
_disable_gitlab_direct_probe(mock_dep_cls)
443+
444+
logger = MagicMock()
445+
logger.verbose = False
446+
447+
_resolve_package_references(
448+
["shared-tool@my-marketplace"],
449+
[],
450+
set(),
451+
logger=logger,
452+
)
453+
454+
assert logger.info.call_args_list == []
455+
456+
@patch("apm_cli.commands.install._validate_package_exists", return_value=False)
457+
@patch("apm_cli.commands.install.DependencyReference")
458+
def test_no_hint_for_plain_owner_repo_failure(
459+
self,
460+
mock_dep_cls,
461+
mock_validate,
462+
):
463+
"""A bare ``owner/repo`` (no marketplace) that fails validation
464+
must NOT trigger the hint -- the risk map is only populated by
465+
marketplace resolutions."""
466+
ref = _make_dep_ref(
467+
"platform-team/shared-tool",
468+
"github.com/platform-team/shared-tool",
469+
)
470+
mock_dep_cls.parse.return_value = ref
471+
mock_dep_cls.is_local_path.return_value = False
472+
_disable_gitlab_direct_probe(mock_dep_cls)
473+
474+
logger = MagicMock()
475+
logger.verbose = False
476+
477+
_resolve_package_references(
478+
["platform-team/shared-tool"],
479+
[],
480+
set(),
481+
logger=logger,
482+
)
483+
484+
assert logger.info.call_args_list == []

0 commit comments

Comments
 (0)