Fix: apm install https://... no longer falls back to SSH on generic Git hosts#995
Conversation
…neric hosts Agent-Logs-Url: https://github.com/microsoft/apm/sessions/9a44cd2a-e60a-403e-bba2-214190c29d8b Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
apm install https://... no longer falls back to SSH on generic Git hosts
There was a problem hiding this comment.
Pull request overview
Aligns _validate_package_exists with the install clone path by making transport probing strict-by-default for explicit URL schemes on generic Git hosts, preventing misleading SSH fallbacks and improving verbose diagnostics.
Changes:
- Enforces strict transport probing for explicit
http://,https://, andssh://URLs on generic hosts, withAPM_ALLOW_PROTOCOL_FALLBACK=1to restore the legacy chain. - Improves verbose output by logging each
git ls-remoteattempt with scheme and sanitized stderr. - Adds/updates unit tests and records the behavior change in the changelog.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/install/validation.py |
Implements strict probe ordering for explicit schemes on generic hosts and logs per-attempt ls-remote results. |
tests/unit/install/test_validation_strict_transport.py |
New focused tests for strict transport selection + per-attempt verbose logging. |
tests/unit/test_install_command.py |
Updates existing generic-host validation tests to match the strict-by-default contract and env opt-in fallback. |
CHANGELOG.md |
Adds an Unreleased/Fixed entry documenting the behavior change and verbose logging improvements. |
Copilot's findings
Comments suppressed due to low confidence (1)
tests/unit/test_install_command.py:952
- This assertion
any("git@git.example.org:" in arg for arg in first_cmd)is a URL substring check and is likely to trigger CodeQL'spy/incomplete-url-substring-sanitization(the repo has prior fixes to avoid this). Prefer asserting the exact URL argument value for the SSH probe instead of substring matching.
first_cmd = mock_run.call_args_list[0][0][0]
assert any("git@git.example.org:" in arg for arg in first_cmd)
- Files reviewed: 4/4 changed files
- Comments generated: 2
| first_cmd = mock_run.call_args_list[0][0][0] | ||
| assert any("git@git.example.org:" in arg for arg in first_cmd), ( | ||
| f"Expected SSH URL in only call, got: {first_cmd}" | ||
| ) |
There was a problem hiding this comment.
Avoid substring-matching on an SSH URL argument ("git@git.example.org:" in arg). This pattern is flagged by CodeQL as py/incomplete-url-substring-sanitization in this repo. Prefer asserting exact equality to the expected SCP-style URL argument (or otherwise assert on a parsed/structured form) so CI doesn't fail on security checks.
This issue also appears on line 951 of the same file.
| import pytest | ||
|
|
There was a problem hiding this comment.
pytest is imported but not used in this test module. If linting (ruff/flake8) is enabled in CI, this will fail; please remove the unused import (or use it).
| import pytest |
APM Review Panel VerdictDisposition: APPROVE (with one minor pre-merge cleanup) Per-persona findingsPython Architect: This is a routine bug fix -- one functional change confined to OO / class diagram classDiagram
direction LR
class validation {
<<IOBoundary>>
+_validate_package_exists(package, verbose, auth_resolver, logger) bool
+_local_path_failure_reason(dep_ref) str
}
class DependencyReference {
<<ValueObject>>
+explicit_scheme: Optional[str]
+is_insecure: bool
+host: str
+repo_url: str
}
class TransportSelection {
<<Pure>>
+is_fallback_allowed(cli_flag, env) bool
}
class GitHubPackageDownloader {
+_build_repo_url(url, use_ssh, dep_ref, token) str
+_build_noninteractive_git_env(...) dict
+_sanitize_git_error(raw_stderr) str
}
class AuthResolver {
<<Strategy>>
+classify_host(host) HostInfo
+resolve_for_dep(dep_ref) AuthContext
}
class HostInfo {
<<ValueObject>>
+kind: str
}
validation ..> DependencyReference : reads explicit_scheme
validation ..> TransportSelection : reads is_fallback_allowed
validation ..> GitHubPackageDownloader : delegates URL build and env
validation ..> AuthResolver : resolves token per dep
class validation:::touched
classDef touched fill:#fff3b0,stroke:#d47600
Execution flow diagram flowchart TD
A([apm install pkg_url]) --> B["_validate_package_exists(package, verbose, auth_resolver)"]
B --> C{dep_ref.host\nclassification}
C -->|GitHub.com| D["[AUTH] unauth-first API probe"]
C -->|ADO| E["[AUTH] bearer/PAT probe"]
C -->|generic| F["[NET] read explicit_scheme\nfrom DependencyReference"]
F --> G["[NET] is_fallback_allowed()\nreads APM_ALLOW_PROTOCOL_FALLBACK"]
G --> H{explicit_scheme?}
H -->|http or https| I["urls_to_try = [https_url]\nor [https, ssh] if fallback_env"]
H -->|ssh or SCP git@| J["urls_to_try = [ssh_url]\nor [ssh, https] if fallback_env"]
H -->|None - shorthand| K["urls_to_try = [ssh_url, https_url]\nlegacy chain unchanged"]
I --> L["[EXEC] for probe_url in urls_to_try:\ngit ls-remote --heads probe_url"]
J --> L
K --> L
L --> M["[LOG] _log_attempt_result per attempt\n_sanitize_git_error + env var redaction"]
M --> N{returncode == 0?}
N -->|Yes| O([return True])
N -->|No - more attempts| L
N -->|No - all exhausted| P([return False])
Design patterns: Chain of Responsibility (probe loop) -- CLI Logging Expert: Clear improvement. Per-attempt DevX UX Expert: Core UX fix aligns APM with npm/pip/cargo/cargo mental model: give the tool an Supply Chain Security Expert: Net security improvement. The strict-by-default transport enforcement prevents a scenario where attacker-influenced SSH credentials could be used when the user explicitly stated HTTPS. Two-layer stderr sanitization is faithfully preserved in the per-attempt logger -- Auth Expert: Activated (fast-path: OSS Growth Hacker: "APM now respects the URL you type" -- a strong enterprise credibility signal. This fix directly unblocks the Bitbucket Data Center / on-prem GitLab user segment, which is a key enterprise adoption vector. The CHANGELOG entry is well-written and usable as release note material as-is. Side-channel to CEO: CEO arbitrationAll six specialists converge without meaningful disagreement: the fix is well-scoped, well-tested, architecturally consistent, and a net security improvement. The behavior change (explicit Required actions before merge
Optional follow-ups
|
Description
apm install https://corp-bitbucket.example/...was silently retrying SSH on port 22 when the HTTPS validation probe failed, masking the real HTTPS error (auth/redirect) behind a 30s SSH timeout. Verbose logging only surfaced the last attempt, so the misleading SSH error was all the user saw.The
_validate_package_existspre-check was the only remaining place still doing cross-protocol fallback for explicit URLs —_clone_with_fallback/TransportSelectorare already strict-by-default. This change brings validation in line.src/apm_cli/install/validation.py— strict probe order on generic hosts:http:///https://→ web URL onlyssh://(incl. SCP shorthandgit@host:org/repo) → SSH only[ssh, https]chainAPM_ALLOW_PROTOCOL_FALLBACK=1restores the legacy chain (mirrors clone)._sanitize_git_error, so the actual failing leg is visible:tests/unit/install/test_validation_strict_transport.py: explicit https/http/ssh strict mode, shorthand legacy chain, env opt-in, per-attempt verbose log assertions.tests/unit/test_install_command.py::TestGenericHostSshFirstValidationto the strict-by-default contract; added coverage for theAPM_ALLOW_PROTOCOL_FALLBACK=1opt-in.CHANGELOG.md— entry under## [Unreleased] / Fixed.Designed and ratified via the
apm-review-panelskill (Auth Expert + Python Architect + CLI Logging + DevX UX + Supply Chain Security + OSS Growth + CEO).Type of change
Testing