fix(git-env): skip the HTTP header probe for non-HTTP effective URLs

[Arnaud (arnaudoisel)]

fix(git-env): skip HTTP header probes for non-HTTP effective URLs

TL;DR

Fixes private Git dependency downloads when a standard insteadOf rule rewrites HTTPS to SCP-style SSH. HTTP header applicability is checked only for HTTP(S) effective URLs; existing rewrite admission and credential boundaries remain enforced. Genuine probe failures now identify the exit status and recommend inspecting configuration rather than deleting a rule not proven unsafe.

Note

Fixes #2898. Original fix and reproduction by Arnaud (@arnaudoisel); the original commits are preserved. Follow-up consumer regressions, diagnostics, and documentation are included in this PR.

Problem (WHY)

• On the authenticated retry, a command-scoped http.extraHeader caused Git's URL-match probe to receive an SCP target such as git@github.com:owner/repo.
• Git returns status 128 for that input: fatal: invalid URL scheme name or missing '://' suffix. This surfaced as Unable to verify Git URL rewrite safety during the reported private dependency installation.
• [!] The previous recovery text also told users to remove an unsafe rule when only the probe itself had failed.

The contributor's #2898 reproduction requires a matching rewrite, an injected authorization header, and an actual download. A public dependency without an injected header does not trigger the same probe.

[url "git@github.com:"]
    insteadOf = https://github.com/

The contributor measured Git 2.55.0 with a dummy command-scoped header:

Effective URL form	URL-match exit status
https://github.com/o/r	0
ssh://git@github.com/o/r	0
git@github.com:o/r	128

Approach (WHAT)

• Skip HTTP header applicability probes for non-HTTP(S) effective targets.
• Keep rewrite validation in the existing shared owner used by both Git execution and GitConfigInsteadOfResolver.
• Defer malformed URL rejection to the existing wrapped validator, without leaking parser details.
• Keep inspection guidance for probe failures separate from unsafe-rule removal guidance for proven policy violations.
• Exercise the real Git config subprocess and production consumers with isolated files and dummy credentials.

Implementation (HOW)

File	Change
src/apm_cli/utils/git_env.py	Adds the protocol guard, preserves malformed-URL error wrapping, includes probe exit status, and separates inspection from unsafe-rule recovery.
tests/unit/cache/test_git_env.py	Covers SCP cloning with an injected header, no-probe behavior, malformed targets, and status/redaction diagnostics.
tests/unit/test_transport_selection.py	Uses the real resolver, selector, Git configuration, and child environment; covers SCP, no header, unmatched rules, ssh://, and authenticated HTTPS origin rejection.
tests/unit/core/test_public_github_anonymous_first.py	Drives the real anonymous-to-authenticated retry with Git config fixtures; verifies HTTP authorization is absent from the SSH child while its rewrite remains.
docs/src/content/docs/getting-started/authentication.md	Documents the HTTP(S)-only probe boundary and configuration inspection guidance.
packages/apm-guide/.apm/skills/apm-usage/authentication.md	Mirrors the authentication guidance for agents.
CHANGELOG.md	Records the private dependency install fix.

Diagrams

The new non-HTTP branch skips only header applicability; both branches still reach rewrite validation.

flowchart LR
    R["resolve_git_url_rewrite"] --> H["_has_applicable_http_authorization"]
    H --> S{"HTTP or HTTPS?"}
    S -->|"yes"| P["Git URL-match probe"]
    S -->|"no"| N["Return no HTTP authorization"]
    P --> V["validate_resolved_git_url_rewrite"]
    N --> V
    V --> C["Validated child Git environment"]
    classDef changed stroke-dasharray: 5 5;
    class S,N changed;

Loading

Trade-offs

• This extends the existing owner rather than changing AuthResolver or credential selection. Same-host SSH remains credential-free; HTTPS origin checks remain active.
• Consumer regressions use real Git config and real production auth/transport boundaries with a controlled failing network callback. They do not claim a full installed-CLI private-network download.
• Parser ValueError is deferred to the shared validator so malformed targets still fail with the wrapped, redacted message; it is not treated as an accepted rewrite.

Benefits

1. The authenticated HTTPS-to-SCP reproduction no longer fails at Git's HTTP URL-match parser.
2. Matching HTTPS rewrites still reject credential origin changes.
3. Probe failures retain actionable inspection guidance without declaring an unverified rule unsafe.

Validation

Exact head: 221b7debb069e057861b2a50f96bcc4e9918feff; integrated main: e38261c5db4d893d6ddebc3925742e4e3bd2ba74.

