diff --git a/scripts/ci/pingora_edge_policy.py b/scripts/ci/pingora_edge_policy.py index 706fe69fc1..648bfd064b 100644 --- a/scripts/ci/pingora_edge_policy.py +++ b/scripts/ci/pingora_edge_policy.py @@ -29,6 +29,16 @@ GITHUB_API_ORIGIN = "https://api.github.com" DOCUMENT_SUFFIXES = frozenset({".md", ".mdx", ".rst", ".adoc", ".txt"}) +# Opaque binary document formats that cannot embed an interpretable, active +# Nginx runtime artifact (unlike a text config, script, or container image +# reference). Without this, any such file placed under a documentation +# directory still falls through to `_needs_content_scan` -> `True` (binary +# files never carry a GitHub diff `patch`), and then `_load_file_content` +# fails closed with a `PolicyError` for any instance over the Contents API's +# 1 MiB base64 ceiling -- rejecting a legitimate research-paper citation +# (this org's own "attach the relevant paper PDF" convention) for a reason +# that has nothing to do with the Nginx runtime policy this module enforces. +BINARY_DOCUMENT_SUFFIXES = frozenset({".pdf"}) SOURCE_TEST_SUFFIXES = frozenset({".py", ".pyi", ".js", ".mjs", ".cjs", ".ts", ".tsx", ".rs"}) LICENSE_NAMES = frozenset({"license", "license.md", "copying", "copyrights", "notice"}) DOCUMENTATION_DIRECTORIES = frozenset({"doc", "docs", "documentation"}) @@ -120,6 +130,19 @@ class PolicyError(RuntimeError): """Raised when policy evidence cannot be collected or validated safely.""" +class ContentSizeExceededError(PolicyError): + """Raised when a well-formed Contents API response exceeds MAX_FILE_BYTES. + + Distinct from every other ``PolicyError`` cause (a malformed response, a + non-file/non-base64 entry, corrupt base64, a declared size that does not + match the decoded bytes) so a caller can choose to trust a narrow, + path-scoped convention -- a genuinely oversized documentation PDF, the + one case this module cannot verify by content at all -- instead of + failing the whole check closed. Every other content-evidence failure + still fails closed exactly as before. + """ + + OpenJson = Callable[[str, str], object] @@ -134,21 +157,47 @@ def redirect_request(self, *_args: object, **_kwargs: object) -> None: github_opener = build_opener(NoRedirectHandler()) -def _is_documentation_or_source_fixture(path: str) -> bool: - """Return whether *path* is prose, license text, or scanner source fixture.""" +def _is_known_documentation_path(pure: PurePosixPath) -> bool: + """Return whether *pure* sits in a recognized documentation location.""" - pure = PurePosixPath(path) - lower_name = pure.name.lower() stem = pure.stem.lower() - is_known_documentation_path = pure.parts and ( + return bool(pure.parts) and ( any(part.lower() in DOCUMENTATION_DIRECTORIES for part in pure.parts) or (len(pure.parts) == 1 and stem in DOCUMENTATION_ROOT_NAMES) ) + + +def _is_documentation_or_source_fixture(path: str) -> bool: + """Return whether *path* is prose, license text, or scanner source fixture. + + Textual suffixes only: a ``.pdf`` is handled separately by + ``_is_binary_documentation_pdf`` and gated on GitHub reporting no diff + ``patch`` for it, so a textual file merely named with a ``.pdf`` suffix + (one GitHub *can* diff, meaning it could carry inspectable content) is + never exempted here. + + ``tests/test_pingora_edge_policy.py`` is exempted the same way this + module's own source is: a scanner's regression suite necessarily + contains the denied Nginx runtime forms it verifies detection of as + fixture strings, so a PR whose diff to that file happens to add a line + matching a ``CONTENT_RULES`` pattern (triggering `_needs_content_scan`'s + "nginx" in the patch heuristic) does not then get the file's *entire* + content -- full of intentional denied forms throughout -- scanned and + rejected. A ``.py`` test file cannot itself be deployed as an active + Nginx runtime artifact, unlike the config/Dockerfile/service forms this + policy actually guards against. + """ + + pure = PurePosixPath(path) + lower_name = pure.name.lower() if lower_name in LICENSE_NAMES or ( - is_known_documentation_path and pure.suffix.lower() in DOCUMENT_SUFFIXES + _is_known_documentation_path(pure) and pure.suffix.lower() in DOCUMENT_SUFFIXES ): return True - if pure.as_posix() == "scripts/ci/pingora_edge_policy.py": + if pure.as_posix() in ( + "scripts/ci/pingora_edge_policy.py", + "tests/test_pingora_edge_policy.py", + ): return True lower_parts = tuple(part.lower() for part in pure.parts) is_tests_fixture = len(lower_parts) >= 2 and lower_parts[:2] == ("tests", "fixtures") @@ -157,6 +206,28 @@ def _is_documentation_or_source_fixture(path: str) -> bool: return False +def _is_binary_documentation_pdf(changed: ChangedFile) -> bool: + """Return whether *changed* is a plausibly binary documentation PDF. + + This is only the cheap, patch-presence pre-filter: GitHub's changed-files + API never returns a diff ``patch`` for a true binary file, so a missing + ``patch`` is *necessary* but not *sufficient* evidence -- GitHub also + omits one for a textual diff that merely exceeds its own rendering + limit. A caller with network access (``evaluate_pull_request``) must + still confirm this with ``_pdf_evidence_confirms_binary`` before + trusting it; a caller without one (this module's own unit tests calling + this function directly) is only checking the necessary condition. + """ + + if changed.patch_available: + return False + pure = PurePosixPath(changed.path) + return ( + pure.suffix.lower() in BINARY_DOCUMENT_SUFFIXES + and _is_known_documentation_path(pure) + ) + + def _runtime_path_rule(path: str) -> str | None: """Return a path-level violation rule for active Nginx runtime artifacts.""" @@ -274,37 +345,110 @@ def _load_changed_files(api_url: str, repository: str, pull_request: int, token: raise PolicyError("GitHub changed-file pagination exceeded 3,000 files") -def _load_file_content(api_url: str, repository: str, path: str, head_sha: str, token: str, opener: OpenJson) -> str: - """Load one final head file as bounded UTF-8 text from the Contents API.""" +def _load_raw_file_bytes(api_url: str, repository: str, path: str, head_sha: str, token: str, opener: OpenJson) -> bytes: + """Load one final head file's raw decoded bytes from the Contents API. + + Raises ``ContentSizeExceededError`` specifically when the declared size + is a well-formed positive integer over ``MAX_FILE_BYTES`` -- a signal a + caller may treat differently from every other, genuinely malformed + response shape, which always raises the base ``PolicyError`` instead. + + GitHub's Contents API returns two distinct shapes for a file it cannot + inline: some responses still report ``encoding: "base64"`` with a + ``size`` over the inline-content ceiling and empty/absent ``content``; + for files whose blob exceeds that ceiling, GitHub instead reports + ``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. + """ encoded_path = quote(path, safe="/") url = f"{api_url}/repos/{repository}/contents/{encoded_path}?ref={head_sha}" payload = opener(url, token) if not isinstance(payload, Mapping): raise PolicyError(f"GitHub content evidence for {path} is not an object") - if payload.get("type") != "file" or payload.get("encoding") != "base64": + if payload.get("type") != "file": + raise PolicyError(f"GitHub content evidence for {path} is not a regular file") + encoding = payload.get("encoding") + declared_size = payload.get("size") + if encoding == "none": + if isinstance(declared_size, int) and declared_size > MAX_FILE_BYTES: + raise ContentSizeExceededError(f"GitHub content evidence for {path} exceeds the size contract") + raise PolicyError(f"GitHub content evidence for {path} has no inline content and no verifiable oversized size") + if encoding != "base64": raise PolicyError(f"GitHub content evidence for {path} is not a regular base64 file") encoded = payload.get("content") - declared_size = payload.get("size") - if not isinstance(encoded, str) or not isinstance(declared_size, int) or declared_size < 0 or declared_size > MAX_FILE_BYTES: - raise PolicyError(f"GitHub content evidence for {path} exceeds or violates the size contract") + if not isinstance(encoded, str) or not isinstance(declared_size, int) or declared_size < 0: + raise PolicyError(f"GitHub content evidence for {path} has a malformed size or content field") + if declared_size > MAX_FILE_BYTES: + raise ContentSizeExceededError(f"GitHub content evidence for {path} exceeds the size contract") try: raw = base64.b64decode("".join(encoded.split()), validate=True) except (ValueError, TypeError) as exc: raise PolicyError(f"GitHub content evidence for {path} is invalid base64") from exc if len(raw) != declared_size: raise PolicyError(f"GitHub content evidence for {path} has a size mismatch") + return raw + + +def _load_file_content(api_url: str, repository: str, path: str, head_sha: str, token: str, opener: OpenJson) -> str: + """Load one final head file as bounded UTF-8 text from the Contents API.""" + + raw = _load_raw_file_bytes(api_url, repository, path, head_sha, token, opener) try: return raw.decode("utf-8") except UnicodeDecodeError as exc: raise PolicyError(f"Runtime policy candidate {path} is not valid UTF-8") from exc +_PDF_MAGIC_PREFIX = b"%PDF-" + + +def _pdf_evidence_confirms_binary( + changed: ChangedFile, + *, + api_url: str, + repository: str, + head_sha: str, + token: str, + opener: OpenJson, +) -> bool: + """Return whether a claimed binary documentation PDF is genuinely binary. + + A missing diff ``patch`` alone is not proof of binary content: GitHub + also omits a patch for a textual diff that exceeds its own rendering + limit, well under this module's ``MAX_FILE_BYTES`` content-fetch + ceiling. Whenever the file's raw bytes can be fetched at all, this + verifies the real ``%PDF-`` magic prefix instead of trusting + patch-presence alone. Only a file whose content evidently exceeds the + Contents API's size ceiling -- the exact case ``_is_binary_documentation_pdf`` + exists for, a cited, large research paper -- falls back to trusting the + path+suffix convention; every other content-evidence failure (a + 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. + """ + + try: + raw = _load_raw_file_bytes(api_url, repository, changed.path, head_sha, token, opener) + except ContentSizeExceededError: + return True + return raw.startswith(_PDF_MAGIC_PREFIX) + + def _needs_content_scan(changed: ChangedFile) -> bool: - """Return whether a changed final file can carry an active edge runtime.""" + """Return whether a changed final file can carry an active edge runtime. + + A claimed binary documentation PDF (``_is_binary_documentation_pdf``) + exempts here on the cheap, offline pre-filter alone; ``evaluate_pull_request`` + never actually relies on that -- it runs ``_pdf_evidence_confirms_binary`` + for that case before this function is even consulted. + """ if changed.status == "removed" or _is_documentation_or_source_fixture(changed.path): return False + if _is_binary_documentation_pdf(changed): + return False if not changed.patch_available: return True if _runtime_path_rule(changed.path) is not None: @@ -342,7 +486,26 @@ def evaluate_pull_request( changed_files = _load_changed_files(api_url.rstrip("/"), repository, pull_request, token, opener) violations: list[Violation] = [] for changed in changed_files: - if not _needs_content_scan(changed): + # A claimed binary documentation PDF 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 + # also omits one for an oversized textual diff), so this confirms + # the real %PDF- magic prefix whenever the bytes can be fetched at + # all, falling back to the path+suffix convention only when the + # 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_pdf(changed): + if _pdf_evidence_confirms_binary( + changed, + api_url=api_url.rstrip("/"), + repository=repository, + head_sha=head_sha, + token=token, + opener=opener, + ): + continue + elif not _needs_content_scan(changed): continue content = _load_file_content(api_url.rstrip("/"), repository, changed.path, head_sha, token, opener) violations.extend(scan_content(changed.path, content)) diff --git a/tests/test_pingora_edge_policy.py b/tests/test_pingora_edge_policy.py index 584e540749..26434207cb 100644 --- a/tests/test_pingora_edge_policy.py +++ b/tests/test_pingora_edge_policy.py @@ -61,6 +61,7 @@ def test_scan_content_allows_prose_license_and_source_negative_fixtures() -> Non assert policy.scan_content("docs/migration.md", sample) == () assert policy.scan_content("COPYING", sample) == () assert policy.scan_content("scripts/ci/pingora_edge_policy.py", sample) == () + assert policy.scan_content("tests/test_pingora_edge_policy.py", sample) == () assert policy.scan_content("tests/fixtures/policy_samples.py", sample) == () assert policy.scan_content("tests/fixtures/negative_fixture.rs", sample) == () assert policy.scan_content("deploy/fixtures/runtime.yaml", sample) @@ -72,12 +73,74 @@ def test_scan_content_allows_prose_license_and_source_negative_fixtures() -> Non ) +def test_this_test_files_own_content_is_exempt() -> None: + """This file's own fixture strings (denied Nginx forms) must never self-trip. + + Regression coverage for a real required-workflow-bootstrap failure: a + diff to this file that happens to add a line matching a CONTENT_RULES + pattern (e.g. a new test fixture containing "/etc/nginx/") triggers + _needs_content_scan's "nginx" in the patch heuristic, which then scans + this file's *entire* current content -- full of intentional denied + forms by design -- unless this exact path is self-exempted the same way + scripts/ci/pingora_edge_policy.py already is. + """ + + own_content = Path(__file__).read_text(encoding="utf-8") + assert policy.scan_content("tests/test_pingora_edge_policy.py", own_content) == () + + def test_nested_documentation_path_allows_prose_samples() -> None: """Documentation directories remain exempt when nested below a package.""" assert policy.scan_content("packages/component/docs/migration.md", fixture_text()) == () +def test_needs_content_scan_exempts_documentation_pdfs() -> None: + """A cited research-paper PDF under docs/ never reaches content scanning. + + Binary files never carry a GitHub diff `patch`, so without this exemption + `_needs_content_scan` falls through to its `not patch_available` branch and + always returns True for a PDF -- and any such file over the Contents API's + 1 MiB base64 ceiling then fails closed in `_load_file_content` for a + reason unrelated to the Nginx runtime policy this module enforces (see + this org's "attach the relevant paper PDF under docs/papers/" convention). + """ + + changed = policy.ChangedFile + assert not policy._needs_content_scan( + changed("docs/papers/helm-holistic-evaluation-2211.09110.pdf", "added", "", patch_available=False) + ) + assert not policy._needs_content_scan( + changed("docs/papers/README.md", "modified", "", patch_available=False) + ) + # A PDF outside a recognized documentation directory is not exempted -- + # only prose/paper locations are trusted to be inert. + assert policy._needs_content_scan( + changed("scripts/ci/payload.pdf", "added", "", patch_available=False) + ) + + +def test_needs_content_scan_still_inspects_a_textual_pdf_with_a_patch() -> None: + """A '.pdf'-suffixed file GitHub *can* diff is not the binary case exempted. + + GitHub never returns a diff `patch` for a true binary file, so + `patch_available=True` here means this file is textual despite its + suffix -- exactly the case that could smuggle an active Nginx runtime + artifact under a docs/ path if the PDF exemption were suffix-only rather + than gated on patch availability. + """ + + changed = policy.ChangedFile + assert policy._needs_content_scan( + changed( + "docs/papers/not-really-a-pdf.pdf", + "added", + "+load_module modules/ngx_http_nginx_module.so;", + patch_available=True, + ) + ) + + @pytest.mark.parametrize("directory", ["testing", "contests", "assert", "my_tests"]) def test_scan_content_does_not_treat_test_name_substrings_as_fixtures( directory: str, @@ -232,6 +295,121 @@ def opener(url: str, _token: str) -> object: assert [item.rule for item in result] == ["nginx_container_image"] +def test_evaluate_pull_request_exempts_an_oversized_documentation_pdf() -> None: + """A genuinely oversized documentation PDF still cannot be verified by content. + + GitHub's real Contents API response for a file whose blob exceeds the + inline-content ceiling reports ``encoding: "none"`` with an accurate + ``size`` and no ``content`` at all (not a ``base64``-encoded entry with + an oversized declared size) -- this is that real shape, not a synthetic + one, per Devin Review's finding that the earlier version of this test + used a response shape GitHub never actually returns. This is the one + case that still falls back to the path+suffix convention -- the real + research-paper-citation use case this whole exemption exists for. + """ + + def opener(url: str, _token: str) -> object: + if "/pulls/11/files" in url: + return [ + {"filename": "docs/papers/big-paper.pdf", "status": "added"}, + ] + assert "/contents/docs/papers/big-paper.pdf" in url + return {"type": "file", "encoding": "none", "size": policy.MAX_FILE_BYTES + 1, "content": ""} + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=11, + head_sha="c" * 40, + event_action="opened", + token="token", + opener=opener, + ) + assert result == () + + +def test_evaluate_pull_request_scans_a_disguised_textual_pdf_without_a_patch() -> None: + """A patchless '.pdf' file that fetches as real content is still scanned. + + Regression coverage for Devin Review's second finding: a missing diff + patch is not proof of binary content by itself (GitHub also omits one + for a textual diff over its own rendering limit, well under this + module's MAX_FILE_BYTES fetch ceiling), so a file this small must be + verified by its real magic bytes, not trusted on patch-absence alone. + """ + + def opener(url: str, _token: str) -> object: + if "/pulls/12/files" in url: + return [ + {"filename": "docs/papers/not-really-a-pdf.pdf", "status": "added"}, + ] + assert "/contents/docs/papers/not-really-a-pdf.pdf" 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=12, + head_sha="d" * 40, + event_action="opened", + token="token", + opener=opener, + ) + assert [item.rule for item in result] == ["nginx_runtime_path"] + + +def test_evaluate_pull_request_exempts_a_real_pdf_under_the_size_ceiling() -> None: + """A genuine, fetchable PDF (verified by its magic bytes) is exempt too.""" + + def opener(url: str, _token: str) -> object: + if "/pulls/13/files" in url: + return [ + {"filename": "docs/papers/small-paper.pdf", "status": "added"}, + ] + assert "/contents/docs/papers/small-paper.pdf" in url + return encoded_file("%PDF-1.7\nupstream nginx { server 127.0.0.1:9; }\n") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=13, + head_sha="e" * 40, + event_action="opened", + token="token", + opener=opener, + ) + assert result == () + + +def test_evaluate_pull_request_does_not_fetch_a_removed_binary_pdf() -> None: + """A removed documentation PDF has no head content to fetch at all. + + Regression coverage for Devin Review's finding: _is_binary_documentation_pdf + does not itself check status, so without an explicit removed-status guard + in evaluate_pull_request's own loop, a deleted PDF would try to fetch its + (nonexistent) head content and fail evidence collection for every such + deletion. + """ + + def opener(url: str, _token: str) -> object: + if "/pulls/14/files" in url: + return [ + {"filename": "docs/papers/removed-paper.pdf", "status": "removed"}, + ] + raise AssertionError(f"must not fetch content for a removed file: {url}") + + result = policy.evaluate_pull_request( + api_url="https://api.github.test", + repository="ContextualWisdomLab/example", + pull_request=14, + head_sha="f" * 40, + event_action="opened", + token="token", + opener=opener, + ) + assert result == () + + def test_closed_event_skips_without_credentials_or_identity_validation() -> None: """Closed-event cleanup remains a no-op for the required-workflow context.""" @@ -318,7 +496,14 @@ def opener(url: str, _token: str) -> object: [ ([], "not an object"), ({"type": "symlink", "encoding": "base64", "size": 0, "content": ""}, "not a regular"), + ({"type": "file", "encoding": "base64", "size": -1, "content": ""}, "malformed size"), ({"type": "file", "encoding": "base64", "size": policy.MAX_FILE_BYTES + 1, "content": ""}, "size contract"), + # GitHub's real response shape for a file whose blob exceeds the + # inline-content ceiling: no content at all, encoding "none". + ({"type": "file", "encoding": "none", "size": policy.MAX_FILE_BYTES + 1}, "size contract"), + ({"type": "file", "encoding": "none", "size": 1}, "no inline content"), + ({"type": "file", "encoding": "none", "size": "not-an-int"}, "no inline content"), + ({"type": "file", "encoding": "utf-8", "size": 1, "content": "x"}, "not a regular base64 file"), ({"type": "file", "encoding": "base64", "size": 1, "content": "!"}, "invalid base64"), ({"type": "file", "encoding": "base64", "size": 2, "content": base64.b64encode(b"x").decode()}, "size mismatch"), ({"type": "file", "encoding": "base64", "size": 1, "content": base64.b64encode(b"\xff").decode()}, "not valid UTF-8"),