diff --git a/CHANGELOG.md b/CHANGELOG.md index d75203058..64ed5b606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Strict-by-default transport selection: explicit `ssh://`/`https://` URLs no longer silently fall back to the other protocol; shorthand consults `git config url..insteadOf` and otherwise defaults to HTTPS. Set `APM_ALLOW_PROTOCOL_FALLBACK=1` (or pass `--allow-protocol-fallback`) to restore the legacy permissive chain; cross-protocol retries then emit a `[!]` warning. Closes #328 (#778) +### Changed + +- `apm install --allow-protocol-fallback` now emits a one-shot `[!]` warning before the first clone attempt when a dependency carries a custom port and both SSH and HTTPS attempts are planned, naming the dependency and recommending either pinning the URL scheme (with fallback disabled) or dropping the flag to fail fast. Servers like Bitbucket Datacenter that serve SSH and HTTPS on different ports can otherwise hit a silent port mismatch. Closes #786 (#789) + ### Added - `apm install --ssh` / `--https` flags and `APM_GIT_PROTOCOL=ssh|https` env to pick the initial transport for shorthand dependencies (#778) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 03cd6e06a..60921106a 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -331,3 +331,15 @@ URL scheme honored exactly; shorthand uses HTTPS unless selection matrix, the `--ssh` / `--https` flags, the `APM_GIT_PROTOCOL` env var, and the `--allow-protocol-fallback` escape hatch, see [Dependencies: Transport selection](../../guides/dependencies/#transport-selection-ssh-vs-https). + +:::caution[Custom ports and cross-protocol fallback] +When `--allow-protocol-fallback` is in effect, APM reuses the +dependency URL's port on both SSH and HTTPS attempts. On servers that +use different ports per protocol (e.g. Bitbucket Datacenter: SSH 7999, +HTTPS 7990), the off-protocol URL will be wrong. APM emits a `[!]` +warning before the first clone attempt to flag this. To avoid +cross-protocol retries, leave `--allow-protocol-fallback` disabled +(strict mode) and pin the dependency with an explicit `ssh://...` or +`https://...` URL. If the flag is enabled, APM may still try the +other protocol even when the URL uses an explicit scheme. +::: diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 8f7c93c3f..6c14619ea 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -484,6 +484,19 @@ When fallback runs, each cross-protocol retry emits a `[!]` warning naming both protocols. Use this to unblock a pipeline while you fix the root cause -- not as a long-term setting. +:::caution[Cross-protocol fallback reuses the same port] +Fallback reuses the dependency's custom port for both schemes. On +servers that use different ports per protocol (e.g. Bitbucket +Datacenter: SSH 7999, HTTPS 7990), the off-protocol URL will be +wrong. APM emits a `[!]` warning before the first clone attempt when +a custom port is set and fallback is enabled. To avoid cross-protocol +retries entirely, leave `--allow-protocol-fallback` disabled (strict +mode) and pin the dependency with an explicit `ssh://...` or +`https://...` URL in `apm.yml`. If fallback is enabled, APM may still +try the other protocol even when the URL uses an explicit scheme -- +pinning only hard-stops cross-protocol retries in strict mode. +::: + For SSH key selection (ssh-agent, `~/.ssh/config`) and HTTPS token resolution, see [Authentication](../../getting-started/authentication/#choosing-transport-ssh-vs-https). diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 32546aa98..7984df799 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -98,7 +98,7 @@ apm install [PACKAGES...] [OPTIONS] - `-g, --global` - Install to user scope (`~/.apm/`) instead of the current project. Primitives deploy to `~/.copilot/`, `~/.claude/`, etc. MCP servers are only installed for global-capable runtimes (Copilot CLI, Codex CLI); workspace-only runtimes are skipped. - `--ssh` - Force SSH for shorthand (`owner/repo`) dependencies. Mutually exclusive with `--https`. Ignored for URLs with an explicit scheme. - `--https` - Force HTTPS for shorthand dependencies. Mutually exclusive with `--ssh`. Default unless `git config url..insteadOf` rewrites the candidate to SSH. -- `--allow-protocol-fallback` - Restore the legacy permissive cross-protocol fallback chain (HTTPS-then-SSH or vice-versa). Strict-by-default otherwise. Each retry emits a `[!]` warning naming both protocols. +- `--allow-protocol-fallback` - Restore the legacy permissive cross-protocol fallback chain (HTTPS-then-SSH or vice-versa). Strict-by-default otherwise. Each retry emits a `[!]` warning naming both protocols. When the dependency URL carries a custom port, APM also emits a one-shot `[!]` warning before the first clone attempt noting that the same port will be reused across schemes (wrong on servers like Bitbucket Datacenter that serve SSH and HTTPS on different ports) -- to avoid the mismatch, omit this flag and pin the dependency with an explicit `ssh://` or `https://` URL. **Transport env vars:** diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index bd4d1aa9a..dd7d87a48 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -413,7 +413,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo "allow_protocol_fallback", is_flag=True, default=False, - help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1).", + help="Restore the legacy permissive cross-protocol fallback chain (escape hatch for migrating users; also: APM_ALLOW_PROTOCOL_FALLBACK=1). Caveat: fallback reuses the same port across schemes; on servers that use different SSH and HTTPS ports, omit this flag and pin the dependency with an explicit ssh:// or https:// URL.", ) @click.pass_context def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbose, trust_transitive_mcp, parallel_downloads, dev, target, global_, use_ssh, use_https, allow_protocol_fallback): diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 642b45c0b..65cd05bbc 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -54,6 +54,15 @@ protocol_pref_from_env, ) +# Public docs anchor for the cross-protocol fallback caveat surfaced by the +# #786 warning. Lives under the dependencies guide, next to the canonical +# `--allow-protocol-fallback` section (Starlight site defined in +# docs/astro.config.mjs). +_PROTOCOL_FALLBACK_DOCS_URL = ( + "https://microsoft.github.io/apm/guides/dependencies/" + "#restoring-the-legacy-permissive-chain" +) + def normalize_collection_path(virtual_path: str) -> str: """Normalize a collection virtual path by stripping any existing extension. @@ -199,6 +208,11 @@ def __init__( self._allow_fallback = ( allow_fallback if allow_fallback is not None else is_fallback_allowed() ) + # Dedup set for the issue #786 cross-protocol port warning: one install + # run calls _clone_with_fallback multiple times per dep (ref-resolution + # clone, then the actual dep clone). We want the warning exactly once + # per (host, repo, port) identity across all those calls. + self._fallback_port_warned: set = set() def _setup_git_environment(self) -> Dict[str, Any]: """Set up Git environment with authentication using centralized token manager. @@ -739,6 +753,45 @@ def _env_for(use_token_attempt: bool) -> Dict[str, str]: f"strict={plan.strict}, attempts={[(a.scheme, a.use_token, a.label) for a in plan.attempts]}" ) + # Cross-protocol fallback reuses the dependency's port for every + # attempt. On servers that serve SSH and HTTPS on different ports + # (e.g. Bitbucket Datacenter: SSH 7999, HTTPS 7990), the off-protocol + # URL will be wrong. Warn once per dep, before the first attempt, so + # the user can pin the URL scheme (and leave fallback disabled) or + # fail fast by dropping --allow-protocol-fallback. See #786. + # A single install may call this method multiple times for the same + # dep (ref resolution + actual clone), so dedup on (host, repo, port). + dep_port = getattr(dep_ref, "port", None) if dep_ref else None + if ( + not plan.strict + and dep_port is not None + and any(a.scheme == "ssh" for a in plan.attempts) + and any(a.scheme == "https" for a in plan.attempts) + ): + # NOTE: dedup key is case-sensitive. GitHub/Bitbucket hostnames + # are case-insensitive per RFC, so "Example.com" and + # "example.com" dedup as distinct identities -- worst case is a + # duplicate warning. Follow-up issue tracks normalization. + warn_key = (dep_host, repo_url_base, dep_port) + if warn_key not in self._fallback_port_warned: + self._fallback_port_warned.add(warn_key) + initial_scheme = plan.attempts[0].scheme.upper() + fallback_scheme = next( + a.scheme.upper() + for a in plan.attempts + if a.scheme != plan.attempts[0].scheme + ) + host_display = dep_host or "host" + _rich_warning( + f"Custom port {dep_port} on {host_display}/{repo_url_base}: " + f"if {initial_scheme} fails, APM will retry over " + f"{fallback_scheme} on the same port.\n" + f" Pin the URL scheme, or drop " + f"--allow-protocol-fallback to fail fast.\n" + f" See: {_PROTOCOL_FALLBACK_DOCS_URL}", + symbol="warning", + ) + prev_label: Optional[str] = None prev_scheme: Optional[str] = None for attempt in plan.attempts: diff --git a/tests/unit/test_protocol_fallback_warning.py b/tests/unit/test_protocol_fallback_warning.py new file mode 100644 index 000000000..79102e19f --- /dev/null +++ b/tests/unit/test_protocol_fallback_warning.py @@ -0,0 +1,268 @@ +"""Unit tests for the cross-protocol-fallback port warning (issue #786). + +When the user opts into cross-protocol fallback via +``--allow-protocol-fallback`` / ``APM_ALLOW_PROTOCOL_FALLBACK=1`` AND a +dependency carries a custom port AND the plan attempts both SSH and +HTTPS, APM emits a ``[!]`` warning before the first clone attempt. The +same port is reused across schemes, which is incorrect for servers that +serve SSH and HTTPS on different ports (e.g. Bitbucket Datacenter: SSH +7999, HTTPS 7990). The warning names the offending dependency +(``{host}/{repo}``), names the planned initial + fallback schemes, +lists two remediations (pin the URL scheme, or drop +``--allow-protocol-fallback`` to fail fast), and links to the docs. +""" + +import os +import tempfile +from pathlib import Path +from unittest.mock import Mock, patch + +import pytest +from git.exc import GitCommandError + +from apm_cli.deps.github_downloader import GitHubPackageDownloader +from apm_cli.models.apm_package import DependencyReference + + +def _make_downloader(): + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ): + return GitHubPackageDownloader() + + +def _run_clone_capture_warnings(dep, allow_fallback=False): + """Run _clone_with_fallback and return every ``_rich_warning`` call. + + Returns a list of (message, symbol) tuples, one per call. Clones + always fail so any chained attempts run. + """ + dl = _make_downloader() + dl.auth_resolver._cache.clear() + if allow_fallback: + dl._allow_fallback = True + + def _fake_clone(url, *a, **kw): + raise GitCommandError("clone", "failed") + + captured = [] + + def _capture(message, symbol=None): + captured.append((message, symbol)) + + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), patch( + "apm_cli.deps.github_downloader.Repo" + ) as MockRepo, patch( + "apm_cli.deps.github_downloader._rich_warning", side_effect=_capture + ): + MockRepo.clone_from.side_effect = _fake_clone + target = Path(tempfile.mkdtemp()) + try: + dl._clone_with_fallback(dep.repo_url, target, dep_ref=dep) + except (RuntimeError, GitCommandError): + pass + finally: + import shutil + shutil.rmtree(target, ignore_errors=True) + return captured + + +class TestProtocolFallbackPortWarning: + """Issue #786: warn once per dep when fallback reuses a custom port.""" + + def _port_warnings(self, calls): + return [m for m, _s in calls if "Custom port" in m] + + def test_warning_fires_on_ssh_url_with_port_when_fallback_allowed(self): + """ssh:// URL with port + allow_fallback => plan has SSH and HTTPS => + exactly one warning naming the offender, both schemes, both + remediations, and the docs URL.""" + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + assert dep.port == 7999 + + calls = _run_clone_capture_warnings(dep, allow_fallback=True) + port_warnings = self._port_warnings(calls) + assert len(port_warnings) == 1, ( + f"expected exactly one port warning, got {len(port_warnings)}: {port_warnings!r}" + ) + msg = port_warnings[0] + # Assertions anchor on the emitted format ("Custom port ... on + # host/repo:" + "See: https://...") instead of bare host/URL + # substrings, so the test both documents the wire format and avoids + # CodeQL's "incomplete URL substring sanitization" false positive. + assert "Custom port 7999 on bitbucket.example.com/project/repo:" in msg, ( + f"warning must name the offender in 'Custom port {{port}} on " + f"{{host}}/{{repo}}:' form: {msg!r}" + ) + assert "SSH" in msg and "HTTPS" in msg, ( + f"warning must name both planned schemes: {msg!r}" + ) + assert "Pin the URL scheme" in msg, ( + f"warning must offer the 'pin the URL scheme' remediation: {msg!r}" + ) + assert "--allow-protocol-fallback" in msg, ( + f"warning must offer the 'drop --allow-protocol-fallback' escape " + f"hatch as an alternative: {msg!r}" + ) + assert "See: https://microsoft.github.io/apm/" in msg, ( + f"warning must link to the public docs via the 'See: ' prefix: {msg!r}" + ) + + def test_warning_fires_on_https_url_with_port_when_fallback_allowed(self): + """https:// URL with port + allow_fallback => plan has HTTPS and SSH => + warning fires, naming the offender and schemes.""" + dep = DependencyReference.parse( + "https://git.company.internal:8443/team/repo.git" + ) + assert dep.port == 8443 + + calls = _run_clone_capture_warnings(dep, allow_fallback=True) + port_warnings = self._port_warnings(calls) + assert len(port_warnings) == 1, ( + f"expected exactly one port warning, got: {port_warnings!r}" + ) + msg = port_warnings[0] + assert "Custom port 8443 on git.company.internal/team/repo:" in msg, ( + f"warning must name the offender in 'Custom port {{port}} on " + f"{{host}}/{{repo}}:' form: {msg!r}" + ) + assert "SSH" in msg and "HTTPS" in msg, ( + f"warning must name both planned schemes: {msg!r}" + ) + + def test_warning_silent_in_strict_mode_even_with_custom_port(self): + """Strict mode (default) only plans one attempt; the warning must not + fire even when a custom port is set. This is the whole point of + strict-by-default (#778).""" + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + assert dep.port == 7999 + + calls = _run_clone_capture_warnings(dep, allow_fallback=False) + port_warnings = self._port_warnings(calls) + assert port_warnings == [], ( + f"strict mode must not emit the port warning, got: {port_warnings!r}" + ) + + def test_warning_silent_when_no_port_set(self): + """Shorthand with no port => no warning, even under allow_fallback.""" + dep = DependencyReference.parse("owner/repo") + assert dep.port is None + + calls = _run_clone_capture_warnings(dep, allow_fallback=True) + port_warnings = self._port_warnings(calls) + assert port_warnings == [], ( + f"no-port deps must not emit the port warning, got: {port_warnings!r}" + ) + + def test_warning_fires_once_not_per_attempt(self): + """Regression guard: the warning must be emitted once per dep (before + the attempt loop), not per clone attempt in the plan.""" + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + calls = _run_clone_capture_warnings(dep, allow_fallback=True) + port_warnings = self._port_warnings(calls) + assert len(port_warnings) == 1, ( + f"warning must fire exactly once per dep, got {len(port_warnings)}" + ) + + def test_warning_dedup_across_multiple_clone_calls_for_same_dep(self): + """Regression guard: a single install run can call + ``_clone_with_fallback`` multiple times for the same dep (ref- + resolution clone, then the actual dep clone). The warning must still + fire only once per (host, repo, port) across all those calls.""" + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + dl = _make_downloader() + dl.auth_resolver._cache.clear() + dl._allow_fallback = True + + def _fake_clone(url, *a, **kw): + raise GitCommandError("clone", "failed") + + captured = [] + + def _capture(message, symbol=None): + captured.append((message, symbol)) + + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), patch( + "apm_cli.deps.github_downloader.Repo" + ) as MockRepo, patch( + "apm_cli.deps.github_downloader._rich_warning", side_effect=_capture + ): + MockRepo.clone_from.side_effect = _fake_clone + for _ in range(3): + target = Path(tempfile.mkdtemp()) + try: + dl._clone_with_fallback(dep.repo_url, target, dep_ref=dep) + except (RuntimeError, GitCommandError): + pass + finally: + import shutil + shutil.rmtree(target, ignore_errors=True) + + port_warnings = [m for m, _s in captured if "Custom port" in m] + assert len(port_warnings) == 1, ( + f"warning must dedup across repeated _clone_with_fallback calls " + f"for the same dep, got {len(port_warnings)}: {port_warnings!r}" + ) + + def test_warning_fires_again_for_different_dep(self): + """Dedup must be per-dep, not global: a second dep with a different + (host, repo, port) identity gets its own warning.""" + dep_a = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + dep_b = DependencyReference.parse( + "https://git.other.example:8443/team/repo.git" + ) + dl = _make_downloader() + dl.auth_resolver._cache.clear() + dl._allow_fallback = True + + def _fake_clone(url, *a, **kw): + raise GitCommandError("clone", "failed") + + captured = [] + + def _capture(message, symbol=None): + captured.append((message, symbol)) + + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), patch( + "apm_cli.deps.github_downloader.Repo" + ) as MockRepo, patch( + "apm_cli.deps.github_downloader._rich_warning", side_effect=_capture + ): + MockRepo.clone_from.side_effect = _fake_clone + for dep in (dep_a, dep_a, dep_b, dep_b): + target = Path(tempfile.mkdtemp()) + try: + dl._clone_with_fallback(dep.repo_url, target, dep_ref=dep) + except (RuntimeError, GitCommandError): + pass + finally: + import shutil + shutil.rmtree(target, ignore_errors=True) + + port_warnings = [m for m, _s in captured if "Custom port" in m] + assert len(port_warnings) == 2, ( + f"expected one warning per distinct dep identity (2), " + f"got {len(port_warnings)}: {port_warnings!r}" + ) + assert any("7999" in m for m in port_warnings), port_warnings + assert any("8443" in m for m in port_warnings), port_warnings