diff --git a/CHANGELOG.md b/CHANGELOG.md index b528c8743..f17167ec9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `apm install` now resolves relative `path:` deps declared by remote monorepo packages when they stay inside the same remote repo, while still rejecting absolute, escaping, or cross-repo paths; closes #1571. (#1732) +- Dependencies with the same path on different git hosts no longer collide in `apm.lock.yaml`; `apm install` keeps GitHub/GitLab PATs off generic-host file downloads, routes bespoke GitLab hosts through `type: gitlab`, and surfaces non-404 download failures with host and endpoint context. Reading private files from a generic non-default host now requires a whole-repo git dependency or explicit backend signal; see the dependency and lockfile docs (closes #773). (#1735) ## [0.19.0] - 2026-06-09 diff --git a/docs/src/content/docs/consumer/manage-dependencies.md b/docs/src/content/docs/consumer/manage-dependencies.md index 6b72f6c42..e0efd58a8 100644 --- a/docs/src/content/docs/consumer/manage-dependencies.md +++ b/docs/src/content/docs/consumer/manage-dependencies.md @@ -62,7 +62,7 @@ parser. The supported forms: | SSH protocol | `ssh://git@gitlab.com/acme/repo.git` | SSH with explicit scheme or port. | | SSH with non-default user | `myuser@host:acme/repo.git` or `ssh://myuser@host/acme/repo.git` | Honors a non-`git` SSH user from the URL — useful for Enterprise Managed User (EMU) accounts or any server where the SSH login is not `git`. Username is validated against `^[a-zA-Z0-9_][a-zA-Z0-9_.+-]*$` (64-char cap); percent-encoded userinfo is rejected. The username is presentation-only and not part of dependency identity. | | Local path | `./packages/shared` or `/abs/path` | Sibling package on disk. | -| Object form (git) | `{ git: , path: , ref: , alias: }` | Custom directory name (`alias`), nested groups, monorepo subpaths, or anything the string forms cannot express. | +| Object form (git) | `{ git: , path: , ref: , alias: , type: gitlab }` | Aliases, nested groups, monorepo subpaths, bespoke GitLab hosts, or anything string forms cannot express. | | Marketplace dict | `{ name: , marketplace: , version: }` | Install a plugin from a registered marketplace. Optional `version` accepts a semver range (e.g. `~2.1.0`). Resolved to a concrete git ref at install time. | | Registry shorthand | `owner/repo#^2.0.0` with a default registry configured | Routes dep through the default registry instead of git. Default may come from `apm.yml` or `~/.apm/config.json`. Requires `registries` experimental flag. | | Registry object form | `{ id: owner/repo, version: ^2.0.0 }` | Explicit registry dep. `registry:` optional when a default registry is configured. Requires `registries` experimental flag. | @@ -80,6 +80,10 @@ dependencies: ref: v2.0 alias: security + # Self-managed GitLab on a bespoke hostname + - git: https://code.acme.com/platform/standards.git + type: gitlab + # Local: filesystem path (development only) - path: ./packages/shared-skills @@ -113,6 +117,23 @@ host/repo/ref and downloads the sibling from the same origin. Absolute paths, paths that escape the repo root, and cross-repo local paths are rejected. This is for same-repo monorepo siblings, not general workspace semantics. +Use `type: gitlab` only on `git` object entries for self-managed GitLab +instances whose hostname does not make the platform obvious: + +```yaml +- git: https://code.acme.com/platform/standards.git + type: gitlab +``` + +That routes REST file reads and token lookup through the GitLab path without +relying on hostname heuristics. For the token precedence chain, see +[Authentication](../../getting-started/authentication/). + +Generic non-default hosts do not receive APM-managed GitHub or GitLab PATs on +the HTTP file-read path. If a private host fails with 401/403, use a whole-repo +git dependency for full clone auth support, or choose the supported HTTP backend +signal (`type: gitlab` for GitLab-compatible hosts, `GITHUB_HOST` for GHES). + For private repos and non-GitHub hosts, see [Private and org packages](../private-and-org-packages/). diff --git a/docs/src/content/docs/getting-started/authentication.md b/docs/src/content/docs/getting-started/authentication.md index 9b0751c93..a6254258b 100644 --- a/docs/src/content/docs/getting-started/authentication.md +++ b/docs/src/content/docs/getting-started/authentication.md @@ -259,12 +259,21 @@ When authentication fails, APM prints a targeted diagnostic instead of a generic APM must classify a host as GitLab to use **GitLab REST v4** (for example `marketplace.json` fetches and install-time single-file reads). Configuration mirrors GHES-style host overrides: -| Variable | Purpose | -|----------|---------| -| `GITLAB_HOST` | One self-managed GitLab FQDN (e.g. `git.company.com`) | -| `APM_GITLAB_HOSTS` | Several self-managed GitLab FQDNs, comma-separated | +| Signal | Purpose | +|--------|---------| +| `GITLAB_HOST` | Environment variable for one self-managed GitLab FQDN (e.g. `git.company.com`) | +| `APM_GITLAB_HOSTS` | Environment variable for several self-managed GitLab FQDNs, comma-separated | +| `type: gitlab` | Manifest object-form hint for one bespoke GitLab host | + +`gitlab.com` is detected automatically. For a single dependency on a bespoke +hostname, use object form instead of a hostname convention: + +```yaml +- git: https://code.acme.com/platform/standards.git + type: gitlab +``` -`gitlab.com` is detected automatically. For GitLab-class hosts, resolved credentials follow **`GITLAB_APM_PAT` → `GITLAB_TOKEN`** and then **`git credential fill`** (see [GitLab-class hosts](#gitlab-class-hosts-gitlabcom-gitlab_host-apm_gitlab_hosts) under [Token lookup](#token-lookup)). GitHub PAT env vars are not used on GitLab. Use a GitLab personal or project access token with API read access where your policy requires it. +For GitLab-class hosts, resolved credentials follow **`GITLAB_APM_PAT` → `GITLAB_TOKEN`** and then **`git credential fill`** (see [GitLab-class hosts](#gitlab-class-hosts-gitlabcom-gitlab_host-apm_gitlab_hosts) under [Token lookup](#token-lookup)). GitHub PAT env vars are not used on GitLab. Use a GitLab personal or project access token with API read access where your policy requires it. ### REST headers (GitLab vs GitHub) diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 52db0df65..d4bba9827 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -118,8 +118,9 @@ Each item in `dependencies` describes one resolved package. | Field | Type | Required | Notes | |---|---|---|---| -| `repo_url` | string | yes | Canonical repo URL (e.g. `github.com/owner/repo`). Unique key for the entry, except for virtual and local entries (see below). | +| `repo_url` | string | yes | Canonical repository path or URL. Entry identity is derived from `repo_url`, `host`, and virtual/local markers; see [lockfile identity keys](#lockfile-identity-keys). | | `host` | string | no | FQDN when not inferable from `repo_url` (e.g. for registry proxies or non-GitHub hosts). | +| `host_type` | string | no | Explicit host-kind hint, currently `gitlab`, copied from object-form `type: gitlab`. | | `port` | int | no | Non-standard SSH/HTTPS port. Validated to `1..65535` on read. | | `registry_prefix` | string | no | URL path prefix when resolved through a registry proxy (e.g. `artifactory/github`). | | `resolved_ref` | string | no | The user-supplied ref from `apm.yml` (`main`, `v1.2.0`, a SHA). | @@ -150,6 +151,17 @@ Each item in `dependencies` describes one resolved package. Fields are emitted only when set. A minimal entry is just `repo_url` plus `resolved_commit`. +## Lockfile identity keys + +Lockfile dependency keys keep `github.com` implicit for migration stability: +existing `github.com` entries remain keyed as `owner/repo`. Local dependencies +use `local_path` directly. Virtual dependencies append `virtual_path` to the +base repo key. Entries for non-default hosts prefix the key with the lowercased +host (`host/owner/repo`), so `github.com/team/skills` and +`gitea.myorg.com/team/skills` can coexist without overwriting each other, and +host casing cannot create duplicate keys. Registry-proxy entries keep the bare +logical key because the proxy host is transport, not package identity. + ## Self entry A project that ships its own primitives (skills, agents, prompts under diff --git a/packages/apm-guide/.apm/skills/apm-usage/authentication.md b/packages/apm-guide/.apm/skills/apm-usage/authentication.md index 614d2395d..64f51b9fd 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/authentication.md +++ b/packages/apm-guide/.apm/skills/apm-usage/authentication.md @@ -18,6 +18,22 @@ APM checks the active `gh` CLI account before invoking OS credential helpers. Th For multi-account Git Credential Manager setups, see the [Multi-account Git Credential Manager](https://microsoft.github.io/apm/getting-started/authentication/#multi-account-git-credential-manager) section in the main authentication guide. +## GitLab hosts + +`gitlab.com` is detected automatically. For self-managed GitLab, set +`GITLAB_HOST`, list multiple hosts in `APM_GITLAB_HOSTS`, or mark a single +object-form dependency with `type: gitlab`: + +```yaml +- git: https://code.acme.com/platform/standards.git + type: gitlab +``` + +GitLab credentials use `GITLAB_APM_PAT`, then `GITLAB_TOKEN`, then host +credentials. GitHub PAT variables are not used for GitLab-class hosts. See the +main [authentication guide](https://microsoft.github.io/apm/getting-started/authentication/) +for the full host-class precedence rules. + ## Per-org setup Use per-org tokens when accessing packages across multiple organizations: diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 431276739..352eaaab8 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -138,6 +138,7 @@ instead so `@` remains reserved for git usernames and version syntax. | `path` | OPTIONAL | Subdirectory or file within the repo (virtual package). | | `ref` | OPTIONAL | Branch, tag, or commit SHA. | | `alias` | OPTIONAL | Install under a custom directory name (`^[a-zA-Z0-9._-]+$`). | +| `type` | OPTIONAL | Set to `gitlab` for self-managed GitLab on a bespoke hostname. Generic hosts do not receive APM-managed PATs on HTTP file reads. See the [lockfile spec](https://microsoft.github.io/apm/reference/lockfile-spec/#lockfile-identity-keys) for keying rules. | ```yaml - git: https://gitlab.com/acme/repo.git @@ -150,6 +151,9 @@ instead so `@` remains reserved for git usernames and version syntax. - git: ssh://git@bitbucket.example.com:7999/project/repo.git # custom SSH port ref: v1.0 + +- git: https://code.acme.com/platform/standards.git # bespoke GitLab + type: gitlab ``` ### Local (`path`) @@ -458,3 +462,7 @@ enterprise security guide for the threat model. `apm.lock.yaml` records the exact commit SHA for every dependency, regardless of the ref format in apm.yml. Running `apm install` without `--update` always uses the locked SHA, ensuring reproducible installs across machines. + +Lockfile keys keep `github.com` implicit for migration stability while +non-default hosts add the lowercased host segment. See the [lockfile spec](https://microsoft.github.io/apm/reference/lockfile-spec/#lockfile-identity-keys) +for the full keying rules. diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 34d702375..97c3ec61e 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -152,8 +152,13 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): if lockfile_path.exists(): lockfile = LockFile.read(lockfile_path) for dep in lockfile.dependencies.values(): - # Lockfile keys match declared_sources format (owner/repo) - dep_key = dep.get_unique_key() + # Orphan / source matching is host-blind: it compares against + # the apm.yml-derived keys above and the host-blind apm_modules/ + # filesystem layout. get_unique_key() is the host-qualified + # lockfile dedup key (#773) and must NOT be used here, or a + # non-default-host dep never matches its installed directory + # (it would be wrongly flagged orphaned and lose its Source). + dep_key = dep.get_canonical_dependency_string() if dep_key and dep_key not in declared_sources: declared_sources[dep_key] = _deps_list_source_label( dep.host, diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index f75e6df5e..052f4a828 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -58,7 +58,7 @@ # Patterns that indicate a secret value follows. Covers: # token=VALUE, Authorization: Bearer VALUE, Authorization: Basic VALUE, -# URL credentials (https://user:pass@host), and bare bearer strings. +# URL credentials (https://user:pass@host), and bare PAT-like strings. _SECRET_RE = re.compile( r"(?:" r"(?:token|password|secret|authorization|bearer)" # keyword prefix @@ -66,6 +66,8 @@ r"[\w.~!*\'();:@&=+$,/?#\[\]\-]{4,}" # value (>= 4 chars) r"|" r"://[^:@/\s]+:[^:@/\s]+@" # URL user:pass@ + r"|" + r"\b(?:github_pat_[A-Za-z0-9_]{20,}|gh[psour]_[A-Za-z0-9_]{20,})\b" # bare PAT r")", re.IGNORECASE, ) @@ -181,6 +183,15 @@ class BearerFallbackOutcome(NamedTuple): bearer_attempted: bool +class AuthCacheKey(NamedTuple): + """Stable cache key for AuthResolver lookups.""" + + host: str | None + port: int | None + host_type: str # Empty string represents an absent or canonical host_type. + org: str + + class AuthResolver: """Single source of truth for auth resolution. @@ -194,7 +205,7 @@ def __init__( logger: object | None = None, ): self._token_manager = token_manager or GitHubTokenManager() - self._cache: dict[tuple, AuthContext] = {} + self._cache: dict[AuthCacheKey, AuthContext] = {} self._lock = threading.Lock() # F2/F3 #852: optional logger lets the install command route the # verbose auth-source line through CommandLogger and the deferred @@ -220,7 +231,11 @@ def set_logger(self, logger: object) -> None: # -- host classification ------------------------------------------------ @staticmethod - def classify_host(host: str, port: int | None = None) -> HostInfo: + def classify_host( + host: str, + port: int | None = None, + host_type: str | None = None, + ) -> HostInfo: """Return a ``HostInfo`` describing *host*. ``port`` is carried through onto the returned ``HostInfo`` so that @@ -228,8 +243,11 @@ def classify_host(host: str, port: int | None = None) -> HostInfo: can discriminate between the same hostname on different ports. Host-kind classification itself is transport-agnostic -- the port never influences whether a host is GitHub/GHES/ADO/generic. + ``host_type`` is an explicit manifest hint for hosts whose names do + not reveal the backing service. """ h = host.lower() + host_type_value = (host_type or "").strip().lower() if h == "github.com": return HostInfo( @@ -258,6 +276,20 @@ def classify_host(host: str, port: int | None = None) -> HostInfo: port=port, ) + if host_type_value == "gitlab": + api_base = "https://gitlab.com/api/v4" if h == "gitlab.com" else f"https://{h}/api/v4" + return HostInfo( + host=host, + kind="gitlab", + has_public_repos=True, + api_base=api_base, + port=port, + ) + if host_type_value: + raise ValueError( + f"Unsupported dependency host type: {host_type_value}. Supported values: gitlab" + ) + # GHES: GITHUB_HOST is set to a non-github.com, non-ghe.com FQDN ghes_host = os.environ.get("GITHUB_HOST", "").lower() if ( @@ -275,12 +307,9 @@ def classify_host(host: str, port: int | None = None) -> HostInfo: port=port, ) - # GitLab (SaaS + env-configured self-managed) — after GHES per spec (no silent GHES → GitLab) + # GitLab (SaaS + env-configured self-managed) -- after GHES per spec (no silent GHES -> GitLab) if is_gitlab_hostname(host): - if h == "gitlab.com": - api_base = "https://gitlab.com/api/v4" - else: - api_base = f"https://{host}/api/v4" + api_base = "https://gitlab.com/api/v4" if h == "gitlab.com" else f"https://{h}/api/v4" return HostInfo( host=host, kind="gitlab", @@ -353,12 +382,21 @@ def gitlab_rest_headers( # -- core resolution ---------------------------------------------------- + @staticmethod + def _cache_host_type(host: str, host_type: str | None) -> str: + """Return the cache-discriminating host_type value for a host.""" + value = (host_type or "").strip().lower() + if value == "gitlab" and is_gitlab_hostname(host): + return "" + return value + def resolve( self, host: str, org: str | None = None, *, port: int | None = None, + host_type: str | None = None, ) -> AuthContext: """Resolve auth for *(host, port, org)*. Cached & thread-safe. @@ -368,9 +406,10 @@ def resolve( ``AuthContext``. Also flows into ``git credential fill`` so git's helpers can return port-specific credentials. """ - key = ( + key = AuthCacheKey( host.lower() if host else host, port, + self._cache_host_type(host, host_type), org.lower() if org else "", ) with self._lock: @@ -384,7 +423,7 @@ def resolve( # all subsequent callers for the same key become O(1) cache hits. # Bounded by APM_GIT_CREDENTIAL_TIMEOUT (default 60s). No deadlock # risk: single lock, never nested. - host_info = self.classify_host(host, port=port) + host_info = self.classify_host(host, port=port, host_type=host_type) token, source, scheme = self._resolve_token(host_info, org) token_type = self.detect_token_type(token) if token else "unknown" git_env = self._build_git_env(token, scheme=scheme, host_kind=host_info.kind) @@ -412,7 +451,12 @@ def resolve_for_dep(self, dep_ref: DependencyReference) -> AuthContext: parts = dep_ref.repo_url.split("/") if parts: org = parts[0] - return self.resolve(host, org, port=dep_ref.port) + return self.resolve( + host, + org, + port=dep_ref.port, + host_type=dep_ref.host_type, + ) # -- fallback strategy -------------------------------------------------- diff --git a/src/apm_cli/deps/download_strategies.py b/src/apm_cli/deps/download_strategies.py index 2c7bdbf78..6eeb85ac0 100644 --- a/src/apm_cli/deps/download_strategies.py +++ b/src/apm_cli/deps/download_strategies.py @@ -632,13 +632,22 @@ def download_gitlab_file( ) -> bytes: """Download a file via GitLab REST v4 ``repository/files/.../raw``.""" host = dep_ref.host or default_host() - host_info = self._host.auth_resolver.classify_host(host) + host_info = self._host.auth_resolver.classify_host( + host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ) project_path = dep_ref.repo_url if not project_path: raise RuntimeError("Missing repository path for GitLab file download") org = project_path.split("/")[0] - file_ctx = self._host.auth_resolver.resolve(host, org, port=dep_ref.port) + file_ctx = self._host.auth_resolver.resolve( + host, + org, + port=dep_ref.port, + host_type=dep_ref.host_type, + ) token = file_ctx.token headers = AuthResolver.gitlab_rest_headers(token) @@ -733,14 +742,11 @@ def download_github_file( # Parse owner/repo from repo_url owner, repo = dep_ref.repo_url.split("/", 1) - # Resolve token via AuthResolver for CDN fast-path decision - org = None - if dep_ref and dep_ref.repo_url: - parts = dep_ref.repo_url.split("/") - if parts: - org = parts[0] - file_ctx = self._host.auth_resolver.resolve(host, org, port=dep_ref.port) - token = file_ctx.token + # Resolve auth once through the same per-dependency boundary used by + # clone URLs. Generic hosts intentionally return None here so APM + # does not attach managed PATs to ad-hoc HTTP requests. + file_ctx = self._host._resolve_dep_auth_ctx(dep_ref) + token = file_ctx.token if file_ctx else None # --- CDN fast-path for github.com without a token --- # raw.githubusercontent.com is served from GitHub's CDN and is not @@ -775,16 +781,20 @@ def download_github_file( verbose_callback(f"Trying raw URL on generic host {host}: {raw_url}") try: response = self._host._resilient_get(raw_url, headers=raw_headers, timeout=30) - if response.status_code == 200: - if verbose_callback: - verbose_callback(f"Downloaded file: {host}/{dep_ref.repo_url}/{file_path}") - return response.content except (requests.RequestException, OSError) as raw_err: + raise RuntimeError( + self._build_download_network_error(host, file_path, "raw URL", raw_err) + ) from raw_err + if response.status_code == 200: if verbose_callback: - verbose_callback( - f"Raw URL on {host} failed for {file_path}@{ref}: " - f"{type(raw_err).__name__}; falling back to Contents API." + verbose_callback(f"Downloaded file: {host}/{dep_ref.repo_url}/{file_path}") + return response.content + if response.status_code != 404: + raise RuntimeError( + self._build_download_http_error( + host, file_path, response.status_code, "raw URL" ) + ) # --- Contents API path (authenticated, enterprise, or raw fallback) --- # Build API URL candidates - format differs by host type @@ -806,7 +816,7 @@ def download_github_file( # Try to download with the specified ref try: - if verbose_callback and not is_github_host: + if verbose_callback and host.lower() != "github.com": verbose_callback(f"Trying Contents API on {host}: {api_url}") response = self._host._resilient_get(api_url, headers=headers, timeout=30) response.raise_for_status() @@ -814,7 +824,8 @@ def download_github_file( verbose_callback(f"Downloaded file: {host}/{dep_ref.repo_url}/{file_path}") return self._extract_contents_api_payload(response, is_github_host) except requests.exceptions.HTTPError as e: - if e.response.status_code == 404: + status = e.response.status_code if e.response is not None else "unknown" + if status == 404: # For generic hosts, try remaining API version candidates before ref fallback for candidate_url in api_url_candidates[1:]: try: @@ -832,10 +843,17 @@ def download_github_file( ) return self._extract_contents_api_payload(candidate_resp, is_github_host) except requests.exceptions.HTTPError as ce: - if ce.response.status_code != 404: + status = ce.response.status_code if ce.response is not None else "unknown" + if status != 404: raise RuntimeError( # noqa: B904 - f"Failed to download {file_path}: HTTP {ce.response.status_code}" + self._build_download_http_error( + host, file_path, status, "Contents API" + ) ) + except requests.exceptions.RequestException as ce: + raise RuntimeError( # noqa: B904 + self._build_download_network_error(host, file_path, "Contents API", ce) + ) # Try fallback branches if the specified ref fails if ref not in ["main", "master"]: @@ -853,11 +871,18 @@ def download_github_file( # Try the other default branch fallback_ref = "master" if ref == "main" else "main" fallback_url_candidates = self._build_contents_api_urls( - host, owner, repo, file_path, fallback_ref + host, + owner, + repo, + file_path, + fallback_ref, + is_github_host=is_github_host, ) for fallback_url in fallback_url_candidates: try: + if verbose_callback: + verbose_callback(f"Trying Contents API on {host}: {fallback_url}") response = self._host._resilient_get( fallback_url, headers=headers, timeout=30 ) @@ -868,10 +893,17 @@ def download_github_file( ) return self._extract_contents_api_payload(response, is_github_host) except requests.exceptions.HTTPError as fe: - if fe.response.status_code != 404: + status = fe.response.status_code if fe.response is not None else "unknown" + if status != 404: raise RuntimeError( # noqa: B904 - f"Failed to download {file_path}: HTTP {fe.response.status_code}" + self._build_download_http_error( + host, file_path, status, "Contents API" + ) ) + except requests.exceptions.RequestException as fe: + raise RuntimeError( # noqa: B904 + self._build_download_network_error(host, file_path, "Contents API", fe) + ) raise RuntimeError( # noqa: B904 self._build_unsupported_or_missing_error( @@ -884,7 +916,7 @@ def download_github_file( fallback_ref=fallback_ref, ) ) - elif e.response.status_code in (401, 403): + elif status in (401, 403): # Distinguish rate limiting from auth failure. # X-RateLimit-* headers are GitHub-specific; treat as # rate-limit only when the host is in the GitHub family. @@ -941,13 +973,22 @@ def download_github_file( f"(file: {file_path}, ref: {ref}). " ) if not token: - error_msg += self._host.auth_resolver.build_error_context( - host, - "download", - org=owner, - port=dep_ref.port if dep_ref else None, - dep_url=dep_ref.repo_url if dep_ref else None, - ) + if is_github_host: + error_msg += self._host.auth_resolver.build_error_context( + host, + "download", + org=owner, + port=dep_ref.port if dep_ref else None, + dep_url=dep_ref.repo_url if dep_ref else None, + ) + else: + error_msg += ( + "No APM-managed token was sent for generic host file download. " + "Use a whole-repo git dependency for full clone auth support. " + "For platform-specific HTTP file reads, use object-form type: gitlab " + f"for GitLab-compatible hosts or set GITHUB_HOST={host} for GitHub " + "Enterprise Server. Re-run with --verbose to see attempted URLs." + ) elif is_github_host and not host.lower().endswith(".ghe.com"): error_msg += ( "Both authenticated and unauthenticated access " @@ -967,13 +1008,17 @@ def download_github_file( f"GITHUB_HOST={host} when this host is your GitHub " "Enterprise Server." ) + if is_github_host: + error_msg += " Re-run with --verbose to see attempted URLs." raise RuntimeError(error_msg) # noqa: B904 else: raise RuntimeError( - f"Failed to download {file_path}: HTTP {e.response.status_code}" + self._build_download_http_error(host, file_path, status, "Contents API") ) from e except requests.exceptions.RequestException as e: - raise RuntimeError(f"Network error downloading {file_path}: {e}") # noqa: B904 + raise RuntimeError( + self._build_download_network_error(host, file_path, "Contents API", e) + ) from e # ------------------------------------------------------------------ # Helpers for download_github_file @@ -1061,24 +1106,13 @@ def _build_generic_host_auth_headers( ) -> dict[str, str]: """Build HTTP headers for a generic-host (non-GitHub) request. - SECURITY GUARD: Only attach Authorization when the token is - unambiguously intended for this host. A token resolved from a - global env var (GITHUB_APM_PAT, GITHUB_TOKEN, GH_TOKEN) MUST NOT - be sent to an arbitrary non-GitHub host -- doing so leaks the - user's GitHub PAT to whatever FQDN is in the dependency line. - The clone path at ``get_clone_url`` already enforces the same - guard via ``is_github_hostname``; this mirrors it for HTTP file - downloads. - - Forwarding is allowed when: - - source == ``git-credential-fill``: git's credential helper - looks tokens up by host, so they are host-scoped by - construction. - - source == ``GITHUB_APM_PAT_``: per-org env var is - explicit user opt-in for that org's host. - - the user opted into this host as their GitHub Enterprise - Server via ``GITHUB_HOST=``: the token is intended for - this host, even if the FQDN is not under ``*.ghe.com``. + SECURITY GUARD: HTTP single-file downloads use the same token + boundary as clone URLs. Generic hosts receive no APM-managed + Authorization header; they must succeed unauthenticated or use a + host-specific backend such as ``type: gitlab``. + + Forwarding is allowed only when the caller passes a non-generic + auth context whose token is unambiguously intended for this host. """ headers: dict[str, str] = {} if accept: @@ -1141,6 +1175,33 @@ def _extract_contents_api_payload(response, is_github_host: bool) -> bytes: return content_field.encode("utf-8") return body + @staticmethod + def _build_download_http_error( + host: str, + file_path: str, + status: int | str, + endpoint: str, + ) -> str: + """Build a host- and endpoint-specific HTTP download error.""" + return ( + f"Failed to download {file_path} from {host}: HTTP {status} " + f"from {endpoint} endpoint. Re-run with --verbose to see attempted URLs." + ) + + @staticmethod + def _build_download_network_error( + host: str, + file_path: str, + endpoint: str, + error: BaseException, + ) -> str: + """Build a host- and endpoint-specific network download error.""" + detail = str(error).strip().splitlines()[0] if str(error).strip() else type(error).__name__ + return ( + f"Network error downloading {file_path} from {host} via {endpoint} endpoint: " + f"{detail}. Re-run with --verbose to see attempted URLs." + ) + @staticmethod def _build_unsupported_or_missing_error( host: str, diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index e58ee3bab..6a1eb38e3 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -409,7 +409,14 @@ def _is_generic_dependency_host(self, dep_ref: DependencyReference | None) -> bo dep_host = dep_ref.host if not dep_host or is_github_hostname(dep_host): return False - return self.auth_resolver.classify_host(dep_host, port=dep_ref.port).kind != "gitlab" + return ( + self.auth_resolver.classify_host( + dep_host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ).kind + != "gitlab" + ) def _parse_artifactory_base_url(self) -> tuple | None: """Backward-compat stub -- delegates to ArtifactoryRouter.""" @@ -792,7 +799,14 @@ def _download_github_file( ) -> bytes: """Backward-compat stub -- delegates to backend-specific strategies.""" host = dep_ref.host or default_host() - if self.auth_resolver.classify_host(host).kind == "gitlab": + if ( + self.auth_resolver.classify_host( + host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ).kind + == "gitlab" + ): return self._download_gitlab_file( dep_ref, file_path, ref, verbose_callback=verbose_callback ) @@ -1030,9 +1044,9 @@ def _try_sparse_checkout( try: temp_clone_path.mkdir(parents=True, exist_ok=True) - # Resolve per-dependency token via AuthResolver. - dep_token = self._resolve_dep_token(dep_ref) + # Resolve per-dependency auth via AuthResolver. dep_auth_ctx = self._resolve_dep_auth_ctx(dep_ref) + dep_token = dep_auth_ctx.token if dep_auth_ctx else self.github_token dep_auth_scheme = dep_auth_ctx.auth_scheme if dep_auth_ctx else "basic" # For ADO bearer, use the AuthContext git_env with header injection diff --git a/src/apm_cli/deps/github_downloader_validation.py b/src/apm_cli/deps/github_downloader_validation.py index e3096ee9d..010d85d10 100644 --- a/src/apm_cli/deps/github_downloader_validation.py +++ b/src/apm_cli/deps/github_downloader_validation.py @@ -334,7 +334,11 @@ def _build_validation_attempts( is_insecure: bool = bool(getattr(dep_ref, "is_insecure", False)) is_ado: bool = dep_ref.is_azure_devops() host_info = ( - downloader.auth_resolver.classify_host(dep_ref.host, port=dep_ref.port) + downloader.auth_resolver.classify_host( + dep_ref.host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ) if getattr(dep_ref, "host", None) else None ) diff --git a/src/apm_cli/deps/host_backends.py b/src/apm_cli/deps/host_backends.py index 629728ac3..b14552fcd 100644 --- a/src/apm_cli/deps/host_backends.py +++ b/src/apm_cli/deps/host_backends.py @@ -436,14 +436,12 @@ def build_contents_api_urls( @dataclass(frozen=True) class GenericGitBackend: - """Backend for non-GitHub non-ADO managed hosts (GitLab, Gitea, Gogs, Bitbucket). + """Backend for non-GitHub non-ADO generic hosts (Gitea, Gogs, Bitbucket). These hosts have heterogeneous APIs but support a common shape: HTTPS / SSH clones plus a Gitea-compatible Contents API at ``/api/v1/`` with a ``/api/v3/`` fallback for v3-only deployments. - - GitLab is currently classified as ``"generic"`` and accessed via the - full repo URL (clone + sparse checkout), not the Contents API. + GitLab-class hosts use :class:`GitLabBackend` instead. """ host_info: HostInfo @@ -524,6 +522,11 @@ def build_contents_api_urls( } +def _host_type_for_backend_dispatch(dep_ref: DependencyReference | None) -> str | None: + """Return a structural host_type from dependency-like refs.""" + return getattr(dep_ref, "host_type", None) + + def backend_for( dep_ref: DependencyReference | None, auth_resolver: AuthResolver, @@ -549,6 +552,7 @@ def backend_for( Returns: The :class:`HostBackend` for the resolved host. """ + host_type = _host_type_for_backend_dispatch(dep_ref) if dep_ref is not None and dep_ref.host: host = dep_ref.host port = getattr(dep_ref, "port", None) @@ -562,7 +566,11 @@ def backend_for( if dep_ref is not None: try: if dep_ref.is_azure_devops(): - info = auth_resolver.classify_host(host, port=port) + info = auth_resolver.classify_host( + host, + port=port, + host_type=host_type, + ) if not isinstance(info, HostInfo): info = HostInfo( host=host, @@ -575,7 +583,11 @@ def backend_for( except (AttributeError, TypeError): pass - info = auth_resolver.classify_host(host, port=port) + info = auth_resolver.classify_host( + host, + port=port, + host_type=host_type, + ) cls: type | None = None if isinstance(info, HostInfo): cls = _BACKEND_BY_KIND.get(info.kind) diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 5de15840c..516735cce 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -14,10 +14,30 @@ import yaml from ..models.apm_package import DependencyReference +from ..models.dependency.reference import ( + build_canonical_dependency_string, + build_dependency_unique_key, +) logger = logging.getLogger(__name__) _SELF_KEY = "." +_ALLOWED_HOST_TYPES = {"gitlab"} + + +def _normalize_lockfile_host_type(raw: Any) -> str | None: + """Validate and normalize the optional lockfile host_type field.""" + if raw is None: + return None + if not isinstance(raw, str) or not raw.strip(): + raise ValueError("lockfile host_type must be a non-empty string") + value = raw.strip().lower() + if value not in _ALLOWED_HOST_TYPES: + raise ValueError( + f"Unsupported lockfile host_type: {raw}. Supported values: " + f"{', '.join(sorted(_ALLOWED_HOST_TYPES))}" + ) + return value def _dedupe_preserving_order(values: list[str]) -> list[str]: @@ -31,6 +51,7 @@ class LockedDependency: repo_url: str host: str | None = None + host_type: str | None = None port: int | None = None # Non-standard SSH/HTTPS port (e.g. 7999 for Bitbucket DC) registry_prefix: str | None = None # Registry path prefix, e.g. "artifactory/github" resolved_commit: str | None = None @@ -80,17 +101,39 @@ class LockedDependency: def get_unique_key(self) -> str: """Returns unique key for this dependency.""" - if self.source == "local" and self.local_path: - return self.local_path - if self.is_virtual and self.virtual_path: - return f"{self.repo_url}/{self.virtual_path}" - return self.repo_url + return build_dependency_unique_key( + self.repo_url, + host=self.host, + source=self.source, + local_path=self.local_path, + is_virtual=self.is_virtual, + virtual_path=self.virtual_path, + registry_prefix=self.registry_prefix, + ) + + def get_canonical_dependency_string(self) -> str: + """Host-blind canonical key for filesystem / orphan-detection matching. + + Mirrors :meth:`DependencyReference.get_canonical_dependency_string`: + returns the bare ``repo_url`` (+ ``virtual_path``), never host-qualified, + so it matches the host-blind ``apm_modules/`` layout. Use + :meth:`get_unique_key` for the host-qualified lockfile dedup key. + """ + return build_canonical_dependency_string( + self.repo_url, + is_local=(self.source == "local"), + local_path=self.local_path, + is_virtual=self.is_virtual, + virtual_path=self.virtual_path, + ) def to_dict(self) -> dict[str, Any]: """Serialize to dict for YAML output.""" result: dict[str, Any] = {"repo_url": self.repo_url} if self.host: result["host"] = self.host + if self.host_type: + result["host_type"] = self.host_type if self.port: result["port"] = self.port if self.registry_prefix: @@ -177,6 +220,8 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: if _p_int is not None and 1 <= _p_int <= 65535: port = _p_int + host_type = _normalize_lockfile_host_type(data.get("host_type")) + # Recognised keys this build knows about. Anything else is captured # as ``_unknown_fields`` so a re-emit preserves forward-introduced # fields rather than silently dropping them. ``deployed_skills`` is @@ -184,6 +229,7 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: _known_keys = { "repo_url", "host", + "host_type", "port", "registry_prefix", "resolved_commit", @@ -218,6 +264,7 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: return cls( repo_url=data["repo_url"], host=data.get("host"), + host_type=host_type, port=port, registry_prefix=data.get("registry_prefix"), resolved_commit=data.get("resolved_commit"), @@ -329,6 +376,7 @@ def from_dependency_ref( return cls( repo_url=dep_ref.repo_url, host=host, + host_type=dep_ref.host_type, port=dep_ref.port, registry_prefix=registry_prefix, resolved_commit=resolved_commit, @@ -388,6 +436,7 @@ def to_dependency_ref(self) -> DependencyReference: return DependencyReference( repo_url=self.repo_url, host=self.host, + host_type=self.host_type, port=self.port, reference=ref, virtual_path=self.virtual_path, diff --git a/src/apm_cli/install/validation.py b/src/apm_cli/install/validation.py index 14b159fcc..3546f2bd6 100644 --- a/src/apm_cli/install/validation.py +++ b/src/apm_cli/install/validation.py @@ -287,7 +287,14 @@ def _validate_ado_git_package( # from APM-managed auth; they rely on git credential helpers via the # relaxed validate_env below. GitLab hosts are managed when classified # as GitLab because they need oauth2 HTTPS token formatting. - is_gitlab = auth_resolver.classify_host(dep_ref.host).kind == "gitlab" + is_gitlab = ( + auth_resolver.classify_host( + dep_ref.host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ).kind + == "gitlab" + ) is_generic = ( not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index fe5cca9ae..b39fddc9e 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -49,6 +49,71 @@ _REF_VERSION_SUFFIX_RE = re.compile(r"^v?\d+(?:\.\d+)*(?:[-+][A-Za-z0-9][A-Za-z0-9._-]*)?$") +def build_dependency_unique_key( + repo_url: str, + *, + host: str | None = None, + source: str | None = None, + local_path: str | None = None, + is_virtual: bool = False, + virtual_path: str | None = None, + registry_prefix: str | None = None, +) -> str: + """Return the lockfile/dedup key for a dependency identity. + + github.com remains the implicit default so existing lockfiles keep bare + ``owner/repo`` keys. Non-default hosts include the host segment to avoid + collisions between the same ``owner/repo`` on different servers. + + Registry-proxy deps (``registry_prefix`` set, e.g. an Artifactory mirror) + keep the bare logical key: the proxy host is a transport detail, not the + package identity, and the manifest side always declares the upstream + ``owner/repo`` shorthand. Host-qualifying them would break the manifest / + lockfile key correspondence used by re-install and orphan detection. + """ + if source == "local" and local_path: + return local_path + + key = repo_url + if is_virtual and virtual_path: + key = f"{key}/{virtual_path}" + + if registry_prefix: + return key + + host_value = (host or "").strip() + normalized_host = host_value.lower() + if normalized_host and normalized_host != "github.com": + return f"{normalized_host}/{key}" + return key + + +def build_canonical_dependency_string( + repo_url: str, + *, + is_local: bool = False, + local_path: str | None = None, + is_virtual: bool = False, + virtual_path: str | None = None, +) -> str: + """Return the host-blind canonical string for filesystem / orphan matching. + + Host-blind by construction: it never prefixes the host, so it matches the + host-blind ``apm_modules/`` layout. Use :func:`build_dependency_unique_key` + for the host-qualified lockfile dedup key. + + Callers pass their own ``is_local`` signal -- ``DependencyReference`` + derives it from its ``is_local`` property while ``LockedDependency`` derives + it from ``source == "local"`` -- so single-sourcing the body shape does not + collapse the two identity models' distinct local-detection semantics. + """ + if is_local and local_path: + return local_path + if is_virtual and virtual_path: + return f"{repo_url}/{virtual_path}" + return repo_url + + def _path_segment_pattern(is_ado_host: bool) -> str: """Return the allowed-character regex for a single repo path segment.""" return _ADO_PATH_SEGMENT_RE if is_ado_host else _NON_ADO_PATH_SEGMENT_RE @@ -79,6 +144,7 @@ class DependencyReference: repo_url: str # e.g., "user/repo" for GitHub or "org/project/repo" for Azure DevOps host: str | None = None # Optional host (github.com, dev.azure.com, or enterprise host) + host_type: str | None = None # Explicit host kind override (currently: "gitlab") port: int | None = None # Non-standard SSH/HTTPS port (e.g. 7999 for Bitbucket DC) explicit_scheme: str | None = ( None # User-stated transport: "ssh", "https", "http", or None for shorthand @@ -301,11 +367,15 @@ def get_unique_key(self) -> str: Returns: str: Unique key for this dependency """ - if self.is_local and self.local_path: - return self.local_path - if self.is_virtual and self.virtual_path: - return f"{self.repo_url}/{self.virtual_path}" - return self.repo_url + return build_dependency_unique_key( + self.repo_url, + host=self.host, + source="local" if self.is_local else self.source, + local_path=self.local_path, + is_virtual=self.is_virtual, + virtual_path=self.virtual_path, + registry_prefix=self.artifactory_prefix, + ) def to_canonical(self) -> str: """Return the canonical scheme-free identity string for this dependency. @@ -401,11 +471,18 @@ def get_canonical_dependency_string(self) -> str: For identity-based matching that includes non-default hosts, use get_identity(). For the transport-aware apm.yml entry, use to_apm_yml_entry(). + For the lockfile dedup key (host-qualified for non-default hosts), use get_unique_key(). Returns: str: Host-blind canonical string (e.g., "owner/repo") """ - return self.get_unique_key() + return build_canonical_dependency_string( + self.repo_url, + is_local=self.is_local, + local_path=self.local_path, + is_virtual=self.is_virtual, + virtual_path=self.virtual_path, + ) def get_install_path(self, apm_modules_dir: Path) -> Path: """Get the canonical filesystem path where this package should be installed. @@ -747,9 +824,12 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": git_url = entry["git"] if not isinstance(git_url, str) or not git_url.strip(): raise ValueError("'git' field must be a non-empty string") + host_type = cls._parse_host_type(entry.get("type")) # Monorepo parent inheritance (literal ``git: parent`` only; resolver expands) if git_url == "parent": + if host_type is not None: + raise ValueError("'type' is only supported for remote git dependencies") path_raw = entry.get("path") if path_raw is None: raise ValueError( @@ -807,6 +887,7 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": # Parse the git URL using the standard parser dep = cls.parse(git_url) + dep.host_type = host_type dep.allow_insecure = allow_insecure # Object-form ``- git:`` is an explicit Git resolver pin, even when # a top-level ``registries.default`` is set. Mark source so the @@ -859,6 +940,27 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": return dep + @staticmethod + def _parse_host_type(raw: object) -> str | None: + """Parse the optional object-form ``type`` host-kind hint. + + Currently only ``gitlab`` is accepted; any other value fails closed with + a ``ValueError``. This is a deliberate gate, not an oversight: future + host kinds (e.g. ``gitea``, ``bitbucket``) would extend the accepted set + here and thread a matching branch through ``AuthResolver.classify_host`` + and ``host_backends.backend_for``. Until those backends exist, rejecting + unknown hints keeps classification explicit rather than silently + mis-routing a bespoke host to the GitHub path. + """ + if raw is None: + return None + if not isinstance(raw, str) or not raw.strip(): + raise ValueError("'type' field must be a non-empty string") + value = raw.strip().lower() + if value != "gitlab": + raise ValueError(f"'type' field only supports 'gitlab'; got {raw!r}") + return value + @classmethod def virtual_suffix_is_installable_shape(cls, virtual_path: str) -> bool: """Return whether *virtual_path* matches APM virtual package shape rules. diff --git a/tests/integration/test_default_port_normalisation_e2e.py b/tests/integration/test_default_port_normalisation_e2e.py index 5bb7164eb..7d80ecfaa 100644 --- a/tests/integration/test_default_port_normalisation_e2e.py +++ b/tests/integration/test_default_port_normalisation_e2e.py @@ -159,7 +159,8 @@ def test_lockfile_yaml_roundtrip_preserves_non_default_port(self, tmp_path): loaded = LockFile.read(lock_path) assert loaded is not None - reloaded_dep = loaded.dependencies.get("team/repo") + # Non-default host (#773): lockfile dedup key is host-qualified. + reloaded_dep = loaded.dependencies.get("bitbucket.corp.com/team/repo") assert reloaded_dep is not None assert reloaded_dep.port == 7990 @@ -191,6 +192,7 @@ def test_ssh_default_port_lockfile_roundtrip(self, tmp_path): assert "port:" not in raw_yaml loaded = LockFile.read(lock_path) - reloaded = loaded.dependencies.get("acme/tools") + # Non-default host (#773): lockfile dedup key is host-qualified. + reloaded = loaded.dependencies.get("gitlab.com/acme/tools") assert reloaded is not None assert reloaded.port is None diff --git a/tests/integration/test_install_sources_classification.py b/tests/integration/test_install_sources_classification.py index 51beef131..f568bf699 100644 --- a/tests/integration/test_install_sources_classification.py +++ b/tests/integration/test_install_sources_classification.py @@ -626,3 +626,34 @@ def test_escaping_symlink_hard_fails_through_pipeline(self, tmp_path): # The hard-fail must leave no partial staged content behind. assert not install_path.exists() + + +class TestGitLabHostTypeInstallRoundTrip: + def test_manifest_host_type_selects_gitlab_backend_and_lockfile_entry(self): + from urllib.parse import urlparse + + from apm_cli.core.auth import AuthResolver + from apm_cli.deps.host_backends import GitLabBackend, backend_for + from apm_cli.deps.lockfile import LockedDependency + from apm_cli.models.dependency.reference import DependencyReference + + dep_ref = DependencyReference.parse_from_dict( + {"git": "https://code.acme.com/group/sub/repo.git", "type": "gitlab"} + ) + + host_info = AuthResolver.classify_host( + dep_ref.host, + port=dep_ref.port, + host_type=dep_ref.host_type, + ) + backend = backend_for(dep_ref, AuthResolver()) + locked = LockedDependency.from_dependency_ref(dep_ref, "abc123", 1, None) + restored = LockedDependency.from_dict(locked.to_dict()).to_dependency_ref() + + assert host_info.kind == "gitlab" + parsed_api_base = urlparse(host_info.api_base) + assert parsed_api_base.hostname == "code.acme.com" + assert parsed_api_base.path == "/api/v4" + assert isinstance(backend, GitLabBackend) + assert locked.host_type == "gitlab" + assert restored.host_type == "gitlab" diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index bba274930..c4260d556 100644 --- a/tests/test_github_downloader.py +++ b/tests/test_github_downloader.py @@ -1389,10 +1389,10 @@ def test_env_token_takes_priority_over_credential_fill(self): assert downloader._github_token_from_credential_fill is False mock_cred.assert_not_called() - def test_credential_fill_for_non_default_host(self): - """Non-default hosts should try credential fill on demand in _download_github_file.""" + def test_credential_fill_for_configured_enterprise_host(self): + """Configured GHES hosts try credential fill on demand in _download_github_file.""" with ( - patch.dict(os.environ, {}, clear=True), + patch.dict(os.environ, {"GITHUB_HOST": "ghes.company.com"}, clear=True), patch( "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", ) as mock_cred, @@ -2271,6 +2271,24 @@ def _gitea_json_envelope(file_bytes: bytes) -> bytes: ).encode("utf-8") +def _error_url_components(text: str) -> list[tuple[str, str, str]]: + """Extract (scheme, hostname, path) tuples for any URLs embedded in *text*. + + Download-error messages embed the failed endpoint as a URL. Asserting on + parsed components (per tests.instructions.md: never substring-match URLs) + rather than scraping with ``startswith``/``rstrip`` keeps the assertion + robust to wording or punctuation tweaks in the error string. + """ + out: list[tuple[str, str, str]] = [] + for token in text.split(): + cleaned = token.strip("(),.;'\"") + if "://" not in cleaned: + continue + parsed = urlparse(cleaned) + out.append((parsed.scheme, parsed.hostname or "", parsed.path)) + return out + + class TestGiteaRawUrlDownload: """Gitea raw URL path: /{owner}/{repo}/raw/{ref}/{file}.""" @@ -2352,13 +2370,8 @@ def test_token_still_sent_when_host_is_ghe(self): api_headers = mock_get.call_args_list[0][1].get("headers", {}) assert api_headers.get("Authorization") == "token ghp_ghe" - def test_git_credential_helper_token_sent_to_generic_host(self): - """Host-scoped credentials (git credential helper) ARE sent to generic hosts. - - The credential helper is host-scoped by construction, so forwarding - is safe and necessary for private Gitea/Gogs repos. This is the - symmetric case to the security guard test above. - """ + def test_git_credential_helper_token_is_not_sent_by_generic_http_download(self): + """Generic HTTP file downloads use the same token boundary as clone URLs.""" dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") raw_ok = _make_resp(200, b"data") @@ -2372,9 +2385,55 @@ def test_git_credential_helper_token_sent_to_generic_host(self): downloader.download_raw_file(dep_ref, "README.md", "main") raw_headers = mock_get.call_args_list[0][1].get("headers", {}) - assert raw_headers.get("Authorization") == "token gitea-host-scoped-token" + assert "Authorization" not in raw_headers + + def test_generic_download_uses_resolve_dep_auth_context_boundary(self): + dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") + raw_ok = _make_resp(200, b"data") + + with patch.dict(os.environ, {}, clear=True), _CRED_FILL_PATCH: + downloader = GitHubPackageDownloader() + with ( + patch.object( + downloader, "_resolve_dep_auth_ctx", return_value=None + ) as mock_resolve, + patch.object( + downloader, + "_resolve_dep_token", + side_effect=AssertionError("download performed duplicate token resolution"), + ), + patch.object( + downloader.auth_resolver, + "resolve", + side_effect=AssertionError("generic download bypassed _resolve_dep_auth_ctx"), + ), + patch.object(downloader, "_resilient_get", return_value=raw_ok), + ): + assert downloader.download_raw_file(dep_ref, "README.md", "main") == b"data" - def test_falls_back_to_api_v1_when_raw_returns_non_200(self): + mock_resolve.assert_called_once_with(dep_ref) + + def test_generic_host_403_without_credentials_explains_opt_in_paths(self): + dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") + + with patch.dict(os.environ, {}, clear=True), _CRED_FILL_PATCH: + downloader = GitHubPackageDownloader() + with patch.object( + downloader, + "_resilient_get", + side_effect=[_make_resp(404), _make_resp(403)], + ): + with pytest.raises(RuntimeError) as exc_info: + downloader.download_raw_file(dep_ref, "README.md", "main") + + msg = str(exc_info.value) + assert "No APM-managed token was sent" in msg + assert "whole-repo git dependency" in msg + assert "type: gitlab" in msg + assert "GITHUB_HOST" in msg + assert "Re-run with --verbose" in msg + + def test_falls_back_to_api_v1_when_raw_returns_404(self): """When the raw URL returns 404, the API v1 path is tried next.""" dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") expected = b"file via API" @@ -2391,44 +2450,72 @@ def test_falls_back_to_api_v1_when_raw_returns_non_200(self): assert urls[0] == "https://gitea.myorg.com/owner/repo/raw/main/README.md" assert urlparse(urls[1]).path.startswith("/api/v1/") - def test_raw_url_request_exception_falls_through_to_api(self): - """RequestException on the raw URL path must not abort -- API path runs. + def test_raw_url_request_exception_surfaces_with_endpoint_context(self): + """Only 404 falls through; network errors name the failed endpoint.""" + dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") - Regression trap for the ``except (RequestException, OSError)`` - swallow at the raw-URL try block. Previously this only had unit - coverage that pinned the swallow itself; this test exercises the - downstream "fallthrough must reach the API path" promise. - """ + with patch.object( + self.downloader, + "_resilient_get", + side_effect=requests_lib.exceptions.ConnectionError("boom"), + ): + with pytest.raises(RuntimeError) as exc_info: + self.downloader.download_raw_file(dep_ref, "README.md", "main") + + msg = str(exc_info.value) + assert ( + "Network error downloading README.md from gitea.myorg.com via raw URL endpoint" in msg + ) + assert "boom" in msg + assert "Re-run with --verbose to see attempted URLs" in msg + + def test_raw_url_500_surfaces_with_endpoint_context(self): dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") - expected = b"recovered via api" - api_ok = _make_resp(200, expected) - side_effects = [ - requests_lib.exceptions.ConnectionError("boom"), - api_ok, - ] - with patch.object(self.downloader, "_resilient_get", side_effect=side_effects) as mock_get: - result = self.downloader.download_raw_file(dep_ref, "README.md", "main") + with patch.object(self.downloader, "_resilient_get", return_value=_make_resp(500)): + with pytest.raises(RuntimeError) as exc_info: + self.downloader.download_raw_file(dep_ref, "README.md", "main") - assert result == expected - urls = [c[0][0] for c in mock_get.call_args_list] - assert urls[0].endswith("/owner/repo/raw/main/README.md") - assert urlparse(urls[1]).path.startswith("/api/v1/") + msg = str(exc_info.value) + assert "Failed to download README.md from gitea.myorg.com" in msg + assert "HTTP 500" in msg + assert "raw URL endpoint" in msg + assert "Re-run with --verbose to see attempted URLs" in msg class TestGiteaGogsApiVersionNegotiation: """API version negotiation: raw URL -> v1 -> v3 for Gitea/Gogs generic hosts. - The implementation intentionally stops at v3. GitLab uses a completely - different API shape (/api/v4/projects/:id/repository/files/...) that is - not compatible with the GitHub Contents-style endpoint negotiated here; - GitLab support is limited to git-clone operations only. + The generic-host implementation intentionally stops at v3. Explicit + ``type: gitlab`` dependencies use GitLab's separate + /api/v4/projects/:id/repository/files/... path instead of this negotiation. """ def setup_method(self): with patch.dict(os.environ, {}, clear=True), _CRED_FILL_PATCH: self.downloader = GitHubPackageDownloader() + def test_object_form_type_gitlab_routes_bespoke_host_to_gitlab_api(self): + dep_ref = DependencyReference.parse_from_dict( + {"git": "https://code.acme.com/group/sub/repo.git", "type": "gitlab"} + ) + expected = b"gitlab raw" + response = _make_resp(200, expected) + + with patch.dict(os.environ, {"GITLAB_APM_PAT": "glpat-bespoke"}, clear=True): + downloader = GitHubPackageDownloader() + with patch.object(downloader, "_resilient_get", return_value=response) as mock_get: + result = downloader._download_github_file(dep_ref, "SKILL.md", "main") + + assert result == expected + request_url = mock_get.call_args[0][0] + parsed = urlparse(request_url) + assert parsed.hostname == "code.acme.com" + assert parsed.path.endswith("/repository/files/SKILL.md/raw") + headers = mock_get.call_args[1]["headers"] + assert headers.get("PRIVATE-TOKEN") == "glpat-bespoke" + assert dep_ref.host_type == "gitlab" + def test_v1_falls_back_to_v3_for_generic_hosts(self): """When Gitea raw URL and v1 both return 404, v3 is tried and succeeds.""" dep_ref = DependencyReference.parse("gitea.myorg.com/owner/repo") @@ -2510,12 +2597,7 @@ def test_gitea_api_passthrough_when_server_returns_raw_bytes(self): assert result == expected def test_fallback_candidate_loop_reraises_non_404(self): - """500 on a candidate URL must surface as RuntimeError, not silent skip. - - Pins the symmetry-fix between the primary loop (already re-raised - non-404) and the fallback-ref loop (previously swallowed all - HTTPErrors via bare ``pass``). - """ + """500 on a candidate URL must surface as RuntimeError, not silent skip.""" dep_ref = DependencyReference.parse("gitea.example.com/owner/repo") # raw=404, v1=404 (forces ref-fallback), v1@master=500 diff --git a/tests/test_lockfile.py b/tests/test_lockfile.py index 574a6d584..47995b4b7 100644 --- a/tests/test_lockfile.py +++ b/tests/test_lockfile.py @@ -3,7 +3,7 @@ from pathlib import Path # noqa: F401 from unittest.mock import Mock -import pytest # noqa: F401 +import pytest import yaml from apm_cli.deps.lockfile import ( @@ -28,6 +28,32 @@ def test_get_unique_key_virtual(self): ) assert dep.get_unique_key() == "owner/repo/prompts/file.md" + def test_get_unique_key_preserves_github_default_host(self): + dep = LockedDependency(repo_url="owner/repo", host="github.com") + assert dep.get_unique_key() == "owner/repo" + + def test_get_unique_key_includes_non_default_host(self): + dep = LockedDependency(repo_url="team/skills", host="gitea.myorg.com") + assert dep.get_unique_key() == "gitea.myorg.com/team/skills" + + def test_get_unique_key_lowercases_non_default_host(self): + mixed_case = LockedDependency(repo_url="team/skills", host="Gitea.MyOrg.com") + lower_case = LockedDependency(repo_url="team/skills", host="gitea.myorg.com") + + assert mixed_case.get_unique_key() == lower_case.get_unique_key() + assert lower_case.get_unique_key() == "gitea.myorg.com/team/skills" + + def test_get_unique_key_includes_non_default_host_for_virtual_dep(self): + dep = LockedDependency( + repo_url="team/skills", + host="git.internal.example.com", + virtual_path="prompts/review.prompt.md", + is_virtual=True, + ) + assert ( + dep.get_unique_key() == "git.internal.example.com/team/skills/prompts/review.prompt.md" + ) + def test_to_dict_minimal(self): dep = LockedDependency(repo_url="owner/repo") result = dep.to_dict() @@ -99,6 +125,33 @@ def test_port_from_dependency_ref(self): locked = LockedDependency.from_dependency_ref(dep_ref, "abc123", 1, None) assert locked.port == 7999 + def test_host_type_round_trip(self): + dep = LockedDependency( + repo_url="team/repo", + host="code.acme.com", + host_type="gitlab", + ) + data = dep.to_dict() + assert data["host_type"] == "gitlab" + restored = LockedDependency.from_dict(data) + assert restored.host_type == "gitlab" + assert restored.to_dependency_ref().host_type == "gitlab" + + def test_rejects_unknown_host_type_from_lockfile(self): + with pytest.raises(ValueError, match="Supported values: gitlab"): + LockedDependency.from_dict( + {"repo_url": "team/repo", "host": "code.acme.com", "host_type": "gitea"} + ) + + def test_host_type_from_dependency_ref(self): + dep_ref = DependencyReference( + repo_url="team/repo", + host="code.acme.com", + host_type="gitlab", + ) + locked = LockedDependency.from_dependency_ref(dep_ref, "abc123", 1, None) + assert locked.host_type == "gitlab" + def test_deployed_file_hashes_round_trip(self): dep = LockedDependency( repo_url="owner/repo", @@ -147,6 +200,23 @@ def test_add_and_get_dependency(self): assert lock.has_dependency("owner/repo") assert not lock.has_dependency("other/repo") + def test_add_dependency_keeps_same_repo_from_different_hosts(self): + lock = LockFile() + lock.add_dependency(LockedDependency(repo_url="team/skills", host="github.com")) + lock.add_dependency(LockedDependency(repo_url="team/skills", host="gitea.myorg.com")) + + assert set(lock.dependencies) == {"team/skills", "gitea.myorg.com/team/skills"} + assert lock.get_dependency("team/skills").host == "github.com" + assert lock.get_dependency("gitea.myorg.com/team/skills").host == "gitea.myorg.com" + + def test_dependency_reference_key_includes_non_default_host(self): + dep = DependencyReference.parse("gitea.myorg.com/team/skills") + assert dep.get_unique_key() == "gitea.myorg.com/team/skills" + + def test_dependency_reference_key_preserves_github_com_default(self): + dep = DependencyReference.parse("github.com/team/skills") + assert dep.get_unique_key() == "team/skills" + def test_to_yaml(self): lock = LockFile(apm_version="1.0.0") lock.add_dependency(LockedDependency(repo_url="owner/repo")) diff --git a/tests/unit/commands/test_deps_cli_cli_surface.py b/tests/unit/commands/test_deps_cli_cli_surface.py index 4004333c4..397a33335 100644 --- a/tests/unit/commands/test_deps_cli_cli_surface.py +++ b/tests/unit/commands/test_deps_cli_cli_surface.py @@ -53,6 +53,7 @@ def _make_dep( ) -> MagicMock: dep = MagicMock() dep.get_unique_key.return_value = key + dep.get_canonical_dependency_string.return_value = key dep.version = version dep.resolved_commit = resolved_commit dep.resolved_ref = resolved_ref @@ -345,6 +346,7 @@ def test_insecure_dep_from_lockfile_flagged(self, tmp_path: Path) -> None: insecure_dep = MagicMock() insecure_dep.get_unique_key.return_value = "owner/repo" + insecure_dep.get_canonical_dependency_string.return_value = "owner/repo" insecure_dep.host = None insecure_dep.source = "github" insecure_dep.is_insecure = True diff --git a/tests/unit/commands/test_deps_cli_phase3.py b/tests/unit/commands/test_deps_cli_phase3.py index 14e70c715..a14f868be 100644 --- a/tests/unit/commands/test_deps_cli_phase3.py +++ b/tests/unit/commands/test_deps_cli_phase3.py @@ -53,6 +53,7 @@ def _make_dep( ) -> MagicMock: dep = MagicMock() dep.get_unique_key.return_value = key + dep.get_canonical_dependency_string.return_value = key dep.version = version dep.resolved_commit = resolved_commit dep.resolved_ref = resolved_ref @@ -345,6 +346,7 @@ def test_insecure_dep_from_lockfile_flagged(self, tmp_path: Path) -> None: insecure_dep = MagicMock() insecure_dep.get_unique_key.return_value = "owner/repo" + insecure_dep.get_canonical_dependency_string.return_value = "owner/repo" insecure_dep.host = None insecure_dep.source = "github" insecure_dep.is_insecure = True diff --git a/tests/unit/core/test_auth_phase3.py b/tests/unit/core/test_auth_phase3.py index 1d215e5ea..b18098830 100644 --- a/tests/unit/core/test_auth_phase3.py +++ b/tests/unit/core/test_auth_phase3.py @@ -267,6 +267,7 @@ def _make_dep_ref(self, host: str, repo_url: str | None = None, port: int | None dep.host = host dep.repo_url = repo_url dep.port = port + dep.host_type = None return dep def test_resolve_for_dep_uses_host(self) -> None: @@ -299,6 +300,15 @@ def test_resolve_for_dep_threads_port(self) -> None: ctx = resolver.resolve_for_dep(dep) assert ctx.host_info.port == 7990 + def test_resolve_for_dep_threads_host_type(self) -> None: + with patch.dict(os.environ, {}, clear=True): + resolver = AuthResolver() + dep = self._make_dep_ref("code.acme.com", "group/repo") + dep.host_type = "gitlab" + ctx = resolver.resolve_for_dep(dep) + assert ctx.host_info.kind == "gitlab" + assert ctx.host_info.api_base == "https://code.acme.com/api/v4" + # --------------------------------------------------------------------------- # _purpose_for_host / _identify_env_source diff --git a/tests/unit/core/test_auth_validation.py b/tests/unit/core/test_auth_validation.py index c8685d0df..a0b82f3ef 100644 --- a/tests/unit/core/test_auth_validation.py +++ b/tests/unit/core/test_auth_validation.py @@ -267,6 +267,7 @@ def _make_dep_ref(self, host: str, repo_url: str | None = None, port: int | None dep.host = host dep.repo_url = repo_url dep.port = port + dep.host_type = None return dep def test_resolve_for_dep_uses_host(self) -> None: diff --git a/tests/unit/deps/test_download_strategies_phase3.py b/tests/unit/deps/test_download_strategies_phase3.py index 211e91b28..18e01fb59 100644 --- a/tests/unit/deps/test_download_strategies_phase3.py +++ b/tests/unit/deps/test_download_strategies_phase3.py @@ -68,6 +68,19 @@ def _make_host( api_base="https://api.github.com", ) auth.build_error_context.return_value = "Set GITHUB_APM_PAT." + + def _is_generic_dep(dep_ref) -> bool: + dep_host = getattr(dep_ref, "host", None) + if not dep_host: + return False + return dep_host != "github.com" and not str(dep_host).endswith(".ghe.com") + + host._resolve_dep_token.side_effect = lambda dep_ref=None: ( + None if _is_generic_dep(dep_ref) else github_token + ) + host._resolve_dep_auth_ctx.side_effect = lambda dep_ref=None: ( + None if _is_generic_dep(dep_ref) else ctx + ) host.auth_resolver = auth return host diff --git a/tests/unit/deps/test_download_strategies_selection.py b/tests/unit/deps/test_download_strategies_selection.py index d01544c74..c5b36ceb9 100644 --- a/tests/unit/deps/test_download_strategies_selection.py +++ b/tests/unit/deps/test_download_strategies_selection.py @@ -68,6 +68,19 @@ def _make_host( api_base="https://api.github.com", ) auth.build_error_context.return_value = "Set GITHUB_APM_PAT." + + def _is_generic_dep(dep_ref) -> bool: + dep_host = getattr(dep_ref, "host", None) + if not dep_host: + return False + return dep_host != "github.com" and not str(dep_host).endswith(".ghe.com") + + host._resolve_dep_token.side_effect = lambda dep_ref=None: ( + None if _is_generic_dep(dep_ref) else github_token + ) + host._resolve_dep_auth_ctx.side_effect = lambda dep_ref=None: ( + None if _is_generic_dep(dep_ref) else ctx + ) host.auth_resolver = auth return host diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 608fd6ad7..c3277b849 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -82,6 +82,22 @@ def test_gitlab_self_managed_gitlab_host_env(self): assert hi.kind == "gitlab" assert hi.api_base == "https://git.corp.example.com/api/v4" + def test_host_type_gitlab_reclassifies_bespoke_host(self): + hi = AuthResolver.classify_host("Code.Acme.COM", host_type="gitlab") + assert hi.kind == "gitlab" + assert hi.api_base == "https://code.acme.com/api/v4" + + def test_unsupported_host_type_lists_supported_values(self): + with pytest.raises(ValueError, match="Supported values: gitlab"): + AuthResolver.classify_host("code.acme.com", host_type="gitea") + + def test_gitlab_host_type_hint_reuses_gitlab_cache_entry(self): + with patch.dict(os.environ, {}, clear=True): + resolver = AuthResolver() + ctx_a = resolver.resolve("gitlab.com") + ctx_b = resolver.resolve("gitlab.com", host_type="gitlab") + assert ctx_a is ctx_b + def test_gitlab_self_managed_apm_gitlab_hosts_env(self): with patch.dict( os.environ, @@ -1611,3 +1627,11 @@ def op_embed_secret(token, env): assert sentinel not in full_log, ( "Secret embedded in exception message must be redacted by SecretRedactionFilter" ) + + @pytest.mark.parametrize("sentinel", ["github_pat_" + "A" * 24, "ghr_" + "B" * 24]) + def test_bare_pat_like_token_is_redacted(self, sentinel): + from apm_cli.core.auth import _redact_secrets + + redacted = _redact_secrets(f"git stderr leaked {sentinel} without a label") + assert sentinel not in redacted + assert "[REDACTED]" in redacted diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 6f6916e76..e31d36b41 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -201,11 +201,13 @@ def test_list_insecure_filters_to_http_locked_deps(self): secure_dep = MagicMock() secure_dep.get_unique_key.return_value = "safeorg/saferepo" + secure_dep.get_canonical_dependency_string.return_value = "safeorg/saferepo" secure_dep.is_insecure = False secure_dep.resolved_by = None insecure_dep = MagicMock() insecure_dep.get_unique_key.return_value = "insecureorg/insecurerepo" + insecure_dep.get_canonical_dependency_string.return_value = "insecureorg/insecurerepo" insecure_dep.is_insecure = True insecure_dep.resolved_by = None @@ -239,6 +241,7 @@ def test_list_insecure_shows_transitive_provenance(self): insecure_dep = MagicMock() insecure_dep.get_unique_key.return_value = "childorg/childrepo" + insecure_dep.get_canonical_dependency_string.return_value = "childorg/childrepo" insecure_dep.is_insecure = True insecure_dep.resolved_by = "parentorg/parentrepo" @@ -270,6 +273,7 @@ def test_list_insecure_reports_clean_when_no_http_locked_deps(self): secure_dep = MagicMock() secure_dep.get_unique_key.return_value = "safeorg/saferepo" + secure_dep.get_canonical_dependency_string.return_value = "safeorg/saferepo" secure_dep.is_insecure = False secure_dep.resolved_by = None diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index 4e54071b9..a3a925355 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -415,7 +415,9 @@ def test_to_yaml_round_trip_preserves_http_fields(self): ) parsed = LockFile.from_yaml(lockfile.to_yaml()) - dep = parsed.get_dependency("acme/rules") + # Non-default hosts are host-qualified in the lockfile dedup key (#773), + # so look the entry up by its host-qualified key. + dep = parsed.get_dependency("git.company.internal/acme/rules") assert dep is not None assert dep.is_insecure is True