UV_FROZEN=true uv run --extra dev pytest -q tests/unit/cache/test_git_env.py tests/unit/core/test_git_transport_policy.py tests/unit/test_transport_selection.py tests/unit/core/test_public_github_anonymous_first.py tests/unit/install/test_validation_strict_transport.py tests/unit/cache/test_proxy_compat.py

253 passed in 17.72s

All seven local CI lint gates and the architecture boundary lint passed on this head. Mutation checks produced five SCP failures without the protocol guard, one malformed-URL failure without its wrapper, two diagnostic failures with old wording, and one HTTPS failure with a blanket probe skip. All mutations were restored before the final run.

Warning

Hosted CI has not run successfully on this head. CI, CodeQL, Spec conformance, Merge Gate, NOTICE Drift Check, and Deploy Docs concluded action_required. See CI run 34413898041. Maintainer action is required; local results are not a substitute for hosted CI.

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	A standard HTTPS-to-SCP rewrite works while an HTTP authorization header is present.	DevX (pragmatic as npm), Secure by default	tests/unit/test_transport_selection.py::TestGitConfigInsteadOfResolver::test_real_resolver_allows_https_to_scp_ssh_rewrite (regression-trap for #2898)	integration
2	A private dependency's authenticated retry keeps the SSH rewrite without passing HTTP credentials to its child.	Secure by default, DevX (pragmatic as npm)	tests/unit/core/test_public_github_anonymous_first.py::test_public_github_authenticated_retry_drops_header_after_real_ssh_rewrite (regression-trap for #2898)	integration
3	Existing no-header, unmatched-rule, and ssh:// configurations retain their transport selection.	DevX (pragmatic as npm)	tests/unit/test_transport_selection.py::TestGitConfigInsteadOfResolver	integration
4	An authenticated HTTPS rewrite cannot silently change credential origin.	Secure by default	tests/unit/test_transport_selection.py::TestGitConfigInsteadOfResolver::test_real_resolver_checks_http_authorization_for_https_rewrite	integration
5	Malformed targets fail with the wrapped safety message, not raw parser details.	Secure by default, DevX (pragmatic as npm)	tests/unit/cache/test_git_env.py::TestGitSubprocessEnv::test_malformed_rewrite_target_keeps_the_wrapped_safety_error	integration
6	A real probe failure identifies its exit status without exposing raw Git config or recommending unsafe-rule removal.	DevX (pragmatic as npm), Secure by default	tests/unit/cache/test_git_env.py::TestGitSubprocessEnv::test_http_urlmatch_failure_reports_status_without_raw_config	unit

Here, integration denotes real Git configuration and production cross-module boundaries, independent of the existing test directory names.

How to test

1. Run the exact targeted pytest command above.
2. Run the repository's canonical lint mirror, including the architecture boundary lint.
3. Inspect test_real_resolver_consumer_drops_dummy_http_header_after_scp_rewrite for a fixture-backed demonstration; no real token or global Git config change is needed.

Spec conformance (OpenAPM v0.1)

• N/A -- this PR does not change the manifest or specification contract.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

[Arnaud (arnaudoisel)]

Agreement

@microsoft-github-policy-service agree company="Lucca"

[Copilot]

🟡 Changes recommended

_has_applicable_http_authorization now calls urlsplit() without guarding ValueError, which can surface as an unwrapped exception for malformed URLs and bypass the module’s consistent rewrite-safety error handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a regression in Git URL rewrite safety validation where the HTTP extraHeader URL-match probe was being executed against SCP-style SSH effective URLs (e.g., git@github.com:owner/repo), which Git rejects, breaking authenticated private Git dependency installs.

Changes:

• Skip the HTTP header URL-match probe when the effective URL is not HTTP(S).
• Improve probe failure diagnostics by including the probe exit status.
• Add unit regressions covering SCP-style rewrites during authenticated retries and asserting the probe is not spawned for non-HTTP URLs.

File summaries

File	Description
src/apm_cli/utils/git_env.py	Adds an HTTP(S)-scheme guard before probing --get-urlmatch, and improves probe error messaging.
tests/unit/cache/test_git_env.py	Adds regression tests for SCP-style SSH rewrites under injected auth headers and ensures the probe is skipped.
CHANGELOG.md	Documents the fix under Unreleased/Fixed.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Arnaud (arnaudoisel)]

@microsoft-github-policy-service terminate

[Arnaud (arnaudoisel)]

@microsoft-github-policy-service agree

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_now

PR #2906 removes a false fail-closed on standard HTTPS-to-SSH Git rewrites while preserving the real credential and rewrite trust boundaries that protect private installs.

