diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a37749a9..e691da4a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 of `~/.hermes/config.yaml` (written atomically with `0o600` perms, preserving unrelated config keys and refusing to overwrite a malformed file). `HERMES_HOME` overrides the Hermes home directory. See the [Hermes integration guide](https://microsoft.github.io/apm/integrations/hermes/). +- `apm marketplace add` now accepts git URLs with `#ref`, local file paths, + and hosted `marketplace.json` URLs -- so teams can consume private, + offline, or hosted catalogs without publishing a GitHub repo (closes + #676). (#1739) - Enterprise bootstrap mirror mode lets `install.sh`, `install.ps1`, and `apm self-update` use internal release, installer, and PyPI mirrors with fail-closed public fallback, and closes #1680. (#1733) ### Fixed diff --git a/docs/src/content/docs/consumer/installing-from-marketplaces.md b/docs/src/content/docs/consumer/installing-from-marketplaces.md index 146fd1f6e..1c1be9b04 100644 --- a/docs/src/content/docs/consumer/installing-from-marketplaces.md +++ b/docs/src/content/docs/consumer/installing-from-marketplaces.md @@ -65,36 +65,53 @@ reproducible across machines and CI. ## Local and self-hosted marketplaces -`apm marketplace add` accepts more than GitHub-hosted repos. The same -register / browse / install / update workflow works against: +`apm marketplace add` can register marketplaces from git repositories, +local files, or hosted `marketplace.json` catalogs. The same register / +browse / install / update workflow works against: - **Local filesystem paths** -- `apm marketplace add /srv/marketplaces/agent-forge` - (or a relative path, or `~/code/marketplace`). Useful for - privacy-sensitive packages, offline workflows, and air-gapped - environments. Local marketplaces with relative plugin sources - install by copying from disk via `LocalDependencySource`. + (or a relative path, `~/code/marketplace`, or a direct + `marketplace.json` file). Useful for privacy-sensitive packages, + offline workflows, and air-gapped environments. Local marketplaces + with relative plugin sources install by copying from disk via + `LocalDependencySource`. - **`file://` URIs** -- `apm marketplace add file:///srv/marketplaces/agent-forge.git`. Behaves the same as a local path. +- **Git HTTPS URLs with `#ref`** -- + `apm marketplace add https://gitlab.com/acme/agent-forge.git#v1.0.0`. + APM strips the fragment into the stored ref. If you omit `#ref` + (or `--ref`), APM warns because the default branch can move. +- **Hosted `marketplace.json` URLs** -- + `apm marketplace add https://catalog.example.com/marketplace.json --name catalog`. + APM fetches the JSON directly over HTTPS, caches ETag/Last-Modified + metadata, and records the source URL plus content digest in the + lockfile when packages are installed from it. - **Generic git URLs** -- any host APM does not classify as GitHub or GitLab family flows through subprocess `git` and `GitCache`. Includes Azure DevOps (auth via `ADO_APM_PAT`), Gitea, Bitbucket Server, and self-hosted git servers. - **SSH URLs** -- `git@gitea.example.com:org/repo.git`. The host - is extracted, classified, and routed through the matching - fetcher. + is extracted, classified, and routed through the matching fetcher. + +:::note[Hosted JSON is public HTTPS] +Hosted `marketplace.json` URLs are public HTTPS sources: APM does not +send custom auth headers. Use git-backed marketplaces for private +catalogs. +::: For generic-git marketplaces, `marketplace.json` is fetched via a sparse-cone clone (only the manifest path is downloaded); APM does -not forward `GITHUB_APM_PAT` or `GITLAB_APM_TOKEN` to non-GitHub / -non-GitLab hosts. Authentication falls through to the host's -`*_APM_PAT` (e.g. `ADO_APM_PAT`) or your local -`git credential-manager`. See +not forward `GITHUB_APM_PAT` or `GITLAB_APM_PAT` to non-GitHub / +non-GitLab hosts. Authentication falls through to the host's matching +`*_APM_PAT` variable (such as `ADO_APM_PAT`) or local git credential +helper when one is configured. See [Authentication](../authentication/). **Lockfile note.** Installs from a local marketplace record a local-path source in `apm.lock.yaml`. Lockfiles produced this way -are machine-specific -- do not commit them into a shared repo. See -[lockfile reference](../../reference/lockfile-spec/). +are machine-specific -- do not commit them into a shared repo. Remote +`marketplace.json` installs record `source_url` and `source_digest` +provenance. See [lockfile reference](../../reference/lockfile-spec/). ## Where next diff --git a/docs/src/content/docs/reference/cli/marketplace.md b/docs/src/content/docs/reference/cli/marketplace.md index bec4ddb64..5ba0692f5 100644 --- a/docs/src/content/docs/reference/cli/marketplace.md +++ b/docs/src/content/docs/reference/cli/marketplace.md @@ -61,11 +61,16 @@ Register a marketplace from a source reference. Accepted forms: - `OWNER/REPO` -- GitHub shorthand (`acme/marketplace`). - `HOST/OWNER/.../REPO` -- non-GitHub host shorthand (`gitlab.com/team/marketplace`). -- HTTPS URL -- any git host, including Azure DevOps, GitLab, - Gitea, Bitbucket Server, or a self-hosted git server. +- HTTPS git URL -- any git host, including Azure DevOps, GitLab, + Gitea, Bitbucket Server, or a self-hosted git server. Add `#ref` + to pin the marketplace, for example + `https://gitlab.com/acme/marketplace.git#v1.0.0`. +- Hosted `marketplace.json` URL -- + `https://catalog.example.com/marketplace.json`. - SSH URL -- `git@host:org/repo.git` style. - Local filesystem path -- absolute (`/srv/marketplaces/agent-forge`), - relative (`./local-mkt`), or home-based (`~/code/marketplace`). + relative (`./local-mkt`), home-based (`~/code/marketplace`), or a + direct `marketplace.json` file. - `file://` URI -- `file:///srv/marketplaces/agent-forge.git`. ```bash @@ -79,14 +84,18 @@ apm marketplace add gitlab.com/my-org/awesome-agents --host gitlab.com apm marketplace add https://dev.azure.com/contoso/eng/_git/agent-forge \ --name agent-forge -# Gitea / Bitbucket Server / self-hosted git -apm marketplace add https://gitea.example.com/org/repo.git --name custom +# Gitea / Bitbucket Server / self-hosted git, pinned with #ref +apm marketplace add https://gitea.example.com/org/repo.git#v1.0.0 --name custom + +# Hosted marketplace.json URL +apm marketplace add https://catalog.example.com/marketplace.json --name catalog # SSH apm marketplace add git@gitea.example.com:org/repo.git --name custom -# Local filesystem (bare repo or working directory) +# Local filesystem (bare repo, working directory, or marketplace.json file) apm marketplace add /srv/marketplaces/agent-forge.git --name agent-forge +apm marketplace add ./vendor/marketplace.json --name vendor # file:// URI apm marketplace add file:///srv/marketplaces/agent-forge.git --name agent-forge @@ -95,19 +104,22 @@ apm marketplace add file:///srv/marketplaces/agent-forge.git --name agent-forge | Flag | Description | |---|---| | `--name`, `-n` | Display name. Defaults to the repo name. | -| `--ref`, `-r` | Git ref (branch, tag, or SHA). Default: `main`. | +| `--ref`, `-r` | Git ref (branch, tag, or SHA). Default: `main`. Applies only to git-backed sources. For HTTPS git URLs, a `#ref` fragment is equivalent and is stored as the ref. | | `--branch`, `-b` | Deprecated alias for `--ref`. | -| `--host` | Git host FQDN. Default: `github.com`. Ignored when `SOURCE` is a URL or local path. | +| `--host` | Git host FQDN for `OWNER/REPO` shorthand. Default: `github.com`. Ignored when `SOURCE` already carries a host (URL, hosted `marketplace.json`, or local path); a warning is shown for hosted JSON, local paths, or a conflicting embedded host. | | `--verbose`, `-v` | Show detailed output. | **Trust boundary.** APM forwards its authentication tokens (`GITHUB_APM_PAT`, `GITLAB_APM_PAT`) only when the marketplace -host is classified as GitHub or GitLab family. For any other host --- generic HTTPS, SSH, Azure DevOps, self-hosted -- the +host is classified as GitHub or GitLab family. For any other git +host -- generic HTTPS, SSH, Azure DevOps, self-hosted -- the marketplace is fetched via subprocess `git` through `GitCache`, -and authentication falls through to the host's APM PAT (e.g. -`ADO_APM_PAT` for Azure DevOps) or your local -`git credential-manager`. See +and authentication falls through to the host's local git credential +helper and matching `*_APM_PAT` variables such as `ADO_APM_PAT`. +Hosted `marketplace.json` URLs are public HTTPS only: APM sends no auth +headers. Use a git-backed marketplace for private catalogs. When packages +are installed from a hosted JSON URL, the lockfile records the source URL and +fetched content digest. See [`getting-started/authentication`](../../../getting-started/authentication/). **Azure DevOps.** ADO-hosted marketplaces fetch `marketplace.json` diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 52db0df65..2821d5c77 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -141,6 +141,8 @@ Each item in `dependencies` describes one resolved package. | `is_dev` | bool | no | `true` when the dep was declared under `devDependencies`. | | `discovered_via` | string | no | Marketplace name that surfaced this package (provenance). | | `marketplace_plugin_name` | string | no | Plugin name as listed in that marketplace. | +| `source_url` | string | no | Canonical marketplace source URL when the package came from a hosted `marketplace.json` catalog. | +| `source_digest` | string | no | `sha256:` digest of the hosted `marketplace.json` bytes used for resolution. | | `is_insecure` | bool | no | `true` when the source URL was `http://`. | | `allow_insecure` | bool | no | `true` when the manifest explicitly opted in to the insecure source. | | `constraint` | string | git-source semver only | The original semver range from `apm.yml` (`^1.2.0`, `~1.4`). Present when `ref:` was a range; used by drift detection so a manifest range vs. a locked tag (`v1.5.3`) is not a false positive, and by lockfile replay to pin the resolved tag deterministically across installs. | diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 0c07e020f..e6ea615d9 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -118,7 +118,7 @@ Credentials resolve via `APM_REGISTRY_TOKEN_{NAME}` env var (or `apm config set | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm marketplace add SOURCE` | Register a marketplace. `SOURCE` accepts `OWNER/REPO`, `HOST/OWNER/REPO`, nested `HOST/group/sub/.../REPO`, HTTPS URL (any git host -- GitHub, GitLab, ADO, Gitea, self-hosted), SSH URL (`git@host:org/repo.git`), local filesystem path, or `file://` URI. | `-n NAME`, `-r REF`, `--host HOST` | +| `apm marketplace add SOURCE` | Register a marketplace. `SOURCE` accepts `OWNER/REPO`, `HOST/OWNER/REPO`, nested `HOST/group/sub/.../REPO`, HTTPS git URL with optional `#ref`, hosted `marketplace.json` URL, SSH URL (`git@host:org/repo.git`), local directory or file path, or `file://` URI. `--ref` applies only to git-backed sources; `--host` applies only to shorthand sources. | `-n NAME`, `-r REF`, `--host HOST` | | `apm marketplace list` | List registered marketplaces | -- | | `apm marketplace browse NAME` | Browse marketplace plugins | -- | | `apm marketplace update [NAME]` | Update marketplace index | -- | diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index fbeca6a6a..6df1e5c6d 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -430,10 +430,7 @@ def warning_handler(msg): if logger: logger.validation_fail(package, reason) continue - marketplace_provenance = { - "discovered_via": marketplace_name, - "marketplace_plugin_name": plugin_name, - } + marketplace_provenance = resolution.provenance(marketplace_name, plugin_name) package = canonical_str marketplace_dep_ref = getattr(resolution, "dependency_reference", None) except Exception as mkt_err: diff --git a/src/apm_cli/commands/marketplace/__init__.py b/src/apm_cli/commands/marketplace/__init__.py index 764e223c2..e5ec2fafa 100644 --- a/src/apm_cli/commands/marketplace/__init__.py +++ b/src/apm_cli/commands/marketplace/__init__.py @@ -13,6 +13,8 @@ import sys import traceback from pathlib import Path +from typing import TYPE_CHECKING +from urllib.parse import urlsplit, urlunsplit import click import yaml @@ -50,6 +52,9 @@ from ...utils.path_security import PathTraversalError, validate_path_segments from .._helpers import _get_console, _is_interactive +if TYPE_CHECKING: + from ...marketplace.models import MarketplaceSource + logger = logging.getLogger(__name__) # Restore builtins shadowed by subcommand names @@ -495,6 +500,8 @@ def _marketplace_add_unsupported_host_error( Examples: apm marketplace add owner/repo apm marketplace add github.com/owner/repo + apm marketplace add https://github.com/owner/repo#v1.0.0 + apm marketplace add https://catalog.example.com/marketplace.json --name catalog apm marketplace add https://gitlab.com/group/repo apm marketplace add https://dev.azure.com/org/proj/_git/repo --name apm-mkt apm marketplace add git@gitea.example.com:org/repo.git --name custom @@ -505,16 +512,26 @@ def _marketplace_add_unsupported_host_error( @marketplace.command(help="Register a marketplace", epilog=_ADD_EPILOG) @click.argument("source", metavar="SOURCE", required=True) @click.option("--name", "-n", default=None, help="Display name (defaults to repo name)") -@click.option("--ref", "-r", default=None, help="Branch, tag, or commit to use (default: main)") +@click.option( + "--ref", + "-r", + default=None, + help="Git ref (branch, tag, or commit). Default: main. Applies to git-backed sources only.", +) @click.option("--branch", "-b", default=None, help="Deprecated alias for --ref", hidden=True) -@click.option("--host", default=None, help="Git host FQDN (default: github.com)") +@click.option( + "--host", + default=None, + help="Git host FQDN for OWNER/REPO shorthand (default: github.com)", +) @click.option("--verbose", "-v", is_flag=True, help="Show detailed output") def add(source, name, ref, branch, host, verbose): """Register a marketplace. SOURCE accepts: OWNER/REPO shorthand, HOST/OWNER/REPO shorthand, a full - HTTPS URL (GitHub, GitLab, Azure DevOps, Gitea, Bitbucket Server, or - any self-hosted git server), an SSH URL (``git@host:org/repo.git``), + HTTPS git URL with optional ``#ref`` (GitHub, GitLab, Azure DevOps, + Gitea, Bitbucket Server, or any self-hosted git server), a hosted + ``marketplace.json`` URL, an SSH URL (``git@host:org/repo.git``), a local filesystem path, or a ``file://`` URI. """ logger = CommandLogger("marketplace-add", verbose=verbose) @@ -524,19 +541,28 @@ def add(source, name, ref, branch, host, verbose): from ...marketplace.registry import add_marketplace from ...utils.github_host import is_valid_fqdn + source_arg, fragment_ref = _split_source_fragment_ref(source) + # --ref / --branch reconciliation. --branch stays as a hidden alias - # for one release so legacy invocations keep working; passing both - # is a hard error so we never silently pick one. + # for one release so legacy invocations keep working; passing multiple + # ref sources is a hard error so we never silently pick one. + explicit_ref = ref is not None or branch is not None if ref is not None and branch is not None: logger.error( "--ref and --branch are mutually exclusive. Use --ref (--branch is a deprecated alias).", symbol="error", ) sys.exit(1) - effective_ref = ref if ref is not None else (branch if branch is not None else "main") + if fragment_ref and explicit_ref: + logger.error( + "Do not combine a git URL #ref with --ref or --branch. Use one ref source.", + symbol="error", + ) + sys.exit(1) + effective_ref = fragment_ref or ref or branch or "main" try: - url, kind, resolved_host = _parse_marketplace_source(source, host) + url, kind, resolved_host = _parse_marketplace_source(source_arg, host) except PathTraversalError: logger.error( f"Invalid source '{source}': contains a path-traversal sequence. " @@ -557,7 +583,14 @@ def add(source, name, ref, branch, host, verbose): # --host is meaningful only for shorthand OWNER/REPO inputs. For URL # / SSH / local-path inputs the host is already embedded; warn that # --host is being ignored rather than silently overriding. - if host is not None and kind == "local": + is_direct_url = _is_remote_marketplace_json_url(url) + + if host is not None and is_direct_url: + logger.warning( + "--host is ignored when SOURCE is a hosted marketplace.json URL.", + symbol="warning", + ) + elif host is not None and kind == "local": logger.warning( "--host is ignored when SOURCE is a local filesystem path.", symbol="warning", @@ -566,7 +599,7 @@ def add(source, name, ref, branch, host, verbose): host is not None and host.strip().lower() != (resolved_host or "").lower() and kind in ("git", "github", "gitlab") - and (source.startswith(("https://", "git@", "file://"))) + and (source_arg.startswith(("https://", "git@", "file://"))) ): logger.warning( "--host is ignored when SOURCE is a full URL.", @@ -575,7 +608,7 @@ def add(source, name, ref, branch, host, verbose): # Trust gate is now scoped to kinds that would forward an APM token # via header injection. The subprocess git path (kind == "git") - # never forwards GITHUB_APM_PAT / GITLAB_APM_TOKEN -- AuthResolver + # never forwards GITHUB_APM_PAT / GITLAB_APM_PAT -- AuthResolver # only emits credentials matching the classified host. Local-kind # fetches use no credentials at all. if kind in ("github", "gitlab"): @@ -607,8 +640,18 @@ def add(source, name, ref, branch, host, verbose): # Surface progress before the slow probe + fetch (5-30s for generic-git) # so the user sees activity instead of staring at a blank terminal. - provisional_label = name or _default_alias_from_url(url) + provisional_label = name or ( + _default_alias_from_remote_url(url) if is_direct_url else _default_alias_from_url(url) + ) logger.start(f"Registering marketplace '{provisional_label}'...", symbol="gear") + if _should_warn_unpinned_git_url( + source_arg, kind, is_direct_url, fragment_ref, explicit_ref + ): + logger.warning( + "Pin this git marketplace with a #ref (for example, " + f"{source_arg}#v1.0.0) or --ref to avoid mutable branch updates.", + symbol="warning", + ) # Probe for marketplace.json location. The probe source's name is a # placeholder -- _auto_detect_path only consults url/ref/path/kind. @@ -616,9 +659,13 @@ def add(source, name, ref, branch, host, verbose): probe_source = MarketplaceSource( name=probe_name, url=url, - ref=effective_ref, + ref="" if is_direct_url else effective_ref, + path="" if is_direct_url else "marketplace.json", ) - detected_path = _auto_detect_path(probe_source) + if is_direct_url or _local_source_points_to_file(probe_source): + detected_path = "" + else: + detected_path = _auto_detect_path(probe_source) if detected_path is None: logger.error( @@ -632,7 +679,7 @@ def add(source, name, ref, branch, host, verbose): fetch_source = MarketplaceSource( name=probe_name, url=url, - ref=effective_ref, + ref="" if is_direct_url else effective_ref, path=detected_path, ) manifest = fetch_marketplace(fetch_source, force_refresh=True) @@ -663,15 +710,21 @@ def add(source, name, ref, branch, host, verbose): ) logger.verbose_detail(f" Source: {fetch_source.display_source}") - logger.verbose_detail(f" Kind: {kind}") - logger.verbose_detail(f" Ref: {effective_ref}") - logger.verbose_detail(f" Detected path: {detected_path}") + logger.verbose_detail( + f" Source type: {_display_source_kind(fetch_source.kind, is_direct_url)}" + ) + if not is_direct_url: + logger.verbose_detail(f" Ref: {effective_ref}") + if detected_path: + logger.verbose_detail(f" Detected path: {detected_path}") + elif not is_direct_url: + logger.verbose_detail(" Detected path: direct local file") logger.verbose_detail(f" Alias source: {alias_source}") final_source = MarketplaceSource( name=display_name, url=url, - ref=effective_ref, + ref="" if is_direct_url else effective_ref, path=detected_path, ) add_marketplace(final_source) @@ -696,6 +749,82 @@ def add(source, name, ref, branch, host, verbose): sys.exit(1) +def _split_source_fragment_ref(source: str) -> tuple[str, str]: + """Split an HTTPS git URL #ref fragment from the URL stored in the registry.""" + raw = (source or "").strip() + if not raw.lower().startswith("https://"): + return raw, "" + parsed = urlsplit(raw) + if not parsed.fragment: + return raw, "" + clean_url = urlunsplit((parsed.scheme, parsed.netloc, parsed.path, parsed.query, "")) + return clean_url, parsed.fragment + + +def _is_remote_marketplace_json_url(url: str) -> bool: + """Return True when *url* names a hosted marketplace.json document. + + Delegates to the single shared classifier in ``marketplace.models`` so + the CLI and ``MarketplaceSource`` cannot drift on the url-kind decision. + """ + from ...marketplace.models import url_names_remote_manifest + + return url_names_remote_manifest(url) + + +def _should_warn_unpinned_git_url( + source: str, + kind: str, + is_direct_url: bool, + fragment_ref: str, + explicit_ref: bool, +) -> bool: + """Return True when a git URL source uses the implicit mutable default ref.""" + if is_direct_url or fragment_ref or explicit_ref: + return False + return source.lower().startswith("https://") and kind in {"github", "gitlab", "git"} + + +def _local_source_points_to_file(source: MarketplaceSource) -> bool: + """Return True when a local marketplace source points directly to a file.""" + if source.kind != "local": + return False + try: + return Path(source.local_path).expanduser().is_file() + except OSError: + return False + + +def _display_source_kind(kind: str, is_direct_url: bool) -> str: + """Return a human-readable source kind for verbose CLI output.""" + if is_direct_url: + return "hosted marketplace.json URL" + labels = { + "github": "GitHub repository", + "gitlab": "GitLab repository", + "git": "generic git repository", + "local": "local filesystem path", + } + return labels.get(kind, kind) + + +def _default_alias_from_remote_url(url: str) -> str: + """Derive a stable default alias for a direct remote marketplace.json URL.""" + try: + parsed = urlsplit(url) + except ValueError: + return "marketplace" + host = (parsed.hostname or "marketplace").lower().split(":", 1)[0] + path_segments = [seg for seg in (parsed.path or "").split("/") if seg] + parent = "" + if len(path_segments) >= 2 and path_segments[-1].lower() == "marketplace.json": + parent = path_segments[-2] + if parent: + alias = f"{host}-{parent}" + return re.sub(r"[^a-zA-Z0-9._-]", "_", alias).strip("._-") or host + return host + + def _default_alias_from_url(url: str) -> str: """Derive a default marketplace alias from a parsed URL. @@ -850,7 +979,7 @@ def update(name, verbose): if name: source = get_marketplace_by_name(name) logger.start(f"Refreshing marketplace '{name}'...", symbol="gear") - clear_marketplace_cache(name, host=source.host) + clear_marketplace_cache(source=source) manifest = fetch_marketplace(source, force_refresh=True) logger.success( f"Marketplace '{name}' updated ({len(manifest.plugins)} plugins)", @@ -864,7 +993,7 @@ def update(name, verbose): logger.start(f"Refreshing {len(sources)} marketplace(s)...", symbol="gear") for s in sources: try: - clear_marketplace_cache(s.name, host=s.host) + clear_marketplace_cache(source=s) manifest = fetch_marketplace(s, force_refresh=True) logger.tree_item(f" {s.name} ({len(manifest.plugins)} plugins)") except Exception as exc: @@ -910,7 +1039,7 @@ def remove(name, yes, verbose): return remove_marketplace(name) - clear_marketplace_cache(name, host=source.host) + clear_marketplace_cache(source=source) logger.success(f"Marketplace '{name}' removed", symbol="check") except Exception as e: diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 5de15840c..203b98b0a 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -52,6 +52,8 @@ class LockedDependency: is_dev: bool = False # True for devDependencies discovered_via: str | None = None # Marketplace name (provenance) marketplace_plugin_name: str | None = None # Plugin name in marketplace + source_url: str | None = None # Canonical marketplace source URL + source_digest: str | None = None # sha256 digest of the marketplace manifest is_insecure: bool = False # True when the locked source was http:// allow_insecure: bool = False # True when the manifest explicitly allowed HTTP skill_subset: list[str] = field(default_factory=list) # Sorted skill names for SKILL_BUNDLE @@ -127,6 +129,10 @@ def to_dict(self) -> dict[str, Any]: result["discovered_via"] = self.discovered_via if self.marketplace_plugin_name: result["marketplace_plugin_name"] = self.marketplace_plugin_name + if self.source_url: + result["source_url"] = self.source_url + if self.source_digest: + result["source_digest"] = self.source_digest if self.is_insecure: result["is_insecure"] = True if self.allow_insecure: @@ -202,6 +208,8 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: "is_dev", "discovered_via", "marketplace_plugin_name", + "source_url", + "source_digest", "is_insecure", "allow_insecure", "skill_subset", @@ -236,6 +244,8 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: is_dev=data.get("is_dev", False), discovered_via=data.get("discovered_via"), marketplace_plugin_name=data.get("marketplace_plugin_name"), + source_url=data.get("source_url"), + source_digest=data.get("source_digest"), is_insecure=data.get("is_insecure", False), allow_insecure=data.get("allow_insecure", False), skill_subset=list(data.get("skill_subset") or []), diff --git a/src/apm_cli/install/phases/lockfile.py b/src/apm_cli/install/phases/lockfile.py index f6c1edbc0..73f1d9a53 100644 --- a/src/apm_cli/install/phases/lockfile.py +++ b/src/apm_cli/install/phases/lockfile.py @@ -194,6 +194,8 @@ def _attach_marketplace_provenance(self, lockfile: LockFile) -> None: lockfile.dependencies[dep_key].marketplace_plugin_name = prov.get( "marketplace_plugin_name" ) + lockfile.dependencies[dep_key].source_url = prov.get("source_url") + lockfile.dependencies[dep_key].source_digest = prov.get("source_digest") def _merge_existing(self, lockfile: LockFile) -> None: if self.ctx.existing_lockfile and not self.ctx.update_refs: diff --git a/src/apm_cli/marketplace/client.py b/src/apm_cli/marketplace/client.py index 63752a502..f7c9eb232 100644 --- a/src/apm_cli/marketplace/client.py +++ b/src/apm_cli/marketplace/client.py @@ -7,8 +7,10 @@ - ``git`` -> generic git URL (ADO, Gitea, self-hosted, etc.) via subprocess through ``GitCache``; ``git ls-remote`` is the freshness check, no JSON sidecar cache. -- ``local`` -> bare repo (``git --git-dir=... show :``) or working - directory (direct file read with symlink-escape guard); no cache. +- ``local`` -> bare repo (``git --git-dir=... show :``), working + directory, or direct manifest file (path-containment guard); no cache. +- ``url`` -> direct hosted ``marketplace.json`` over HTTPS with digest and + ETag/Last-Modified cache metadata. When ``PROXY_REGISTRY_URL`` is set, GitHub/GHES fetches go through the Artifactory Archive Entry Download proxy first. Cache lives at @@ -16,6 +18,7 @@ """ import contextlib +import hashlib import json import logging import os @@ -23,6 +26,7 @@ import subprocess import time from collections.abc import Callable +from dataclasses import dataclass from pathlib import Path from urllib.parse import quote, urlsplit @@ -39,8 +43,23 @@ logger = logging.getLogger(__name__) + +@dataclass(frozen=True) +class FetchResult: + """Cache-layer DTO for a direct remote marketplace.json fetch.""" + + data: dict + digest: str + etag: str = "" + last_modified: str = "" + + _CACHE_TTL_SECONDS = 3600 # 1 hour +_MAX_MARKETPLACE_JSON_BYTES = 10 * 1024 * 1024 +_HTTP_CHUNK_BYTES = 1024 * 1024 _CACHE_DIR_NAME = os.path.join("cache", "marketplace") +_HTTP_SESSION = requests.Session() +_HTTP_SESSION.max_redirects = 5 # Candidate locations for marketplace.json in a repository (priority order) _MARKETPLACE_PATHS = [ @@ -96,6 +115,8 @@ def _sanitize_cache_name(name: str) -> str: def _cache_key(source: MarketplaceSource) -> str: """Cache key that includes kind+host to avoid collisions across hosts.""" kind = source.kind + if kind == "url": + return f"url__{hashlib.sha256(source.url.encode()).hexdigest()[:16]}" if kind == "local": return f"local__{_sanitize_cache_name(source.name)}" if kind == "git": @@ -148,18 +169,29 @@ def _read_stale_cache(name: str) -> dict | None: return None -def _write_cache(name: str, data: dict) -> None: +def _write_cache( + name: str, + data: dict, + *, + index_digest: str = "", + etag: str = "", + last_modified: str = "", +) -> None: """Write marketplace data and metadata to cache.""" data_path = _cache_data_path(name) meta_path = _cache_meta_path(name) try: with open(data_path, "w", encoding="utf-8") as f: json.dump(data, f, indent=2) + meta = {"fetched_at": time.time(), "ttl_seconds": _CACHE_TTL_SECONDS} + if index_digest: + meta["index_digest"] = index_digest + if etag: + meta["etag"] = etag + if last_modified: + meta["last_modified"] = last_modified with open(meta_path, "w", encoding="utf-8") as f: - json.dump( - {"fetched_at": time.time(), "ttl_seconds": _CACHE_TTL_SECONDS}, - f, - ) + json.dump(meta, f, indent=2) except OSError as exc: logger.debug("Cache write failed for '%s': %s", name, exc) @@ -171,6 +203,127 @@ def _clear_cache(name: str) -> None: os.remove(path) +def _read_stale_meta(name: str) -> dict | None: + """Read cache metadata even when the data cache is expired.""" + meta_path = _cache_meta_path(name) + if not os.path.exists(meta_path): + return None + try: + with open(meta_path, encoding="utf-8") as f: + return json.load(f) + except (json.JSONDecodeError, OSError): + return None + + +# --------------------------------------------------------------------------- +# Network fetch -- direct remote marketplace.json URL +# --------------------------------------------------------------------------- + + +def _http_get(url: str, **kwargs: object): + """Issue HTTP GET through a shared session without persisting cookies.""" + cookies = getattr(_HTTP_SESSION, "cookies", None) + if cookies is not None: + cookies.clear() + response = _HTTP_SESSION.get(url, **kwargs) + if cookies is not None: + cookies.clear() + return response + + +def _read_bounded_response_bytes(resp, url: str, max_bytes: int) -> bytes: + """Read response body from streaming chunks, enforcing *max_bytes*.""" + chunks: list[bytes] = [] + total = 0 + for chunk in resp.iter_content(chunk_size=_HTTP_CHUNK_BYTES): + if not chunk: + continue + total += len(chunk) + if total > max_bytes: + raise MarketplaceFetchError(url, f"marketplace.json exceeds {max_bytes} bytes") + chunks.append(chunk) + return b"".join(chunks) + + +def _fetch_url_direct( + url: str, + *, + etag: str = "", + last_modified: str = "", + expected_digest: str = "", +) -> FetchResult | None: + """Fetch a remote marketplace.json URL over HTTPS. + + Returns ``None`` for HTTP 304 so callers can serve cached data. + """ + parsed = urlsplit(url) + if parsed.scheme.lower() != "https": + raise MarketplaceFetchError(url, "remote marketplace.json URLs must use HTTPS") + + headers = {"User-Agent": "apm-cli"} + if etag: + headers["If-None-Match"] = etag + if last_modified: + headers["If-Modified-Since"] = last_modified + + resp = None + try: + resp = _http_get(url, headers=headers, timeout=30, stream=True) + except requests.exceptions.RequestException as exc: + raise MarketplaceFetchError(url, str(exc)) from exc + + try: + final_url = getattr(resp, "url", url) + if isinstance(final_url, str) and urlsplit(final_url).scheme.lower() != "https": + raise MarketplaceFetchError(url, "redirect to non-HTTPS URL rejected") + + if resp.status_code == 304: + return None + if resp.status_code == 404: + raise MarketplaceFetchError(url, "404 Not Found") + + try: + resp.raise_for_status() + except requests.exceptions.RequestException as exc: + raise MarketplaceFetchError(url, str(exc)) from exc + + content_length = resp.headers.get("Content-Length", "") + if content_length: + with contextlib.suppress(ValueError): + if int(content_length) > _MAX_MARKETPLACE_JSON_BYTES: + raise MarketplaceFetchError( + url, + f"marketplace.json exceeds {_MAX_MARKETPLACE_JSON_BYTES} bytes", + ) + + raw = _read_bounded_response_bytes(resp, url, _MAX_MARKETPLACE_JSON_BYTES) + finally: + if resp is not None: + close = getattr(resp, "close", None) + if callable(close): + close() + + digest = "sha256:" + hashlib.sha256(raw).hexdigest() + if expected_digest and digest != expected_digest: + raise MarketplaceFetchError( + url, f"digest mismatch: expected {expected_digest}, got {digest}" + ) + + try: + data = json.loads(raw.decode("utf-8")) + except (json.JSONDecodeError, UnicodeDecodeError) as exc: + raise MarketplaceFetchError(url, f"invalid JSON response: {exc}") from exc + if not isinstance(data, dict): + raise MarketplaceFetchError(url, "marketplace.json root must be an object") + + return FetchResult( + data=data, + digest=digest, + etag=resp.headers.get("ETag", ""), + last_modified=resp.headers.get("Last-Modified", ""), + ) + + # --------------------------------------------------------------------------- # Network fetch -- API path (GitHub / GitLab) # --------------------------------------------------------------------------- @@ -283,7 +436,7 @@ def _fetch_via_api( url = url_builder(source, file_path, host_info) def _do_fetch(token, _git_env): - resp = requests.get(url, headers=header_builder(token), timeout=30) + resp = _http_get(url, headers=header_builder(token), timeout=30) if resp.status_code == 404: return None resp.raise_for_status() @@ -449,6 +602,9 @@ def _fetch_local( source.name, f"local marketplace path does not exist: {repo_path}" ) + if repo_path.is_file(): + return _fetch_local_file(source, repo_path) + # Detect bare repo: it's either a directory with HEAD + objects/ (bare layout) # or it ends in .git, or it has a .git subdirectory (worktree). is_bare = (repo_path / "HEAD").is_file() and (repo_path / "objects").is_dir() @@ -463,6 +619,29 @@ def _fetch_local( return _fetch_local_direct_read(source, file_path, repo_path) +def _fetch_local_file(source: MarketplaceSource, manifest_file: Path) -> dict | None: + """Read an explicit local marketplace.json file. + + The parent directory is the containment boundary by design: unlike a + directory source, a direct file source is a single user-selected file, so + there is no broader marketplace root to enforce. + """ + from ..utils.path_security import PathTraversalError, ensure_path_within + + try: + safe_file = ensure_path_within(manifest_file, manifest_file.parent) + except PathTraversalError as exc: + raise MarketplaceFetchError( + source.name, "local marketplace file escapes its parent" + ) from exc + + try: + with open(safe_file, encoding="utf-8") as f: + return json.load(f) + except (json.JSONDecodeError, OSError) as exc: + raise MarketplaceFetchError(source.name, f"failed to read {safe_file}: {exc}") from exc + + def _fetch_local_via_git_show( source: MarketplaceSource, file_path: str, git_dir: Path ) -> dict | None: @@ -597,7 +776,7 @@ def fetch_raw( def _do_fetch(token, _git_env): headers = _github_headers(token) - resp = requests.get(url, headers=headers, timeout=30) + resp = _http_get(url, headers=headers, timeout=30) if resp.status_code == 404: return None resp.raise_for_status() @@ -712,7 +891,7 @@ def fetch_marketplace( ) -> MarketplaceManifest: """Fetch and parse a marketplace manifest. - Uses the JSON sidecar cache for ``kind in ("github","gitlab")`` only. + Uses the JSON sidecar cache for ``kind in ("github", "gitlab", "url")``. Generic-git fetches rely on ``GitCache`` + ``git ls-remote`` for freshness; local fetches read directly without caching. @@ -728,17 +907,63 @@ def fetch_marketplace( MarketplaceFetchError: If fetch fails and no cache is available. """ cache_name = _cache_key(source) - use_sidecar_cache = source.kind in ("github", "gitlab") + use_sidecar_cache = source.kind in ("github", "gitlab", "url") # Try fresh cache first (API kinds only) if use_sidecar_cache and not force_refresh: cached = _read_cache(cache_name) if cached is not None: logger.debug("Using cached marketplace data for '%s'", source.name) - return parse_marketplace_json(cached, source.name) + meta = _read_stale_meta(cache_name) or {} + return parse_marketplace_json( + cached, + source.name, + source_url=source.url if source.kind == "url" else "", + source_digest=meta.get("index_digest", "") if source.kind == "url" else "", + ) # Fetch from source try: + if source.kind == "url": + stale_meta = _read_stale_meta(cache_name) or {} + result = _fetch_url_direct( + source.url, + etag=stale_meta.get("etag", ""), + last_modified=stale_meta.get("last_modified", ""), + ) + if result is None: + stale = _read_stale_cache(cache_name) + if stale is None: + raise MarketplaceFetchError( + source.name, "got 304 Not Modified but no cached data is available" + ) + _write_cache( + cache_name, + stale, + index_digest=stale_meta.get("index_digest", ""), + etag=stale_meta.get("etag", ""), + last_modified=stale_meta.get("last_modified", ""), + ) + return parse_marketplace_json( + stale, + source.name, + source_url=source.url, + source_digest=stale_meta.get("index_digest", ""), + ) + _write_cache( + cache_name, + result.data, + index_digest=result.digest, + etag=result.etag, + last_modified=result.last_modified, + ) + return parse_marketplace_json( + result.data, + source.name, + source_url=source.url, + source_digest=result.digest, + ) + data = _fetch_file(source, source.path, auth_resolver=auth_resolver) if data is None: raise MarketplaceFetchError( @@ -754,7 +979,13 @@ def fetch_marketplace( stale = _read_stale_cache(cache_name) if stale is not None: logger.warning("Network error fetching '%s'; using stale cache", source.name) - return parse_marketplace_json(stale, source.name) + meta = _read_stale_meta(cache_name) or {} + return parse_marketplace_json( + stale, + source.name, + source_url=source.url if source.kind == "url" else "", + source_digest=meta.get("index_digest", "") if source.kind == "url" else "", + ) raise @@ -800,18 +1031,22 @@ def search_all_marketplaces( def clear_marketplace_cache( name: str | None = None, host: str = "github.com", + source: MarketplaceSource | None = None, ) -> int: """Clear cached data for one or all marketplaces. Returns the number of caches cleared. """ + if source is not None: + _clear_cache(_cache_key(source)) + return 1 if name: # Build a minimal source to derive the cache key _src = MarketplaceSource(name=name, owner="", repo="", host=host) _clear_cache(_cache_key(_src)) return 1 count = 0 - for source in get_registered_marketplaces(): - _clear_cache(_cache_key(source)) + for registered_source in get_registered_marketplaces(): + _clear_cache(_cache_key(registered_source)) count += 1 return count diff --git a/src/apm_cli/marketplace/models.py b/src/apm_cli/marketplace/models.py index 8132a7f6b..38dece172 100644 --- a/src/apm_cli/marketplace/models.py +++ b/src/apm_cli/marketplace/models.py @@ -48,6 +48,28 @@ def _extract_host_from_url(url: str) -> str: return parsed.hostname or "" +def url_names_remote_manifest(url: str) -> bool: + """Return True when *url* is a direct hosted ``marketplace.json`` document. + + Single source of truth for the "is this a hosted marketplace.json URL" + decision, shared by the marketplace CLI (``_is_remote_marketplace_json_url``) + and ``MarketplaceSource.is_remote_manifest_url`` so the two predicates + cannot drift (see #692 forward-compat constraint 1: the source-kind + discriminator stays a derived, validated string). Matches Anthropic's + hosted ``marketplace.json`` shape: HTTPS scheme, a host, and a path that + ends in ``/marketplace.json``. + """ + if not url: + return False + try: + parsed = urlsplit(url) + except ValueError: + return False + if parsed.scheme.lower() != "https" or not parsed.hostname: + return False + return (parsed.path or "").rstrip("/").endswith("/marketplace.json") + + def _extract_owner_repo_from_url(url: str) -> tuple[str, str]: """Best-effort owner/repo extraction. Empty strings if not derivable.""" if not url or _looks_like_local_path(url): @@ -139,31 +161,39 @@ def __post_init__(self) -> None: # that pass only name=... will fail later when something tries to use it. # Backfill legacy mirror fields from URL when caller used URL-only signature. - if self.url and not self.owner and not self.repo: + if self.url and self.path != "" and not self.owner and not self.repo: o, r = _extract_owner_repo_from_url(self.url) if o: object.__setattr__(self, "owner", o) if r: object.__setattr__(self, "repo", r) - if self.url and self.host == "github.com": + if self.url and self.path != "" and self.host == "github.com": h = _extract_host_from_url(self.url) if h: object.__setattr__(self, "host", h) # -- derived properties -------------------------------------------------- + @property + def is_remote_manifest_url(self) -> bool: + """Return True for direct remote marketplace.json URL sources.""" + return self.path == "" and url_names_remote_manifest(self.url) + @property def kind(self) -> str: - """Derived source kind: ``local`` | ``github`` | ``gitlab`` | ``git``. + """Derived source kind: ``local`` | ``url`` | ``github`` | ``gitlab`` | ``git``. Classification: - Local filesystem path or ``file://`` URI -> ``local`` + - Direct remote marketplace.json URL (``path == ""``) -> ``url`` - Host classified by AuthResolver as github/ghe_cloud/ghes -> ``github`` - Host classified as gitlab -> ``gitlab`` - Anything else (ado, generic, ssh to non-classified host) -> ``git`` """ if not self.url or _looks_like_local_path(self.url): return "local" + if self.is_remote_manifest_url: + return "url" host = _extract_host_from_url(self.url) if not host: return "git" @@ -193,6 +223,8 @@ def display_source(self) -> str: k = self.kind if k in ("github", "gitlab") and self.owner and self.repo: return f"{self.owner}/{self.repo}" + if k == "url": + return self.url if k == "local": lp = self.local_path home = os.path.expanduser("~") @@ -295,6 +327,8 @@ class MarketplaceManifest: owner_name: str = "" description: str = "" plugin_root: str = "" # metadata.pluginRoot - base path for bare-name sources + source_url: str = "" + source_digest: str = "" def find_plugin(self, plugin_name: str) -> MarketplacePlugin | None: """Find a plugin by exact name (case-insensitive).""" @@ -417,7 +451,13 @@ def _parse_plugin_entry(entry: dict[str, Any], source_name: str) -> MarketplaceP ) -def parse_marketplace_json(data: dict[str, Any], source_name: str = "") -> MarketplaceManifest: +def parse_marketplace_json( + data: dict[str, Any], + source_name: str = "", + *, + source_url: str = "", + source_digest: str = "", +) -> MarketplaceManifest: """Parse a marketplace.json dict into a ``MarketplaceManifest``. Accepts both Copilot CLI and Claude Code marketplace formats. @@ -426,6 +466,8 @@ def parse_marketplace_json(data: dict[str, Any], source_name: str = "") -> Marke Args: data: Parsed JSON content of marketplace.json. source_name: Display name of the marketplace (for provenance). + source_url: Canonical marketplace source URL for lockfile provenance. + source_digest: SHA-256 digest of the fetched marketplace.json bytes. Returns: MarketplaceManifest: Parsed manifest with valid plugin entries. @@ -468,4 +510,6 @@ def parse_marketplace_json(data: dict[str, Any], source_name: str = "") -> Marke owner_name=owner_name, description=description, plugin_root=plugin_root, + source_url=source_url, + source_digest=source_digest, ) diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index ba97e3407..6e0463a6d 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -105,11 +105,25 @@ class MarketplacePluginResolution: plugin: MarketplacePlugin dependency_reference: DependencyReference | None = None cross_repo_misconfig_risk: CrossRepoMisconfigRisk | None = None + source_url: str = "" + source_digest: str = "" def __iter__(self) -> Iterator[str | MarketplacePlugin]: yield self.canonical yield self.plugin + def provenance(self, marketplace_name: str, plugin_name: str) -> dict[str, str]: + """Return lockfile provenance for this resolved marketplace plugin.""" + data = { + "discovered_via": marketplace_name, + "marketplace_plugin_name": plugin_name, + } + if self.source_url: + data["source_url"] = self.source_url + if self.source_digest: + data["source_digest"] = self.source_digest + return data + def _normalize_owner_repo_slug(repo: str) -> str: """Lowercase ``owner/repo`` slug with optional ``.git`` suffix stripped.""" @@ -789,6 +803,8 @@ def _emit_warning(msg: str) -> None: plugin=plugin, dependency_reference=None, cross_repo_misconfig_risk=None, + source_url=manifest.source_url, + source_digest=manifest.source_digest, ) canonical = resolve_plugin_source( @@ -950,4 +966,6 @@ def _emit_warning(msg: str) -> None: plugin=plugin, dependency_reference=dep_ref, cross_repo_misconfig_risk=cross_repo_misconfig_risk, + source_url=manifest.source_url, + source_digest=manifest.source_digest, ) diff --git a/src/apm_cli/utils/archive.py b/src/apm_cli/utils/archive.py new file mode 100644 index 000000000..ef502df8e --- /dev/null +++ b/src/apm_cli/utils/archive.py @@ -0,0 +1,261 @@ +"""Download and safely extract archive packages (zip / tar.gz). + +Generic archive helper, decoupled from the marketplace layer: any +URL-sourced package installer can reuse it without importing marketplace +internals. Keeps the zip-slip / path-traversal / decompression-bomb guards +here so they apply to every caller (see #692 forward-compat constraints).""" + +from __future__ import annotations + +import contextlib +import io +import os +import shutil +import tarfile +import uuid +import zipfile +from pathlib import Path +from typing import IO +from urllib.parse import urlparse + +import requests + +from apm_cli.utils.path_security import ( + PathTraversalError, + ensure_path_within, + validate_path_segments, +) + +_MAX_UNCOMPRESSED_BYTES = 512 * 1024 * 1024 +_MAX_ARCHIVE_DOWNLOAD_BYTES = _MAX_UNCOMPRESSED_BYTES +_COPY_CHUNK_BYTES = 1024 * 1024 +_DOWNLOAD_CHUNK_BYTES = 1024 * 1024 +_ARCHIVE_SESSION = requests.Session() +_ARCHIVE_SESSION.max_redirects = 5 + + +class ArchiveError(Exception): + """Raised when an archive cannot be downloaded or extracted safely.""" + + +def _copy_member_within_limit(src: IO[bytes], dst: IO[bytes], running_total: int) -> int: + """Stream *src* into *dst*, enforcing the cumulative uncompressed cap. + + Counts the ACTUAL bytes read from the (decompressed) member stream and + aborts mid-stream once the running total exceeds the cap, rather than + trusting the archive's header-declared member sizes. A crafted archive + that under-declares its sizes therefore cannot slip past the + decompression-bomb guard, and a single huge member is never buffered + whole in memory. + """ + total = running_total + while True: + chunk = src.read(_COPY_CHUNK_BYTES) + if not chunk: + break + total += len(chunk) + if total > _MAX_UNCOMPRESSED_BYTES: + raise ArchiveError(f"Archive exceeds size limit of {_MAX_UNCOMPRESSED_BYTES} bytes") + dst.write(chunk) + return total + + +def _check_archive_member(member_path: str) -> None: + """Validate one archive member path before extraction.""" + if "\x00" in member_path: + raise ArchiveError(f"Archive member path contains null byte: {member_path!r}") + if os.path.isabs(member_path): + raise ArchiveError(f"Archive member has absolute path: {member_path!r}") + normalized = member_path.replace("\\", "/") + if normalized.startswith("//") or ( + len(normalized) >= 2 and normalized[1] == ":" and normalized[0].isalpha() + ): + raise ArchiveError(f"Archive member has absolute path: {member_path!r}") + try: + validate_path_segments(member_path, context="archive member") + except PathTraversalError as exc: + raise ArchiveError(str(exc)) from exc + + +def _detect_archive_format(content_type: str, url: str) -> str: + """Return ``tar.gz`` or ``zip`` from Content-Type or URL extension.""" + media_type = content_type.lower().split(";", 1)[0].strip() + if media_type == "application/x-tar": + raise ArchiveError( + "Uncompressed tar archives are not supported; " + "only gzip-compressed tarballs (.tar.gz) and zip archives are supported" + ) + if media_type in {"application/gzip", "application/x-gzip"}: + return "tar.gz" + if media_type in {"application/zip", "application/x-zip-compressed"}: + return "zip" + + lower_url = url.lower().split("?", 1)[0] + if lower_url.endswith((".tar.gz", ".tgz")): + return "tar.gz" + if lower_url.endswith(".zip"): + return "zip" + raise ArchiveError( + f"Cannot determine archive format from Content-Type={content_type!r} and URL={url!r}" + ) + + +def _safe_destination(dest_dir: str, member_name: str) -> Path: + """Return a contained destination path for an archive member.""" + destination_root = Path(dest_dir) + destination = destination_root / member_name + try: + return ensure_path_within(destination, destination_root) + except PathTraversalError as exc: + raise ArchiveError( + f"Archive member would extract outside destination: {member_name!r}" + ) from exc + + +def _extract_tar_archive(archive: tarfile.TarFile, dest_dir: str) -> list[str]: + """Extract an opened tar archive into *dest_dir* with safety checks.""" + extracted: list[str] = [] + total_size = 0 + for member in archive.getmembers(): + if member.isdir(): + continue + if member.issym() or member.islnk(): + raise ArchiveError(f"Symlinks and hard links are not supported: {member.name!r}") + if not member.isreg(): + continue + _check_archive_member(member.name) + destination = _safe_destination(dest_dir, member.name) + destination.parent.mkdir(parents=True, exist_ok=True) + src = archive.extractfile(member) + if src is None: + continue + with src, open(destination, "wb") as dst: + total_size = _copy_member_within_limit(src, dst, total_size) + extracted.append(member.name) + return extracted + + +def _extract_tar_gz(data: bytes, dest_dir: str) -> list[str]: + """Extract a tar.gz archive into *dest_dir* with safety checks.""" + try: + with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as archive: + return _extract_tar_archive(archive, dest_dir) + except tarfile.TarError as exc: + raise ArchiveError(f"Failed to read tar.gz archive: {exc}") from exc + + +def _extract_tar_gz_file(path: Path, dest_dir: str) -> list[str]: + """Extract a tar.gz archive file into *dest_dir* with safety checks.""" + try: + with tarfile.open(path, mode="r:gz") as archive: + return _extract_tar_archive(archive, dest_dir) + except tarfile.TarError as exc: + raise ArchiveError(f"Failed to read tar.gz archive: {exc}") from exc + + +def _extract_zip_archive(archive: zipfile.ZipFile, dest_dir: str) -> list[str]: + """Extract an opened zip archive into *dest_dir* with safety checks.""" + extracted: list[str] = [] + total_size = 0 + for info in archive.infolist(): + if info.filename.endswith("/"): + continue + _check_archive_member(info.filename) + destination = _safe_destination(dest_dir, info.filename) + destination.parent.mkdir(parents=True, exist_ok=True) + with archive.open(info) as src, open(destination, "wb") as dst: + total_size = _copy_member_within_limit(src, dst, total_size) + extracted.append(info.filename) + return extracted + + +def _extract_zip(data: bytes, dest_dir: str) -> list[str]: + """Extract a zip archive into *dest_dir* with safety checks.""" + try: + with zipfile.ZipFile(io.BytesIO(data)) as archive: + return _extract_zip_archive(archive, dest_dir) + except zipfile.BadZipFile as exc: + raise ArchiveError(f"Failed to read zip archive: {exc}") from exc + + +def _extract_zip_file(path: Path, dest_dir: str) -> list[str]: + """Extract a zip archive file into *dest_dir* with safety checks.""" + try: + with zipfile.ZipFile(path) as archive: + return _extract_zip_archive(archive, dest_dir) + except zipfile.BadZipFile as exc: + raise ArchiveError(f"Failed to read zip archive: {exc}") from exc + + +def _stream_download_to_file(response: object, output_path: Path, url: str) -> None: + """Stream an HTTP response body to *output_path* with a compressed-byte cap.""" + content_length = getattr(response, "headers", {}).get("Content-Length", "") + if content_length: + with contextlib.suppress(ValueError): + if int(content_length) > _MAX_ARCHIVE_DOWNLOAD_BYTES: + raise ArchiveError( + f"Archive download exceeds size limit of {_MAX_ARCHIVE_DOWNLOAD_BYTES} bytes" + ) + + total = 0 + with open(output_path, "wb") as dst: + for chunk in response.iter_content(chunk_size=_DOWNLOAD_CHUNK_BYTES): + if not chunk: + continue + total += len(chunk) + if total > _MAX_ARCHIVE_DOWNLOAD_BYTES: + raise ArchiveError( + f"Archive download exceeds size limit of {_MAX_ARCHIVE_DOWNLOAD_BYTES} bytes" + ) + dst.write(chunk) + if total == 0: + raise ArchiveError(f"Archive download from {url!r} returned an empty body") + + +def _archive_get(url: str, **kwargs: object): + """Issue archive HTTP GET through a cookie-cleared session.""" + _ARCHIVE_SESSION.cookies.clear() + response = _ARCHIVE_SESSION.get(url, **kwargs) + _ARCHIVE_SESSION.cookies.clear() + return response + + +def download_and_extract_archive( + url: str, dest_dir: str, *, headers: dict[str, str] | None = None +) -> list[str]: + """Download an HTTPS archive URL and extract it safely into *dest_dir*.""" + if urlparse(url).scheme.lower() != "https": + raise ArchiveError(f"Only HTTPS URLs are supported for archive download: {url!r}") + + destination_root = Path(dest_dir) + destination_root.mkdir(parents=True, exist_ok=True) + staging_root = destination_root.parent / f".apm-archive-staging-{uuid.uuid4().hex}" + staging_root.mkdir(parents=True, exist_ok=False) + download_path = staging_root / "archive-download" + response = None + try: + request_headers = {"User-Agent": "apm-cli"} + if headers: + request_headers.update(headers) + response = _archive_get(url, headers=request_headers, timeout=60, stream=True) + response.raise_for_status() + + final_url = getattr(response, "url", url) + if isinstance(final_url, str) and urlparse(final_url).scheme.lower() != "https": + raise ArchiveError(f"Redirect to non-HTTPS URL rejected: {final_url!r}") + detection_url = final_url if isinstance(final_url, str) and final_url else url + archive_format = _detect_archive_format( + response.headers.get("Content-Type", ""), detection_url + ) + _stream_download_to_file(response, download_path, url) + if archive_format == "tar.gz": + return _extract_tar_gz_file(download_path, dest_dir) + return _extract_zip_file(download_path, dest_dir) + except requests.exceptions.RequestException as exc: + raise ArchiveError(f"Failed to download archive from {url!r}: {exc}") from exc + finally: + if response is not None: + close = getattr(response, "close", None) + if callable(close): + close() + shutil.rmtree(staging_root, ignore_errors=True) diff --git a/tests/unit/marketplace/test_client_local.py b/tests/unit/marketplace/test_client_local.py index 04391232f..1a7a87280 100644 --- a/tests/unit/marketplace/test_client_local.py +++ b/tests/unit/marketplace/test_client_local.py @@ -64,6 +64,16 @@ def test_fetch_local_working_dir_direct_read(tmp_path: Path) -> None: assert result == manifest +def test_fetch_local_file_direct_read(tmp_path: Path) -> None: + """Anthropic local file shape reads a marketplace.json file directly.""" + manifest_file = tmp_path / "marketplace.json" + manifest = {"name": "acme", "plugins": []} + manifest_file.write_text(json.dumps(manifest)) + + result = _fetch_local(_local_source("acme", manifest_file), "") + assert result == manifest + + def test_fetch_local_missing_path_raises(tmp_path: Path) -> None: """Path that does not exist raises ``MarketplaceFetchError``.""" with pytest.raises(MarketplaceFetchError, match="does not exist"): diff --git a/tests/unit/marketplace/test_lockfile_provenance.py b/tests/unit/marketplace/test_lockfile_provenance.py index 4c48eb5f0..9f95305bc 100644 --- a/tests/unit/marketplace/test_lockfile_provenance.py +++ b/tests/unit/marketplace/test_lockfile_provenance.py @@ -1,8 +1,12 @@ """Tests for lockfile provenance fields -- serialization round-trip and backward compat.""" +from types import SimpleNamespace +from urllib.parse import urlparse + import pytest # noqa: F401 -from apm_cli.deps.lockfile import LockedDependency +from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.install.phases.lockfile import LockfileBuilder class TestLockedDependencyProvenance: @@ -12,22 +16,35 @@ def test_default_none(self): dep = LockedDependency(repo_url="owner/repo") assert dep.discovered_via is None assert dep.marketplace_plugin_name is None + assert dep.source_url is None + assert dep.source_digest is None def test_to_dict_omits_none(self): dep = LockedDependency(repo_url="owner/repo") d = dep.to_dict() assert "discovered_via" not in d assert "marketplace_plugin_name" not in d + assert "source_url" not in d + assert "source_digest" not in d def test_to_dict_includes_values(self): dep = LockedDependency( repo_url="owner/repo", discovered_via="acme-tools", marketplace_plugin_name="security-checks", + source_url="https://catalog.example.com/marketplace.json", + source_digest="sha256:" + "a" * 64, ) d = dep.to_dict() assert d["discovered_via"] == "acme-tools" assert d["marketplace_plugin_name"] == "security-checks" + source_url = urlparse(d["source_url"]) + assert (source_url.scheme, source_url.hostname, source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert d["source_digest"] == "sha256:" + "a" * 64 def test_from_dict_missing_fields(self): """Old lockfiles without provenance fields still deserialize.""" @@ -41,10 +58,19 @@ def test_from_dict_with_fields(self): "repo_url": "owner/repo", "discovered_via": "acme-tools", "marketplace_plugin_name": "security-checks", + "source_url": "https://catalog.example.com/marketplace.json", + "source_digest": "sha256:" + "b" * 64, } ) assert dep.discovered_via == "acme-tools" assert dep.marketplace_plugin_name == "security-checks" + source_url = urlparse(dep.source_url) + assert (source_url.scheme, source_url.hostname, source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert dep.source_digest == "sha256:" + "b" * 64 def test_roundtrip(self): original = LockedDependency( @@ -53,10 +79,19 @@ def test_roundtrip(self): resolved_ref="v1.0", discovered_via="acme-tools", marketplace_plugin_name="security-checks", + source_url="https://catalog.example.com/marketplace.json", + source_digest="sha256:" + "c" * 64, ) restored = LockedDependency.from_dict(original.to_dict()) assert restored.discovered_via == "acme-tools" assert restored.marketplace_plugin_name == "security-checks" + source_url = urlparse(restored.source_url) + assert (source_url.scheme, source_url.hostname, source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert restored.source_digest == "sha256:" + "c" * 64 assert restored.resolved_commit == "abc123" assert restored.resolved_ref == "v1.0" @@ -75,3 +110,34 @@ def test_backward_compat_existing_fields(self): assert dep.content_hash == "sha256:def456" assert dep.is_dev is True assert dep.discovered_via == "mkt" + + def test_lockfile_builder_attaches_marketplace_source_provenance(self): + lockfile = LockFile( + dependencies={ + "owner/repo": LockedDependency(repo_url="owner/repo"), + } + ) + ctx = SimpleNamespace( + marketplace_provenance={ + "owner/repo": { + "discovered_via": "catalog", + "marketplace_plugin_name": "tool", + "source_url": "https://catalog.example.com/marketplace.json", + "source_digest": "sha256:" + "f" * 64, + } + } + ) + builder = LockfileBuilder(ctx) + + builder._attach_marketplace_provenance(lockfile) + + dep = lockfile.dependencies["owner/repo"] + assert dep.discovered_via == "catalog" + assert dep.marketplace_plugin_name == "tool" + source_url = urlparse(dep.source_url) + assert (source_url.scheme, source_url.hostname, source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert dep.source_digest == "sha256:" + "f" * 64 diff --git a/tests/unit/marketplace/test_marketplace_client.py b/tests/unit/marketplace/test_marketplace_client.py index 893038665..cc2180065 100644 --- a/tests/unit/marketplace/test_marketplace_client.py +++ b/tests/unit/marketplace/test_marketplace_client.py @@ -168,6 +168,244 @@ def test_no_cache_no_network_raises(self, tmp_path): with pytest.raises(MarketplaceFetchError): client_mod.fetch_marketplace(source, force_refresh=True, auth_resolver=mock_resolver) + def test_fetch_remote_marketplace_url_records_digest_and_etag(self, monkeypatch): + source = MarketplaceSource( + name="catalog", + url="https://catalog.example.com/marketplace.json", + path="", + ) + raw = b'{"name":"catalog","plugins":[{"name":"tool","repository":"acme/tool"}]}' + + class Response: + status_code = 200 + url = "https://catalog.example.com/marketplace.json" + + def __init__(self): + self.headers = { + "ETag": "etag-1", + "Last-Modified": "Mon, 01 Jan 2024 00:00:00 GMT", + } + + @property + def content(self): + raise AssertionError( + "remote marketplace.json fetch must stream, not read response.content" + ) + + def iter_content(self, chunk_size): + yield raw[:chunk_size] + yield raw[chunk_size:] + + def raise_for_status(self): + return None + + def close(self): + return None + + seen_calls: list[tuple[dict, bool]] = [] + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + parsed = urlparse(url) + assert (parsed.scheme, parsed.hostname, parsed.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + seen_calls.append((headers or {}, stream)) + return Response() + + monkeypatch.setattr(client_mod, "_HTTP_SESSION", Session(), raising=False) + + manifest = client_mod.fetch_marketplace(source, force_refresh=True) + + parsed_source_url = urlparse(manifest.source_url) + assert (parsed_source_url.scheme, parsed_source_url.hostname, parsed_source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert manifest.source_digest.startswith("sha256:") + assert len(manifest.source_digest) == len("sha256:") + 64 + assert seen_calls == [({"User-Agent": "apm-cli"}, True)] + meta = client_mod._read_stale_meta(client_mod._cache_key(source)) + assert meta["etag"] == "etag-1" + assert meta["index_digest"] == manifest.source_digest + + def test_fetch_remote_marketplace_url_sends_conditional_headers(self, monkeypatch): + source = MarketplaceSource( + name="catalog", + url="https://catalog.example.com/marketplace.json", + path="", + ) + cached = {"name": "catalog", "plugins": []} + cache_key = client_mod._cache_key(source) + client_mod._write_cache( + cache_key, + cached, + index_digest="sha256:" + "a" * 64, + etag="etag-1", + last_modified="Mon, 01 Jan 2024 00:00:00 GMT", + ) + + seen_headers: list[dict] = [] + + class Response: + status_code = 304 + url = "https://catalog.example.com/marketplace.json" + + def __init__(self): + self.headers = {} + + def raise_for_status(self): + return None + + def close(self): + return None + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + seen_headers.append(headers or {}) + assert stream is True + return Response() + + monkeypatch.setattr(client_mod, "_HTTP_SESSION", Session(), raising=False) + + manifest = client_mod.fetch_marketplace(source, force_refresh=True) + + assert manifest.name == "catalog" + assert manifest.source_digest == "sha256:" + "a" * 64 + assert seen_headers[0]["If-None-Match"] == "etag-1" + assert seen_headers[0]["If-Modified-Since"] == "Mon, 01 Jan 2024 00:00:00 GMT" + + def test_fetch_remote_marketplace_url_rejects_redirect_to_http(self, monkeypatch): + source_url = "https://catalog.example.com/marketplace.json" + + class Response: + status_code = 200 + url = "http://catalog.example.com/marketplace.json" + + def __init__(self): + self.headers: dict[str, str] = {} + + def raise_for_status(self): + return None + + def close(self): + return None + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + assert stream is True + return Response() + + monkeypatch.setattr(client_mod, "_HTTP_SESSION", Session(), raising=False) + + with pytest.raises(MarketplaceFetchError, match="redirect to non-HTTPS URL rejected"): + client_mod._fetch_url_direct(source_url) + + def test_http_get_clears_session_cookies_between_hosts(self, monkeypatch): + class Cookies: + def __init__(self): + self.clear_count = 0 + + def clear(self): + self.clear_count += 1 + + class Session: + def __init__(self): + self.cookies = Cookies() + self.calls: list[str] = [] + + def get(self, url, **kwargs): + self.calls.append(url) + return object() + + session = Session() + monkeypatch.setattr(client_mod, "_HTTP_SESSION", session, raising=False) + + assert client_mod._http_get("https://one.example.test") is not None + assert client_mod._http_get("https://two.example.test") is not None + assert session.calls == ["https://one.example.test", "https://two.example.test"] + assert session.cookies.clear_count == 4 + + def test_fetch_remote_marketplace_url_streams_without_content_length(self, monkeypatch): + source_url = "https://catalog.example.com/marketplace.json" + raw = b'{"name":"catalog","plugins":[]}' + seen_calls: list[dict[str, object]] = [] + + class Response: + status_code = 200 + url = source_url + + def __init__(self): + self.headers: dict[str, str] = {} + + @property + def content(self): + raise AssertionError("remote marketplace.json fetch must stream bounded chunks") + + def iter_content(self, chunk_size): + for idx in range(0, len(raw), 5): + yield raw[idx : idx + 5] + + def raise_for_status(self): + return None + + def close(self): + return None + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + seen_calls.append({"url": url, "timeout": timeout, "stream": stream}) + return Response() + + monkeypatch.setattr(client_mod, "_HTTP_SESSION", Session(), raising=False) + + result = client_mod._fetch_url_direct(source_url) + + assert result is not None + assert result.data == {"name": "catalog", "plugins": []} + assert seen_calls == [{"url": source_url, "timeout": 30, "stream": True}] + + def test_fetch_remote_marketplace_url_rejects_oversized_stream_without_content_length( + self, monkeypatch + ): + source_url = "https://catalog.example.com/marketplace.json" + monkeypatch.setattr(client_mod, "_MAX_MARKETPLACE_JSON_BYTES", 8) + + class Response: + status_code = 200 + url = source_url + + def __init__(self): + self.headers: dict[str, str] = {} + + @property + def content(self): + raise AssertionError("remote marketplace.json fetch must stream bounded chunks") + + def iter_content(self, chunk_size): + yield b"1234" + yield b"5678" + yield b"9" + + def raise_for_status(self): + return None + + def close(self): + return None + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + assert stream is True + return Response() + + monkeypatch.setattr(client_mod, "_HTTP_SESSION", Session(), raising=False) + + with pytest.raises(MarketplaceFetchError, match=r"marketplace\.json exceeds"): + client_mod._fetch_url_direct(source_url) + class TestAutoDetectPath: """Auto-detect marketplace.json location in a repo.""" @@ -470,7 +708,7 @@ def fake_get(url, headers=None, timeout=None): with ( patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get", side_effect=fake_get), + patch("apm_cli.marketplace.client._http_get", side_effect=fake_get), ): resolver = AuthResolver() result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=resolver) @@ -510,7 +748,7 @@ def fake_get(url, headers=None, timeout=None): with ( patch.dict(os.environ, {"GITLAB_APM_PAT": "glpat-test"}, clear=False), patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get", side_effect=fake_get), + patch("apm_cli.marketplace.client._http_get", side_effect=fake_get), ): resolver = AuthResolver() result = client_mod._fetch_file(source, "marketplace.json", auth_resolver=resolver) @@ -537,7 +775,7 @@ def fake_get(url, headers=None, timeout=None): with ( patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get", side_effect=fake_get), + patch("apm_cli.marketplace.client._http_get", side_effect=fake_get), ): resolver = AuthResolver() client_mod._fetch_file(source, "marketplace.json", auth_resolver=resolver) @@ -557,7 +795,7 @@ def fake_get(url, headers=None, timeout=None): with ( patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get", side_effect=fake_get), + patch("apm_cli.marketplace.client._http_get", side_effect=fake_get), ): resolver = AuthResolver() client_mod._fetch_file(source, "marketplace.json", auth_resolver=resolver) @@ -588,7 +826,7 @@ def fake_get(url, headers=None, timeout=None): "apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None, ), - patch("apm_cli.marketplace.client.requests.get", side_effect=fake_get), + patch("apm_cli.marketplace.client._http_get", side_effect=fake_get), ): resolver = AuthResolver() client_mod._fetch_file(source, "marketplace.json", auth_resolver=resolver) @@ -614,7 +852,7 @@ def test_generic_host_routes_through_git_fetcher(self): mock_git = MagicMock(return_value=None) with ( patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get") as mock_get, + patch("apm_cli.marketplace.client._http_get") as mock_get, patch.dict("apm_cli.marketplace.client._FETCHERS", {"git": mock_git}), ): resolver = AuthResolver() @@ -643,7 +881,7 @@ def test_proxy_success_does_not_call_requests_get(self): "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=raw, ), - patch("apm_cli.marketplace.client.requests.get", mock_get), + patch("apm_cli.marketplace.client._http_get", mock_get), ): result = client_mod._fetch_file(source, "marketplace.json") @@ -680,7 +918,7 @@ def test_proxy_only_blocks_gitlab_v4_fallback(self): "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None, ), - patch("apm_cli.marketplace.client.requests.get", mock_get), + patch("apm_cli.marketplace.client._http_get", mock_get), ): result = client_mod._fetch_file(source, "marketplace.json") @@ -704,7 +942,7 @@ def test_proxy_only_blocks_github_contents_fallback(self): "apm_cli.deps.artifactory_entry.fetch_entry_from_archive", return_value=None, ), - patch("apm_cli.marketplace.client.requests.get", mock_get), + patch("apm_cli.marketplace.client._http_get", mock_get), ): result = client_mod._fetch_file(source, "marketplace.json") @@ -800,7 +1038,7 @@ def test_generic_host_routes_through_git_fetcher(self): mock_git = MagicMock(return_value=None) with ( patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None), - patch("apm_cli.marketplace.client.requests.get") as mock_get, + patch("apm_cli.marketplace.client._http_get") as mock_get, patch.dict("apm_cli.marketplace.client._FETCHERS", {"git": mock_git}), ): client_mod._fetch_file(source, "marketplace.json") @@ -925,7 +1163,7 @@ def test_non_github_host_rejected_before_request(self): return_value=None, ), patch( - "apm_cli.marketplace.client.requests.get", + "apm_cli.marketplace.client._http_get", ) as mock_get, ): with pytest.raises(MarketplaceError) as excinfo: diff --git a/tests/unit/marketplace/test_marketplace_commands.py b/tests/unit/marketplace/test_marketplace_commands.py index 9fe192a8a..6a46122eb 100644 --- a/tests/unit/marketplace/test_marketplace_commands.py +++ b/tests/unit/marketplace/test_marketplace_commands.py @@ -190,6 +190,15 @@ def test_add_verbose_shows_alias_source(self, mock_detect, mock_fetch, runner): assert result.exit_code == 0 assert "Alias source: manifest.name" in result.output + def test_remote_url_default_alias_includes_nearest_path_segment(self): + """Hosted JSON aliases should not collapse every path on a host to the same name.""" + from apm_cli.commands.marketplace import _default_alias_from_remote_url + + assert ( + _default_alias_from_remote_url("https://catalog.example.com/teams/sec/marketplace.json") + == "catalog.example.com-sec" + ) + @patch("apm_cli.marketplace.client.fetch_marketplace") @patch("apm_cli.marketplace.client._auto_detect_path") def test_successful_add(self, mock_detect, mock_fetch, runner): @@ -263,6 +272,171 @@ def test_add_accepts_full_https_url(self, mock_detect, mock_fetch, runner): assert probe_source.owner == "acme-org" assert probe_source.repo == "plugin-marketplace" + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_git_url_fragment_sets_ref_and_canonical_url(self, mock_detect, mock_fetch, runner): + """Anthropic git URL shape accepts #ref and stores the URL without the fragment.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="m", plugins=(MarketplacePlugin(name="p1"),) + ) + + result = runner.invoke( + marketplace, + ["add", "https://gitlab.com/acme/team/plugin-marketplace.git#v1.2.3"], + ) + + assert result.exit_code == 0, result.output + probe_source = mock_detect.call_args[0][0] + final_source = mock_fetch.call_args[0][0] + assert probe_source.url == "https://gitlab.com/acme/team/plugin-marketplace.git" + assert probe_source.ref == "v1.2.3" + assert final_source.url == "https://gitlab.com/acme/team/plugin-marketplace.git" + assert final_source.ref == "v1.2.3" + + @patch("apm_cli.marketplace.client.fetch_marketplace") + @patch("apm_cli.marketplace.client._auto_detect_path") + def test_add_unpinned_git_url_warns_about_mutable_ref(self, mock_detect, mock_fetch, runner): + """Git URL sources without #ref get an actionable pinning warning.""" + from apm_cli.commands.marketplace import marketplace + + mock_detect.return_value = "marketplace.json" + mock_fetch.return_value = MarketplaceManifest( + name="m", plugins=(MarketplacePlugin(name="p1"),) + ) + + result = runner.invoke( + marketplace, ["add", "https://gitlab.com/acme/team/plugin-marketplace.git"] + ) + + assert result.exit_code == 0, result.output + assert "Pin this git marketplace" in result.output + assert "#v1.0.0" in result.output + + @patch("apm_cli.marketplace.registry.add_marketplace") + def test_add_local_marketplace_json_file(self, mock_add, runner, tmp_path): + """Local file sources register through apm marketplace add.""" + from apm_cli.commands.marketplace import marketplace + + manifest = tmp_path / "marketplace.json" + manifest.write_text( + '{"name":"local-catalog","plugins":[{"name":"tool","repository":"acme/tool"}]}' + ) + + result = runner.invoke(marketplace, ["add", str(manifest), "--name", "local-catalog"]) + + assert result.exit_code == 0, result.output + registered = mock_add.call_args[0][0] + assert registered.name == "local-catalog" + assert registered.path == "" + assert registered.kind == "local" + + @patch("apm_cli.marketplace.registry.add_marketplace") + def test_add_remote_marketplace_json_url_fetches_directly(self, mock_add, runner, monkeypatch): + """Remote marketplace.json URLs are direct JSON sources, not git repositories.""" + from apm_cli.commands.marketplace import marketplace + + raw = b'{"name":"catalog","plugins":[{"name":"tool","repository":"acme/tool"}]}' + + class Response: + status_code = 200 + url = "https://catalog.example.com/marketplace.json" + + def __init__(self): + self.headers = {"ETag": "abc"} + + def iter_content(self, chunk_size): + yield raw[:chunk_size] + yield raw[chunk_size:] + + def raise_for_status(self): + return None + + def close(self): + return None + + requests_seen: list[str] = [] + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + assert stream is True + requests_seen.append(url) + return Response() + + monkeypatch.setattr("apm_cli.marketplace.client._HTTP_SESSION", Session()) + + result = runner.invoke( + marketplace, + ["add", "https://catalog.example.com/marketplace.json", "--name", "catalog"], + ) + + assert result.exit_code == 0, result.output + assert len(requests_seen) == 1 + requested = urlparse(requests_seen[0]) + assert (requested.scheme, requested.hostname, requested.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + registered = mock_add.call_args[0][0] + assert registered.name == "catalog" + registered_url = urlparse(registered.url) + assert (registered_url.scheme, registered_url.hostname, registered_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert registered.path == "" + assert registered.kind == "url" + + @patch("apm_cli.marketplace.registry.add_marketplace") + def test_add_remote_marketplace_json_url_warns_host_flag_is_ignored( + self, mock_add, runner, monkeypatch + ): + """--host is a shorthand-only flag; hosted marketplace.json URLs carry their own host.""" + from apm_cli.commands.marketplace import marketplace + + raw = b'{"name":"catalog","plugins":[]}' + + class Response: + status_code = 200 + url = "https://catalog.example.com/marketplace.json" + + def __init__(self): + self.headers: dict[str, str] = {} + + def iter_content(self, chunk_size): + yield raw + + def raise_for_status(self): + return None + + def close(self): + return None + + class Session: + def get(self, url, headers=None, timeout=None, stream=False): + assert stream is True + return Response() + + monkeypatch.setattr("apm_cli.marketplace.client._HTTP_SESSION", Session()) + + result = runner.invoke( + marketplace, + [ + "add", + "https://catalog.example.com/marketplace.json", + "--host", + "catalog.example.com", + ], + ) + + assert result.exit_code == 0, result.output + assert "--host is ignored when SOURCE is a hosted marketplace.json URL" in result.output + assert mock_add.call_args[0][0].name == "catalog" + @patch("apm_cli.marketplace.client.fetch_marketplace") @patch("apm_cli.marketplace.client._auto_detect_path") def test_add_strips_dot_git_suffix(self, mock_detect, mock_fetch, runner): diff --git a/tests/unit/marketplace/test_marketplace_install_integration.py b/tests/unit/marketplace/test_marketplace_install_integration.py index 2320b6c44..fe52d0cc8 100644 --- a/tests/unit/marketplace/test_marketplace_install_integration.py +++ b/tests/unit/marketplace/test_marketplace_install_integration.py @@ -2,6 +2,7 @@ import json from unittest.mock import MagicMock, patch +from urllib.parse import urlparse from apm_cli.marketplace.models import MarketplacePlugin, MarketplaceSource from apm_cli.marketplace.resolver import ( @@ -53,11 +54,19 @@ def test_outcome_has_provenance_field(self): "owner/repo": { "discovered_via": "acme-tools", "marketplace_plugin_name": "security-checks", + "source_url": "https://catalog.example.com/marketplace.json", + "source_digest": "sha256:" + "a" * 64, } }, ) assert outcome.marketplace_provenance is not None assert "owner/repo" in outcome.marketplace_provenance + source_url = urlparse(outcome.marketplace_provenance["owner/repo"]["source_url"]) + assert (source_url.scheme, source_url.hostname, source_url.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) def test_outcome_no_provenance(self): from apm_cli.core.command_logger import _ValidationOutcome @@ -114,6 +123,40 @@ def test_all_failed_exits_nonzero( mock_validate.assert_called_once() +class TestMarketplaceResolutionProvenance: + """Resolver carries marketplace source provenance to install validation.""" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_resolution_includes_manifest_source_url_and_digest(self, mock_get_source, mock_fetch): + from apm_cli.marketplace.models import MarketplaceManifest + from apm_cli.marketplace.resolver import resolve_marketplace_plugin + + mock_get_source.return_value = MarketplaceSource( + name="catalog", + url="https://catalog.example.com/marketplace.json", + path="", + ) + mock_fetch.return_value = MarketplaceManifest( + name="catalog", + plugins=( + MarketplacePlugin(name="tool", source={"type": "github", "repo": "acme/tool"}), + ), + source_url="https://catalog.example.com/marketplace.json", + source_digest="sha256:" + "d" * 64, + ) + + resolution = resolve_marketplace_plugin("tool", "catalog") + + resolved_source = urlparse(resolution.source_url) + assert (resolved_source.scheme, resolved_source.hostname, resolved_source.path) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert resolution.source_digest == "sha256:" + "d" * 64 + + class TestInstallMarketplaceGitLabMonorepoWiring: """Install uses resolver ``dependency_reference`` for GitLab-class monorepo plugins.""" @@ -142,6 +185,8 @@ def test_validation_receives_prefetched_gitlab_dep_ref( canonical=canonical, plugin=plugin, dependency_reference=dep_ref, + source_url="https://catalog.example.com/marketplace.json", + source_digest="sha256:" + "e" * 64, ) apm_yml = tmp_path / "apm.yml" @@ -164,6 +209,17 @@ def test_validation_receives_prefetched_gitlab_dep_ref( identity = dep_ref.get_identity() assert identity in outcome.marketplace_provenance assert outcome.marketplace_provenance[identity]["discovered_via"] == "apm-reg" + provenance_source_url = urlparse(outcome.marketplace_provenance[identity]["source_url"]) + assert ( + provenance_source_url.scheme, + provenance_source_url.hostname, + provenance_source_url.path, + ) == ( + "https", + "catalog.example.com", + "/marketplace.json", + ) + assert outcome.marketplace_provenance[identity]["source_digest"] == "sha256:" + "e" * 64 @patch("apm_cli.commands.install._validate_package_exists", return_value=True) @patch("apm_cli.commands.install._rich_success") @@ -292,7 +348,7 @@ def _setup_apm_yml(self, tmp_path, monkeypatch): @patch("apm_cli.commands.install._validate_package_exists", return_value=True) @patch("apm_cli.commands.install._rich_success") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - @patch("apm_cli.marketplace.client.requests.get") + @patch("apm_cli.marketplace.client._http_get") @patch("apm_cli.deps.registry_proxy.RegistryConfig.from_env", return_value=None) def test_gitlab_marketplace_in_repo_plugin_resolves_to_git_path( self, @@ -319,6 +375,7 @@ def test_gitlab_marketplace_in_repo_plugin_resolves_to_git_path( cache_root = tmp_path / "apm_home" cache_root.mkdir() monkeypatch.setattr("apm_cli.config.CONFIG_DIR", str(cache_root)) + monkeypatch.setattr("apm_cli.config.CONFIG_FILE", str(cache_root / "config.json")) source = MarketplaceSource( name="apm-reg", diff --git a/tests/unit/utils/test_archive.py b/tests/unit/utils/test_archive.py new file mode 100644 index 000000000..3932678ed --- /dev/null +++ b/tests/unit/utils/test_archive.py @@ -0,0 +1,220 @@ +"""Safety tests for archive extraction helpers.""" + +from __future__ import annotations + +import io +import tarfile +import zipfile +from pathlib import Path + +import pytest + +from apm_cli.utils import archive as archive_mod +from apm_cli.utils.archive import ( + ArchiveError, + _detect_archive_format, + _extract_tar_gz, + _extract_zip, + download_and_extract_archive, +) + + +def _zip_bytes(entries: dict[str, bytes]) -> bytes: + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + for name, data in entries.items(): + zf.writestr(name, data) + return buf.getvalue() + + +def _tar_gz_bytes(entries: dict[str, bytes]) -> bytes: + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tf: + for name, data in entries.items(): + info = tarfile.TarInfo(name) + info.size = len(data) + tf.addfile(info, io.BytesIO(data)) + return buf.getvalue() + + +def test_detect_archive_format_rejects_uncompressed_tar() -> None: + with pytest.raises(ArchiveError, match=r"gzip-compressed tarballs"): + _detect_archive_format("application/x-tar", "https://example.test/archive.tar") + + +def test_download_and_extract_archive_rejects_non_https_before_request( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + called = False + + def fake_get(_url: str, **_kwargs: object) -> object: + nonlocal called + called = True + raise AssertionError("network should not be called for non-HTTPS archives") + + monkeypatch.setattr(archive_mod, "_archive_get", fake_get) + + with pytest.raises(ArchiveError, match="Only HTTPS URLs"): + download_and_extract_archive("http://example.test/plugin.zip", str(tmp_path / "out")) + + assert called is False + + +def test_download_and_extract_archive_rejects_redirect_to_http( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + archive = _zip_bytes({"plugin/SKILL.md": b"content"}) + + class Response: + def __init__(self) -> None: + self.headers = {"Content-Type": ""} + self.url = "http://cdn.example.test/plugin.zip" + + def raise_for_status(self) -> None: + return None + + def iter_content(self, chunk_size: int): + yield archive + + def close(self) -> None: + return None + + def fake_get(_url: str, **_kwargs: object) -> Response: + return Response() + + monkeypatch.setattr(archive_mod, "_archive_get", fake_get) + + with pytest.raises(ArchiveError, match="Redirect to non-HTTPS URL rejected"): + download_and_extract_archive("https://example.test/download", str(tmp_path / "out")) + + +def test_download_and_extract_archive_stages_raw_file_outside_destination( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + archive = _zip_bytes({"plugin/SKILL.md": b"content"}) + out = tmp_path / "out" + + class Response: + def __init__(self) -> None: + self.headers = {"Content-Type": "application/zip"} + self.url = "https://cdn.example.test/plugin.zip" + + def raise_for_status(self) -> None: + return None + + def iter_content(self, chunk_size: int): + assert not list(out.glob(".apm-archive-download-*")) + yield archive + + def close(self) -> None: + return None + + def fake_get(_url: str, **_kwargs: object) -> Response: + return Response() + + monkeypatch.setattr(archive_mod, "_archive_get", fake_get) + + extracted = download_and_extract_archive("https://example.test/download", str(out)) + + assert extracted == ["plugin/SKILL.md"] + assert not list(out.glob(".apm-archive-*")) + + +def test_download_and_extract_archive_uses_redirect_url_for_format( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + archive = _zip_bytes({"plugin/SKILL.md": b"content"}) + seen_kwargs: list[dict[str, object]] = [] + + class Response: + def __init__(self) -> None: + self.headers = {"Content-Type": ""} + self.url = "https://cdn.example.test/plugin.zip" + + @property + def content(self) -> bytes: + raise AssertionError( + "download_and_extract_archive must stream, not read response.content" + ) + + def raise_for_status(self) -> None: + return None + + def iter_content(self, chunk_size: int): + for idx in range(0, len(archive), chunk_size): + yield archive[idx : idx + chunk_size] + + def close(self) -> None: + return None + + def fake_get(_url: str, **kwargs: object) -> Response: + seen_kwargs.append(kwargs) + return Response() + + monkeypatch.setattr(archive_mod, "_archive_get", fake_get) + + out = tmp_path / "out" + extracted = download_and_extract_archive("https://example.test/download", str(out)) + + assert seen_kwargs == [{"headers": {"User-Agent": "apm-cli"}, "timeout": 60, "stream": True}] + assert extracted == ["plugin/SKILL.md"] + assert (out / "plugin" / "SKILL.md").read_text() == "content" + + +def test_extract_zip_rejects_path_traversal(tmp_path: Path) -> None: + archive = _zip_bytes({"../escape.txt": b"nope"}) + + with pytest.raises(ArchiveError, match=r"traversal|outside"): + _extract_zip(archive, str(tmp_path / "out")) + + +def test_extract_tar_gz_rejects_symlink(tmp_path: Path) -> None: + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tf: + info = tarfile.TarInfo("link") + info.type = tarfile.SYMTYPE + info.linkname = "target" + tf.addfile(info) + + with pytest.raises(ArchiveError, match="links are not supported"): + _extract_tar_gz(buf.getvalue(), str(tmp_path / "out")) + + +def test_extract_zip_rejects_decompression_bomb( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(archive_mod, "_MAX_UNCOMPRESSED_BYTES", 1024) + archive = _zip_bytes({"plugin/big.bin": b"x" * 4096}) + + with pytest.raises(ArchiveError, match=r"exceeds size limit"): + _extract_zip(archive, str(tmp_path / "out")) + + +def test_extract_tar_gz_rejects_decompression_bomb( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(archive_mod, "_MAX_UNCOMPRESSED_BYTES", 1024) + archive = _tar_gz_bytes({"plugin/big.bin": b"x" * 4096}) + + with pytest.raises(ArchiveError, match=r"exceeds size limit"): + _extract_tar_gz(archive, str(tmp_path / "out")) + + +def test_extract_zip_writes_safe_members(tmp_path: Path) -> None: + archive = _zip_bytes({"plugin/SKILL.md": b"content"}) + out = tmp_path / "out" + + extracted = _extract_zip(archive, str(out)) + + assert extracted == ["plugin/SKILL.md"] + assert (out / "plugin" / "SKILL.md").read_text() == "content" + + +def test_extract_tar_gz_writes_safe_members(tmp_path: Path) -> None: + archive = _tar_gz_bytes({"plugin/SKILL.md": b"content"}) + out = tmp_path / "out" + + extracted = _extract_tar_gz(archive, str(out)) + + assert extracted == ["plugin/SKILL.md"] + assert (out / "plugin" / "SKILL.md").read_text() == "content"