diff --git a/CHANGELOG.md b/CHANGELOG.md index c5b99633..b191dd6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`apm compile -t copilot` now emits `.github/copilot-instructions.md` with zero user configuration** -- APM's first Copilot-native compile target. Global instructions in `.apm/instructions/` are assembled into the file VS Code and GitHub Copilot read automatically; switching targets cleans it up. APM dogfoods this target. (#1048) - **`apm marketplace add` accepts full HTTPS URLs and nested HOST/group/sub/.../REPO shorthands.** You can now paste a repository URL straight from the browser (e.g., `apm marketplace add https://github.com/acme/plugin-marketplace`) and register marketplaces hosted under nested sub-paths on GitHub Enterprise (`ghes.corp.example.com/org/team/repo`). Path-traversal sequences in the parsed segments are rejected via `validate_path_segments`. Non-GitHub hosts (GitLab, Bitbucket, etc.) are explicitly rejected at registration time with an actionable error -- this avoids forwarding GitHub credentials to unintended hosts and the silent fetch-time 404 that previously resulted; native non-GitHub support is tracked separately. (#1034, closes #1027) - Regression tests for `apm compile` placement of narrow `applyTo` patterns: instructions whose matches all live deep inside one subtree are now pinned to the deepest covering directory instead of being hoisted to the project root, across both selective and single-point placement strategies. Also covers the file-walk cache that skips repeated filesystem scans for the same glob. (#871) +- **`apm marketplace audit `** -- supply-chain audit that fetches each plugin's own `apm.yml` at its pinned ref and warns when a `dependencies.apm` entry would be resolved outside the marketplace catalogue (direct repo paths, git URLs, or `{git: ...}` object-form entries). Default run is informational and exits 0; `--strict` exits non-zero on bypass warnings or unverifiable plugins, for use in CI. Complements the existing `apm marketplace doctor` (environment diagnostics). (#881) - **`apm pack` marketplace builder hardening.** Local source paths are now emitted relative to `metadata.pluginRoot` (fixes double-prefix bug). New pass-through fields: `author`, `license`, `repository`, `keywords` (alias for `tags`). Curator-wins override semantics for `description`/`version` on remote entries. Security guards reject path traversal and absolute paths post-subtraction. (#1061) - **Plugin manifest schema-conformance tests.** `tests/unit/test_plugin_exporter_schema.py` validates every shape of `plugin.json` produced by `apm pack` (synthesized, authored, and authored-with-stale-keys) against the vendored official schema. Companion marketplace conformance lives in `tests/unit/marketplace/test_schema_conformance.py`. (#1061) - **APM now compiles and integrates to Windsurf/Cascade.** New first-class `--target windsurf` support: instructions deploy as `.windsurf/rules/` with trigger frontmatter, agents deploy as `.windsurf/skills//SKILL.md`, commands as `.windsurf/workflows/`, hooks merge into `.windsurf/hooks.json`, and MCP servers configure via `~/.codeium/windsurf/mcp_config.json`. Auto-detection, user-scope deployment, and `apm pack` all support the new target. (#1066) diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 27256443..80175387 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -1421,6 +1421,40 @@ apm marketplace check apm marketplace check --offline ``` +#### `apm marketplace audit` - Audit transitive dependencies for marketplace bypass + +Fetch each plugin's own `apm.yml` at its pinned ref and warn when a `dependencies.apm` entry would be resolved outside the marketplace catalogue. Such transitive deps would resolve via direct git clone and track HEAD, defeating the supply-chain pinning the marketplace provides. + +The marketplace manifest is always re-fetched fresh (the 1-hour `marketplace.json` cache is bypassed) so the audit is run against the current published catalogue rather than stale local data. + +```bash +apm marketplace audit NAME [OPTIONS] +``` + +**Arguments:** +- `NAME` - Registered marketplace name (required) + +**Options:** +- `--strict` - Exit non-zero on any bypass warning or unverifiable plugin (CI-friendly) +- `-v, --verbose` - Show clean plugins and skipped reasons inline + +**Classification:** +- *Bypasses (warn)* - bare `owner/repo`, `owner/repo/subpath`, `https://`/`ssh://` git URLs, and `{git: URL, ...}` object-form entries +- *Clean* - `name@marketplace` refs and local paths (`./x`, `/abs`, `../x`) +- *Skipped* - plugin has no `apm.yml` at the pinned ref, or the source type is not an addressable github manifest +- *Unverifiable* - fetch failure or malformed YAML (per-plugin isolation: one bad plugin does not abort the run) + +**Exit codes:** +- `0` - Default mode always exits 0; `--strict` exits 0 only when nothing bypasses and no plugin is unverifiable +- `1` - `--strict` mode with bypass warnings or unverifiable plugins + +**Examples:** +```bash +apm marketplace audit my-marketplace +apm marketplace audit my-marketplace --strict # CI gate +apm marketplace audit my-marketplace -v # show clean & skipped detail +``` + #### `apm marketplace doctor` - Environment diagnostics Check git, network reachability, authentication, `gh` CLI availability, and the presence of a marketplace config (in `apm.yml` or legacy `marketplace.yml`). Run this first when `apm pack` or `publish` fails in an unfamiliar environment. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index ff14e95c..a8529569 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -78,6 +78,7 @@ | `apm marketplace migrate` | Fold a legacy `marketplace.yml` into `apm.yml`'s `marketplace:` block; deletes `marketplace.yml` on success | `--force`/`--yes`/`-y`, `--dry-run`, `-v` | | `apm marketplace outdated` | Report upgradable plugins, range-aware | `--offline`, `--include-prerelease`, `-v` | | `apm marketplace check` | Validate the `marketplace:` block and verify refs resolve | `--offline`, `-v` | +| `apm marketplace audit NAME` | Supply-chain audit: warn when plugin transitive deps bypass marketplace pinning | `--strict` (CI exit-1 on bypass), `-v` | | `apm marketplace doctor` | Diagnose git, network, auth, and marketplace config readiness | `-v` | | `apm marketplace publish` | Open PRs on consumer repos from `consumer-targets.yml` | `--targets PATH`, `--dry-run`, `--no-pr`, `--draft`, `--allow-downgrade`, `--allow-ref-change`, `--parallel N`, `-y` | | `apm marketplace package add ` | Add a plugin entry to `marketplace.plugins` (source accepts `owner/repo` or `./path`) | `--name`, `--version`, `--ref` (mutable refs auto-resolved to SHA), `-d`/`--description`, `-s`/`--subdir`, `--tag-pattern`, `--tags`, `--include-prerelease`, `--no-verify` | diff --git a/src/apm_cli/commands/marketplace/__init__.py b/src/apm_cli/commands/marketplace/__init__.py index 39d03917..7ef9f721 100644 --- a/src/apm_cli/commands/marketplace/__init__.py +++ b/src/apm_cli/commands/marketplace/__init__.py @@ -84,6 +84,7 @@ class MarketplaceGroup(click.Group): "init", "check", "outdated", + "audit", "doctor", "publish", "package", @@ -1342,6 +1343,7 @@ def search(expression, limit, verbose): sys.exit(1) +from .audit import audit # noqa: E402 from .check import check # noqa: E402 from .doctor import doctor # noqa: E402 from .init import init # noqa: E402 @@ -1382,6 +1384,7 @@ def search(expression, limit, verbose): "SemVer", "TargetResult", "add", + "audit", "browse", "check", "detect_config_source", diff --git a/src/apm_cli/commands/marketplace/audit.py b/src/apm_cli/commands/marketplace/audit.py new file mode 100644 index 00000000..1457d615 --- /dev/null +++ b/src/apm_cli/commands/marketplace/audit.py @@ -0,0 +1,127 @@ +"""``apm marketplace audit`` command.""" + +from __future__ import annotations + +import sys +import traceback + +import click + +from ...core.command_logger import CommandLogger +from ...marketplace.audit import FetchStatus, run_audit +from ...marketplace.client import fetch_marketplace +from ...marketplace.registry import get_marketplace_by_name +from . import marketplace + + +@marketplace.command( + help="Audit plugin transitive deps for marketplace-bypass risk" +) +@click.argument("name", required=True) +@click.option( + "--strict", + is_flag=True, + help="Exit non-zero when any plugin has bypass dependencies or fetch errors", +) +@click.option("--verbose", "-v", is_flag=True, help="Show detailed output") +def audit(name, strict, verbose): + """Audit a registered marketplace's supply-chain pinning. + + For each plugin in ``NAME``'s manifest, fetch the plugin's own + ``apm.yml`` (at its pinned ref) and warn when ``dependencies.apm`` + entries use direct repo paths, which bypass the marketplace's + version pinning and make transitive deps track HEAD. + """ + logger = CommandLogger("marketplace-audit", verbose=verbose) + try: + source = get_marketplace_by_name(name) + logger.start(f"Auditing marketplace '{name}'...", symbol="gear") + + manifest = fetch_marketplace(source, force_refresh=True) + n = len(manifest.plugins) + logger.progress( + f"Checking {n} plugin{'' if n == 1 else 's'}...", symbol="info" + ) + + reports = run_audit(manifest, source) + + ok_count = 0 + bypass_total = 0 + fetch_error_count = 0 + skipped_count = 0 + + # Suppress the per-plugin section header when there is nothing to + # report and the user did not opt into verbose: in the all-clean + # default run the header would otherwise hang above an empty body. + has_findings = any( + rep.fetch_status != FetchStatus.OK or rep.issues + for rep in reports + ) + + click.echo() + if has_findings or verbose: + click.echo("Audit Results:") + for rep in reports: + if rep.fetch_status == FetchStatus.OK: + if not rep.issues: + ok_count += 1 + if verbose: + logger.success( + f" {rep.plugin_name}: deps are marketplace-resolved", + symbol="check", + ) + continue + bypass_total += len(rep.issues) + if len(rep.issues) == 1: + verb_phrase = "1 dependency bypasses" + else: + verb_phrase = f"{len(rep.issues)} dependencies bypass" + logger.warning( + f" {rep.plugin_name}: {verb_phrase} the marketplace", + symbol="warning", + ) + for issue in rep.issues: + click.echo(f" - '{issue.dep}'") + click.echo(f" hint: {issue.suggestion}") + elif rep.fetch_status in ( + FetchStatus.NO_MANIFEST, + FetchStatus.UNSUPPORTED_SOURCE, + ): + skipped_count += 1 + if verbose: + logger.verbose_detail( + f" {rep.plugin_name}: skipped ({rep.fetch_status.value}" + f"{' - ' + rep.detail if rep.detail else ''})" + ) + else: + fetch_error_count += 1 + logger.warning( + f" {rep.plugin_name}: could not verify " + f"({rep.fetch_status.value}: {rep.detail})", + symbol="warning", + ) + + click.echo() + warn_noun = "warning" if bypass_total == 1 else "warnings" + err_noun = "error" if fetch_error_count == 1 else "errors" + click.echo( + f"Summary: {ok_count} clean, {bypass_total} bypass {warn_noun}, " + f"{skipped_count} skipped, " + f"{fetch_error_count} unverifiable {err_noun}" + ) + if bypass_total: + click.echo() + click.echo( + "Marketplace refs (name@marketplace) pin transitive deps " + "through the catalogue so consumers get the same versions " + "you tested. See: " + "https://microsoft.github.io/apm/guides/marketplaces/" + ) + + if strict and (bypass_total or fetch_error_count): + sys.exit(1) + + except Exception as e: + logger.error(f"Failed to audit marketplace: {e}") + logger.verbose_detail(traceback.format_exc()) + sys.exit(1) diff --git a/src/apm_cli/marketplace/audit.py b/src/apm_cli/marketplace/audit.py new file mode 100644 index 00000000..7dd44eaa --- /dev/null +++ b/src/apm_cli/marketplace/audit.py @@ -0,0 +1,339 @@ +"""Marketplace audit: supply-chain checks for a registered marketplace. + +Resolves each plugin's own ``apm.yml`` at its pinned ref and flags +``dependencies.apm`` entries that would be resolved outside the +marketplace catalogue. Such transitive deps would resolve via git +clone and track HEAD -- **bypassing the marketplace's version +pinning** -- which defeats the supply-chain guarantee the marketplace +is supposed to provide for consumers. + +This module contains only pure logic and an opt-in network fetcher; the CLI +wiring lives in :mod:`apm_cli.commands.marketplace.audit`. +""" + +from __future__ import annotations + +import enum +import logging +from collections.abc import Callable +from dataclasses import dataclass + +import yaml + +from ..utils.path_security import PathTraversalError, validate_path_segments +from .client import fetch_raw +from .errors import MarketplaceError +from .models import MarketplaceManifest, MarketplacePlugin, MarketplaceSource +from .resolver import parse_marketplace_ref + +logger = logging.getLogger(__name__) + + +class DepClassification(enum.Enum): + """How a dependency string resolves from the consumer's perspective.""" + + MARKETPLACE = "marketplace" + LOCAL = "local" + BYPASSES_MARKETPLACE = "bypasses_marketplace" + EMPTY = "empty" + + +class FetchStatus(enum.Enum): + """Outcome of fetching a single plugin's apm.yml.""" + + OK = "ok" + NO_MANIFEST = "no_manifest" + UNSUPPORTED_SOURCE = "unsupported_source" + NETWORK_ERROR = "network_error" + PARSE_ERROR = "parse_error" + + +@dataclass(frozen=True) +class DepIssue: + dep: str + classification: DepClassification + suggestion: str + + +@dataclass(frozen=True) +class PluginDepReport: + plugin_name: str + fetch_status: FetchStatus + issues: tuple[DepIssue, ...] = () + detail: str = "" + + +# --------------------------------------------------------------------------- +# Pure logic +# --------------------------------------------------------------------------- + + +def classify_dependency(dep: str) -> DepClassification: + """Classify an entry from ``dependencies.apm`` of a plugin's ``apm.yml``. + + Uses :func:`parse_marketplace_ref` as the authority for marketplace-ref + grammar (so behavior stays in sync if that grammar evolves), and + ``DependencyReference.is_local_path`` for local paths. Anything else + hits a git remote directly, bypassing the marketplace's version pinning. + """ + stripped = (dep or "").strip() + if not stripped: + return DepClassification.EMPTY + + try: + if parse_marketplace_ref(stripped) is not None: + return DepClassification.MARKETPLACE + except ValueError: + # Semver range in marketplace ref -- still marketplace-shaped intent. + # Let the normal install path surface that error; audit does not + # relitigate grammar. + return DepClassification.MARKETPLACE + + from ..models.dependency.reference import DependencyReference + + if DependencyReference.is_local_path(stripped): + return DepClassification.LOCAL + + return DepClassification.BYPASSES_MARKETPLACE + + +def _suggest_replacement(dep: str) -> str: + """Best-effort suggestion text for a bypassing dep.""" + base = dep.split("#", 1)[0] + pkg_hint = base.rsplit("/", 1)[-1] if "/" in base else base + if pkg_hint.endswith(".git"): + pkg_hint = pkg_hint[: -len(".git")] + pkg_hint = pkg_hint.strip() or "package" + return f"publish via marketplace and depend on it as '{pkg_hint}@'" + + +def _normalize_dep_entry(entry) -> str | None: + """Flatten a dep entry to the string shape expected by the classifier. + + Matches :meth:`DependencyReference.parse_from_dict`: a dict entry is + either ``{git: URL, path?: SUB, ref?: REF}`` (direct git URL -- bypasses + the marketplace) or ``{path: ./local}`` (local filesystem path). + + Returns the canonical string form, or ``None`` if the entry cannot be + flattened (e.g. bare primitives we do not recognise). + """ + if isinstance(entry, str): + return entry + if not isinstance(entry, dict): + return None + + if "git" in entry: + git_url = entry.get("git") + if not isinstance(git_url, str) or not git_url.strip(): + return None + # For classification we only care about the scheme/host shape, not + # the sub-path or ref. classify_dependency will inspect this and + # see that it is not a ``name@marketplace`` ref, flagging it. + return git_url.strip() + + if "path" in entry: + path = entry.get("path") + if isinstance(path, str) and path.strip(): + return path.strip() + + return None + + +def _collect_apm_dep_strings(apm_yml_data: dict) -> list[str]: + """Collect dep entries from both dependencies and devDependencies. + + Handles both string and dict entry forms (the two apm.yml supports) so + the classifier does not miss ``{git: ...}`` object-style deps, which + also bypass the marketplace. + """ + results: list[str] = [] + for section_name in ("dependencies", "devDependencies"): + section = apm_yml_data.get(section_name) + if not isinstance(section, dict): + continue + apm_list = section.get("apm") + if not isinstance(apm_list, list): + continue + for entry in apm_list: + flat = _normalize_dep_entry(entry) + if flat is not None: + results.append(flat) + return results + + +# --------------------------------------------------------------------------- +# Plugin source resolution (github-dict sources only, matches resolver.py) +# --------------------------------------------------------------------------- + + +def _resolve_plugin_github_coords( + plugin: MarketplacePlugin, + fallback_host: str, +) -> tuple[str, str, str, str, str] | None: + """Extract ``(host, owner, repo, ref, apm_yml_path)`` for a plugin. + + Returns ``None`` for plugin sources the audit cannot address (string + sources, non-github dict types, malformed entries). A returned value + is safe to hand to :func:`fetch_raw`; path components have been + traversal-checked. + """ + source = plugin.source + if not isinstance(source, dict): + return None + if source.get("type") != "github": + return None + repo = source.get("repo", "") + # Require the exact ``owner/name`` shape. Sub-paths belong in the + # separate ``path`` field, so a value like ``owner/repo/extra`` is + # a misconfiguration -- if we accepted it we would build a + # malformed GitHub URL that 404s and gets silently reported as + # NO_MANIFEST instead of UNSUPPORTED_SOURCE. + if not isinstance(repo, str) or repo.count("/") != 1: + return None + owner, _, name = repo.partition("/") + if not owner or not name: + return None + ref = source.get("ref") or "HEAD" + host = source.get("host") or fallback_host or "github.com" + path = (source.get("path") or "").strip("/") + if path: + try: + validate_path_segments(path, context="plugin source path") + except PathTraversalError: + return None + apm_yml_path = f"{path}/apm.yml" + else: + apm_yml_path = "apm.yml" + return host, owner, name, ref, apm_yml_path + + +# --------------------------------------------------------------------------- +# Fetchers and orchestration +# --------------------------------------------------------------------------- + + +def fetch_plugin_apm_yml( + plugin: MarketplacePlugin, + marketplace_source: MarketplaceSource, + auth_resolver: object | None = None, +) -> tuple[FetchStatus, dict | None, str]: + """Fetch and parse a plugin's ``apm.yml`` at its pinned ref. + + Returns a tuple ``(status, data_or_None, detail_message)``. Never + raises -- every failure is reported through the status enum so that a + single bad plugin cannot abort a whole-marketplace audit run. + """ + coords = _resolve_plugin_github_coords(plugin, marketplace_source.host) + if coords is None: + return ( + FetchStatus.UNSUPPORTED_SOURCE, + None, + "plugin source is not an addressable github manifest", + ) + + host, owner, name, ref, apm_yml_path = coords + try: + raw = fetch_raw( + host, + owner, + name, + apm_yml_path, + ref, + auth_resolver=auth_resolver, + ) + except MarketplaceError as exc: + return FetchStatus.NETWORK_ERROR, None, str(exc) + + if raw is None: + return ( + FetchStatus.NO_MANIFEST, + None, + f"no apm.yml at '{apm_yml_path}' @ {ref}", + ) + + try: + data = yaml.safe_load(raw.decode("utf-8", errors="replace")) + except yaml.YAMLError as exc: + return FetchStatus.PARSE_ERROR, None, f"malformed YAML: {exc}" + + if not isinstance(data, dict): + return FetchStatus.PARSE_ERROR, None, "apm.yml root is not a mapping" + + return FetchStatus.OK, data, "" + + +def check_plugin( + plugin: MarketplacePlugin, + marketplace_source: MarketplaceSource, + auth_resolver: object | None = None, + *, + _fetcher: Callable | None = None, +) -> PluginDepReport: + """Run audit checks against a single plugin. + + ``_fetcher`` is a test seam with the same signature as + :func:`fetch_plugin_apm_yml`. + """ + fetcher = _fetcher or fetch_plugin_apm_yml + status, data, detail = fetcher(plugin, marketplace_source, auth_resolver) + if status != FetchStatus.OK or data is None: + return PluginDepReport( + plugin_name=plugin.name, + fetch_status=status, + detail=detail, + ) + + issues: list[DepIssue] = [] + for dep in _collect_apm_dep_strings(data): + cls = classify_dependency(dep) + if cls == DepClassification.BYPASSES_MARKETPLACE: + issues.append( + DepIssue( + dep=dep, + classification=cls, + suggestion=_suggest_replacement(dep), + ) + ) + return PluginDepReport( + plugin_name=plugin.name, + fetch_status=FetchStatus.OK, + issues=tuple(issues), + ) + + +def run_audit( + manifest: MarketplaceManifest, + marketplace_source: MarketplaceSource, + auth_resolver: object | None = None, + *, + _fetcher: Callable | None = None, +) -> list[PluginDepReport]: + """Run all audit checks across every plugin of a marketplace manifest. + + Each plugin is isolated: a failure inside one check produces a + :class:`PluginDepReport` with a non-``OK`` status rather than + propagating. + """ + reports: list[PluginDepReport] = [] + for plugin in manifest.plugins: + try: + reports.append( + check_plugin( + plugin, + marketplace_source, + auth_resolver, + _fetcher=_fetcher, + ) + ) + except Exception as exc: # pragma: no cover -- defensive + logger.warning( + "Audit check failed for plugin '%s': %s", plugin.name, exc + ) + reports.append( + PluginDepReport( + plugin_name=plugin.name, + fetch_status=FetchStatus.NETWORK_ERROR, + detail=f"unexpected error: {exc}", + ) + ) + return reports diff --git a/src/apm_cli/marketplace/client.py b/src/apm_cli/marketplace/client.py index 4035c17b..0207ef78 100644 --- a/src/apm_cli/marketplace/client.py +++ b/src/apm_cli/marketplace/client.py @@ -17,7 +17,7 @@ import requests -from .errors import MarketplaceFetchError +from .errors import MarketplaceError, MarketplaceFetchError from .models import ( MarketplaceManifest, MarketplacePlugin, @@ -143,13 +143,17 @@ def _clear_cache(name: str) -> None: # --------------------------------------------------------------------------- -def _try_proxy_fetch( - source: MarketplaceSource, +def _try_proxy_fetch_raw( + owner: str, + repo: str, file_path: str, -) -> dict | None: - """Try to fetch marketplace JSON via the registry proxy. + ref: str, +) -> bytes | None: + """Try to fetch a file as raw bytes via the registry proxy. - Returns parsed JSON dict on success, ``None`` when no proxy is + The proxy host is governed by ``PROXY_REGISTRY_URL``; the caller's git + host is intentionally ignored here -- the proxy is the single routing + endpoint. Returns raw bytes on success, ``None`` when no proxy is configured or the entry download fails. """ from ..deps.registry_proxy import RegistryConfig @@ -160,16 +164,32 @@ def _try_proxy_fetch( from ..deps.artifactory_entry import fetch_entry_from_archive - content = fetch_entry_from_archive( + return fetch_entry_from_archive( host=cfg.host, prefix=cfg.prefix, - owner=source.owner, - repo=source.repo, + owner=owner, + repo=repo, file_path=file_path, - ref=source.branch, + ref=ref, scheme=cfg.scheme, headers=cfg.get_headers(), ) + + +def _try_proxy_fetch( + source: MarketplaceSource, + file_path: str, +) -> dict | None: + """Try to fetch marketplace JSON via the registry proxy. + + Returns parsed JSON dict on success, ``None`` when no proxy is + configured or the entry download fails. Delegates the byte-level + fetch to :func:`_try_proxy_fetch_raw` so the proxy I/O path stays + DRY between marketplace.json and arbitrary-file callers. + """ + content = _try_proxy_fetch_raw( + source.owner, source.repo, file_path, source.branch + ) if content is None: return None @@ -185,13 +205,121 @@ def _try_proxy_fetch( return None -def _github_contents_url(source: MarketplaceSource, file_path: str) -> str: - """Build the GitHub Contents API URL for a file.""" +def _github_contents_url_generic( + host: str, owner: str, repo: str, file_path: str, ref: str +) -> str: + """Build the GitHub Contents API URL for an arbitrary (host, owner, repo).""" from ..core.auth import AuthResolver - host_info = AuthResolver.classify_host(source.host) + host_info = AuthResolver.classify_host(host) api_base = host_info.api_base - return f"{api_base}/repos/{source.owner}/{source.repo}/contents/{file_path}?ref={source.branch}" + return f"{api_base}/repos/{owner}/{repo}/contents/{file_path}?ref={ref}" + + +def _github_contents_url(source: MarketplaceSource, file_path: str) -> str: + """Build the GitHub Contents API URL for a marketplace source.""" + return _github_contents_url_generic( + source.host, source.owner, source.repo, file_path, source.branch + ) + + +def fetch_raw( + host: str, + owner: str, + repo: str, + file_path: str, + ref: str, + *, + auth_resolver: object | None = None, +) -> bytes | None: + """Fetch a file as raw bytes from a GitHub-compatible host. + + Low-level primitive used by ``marketplace audit`` to fetch plugin + apm.yml content. Honors the same proxy + auth semantics as + :func:`_fetch_file`. + + Returns raw bytes on success, or ``None`` if the file is confirmed + not to exist (HTTP 404 from the upstream API). + + Raises: + MarketplaceError: On unexpected failures (network, auth, 5xx); + when ``PROXY_REGISTRY_ONLY=1`` blocks the GitHub fallback + after a proxy miss -- in that case the file's existence + cannot be verified, which is distinct from a confirmed 404 + and must not be silently treated as ``no manifest`` by + callers like ``marketplace audit``; and when ``host`` is + not a supported GitHub variant (defense-in-depth so we + never attach a GitHub PAT to a request aimed at an + unrelated host). + Raised as the neutral base class so callers can wrap the + error with their own context (e.g. plugin-specific wording). + """ + proxy_bytes = _try_proxy_fetch_raw(owner, repo, file_path, ref) + if proxy_bytes is not None: + return proxy_bytes + + from ..deps.registry_proxy import RegistryConfig + + cfg = RegistryConfig.from_env() + if cfg is not None and cfg.enforce_only: + raise MarketplaceError( + f"cannot verify {owner}/{repo}/{file_path}@{ref}: " + "PROXY_REGISTRY_ONLY blocks the GitHub fallback after a proxy miss" + ) + + # Defense-in-depth host-kind guard, mirroring the check in + # ``_fetch_file``. Plugin sources are validated upstream, but if a + # non-GitHub host (e.g. ``gitlab.com``) reached this fallback path we + # MUST NOT attach ``Authorization: token `` to a request + # aimed at it -- doing so would leak GitHub credentials to an + # unrelated host. Fail closed. + from ..core.auth import AuthResolver as _AuthResolver + + host_info = _AuthResolver.classify_host(host) + if host_info.kind not in ("github", "ghe_cloud", "ghes"): + raise MarketplaceError( + f"cannot verify {owner}/{repo}/{file_path}@{ref}: " + f"host {host!r} is not a supported plugin source. Only GitHub, " + f"GitHub Enterprise Cloud (*.ghe.com), and GHES (GITHUB_HOST) " + f"are supported; refusing to fetch to avoid forwarding GitHub " + f"credentials to a non-GitHub host." + ) + + url = _github_contents_url_generic(host, owner, repo, file_path, ref) + + def _do_fetch(token, _git_env): + headers = { + "Accept": "application/vnd.github.v3.raw", + "User-Agent": "apm-cli", + } + # Only attach GitHub-namespaced credentials when the resolver-derived + # host kind is a GitHub variant. The outer guard already enforces + # this, but keep the conditional explicit so the credential-attach + # site is locally auditable. + if token and host_info.kind in ("github", "ghe_cloud", "ghes"): + headers["Authorization"] = f"token {token}" + resp = requests.get(url, headers=headers, timeout=30) + if resp.status_code == 404: + return None + resp.raise_for_status() + return resp.content + + if auth_resolver is None: + from ..core.auth import AuthResolver + + auth_resolver = AuthResolver() + + try: + return auth_resolver.try_with_fallback( + host, + _do_fetch, + org=owner, + unauth_first=False, + ) + except Exception as exc: + raise MarketplaceError( + f"fetching {owner}/{repo}/{file_path}@{ref}: {exc}" + ) from exc def _fetch_file( diff --git a/tests/unit/marketplace/test_marketplace_audit.py b/tests/unit/marketplace/test_marketplace_audit.py new file mode 100644 index 00000000..53b1d32d --- /dev/null +++ b/tests/unit/marketplace/test_marketplace_audit.py @@ -0,0 +1,694 @@ +"""Tests for marketplace audit.""" + +from unittest.mock import patch + +import pytest +from click.testing import CliRunner + +from apm_cli.marketplace.audit import ( + DepClassification, + FetchStatus, + _collect_apm_dep_strings, + _normalize_dep_entry, + _resolve_plugin_github_coords, + _suggest_replacement, + check_plugin, + classify_dependency, + fetch_plugin_apm_yml, + run_audit, +) +from apm_cli.marketplace.errors import MarketplaceError +from apm_cli.marketplace.models import ( + MarketplaceManifest, + MarketplacePlugin, + MarketplaceSource, +) + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def runner(): + return CliRunner() + + +@pytest.fixture(autouse=True) +def _isolate_config(tmp_path, monkeypatch): + """Mirror the isolation used by other marketplace tests.""" + config_dir = str(tmp_path / ".apm") + monkeypatch.setattr("apm_cli.config.CONFIG_DIR", config_dir) + monkeypatch.setattr( + "apm_cli.config.CONFIG_FILE", str(tmp_path / ".apm" / "config.json") + ) + monkeypatch.setattr("apm_cli.config._config_cache", None) + monkeypatch.setattr("apm_cli.marketplace.registry._registry_cache", None) + + +def _gh_plugin(name="p", repo="acme/agent-forge", ref="abc123", path=""): + src = {"type": "github", "repo": repo, "ref": ref} + if path: + src["path"] = path + return MarketplacePlugin(name=name, source=src) + + +def _manifest(*plugins, name="test-marketplace"): + return MarketplaceManifest(name=name, plugins=tuple(plugins)) + + +def _source(name="test-marketplace"): + return MarketplaceSource(name=name, owner="acme", repo="marketplace") + + +# =================================================================== +# classify_dependency +# =================================================================== + + +class TestClassifyDependency: + """Cover every path of the dep classifier.""" + + @pytest.mark.parametrize( + "dep", + [ + "conventions@my-marketplace", + "conventions@my-marketplace#v1.2.3", + "code-quality@acme#feat/some-branch", + ], + ) + def test_marketplace_refs(self, dep): + assert classify_dependency(dep) == DepClassification.MARKETPLACE + + def test_marketplace_ref_with_semver_range_is_still_marketplace(self): + # parse_marketplace_ref raises ValueError for semver ranges; audit + # should classify as MARKETPLACE so that the install path (not + # audit) surfaces the grammar error. + assert ( + classify_dependency("pkg@mkt#^1.2.3") + == DepClassification.MARKETPLACE + ) + + @pytest.mark.parametrize( + "dep", + [ + "./local/pkg", + "../sibling-pkg", + "/abs/path/pkg", + ], + ) + def test_local_paths(self, dep): + assert classify_dependency(dep) == DepClassification.LOCAL + + @pytest.mark.parametrize( + "dep", + [ + "acme/agent-forge", + "acme/agent-forge/general/conventions", + "acme/agent-forge#abc123", + "acme/agent-forge/general/conventions#v1.0.0", + "https://gitlab.com/acme/agent-forge.git", + "git@gitlab.com:acme/agent-forge.git", + "ssh://git@gitlab.com/acme/agent-forge.git", + "github.com/acme/agent-forge", + ], + ) + def test_bypass_paths(self, dep): + assert ( + classify_dependency(dep) + == DepClassification.BYPASSES_MARKETPLACE + ) + + @pytest.mark.parametrize("dep", ["", " ", None]) + def test_empty(self, dep): + # None and empty-ish strings are reported as EMPTY, letting the + # normal schema validator (not audit) surface the underlying issue. + assert classify_dependency(dep) == DepClassification.EMPTY # type: ignore[arg-type] + + +# =================================================================== +# _collect_apm_dep_strings +# =================================================================== + + +class TestCollectApmDepStrings: + def test_both_sections(self): + data = { + "dependencies": {"apm": ["a@mkt", "owner/b"]}, + "devDependencies": {"apm": ["owner/c"]}, + } + assert _collect_apm_dep_strings(data) == [ + "a@mkt", "owner/b", "owner/c" + ] + + def test_dict_form_git_entry_is_captured(self): + """Dict-form deps bypass the marketplace too -- string-only + collection would silently drop ``{git: ...}`` object-style + entries, which must be flagged just like bare URL strings. + """ + data = { + "dependencies": { + "apm": [ + "name@mkt", + { + "git": "https://gitlab.com/acme/coding-standards.git", + "path": "instructions/security", + "ref": "v2.0", + }, + ] + } + } + flat = _collect_apm_dep_strings(data) + assert "name@mkt" in flat + assert any( + "gitlab.com/acme/coding-standards.git" in s for s in flat + ) + + def test_dict_form_local_path_entry_is_captured(self): + data = { + "dependencies": { + "apm": [{"path": "./packages/shared"}] + } + } + assert _collect_apm_dep_strings(data) == ["./packages/shared"] + + def test_skip_unrecognised_primitives(self): + # 42 and None cannot be flattened, but the valid string is kept + data = {"dependencies": {"apm": ["a@mkt", 42, None]}} + assert _collect_apm_dep_strings(data) == ["a@mkt"] + + def test_skip_malformed_dict_entries(self): + # Missing both git and path -> cannot flatten -> dropped + data = { + "dependencies": { + "apm": [{"alias": "orphan"}, {"git": "", "path": "x"}] + } + } + assert _collect_apm_dep_strings(data) == [] + + def test_missing_sections(self): + assert _collect_apm_dep_strings({}) == [] + assert _collect_apm_dep_strings({"dependencies": None}) == [] + assert _collect_apm_dep_strings({"dependencies": {}}) == [] + assert _collect_apm_dep_strings( + {"dependencies": {"apm": "not-a-list"}} + ) == [] + + +class TestNormalizeDepEntry: + def test_string_passthrough(self): + assert _normalize_dep_entry("a@mkt") == "a@mkt" + + def test_dict_git_entry(self): + out = _normalize_dep_entry( + {"git": "https://gitlab.com/acme/x.git", "path": "sub", "ref": "v1"} + ) + assert out == "https://gitlab.com/acme/x.git" + + def test_dict_path_entry(self): + assert ( + _normalize_dep_entry({"path": "./local/pkg"}) == "./local/pkg" + ) + + def test_dict_path_whitespace_stripped(self): + assert _normalize_dep_entry({"path": " ./x "}) == "./x" + + def test_dict_missing_fields(self): + assert _normalize_dep_entry({"alias": "x"}) is None + assert _normalize_dep_entry({"git": ""}) is None + assert _normalize_dep_entry({"path": " "}) is None + + def test_non_string_non_dict(self): + assert _normalize_dep_entry(42) is None + assert _normalize_dep_entry(None) is None + assert _normalize_dep_entry(["nope"]) is None + + +class TestSuggestReplacement: + def test_strips_git_suffix(self): + """Review finding: suggestion should not end with .git.""" + assert ( + "coding-standards@" + in _suggest_replacement( + "https://gitlab.com/acme/coding-standards.git" + ) + ) + assert ".git@" not in _suggest_replacement( + "https://gitlab.com/acme/x.git" + ) + + +# =================================================================== +# _resolve_plugin_github_coords +# =================================================================== + + +class TestResolvePluginGithubCoords: + def test_basic_github_dict(self): + plugin = _gh_plugin(repo="acme/forge", ref="abc123") + coords = _resolve_plugin_github_coords(plugin, "github.com") + assert coords == ("github.com", "acme", "forge", "abc123", "apm.yml") + + def test_with_subdir_path(self): + plugin = _gh_plugin( + repo="acme/forge", ref="abc", path="agents/code-quality" + ) + coords = _resolve_plugin_github_coords(plugin, "github.com") + assert coords == ( + "github.com", + "acme", + "forge", + "abc", + "agents/code-quality/apm.yml", + ) + + def test_string_source_unsupported(self): + plugin = MarketplacePlugin(name="p", source="acme/forge") + assert _resolve_plugin_github_coords(plugin, "github.com") is None + + def test_non_github_type(self): + plugin = MarketplacePlugin( + name="p", source={"type": "npm", "name": "x"} + ) + assert _resolve_plugin_github_coords(plugin, "github.com") is None + + def test_repo_without_slash(self): + plugin = MarketplacePlugin( + name="p", source={"type": "github", "repo": "no-slash"} + ) + assert _resolve_plugin_github_coords(plugin, "github.com") is None + + def test_repo_with_extra_slashes_unsupported(self): + # A misconfigured ``repo: 'org/repo/extra'`` would otherwise be + # silently coerced to owner='org', name='repo/extra' -- yielding + # a malformed GitHub URL that 404s and gets reported as + # NO_MANIFEST (skipped). Reject it at the resolver so audit + # surfaces UNSUPPORTED_SOURCE instead. Sub-paths belong in the + # separate ``path`` field. + plugin = MarketplacePlugin( + name="p", + source={"type": "github", "repo": "acme/forge/extra"}, + ) + assert _resolve_plugin_github_coords(plugin, "github.com") is None + + def test_path_traversal_rejected(self): + plugin = _gh_plugin(path="../../etc") + assert _resolve_plugin_github_coords(plugin, "github.com") is None + + def test_ref_defaults_to_head(self): + plugin = MarketplacePlugin( + name="p", source={"type": "github", "repo": "acme/forge"} + ) + _host, _owner, _name, ref, _path = _resolve_plugin_github_coords( + plugin, "github.com" + ) + assert ref == "HEAD" + + def test_source_host_overrides_fallback(self): + plugin = MarketplacePlugin( + name="p", + source={ + "type": "github", + "repo": "acme/forge", + "host": "gitlab.com", + }, + ) + coords = _resolve_plugin_github_coords(plugin, "github.com") + assert coords[0] == "gitlab.com" + + +# =================================================================== +# check_plugin (with fake fetcher) +# =================================================================== + + +def _fake_fetcher(result_by_plugin): + """Build a fetcher that returns pre-seeded results keyed by plugin name.""" + + def _fetcher(plugin, marketplace_source, auth_resolver=None): + return result_by_plugin[plugin.name] + + return _fetcher + + +class TestCheckPlugin: + def test_plugin_with_direct_path_dep_is_flagged(self): + plugin = _gh_plugin("code-quality") + fetcher = _fake_fetcher( + { + "code-quality": ( + FetchStatus.OK, + { + "dependencies": { + "apm": [ + "acme/agent-forge/general/conventions", + "standards@acme-mkt", + ] + } + }, + "", + ) + } + ) + report = check_plugin(plugin, _source(), _fetcher=fetcher) + assert report.fetch_status == FetchStatus.OK + assert len(report.issues) == 1 + assert ( + report.issues[0].dep + == "acme/agent-forge/general/conventions" + ) + assert ( + report.issues[0].classification + == DepClassification.BYPASSES_MARKETPLACE + ) + assert "marketplace" in report.issues[0].suggestion + + def test_clean_plugin_has_no_issues(self): + plugin = _gh_plugin("clean") + fetcher = _fake_fetcher( + { + "clean": ( + FetchStatus.OK, + { + "dependencies": { + "apm": ["x@mkt", "./local-dev-pkg"] + } + }, + "", + ) + } + ) + report = check_plugin(plugin, _source(), _fetcher=fetcher) + assert report.fetch_status == FetchStatus.OK + assert report.issues == () + + def test_missing_manifest_is_skipped(self): + plugin = _gh_plugin("no-manifest") + fetcher = _fake_fetcher( + {"no-manifest": (FetchStatus.NO_MANIFEST, None, "not found")} + ) + report = check_plugin(plugin, _source(), _fetcher=fetcher) + assert report.fetch_status == FetchStatus.NO_MANIFEST + assert report.issues == () + + def test_parse_error_propagates_status(self): + plugin = _gh_plugin("bad-yaml") + fetcher = _fake_fetcher( + {"bad-yaml": (FetchStatus.PARSE_ERROR, None, "bad yaml")} + ) + report = check_plugin(plugin, _source(), _fetcher=fetcher) + assert report.fetch_status == FetchStatus.PARSE_ERROR + assert "bad yaml" in report.detail + + def test_network_error_propagates(self): + plugin = _gh_plugin("offline") + fetcher = _fake_fetcher( + {"offline": (FetchStatus.NETWORK_ERROR, None, "DNS")} + ) + report = check_plugin(plugin, _source(), _fetcher=fetcher) + assert report.fetch_status == FetchStatus.NETWORK_ERROR + + +class TestFetchPluginApmYmlErrorMessage: + """Regression: plugin fetch errors must not leak the MarketplaceFetchError + retry hint ('Run apm marketplace update X'), which is invalid for + plugin-level failures. + """ + + def test_network_error_message_has_no_marketplace_update_hint(self): + plugin = _gh_plugin("code-quality") + source = _source("acme-agents") + + # Make fetch_raw raise as the real auth layer would on network error + with patch( + "apm_cli.marketplace.audit.fetch_raw", + side_effect=MarketplaceError( + "fetching acme/forge/agents/x/apm.yml@abc: connection reset" + ), + ): + status, data, detail = fetch_plugin_apm_yml(plugin, source) + + assert status == FetchStatus.NETWORK_ERROR + assert data is None + # Positive assertion: the underlying reason surfaces + assert "connection reset" in detail + # Negative assertion: the misleading retry hint is not present + assert "apm marketplace update" not in detail + assert "Failed to fetch marketplace" not in detail + + +# =================================================================== +# run_audit aggregates and isolates failures +# =================================================================== + + +class TestRunAudit: + def test_mixed_marketplace(self): + clean = _gh_plugin("clean") + bad = _gh_plugin("bad") + missing = _gh_plugin("no-manifest") + manifest = _manifest(clean, bad, missing) + + fetcher = _fake_fetcher( + { + "clean": ( + FetchStatus.OK, + {"dependencies": {"apm": ["x@mkt"]}}, + "", + ), + "bad": ( + FetchStatus.OK, + { + "dependencies": { + "apm": ["acme/forge/general/conventions"] + } + }, + "", + ), + "no-manifest": (FetchStatus.NO_MANIFEST, None, ""), + } + ) + reports = run_audit(manifest, _source(), _fetcher=fetcher) + assert len(reports) == 3 + status_by_name = {r.plugin_name: r for r in reports} + assert status_by_name["clean"].issues == () + assert len(status_by_name["bad"].issues) == 1 + assert ( + status_by_name["no-manifest"].fetch_status + == FetchStatus.NO_MANIFEST + ) + + def test_one_plugin_crash_isolated(self): + good = _gh_plugin("good") + bomb = _gh_plugin("bomb") + manifest = _manifest(good, bomb) + + def fetcher(plugin, source, auth_resolver=None): + if plugin.name == "bomb": + raise RuntimeError("boom") + return ( + FetchStatus.OK, + {"dependencies": {"apm": ["x@mkt"]}}, + "", + ) + + reports = run_audit(manifest, _source(), _fetcher=fetcher) + names = [r.plugin_name for r in reports] + assert names == ["good", "bomb"] + by_name = {r.plugin_name: r for r in reports} + assert by_name["good"].fetch_status == FetchStatus.OK + assert by_name["bomb"].fetch_status == FetchStatus.NETWORK_ERROR + assert "boom" in by_name["bomb"].detail + + +# =================================================================== +# CLI integration +# =================================================================== + + +class TestAuditCLI: + """End-to-end through the Click command, fetcher stubbed at the seam.""" + + def _invoke(self, runner, extra_args=()): + from apm_cli.commands.marketplace import marketplace + + return runner.invoke(marketplace, ["audit", "mymarket", *extra_args]) + + def _stub_registry_and_manifest(self, manifest, source): + # Patch at the import site (apm_cli.commands.marketplace.audit) so the + # module-level `from ...marketplace.X import Y` bindings are replaced. + patches = [ + patch( + "apm_cli.commands.marketplace.audit.get_marketplace_by_name", + return_value=source, + ), + patch( + "apm_cli.commands.marketplace.audit.fetch_marketplace", + return_value=manifest, + ), + ] + for p in patches: + p.start() + return patches + + def _stop_patches(self, patches): + for p in patches: + p.stop() + + def test_reports_bypass_dep(self, runner): + source = _source("mymarket") + manifest = _manifest(_gh_plugin("code-quality")) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + return_value=( + FetchStatus.OK, + { + "dependencies": { + "apm": ["acme/forge/general/conventions"] + } + }, + "", + ), + ): + result = self._invoke(runner) + finally: + self._stop_patches(patches) + + assert result.exit_code == 0, result.output + assert "code-quality" in result.output + assert "acme/forge/general/conventions" in result.output + assert "1 bypass warning" in result.output + + def test_strict_exits_nonzero_on_bypass(self, runner): + source = _source("mymarket") + manifest = _manifest(_gh_plugin("code-quality")) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + return_value=( + FetchStatus.OK, + {"dependencies": {"apm": ["acme/forge/general/x"]}}, + "", + ), + ): + result = self._invoke(runner, extra_args=("--strict",)) + finally: + self._stop_patches(patches) + + assert result.exit_code == 1, result.output + + def test_strict_exits_nonzero_on_unverifiable_plugin(self, runner): + # PR contract: --strict exits non-zero on bypass warnings *or* + # unverifiable plugins (NETWORK_ERROR / PARSE_ERROR). This guards + # the second half of that contract, complementing the bypass test + # above. + source = _source("mymarket") + manifest = _manifest(_gh_plugin("offline-plugin")) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + return_value=( + FetchStatus.NETWORK_ERROR, + None, + "fetching offline-plugin: DNS failure", + ), + ): + result = self._invoke(runner, extra_args=("--strict",)) + finally: + self._stop_patches(patches) + + assert result.exit_code == 1, result.output + assert "could not verify" in result.output + assert "1 unverifiable error" in result.output + + def test_unverifiable_without_strict_exits_zero(self, runner): + # Default mode is informational: unverifiable plugins surface in + # output but the command still exits 0. + source = _source("mymarket") + manifest = _manifest(_gh_plugin("offline-plugin")) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + return_value=( + FetchStatus.NETWORK_ERROR, + None, + "DNS failure", + ), + ): + result = self._invoke(runner) + finally: + self._stop_patches(patches) + + assert result.exit_code == 0, result.output + assert "1 unverifiable error" in result.output + + def test_clean_marketplace_exits_zero(self, runner): + source = _source("mymarket") + manifest = _manifest(_gh_plugin("clean")) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + return_value=( + FetchStatus.OK, + {"dependencies": {"apm": ["x@mkt"]}}, + "", + ), + ): + result = self._invoke(runner, extra_args=("--strict",)) + finally: + self._stop_patches(patches) + + assert result.exit_code == 0, result.output + assert "0 bypass warnings" in result.output + # Default mode hides the "Audit Results:" header on an all-clean run + # so the summary is not preceded by an empty section. + assert "Audit Results:" not in result.output + + def test_verbose_shows_clean_and_skipped_detail(self, runner): + # Verbose surfaces per-plugin context that the default mode keeps + # silent: clean plugins get a "marketplace-resolved" success line, + # and skipped plugins get their skip reason inline with the status + # code. Without this coverage, regressions in those branches would + # only surface when a user runs the command interactively. + source = _source("mymarket") + manifest = _manifest(_gh_plugin("clean"), _gh_plugin("missing-yml")) + + def fetcher(plugin, _src, _auth=None): + if plugin.name == "clean": + return ( + FetchStatus.OK, + {"dependencies": {"apm": ["x@mkt"]}}, + "", + ) + return ( + FetchStatus.NO_MANIFEST, + None, + "no apm.yml at 'apm.yml' @ HEAD", + ) + + patches = self._stub_registry_and_manifest(manifest, source) + try: + with patch( + "apm_cli.marketplace.audit.fetch_plugin_apm_yml", + side_effect=fetcher, + ): + result = self._invoke(runner, extra_args=("-v",)) + finally: + self._stop_patches(patches) + + assert result.exit_code == 0, result.output + # Clean plugin's verbose-only success line + assert "clean: deps are marketplace-resolved" in result.output + # Skipped plugin's verbose-only detail with status code + assert "missing-yml: skipped (no_manifest" in result.output diff --git a/tests/unit/marketplace/test_marketplace_client.py b/tests/unit/marketplace/test_marketplace_client.py index 443d97f1..e6f441f3 100644 --- a/tests/unit/marketplace/test_marketplace_client.py +++ b/tests/unit/marketplace/test_marketplace_client.py @@ -9,7 +9,7 @@ import pytest from apm_cli.marketplace import client as client_mod -from apm_cli.marketplace.errors import MarketplaceFetchError +from apm_cli.marketplace.errors import MarketplaceError, MarketplaceFetchError from apm_cli.marketplace.models import MarketplaceSource @@ -527,3 +527,119 @@ def test_github_host_passes_guard(self): result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=mock_resolver) assert result == {"name": "ok", "plugins": []} + + +class TestFetchRaw: + """``fetch_raw`` -- public primitive for fetching arbitrary files at a ref.""" + + def test_wraps_auth_failure_in_marketplace_error_not_fetch_error(self): + # Audit relies on this contract: fetch_raw raises the neutral + # MarketplaceError so callers can craft per-plugin context, instead + # of inheriting MarketplaceFetchError's "run apm marketplace update" + # hint -- which is wrong at plugin granularity. + auth_resolver = MagicMock() + auth_resolver.try_with_fallback.side_effect = RuntimeError("connection reset") + + with patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", + return_value=None, + ): + with pytest.raises(MarketplaceError) as excinfo: + client_mod.fetch_raw( + host="github.com", + owner="acme", + repo="forge", + file_path="agents/x/apm.yml", + ref="abc123", + auth_resolver=auth_resolver, + ) + + # Must NOT be the marketplace-specific subclass (whose default + # message format embeds "Run 'apm marketplace update X' to retry"). + assert not isinstance(excinfo.value, MarketplaceFetchError) + # Must preserve the underlying reason and the file coords so the + # caller can build a useful per-plugin diagnostic. + msg = str(excinfo.value) + assert "connection reset" in msg + assert "acme/forge/agents/x/apm.yml@abc123" in msg + + def test_returns_proxy_bytes_when_proxy_hits(self): + # Proxy success short-circuits before any auth path is consulted. + with patch( + "apm_cli.marketplace.client._try_proxy_fetch_raw", + return_value=b"hello", + ): + result = client_mod.fetch_raw( + host="github.com", + owner="o", + repo="r", + file_path="f", + ref="main", + ) + assert result == b"hello" + + def test_raises_when_proxy_only_blocks_github(self): + # PROXY_REGISTRY_ONLY=1 must keep us off GitHub even when the + # proxy has no entry. Because that path cannot verify whether + # the file exists, fetch_raw must raise MarketplaceError instead + # of returning None -- otherwise audit would silently report the + # plugin as NO_MANIFEST in proxy-only environments rather than + # surfacing it as unverifiable. + cfg = MagicMock(enforce_only=True) + with patch( + "apm_cli.marketplace.client._try_proxy_fetch_raw", + return_value=None, + ), patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", + return_value=cfg, + ): + with pytest.raises(MarketplaceError) as excinfo: + client_mod.fetch_raw( + host="github.com", + owner="o", + repo="r", + file_path="f", + ref="main", + ) + + msg = str(excinfo.value) + assert "PROXY_REGISTRY_ONLY" in msg + # File coordinates surface so the audit log line is actionable. + assert "o/r/f@main" in msg + # Must remain the neutral base class, not the marketplace-specific + # subclass whose default message embeds an "apm marketplace update" + # retry hint that does not apply here. + assert not isinstance(excinfo.value, MarketplaceFetchError) + + def test_non_github_host_rejected_before_request(self): + # Defense-in-depth, mirrors TestFetchFileHostKindGuard for + # _fetch_file: if a plugin source somehow surfaces a non-GitHub + # host (e.g. ``gitlab.com``) we MUST NOT attach a GitHub PAT to + # the fallback request -- doing so would leak credentials. + # ``fetch_raw`` must fail closed with MarketplaceError and never + # invoke requests.get. + with patch( + "apm_cli.marketplace.client._try_proxy_fetch_raw", + return_value=None, + ), patch( + "apm_cli.deps.registry_proxy.RegistryConfig.from_env", + return_value=None, + ), patch( + "apm_cli.marketplace.client.requests.get", + ) as mock_get: + with pytest.raises(MarketplaceError) as excinfo: + client_mod.fetch_raw( + host="gitlab.com", + owner="acme", + repo="forge", + file_path="apm.yml", + ref="v1", + ) + + # No HTTP request must be issued -- proves credentials cannot leak. + mock_get.assert_not_called() + msg = str(excinfo.value) + assert _quoted_hosts(msg) == {"gitlab.com"} + assert "not a supported plugin source" in msg + # Coordinates remain in the message so audit's per-plugin line is actionable. + assert "acme/forge/apm.yml@v1" in msg