fix(install): block bare cross-repo on enterprise marketplaces (#1326)

[edenfunf]

Description

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 earlier #1305 fix surfaced an advisory hint only on the validation FAILURE path; the SUCCESS path (an attacker pre-stages the bare namespace on public github.com) reached no signal and the install fetched attacker content under enterprise auth.

The fix moves the check to the install command's pre-validation boundary: when the resolver attaches CrossRepoMisconfigRisk, the package is rejected immediately, before _validate_package_exists, requests.get, or requests.head can fire. No probe reaches the potentially-attacker-controlled URL.

The escape hatch is the existing host-qualification syntax. The resolver now recognises an explicitly-qualified repo: field -- repo: github.com/owner/repo for declared cross-host intent, or repo: corp.ghe.com/owner/repo for an enterprise dep -- and does not attach the sentinel, so legitimate cross-host installs proceed. No new CLI flag, env var, or schema field is introduced.

Fixes #1326

Type of change

• Bug fix (security)
• BREAKING change

Breaking semantics: marketplaces that relied on bare cross-repo repo: working today must host-qualify the field in marketplace.json. The refusal message names both qualification options inline so operators see the migration path on the next apm install.

Changes

1. src/apm_cli/commands/install.py -- added pre-validation fail-closed gate immediately after resolve_marketplace_plugin returns. Removed the now-dead _misconfig_risks dict and the failure-path hint block (the gate fires first; the hint block was unreachable for cross-repo cases).
2. src/apm_cli/marketplace/resolver.py -- _compute_cross_repo_misconfig_risk now also returns None when the raw repo: field is explicitly host-qualified to any supported git host. The existing same-host idempotency check via _needs_canonical_host_prefix only covered qualify-to-marketplace-host; this is the symmetric guard for cross-host declared intent. Resolver routing is otherwise unchanged.
3. CHANGELOG.md -- Unreleased / Security entry documenting the breaking change and the migration path.
4. packages/apm-guide/.apm/skills/apm-usage/package-authoring.md -- new subsection covering cross-repo plugin sources on enterprise marketplaces.
5. docs/src/content/docs/producer/publish-to-a-marketplace.md -- added pitfall bullet.

Testing

• Tested locally
• All existing tests pass (125 tests in the change area: integration + unit/commands + unit/marketplace)
• Added regression tests for new functionality

Regression coverage:

• test_ghe_marketplace_install_e2e.py::TestCrossRepoFailClosedIntegration
  • test_cross_repo_bare_blocks_install_before_validation -- the attack PoC. Mocks _validate_package_exists to return True (modelling the attacker namespace existing on github.com) and asserts call_count == 0 on _validate_package_exists AND on requests.get / requests.head (HTTP-layer defense-in-depth catches a future refactor that moves validation elsewhere).
  • test_cross_repo_qualified_to_enterprise_proceeds -- escape hatch for same-host intent (repo: corp.ghe.com/owner/repo).
  • test_cross_repo_qualified_to_github_com_proceeds -- escape hatch for declared cross-host intent (repo: github.com/owner/repo).
• test_install_resolve_refs.py::TestResolvePackageReferencesCrossRepoFailClosed -- 3 unit tests covering refusal on risk attached, proceed when no risk, and no-marketplace bypass.
• test_marketplace_resolver.py::TestCrossRepoMisconfigRisk::test_cross_repo_qualified_to_github_com_no_risk -- locks the new resolver-layer cross-host recognition.

Toggle-verified: the attack PoC test fails without the fix (silent success path) and passes with the fix.

Local lint mirror:

• ruff check src/ tests/ -- clean
• ruff format --check src/ tests/ -- clean
• scripts/lint-auth-signals.sh -- clean
• File length: install.py 1989 / resolver.py 786 (cap 2450)

Migration

Marketplace authors with a dict-form plugin on a *.ghe.com marketplace must update marketplace.json:

# Before -- ambiguous, refused after this fix
- name: shared-tool
  source:
    type: github
    repo: platform-team/shared-tool
    path: plugins/shared

# After -- enterprise dep (most common)
- name: shared-tool
  source:
    type: github
    repo: corp.ghe.com/platform-team/shared-tool
    path: plugins/shared

# After -- declared cross-host dep
- name: shared-tool
  source:
    type: github
    repo: github.com/platform-team/shared-tool
    path: plugins/shared

The refusal message names both options at install time so consumers can either fix the manifest themselves (if they control it) or report the qualification value to the marketplace owner.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Implements a fail-closed install gate to prevent dependency-confusion when resolving cross-repo GitHub plugin sources from *.ghe.com marketplaces, and updates tests/docs to reflect the new security posture.

Changes:

• Add a pre-validation refusal in apm install when a marketplace resolution carries CrossRepoMisconfigRisk (#1326).
• Update resolver logic and tests to treat explicit cross-host qualification (e.g., repo: github.com/owner/repo) as non-ambiguous (no sentinel).
• Update integration/unit tests plus docs/CHANGELOG to reflect the new breaking security behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/apm_cli/commands/install.py	Adds the pre-validation fail-closed gate that refuses ambiguous cross-repo marketplace installs.
src/apm_cli/marketplace/resolver.py	Prevents sentinel attachment when repo: is explicitly host-qualified via a supported host prefix.
tests/unit/commands/test_install_resolve_refs.py	Updates unit expectations to assert refusal occurs before validation and no HTTP probing occurs.
tests/unit/marketplace/test_marketplace_resolver.py	Adds a unit test ensuring repo: github.com/... on GHE marketplace is treated as explicit cross-host intent.
tests/integration/test_ghe_marketplace_install_e2e.py	Reworks E2E coverage to assert fail-closed behavior and both escape hatches.
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md	Documents the new requirement to host-qualify cross-repo repo: on enterprise marketplaces.
docs/src/content/docs/producer/publish-to-a-marketplace.md	Adds a producer-facing note about install-time refusal for bare cross-repo repo: on GHE.
CHANGELOG.md	Records the breaking security change and remediation options (host-qualified repo:).

[sergio-sisternes-epam]

APM Review Panel: ship_with_followups

Community-contributed fail-closed gate blocks enterprise dependency-confusion; 7/7 tests pass, zero blocking findings across 8 panelists. Ship with doc follow-ups.

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

Eight active panelists converge on a clear signal: this is a well-executed, security-critical fix from a community contributor that closes a real dependency-confusion attack vector on enterprise marketplaces. The sentinel-pattern architecture is sound (python-architect), the fail-closed semantics are correct (supply-chain-security, auth-expert), the breaking-change UX is actionable with copy-paste escape hatches (devx-ux), and the test coverage is comprehensive with 7 evidence-backed passing tests across unit and integration tiers (test-coverage-expert). No panelist raised a blocking finding. All findings are nits or recommended doc improvements.

There is no dissent to arbitrate. The only substantive gap is documentation: the install CLI reference page does not yet document the new gate (doc-writer, recommended), and the CHANGELOG entry is maintainer-dense rather than user-facing (oss-growth-hacker, devx-ux). These are real but non-blocking -- the error message itself is self-documenting with actionable fix instructions, so users who hit the gate are not stranded.

Strategically, this PR is exactly the kind of contribution APM needs to be seen accepting and shipping quickly: an external contributor closing a real enterprise security hole with a clean breaking change, proper CHANGELOG entry, and comprehensive tests. The speed of merge signals that APM takes enterprise supply-chain security seriously and that community contributions land fast. The CHANGELOG rewrite and doc update should land same-day or next-day as fast-follows, not as merge blockers.

Aligned with: Secure by default -- fail-closed gate refuses ambiguous bare cross-repo references before any HTTP probe runs. Governed by policy -- enterprise marketplace operators get deterministic refusal of ambiguous sources. Multi-harness/multi-host -- the escape hatch preserves multi-host flexibility. OSS community-driven -- community-contributed security fix from @edenfunf closing issue #1326.

Growth signal. This PR is a strong enterprise trust signal: a community contributor found and fixed a real supply-chain attack vector, and the project shipped it with a clean breaking change, actionable migration path, and comprehensive tests. The CHANGELOG entry should be rewritten to lead with the user-facing 'before/after' YAML and the security rationale, not the internal implementation details -- that rewrite turns a routine version bump into a release narrative that enterprise evaluators will notice. Credit @edenfunf in the CHANGELOG to reinforce the contributor flywheel.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Clean sentinel-pattern security fix: fail-closed gate at install boundary before HTTP probes. Architecture is sound.
CLI Logging Expert	0	0	2	Level upgrade from warning to validation_fail is correct for the fail-closed gate; minor alignment nit.
DevX UX Expert	0	0	4	Strong breaking-change UX: actionable error with two copy-paste escape hatches, no silent behavior change.
Supply Chain Security Expert	0	0	2	Fail-closed gate correctly blocks dependency-confusion attack on success path; host-extraction is defense-in-depth.
OSS Growth Hacker	0	1	2	Community security fix is a strong enterprise trust signal; CHANGELOG needs user-facing rewrite for narrative leverage.
Auth Expert	0	0	1	Fail-closed gate correctly isolates enterprise auth from github.com default; no credential leak path remains.
Doc Writer	0	2	3	Doc surfaces are accurate and well-scoped; install CLI reference is the main gap.
Test Coverage Expert	0	0	1	Security fix has comprehensive test coverage at both unit and integration tiers; all 8 new tests pass.

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] Rewrite CHANGELOG entry with before/after YAML, user-facing security rationale, and @edenfunf credit -- Current entry is maintainer-dense; a user-facing rewrite turns the release into an enterprise trust narrative and reinforces the contributor flywheel
2. [Doc Writer] Add enterprise marketplace gate to install CLI reference page (docs/src/content/docs/reference/cli/install.md) -- Users consulting CLI reference docs won't discover the new validation behavior; this is the primary doc surface gap
3. [Doc Writer] Add minimal YAML example to Starlight pitfall in publish-to-a-marketplace.md showing qualified vs bare repo form -- Pitfall text without a concrete example forces users to guess the correct syntax
4. [Supply Chain Security Expert] Add code comment on is_supported_git_host explaining why any valid FQDN is accepted (defense-in-depth, not allowlist) -- Future reviewers will question why evil.com passes host validation; a one-line comment prevents a misguided tightening PR
5. [Python Architect] Replace getattr duck-typing with direct attribute access on typed dataclass (install.py:417) -- Minor code hygiene; direct access is clearer on a typed dataclass and lets static analysis catch renames

Architecture

classDiagram
    direction LR
    class MarketplacePluginResolution {
        <<ValueObject>>
        +canonical str
        +plugin MarketplacePlugin
        +dependency_reference DependencyReference | None
        +cross_repo_misconfig_risk CrossRepoMisconfigRisk | None
    }
    class CrossRepoMisconfigRisk {
        <<Sentinel ValueObject>>
        +marketplace_host str
        +bare_repo_field str
        +suggested_qualified_repo str
    }
    class MarketplacePlugin {
        +name str
        +source dict | str
    }
    class MarketplaceSource {
        +host str
        +owner str
        +repo str
    }
    class _resolve_package_references {
        <<IOBoundary>>
        +install_gate(resolution)
    }
    class _compute_cross_repo_misconfig_risk {
        <<Pure>>
        +returns CrossRepoMisconfigRisk | None
    }
    class is_supported_git_host {
        <<Pure>>
        +returns bool
    }
    MarketplacePluginResolution *-- CrossRepoMisconfigRisk : optional sentinel
    MarketplacePluginResolution *-- MarketplacePlugin : carries
    _compute_cross_repo_misconfig_risk ..> MarketplacePlugin : reads source dict
    _compute_cross_repo_misconfig_risk ..> MarketplaceSource : reads host
    _compute_cross_repo_misconfig_risk ..> CrossRepoMisconfigRisk : creates
    _compute_cross_repo_misconfig_risk ..> is_supported_git_host : explicit-host guard
    _resolve_package_references ..> MarketplacePluginResolution : consumes
    note for CrossRepoMisconfigRisk "Sentinel pattern: attached at resolver,\nconsumed fail-closed at install gate"
    note for _compute_cross_repo_misconfig_risk "#1326: new explicit-host guard\nskips sentinel when repo is host-qualified"
    class _compute_cross_repo_misconfig_risk:::touched
    class _resolve_package_references:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install plugin@marketplace"] --> B["resolve_marketplace_plugin()\nmarketplace/resolver.py"]
    B --> C["_resolve_github_source()"]
    C --> D["_compute_cross_repo_misconfig_risk()"]
    D --> E{"repo: field\nexplicitly host-qualified?"}
    E -->|"URL/SCP/shorthand with\nis_supported_git_host()"| F["return None\n(no sentinel)"]
    E -->|"bare owner/repo on\n*.ghe.com marketplace"| G["return CrossRepoMisconfigRisk\n(sentinel attached)"]
    F --> H["MarketplacePluginResolution\n.cross_repo_misconfig_risk = None"]
    G --> I["MarketplacePluginResolution\n.cross_repo_misconfig_risk = risk"]
    H --> J["_resolve_package_references()\ninstall.py: #1326 gate"]
    I --> J
    J --> K{"risk is not None?"}
    K -->|Yes| L["invalid_outcomes.append()\nlogger.validation_fail()\ncontinue -- NO HTTP"]
    K -->|No| M["[NET] _validate_package_exists()\noutbound HTTP probe"]
    M --> N["install proceeds"]
    style D fill:#fff3b0,stroke:#d47600
    style J fill:#fff3b0,stroke:#d47600
    style L fill:#ffcccc,stroke:#cc0000

Loading

Recommendation

Merge now. Zero blocking findings across 8 panelists, 7/7 tests pass with evidence, and the security fix is time-sensitive -- every day the ambiguous bare-repo form resolves silently is a day the attack vector is open. The CHANGELOG rewrite and doc updates should land as fast-follow commits (same day or next day), not as merge blockers. Credit @edenfunf in the merge commit or CHANGELOG.

---

Full per-persona findings

Python Architect

• [nit] getattr duck-typing on a typed dataclass attribute at src/apm_cli/commands/install.py:417
  Line 417 uses getattr(resolution, "cross_repo_misconfig_risk", None) but resolution is already a MarketplacePluginResolution dataclass with a typed cross_repo_misconfig_risk attribute (default None). Direct attribute access is clearer and type-checker-friendly.
  Suggested: Replace with resolution.cross_repo_misconfig_risk.
• [nit] Verify stale docstring on CrossRepoMisconfigRisk at src/apm_cli/marketplace/resolver.py:51
  The docstring should reflect fail-closed semantics, not advisory-hint semantics. Verify the diff was applied cleanly.

CLI Logging Expert

• [nit] Multi-line reason continuation lines misalign with [x] prefix at src/apm_cli/commands/install.py:430
  validation_fail prepends '[x] ' (4 chars) to the first line. The two ' - ' bullet lines start at column 0. Padding each continuation line with 4 leading spaces would align them.
  Suggested: Change ' - ' to ' - ' (6 chars = 4 for [x]-prefix + 2 for bullet indent).
• [nit] Issue number [BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326 in user-facing error text is opaque to external operators at src/apm_cli/commands/install.py:423
  Consider dropping '[BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326' from the user-facing string and keeping it only in code comments or verbose output.

DevX UX Expert

• [nit] Error lead-in says 'to one of:' but bullets are alternatives at src/apm_cli/commands/install.py:430
  Consider rewording to 'choose one of:' or 'pick the form that matches your intent:'. Minor -- the parenthetical annotations already disambiguate.
• [nit] Issue number [BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326 in user-facing error is maintainer jargon at src/apm_cli/commands/install.py:428
  Consider moving the issue reference to verbose/debug path.
• [nit] CHANGELOG entry is a single dense paragraph at CHANGELOG.md:10
  Enterprise operators scanning a changelog for 'what broke' benefit from a shorter lead sentence followed by a sub-bullet with the migration path.
• [nit] Docs mention refusal but don't show the error the user will see at docs/src/content/docs/producer/publish-to-a-marketplace.md:223
  Adding a 'You will see an error like: ...' block would close the loop for operators.

Supply Chain Security Expert

• [nit] FQDN-shaped owner names bypass sentinel (theoretical, not exploitable on GitHub) at src/apm_cli/marketplace/resolver.py:310-320
  If a repo: field were my.org/repo, the first segment passes FQDN check. Not exploitable on GitHub (org names cannot contain dots), but a code comment should document this assumption.
• [nit] ssh:// URL form hostname extraction includes port at src/apm_cli/marketplace/resolver.py:314-316
  Handled safely by the fallback. Consider adding a test case for ssh://host:22/owner/repo.

OSS Growth Hacker

• [recommended] CHANGELOG entry is maintainer-dense at CHANGELOG.md:10
  Needs user-facing rewrite with before/after YAML. This is the repostable artifact enterprise operators paste into team Slack.
• [nit] Community contributor credit (@edenfunf) missing from CHANGELOG entry at CHANGELOG.md:10
  A '(contributed by @edenfunf)' trailer is the cheapest retention lever in OSS.
• [nit] Starlight doc pitfall should mirror YAML examples from skill doc at docs/src/content/docs/producer/publish-to-a-marketplace.md:223

Auth Expert

• [nit] is_supported_git_host accepts any valid FQDN at src/apm_cli/marketplace/resolver.py
  Code comment explaining this design choice would help future reviewers understand why the gate does not also validate that the explicit host is reachable or credential-bearing.

Doc Writer

• [recommended] Install CLI reference does not document the new enterprise marketplace gate at docs/src/content/docs/reference/cli/install.md:98
  The Behavior section lists security-relevant install behaviors but omits the new fail-closed gate.
• [recommended] Pitfall lacks a minimal YAML example or cross-link at docs/src/content/docs/producer/publish-to-a-marketplace.md:224
  The skill doc has a clear two-form YAML snippet; the Starlight pitfall should mirror it.
• [nit] package-authoring.md CHANGELOG reference fragile after release cut at packages/apm-guide/.apm/skills/apm-usage/package-authoring.md:355
  Consider linking to the issue itself for permalink stability.
• [nit] CHANGELOG migration path lists qualification forms in reversed priority order at CHANGELOG.md:12
  Enterprise-first order would match skill doc and Starlight pitfall.
• [nit] Pitfall mentions 'Dict-form plugin sources' without context at docs/src/content/docs/producer/publish-to-a-marketplace.md:227
  A parenthetical '(the source: mapping with nested type: and repo: keys)' would remove ambiguity.

Test Coverage Expert

• [nit] All critical surfaces are covered -- no actionable gap found
  7 evidence-backed passing tests: attack PoC (integration), enterprise escape hatch (integration), github.com escape hatch (integration), gate-fires unit, gate-dormant unit, cross-host qualifier unit, URL/SCP defense-in-depth unit.

Performance Expert -- inactive

No performance-critical paths touched.

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

[sergio-sisternes-epam]

Completion: follow-up items resolved

All 5 recommended follow-ups from the review panel have been folded into the PR branch (7d4d92a5). No blocking items were identified.

Folded items

#	ID	Summary	Files
1	changelog-rewrite	Rewrote CHANGELOG entry with before/after YAML, security rationale, and @edenfunf credit	CHANGELOG.md
2	install-cli-ref-doc	Added enterprise marketplace gate to install CLI reference Behavior section	docs/src/content/docs/reference/cli/install.md
3	starlight-yaml-example	Added qualified vs bare YAML example to Starlight pitfall	docs/src/content/docs/producer/publish-to-a-marketplace.md
4	fqdn-design-comment	Added code comment on is_supported_git_host explaining FQDN-acceptance design	src/apm_cli/marketplace/resolver.py
5	getattr-direct-access	Replaced getattr duck-typing with direct attribute access on typed dataclass	src/apm_cli/commands/install.py

Deferred items

None.

Validation

• Lint: ruff check and ruff format --check both silent (exit 0)
• CI: all checks passed
• Pushed to contributor fork (edenfunf/apm) on branch fix/1326-cross-repo-fail-closed

[github-actions]

APM Review Panel: needs_rework

Two must-fix defects before re-review: type: git doc error misleads marketplace authors into a schema error instead of the dep-confusion refusal, and enterprise/security.md not updated per project security-boundary rule.

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

The core security fix is architecturally correct and closes a real, previously exploitable dependency-confusion path. The pre-validation sentinel placement is the only safe location; three integration tests plus three unit tests give adequate automated proof at the critical surfaces. This PR should ship -- but two defects must be corrected first.

The doc-writer blocking finding is load-bearing: every YAML example in CHANGELOG.md and publish-to-a-marketplace.md uses type: git, which the implementation rejects with a ValueError before the new gate is even reached. A marketplace author copying the migration example verbatim gets a schema error, not the actionable dep-confusion refusal. This inverts the UX promise of the escape-hatch guidance and will generate avoidable support noise on the day of release. package-authoring.md uses type: github correctly, confirming this is a copy-paste error, not a design ambiguity. Fix is mechanical: replace type: git with type: github in all three YAML blocks in CHANGELOG.md and the parallel blocks in publish-to-a-marketplace.md.

The supply-chain-security finding that enterprise/security.md was not updated is also a must-fix under project rules: security boundary changes must update that doc in the same PR. An auditor reading the current doc has no way to discover that bare cross-repo refs on *.ghe.com marketplaces are now refused before any outbound probe. This is a one-paragraph addition, not a rework, but it is required.

The FQDN-lookalike bypass (dotted owner like platform.team passing is_valid_fqdn) is a real edge case. However, GitHub cloud forbids dots in usernames, substantially reducing practical exploit surface. The auth-expert confirms that marketplace.json is authored by a trusted operator in the attacker model this PR targets, which further limits the window. This is a recommended follow-up (open a tracking issue, add suffix-list guard in a subsequent PR) rather than a must-fix for this PR.

The test-coverage-expert's missing integration-with-fixtures test for URL/SCP escape-hatch forms carries evidence outcome: missing on a secure-by-default surface. The unit test proves the logic but does not prove the escape hatch at the full pipeline tier. This should be the first follow-up merged after this PR.

Dissent. No panelist disagreement on severity for the two must-fix items. The supply-chain-security expert flagged the FQDN-lookalike bypass as recommended; auth-expert confirmed the marketplace.json trust boundary, upholding recommended (not blocking) given GitHub cloud's dots-in-username prohibition. The test-coverage-expert returned outcome: unknown on the three integration tests due to environment constraints -- those tests are unverified-by-panel but not failed; the unit test for URL/SCP forms returned outcome: passed, confirming sentinel logic is correct at that tier.

Aligned with: Secure by default (core attack vector closed, escape hatch sound), Portable by manifest (pending YAML type: git -> type: github fix), Governed by policy (pending security.md update), Pragmatic as npm (clear escape-hatch guidance, error ergonomics follow-up queued)

Growth signal. APM is now the only agent package manager with fail-closed dependency-confusion protection for GHE marketplaces -- a credible, specific, and differentiating claim. The release post hook ('APM closes the dependency-confusion vector on enterprise marketplaces: bare repo: fields are refused before any network request touches public github.com') should lead the release announcement. Reframe the CHANGELOG entry to lead with the security benefit before the BREAKING annotation -- this converts migration friction into a security guarantee narrative. @edenfunf credit is present and correct; amplify in the release post.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Architecturally sound; sentinel placement at pre-validation is correct; IPv6 bare-URL false positive is the one edge case worth fixing.
CLI Logging Expert	0	1	1	validation_fail is the right call; first reason line wraps badly on narrow terminals; continuation bullets lack symbol prefix.
DevX UX Expert	0	2	2	Refusal error is well-constructed with actionable escape hatches; drops internal issue reference (#1326) in operator-facing text; omits same-repo scope clarifier.
Supply Chain Security Expert	0	2	2	Core attack closed; FQDN-lookalike bypass and missing security.md update are the residual gaps.
OSS Growth Hacker	0	1	2	Strong security story; CHANGELOG entry should lead with security win before BREAKING cost; @edenfunf attribution is correct.
Auth Expert	0	0	2	Auth routing is correct for both escape hatch forms; sentinel fires before any AuthResolver call; FQDN trust assumption should be documented.
Doc Writer	1	2	1	Blocking: type: git in YAML examples is not a valid marketplace source type; implementation requires type: github. Consumer-side callout missing; Python jargon in pitfall.
Test Coverage Expert	0	2	1	All six critical surfaces have automated tests; URL/SCP guard sits below integration-with-fixtures floor (unit-only); one missing tier.

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

Top 5 follow-ups

1. [Doc Writer] (blocking-severity) Replace type: git with type: github in all YAML examples in CHANGELOG.md and publish-to-a-marketplace.md -- Factual error: type: git triggers a ValueError before the new gate is reached; marketplace authors copying the migration example get a schema error, not the dep-confusion refusal.
2. [Supply Chain Security Expert] (blocking-severity) Add one paragraph to enterprise/security.md documenting the pre-validation dep-confusion gate for *.ghe.com marketplaces -- Project rule: security boundary changes must update enterprise/security.md in the same PR; one paragraph, no design rework required.
3. [Test Coverage Expert] Add integration-with-fixtures test for URL/SCP escape-hatch forms driving full _resolve_package_references() pipeline -- Evidence outcome: missing on a secure-by-default surface; mirror test_cross_repo_qualified_to_enterprise_proceeds with repo: 'https://github.com/platform-team/shared-tool'.
4. [Supply Chain Security Expert] Harden FQDN-lookalike bypass: restrict bare-form escape hatch to known git host suffix patterns or require scheme presence -- repo: platform.team/shared-tool passes is_valid_fqdn and suppresses the sentinel; GitHub cloud forbids dots in usernames (low practical risk today), but the heuristic is fragile.
5. [DevX UX Expert] Replace [BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326 internal issue reference in error message with a stable public docs URL; add in-marketplace scope clarifier -- External enterprise operators cannot resolve [BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326; omitting the in-marketplace scope clarifier causes unnecessary audit anxiety for large deployments.

Architecture

classDiagram
    direction LR
    class MarketplacePluginResolution {
        <<ValueObject>>
        +canonical str
        +plugin MarketplacePlugin
        +dependency_reference DependencyReference | None
        +cross_repo_misconfig_risk CrossRepoMisconfigRisk | None
        +__iter__() Iterator
    }
    class CrossRepoMisconfigRisk {
        <<ValueObject>>
        +marketplace_host str
        +bare_repo_field str
        +suggested_qualified_repo str
    }
    class _compute_cross_repo_misconfig_risk {
        <<Pure>>
        +plugin MarketplacePlugin
        +canonical str
        +source MarketplaceSource
        +returns CrossRepoMisconfigRisk | None
    }
    class _resolve_package_references {
        <<IOBoundary>>
        +packages list
        +auth_resolver AuthResolver
        +logger CommandLogger | None
        +returns tuple
    }
    class is_supported_git_host {
        <<Pure>>
        +hostname str | None
        +returns bool
    }
    class MarketplaceSource {
        <<ValueObject>>
        +host str
        +url str
    }
    _resolve_package_references ..> MarketplacePluginResolution : unpacks
    _resolve_package_references ..> CrossRepoMisconfigRisk : guards on (pre-validation)
    MarketplacePluginResolution *-- CrossRepoMisconfigRisk : sentinel field
    _compute_cross_repo_misconfig_risk ..> CrossRepoMisconfigRisk : returns
    _compute_cross_repo_misconfig_risk ..> is_supported_git_host : host check
    _compute_cross_repo_misconfig_risk ..> MarketplaceSource : reads host
    note for CrossRepoMisconfigRisk "Frozen sentinel: resolver attaches,\ninstall command consumes pre-validation (#1326)"
    note for _resolve_package_references "Fail-closed gate: if risk != None then invalid_outcomes, continue\nNo outbound HTTP probe issued"
    class CrossRepoMisconfigRisk:::touched
    class MarketplacePluginResolution:::touched
    class _compute_cross_repo_misconfig_risk:::touched
    class _resolve_package_references:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install pkg"])
    B["_resolve_package_references()\ninstall.py:352"]
    C["resolve_marketplace_plugin()\nresolver.py:647"]
    D["_compute_cross_repo_misconfig_risk()\nresolver.py:267"]
    E{"bare.startswith https/http/ssh?"}
    F["urlparse().hostname"]
    G{"bare.startswith git@?"}
    H["SCP split: bare[4:].split(':',1)[0]"]
    I["else: bare.split('/',1)[0]"]
    J["is_supported_git_host(explicit_host)"]
    K["return None (no sentinel)"]
    L["return CrossRepoMisconfigRisk"]
    M["MarketplacePluginResolution returned"]
    N{"resolution.cross_repo_misconfig_risk\nis not None?"}
    O["invalid_outcomes.append()\nlogger.validation_fail()\ncontinue\n(NO outbound probe)"]
    P["_validate_package_exists() - HTTP probe"]
    Q["valid / invalid outcomes"]
    A --> B
    B --> C
    C --> D
    D --> E
    E -- yes --> F
    E -- no --> G
    G -- yes --> H
    G -- no --> I
    F --> J
    H --> J
    I --> J
    J -- True --> K
    J -- False --> L
    K --> M
    L --> M
    M --> N
    N -- "yes: FAIL-CLOSED" --> O
    N -- no --> P
    P --> Q
    O --> Q
    style O fill:#ffdddd,stroke:#cc0000
    style N fill:#fff3b0,stroke:#d47600
    style P fill:#ddeeff,stroke:#0066cc

Loading

Recommendation

Two items need to land before requesting re-review: (1) replace type: git with type: github in all YAML examples in CHANGELOG.md and publish-to-a-marketplace.md -- the current examples actively mislead marketplace authors into a schema error rather than the actionable migration path; (2) add the security.md paragraph documenting the pre-validation gate -- required by project rule for all security-boundary changes. Both fixes are non-design, non-invasive, and should take under 30 minutes. Once those two items land, this PR ships. The core security fix is correct, well-tested, and closes a real attack vector. Queue the integration-with-fixtures test for URL/SCP escape-hatch forms and the FQDN-lookalike hardening as the first two follow-on issues.

---

Full per-persona findings

Python Architect

• [recommended] IPv6 bare-URL host extraction produces a false-positive sentinel attach at src/apm_cli/marketplace/resolver.py:328
urlparse('https://[::1]/owner/repo').hostname returns '::1' (brackets stripped). is_valid_fqdn('::1') returns False, so is_supported_git_host returns False, and the sentinel attaches -- blocking a legitimately host-qualified repo. IPv6 self-hosted Git exists in air-gapped enterprise environments.
  Suggested: Add import ipaddress; try: ipaddress.ip_address(explicit_host); return None; except ValueError: pass in the URL branch before is_supported_git_host.
• [nit] getattr(resolution, 'dependency_reference', None) is inconsistent with the new direct-attribute pattern at src/apm_cli/commands/install.py:450
  The PR correctly replaces getattr for cross_repo_misconfig_risk but leaves getattr for dependency_reference on the sibling line. Both are declared with = None defaults on the typed frozen dataclass.
  Suggested: marketplace_dep_ref = resolution.dependency_reference
• [nit] Design pattern note: frozen dataclass sentinel (Value Object) + Guard Clause is the correct pattern for this cross-boundary signal; no structural issues.

CLI Logging Expert

• [recommended] First reason line is ~170+ chars -- wraps badly on narrow terminals at src/apm_cli/commands/install.py
  The four f-strings concatenate into a single line of ~170 characters before variable substitution (longer with real hostnames). Split the lead-in at the period: end line 1 after 'is ambiguous.' and begin a new line 'Host-qualify the plugin repo field in marketplace.json to one of:'.
• [nit] Bullet continuation lines share a single _rich_echo call -- no symbol prefix on lines 2-3 at src/apm_cli/commands/install.py
  Rich renders embedded newlines as real newlines; the two bullet lines appear as raw red text with no [x] prefix and no indentation to the symbol column. Prefix each bullet with four spaces instead of two so they indent past the [x] leader.

DevX UX Expert

• [recommended] Error message embeds internal issue tracker ID ([BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326) -- use a docs URL instead at src/apm_cli/commands/install.py
  External enterprise operators cannot resolve [BUG] Cross-repo bare repo on *.ghe.com marketplace silently resolves at github.com on validation success (dependency-confusion vector) #1326. npm, pip, and cargo all direct users to a documentation URL, not an internal tracker reference. Replace with a stable anchor in cli/install.md (e.g. `(aka.ms/redacted) so operators can read the full context.
• [recommended] Error message does not clarify that in-marketplace (same-repo) plugins are unaffected at src/apm_cli/commands/install.py
  When the gate fires for one plugin, operators fear all plugins need auditing. Add: 'In-marketplace plugins (source: ./... or same-repo sources) are not affected by this check.'
• [nit] No proactive discovery path -- consider noting that operators can grep marketplace.json for bare repo: dict-source fields before a CI run at docs/src/content/docs/producer/publish-to-a-marketplace.md
• [nit] Error lead-in format refused (dependency-confusion risk ...): deviates from conventional CLI error phrasing (npm/cargo/pip all lead with the tool prefix, then the message) at src/apm_cli/commands/install.py

Supply Chain Security Expert

• [recommended] Dotted owner-segment bypasses escape-hatch FQDN check at src/apm_cli/marketplace/resolver.py
repo: platform.team/shared-tool produces explicit_host = 'platform.team', which passes is_valid_fqdn (two dot-separated labels) and is_supported_git_host, suppressing the sentinel. GitHub cloud forbids dots in usernames (low practical risk), but the FQDN heuristic is fragile. Restrict bare-form escape hatch to first segments matching known git host suffix patterns (.com, .org, .io, .net, *.ghe.com) or require scheme presence.
• [recommended] enterprise/security.md not updated to document new pre-validation gate at docs/src/content/docs/enterprise/security.md
  Per project rule, security boundary changes must update enterprise/security.md in the same PR. The canonical security model has no reference to the new dep-confusion gate; an auditor reading the doc cannot discover that bare cross-repo refs on *.ghe.com marketplaces are refused before any outbound probe.
• [nit] GHES on-prem scope exclusion (GITHUB_HOST structured dep_refs bypass the sentinel path) is code-comment-only, not surfaced to operators at src/apm_cli/marketplace/resolver.py:244-249
• [nit] SCP branch is case-sensitive (git@) while URL branch uses bare_lower at src/apm_cli/marketplace/resolver.py:330-332
GIT@host:owner/repo falls through to bare-segment branch and correctly fires the sentinel (safe/fail-closed), but the inconsistency is a future-regression risk.

OSS Growth Hacker

• [recommended] CHANGELOG opens with migration cost (BREAKING) before establishing the security benefit at CHANGELOG.md:12
  Entries that lead with the benefit before the migration ask retain more readers. Reframe: 'Security fix (breaking): apm install on *.ghe.com marketplaces now fail-closes on bare cross-repo repo: fields, blocking dependency-confusion attacks...'
• [nit] Producer docs frame feature entirely as a refusal with no positive security-guarantee framing at docs/src/content/docs/producer/publish-to-a-marketplace.md
  Add: 'APM protects enterprise marketplace installs from dependency-confusion attacks by refusing ambiguous cross-repo sources before any network request.' Converts a rule into a guarantee.
• [nit] Attribution -- by @edenfunf (#1459) is correct and present -- confirmed good practice; no action needed.

Auth Expert

• [nit] is_supported_git_host accepts any valid FQDN -- trust assumption should be documented at src/apm_cli/marketplace/resolver.py:335
  The design is intentional (restricting to a known-host allowlist would break self-hosted GitLab/Bitbucket installs) but the trust boundary (marketplace.json is authored by a trusted operator) should be stated explicitly so future reviewers do not silently extend the escape hatch.
  Suggested: Add: 'Trust boundary: this guard assumes marketplace.json is authored by a trusted operator; the FQDN escape hatch is therefore scoped to that trust level.'
• [nit] suggested_public_qualified_repo built inline in install.py rather than in CrossRepoMisconfigRisk dataclass at src/apm_cli/commands/install.py:435
install.py builds github.com/{_risk.bare_repo_field} inline while the enterprise suggestion comes from _risk.suggested_qualified_repo. Two code paths that must stay in sync. Moving to a second field in CrossRepoMisconfigRisk would co-locate both computed values where they are testable.

Doc Writer

• [blocking] YAML examples use type: git -- not a valid marketplace source type; implementation requires type: github at CHANGELOG.md:15-29
_coerce_dict_plugin_type raises ValueError for 'git'; _compute_cross_repo_misconfig_risk returns None immediately if type != 'github'. A marketplace author copying the YAML verbatim gets a schema error, not the actionable dep-confusion refusal. package-authoring.md correctly uses type: github throughout -- this is a copy-paste error.
  Suggested: Replace type: git with type: github in all three YAML blocks in CHANGELOG.md (lines ~15-29) and identically in publish-to-a-marketplace.md (lines ~233-247).
• [recommended] No consumer-side callout: what must consumers change? at docs/src/content/docs/producer/publish-to-a-marketplace.md
  The BREAKING change is for marketplace authors only; consumers just re-run apm install after the author fixes marketplace.json. Without this stated, operators may audit consumer apm.yml unnecessarily. Add: 'Consumers re-run apm install after the marketplace author updates marketplace.json; no changes to apm.yml are required on the consumer side.'
• [recommended] Python-centric jargon: 'Dict-form plugin sources' at docs/src/content/docs/producer/publish-to-a-marketplace.md:221
  'Dict-form' is Python-specific. Replace with 'Mapping-style source: entries' or 'Plugin source: entries using the mapping form' to stay within YAML vocabulary.
• [nit] install.md bullet uses 'e.g.' without consistent period style vs neighbouring bullets at docs/src/content/docs/reference/cli/install.md:98

Test Coverage Expert

• [recommended] URL/SCP repo-field guard has unit coverage only; integration-with-fixtures tier is missing
  The new resolver guard that recognises https://, (redacted) (redacted) and git@host:... forms as host-qualified is exercised by a direct-call unit test only. No integration test drives a full _resolve_package_references() call with a URL or SCP repo: field. Surface floor for marketplace resolver escape-hatch behavior is integration-with-fixtures.
  Proof (passed at unit): tests/unit/marketplace/test_marketplace_resolver.py::test_compute_returns_none_on_url_or_scp_repo_field_when_filter_bypassed -- proves: URL and SCP repo-field forms are not mis-classified as bare repos by the sentinel logic [secure-by-default, devx]
• [recommended] URL/SCP escape hatch missing integration-with-fixtures test driving full install pipeline
  No integration test calls _resolve_package_references() with a URL/SCP repo: field and asserts invalid_outcomes == [] and mock_validate.call_count == 1. Add a test mirroring test_cross_repo_qualified_to_enterprise_proceeds but with repo: 'https://github.com/platform-team/shared-tool'.
  Proof (missing at integration-with-fixtures): tests/integration/test_ghe_marketplace_install_e2e.py::test_cross_repo_url_scp_form_proceeds_without_gate -- proves: An operator whose marketplace.json uses repo: https://github.com/owner/repo is not gated by the fail-closed check [secure-by-default, devx]
• [nit] Integration tests not S7-probed due to environment constraints -- code review only
  pytest not available in review environment. Three integration tests in TestCrossRepoFailClosedIntegration were fully read and assertion logic verified by code review; CI run provides S7 proof.
  Proof (unknown, environment): tests/integration/test_ghe_marketplace_install_e2e.py::TestCrossRepoFailClosedIntegration::test_cross_repo_bare_blocks_install_before_validation -- proves: HTTP probe never fires and install is rejected before validation when CrossRepoMisconfigRisk sentinel is attached [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.}

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pypi.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pypi.org"

See Network Configuration for more information.

Note

🔒 Integrity filter blocked 2 items

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

• #1459 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• fix(install): block bare cross-repo on enterprise marketplaces (#1326) #1459 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

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 #1459 · ● 5.5M · ◷