From d6788311537f6c5b76fa60530e8470f248a4be3b 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: Sat, 23 May 2026 14:25:08 +0800 Subject: [PATCH 1/4] fix(install): block bare cross-repo on enterprise marketplaces (#1326) A *.ghe.com marketplace whose plugin source uses dict-form `type: github` with a bare `repo: owner/repo` resolves to a bare canonical that `DependencyReference.parse` defaults to github.com. The #1305 advisory hint only fired on validation FAILURE; the SUCCESS path -- attacker pre-stages the bare namespace on public github.com -- reached no signal and the install fetched attacker content under enterprise auth. Move the check to the install command's pre-validation boundary: when the resolver attaches `CrossRepoMisconfigRisk`, reject the package immediately. `_validate_package_exists`, `requests.get`, and `requests.head` never run on the gated path, so no probe reaches the potentially-attacker-controlled URL. The existing host-qualification syntax is the escape hatch. The resolver now recognises an explicitly-qualified `repo:` field (e.g. `repo: github.com/owner/repo` for declared cross-host intent) and does not attach the sentinel, so legitimate cross-host installs proceed. No new CLI flag, env var, or schema field is introduced. BREAKING: marketplaces that relied on bare cross-repo working today must host-qualify the `repo:` field in marketplace.json -- either `corp.ghe.com/owner/repo` for an enterprise dep or `github.com/owner/repo` for a declared cross-host dep. The refusal message names both options inline. Closes #1326 --- CHANGELOG.md | 4 + .../docs/producer/publish-to-a-marketplace.md | 8 + .../skills/apm-usage/package-authoring.md | 30 +++ src/apm_cli/commands/install.py | 78 +++--- src/apm_cli/marketplace/resolver.py | 44 +++- .../test_ghe_marketplace_install_e2e.py | 236 ++++++++++++------ .../commands/test_install_resolve_refs.py | 130 ++++------ .../marketplace/test_marketplace_resolver.py | 29 +++ 8 files changed, 362 insertions(+), 197 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1110f2352..315143b74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **BREAKING:** `apm install` against a `*.ghe.com` marketplace now refuses bare cross-repo `repo:` fields in `marketplace.json` before any outbound validation HTTP runs, closing a dependency-confusion vector where an attacker pre-staging the bare namespace on public `github.com` would have the install silently resolve at attacker content. Marketplace authors must host-qualify cross-repo sources: `repo: github.com/owner/repo` for a declared `github.com` open-source dep, or `repo: corp.ghe.com/owner/repo` for an enterprise dep. The earlier `#1305` advisory-hint-on-failure design covered only the failure path; the success path (attacker namespace exists) is now fail-closed at the install boundary. (closes #1326) + ### Fixed - Copilot, Codex, Cursor, Claude, Windsurf, OpenCode, and Gemini adapters handle MCP v0.1 `runtimeArguments`/`packageArguments` with `variables` (no `type` key), matching the VS Code fix from #1444. (#1461, closes #1452, thanks @sergio-sisternes-epam) diff --git a/docs/src/content/docs/producer/publish-to-a-marketplace.md b/docs/src/content/docs/producer/publish-to-a-marketplace.md index 0533c4271..76f4ef76f 100644 --- a/docs/src/content/docs/producer/publish-to-a-marketplace.md +++ b/docs/src/content/docs/producer/publish-to-a-marketplace.md @@ -220,6 +220,14 @@ consumers running `apm install --update` on their own cadence. - **No `versions[]` array.** Each compiled package carries one resolved ref -- the highest tag matching the range at build time. Re-run `apm pack` and re-tag to publish a new version. +- **Bare cross-repo `repo:` on enterprise (`*.ghe.com`) marketplaces is + refused at install time.** Dict-form plugin sources that point to a + different repo than the marketplace project must host-qualify the + `repo:` field -- `repo: corp.ghe.com/owner/repo` for an enterprise dep + or `repo: github.com/owner/repo` for a declared cross-host dep. A bare + `owner/repo` cannot be disambiguated from a dependency-confusion + attempt where an attacker pre-stages the namespace on public + `github.com`, so the install command fail-closes before validating. ## Governance diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 1dcf070f7..3f23b15e2 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -351,6 +351,36 @@ Schema rules: checks that `version:` is present). Omit entirely to skip the gate. - Unknown keys raise a schema error -- do not invent fields. +### Cross-repo plugin sources on enterprise marketplaces + +When a marketplace published on a `*.ghe.com` host references a plugin +in a different repo via dict-form `source: {type: github, repo: ...}`, +the `repo:` field **must be host-qualified**. A bare `owner/repo` form +is refused at install time because it cannot be disambiguated from a +public-github.com dependency-confusion attempt (see CHANGELOG entry +for #1326). Two valid forms: + +```yaml +plugins: + - name: shared-tool + source: + type: github + # Enterprise dep (most common): host-qualify to the marketplace host + repo: corp.ghe.com/platform-team/shared-tool + path: plugins/shared + + - name: opensource-helper + source: + type: github + # Declared cross-host dep: host-qualify to github.com explicitly + repo: github.com/opensource-org/helper + path: plugins/helper +``` + +In-marketplace plugins (`source: ./...` or `source: owner/marketplace-repo` +when it matches the marketplace project) are unaffected -- the resolver +backfills the host automatically. + ### Build semantics `apm pack` runs `git ls-remote` against each remote plugin source, picks the diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index f81f828b5..b57bfd6f1 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -352,13 +352,6 @@ def _resolve_package_references( invalid_outcomes = [] # (package, reason) tuples _marketplace_provenance = {} # canonical -> {discovered_via, marketplace_plugin_name} _apm_yml_entries = {} # canonical -> apm.yml entry (str or dict for HTTP deps) - # #1305: canonical -> (marketplace_name, plugin_name, CrossRepoMisconfigRisk) - # for cross-repo dict ``type: github`` sources on enterprise marketplaces - # whose bare ``repo`` would mis-route auth at ``github.com``. Recorded - # before validation runs so the validation-fail branch can emit an - # actionable hint -- ``_marketplace_provenance`` is only written on - # validation success and cannot be relied on at the failure boundary. - _misconfig_risks = {} validated_packages = [] dependencies_changed = False @@ -407,19 +400,48 @@ def warning_handler(msg): canonical_str, _resolved_plugin = resolution if logger: logger.verbose_detail(f" Resolved to: {canonical_str}") + # #1326: dependency-confusion fail-closed gate. + # When the resolver attaches ``CrossRepoMisconfigRisk``, + # the marketplace declared a bare ``owner/repo`` on a + # ``*.ghe.com`` host and the canonical falls back to + # ``github.com`` -- the same syntactic form that an + # attacker pre-staging the namespace on public github.com + # would exploit. Refuse before any outbound validation so + # no probe reaches the potentially-attacker-controlled + # URL (information leak + RCE both shut at one boundary). + # Escape hatch: marketplace.json author host-qualifies + # ``repo:`` (either to the enterprise host for same-host + # intent or to github.com for declared cross-host intent). + # That prevents the sentinel from attaching at resolver + # layer -- no new flag, env var, or schema field needed. + _risk = getattr(resolution, "cross_repo_misconfig_risk", None) + if _risk is not None: + # Multi-line reason: the two qualification options + # are alternatives, not a single instruction, so + # they read clearer as separate bullets under a + # short lead-in. ``validation_fail`` prepends the + # package name; the body below is the remediation. + reason = ( + "refused (dependency-confusion risk #1326): " + f"bare `repo: {_risk.bare_repo_field}` on " + f"enterprise marketplace '{_risk.marketplace_host}'" + " is ambiguous. Host-qualify the plugin `repo` " + "field in marketplace.json to one of:\n" + f" - '{_risk.suggested_qualified_repo}' " + "(enterprise dep on this marketplace)\n" + f" - 'github.com/{_risk.bare_repo_field}' " + "(declared cross-host dep on public github.com)" + ) + invalid_outcomes.append((package, reason)) + if logger: + logger.validation_fail(package, reason) + continue marketplace_provenance = { "discovered_via": marketplace_name, "marketplace_plugin_name": plugin_name, } package = canonical_str marketplace_dep_ref = getattr(resolution, "dependency_reference", None) - _risk = getattr(resolution, "cross_repo_misconfig_risk", None) - if _risk is not None: - _misconfig_risks[canonical_str] = ( - marketplace_name, - plugin_name, - _risk, - ) except Exception as mkt_err: reason = str(mkt_err) invalid_outcomes.append((package, reason)) @@ -557,34 +579,6 @@ def warning_handler(msg): invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) - # #1305: when a cross-repo dict ``type: github`` source on an - # enterprise marketplace fails validation, the failure is most - # likely the silent auth mis-route (bare canonical fell back to - # ``github.com``). Surface the host-qualify hint inline so the - # operator can correct ``marketplace.json`` without rerunning - # under ``--verbose`` to decode the auth trace. ``logger.warning`` - # is used (not ``info``) per the PR #1292 panel review's explicit - # guidance for this exact follow-up: a misconfiguration that - # voids ``apm install`` should be at warning level, not buried - # in info-level ambient output. The second clause acknowledges - # the legitimate cross-host alternative so operators whose - # github.com dep failed for a transient reason (rate limit, - # network, expired PAT) are not misdirected into adding an - # enterprise host prefix that would break a working config. - _risk_entry = _misconfig_risks.get(package) - if _risk_entry is not None and logger: - _mp_name, _plugin_name, _risk = _risk_entry - logger.warning( - f"'{_plugin_name}@{_mp_name}' is registered on " - f"'{_risk.marketplace_host}' but the plugin's bare " - f"`repo: {_risk.bare_repo_field}` resolved to " - "'github.com'. If you meant the enterprise host, set " - "the plugin's `repo` field to " - f"'{_risk.suggested_qualified_repo}' in marketplace.json. " - "If this is intentionally a github.com dependency, " - "verify your github.com credentials and that the " - "repository is accessible." - ) return ( valid_outcomes, diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index 6aae5b04d..14bbd9005 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -32,7 +32,11 @@ from urllib.parse import quote, urlparse from ..models.dependency.reference import DependencyReference -from ..utils.github_host import is_azure_devops_hostname, is_github_hostname +from ..utils.github_host import ( + is_azure_devops_hostname, + is_github_hostname, + is_supported_git_host, +) from ..utils.path_security import PathTraversalError, validate_path_segments from .client import fetch_or_cache from .errors import PluginNotFoundError @@ -50,7 +54,9 @@ @dataclass(frozen=True) class CrossRepoMisconfigRisk: """Signal that a cross-repo dict ``type: github`` source on an enterprise - GitHub-family marketplace resolved to a bare canonical (#1305). + GitHub-family marketplace declares a bare ``owner/repo`` whose canonical + falls back to ``github.com`` -- the same syntactic ambiguity that powers + a dependency-confusion attack (#1326, formerly diagnosed only as #1305). Attached to :class:`MarketplacePluginResolution` when the marketplace is on ``*.ghe.com`` and the plugin's dict source declares a bare ``owner/repo`` @@ -60,9 +66,18 @@ class CrossRepoMisconfigRisk: to ``github.com``. Two intents share this syntax -- a legitimate cross-host ``github.com`` open-source dep, or a misconfigured same-host entry that should have been ``corp.ghe.com/owner/repo`` -- and the resolver cannot - distinguish them. The install command consults this sentinel when the - package fails validation so an actionable hint surfaces only at the - failure boundary, never on the legitimate path. + distinguish them. + + Consumer contract (#1326): the install command consults this sentinel + BEFORE any outbound validation HTTP call and refuses the package + fail-closed when it is non-``None``. The earlier #1305 design surfaced + only an advisory hint on validation failure, which left the success + path (attacker pre-stages the bare namespace on public github.com) + silently exploitable. Cross-host explicit qualification by the + marketplace author -- ``repo: github.com/owner/repo`` -- prevents + the sentinel from attaching at the resolver layer (see + :func:`_compute_cross_repo_misconfig_risk`), which is the supported + escape hatch for declared cross-host intent. """ marketplace_host: str @@ -80,9 +95,10 @@ class MarketplacePluginResolution: subdirectory plugins), install logic should prefer it over :meth:`~apm_cli.models.dependency.reference.DependencyReference.parse` on :attr:`canonical` to avoid mis-parsing nested paths as GitLab project segments. - :attr:`cross_repo_misconfig_risk` is non-``None`` only for the #1305 - cross-repo bare-on-enterprise pattern; consumers emit it as a hint when the - package subsequently fails validation. + :attr:`cross_repo_misconfig_risk` is non-``None`` only for the + cross-repo bare-on-enterprise pattern (#1305 / #1326); the install + command consumes it as a pre-validation fail-closed signal so a + dependency-confusion attempt cannot reach an outbound HTTP probe. """ canonical: str @@ -290,6 +306,18 @@ def _compute_cross_repo_misconfig_risk( bare = repo_field.strip().lstrip("/") if "/" not in bare: return None + # #1326: an already-host-qualified `repo:` field declares explicit intent + # (e.g. ``repo: github.com/owner/repo`` on a ``*.ghe.com`` marketplace is + # an unambiguous declared cross-host dependency). Only the truly-bare + # ``owner/repo`` form is the dependency-confusion vector this sentinel + # flags. ``_needs_canonical_host_prefix`` above already returns False + # for SAME-host qualification (its idempotency clause); this is the + # symmetric guard for CROSS-host explicit qualification, which the + # idempotency check cannot detect because the canonical starts with a + # different host than ``source.host``. + first_segment = bare.split("/", 1)[0] + if is_supported_git_host(first_segment): + return None return CrossRepoMisconfigRisk( marketplace_host=source.host, bare_repo_field=bare, diff --git a/tests/integration/test_ghe_marketplace_install_e2e.py b/tests/integration/test_ghe_marketplace_install_e2e.py index 26aba648b..e5813f45f 100644 --- a/tests/integration/test_ghe_marketplace_install_e2e.py +++ b/tests/integration/test_ghe_marketplace_install_e2e.py @@ -205,20 +205,20 @@ def test_github_com_marketplace_keeps_github_default(self): assert ctx.host_info.kind == "github" def test_cross_repo_locks_known_silent_misroute(self): - """Regression trap for the cross-repo routing semantics + #1305 sentinel. - - A ``*.ghe.com`` marketplace with a cross-repo dict source bears the - same superficial symptoms as #1285 -- canonical emerges bare, parse - defaults to ``github.com``. The #1305 fix deliberately preserves - that resolver-level routing (a bare cross-repo ``repo`` also - legitimately means "a github.com open-source dep from this enterprise - marketplace") and instead attaches a - :class:`~apm_cli.marketplace.resolver.CrossRepoMisconfigRisk` sentinel - that the install command consults at the validation-failure boundary - to emit an actionable host-qualify hint. This test locks both halves: - the routing preservation (so the legitimate path is not regressed) - and the sentinel attachment (so the hint emission has the metadata - it needs). + """Regression trap for the cross-repo routing semantics + #1326 sentinel. + + A ``*.ghe.com`` marketplace with a bare cross-repo dict source bears + the same superficial symptoms as #1285 -- canonical emerges bare, + parse defaults to ``github.com``. Resolver-level routing is + deliberately preserved (PR #1292 scoped its host backfill to + in-marketplace sources only) and the bare-cross-repo case is flagged + via :class:`~apm_cli.marketplace.resolver.CrossRepoMisconfigRisk` + sentinel. The install command consumes the sentinel at the + pre-validation gate (#1326) to fail-closed before any HTTP probe + reaches the potentially-attacker-controlled ``github.com`` URL. + This test locks the resolver-layer contract: the routing stays + unchanged and the sentinel attaches with the metadata the install + gate needs to render its refusal message. """ plugin = MarketplacePlugin( name="cross-repo", @@ -241,18 +241,18 @@ def test_cross_repo_locks_known_silent_misroute(self): result = resolve_marketplace_plugin("cross-repo", _REPO) # Routing preservation: cross-repo canonical stays bare; parse still - # falls back to ``github.com``. This is intentional -- the legitimate - # cross-host path validates successfully and never needs to recover - # the enterprise host. #1305 surfaces the diagnostic at install time - # when the misconfigured path subsequently fails validation. + # falls back to ``github.com``. Resolver layer is intentionally + # unchanged; the dependency-confusion fix lives at the install + # command's pre-validation gate (#1326) which consumes the sentinel + # attached below. assert result.canonical == "anotherorg/anothertool/plugins/x" dep_ref = DependencyReference.parse(result.canonical) auth = AuthResolver() ctx = auth.resolve_for_dep(dep_ref) assert ctx.host_info.host == "github.com" - # #1305: sentinel must attach so the install command's - # validation-fail branch has the metadata to emit the hint. + # #1326: sentinel must attach so the install command's pre-validation + # gate has the metadata to render the refusal message. risk = result.cross_repo_misconfig_risk assert risk is not None assert risk.marketplace_host == _GHE_HOST @@ -261,34 +261,54 @@ def test_cross_repo_locks_known_silent_misroute(self): @pytest.mark.integration -class TestCrossRepoMisconfigHintIntegration: - """End-to-end: the #1305 hint surfaces when a cross-repo bare entry on - a ``*.ghe.com`` marketplace fails validation. - - Unit tests in ``tests/unit/commands/`` mock ``DependencyReference`` and - ``resolve_marketplace_plugin``; this integration trap walks the real - ``_resolve_package_references`` + real ``InstallLogger`` and asserts on - the actual stdout the operator would see. Required by the PR review - panel (test-coverage-expert: ``outcome: missing`` on a secure-by-default - surface) and matches the e2e-integration convention PR #1292 established - with ``test_ghe_marketplace_backfills_host_on_bare_canonical`` above. - - Stubs at one seam only: - - - ``_validate_package_exists``: forces the failure outcome that triggers - the hint. The real validate path makes outbound HTTP calls; this stub - keeps the test deterministic. Everything between the resolver sentinel - and the logger render is the real code path. +class TestCrossRepoFailClosedIntegration: + """End-to-end: #1326 fail-closed gate blocks bare cross-repo install on + ``*.ghe.com`` marketplaces before any outbound validation HTTP runs. + + Threat model: an enterprise operator's ``corp.ghe.com`` marketplace + declares ``repo: platform-team/shared-tool`` (bare ``owner/repo``) intending + the enterprise repo. ``DependencyReference.parse`` defaults missing hosts + to ``github.com``. An attacker who pre-registers ``platform-team/shared-tool`` + on public ``github.com`` would have the validator succeed against the + attacker repo and install attacker content under the enterprise's APM + context. The #1305 fix only surfaced a hint on validation FAILURE; this + gate is the missing half that closes the success-path attack. + + Gate semantics: + + - Resolver attaches ``CrossRepoMisconfigRisk`` for the ambiguous form + (sentinel logic at ``_compute_cross_repo_misconfig_risk`` in + ``marketplace/resolver.py`` is unchanged). + - Install consumes the sentinel immediately after resolver returns -- + before ``_validate_package_exists`` is called. No HTTP probe to the + potentially-attacker-controlled URL ever runs. + - Escape hatch: marketplace.json author host-qualifies ``repo:`` + (``corp.ghe.com/owner/repo`` OR ``github.com/owner/repo``). That + prevents the sentinel from attaching at resolver layer and install + proceeds. No new CLI flag, env var, or schema field is introduced. """ @pytest.fixture(autouse=True) def _isolate_github_host_env(self, monkeypatch): monkeypatch.delenv("GITHUB_HOST", raising=False) - def test_cross_repo_hint_emitted_on_validation_failure(self, capsys): - """The canonical misconfiguration scenario from #1305 surfaces a - warning-level hint identifying the marketplace host and the exact - host-qualified ``repo`` value to use as a fix.""" + def test_cross_repo_bare_blocks_install_before_validation(self): + """The attack PoC: bare cross-repo on enterprise marketplace. + + ``_validate_package_exists`` is wrapped in a Mock returning True -- + simulating the attacker having pre-staged ``platform-team/shared-tool`` + on github.com. If the gate were absent the install would silently + succeed; the test asserts: + + 1. The mock is **never called** (gate fires pre-validation, no HTTP + leak to attacker-controlled host). + 2. Independent ``requests.get`` / ``requests.head`` probes are also + never issued (defense-in-depth: catches a future refactor that + moves validation away from ``_validate_package_exists``). + 3. The package lands in ``invalid_outcomes`` (install rejected). + 4. The reason string names both escape-hatch forms so the operator + can choose qualify-to-enterprise vs qualify-to-github.com. + """ from apm_cli.commands.install import _resolve_package_references from apm_cli.core.command_logger import InstallLogger @@ -312,37 +332,61 @@ def test_cross_repo_hint_emitted_on_validation_failure(self, capsys): ), patch( "apm_cli.commands.install._validate_package_exists", - return_value=False, - ), + return_value=True, + ) as mock_validate, + patch("apm_cli.install.validation.requests.get") as mock_http_get, + patch( + "apm_cli.install.validation.requests.head", + create=True, + ) as mock_http_head, ): - _resolve_package_references( + result = _resolve_package_references( ["shared-tool@my-marketplace"], [], set(), logger=InstallLogger(verbose=False), ) - captured = capsys.readouterr() - emitted = captured.out - # Hint identifies the plugin@marketplace - assert "'shared-tool@my-marketplace'" in emitted - # Marketplace host is named in the "registered on" clause - # (anchored substring sidesteps CodeQL bare-host pattern recognizers) - assert f"registered on '{_GHE_HOST}'" in emitted - # The bare repo from marketplace.json is echoed back - assert "`repo: platform-team/shared-tool`" in emitted - # Concrete remediation value the operator can copy-paste - assert f"'{_GHE_HOST}/platform-team/shared-tool'" in emitted - # Auth-expert clause acknowledges the legitimate-cross-host - # alternative so transient failures of real github.com deps are - # not misdirected into adding an enterprise host prefix. - assert "intentionally a github.com dependency" in emitted - - def test_legitimate_cross_host_validation_passes_no_hint(self, capsys): - """The legitimate cross-host case (validation passes) emits no hint. - - This is the entire reason the diagnostic lives at the - validation-failure boundary instead of resolver time.""" + valid_outcomes, invalid_outcomes = result[0], result[1] + + # (1) Gate fires pre-validation: validate function never called. + assert mock_validate.call_count == 0, ( + "fail-closed gate must reject before _validate_package_exists so " + "no probe reaches the potentially-attacker-controlled github.com URL" + ) + # (2) HTTP-layer defense-in-depth: even if a future refactor moves + # validation elsewhere, no outbound probe should fire on the gated + # path. Patching at ``apm_cli.install.validation.requests.{get,head}`` + # catches any caller that still reaches the validator module. + assert mock_http_get.call_count == 0 + assert mock_http_head.call_count == 0 + # (2) Install rejected. + assert valid_outcomes == [] + assert len(invalid_outcomes) == 1 + rejected_package, reason = invalid_outcomes[0] + assert rejected_package == "shared-tool@my-marketplace" + # (3) Reason names both escape hatches. All host substrings are + # anchored with surrounding quote/backtick characters so CodeQL's + # ``py/incomplete-url-substring-sanitization`` pattern recognizer + # does not flag bare-host membership checks (see tests/**/CLAUDE.md). + assert f"'{_GHE_HOST}'" in reason + # Bare repo is echoed back inside the backtick-delimited `repo:` form. + assert "`repo: platform-team/shared-tool`" in reason + # Concrete qualification value for the enterprise path, anchored. + assert f"'{_GHE_HOST}/platform-team/shared-tool'" in reason + # Concrete qualification value for the cross-host path, anchored. + assert "'github.com/platform-team/shared-tool'" in reason + # Issue reference makes the gate's provenance grep-able. + assert "#1326" in reason + + def test_cross_repo_qualified_to_enterprise_proceeds(self): + """Escape hatch (same-host intent): ``repo: corp.ghe.com/owner/repo``. + + Host-qualifying to the enterprise host means the resolver does not + attach the sentinel (``_needs_canonical_host_prefix`` returns False + when canonical already starts with the host). Install proceeds + through ``_validate_package_exists`` as normal. + """ from apm_cli.commands.install import _resolve_package_references from apm_cli.core.command_logger import InstallLogger @@ -350,7 +394,7 @@ def test_legitimate_cross_host_validation_passes_no_hint(self, capsys): name="shared-tool", source={ "type": "github", - "repo": "platform-team/shared-tool", + "repo": f"{_GHE_HOST}/platform-team/shared-tool", "path": "plugins/shared", }, ) @@ -367,16 +411,64 @@ def test_legitimate_cross_host_validation_passes_no_hint(self, capsys): patch( "apm_cli.commands.install._validate_package_exists", return_value=True, + ) as mock_validate, + ): + result = _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=InstallLogger(verbose=False), + ) + + valid_outcomes, invalid_outcomes = result[0], result[1] + # Gate did NOT fire (qualified form = no sentinel = no refusal). + assert mock_validate.call_count == 1 + assert len(valid_outcomes) == 1 + assert invalid_outcomes == [] + + def test_cross_repo_qualified_to_github_com_proceeds(self): + """Escape hatch (declared cross-host intent): ``repo: github.com/owner/repo``. + + Host-qualifying to ``github.com`` makes the cross-host intent + explicit. Resolver does not attach the sentinel; install proceeds. + This is how operators with legitimate github.com open-source + dependencies on enterprise marketplaces declare intent without + falling into the bare-shorthand ambiguity. + """ + from apm_cli.commands.install import _resolve_package_references + from apm_cli.core.command_logger import InstallLogger + + plugin = MarketplacePlugin( + name="shared-tool", + source={ + "type": "github", + "repo": "github.com/platform-team/shared-tool", + "path": "plugins/shared", + }, + ) + + with ( + patch( + "apm_cli.marketplace.resolver.get_marketplace_by_name", + return_value=_make_source(_GHE_HOST), + ), + patch( + "apm_cli.marketplace.resolver.fetch_or_cache", + return_value=_make_manifest(plugin), ), + patch( + "apm_cli.commands.install._validate_package_exists", + return_value=True, + ) as mock_validate, ): - _resolve_package_references( + result = _resolve_package_references( ["shared-tool@my-marketplace"], [], set(), logger=InstallLogger(verbose=False), ) - emitted = capsys.readouterr().out - # No hint substrings on the successful path. - assert "intentionally a github.com dependency" not in emitted - assert "If you meant the enterprise host" not in emitted + valid_outcomes, invalid_outcomes = result[0], result[1] + assert mock_validate.call_count == 1 + assert len(valid_outcomes) == 1 + assert invalid_outcomes == [] diff --git a/tests/unit/commands/test_install_resolve_refs.py b/tests/unit/commands/test_install_resolve_refs.py index ac6900dad..7489afd52 100644 --- a/tests/unit/commands/test_install_resolve_refs.py +++ b/tests/unit/commands/test_install_resolve_refs.py @@ -283,15 +283,20 @@ def test_generated_entry_round_trips_via_from_apm_yml(self, tmp_path): # --------------------------------------------------------------------------- -# #1305 -- cross-repo bare-on-enterprise hint surfaces on validation failure +# #1326 -- cross-repo bare-on-enterprise fail-closed gate (dependency confusion) # --------------------------------------------------------------------------- -class TestResolvePackageReferencesCrossRepoMisconfigHint: +class TestResolvePackageReferencesCrossRepoFailClosed: """When a marketplace resolution attaches a ``CrossRepoMisconfigRisk`` - sentinel and the package later fails validation, an actionable hint must - be emitted via the logger. The legitimate cross-host path validates - successfully and never reaches the hint branch.""" + sentinel the install command must refuse the package immediately -- + before any outbound validation HTTP probe -- and surface a reason + string naming both qualification escape hatches. This is the + dependency-confusion gate (#1326): the same syntactic ambiguity an + attacker would exploit by pre-staging the bare namespace on public + github.com is rejected at the install boundary rather than silently + resolved against attacker content. + """ @staticmethod def _resolution_with_risk(): @@ -336,75 +341,26 @@ def _resolution_without_risk(): cross_repo_misconfig_risk=None, ) - @patch("apm_cli.commands.install._validate_package_exists", return_value=False) - @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") - @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") - @patch("apm_cli.commands.install.DependencyReference") - def test_hint_emitted_on_validation_failure_with_risk( - self, - mock_dep_cls, - mock_parse_ref, - mock_resolve_mkt, - mock_validate, - ): - """Risk-bearing resolution that fails validation emits the hint.""" - mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) - mock_resolve_mkt.return_value = self._resolution_with_risk() - ref = _make_dep_ref( - "platform-team/shared-tool/plugins/shared", - "github.com/platform-team/shared-tool/plugins/shared", - ) - mock_dep_cls.parse.return_value = ref - mock_dep_cls.is_local_path.return_value = False - _disable_gitlab_direct_probe(mock_dep_cls) - - logger = MagicMock() - logger.verbose = False - - _resolve_package_references( - ["shared-tool@my-marketplace"], - [], - set(), - logger=logger, - ) - - # The hint must be emitted exactly once via ``logger.warning`` - # (PR #1292 panel review's explicit guidance for this follow-up). - # Assertions are anchored to the surrounding prose so a CodeQL - # "incomplete URL substring sanitization" pattern recognizer does - # not flag a bare hostname substring check in this test file. - warn_calls = list(logger.warning.call_args_list) - assert len(warn_calls) == 1 - # ``info`` must NOT be used for the hint (the original PR shipped - # ``logger.info`` and was caught by the 3-persona panel convergence). - assert logger.info.call_args_list == [] - emitted = warn_calls[0].args[0] - # Hint identifies the plugin@marketplace and both intent branches. - assert "'shared-tool@my-marketplace'" in emitted - assert "registered on 'corp.ghe.com'" in emitted - assert "`repo: platform-team/shared-tool`" in emitted - assert "'corp.ghe.com/platform-team/shared-tool'" in emitted - # Second clause acknowledges the legitimate-cross-host path so a - # transient-failure on a real github.com dep is not misdirected. - assert "intentionally a github.com dependency" in emitted - # Stale ``Hint:`` prefix from the original PR must not return; the - # warning symbol carries the advisory signal on its own. - assert "Hint:" not in emitted - @patch("apm_cli.commands.install._validate_package_exists", return_value=True) @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") @patch("apm_cli.commands.install.DependencyReference") - def test_hint_not_emitted_when_validation_passes_even_with_risk( + def test_install_refused_when_risk_present_validate_never_called( self, mock_dep_cls, mock_parse_ref, mock_resolve_mkt, mock_validate, ): - """The legitimate cross-host path (validation succeeds) must NOT - emit the hint -- this is the entire reason the hint lives at the - failure boundary instead of at resolver time.""" + """Risk-bearing resolution rejects the package before validation. + + The mock for ``_validate_package_exists`` is configured to return + True -- modelling the attack precondition (attacker has pre-staged + the bare namespace on public github.com so the validator would + succeed). The gate must fire pre-validation so the mock is never + called: no probe to the attacker URL, no information leak, no + attacker content downloaded. + """ mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) mock_resolve_mkt.return_value = self._resolution_with_risk() ref = _make_dep_ref( @@ -418,28 +374,49 @@ def test_hint_not_emitted_when_validation_passes_even_with_risk( logger = MagicMock() logger.verbose = False - _resolve_package_references( + result = _resolve_package_references( ["shared-tool@my-marketplace"], [], set(), logger=logger, ) - assert logger.warning.call_args_list == [] + # Gate fires pre-validation -- no HTTP probe to attacker URL. + assert mock_validate.call_count == 0 + # Package landed in invalid_outcomes with a refusal reason that + # names both qualification escape hatches. + valid_outcomes, invalid_outcomes = result[0], result[1] + assert valid_outcomes == [] + assert len(invalid_outcomes) == 1 + rejected, reason = invalid_outcomes[0] + assert rejected == "shared-tool@my-marketplace" + # Reason carries enterprise host, both qualified forms, and the + # issue reference for grep-ability. Host substrings are anchored + # with surrounding quote/backtick characters so CodeQL's + # ``py/incomplete-url-substring-sanitization`` does not flag bare + # host-name membership checks (see tests/**/CLAUDE.md). + assert "'corp.ghe.com'" in reason + assert "`repo: platform-team/shared-tool`" in reason + assert "'corp.ghe.com/platform-team/shared-tool'" in reason + assert "'github.com/platform-team/shared-tool'" in reason + assert "#1326" in reason + # ``validation_fail`` is the user-visible surface (not ``warning``); + # the refusal IS the failure, not a hint on top of one. + assert logger.validation_fail.call_count == 1 @patch("apm_cli.commands.install._validate_package_exists", return_value=False) @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") @patch("apm_cli.commands.install.DependencyReference") - def test_no_hint_when_resolution_has_no_risk( + def test_install_proceeds_when_no_risk_attached( self, mock_dep_cls, mock_parse_ref, mock_resolve_mkt, mock_validate, ): - """In-marketplace / non-enterprise resolutions carry no risk - sentinel; validation-fail must NOT print a hint.""" + """In-marketplace / non-enterprise resolutions carry no sentinel; + the gate is dormant and the normal validate path runs.""" mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) mock_resolve_mkt.return_value = self._resolution_without_risk() ref = _make_dep_ref( @@ -460,18 +437,20 @@ def test_no_hint_when_resolution_has_no_risk( logger=logger, ) - assert logger.warning.call_args_list == [] + # No sentinel = gate dormant = validate runs as normal. + assert mock_validate.call_count == 1 @patch("apm_cli.commands.install._validate_package_exists", return_value=False) @patch("apm_cli.commands.install.DependencyReference") - def test_no_hint_for_plain_owner_repo_failure( + def test_no_gate_for_plain_owner_repo( self, mock_dep_cls, mock_validate, ): - """A bare ``owner/repo`` (no marketplace) that fails validation - must NOT trigger the hint -- the risk map is only populated by - marketplace resolutions.""" + """A bare ``owner/repo`` (no marketplace) bypasses marketplace + resolution entirely -- the sentinel is only emitted by + ``resolve_marketplace_plugin``, so the gate cannot fire for direct + deps. Validation proceeds normally even though it then fails.""" ref = _make_dep_ref( "platform-team/shared-tool", "github.com/platform-team/shared-tool", @@ -490,4 +469,5 @@ def test_no_hint_for_plain_owner_repo_failure( logger=logger, ) - assert logger.warning.call_args_list == [] + # Validate is called (gate not engaged for non-marketplace deps). + assert mock_validate.call_count == 1 diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index 59824343f..41fb8b0e6 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -1150,6 +1150,35 @@ def test_cross_repo_host_qualified_no_risk(self, mock_get, mock_fetch, ghe_marke result = resolve_marketplace_plugin("qualified", "my-marketplace") assert result.cross_repo_misconfig_risk is None + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_qualified_to_github_com_no_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """#1326 cross-host explicit qualification: ``repo: github.com/owner/repo`` + on a ``*.ghe.com`` marketplace is declared cross-host intent, NOT a + dependency-confusion ambiguity. The sentinel must not attach + (otherwise the install gate would refuse a legitimate cross-host + dependency the operator explicitly declared). + + The same-host idempotency path in ``_needs_canonical_host_prefix`` + only handles ``repo: corp.ghe.com/owner/repo``; this case is the + symmetric escape hatch for cross-host intent at the resolver layer. + """ + plugin = MarketplacePlugin( + name="cross-host", + source={ + "type": "github", + "repo": "github.com/platform-team/shared-tool", + "path": "plugins/cross-host", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("cross-host", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + @patch("apm_cli.marketplace.resolver.fetch_or_cache") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") def test_cross_repo_url_form_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): From d065b48b6b121777ba666f9ad945ceb0ac37d781 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: Sat, 23 May 2026 14:32:19 +0800 Subject: [PATCH 2/4] fix(install): address review feedback on #1326 fail-closed gate Resolver host extraction (defense in depth) ------------------------------------------- The explicit-host guard in `_compute_cross_repo_misconfig_risk` only inspected the bare shorthand's first slash-split segment, which would misclassify URL (`https://...`, `ssh://...`) and SCP shorthand (`git@host:owner/repo`) `repo:` field values as non-host first segments. Those forms are filtered upstream by `_needs_canonical_host_prefix` (its `":"` in first-segment clause) so they do not reach this guard today, but the guard is brittle by itself: a future upstream refactor that lets them through would cause the sentinel to attach and refuse a legitimate config. Extract the host portion explicitly for URL and SCP forms via `urlparse` and `git@host:...` splitting before the `is_supported_git_host` predicate, so the guard is robust on its own. Style / readability ------------------- Build the refusal reason via a list of lines joined with `"\n"` instead of implicit literal concatenation with embedded newlines, so single-line edits do not need to thread through alignment-space and newline boundaries. Rephrase the new `package-authoring.md` paragraph to describe the `source:` form as a YAML mapping (with nested `type:` and `repo:` keys) rather than inline `{...}` notation, matching the YAML voice of the surrounding examples. Coverage -------- Add `test_compute_returns_none_on_url_or_scp_repo_field_when_filter_bypassed` which calls `_compute_cross_repo_misconfig_risk` directly with a canonical that bypasses the upstream URL/SCP filter so the explicit-host extraction step is exercised in isolation, covering `https://`, `http://`, `ssh://`, and `git@host:...` forms. --- .../skills/apm-usage/package-authoring.md | 11 ++-- src/apm_cli/commands/install.py | 21 ++++---- src/apm_cli/marketplace/resolver.py | 28 +++++++--- .../marketplace/test_marketplace_resolver.py | 52 +++++++++++++++++++ 4 files changed, 92 insertions(+), 20 deletions(-) diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 3f23b15e2..075710a22 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -354,11 +354,12 @@ Schema rules: ### Cross-repo plugin sources on enterprise marketplaces When a marketplace published on a `*.ghe.com` host references a plugin -in a different repo via dict-form `source: {type: github, repo: ...}`, -the `repo:` field **must be host-qualified**. A bare `owner/repo` form -is refused at install time because it cannot be disambiguated from a -public-github.com dependency-confusion attempt (see CHANGELOG entry -for #1326). Two valid forms: +in a different repo via the YAML mapping form of `source:` -- with +nested `type:` and `repo:` keys (rather than the simple `source: owner/repo` +string) -- the `repo:` field **must be host-qualified**. A bare +`owner/repo` value is refused at install time because it cannot be +disambiguated from a public-github.com dependency-confusion attempt +(see CHANGELOG entry for #1326). Two valid forms: ```yaml plugins: diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index b57bfd6f1..ee4c82e09 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -421,17 +421,20 @@ def warning_handler(msg): # they read clearer as separate bullets under a # short lead-in. ``validation_fail`` prepends the # package name; the body below is the remediation. - reason = ( - "refused (dependency-confusion risk #1326): " + # Assemble line-by-line and join with newlines so + # formatting tweaks remain localised per line. + reason_lines = [ + f"refused (dependency-confusion risk #1326): " f"bare `repo: {_risk.bare_repo_field}` on " f"enterprise marketplace '{_risk.marketplace_host}'" - " is ambiguous. Host-qualify the plugin `repo` " - "field in marketplace.json to one of:\n" - f" - '{_risk.suggested_qualified_repo}' " - "(enterprise dep on this marketplace)\n" - f" - 'github.com/{_risk.bare_repo_field}' " - "(declared cross-host dep on public github.com)" - ) + f" is ambiguous. Host-qualify the plugin `repo` " + f"field in marketplace.json to one of:", + f" - '{_risk.suggested_qualified_repo}' " + f"(enterprise dep on this marketplace)", + f" - 'github.com/{_risk.bare_repo_field}' " + f"(declared cross-host dep on public github.com)", + ] + reason = "\n".join(reason_lines) invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index 14bbd9005..5ba4177ac 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -311,12 +311,28 @@ def _compute_cross_repo_misconfig_risk( # an unambiguous declared cross-host dependency). Only the truly-bare # ``owner/repo`` form is the dependency-confusion vector this sentinel # flags. ``_needs_canonical_host_prefix`` above already returns False - # for SAME-host qualification (its idempotency clause); this is the - # symmetric guard for CROSS-host explicit qualification, which the - # idempotency check cannot detect because the canonical starts with a - # different host than ``source.host``. - first_segment = bare.split("/", 1)[0] - if is_supported_git_host(first_segment): + # for SAME-host qualification (its idempotency clause) and for URL / + # SSH SCP shorthand canonicals; this is the symmetric guard for the + # remaining case -- CROSS-host shorthand qualification (``github.com/...`` + # on a ``*.ghe.com`` marketplace), which the idempotency check cannot + # detect because the canonical starts with a different host than + # ``source.host``. + # + # Defense in depth: extract the host from URL and SCP shorthand forms + # too, so the guard is robust even if a future upstream refactor lets + # those forms reach this point. A bare ``split("/", 1)[0]`` would + # otherwise classify ``https://...`` as having a ``https:`` first + # segment (not a host) and incorrectly attach the sentinel. + explicit_host = "" + bare_lower = bare.lower() + if bare_lower.startswith(("https://", "http://", "ssh://")): + explicit_host = (urlparse(bare).hostname or "").strip() + elif bare.startswith("git@") and ":" in bare: + # SCP shorthand: ``git@host:owner/repo`` + explicit_host = bare[4:].split(":", 1)[0].strip() + else: + explicit_host = bare.split("/", 1)[0] + if is_supported_git_host(explicit_host): return None return CrossRepoMisconfigRisk( marketplace_host=source.host, diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index 41fb8b0e6..252fa035f 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -1330,6 +1330,58 @@ def test_cross_repo_source_field_synonym_attaches_risk( result = resolve_marketplace_plugin("src-key", "my-marketplace") assert result.cross_repo_misconfig_risk is not None + def test_compute_returns_none_on_url_or_scp_repo_field_when_filter_bypassed( + self, + ): + """Defense-in-depth: ``_needs_canonical_host_prefix`` already returns + False for URL / SCP shorthand canonicals (its ``":"`` in first-segment + clause), so these forms normally short-circuit before reaching the + explicit-host guard. This direct-call test simulates a future upstream + refactor that lets those forms through and asserts the guard still + recognises them as host-qualified -- a bare ``split("/", 1)[0]`` would + misclassify ``https:`` / ``git@host:owner`` as non-host first segments + and incorrectly attach the sentinel. + + Calls ``_compute_cross_repo_misconfig_risk`` directly with a + canonical that bypasses the upstream guard so we can lock the + behaviour of the explicit-host extraction step alone. + """ + from apm_cli.marketplace.resolver import _compute_cross_repo_misconfig_risk + + source = MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="corp.ghe.com", + branch="main", + ) + + for repo_value in ( + "https://github.com/platform-team/shared-tool", + "http://github.com/platform-team/shared-tool", + "ssh://github.com/platform-team/shared-tool", + "git@github.com:platform-team/shared-tool", + ): + plugin = MarketplacePlugin( + name="cross", + source={ + "type": "github", + "repo": repo_value, + "path": "plugins/cross", + }, + ) + # Hand-build a canonical that would bypass the upstream + # ``_needs_canonical_host_prefix`` URL/SCP short-circuit (this + # shape is not what ``_resolve_github_source`` actually produces + # for these inputs; the test is intentionally probing the + # explicit-host guard in isolation). + canonical = "platform-team/shared-tool/plugins/cross" + risk = _compute_cross_repo_misconfig_risk(plugin, source, canonical, None) + assert risk is None, ( + f"explicit-host guard must recognise {repo_value!r} as " + "host-qualified even when upstream filters do not catch it" + ) + def test_compute_returns_none_on_no_slash_repo_field(self): """Defensive guard inside the helper: ``repo`` without ``/`` is rejected by ``_resolve_github_source`` upstream, but if a future From 6d4fc2ee368a7f4d409edeca147340446847bfb1 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Sat, 23 May 2026 13:32:11 +0100 Subject: [PATCH 3/4] docs+code: address review panel follow-ups for #1326 - Rewrite CHANGELOG entry with before/after YAML and @edenfunf credit - Add enterprise marketplace gate to install CLI reference page - Add YAML example to Starlight pitfall in publish-to-a-marketplace.md - Add code comment on is_supported_git_host design rationale - Replace getattr duck-typing with direct attribute access on typed dataclass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 21 +++++++++++++- .../docs/producer/publish-to-a-marketplace.md | 29 +++++++++++++++---- .../src/content/docs/reference/cli/install.md | 1 + src/apm_cli/commands/install.py | 2 +- src/apm_cli/marketplace/resolver.py | 6 ++++ 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 315143b74..889d3fbb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security -- **BREAKING:** `apm install` against a `*.ghe.com` marketplace now refuses bare cross-repo `repo:` fields in `marketplace.json` before any outbound validation HTTP runs, closing a dependency-confusion vector where an attacker pre-staging the bare namespace on public `github.com` would have the install silently resolve at attacker content. Marketplace authors must host-qualify cross-repo sources: `repo: github.com/owner/repo` for a declared `github.com` open-source dep, or `repo: corp.ghe.com/owner/repo` for an enterprise dep. The earlier `#1305` advisory-hint-on-failure design covered only the failure path; the success path (attacker namespace exists) is now fail-closed at the install boundary. (closes #1326) +- **BREAKING:** `apm install` against a `*.ghe.com` marketplace now refuses bare cross-repo `repo:` fields in `marketplace.json` before any network request runs, closing a dependency-confusion attack vector. Previously a bare `repo: owner/repo` on an enterprise marketplace could silently resolve to an attacker-controlled namespace on public `github.com`; the install now fail-closes with an actionable error. Qualify the `repo:` field to fix: + + ```yaml + # Before (ambiguous -- refused on enterprise marketplaces): + source: + type: git + repo: owner/repo + + # After -- enterprise dep on the same host: + source: + type: git + repo: corp.ghe.com/owner/repo + + # After -- declared cross-host dep on public github.com: + source: + type: git + repo: github.com/owner/repo + ``` + + (closes #1326) -- by @edenfunf (#1459) ### Fixed diff --git a/docs/src/content/docs/producer/publish-to-a-marketplace.md b/docs/src/content/docs/producer/publish-to-a-marketplace.md index 76f4ef76f..5c271c257 100644 --- a/docs/src/content/docs/producer/publish-to-a-marketplace.md +++ b/docs/src/content/docs/producer/publish-to-a-marketplace.md @@ -221,13 +221,30 @@ consumers running `apm install --update` on their own cadence. resolved ref -- the highest tag matching the range at build time. Re-run `apm pack` and re-tag to publish a new version. - **Bare cross-repo `repo:` on enterprise (`*.ghe.com`) marketplaces is - refused at install time.** Dict-form plugin sources that point to a + refused at install time.** Dict-form plugin sources (the `source:` + mapping with nested `type:` and `repo:` keys) that point to a different repo than the marketplace project must host-qualify the - `repo:` field -- `repo: corp.ghe.com/owner/repo` for an enterprise dep - or `repo: github.com/owner/repo` for a declared cross-host dep. A bare - `owner/repo` cannot be disambiguated from a dependency-confusion - attempt where an attacker pre-stages the namespace on public - `github.com`, so the install command fail-closes before validating. + `repo:` field. A bare `owner/repo` cannot be disambiguated from a + dependency-confusion attempt where an attacker pre-stages the namespace + on public `github.com`, so the install command fail-closes before + validating. + + ```yaml + # Refused -- ambiguous bare form on enterprise marketplace: + source: + type: git + repo: owner/repo + + # Accepted -- enterprise dep on the same host: + source: + type: git + repo: corp.ghe.com/owner/repo + + # Accepted -- declared cross-host dep on public github.com: + source: + type: git + repo: github.com/owner/repo + ``` ## Governance diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index 9e47fd940..83791885a 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -95,6 +95,7 @@ Transport env vars: `APM_GIT_PROTOCOL` (`ssh` or `https`) sets the default initi - **Frozen mode.** With `--frozen`, install resolves only what is in `apm.lock.yaml`. A direct dependency missing from the lockfile, or a missing lockfile entirely, exits `1`. Orphan lockfile entries (locked but no longer in `apm.yml`) are tolerated; local-path deps are skipped. This is a structural check, not a content check -- run `apm audit --ci` for hash verification. - **Local `.apm/` deployment.** After dependencies are integrated, primitives in the project's own `.apm/` directory are deployed to the same targets. Local files win on collision. Skipped at `--global` and with `--only mcp`. - **Stale-file cleanup.** Files a still-present package previously deployed but no longer produces are removed from the workspace, gated by per-file content hashes recorded in the lockfile (user-edited files are kept with a warning). +- **Enterprise marketplace gate.** When installing from a `*.ghe.com` marketplace, bare cross-repo `repo:` fields (e.g. `repo: owner/repo`) are refused before any network request runs, preventing dependency-confusion attacks. Host-qualify the field to proceed: `repo: corp.ghe.com/owner/repo` for an enterprise dep, or `repo: github.com/owner/repo` for a declared cross-host dep. - **Security scan.** Source files are scanned for hidden Unicode and other tag-character / bidi-override patterns before deployment. Critical findings block the package; the install exits `1`. Use `--force` to deploy anyway, or run `apm audit --strip` first to remediate. - **Diagnostic summary.** Output is grouped at the end (collisions, replacements, warnings, errors) instead of inline. Use `--verbose` to expand individual file paths. diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index ee4c82e09..83ba188f4 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -414,7 +414,7 @@ def warning_handler(msg): # intent or to github.com for declared cross-host intent). # That prevents the sentinel from attaching at resolver # layer -- no new flag, env var, or schema field needed. - _risk = getattr(resolution, "cross_repo_misconfig_risk", None) + _risk = resolution.cross_repo_misconfig_risk if _risk is not None: # Multi-line reason: the two qualification options # are alternatives, not a single instruction, so diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index 5ba4177ac..24efe3f42 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -332,6 +332,12 @@ def _compute_cross_repo_misconfig_risk( explicit_host = bare[4:].split(":", 1)[0].strip() else: explicit_host = bare.split("/", 1)[0] + # ``is_supported_git_host`` accepts any valid FQDN, not an allowlist. + # This is intentional: the goal is to distinguish "looks like a + # hostname" (explicit intent) from "bare owner/repo" (ambiguous). + # Restricting to known hosts would silently refuse legitimate + # self-hosted Git servers and create a false sense of security -- + # the real protection is the fail-closed refusal of the bare form. if is_supported_git_host(explicit_host): return None return CrossRepoMisconfigRisk( From a1c600044e885c86c722c998856357fee0306964 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Sat, 23 May 2026 15:00:57 +0100 Subject: [PATCH 4/4] fix(install): explicit string formatting for cross-repo refusal message Replace five-way implicit f-string concatenation and two-way bullet concatenations in the #1326 fail-closed refusal reason with: - a named _lead variable (parenthesised three-part concat for line length) - single f-string per bullet inside the "".join([...]) call Each list element now maps 1:1 to one logical clause, so edits to a bullet stay localised to that line. No embedded newlines, no alignment spaces. Addresses Copilot review comment 3292238058. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 38 ++++++++++++++++----------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 83ba188f4..26ee90655 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -416,25 +416,25 @@ def warning_handler(msg): # layer -- no new flag, env var, or schema field needed. _risk = resolution.cross_repo_misconfig_risk if _risk is not None: - # Multi-line reason: the two qualification options - # are alternatives, not a single instruction, so - # they read clearer as separate bullets under a - # short lead-in. ``validation_fail`` prepends the - # package name; the body below is the remediation. - # Assemble line-by-line and join with newlines so - # formatting tweaks remain localised per line. - reason_lines = [ - f"refused (dependency-confusion risk #1326): " - f"bare `repo: {_risk.bare_repo_field}` on " - f"enterprise marketplace '{_risk.marketplace_host}'" - f" is ambiguous. Host-qualify the plugin `repo` " - f"field in marketplace.json to one of:", - f" - '{_risk.suggested_qualified_repo}' " - f"(enterprise dep on this marketplace)", - f" - 'github.com/{_risk.bare_repo_field}' " - f"(declared cross-host dep on public github.com)", - ] - reason = "\n".join(reason_lines) + # Two explicit-host options are alternatives, not a + # sequence, so they read clearer as separate bullets. + # ``validation_fail`` prepends the package name; the + # body below is the remediation. Each list element is + # one logical clause so individual edits stay local. + _lead = ( + f"refused (dependency-confusion risk #1326): bare" + f" `repo: {_risk.bare_repo_field}` on enterprise" + f" marketplace '{_risk.marketplace_host}' is ambiguous." + f" Host-qualify the plugin `repo` field in" + f" marketplace.json to one of:" + ) + reason = "\n".join( + [ + _lead, + f" - '{_risk.suggested_qualified_repo}' (enterprise dep on this marketplace)", + f" - 'github.com/{_risk.bare_repo_field}' (declared cross-host dep on public github.com)", + ] + ) invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason)