diff --git a/CHANGELOG.md b/CHANGELOG.md index 7693837b7..72b88484c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`apm install` works with your existing credential chain (SSO, EMU, GHES tokens).** Validation now uses the same credentials as the actual clone (PAT header-injected, then git credential helper, then SSH for explicit `#ref` pins) -- enterprise users whose env-var PAT has narrower SSO/EMU access than their `gh auth setup-git` / OS keychain are no longer false-rejected by the installer's API probe. Validation logic is now a separate module (`github_downloader_validation.py`), laying groundwork for future credential-provider extensibility. (#941) - **`shared/apm.md` single-credential-group runs no longer fail validation** with a spurious `missing APM bundles: apm-default` -- a normalisation step recreates the per-group subdir layout that `actions/download-artifact@v5+` flattens away. (#1051) - **`apm pack` works against GitHub Enterprise and other Git hosts** -- honors `GITHUB_HOST` for GHES auth and accepts GitHub / GHES / GitLab / Bitbucket / ADO / SSH URL forms. (#1008) - **ADO Entra ID auth no longer silently fails.** Bearer tokens from `az account get-access-token` are plumbed through, errors are typed + actionable (4-case diagnostic), and `apm install --update` pre-flights auth before touching files. (#1015) diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 52fed8138..eaad9ff64 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -20,6 +20,8 @@ Results are cached per-process — the same `(host, org)` pair is resolved once. All token-bearing requests use HTTPS. Tokens are never sent over unencrypted connections. +`apm install ` validation walks the same chain as the actual install: an authenticated attempt with the resolved token first, then a credential-helper fallback (plain HTTPS where the system credential helper provides the token). This means `apm install` from the CLI never rejects a package the lockfile-driven install would accept -- useful when an env-var PAT has narrower SSO/EMU access than the token your `gh auth setup-git` / OS keychain has cached. + ## Token lookup | Priority | Variable | Scope | Notes | diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index f8033fe67..82f6f98af 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -305,7 +305,7 @@ When you run `apm install`, APM automatically integrates primitives from install After installation completes, APM prints a grouped diagnostic summary instead of inline warnings. Categories include collisions (skipped files), cross-package skill replacements, warnings, and errors. - **Normal mode**: Shows counts and actionable tips (e.g., "9 files skipped -- use `apm install --force` to overwrite") -- **Verbose mode** (`--verbose`): Additionally lists individual file paths grouped by package, full error details, and **the resolved auth source per remote host** (e.g., `[i] dev.azure.com -- using bearer from az cli (source: AAD_BEARER_AZ_CLI)` or `[i] github.com -- token from GITHUB_APM_PAT`). Useful for diagnosing PAT vs. Entra-ID-bearer behaviour against Azure DevOps. +- **Verbose mode** (`--verbose`): Additionally lists individual file paths grouped by package, full error details, and **the resolved auth source per remote host** (e.g., `[i] dev.azure.com -- using bearer from az cli (source: AAD_BEARER_AZ_CLI)` or `[i] github.com -- token from GITHUB_APM_PAT`). Useful for diagnosing PAT vs. Entra-ID-bearer behaviour against Azure DevOps. For subdirectory packages with an explicit `#ref` (e.g. `owner/repo/sub#v1.2.0`), `--verbose` also shows each validation probe attempt -- marker-file lookups, the Contents API directory probe, and the `git ls-remote` fallback -- including which auth step (token, credential-helper, SSH) resolved the ref. ```bash # See exactly which files were skipped or had issues, and which auth source was used diff --git a/packages/apm-guide/.apm/skills/apm-usage/authentication.md b/packages/apm-guide/.apm/skills/apm-usage/authentication.md index 27a7d3f05..b957e396c 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/authentication.md +++ b/packages/apm-guide/.apm/skills/apm-usage/authentication.md @@ -117,6 +117,26 @@ export PROXY_REGISTRY_ONLY=1 # optional: proxy-only mode When `PROXY_REGISTRY_ONLY=1`, APM routes all traffic through the proxy and never contacts GitHub directly. +## Install validation chain + +`apm install ` validates a virtual subdirectory package (`owner/repo/path#ref`) before writing it to `apm.yml`. The chain mirrors the actual clone auth path so a credential that succeeds for `git clone` is never false-rejected by the installer: + +1. **Marker-file probes** via raw content -- `apm.yml`, `SKILL.md`, `plugin.json`, `README.md`. Fast positive signal; absence is not a failure. +2. **Contents API directory probe** -- `GET /repos/{owner}/{repo}/contents/{path}?ref={ref}`. Confirms the directory exists at the ref. +3. **`git ls-remote`** with the install auth chain (PAT header-injected, then plain HTTPS w/ credential helper, then SSH if `--ssh` or `--allow-protocol-fallback`). Confirms the ref exists. +4. **Shallow `git fetch --depth=1 --filter=tree:0` + `git ls-tree`** at the resolved ref -- the path probe that confirms the subdirectory exists at that ref. Required to close the fail-open hole where step 3 would otherwise pass any successful repo handshake. + +Steps 3 and 4 only run for explicit `#ref` pins (not for unpinned default-branch deps), and only when the API steps fail. Azure DevOps tokens (PAT or AAD bearer) are injected via `http.extraheader` (`Authorization: Bearer ...`) and never embedded in the clone URL. + +**Yellow signal:** when steps 1-2 fail and steps 3-4 succeed, APM emits a stderr warning -- `[!] API validation skipped for {pkg}; resolved via git credential fallback.` This is security-relevant: a scoped fine-grained PAT may have *correctly* rejected a package on the API surface and the broader git credential chain accepted it. Operators should be able to see that signal in default CI logs. + +**Terminal error** when all four steps fail: `[x] all probes failed (marker-file, Contents API, git ls-remote, shallow-fetch) -- verify the path and ref exist and that your credentials have read access (run with --verbose for the full probe log)`. + +```bash +# See the full probe log when validation fails +apm install --verbose owner/repo/path#v1.2.0 +``` + ## Troubleshooting ```bash diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 8e9e9f6c2..8edd6a8b6 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -21,6 +21,10 @@ | `apm deps clean` | Clean dependency cache | `--dry-run`, `-y` skip confirm | | `apm deps update [PKGS...]` | Update specific packages | `--verbose`, `--force`, `--target` (comma-separated), `--parallel-downloads N` | +### Install validation chain (virtual subdirectory packages) + +`apm install` validates subdirectory packages (`owner/repo/path#ref`) before writing to `apm.yml` using the same credential chain as the actual install. See [Authentication > Install validation chain](../authentication/) for the full probe sequence and troubleshooting. + ## Compilation | Command | Purpose | Key flags | diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index c0228b6f0..cbd0c4a77 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -421,9 +421,29 @@ def _resolve_package_references( else: reason = _local_path_failure_reason(dep_ref) if not reason: - reason = "not accessible or doesn't exist" - if not verbose: - reason += " -- run with --verbose for auth details" + # Round-4 panel fix (devx-ux): name the four-step probe + # chain explicitly when the validator exhausted it + # (virtual subdirectory + explicit ref). Generic "not + # accessible" hides the failure mode for the precise + # case where the most diagnostics are available. + is_subdir_ref_chain = ( + dep_ref.is_virtual + and dep_ref.is_virtual_subdirectory() + and bool(dep_ref.reference) + ) + if is_subdir_ref_chain: + reason = ( + "all probes failed (marker-file, Contents API, " + "git ls-remote, shallow-fetch) -- verify the path " + "and ref exist and that your credentials have " + "read access" + ) + if not verbose: + reason += " (run with --verbose for the full probe log)" + else: + reason = "not accessible or doesn't exist" + if not verbose: + reason += " -- run with --verbose for auth details" invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index c7b7a22ea..a1aaa5ef6 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -8,7 +8,7 @@ import sys import tempfile import time # noqa: F401 -from collections.abc import Callable # noqa: F401 +from collections.abc import Callable from datetime import datetime from pathlib import Path from typing import Any, Dict, List, Optional, Union # noqa: F401, UP035 @@ -1235,92 +1235,61 @@ def _download_github_file( verbose_callback=verbose_callback, ) - def validate_virtual_package_exists(self, dep_ref: DependencyReference) -> bool: - """Validate that a virtual package (file, collection, or subdirectory) exists on GitHub. - - Supports: - - Virtual files: owner/repo/path/file.prompt.md - - Collections: owner/repo/collections/name (checks for .collection.yml) - - Subdirectory packages: owner/repo/path/subdir (checks for apm.yml, SKILL.md, or plugin.json) - - Args: - dep_ref: Parsed dependency reference for virtual package + def validate_virtual_package_exists( + self, + dep_ref: DependencyReference, + verbose_callback: Callable[[str], None] | None = None, + warn_callback: Callable[[str], None] | None = None, + ) -> bool: + """Validate that a virtual package exists at ``dep_ref``. - Returns: - bool: True if the package exists and is accessible, False otherwise + Thin delegation to :func:`github_downloader_validation.validate_virtual_package_exists` + -- see that module for the full validation strategy (marker-file + probes, Contents API directory probe, ``git ls-remote`` fallback). """ - if not dep_ref.is_virtual: - raise ValueError("Can only validate virtual packages with this method") + from .github_downloader_validation import validate_virtual_package_exists as _v - ref = dep_ref.reference or "main" - file_path = dep_ref.virtual_path + return _v( + self, + dep_ref, + verbose_callback=verbose_callback, + warn_callback=warn_callback, + ) - # For collections, check for .collection.yml file - if dep_ref.is_virtual_collection(): - file_path = f"{dep_ref.virtual_path}.collection.yml" - try: - self.download_raw_file(dep_ref, file_path, ref) - return True - except RuntimeError: - return False + def _directory_exists_at_ref( + self, + dep_ref: DependencyReference, + path: str, + ref: str, + log: Callable[[str], None], + ) -> bool: + """Backward-compat shim -- delegates to the validation module.""" + from .github_downloader_validation import _directory_exists_at_ref as _impl - # For virtual files, check the file directly - if dep_ref.is_virtual_file(): - try: - self.download_raw_file(dep_ref, file_path, ref) - return True - except RuntimeError: - return False - - # For subdirectory packages: apm.yml or SKILL.md confirm the type; - # plugin.json confirms a Claude plugin; README.md is a last-resort - # signal that the directory exists (any directory that follows the - # Claude plugin spec may have none of the above). - if dep_ref.is_virtual_subdirectory(): - # Try apm.yml first - try: - self.download_raw_file(dep_ref, f"{dep_ref.virtual_path}/apm.yml", ref) - return True - except RuntimeError: - pass + return _impl(self, dep_ref, path, ref, log) - # Try SKILL.md - try: - self.download_raw_file(dep_ref, f"{dep_ref.virtual_path}/SKILL.md", ref) - return True - except RuntimeError: - pass + def _ref_exists_via_ls_remote( + self, + dep_ref: DependencyReference, + ref: str, + log: Callable[[str], None], + ) -> bool: + """Backward-compat shim -- delegates to the validation module. - # Try plugin.json at various plugin locations - plugin_locations = [ - f"{dep_ref.virtual_path}/plugin.json", # Root - f"{dep_ref.virtual_path}/.github/plugin/plugin.json", # GitHub Copilot format - f"{dep_ref.virtual_path}/.claude-plugin/plugin.json", # Claude format - f"{dep_ref.virtual_path}/.cursor-plugin/plugin.json", # Cursor format - ] + Returns ``bool`` (success only); the underlying impl now also + returns the winning AttemptSpec, but legacy callers only need + the success flag. + """ + from .github_downloader_validation import _ref_exists_via_ls_remote as _impl - for plugin_path in plugin_locations: - try: - self.download_raw_file(dep_ref, plugin_path, ref) - return True - except RuntimeError: - continue + ok, _winning = _impl(self, dep_ref, ref, log) + return ok - # Last resort: README.md -- any well-formed directory should have one. - # A directory that follows the Claude plugin spec (agents/, commands/, - # skills/ ...) with no manifest files is still a valid plugin. - try: - self.download_raw_file(dep_ref, f"{dep_ref.virtual_path}/README.md", ref) - return True - except RuntimeError: - pass + def _ssh_attempt_allowed(self) -> bool: + """Backward-compat shim -- delegates to the validation module.""" + from .github_downloader_validation import _ssh_attempt_allowed as _impl - # Fallback: try to download the file directly - try: - self.download_raw_file(dep_ref, file_path, ref) - return True - except RuntimeError: - return False + return _impl(self) def download_virtual_file_package( self, diff --git a/src/apm_cli/deps/github_downloader_validation.py b/src/apm_cli/deps/github_downloader_validation.py new file mode 100644 index 000000000..4d9ac1a5a --- /dev/null +++ b/src/apm_cli/deps/github_downloader_validation.py @@ -0,0 +1,542 @@ +"""Virtual-package validation helpers for ``GitHubPackageDownloader``. + +Extracted from ``github_downloader.py`` to keep the downloader module under +the repo's 2400-line cap. These helpers were added by PR #941 to align +``apm install`` validation with the actual install auth chain (Contents +API directory probe + ``git ls-remote`` fallback) so subdirectory +packages with an explicit ``#ref`` no longer false-fail when the API +token is narrower than the user's git credential helper. + +The helpers are module-level functions taking the downloader instance as +the first argument; the public class still exposes +``validate_virtual_package_exists`` as a thin delegating method so test +mocks (``patch("...GitHubPackageDownloader.validate_virtual_package_exists")``) +keep working unchanged. + +Security gates (round-2 panel findings) +--------------------------------------- +* ``validate_path_segments`` is invoked at the entry point on the + user-supplied ``virtual_path`` before any URL interpolation, blocking + ``..`` traversal segments from leaking into Contents API or archive + URLs. +* The ``ls-remote`` fallback no longer fails open: a successful + ``ls-remote`` only proves the *ref* exists, so we additionally + shallow-fetch + ``ls-tree`` to confirm ``vpath`` resolves at that ref + before returning ``True``. +* For Azure DevOps, credentials (PAT or AAD bearer) are injected via + ``http.extraheader`` (see ``build_authorization_header_git_env``) and + never embedded in the clone URL. This keeps tokens out of the OS + process table, git's own logs, and any downstream debug output. +""" + +from __future__ import annotations + +import base64 +import contextlib +import re +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import TYPE_CHECKING, NamedTuple + +import git +import requests +from git.exc import GitCommandError + +from ..config import get_apm_temp_dir +from ..utils.github_host import ( + build_authorization_header_git_env, + default_host, + is_github_hostname, +) +from ..utils.path_security import ( + PathTraversalError, + safe_rmtree, + validate_path_segments, +) + +if TYPE_CHECKING: + from ..models.dependency.reference import DependencyReference + from .github_downloader import GitHubPackageDownloader + + +_SHA_RE = re.compile(r"[0-9a-fA-F]{7,40}") + + +class AttemptSpec(NamedTuple): + """A single (label, url, env) attempt in the auth-chain.""" + + label: str + url: str + env: dict + + +def _is_sha_pin(ref: str) -> bool: + """Return True when ``ref`` looks like an abbreviated or full git SHA.""" + return bool(_SHA_RE.fullmatch(ref)) + + +def _split_owner_repo(repo_url: str) -> tuple[str, str] | None: + """Split ``owner/repo`` safely; return ``None`` if the shape is wrong. + + Guards against ``ValueError`` on tuple-unpacking when ``repo_url`` + has no ``/`` (panel round-2 finding 2). + """ + parts = repo_url.split("/", 1) + if len(parts) != 2 or not parts[0] or not parts[1]: + return None + return parts[0], parts[1] + + +def validate_virtual_package_exists( + downloader: GitHubPackageDownloader, + dep_ref: DependencyReference, + verbose_callback: Callable[[str], None] | None = None, + warn_callback: Callable[[str], None] | None = None, +) -> bool: + """Validate that a virtual package exists at ``dep_ref``. + + Supports virtual files, collections, and subdirectory packages. For + subdirectory packages the marker-file probes are a fast positive + signal; their absence is not a failure -- two fallbacks (Contents API + directory probe, then ``git ls-remote`` + shallow-fetch path probe + mirroring the install auth chain) catch packages whose API auth is + stricter than their git auth. See PR #941 for the auth-alignment + rationale. + + Args: + downloader: The ``GitHubPackageDownloader`` instance providing + transport, auth, and helper methods. + dep_ref: Parsed dependency reference for the virtual package. + verbose_callback: Optional per-probe log callback (verbose mode). + warn_callback: Optional non-verbose warning callback. Fired + when the ls-remote + shallow-fetch fallback resolves both + the ref and the path. Yellow-traffic-light signal: the + git-credential chain validated a package the API check + could not, which may indicate a credential-scope mismatch + an operator must see in default-verbosity CI runs. + + Returns: + True if the package exists / is accessible, False otherwise. + """ + if not dep_ref.is_virtual: + raise ValueError("Can only validate virtual packages with this method") + + ref: str = dep_ref.reference or "main" + vpath: str = dep_ref.virtual_path + + # SECURITY (round-2 finding 7 + round-3 finding 2): reject traversal + # segments before any URL interpolation, and reject empty vpath + # outright. Empty vpath is not a traversal but `git ls-tree + # FETCH_HEAD ""` is implementation-defined; some git versions emit a + # root listing and falsely validate any successfully-fetched repo. + # `reject_empty=True` closes that hole at the entry point. + try: + validate_path_segments(vpath, context="virtual path", reject_empty=True) + except PathTraversalError as exc: + if verbose_callback: + verbose_callback(f" [x] virtual path rejected: {exc}") + return False + + def _log(msg: str) -> None: + if verbose_callback: + verbose_callback(msg) + + def _probe(path: str) -> bool: + try: + downloader.download_raw_file(dep_ref, path, ref) + _log(f" [+] {path}@{ref}") + return True + except RuntimeError as exc: + # Marker-file misses on the success path are expected, not + # errors -- reserve [x] for genuine failures. + _log(f" [i] {path}@{ref} ({exc})") + return False + + _log(f' [i] Validating virtual package at ref "{ref}": {dep_ref.repo_url}/{vpath}') + + if dep_ref.is_virtual_collection(): + return _probe(f"{vpath}.collection.yml") + + if dep_ref.is_virtual_file(): + return _probe(vpath) + + if dep_ref.is_virtual_subdirectory(): + marker_paths = [ + f"{vpath}/apm.yml", + f"{vpath}/SKILL.md", + f"{vpath}/plugin.json", + f"{vpath}/.github/plugin/plugin.json", + f"{vpath}/.claude-plugin/plugin.json", + f"{vpath}/.cursor-plugin/plugin.json", + f"{vpath}/README.md", + ] + for marker_path in marker_paths: + if _probe(marker_path): + return True + + # Fallback 1: directory-exists probe via Contents API. + if _directory_exists_at_ref(downloader, dep_ref, vpath, ref, _log): + return True + + # Fallback 2: explicit ref + git ls-remote + shallow-fetch path + # probe. Mirrors install's auth chain so we accept packages + # whose API auth is stricter than their git auth. Only kicks in + # with an explicit, NON-EMPTY ref -- without one, strict + # validation keeps path typos failing fast on the default + # branch. Round-3 finding 1: a bare `#` fragment produces + # `reference == ""`, which `is not None` would let through; the + # truthy check below rejects it so the fallback is reachable + # only for explicitly-pinned refs. + if dep_ref.reference: + ref_ok, winning_attempt = _ref_exists_via_ls_remote(downloader, dep_ref, ref, _log) + if ref_ok and winning_attempt is not None: + # SECURITY (round-2 finding 6): close the fail-open. ls-remote + # only confirms the ref exists; we MUST also confirm the + # subdirectory exists at that ref via a shallow-fetch + + # ls-tree probe, otherwise a typo'd vpath silently passes + # validation. Reuse the WINNING attempt (panel round-3 + # auth-chain bug fix) so we don't fall back to attempts[0]. + if _path_exists_in_tree_at_ref( + downloader, dep_ref, vpath, ref, _log, winning_attempt + ): + _log(f' [+] "{vpath}@{ref}" confirmed via shallow-fetch + ls-tree') + if warn_callback is not None: + # devx-ux + cli-logging (round-3): name the + # security-relevant outcome explicitly. A scoped + # PAT may have *correctly* rejected this package + # on the API surface; the operator must be able + # to distinguish that from a legitimate API hit. + warn_callback( + f"API validation skipped for {dep_ref.to_canonical()}; " + "resolved via git credential fallback. " + "Run with --verbose for details." + ) + return True + _log( + f' [!] ref "{ref}" resolves but "{vpath}" not present in the tree at that ref' + ) + return False + return False + + return _probe(vpath) + + +def _directory_exists_at_ref( + downloader: GitHubPackageDownloader, + dep_ref: DependencyReference, + path: str, + ref: str, + log: Callable[[str], None], +) -> bool: + """Check if a directory exists at ``ref`` via the Contents API. + + Uses the default ``Accept: application/vnd.github+json`` so the + endpoint returns the directory listing for directories (and file + metadata for files). A 200 means the path resolves at the ref, + which is what install needs. + + Returns ``True`` on 200; ``False`` on 404 or any error. Only + implemented for github.com / GHE; non-GitHub hosts return ``False`` + and rely on the marker-file probes above. + """ + host: str = dep_ref.host or default_host() + if dep_ref.is_azure_devops() or not is_github_hostname(host): + log(f" [i] directory-exists probe skipped (host {host} not supported)") + return False + + parts = _split_owner_repo(dep_ref.repo_url) + if parts is None: + log(f" [x] repo_url '{dep_ref.repo_url}' missing owner/repo split") + return False + owner, repo = parts + token = downloader.auth_resolver.resolve_for_dep(dep_ref).token + + from urllib.parse import quote + + encoded_path = quote(path, safe="/") + encoded_ref = quote(ref, safe="") + + host_lc = host.lower() + if host_lc == "github.com": + api_url = ( + f"https://api.github.com/repos/{owner}/{repo}/contents/{encoded_path}?ref={encoded_ref}" + ) + elif host_lc.endswith(".ghe.com"): + api_url = ( + f"https://api.{host}/repos/{owner}/{repo}/contents/{encoded_path}?ref={encoded_ref}" + ) + else: + api_url = ( + f"https://{host}/api/v3/repos/{owner}/{repo}/contents/{encoded_path}?ref={encoded_ref}" + ) + + headers: dict[str, str] = {"Accept": "application/vnd.github+json"} + if token: + headers["Authorization"] = f"token {token}" + + try: + response = downloader._resilient_get(api_url, headers=headers, timeout=30) + if response.status_code == 200: + log(f" [+] {path}@{ref} (directory)") + return True + # 404 is the expected "not present at this ref" outcome -- the + # marker-file fallback path treats this as informational, not + # an error. Reserve [x] for unexpected HTTP statuses. + if response.status_code == 404: + log(f" [i] {path}@{ref} (HTTP 404)") + else: + log(f" [x] {path}@{ref} (HTTP {response.status_code})") + return False + except (requests.exceptions.RequestException, RuntimeError) as exc: + log(f" [x] {path}@{ref} ({exc})") + return False + + +def _build_validation_attempts( + downloader: GitHubPackageDownloader, + dep_ref: DependencyReference, + log: Callable[[str], None], +) -> list[AttemptSpec]: + """Return the AttemptSpec chain for a probe against ``dep_ref``. + + Mirrors the auth chain in ``_clone_with_fallback`` and centralises the + header-injection switch so both ``ls-remote`` and the shallow-fetch + path probe reuse it. + + SECURITY (panel round-3 finding): for ALL HTTPS attempts (ADO and + non-ADO) we inject credentials via ``http.extraheader`` rather than + embedding them in the URL. This keeps tokens out of the OS process + table, git's own logs, and any temp ``.git/config`` written by the + shallow-fetch probe. + + Auth scheme handling (panel round-3 ADO Basic finding): + * ADO + ``auth_scheme == "basic"`` (PAT): ``Authorization: Basic + base64(":" + PAT)`` per ADO's HTTP Basic convention. A raw + ``Bearer `` is rejected with 401. + * ADO + ``auth_scheme == "bearer"`` (AAD JWT): ``Authorization: + Bearer ``. + * Non-ADO with ``auth_scheme == "bearer"``: ``Authorization: Bearer + `` (matches GitHub recommendation for OAuth/App tokens). + * Non-ADO with ``auth_scheme == "basic"`` (legacy classic PAT): + ``Authorization: Bearer `` -- GitHub accepts both forms; + Bearer keeps the token out of any URL component. + """ + if dep_ref.is_artifactory(): + return [] + + dep_token: str | None = downloader._resolve_dep_token(dep_ref) + dep_auth_ctx = downloader._resolve_dep_auth_ctx(dep_ref) + dep_auth_scheme: str = dep_auth_ctx.auth_scheme if dep_auth_ctx else "basic" + is_insecure: bool = bool(getattr(dep_ref, "is_insecure", False)) + is_ado: bool = dep_ref.is_azure_devops() + + attempts: list[AttemptSpec] = [] + + # Attempt 1: explicit token, header-injected. Skipped when no token. + if dep_token: + if is_ado and dep_auth_scheme == "basic": + # ADO PAT requires HTTP Basic with base64(":PAT"). A raw + # Bearer header would 401 every ADO PAT user. + encoded = base64.b64encode(f":{dep_token}".encode()).decode("ascii") + auth_env = build_authorization_header_git_env("Basic", encoded) + label = "ADO authenticated HTTPS (basic header)" + elif is_ado: # bearer (AAD JWT) + auth_env = build_authorization_header_git_env("Bearer", dep_token) + label = "ADO authenticated HTTPS (bearer header)" + else: + # Non-ADO: header injection rather than URL embedding so the + # token never appears in argv or temp .git/config. + auth_env = build_authorization_header_git_env("Bearer", dep_token) + label = "authenticated HTTPS (header)" + + token_env = {**downloader.git_env, **auth_env} + token_url = downloader._build_repo_url( + dep_ref.repo_url, + use_ssh=False, + dep_ref=dep_ref, + token="", # tokenless URL: credentials live in the env header + auth_scheme=dep_auth_scheme if is_ado else "basic", + ) + attempts.append(AttemptSpec(label, token_url, token_env)) + + # Attempt 2: plain HTTPS w/ credential helper (no token, no header). + plain_env = downloader._build_noninteractive_git_env( + preserve_config_isolation=is_insecure, + suppress_credential_helpers=is_insecure, + ) + plain_url = downloader._build_repo_url( + dep_ref.repo_url, + use_ssh=False, + dep_ref=dep_ref, + token="", + ) + attempts.append(AttemptSpec("plain HTTPS w/ credential helper", plain_url, plain_env)) + + # Attempt 3 (SSH): only when allowed. StrictHostKeyChecking is + # intentionally inherited from the user's ssh config; do NOT add + # `-o StrictHostKeyChecking=no` thinking it's safer -- it isn't. + if not is_insecure and _ssh_attempt_allowed(downloader): + try: + ssh_url = downloader._build_repo_url( + dep_ref.repo_url, + use_ssh=True, + dep_ref=dep_ref, + ) + ssh_env = dict(plain_env) + ssh_env["GIT_SSH_COMMAND"] = "ssh -o BatchMode=yes -o ConnectTimeout=10" + attempts.append(AttemptSpec("SSH", ssh_url, ssh_env)) + except Exception as exc: + log(f" [!] SSH URL build skipped: {exc}") + + return attempts + + +def _ref_exists_via_ls_remote( + downloader: GitHubPackageDownloader, + dep_ref: DependencyReference, + ref: str, + log: Callable[[str], None], +) -> tuple[bool, AttemptSpec | None]: + """Check if ``ref`` exists in the remote repo via ``git ls-remote``. + + Lenient fallback for when the Contents API rejects a path with 404 + even though ``git clone`` would succeed -- e.g. SSO-half-authorized + PATs, fine-grained PAT scope mismatches between API and git + protocols, or repo policies that gate the Contents API more + strictly than git. + + For SHA-pinned refs (hex-only, 7-40 chars) the ls-remote call omits + ``--heads --tags`` because those filters silently drop commit SHAs + -- the full ref list is scanned for a SHA-prefix match instead. + + Returns: + ``(True, winning_attempt)`` on the first attempt that resolves + the ref; ``(False, None)`` if every attempt fails. Callers MUST + reuse ``winning_attempt`` for any follow-up probe at the same + ref so the auth-chain promise holds end-to-end (panel round-3: + if ls-remote succeeded via SSH but the follow-up probe used the + rejected PAT, the fallback would silently false-reject). + """ + attempts = _build_validation_attempts(downloader, dep_ref, log) + if not attempts: + return False, None + + is_sha = _is_sha_pin(ref) + ref_lc = ref.lower() + g = git.cmd.Git() + for attempt in attempts: + label, url, env = attempt + try: + if is_sha: + # SHA pins: scan the full advertised-refs list. The + # ``--heads --tags`` filters scan only ``refs/heads/*`` + # and ``refs/tags/*`` and silently drop commit SHAs. + output = g.ls_remote(url, env=env) + if output and any( + line.split("\t", 1)[0].lower().startswith(ref_lc) + for line in output.splitlines() + if line + ): + log(f" [+] ls-remote ok via {label}") + return True, attempt + log(f" [!] ls-remote returned no SHA match via {label}") + else: + output = g.ls_remote("--heads", "--tags", url, ref, env=env) + if output and output.strip(): + log(f" [+] ls-remote ok via {label}") + return True, attempt + log(f" [!] ls-remote returned no matching refs via {label}") + except (GitCommandError, OSError) as exc: + log(f" [x] ls-remote failed via {label}: {downloader._sanitize_git_error(str(exc))}") + + return False, None + + +def _path_exists_in_tree_at_ref( + downloader: GitHubPackageDownloader, + dep_ref: DependencyReference, + vpath: str, + ref: str, + log: Callable[[str], None], + winning_attempt: AttemptSpec, +) -> bool: + """Confirm ``vpath`` exists at ``ref`` via shallow fetch + ``ls-tree``. + + Closes the fail-open hole in ``_ref_exists_via_ls_remote``: knowing + that the ref exists is not the same as knowing the subdirectory + exists at that ref. This helper initialises a temporary bare repo, + fetches a single commit with ``--filter=tree:0`` (no blob bodies, + cheap), and then runs ``ls-tree`` to assert the path is present in + the resolved tree. Cleans up the temp dir regardless of outcome. + + Args: + winning_attempt: The AttemptSpec returned by + ``_ref_exists_via_ls_remote`` -- MUST be reused so the same + credential that proved the ref exists is the one used to + fetch the tree. Panel round-3 closed the + ``attempts[0]``-only bug here. + + Returns: + True iff the shallow fetch succeeded AND ``ls-tree`` reported + at least one entry for ``vpath`` at the resolved ref. + """ + label, url, env = winning_attempt + + base_temp = get_apm_temp_dir() + tmpdir = Path(tempfile.mkdtemp(prefix="apm-validate-", dir=base_temp)) + try: + bare = tmpdir / "probe.git" + bare.mkdir() + g = git.cmd.Git(str(bare)) + try: + g.init("--bare") + g.remote("add", "origin", url) + # --filter=tree:0 keeps the fetch payload tiny: we get the + # commit + a single tree object, no blob contents. + g.fetch( + "--depth=1", + "--filter=tree:0", + "origin", + ref, + env=env, + ) + except (GitCommandError, OSError) as exc: + log( + f" [x] shallow fetch failed via {label}: " + f"{downloader._sanitize_git_error(str(exc))}" + ) + return False + + try: + output = g.ls_tree("FETCH_HEAD", vpath, env=env) + except (GitCommandError, OSError) as exc: + log(f" [x] ls-tree failed via {label}: {downloader._sanitize_git_error(str(exc))}") + return False + + if output and output.strip(): + log(f" [+] {vpath}@{ref} present in tree") + return True + log(f" [!] {vpath} not present in tree at {ref}") + return False + finally: + # safe_rmtree wraps robust_rmtree with an ensure_path_within + # containment assertion -- never call robust_rmtree directly. + with contextlib.suppress(Exception): + safe_rmtree(tmpdir, base_temp) + + +def _ssh_attempt_allowed(downloader: GitHubPackageDownloader) -> bool: + """Whether the SSH ls-remote attempt should run. + + Mirrors ``_clone_with_fallback``'s gating: SSH is in scope when the + user explicitly preferred it (``--ssh``) or when cross-protocol + fallback is allowed. Default HTTPS-preferring users get no SSH + attempt -- keeps validation output clean and never invokes ssh on + machines that don't have it configured. + """ + try: + from .transport_selection import ProtocolPreference + except ImportError: + return False + return downloader._protocol_pref == ProtocolPreference.SSH or downloader._allow_fallback diff --git a/src/apm_cli/install/validation.py b/src/apm_cli/install/validation.py index 0ff034adb..fae11abe5 100644 --- a/src/apm_cli/install/validation.py +++ b/src/apm_cli/install/validation.py @@ -27,7 +27,7 @@ import requests -from ..utils.console import _rich_echo, _rich_info +from ..utils.console import _rich_echo, _rich_info, _rich_warning from ..utils.github_host import default_host from .errors import AuthenticationError @@ -189,7 +189,38 @@ def _validate_package_exists(package, verbose=False, auth_resolver=None, logger= f"Auth resolved: host={host}, org={org}, source={ctx.source}, type={ctx.token_type}" ) virtual_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) - result = virtual_downloader.validate_virtual_package_exists(dep_ref) + + def _warn(msg: str) -> None: + # Round-4 panel fix (cli-logging + devx-ux converge): + # * Yellow warnings MUST reach the user in BOTH + # verbose and non-verbose modes -- the git-fallback + # signal is security-relevant (a scoped PAT may + # have correctly rejected the package on the API + # surface and the broader git-credential chain + # accepted it). Operators must see this in default + # CI logs. + # * Strip the "Run with --verbose for details." + # suffix only when --verbose is already set; the + # suffix is meaningful only when it tells the user + # a follow-up is available. + # * Fall back to ``_rich_warning`` when ``logger`` is + # None so production callers without a + # CommandLogger still emit the yellow signal -- + # comments are not enforcement. + display = msg + verbose_suffix = " Run with --verbose for details." + if verbose and msg.endswith(verbose_suffix): + display = msg[: -len(verbose_suffix)] + if logger: + logger.warning(display) + else: + _rich_warning(display) + + result = virtual_downloader.validate_virtual_package_exists( + dep_ref, + verbose_callback=verbose_log, + warn_callback=_warn, + ) if not result and verbose_log: try: err_ctx = auth_resolver.build_error_context( diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index b3dd2d87c..bfe7a1ab4 100644 --- a/tests/test_github_downloader.py +++ b/tests/test_github_downloader.py @@ -1879,5 +1879,272 @@ def _fake_download(dep_ref_arg, path, ref): assert parsed["tags"] == ["scope: engineering", "plain-tag"] +class TestRefExistsViaLsRemote: + """Tests for the ``_ref_exists_via_ls_remote`` two/three-attempt chain. + + The chain mirrors ``_clone_with_fallback``'s auth path so validation + accepts what install would actually clone. These tests pin that + behavior so a refactor of the auth chain can't silently regress + validation lenience for users with SSO-half-authorized PATs or + SSH-only setups. + """ + + def _make_dep_ref(self, repo: str = "owner/repo") -> DependencyReference: + return DependencyReference(repo_url=repo) + + def _patch_auth(self, downloader, *, has_token: bool): + """Stub out auth resolution so tests don't hit the real env / git.""" + token = "test-token" if has_token else None + return [ + patch.object(downloader, "_resolve_dep_token", return_value=token), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=None), + patch.object(downloader, "_build_repo_url", return_value="https://example/repo.git"), + ] + + def _enter(self, ctxs): + return [c.__enter__() for c in ctxs] + + def _exit(self, ctxs): + for c in reversed(ctxs): + c.__exit__(None, None, None) + + def test_first_attempt_with_token_succeeds_short_circuits(self): + """When the authenticated HTTPS attempt resolves the ref, no second attempt fires.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=True) + self._enter(ctxs) + try: + ls_remote_mock = MagicMock(return_value="abc123\trefs/heads/main\n") + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = ls_remote_mock + + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ok is True + assert ls_remote_mock.call_count == 1 + finally: + self._exit(ctxs) + + def test_authenticated_403_falls_back_to_credential_helper(self): + """403 on the PAT attempt MUST trigger the plain-HTTPS attempt.""" + from git.exc import GitCommandError + + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=True) + self._enter(ctxs) + try: + calls = [] + + def _ls_remote(*args, **kwargs): + calls.append(args) + if len(calls) == 1: + raise GitCommandError( + ["git", "ls-remote"], + 128, + b"403", + b"Write access not granted", + ) + return "deadbeef\trefs/heads/main\n" + + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = _ls_remote + + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ok is True + assert len(calls) == 2 + finally: + self._exit(ctxs) + + def test_no_token_skips_first_attempt(self): + """Without a resolved token, only the credential-helper attempt should run.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=False) + self._enter(ctxs) + try: + ls_remote_mock = MagicMock(return_value="abc\trefs/heads/main\n") + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = ls_remote_mock + + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ok is True + assert ls_remote_mock.call_count == 1 + finally: + self._exit(ctxs) + + def test_all_attempts_fail_returns_false(self): + """If every attempt errors, the helper returns False (validation rejects).""" + from git.exc import GitCommandError + + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=True) + self._enter(ctxs) + try: + + def _always_fail(*args, **kwargs): + raise GitCommandError(["git", "ls-remote"], 128, b"403", b"forbidden") + + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = _always_fail + + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "loo", + log=lambda _msg: None, + ) + + assert ok is False + finally: + self._exit(ctxs) + + def test_empty_output_means_ref_not_found(self): + """ls-remote returning no matching refs MUST be treated as a miss, not a hit.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=False) + self._enter(ctxs) + try: + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = MagicMock(return_value=" \n ") + + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "missing", + log=lambda _msg: None, + ) + + assert ok is False + finally: + self._exit(ctxs) + + def test_artifactory_dep_short_circuits_without_calling_git(self): + """Artifactory deps have no git surface; helper must not invoke ls-remote.""" + downloader = GitHubPackageDownloader() + dep_ref = DependencyReference( + repo_url="owner/repo", + host="artifactory.example.com", + artifactory_prefix="artifactory/github", + ) + + with patch("git.cmd.Git") as MockGit: + ok = downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ok is False + MockGit.assert_not_called() + + def test_ssh_attempt_skipped_by_default(self): + """Default protocol_pref must NOT add an SSH attempt -- keeps validation quiet.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=True) + self._enter(ctxs) + try: + ls_remote_mock = MagicMock(return_value="") + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = ls_remote_mock + + downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ls_remote_mock.call_count == 2 + finally: + self._exit(ctxs) + + def test_ssh_attempt_added_when_protocol_pref_is_ssh(self): + """--ssh / ProtocolPreference.SSH MUST surface an SSH ls-remote attempt.""" + from apm_cli.deps.transport_selection import ProtocolPreference + + downloader = GitHubPackageDownloader() + downloader._protocol_pref = ProtocolPreference.SSH + dep_ref = self._make_dep_ref() + ctxs = self._patch_auth(downloader, has_token=True) + self._enter(ctxs) + try: + ls_remote_mock = MagicMock(return_value="") + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = ls_remote_mock + + downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=lambda _msg: None, + ) + + assert ls_remote_mock.call_count == 3 + finally: + self._exit(ctxs) + + def test_ls_remote_failure_log_scrubs_token_from_url(self): + """Verbose log MUST NOT leak embedded tokens from a failing ls-remote URL. + + If git surfaces the full ``https://ghp_xxx@github.com/owner/repo.git`` + URL in its error (which it does for basic-auth URLs), the verbose log + must route it through ``_sanitize_git_error`` so the token is masked. + Pins the token-leakage guard for the new ls-remote fallback chain. + """ + from git.exc import GitCommandError + + downloader = GitHubPackageDownloader() + dep_ref = self._make_dep_ref() + ctxs = [ + patch.object(downloader, "_resolve_dep_token", return_value="ghp_supersecret"), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=None), + ] + self._enter(ctxs) + try: + + def _always_fail(*args, **kwargs): + raise GitCommandError( + [ + "git", + "ls-remote", + "https://ghp_supersecret@github.com/owner/repo.git", + "main", + ], + 128, + b"fatal: Authentication failed for 'https://ghp_supersecret@github.com/owner/repo.git/'", + b"", + ) + + captured: list[str] = [] + with patch("git.cmd.Git") as MockGit: + MockGit.return_value.ls_remote = _always_fail + + downloader._ref_exists_via_ls_remote( + dep_ref, + "main", + log=captured.append, + ) + + joined = "\n".join(captured) + assert "ghp_supersecret" not in joined, f"Token leaked into verbose log: {joined!r}" + finally: + self._exit(ctxs) + + if __name__ == "__main__": pytest.main([__file__]) diff --git a/tests/unit/compilation/test_compile_target_flag.py b/tests/unit/compilation/test_compile_target_flag.py index c3525d2d1..7cc6d75d7 100644 --- a/tests/unit/compilation/test_compile_target_flag.py +++ b/tests/unit/compilation/test_compile_target_flag.py @@ -657,9 +657,9 @@ def test_dry_run_respects_hand_authored_guard(self, temp_project): "dry-run must surface the skip the real run would record" ) assert result.stats["copilot_root_instructions_unchanged"] == 0 - assert any( - "hand-authored file will not be overwritten" in w for w in result.warnings - ), "dry-run must surface the same warning a real run would emit" + assert any("hand-authored file will not be overwritten" in w for w in result.warnings), ( + "dry-run must surface the same warning a real run would emit" + ) def test_generated_footer_uses_apm_compile_not_specify(self, temp_project): """Generated footer must read `apm compile`, not `specify apm compile`.""" diff --git a/tests/unit/deps/__init__.py b/tests/unit/deps/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/deps/test_github_downloader_validation.py b/tests/unit/deps/test_github_downloader_validation.py new file mode 100644 index 000000000..4ec11bf3b --- /dev/null +++ b/tests/unit/deps/test_github_downloader_validation.py @@ -0,0 +1,688 @@ +"""Regression tests for round-2 panel findings on PR #941. + +Covers the security gates added to ``github_downloader_validation``: + +* finding 6 -- ``ls-remote`` no longer fails open; a successful ref + resolution must be paired with a positive shallow-fetch + ``ls-tree`` + path probe before validation returns ``True``. +* finding 7 -- ``virtual_path`` is screened by + ``validate_path_segments`` before any URL interpolation, so traversal + segments cannot leak into Contents-API or archive URLs. +* finding 8 -- Azure DevOps tokens are injected via + ``http.extraheader`` (``Authorization: Bearer ...``) and never + embedded in the clone URL or visible on the subprocess argv. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from apm_cli.deps import github_downloader_validation as gdv +from apm_cli.deps.github_downloader import GitHubPackageDownloader +from apm_cli.models.apm_package import DependencyReference + + +def _make_subdir_dep( + repo_url: str = "owner/repo", + vpath: str = "skills/my-skill", + ref: str | None = "main", + host: str | None = None, +) -> DependencyReference: + """Build a virtual-subdirectory ``DependencyReference`` for tests.""" + return DependencyReference( + repo_url=repo_url, + host=host, + reference=ref, + virtual_path=vpath, + is_virtual=True, + ) + + +# --------------------------------------------------------------------------- +# Finding 7: path traversal rejection +# --------------------------------------------------------------------------- + + +class TestVirtualPathTraversalRejection: + """``..`` segments in ``virtual_path`` MUST be rejected before any HTTP.""" + + @pytest.mark.parametrize( + "bad_path", + [ + "../etc/passwd", + "skills/../../../secret", + "..\\windows\\system32", + "ok/../bad", + ], + ) + def test_traversal_segment_rejected_without_network(self, bad_path: str) -> None: + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath=bad_path) + + # Patch download_raw_file to assert it is never reached: validation + # must fail BEFORE any URL interpolation occurs. + with patch.object(downloader, "download_raw_file") as raw_mock: + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False + raw_mock.assert_not_called() + + def test_clean_path_not_rejected(self) -> None: + """A normal path falls through to the marker probes (which we mock).""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/clean") + + with patch.object(downloader, "download_raw_file") as raw_mock: + raw_mock.side_effect = RuntimeError("404") + with ( + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(False, None)), + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False + # Marker probes ran (proving we got past the path-security gate). + assert raw_mock.call_count >= 1 + + +# --------------------------------------------------------------------------- +# Finding 6: fail-open close +# --------------------------------------------------------------------------- + + +class TestLsRemoteFailOpenClose: + """ls-remote success alone MUST NOT validate a typo'd subdirectory.""" + + def _patch_marker_misses(self, downloader: GitHubPackageDownloader): + """Make every download_raw_file probe miss (404).""" + return patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")) + + def test_ls_remote_alone_does_not_validate_when_path_missing(self) -> None: + """Round-2 finding 6: typo'd vpath after a valid ref must return False. + + Reproduces the security regression: previously, a successful + ls-remote on the ref bypassed all path validation. Now the + shallow-fetch + ls-tree probe must also confirm vpath. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/typo-not-real", ref="main") + winning = gdv.AttemptSpec("plain HTTPS w/ credential helper", "https://x", {}) + + with ( + self._patch_marker_misses(downloader), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(True, winning)), + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=False) as path_probe, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False, ( + "Validation must not pass when ls-remote sees the ref but " + "the subdirectory is absent from the tree." + ) + path_probe.assert_called_once() + # Round-3: the winning attempt MUST be threaded through to the + # tree probe (positional or keyword) -- never attempts[0]. + kwargs = path_probe.call_args.kwargs + args = path_probe.call_args.args + assert winning in args or kwargs.get("winning_attempt") is winning + + def test_ls_remote_plus_path_probe_validates(self) -> None: + """Both gates pass -> validation succeeds, with a deferred-probe warning.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/exists", ref="v1.0.0") + warnings: list[str] = [] + winning = gdv.AttemptSpec("authenticated HTTPS (header)", "https://x", {}) + + with ( + self._patch_marker_misses(downloader), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(True, winning)), + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=True), + ): + ok = gdv.validate_virtual_package_exists( + downloader, dep_ref, warn_callback=warnings.append + ) + + assert ok is True + assert len(warnings) == 1, "expected exactly one deferred-probe warning" + # Warning text must NOT include literal '[!]' (the logger + # prepends the symbol). + assert "[!]" not in warnings[0] + # Round-3: warning must name the dep and use '#' (CLI canonical), + # not '@' (the version-pin separator from npm/pip/cargo). + assert "owner/repo" in warnings[0] + assert "#v1.0.0" in warnings[0] + assert "@v1.0.0" not in warnings[0] + + def test_ls_remote_only_runs_when_explicit_ref(self) -> None: + """Without an explicit ``#ref`` the lenient fallback is skipped.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/x", ref=None) + winning = gdv.AttemptSpec("plain HTTPS w/ credential helper", "https://x", {}) + + with ( + self._patch_marker_misses(downloader), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object( + gdv, "_ref_exists_via_ls_remote", return_value=(True, winning) + ) as ls_remote_mock, + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=True) as path_mock, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False + ls_remote_mock.assert_not_called() + path_mock.assert_not_called() + + +# --------------------------------------------------------------------------- +# Finding 8: ADO bearer header injection +# --------------------------------------------------------------------------- + + +class TestAdoBearerHeaderInjection: + """ADO tokens must travel via ``http.extraheader``, never the URL.""" + + def _make_ado_dep(self) -> DependencyReference: + return DependencyReference( + repo_url="myorg/myproj/myrepo", + host="dev.azure.com", + reference="main", + virtual_path="skills/x", + is_virtual=True, + ado_organization="myorg", + ado_project="myproj", + ado_repo="myrepo", + ) + + def test_ado_basic_pat_injected_as_basic_header_not_url(self) -> None: + """ADO PAT (auth_scheme=basic) must use Basic base64(:PAT) header.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_ado_dep() + secret = "ADO_PAT_SECRET_VALUE_DO_NOT_LEAK" + + ado_mock_ctx = MagicMock() + ado_mock_ctx.auth_scheme = "basic" + ado_mock_ctx.git_env = {} + + with ( + patch.object(downloader, "_resolve_dep_token", return_value=secret), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=ado_mock_ctx), + patch.object( + downloader, + "_build_repo_url", + return_value="https://dev.azure.com/myorg/myproj/_git/myrepo", + ), + patch.object( + downloader, + "_build_noninteractive_git_env", + return_value={}, + ), + ): + attempts = gdv._build_validation_attempts(downloader, dep_ref, log=lambda _m: None) + + assert attempts, "expected at least the token attempt" + labels = [a.label for a in attempts] + # Round-3 ADO Basic finding: PAT -> Basic header, NOT raw Bearer. + assert any("basic header" in label.lower() for label in labels), labels + + ado_attempts = [a for a in attempts if "basic header" in a.label.lower()] + assert len(ado_attempts) == 1 + _label, url, env = ado_attempts[0] + + assert secret not in url, "ADO PAT must not appear in the URL" + # The env must carry the Basic header. + assert env.get("GIT_CONFIG_KEY_0") == "http.extraheader" + header_value = env.get("GIT_CONFIG_VALUE_0", "") + assert header_value.startswith("Authorization: Basic "), header_value + # base64(":") must contain the expected encoded form. + import base64 + + expected = base64.b64encode(f":{secret}".encode()).decode("ascii") + assert expected in header_value, "PAT must be base64-encoded as ':'" + # Raw PAT must NOT appear in plaintext anywhere in the env value + # (only the base64-encoded form is permitted). + assert secret not in header_value + + def test_ado_bearer_aad_injected_as_bearer_header(self) -> None: + """ADO + auth_scheme=bearer (AAD JWT) uses raw Bearer header.""" + downloader = GitHubPackageDownloader() + dep_ref = self._make_ado_dep() + secret = "fake-aad-jwt-token" + + ado_mock_ctx = MagicMock() + ado_mock_ctx.auth_scheme = "bearer" + ado_mock_ctx.git_env = {} + + with ( + patch.object(downloader, "_resolve_dep_token", return_value=secret), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=ado_mock_ctx), + patch.object( + downloader, + "_build_repo_url", + return_value="https://dev.azure.com/myorg/myproj/_git/myrepo", + ), + patch.object( + downloader, + "_build_noninteractive_git_env", + return_value={}, + ), + ): + attempts = gdv._build_validation_attempts(downloader, dep_ref, log=lambda _m: None) + + ado_attempts = [a for a in attempts if "bearer header" in a.label.lower()] + assert len(ado_attempts) == 1 + _label, url, env = ado_attempts[0] + assert secret not in url + header_value = env.get("GIT_CONFIG_VALUE_0", "") + assert header_value == f"Authorization: Bearer {secret}" + + def test_non_ado_token_uses_header_not_url(self) -> None: + """GitHub deps now also use header injection (round-3 security finding). + + Round-3 closed the gap where non-ADO tokens were embedded in the + clone URL, leaking via the OS process table and into the temp + bare repo's .git/config during the shallow-fetch path probe. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(repo_url="owner/repo", host="github.com") + secret = "GH_PAT_SECRET" + + captured_token_args: list[str] = [] + + def _capture_build_repo_url(*args, **kwargs): + # Capture the token positional or kwarg so we can assert it + # is empty for the authenticated attempt. + captured_token_args.append(kwargs.get("token", "")) + return "https://github.com/owner/repo.git" + + with ( + patch.object(downloader, "_resolve_dep_token", return_value=secret), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=None), + patch.object(downloader, "_build_repo_url", side_effect=_capture_build_repo_url), + patch.object( + downloader, + "_build_noninteractive_git_env", + return_value={}, + ), + ): + attempts = gdv._build_validation_attempts(downloader, dep_ref, log=lambda _m: None) + + # The token MUST NOT be passed into the URL builder for the + # authenticated attempt -- it travels in the env header instead. + assert all(t == "" for t in captured_token_args), ( + "Round-3 security: non-ADO token must not be embedded in the URL" + ) + + labels = [a.label for a in attempts] + # Header label, no longer the bare 'authenticated HTTPS'. + assert any(lbl == "authenticated HTTPS (header)" for lbl in labels), labels + + auth_attempts = [a for a in attempts if a.label == "authenticated HTTPS (header)"] + assert len(auth_attempts) == 1 + _label, url, env = auth_attempts[0] + assert secret not in url + # Header carries the token. + header_value = env.get("GIT_CONFIG_VALUE_0", "") + assert header_value == f"Authorization: Bearer {secret}" + + +# --------------------------------------------------------------------------- +# Mechanical guards +# --------------------------------------------------------------------------- + + +class TestSplitOwnerRepoGuard: + """Round-2 finding 2: ``repo_url`` without a slash must not raise.""" + + def test_returns_none_on_missing_slash(self) -> None: + assert gdv._split_owner_repo("just-one-segment") is None + + def test_returns_none_on_empty_owner(self) -> None: + assert gdv._split_owner_repo("/repo") is None + + def test_returns_none_on_empty_repo(self) -> None: + assert gdv._split_owner_repo("owner/") is None + + def test_returns_pair_for_valid(self) -> None: + assert gdv._split_owner_repo("owner/repo") == ("owner", "repo") + + def test_directory_probe_returns_false_on_malformed_repo_url(self) -> None: + """Malformed ``repo_url`` falls through to a clean ``False``.""" + downloader = GitHubPackageDownloader() + dep_ref = DependencyReference( + repo_url="malformed-no-slash", + host="github.com", + reference="main", + virtual_path="skills/x", + is_virtual=True, + ) + ok = gdv._directory_exists_at_ref( + downloader, dep_ref, "skills/x", "main", log=lambda _m: None + ) + assert ok is False + + +# --------------------------------------------------------------------------- +# Round-3 regression tests +# --------------------------------------------------------------------------- + + +class TestRound3PathTreeProbeUsesWinningAttempt: + """Round-3: ``_path_exists_in_tree_at_ref`` must reuse the winning attempt. + + Previously it used ``attempts[0]`` unconditionally, breaking the + auth-chain promise when ls-remote succeeded via SSH or plain HTTPS + but the leading PAT attempt would fail the shallow fetch. + """ + + @pytest.mark.parametrize( + "winning_label,winning_url", + [ + ("SSH", "git@github.com:owner/repo.git"), + ("plain HTTPS w/ credential helper", "https://github.com/owner/repo.git"), + ], + ) + def test_tree_probe_uses_winning_attempt_not_attempts_zero( + self, winning_label: str, winning_url: str + ) -> None: + """The shallow-fetch must use the URL/env from the winning ls-remote attempt.""" + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/x", ref="main") + winning_env = {"GIT_SSH_COMMAND": "ssh -o BatchMode=yes"} if "SSH" in winning_label else {} + winning = gdv.AttemptSpec(winning_label, winning_url, winning_env) + + with ( + patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(True, winning)), + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=True) as path_probe, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + path_probe.assert_called_once() + # The winning AttemptSpec must be passed positionally or by kw. + args = path_probe.call_args.args + kwargs = path_probe.call_args.kwargs + assert winning in args or kwargs.get("winning_attempt") is winning, ( + "Path probe must receive the winning attempt -- not attempts[0]." + ) + + +class TestRound3NonAdoTokenNotInProcessArgv: + """Round-3: non-ADO HTTPS token MUST NOT appear in subprocess argv.""" + + def test_non_ado_token_not_in_url_or_argv(self) -> None: + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(repo_url="owner/repo", host="github.com") + secret = "GH_PAT_NEVER_IN_ARGV" + + with ( + patch.object(downloader, "_resolve_dep_token", return_value=secret), + patch.object(downloader, "_resolve_dep_auth_ctx", return_value=None), + patch.object( + downloader, + "_build_repo_url", + return_value="https://github.com/owner/repo.git", + ), + patch.object(downloader, "_build_noninteractive_git_env", return_value={}), + ): + attempts = gdv._build_validation_attempts(downloader, dep_ref, log=lambda _m: None) + + for attempt in attempts: + assert secret not in attempt.url, f"Token leaked into URL for attempt '{attempt.label}'" + # Header injection: the auth attempt env must contain a Bearer header. + auth_attempt = next(a for a in attempts if "header" in a.label) + assert auth_attempt.env.get("GIT_CONFIG_KEY_0") == "http.extraheader" + assert auth_attempt.env["GIT_CONFIG_VALUE_0"] == f"Authorization: Bearer {secret}" + + +class TestRound3SafeRmtreeNotRobustRmtreeDirect: + """Round-3: cleanup MUST go through safe_rmtree (containment gate).""" + + def test_safe_rmtree_called_not_robust_rmtree_direct(self) -> None: + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/x", ref="main") + winning = gdv.AttemptSpec("plain HTTPS w/ credential helper", "https://x", {}) + + # Patch git.cmd.Git so init/fetch/ls_tree don't actually run. + with ( + patch("apm_cli.deps.github_downloader_validation.safe_rmtree") as safe_rm_mock, + patch("apm_cli.deps.github_downloader_validation.git.cmd.Git") as MockGit, + ): + MockGit.return_value.init = MagicMock() + MockGit.return_value.remote = MagicMock() + MockGit.return_value.fetch = MagicMock() + MockGit.return_value.ls_tree = MagicMock(return_value="100644 blob abc\tskills/x") + ok = gdv._path_exists_in_tree_at_ref( + downloader, + dep_ref, + "skills/x", + "main", + log=lambda _m: None, + winning_attempt=winning, + ) + + assert ok is True + safe_rm_mock.assert_called_once() + # Containment: first arg is the tmpdir, second is the base. + call_args = safe_rm_mock.call_args.args + assert len(call_args) == 2, "safe_rmtree must be called with (path, base_dir)" + + +class TestRound3WarnMessage: + """Round-3: warn message names dep with '#' separator and is verbose-gated.""" + + def test_warn_message_uses_hash_separator_and_names_dep(self) -> None: + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/cool", ref="v2.0.0") + warnings: list[str] = [] + winning = gdv.AttemptSpec("authenticated HTTPS (header)", "https://x", {}) + + with ( + patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote", return_value=(True, winning)), + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=True), + ): + ok = gdv.validate_virtual_package_exists( + downloader, dep_ref, warn_callback=warnings.append + ) + + assert ok is True + assert len(warnings) == 1 + msg = warnings[0] + # Names the dep (canonical form). + assert "owner/repo" in msg + assert "skills/cool" in msg + # Uses '#' (CLI canonical), not '@' (the npm/pip version-pin separator). + assert "#v2.0.0" in msg + assert "@v2.0.0" not in msg + # Avoids the bogus 'Contents-API scope' jargon flagged by growth. + assert "Contents-API scope" not in msg + + +class TestRound4WarnSurfacedOnHappyPath: + """Round-4 (cli-logging + devx-ux): the git-fallback warning is yellow. + + The remote panel rejected round-3's suppression-on-happy-path: a + scoped PAT may have correctly rejected a package on the API surface + while the git credential chain accepted it; operators MUST see that + in default CI logs. Round-4 surfaces the warning in both verbose + and non-verbose modes, and strips the "Run with --verbose for + details." suffix only when --verbose is already set. + """ + + def _patch_validate(self, msg: str): + """Patch the downloader to invoke warn_callback once with ``msg``.""" + + def fake_validate(self, dep_ref, verbose_callback=None, warn_callback=None): + if warn_callback is not None: + warn_callback(msg) + return True + + return patch( + "apm_cli.deps.github_downloader.GitHubPackageDownloader." + "validate_virtual_package_exists", + new=fake_validate, + ) + + def _stub_resolver(self) -> MagicMock: + auth_resolver = MagicMock() + ctx = MagicMock() + ctx.source = "env" + ctx.token_type = "pat" + auth_resolver.resolve_for_dep.return_value = ctx + return auth_resolver + + def test_warn_emits_in_non_verbose_mode_with_suffix_kept(self) -> None: + from apm_cli.install import validation as install_validation + + logger = MagicMock() + msg = ( + "API validation skipped for owner/repo/sub#v1; " + "resolved via git credential fallback. " + "Run with --verbose for details." + ) + with self._patch_validate(msg): + ok = install_validation._validate_package_exists( + "owner/repo/sub#v1", + verbose=False, + auth_resolver=self._stub_resolver(), + logger=logger, + ) + assert ok is True + # Yellow signal must reach the user even in default-verbosity. + logger.warning.assert_called_once() + # Suffix is kept in non-verbose so the user knows --verbose digs deeper. + assert "Run with --verbose for details." in logger.warning.call_args[0][0] + + def test_warn_emits_in_verbose_mode_with_suffix_stripped(self) -> None: + from apm_cli.install import validation as install_validation + + logger = MagicMock() + msg = ( + "API validation skipped for owner/repo/sub#v1; " + "resolved via git credential fallback. " + "Run with --verbose for details." + ) + with self._patch_validate(msg): + ok = install_validation._validate_package_exists( + "owner/repo/sub#v1", + verbose=True, + auth_resolver=self._stub_resolver(), + logger=logger, + ) + assert ok is True + logger.warning.assert_called_once() + # In verbose mode, the suffix becomes a no-op: strip it so output + # is not self-referential. + emitted = logger.warning.call_args[0][0] + assert "Run with --verbose for details." not in emitted + assert "API validation skipped for owner/repo/sub#v1" in emitted + + def test_warn_falls_back_to_rich_warning_when_logger_is_none(self) -> None: + """No-logger production callers must still emit the yellow signal. + + Round-3 left a comment claiming "logger is always present in the + install code path"; round-4 enforces it via a ``_rich_warning`` + fallback so silent-drop is impossible regardless of caller wiring. + """ + from apm_cli.install import validation as install_validation + + msg = ( + "API validation skipped for owner/repo/sub#v1; " + "resolved via git credential fallback. " + "Run with --verbose for details." + ) + with ( + self._patch_validate(msg), + patch("apm_cli.install.validation._rich_warning") as rich, + ): + ok = install_validation._validate_package_exists( + "owner/repo/sub#v1", + verbose=False, + auth_resolver=self._stub_resolver(), + logger=None, + ) + assert ok is True + rich.assert_called_once() + assert "API validation skipped" in rich.call_args[0][0] + + +class TestRound4EmptyRefAndEmptyVpathGates: + """Round-4 supply-chain: bare `#` and empty vpath must NOT bypass gates.""" + + def test_empty_string_ref_does_not_activate_git_fallback(self) -> None: + """A bare ``#`` fragment yields ``reference=""``; the git fallback + must remain unreachable. Round-3 used ``is not None`` and let + empty string through, contradicting the documented invariant + that the fallback is only reachable for explicitly-pinned refs. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/x", ref="") + + with ( + patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object(gdv, "_ref_exists_via_ls_remote") as ls_remote, + patch.object(gdv, "_path_exists_in_tree_at_ref") as ls_tree, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False + ls_remote.assert_not_called() + ls_tree.assert_not_called() + + def test_empty_vpath_rejected_before_any_network(self) -> None: + """Empty ``virtual_path`` must be rejected at the entry point. + + ``git ls-tree FETCH_HEAD ""`` is implementation-defined: some + git versions root-list the tree, which would falsely validate + any successfully-fetched repo. ``reject_empty=True`` on the + ``validate_path_segments`` call closes that hole before any + network or git operation runs. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="", ref="v1.0.0") + + with ( + patch.object(downloader, "download_raw_file") as raw_mock, + patch.object(gdv, "_directory_exists_at_ref") as dir_mock, + patch.object(gdv, "_ref_exists_via_ls_remote") as ls_remote, + patch.object(gdv, "_path_exists_in_tree_at_ref") as ls_tree, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is False + raw_mock.assert_not_called() + dir_mock.assert_not_called() + ls_remote.assert_not_called() + ls_tree.assert_not_called() + + def test_explicit_ref_still_activates_git_fallback(self) -> None: + """Sanity: a real ref pin (``v1.0.0``) MUST still reach the + git fallback. Guards against an over-eager fix that would also + block legitimate explicit-ref flows. + """ + downloader = GitHubPackageDownloader() + dep_ref = _make_subdir_dep(vpath="skills/x", ref="v1.0.0") + winning = gdv.AttemptSpec("authenticated HTTPS (header)", "https://x", {}) + + with ( + patch.object(downloader, "download_raw_file", side_effect=RuntimeError("404")), + patch.object(gdv, "_directory_exists_at_ref", return_value=False), + patch.object( + gdv, "_ref_exists_via_ls_remote", return_value=(True, winning) + ) as ls_remote, + patch.object(gdv, "_path_exists_in_tree_at_ref", return_value=True) as ls_tree, + ): + ok = gdv.validate_virtual_package_exists(downloader, dep_ref) + + assert ok is True + ls_remote.assert_called_once() + ls_tree.assert_called_once() diff --git a/tests/unit/marketplace/test_marketplace_commands.py b/tests/unit/marketplace/test_marketplace_commands.py index 41602aa3a..440552867 100644 --- a/tests/unit/marketplace/test_marketplace_commands.py +++ b/tests/unit/marketplace/test_marketplace_commands.py @@ -453,9 +453,7 @@ def test_conflicting_host_error_includes_runnable_command(self, runner): # Rich console may soft-wrap long lines; collapse whitespace before # asserting the runnable command appears intact. normalized = " ".join(result.output.split()) - assert ( - "apm marketplace add https://github.com/acme/plugin-marketplace" in normalized - ) + assert "apm marketplace add https://github.com/acme/plugin-marketplace" in normalized class TestMarketplaceList: diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 3627c9b73..80d7a1bfd 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -298,6 +298,36 @@ def test_validation_failure_with_verbose_omits_verbose_hint(self, mock_validate) assert "not accessible or doesn't exist" in result.output assert "run with --verbose for auth details" not in result.output + @patch("apm_cli.commands.install._validate_package_exists", return_value=False) + def test_subdir_with_ref_failure_names_all_probes(self, mock_validate): + """Round-4 (devx-ux): when a virtual subdir+ref exhausts all four + probes, the failure reason must name them by step so the user + knows what was attempted before the failure. + """ + with self._chdir_tmp(): + Path("apm.yml").write_text("name: test\ndependencies:\n apm: []\n mcp: []\n") + result = self.runner.invoke(cli, ["install", "owner/repo/skills/foo#v1.2.0"]) + output = " ".join(result.output.split()) + assert "all probes failed" in output + assert "marker-file" in output + assert "Contents API" in output + assert "git ls-remote" in output + assert "shallow-fetch" in output + assert "run with --verbose for the full probe log" in output + + @patch("apm_cli.commands.install._validate_package_exists", return_value=False) + def test_subdir_with_ref_failure_verbose_omits_probe_log_hint(self, mock_validate): + with self._chdir_tmp(): + Path("apm.yml").write_text("name: test\ndependencies:\n apm: []\n mcp: []\n") + result = self.runner.invoke( + cli, ["install", "owner/repo/skills/foo#v1.2.0", "--verbose"] + ) + output = " ".join(result.output.split()) + assert "all probes failed" in output + # The "(run with --verbose...)" hint is suppressed once the + # user is already in verbose mode. + assert "run with --verbose for the full probe log" not in output + @patch( "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", return_value=None,