test(integration): regression trap for *.ghe.com marketplace auth routing (closes #1304)#1312
Merged
danielmeppiel merged 2 commits intoMay 14, 2026
Conversation
…ting (closes microsoft#1304) Adds tests/integration/test_ghe_marketplace_install_e2e.py exercising the full install pipeline from resolve_marketplace_plugin through DependencyReference.parse to AuthResolver.resolve_for_dep. The unit tests in tests/unit/marketplace/ cover the resolver layer directly but stop at the canonical string; the review panel for PR microsoft#1292 (closes microsoft#1285) flagged the absence of integration-tier coverage on this auth-routing contract as a secure-by-default + governed-by-policy test floor. Five test cases: - Three parametrized cases (relative source, dict github bare repo, dict github host-qualified repo) assert the full chain lands on the enterprise host: canonical carries the prefix, parse recovers it, AuthContext.host_info.host == "corp.ghe.com" with kind "ghe_cloud". - A github.com marketplace regression case locks the pre-existing default-host behaviour so the fix did not silently change it. - A regression trap for the cross-repo silent mis-route tracked in microsoft#1305: asserts the current (buggy) behaviour with a docstring pointing at the issue so the future fix has an explicit before/after diff to assert against. Stubs at two seams only: - get_marketplace_by_name / fetch_or_cache: skip marketplace registry + manifest network I/O. MarketplaceSource is trust-boundary config, not manifest content (the auth-expert confirmed this distinction during PR microsoft#1292 review). - AuthResolver._resolve_token: skip env/gh-cli/credential-helper I/O so the test is deterministic and runs without tokens. host_info on the returned AuthContext is still real (built by classify_host) -- that is the routing contract under test.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an integration test that locks the end-to-end *.ghe.com marketplace auth-routing contract fixed in PR #1292, exercising the full pipeline from resolve_marketplace_plugin through DependencyReference.parse to AuthResolver.resolve_for_dep. Stubs only the marketplace registry/manifest fetch and AuthResolver._resolve_token seams.
Changes:
- New parametrized test class
TestGHEMarketplaceInstallAuthRoutingwith 3 happy-path cases (relative, dict bare-repo, dict host-qualified-repo) asserting theAuthContextlands on the enterprise host. - Adds a
github.comregression test confirming the default behavior is unchanged. - Adds a "known silent mis-route" trap for cross-repo dict sources (tracked in #1305) so a future fix has a forcing-function before/after.
…lly traps Local regression-trap verification (reverting _needs_canonical_host_prefix to ``return False`` and re-running) revealed that one of the three parametrized cases -- ``dict-host-qualified-repo`` -- passes regardless of whether the fix is enabled. The manifest's ``repo`` field carries the host through ``_resolve_github_source`` so the canonical reaches the prefix step already host-qualified; the fix's idempotent guard makes it a no-op there. That case is therefore an idempotency lock, not a microsoft#1285 regression trap. Bundling it inside ``test_ghe_marketplace_routes_auth_at_enterprise_host`` with the genuinely-trapping ``relative-source`` and ``dict-bare-repo`` cases implied a trapping contract it does not deliver. Restructure to be honest about contracts: - ``test_ghe_marketplace_backfills_host_on_bare_canonical`` (parametrized, 2 cases) -- the actual microsoft#1285 regression trap. Verified by toggling the fix off: both cases fail at all three layers (canonical, parse host, AuthContext). - ``test_ghe_marketplace_host_qualified_dict_source_routes_idempotently`` -- separate test, named for what it locks (idempotent guard + correct routing on the already-host-qualified path). Documented as NOT a trap. The github.com regression check and the microsoft#1305 cross-repo trap are unchanged in scope; they were already correctly framed as non-microsoft#1285 contracts. Total test count stays at 5; verdict surface is the same.
danielmeppiel
approved these changes
May 14, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Collaborator
|
@edenfunf there are 10 failing integration tests in the merge queue: |
Contributor
Author
|
@danielmeppiel Thanks for the heads up. The 10 failures all hit os.getcwd() at line 110 of _invoke on worker gw1 — looks like the same kind of cwd contamination from a previous test on that worker that #1257 and #1261 dealt with earlier. This PR only adds one in-memory integration test (no os.chdir, no tmp_path, no filesystem writes; all patches use context managers or monkeypatch autouse), and test_target_resolution_e2e.py itself hasn't been modified since #1165. I tried a few xdist combos locally and couldn't reproduce. Could you retry the merge queue once? Happy to dig further if it shows up again. |
edenfunf
added a commit
to edenfunf/apm
that referenced
this pull request
May 14, 2026
Address PR microsoft#1319 review panel findings and the github-advanced-security ``Incomplete URL substring sanitization`` flag without changing the fix's core design. logger.info -> logger.warning ============================= The PR microsoft#1292 review panel's top-five follow-up microsoft#3 (the seed for microsoft#1305) explicitly recommended ``logger.warning`` for this exact diagnostic: "A single ``logger.warning`` (or structured install-time check) would close the UX gap... converts a silent failure into an actionable error and prevents repeat microsoft#1285-class support tickets." The original PR microsoft#1319 used ``logger.info`` on the rationale that ``CommandLogger.info`` is documented for "persistent advisory context... must survive quiet-mode suppression". The current panel's three-persona convergence (Python Architect + CLI Logging Expert + DevX UX) is that ``info`` is still visually ambient at default verbosity -- an operator scanning a red ``[x]`` line will not register an adjacent ``[i]`` as the recovery action. ``warning`` renders ``[!]`` and matches the traffic-light convention the codebase uses elsewhere. As a side benefit, ``warning`` is implemented on both ``CommandLogger`` and ``NullCommandLogger`` (``info`` is not on the latter), so the message shape is now safe against any future caller variant. Remove the ``Hint:`` prefix; the ``[!]`` symbol carries the advisory signal on its own. Inline the resolved enterprise hostname into the "registered on" clause so the test assertions can anchor on contextual prose (e.g. ``"registered on 'corp.ghe.com'"``) instead of bare hostname substrings -- which silences the CodeQL flag without weakening what the assertion verifies. Auth-expert second clause ========================= The original hint read as if the misconfigured case was the only explanation for a validation failure: "If you meant the enterprise host, set the plugin's repo field to corp.ghe.com/...". A legitimate ``github.com`` cross-host dep that fails for a transient reason (rate-limit, network, expired PAT) would read that hint and add an enterprise host prefix that breaks a working config. Append the auth-expert recommended second clause: "If this is intentionally a github.com dependency, verify your github.com credentials and that the repository is accessible." Both clauses are explicitly conditional, so neither path is misdirected. The original issue's "two intents" framing assumed validation success vs 401; this clause covers the third path (validation failure on a legitimate dep) that was not in the issue spec but is real. Integration trap + new e2e test =============================== ``test_cross_repo_locks_known_silent_misroute`` in ``tests/integration/test_ghe_marketplace_install_e2e.py`` was authored by PR microsoft#1292 specifically to give "the future microsoft#1305 fix an explicit before/after diff to assert against". The microsoft#1305 fix deliberately preserves the resolver-level routing (bare cross-repo -> github.com is correct for legitimate cross-host deps) and adds a sentinel + an install-time hint instead. Update the test's docstring to reflect this, keep the routing-preservation assertions, and add three new sentinel assertions so the metadata the install command consumes is locked at the integration tier. Add ``TestCrossRepoMisconfigHintIntegration`` with two scenarios: - ``test_cross_repo_hint_emitted_on_validation_failure``: drives the real ``_resolve_package_references`` + ``InstallLogger`` through ``capsys`` and asserts the warning-level hint contains the plugin@marketplace identity, the enterprise host anchored to its "registered on" clause, the bare repo, the host-qualified fix value, and the auth-expert second clause. - ``test_legitimate_cross_host_validation_passes_no_hint``: locks the no-pollution contract for the legitimate cross-host path that validates successfully. This matches the convention PR microsoft#1292 established with PR microsoft#1312 (microsoft#1304 closer): panel-flagged ``outcome: missing`` integration findings on secure-by-default surfaces should land an integration-tier trap, not just unit coverage. CHANGELOG ========= Add the ``[Unreleased] Fixed`` entry naming GHE enterprise marketplace explicitly so enterprise teams scanning the changelog for cross-repo misconfiguration symptoms recognize the fix on upgrade. Mirrors the PR microsoft#1292 entry style. Out of scope ============ The supply-chain finding (cross-repo bare where the same owner/repo exists on github.com with attacker-staged content) is a real dependency-confusion vector but is not the diagnostic-surface problem microsoft#1305 targets; tracked as a separate follow-up issue. The doc-writer finding referenced ``docs/manifest-schema.md`` which does not exist in this repository; documentation additions deferred to a focused docs PR.
danielmeppiel
added a commit
to edenfunf/apm
that referenced
this pull request
May 14, 2026
) (microsoft#1319) * fix: hint to host-qualify cross-repo on *.ghe.com (closes microsoft#1305) PR microsoft#1292 fixed the silent ``github.com`` auth fallback for **in-marketplace** plugin sources on ``*.ghe.com`` marketplaces but deliberately scoped its host backfill via ``_is_in_marketplace_source`` to avoid changing routing for cross-repo dict sources. A bare cross-repo ``repo: owner/proj`` on an enterprise marketplace still legitimately means two different things -- a real ``github.com`` open-source dep, or a misconfigured same-host entry that should have been ``corp.ghe.com/owner/proj`` -- and the resolver cannot disambiguate them. The silent mis-route survives for the second intent: the canonical stays bare, ``DependencyReference.parse`` defaults the host to ``github.com``, and the install path reports the generic ``not accessible or doesn't exist -- run with --verbose for auth details`` with zero pointer at the marketplace's enterprise host. Surface the diagnostic at the install-time validation-failure boundary, not at resolver time. The legitimate cross-host case validates successfully and never sees a hint; the misconfigured case fails validation and gets an actionable host-qualified suggestion. The resolver-time always-on warning the PR microsoft#1292 review panel rejected -- which would false-positive on the legitimate case and train operators to ignore -- is avoided. Approach ======== Resolver attaches a typed ``CrossRepoMisconfigRisk`` sentinel to ``MarketplacePluginResolution`` when **all** of: - ``dependency_reference`` is ``None`` (GitHub-family virtual-shorthand path; GitLab-class and self-managed FQDN marketplaces build a structured ref upstream and sidestep the bug) - ``plugin.source`` is a dict whose normalized type is ``github`` -- via the existing ``_coerce_dict_plugin_type`` (covers ``type``/``kind``/``source`` synonyms plus the inferred-github fallback). Cross-repo ``gitlab`` / ``git-subdir`` dict sources on enterprise marketplaces hit the same auth-routing bug but the "host-qualify with marketplace host" remediation only matches operator intent for the GitHub family. - the source is NOT an in-marketplace reference (PR microsoft#1292's domain) - ``_needs_canonical_host_prefix`` agrees the canonical is bare and the host is GitHub-family enterprise (``*.ghe.com``; idempotent against already host-qualified, URL, and SSH forms) - the ``repo`` field is a non-empty ``owner/repo`` shorthand The helper is pure -- no logging, no canonical mutation. Resolver behavior is unchanged; only the resolution object carries one extra optional field. Install command records the risk in a per-call ``_misconfig_risks`` dict **before** validation runs. The existing ``_marketplace_provenance`` map only gets written on validation success and cannot be relied on at the failure boundary. When ``_validate_package_exists`` returns ``False`` (which is how the GitHub-family auth failure surfaces -- ``AuthResolver.try_with_fallback`` collapses 401/404/network into a single ``False``, no typed ``AuthenticationError``), the validation-fail branch emits the hint inline via ``logger.info`` so the operator can correct ``marketplace.json`` without rerunning under ``--verbose`` to decode the auth trace. Why this layer, not ``AuthenticationError`` =========================================== The two ``raise AuthenticationError`` sites in the install pipeline are both gated to non-GitHub hosts (ADO / self-managed): ``pipeline.py`` preflight skips ``is_github_hostname(host)`` early; ``validation.py`` requires the ``is_ado_auth_failure_signal`` stderr pattern. The github.com fallback path goes through ``try_with_fallback`` which returns ``False`` on failure, and the caller records ``(canonical, reason)`` into ``invalid_outcomes``. Decorating ``AuthenticationError`` would be a dead-code hook for this bug -- the typed exception never fires on the github.com path. The validation-fail branch is the actual choke point. Scope and tradeoffs =================== - 404 typo on the cross-repo ``repo`` field and network failures will also trigger the hint; the wording leads with the routing fact ("resolved to 'github.com'") and the suggestion is conditional ("If you meant the enterprise host"), so the false-positive remains advisory rather than misleading. Distinguishing 401 from 404/network here would require threading HTTP status out of ``try_with_fallback`` -- a much broader cross-cutting change. - The silent-success-on-wrong-host case (cross-repo bare where the same ``owner/repo`` happens to exist on github.com with different content) cannot be detected without changing the routing semantics PR microsoft#1292 preserved. This is acknowledged out of scope in the issue. Tests ===== ``TestCrossRepoMisconfigRisk`` (resolver, 14 cases) locks the truth table for sentinel attach / no-attach across the dict-type synonyms (``type``, ``kind``, ``source``, inferred-github), host-qualified / URL / SSH / no-slash defensive guards, the gitlab / git-subdir exclusion, and pure ``github.com`` marketplace non-pollution. ``TestResolvePackageReferencesCrossRepoMisconfigHint`` (install, 4 cases) locks the hint emission contract: hint fires only when a risk-bearing marketplace resolution subsequently fails validation; the legitimate cross-host path that validates successfully emits no hint; in-marketplace and plain owner/repo failures emit no hint. Both test suites were toggle-verified -- removing the resolver helper call or the install-side emission block makes the corresponding positive case fail. * review: warning-level hint, anchored test substrings, integration trap Address PR microsoft#1319 review panel findings and the github-advanced-security ``Incomplete URL substring sanitization`` flag without changing the fix's core design. logger.info -> logger.warning ============================= The PR microsoft#1292 review panel's top-five follow-up microsoft#3 (the seed for microsoft#1305) explicitly recommended ``logger.warning`` for this exact diagnostic: "A single ``logger.warning`` (or structured install-time check) would close the UX gap... converts a silent failure into an actionable error and prevents repeat microsoft#1285-class support tickets." The original PR microsoft#1319 used ``logger.info`` on the rationale that ``CommandLogger.info`` is documented for "persistent advisory context... must survive quiet-mode suppression". The current panel's three-persona convergence (Python Architect + CLI Logging Expert + DevX UX) is that ``info`` is still visually ambient at default verbosity -- an operator scanning a red ``[x]`` line will not register an adjacent ``[i]`` as the recovery action. ``warning`` renders ``[!]`` and matches the traffic-light convention the codebase uses elsewhere. As a side benefit, ``warning`` is implemented on both ``CommandLogger`` and ``NullCommandLogger`` (``info`` is not on the latter), so the message shape is now safe against any future caller variant. Remove the ``Hint:`` prefix; the ``[!]`` symbol carries the advisory signal on its own. Inline the resolved enterprise hostname into the "registered on" clause so the test assertions can anchor on contextual prose (e.g. ``"registered on 'corp.ghe.com'"``) instead of bare hostname substrings -- which silences the CodeQL flag without weakening what the assertion verifies. Auth-expert second clause ========================= The original hint read as if the misconfigured case was the only explanation for a validation failure: "If you meant the enterprise host, set the plugin's repo field to corp.ghe.com/...". A legitimate ``github.com`` cross-host dep that fails for a transient reason (rate-limit, network, expired PAT) would read that hint and add an enterprise host prefix that breaks a working config. Append the auth-expert recommended second clause: "If this is intentionally a github.com dependency, verify your github.com credentials and that the repository is accessible." Both clauses are explicitly conditional, so neither path is misdirected. The original issue's "two intents" framing assumed validation success vs 401; this clause covers the third path (validation failure on a legitimate dep) that was not in the issue spec but is real. Integration trap + new e2e test =============================== ``test_cross_repo_locks_known_silent_misroute`` in ``tests/integration/test_ghe_marketplace_install_e2e.py`` was authored by PR microsoft#1292 specifically to give "the future microsoft#1305 fix an explicit before/after diff to assert against". The microsoft#1305 fix deliberately preserves the resolver-level routing (bare cross-repo -> github.com is correct for legitimate cross-host deps) and adds a sentinel + an install-time hint instead. Update the test's docstring to reflect this, keep the routing-preservation assertions, and add three new sentinel assertions so the metadata the install command consumes is locked at the integration tier. Add ``TestCrossRepoMisconfigHintIntegration`` with two scenarios: - ``test_cross_repo_hint_emitted_on_validation_failure``: drives the real ``_resolve_package_references`` + ``InstallLogger`` through ``capsys`` and asserts the warning-level hint contains the plugin@marketplace identity, the enterprise host anchored to its "registered on" clause, the bare repo, the host-qualified fix value, and the auth-expert second clause. - ``test_legitimate_cross_host_validation_passes_no_hint``: locks the no-pollution contract for the legitimate cross-host path that validates successfully. This matches the convention PR microsoft#1292 established with PR microsoft#1312 (microsoft#1304 closer): panel-flagged ``outcome: missing`` integration findings on secure-by-default surfaces should land an integration-tier trap, not just unit coverage. CHANGELOG ========= Add the ``[Unreleased] Fixed`` entry naming GHE enterprise marketplace explicitly so enterprise teams scanning the changelog for cross-repo misconfiguration symptoms recognize the fix on upgrade. Mirrors the PR microsoft#1292 entry style. Out of scope ============ The supply-chain finding (cross-repo bare where the same owner/repo exists on github.com with attacker-staged content) is a real dependency-confusion vector but is not the diagnostic-surface problem microsoft#1305 targets; tracked as a separate follow-up issue. The doc-writer finding referenced ``docs/manifest-schema.md`` which does not exist in this repository; documentation additions deferred to a focused docs PR. * style: ruff format new test files --------- Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Adds an integration-tier test (5 cases) for the
*.ghe.commarketplace auth-routing contract fixed in PR #1292 (closes #1285). Drives the full pipeline fromresolve_marketplace_pluginthroughDependencyReference.parsetoAuthResolver.resolve_for_dep, asserting theAuthContextlands on the enterprise host instead of falling back togithub.com. Test contracts are split by what each case actually traps -- two cases are #1285 regression traps verified by toggling the fix off; the rest are idempotency / regression / non-#1285 locks. Closes #1304.Why this matters
PR #1292's fix is covered by 9 unit tests at the resolver layer, but those stop at the canonical string. The review panel flagged the absence of integration-tier coverage on the full chain as a
secure-by-default+governed-by-policytest floor: the auth-routing contract for enterprise marketplaces is policy-load-bearing but not machine-verified end-to-end today.What this adds
tests/integration/test_ghe_marketplace_install_e2e.py-- one classTestGHEMarketplaceInstallAuthRouting, 5 tests organised by what each one actually locks:test_ghe_marketplace_backfills_host_on_bare_canonical[relative-source]test_ghe_marketplace_backfills_host_on_bare_canonical[dict-bare-repo]test_ghe_marketplace_host_qualified_dict_source_routes_idempotentlytest_github_com_marketplace_keeps_github_defaulttest_cross_repo_locks_known_silent_misrouteStubbing strategy
Two seams only:
get_marketplace_by_name/fetch_or_cache: skip marketplace registry + manifest network I/O.MarketplaceSourceis trust-boundary registry config, not manifest content.AuthResolver._resolve_token: skip env /ghCLI / credential-helper I/O so the test is deterministic and runs without tokens. Thehost_infofield on the returnedAuthContextis still real (built byclassify_host) -- that is the routing contract under test.Also
monkeypatch.delenv("GITHUB_HOST"): #1285 explicitly notesGITHUB_HOST=corp.ghe.comis not a viable workaround. Clearing the env confirms we are validating the fix, not env masking.Validation
I verified the regression-trap claim end-to-end by temporarily reverting
_needs_canonical_host_prefixtoreturn False(simulating the pre-fix state) and re-running the suite. Result:The two failing cases fail at three layers simultaneously (canonical mismatch, parse host wrong,
AuthContext.host_info.hostwrong) -- a defense-in-depth trap rather than a single boundary check. The three passing cases are correctly labelled as non-#1285 contracts.This verification surfaced an earlier draft of the test that bundled the
dict-host-qualified-repocase inside the same parametrize as the two real trap cases -- it would have passed with or without the fix because the repo field carried the host through_resolve_github_sourcebefore the prefix step. Splitting it out (force-push 16f265b) makes each test name align with what it actually traps.Deviations from the issue spec
Two small deviations from #1304 to better align with existing project conventions:
1. File path: top-level
tests/integration/instead oftests/integration/marketplace/.The marketplace/ subdir is dedicated to marketplace authoring tests (
apm marketplace build/check/init/publish/doctor/outdated). Every existing file there is an authoring test. Consumer-flow install tests -- which is what this is -- live at the top level alongsidetest_gitlab_install_e2e.py,test_ado_e2e.py, andtest_marketplace_e2e.py. (I wrote the original issue text without auditing the existing layout.)2. Marker:
@pytest.mark.integrationinstead of arequires_*marker.The conftest marker registry (
_MARKER_CHECKS) gates tests on real preconditions: tokens, runtimes, e2e mode, real binary, real network. This test has none -- it is fully mocked at the registry and credential seams and runs in-process.tests/integration/test_gitlab_install_e2e.pyfollows the same pattern:@pytest.mark.integrationlabel only, norequires_*.tests/integration/test_marker_registry_sync.pycontinues to pass.How to test
Local: 5/5 pass in ~0.5s.