cc Arnaud (@arnaudoisel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

Panel consensus on the code is clear: no in-scope defect remains on HEAD 221b7debb069e057861b2a50f96bcc4e9918feff. The load-bearing evidence is automated and passed at the exact head. tests/unit/test_transport_selection.py::TestGitConfigInsteadOfResolver::test_real_resolver_allows_https_to_scp_ssh_rewrite proves the reported path now resolves to SSH with assert plan.attempts[0].effective_url == "git@github.com:owner/repo" and assert plan.attempts[0].use_token is False. tests/unit/core/test_public_github_anonymous_first.py::test_public_github_authenticated_retry_drops_header_after_real_ssh_rewrite proves the authenticated retry keeps the rewrite while stripping HTTP auth from the child with assert _git_auth_entries(auth_retry_child) == []. tests/unit/test_transport_selection.py::TestGitConfigInsteadOfResolver::test_real_resolver_checks_http_authorization_for_https_rewrite also preserves the fail-closed HTTPS boundary with with pytest.raises(GitUrlRewriteError, match="different HTTPS origin").

Strategically, this is the right kind of fix for APM: narrow the HTTP header applicability probe to the protocols where Git's URL-match semantics actually apply, keep rewrite validation in the shared owner used by both execution and selection paths, and correct the recovery prose without turning a scoped bug fix into a broad auth redesign. That improves trust on a real private-dependency workflow contributors already use, while keeping the default security posture intact. The changelog entry and both authentication docs are now truthful about the boundary, so community cost is low and communication is explicit.

The only material caution left is outside the code assessment: hosted GitHub workflows are still action_required, so this head is not yet operationally merge-ready. That should be treated as a human authorization and policy gate, not as a reason to reopen the scoped implementation. The earlier test-coverage complaint about missing Scenario Evidence is superseded by the live PR body, which now carries the six-scenario mapping and keeps the governance trail honest about fixture tier and validation scope.

Dissent. No live panel dissent remains. The one residual recommendation from test-coverage was based on an earlier PR body snapshot and is now satisfied by the final Scenario Evidence table already folded into the live description.

Aligned with: Secure by default, Governed by policy, OSS community-driven, Pragmatic as npm

Growth signal. Worth amplifying in release notes as a small but meaningful trust win: private Git installs now honor standard HTTPS-to-SSH insteadOf rewrites without tripping a false safety error, and the docs explain the boundary plainly.

Panel summary

Persona	B	R	N	Takeaway
Auth Expert	0	0	0	No auth regression found: the non-HTTP scheme guard preserves HTTPS credential boundaries, real retry behavior, and resolver-level same-host SSH rewrite handling.
CLI Logging Expert	0	0	0	Probe-failure and unsafe-rewrite recovery are now separated cleanly; no CLI logging follow-up in the scoped final diff.
DevX UX Expert	0	0	0	Scoped install-failure UX fix is now accurate, actionable, and mirrored in the user docs; ship.
Doc Writer	0	0	0	No docs fault remains: CHANGELOG and both auth guides now match the HTTP(S)-only probe boundary and probe-failure recovery; ship.
OSS Growth Hacker	0	0	0	This lands as a clean friction-removal fix with truthful changelog/docs support; no OSS-growth follow-up needed before merge.
performance-expert	0	0	0	Ship: src/apm_cli/utils/git_env.py:594-606 adds an O(1) scheme guard that skips one git config probe subprocess on non-HTTP(S) rewrites; no new loops or cache-layer regressions.
Python Architect	0	0	1	Scoped fix preserves the shared rewrite-safety owner and keeps the fail-closed auth boundary intact; ship.
Supply Chain Security	0	0	0	Change narrows the HTTP header probe to HTTP(S), preserves fail-closed rewrite validation and credential boundaries, and adds regression coverage; ship.
Test Coverage	0	1	0	Real Git-backed regression traps cover the SCP rewrite/auth retry fix, but the PR body still lacks the required Scenario Evidence table. The body-only follow-up is now resolved.

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

Architecture

classDiagram
    direction LR
    class GitEnvUrlRewritePolicy {
      <<IOBoundary>>
      +_validated_git_url_rewrite_policy(remote_url, env, git_dir, worktree)
      +validate_resolved_git_url_rewrite(remote_url, effective_url, has_authorization, managed_auth_intent)
      +_has_applicable_http_authorization(remote_url, headers, env)
      +_urlmatched_header_group(remote_url, headers, env)
    }
    class GitNetworkEnvBuilder {
      <<ProceduralBoundary>>
      +git_network_env(remote_url, overrides, git_dir, worktree)
      +_build_git_auth_fence(transport_url, snapshot, env, intent_snapshot, managed_auth_intent)
    }
    class GitConfigInsteadOfResolver {
      <<Adapter>>
      +resolve(candidate_url) str|None
      +_load_rewrites() tuple[list[tuple], tuple]
    }
    class GitConfigEntry {
      <<ValueObject>>
      +scope str
      +key str
      +value str
    }
    class _GitConfigSnapshot {
      <<ValueObject>>
      +entries tuple[GitConfigEntry,...]
      +rewrites tuple[tuple[str,str],...]
      +http_headers tuple[GitConfigEntry,...]
    }
    class _GitAuthFence {
      <<ValueObject>>
      +remote_url str
      +reset_headers bool
      +suppress_helpers bool
      +safe_headers tuple[str,...]
      +managed_header str|None
    }
    GitConfigInsteadOfResolver ..> GitEnvUrlRewritePolicy : delegates
    GitEnvUrlRewritePolicy ..> GitConfigEntry : inspects
    GitEnvUrlRewritePolicy ..> _GitConfigSnapshot : returns
    GitNetworkEnvBuilder ..> GitEnvUrlRewritePolicy : reuses owner
    GitNetworkEnvBuilder ..> _GitConfigSnapshot : materializes
    GitNetworkEnvBuilder ..> _GitAuthFence : produces
    note for GitEnvUrlRewritePolicy "No OO change in this PR.\nThis procedural boundary remains the canonical owner for rewrite safety and HTTP header applicability."
    note for GitNetworkEnvBuilder "Collect then render:\nvalidate the snapshot, then materialize the child env."
    class GitEnvUrlRewritePolicy:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

Recommendation

Advisory code stance: ship this change as-is. Do not treat that as a recommendation to merge immediately; first clear the six hosted action_required workflows and whatever maintainer authorization or policy step is blocking them, then merge once those external gates are satisfied.

---

Full per-persona findings

Python Architect

• [nit] No architecture follow-up needed.
  Design patterns
• Used in this PR: Adapter plus dataclass-as-value-object -- GitConfigInsteadOfResolver.resolve() continues to delegate rewrite safety and HTTP header applicability to the shared git_env.py owner, while GitConfigEntry and _GitConfigSnapshot carry validated config facts across callers.
• Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security

No findings.

OSS Growth Hacker

No findings.

Auth Expert

No findings.

Doc Writer

No findings.

Test Coverage

• [recommended] Add the required Scenario Evidence table to the PR body.
  This is a behavior-change bug-fix PR, so the PR description should carry the Scenario Evidence table required by .github/skills/pr-description-skill/assets/scenario-evidence-rubric.md. I probed the supplied PR body with rg for 'Scenario Evidence' and found no matching section or table in files/pr-context.json. The code-side regression coverage now looks sufficient by boundary: the added tests run real Git config subprocesses through clone_git_worktree, GitConfigInsteadOfResolver plus TransportSelector, and AuthResolver.try_with_fallback plus git_network_env, and the targeted exact-head nodeids passed locally (11 passed in 2.59s). The remaining gap is governance of that evidence in the PR body, so a maintainer cannot audit scenario-to-test mapping from the description alone.
  Suggested: Add a Scenario Evidence table that names the user-facing SCP rewrite retry scenario, the preserved HTTPS origin rejection controls, the exact test paths/nodeids, and the APM principles each row defends.
  Proof (unknown): (no test ref) -- proves: The PR body maps each user-visible scenario to the automated tests that defend it. [devx,oss]
  rg 'Scenario Evidence' /Users/danielmeppiel/.copilot/session-state/5ccd713d-99fe-4376-871d-d73087091011/files/pr-context.json -> no matches

Performance Expert

No findings. The non-HTTP path avoids the unnecessary probe subprocess.

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

Reservations carried from strategic-alignment

• Preserve fail-closed handling of injected HTTP authorization headers and existing trust boundaries; only skip probes in protocols where HTTP headers cannot apply. -- Addressed by the scheme guard and mutation-proven HTTPS origin rejection.
• Bound repair to transport/probe handling, not broad AuthResolver or credential injection changes. -- Existing owner extended; no auth resolver production changes.
• Regression must cover real SCP matching rewrite with authenticated retry and preserve no-header, unmatched-rule, ssh:// and HTTPS controls. -- Real Git config fixtures exercise the resolver, selector, child environment, and actual authenticated retry.
• Legitimate failures should explain probe failure without telling users to remove standard insteadOf config. -- Probe-specific inspection guidance is separate from proven-unsafe rewrite recovery.

Folded in this run

• Add real Git config resolver, transport selector, and authenticated-retry consumer regressions with SCP/no-header/unmatched/ssh/HTTPS controls. -- 221b7debb0.
• Separate probe-failure inspection guidance from proven-unsafe rewrite recovery and protect exit-status/redaction diagnostics. -- 221b7debb0.
• Synchronize Starlight and apm-guide authentication documentation. -- 221b7debb0.
• Add the six-scenario evidence table and honest validation scope to the PR body for this exact head (metadata-only fold). -- 221b7debb0.

Copilot signals reviewed

• 3967028719: LEGIT malformed-host wrapping concern, already fixed and thread-resolved by the contributor in 7ab2b0de. The suggested invalid-port example was imprecise; the bracketed-host concern was reproduced. Two fetch rounds, no new inline findings.

Regression-trap evidence

Removing the protocol guard produced five SCP failures (including real resolver and authenticated retry consumers); removing the parser wrapper produced one failure; restoring old diagnostics produced two failures; a blanket header-probe skip produced one HTTPS-origin failure. All mutations were restored.

Local validation

Exact head 221b7debb069e057861b2a50f96bcc4e9918feff: 253 targeted tests passed. All seven local CI lint gates plus architecture boundaries passed after integrating main e38261c5db4d893d6ddebc3925742e4e3bd2ba74.

Version 2 owner evidence detects one owner: Git child-process repository location and URL rewrite safety. Four exact-head functional test IDs cover it; schema validation and strict semantic verification passed. This is an existing-owner extension, not a centralization or authority split.

CI and mergeability

Hosted CI is not green. All six Actions runs for this exact head concluded action_required; the check rollup exposes only the passing CLA. CI run. Maintainer authorization/policy action is required; the exact policy is not established by the API evidence.

PR	Head	Code stance	Iterations	Folds	Deferrals	Copilot rounds	CI	mergeable	mergeStateStatus
#2906	221b7debb0	ship_now	2	4	0	2	action_required	MERGEABLE	BLOCKED

Convergence

No scoped code follow-up remains. The code advisory is positive, but this PR is not ready to merge until hosted workflows and repository policy are satisfied. No approval, bypass, or merge was performed. This comment replaces the initial advisory rather than adding another panel comment.

[Daniel Meppiel (danielmeppiel)]

Docs sync advisory

Verdict: in_place * Pages affected: 2 * LLM calls: 10/15

Summary

Classifier verdict was in_place for docs/src/content/docs/getting-started/authentication.md. The panel found one real drift: the docs did not say that the http.extraHeader URL-match probe only runs for HTTP(S) effective URLs. Verifier checks confirmed that same-host SSH rewrites still remain credential-free and that HTTP(S) safety checks still stand. The verifier also refuted a stronger claim about current CLI wording, so the final docs stay message-agnostic: they tell users to inspect matching rules and retry before removing a rule that may actually be safe.

Applied patches

• docs/src/content/docs/getting-started/authentication.md
  • Added the missing HTTP(S)-only probe sentence.
  • Clarified that safe same-host SSH rewrites stay credential-free.
  • Reworked troubleshooting toward inspecting matching rules and retrying before removing a rule.
• packages/apm-guide/.apm/skills/apm-usage/authentication.md
  • Synced the same guidance into the required auth guide resource file.
  • Added the explicit HTTP(S)-only probe note.
  • Kept unsafe-rule removal guidance limited to unsafe or misconfigured rewrites.

Validation

• Schema validation passed for the classifier artifact and every shared panelist artifact. See docs-schema-validation.json.
• Verifier findings: 3 total claims, 2 verified, 1 refuted, 0 inconclusive.
• CDO synthesis: 1 redraft round, then approved the minimal docs-only patch.
• Final edited files are ASCII-only and limited to the two requested doc paths.

Artifacts

• docs-classifier.json
• docs-localizer.json
• docs-writer-corpus-initial.json
• docs-writer-guide-initial.json
• docs-verifier.json
• docs-writer-corpus-final.json
• docs-writer-guide-final.json
• docs-editorial.json
• docs-growth.json
• docs-cdo.json
• docs-schema-validation.json

[iLLeniumStudios (TheiLLeniumStudios)]

Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) can we please get this reviewed / fixed soon? This is currently a blocker for us to use apm at Electronic Arts. I can help contribute to / provide more details around it if needed to speed things up. Thanks