diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 19ea58003f..53d0fb9b0f 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -226,15 +226,26 @@ jobs: TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number || 0 }} PULL_REQUEST_HEAD_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + # The base branch's tip SHA at event time -- already-reviewed, + # already-merged state. Threaded through so the issue #2193 + # research/data artifact path declaration can be resolved only + # from here, never from the untrusted PR head; see + # `evaluate_pull_request`'s `base_ref` parameter. + PULL_REQUEST_BASE_SHA: ${{ github.event.pull_request.base.sha || '' }} EVENT_ACTION: ${{ github.event.action || 'unknown' }} run: | set -euo pipefail + base_ref_args=() + if [ -n "$PULL_REQUEST_BASE_SHA" ]; then + base_ref_args=(--base-ref "$PULL_REQUEST_BASE_SHA") + fi python3 .cwl-required-source/scripts/ci/pingora_edge_policy.py \ --repository "$TARGET_REPOSITORY" \ --pull-request "$PULL_REQUEST_NUMBER" \ --head-sha "$PULL_REQUEST_HEAD_SHA" \ --event-action "$EVENT_ACTION" \ - --api-url "https://api.github.com" + --api-url "https://api.github.com" \ + "${base_ref_args[@]}" admit-current-head: name: admit-current-head diff --git a/CHANGELOG.d/20260914-pingora-declared-artifact-paths.md b/CHANGELOG.d/20260914-pingora-declared-artifact-paths.md new file mode 100644 index 0000000000..e5f375c909 --- /dev/null +++ b/CHANGELOG.d/20260914-pingora-declared-artifact-paths.md @@ -0,0 +1,3 @@ +### Pingora edge policy admits declared research/data artifact paths + +- `scripts/ci/pingora_edge_policy.py` previously admitted binary or non-UTF-8 content only by path shape (`DOCUMENTATION_DIRECTORIES` via `_is_known_documentation_path`, plus the `evidence`/`figures` publication directories from #2149), so a research repository's raw data and fitted-model artefacts kept elsewhere by deliberate, owner-approved design -- e.g. `ContextualWisdomLab/late-life-anxiety-reanalysis`'s `local/` and evidence-preservation paths -- were rejected on path shape alone, with no route except relocating them under `docs/` (already done once, for 66 images) or leaving the PR unmergeable. `evaluate_pull_request` now accepts an optional `base_ref` and, when given, resolves a new `.github/edge-policy-artifact-paths.txt` declaration (one relative path prefix per line, no globs, capped at `MAX_DECLARED_ARTIFACT_PREFIXES=64` entries and `MAX_DECLARED_ARTIFACT_PREFIX_DEPTH=8` segments) **only from that base ref, never the pull-request head** -- a PR that adds or widens the declaration gets no benefit from it until that change is itself reviewed and merged, proven by a same-PR self-authorization regression test. The declaration replaces only the path-shape test: `_runtime_path_rule` matches stay rejected inside a declared prefix exactly as inside `docs/` today, and a suffix with no `BINARY_DOCUMENT_MAGIC` entry (most research-data formats have none -- `.xlsx`, `.sav`, `.rds`, `.npz`, …) is admitted only on the stricter "no diff patch + fetched bytes are not valid UTF-8" evidence, so a file that decodes as valid UTF-8 is always still content-scanned, never silently admitted. `.hwpx`/`.pdf`/`.png` under a declared prefix keep the existing structural-evidence checks. A malformed declaration (absolute path, `..`, bare `.`/`/`, a glob character, or over either bound) is a hard `PolicyError` naming the offending entry; a repository with no declaration file at all behaves identically to before this feature existed. `evaluate_pull_request` now also emits a `::notice::` naming the declared prefix and the base ref it came from whenever a declared-prefix admission occurs, so a reviewer can trace it back to the reviewed declaration. `.github/workflows/opencode-review.yml`'s `pull_request_target`-derived `github.event.pull_request.base.sha` is threaded through as `--base-ref` with no new permissions. `tests/test_pingora_edge_policy.py` adds coverage for base-ref admission, the self-authorization refusal, runtime-form and valid-UTF-8 rejection inside a declared prefix, every malformed-declaration shape, and the no-declaration regression guard; `tests/test_pingora_edge_workflow_contract.py` pins the new workflow wiring. `pingora_edge_policy.py` remains 100% branch coverage and 100% `interrogate` docstring coverage. Refs #2193, #2149, #2116. diff --git a/docs/policies/PINGORA_EDGE_POLICY.md b/docs/policies/PINGORA_EDGE_POLICY.md index 619374a13d..e7fd78c563 100644 --- a/docs/policies/PINGORA_EDGE_POLICY.md +++ b/docs/policies/PINGORA_EDGE_POLICY.md @@ -63,6 +63,59 @@ This is a bounded binary-evidence classifier, not a general image renderer; visual fidelity and optional ancillary-chunk semantics are outside this gate. Other binary files remain unavailable evidence and fail closed. +## Declared research/data artifact paths + +The scanner's binary exemption is otherwise shaped by path only (`doc`/ +`docs`/`documentation`, plus the `evidence`/`figures` publication +directories). A research repository whose raw data and fitted-model +artefacts live elsewhere by deliberate, owner-approved design -- SPSS +`.sav` files, serialized model objects, compressed numeric arrays -- can +opt in without relocating that data under `docs/`. + +Add `.github/edge-policy-artifact-paths.txt` at the repository root: one +explicit relative path prefix per non-blank line, no globs or wildcards. +For example: + +``` +local +evidence/raw +``` + +**Security property.** `evaluate_pull_request` resolves this file only +from the pull request's *base ref* -- never its head. A pull request that +adds or widens the declaration is not self-authorizing: it gets no benefit +from that change until the change itself is reviewed and merged into the +base branch. This mirrors how the required workflow already treats every +other piece of policy evidence -- current-head content only, no +pull-request-controlled trust. + +**What the declaration replaces, and what it does not.** A file under a +declared prefix is admitted on exactly the same evidence documentation +paths already require: `_runtime_path_rule` matches (`Dockerfile`, +`nginx.conf`, service files, and the like) are rejected inside a declared +prefix exactly as inside `docs/` today, and any file that decodes as valid +UTF-8 is still fully content-scanned, never silently admitted. A file whose +suffix has a known magic byte (`.hwpx`, `.pdf`, `.png`) is verified by that +format's structural evidence; a file with no known magic entry (most +research-data formats) is admitted only on the stricter combination of "no +diff patch" and "the fetched bytes are not valid UTF-8" -- a text file can +never be mistaken for a binary artefact merely by sitting under a declared +prefix. + +**Bounds.** The declaration is capped at 64 entries and 8 path segments of +depth per entry (`MAX_DECLARED_ARTIFACT_PREFIXES` / +`MAX_DECLARED_ARTIFACT_PREFIX_DEPTH` in `scripts/ci/pingora_edge_policy.py`) +-- parsing-safety bounds, not a product limit on how many locations a +repository may declare. An absolute path, a `..` traversal component, a +bare `.`/`/`, or a glob character in any entry is a hard `PolicyError` +naming the offending entry; a repository with no declaration file behaves +identically to before this feature existed. When a declared prefix admits a +file, the required workflow logs a `::notice::` naming the prefix and the +base ref the declaration was read from, so a reviewer can trace the +admission back to the reviewed declaration it relied on. + +Refs #2193, #2149, #2116. + ## Exception process There is no standing Nginx exception. A temporary exception requires a public ADR diff --git a/scripts/ci/pingora_edge_policy.py b/scripts/ci/pingora_edge_policy.py index eb0e3a741a..220c6dda02 100644 --- a/scripts/ci/pingora_edge_policy.py +++ b/scripts/ci/pingora_edge_policy.py @@ -5,6 +5,35 @@ bounded UTF-8 file content through the GitHub REST API, then rejects active Nginx runtime artifacts while allowing documentation, license text, and source-level negative test fixtures. + +Issue #2193 -- declared research/data artifact paths: a consumer repository may +declare literal path prefixes (``ARTIFACT_PATH_DECLARATION_PATH``) that hold +binary research or data artefacts not shaped like documentation (raw response +workbooks, SPSS ``.sav`` files, serialized model objects, compressed numeric +arrays). That declaration is resolved *only* from the pull request's base ref, +never its head, so a pull request cannot self-authorize admission of its own +binary by adding or widening the declaration in the same diff -- see +``_load_artifact_path_declaration`` and ``evaluate_pull_request``'s ``base_ref`` +parameter. The declaration replaces only the path-shape test +(`_is_known_documentation_path`'s equivalent for declared prefixes); it never +substitutes for content evidence, and an active-runtime-named file +(`_runtime_path_rule`) stays rejected inside a declared prefix exactly as inside +``docs/`` today. + +Suffix decision: most research-data formats (``.xlsx``, ``.sav``, ``.rds``, +``.npz``, ...) have no entry in ``BINARY_DOCUMENT_MAGIC``, which only knows +``.hwpx``/``.pdf``/``.png``. Rather than grow that registry for every such +format, a file under a declared prefix whose suffix has no magic entry is +admitted on the stricter complement of the UTF-8 decode this module already +performs for every ordinarily-scanned file: no diff patch available, *and* the +fetched bytes fail to decode as UTF-8. That keeps the module's central +guarantee honest -- a file that decodes as valid UTF-8 is never treated as a +binary artifact, since scanning exactly that content is what this module +exists to do -- while still admitting genuinely opaque research binaries +without maintaining an open-ended magic-byte catalog. A suffix that *does* +have a magic entry keeps that entry's existing structural evidence check +(``_is_complete_png``, ``_is_complete_hwpx``, or the raw magic-prefix check for +``.pdf``) even under a declared prefix. """ from __future__ import annotations @@ -29,6 +58,11 @@ MAX_RESPONSE_BYTES = 16_777_216 REPOSITORY_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") SHA_RE = re.compile(r"^[0-9a-f]{40}$") +# A base ref threaded into evaluate_pull_request may be either a branch name +# (e.g. "main", "release/2026.09") or a commit SHA -- whatever the calling +# workflow already has on the pull_request event without new permissions. +# Bounded charset/length, no ".." traversal, and no leading/trailing "/". +BASE_REF_RE = re.compile(r"^[A-Za-z0-9](?:[A-Za-z0-9._/-]{0,253}[A-Za-z0-9])?$") GITHUB_API_ORIGIN = "https://api.github.com" DOCUMENT_SUFFIXES = frozenset({".md", ".mdx", ".rst", ".adoc", ".txt"}) @@ -52,6 +86,20 @@ DOCUMENTATION_DIRECTORIES = frozenset({"doc", "docs", "documentation"}) DOCUMENTATION_ROOT_NAMES = frozenset({"readme", "changelog", "changes"}) +# Consumer-repository declaration of research/data artifact path prefixes +# (issue #2193). Resolved *only* from the pull request's base ref -- never +# its head -- so a PR cannot self-authorize admission of its own binary by +# adding or widening the declaration in the same diff; see +# `_load_artifact_path_declaration`. +ARTIFACT_PATH_DECLARATION_PATH = ".github/edge-policy-artifact-paths.txt" +# Parsing-safety bounds only, not a product limit on how many research/data +# artifact locations a repository may declare: they exist so a pathological +# declaration file cannot make policy evaluation walk an unbounded number of +# entries, or match against an unbounded path depth, for every changed file +# in every pull request the required workflow evaluates. +MAX_DECLARED_ARTIFACT_PREFIXES = 64 +MAX_DECLARED_ARTIFACT_PREFIX_DEPTH = 8 + RUNTIME_PATH_NAMES = frozenset({ "dockerfile", "containerfile", @@ -151,6 +199,19 @@ class ContentSizeExceededError(PolicyError): """ +class ArtifactDeclarationNotFoundError(PolicyError): + """Raised when the GitHub API reports no resource at a requested path. + + Distinguished from every other ``PolicyError`` cause via the source + HTTP 404 status specifically, so ``_load_artifact_path_declaration`` can + treat "no declaration file at this base ref" as the repository simply + not having opted into the research/data artifact-path exemption -- + identical to today's behavior -- while every other evidence failure + (malformed JSON, an invalid declared entry, a transient network error) + still fails the whole check closed exactly like any other ``PolicyError``. + """ + + OpenJson = Callable[[str, str], object] @@ -175,6 +236,85 @@ def _is_known_documentation_path(pure: PurePosixPath) -> bool: ) +def _parse_artifact_path_declaration(text: str) -> tuple[str, ...]: + """Parse a declared research/data artifact path-prefix list. + + One explicit path prefix per non-blank line; no globs or wildcards -- + every entry names a literal directory prefix, matched segment-wise by + ``_declared_prefix_for_path``. Rejects an absolute path, a ``..`` + traversal component, an empty entry, or a bare ``.``/``/``. Bounded by + ``MAX_DECLARED_ARTIFACT_PREFIXES`` (entry count) and + ``MAX_DECLARED_ARTIFACT_PREFIX_DEPTH`` (path segment depth) -- both are + parsing-safety bounds, not a product limit on how many locations a + repository may declare. A malformed entry always raises ``PolicyError`` + naming the offending entry; this never falls back to admitting nothing + or everything. + """ + + prefixes: list[str] = [] + for raw_line in text.splitlines(): + entry = raw_line.strip() + if not entry: + continue + if len(prefixes) >= MAX_DECLARED_ARTIFACT_PREFIXES: + raise PolicyError( + f"Artifact path declaration exceeds {MAX_DECLARED_ARTIFACT_PREFIXES} entries at {entry!r}" + ) + if entry.startswith("/") or entry in (".", "/"): + raise PolicyError(f"Artifact path declaration entry must be a relative path prefix: {entry!r}") + if any(char in entry for char in "*?[]"): + raise PolicyError(f"Artifact path declaration entry must not use glob syntax: {entry!r}") + parts = PurePosixPath(entry).parts + if not parts or any(part in ("", ".", "..") for part in parts): + raise PolicyError(f"Artifact path declaration entry is malformed: {entry!r}") + if len(parts) > MAX_DECLARED_ARTIFACT_PREFIX_DEPTH: + raise PolicyError( + f"Artifact path declaration entry exceeds depth {MAX_DECLARED_ARTIFACT_PREFIX_DEPTH}: {entry!r}" + ) + prefixes.append(entry) + return tuple(prefixes) + + +def _declared_prefix_for_path(path: str, declared_prefixes: Sequence[str]) -> str | None: + """Return the first declared prefix *path* falls under, else ``None``. + + Matched by path segment, not raw string prefix, so a declared ``local`` + does not also match an unrelated ``local-cache`` directory. + """ + + parts = PurePosixPath(path).parts + for prefix in declared_prefixes: + prefix_parts = PurePosixPath(prefix).parts + if parts[: len(prefix_parts)] == prefix_parts: + return prefix + return None + + +def _load_artifact_path_declaration( + *, api_url: str, repository: str, base_ref: str, token: str, opener: OpenJson +) -> tuple[str, ...]: + """Load and parse the research/data artifact path declaration at *base_ref*. + + Resolved **only** from the pull request's base ref -- never its head -- + so a pull request cannot self-authorize admission of its own binary by + adding or widening the declaration in the same diff: a PR that adds or + widens the declaration gets no benefit from it until that change is + itself reviewed and merged into the base branch. + + A declaration file absent from the base ref (HTTP 404) is not a policy + failure: it means the repository has not opted in, identical to today's + behavior before this feature existed. Every other failure to load or + parse it (malformed API shape, an invalid declared entry) still fails + the whole check closed via ``PolicyError``. + """ + + try: + content = _load_file_content(api_url, repository, ARTIFACT_PATH_DECLARATION_PATH, base_ref, token, opener) + except ArtifactDeclarationNotFoundError: + return () + return _parse_artifact_path_declaration(content) + + def _is_documentation_or_source_fixture(path: str) -> bool: """Return whether *path* is prose, license text, or scanner source fixture. @@ -214,7 +354,7 @@ def _is_documentation_or_source_fixture(path: str) -> bool: return False -def _is_binary_documentation_asset(changed: ChangedFile) -> bool: +def _is_binary_documentation_asset(changed: ChangedFile, declared_prefixes: Sequence[str] = ()) -> bool: """Return whether *changed* is a plausibly binary documentation asset. This is only the cheap, patch-presence pre-filter: GitHub's changed-files @@ -225,16 +365,34 @@ def _is_binary_documentation_asset(changed: ChangedFile) -> bool: still confirm this with ``_binary_documentation_evidence_confirms`` before trusting it; a caller without one (this module's own unit tests calling this function directly) is only checking the necessary condition. + + *declared_prefixes* (issue #2193) is the base-ref-only research/data + artifact declaration: it replaces ONLY this function's path-shape test, + never the content evidence a caller still confirms below. A file whose + suffix is a recognized ``BINARY_DOCUMENT_MAGIC`` format (``.hwpx``/ + ``.pdf``/``.png``) is admitted under a declared prefix on the exact same + format evidence documentation paths already require. A file whose + suffix has no magic entry at all (research formats such as ``.xlsx``, + ``.sav``, ``.rds``, ``.npz`` have none) can ONLY be admitted through a + declared prefix, and only on the stricter "no patch + genuinely + non-UTF-8 bytes" evidence ``_binary_documentation_evidence_confirms`` + checks for that case -- a file that decodes as valid UTF-8 must never + be treated as a binary artifact, since that is exactly the case this + scanner exists to inspect. """ - if changed.patch_available: + if changed.patch_available or _runtime_path_rule(changed.path) is not None: return False pure = PurePosixPath(changed.path) - return ( - pure.suffix.lower() in BINARY_DOCUMENT_MAGIC - and (_is_known_documentation_path(pure) or (pure.suffix.lower() == ".hwpx" and "evidence" in (part.lower() for part in pure.parts))) - and _runtime_path_rule(changed.path) is None - ) + suffix = pure.suffix.lower() + declared_prefix = _declared_prefix_for_path(changed.path, declared_prefixes) + if suffix in BINARY_DOCUMENT_MAGIC: + return ( + _is_known_documentation_path(pure) + or (suffix == ".hwpx" and "evidence" in (part.lower() for part in pure.parts)) + or declared_prefix is not None + ) + return declared_prefix is not None def _runtime_path_rule(path: str) -> str | None: @@ -307,6 +465,10 @@ def _github_open_json(url: str, token: str) -> object: with github_opener.open(request, timeout=30) as response: payload = response.read(MAX_RESPONSE_BYTES + 1) except (HTTPError, URLError, TimeoutError) as exc: + if isinstance(exc, HTTPError) and exc.code == 404: + raise ArtifactDeclarationNotFoundError( + f"GitHub API reported no resource for policy evidence at {url}" + ) from exc raise PolicyError(f"GitHub API request failed for policy evidence: {type(exc).__name__}") from exc if len(payload) > MAX_RESPONSE_BYTES: raise PolicyError("GitHub API policy response exceeded the bounded response size") @@ -382,10 +544,16 @@ def _load_raw_file_bytes(api_url: str, repository: str, path: str, head_sha: str ``encoding: "none"`` with an accurate ``size`` and no ``content`` at all. Both are treated as the same size-exceeded evidence; every other response shape still fails closed. + + *head_sha* is also reused, unchanged, to fetch a base-ref-scoped file + (the issue #2193 artifact-path declaration): any git ref -- a commit SHA + or a branch name -- works here, so it is URL-encoded rather than assumed + to be the hex-only pull-request head SHA ``evaluate_pull_request`` + validates separately. """ encoded_path = quote(path, safe="/") - url = f"{api_url}/repos/{repository}/contents/{encoded_path}?ref={head_sha}" + url = f"{api_url}/repos/{repository}/contents/{encoded_path}?ref={quote(head_sha, safe='')}" payload = opener(url, token) if not isinstance(payload, Mapping): raise PolicyError(f"GitHub content evidence for {path} is not an object") @@ -447,6 +615,16 @@ def _binary_documentation_evidence_confirms( malformed API response, corrupt base64, a declared size that does not match the decoded bytes) propagates and fails the whole check closed, same as for any other file that needs scanning. + + A suffix with no ``BINARY_DOCUMENT_MAGIC`` entry only reaches this + branch when ``_is_binary_documentation_asset`` admitted it through a + declared research/data artifact prefix (issue #2193), which has no + magic byte to check. That case is confirmed by the strict complement of + the UTF-8 decode ``_load_file_content`` uses for every ordinarily-scanned + file: bytes that fail to decode as UTF-8 are genuinely binary evidence; + bytes that decode cleanly are never admitted this way, so a valid-UTF-8 + file cannot be mistaken for a binary artifact merely by sitting under a + declared prefix -- it still reaches the normal content scan instead. """ try: @@ -458,6 +636,12 @@ def _binary_documentation_evidence_confirms( return _is_complete_png(raw) if suffix == ".hwpx": return _is_complete_hwpx(raw) + if suffix not in BINARY_DOCUMENT_MAGIC: + try: + raw.decode("utf-8") + except UnicodeDecodeError: + return True + return False return raw.startswith(BINARY_DOCUMENT_MAGIC[suffix]) @@ -642,18 +826,20 @@ def _is_complete_png(raw: bytes) -> bool: return False -def _needs_content_scan(changed: ChangedFile) -> bool: +def _needs_content_scan(changed: ChangedFile, declared_prefixes: Sequence[str] = ()) -> bool: """Return whether a changed final file can carry an active edge runtime. A claimed binary documentation asset (``_is_binary_documentation_asset``) exempts here on the cheap, offline pre-filter alone; ``evaluate_pull_request`` never actually relies on that -- it runs ``_binary_documentation_evidence_confirms`` - for that case before this function is even consulted. + for that case before this function is even consulted. *declared_prefixes* + is the base-ref-only research/data artifact declaration from issue + #2193; it is passed straight through to ``_is_binary_documentation_asset``. """ if changed.status == "removed" or _is_documentation_or_source_fixture(changed.path): return False - if _is_binary_documentation_asset(changed): + if _is_binary_documentation_asset(changed, declared_prefixes): return False if not changed.patch_available: return True @@ -675,9 +861,20 @@ def evaluate_pull_request( head_sha: str, event_action: str, token: str, + base_ref: str | None = None, opener: OpenJson = _github_open_json, ) -> tuple[Violation, ...]: - """Evaluate one pull request without checking out or executing its content.""" + """Evaluate one pull request without checking out or executing its content. + + *base_ref* (issue #2193) is an optional pull-request base ref -- a + branch name or a commit SHA, whatever the calling workflow already has + on the ``pull_request`` event without new permissions. When given, the + research/data artifact path declaration at ``ARTIFACT_PATH_DECLARATION_PATH`` + is resolved from that ref (never from ``head_sha``) and its declared + prefixes are admitted on the same content-evidence terms as documentation + paths. Omitting it (the default) reproduces this module's exact prior + behavior: no declared prefixes, no declaration fetch at all. + """ if event_action == "closed": return () @@ -689,9 +886,18 @@ def evaluate_pull_request( raise PolicyError("Pull-request head SHA is malformed") if not token: raise PolicyError("GITHUB_TOKEN is required for policy evidence") - changed_files = _load_changed_files(api_url.rstrip("/"), repository, pull_request, token, opener) + if base_ref is not None and (".." in base_ref or not BASE_REF_RE.fullmatch(base_ref)): + raise PolicyError("Pull-request base ref is malformed") + resolved_api_url = api_url.rstrip("/") + declared_prefixes: tuple[str, ...] = () + if base_ref is not None: + declared_prefixes = _load_artifact_path_declaration( + api_url=resolved_api_url, repository=repository, base_ref=base_ref, token=token, opener=opener + ) + changed_files = _load_changed_files(resolved_api_url, repository, pull_request, token, opener) violations: list[Violation] = [] for changed in changed_files: + declared_prefix = _declared_prefix_for_path(changed.path, declared_prefixes) # A claimed binary documentation asset gets its own network-verified # check ahead of _needs_content_scan's patch-presence-only signal: # a missing patch does not by itself prove binary content (GitHub @@ -701,23 +907,39 @@ def evaluate_pull_request( # content genuinely exceeds the Contents API's size ceiling. A # removed file has no head content to fetch at all -- _needs_content_scan # already special-cases this the same way for every other file. - if changed.status != "removed" and _is_binary_documentation_asset(changed): + if changed.status != "removed" and _is_binary_documentation_asset(changed, declared_prefixes): if _binary_documentation_evidence_confirms( changed, - api_url=api_url.rstrip("/"), + api_url=resolved_api_url, repository=repository, head_sha=head_sha, token=token, opener=opener, ): + if declared_prefix is not None: + # Names the reviewed declaration this admission relied + # on, so a reviewer can trace it back to the base ref. + print(_declared_prefix_notice(changed.path, declared_prefix, base_ref)) continue - elif not _needs_content_scan(changed): + elif not _needs_content_scan(changed, declared_prefixes): continue - content = _load_file_content(api_url.rstrip("/"), repository, changed.path, head_sha, token, opener) + content = _load_file_content(resolved_api_url, repository, changed.path, head_sha, token, opener) violations.extend(scan_content(changed.path, content)) return tuple(violations) +def _declared_prefix_notice(path: str, prefix: str, base_ref: str) -> str: + """Render one bounded GitHub workflow notice for a declared-prefix admission.""" + + escaped_path = path.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A").replace(",", "%2C") + message = ( + f"CWL edge policy admitted a research/data artifact under declared prefix " + f"'{prefix}' (declaration read from base ref '{base_ref}')" + ) + message = message.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + return f"::notice file={escaped_path}::{message}" + + def _annotation(violation: Violation) -> str: """Render one bounded GitHub workflow command annotation.""" @@ -736,6 +958,15 @@ def build_parser() -> argparse.ArgumentParser: parser.add_argument("--head-sha", required=True) parser.add_argument("--event-action", required=True) parser.add_argument("--api-url", default=GITHUB_API_ORIGIN) + parser.add_argument( + "--base-ref", + default=None, + help=( + "Pull-request base ref (branch name or commit SHA) used to resolve the " + "issue #2193 research/data artifact path declaration. Omit to disable " + "that declaration entirely (identical to this module's prior behavior)." + ), + ) return parser @@ -752,6 +983,7 @@ def main(argv: Sequence[str] | None = None, environ: Mapping[str, str] | None = head_sha=args.head_sha, event_action=args.event_action, token=env.get("GITHUB_TOKEN", ""), + base_ref=args.base_ref, ) except PolicyError as exc: print(f"::error::Pingora edge policy could not establish complete evidence: {exc}") diff --git a/tests/test_pingora_edge_policy.py b/tests/test_pingora_edge_policy.py index c5d4e9d7a3..c393972cd5 100644 --- a/tests/test_pingora_edge_policy.py +++ b/tests/test_pingora_edge_policy.py @@ -460,6 +460,241 @@ def opener(url: str, _token: str) -> object: ) +def _declaration_url_fragment(base_ref: str) -> str: + """Return the substring identifying the declaration-fetch request URL.""" + return f"/contents/{policy.ARTIFACT_PATH_DECLARATION_PATH}?ref={base_ref}" + + +def test_declared_prefix_from_base_ref_admits_a_real_binary_artifact(capsys: pytest.CaptureFixture[str]) -> None: + """A base-ref-declared prefix admits a genuine non-UTF-8 research artifact. + + ``local/model.npz`` has no ``BINARY_DOCUMENT_MAGIC`` entry, so admission + depends entirely on the declared prefix plus the "no patch + not valid + UTF-8" evidence -- the option (a) suffix decision from issue #2193. + """ + + artifact_bytes = b"\x93NUMPY\x01\x00\xff\xfe\x00\x01\x02\x80\x81\x82\xf0\x0f" + with pytest.raises(UnicodeDecodeError): + artifact_bytes.decode("utf-8") + + def opener(url: str, _token: str) -> object: + if "/pulls/2193/files" in url: + return [{"filename": "local/model.npz", "status": "added"}] + if _declaration_url_fragment("main") in url: + return encoded_file("\nlocal\n\n") + assert "/contents/local/model.npz" in url + return { + "type": "file", "encoding": "base64", "size": len(artifact_bytes), + "content": base64.b64encode(artifact_bytes).decode("ascii"), + } + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2193, + head_sha="a" * 40, + event_action="opened", + token="token", + base_ref="main", + opener=opener, + ) + assert result == () + notice = capsys.readouterr().out + assert "declared prefix 'local'" in notice + assert "base ref 'main'" in notice + assert "local/model.npz" in notice + + +def test_same_pr_self_authorization_is_refused() -> None: + """A declaration added only at the PR head grants no admission. + + The declaration is resolved *only* from ``base_ref``; when it is absent + there (the same PR adds the declaration and the binary together), the + artifact is scanned exactly as if no declaration existed anywhere, and a + genuinely non-UTF-8 file with no diff patch fails closed the same way + any other unrecognized binary format does. + """ + + artifact_bytes = b"\x93NUMPY\x01\x00\xff\xfe\x00\x01\x02\x80\x81\x82\xf0\x0f" + + def opener(url: str, _token: str) -> object: + if "/pulls/2194/files" in url: + return [{"filename": "local/model.npz", "status": "added"}] + if _declaration_url_fragment("main") in url: + raise policy.ArtifactDeclarationNotFoundError("no declaration at base ref") + assert "/contents/local/model.npz" in url + return { + "type": "file", "encoding": "base64", "size": len(artifact_bytes), + "content": base64.b64encode(artifact_bytes).decode("ascii"), + } + + with pytest.raises(policy.PolicyError, match="not valid UTF-8"): + policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2194, + head_sha="b" * 40, + event_action="opened", + token="token", + base_ref="main", + opener=opener, + ) + + +def test_runtime_form_under_declared_prefix_is_still_rejected() -> None: + """A declared prefix cannot launder an active Nginx runtime artifact.""" + + def opener(url: str, _token: str) -> object: + if "/pulls/2195/files" in url: + return [{"filename": "local/nginx.conf", "status": "added", "patch": "+listen 80;"}] + if _declaration_url_fragment("main") in url: + return encoded_file("local\n") + assert "/contents/local/nginx.conf" in url + return encoded_file("server { listen 80; }\n") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2195, + head_sha="c" * 40, + event_action="opened", + token="token", + base_ref="main", + opener=opener, + ) + assert [item.rule for item in result] == ["nginx_runtime_artifact"] + + +def test_valid_utf8_file_under_declared_prefix_is_still_scanned() -> None: + """A declared prefix never admits a file that decodes as valid UTF-8. + + Without a diff patch, this would otherwise look like the exact binary + pre-filter shape (`patch_available=False`); the strict UTF-8 complement + in `_binary_documentation_evidence_confirms` refuses to trust it, so it + falls through to the ordinary scan and still gets flagged. + """ + + def opener(url: str, _token: str) -> object: + if "/pulls/2196/files" in url: + return [{"filename": "local/notes.dat", "status": "added"}] + if _declaration_url_fragment("main") in url: + return encoded_file("local\n") + assert "/contents/local/notes.dat" in url + return encoded_file("cat /etc/nginx/nginx.conf\n") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2196, + head_sha="d" * 40, + event_action="opened", + token="token", + base_ref="main", + opener=opener, + ) + assert [item.rule for item in result] == ["nginx_runtime_path"] + + +@pytest.mark.parametrize( + ("declaration_text", "message"), + [ + ("/etc/passwd\n", "must be a relative path prefix"), + ("local/../etc\n", "malformed"), + ("..\n", "malformed"), + (".\n", "must be a relative path prefix"), + ("/\n", "must be a relative path prefix"), + ("data/*.npz\n", "glob"), + ("\n".join(f"path-{index}" for index in range(policy.MAX_DECLARED_ARTIFACT_PREFIXES + 1)), "exceeds 64 entries"), + ("a/" * (policy.MAX_DECLARED_ARTIFACT_PREFIX_DEPTH + 1) + "b\n", "exceeds depth 8"), + ], +) +def test_malformed_declaration_raises_naming_the_offending_entry(declaration_text: str, message: str) -> None: + """Every malformed declaration shape is a hard PolicyError, never silent.""" + + with pytest.raises(policy.PolicyError, match=message): + policy._parse_artifact_path_declaration(declaration_text) + + +def test_no_declaration_file_present_is_a_regression_guard() -> None: + """A repository with no declaration file behaves identically to today.""" + + def opener(url: str, _token: str) -> object: + if "/pulls/2197/files" in url: + return [{"filename": "docker-compose.yml", "status": "modified", "patch": "+image: nginx"}] + if _declaration_url_fragment("main") in url: + raise policy.ArtifactDeclarationNotFoundError("no declaration file in this repository") + return encoded_file("services:\n edge:\n image: nginx:1.27-alpine\n") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2197, + head_sha="e" * 40, + event_action="opened", + token="token", + base_ref="main", + opener=opener, + ) + assert [item.rule for item in result] == ["nginx_container_image"] + + +def test_omitting_base_ref_never_fetches_a_declaration() -> None: + """The default (no ``base_ref``) reproduces this module's exact prior behavior.""" + + def opener(url: str, _token: str) -> object: + if "/pulls/2198/files" in url: + return [{"filename": "docker-compose.yml", "status": "modified", "patch": "+image: nginx"}] + assert "edge-policy-artifact-paths" not in url + return encoded_file("services:\n edge:\n image: nginx:1.27-alpine\n") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=2198, + head_sha="f" * 40, + event_action="opened", + token="token", + opener=opener, + ) + assert [item.rule for item in result] == ["nginx_container_image"] + + +def test_evaluate_pull_request_rejects_malformed_base_ref() -> None: + """A malformed base ref fails before any network access.""" + + with pytest.raises(policy.PolicyError, match="base ref"): + policy.evaluate_pull_request( + api_url="x", + repository="a/b", + pull_request=1, + head_sha="a" * 40, + event_action="opened", + token="x", + base_ref="../etc/passwd", + opener=lambda _url, _token: pytest.fail("must not open"), + ) + + +def test_declared_prefix_for_path_matches_by_path_segment() -> None: + """A declared prefix matches whole path segments, not a raw string prefix.""" + + assert policy._declared_prefix_for_path("local/model.npz", ("local",)) == "local" + assert policy._declared_prefix_for_path("local-cache/model.npz", ("local",)) is None + assert policy._declared_prefix_for_path("evidence/raw/data.sav", ("evidence/raw",)) == "evidence/raw" + assert policy._declared_prefix_for_path("evidence/other.sav", ("evidence/raw",)) is None + + +def test_github_open_json_maps_not_found_to_artifact_declaration_error(monkeypatch: pytest.MonkeyPatch) -> None: + """A 404 from the GitHub API is distinguished from every other transport failure.""" + + monkeypatch.setattr( + policy.github_opener, "open", + lambda _request, timeout: (_ for _ in ()).throw(HTTPError("x", 404, "not found", {}, BytesIO())), + ) + with pytest.raises(policy.ArtifactDeclarationNotFoundError): + policy._github_open_json("https://api.github.com/repos/a/b", "token") + + def test_png_structure_validation_fails_closed_on_malformed_chunks() -> None: """Every malformed PNG boundary returns false without parsing past bounds.""" diff --git a/tests/test_pingora_edge_workflow_contract.py b/tests/test_pingora_edge_workflow_contract.py index ad0667cc6b..2267a45970 100644 --- a/tests/test_pingora_edge_workflow_contract.py +++ b/tests/test_pingora_edge_workflow_contract.py @@ -45,3 +45,9 @@ def test_required_workflow_enforces_pingora_without_executing_pr_content() -> No assert text.index("Verify immutable central policy source") < text.index( "Enforce Cloudflare Pingora edge policy" ) + + # issue #2193: the research/data artifact path declaration must be + # resolved only from the base ref the pull_request_target event already + # carries, never from the untrusted PR head. + assert "PULL_REQUEST_BASE_SHA: ${{ github.event.pull_request.base.sha || '' }}" in text + assert "--base-ref" in text