From 945a4c1f9eb9e48ec2f32b8da0ff916b2a5a4592 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:08:52 -0300 Subject: [PATCH 01/14] feat(policy): walk GitLab subgroups for org policy discovery (closes #2753) GitLab org-policy discovery previously probed only the top-level group (`/apm-policy`), so a project under nested subgroups could not scope its own governance without changing the whole org. Walk the subgroup tree from the project's own group up to the top-level group and apply the closest `apm-policy` (closest wins); `absent` at an inner level ascends to the parent group, while any error/malformed outcome still fails closed. A flat `/` remote is unchanged. `extends:` now accepts nested-namespace references (`acme/dept-a/apm-policy`), and the pre-fetch host-pin guard no longer misreads a bare nested namespace's first segment as a cross-host FQDN (a real attacker FQDN is still rejected). Remote-URL splitting is centralized in a single `_remote_url_parts` owner (consumed by both `_parse_remote_url` and the new `_gitlab_namespace_descending`), and the `git remote get-url origin` subprocess in a single `_git_remote_origin_url` owner. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + CONFORMANCE.json | 3 +- CONFORMANCE.md | 2 +- .../src/content/docs/enterprise/apm-policy.md | 17 +- .../docs/enterprise/governance-guide.md | 2 +- .../docs/enterprise/policy-reference.md | 5 +- .../.apm/skills/apm-usage/governance.md | 10 +- src/apm_cli/policy/_gitlab.py | 16 +- src/apm_cli/policy/discovery.py | 245 +++++++++++---- tests/spec_conformance/test_policy_reqs.py | 20 ++ tests/unit/policy/test_discovery.py | 289 ++++++++++++++++++ 11 files changed, 536 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ef4721095..4ed547d182 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- GitLab org-policy discovery now walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` project, so a team can scope its own governance under a subgroup instead of only the top-level group; a subgroup policy can `extends:` an ancestor group's policy (including nested-namespace references). Flat `/` remotes are unchanged. (closes #2753) (#PR) - gh-aw's shared APM import now supports `token-source: github-token`; after consumers re-vendor the workflow, its read-only current-repository identity can fetch same-repository private packages, while `cascade` remains the default and cross-repository packages still require a dedicated token or GitHub App. (#2706) - OpenAPM v0.1 adds `req-pl-018` for dependency-policy identity casing and amends `req-rs-016` clause (3), the Section 6.4 merge rules, and the Section 6.5 pattern grammar so repository identity and policy matching cannot diverge; Section 11.2 item 6 now requires the per-host case rule in `CONFORMANCE.md`. (#2706) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index b2fbc74e4d..38d69cd4c2 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -696,9 +696,10 @@ "keyword": "MUST", "section": "6.1.1", "status": "active", - "test_count": 2, + "test_count": 3, "tests": [ "tests/spec_conformance/test_policy_reqs.py::test_policy_gitlab_discovery_provider_is_a_distinct_convention", + "tests/spec_conformance/test_policy_reqs.py::test_policy_gitlab_discovery_walks_subgroups_closest_first", "tests/spec_conformance/test_policy_reqs.py::test_policy_provides_default_allow_list_shape" ] }, diff --git a/CONFORMANCE.md b/CONFORMANCE.md index a6c0ce79fd..66cc6e48ff 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -97,7 +97,7 @@ Repository-coordinate segments are case-insensitive for `github.com`, GitHub Ent | [req-pl-008](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-008) | MUST | 6.3.1 | governance | active | 1 | - | | [req-pl-009](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-009) | MUST | 6.6 | governance | active | 1 | - | | [req-pl-010](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-010) | MUST | 6.2 | governance | active | 1 | - | -| [req-pl-011](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-011) | MUST | 6.1.1 | governance | active | 2 | - | +| [req-pl-011](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-011) | MUST | 6.1.1 | governance | active | 3 | - | | [req-pl-012](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-012) | MUST | 6.1.1 | governance | active | 1 | - | | [req-pl-013](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-013) | MUST | 6.8 | governance | active | 1 | - | | [req-pl-014](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-014) | MUST | 6.8 | governance | active | 1 | - | diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index 6fd3b4e025..5deaed7063 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -74,11 +74,22 @@ On GitHub and GitHub API-compatible hosts, the `.github-private` repo is preferr ``` / - apm-policy/ - apm-policy.yml # auto-discovered by projects whose remote starts with / + [/] # optional -- apm-policy may sit at any level + apm-policy/ + apm-policy.yml # discovered by projects under this group or any descendant ``` -GitLab discovery uses only the top-level group: APM takes the first path segment from the project remote and looks for `/apm-policy`. It does not search nested subgroup scopes. Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project without `apm-policy` (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. +GitLab discovery walks the subgroup tree from the project's own group up to the top-level group, and the **closest** `apm-policy` wins. For a project remote `gitlab.com/acme/dept-a/team-x/my-project`, APM probes in order: + +1. `acme/dept-a/team-x/apm-policy` +2. `acme/dept-a/apm-policy` +3. `acme/apm-policy` + +This lets an individual team publish its own `apm-policy` under its subgroup without affecting the rest of the organization, while a top-level `acme/apm-policy` still applies to every project that has no closer policy. A flat `/` remote (including a personal namespace like `gitlab.com//`) probes only `/apm-policy`, identical to the previous behaviour. + +Composing policies across levels is opt-in via `extends:`. A team policy can extend an ancestor group's policy to inherit and tighten it, using either an explicit path (`extends: "acme/dept-a/apm-policy"` for the immediate parent group, or a deeper ancestor) or the `extends: "org"` shorthand, which on GitLab always resolves to the **top-level** group's policy (`/apm-policy`) -- not the nearest ancestor. Children can only tighten, never relax. + +Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. :::caution[Self-managed GitLab requires GITLAB_HOST or APM_GITLAB_HOSTS] An arbitrary FQDN is never auto-classified as GitLab -- the same domain shape could be Bitbucket, Gitea, or a plain git server. `gitlab.com` is recognised automatically, but a self-managed instance (e.g. `gitlab.example.com`) is only recognised once you set `GITLAB_HOST=gitlab.example.com` (or `APM_GITLAB_HOSTS` for more than one instance). Without it, APM falls through to the GitHub-style cascade above, which is invalid on GitLab and behaves exactly like the unfixed discovery this section describes. This mirrors `GITHUB_HOST` for GitHub Enterprise Server and `ADO_HOST` for on-prem Azure DevOps Server -- see [Environment Variables](../../reference/environment-variables/). diff --git a/docs/src/content/docs/enterprise/governance-guide.md b/docs/src/content/docs/enterprise/governance-guide.md index 9b77060b59..8899048361 100644 --- a/docs/src/content/docs/enterprise/governance-guide.md +++ b/docs/src/content/docs/enterprise/governance-guide.md @@ -504,7 +504,7 @@ These are the sharp edges. Plan around them; do not assume they are solved. - **`apm compile` and `apm run` do not re-check policy.** They trust install. Operational mitigation: ensure that no compile or run step in CI is reachable without a preceding `apm install` that ran the gate. - **`apm audit --ci` in `warn` mode rewrites violations to `passed=True`.** Warn mode never fails CI exit. The visibility is in the SARIF output, not the exit code. Operational mitigation: monitor Code Scanning alerts during the warn-mode rollout phase; do not assume CI green means "no policy violations" while in warn mode. - **`apm install` has no `--policy ` flag.** Only `apm audit` does. This is the air-gapped install gap. Operational mitigation: use `extends:` from a reachable mirror, or run audit (which does support `--policy `) as the gating check and skip install-time enforcement in air-gapped CI. -- **Plain git remotes are not auto-discovered.** GitHub and GitHub Enterprise use the `.github` cascade, Azure DevOps uses the org `apm` project and `apm-policy` repository (with `_apm/_apm` as a legacy fallback), and GitLab uses `/apm-policy/apm-policy.yml`, where the top-level group is the first path segment of the remote. Nested subgroup scopes are not searched. Configure self-managed GitLab with `GITLAB_HOST` or `APM_GITLAB_HOSTS`. For unsupported remotes, pass `apm audit --ci --policy ` explicitly. +- **Plain git remotes are not auto-discovered.** GitHub and GitHub Enterprise use the `.github` cascade, Azure DevOps uses the org `apm` project and `apm-policy` repository (with `_apm/_apm` as a legacy fallback), and GitLab walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` (see [Policy Files](./apm-policy/#where-it-lives)). Configure self-managed GitLab with `GITLAB_HOST` or `APM_GITLAB_HOSTS`. For unsupported remotes, pass `apm audit --ci --policy ` explicitly. - **Trust anchor is `git remote get-url origin`.** A developer who pushes the project to a personal org will have policy discovery resolve `/.github/apm-policy.yml` -- which they control. Operational mitigation: branch protection on the canonical repo is the trust boundary; nothing about a personal fork can bypass what your CI requires before merge. - **`apm install --dry-run` silently downgrades hash-mismatch.** In dry-run, `raise_blocking_errors=False` (outcome_routing.py:104-119) causes the mismatch to surface as `discovery_miss` with no "Would be blocked" line and exit 0. Operational mitigation: rely on `apm audit --ci` in CI for hash-pin verification, not on `apm install --dry-run`. - **`apm audit --ci --no-policy` and `APM_POLICY_DISABLE=1` skip policy checks.** The 21 policy checks are bypassed in audit, but the 8 baseline lockfile checks still run. Operational mitigation: keep bypass flags out of required CI workflows; the bypass contract in section 7 is authoritative. diff --git a/docs/src/content/docs/enterprise/policy-reference.md b/docs/src/content/docs/enterprise/policy-reference.md index 827ca3601b..b6df0a967a 100644 --- a/docs/src/content/docs/enterprise/policy-reference.md +++ b/docs/src/content/docs/enterprise/policy-reference.md @@ -619,8 +619,9 @@ found in the organization cascade: `.github-private`, `.github`, `.apm`, then `_apm`. Azure DevOps remotes use the org `apm` project and `apm-policy` repository. Legacy `_apm/_apm` is a temporary fallback after a 404 from the primary coordinate. -GitLab remotes use `/apm-policy/apm-policy.yml`, using the -first path segment of the remote; nested subgroup scopes are not searched. Set +GitLab walks the subgroup tree from the project's own group up to the top-level +group and applies the closest `apm-policy` project; a flat `/` +remote probes only `/apm-policy`. Set `GITLAB_HOST` or `APM_GITLAB_HOSTS` to recognize a self-managed host, and use `APM_GITLAB_POLICY_REPO` to select another project name. Plain git remotes fall through with no policy applied. Repositories with no detectable git remote diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index b2abf2cdfc..515ddb97bd 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -423,9 +423,13 @@ may use. This section covers how that contract is enforced at `apm install` time APM auto-discovers org policy from the project's git remote by checking `.github-private`, `.github`, `.apm`, and `_apm` policy repos in order on GitHub API-compatible hosts. Azure DevOps hosts use repository `apm-policy` in project -`apm`, with a legacy `_apm/_apm` fallback after a 404. GitLab uses -`/apm-policy/apm-policy.yml`, derived from the first remote -path segment; nested subgroup scopes are not searched. Configure a self-managed host with +`apm`, with a legacy `_apm/_apm` fallback after a 404. GitLab walks the subgroup +tree from the project's own group up to the top-level group and applies the +closest `apm-policy` (e.g. `acme/dept-a/team-x/apm-policy` before +`acme/dept-a/apm-policy` before `acme/apm-policy`), so a team can scope its own +policy under a subgroup; a team policy can `extends:` an ancestor group's policy +to inherit it. A flat `/` remote probes only `/apm-policy`. +Configure a self-managed host with `GITLAB_HOST` or `APM_GITLAB_HOSTS`, and use `APM_GITLAB_POLICY_REPO` to select another project name. Repositories with no detectable git remote (unpacked bundles, temp dirs) emit an explicit "could not determine org" line and skip diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index 1175d6cac7..da1274fbd4 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -136,8 +136,12 @@ def _fetch_gitlab_chain_parent( ) org = current_org else: - parts = parent_ref.strip("/").split("/") - if len(parts) == 3: + parts = [p for p in parent_ref.strip("/").split("/") if p] + # Strip an explicit leaf-host prefix so ``host/namespace/.../repo`` and + # ``namespace/.../repo`` are treated the same. Only the leaf host (with + # matching port) is accepted; cross-host refs are rejected upstream by + # ``_validate_extends_host``. Requires at least host + namespace + repo. + if len(parts) >= 3: try: explicit = urlsplit(f"//{parts[0]}") except ValueError: @@ -149,13 +153,17 @@ def _fetch_gitlab_chain_parent( and explicit.port == port ): parts = parts[1:] - if len(parts) != 2: + # A GitLab namespace may be nested (subgroups, see #2753): everything + # before the final segment is the namespace, the final segment is the + # policy repo. Requires at least ``namespace/repo``. + if len(parts) < 2: return PolicyFetchResult( source=f"org:{parent_ref}", error=f"Invalid GitLab policy reference: {parent_ref}", outcome="cache_miss_fetch_fail", ) - org, repo = parts + org = "/".join(parts[:-1]) + repo = parts[-1] return _fetch_from_gitlab_repo( org=org, repo=repo, diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 670457d1bd..28651f0f06 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -411,10 +411,19 @@ def _extract_extends_host(ref: str) -> str | None: return None parts = ref.split("/") if len(parts) >= 3: - try: - return (urlsplit(f"//{parts[0]}").hostname or "").lower() or None - except ValueError: - return None + # Only treat the first segment as a host when it is host-like: an FQDN + # (contains a dot) or an explicit ``host:port``. A bare single label is + # a namespace segment, not a host -- e.g. a GitLab nested-subgroup ref + # ``extends: "acme/dept-a/apm-policy"`` (#2753), whose first segment is + # the top-level group, must NOT trip the cross-host guard. A real + # attacker host (``evil.example.com/org/repo``) is an FQDN and is still + # detected and rejected. + first = parts[0] + if "." in first or ":" in first: + try: + return (urlsplit(f"//{first}").hostname or "").lower() or None + except ValueError: + return None return None @@ -858,6 +867,11 @@ def _auto_discover( except ValueError as exc: return PolicyFetchResult(error=str(exc), outcome="cache_miss_fetch_fail") is_ado = is_azure_devops_hostname(host) + is_gitlab = is_gitlab_hostname(host) + # GitLab discovery walks the subgroup tree from the deepest subgroup up to + # the top-level group (closest policy wins, see #2753). Fall back to the + # single top-level org when the namespace cannot be derived. + gitlab_namespaces = (_gitlab_namespace_descending(project_root) or [org]) if is_gitlab else [] for candidate_repo in candidates: logger.debug("Trying org policy repo candidate %s on host %s", candidate_repo, host) @@ -871,10 +885,10 @@ def _auto_discover( expected_hash=expected_hash, cache_only=cache_only, ) - elif is_gitlab_hostname(host): - result = _gitlab._fetch_from_gitlab_repo( - org=org, - repo=candidate_repo, + elif is_gitlab: + result = _gitlab_walk_candidate( + candidate_repo=candidate_repo, + namespaces=gitlab_namespaces, host=host, port=port, project_root=project_root, @@ -937,10 +951,13 @@ def _extract_org_from_git_remote( return (identity[0], identity[1]) if identity is not None else None -def _extract_org_host_port_from_git_remote( - project_root: Path, -) -> tuple[str, str, int | None] | None: - """Extract ``(org, host, port)`` from git remote origin.""" +def _git_remote_origin_url(project_root: Path) -> str | None: + """Return the ``origin`` remote URL, or ``None`` when unavailable. + + Canonical reader of the project's ``origin`` remote, shared by + :func:`_extract_org_host_port_from_git_remote` and GitLab subgroup + discovery so the ``git remote get-url`` subprocess lives in one place. + """ try: result = subprocess.run( [get_git_executable(), "remote", "get-url", "origin"], @@ -950,33 +967,113 @@ def _extract_org_host_port_from_git_remote( cwd=project_root, timeout=5, ) - if result.returncode != 0: - return None - remote_url = result.stdout.strip() + except (subprocess.TimeoutExpired, FileNotFoundError): + return None + if result.returncode != 0: + return None + return result.stdout.strip() + + +def _extract_org_host_port_from_git_remote( + project_root: Path, +) -> tuple[str, str, int | None] | None: + """Extract ``(org, host, port)`` from git remote origin.""" + remote_url = _git_remote_origin_url(project_root) + if not remote_url: + return None + try: parsed_identity = _parse_remote_url(remote_url) - if parsed_identity is None: + except ValueError: + return None + if parsed_identity is None: + return None + port = None + if "://" in remote_url: + try: + port = urlparse(remote_url).port + except ValueError: return None - port = None - if "://" in remote_url: - try: - port = urlparse(remote_url).port - except ValueError: - return None - return parsed_identity[0], parsed_identity[1], port - except (ValueError, subprocess.TimeoutExpired, FileNotFoundError): + return parsed_identity[0], parsed_identity[1], port + + +def _gitlab_namespace_descending(project_root: Path) -> list[str] | None: + """Return GitLab policy namespaces from the deepest subgroup to the top. + + Probe order is closest-first. For a project remote + ``gitlab.com/acme/dept-a/team-x/my-project`` this returns + ``["acme/dept-a/team-x", "acme/dept-a", "acme"]``; a flat + ``acme/my-project`` returns ``["acme"]`` (identical to top-level-only + discovery). Returns ``None`` when the origin remote is missing or has no + namespace segment (a degenerate remote with no owner), so callers can fall + back to their existing single-org behaviour. + """ + remote_url = _git_remote_origin_url(project_root) + if not remote_url: + return None + parts = _remote_url_parts(remote_url) + if parts is None: return None + _host, segments = parts + # The final segment is the project; everything before it is the namespace. + namespace_segments = segments[:-1] + if not namespace_segments: + return None + return ["/".join(namespace_segments[:n]) for n in range(len(namespace_segments), 0, -1)] -def _parse_remote_url(url: str) -> tuple[str, str] | None: - """Parse a git remote URL into (org, host). +def _gitlab_walk_candidate( + *, + candidate_repo: str, + namespaces: list[str], + host: str, + port: int | None, + project_root: Path, + no_cache: bool, + expected_hash: str | None, + cache_only: bool, +) -> PolicyFetchResult: + """Probe one GitLab policy repo up the subgroup tree (closest wins, #2753). - Accepts SCP-style SSH URLs with any username (not just ``git@``), so - EMU/GHE deployments that use a non-``git`` SSH user - (e.g. ``enterprise-user@ghe.corp.com:org/repo.git``) parse correctly. - Also handles Azure DevOps SSH URLs which carry an extra ``v3/`` - path prefix (``git@ssh.dev.azure.com:v3///``). + Fetches ``//apm-policy.yml`` for each namespace + in *namespaces* (already ordered deepest-first). The first non-``absent`` + outcome wins; an ``absent`` level continues to the next-shallower group. + When every level is absent the last ``absent`` result is returned so the + caller's candidate cascade can proceed unchanged. + """ + result = PolicyFetchResult(error=None, outcome="absent") + for namespace in namespaces: + result = _gitlab._fetch_from_gitlab_repo( + org=namespace, + repo=candidate_repo, + host=host, + port=port, + project_root=project_root, + no_cache=no_cache, + expected_hash=expected_hash, + cache_only=cache_only, + ) + if result.outcome != "absent": + return result + logger.debug( + "GitLab policy absent at %s/%s; trying parent group", + namespace, + candidate_repo, + ) + return result - Returns None if URL can't be parsed. + +def _remote_url_parts(url: str) -> tuple[str, list[str]] | None: + """Split a git remote URL into ``(host, path_segments)``. + + Canonical splitter shared by :func:`_parse_remote_url` (which applies + host-specific org interpretation on top) and GitLab subgroup discovery + (:func:`_gitlab_namespace_descending`). Handles SCP-like SSH URLs with any + username (not just ``git@``) and ``scheme://`` URLs. Path segments are + cleaned of empty parts and the trailing ``.git`` suffix; no host-specific + interpretation (ADO ``v3/`` prefix, visualstudio subdomain, ...) is applied + here -- that stays the caller's responsibility. + + Returns ``None`` when the URL cannot be split. """ if not url: return None @@ -987,45 +1084,75 @@ def _parse_remote_url(url: str) -> tuple[str, str] | None: if scp_match: host = scp_match.group("host") path_part = scp_match.group("path") - try: - parts = path_part.rstrip("/").removesuffix(".git").split("/") - parts = [p for p in parts if p] - if not parts: - return None - # Azure DevOps SSH carries a leading 'v3/' segment that is - # NOT the org. The org is the second segment. - if host == "ssh.dev.azure.com" and parts[0] == "v3" and len(parts) >= 2: - return (parts[1], host) - return (parts[0], host) - except (ValueError, IndexError): + segments = [p for p in path_part.rstrip("/").removesuffix(".git").split("/") if p] + if not host or not segments: return None + return (host, segments) # HTTPS: https://github.com/owner/repo.git - # ADO: https://dev.azure.com/org/project/_git/repo if "://" in url: try: parsed = urlparse(url) - host = parsed.hostname or "" - if is_azure_devops_hostname(host): - ado_coordinates = parse_ado_repo_url(url) - if ado_coordinates is None: - return None - return ado_coordinates[0], host - path_parts = parsed.path.strip("/").removesuffix(".git").rstrip("/").split("/") - if is_visualstudio_legacy_hostname(host): - return (host[: -len(".visualstudio.com")], host) - if host and path_parts and path_parts[0]: - return (path_parts[0], host) - except ValueError as exc: - if "mounted below '/tfs/'" in str(exc): - raise - return None except Exception: + # urlparse only raises ValueError in practice, but the legacy + # parser swallowed any exception here; preserve that. The + # ``/tfs/`` ValueError comes from parse_ado_repo_url (in + # _parse_remote_url), never from urlparse, so nothing to re-raise. return None + host = parsed.hostname or "" + segments = [ + p for p in parsed.path.strip("/").removesuffix(".git").rstrip("/").split("/") if p + ] + if not host or not segments: + return None + return (host, segments) return None +def _parse_remote_url(url: str) -> tuple[str, str] | None: + """Parse a git remote URL into (org, host). + + Accepts SCP-style SSH URLs with any username (not just ``git@``), so + EMU/GHE deployments that use a non-``git`` SSH user + (e.g. ``enterprise-user@ghe.corp.com:org/repo.git``) parse correctly. + Also handles Azure DevOps SSH URLs which carry an extra ``v3/`` + path prefix (``git@ssh.dev.azure.com:v3///``). + + Returns None if URL can't be parsed. + """ + parts = _remote_url_parts(url) + if parts is None: + return None + host, segments = parts + + # SCP-like SSH: @: -- any user, not just `git`. + if SCP_LIKE_RE.match(url): + # Azure DevOps SSH carries a leading 'v3/' segment that is + # NOT the org. The org is the second segment. + if host == "ssh.dev.azure.com" and segments[0] == "v3" and len(segments) >= 2: + return (segments[1], host) + return (segments[0], host) + + # HTTPS: https://github.com/owner/repo.git + # ADO: https://dev.azure.com/org/project/_git/repo + try: + if is_azure_devops_hostname(host): + ado_coordinates = parse_ado_repo_url(url) + if ado_coordinates is None: + return None + return ado_coordinates[0], host + if is_visualstudio_legacy_hostname(host): + return (host[: -len(".visualstudio.com")], host) + return (segments[0], host) + except ValueError as exc: + if "mounted below '/tfs/'" in str(exc): + raise + return None + except Exception: + return None + + def _fetch_from_url( url: str, project_root: Path, diff --git a/tests/spec_conformance/test_policy_reqs.py b/tests/spec_conformance/test_policy_reqs.py index edf6765214..9dde24f077 100644 --- a/tests/spec_conformance/test_policy_reqs.py +++ b/tests/spec_conformance/test_policy_reqs.py @@ -143,6 +143,26 @@ def test_policy_gitlab_discovery_provider_is_a_distinct_convention(): assert_spec_contains("gitlab-project-yml") +@pytest.mark.req("req-pl-011") +def test_policy_gitlab_discovery_walks_subgroups_closest_first(): + """req-pl-011: the GitLab provider probes the subgroup tree in a defined, + ordered manner (closest subgroup first, up to the top-level group), rather + than hard-coding a single top-level-only convention (#2753).""" + from unittest.mock import patch + + from apm_cli.policy.discovery import _gitlab_namespace_descending + + with patch( + "apm_cli.policy.discovery._git_remote_origin_url", + return_value="https://gitlab.com/acme/dept-a/team-x/my-project.git", + ): + order = _gitlab_namespace_descending(object()) + + # Deepest subgroup first, then each shallower group, ending at the + # top-level group -- a deterministic, documented probe order. + assert order == ["acme/dept-a/team-x", "acme/dept-a", "acme"] + + @pytest.mark.req("req-pl-012") def test_policy_provides_default_deny_list_shape(): schema = load_schema("policy-v0.1.schema.json") diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 95546ce2e2..8a3376ece1 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -29,6 +29,7 @@ PolicyFetchResult, _auto_discover, _cache_key, + _extract_extends_host, _extract_org_from_git_remote, _extract_org_host_port_from_git_remote, _fetch_ado_contents, @@ -39,11 +40,14 @@ _fetch_from_url, _fetch_github_contents, _get_cache_dir, + _gitlab_namespace_descending, _load_from_file, _parse_remote_url, _policy_repo_candidates, _read_cache, _read_cache_entry, + _remote_url_parts, + _validate_extends_host, _write_cache, discover_policy, ) @@ -157,6 +161,78 @@ def test_ado_ssh_v3_prefix_with_git_suffix(self): result = _parse_remote_url("git@ssh.dev.azure.com:v3/myorg/myproject/myrepo.git") self.assertEqual(result, ("myorg", "ssh.dev.azure.com")) + # --- Nested GitLab subgroups: org stays the top-level group (#2753) --- + + def test_https_gitlab_nested_subgroups_org_is_top_level(self): + result = _parse_remote_url("https://gitlab.com/acme/dept-a/team-x/my-project.git") + self.assertEqual(result, ("acme", "gitlab.com")) + + def test_ssh_gitlab_nested_subgroups_org_is_top_level(self): + result = _parse_remote_url("git@gitlab.com:acme/dept-a/team-x/my-project.git") + self.assertEqual(result, ("acme", "gitlab.com")) + + +class TestRemoteUrlParts(unittest.TestCase): + """Test _remote_url_parts -- the shared host + path-segments splitter.""" + + def test_https_nested_segments(self): + result = _remote_url_parts("https://gitlab.com/acme/dept-a/team-x/my-project.git") + self.assertEqual(result, ("gitlab.com", ["acme", "dept-a", "team-x", "my-project"])) + + def test_ssh_nested_segments(self): + result = _remote_url_parts("git@gitlab.com:acme/dept-a/team-x/my-project.git") + self.assertEqual(result, ("gitlab.com", ["acme", "dept-a", "team-x", "my-project"])) + + def test_https_flat_segments(self): + result = _remote_url_parts("https://gitlab.com/acme/my-project") + self.assertEqual(result, ("gitlab.com", ["acme", "my-project"])) + + def test_https_trailing_slash_is_cleaned(self): + result = _remote_url_parts("https://gitlab.com/acme/my-project/") + self.assertEqual(result, ("gitlab.com", ["acme", "my-project"])) + + def test_empty_string_returns_none(self): + self.assertIsNone(_remote_url_parts("")) + + def test_non_url_returns_none(self): + self.assertIsNone(_remote_url_parts("not-a-url")) + + def test_ssh_empty_path_returns_none(self): + self.assertIsNone(_remote_url_parts("git@gitlab.com:")) + + def test_https_no_path_returns_none(self): + self.assertIsNone(_remote_url_parts("https://gitlab.com/")) + + +class TestGitlabNamespaceDescending(unittest.TestCase): + """Test _gitlab_namespace_descending -- subgroup probe order (#2753).""" + + @patch("apm_cli.policy.discovery._git_remote_origin_url") + def test_nested_subgroups_deepest_first(self, mock_url): + mock_url.return_value = "https://gitlab.com/acme/dept-a/team-x/my-project.git" + result = _gitlab_namespace_descending(Path("/fake")) + self.assertEqual( + result, + ["acme/dept-a/team-x", "acme/dept-a", "acme"], + ) + + @patch("apm_cli.policy.discovery._git_remote_origin_url") + def test_flat_project_yields_single_top_level(self, mock_url): + mock_url.return_value = "git@gitlab.com:acme/my-project.git" + result = _gitlab_namespace_descending(Path("/fake")) + self.assertEqual(result, ["acme"]) + + @patch("apm_cli.policy.discovery._git_remote_origin_url") + def test_no_remote_returns_none(self, mock_url): + mock_url.return_value = None + self.assertIsNone(_gitlab_namespace_descending(Path("/fake"))) + + @patch("apm_cli.policy.discovery._git_remote_origin_url") + def test_no_namespace_segment_returns_none(self, mock_url): + # A remote with only one path segment has no owning namespace. + mock_url.return_value = "https://gitlab.com/solo" + self.assertIsNone(_gitlab_namespace_descending(Path("/fake"))) + class TestExtractOrgFromGitRemote(unittest.TestCase): """Test _extract_org_from_git_remote with mocked subprocess.""" @@ -1112,6 +1188,134 @@ def test_gitlab_absent_is_clean_no_op(self, mock_extract, mock_gitlab_fetch): self.assertEqual(result.outcome, "absent") self.assertIsNone(result.error) + # --- GitLab subgroup walk: closest policy wins (#2753) --- + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_closest_wins(self, mock_extract, mock_ns, mock_gitlab_fetch): + """The deepest subgroup with a policy wins; parents are not probed.""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.return_value = PolicyFetchResult( + policy=ApmPolicy(), + source="org:gitlab.com/acme/dept-a/team-x/apm-policy", + outcome="found", + ) + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertTrue(result.found) + mock_gitlab_fetch.assert_called_once() + self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "acme/dept-a/team-x") + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_absent_ascends_to_parent( + self, mock_extract, mock_ns, mock_gitlab_fetch + ): + """Absent at the deepest levels -> ascend until a policy is found.""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.side_effect = [ + PolicyFetchResult(outcome="absent"), # team-x: no policy + PolicyFetchResult(outcome="absent"), # dept-a: no policy + PolicyFetchResult( + policy=ApmPolicy(), + source="org:gitlab.com/acme/apm-policy", + outcome="found", + ), # acme: top-level policy + ] + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertTrue(result.found) + self.assertEqual(mock_gitlab_fetch.call_count, 3) + probed = [c.kwargs["org"] for c in mock_gitlab_fetch.call_args_list] + self.assertEqual(probed, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_all_absent_is_clean_absent( + self, mock_extract, mock_ns, mock_gitlab_fetch + ): + """No policy at any level -> clean absent, every level probed once.""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.return_value = PolicyFetchResult(outcome="absent") + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertEqual(result.outcome, "absent") + self.assertIsNone(result.error) + self.assertEqual(mock_gitlab_fetch.call_count, 3) + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_error_fails_closed(self, mock_extract, mock_ns, mock_gitlab_fetch): + """A fetch error at the deepest level fails closed -- parents not probed.""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.side_effect = [ + PolicyFetchResult(error="403: Access denied", outcome="cache_miss_fetch_fail"), + PolicyFetchResult(policy=ApmPolicy(), outcome="found"), + ] + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertFalse(result.found) + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + mock_gitlab_fetch.assert_called_once() + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_error_at_inner_level_fails_closed( + self, mock_extract, mock_ns, mock_gitlab_fetch + ): + """Fail-closed holds when the error appears after ascending (not first).""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.side_effect = [ + PolicyFetchResult(outcome="absent"), # team-x: no policy -> ascend + PolicyFetchResult(error="403: Access denied", outcome="cache_miss_fetch_fail"), + PolicyFetchResult(policy=ApmPolicy(), outcome="found"), # must NOT be reached + ] + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertFalse(result.found) + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + self.assertEqual(mock_gitlab_fetch.call_count, 2) + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_flat_project_falls_back_to_top_level_org( + self, mock_extract, mock_ns, mock_gitlab_fetch + ): + """No derivable namespace -> single top-level org probe (back-compat).""" + mock_extract.return_value = ("contoso", "gitlab.com", None) + mock_ns.return_value = None # namespace not derivable + mock_gitlab_fetch.return_value = PolicyFetchResult( + policy=ApmPolicy(), source="org:gitlab.com/contoso/apm-policy", outcome="found" + ) + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertTrue(result.found) + mock_gitlab_fetch.assert_called_once() + self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "contoso") + class TestPolicyRepoCandidates(unittest.TestCase): """Test _policy_repo_candidates host profile selection.""" @@ -1232,6 +1436,91 @@ def test_gitlab_parent_accepts_same_host_qualified_reference(self, mock_fetch): self.assertEqual(mock_fetch.call_args.kwargs["org"], "platform") self.assertEqual(mock_fetch.call_args.kwargs["repo"], "baseline") + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_accepts_nested_namespace_reference(self, mock_fetch): + """An extends: ref into an intermediate subgroup resolves (#2753).""" + mock_fetch.return_value = PolicyFetchResult(outcome="absent") + + with tempfile.TemporaryDirectory() as tmpdir: + _fetch_chain_parent( + "acme/dept-a/apm-policy", + current_source="org:gitlab.com/acme/dept-a/team-x/apm-policy", + leaf_host="gitlab.com", + leaf_port=None, + project_root=Path(tmpdir), + no_cache=True, + ) + + # Namespace is everything before the final segment; repo is the last. + self.assertEqual(mock_fetch.call_args.kwargs["org"], "acme/dept-a") + self.assertEqual(mock_fetch.call_args.kwargs["repo"], "apm-policy") + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_accepts_host_qualified_nested_namespace(self, mock_fetch): + """A host-qualified extends: ref into a nested subgroup strips the host.""" + mock_fetch.return_value = PolicyFetchResult(outcome="absent") + + with tempfile.TemporaryDirectory() as tmpdir: + _fetch_chain_parent( + "gitlab.com/acme/dept-a/apm-policy", + current_source="org:gitlab.com/acme/dept-a/team-x/apm-policy", + leaf_host="gitlab.com", + leaf_port=None, + project_root=Path(tmpdir), + no_cache=True, + ) + + self.assertEqual(mock_fetch.call_args.kwargs["org"], "acme/dept-a") + self.assertEqual(mock_fetch.call_args.kwargs["repo"], "apm-policy") + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_rejects_single_segment_reference(self, mock_fetch): + """A bare single-segment extends: ref fails closed, never probes.""" + mock_fetch.return_value = PolicyFetchResult(outcome="found", policy=ApmPolicy()) + + with tempfile.TemporaryDirectory() as tmpdir: + result = _fetch_chain_parent( + "apm-policy", + current_source="org:gitlab.com/acme/apm-policy", + leaf_host="gitlab.com", + leaf_port=None, + project_root=Path(tmpdir), + no_cache=True, + ) + + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + self.assertIn("Invalid GitLab policy reference", result.error) + mock_fetch.assert_not_called() + + +class TestValidateExtendsHostNestedNamespace(unittest.TestCase): + """The pre-fetch host-pin guard must allow nested GitLab namespaces (#2753). + + ``_validate_extends_host`` runs BEFORE the GitLab adapter fetch, so a bare + nested-namespace ``extends:`` ref (single-label first segment) must not be + misread as a cross-host reference -- while a real attacker FQDN still is. + """ + + def test_nested_namespace_ref_is_not_a_host(self): + # First segment is a top-level group, not a host -> shorthand. + self.assertIsNone(_extract_extends_host("acme/dept-a/apm-policy")) + + def test_host_qualified_nested_ref_extracts_host(self): + self.assertEqual( + _extract_extends_host("gitlab.com/acme/dept-a/apm-policy"), + "gitlab.com", + ) + + def test_validate_allows_nested_same_host_reference(self): + # Must NOT raise: this is the exact ref the subgroup feature enables. + _validate_extends_host("gitlab.com", "acme/dept-a/apm-policy") + + def test_validate_still_rejects_cross_host_fqdn(self): + import apm_cli.policy.inheritance as _inh + + with self.assertRaisesRegex(_inh.PolicyInheritanceError, "cross-host"): + _validate_extends_host("gitlab.com", "evil.example.com/org/apm-policy") + class TestFetchAdoContents(unittest.TestCase): """Test _fetch_ado_contents for Azure DevOps Items API.""" From 24afdadf7b3d2ad9723c364d1dbd32255e5c0b62 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:11:41 -0300 Subject: [PATCH 02/14] docs(changelog): reference PR #2933 Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed547d182..bcc1b5b933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- GitLab org-policy discovery now walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` project, so a team can scope its own governance under a subgroup instead of only the top-level group; a subgroup policy can `extends:` an ancestor group's policy (including nested-namespace references). Flat `/` remotes are unchanged. (closes #2753) (#PR) +- GitLab org-policy discovery now walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` project, so a team can scope its own governance under a subgroup instead of only the top-level group; a subgroup policy can `extends:` an ancestor group's policy (including nested-namespace references). Flat `/` remotes are unchanged. (closes #2753) (#2933) - gh-aw's shared APM import now supports `token-source: github-token`; after consumers re-vendor the workflow, its read-only current-repository identity can fetch same-repository private packages, while `cascade` remains the default and cross-repository packages still require a dedicated token or GitHub App. (#2706) - OpenAPM v0.1 adds `req-pl-018` for dependency-policy identity casing and amends `req-rs-016` clause (3), the Section 6.4 merge rules, and the Section 6.5 pattern grammar so repository identity and policy matching cannot diverge; Section 11.2 item 6 now requires the per-host case rule in `CONFORMANCE.md`. (#2706) From e9cf62ef4d37d520f9009613e4efb3204753648b Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:43:21 -0300 Subject: [PATCH 03/14] fix(policy): address Copilot review on GitLab subgroup discovery - host-pin (F1): scope the "single-label first segment is a namespace" relaxation to GitLab leaves only. On GitHub/ADO leaves a 3-segment ref is host/owner/repo, so `extends: "evil/org/repo"` is again rejected as cross-host instead of routing a credential to `evil`. - GitLab extends: a host-like first segment must match the leaf host+port exactly or the ref is rejected (never folded into the namespace); a malformed port now fails closed instead of raising ValueError. - discovery: read `git remote get-url origin` once and reuse it for both identity and the namespace walk (`_gitlab_namespace_descending` takes the URL), removing the double subprocess and the TOCTOU on a changing remote. - docs: publishing checklist now says GitLab policy may live under any ancestor namespace (closest wins), not the top-level group only. - tests: cover GitHub-leaf cross-host rejection, port-mismatch and malformed-port rejection, and the pure namespace helper. Co-Authored-By: Claude Opus 4.8 --- .../.apm/skills/apm-usage/governance.md | 3 +- src/apm_cli/policy/_gitlab.py | 47 ++++---- src/apm_cli/policy/discovery.py | 67 ++++++++---- tests/spec_conformance/test_policy_reqs.py | 8 +- tests/unit/policy/test_discovery.py | 100 +++++++++++++----- 5 files changed, 151 insertions(+), 74 deletions(-) diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index 515ddb97bd..277410d8f0 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -679,7 +679,8 @@ as `[x]` errors and exit `1`. Checklist to publish a policy: 1. Create `apm-policy.yml` in the org policy repo (`.github-private` or `.github` on GitHub, `apm` - project and `apm-policy` repository on Azure DevOps, or `apm-policy` under the top-level GitLab group). + project and `apm-policy` repository on Azure DevOps, or an `apm-policy` project under the GitLab + group you want it to govern -- any ancestor namespace of the project, closest wins). 2. Start from the recommended starter below and trim to the minimum reflecting your governance posture. 3. Set `enforcement: warn` first. Let CI surface diagnostics across consuming diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index da1274fbd4..088860a68c 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -137,31 +137,36 @@ def _fetch_gitlab_chain_parent( org = current_org else: parts = [p for p in parent_ref.strip("/").split("/") if p] - # Strip an explicit leaf-host prefix so ``host/namespace/.../repo`` and - # ``namespace/.../repo`` are treated the same. Only the leaf host (with - # matching port) is accepted; cross-host refs are rejected upstream by - # ``_validate_extends_host``. Requires at least host + namespace + repo. - if len(parts) >= 3: + invalid = PolicyFetchResult( + source=f"org:{parent_ref}", + error=f"Invalid GitLab policy reference: {parent_ref}", + outcome="cache_miss_fetch_fail", + ) + # A host-like first segment (FQDN or ``host:port``) MUST match the leaf + # host and port exactly, then it is stripped so ``host/namespace/.../repo`` + # and ``namespace/.../repo`` are treated the same. A host-like segment + # that does not match the leaf -- or carries a malformed port -- is a + # cross-host/invalid ref and is rejected, never silently folded into the + # namespace. A bare single-label first segment is a namespace segment + # (GitLab nested subgroups, see #2753) and is left in place. + if len(parts) >= 3 and ("." in parts[0] or ":" in parts[0]): try: explicit = urlsplit(f"//{parts[0]}") + same_leaf = ( + explicit.hostname is not None + and explicit.hostname.lower() == leaf_host.lower() + and explicit.port == port + ) except ValueError: - explicit = None - if ( - explicit is not None - and explicit.hostname is not None - and explicit.hostname.lower() == leaf_host.lower() - and explicit.port == port - ): - parts = parts[1:] - # A GitLab namespace may be nested (subgroups, see #2753): everything - # before the final segment is the namespace, the final segment is the - # policy repo. Requires at least ``namespace/repo``. + same_leaf = False + if not same_leaf: + return invalid + parts = parts[1:] + # A GitLab namespace may be nested (subgroups): everything before the + # final segment is the namespace, the final segment is the policy repo. + # Requires at least ``namespace/repo``. if len(parts) < 2: - return PolicyFetchResult( - source=f"org:{parent_ref}", - error=f"Invalid GitLab policy reference: {parent_ref}", - outcome="cache_miss_fetch_fail", - ) + return invalid org = "/".join(parts[:-1]) repo = parts[-1] return _fetch_from_gitlab_repo( diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 28651f0f06..418440b0b9 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -389,13 +389,17 @@ def _derive_leaf_host(source: str, project_root: Path) -> str | None: return _derive_leaf_identity(source, project_root)[0] -def _extract_extends_host(ref: str) -> str | None: +def _extract_extends_host(ref: str, leaf_host: str | None = None) -> str | None: """Return the host an ``extends:`` ref resolves against, if explicit. * Full URL -> URL host (lowercase) * ``//`` (3+ slash-segments) -> ```` (lowercase) * ``/`` shorthand -> None (intrinsically same-host) * ```` shorthand (no slash) -> None (intrinsically same-host) + + ``leaf_host`` gates the GitLab-only relaxation for nested-subgroup refs + (see the 3-segment branch below); it defaults to ``None`` so non-GitLab + callers keep the strict host-pin behaviour. """ if not ref: return None @@ -411,19 +415,21 @@ def _extract_extends_host(ref: str) -> str | None: return None parts = ref.split("/") if len(parts) >= 3: - # Only treat the first segment as a host when it is host-like: an FQDN - # (contains a dot) or an explicit ``host:port``. A bare single label is - # a namespace segment, not a host -- e.g. a GitLab nested-subgroup ref - # ``extends: "acme/dept-a/apm-policy"`` (#2753), whose first segment is - # the top-level group, must NOT trip the cross-host guard. A real - # attacker host (``evil.example.com/org/repo``) is an FQDN and is still - # detected and rejected. first = parts[0] - if "." in first or ":" in first: - try: - return (urlsplit(f"//{first}").hostname or "").lower() or None - except ValueError: - return None + host_like = "." in first or ":" in first + # A bare single-label first segment is a namespace segment ONLY on + # GitLab, where nested subgroups make ``acme/dept-a/apm-policy`` a valid + # same-host ``extends:`` ref (#2753). On every other provider a + # 3-segment ref is ``host/owner/repo``; treating a single label as a + # namespace there would let ``extends: "evil/org/repo"`` bypass the + # host-pin and route a credential to ``evil`` (Security Finding F1), so + # the first segment is parsed as a (cross-)host and rejected. + if is_gitlab_hostname(leaf_host) and not host_like: + return None + try: + return (urlsplit(f"//{first}").hostname or "").lower() or None + except ValueError: + return None return None @@ -441,7 +447,7 @@ def _validate_extends_host(leaf_host: str | None, extends_ref: str) -> None: """ from . import inheritance as _inheritance_mod - extends_host = _extract_extends_host(extends_ref) + extends_host = _extract_extends_host(extends_ref, leaf_host) if extends_host is None: return # shorthand: intrinsically same-host, allowed. @@ -854,7 +860,10 @@ def _auto_discover( - Found -> return (first match wins) 5. All candidates exhausted -> outcome="absent" """ - identity = _extract_org_host_port_from_git_remote(project_root) + # Read origin once and reuse it for both identity and the GitLab namespace + # walk so discovery never invokes ``git remote get-url`` more than once. + remote_url = _git_remote_origin_url(project_root) + identity = _extract_org_host_port_from_git_remote(project_root, remote_url=remote_url) if identity is None: return PolicyFetchResult( error="Could not determine org from git remote", @@ -871,7 +880,7 @@ def _auto_discover( # GitLab discovery walks the subgroup tree from the deepest subgroup up to # the top-level group (closest policy wins, see #2753). Fall back to the # single top-level org when the namespace cannot be derived. - gitlab_namespaces = (_gitlab_namespace_descending(project_root) or [org]) if is_gitlab else [] + gitlab_namespaces = (_gitlab_namespace_descending(remote_url) or [org]) if is_gitlab else [] for candidate_repo in candidates: logger.debug("Trying org policy repo candidate %s on host %s", candidate_repo, host) @@ -951,6 +960,13 @@ def _extract_org_from_git_remote( return (identity[0], identity[1]) if identity is not None else None +class _Unset: + """Sentinel type: an argument was not provided (distinct from ``None``).""" + + +_UNSET = _Unset() + + def _git_remote_origin_url(project_root: Path) -> str | None: """Return the ``origin`` remote URL, or ``None`` when unavailable. @@ -976,9 +992,18 @@ def _git_remote_origin_url(project_root: Path) -> str | None: def _extract_org_host_port_from_git_remote( project_root: Path, + *, + remote_url: str | None | _Unset = _UNSET, ) -> tuple[str, str, int | None] | None: - """Extract ``(org, host, port)`` from git remote origin.""" - remote_url = _git_remote_origin_url(project_root) + """Extract ``(org, host, port)`` from git remote origin. + + ``remote_url`` lets a caller that already read ``origin`` (e.g. + :func:`_auto_discover`) pass it in so the ``git remote get-url`` subprocess + runs exactly once per discovery. When left unset the origin is read here; + an explicit ``None`` means "no remote" and is not re-read. + """ + if isinstance(remote_url, _Unset): + remote_url = _git_remote_origin_url(project_root) if not remote_url: return None try: @@ -996,7 +1021,7 @@ def _extract_org_host_port_from_git_remote( return parsed_identity[0], parsed_identity[1], port -def _gitlab_namespace_descending(project_root: Path) -> list[str] | None: +def _gitlab_namespace_descending(remote_url: str | None) -> list[str] | None: """Return GitLab policy namespaces from the deepest subgroup to the top. Probe order is closest-first. For a project remote @@ -1005,9 +1030,9 @@ def _gitlab_namespace_descending(project_root: Path) -> list[str] | None: ``acme/my-project`` returns ``["acme"]`` (identical to top-level-only discovery). Returns ``None`` when the origin remote is missing or has no namespace segment (a degenerate remote with no owner), so callers can fall - back to their existing single-org behaviour. + back to their existing single-org behaviour. Takes the already-read + ``origin`` URL so discovery reads the remote exactly once. """ - remote_url = _git_remote_origin_url(project_root) if not remote_url: return None parts = _remote_url_parts(remote_url) diff --git a/tests/spec_conformance/test_policy_reqs.py b/tests/spec_conformance/test_policy_reqs.py index 9dde24f077..c8e8d74031 100644 --- a/tests/spec_conformance/test_policy_reqs.py +++ b/tests/spec_conformance/test_policy_reqs.py @@ -148,15 +148,9 @@ def test_policy_gitlab_discovery_walks_subgroups_closest_first(): """req-pl-011: the GitLab provider probes the subgroup tree in a defined, ordered manner (closest subgroup first, up to the top-level group), rather than hard-coding a single top-level-only convention (#2753).""" - from unittest.mock import patch - from apm_cli.policy.discovery import _gitlab_namespace_descending - with patch( - "apm_cli.policy.discovery._git_remote_origin_url", - return_value="https://gitlab.com/acme/dept-a/team-x/my-project.git", - ): - order = _gitlab_namespace_descending(object()) + order = _gitlab_namespace_descending("https://gitlab.com/acme/dept-a/team-x/my-project.git") # Deepest subgroup first, then each shallower group, ending at the # top-level group -- a deterministic, documented probe order. diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 8a3376ece1..c151db3783 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -205,33 +205,28 @@ def test_https_no_path_returns_none(self): class TestGitlabNamespaceDescending(unittest.TestCase): - """Test _gitlab_namespace_descending -- subgroup probe order (#2753).""" + """Test _gitlab_namespace_descending -- subgroup probe order (#2753). - @patch("apm_cli.policy.discovery._git_remote_origin_url") - def test_nested_subgroups_deepest_first(self, mock_url): - mock_url.return_value = "https://gitlab.com/acme/dept-a/team-x/my-project.git" - result = _gitlab_namespace_descending(Path("/fake")) - self.assertEqual( - result, - ["acme/dept-a/team-x", "acme/dept-a", "acme"], + The helper is pure: it takes the already-read ``origin`` URL so discovery + reads the remote exactly once. + """ + + def test_nested_subgroups_deepest_first(self): + result = _gitlab_namespace_descending( + "https://gitlab.com/acme/dept-a/team-x/my-project.git" ) + self.assertEqual(result, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) - @patch("apm_cli.policy.discovery._git_remote_origin_url") - def test_flat_project_yields_single_top_level(self, mock_url): - mock_url.return_value = "git@gitlab.com:acme/my-project.git" - result = _gitlab_namespace_descending(Path("/fake")) + def test_flat_project_yields_single_top_level(self): + result = _gitlab_namespace_descending("git@gitlab.com:acme/my-project.git") self.assertEqual(result, ["acme"]) - @patch("apm_cli.policy.discovery._git_remote_origin_url") - def test_no_remote_returns_none(self, mock_url): - mock_url.return_value = None - self.assertIsNone(_gitlab_namespace_descending(Path("/fake"))) + def test_no_remote_returns_none(self): + self.assertIsNone(_gitlab_namespace_descending(None)) - @patch("apm_cli.policy.discovery._git_remote_origin_url") - def test_no_namespace_segment_returns_none(self, mock_url): + def test_no_namespace_segment_returns_none(self): # A remote with only one path segment has no owning namespace. - mock_url.return_value = "https://gitlab.com/solo" - self.assertIsNone(_gitlab_namespace_descending(Path("/fake"))) + self.assertIsNone(_gitlab_namespace_descending("https://gitlab.com/solo")) class TestExtractOrgFromGitRemote(unittest.TestCase): @@ -1492,6 +1487,51 @@ def test_gitlab_parent_rejects_single_segment_reference(self, mock_fetch): self.assertIn("Invalid GitLab policy reference", result.error) mock_fetch.assert_not_called() + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_rejects_host_prefix_with_mismatched_port(self, mock_fetch): + """A host-like prefix that does not match the leaf host+port is rejected, + never silently folded into the namespace.""" + mock_fetch.return_value = PolicyFetchResult(outcome="found", policy=ApmPolicy()) + + with ( + patch.dict(os.environ, {"GITLAB_HOST": "gitlab.example.test"}, clear=False), + tempfile.TemporaryDirectory() as tmpdir, + ): + result = _fetch_chain_parent( + "gitlab.example.test/acme/apm-policy", # host prefix, but leaf has :8443 + current_source="org:gitlab.example.test:8443/acme/team/apm-policy", + leaf_host="gitlab.example.test", + leaf_port=8443, + project_root=Path(tmpdir), + no_cache=True, + ) + + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + self.assertIn("Invalid GitLab policy reference", result.error) + mock_fetch.assert_not_called() + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_rejects_malformed_port_without_crashing(self, mock_fetch): + """A host-like prefix with a malformed port fails closed, not raises.""" + mock_fetch.return_value = PolicyFetchResult(outcome="found", policy=ApmPolicy()) + + with ( + patch.dict(os.environ, {"GITLAB_HOST": "gitlab.example.test"}, clear=False), + tempfile.TemporaryDirectory() as tmpdir, + ): + result = _fetch_chain_parent( + "gitlab.example.test:not-a-port/acme/apm-policy", + current_source="org:gitlab.example.test/acme/team/apm-policy", + leaf_host="gitlab.example.test", + leaf_port=None, + project_root=Path(tmpdir), + no_cache=True, + ) + + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + self.assertIn("Invalid GitLab policy reference", result.error) + mock_fetch.assert_not_called() + class TestValidateExtendsHostNestedNamespace(unittest.TestCase): """The pre-fetch host-pin guard must allow nested GitLab namespaces (#2753). @@ -1501,13 +1541,19 @@ class TestValidateExtendsHostNestedNamespace(unittest.TestCase): misread as a cross-host reference -- while a real attacker FQDN still is. """ - def test_nested_namespace_ref_is_not_a_host(self): - # First segment is a top-level group, not a host -> shorthand. - self.assertIsNone(_extract_extends_host("acme/dept-a/apm-policy")) + def test_nested_namespace_ref_is_not_a_host_on_gitlab_leaf(self): + # First segment is a top-level group, not a host -> shorthand (GitLab). + self.assertIsNone(_extract_extends_host("acme/dept-a/apm-policy", "gitlab.com")) + + def test_single_label_first_segment_stays_a_host_on_github_leaf(self): + # F1: on a non-GitLab leaf a single-label first segment is a host, so a + # cross-host ``extends: "evil/org/repo"`` must NOT be treated as + # same-host (which would route a credential to ``evil``). + self.assertEqual(_extract_extends_host("evil/org/.github", "github.com"), "evil") def test_host_qualified_nested_ref_extracts_host(self): self.assertEqual( - _extract_extends_host("gitlab.com/acme/dept-a/apm-policy"), + _extract_extends_host("gitlab.com/acme/dept-a/apm-policy", "gitlab.com"), "gitlab.com", ) @@ -1515,6 +1561,12 @@ def test_validate_allows_nested_same_host_reference(self): # Must NOT raise: this is the exact ref the subgroup feature enables. _validate_extends_host("gitlab.com", "acme/dept-a/apm-policy") + def test_validate_rejects_single_label_cross_host_on_github_leaf(self): + import apm_cli.policy.inheritance as _inh + + with self.assertRaisesRegex(_inh.PolicyInheritanceError, "cross-host"): + _validate_extends_host("github.com", "evil/org/.github") + def test_validate_still_rejects_cross_host_fqdn(self): import apm_cli.policy.inheritance as _inh From 7ae2ce674c85066e81b6e3ff704e5d10f27b6d50 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 09:33:44 -0300 Subject: [PATCH 04/14] fix(policy): address second Copilot review on GitLab subgroup discovery - architecture: add a registered static delegation guard (install-deployment-policy-remote-origin-owner) enforcing that only discovery.py reads `git remote get-url origin` and defines the remote-URL splitter/parsers; no other policy-tree module may re-read or re-parse the remote. Backs the single-owner claim the refactor makes. - docs: document that a bare nested `extends:` ref whose first segment contains a dot (group named e.g. `acme.tools`) must be host-qualified, since a dotted first segment is read as a host. - conformance: drop the req-pl-011 test that only re-asserted the GitLab provider's namespace order. Per req-pl-011 that order is implementation-defined, so it is not a normative conformance property; the unit suite already covers it. Regenerated CONFORMANCE.*. apm-spec-waiver: GitLab subgroup walk refines the existing req-pl-011 discovery provider (intra-provider search order is implementation-defined per spec 6.1.1); no new normative requirement, so no new anchor/manifest row/marker. Co-Authored-By: Claude Opus 4.8 --- CONFORMANCE.json | 3 +- CONFORMANCE.md | 2 +- .../src/content/docs/enterprise/apm-policy.md | 4 ++ .../install_policy_gitlab_and_bundle.py | 51 +++++++++++++++++++ .../checks/install_policy_intent.py | 7 +++ tests/spec_conformance/test_policy_reqs.py | 14 ----- 6 files changed, 64 insertions(+), 17 deletions(-) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index 38d69cd4c2..b2fbc74e4d 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -696,10 +696,9 @@ "keyword": "MUST", "section": "6.1.1", "status": "active", - "test_count": 3, + "test_count": 2, "tests": [ "tests/spec_conformance/test_policy_reqs.py::test_policy_gitlab_discovery_provider_is_a_distinct_convention", - "tests/spec_conformance/test_policy_reqs.py::test_policy_gitlab_discovery_walks_subgroups_closest_first", "tests/spec_conformance/test_policy_reqs.py::test_policy_provides_default_allow_list_shape" ] }, diff --git a/CONFORMANCE.md b/CONFORMANCE.md index 66cc6e48ff..a6c0ce79fd 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -97,7 +97,7 @@ Repository-coordinate segments are case-insensitive for `github.com`, GitHub Ent | [req-pl-008](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-008) | MUST | 6.3.1 | governance | active | 1 | - | | [req-pl-009](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-009) | MUST | 6.6 | governance | active | 1 | - | | [req-pl-010](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-010) | MUST | 6.2 | governance | active | 1 | - | -| [req-pl-011](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-011) | MUST | 6.1.1 | governance | active | 3 | - | +| [req-pl-011](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-011) | MUST | 6.1.1 | governance | active | 2 | - | | [req-pl-012](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-012) | MUST | 6.1.1 | governance | active | 1 | - | | [req-pl-013](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-013) | MUST | 6.8 | governance | active | 1 | - | | [req-pl-014](docs/src/content/docs/specs/openapm-v0.1.md#req-pl-014) | MUST | 6.8 | governance | active | 1 | - | diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index 5deaed7063..f101b93fe4 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -89,6 +89,10 @@ This lets an individual team publish its own `apm-policy` under its subgroup wit Composing policies across levels is opt-in via `extends:`. A team policy can extend an ancestor group's policy to inherit and tighten it, using either an explicit path (`extends: "acme/dept-a/apm-policy"` for the immediate parent group, or a deeper ancestor) or the `extends: "org"` shorthand, which on GitLab always resolves to the **top-level** group's policy (`/apm-policy`) -- not the nearest ancestor. Children can only tighten, never relax. +:::note[Group names containing a dot] +A bare nested `extends:` ref whose first segment contains a dot (e.g. a group named `acme.tools`) is read as a host, so `extends: "acme.tools/team/apm-policy"` is rejected as cross-host. Spell it host-qualified instead -- `extends: "gitlab.com/acme.tools/team/apm-policy"` (or `/...` on self-managed) -- so the leaf host is matched and stripped. +::: + Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. :::caution[Self-managed GitLab requires GITLAB_HOST or APM_GITLAB_HOSTS] diff --git a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py index da1b284f15..3ed19dbd85 100644 --- a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py +++ b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py @@ -43,6 +43,9 @@ RULE_WINNER_SELECTION = "install-deployment-dependency-winner-selection" +RULE_REMOTE_ORIGIN_OWNER = "install-deployment-policy-remote-origin-owner" + + def _count_text(lines: Sequence[tuple[int, str]], needle: str) -> int: """Return how many lines contain `needle` (``grep -Fc``).""" return sum(1 for _, text in lines if needle in text) @@ -113,6 +116,54 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... return tuple(findings) +_POLICY_DISCOVERY_OWNER = "src/apm_cli/policy/discovery.py" + + +_REMOTE_ORIGIN_ARGV = re.compile(r'"remote",\s*"get-url",\s*"origin"') + + +_REMOTE_PARSER_DEFS = re.compile( + r"^def (_remote_url_parts|_parse_remote_url|_git_remote_origin_url)\(" +) + + +def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation, ...]: + """Reading and parsing the project git remote for policy discovery has one owner. + + ``discovery.py`` is the sole reader of ``git remote get-url origin`` and the + sole home of the remote-URL splitter/parsers (``_remote_url_parts``, + ``_parse_remote_url``, ``_git_remote_origin_url``). This forbids any other + module in the policy tree from re-reading or re-parsing the remote, which + would reintroduce the double-read / divergent-parse the single-owner + refactor removed (#2753). + """ + rule_id = RULE_REMOTE_ORIGIN_OWNER + findings: list[Violation] = [] + findings.extend( + _banned( + provider, + rule_id=rule_id, + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY_OWNER,)), + pattern=_REMOTE_ORIGIN_ARGV, + message="Read the git remote origin only via discovery.py::_git_remote_origin_url", + configured=False, + respect_exempt=True, + ) + ) + findings.extend( + _banned( + provider, + rule_id=rule_id, + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY_OWNER,)), + pattern=_REMOTE_PARSER_DEFS, + message="Remote-URL split/parse owner is discovery.py; do not redefine these helpers", + configured=False, + respect_exempt=True, + ) + ) + return tuple(findings) + + _GITLAB_BRANCH_START = re.compile(r"^[ \t]*elif is_gitlab_hostname\(host\):") diff --git a/scripts/architecture_linter/checks/install_policy_intent.py b/scripts/architecture_linter/checks/install_policy_intent.py index 173d9f2024..de0577e689 100644 --- a/scripts/architecture_linter/checks/install_policy_intent.py +++ b/scripts/architecture_linter/checks/install_policy_intent.py @@ -28,12 +28,14 @@ RULE_GITLAB_ADAPTER, RULE_GITLAB_FACADE, RULE_LOCAL_BUNDLE_PREFLIGHT, + RULE_REMOTE_ORIGIN_OWNER, RULE_REQUIRE_HASHES, RULE_WINNER_SELECTION, check_dependency_winner_selection, check_gitlab_facade_orchestration, check_gitlab_policy_adapter, check_local_bundle_preflight, + check_policy_remote_origin_owner, check_require_hashes_enforcement, ) from scripts.architecture_linter.checks.install_policy_plugin_and_approval import ( @@ -101,6 +103,11 @@ "GitLab policy cache and transport remain in policy/_gitlab.py.", check_gitlab_facade_orchestration, ), + _semantic_rule( + RULE_REMOTE_ORIGIN_OWNER, + "Policy discovery reads and parses the git remote through one owner (discovery.py).", + check_policy_remote_origin_owner, + ), _semantic_rule( RULE_LOCAL_BUNDLE_PREFLIGHT, "Local bundle installs route policy through install_preflight.py.", diff --git a/tests/spec_conformance/test_policy_reqs.py b/tests/spec_conformance/test_policy_reqs.py index c8e8d74031..edf6765214 100644 --- a/tests/spec_conformance/test_policy_reqs.py +++ b/tests/spec_conformance/test_policy_reqs.py @@ -143,20 +143,6 @@ def test_policy_gitlab_discovery_provider_is_a_distinct_convention(): assert_spec_contains("gitlab-project-yml") -@pytest.mark.req("req-pl-011") -def test_policy_gitlab_discovery_walks_subgroups_closest_first(): - """req-pl-011: the GitLab provider probes the subgroup tree in a defined, - ordered manner (closest subgroup first, up to the top-level group), rather - than hard-coding a single top-level-only convention (#2753).""" - from apm_cli.policy.discovery import _gitlab_namespace_descending - - order = _gitlab_namespace_descending("https://gitlab.com/acme/dept-a/team-x/my-project.git") - - # Deepest subgroup first, then each shallower group, ending at the - # top-level group -- a deterministic, documented probe order. - assert order == ["acme/dept-a/team-x", "acme/dept-a", "acme"] - - @pytest.mark.req("req-pl-012") def test_policy_provides_default_deny_list_shape(): schema = load_schema("policy-v0.1.schema.json") From 0eebdf871af28f65c595f49368f3e71241bb8a9d Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:20:39 -0300 Subject: [PATCH 05/14] fix(policy): repair architecture mutation coverage + doc consistency - gitlab-facade-orchestration guard: the branch was renamed to `elif is_gitlab:` and the adapter call moved into `_gitlab_walk_candidate`, so the check scanned a stale `elif is_gitlab_hostname(host):` region and passed vacuously (its mutation case no longer produced a violation). Scope the orchestration scan to `_gitlab_walk_candidate`'s body instead; mutation case fires again. - policy-remote-origin-owner guard: give it teeth the mutation matrix can exercise -- require discovery.py to define all three canonical read/parse helpers -- and add its guard-less mutation case so the frozen matrix set-equality holds. - docs: reconcile policy-reference.md's `extends` table and the Discovery-vs-extends note with GitLab nested refs + closest-subgroup discovery; document that a host-qualified GitLab `extends:` ref must carry the exact `host:port` authority when the origin uses a port. Co-Authored-By: Claude Opus 4.8 --- .../src/content/docs/enterprise/apm-policy.md | 4 +- .../docs/enterprise/policy-reference.md | 5 ++- .../install_policy_gitlab_and_bundle.py | 45 +++++++++++++------ ...st_architecture_semantic_rule_mutations.py | 7 +++ 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index f101b93fe4..28f5ae4d01 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -89,8 +89,8 @@ This lets an individual team publish its own `apm-policy` under its subgroup wit Composing policies across levels is opt-in via `extends:`. A team policy can extend an ancestor group's policy to inherit and tighten it, using either an explicit path (`extends: "acme/dept-a/apm-policy"` for the immediate parent group, or a deeper ancestor) or the `extends: "org"` shorthand, which on GitLab always resolves to the **top-level** group's policy (`/apm-policy`) -- not the nearest ancestor. Children can only tighten, never relax. -:::note[Group names containing a dot] -A bare nested `extends:` ref whose first segment contains a dot (e.g. a group named `acme.tools`) is read as a host, so `extends: "acme.tools/team/apm-policy"` is rejected as cross-host. Spell it host-qualified instead -- `extends: "gitlab.com/acme.tools/team/apm-policy"` (or `/...` on self-managed) -- so the leaf host is matched and stripped. +:::note[Host-qualified `extends:` on GitLab] +A bare nested `extends:` ref whose first segment contains a dot (e.g. a group named `acme.tools`) is read as a host, so `extends: "acme.tools/team/apm-policy"` is rejected as cross-host. Spell it host-qualified instead -- `extends: "gitlab.com/acme.tools/team/apm-policy"` (or `/...` on self-managed) -- so the leaf host is matched and stripped. When the project's remote uses an explicit port, the host-qualified ref must carry the exact `host:port` authority (e.g. `gitlab.example.com:8443/acme/team/apm-policy`); `GITLAB_HOST` is only the hostname, so a ref without the matching port is rejected. ::: Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. diff --git a/docs/src/content/docs/enterprise/policy-reference.md b/docs/src/content/docs/enterprise/policy-reference.md index b6df0a967a..26e10ab22b 100644 --- a/docs/src/content/docs/enterprise/policy-reference.md +++ b/docs/src/content/docs/enterprise/policy-reference.md @@ -100,8 +100,9 @@ Inherit from a parent policy. See [Inheritance](#inheritance). | Value | Source | |-------|--------| -| `org` | Parent org's `.github-private/apm-policy.yml` (falls back to `.github`) | +| `org` | Parent org's `.github-private/apm-policy.yml` (falls back to `.github`); on GitLab, the top-level group's `apm-policy` | | `owner/repo` | Cross-org policy from a specific repository | +| `namespace/.../repo` | On GitLab, an ancestor subgroup's policy, e.g. `acme/dept-a/apm-policy` (host-qualify a namespace whose first segment contains a dot) | | `https://...` | Direct URL to a policy file | ### `fetch_failure` @@ -475,7 +476,7 @@ There are 21 policy checks. ## Inheritance :::note[Discovery vs. `extends:` -- two different concepts] -APM auto-discovers exactly **one** policy file: `/.github/apm-policy.yml`, derived from the project's git remote. There is no automatic per-repo or per-enterprise discovery. `extends:` is what composes policies **inside** that one discovered file -- it lets the discovered policy pull in a parent (and that parent's parent, up to `MAX_CHAIN_DEPTH=5`) so you can model an enterprise -> org -> team chain through composition. Most teams who say "3 levels (repo, org, enterprise)" actually want `extends:`, not more discovery sites. +APM auto-discovers exactly **one** policy file per project, derived from the git remote: on GitHub `/.github/apm-policy.yml`, and on GitLab the **closest** `apm-policy` walking up the subgroup tree to the top-level group (see [Policy Files](./apm-policy/#where-it-lives)). There is no automatic per-repo or per-enterprise discovery. `extends:` is what composes policies **inside** that one discovered file -- it lets the discovered policy pull in a parent (and that parent's parent, up to `MAX_CHAIN_DEPTH=5`) so you can model an enterprise -> org -> team chain through composition. Most teams who say "3 levels (repo, org, enterprise)" actually want `extends:`, not more discovery sites. ::: Policies can inherit from a parent using `extends`. This enables a three-level chain: diff --git a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py index 3ed19dbd85..73a6e39145 100644 --- a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py +++ b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py @@ -13,7 +13,6 @@ from scripts.architecture_linter.checks.install_policy_shared import ( _APM_RESOLVER, - _ELSE_TERMINATOR, _POLICY_DISCOVERY, _banned, _configured, @@ -116,9 +115,6 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... return tuple(findings) -_POLICY_DISCOVERY_OWNER = "src/apm_cli/policy/discovery.py" - - _REMOTE_ORIGIN_ARGV = re.compile(r'"remote",\s*"get-url",\s*"origin"') @@ -127,23 +123,40 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... ) +_REMOTE_PARSER_DEF_COUNT = 3 + + def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation, ...]: """Reading and parsing the project git remote for policy discovery has one owner. ``discovery.py`` is the sole reader of ``git remote get-url origin`` and the sole home of the remote-URL splitter/parsers (``_remote_url_parts``, - ``_parse_remote_url``, ``_git_remote_origin_url``). This forbids any other - module in the policy tree from re-reading or re-parsing the remote, which - would reintroduce the double-read / divergent-parse the single-owner - refactor removed (#2753). + ``_parse_remote_url``, ``_git_remote_origin_url``). The owner MUST define all + three helpers, and no other module in the policy tree may re-read or re-parse + the remote -- either would reintroduce the double-read / divergent-parse the + single-owner refactor removed (#2753). """ rule_id = RULE_REMOTE_ORIGIN_OWNER + owner, owner_fail = _configured(provider, _POLICY_DISCOVERY, rule_id) + if owner_fail: + return tuple(owner_fail) findings: list[Violation] = [] + definitions = _count_re(owner, _REMOTE_PARSER_DEFS) + if definitions != _REMOTE_PARSER_DEF_COUNT: + findings.append( + _report( + rule_id, + _POLICY_DISCOVERY, + "Policy discovery must define exactly " + f"{_REMOTE_PARSER_DEF_COUNT} canonical git-remote read/parse helpers " + f"(found {definitions})", + ) + ) findings.extend( _banned( provider, rule_id=rule_id, - paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY_OWNER,)), + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY,)), pattern=_REMOTE_ORIGIN_ARGV, message="Read the git remote origin only via discovery.py::_git_remote_origin_url", configured=False, @@ -154,7 +167,7 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation _banned( provider, rule_id=rule_id, - paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY_OWNER,)), + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY,)), pattern=_REMOTE_PARSER_DEFS, message="Remote-URL split/parse owner is discovery.py; do not redefine these helpers", configured=False, @@ -164,7 +177,13 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation return tuple(findings) -_GITLAB_BRANCH_START = re.compile(r"^[ \t]*elif is_gitlab_hostname\(host\):") +# The facade delegation now lives in ``_gitlab_walk_candidate`` (the GitLab +# branch of ``_auto_discover`` calls it; see #2753). Scope the orchestration +# scan to that helper's body -- from its ``def`` to the next top-level ``def``. +_GITLAB_WALK_START = re.compile(r"^def _gitlab_walk_candidate\(") + + +_TOP_LEVEL_DEF = re.compile(r"^def ") _NON_WHITESPACE = re.compile(r"[^\s]") @@ -185,8 +204,8 @@ def check_gitlab_facade_orchestration(provider: FactsProvider) -> tuple[Violatio branch = _indent_scoped_branch( lines, - start=_GITLAB_BRANCH_START, - terminator=_ELSE_TERMINATOR, + start=_GITLAB_WALK_START, + terminator=_TOP_LEVEL_DEF, probe=_NON_WHITESPACE, include_start=False, restart_skips=True, diff --git a/tests/integration/test_architecture_semantic_rule_mutations.py b/tests/integration/test_architecture_semantic_rule_mutations.py index 5f6661d20c..7d36b49c18 100644 --- a/tests/integration/test_architecture_semantic_rule_mutations.py +++ b/tests/integration/test_architecture_semantic_rule_mutations.py @@ -215,6 +215,13 @@ class MutationCase: new="def plugin_bin_deployable_impl(", intent="The exec gate stops defining the one plugin bin eligibility owner.", ), + MutationCase( + rule_id="install-deployment-policy-remote-origin-owner", + path="src/apm_cli/policy/discovery.py", + old="def _git_remote_origin_url(", + new="def _git_remote_origin_url_impl(", + intent="Policy discovery drops its canonical git-remote origin reader.", + ), MutationCase( rule_id="install-deployment-ref-recheck-ownership", path="src/apm_cli/install/phases/resolve.py", From 7bcbc18545f56fad41502b7bbd037ca84d21459a Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:08:09 -0300 Subject: [PATCH 06/14] test(arch): register remote-origin guard in frozen rule inventory The frozen semantic-contract inventory in test_architecture_runner.py (_EXPECTED_RULE_ID_TEXT) must list every registered rule id; add install-deployment-policy-remote-origin-owner so the set-equality holds. Co-Authored-By: Claude Opus 4.8 --- tests/unit/scripts/test_architecture_runner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/scripts/test_architecture_runner.py b/tests/unit/scripts/test_architecture_runner.py index 59f7b4ed61..9ac28393cb 100644 --- a/tests/unit/scripts/test_architecture_runner.py +++ b/tests/unit/scripts/test_architecture_runner.py @@ -643,6 +643,7 @@ def exiting_import( install-deployment-outcome install-deployment-package-target-authorization install-deployment-plugin-bin-eligibility +install-deployment-policy-remote-origin-owner install-deployment-primitive-classification install-deployment-prospective-dry-run-plan install-deployment-provenance-state From fcacaa26ede0964c5ef347477cbca8031092a653 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:28:29 -0300 Subject: [PATCH 07/14] fix(arch): bind remote-origin guard to the owner registry Copilot review: a canonical-owner rule should be an owner-guard bound to an owner-registry record, not a guard-less semantic rule, so the registry can validate it bidirectionally and enforce single execution. - Register `install-deployment-policy-remote-origin-owner` with `guard_ids=(,)` (a `Rule(...)`, not `_semantic_rule`), making it a known owner-guard that executes exactly once. - Reference it from discovery.py's existing owner record (`cached-policy-shape` in contracts-tooling.json); discovery.py can hold only one owner record, and a guard's shard is independent of its id prefix, so the remote-origin decision joins that record's guard list. - Move its mutation case from the guard-less matrix to the owner-guard matrix (test_architecture_owner_rule_mutations.py); both matrices' set-equality and the frozen inventory now hold. Co-Authored-By: Claude Opus 4.8 --- .apm/architecture/owners/contracts-tooling.json | 9 ++++++--- .../checks/install_policy_intent.py | 12 ++++++++---- .../test_architecture_owner_rule_mutations.py | 8 ++++++++ .../test_architecture_semantic_rule_mutations.py | 7 ------- 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/.apm/architecture/owners/contracts-tooling.json b/.apm/architecture/owners/contracts-tooling.json index 31ef286093..19009f925b 100644 --- a/.apm/architecture/owners/contracts-tooling.json +++ b/.apm/architecture/owners/contracts-tooling.json @@ -21,10 +21,13 @@ }, { "id": "cached-policy-shape", - "decision": "Cached policy shape", - "owner": "policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY)", + "decision": "Cached policy shape; git-remote reading and URL parsing for org-policy discovery", + "owner": "policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY; _git_remote_origin_url; _remote_url_parts; _parse_remote_url)", "selectors": ["src/apm_cli/policy/discovery.py"], - "guards": ["contracts-tooling-cached-policy-shape"] + "guards": [ + "contracts-tooling-cached-policy-shape", + "install-deployment-policy-remote-origin-owner" + ] }, { "id": "policy-content-hash", diff --git a/scripts/architecture_linter/checks/install_policy_intent.py b/scripts/architecture_linter/checks/install_policy_intent.py index de0577e689..707e6f795b 100644 --- a/scripts/architecture_linter/checks/install_policy_intent.py +++ b/scripts/architecture_linter/checks/install_policy_intent.py @@ -103,10 +103,14 @@ "GitLab policy cache and transport remain in policy/_gitlab.py.", check_gitlab_facade_orchestration, ), - _semantic_rule( - RULE_REMOTE_ORIGIN_OWNER, - "Policy discovery reads and parses the git remote through one owner (discovery.py).", - check_policy_remote_origin_owner, + Rule( + id=RULE_REMOTE_ORIGIN_OWNER, + group=GROUP, + guard_ids=(RULE_REMOTE_ORIGIN_OWNER,), + description=( + "Policy discovery reads and parses the git remote through one owner (discovery.py)." + ), + check=check_policy_remote_origin_owner, ), _semantic_rule( RULE_LOCAL_BUNDLE_PREFLIGHT, diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 036f70b41d..e0112405eb 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -397,6 +397,14 @@ class MutationCase: new="def resolve_effective_package_targets_disabled(", intent="Effective package-target authorization loses its single resolver.", ), + MutationCase( + guard_id="install-deployment-policy-remote-origin-owner", + rule_id="install-deployment-policy-remote-origin-owner", + path="src/apm_cli/policy/discovery.py", + old="def _git_remote_origin_url(", + new="def _git_remote_origin_url_impl(", + intent="Policy discovery drops its canonical git-remote origin reader.", + ), MutationCase( guard_id="install-deployment-primitive-classification", rule_id="install-deployment-primitive-classification", diff --git a/tests/integration/test_architecture_semantic_rule_mutations.py b/tests/integration/test_architecture_semantic_rule_mutations.py index 7d36b49c18..5f6661d20c 100644 --- a/tests/integration/test_architecture_semantic_rule_mutations.py +++ b/tests/integration/test_architecture_semantic_rule_mutations.py @@ -215,13 +215,6 @@ class MutationCase: new="def plugin_bin_deployable_impl(", intent="The exec gate stops defining the one plugin bin eligibility owner.", ), - MutationCase( - rule_id="install-deployment-policy-remote-origin-owner", - path="src/apm_cli/policy/discovery.py", - old="def _git_remote_origin_url(", - new="def _git_remote_origin_url_impl(", - intent="Policy discovery drops its canonical git-remote origin reader.", - ), MutationCase( rule_id="install-deployment-ref-recheck-ownership", path="src/apm_cli/install/phases/resolve.py", From 4de20be38ff3d92e76aa9478856a78a2be342fd4 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:03:55 -0300 Subject: [PATCH 08/14] fix(policy): fail closed on concealed GitLab policy; tighten origin guard Copilot review: - Concealed-404 fail-open (discovery.py): GitLab returns 404 both for a missing project and for a private one the token cannot read, so the subgroup walk could silently apply a weaker ancestor policy over a closer team policy the token was denied. Before applying an ancestor over skipped closer levels, verify via authenticated Git (`_gitlab.first_concealed_closer_policy`) that no skipped closer `apm-policy` project exists; if one is confirmed, fail closed. The residual case (token blind to both REST and Git) is indistinguishable from genuinely absent and still ascends -- documented. - Origin-read guard: the guard only required the three helper names to exist, so a second `git remote get-url origin` elsewhere in discovery.py would pass. Add an exact-count clause (one origin-read argv in the owner) and switch the owner-guard mutation to introduce a duplicate read, so the single-reader invariant is enforced and proven. Co-Authored-By: Claude Opus 4.8 --- .../src/content/docs/enterprise/apm-policy.md | 4 ++ .../install_policy_gitlab_and_bundle.py | 14 +++++ src/apm_cli/policy/_gitlab.py | 25 ++++++++ src/apm_cli/policy/discovery.py | 32 +++++++++++ .../test_architecture_owner_rule_mutations.py | 9 ++- tests/unit/policy/test_discovery.py | 57 ++++++++++++++++++- 6 files changed, 137 insertions(+), 4 deletions(-) diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index 28f5ae4d01..de379ec30a 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -95,6 +95,10 @@ A bare nested `extends:` ref whose first segment contains a dot (e.g. a group na Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. +:::caution[Concealed private policy projects] +GitLab returns HTTP 404 both for a missing project and for a private one the token cannot read, so a closer `apm-policy` a team publishes could otherwise be silently skipped in favour of a weaker ancestor. Before applying an ancestor policy over a skipped closer level, APM confirms via authenticated Git that the skipped closer `apm-policy` project does not exist; if Git confirms it *does* exist, discovery fails closed rather than downgrade. The residual case where the token can read neither the policy file nor the project over Git is indistinguishable from a genuinely absent project and still ascends -- grant the CI token read access to every `apm-policy` project it should honour. +::: + :::caution[Self-managed GitLab requires GITLAB_HOST or APM_GITLAB_HOSTS] An arbitrary FQDN is never auto-classified as GitLab -- the same domain shape could be Bitbucket, Gitea, or a plain git server. `gitlab.com` is recognised automatically, but a self-managed instance (e.g. `gitlab.example.com`) is only recognised once you set `GITLAB_HOST=gitlab.example.com` (or `APM_GITLAB_HOSTS` for more than one instance). Without it, APM falls through to the GitHub-style cascade above, which is invalid on GitLab and behaves exactly like the unfixed discovery this section describes. This mirrors `GITHUB_HOST` for GitHub Enterprise Server and `ADO_HOST` for on-prem Azure DevOps Server -- see [Environment Variables](../../reference/environment-variables/). ::: diff --git a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py index 73a6e39145..4d285e2074 100644 --- a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py +++ b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py @@ -126,6 +126,9 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... _REMOTE_PARSER_DEF_COUNT = 3 +_REMOTE_ORIGIN_READ_COUNT = 1 + + def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation, ...]: """Reading and parsing the project git remote for policy discovery has one owner. @@ -152,6 +155,17 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation f"(found {definitions})", ) ) + origin_reads = _count_re(owner, _REMOTE_ORIGIN_ARGV) + if origin_reads != _REMOTE_ORIGIN_READ_COUNT: + findings.append( + _report( + rule_id, + _POLICY_DISCOVERY, + "Policy discovery must read the git remote origin exactly " + f"{_REMOTE_ORIGIN_READ_COUNT} time via _git_remote_origin_url " + f"(found {origin_reads} origin-read argv occurrences)", + ) + ) findings.extend( _banned( provider, diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index 088860a68c..4b7c35c16f 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -110,6 +110,31 @@ def _probe(_token: str | None, git_env: dict[str, str]) -> bool | None: return None +def first_concealed_closer_policy( + skipped_namespaces: list[str], + repo: str, + *, + host: str, + port: int | None, +) -> str | None: + """Return the closest skipped namespace whose ``apm-policy`` project exists. + + During the subgroup walk (see :func:`discovery._gitlab_walk_candidate`) an + ``absent`` level is ambiguous -- GitLab returns 404 both for a missing + project and for a private one the token cannot read. Before an ancestor + policy is applied over the skipped closer levels, this confirms via + authenticated Git whether any skipped closer ``apm-policy`` project actually + exists. It returns the closest such namespace (its ``absent`` was a + concealed 404, so the caller must fail closed), or ``None`` when no skipped + level's project can be confirmed. ``skipped_namespaces`` is ordered + closest-first. + """ + for namespace in skipped_namespaces: + if _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) is True: + return namespace + return None + + def _fetch_gitlab_chain_parent( parent_ref: str, *, diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 418440b0b9..0f68a08e43 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1064,8 +1064,16 @@ def _gitlab_walk_candidate( outcome wins; an ``absent`` level continues to the next-shallower group. When every level is absent the last ``absent`` result is returned so the caller's candidate cascade can proceed unchanged. + + GitLab returns 404 both for a missing project and for a private project the + token cannot read, so an ``absent`` is ambiguous. Before applying an + ancestor policy over one or more skipped closer levels, verify (via + authenticated Git) that no skipped closer ``apm-policy`` project actually + exists; if one does, its policy was concealed and we fail closed rather than + silently apply the weaker ancestor. """ result = PolicyFetchResult(error=None, outcome="absent") + skipped: list[str] = [] for namespace in namespaces: result = _gitlab._fetch_from_gitlab_repo( org=namespace, @@ -1078,7 +1086,14 @@ def _gitlab_walk_candidate( cache_only=cache_only, ) if result.outcome != "absent": + if skipped and result.outcome in {"found", "empty"}: + concealed = _gitlab.first_concealed_closer_policy( + skipped, candidate_repo, host=host, port=port + ) + if concealed is not None: + return _gitlab_concealed_closer_result(concealed, candidate_repo, host, port) return result + skipped.append(namespace) logger.debug( "GitLab policy absent at %s/%s; trying parent group", namespace, @@ -1087,6 +1102,23 @@ def _gitlab_walk_candidate( return result +def _gitlab_concealed_closer_result( + namespace: str, repo: str, host: str, port: int | None +) -> PolicyFetchResult: + """Fail-closed result when a skipped closer GitLab policy project exists.""" + host_label = f"{host}:{port}" if port is not None else host + return PolicyFetchResult( + source=f"org:{host_label}/{namespace}/{repo}", + error=( + f"A closer GitLab policy project {namespace}/{repo} exists but its " + "apm-policy.yml could not be read (GitLab returns 404 for a private " + "project the token cannot access); refusing to silently apply a weaker " + "ancestor policy. Grant the token read access to that project, or remove it." + ), + outcome="cache_miss_fetch_fail", + ) + + def _remote_url_parts(url: str) -> tuple[str, list[str]] | None: """Split a git remote URL into ``(host, path_segments)``. diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index e0112405eb..f2362c52d3 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -401,9 +401,12 @@ class MutationCase: guard_id="install-deployment-policy-remote-origin-owner", rule_id="install-deployment-policy-remote-origin-owner", path="src/apm_cli/policy/discovery.py", - old="def _git_remote_origin_url(", - new="def _git_remote_origin_url_impl(", - intent="Policy discovery drops its canonical git-remote origin reader.", + old='[get_git_executable(), "remote", "get-url", "origin"],', + new=( + '[get_git_executable(), "remote", "get-url", "origin"],\n' + ' [get_git_executable(), "remote", "get-url", "origin"],' + ), + intent="A second git-remote origin read appears outside the single canonical reader.", ), MutationCase( guard_id="install-deployment-primitive-classification", diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index c151db3783..403421fe6a 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -21,6 +21,7 @@ _fetch_from_gitlab_repo, _fetch_gitlab_contents, _gitlab_project_state_via_git, + first_concealed_closer_policy, ) from apm_cli.policy.discovery import ( CACHE_SCHEMA_VERSION, @@ -229,6 +230,27 @@ def test_no_namespace_segment_returns_none(self): self.assertIsNone(_gitlab_namespace_descending("https://gitlab.com/solo")) +class TestFirstConcealedCloserPolicy(unittest.TestCase): + """first_concealed_closer_policy verifies skipped closer projects (#2753).""" + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_returns_closest_namespace_git_confirms_exists(self, mock_state): + # team-x exists (concealed 404); dept-a not confirmed. + mock_state.side_effect = lambda **kw: True if kw["org"] == "acme/dept-a/team-x" else None + result = first_concealed_closer_policy( + ["acme/dept-a/team-x", "acme/dept-a"], "apm-policy", host="gitlab.com", port=None + ) + self.assertEqual(result, "acme/dept-a/team-x") + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_returns_none_when_no_skipped_project_confirmed(self, mock_state): + mock_state.return_value = None # git cannot establish any project's state + result = first_concealed_closer_policy( + ["acme/dept-a/team-x", "acme/dept-a"], "apm-policy", host="gitlab.com", port=None + ) + self.assertIsNone(result) + + class TestExtractOrgFromGitRemote(unittest.TestCase): """Test _extract_org_from_git_remote with mocked subprocess.""" @@ -1205,11 +1227,12 @@ def test_gitlab_subgroup_closest_wins(self, mock_extract, mock_ns, mock_gitlab_f mock_gitlab_fetch.assert_called_once() self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "acme/dept-a/team-x") + @patch("apm_cli.policy._gitlab.first_concealed_closer_policy", return_value=None) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") def test_gitlab_subgroup_absent_ascends_to_parent( - self, mock_extract, mock_ns, mock_gitlab_fetch + self, mock_extract, mock_ns, mock_gitlab_fetch, mock_concealed ): """Absent at the deepest levels -> ascend until a policy is found.""" mock_extract.return_value = ("acme", "gitlab.com", None) @@ -1231,6 +1254,38 @@ def test_gitlab_subgroup_absent_ascends_to_parent( self.assertEqual(mock_gitlab_fetch.call_count, 3) probed = [c.kwargs["org"] for c in mock_gitlab_fetch.call_args_list] self.assertEqual(probed, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) + # Skipped closer levels were verified genuinely empty before ascending. + mock_concealed.assert_called_once() + self.assertEqual(mock_concealed.call_args.args[0], ["acme/dept-a/team-x", "acme/dept-a"]) + + @patch("apm_cli.policy._gitlab.first_concealed_closer_policy") + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_concealed_closer_policy_fails_closed( + self, mock_extract, mock_ns, mock_gitlab_fetch, mock_concealed + ): + """A closer apm-policy project confirmed to exist (concealed 404) fails + closed instead of silently applying the weaker ancestor (#2753 review).""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.side_effect = [ + PolicyFetchResult(outcome="absent"), # team-x: concealed 404 + PolicyFetchResult( + policy=ApmPolicy(), + source="org:gitlab.com/acme/dept-a/apm-policy", + outcome="found", + ), # dept-a: an ancestor policy + ] + # Git confirms the skipped closer project (team-x) actually exists. + mock_concealed.return_value = "acme/dept-a/team-x" + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertFalse(result.found) + self.assertEqual(result.outcome, "cache_miss_fetch_fail") + self.assertIn("acme/dept-a/team-x/apm-policy", result.error) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") From 47417b29db14c71bf52e71f43653bdb2447f03c6 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:39:42 -0300 Subject: [PATCH 09/14] refactor(policy): extract git-remote parsing into policy/_remote.py Pre-existing verification sweep caught the CI file-length guard: this PR's additions pushed discovery.py from 2068 to 2252 lines, over the 2100-line ratchet. Extract the cohesive git-remote identity family -- _git_remote_origin_url, _remote_url_parts, _parse_remote_url, _extract_org_host_port_from_git_remote, _extract_org_from_git_remote -- into a new policy/_remote.py (discovery.py back to ~2091, re-exports the names so callers/tests are unchanged). This also makes the remote-origin owner-guard cleaner: _remote.py is a dedicated, single-decision module, so the guard binds to its own owner-registry record (selector free) instead of piggy-backing on discovery.py's cached-policy-shape record. Guard, mutation case, and the tests that patch the moved internals (subprocess/urlparse/_parse_remote_url) are re-pointed at _remote. Co-Authored-By: Claude Opus 4.8 --- .../owners/contracts-tooling.json | 9 +- .../owners/install-deployment.json | 7 + .../install_policy_gitlab_and_bundle.py | 25 ++- src/apm_cli/policy/_remote.py | 189 ++++++++++++++++++ src/apm_cli/policy/discovery.py | 177 +--------------- .../test_architecture_owner_rule_mutations.py | 2 +- tests/unit/policy/test_discovery.py | 30 +-- tests/unit/policy/test_discovery_phase3w4.py | 2 +- .../test_discovery_policy_resolution.py | 2 +- 9 files changed, 238 insertions(+), 205 deletions(-) create mode 100644 src/apm_cli/policy/_remote.py diff --git a/.apm/architecture/owners/contracts-tooling.json b/.apm/architecture/owners/contracts-tooling.json index 19009f925b..31ef286093 100644 --- a/.apm/architecture/owners/contracts-tooling.json +++ b/.apm/architecture/owners/contracts-tooling.json @@ -21,13 +21,10 @@ }, { "id": "cached-policy-shape", - "decision": "Cached policy shape; git-remote reading and URL parsing for org-policy discovery", - "owner": "policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY; _git_remote_origin_url; _remote_url_parts; _parse_remote_url)", + "decision": "Cached policy shape", + "owner": "policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY)", "selectors": ["src/apm_cli/policy/discovery.py"], - "guards": [ - "contracts-tooling-cached-policy-shape", - "install-deployment-policy-remote-origin-owner" - ] + "guards": ["contracts-tooling-cached-policy-shape"] }, { "id": "policy-content-hash", diff --git a/.apm/architecture/owners/install-deployment.json b/.apm/architecture/owners/install-deployment.json index a0872223aa..8b40e71348 100644 --- a/.apm/architecture/owners/install-deployment.json +++ b/.apm/architecture/owners/install-deployment.json @@ -157,6 +157,13 @@ "owner": "models/dependency/selection.py (via DependencyReference)", "selectors": ["src/apm_cli/models/dependency/selection.py"], "guards": ["install-deployment-uninstall-selection"] + }, + { + "id": "policy-remote-origin-reader", + "decision": "Reading and parsing the project git remote for org-policy discovery", + "owner": "policy/_remote.py (_git_remote_origin_url, _remote_url_parts, _parse_remote_url, _extract_org_host_port_from_git_remote)", + "selectors": ["src/apm_cli/policy/_remote.py"], + "guards": ["install-deployment-policy-remote-origin-owner"] } ] } diff --git a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py index 4d285e2074..06bf99a1af 100644 --- a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py +++ b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py @@ -129,18 +129,21 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... _REMOTE_ORIGIN_READ_COUNT = 1 +_REMOTE_MODULE = "src/apm_cli/policy/_remote.py" + + def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation, ...]: """Reading and parsing the project git remote for policy discovery has one owner. - ``discovery.py`` is the sole reader of ``git remote get-url origin`` and the - sole home of the remote-URL splitter/parsers (``_remote_url_parts``, + ``policy/_remote.py`` is the sole reader of ``git remote get-url origin`` and + the sole home of the remote-URL splitter/parsers (``_remote_url_parts``, ``_parse_remote_url``, ``_git_remote_origin_url``). The owner MUST define all three helpers, and no other module in the policy tree may re-read or re-parse the remote -- either would reintroduce the double-read / divergent-parse the single-owner refactor removed (#2753). """ rule_id = RULE_REMOTE_ORIGIN_OWNER - owner, owner_fail = _configured(provider, _POLICY_DISCOVERY, rule_id) + owner, owner_fail = _configured(provider, _REMOTE_MODULE, rule_id) if owner_fail: return tuple(owner_fail) findings: list[Violation] = [] @@ -149,8 +152,8 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation findings.append( _report( rule_id, - _POLICY_DISCOVERY, - "Policy discovery must define exactly " + _REMOTE_MODULE, + "policy/_remote.py must define exactly " f"{_REMOTE_PARSER_DEF_COUNT} canonical git-remote read/parse helpers " f"(found {definitions})", ) @@ -160,8 +163,8 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation findings.append( _report( rule_id, - _POLICY_DISCOVERY, - "Policy discovery must read the git remote origin exactly " + _REMOTE_MODULE, + "policy/_remote.py must read the git remote origin exactly " f"{_REMOTE_ORIGIN_READ_COUNT} time via _git_remote_origin_url " f"(found {origin_reads} origin-read argv occurrences)", ) @@ -170,9 +173,9 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation _banned( provider, rule_id=rule_id, - paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY,)), + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_REMOTE_MODULE,)), pattern=_REMOTE_ORIGIN_ARGV, - message="Read the git remote origin only via discovery.py::_git_remote_origin_url", + message="Read the git remote origin only via _remote.py::_git_remote_origin_url", configured=False, respect_exempt=True, ) @@ -181,9 +184,9 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation _banned( provider, rule_id=rule_id, - paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_POLICY_DISCOVERY,)), + paths=_tree_python_paths(provider, _POLICY_TREE, excluded=(_REMOTE_MODULE,)), pattern=_REMOTE_PARSER_DEFS, - message="Remote-URL split/parse owner is discovery.py; do not redefine these helpers", + message="Remote-URL split/parse owner is _remote.py; do not redefine these helpers", configured=False, respect_exempt=True, ) diff --git a/src/apm_cli/policy/_remote.py b/src/apm_cli/policy/_remote.py new file mode 100644 index 0000000000..9a3dadc08f --- /dev/null +++ b/src/apm_cli/policy/_remote.py @@ -0,0 +1,189 @@ +"""Git remote-URL identity parsing for org-policy discovery. + +Split out of ``discovery.py`` (which grew past the file-length ratchet): the +canonical splitter :func:`_remote_url_parts` and the host-specific +:func:`_parse_remote_url` live here. ``discovery.py`` re-exports both and reads +the ``origin`` remote via its own ``_git_remote_origin_url``; the architecture +guard ``install-deployment-policy-remote-origin-owner`` keeps these the single +owners of git-remote reading and URL parsing. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from urllib.parse import urlparse + +from ..cache.url_normalize import SCP_LIKE_RE +from ..utils.git_env import get_git_executable +from ..utils.github_host import ( + is_azure_devops_hostname, + is_visualstudio_legacy_hostname, + parse_ado_repo_url, +) + + +class _Unset: + """Sentinel type: an argument was not provided (distinct from ``None``).""" + + +_UNSET = _Unset() + + +def _git_remote_origin_url(project_root: Path) -> str | None: + """Return the ``origin`` remote URL, or ``None`` when unavailable. + + Canonical reader of the project's ``origin`` remote, shared by + :func:`_extract_org_host_port_from_git_remote` and GitLab subgroup + discovery so the ``git remote get-url`` subprocess lives in one place. + """ + try: + result = subprocess.run( + [get_git_executable(), "remote", "get-url", "origin"], + capture_output=True, + text=True, + encoding="utf-8", + cwd=project_root, + timeout=5, + ) + except (subprocess.TimeoutExpired, FileNotFoundError): + return None + if result.returncode != 0: + return None + return result.stdout.strip() + + +def _extract_org_host_port_from_git_remote( + project_root: Path, + *, + remote_url: str | None | _Unset = _UNSET, +) -> tuple[str, str, int | None] | None: + """Extract ``(org, host, port)`` from git remote origin. + + ``remote_url`` lets a caller that already read ``origin`` (e.g. + ``discovery._auto_discover``) pass it in so the ``git remote get-url`` + subprocess runs exactly once per discovery. When left unset the origin is + read here; an explicit ``None`` means "no remote" and is not re-read. + """ + if isinstance(remote_url, _Unset): + remote_url = _git_remote_origin_url(project_root) + if not remote_url: + return None + try: + parsed_identity = _parse_remote_url(remote_url) + except ValueError: + return None + if parsed_identity is None: + return None + port = None + if "://" in remote_url: + try: + port = urlparse(remote_url).port + except ValueError: + return None + return parsed_identity[0], parsed_identity[1], port + + +def _extract_org_from_git_remote( + project_root: Path, +) -> tuple[str, str] | None: + """Extract (org, host) from git remote origin URL. + + Handles: + - https://github.com/contoso/my-project.git -> ("contoso", "github.com") + - git@github.com:contoso/my-project.git -> ("contoso", "github.com") + - https://github.example.com/contoso/my-project.git -> ("contoso", "github.example.com") + """ + identity = _extract_org_host_port_from_git_remote(project_root) + return (identity[0], identity[1]) if identity is not None else None + + +def _remote_url_parts(url: str) -> tuple[str, list[str]] | None: + """Split a git remote URL into ``(host, path_segments)``. + + Canonical splitter shared by :func:`_parse_remote_url` (which applies + host-specific org interpretation on top) and GitLab subgroup discovery + (:func:`discovery._gitlab_namespace_descending`). Handles SCP-like SSH URLs + with any username (not just ``git@``) and ``scheme://`` URLs. Path segments + are cleaned of empty parts and the trailing ``.git`` suffix; no host-specific + interpretation (ADO ``v3/`` prefix, visualstudio subdomain, ...) is applied + here -- that stays the caller's responsibility. + + Returns ``None`` when the URL cannot be split. + """ + if not url: + return None + + # SCP-like SSH: @: -- any user, not just `git`. + # Closes #1159 for non-`git` SSH users (EMU, custom GHE accounts). + scp_match = SCP_LIKE_RE.match(url) + if scp_match: + host = scp_match.group("host") + path_part = scp_match.group("path") + segments = [p for p in path_part.rstrip("/").removesuffix(".git").split("/") if p] + if not host or not segments: + return None + return (host, segments) + + # HTTPS: https://github.com/owner/repo.git + if "://" in url: + try: + parsed = urlparse(url) + except Exception: + # urlparse only raises ValueError in practice, but the legacy + # parser swallowed any exception here; preserve that. The + # ``/tfs/`` ValueError comes from parse_ado_repo_url (in + # _parse_remote_url), never from urlparse, so nothing to re-raise. + return None + host = parsed.hostname or "" + segments = [ + p for p in parsed.path.strip("/").removesuffix(".git").rstrip("/").split("/") if p + ] + if not host or not segments: + return None + return (host, segments) + + return None + + +def _parse_remote_url(url: str) -> tuple[str, str] | None: + """Parse a git remote URL into (org, host). + + Accepts SCP-style SSH URLs with any username (not just ``git@``), so + EMU/GHE deployments that use a non-``git`` SSH user + (e.g. ``enterprise-user@ghe.corp.com:org/repo.git``) parse correctly. + Also handles Azure DevOps SSH URLs which carry an extra ``v3/`` + path prefix (``git@ssh.dev.azure.com:v3///``). + + Returns None if URL can't be parsed. + """ + parts = _remote_url_parts(url) + if parts is None: + return None + host, segments = parts + + # SCP-like SSH: @: -- any user, not just `git`. + if SCP_LIKE_RE.match(url): + # Azure DevOps SSH carries a leading 'v3/' segment that is + # NOT the org. The org is the second segment. + if host == "ssh.dev.azure.com" and segments[0] == "v3" and len(segments) >= 2: + return (segments[1], host) + return (segments[0], host) + + # HTTPS: https://github.com/owner/repo.git + # ADO: https://dev.azure.com/org/project/_git/repo + try: + if is_azure_devops_hostname(host): + ado_coordinates = parse_ado_repo_url(url) + if ado_coordinates is None: + return None + return ado_coordinates[0], host + if is_visualstudio_legacy_hostname(host): + return (host[: -len(".visualstudio.com")], host) + return (segments[0], host) + except ValueError as exc: + if "mounted below '/tfs/'" in str(exc): + raise + return None + except Exception: + return None diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 0f68a08e43..3f0a6a94c8 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -7,7 +7,6 @@ import json import logging import os -import subprocess import threading import time from dataclasses import dataclass, field @@ -18,18 +17,22 @@ import yaml from ..cache.paths import get_cache_root -from ..cache.url_normalize import SCP_LIKE_RE -from ..utils.git_env import get_git_executable from ..utils.github_host import ( build_ado_api_url, is_azure_devops_hostname, is_gitlab_hostname, is_visualstudio_legacy_hostname, - parse_ado_repo_url, ) from ..utils.path_security import ensure_path_within from ..utils.yaml_io import load_yaml_str from . import _gitlab +from ._remote import ( # noqa: F401 -- re-exported for callers/tests + _extract_org_from_git_remote, + _extract_org_host_port_from_git_remote, + _git_remote_origin_url, + _parse_remote_url, + _remote_url_parts, +) from .parser import PolicyValidationError, load_policy from .project_config import ( _DEFAULT_HASH_ALGORITHM, @@ -946,81 +949,6 @@ def _auto_discover( return PolicyFetchResult(error=None, outcome="absent") -def _extract_org_from_git_remote( - project_root: Path, -) -> tuple[str, str] | None: - """Extract (org, host) from git remote origin URL. - - Handles: - - https://github.com/contoso/my-project.git -> ("contoso", "github.com") - - git@github.com:contoso/my-project.git -> ("contoso", "github.com") - - https://github.example.com/contoso/my-project.git -> ("contoso", "github.example.com") - """ - identity = _extract_org_host_port_from_git_remote(project_root) - return (identity[0], identity[1]) if identity is not None else None - - -class _Unset: - """Sentinel type: an argument was not provided (distinct from ``None``).""" - - -_UNSET = _Unset() - - -def _git_remote_origin_url(project_root: Path) -> str | None: - """Return the ``origin`` remote URL, or ``None`` when unavailable. - - Canonical reader of the project's ``origin`` remote, shared by - :func:`_extract_org_host_port_from_git_remote` and GitLab subgroup - discovery so the ``git remote get-url`` subprocess lives in one place. - """ - try: - result = subprocess.run( - [get_git_executable(), "remote", "get-url", "origin"], - capture_output=True, - text=True, - encoding="utf-8", - cwd=project_root, - timeout=5, - ) - except (subprocess.TimeoutExpired, FileNotFoundError): - return None - if result.returncode != 0: - return None - return result.stdout.strip() - - -def _extract_org_host_port_from_git_remote( - project_root: Path, - *, - remote_url: str | None | _Unset = _UNSET, -) -> tuple[str, str, int | None] | None: - """Extract ``(org, host, port)`` from git remote origin. - - ``remote_url`` lets a caller that already read ``origin`` (e.g. - :func:`_auto_discover`) pass it in so the ``git remote get-url`` subprocess - runs exactly once per discovery. When left unset the origin is read here; - an explicit ``None`` means "no remote" and is not re-read. - """ - if isinstance(remote_url, _Unset): - remote_url = _git_remote_origin_url(project_root) - if not remote_url: - return None - try: - parsed_identity = _parse_remote_url(remote_url) - except ValueError: - return None - if parsed_identity is None: - return None - port = None - if "://" in remote_url: - try: - port = urlparse(remote_url).port - except ValueError: - return None - return parsed_identity[0], parsed_identity[1], port - - def _gitlab_namespace_descending(remote_url: str | None) -> list[str] | None: """Return GitLab policy namespaces from the deepest subgroup to the top. @@ -1119,97 +1047,6 @@ def _gitlab_concealed_closer_result( ) -def _remote_url_parts(url: str) -> tuple[str, list[str]] | None: - """Split a git remote URL into ``(host, path_segments)``. - - Canonical splitter shared by :func:`_parse_remote_url` (which applies - host-specific org interpretation on top) and GitLab subgroup discovery - (:func:`_gitlab_namespace_descending`). Handles SCP-like SSH URLs with any - username (not just ``git@``) and ``scheme://`` URLs. Path segments are - cleaned of empty parts and the trailing ``.git`` suffix; no host-specific - interpretation (ADO ``v3/`` prefix, visualstudio subdomain, ...) is applied - here -- that stays the caller's responsibility. - - Returns ``None`` when the URL cannot be split. - """ - if not url: - return None - - # SCP-like SSH: @: -- any user, not just `git`. - # Closes #1159 for non-`git` SSH users (EMU, custom GHE accounts). - scp_match = SCP_LIKE_RE.match(url) - if scp_match: - host = scp_match.group("host") - path_part = scp_match.group("path") - segments = [p for p in path_part.rstrip("/").removesuffix(".git").split("/") if p] - if not host or not segments: - return None - return (host, segments) - - # HTTPS: https://github.com/owner/repo.git - if "://" in url: - try: - parsed = urlparse(url) - except Exception: - # urlparse only raises ValueError in practice, but the legacy - # parser swallowed any exception here; preserve that. The - # ``/tfs/`` ValueError comes from parse_ado_repo_url (in - # _parse_remote_url), never from urlparse, so nothing to re-raise. - return None - host = parsed.hostname or "" - segments = [ - p for p in parsed.path.strip("/").removesuffix(".git").rstrip("/").split("/") if p - ] - if not host or not segments: - return None - return (host, segments) - - return None - - -def _parse_remote_url(url: str) -> tuple[str, str] | None: - """Parse a git remote URL into (org, host). - - Accepts SCP-style SSH URLs with any username (not just ``git@``), so - EMU/GHE deployments that use a non-``git`` SSH user - (e.g. ``enterprise-user@ghe.corp.com:org/repo.git``) parse correctly. - Also handles Azure DevOps SSH URLs which carry an extra ``v3/`` - path prefix (``git@ssh.dev.azure.com:v3///``). - - Returns None if URL can't be parsed. - """ - parts = _remote_url_parts(url) - if parts is None: - return None - host, segments = parts - - # SCP-like SSH: @: -- any user, not just `git`. - if SCP_LIKE_RE.match(url): - # Azure DevOps SSH carries a leading 'v3/' segment that is - # NOT the org. The org is the second segment. - if host == "ssh.dev.azure.com" and segments[0] == "v3" and len(segments) >= 2: - return (segments[1], host) - return (segments[0], host) - - # HTTPS: https://github.com/owner/repo.git - # ADO: https://dev.azure.com/org/project/_git/repo - try: - if is_azure_devops_hostname(host): - ado_coordinates = parse_ado_repo_url(url) - if ado_coordinates is None: - return None - return ado_coordinates[0], host - if is_visualstudio_legacy_hostname(host): - return (host[: -len(".visualstudio.com")], host) - return (segments[0], host) - except ValueError as exc: - if "mounted below '/tfs/'" in str(exc): - raise - return None - except Exception: - return None - - def _fetch_from_url( url: str, project_root: Path, diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index f2362c52d3..cd1f810321 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -400,7 +400,7 @@ class MutationCase: MutationCase( guard_id="install-deployment-policy-remote-origin-owner", rule_id="install-deployment-policy-remote-origin-owner", - path="src/apm_cli/policy/discovery.py", + path="src/apm_cli/policy/_remote.py", old='[get_git_executable(), "remote", "get-url", "origin"],', new=( '[get_git_executable(), "remote", "get-url", "origin"],\n' diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 403421fe6a..b07492732b 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -254,7 +254,7 @@ def test_returns_none_when_no_skipped_project_confirmed(self, mock_state): class TestExtractOrgFromGitRemote(unittest.TestCase): """Test _extract_org_from_git_remote with mocked subprocess.""" - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_successful_remote(self, mock_run): mock_run.return_value = MagicMock( returncode=0, @@ -271,26 +271,26 @@ def test_successful_remote(self, mock_run): timeout=5, ) - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_git_command_fails(self, mock_run): mock_run.return_value = MagicMock(returncode=1, stdout="") result = _extract_org_from_git_remote(Path("/fake")) self.assertIsNone(result) - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_git_not_found(self, mock_run): mock_run.side_effect = FileNotFoundError("git not found") result = _extract_org_from_git_remote(Path("/fake")) self.assertIsNone(result) - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_timeout(self, mock_run): mock_run.side_effect = subprocess.TimeoutExpired(cmd="git", timeout=5) result = _extract_org_from_git_remote(Path("/fake")) self.assertIsNone(result) - @patch("apm_cli.policy.discovery._parse_remote_url") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote._parse_remote_url") + @patch("apm_cli.policy._remote.subprocess.run") def test_remote_parser_value_error_returns_none(self, mock_run, mock_parse): mock_run.return_value = MagicMock( returncode=0, @@ -302,9 +302,9 @@ def test_remote_parser_value_error_returns_none(self, mock_run, mock_parse): self.assertIsNone(result) - @patch("apm_cli.policy.discovery.urlparse") - @patch("apm_cli.policy.discovery._parse_remote_url") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.urlparse") + @patch("apm_cli.policy._remote._parse_remote_url") + @patch("apm_cli.policy._remote.subprocess.run") def test_remote_port_value_error_returns_none(self, mock_run, mock_parse, mock_urlparse): mock_run.return_value = MagicMock( returncode=0, @@ -823,7 +823,7 @@ def test_override_owner_repo(self, mock_fetch): self.assertIn("org:", result.source) @patch("apm_cli.policy.discovery._fetch_github_contents") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_override_org_auto_discovers(self, mock_run, mock_fetch): mock_run.return_value = MagicMock( returncode=0, @@ -837,7 +837,7 @@ def test_override_org_auto_discovers(self, mock_run, mock_fetch): mock_fetch.assert_called_once() @patch("apm_cli.policy.discovery._fetch_github_contents") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_none_auto_discovers(self, mock_run, mock_fetch): mock_run.return_value = MagicMock( returncode=0, @@ -850,7 +850,7 @@ def test_none_auto_discovers(self, mock_run, mock_fetch): self.assertTrue(result.found) self.assertEqual(result.source, "org:contoso/.github-private") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_no_git_remote(self, mock_run): mock_run.return_value = MagicMock(returncode=1, stdout="") @@ -860,7 +860,7 @@ def test_no_git_remote(self, mock_run): self.assertIn("Could not determine org", result.error) @patch("apm_cli.policy.discovery._fetch_github_contents") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_cache_hit_returns_cached(self, mock_run, mock_fetch): mock_run.return_value = MagicMock( returncode=0, @@ -881,7 +881,7 @@ def test_cache_hit_returns_cached(self, mock_run, mock_fetch): self.assertEqual(mock_fetch.call_count, 1) @patch("apm_cli.policy.discovery._fetch_github_contents") - @patch("apm_cli.policy.discovery.subprocess.run") + @patch("apm_cli.policy._remote.subprocess.run") def test_ghe_repo_ref_includes_host(self, mock_run, mock_fetch): mock_run.return_value = MagicMock( returncode=0, @@ -1140,7 +1140,7 @@ def fake_run(cmd, **kwargs): mock_candidates.return_value = ("_apm",) with ( - patch("apm_cli.policy.discovery.subprocess.run", side_effect=fake_run) as mock_run, + patch("apm_cli.policy._remote.subprocess.run", side_effect=fake_run) as mock_run, patch.dict(os.environ, {"ADO_HOST": "ado.example.test"}, clear=False), tempfile.TemporaryDirectory() as tmpdir, ): diff --git a/tests/unit/policy/test_discovery_phase3w4.py b/tests/unit/policy/test_discovery_phase3w4.py index 8da423e05e..dfb370b032 100644 --- a/tests/unit/policy/test_discovery_phase3w4.py +++ b/tests/unit/policy/test_discovery_phase3w4.py @@ -358,7 +358,7 @@ def test_ssh_empty_path_returns_none(self) -> None: def test_https_url_parse_exception_returns_none(self) -> None: """HTTPS URL where urlparse raises -> returns None.""" - with patch("apm_cli.policy.discovery.urlparse", side_effect=Exception("boom")): + with patch("apm_cli.policy._remote.urlparse", side_effect=Exception("boom")): result = _parse_remote_url("https://github.com/owner/repo") assert result is None diff --git a/tests/unit/policy/test_discovery_policy_resolution.py b/tests/unit/policy/test_discovery_policy_resolution.py index ff3a96f9af..29cdb10444 100644 --- a/tests/unit/policy/test_discovery_policy_resolution.py +++ b/tests/unit/policy/test_discovery_policy_resolution.py @@ -435,7 +435,7 @@ def test_ssh_empty_path_returns_none(self) -> None: def test_https_url_parse_exception_returns_none(self) -> None: """HTTPS URL where urlparse raises -> returns None.""" - with patch("apm_cli.policy.discovery.urlparse", side_effect=Exception("boom")): + with patch("apm_cli.policy._remote.urlparse", side_effect=Exception("boom")): result = _parse_remote_url("https://github.com/owner/repo") assert result is None From af821d5f5ba30a876cb1a4f6c355668b505a13cd Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:14:49 -0300 Subject: [PATCH 10/14] fix(policy): concealed GitLab policy fails closed unconditionally + cache probe Local apm-review-panel dogfood (supply-chain, auth, perf, test-coverage, devx, doc-writer, architect) surfaced: - Fail-OPEN by default (devx): the concealed-closer result returned `cache_miss_fetch_fail`, which honours `policy.fetch_failure_default` (default `warn`) -- so a concealed private closer policy silently proceeded with no enforcement, contradicting the doc's "fails closed." Return `incomplete_chain` instead, which ALWAYS fails closed regardless of the knob; the CLI now surfaces the specific "closer policy concealed" message (not the generic "check connectivity" copy). - Perf (perf persona): the concealment `git ls-remote` probe ran uncached on every install in the common "team inherits org policy" path (N-1 round-trips). Cache each probe verdict per `(host, namespace, repo)` under the policy cache (same TTL); `no_cache` bypasses it. - Coverage (test-coverage): add closest-first tests for a middle/among-many confirmed level, the fail-closed-under-warn gate assertion, and the cache hit / no-cache-bypass paths. - Docs (devx, doc-writer): reword the concealed-policy caution to state the unconditional block; add a governance-guide "sharp edges" bullet (closest-wins shadowing + concealed-blocks-install) and the skill note. Co-Authored-By: Claude Opus 4.8 --- .../src/content/docs/enterprise/apm-policy.md | 2 +- .../docs/enterprise/governance-guide.md | 1 + .../.apm/skills/apm-usage/governance.md | 5 ++ src/apm_cli/policy/_gitlab.py | 77 +++++++++++++++++- src/apm_cli/policy/discovery.py | 18 ++++- tests/unit/policy/test_discovery.py | 78 ++++++++++++++++++- 6 files changed, 175 insertions(+), 6 deletions(-) diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index de379ec30a..d723b05ad2 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -96,7 +96,7 @@ A bare nested `extends:` ref whose first segment contains a dot (e.g. a group na Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. :::caution[Concealed private policy projects] -GitLab returns HTTP 404 both for a missing project and for a private one the token cannot read, so a closer `apm-policy` a team publishes could otherwise be silently skipped in favour of a weaker ancestor. Before applying an ancestor policy over a skipped closer level, APM confirms via authenticated Git that the skipped closer `apm-policy` project does not exist; if Git confirms it *does* exist, discovery fails closed rather than downgrade. The residual case where the token can read neither the policy file nor the project over Git is indistinguishable from a genuinely absent project and still ascends -- grant the CI token read access to every `apm-policy` project it should honour. +GitLab returns HTTP 404 both for a missing project and for a private one the token cannot read, so a closer `apm-policy` a team publishes could otherwise be silently skipped in favour of a weaker ancestor. Before applying an ancestor policy over a skipped closer level, APM checks via authenticated Git whether that skipped closer `apm-policy` project actually exists; if it does, discovery **fails closed unconditionally** (the install is blocked regardless of `policy.fetch_failure_default`) rather than downgrade to the weaker ancestor -- the error names the concealed project so you can grant the token read access or remove it. The residual case where the token can read neither the policy file nor the project over Git is indistinguishable from a genuinely absent project and still ascends -- grant the CI token read access to every `apm-policy` project it should honour. ::: :::caution[Self-managed GitLab requires GITLAB_HOST or APM_GITLAB_HOSTS] diff --git a/docs/src/content/docs/enterprise/governance-guide.md b/docs/src/content/docs/enterprise/governance-guide.md index 8899048361..a45a6714a0 100644 --- a/docs/src/content/docs/enterprise/governance-guide.md +++ b/docs/src/content/docs/enterprise/governance-guide.md @@ -505,6 +505,7 @@ These are the sharp edges. Plan around them; do not assume they are solved. - **`apm audit --ci` in `warn` mode rewrites violations to `passed=True`.** Warn mode never fails CI exit. The visibility is in the SARIF output, not the exit code. Operational mitigation: monitor Code Scanning alerts during the warn-mode rollout phase; do not assume CI green means "no policy violations" while in warn mode. - **`apm install` has no `--policy ` flag.** Only `apm audit` does. This is the air-gapped install gap. Operational mitigation: use `extends:` from a reachable mirror, or run audit (which does support `--policy `) as the gating check and skip install-time enforcement in air-gapped CI. - **Plain git remotes are not auto-discovered.** GitHub and GitHub Enterprise use the `.github` cascade, Azure DevOps uses the org `apm` project and `apm-policy` repository (with `_apm/_apm` as a legacy fallback), and GitLab walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` (see [Policy Files](./apm-policy/#where-it-lives)). Configure self-managed GitLab with `GITLAB_HOST` or `APM_GITLAB_HOSTS`. For unsupported remotes, pass `apm audit --ci --policy ` explicitly. +- **GitLab closest-wins can be shadowed by a closer subgroup policy, and a concealed closer policy blocks install.** Two edges of subgroup-scoped discovery: (1) anyone who can create an `apm-policy` project in an intermediate subgroup can publish a *weaker* policy that overrides a stricter ancestor for every project under that subgroup (closest wins; discovery does not enforce tighten-only -- only `extends:` chains do). Operational mitigation: restrict who can create `apm-policy` projects at the GitLab RBAC layer. (2) Because GitLab 404s a private project the token cannot read, a closer `apm-policy` the CI token is denied causes discovery to **fail closed unconditionally** (blocking install) rather than silently downgrade -- grant the CI token read access to every `apm-policy` project it should honour, or the concealed level will block. See [Policy Files](./apm-policy/#where-it-lives). - **Trust anchor is `git remote get-url origin`.** A developer who pushes the project to a personal org will have policy discovery resolve `/.github/apm-policy.yml` -- which they control. Operational mitigation: branch protection on the canonical repo is the trust boundary; nothing about a personal fork can bypass what your CI requires before merge. - **`apm install --dry-run` silently downgrades hash-mismatch.** In dry-run, `raise_blocking_errors=False` (outcome_routing.py:104-119) causes the mismatch to surface as `discovery_miss` with no "Would be blocked" line and exit 0. Operational mitigation: rely on `apm audit --ci` in CI for hash-pin verification, not on `apm install --dry-run`. - **`apm audit --ci --no-policy` and `APM_POLICY_DISABLE=1` skip policy checks.** The 21 policy checks are bypassed in audit, but the 8 baseline lockfile checks still run. Operational mitigation: keep bypass flags out of required CI workflows; the bypass contract in section 7 is authoritative. diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index 277410d8f0..71888ac718 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -429,6 +429,11 @@ closest `apm-policy` (e.g. `acme/dept-a/team-x/apm-policy` before `acme/dept-a/apm-policy` before `acme/apm-policy`), so a team can scope its own policy under a subgroup; a team policy can `extends:` an ancestor group's policy to inherit it. A flat `/` remote probes only `/apm-policy`. +Because GitLab 404s a private project the token cannot read, before applying an +ancestor policy over a skipped closer level APM confirms via authenticated Git +that the closer `apm-policy` project does not exist; if it does, discovery fails +closed unconditionally (blocking install) rather than downgrade to the weaker +ancestor -- grant the token read access to every `apm-policy` it should honour. Configure a self-managed host with `GITLAB_HOST` or `APM_GITLAB_HOSTS`, and use `APM_GITLAB_POLICY_REPO` to select another project name. Repositories with no detectable git remote (unpacked diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index 4b7c35c16f..6cd0b1559f 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -116,6 +116,8 @@ def first_concealed_closer_policy( *, host: str, port: int | None, + project_root: Path | None = None, + no_cache: bool = False, ) -> str | None: """Return the closest skipped namespace whose ``apm-policy`` project exists. @@ -128,13 +130,86 @@ def first_concealed_closer_policy( concealed 404, so the caller must fail closed), or ``None`` when no skipped level's project can be confirmed. ``skipped_namespaces`` is ordered closest-first. + + Each ``git ls-remote`` probe is cached per ``(host, namespace, repo)`` under + the project's policy cache (same TTL as policies) so the common "team + inherits the org policy" path -- every install re-walking the same absent + closer levels -- does not pay a fresh network round-trip per level on every + run. Pass ``project_root`` to enable the cache; ``no_cache`` bypasses it. """ for namespace in skipped_namespaces: - if _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) is True: + cached = ( + None + if (no_cache or project_root is None) + else _read_concealment_verdict(project_root, host, port, namespace, repo) + ) + if cached == "present": + return namespace + if cached == "absent": + continue + exists = _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) + if not no_cache and project_root is not None: + _write_concealment_verdict( + project_root, host, port, namespace, repo, "present" if exists else "absent" + ) + if exists is True: return namespace return None +def _concealment_cache_file(project_root: Path, host: str, port: int | None, ns: str, repo: str): + """Return the cache file path for one namespace's concealment verdict.""" + from .discovery import _cache_key, _get_cache_dir + + host_label = f"{host}:{port}" if port is not None else host + key = _cache_key(f"concealment:{host_label}/{ns}/{repo}") + return _get_cache_dir(project_root) / f"concealment_{key}.json" + + +def _read_concealment_verdict( + project_root: Path, host: str, port: int | None, ns: str, repo: str +) -> str | None: + """Return a fresh cached ``"present"``/``"absent"`` verdict, or ``None``.""" + import json + import time + + from .discovery import CACHE_SCHEMA_VERSION, DEFAULT_CACHE_TTL + + try: + raw = _concealment_cache_file(project_root, host, port, ns, repo).read_text( + encoding="utf-8" + ) + data = json.loads(raw) + except (OSError, ValueError): + return None + if data.get("schema") != CACHE_SCHEMA_VERSION: + return None + if time.time() - float(data.get("ts", 0)) > DEFAULT_CACHE_TTL: + return None + verdict = data.get("verdict") + return verdict if verdict in ("present", "absent") else None + + +def _write_concealment_verdict( + project_root: Path, host: str, port: int | None, ns: str, repo: str, verdict: str +) -> None: + """Persist a concealment verdict (best-effort; cache write never blocks).""" + import json + import time + + from .discovery import CACHE_SCHEMA_VERSION + + path = _concealment_cache_file(project_root, host, port, ns, repo) + try: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + json.dumps({"schema": CACHE_SCHEMA_VERSION, "verdict": verdict, "ts": time.time()}), + encoding="utf-8", + ) + except OSError: + return + + def _fetch_gitlab_chain_parent( parent_ref: str, *, diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 3f0a6a94c8..8fbf9399bc 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1016,7 +1016,12 @@ def _gitlab_walk_candidate( if result.outcome != "absent": if skipped and result.outcome in {"found", "empty"}: concealed = _gitlab.first_concealed_closer_policy( - skipped, candidate_repo, host=host, port=port + skipped, + candidate_repo, + host=host, + port=port, + project_root=project_root, + no_cache=no_cache, ) if concealed is not None: return _gitlab_concealed_closer_result(concealed, candidate_repo, host, port) @@ -1033,7 +1038,14 @@ def _gitlab_walk_candidate( def _gitlab_concealed_closer_result( namespace: str, repo: str, host: str, port: int | None ) -> PolicyFetchResult: - """Fail-closed result when a skipped closer GitLab policy project exists.""" + """Fail-closed result when a skipped closer GitLab policy project exists. + + Uses ``incomplete_chain`` -- an outcome that ALWAYS fails closed in + :func:`outcome_routing.route_discovery_outcome`, regardless of the project's + ``policy.fetch_failure_default`` -- because a concealed closer policy is a + governance ambiguity that must never silently downgrade to a weaker ancestor + (a ``cache_miss_fetch_fail`` here would default to ``warn`` and proceed). + """ host_label = f"{host}:{port}" if port is not None else host return PolicyFetchResult( source=f"org:{host_label}/{namespace}/{repo}", @@ -1043,7 +1055,7 @@ def _gitlab_concealed_closer_result( "project the token cannot access); refusing to silently apply a weaker " "ancestor policy. Grant the token read access to that project, or remove it." ), - outcome="cache_miss_fetch_fail", + outcome="incomplete_chain", ) diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index b07492732b..9c3af768ef 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -250,6 +250,64 @@ def test_returns_none_when_no_skipped_project_confirmed(self, mock_state): ) self.assertIsNone(result) + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_returns_closest_confirmed_when_match_is_a_middle_level(self, mock_state): + # Only the MIDDLE skipped namespace is git-confirmed to exist; the + # closest-first scan must return it, not None and not a farther level. + mock_state.side_effect = lambda **kw: True if kw["org"] == "acme/dept-a" else None + result = first_concealed_closer_policy( + ["acme/dept-a/team-x", "acme/dept-a", "acme"], + "apm-policy", + host="gitlab.com", + port=None, + ) + self.assertEqual(result, "acme/dept-a") + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_returns_the_closest_of_multiple_confirmed(self, mock_state): + # When several skipped levels exist, the CLOSEST (first) wins. + mock_state.return_value = True + result = first_concealed_closer_policy( + ["acme/dept-a/team-x", "acme/dept-a", "acme"], + "apm-policy", + host="gitlab.com", + port=None, + ) + self.assertEqual(result, "acme/dept-a/team-x") + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_absent_verdict_is_cached_so_the_probe_runs_once(self, mock_state): + # With project_root set, a genuine-absent git verdict is cached, so a + # second walk over the same level does not re-probe the network. + mock_state.return_value = None + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + first = first_concealed_closer_policy( + ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root + ) + second = first_concealed_closer_policy( + ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root + ) + self.assertIsNone(first) + self.assertIsNone(second) + self.assertEqual(mock_state.call_count, 1) # cached: probed once, not twice + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_no_cache_bypasses_the_concealment_cache(self, mock_state): + mock_state.return_value = None + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + for _ in range(2): + first_concealed_closer_policy( + ["acme/dept-a"], + "apm-policy", + host="gitlab.com", + port=None, + project_root=root, + no_cache=True, + ) + self.assertEqual(mock_state.call_count, 2) # no_cache: re-probes each time + class TestExtractOrgFromGitRemote(unittest.TestCase): """Test _extract_org_from_git_remote with mocked subprocess.""" @@ -1284,9 +1342,27 @@ def test_gitlab_subgroup_concealed_closer_policy_fails_closed( result = _auto_discover(Path(tmpdir), no_cache=True) self.assertFalse(result.found) - self.assertEqual(result.outcome, "cache_miss_fetch_fail") + # ``incomplete_chain`` ALWAYS fails closed (unlike cache_miss_fetch_fail, + # which defaults to warn) -- the concealed policy must never downgrade. + self.assertEqual(result.outcome, "incomplete_chain") self.assertIn("acme/dept-a/team-x/apm-policy", result.error) + def test_gitlab_concealed_result_blocks_even_with_fetch_failure_default_warn(self): + """The concealed-closer outcome blocks install under the DEFAULT warn + knob -- proving the fix is fail-closed, not fail-open (#2753 review).""" + from apm_cli.install.errors import PolicyViolationError + from apm_cli.policy.discovery import _gitlab_concealed_closer_result + from apm_cli.policy.outcome_routing import route_discovery_outcome + + concealed = _gitlab_concealed_closer_result("acme/dept-a", "apm-policy", "gitlab.com", None) + with self.assertRaises(PolicyViolationError): + route_discovery_outcome( + concealed, + logger=None, + fetch_failure_default="warn", + raise_blocking_errors=True, + ) + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") From 7e7b1f4d6aca0a496e31913e31744b0eb0f1e512 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:18:48 -0300 Subject: [PATCH 11/14] fix(policy): sound concealment caching + cache-only + stale-ancestor gate Fourth Copilot review on the concealment mechanism: - Cache only the DEFINITIVE `present` verdict. `_gitlab_project_state_via_git` returns `None` for a missing project AND for auth/network/timeout failures alike, so caching an `absent` verdict let a transient failure suppress re-probing for the whole TTL and apply a weaker ancestor. Absence is never definitive, so the common inherit-from-org path re-probes each install (cost accepted; documented in the PR trade-offs). - Cache-only (offline local-bundle) never issues a network probe: a cached `present` fails closed, and an unverifiable level fails closed deterministically instead of reaching out to the network. - Gate the concealment check on a usable policy (`result.policy is not None`), not just `found`/`empty` -- a `cached_stale` ancestor also carries a policy and must be checked for a concealed closer before it is applied. - Align the owner-guard rule description with the canonical owner (`policy/_remote.py`, not `discovery.py`). Co-Authored-By: Claude Opus 4.8 --- .../checks/install_policy_intent.py | 2 +- src/apm_cli/policy/_gitlab.py | 77 ++++++++++--------- src/apm_cli/policy/discovery.py | 15 ++-- tests/unit/policy/test_discovery.py | 43 +++++++---- 4 files changed, 81 insertions(+), 56 deletions(-) diff --git a/scripts/architecture_linter/checks/install_policy_intent.py b/scripts/architecture_linter/checks/install_policy_intent.py index 707e6f795b..c9371fc3f2 100644 --- a/scripts/architecture_linter/checks/install_policy_intent.py +++ b/scripts/architecture_linter/checks/install_policy_intent.py @@ -108,7 +108,7 @@ group=GROUP, guard_ids=(RULE_REMOTE_ORIGIN_OWNER,), description=( - "Policy discovery reads and parses the git remote through one owner (discovery.py)." + "Policy discovery reads and parses the git remote through one owner (policy/_remote.py)." ), check=check_policy_remote_origin_owner, ), diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index 6cd0b1559f..73d5fb5294 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -118,42 +118,46 @@ def first_concealed_closer_policy( port: int | None, project_root: Path | None = None, no_cache: bool = False, + cache_only: bool = False, ) -> str | None: - """Return the closest skipped namespace whose ``apm-policy`` project exists. + """Return the closest skipped namespace that must fail the walk closed. During the subgroup walk (see :func:`discovery._gitlab_walk_candidate`) an ``absent`` level is ambiguous -- GitLab returns 404 both for a missing project and for a private one the token cannot read. Before an ancestor policy is applied over the skipped closer levels, this confirms via authenticated Git whether any skipped closer ``apm-policy`` project actually - exists. It returns the closest such namespace (its ``absent`` was a - concealed 404, so the caller must fail closed), or ``None`` when no skipped - level's project can be confirmed. ``skipped_namespaces`` is ordered - closest-first. - - Each ``git ls-remote`` probe is cached per ``(host, namespace, repo)`` under - the project's policy cache (same TTL as policies) so the common "team - inherits the org policy" path -- every install re-walking the same absent - closer levels -- does not pay a fresh network round-trip per level on every - run. Pass ``project_root`` to enable the cache; ``no_cache`` bypasses it. + exists. It returns the closest namespace the caller must fail closed on (a + confirmed-present concealed project, or -- in ``cache_only`` mode -- one that + cannot be verified offline), or ``None`` when every skipped level is known to + be genuinely absent. ``skipped_namespaces`` is ordered closest-first. + + Only the DEFINITIVE ``present`` verdict is cached (per ``(host, namespace, + repo)`` at the policy-cache TTL): ``_gitlab_project_state_via_git`` returns + ``None`` for a missing project AND for an auth/network/timeout failure alike, + so caching an ``absent`` verdict would let a transient failure suppress + re-probing for the whole TTL and wrongly apply a weaker ancestor. Because + absence is never definitive, the common "team inherits the org policy" path + re-probes each install; that cost is accepted (see the PR trade-offs). + + ``cache_only`` (the offline local-bundle contract) never issues a network + probe: a cached ``present`` fails closed, and an unverifiable level fails + closed deterministically rather than reaching out to the network. """ + use_cache = project_root is not None and not no_cache for namespace in skipped_namespaces: - cached = ( - None - if (no_cache or project_root is None) - else _read_concealment_verdict(project_root, host, port, namespace, repo) - ) - if cached == "present": + if use_cache and _read_concealment_verdict(project_root, host, port, namespace, repo): + return namespace # cached definitive "present" -> concealed, fail closed + if cache_only: + # Offline: no network probe. Without a cached "present" we cannot + # confirm this closer level is genuinely absent, so fail closed + # rather than silently apply a weaker ancestor. return namespace - if cached == "absent": - continue - exists = _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) - if not no_cache and project_root is not None: - _write_concealment_verdict( - project_root, host, port, namespace, repo, "present" if exists else "absent" - ) - if exists is True: + if _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) is True: + if use_cache: + _write_concealment_verdict(project_root, host, port, namespace, repo) return namespace + # Indeterminate (None): missing OR transient failure -- NOT cached. return None @@ -168,8 +172,12 @@ def _concealment_cache_file(project_root: Path, host: str, port: int | None, ns: def _read_concealment_verdict( project_root: Path, host: str, port: int | None, ns: str, repo: str -) -> str | None: - """Return a fresh cached ``"present"``/``"absent"`` verdict, or ``None``.""" +) -> bool: + """Return ``True`` iff a fresh definitive ``"present"`` verdict is cached. + + Only ``present`` is ever cached (see :func:`first_concealed_closer_policy`), + so a hit means the closer project is confirmed to exist. + """ import json import time @@ -181,19 +189,18 @@ def _read_concealment_verdict( ) data = json.loads(raw) except (OSError, ValueError): - return None + return False if data.get("schema") != CACHE_SCHEMA_VERSION: - return None + return False if time.time() - float(data.get("ts", 0)) > DEFAULT_CACHE_TTL: - return None - verdict = data.get("verdict") - return verdict if verdict in ("present", "absent") else None + return False + return data.get("verdict") == "present" def _write_concealment_verdict( - project_root: Path, host: str, port: int | None, ns: str, repo: str, verdict: str + project_root: Path, host: str, port: int | None, ns: str, repo: str ) -> None: - """Persist a concealment verdict (best-effort; cache write never blocks).""" + """Persist the definitive ``present`` verdict (best-effort; never blocks).""" import json import time @@ -203,7 +210,7 @@ def _write_concealment_verdict( try: path.parent.mkdir(parents=True, exist_ok=True) path.write_text( - json.dumps({"schema": CACHE_SCHEMA_VERSION, "verdict": verdict, "ts": time.time()}), + json.dumps({"schema": CACHE_SCHEMA_VERSION, "verdict": "present", "ts": time.time()}), encoding="utf-8", ) except OSError: diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 8fbf9399bc..cbe2c9bff9 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -1014,7 +1014,10 @@ def _gitlab_walk_candidate( cache_only=cache_only, ) if result.outcome != "absent": - if skipped and result.outcome in {"found", "empty"}: + # Gate on a usable policy (found/empty/cached_stale all carry one), + # not just found/empty -- a stale-but-usable ancestor must also be + # checked for a concealed closer before it is applied. + if skipped and result.policy is not None: concealed = _gitlab.first_concealed_closer_policy( skipped, candidate_repo, @@ -1022,6 +1025,7 @@ def _gitlab_walk_candidate( port=port, project_root=project_root, no_cache=no_cache, + cache_only=cache_only, ) if concealed is not None: return _gitlab_concealed_closer_result(concealed, candidate_repo, host, port) @@ -1050,10 +1054,11 @@ def _gitlab_concealed_closer_result( return PolicyFetchResult( source=f"org:{host_label}/{namespace}/{repo}", error=( - f"A closer GitLab policy project {namespace}/{repo} exists but its " - "apm-policy.yml could not be read (GitLab returns 404 for a private " - "project the token cannot access); refusing to silently apply a weaker " - "ancestor policy. Grant the token read access to that project, or remove it." + f"A closer GitLab policy project {namespace}/{repo} could not be confirmed " + "absent (GitLab returns 404 for a private project the token cannot read, or " + "the project state was unverifiable offline in cache-only mode); refusing to " + "silently apply a weaker ancestor policy. Grant the token read access to that " + "project, or remove it." ), outcome="incomplete_chain", ) diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 9c3af768ef..098f8f60ba 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -276,10 +276,10 @@ def test_returns_the_closest_of_multiple_confirmed(self, mock_state): self.assertEqual(result, "acme/dept-a/team-x") @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_absent_verdict_is_cached_so_the_probe_runs_once(self, mock_state): - # With project_root set, a genuine-absent git verdict is cached, so a - # second walk over the same level does not re-probe the network. - mock_state.return_value = None + def test_present_verdict_is_cached_so_the_probe_runs_once(self, mock_state): + # Only the DEFINITIVE "present" verdict is cached: a second walk over a + # confirmed-present level does not re-probe the network. + mock_state.return_value = True with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) first = first_concealed_closer_policy( @@ -288,25 +288,38 @@ def test_absent_verdict_is_cached_so_the_probe_runs_once(self, mock_state): second = first_concealed_closer_policy( ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root ) - self.assertIsNone(first) - self.assertIsNone(second) - self.assertEqual(mock_state.call_count, 1) # cached: probed once, not twice + self.assertEqual(first, "acme/dept-a") + self.assertEqual(second, "acme/dept-a") + self.assertEqual(mock_state.call_count, 1) # present cached: probed once @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_no_cache_bypasses_the_concealment_cache(self, mock_state): + def test_indeterminate_verdict_is_not_cached_so_it_re_probes(self, mock_state): + # None (missing OR transient failure) is NOT cached -- a transient error + # must never be remembered as "absent" and suppress re-probing. mock_state.return_value = None with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) for _ in range(2): first_concealed_closer_policy( - ["acme/dept-a"], - "apm-policy", - host="gitlab.com", - port=None, - project_root=root, - no_cache=True, + ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root ) - self.assertEqual(mock_state.call_count, 2) # no_cache: re-probes each time + self.assertEqual(mock_state.call_count, 2) # not cached: re-probes each time + + @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") + def test_cache_only_never_probes_and_fails_closed_when_unverifiable(self, mock_state): + # Offline (cache_only) must not touch the network; without a cached + # "present" verdict it fails closed rather than applying a weaker ancestor. + with tempfile.TemporaryDirectory() as tmpdir: + result = first_concealed_closer_policy( + ["acme/dept-a"], + "apm-policy", + host="gitlab.com", + port=None, + project_root=Path(tmpdir), + cache_only=True, + ) + self.assertEqual(result, "acme/dept-a") # unverifiable offline -> fail closed + mock_state.assert_not_called() # no network probe in cache_only class TestExtractOrgFromGitRemote(unittest.TestCase): From 9f423145013458df03b47b00d9d043845a53dcce Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:37:03 -0300 Subject: [PATCH 12/14] refactor(policy): drop the concealed-policy probe; a GitLab 404 just ascends The concealed-private-policy detection (git ls-remote existence probe + fail-closed + verdict cache) was scope creep beyond #2753 and, across several review rounds, proved not worth it: it only caught the narrow "project git-readable but policy file 404s" case, could not be soundly negative-cached (git can't tell a missing project from a transient failure), and false-positived on an apm-policy project that exists with only a README -- blocking inheritance from a valid ancestor. Remove it entirely. A GitLab `absent` (404) now simply ascends to the next ancestor -- the same `404 == no policy at this level` behaviour GitHub and ADO discovery already have, and strictly better than the pre-PR single-level behaviour (an ancestor policy applies where nothing did before). The GitLab 404-conceals-private-projects ambiguity is documented as a platform limitation (grant the token read access). The subgroup walk, nested `extends:`, leaf-aware host-pin, and the `_remote.py` extraction + its owner-guard are unchanged. Co-Authored-By: Claude Opus 4.8 --- .../src/content/docs/enterprise/apm-policy.md | 4 +- .../docs/enterprise/governance-guide.md | 2 +- .../.apm/skills/apm-usage/governance.md | 9 +- src/apm_cli/policy/_gitlab.py | 107 ------------- src/apm_cli/policy/discovery.py | 53 +------ tests/unit/policy/test_discovery.py | 146 +----------------- 6 files changed, 13 insertions(+), 308 deletions(-) diff --git a/docs/src/content/docs/enterprise/apm-policy.md b/docs/src/content/docs/enterprise/apm-policy.md index d723b05ad2..fc1a3d1327 100644 --- a/docs/src/content/docs/enterprise/apm-policy.md +++ b/docs/src/content/docs/enterprise/apm-policy.md @@ -95,8 +95,8 @@ A bare nested `extends:` ref whose first segment contains a dot (e.g. a group na Set `APM_GITLAB_POLICY_REPO` to use a different project name if your org already publishes policy under another name. A project with no `apm-policy` at any level (or the configured override) is treated as a clean "no policy" outcome, matching the fallthrough behaviour on GitHub and ADO -- it does not print a warning. -:::caution[Concealed private policy projects] -GitLab returns HTTP 404 both for a missing project and for a private one the token cannot read, so a closer `apm-policy` a team publishes could otherwise be silently skipped in favour of a weaker ancestor. Before applying an ancestor policy over a skipped closer level, APM checks via authenticated Git whether that skipped closer `apm-policy` project actually exists; if it does, discovery **fails closed unconditionally** (the install is blocked regardless of `policy.fetch_failure_default`) rather than downgrade to the weaker ancestor -- the error names the concealed project so you can grant the token read access or remove it. The residual case where the token can read neither the policy file nor the project over Git is indistinguishable from a genuinely absent project and still ascends -- grant the CI token read access to every `apm-policy` project it should honour. +:::caution[GitLab conceals private projects with 404] +GitLab returns HTTP 404 both for a missing `apm-policy` project and for a private one the token cannot read. A closer policy the CI token is denied is therefore indistinguishable from "no policy at this level," so the walk skips it and applies the next ancestor -- the same `404 == no policy` behaviour GitHub and ADO discovery already have. Grant the CI token read access to every `apm-policy` project it should honour, so a closer policy is never silently skipped. ::: :::caution[Self-managed GitLab requires GITLAB_HOST or APM_GITLAB_HOSTS] diff --git a/docs/src/content/docs/enterprise/governance-guide.md b/docs/src/content/docs/enterprise/governance-guide.md index a45a6714a0..239e8f7716 100644 --- a/docs/src/content/docs/enterprise/governance-guide.md +++ b/docs/src/content/docs/enterprise/governance-guide.md @@ -505,7 +505,7 @@ These are the sharp edges. Plan around them; do not assume they are solved. - **`apm audit --ci` in `warn` mode rewrites violations to `passed=True`.** Warn mode never fails CI exit. The visibility is in the SARIF output, not the exit code. Operational mitigation: monitor Code Scanning alerts during the warn-mode rollout phase; do not assume CI green means "no policy violations" while in warn mode. - **`apm install` has no `--policy ` flag.** Only `apm audit` does. This is the air-gapped install gap. Operational mitigation: use `extends:` from a reachable mirror, or run audit (which does support `--policy `) as the gating check and skip install-time enforcement in air-gapped CI. - **Plain git remotes are not auto-discovered.** GitHub and GitHub Enterprise use the `.github` cascade, Azure DevOps uses the org `apm` project and `apm-policy` repository (with `_apm/_apm` as a legacy fallback), and GitLab walks the subgroup tree from the project's own group up to the top-level group and applies the closest `apm-policy` (see [Policy Files](./apm-policy/#where-it-lives)). Configure self-managed GitLab with `GITLAB_HOST` or `APM_GITLAB_HOSTS`. For unsupported remotes, pass `apm audit --ci --policy ` explicitly. -- **GitLab closest-wins can be shadowed by a closer subgroup policy, and a concealed closer policy blocks install.** Two edges of subgroup-scoped discovery: (1) anyone who can create an `apm-policy` project in an intermediate subgroup can publish a *weaker* policy that overrides a stricter ancestor for every project under that subgroup (closest wins; discovery does not enforce tighten-only -- only `extends:` chains do). Operational mitigation: restrict who can create `apm-policy` projects at the GitLab RBAC layer. (2) Because GitLab 404s a private project the token cannot read, a closer `apm-policy` the CI token is denied causes discovery to **fail closed unconditionally** (blocking install) rather than silently downgrade -- grant the CI token read access to every `apm-policy` project it should honour, or the concealed level will block. See [Policy Files](./apm-policy/#where-it-lives). +- **GitLab closest-wins is not tighten-only, and a closer policy the token cannot read is skipped.** Two edges of subgroup-scoped discovery: (1) anyone who can create an `apm-policy` project in an intermediate subgroup can publish a *weaker* policy that overrides a stricter ancestor for every project under that subgroup (closest wins; discovery does not enforce tighten-only -- only `extends:` chains do). Operational mitigation: restrict who can create `apm-policy` projects at the GitLab RBAC layer. (2) GitLab 404s a private project the token cannot read, so a closer `apm-policy` the CI token is denied is indistinguishable from "no policy here" and is skipped in favour of the next ancestor (the same `404 == no policy` behaviour as GitHub/ADO). Operational mitigation: grant the CI token read access to every `apm-policy` project it should honour. See [Policy Files](./apm-policy/#where-it-lives). - **Trust anchor is `git remote get-url origin`.** A developer who pushes the project to a personal org will have policy discovery resolve `/.github/apm-policy.yml` -- which they control. Operational mitigation: branch protection on the canonical repo is the trust boundary; nothing about a personal fork can bypass what your CI requires before merge. - **`apm install --dry-run` silently downgrades hash-mismatch.** In dry-run, `raise_blocking_errors=False` (outcome_routing.py:104-119) causes the mismatch to surface as `discovery_miss` with no "Would be blocked" line and exit 0. Operational mitigation: rely on `apm audit --ci` in CI for hash-pin verification, not on `apm install --dry-run`. - **`apm audit --ci --no-policy` and `APM_POLICY_DISABLE=1` skip policy checks.** The 21 policy checks are bypassed in audit, but the 8 baseline lockfile checks still run. Operational mitigation: keep bypass flags out of required CI workflows; the bypass contract in section 7 is authoritative. diff --git a/packages/apm-guide/.apm/skills/apm-usage/governance.md b/packages/apm-guide/.apm/skills/apm-usage/governance.md index 71888ac718..b3f09f7230 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/governance.md +++ b/packages/apm-guide/.apm/skills/apm-usage/governance.md @@ -429,11 +429,10 @@ closest `apm-policy` (e.g. `acme/dept-a/team-x/apm-policy` before `acme/dept-a/apm-policy` before `acme/apm-policy`), so a team can scope its own policy under a subgroup; a team policy can `extends:` an ancestor group's policy to inherit it. A flat `/` remote probes only `/apm-policy`. -Because GitLab 404s a private project the token cannot read, before applying an -ancestor policy over a skipped closer level APM confirms via authenticated Git -that the closer `apm-policy` project does not exist; if it does, discovery fails -closed unconditionally (blocking install) rather than downgrade to the weaker -ancestor -- grant the token read access to every `apm-policy` it should honour. +GitLab 404s a private project the token cannot read, so a closer `apm-policy` the +token is denied is indistinguishable from "no policy here" and is skipped in +favour of the next ancestor (same `404 == no policy` behaviour as GitHub/ADO) -- +grant the token read access to every `apm-policy` it should honour. Configure a self-managed host with `GITLAB_HOST` or `APM_GITLAB_HOSTS`, and use `APM_GITLAB_POLICY_REPO` to select another project name. Repositories with no detectable git remote (unpacked diff --git a/src/apm_cli/policy/_gitlab.py b/src/apm_cli/policy/_gitlab.py index 73d5fb5294..088860a68c 100644 --- a/src/apm_cli/policy/_gitlab.py +++ b/src/apm_cli/policy/_gitlab.py @@ -110,113 +110,6 @@ def _probe(_token: str | None, git_env: dict[str, str]) -> bool | None: return None -def first_concealed_closer_policy( - skipped_namespaces: list[str], - repo: str, - *, - host: str, - port: int | None, - project_root: Path | None = None, - no_cache: bool = False, - cache_only: bool = False, -) -> str | None: - """Return the closest skipped namespace that must fail the walk closed. - - During the subgroup walk (see :func:`discovery._gitlab_walk_candidate`) an - ``absent`` level is ambiguous -- GitLab returns 404 both for a missing - project and for a private one the token cannot read. Before an ancestor - policy is applied over the skipped closer levels, this confirms via - authenticated Git whether any skipped closer ``apm-policy`` project actually - exists. It returns the closest namespace the caller must fail closed on (a - confirmed-present concealed project, or -- in ``cache_only`` mode -- one that - cannot be verified offline), or ``None`` when every skipped level is known to - be genuinely absent. ``skipped_namespaces`` is ordered closest-first. - - Only the DEFINITIVE ``present`` verdict is cached (per ``(host, namespace, - repo)`` at the policy-cache TTL): ``_gitlab_project_state_via_git`` returns - ``None`` for a missing project AND for an auth/network/timeout failure alike, - so caching an ``absent`` verdict would let a transient failure suppress - re-probing for the whole TTL and wrongly apply a weaker ancestor. Because - absence is never definitive, the common "team inherits the org policy" path - re-probes each install; that cost is accepted (see the PR trade-offs). - - ``cache_only`` (the offline local-bundle contract) never issues a network - probe: a cached ``present`` fails closed, and an unverifiable level fails - closed deterministically rather than reaching out to the network. - """ - use_cache = project_root is not None and not no_cache - for namespace in skipped_namespaces: - if use_cache and _read_concealment_verdict(project_root, host, port, namespace, repo): - return namespace # cached definitive "present" -> concealed, fail closed - if cache_only: - # Offline: no network probe. Without a cached "present" we cannot - # confirm this closer level is genuinely absent, so fail closed - # rather than silently apply a weaker ancestor. - return namespace - if _gitlab_project_state_via_git(org=namespace, repo=repo, host=host, port=port) is True: - if use_cache: - _write_concealment_verdict(project_root, host, port, namespace, repo) - return namespace - # Indeterminate (None): missing OR transient failure -- NOT cached. - return None - - -def _concealment_cache_file(project_root: Path, host: str, port: int | None, ns: str, repo: str): - """Return the cache file path for one namespace's concealment verdict.""" - from .discovery import _cache_key, _get_cache_dir - - host_label = f"{host}:{port}" if port is not None else host - key = _cache_key(f"concealment:{host_label}/{ns}/{repo}") - return _get_cache_dir(project_root) / f"concealment_{key}.json" - - -def _read_concealment_verdict( - project_root: Path, host: str, port: int | None, ns: str, repo: str -) -> bool: - """Return ``True`` iff a fresh definitive ``"present"`` verdict is cached. - - Only ``present`` is ever cached (see :func:`first_concealed_closer_policy`), - so a hit means the closer project is confirmed to exist. - """ - import json - import time - - from .discovery import CACHE_SCHEMA_VERSION, DEFAULT_CACHE_TTL - - try: - raw = _concealment_cache_file(project_root, host, port, ns, repo).read_text( - encoding="utf-8" - ) - data = json.loads(raw) - except (OSError, ValueError): - return False - if data.get("schema") != CACHE_SCHEMA_VERSION: - return False - if time.time() - float(data.get("ts", 0)) > DEFAULT_CACHE_TTL: - return False - return data.get("verdict") == "present" - - -def _write_concealment_verdict( - project_root: Path, host: str, port: int | None, ns: str, repo: str -) -> None: - """Persist the definitive ``present`` verdict (best-effort; never blocks).""" - import json - import time - - from .discovery import CACHE_SCHEMA_VERSION - - path = _concealment_cache_file(project_root, host, port, ns, repo) - try: - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text( - json.dumps({"schema": CACHE_SCHEMA_VERSION, "verdict": "present", "ts": time.time()}), - encoding="utf-8", - ) - except OSError: - return - - def _fetch_gitlab_chain_parent( parent_ref: str, *, diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index cbe2c9bff9..6392b90b32 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -993,15 +993,13 @@ def _gitlab_walk_candidate( When every level is absent the last ``absent`` result is returned so the caller's candidate cascade can proceed unchanged. - GitLab returns 404 both for a missing project and for a private project the - token cannot read, so an ``absent`` is ambiguous. Before applying an - ancestor policy over one or more skipped closer levels, verify (via - authenticated Git) that no skipped closer ``apm-policy`` project actually - exists; if one does, its policy was concealed and we fail closed rather than - silently apply the weaker ancestor. + A GitLab ``absent`` is a 404, which GitLab returns both for a missing + project and for a private one the token cannot read -- the same + "404 == no policy at this level" ambiguity GitHub/ADO discovery already + accept. A closer policy the token cannot read is therefore skipped in + favour of the next ancestor (documented as a GitLab-platform limitation). """ result = PolicyFetchResult(error=None, outcome="absent") - skipped: list[str] = [] for namespace in namespaces: result = _gitlab._fetch_from_gitlab_repo( org=namespace, @@ -1014,23 +1012,7 @@ def _gitlab_walk_candidate( cache_only=cache_only, ) if result.outcome != "absent": - # Gate on a usable policy (found/empty/cached_stale all carry one), - # not just found/empty -- a stale-but-usable ancestor must also be - # checked for a concealed closer before it is applied. - if skipped and result.policy is not None: - concealed = _gitlab.first_concealed_closer_policy( - skipped, - candidate_repo, - host=host, - port=port, - project_root=project_root, - no_cache=no_cache, - cache_only=cache_only, - ) - if concealed is not None: - return _gitlab_concealed_closer_result(concealed, candidate_repo, host, port) return result - skipped.append(namespace) logger.debug( "GitLab policy absent at %s/%s; trying parent group", namespace, @@ -1039,31 +1021,6 @@ def _gitlab_walk_candidate( return result -def _gitlab_concealed_closer_result( - namespace: str, repo: str, host: str, port: int | None -) -> PolicyFetchResult: - """Fail-closed result when a skipped closer GitLab policy project exists. - - Uses ``incomplete_chain`` -- an outcome that ALWAYS fails closed in - :func:`outcome_routing.route_discovery_outcome`, regardless of the project's - ``policy.fetch_failure_default`` -- because a concealed closer policy is a - governance ambiguity that must never silently downgrade to a weaker ancestor - (a ``cache_miss_fetch_fail`` here would default to ``warn`` and proceed). - """ - host_label = f"{host}:{port}" if port is not None else host - return PolicyFetchResult( - source=f"org:{host_label}/{namespace}/{repo}", - error=( - f"A closer GitLab policy project {namespace}/{repo} could not be confirmed " - "absent (GitLab returns 404 for a private project the token cannot read, or " - "the project state was unverifiable offline in cache-only mode); refusing to " - "silently apply a weaker ancestor policy. Grant the token read access to that " - "project, or remove it." - ), - outcome="incomplete_chain", - ) - - def _fetch_from_url( url: str, project_root: Path, diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 098f8f60ba..2584eb9b66 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -21,7 +21,6 @@ _fetch_from_gitlab_repo, _fetch_gitlab_contents, _gitlab_project_state_via_git, - first_concealed_closer_policy, ) from apm_cli.policy.discovery import ( CACHE_SCHEMA_VERSION, @@ -230,98 +229,6 @@ def test_no_namespace_segment_returns_none(self): self.assertIsNone(_gitlab_namespace_descending("https://gitlab.com/solo")) -class TestFirstConcealedCloserPolicy(unittest.TestCase): - """first_concealed_closer_policy verifies skipped closer projects (#2753).""" - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_returns_closest_namespace_git_confirms_exists(self, mock_state): - # team-x exists (concealed 404); dept-a not confirmed. - mock_state.side_effect = lambda **kw: True if kw["org"] == "acme/dept-a/team-x" else None - result = first_concealed_closer_policy( - ["acme/dept-a/team-x", "acme/dept-a"], "apm-policy", host="gitlab.com", port=None - ) - self.assertEqual(result, "acme/dept-a/team-x") - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_returns_none_when_no_skipped_project_confirmed(self, mock_state): - mock_state.return_value = None # git cannot establish any project's state - result = first_concealed_closer_policy( - ["acme/dept-a/team-x", "acme/dept-a"], "apm-policy", host="gitlab.com", port=None - ) - self.assertIsNone(result) - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_returns_closest_confirmed_when_match_is_a_middle_level(self, mock_state): - # Only the MIDDLE skipped namespace is git-confirmed to exist; the - # closest-first scan must return it, not None and not a farther level. - mock_state.side_effect = lambda **kw: True if kw["org"] == "acme/dept-a" else None - result = first_concealed_closer_policy( - ["acme/dept-a/team-x", "acme/dept-a", "acme"], - "apm-policy", - host="gitlab.com", - port=None, - ) - self.assertEqual(result, "acme/dept-a") - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_returns_the_closest_of_multiple_confirmed(self, mock_state): - # When several skipped levels exist, the CLOSEST (first) wins. - mock_state.return_value = True - result = first_concealed_closer_policy( - ["acme/dept-a/team-x", "acme/dept-a", "acme"], - "apm-policy", - host="gitlab.com", - port=None, - ) - self.assertEqual(result, "acme/dept-a/team-x") - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_present_verdict_is_cached_so_the_probe_runs_once(self, mock_state): - # Only the DEFINITIVE "present" verdict is cached: a second walk over a - # confirmed-present level does not re-probe the network. - mock_state.return_value = True - with tempfile.TemporaryDirectory() as tmpdir: - root = Path(tmpdir) - first = first_concealed_closer_policy( - ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root - ) - second = first_concealed_closer_policy( - ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root - ) - self.assertEqual(first, "acme/dept-a") - self.assertEqual(second, "acme/dept-a") - self.assertEqual(mock_state.call_count, 1) # present cached: probed once - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_indeterminate_verdict_is_not_cached_so_it_re_probes(self, mock_state): - # None (missing OR transient failure) is NOT cached -- a transient error - # must never be remembered as "absent" and suppress re-probing. - mock_state.return_value = None - with tempfile.TemporaryDirectory() as tmpdir: - root = Path(tmpdir) - for _ in range(2): - first_concealed_closer_policy( - ["acme/dept-a"], "apm-policy", host="gitlab.com", port=None, project_root=root - ) - self.assertEqual(mock_state.call_count, 2) # not cached: re-probes each time - - @patch("apm_cli.policy._gitlab._gitlab_project_state_via_git") - def test_cache_only_never_probes_and_fails_closed_when_unverifiable(self, mock_state): - # Offline (cache_only) must not touch the network; without a cached - # "present" verdict it fails closed rather than applying a weaker ancestor. - with tempfile.TemporaryDirectory() as tmpdir: - result = first_concealed_closer_policy( - ["acme/dept-a"], - "apm-policy", - host="gitlab.com", - port=None, - project_root=Path(tmpdir), - cache_only=True, - ) - self.assertEqual(result, "acme/dept-a") # unverifiable offline -> fail closed - mock_state.assert_not_called() # no network probe in cache_only - - class TestExtractOrgFromGitRemote(unittest.TestCase): """Test _extract_org_from_git_remote with mocked subprocess.""" @@ -1298,12 +1205,11 @@ def test_gitlab_subgroup_closest_wins(self, mock_extract, mock_ns, mock_gitlab_f mock_gitlab_fetch.assert_called_once() self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "acme/dept-a/team-x") - @patch("apm_cli.policy._gitlab.first_concealed_closer_policy", return_value=None) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") def test_gitlab_subgroup_absent_ascends_to_parent( - self, mock_extract, mock_ns, mock_gitlab_fetch, mock_concealed + self, mock_extract, mock_ns, mock_gitlab_fetch ): """Absent at the deepest levels -> ascend until a policy is found.""" mock_extract.return_value = ("acme", "gitlab.com", None) @@ -1325,56 +1231,6 @@ def test_gitlab_subgroup_absent_ascends_to_parent( self.assertEqual(mock_gitlab_fetch.call_count, 3) probed = [c.kwargs["org"] for c in mock_gitlab_fetch.call_args_list] self.assertEqual(probed, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) - # Skipped closer levels were verified genuinely empty before ascending. - mock_concealed.assert_called_once() - self.assertEqual(mock_concealed.call_args.args[0], ["acme/dept-a/team-x", "acme/dept-a"]) - - @patch("apm_cli.policy._gitlab.first_concealed_closer_policy") - @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") - @patch("apm_cli.policy.discovery._gitlab_namespace_descending") - @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") - def test_gitlab_subgroup_concealed_closer_policy_fails_closed( - self, mock_extract, mock_ns, mock_gitlab_fetch, mock_concealed - ): - """A closer apm-policy project confirmed to exist (concealed 404) fails - closed instead of silently applying the weaker ancestor (#2753 review).""" - mock_extract.return_value = ("acme", "gitlab.com", None) - mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] - mock_gitlab_fetch.side_effect = [ - PolicyFetchResult(outcome="absent"), # team-x: concealed 404 - PolicyFetchResult( - policy=ApmPolicy(), - source="org:gitlab.com/acme/dept-a/apm-policy", - outcome="found", - ), # dept-a: an ancestor policy - ] - # Git confirms the skipped closer project (team-x) actually exists. - mock_concealed.return_value = "acme/dept-a/team-x" - - with tempfile.TemporaryDirectory() as tmpdir: - result = _auto_discover(Path(tmpdir), no_cache=True) - - self.assertFalse(result.found) - # ``incomplete_chain`` ALWAYS fails closed (unlike cache_miss_fetch_fail, - # which defaults to warn) -- the concealed policy must never downgrade. - self.assertEqual(result.outcome, "incomplete_chain") - self.assertIn("acme/dept-a/team-x/apm-policy", result.error) - - def test_gitlab_concealed_result_blocks_even_with_fetch_failure_default_warn(self): - """The concealed-closer outcome blocks install under the DEFAULT warn - knob -- proving the fix is fail-closed, not fail-open (#2753 review).""" - from apm_cli.install.errors import PolicyViolationError - from apm_cli.policy.discovery import _gitlab_concealed_closer_result - from apm_cli.policy.outcome_routing import route_discovery_outcome - - concealed = _gitlab_concealed_closer_result("acme/dept-a", "apm-policy", "gitlab.com", None) - with self.assertRaises(PolicyViolationError): - route_discovery_outcome( - concealed, - logger=None, - fetch_failure_default="warn", - raise_blocking_errors=True, - ) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") From 4851ba7ac47dad2a99efe684704a4dc14c3a23b4 Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:52:23 -0300 Subject: [PATCH 13/14] guard: include identity extractor in remote-origin owner def set The owner registry for policy/_remote.py names _extract_org_host_port_from_git_remote as part of the canonical git-remote decision, but the static guard only counted/banned three defs -- a second identity extractor elsewhere would have passed. Add it to _REMOTE_PARSER_DEFS and bump the expected count to 4 so the guard matches the owner record (Copilot #2933). Co-Authored-By: Claude Opus 4.8 --- .../checks/install_policy_gitlab_and_bundle.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py index 06bf99a1af..de5df2b935 100644 --- a/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py +++ b/scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py @@ -119,11 +119,12 @@ def check_gitlab_policy_adapter(provider: FactsProvider) -> tuple[Violation, ... _REMOTE_PARSER_DEFS = re.compile( - r"^def (_remote_url_parts|_parse_remote_url|_git_remote_origin_url)\(" + r"^def (_remote_url_parts|_parse_remote_url|_git_remote_origin_url" + r"|_extract_org_host_port_from_git_remote)\(" ) -_REMOTE_PARSER_DEF_COUNT = 3 +_REMOTE_PARSER_DEF_COUNT = 4 _REMOTE_ORIGIN_READ_COUNT = 1 @@ -136,11 +137,12 @@ def check_policy_remote_origin_owner(provider: FactsProvider) -> tuple[Violation """Reading and parsing the project git remote for policy discovery has one owner. ``policy/_remote.py`` is the sole reader of ``git remote get-url origin`` and - the sole home of the remote-URL splitter/parsers (``_remote_url_parts``, - ``_parse_remote_url``, ``_git_remote_origin_url``). The owner MUST define all - three helpers, and no other module in the policy tree may re-read or re-parse - the remote -- either would reintroduce the double-read / divergent-parse the - single-owner refactor removed (#2753). + the sole home of the remote-URL splitter/parsers/identity extractor + (``_remote_url_parts``, ``_parse_remote_url``, ``_git_remote_origin_url``, + ``_extract_org_host_port_from_git_remote``). The owner MUST define all four, + and no other module in the policy tree may re-read or re-parse the remote -- + either would reintroduce the double-read / divergent-parse the single-owner + refactor removed (#2753). """ rule_id = RULE_REMOTE_ORIGIN_OWNER owner, owner_fail = _configured(provider, _REMOTE_MODULE, rule_id) From 97767eb9c07399fd64acbcb12489b03a373cd4ea Mon Sep 17 00:00:00 2001 From: Rafael Azevedo <34526585+rrazvd@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:54:41 -0300 Subject: [PATCH 14/14] feat(policy): surface which GitLab subgroup policy applied (panel review) Address the review panel's one REQUIRED finding plus coverage gaps for the subgroup walk (#2753). REQUIRED (devx-ux): at default verbosity in warn mode the closest-wins walk was invisible -- a project silently picking up a subgroup policy instead of the top-level one saw no output. Add a `subgroup_scoped` flag to PolicyFetchResult, set it in the walk when the winning namespace is below the top-level group, thread it through outcome_routing, and show the `Policy: ` line at info even in non-verbose warn mode when it is set. Top-level resolutions keep the pre-existing silence. Tests (test-coverage): add the positive host-pin port branch (host-qualified extends ref with a matching non-null port strips and fetches), an `empty`-outcome short-circuit at a deep subgroup (README- only apm-policy must shadow a real ancestor, not fall through), and an end-to-end wiring test that runs the real `_gitlab_namespace_descending` from a nested origin so the read-origin-once -> derive-namespaces -> walk seam is exercised. Extend the existing walk tests to assert the subgroup_scoped flag. Note: the panel's advisory to narrow the two `except Exception` clauses in _remote.py was NOT applied -- `test_https_url_parse_exception_returns_none` pins the broad catch as intentional cross-version defensiveness, so the premise (urlparse only raises ValueError) does not hold. Clarified the comment instead. Co-Authored-By: Claude Opus 4.8 --- src/apm_cli/core/command_logger.py | 12 ++- src/apm_cli/policy/_remote.py | 10 ++- src/apm_cli/policy/discovery.py | 12 +++ src/apm_cli/policy/outcome_routing.py | 2 + .../install/test_install_logger_policy.py | 31 +++++++ tests/unit/policy/test_discovery.py | 84 +++++++++++++++++++ 6 files changed, 144 insertions(+), 7 deletions(-) diff --git a/src/apm_cli/core/command_logger.py b/src/apm_cli/core/command_logger.py index 7b4a32cafa..4cff63c90c 100644 --- a/src/apm_cli/core/command_logger.py +++ b/src/apm_cli/core/command_logger.py @@ -601,11 +601,15 @@ def policy_resolved( cached: bool, enforcement: str, age_seconds: int | None = None, + subgroup_scoped: bool = False, ): """Log policy discovery outcome. Verbose by default; always shown when ``enforcement == "block"`` - (users must know blocking is active). + (users must know blocking is active) or when ``subgroup_scoped`` + is set -- a GitLab subgroup policy that closest-wins resolved + BELOW the top-level group is surprising enough that the resolved + level must name itself even at default verbosity (#2753). Format: ``[i] Policy: (cached, fetched 5m ago) -- enforcement=block`` """ @@ -629,9 +633,11 @@ def policy_resolved( if enforcement == "block": # Always visible — blocking installs is a big deal _rich_warning(message, symbol="warning") - elif self.verbose: + elif self.verbose or subgroup_scoped: + # Subgroup-scoped GitLab policy: show at info even in non-verbose + # warn mode so the dev sees which subgroup level applied (#2753). _rich_info(message, symbol="info") - # Non-verbose + non-block: silent (no noise for warn/off) + # Non-verbose + non-block + top-level: silent (no noise for warn/off) def policy_discovery_miss( self, diff --git a/src/apm_cli/policy/_remote.py b/src/apm_cli/policy/_remote.py index 9a3dadc08f..24d9118fb5 100644 --- a/src/apm_cli/policy/_remote.py +++ b/src/apm_cli/policy/_remote.py @@ -130,10 +130,12 @@ def _remote_url_parts(url: str) -> tuple[str, list[str]] | None: try: parsed = urlparse(url) except Exception: - # urlparse only raises ValueError in practice, but the legacy - # parser swallowed any exception here; preserve that. The - # ``/tfs/`` ValueError comes from parse_ado_repo_url (in - # _parse_remote_url), never from urlparse, so nothing to re-raise. + # Deliberately broad: the legacy parser swallowed ANY exception + # from ``urlparse`` here (cross-version defensiveness), and + # ``test_https_url_parse_exception_returns_none`` pins that + # contract. The ``/tfs/`` ValueError comes from + # ``parse_ado_repo_url`` (in ``_parse_remote_url``), never from + # ``urlparse`` here, so there is nothing to re-raise. return None host = parsed.hostname or "" segments = [ diff --git a/src/apm_cli/policy/discovery.py b/src/apm_cli/policy/discovery.py index 6392b90b32..aa81b99e63 100644 --- a/src/apm_cli/policy/discovery.py +++ b/src/apm_cli/policy/discovery.py @@ -228,6 +228,13 @@ class PolicyFetchResult: raw_bytes_hash: str | None = None expected_hash: str | None = None # The pin that was checked, if any + # -- Discovery-provenance fields (#2753) -- + # True when GitLab subgroup discovery resolved a policy BELOW the + # top-level group (closest-wins diverged from the legacy top-level + # probe). Surfaced at info even in non-verbose warn mode so a dev can + # see which subgroup's policy applied without needing --verbose. + subgroup_scoped: bool = False + @property def found(self) -> bool: return self.policy is not None @@ -1012,6 +1019,11 @@ def _gitlab_walk_candidate( cache_only=cache_only, ) if result.outcome != "absent": + # A namespace with more than one segment is a subgroup below the + # top-level group, so closest-wins diverged from the legacy + # top-level-only probe; flag it so the resolved level is surfaced + # to the user even at default verbosity (#2753). + result.subgroup_scoped = "/" in namespace return result logger.debug( "GitLab policy absent at %s/%s; trying parent group", diff --git a/src/apm_cli/policy/outcome_routing.py b/src/apm_cli/policy/outcome_routing.py index a562907725..f2a4c79547 100644 --- a/src/apm_cli/policy/outcome_routing.py +++ b/src/apm_cli/policy/outcome_routing.py @@ -194,6 +194,7 @@ def route_discovery_outcome( cached=True, enforcement=policy.enforcement, age_seconds=fetch_result.cache_age_seconds, + subgroup_scoped=fetch_result.subgroup_scoped, ) logger.policy_discovery_miss( outcome="cached_stale", @@ -218,6 +219,7 @@ def route_discovery_outcome( cached=fetch_result.cached, enforcement=policy.enforcement, age_seconds=fetch_result.cache_age_seconds, + subgroup_scoped=fetch_result.subgroup_scoped, ) return policy diff --git a/tests/unit/install/test_install_logger_policy.py b/tests/unit/install/test_install_logger_policy.py index 4e35eab4e2..b5be5c96af 100644 --- a/tests/unit/install/test_install_logger_policy.py +++ b/tests/unit/install/test_install_logger_policy.py @@ -347,6 +347,37 @@ def test_warn_non_verbose_silent(self, mock_info): ) mock_info.assert_not_called() + @patch("apm_cli.core.command_logger._rich_info") + def test_subgroup_scoped_visible_non_verbose_warn(self, mock_info): + """#2753: a GitLab subgroup policy (closest-wins below the top-level + group) names the resolved level at info even in non-verbose warn mode, + so the dev sees which subgroup applied without --verbose.""" + logger = InstallLogger(verbose=False) + logger.policy_resolved( + source="org:gitlab.com/acme/dept-a/apm-policy", + cached=False, + enforcement="warn", + subgroup_scoped=True, + ) + mock_info.assert_called_once() + msg = mock_info.call_args[0][0] + assert "org:gitlab.com/acme/dept-a/apm-policy" in msg + assert "enforcement=warn" in msg + assert mock_info.call_args[1].get("symbol") == "info" + + @patch("apm_cli.core.command_logger._rich_info") + def test_top_level_scope_stays_silent_non_verbose_warn(self, mock_info): + """A top-level (non-subgroup) policy keeps the pre-#2753 silence: + non-verbose + warn emits nothing when subgroup_scoped is False.""" + logger = InstallLogger(verbose=False) + logger.policy_resolved( + source="org:gitlab.com/acme/apm-policy", + cached=False, + enforcement="warn", + subgroup_scoped=False, + ) + mock_info.assert_not_called() + @patch("apm_cli.core.command_logger._rich_info") def test_off_verbose_shows_info(self, mock_info): logger = InstallLogger(verbose=True) diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 2584eb9b66..5667e49a6a 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -1204,6 +1204,9 @@ def test_gitlab_subgroup_closest_wins(self, mock_extract, mock_ns, mock_gitlab_f self.assertTrue(result.found) mock_gitlab_fetch.assert_called_once() self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "acme/dept-a/team-x") + # Won at a subgroup below the top-level group -> flagged so the + # resolved level is surfaced even at default verbosity (#2753). + self.assertTrue(result.subgroup_scoped) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") @@ -1231,6 +1234,9 @@ def test_gitlab_subgroup_absent_ascends_to_parent( self.assertEqual(mock_gitlab_fetch.call_count, 3) probed = [c.kwargs["org"] for c in mock_gitlab_fetch.call_args_list] self.assertEqual(probed, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) + # Resolved at the top-level group -> NOT subgroup-scoped, so the + # pre-#2753 default-verbosity silence is preserved for this case. + self.assertFalse(result.subgroup_scoped) @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") @patch("apm_cli.policy.discovery._gitlab_namespace_descending") @@ -1311,6 +1317,59 @@ def test_gitlab_flat_project_falls_back_to_top_level_org( mock_gitlab_fetch.assert_called_once() self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "contoso") + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy.discovery._gitlab_namespace_descending") + @patch("apm_cli.policy.discovery._extract_org_host_port_from_git_remote") + def test_gitlab_subgroup_empty_at_deep_level_shadows_ancestor( + self, mock_extract, mock_ns, mock_gitlab_fetch + ): + """An ``empty`` (present-but-no-rules) policy at a deep subgroup wins + and stops the ascent -- a README-only apm-policy at team-x is closest + and must NOT fall through to a real ancestor policy (#2753).""" + mock_extract.return_value = ("acme", "gitlab.com", None) + mock_ns.return_value = ["acme/dept-a/team-x", "acme/dept-a", "acme"] + mock_gitlab_fetch.side_effect = [ + PolicyFetchResult( + policy=ApmPolicy(), + source="org:gitlab.com/acme/dept-a/team-x/apm-policy", + outcome="empty", + ), # team-x: present but empty -> closest wins, stop + PolicyFetchResult(policy=ApmPolicy(), outcome="found"), # must NOT be reached + ] + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertEqual(result.outcome, "empty") + mock_gitlab_fetch.assert_called_once() + self.assertEqual(mock_gitlab_fetch.call_args.kwargs["org"], "acme/dept-a/team-x") + self.assertTrue(result.subgroup_scoped) + + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + @patch("apm_cli.policy._remote.subprocess.run") + def test_gitlab_subgroup_walk_wiring_from_real_remote(self, mock_run, mock_gitlab_fetch): + """End-to-end seam: a real nested origin URL flows through the actual + namespace-descending helper into the walk (no _gitlab_namespace_descending + mock), so the ``_auto_discover`` wiring -- read origin once, derive the + descending namespaces from that same URL, probe deepest-first -- is + exercised, not just each half in isolation (#2753).""" + mock_run.return_value = MagicMock( + returncode=0, + stdout="https://gitlab.com/acme/dept-a/team-x/my-project.git\n", + ) + mock_gitlab_fetch.return_value = PolicyFetchResult(outcome="absent") + + with tempfile.TemporaryDirectory() as tmpdir: + result = _auto_discover(Path(tmpdir), no_cache=True) + + self.assertEqual(result.outcome, "absent") + probed = [c.kwargs["org"] for c in mock_gitlab_fetch.call_args_list] + self.assertEqual(probed, ["acme/dept-a/team-x", "acme/dept-a", "acme"]) + # Every level targeted the project's own remote host, never a foreign one. + self.assertTrue( + all(c.kwargs["host"] == "gitlab.com" for c in mock_gitlab_fetch.call_args_list) + ) + class TestPolicyRepoCandidates(unittest.TestCase): """Test _policy_repo_candidates host profile selection.""" @@ -1468,6 +1527,31 @@ def test_gitlab_parent_accepts_host_qualified_nested_namespace(self, mock_fetch) self.assertEqual(mock_fetch.call_args.kwargs["org"], "acme/dept-a") self.assertEqual(mock_fetch.call_args.kwargs["repo"], "apm-policy") + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") + def test_gitlab_parent_accepts_host_qualified_ref_with_matching_port(self, mock_fetch): + """A host-qualified extends: ref whose ``host:port`` matches the leaf's + non-null port is stripped and fetched -- the positive side of the + ``explicit.port == port`` gate (mismatch/malformed are covered + separately). Pins that a matching self-managed port proceeds.""" + mock_fetch.return_value = PolicyFetchResult(outcome="absent") + + with ( + patch.dict(os.environ, {"GITLAB_HOST": "gitlab.example.test"}, clear=False), + tempfile.TemporaryDirectory() as tmpdir, + ): + _fetch_chain_parent( + "gitlab.example.test:8443/acme/dept-a/apm-policy", + current_source="org:gitlab.example.test:8443/acme/dept-a/team-x/apm-policy", + leaf_host="gitlab.example.test", + leaf_port=8443, + project_root=Path(tmpdir), + no_cache=True, + ) + + self.assertEqual(mock_fetch.call_args.kwargs["org"], "acme/dept-a") + self.assertEqual(mock_fetch.call_args.kwargs["repo"], "apm-policy") + self.assertEqual(mock_fetch.call_args.kwargs["port"], 8443) + @patch("apm_cli.policy._gitlab._fetch_from_gitlab_repo") def test_gitlab_parent_rejects_single_segment_reference(self, mock_fetch): """A bare single-segment extends: ref fails closed, never probes."""