From c38aed9a016f5339d91abf9581613e76e43fa7c8 Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Tue, 20 Oct 2026 02:50:16 -0700 Subject: [PATCH 1/5] fix(ci): check public text against a private denylist Refs: ENG-3469 --- .github/workflows/public-text-audit.yml | 29 ++ .github/workflows/public-text-check.yml | 36 +++ .github/workflows/public-text.yml | 22 ++ README.md | 6 + docs/public-text.md | 75 +++++ scripts/public_text_check.py | 291 +++++++++++++++++++ tests/test_public_text_check.py | 356 ++++++++++++++++++++++++ 7 files changed, 815 insertions(+) create mode 100644 .github/workflows/public-text-audit.yml create mode 100644 .github/workflows/public-text-check.yml create mode 100644 .github/workflows/public-text.yml create mode 100644 docs/public-text.md create mode 100644 scripts/public_text_check.py create mode 100644 tests/test_public_text_check.py diff --git a/.github/workflows/public-text-audit.yml b/.github/workflows/public-text-audit.yml new file mode 100644 index 0000000..d01a86a --- /dev/null +++ b/.github/workflows/public-text-audit.yml @@ -0,0 +1,29 @@ +# run-tree-ok: text edits need independent audit concurrency and results; +# CLA and application checks have different event and permission contracts. +# Separate workflow and job name: comment success must not satisfy the PR gate. +name: Public text audit + +on: + issues: + types: [opened, edited] + issue_comment: + types: [created, edited] + pull_request_review: + types: [submitted, edited] + pull_request_review_comment: + types: [created, edited] + +permissions: + contents: read + pull-requests: read + issues: read + +concurrency: + group: public-text-audit-${{ github.event_name }}-${{ github.event.comment.id || github.event.review.id || github.event.issue.number }} + cancel-in-progress: true + +jobs: + public-text-audit: + uses: mindsdb/github-actions/.github/workflows/public-text-check.yml@main + secrets: + PUBLIC_TEXT_DENYLIST: ${{ secrets.PUBLIC_TEXT_DENYLIST }} diff --git a/.github/workflows/public-text-check.yml b/.github/workflows/public-text-check.yml new file mode 100644 index 0000000..695ab7d --- /dev/null +++ b/.github/workflows/public-text-check.yml @@ -0,0 +1,36 @@ +name: Public text reusable + +on: + workflow_call: + secrets: + PUBLIC_TEXT_DENYLIST: + description: Private regular expression for restricted public text + required: false + +permissions: + contents: read + pull-requests: read + issues: read + +jobs: + check: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Check out the trusted scanner + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + repository: mindsdb/github-actions + ref: ${{ job.workflow_sha }} + path: .ci-shared + persist-credentials: false + sparse-checkout: scripts/public_text_check.py + sparse-checkout-cone-mode: false + - name: Scan current public text + env: + GH_TOKEN: ${{ github.token }} + PUBLIC_TEXT_DENYLIST: ${{ secrets.PUBLIC_TEXT_DENYLIST }} + REPO: ${{ github.repository }} + EVENT_NAME: ${{ github.event_name }} + EVENT_PATH: ${{ github.event_path }} + run: python3 -I .ci-shared/scripts/public_text_check.py diff --git a/.github/workflows/public-text.yml b/.github/workflows/public-text.yml new file mode 100644 index 0000000..2020a3f --- /dev/null +++ b/.github/workflows/public-text.yml @@ -0,0 +1,22 @@ +# run-tree-ok: PR metadata edits must rerun this stable text gate without +# retriggering application build or deployment pipelines. +name: Public text + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + contents: read + pull-requests: read + issues: read + +concurrency: + group: public-text-pr-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + public-text: + uses: mindsdb/github-actions/.github/workflows/public-text-check.yml@main + secrets: + PUBLIC_TEXT_DENYLIST: ${{ secrets.PUBLIC_TEXT_DENYLIST }} diff --git a/README.md b/README.md index cea57bb..4667a96 100644 --- a/README.md +++ b/README.md @@ -742,3 +742,9 @@ jobs: uses: mindsdb/github-actions/.github/workflows/release-pr.yml@main secrets: inherit ``` + +## Public text guard + +[`public-text-check.yml`](.github/workflows/public-text-check.yml) checks PR text, commit messages, current added content and supported issue/comment/review text against a private regular expression. It checks only API data, runs the trusted script at the reusable workflow's commit, and never prints matching text or paths. Separate PR and audit callers prevent a comment's result from satisfying the merge check. + +See [coverage, exceptions and rollout](docs/public-text.md) before enabling the required check. Missing configuration and incomplete scans fail; fork events carry an explicit unscanned exception. The guard runs after publication and cannot remove history. The implementation and this repository's callers must merge before application callers use it. diff --git a/docs/public-text.md b/docs/public-text.md new file mode 100644 index 0000000..f5016dd --- /dev/null +++ b/docs/public-text.md @@ -0,0 +1,75 @@ +# Public text guard + +The guard fails a PR or text-audit run when current public text matches the private `PUBLIC_TEXT_DENYLIST` regular expression. It reports a field, ordinal and line number without quoting the text or its filename. A failure catches a disclosure after GitHub saves it; the guard cannot prevent publication or erase older revisions. + +## Coverage + +| Event | Text read from the API | Result context | +| --- | --- | --- | +| PR opened, edited, synchronized or reopened | Current title, description, head branch, all commit messages, current added lines and paths | `public-text / check` (verify on the first live run) | +| Issue opened or edited | Current title and description | `public-text-audit / check` | +| Discussion comment created or edited | Current comment body | `public-text-audit / check` | +| Review submitted or edited | Current review summary | `public-text-audit / check` | +| Inline review comment created or edited | Current comment body | `public-text-audit / check` | + +The PR and audit callers have separate workflow names, job names and concurrency groups. A successful comment audit cannot satisfy the PR check. Reruns read current text. The scanner reads the same fields again before reporting success, and fails if they change. A PR run for an older head fails instead of reporting on a newer head under the old check. + +Release trains receive the full scan. The scanner compares paginated collection sizes with the PR's counts. More than 250 commits or 3,000 files exceeds the REST endpoints' limits and fails as incomplete. Duplicate data, malformed hunks and added-line count mismatches also fail. + +Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A zero-addition, zero-deletion change can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. + +For other missing patches, the scanner reads the blob as data, accepts an empty file or a NUL byte within the first 8,000 bytes as binary, and checks only the binary file's path. Blobs larger than 1 MiB fail this fallback and need maintainer review. Git attributes can classify other files as binary; these conservatively fail when no text patch is available. + +Added text is checked in contiguous blocks so a wrapped phrase can match. The scanner does not join additions across intervening unchanged lines. + +## Trust and exceptions + +The reusable checks out only `mindsdb/github-actions` at `job.workflow_sha`, the commit of the called workflow. It never checks out contributor code. The token has only contents, issues and pull-request read permissions. Callers pass only the named secret. Existing repository convention makes callers track `@main`; a changed shared workflow takes effect for every caller after merge. + +| Situation | Behavior | +| --- | --- | +| Same-repository PR with missing or invalid secret | Fails, unscanned | +| Dependabot event without the configured secret | Fails, unscanned; maintainer action required | +| Fork PR, review or inline comment | Explicit successful exception labeled `NOT SCANNED`; maintainer inspection required | +| Discussion comment on a fork PR | Default-branch audit reads and checks the comment | +| API failure, deleted text, unavailable head, incomplete data or concurrent edit | Fails, incomplete or superseded | + +The guard protects against accidental disclosures. Contributors who can alter trusted workflows or use privileged repository settings can bypass it. A fork exception is not a clean scan. This design does not add a privileged `pull_request_target` workflow. + +## Public-writing policy + +Use neutral roles and public component contracts. Do not identify customers, partners or users, or disclose private repository names, links, PR or issue references, release versions or schedules. Generic deployment terminology and public configuration values remain allowed. + +Maintainers supply restricted terms and aliases through the private regular expression. Do not put real terms in this repository's code, tests, workflow configuration, examples or logs. Fixtures use synthetic text only. The regular expression detects configured strings; it cannot infer that an otherwise ordinary version number or schedule refers to a private project. Human review must check that context. + +The secret keeps the configured list out of source, but public pass/fail results can reveal whether a guessed term matches. The check is not a secrecy guarantee for the list. Use a bounded, reviewed expression; pathological regular expressions can exhaust the job timeout. + +## Rollout + +1. Merge the shared scanner, tests, reusable and this repository's callers first. The initial implementation PR cannot resolve its new `@main` reusable until that merge; use the existing CI and local checks for bootstrap review. Do not require the new context before it exists. +2. Merge each application caller into its integration branch, then promote it to the default branch. Issues and discussion-comment events depend on default-branch workflow availability. +3. An operator configures `PUBLIC_TEXT_DENYLIST` as an organization Actions secret visible only to selected repositories. The pattern includes a synthetic canary such as `zqcanary7731`. Never put the pattern into a public ruleset or shell command history. +4. Verify a same-repository PR fails on the canary and passes after an edit removes it. Observe the actual Actions check context before requiring it for default-branch and staging merges. +5. Verify an audit can fail without replacing the PR check. Edit the audited text, rerun the old event, and verify that the latest body is checked. Test the explicit fork exception and missing-secret failure. +6. The operator installs the verified PR context as a required GitHub Actions check. Required-workflow rules alone do not cover edited PR descriptions. + +Workflow authors do not run these operator steps automatically. Missing configuration deliberately blocks same-repository runs. Coordinate the rollout before making the check required. A large release train or historical commit disclosure can block a merge even when the current diff is clean. + +## Limits and response + +GitHub may not deliver these events in every case. PR workflows do not run on merge conflicts; supported CI-skip directives can suppress runs; most writes made with `GITHUB_TOKEN` do not trigger a new workflow. Review these cases manually before merge. A required check cannot make an event happen. + +The scanner does not read intermediate commit file content, binary content, release descriptions, old text revisions or every existing issue and comment. Added-then-removed content can remain in commit history. A clean result concerns the configured pattern and scanned fields only. Editing a current description leaves older revisions visible; the repository owner handles history cleanup and pushed-commit rewrites separately. + +Actions notifications depend on each person's notification settings. A failed audit creates a failed run, not a guaranteed message to its author. This workflow posts no public comments and sends no chat messages. + +## Local verification + +```sh +uv run --with pyyaml --with pytest --python 3.12 -m pytest tests/ -q +actionlint +uv run --with pyyaml --python 3.12 scripts/workflow_graph.py --workflow-dir .github/workflows --allow-external-reusables +zizmor .github/workflows +``` + +Unit tests use synthetic API responses and patterns, exercise incomplete and stale data, and assert that failures never echo sensitive text. The live event matrix remains an operator acceptance step after publication. diff --git a/scripts/public_text_check.py b/scripts/public_text_check.py new file mode 100644 index 0000000..7822b4f --- /dev/null +++ b/scripts/public_text_check.py @@ -0,0 +1,291 @@ +#!/usr/bin/env python3 +"""Check public GitHub text without echoing text, paths or the private pattern. + +Exit 0: clean or the explicit fork exception. Exit 1: match. Exit 2: incomplete. +Only API data is read. No contributor code is checked out or executed. +""" +from __future__ import annotations + +import base64 +import json +import os +import re +import subprocess +from dataclasses import dataclass +from urllib.parse import quote + + +class ScanError(Exception): + """A scan cannot establish a complete, stable result.""" + + +@dataclass(frozen=True) +class Finding: + where: str + + +@dataclass(frozen=True) +class PullRequest: + title: str + body: str + branch: str + head: str + base: str + head_repo: str + commits: int + files: int + + @classmethod + def read(cls, value: dict) -> PullRequest: + return cls( + text(value["title"]), text(value.get("body")), text(value["head"]["ref"]), + sha(value["head"]["sha"]), sha(value["base"]["sha"]), + text(value["head"]["repo"]["full_name"]), + count(value["commits"]), count(value["changed_files"]), + ) + + +def text(value: object) -> str: + if value is None: + return "" + if not isinstance(value, str): + raise ScanError("invalid text") + return value + + +def count(value: object) -> int: + if type(value) is not int or value < 0: + raise ScanError("invalid count") + return value + + +def identifier(value: object) -> int: + if count(value) == 0: + raise ScanError("invalid identifier") + return value + + +def sha(value: object) -> str: + if not isinstance(value, str) or not re.fullmatch(r"[a-f0-9]{40}", value): + raise ScanError("invalid revision") + return value + + +def gh_api(path: str) -> list | dict: + result = subprocess.run( + ["gh", "api", "--paginate", "--slurp", path], + check=True, capture_output=True, text=True, timeout=90, + ) + pages = json.loads(result.stdout) + if not isinstance(pages, list) or not pages: + raise ScanError("empty response") + if all(isinstance(page, list) for page in pages): + return [item for page in pages for item in page] + if len(pages) == 1 and isinstance(pages[0], dict): + return pages[0] + raise ScanError("invalid pagination") + + +def check_text(rx: re.Pattern[str], *, label: str, text: str) -> list[Finding]: + lines = sorted({text.count("\n", 0, match.start()) + 1 for match in rx.finditer(text)}) + return [Finding(f"{label}, line {line}") for line in lines] + + +def added_lines(patch: str) -> list[tuple[int, str]]: + """Parse complete unified hunks, rejecting omitted or malformed content.""" + added: list[tuple[int, str]] = [] + old_left = new_left = 0 + new_line = 0 + in_hunk = False + for line in patch.split("\n"): + hunk = re.fullmatch(r"@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@.*", line) + if hunk: + if old_left or new_left: + raise ScanError("truncated hunk") + old_left = int(hunk[1] or 1) + new_line = int(hunk[2]) + new_left = int(hunk[3] or 1) + in_hunk = True + elif line == r"\ No newline at end of file": + continue + elif not line and not old_left and not new_left: + continue + elif not in_hunk or not line: + raise ScanError("invalid hunk") + elif line[0] == "+": + added.append((new_line, line[1:])) + new_line += 1 + new_left -= 1 + elif line[0] == "-": + old_left -= 1 + elif line[0] == " ": + old_left -= 1 + new_left -= 1 + new_line += 1 + else: + raise ScanError("invalid hunk line") + if old_left < 0 or new_left < 0: + raise ScanError("hunk overflow") + if old_left or new_left or not in_hunk: + raise ScanError("incomplete patch") + return added + + +def missing_patch_is_empty_or_binary(repo: str, file: dict) -> bool: + # Never trust the extension. NUL in the first 8 KiB follows Git's binary + # heuristic; other missing patches fail closed. Large blobs need human review. + blob = gh_api(f"repos/{repo}/git/blobs/{sha(file['sha'])}") + size = count(blob["size"]) + if size > 1_048_576 or blob["encoding"] != "base64": + return False + content = base64.b64decode("".join(text(blob["content"]).split()), validate=True) + if len(content) != size: + return False + return (not content and count(file["additions"]) == 0) or b"\0" in content[:8000] + + +def check_added(rx: re.Pattern[str], lines: list[tuple[int, str]], ordinal: int) -> list[Finding]: + blocks: list[list[tuple[int, str]]] = [] + for line in lines: + if not blocks or line[0] != blocks[-1][-1][0] + 1: + blocks.append([]) + blocks[-1].append(line) + findings = [] + for block in blocks: + content = "\n".join(value for _, value in block) + numbers = {block[0][0] + content.count("\n", 0, match.start()) for match in rx.finditer(content)} + findings.extend(Finding(f"file {ordinal}, added line {line}") for line in sorted(numbers)) + return findings + + +def content_unchanged(repo: str, base: str, file: dict) -> bool: + if (file["status"] not in {"modified", "renamed", "changed"} + or count(file["additions"]) or count(file["deletions"])): + return False + path = text(file["previous_filename"] if file["status"] == "renamed" else file["filename"]) + original = gh_api(f"repos/{repo}/contents/{quote(path, safe='/')}?ref={base}") + return original["type"] == "file" and sha(original["sha"]) == sha(file["sha"]) + + +def check_pr(rx: re.Pattern[str], *, repo: str, number: int, expected_head: str | None = None) -> list[Finding]: + endpoint = f"repos/{repo}/pulls/{identifier(number)}" + before = PullRequest.read(gh_api(endpoint)) + if before.head_repo != repo or (expected_head and before.head != expected_head): + raise ScanError("superseded or foreign head") + # REST caps these collections even when pagination follows every page. + if before.commits > 250 or before.files > 3000: + raise ScanError("collection cap") + findings = check_text(rx, label="PR title", text=before.title) + findings += check_text(rx, label="PR description", text=before.body) + findings += check_text(rx, label="head branch name", text=before.branch) + commits = gh_api(endpoint + "/commits?per_page=100") + files = gh_api(endpoint + "/files?per_page=100") + if not isinstance(commits, list) or len(commits) != before.commits: + raise ScanError("incomplete commits") + if not isinstance(files, list) or len(files) != before.files: + raise ScanError("incomplete files") + if len({sha(commit["sha"]) for commit in commits}) != len(commits): + raise ScanError("duplicate commits") + if len({text(file["filename"]) for file in files}) != len(files): + raise ScanError("duplicate files") + for i, commit in enumerate(commits, 1): + findings += check_text(rx, label=f"commit {i} message", text=text(commit["commit"]["message"])) + for i, file in enumerate(files, 1): + if file["status"] == "removed": + continue + if file["status"] not in {"added", "modified", "renamed", "copied", "changed", "unchanged"}: + raise ScanError("unknown file status") + findings += check_text(rx, label=f"file {i} path", text=text(file["filename"])) + patch = file.get("patch") + if not patch: + if content_unchanged(repo, before.base, file): + continue + if missing_patch_is_empty_or_binary(repo, file): + continue + raise ScanError("missing text patch") + lines = added_lines(text(patch)) + if len(lines) != count(file["additions"]): + raise ScanError("incomplete added lines") + findings += check_added(rx, lines, i) + if before != PullRequest.read(gh_api(endpoint)): + raise ScanError("PR changes during scan") + return findings + + +def is_fork_event(repo: str, event_name: str, event: dict) -> bool: + if event_name not in {"pull_request", "pull_request_review", "pull_request_review_comment"}: + return False + pull = event["pull_request"] + # A deleted head repository is not evidence of a fork exception. + head_repo = pull["head"]["repo"]["full_name"] + base_repo = pull["base"]["repo"]["full_name"] + if not head_repo or base_repo != repo: + raise ScanError("invalid repository identity") + return head_repo != repo + + +def check_event(rx: re.Pattern[str], *, repo: str, event_name: str, event: dict) -> list[Finding]: + if event_name == "pull_request": + pull = event["pull_request"] + return check_pr(rx, repo=repo, number=pull["number"], expected_head=sha(pull["head"]["sha"])) + if event_name == "issues": + endpoint = f"repos/{repo}/issues/{identifier(event['issue']['number'])}" + labels = (("title", "issue title"), ("body", "issue description")) + elif event_name == "issue_comment": + endpoint = f"repos/{repo}/issues/comments/{identifier(event['comment']['id'])}" + labels = (("body", "comment"),) + elif event_name == "pull_request_review": + endpoint = (f"repos/{repo}/pulls/{identifier(event['pull_request']['number'])}" + f"/reviews/{identifier(event['review']['id'])}") + labels = (("body", "review summary"),) + elif event_name == "pull_request_review_comment": + endpoint = f"repos/{repo}/pulls/comments/{identifier(event['comment']['id'])}" + labels = (("body", "review comment"),) + else: + raise ScanError("unsupported event") + before = gh_api(endpoint) + fields = tuple(text(before[key]) for key, _ in labels) + findings = [finding for (_, label), value in zip(labels, fields) + for finding in check_text(rx, label=label, text=value)] + after = gh_api(endpoint) + if fields != tuple(text(after[key]) for key, _ in labels): + raise ScanError("text changes during scan") + return findings + + +def main() -> int: + try: + repo, event_name = os.environ["REPO"], os.environ["EVENT_NAME"] + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", repo): + raise ScanError("invalid repository") + with open(os.environ["EVENT_PATH"], encoding="utf-8") as handle: + event = json.load(handle) + if is_fork_event(repo, event_name, event): + print("::notice::Fork event: NOT SCANNED. A maintainer must inspect public text before merging.") + return 0 + pattern = os.environ.get("PUBLIC_TEXT_DENYLIST", "") + if not pattern.strip(): + print("::error::Public-text configuration is missing. This event is NOT SCANNED.") + return 2 + try: + rx = re.compile(pattern, re.I) + except re.error: + print("::error::Public-text configuration is invalid. This event is NOT SCANNED.") + return 2 + findings = check_event(rx, repo=repo, event_name=event_name, event=event) + except Exception: + # API stderr, malformed input and exception messages can contain restricted + # text or annotation syntax. Do not print any part of them, even on failure. + print("::error::Public-text scan is incomplete or superseded. Review configuration, API access and data limits; rerun on current text.") + return 2 + for finding in findings: + print(f"::error::Restricted text in {finding.where}") + if findings: + print("Replace restricted text with a neutral description. Older revisions and pushed history need separate owner review.") + return 1 + print("Current public text passes the configured scan. Binary content and intermediate commit file content are outside its scope.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_public_text_check.py b/tests/test_public_text_check.py new file mode 100644 index 0000000..05804da --- /dev/null +++ b/tests/test_public_text_check.py @@ -0,0 +1,356 @@ +"""Synthetic fixtures only: never put actual restricted terms in this file.""" + +import copy +import base64 +import json +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "scripts")) +import public_text_check as ptc + +RX = re.compile("zqcanary7731", re.I) +PR_PATH = "repos/o/r/pulls/7" +SHA = "a" * 40 + + +def pr(): + return { + "title": "fix", "body": "", "number": 7, "commits": 1, "changed_files": 1, + "head": {"ref": "topic", "sha": SHA, "repo": {"full_name": "o/r"}}, + "base": {"sha": "b" * 40, "repo": {"full_name": "o/r"}}, + } + + +def responses(): + return { + PR_PATH: pr(), + PR_PATH + "/commits?per_page=100": [ + {"sha": SHA, "commit": {"message": "clean"}} + ], + PR_PATH + "/files?per_page=100": [ + {"filename": "file.txt", "status": "added", "additions": 1, "deletions": 0, + "sha": "c" * 40, "patch": "@@ -0,0 +1 @@\n+clean"} + ], + } + + +def mock_api(monkeypatch, data): + monkeypatch.setattr(ptc, "gh_api", lambda path: copy.deepcopy(data[path])) + + +def run_main(monkeypatch, tmp_path, event_name="pull_request", event=None, pattern="zqcanary7731"): + path = tmp_path / "event.json" + path.write_text(json.dumps(event or {"pull_request": pr()})) + for name, value in { + "REPO": "o/r", "EVENT_NAME": event_name, "EVENT_PATH": str(path), + "PUBLIC_TEXT_DENYLIST": pattern, + }.items(): + monkeypatch.setenv(name, value) + return ptc.main() + + +def test_missing_secret_fails_same_repository(monkeypatch, tmp_path): + assert run_main(monkeypatch, tmp_path, pattern="") == 2 + + +def test_release_train_scans_commits(monkeypatch): + data = responses() + data[PR_PATH]["head"]["ref"] = "staging" + data[PR_PATH + "/commits?per_page=100"][0]["commit"]["message"] = "zqcanary7731" + mock_api(monkeypatch, data) + assert any("message" in f.where for f in ptc.check_pr(RX, repo="o/r", number=7)) + + +def test_missing_text_patch_never_passes(monkeypatch, tmp_path): + data = responses() + del data[PR_PATH + "/files?per_page=100"][0]["patch"] + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_commit_cap_never_passes(monkeypatch, tmp_path): + data = responses() + data[PR_PATH]["commits"] = 251 + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_clean_paths_are_not_logged(monkeypatch): + data = responses() + data[PR_PATH + "/files?per_page=100"][0].update( + filename="attacker%0A::notice::secret.txt", patch="@@ -0,0 +1 @@\n+zqcanary7731" + ) + mock_api(monkeypatch, data) + found = ptc.check_pr(RX, repo="o/r", number=7) + assert found and all("attacker" not in f.where for f in found) + + +def test_rerun_reads_current_comment(monkeypatch, tmp_path): + mock_api(monkeypatch, {"repos/o/r/issues/comments/12": {"body": "zqcanary7731"}}) + assert run_main(monkeypatch, tmp_path, "issue_comment", + {"comment": {"id": 12, "body": "formerly clean"}}) == 1 + + +def test_removed_path_can_clean_existing_disclosure(monkeypatch): + data = responses() + data[PR_PATH + "/files?per_page=100"][0].update( + filename="zqcanary7731.txt", status="removed", additions=0, + patch="@@ -1 +0,0 @@\n-zqcanary7731" + ) + mock_api(monkeypatch, data) + assert ptc.check_pr(RX, repo="o/r", number=7) == [] + + +@pytest.mark.parametrize("field", ["title", "body", "branch", "commit", "path", "added"]) +def test_all_pr_fields_fail_without_echoing_text(monkeypatch, tmp_path, capsys, field): + data = responses() + if field in {"title", "body"}: + data[PR_PATH][field] = "zqcanary7731" + elif field == "branch": + data[PR_PATH]["head"]["ref"] = "fix/zqcanary7731" + elif field == "commit": + data[PR_PATH + "/commits?per_page=100"][0]["commit"]["message"] = "zqcanary7731" + elif field == "path": + data[PR_PATH + "/files?per_page=100"][0]["filename"] = "zqcanary7731" + else: + data[PR_PATH + "/files?per_page=100"][0]["patch"] = "@@ -0,0 +1 @@\n+zqcanary7731" + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 1 + output = capsys.readouterr() + assert "zqcanary7731" not in output.out + output.err + + +def test_clean_pr_and_generic_deployment_terminology_pass(monkeypatch, tmp_path): + data = responses() + data[PR_PATH]["body"] = "self-hosted deployment with COWORK_DEPLOYMENT_KIND=self-hosted" + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +@pytest.mark.parametrize("event_name", ["pull_request", "pull_request_review", "pull_request_review_comment"]) +def test_explicit_fork_exception_without_secret(monkeypatch, tmp_path, capsys, event_name): + pull = pr() + pull["head"]["repo"]["full_name"] = "outside/fork" + assert run_main(monkeypatch, tmp_path, event_name, {"pull_request": pull}, pattern="") == 0 + assert "NOT SCANNED" in capsys.readouterr().out + + +def test_fork_discussion_comment_is_scanned(monkeypatch, tmp_path): + mock_api(monkeypatch, {"repos/o/r/issues/comments/12": {"body": "zqcanary7731"}}) + event = {"comment": {"id": 12}, "issue": {"pull_request": {}}} + assert run_main(monkeypatch, tmp_path, "issue_comment", event) == 1 + + +def test_dependabot_without_configuration_fails(monkeypatch, tmp_path): + event = {"pull_request": pr(), "sender": {"login": "dependabot[bot]"}} + assert run_main(monkeypatch, tmp_path, event=event, pattern="") == 2 + + +@pytest.mark.parametrize("pattern", ["", " ", "zqcanary7731("]) +def test_invalid_configuration_never_echoes_pattern(monkeypatch, tmp_path, capsys, pattern): + assert run_main(monkeypatch, tmp_path, pattern=pattern) == 2 + assert "zqcanary7731" not in capsys.readouterr().out + + +@pytest.mark.parametrize("field,value", [("commits", 2), ("changed_files", 2), ("changed_files", 3001)]) +def test_incomplete_collections_fail(monkeypatch, tmp_path, field, value): + data = responses() + data[PR_PATH][field] = value + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +@pytest.mark.parametrize("collection,field", [("commits", "commits"), ("files", "changed_files")]) +def test_duplicate_page_data_fails(monkeypatch, tmp_path, collection, field): + data = responses() + data[PR_PATH][field] = 2 + data[PR_PATH + f"/{collection}?per_page=100"] *= 2 + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_truncated_patch_fails(monkeypatch, tmp_path): + data = responses() + data[PR_PATH + "/files?per_page=100"][0]["additions"] = 2 + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +@pytest.mark.parametrize("patch", [ + "@@ -0,0 +1,2 @@\n+clean", "truncated", "@@ -0,0 +1 @@\n+one\n+extra", + "@@ -1 +1 @@\n", "@@ -0,0 +1,2 @@\n+clean\n@@ -0,0 +3 @@\n+clean", +]) +def test_malformed_hunks_fail(patch): + with pytest.raises(ptc.ScanError): + ptc.added_lines(patch) + + +def test_hunks_handle_literal_prefixes_crlf_and_no_newline(): + patch = "@@ -1,2 +1,3 @@\n ctx\r\n-old\r\n+++new\r\n+tail\n\\ No newline at end of file" + assert ptc.added_lines(patch) == [(2, "++new\r"), (3, "tail")] + + +def blob(data, content): + data["repos/o/r/git/blobs/" + "c" * 40] = { + "size": len(content), "encoding": "base64", + "content": base64.b64encode(content).decode(), + } + data[PR_PATH + "/files?per_page=100"][0].pop("patch") + + +def test_binary_content_is_excluded_but_path_is_checked(monkeypatch, tmp_path): + data = responses() + blob(data, b"binary\0zqcanary7731") + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + data[PR_PATH + "/files?per_page=100"][0]["filename"] = "zqcanary7731.png" + assert run_main(monkeypatch, tmp_path) == 1 + + +def test_text_with_binary_extension_is_not_exempt(monkeypatch, tmp_path): + data = responses() + blob(data, b"zqcanary7731") + data[PR_PATH + "/files?per_page=100"][0]["filename"] = "logo.png" + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_empty_file_passes(monkeypatch, tmp_path): + data = responses() + blob(data, b"") + data[PR_PATH + "/files?per_page=100"][0]["additions"] = 0 + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +def test_rename_away_from_restricted_path_passes(monkeypatch, tmp_path): + data = responses() + file = data[PR_PATH + "/files?per_page=100"][0] + file.update(status="renamed", previous_filename="zqcanary7731.txt", additions=0, deletions=0) + file.pop("patch") + data["repos/o/r/contents/zqcanary7731.txt?ref=" + "b" * 40] = {"type": "file", "sha": "c" * 40} + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +@pytest.mark.parametrize("change", ["title", "body", "head", "base", "branch"]) +def test_pr_changes_during_scan_fail(monkeypatch, tmp_path, change): + data = responses() + reads = 0 + + def api(path): + nonlocal reads + value = copy.deepcopy(data[path]) + if path == PR_PATH: + reads += 1 + if reads > 1: + if change in {"title", "body"}: + value[change] = "zqcanary7731" + elif change == "branch": + value["head"]["ref"] = "changed" + else: + value[change]["sha"] = "d" * 40 + return value + + monkeypatch.setattr(ptc, "gh_api", api) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_old_head_rerun_cannot_pass_for_a_new_head(monkeypatch, tmp_path): + data = responses() + data[PR_PATH]["head"]["sha"] = "d" * 40 + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +@pytest.mark.parametrize("name,event,endpoint,value", [ + ("issues", {"issue": {"number": 3}}, "issues/3", {"title": "zqcanary7731", "body": ""}), + ("pull_request_review", {"pull_request": pr(), "review": {"id": 4}}, + "pulls/7/reviews/4", {"body": "zqcanary7731"}), + ("pull_request_review_comment", {"pull_request": pr(), "comment": {"id": 5}}, + "pulls/comments/5", {"body": "zqcanary7731"}), +]) +def test_current_text_audits(monkeypatch, tmp_path, name, event, endpoint, value): + mock_api(monkeypatch, {"repos/o/r/" + endpoint: value}) + assert run_main(monkeypatch, tmp_path, name, event) == 1 + + +def test_comment_changes_during_scan_fail(monkeypatch, tmp_path): + values = iter([{"body": "clean"}, {"body": "zqcanary7731"}]) + monkeypatch.setattr(ptc, "gh_api", lambda _: next(values)) + assert run_main(monkeypatch, tmp_path, "issue_comment", {"comment": {"id": 12}}) == 2 + + +@pytest.mark.parametrize("error", [ + subprocess.CalledProcessError(1, ["gh"], stderr="zqcanary7731\n::notice::poison"), + ValueError("zqcanary7731"), + KeyError("zqcanary7731"), + subprocess.TimeoutExpired("zqcanary7731", 90), +]) +def test_failures_never_echo_data(monkeypatch, tmp_path, capsys, error): + def fail(_): + raise error + monkeypatch.setattr(ptc, "gh_api", fail) + assert run_main(monkeypatch, tmp_path) == 2 + output = capsys.readouterr() + assert "zqcanary7731" not in output.out + output.err + assert "poison" not in output.out + output.err + + +def test_pagination_flattens_every_page(monkeypatch): + def run(args, **kwargs): + assert args[:4] == ["gh", "api", "--paginate", "--slurp"] + assert kwargs["capture_output"] and kwargs["timeout"] + return subprocess.CompletedProcess(args, 0, json.dumps([[{"id": 1}], [{"id": 2}]])) + monkeypatch.setattr(ptc.subprocess, "run", run) + assert ptc.gh_api("repos/o/r/issues") == [{"id": 1}, {"id": 2}] + + +def test_mode_only_text_change_passes(monkeypatch, tmp_path): + data = responses() + blob(data, b"#!/bin/sh\necho clean\n") + data[PR_PATH + "/files?per_page=100"][0].update(status="modified", additions=0, deletions=0) + data["repos/o/r/contents/file.txt?ref=" + "b" * 40] = {"type": "file", "sha": "c" * 40} + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +def test_multiline_pattern_matches_contiguous_added_lines(monkeypatch): + data = responses() + data[PR_PATH + "/files?per_page=100"][0].update( + additions=2, patch="@@ -0,0 +1,2 @@\n+zqcanary\n+7731" + ) + mock_api(monkeypatch, data) + found = ptc.check_pr(re.compile(r"zqcanary\s+7731"), repo="o/r", number=7) + assert [f.where for f in found] == ["file 1, added line 1"] + + +def test_multiline_pattern_does_not_join_across_context(monkeypatch): + data = responses() + data[PR_PATH + "/files?per_page=100"][0].update( + additions=2, patch="@@ -1 +1,3 @@\n+zqcanary\n context\n+7731" + ) + mock_api(monkeypatch, data) + assert ptc.check_pr(re.compile(r"zqcanary\s+7731"), repo="o/r", number=7) == [] + + +def test_zero_counts_do_not_exempt_new_text_classified_as_binary(monkeypatch, tmp_path): + data = responses() + blob(data, b"zqcanary7731") + data[PR_PATH + "/files?per_page=100"][0].update(additions=0, deletions=0) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_zero_counts_with_changed_blob_fail(monkeypatch, tmp_path): + data = responses() + blob(data, b"zqcanary7731") + data[PR_PATH + "/files?per_page=100"][0].update(status="modified", additions=0, deletions=0) + data["repos/o/r/contents/file.txt?ref=" + "b" * 40] = {"type": "file", "sha": "d" * 40} + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 From e42db3642a153a37dc351c58af1df71d8cc5d00d Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Mon, 26 Oct 2026 00:18:23 -0700 Subject: [PATCH 2/5] fix(ci): skip commits an earlier merged train carried and check patchless new files --- docs/public-text.md | 22 ++-- scripts/public_text_check.py | 137 +++++++++++++++++++----- tests/test_public_text_check.py | 184 ++++++++++++++++++++++++++++++-- 3 files changed, 299 insertions(+), 44 deletions(-) diff --git a/docs/public-text.md b/docs/public-text.md index f5016dd..3ab3ccb 100644 --- a/docs/public-text.md +++ b/docs/public-text.md @@ -14,11 +14,13 @@ The guard fails a PR or text-audit run when current public text matches the priv The PR and audit callers have separate workflow names, job names and concurrency groups. A successful comment audit cannot satisfy the PR check. Reruns read current text. The scanner reads the same fields again before reporting success, and fails if they change. A PR run for an older head fails instead of reporting on a newer head under the old check. -Release trains receive the full scan. The scanner compares paginated collection sizes with the PR's counts. More than 250 commits or 3,000 files exceeds the REST endpoints' limits and fails as incomplete. Duplicate data, malformed hunks and added-line count mismatches also fail. +Release trains receive the same scan as any other PR. The scanner compares paginated collection sizes with the PR's counts. The PR commits endpoint stops at 250 commits, so a larger PR lists its commits through the compare endpoint, which must return the PR's exact count and every commit the PR endpoint listed. More than 3,000 files exceeds the files endpoint's limit and fails as incomplete. Duplicate data, malformed hunks and added-line count mismatches also fail. + +A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A zero-addition, zero-deletion change can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. -For other missing patches, the scanner reads the blob as data, accepts an empty file or a NUL byte within the first 8,000 bytes as binary, and checks only the binary file's path. Blobs larger than 1 MiB fail this fallback and need maintainer review. Git attributes can classify other files as binary; these conservatively fail when no text patch is available. +For other missing patches, the scanner reads the blob as data. It accepts an empty file, and treats a blob of up to 10 MiB with a NUL byte within the first 8,000 bytes as binary, checking only its path. GitHub also omits patches for files it renders as binary without a NUL byte, such as small PDFs. For an added file of up to 1 MiB with no NUL byte, the whole content is its added text, so the scanner checks all of it. Any other missing patch fails and needs maintainer review: a larger blob, or a modified file whose new content has no patch. Added text is checked in contiguous blocks so a wrapped phrase can match. The scanner does not join additions across intervening unchanged lines. @@ -29,7 +31,7 @@ The reusable checks out only `mindsdb/github-actions` at `job.workflow_sha`, the | Situation | Behavior | | --- | --- | | Same-repository PR with missing or invalid secret | Fails, unscanned | -| Dependabot event without the configured secret | Fails, unscanned; maintainer action required | +| Dependabot event without the configured secret | Fails, unscanned; Dependabot runs receive only Dependabot secrets, so configure a Dependabot copy of the pattern or handle each Dependabot PR manually | | Fork PR, review or inline comment | Explicit successful exception labeled `NOT SCANNED`; maintainer inspection required | | Discussion comment on a fork PR | Default-branch audit reads and checks the comment | | API failure, deleted text, unavailable head, incomplete data or concurrent edit | Fails, incomplete or superseded | @@ -46,20 +48,20 @@ The secret keeps the configured list out of source, but public pass/fail results ## Rollout -1. Merge the shared scanner, tests, reusable and this repository's callers first. The initial implementation PR cannot resolve its new `@main` reusable until that merge; use the existing CI and local checks for bootstrap review. Do not require the new context before it exists. -2. Merge each application caller into its integration branch, then promote it to the default branch. Issues and discussion-comment events depend on default-branch workflow availability. -3. An operator configures `PUBLIC_TEXT_DENYLIST` as an organization Actions secret visible only to selected repositories. The pattern includes a synthetic canary such as `zqcanary7731`. Never put the pattern into a public ruleset or shell command history. -4. Verify a same-repository PR fails on the canary and passes after an edit removes it. Observe the actual Actions check context before requiring it for default-branch and staging merges. +1. Merge the shared scanner, tests, reusable and this repository's callers first. The initial implementation PR cannot resolve its new `@main` reusable until that merge; use the existing CI and local checks for bootstrap review. Do not require the new context before it exists. Until step 2, this repository's own PRs report the missing-secret failure. +2. An operator configures `PUBLIC_TEXT_DENYLIST` as an organization Actions secret visible only to selected repositories, including this one, and a Dependabot secret with the same value for the same repositories. The pattern includes a synthetic canary such as `zqcanary7731`. Never put the pattern into a public ruleset or shell command history. +3. Merge each application caller into its integration branch, then promote it to the default branch. Issues and discussion-comment events depend on default-branch workflow availability. Callers that land before step 2 turn every same-repository PR red. +4. Verify a same-repository PR fails on the canary and passes after an edit removes it. Put the canary only in a description or comment, never in a commit message or branch name, and remove it before merge: a squash merge can copy the description or commit messages into history that later trains carry. Observe the actual Actions check context before requiring it for default-branch and staging merges. 5. Verify an audit can fail without replacing the PR check. Edit the audited text, rerun the old event, and verify that the latest body is checked. Test the explicit fork exception and missing-secret failure. -6. The operator installs the verified PR context as a required GitHub Actions check. Required-workflow rules alone do not cover edited PR descriptions. +6. The operator installs the verified PR context as a required GitHub Actions check. Require the full `public-text / check` context with the GitHub Actions source, never a bare `check`, which the audit job shares. Automation that pushes the protected branch directly, such as a back-merge from the default branch into staging, needs a bypass entry. Required-workflow rules alone do not cover edited PR descriptions. -Workflow authors do not run these operator steps automatically. Missing configuration deliberately blocks same-repository runs. Coordinate the rollout before making the check required. A large release train or historical commit disclosure can block a merge even when the current diff is clean. +Workflow authors do not run these operator steps automatically. Missing configuration deliberately blocks same-repository runs. Coordinate the rollout before making the check required. A release train's first scan after activation checks every commit since the last merged train, so a disclosure there can block a merge even when the current diff is clean; it needs a maintainer decision because pushed history cannot change. ## Limits and response GitHub may not deliver these events in every case. PR workflows do not run on merge conflicts; supported CI-skip directives can suppress runs; most writes made with `GITHUB_TOKEN` do not trigger a new workflow. Review these cases manually before merge. A required check cannot make an event happen. -The scanner does not read intermediate commit file content, binary content, release descriptions, old text revisions or every existing issue and comment. Added-then-removed content can remain in commit history. A clean result concerns the configured pattern and scanned fields only. Editing a current description leaves older revisions visible; the repository owner handles history cleanup and pushed-commit rewrites separately. +The scanner does not read intermediate commit file content, binary content, release descriptions, old text revisions, commit messages an earlier merged PR from the same branch already carried, or every existing issue and comment. Added-then-removed content can remain in commit history. A clean result concerns the configured pattern and scanned fields only. Editing a current description leaves older revisions visible; the repository owner handles history cleanup and pushed-commit rewrites separately. Actions notifications depend on each person's notification settings. A failed audit creates a failed run, not a guaranteed message to its author. This workflow posts no public comments and sends no chat messages. diff --git a/scripts/public_text_check.py b/scripts/public_text_check.py index 7822b4f..67ea819 100644 --- a/scripts/public_text_check.py +++ b/scripts/public_text_check.py @@ -31,6 +31,7 @@ class PullRequest: branch: str head: str base: str + base_branch: str head_repo: str commits: int files: int @@ -39,12 +40,18 @@ class PullRequest: def read(cls, value: dict) -> PullRequest: return cls( text(value["title"]), text(value.get("body")), text(value["head"]["ref"]), - sha(value["head"]["sha"]), sha(value["base"]["sha"]), + sha(value["head"]["sha"]), sha(value["base"]["sha"]), text(value["base"]["ref"]), text(value["head"]["repo"]["full_name"]), count(value["commits"]), count(value["changed_files"]), ) +@dataclass(frozen=True) +class Comparison: + merge_base: str + commits: tuple[dict, ...] + + def text(value: object) -> str: if value is None: return "" @@ -71,7 +78,7 @@ def sha(value: object) -> str: return value -def gh_api(path: str) -> list | dict: +def gh_pages(path: str) -> list: result = subprocess.run( ["gh", "api", "--paginate", "--slurp", path], check=True, capture_output=True, text=True, timeout=90, @@ -79,6 +86,11 @@ def gh_api(path: str) -> list | dict: pages = json.loads(result.stdout) if not isinstance(pages, list) or not pages: raise ScanError("empty response") + return pages + + +def gh_api(path: str) -> list | dict: + pages = gh_pages(path) if all(isinstance(page, list) for page in pages): return [item for page in pages for item in page] if len(pages) == 1 and isinstance(pages[0], dict): @@ -131,19 +143,6 @@ def added_lines(patch: str) -> list[tuple[int, str]]: return added -def missing_patch_is_empty_or_binary(repo: str, file: dict) -> bool: - # Never trust the extension. NUL in the first 8 KiB follows Git's binary - # heuristic; other missing patches fail closed. Large blobs need human review. - blob = gh_api(f"repos/{repo}/git/blobs/{sha(file['sha'])}") - size = count(blob["size"]) - if size > 1_048_576 or blob["encoding"] != "base64": - return False - content = base64.b64decode("".join(text(blob["content"]).split()), validate=True) - if len(content) != size: - return False - return (not content and count(file["additions"]) == 0) or b"\0" in content[:8000] - - def check_added(rx: re.Pattern[str], lines: list[tuple[int, str]], ordinal: int) -> list[Finding]: blocks: list[list[tuple[int, str]]] = [] for line in lines: @@ -158,6 +157,93 @@ def check_added(rx: re.Pattern[str], lines: list[tuple[int, str]], ordinal: int) return findings +def check_patchless(rx: re.Pattern[str], repo: str, file: dict, ordinal: int) -> list[Finding]: + """Check a file GitHub lists without a patch, or raise when its new text cannot be read whole.""" + # Never trust the extension. NUL in the first 8 KiB follows Git's binary + # heuristic. Larger blobs need human review. + blob = gh_api(f"repos/{repo}/git/blobs/{sha(file['sha'])}") + size = count(blob["size"]) + if size > 10 * 1_048_576 or blob["encoding"] != "base64": + raise ScanError("blob too large") + content = base64.b64decode("".join(text(blob["content"]).split()), validate=True) + if len(content) != size: + raise ScanError("incomplete blob") + if b"\0" in content[:8000] or (not content and count(file["additions"]) == 0): + return [] + # GitHub also omits patches for files it renders as binary, such as small + # PDFs with no NUL byte. A new file's whole content is its added text. + if file["status"] != "added" or size > 1_048_576: + raise ScanError("missing text patch") + lines = content.decode("utf-8", errors="replace").split("\n") + return check_added(rx, list(enumerate(lines, 1)), ordinal) + + +def compare(repo: str, base: str, head: str) -> Comparison: + # Unlike the PR commits endpoint, compare pages past 250 commits. Every + # page repeats the total, which proves that no page went missing. + pages = gh_pages(f"repos/{repo}/compare/{sha(base)}...{sha(head)}?per_page=100") + if not all(isinstance(page, dict) for page in pages): + raise ScanError("invalid pagination") + totals = {count(page["total_commits"]) for page in pages} + merge_bases = {sha(page["merge_base_commit"]["sha"]) for page in pages} + commits = tuple(commit for page in pages for commit in page["commits"]) + if len(totals) != 1 or len(merge_bases) != 1 or len(commits) != totals.pop(): + raise ScanError("incomplete commits") + return Comparison(merge_bases.pop(), commits) + + +def pr_commits(repo: str, endpoint: str, pr: PullRequest) -> list[dict]: + listed = gh_api(endpoint + "/commits?per_page=100") + if not isinstance(listed, list): + raise ScanError("incomplete commits") + commits = listed + if pr.commits > 250: + # The PR endpoint stops at 250 commits. Compare lists the same range + # in full, and must contain every commit the PR endpoint did list. + commits = list(compare(repo, pr.base, pr.head).commits) + if not {sha(c["sha"]) for c in listed} <= {sha(c["sha"]) for c in commits}: + raise ScanError("inconsistent commits") + if len(commits) != pr.commits: + raise ScanError("incomplete commits") + if len({sha(commit["sha"]) for commit in commits}) != len(commits): + raise ScanError("duplicate commits") + return commits + + +def merged_head(repo: str, number: int, pr: PullRequest) -> str | None: + """Head commit of the newest other merged PR from this branch into the same base.""" + head = quote(f"{repo.split('/')[0]}:{pr.branch}", safe="") + pulls = gh_api(f"repos/{repo}/pulls?state=closed&base={quote(pr.base_branch, safe='')}" + f"&head={head}&per_page=100") + if not isinstance(pulls, list): + raise ScanError("invalid pull list") + merged = [pull for pull in pulls + if pull["merged_at"] and identifier(pull["number"]) != number + and (pull["head"]["repo"] or {}).get("full_name") == repo] + if not merged: + return None + return sha(max(merged, key=lambda pull: text(pull["merged_at"]))["head"]["sha"]) + + +def carried_commits(repo: str, number: int, pr: PullRequest, commits: list[dict]) -> set[str]: + """SHAs of this PR's commits that an earlier merged PR from the same branch already carried. + + A squash merge never makes the branch's commits part of the base, so a + release train's next PR lists every earlier train's commits again. Those + messages merged once and stay in the branch's public history; rescanning + them would fail every later train on text nobody can change. + """ + previous = merged_head(repo, number, pr) + if previous is None: + return set() + since = compare(repo, previous, pr.head) + if since.merge_base != previous: + # The branch no longer descends from the merged head: scan everything. + return set() + new = {sha(commit["sha"]) for commit in since.commits} + return {sha(commit["sha"]) for commit in commits} - new + + def content_unchanged(repo: str, base: str, file: dict) -> bool: if (file["status"] not in {"modified", "renamed", "changed"} or count(file["additions"]) or count(file["deletions"])): @@ -172,23 +258,22 @@ def check_pr(rx: re.Pattern[str], *, repo: str, number: int, expected_head: str before = PullRequest.read(gh_api(endpoint)) if before.head_repo != repo or (expected_head and before.head != expected_head): raise ScanError("superseded or foreign head") - # REST caps these collections even when pagination follows every page. - if before.commits > 250 or before.files > 3000: + # REST caps the files collection even when pagination follows every page. + if before.files > 3000: raise ScanError("collection cap") findings = check_text(rx, label="PR title", text=before.title) findings += check_text(rx, label="PR description", text=before.body) findings += check_text(rx, label="head branch name", text=before.branch) - commits = gh_api(endpoint + "/commits?per_page=100") + commits = pr_commits(repo, endpoint, before) + carried = carried_commits(repo, number, before, commits) files = gh_api(endpoint + "/files?per_page=100") - if not isinstance(commits, list) or len(commits) != before.commits: - raise ScanError("incomplete commits") if not isinstance(files, list) or len(files) != before.files: raise ScanError("incomplete files") - if len({sha(commit["sha"]) for commit in commits}) != len(commits): - raise ScanError("duplicate commits") if len({text(file["filename"]) for file in files}) != len(files): raise ScanError("duplicate files") for i, commit in enumerate(commits, 1): + if sha(commit["sha"]) in carried: + continue findings += check_text(rx, label=f"commit {i} message", text=text(commit["commit"]["message"])) for i, file in enumerate(files, 1): if file["status"] == "removed": @@ -198,11 +283,9 @@ def check_pr(rx: re.Pattern[str], *, repo: str, number: int, expected_head: str findings += check_text(rx, label=f"file {i} path", text=text(file["filename"])) patch = file.get("patch") if not patch: - if content_unchanged(repo, before.base, file): - continue - if missing_patch_is_empty_or_binary(repo, file): - continue - raise ScanError("missing text patch") + if not content_unchanged(repo, before.base, file): + findings += check_patchless(rx, repo, file, i) + continue lines = added_lines(text(patch)) if len(lines) != count(file["additions"]): raise ScanError("incomplete added lines") diff --git a/tests/test_public_text_check.py b/tests/test_public_text_check.py index 05804da..818ef3f 100644 --- a/tests/test_public_text_check.py +++ b/tests/test_public_text_check.py @@ -7,25 +7,48 @@ import subprocess import sys from pathlib import Path +from urllib.parse import quote import pytest +import yaml -sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "scripts")) +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / "scripts")) import public_text_check as ptc RX = re.compile("zqcanary7731", re.I) PR_PATH = "repos/o/r/pulls/7" SHA = "a" * 40 +BASE = "b" * 40 +MERGED = "e" * 40 def pr(): return { "title": "fix", "body": "", "number": 7, "commits": 1, "changed_files": 1, "head": {"ref": "topic", "sha": SHA, "repo": {"full_name": "o/r"}}, - "base": {"sha": "b" * 40, "repo": {"full_name": "o/r"}}, + "base": {"sha": BASE, "ref": "main", "repo": {"full_name": "o/r"}}, } +def merged_path(branch="topic"): + return f"repos/o/r/pulls?state=closed&base=main&head={quote('o:' + branch, safe='')}&per_page=100" + + +def compare_path(base, head=SHA): + return f"repos/o/r/compare/{base}...{head}?per_page=100" + + +def compare_pages(commits, merge_base): + chunks = [commits[i:i + 100] for i in range(0, len(commits), 100)] or [[]] + return [{"total_commits": len(commits), "merge_base_commit": {"sha": merge_base}, "commits": chunk} + for chunk in chunks] + + +def commit(n, message="clean"): + return {"sha": f"{n:040x}", "commit": {"message": message}} + + def responses(): return { PR_PATH: pr(), @@ -36,11 +59,13 @@ def responses(): {"filename": "file.txt", "status": "added", "additions": 1, "deletions": 0, "sha": "c" * 40, "patch": "@@ -0,0 +1 @@\n+clean"} ], + merged_path(): [], } def mock_api(monkeypatch, data): monkeypatch.setattr(ptc, "gh_api", lambda path: copy.deepcopy(data[path])) + monkeypatch.setattr(ptc, "gh_pages", lambda path: copy.deepcopy(data[path])) def run_main(monkeypatch, tmp_path, event_name="pull_request", event=None, pattern="zqcanary7731"): @@ -62,6 +87,7 @@ def test_release_train_scans_commits(monkeypatch): data = responses() data[PR_PATH]["head"]["ref"] = "staging" data[PR_PATH + "/commits?per_page=100"][0]["commit"]["message"] = "zqcanary7731" + data[merged_path("staging")] = [] mock_api(monkeypatch, data) assert any("message" in f.where for f in ptc.check_pr(RX, repo="o/r", number=7)) @@ -73,9 +99,110 @@ def test_missing_text_patch_never_passes(monkeypatch, tmp_path): assert run_main(monkeypatch, tmp_path) == 2 -def test_commit_cap_never_passes(monkeypatch, tmp_path): +def train(data, carried_message, new_message, merge_base=MERGED): + """A staging train whose first commit an earlier merged train already carried.""" + data[PR_PATH]["head"]["ref"] = "staging" + data[PR_PATH]["commits"] = 2 + carried, new = commit(1, carried_message), commit(2, new_message) + data[PR_PATH + "/commits?per_page=100"] = [carried, new] + data[merged_path("staging")] = [ + {"number": 5, "merged_at": "2026-10-19T08:00:00Z", "head": {"sha": MERGED, "repo": {"full_name": "o/r"}}}, + ] + # Compare from the merged head also lists base-branch commits merged back + # into the train, which are outside the PR and must never be scanned. + data[compare_path(MERGED)] = compare_pages([new, commit(3, "zqcanary7731")], merge_base) + + +def test_train_skips_commits_an_earlier_merged_train_carried(monkeypatch, tmp_path): + data = responses() + train(data, carried_message="zqcanary7731", new_message="clean") + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +def test_train_scans_commits_no_earlier_train_carried(monkeypatch): + data = responses() + train(data, carried_message="clean", new_message="zqcanary7731") + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 2 message, line 1"] + + +def test_train_that_left_the_merged_head_scans_every_commit(monkeypatch): + data = responses() + train(data, carried_message="zqcanary7731", new_message="clean", merge_base="d" * 40) + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 1 message, line 1"] + + +def test_newest_merged_train_decides_what_was_carried(monkeypatch, tmp_path): + data = responses() + train(data, carried_message="zqcanary7731", new_message="clean") + data[merged_path("staging")].insert(0, { + "number": 4, "merged_at": "2026-10-12T08:00:00Z", "head": {"sha": "d" * 40, "repo": {"full_name": "o/r"}}, + }) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + +@pytest.mark.parametrize("change", ["unmerged", "fork", "deleted fork", "same PR"]) +def test_only_this_repository_s_other_merged_prs_count_as_carried(monkeypatch, change): + data = responses() + train(data, carried_message="zqcanary7731", new_message="clean") + pull = data[merged_path("staging")][0] + if change == "unmerged": + pull["merged_at"] = None + elif change == "fork": + pull["head"]["repo"] = {"full_name": "outside/r"} + elif change == "deleted fork": + pull["head"]["repo"] = None + else: + pull["number"] = 7 + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 1 message, line 1"] + + +def test_merged_pr_lookup_failure_fails(monkeypatch, tmp_path): + data = responses() + train(data, carried_message="clean", new_message="clean") + del data[merged_path("staging")] + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def big_train(data, total=251): + """A PR past the PR endpoint's 250-commit limit, which lists only the newest 250.""" + commits = [commit(n) for n in range(1, total + 1)] + data[PR_PATH]["commits"] = total + data[PR_PATH + "/commits?per_page=100"] = commits[-250:] + data[compare_path(BASE)] = compare_pages(commits, BASE) + return commits + + +def test_commits_past_250_are_listed_through_compare(monkeypatch): data = responses() - data[PR_PATH]["commits"] = 251 + commits = big_train(data) + commits[0]["commit"]["message"] = "zqcanary7731" + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 1 message, line 1"] + + +@pytest.mark.parametrize("damage", ["missing page", "page total", "merge base", "list pages", "foreign commit", "no compare"]) +def test_commits_past_250_without_a_complete_comparison_fail(monkeypatch, tmp_path, damage): + data = responses() + big_train(data) + pages = data[compare_path(BASE)] + if damage == "missing page": + pages.pop() + elif damage == "page total": + pages[0]["total_commits"] = 252 + elif damage == "merge base": + pages[1]["merge_base_commit"]["sha"] = "d" * 40 + elif damage == "list pages": + data[compare_path(BASE)] = [page["commits"] for page in pages] + elif damage == "foreign commit": + data[PR_PATH + "/commits?per_page=100"][0] = commit(999) + else: + del data[compare_path(BASE)] mock_api(monkeypatch, data) assert run_main(monkeypatch, tmp_path) == 2 @@ -113,6 +240,7 @@ def test_all_pr_fields_fail_without_echoing_text(monkeypatch, tmp_path, capsys, data[PR_PATH][field] = "zqcanary7731" elif field == "branch": data[PR_PATH]["head"]["ref"] = "fix/zqcanary7731" + data[merged_path("fix/zqcanary7731")] = [] elif field == "commit": data[PR_PATH + "/commits?per_page=100"][0]["commit"]["message"] = "zqcanary7731" elif field == "path": @@ -200,7 +328,7 @@ def blob(data, content): "size": len(content), "encoding": "base64", "content": base64.b64encode(content).decode(), } - data[PR_PATH + "/files?per_page=100"][0].pop("patch") + data[PR_PATH + "/files?per_page=100"][0].pop("patch", None) def test_binary_content_is_excluded_but_path_is_checked(monkeypatch, tmp_path): @@ -212,11 +340,38 @@ def test_binary_content_is_excluded_but_path_is_checked(monkeypatch, tmp_path): assert run_main(monkeypatch, tmp_path) == 1 -def test_text_with_binary_extension_is_not_exempt(monkeypatch, tmp_path): +def test_text_with_binary_extension_is_checked_as_text(monkeypatch, tmp_path): data = responses() blob(data, b"zqcanary7731") data[PR_PATH + "/files?per_page=100"][0]["filename"] = "logo.png" mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 1 + + +def test_new_file_without_patch_or_nul_is_checked_whole(monkeypatch, tmp_path): + # GitHub shows small PDFs as binary although they carry no NUL byte. + data = responses() + blob(data, b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\nzqcanary7731\n%%EOF\n") + data[PR_PATH + "/files?per_page=100"][0].update(filename="report.pdf", additions=0) + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["file 1, added line 3"] + blob(data, b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\nclean\n%%EOF\n") + assert run_main(monkeypatch, tmp_path) == 0 + + +def test_new_text_over_1_mib_without_patch_fails(monkeypatch, tmp_path): + data = responses() + blob(data, b"a" * (1_048_576 + 1)) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + +def test_binary_up_to_10_mib_passes_and_larger_blobs_fail(monkeypatch, tmp_path): + data = responses() + blob(data, b"\x1f\x8b\x08\0" + b"a" * (2 * 1_048_576)) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + blob(data, b"\x1f\x8b\x08\0" + b"a" * (10 * 1_048_576)) assert run_main(monkeypatch, tmp_path) == 2 @@ -344,7 +499,7 @@ def test_zero_counts_do_not_exempt_new_text_classified_as_binary(monkeypatch, tm blob(data, b"zqcanary7731") data[PR_PATH + "/files?per_page=100"][0].update(additions=0, deletions=0) mock_api(monkeypatch, data) - assert run_main(monkeypatch, tmp_path) == 2 + assert run_main(monkeypatch, tmp_path) == 1 def test_zero_counts_with_changed_blob_fail(monkeypatch, tmp_path): @@ -354,3 +509,18 @@ def test_zero_counts_with_changed_blob_fail(monkeypatch, tmp_path): data["repos/o/r/contents/file.txt?ref=" + "b" * 40] = {"type": "file", "sha": "d" * 40} mock_api(monkeypatch, data) assert run_main(monkeypatch, tmp_path) == 2 + + +def workflow(name): + return yaml.safe_load((ROOT / ".github" / "workflows" / name).read_text()) + + +def test_pr_gate_and_audit_never_share_a_check_or_queue(): + gate, audit = workflow("public-text.yml"), workflow("public-text-audit.yml") + # PyYAML reads the bare `on` key as True. + assert set(gate[True]) == {"pull_request"} + assert "pull_request" not in audit[True] + assert gate["name"] != audit["name"] + assert set(gate["jobs"]).isdisjoint(audit["jobs"]) + gate_group, audit_group = (wf["concurrency"]["group"].split("${{")[0] for wf in (gate, audit)) + assert not gate_group.startswith(audit_group) and not audit_group.startswith(gate_group) From 855f92465d76a02f44fc8eda337a95db1d12f78c Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Mon, 26 Oct 2026 00:46:50 -0700 Subject: [PATCH 3/5] fix(ci): reject duplicate compare commits and pin the guard's new checks with tests --- docs/public-text.md | 6 +-- scripts/public_text_check.py | 2 + tests/test_public_text_check.py | 74 +++++++++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/docs/public-text.md b/docs/public-text.md index 3ab3ccb..afac2d3 100644 --- a/docs/public-text.md +++ b/docs/public-text.md @@ -6,7 +6,7 @@ The guard fails a PR or text-audit run when current public text matches the priv | Event | Text read from the API | Result context | | --- | --- | --- | -| PR opened, edited, synchronized or reopened | Current title, description, head branch, all commit messages, current added lines and paths | `public-text / check` (verify on the first live run) | +| PR opened, edited, synchronized or reopened | Current title, description, head branch, commit messages an earlier merged PR from the branch did not carry, current added lines and paths | `public-text / check` (verify on the first live run) | | Issue opened or edited | Current title and description | `public-text-audit / check` | | Discussion comment created or edited | Current comment body | `public-text-audit / check` | | Review submitted or edited | Current review summary | `public-text-audit / check` | @@ -18,9 +18,9 @@ Release trains receive the same scan as any other PR. The scanner compares pagin A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. -Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A zero-addition, zero-deletion change can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. +Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A modified or renamed file with zero additions and zero deletions can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. -For other missing patches, the scanner reads the blob as data. It accepts an empty file, and treats a blob of up to 10 MiB with a NUL byte within the first 8,000 bytes as binary, checking only its path. GitHub also omits patches for files it renders as binary without a NUL byte, such as small PDFs. For an added file of up to 1 MiB with no NUL byte, the whole content is its added text, so the scanner checks all of it. Any other missing patch fails and needs maintainer review: a larger blob, or a modified file whose new content has no patch. +For other missing patches, the scanner reads the blob as data. It accepts an empty file, and treats a blob of up to 10 MiB with a NUL byte within the first 8,000 bytes as binary, checking only its path. GitHub also omits patches for files it renders as binary without a NUL byte, such as small PDFs. For an added file of up to 1 MiB with no NUL byte, the whole content is its added text, so the scanner checks all of it. Any other missing patch fails and needs maintainer review: a larger blob, or a modified file whose new content has no patch. Text stored with NUL bytes, such as UTF-16, counts as binary. Added text is checked in contiguous blocks so a wrapped phrase can match. The scanner does not join additions across intervening unchanged lines. diff --git a/scripts/public_text_check.py b/scripts/public_text_check.py index 67ea819..0206130 100644 --- a/scripts/public_text_check.py +++ b/scripts/public_text_check.py @@ -189,6 +189,8 @@ def compare(repo: str, base: str, head: str) -> Comparison: commits = tuple(commit for page in pages for commit in page["commits"]) if len(totals) != 1 or len(merge_bases) != 1 or len(commits) != totals.pop(): raise ScanError("incomplete commits") + if len({sha(commit["sha"]) for commit in commits}) != len(commits): + raise ScanError("duplicate commits") return Comparison(merge_bases.pop(), commits) diff --git a/tests/test_public_text_check.py b/tests/test_public_text_check.py index 818ef3f..7525ef6 100644 --- a/tests/test_public_text_check.py +++ b/tests/test_public_text_check.py @@ -92,9 +92,12 @@ def test_release_train_scans_commits(monkeypatch): assert any("message" in f.where for f in ptc.check_pr(RX, repo="o/r", number=7)) -def test_missing_text_patch_never_passes(monkeypatch, tmp_path): +def test_changed_text_without_patch_never_passes(monkeypatch, tmp_path): + # GitHub omits the patch of a large text diff; the new blob alone cannot + # tell which of its lines the PR added. data = responses() - del data[PR_PATH + "/files?per_page=100"][0]["patch"] + blob(data, b"clean\n") + data[PR_PATH + "/files?per_page=100"][0]["status"] = "modified" mock_api(monkeypatch, data) assert run_main(monkeypatch, tmp_path) == 2 @@ -134,16 +137,53 @@ def test_train_that_left_the_merged_head_scans_every_commit(monkeypatch): assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 1 message, line 1"] -def test_newest_merged_train_decides_what_was_carried(monkeypatch, tmp_path): +@pytest.mark.parametrize("position", [0, 1]) +def test_newest_merged_train_decides_what_was_carried(monkeypatch, tmp_path, position): data = responses() train(data, carried_message="zqcanary7731", new_message="clean") - data[merged_path("staging")].insert(0, { + data[merged_path("staging")].insert(position, { "number": 4, "merged_at": "2026-10-12T08:00:00Z", "head": {"sha": "d" * 40, "repo": {"full_name": "o/r"}}, }) mock_api(monkeypatch, data) assert run_main(monkeypatch, tmp_path) == 0 +def long_train(data): + """150 train commits, 140 of them new since the merged train: two compare pages.""" + commits = [commit(n) for n in range(1, 151)] + commits[-1]["commit"]["message"] = "zqcanary7731" + data[PR_PATH]["head"]["ref"] = "staging" + data[PR_PATH]["commits"] = len(commits) + data[PR_PATH + "/commits?per_page=100"] = commits + data[merged_path("staging")] = [ + {"number": 5, "merged_at": "2026-10-19T08:00:00Z", "head": {"sha": MERGED, "repo": {"full_name": "o/r"}}}, + ] + data[compare_path(MERGED)] = compare_pages(commits[10:], MERGED) + + +def test_new_train_commits_on_a_later_compare_page_are_checked(monkeypatch): + data = responses() + long_train(data) + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["commit 150 message, line 1"] + + +@pytest.mark.parametrize("damage", ["missing page", "page total", "duplicate"]) +def test_incomplete_comparison_never_marks_new_commits_carried(monkeypatch, tmp_path, damage): + data = responses() + long_train(data) + pages = data[compare_path(MERGED)] + if damage == "missing page": + pages.pop() + elif damage == "page total": + for page in pages: + page["total_commits"] = 141 + else: + pages[1]["commits"][-1] = pages[0]["commits"][0] + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + @pytest.mark.parametrize("change", ["unmerged", "fork", "deleted fork", "same PR"]) def test_only_this_repository_s_other_merged_prs_count_as_carried(monkeypatch, change): data = responses() @@ -359,6 +399,32 @@ def test_new_file_without_patch_or_nul_is_checked_whole(monkeypatch, tmp_path): assert run_main(monkeypatch, tmp_path) == 0 +def test_whole_file_check_reads_past_the_binary_probe(monkeypatch): + data = responses() + blob(data, b"a" * 9000 + b"\nzqcanary7731\n") + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(RX, repo="o/r", number=7)] == ["file 1, added line 2"] + + +def test_nul_after_the_first_8000_bytes_is_still_text(monkeypatch, tmp_path): + data = responses() + blob(data, b"zqcanary7731\n" + b"a" * 9000 + b"\0") + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 1 + + +@pytest.mark.parametrize("status", ["modified", "renamed"]) +def test_changed_binary_without_patch_checks_only_its_path(monkeypatch, tmp_path, status): + data = responses() + blob(data, b"\x89PNG\r\n\x1a\n\0zqcanary7731") + file = data[PR_PATH + "/files?per_page=100"][0] + file.update(status=status, additions=0, deletions=0, previous_filename="old.png") + data["repos/o/r/contents/old.png?ref=" + BASE] = {"type": "file", "sha": "d" * 40} + data["repos/o/r/contents/file.txt?ref=" + BASE] = {"type": "file", "sha": "d" * 40} + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 0 + + def test_new_text_over_1_mib_without_patch_fails(monkeypatch, tmp_path): data = responses() blob(data, b"a" * (1_048_576 + 1)) From ea601b9d68bd9fcdb15e2b6f81a469da0bd9214d Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Mon, 26 Oct 2026 00:46:30 -0700 Subject: [PATCH 4/5] fix(ci): list only commits since the last merged train in release PR bodies --- .github/workflows/release-pr.yml | 19 ++++++++++++++++++- docs/public-text.md | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release-pr.yml b/.github/workflows/release-pr.yml index eb4bd01..3bc1b42 100644 --- a/.github/workflows/release-pr.yml +++ b/.github/workflows/release-pr.yml @@ -143,7 +143,23 @@ jobs: run: | set -euo pipefail - COMMITS=$(git log --oneline --no-merges "origin/${BASE}..origin/${STAGING}") + # A squash merge never makes the staging commits part of the base, so + # BASE..STAGING still lists every train that already shipped. List only + # what the newest merged train did not carry: the same commits the + # public text check reads. Fall back to the whole range when staging + # no longer descends from that train's head. + RANGE=("origin/${BASE}..origin/${STAGING}") + SINCE_LINE="" + LAST=$(gh api --method GET --paginate --slurp "repos/${REPO}/pulls" \ + -f state=closed -f base="${BASE}" -f head="${REPO%%/*}:${STAGING}" -f per_page=100 \ + | jq -r --arg repo "${REPO}" \ + '[.[][] | select(.merged_at and .head.repo.full_name == $repo)] | max_by(.merged_at) // empty | "\(.number) \(.head.sha)"') \ + || LAST="" + if [ -n "$LAST" ] && git merge-base --is-ancestor "${LAST#* }" "origin/${STAGING}" 2>/dev/null; then + RANGE=("origin/${STAGING}" "^${LAST#* }" "^origin/${BASE}") + SINCE_LINE="Commits since #${LAST%% *}, the last merged release. Earlier ones already shipped." + fi + COMMITS=$(git log --oneline --no-merges "${RANGE[@]}") CONTRIBUTORS=$(git log --format='%an' "origin/${BASE}..origin/${STAGING}" | sort -u | sed 's/^/- /') # Find the PR before writing about it. The body describes whether this is @@ -182,6 +198,7 @@ jobs: **${AHEAD} commit(s)** queued for the next production release. ### Changes + ${SINCE_LINE} \`\`\` ${COMMITS} \`\`\` diff --git a/docs/public-text.md b/docs/public-text.md index afac2d3..d47ec24 100644 --- a/docs/public-text.md +++ b/docs/public-text.md @@ -16,7 +16,7 @@ The PR and audit callers have separate workflow names, job names and concurrency Release trains receive the same scan as any other PR. The scanner compares paginated collection sizes with the PR's counts. The PR commits endpoint stops at 250 commits, so a larger PR lists its commits through the compare endpoint, which must return the PR's exact count and every commit the PR endpoint listed. More than 3,000 files exceeds the files endpoint's limit and fails as incomplete. Duplicate data, malformed hunks and added-line count mismatches also fail. -A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. +A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. The release-train PR description lists the same new commits, so a carried commit's subject does not return through the description. Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A modified or renamed file with zero additions and zero deletions can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. From 50ed3b4f211ef4483572ce53884dd1ed9907bc8e Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Mon, 26 Oct 2026 03:08:23 -0700 Subject: [PATCH 5/5] fix(ci): fail closed on release history and text decoding --- .github/workflows/release-pr.yml | 3 +- docs/public-text.md | 4 +- scripts/public_text_check.py | 19 +++++- tests/test_public_text_check.py | 51 +++++++++++++++ tests/test_release_pr.py | 105 +++++++++++++++++++++++++++++++ 5 files changed, 178 insertions(+), 4 deletions(-) create mode 100644 tests/test_release_pr.py diff --git a/.github/workflows/release-pr.yml b/.github/workflows/release-pr.yml index 3bc1b42..7a8fc85 100644 --- a/.github/workflows/release-pr.yml +++ b/.github/workflows/release-pr.yml @@ -153,8 +153,7 @@ jobs: LAST=$(gh api --method GET --paginate --slurp "repos/${REPO}/pulls" \ -f state=closed -f base="${BASE}" -f head="${REPO%%/*}:${STAGING}" -f per_page=100 \ | jq -r --arg repo "${REPO}" \ - '[.[][] | select(.merged_at and .head.repo.full_name == $repo)] | max_by(.merged_at) // empty | "\(.number) \(.head.sha)"') \ - || LAST="" + '[.[][] | select(.merged_at and .head.repo.full_name == $repo)] | max_by(.merged_at) // empty | "\(.number) \(.head.sha)"') if [ -n "$LAST" ] && git merge-base --is-ancestor "${LAST#* }" "origin/${STAGING}" 2>/dev/null; then RANGE=("origin/${STAGING}" "^${LAST#* }" "^origin/${BASE}") SINCE_LINE="Commits since #${LAST%% *}, the last merged release. Earlier ones already shipped." diff --git a/docs/public-text.md b/docs/public-text.md index d47ec24..152c23d 100644 --- a/docs/public-text.md +++ b/docs/public-text.md @@ -16,12 +16,14 @@ The PR and audit callers have separate workflow names, job names and concurrency Release trains receive the same scan as any other PR. The scanner compares paginated collection sizes with the PR's counts. The PR commits endpoint stops at 250 commits, so a larger PR lists its commits through the compare endpoint, which must return the PR's exact count and every commit the PR endpoint listed. More than 3,000 files exceeds the files endpoint's limit and fails as incomplete. Duplicate data, malformed hunks and added-line count mismatches also fail. -A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. The release-train PR description lists the same new commits, so a carried commit's subject does not return through the description. +A squash merge never makes the head branch's commits part of the base, so a release train's next PR lists every earlier train's commits again. The scanner skips commits that the newest earlier merged PR from the same branch into the same base already carried, when the current head still descends from that PR's head. Those messages merged once and stay in the branch's public history; failing on them would block every later train on text nobody can change. Every other commit message is checked. If the head no longer descends from the merged head, every commit is checked. The release-train PR description lists the same new commits, so a carried commit's subject does not return through the description. Failure to fetch or parse the release history stops the body update before creating or editing a PR. A successful lookup with no earlier train, or an unsuccessful ancestry check, still uses the full range. Removed paths are excluded so a cleanup can remove a disclosure. Renames check only the new path and added content. A modified or renamed file with zero additions and zero deletions can omit its patch only when the scanner verifies that its blob equals the base file's blob, or verifies empty/binary content. This permits mode-only changes and unchanged renames without trusting line counts alone. A missing base path fails conservatively. For other missing patches, the scanner reads the blob as data. It accepts an empty file, and treats a blob of up to 10 MiB with a NUL byte within the first 8,000 bytes as binary, checking only its path. GitHub also omits patches for files it renders as binary without a NUL byte, such as small PDFs. For an added file of up to 1 MiB with no NUL byte, the whole content is its added text, so the scanner checks all of it. Any other missing patch fails and needs maintainer review: a larger blob, or a modified file whose new content has no patch. Text stored with NUL bytes, such as UTF-16, counts as binary. +Patchless text must decode as UTF-8 without replacing characters. The only fallback is a PDF binary-marker comment immediately after a `%PDF-1.0` through `%PDF-1.7` or `%PDF-2.0` header: a second line containing `%` and at least four bytes all in the range 128–255, with LF or CRLF line endings. That marker is represented one-to-one as Latin-1 and scanned; every remaining byte must decode as UTF-8. This preserves small PDFs with a binary marker and readable text, without guessing the encoding of their bodies. Other undecodable content fails as incomplete, including Latin-1 source files. A filename or encoding declaration does not grant an exception. + Added text is checked in contiguous blocks so a wrapped phrase can match. The scanner does not join additions across intervening unchanged lines. ## Trust and exceptions diff --git a/scripts/public_text_check.py b/scripts/public_text_check.py index 0206130..c03090d 100644 --- a/scripts/public_text_check.py +++ b/scripts/public_text_check.py @@ -157,6 +157,23 @@ def check_added(rx: re.Pattern[str], lines: list[tuple[int, str]], ordinal: int) return findings +def decode_patchless(content: bytes) -> str: + """Decode text without losing bytes; only a PDF binary marker may use Latin-1.""" + try: + return content.decode("utf-8") + except UnicodeDecodeError: + # A PDF header may be followed by a binary marker comment. Preserve and + # scan its high bytes one-to-one; everything else still requires UTF-8. + marker = re.match(rb"(%PDF-(?:1\.[0-7]|2\.0)\r?\n)(%[\x80-\xff]{4,})(\r?\n)", content) + if marker: + try: + return (marker[1].decode("ascii") + marker[2].decode("latin-1") + + content[marker.end(2):].decode("utf-8")) + except UnicodeDecodeError: + pass + raise ScanError("unsupported text encoding") from None + + def check_patchless(rx: re.Pattern[str], repo: str, file: dict, ordinal: int) -> list[Finding]: """Check a file GitHub lists without a patch, or raise when its new text cannot be read whole.""" # Never trust the extension. NUL in the first 8 KiB follows Git's binary @@ -174,7 +191,7 @@ def check_patchless(rx: re.Pattern[str], repo: str, file: dict, ordinal: int) -> # PDFs with no NUL byte. A new file's whole content is its added text. if file["status"] != "added" or size > 1_048_576: raise ScanError("missing text patch") - lines = content.decode("utf-8", errors="replace").split("\n") + lines = decode_patchless(content).split("\n") return check_added(rx, list(enumerate(lines, 1)), ordinal) diff --git a/tests/test_public_text_check.py b/tests/test_public_text_check.py index 7525ef6..531d7c1 100644 --- a/tests/test_public_text_check.py +++ b/tests/test_public_text_check.py @@ -399,6 +399,57 @@ def test_new_file_without_patch_or_nul_is_checked_whole(monkeypatch, tmp_path): assert run_main(monkeypatch, tmp_path) == 0 +@pytest.mark.parametrize("content,pattern", [ + (b'# coding: latin-1\nname = "zqcaf\xe97731"\n', "zqcafé7731"), + (b"zqcanary\xa07731\n", r"zqcanary\s+7731"), + (b"clean\n\xe2\x82", "zqcanary7731"), +]) +def test_patchless_text_with_unknown_encoding_fails_without_echo(monkeypatch, tmp_path, capsys, content, pattern): + data = responses() + blob(data, content) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path, pattern=pattern) == 2 + output = capsys.readouterr() + assert "incomplete" in output.out + assert "zqcanary" not in output.out + output.err + assert "zqcaf" not in output.out + output.err + + +@pytest.mark.parametrize("newline", [b"\n", b"\r\n"]) +def test_pdf_marker_fallback_preserves_bytes_and_line_numbers(monkeypatch, newline): + data = responses() + blob(data, newline.join([b"%PDF-1.7", b"%\xe2\xe3\xcf\xd3\xff", b"zqcanary7731", b"%%EOF"])) + mock_api(monkeypatch, data) + pattern = re.compile("âãÏÓÿ|zqcanary7731") + assert [f.where for f in ptc.check_pr(pattern, repo="o/r", number=7)] == [ + "file 1, added line 2", "file 1, added line 3", + ] + + +def test_valid_utf8_pdf_marker_keeps_its_text(monkeypatch): + data = responses() + blob(data, "%PDF-2.0\n%éééé\nclean\n%%EOF".encode()) + mock_api(monkeypatch, data) + assert [f.where for f in ptc.check_pr(re.compile("éééé"), repo="o/r", number=7)] == [ + "file 1, added line 2", + ] + + +@pytest.mark.parametrize("content", [ + b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\nzqcaf\xe97731\n%%EOF\n", + b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\nzqcanary\xa07731\n%%EOF\n", + b"%PDF-1.4\n%\xe2\xe3\xcf\xd3zqcanary\xa07731\n%%EOF\n", + b"%PDF-1.4\n%\xe2\xe3\xcf\nclean\n%%EOF\n", + b"%PDF-1.4\nclean\n%\xe2\xe3\xcf\xd3\n%%EOF\n", + b"not PDF\n%\xe2\xe3\xcf\xd3\nclean\n", +]) +def test_pdf_marker_does_not_exempt_other_undecodable_text(monkeypatch, tmp_path, content): + data = responses() + blob(data, content) + mock_api(monkeypatch, data) + assert run_main(monkeypatch, tmp_path) == 2 + + def test_whole_file_check_reads_past_the_binary_probe(monkeypatch): data = responses() blob(data, b"a" * 9000 + b"\nzqcanary7731\n") diff --git a/tests/test_release_pr.py b/tests/test_release_pr.py new file mode 100644 index 0000000..bca972d --- /dev/null +++ b/tests/test_release_pr.py @@ -0,0 +1,105 @@ +"""Run the release-body step without GitHub access or repository credentials.""" + +import json +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parents[1] +MERGED = "e" * 40 + + +def run_release_step(tmp_path, *, response, api_exit=0, ancestor=0, existing="7"): + workflow = yaml.safe_load((ROOT / ".github/workflows/release-pr.yml").read_text()) + steps = [step for step in workflow["jobs"]["create-pr"]["steps"] + if step.get("name") == "Create or update PR"] + assert len(steps) == 1 + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + (tmp_path / "response.json").write_text(response) + gh = bin_dir / "gh" + gh.write_text(f"#!{sys.executable}\n" + ''' +import json, os, sys +from pathlib import Path +args = sys.argv[1:] +with Path("gh-calls.jsonl").open("a") as handle: + handle.write(json.dumps(args) + "\\n") +if args[0] == "api": + sys.stdout.write(Path("response.json").read_text()) + sys.exit(int(os.environ["API_EXIT"])) +elif args[:2] == ["pr", "list"]: + print(os.environ["EXISTING"]) +elif args[:2] == ["pr", "view"]: + print("true") +elif args[:2] in (["pr", "create"], ["pr", "edit"]): + Path("body.md").write_text(args[args.index("--body") + 1]) + print("https://github.com/o/r/pull/7") +else: + raise AssertionError(args) +''') + git = bin_dir / "git" + git.write_text(f"#!{sys.executable}\n" + ''' +import json, os, sys +from pathlib import Path +args = sys.argv[1:] +with Path("git-calls.jsonl").open("a") as handle: + handle.write(json.dumps(args) + "\\n") +if args[:2] == ["merge-base", "--is-ancestor"]: + assert args[2:] == [os.environ["MERGED"], "origin/staging"] + sys.exit(int(os.environ["ANCESTOR"])) +elif args == ["log", "--format=%an", "origin/main..origin/staging"]: + print("Synthetic contributor") +elif args[:3] == ["log", "--oneline", "--no-merges"]: + if args[3:] == ["origin/main..origin/staging"]: + print("old zqcanary7731 carried subject") + else: + assert args[3:] == ["origin/staging", "^" + os.environ["MERGED"], "^origin/main"] + print("new clean subject") +else: + raise AssertionError(args) +''') + gh.chmod(0o755) + git.chmod(0o755) + jq = shutil.which("jq") + bash = shutil.which("bash") + assert jq and bash, "release workflow tests require jq and bash" + result = subprocess.run( + [bash, "--noprofile", "--norc", "-c", steps[0]["run"]], + cwd=tmp_path, text=True, capture_output=True, timeout=15, + env={ + "PATH": f"{bin_dir}:{Path(jq).parent}:/usr/bin:/bin", + "REPO": "o/r", "BASE": "main", "STAGING": "staging", "AHEAD": "2", + "FROZEN": "false", "MERGED": MERGED, "API_EXIT": str(api_exit), + "ANCESTOR": str(ancestor), "EXISTING": existing, + }, + ) + calls = [json.loads(line) for line in (tmp_path / "gh-calls.jsonl").read_text().splitlines()] + return result, calls + + +@pytest.mark.parametrize("existing", ["", "7"]) +@pytest.mark.parametrize("response,api_exit", [("", 1), ("not JSON", 0)]) +def test_failed_history_lookup_never_creates_or_edits_pr(tmp_path, existing, response, api_exit): + result, calls = run_release_step(tmp_path, response=response, api_exit=api_exit, existing=existing) + assert result.returncode != 0 + assert [call[0] for call in calls] == ["api"] + assert not (tmp_path / "body.md").exists() + + +@pytest.mark.parametrize("history,ancestor", [(False, 0), (True, 1), (True, 0)]) +def test_successful_lookup_preserves_empty_diverged_and_carried_ranges(tmp_path, history, ancestor): + merged = {"number": 5, "merged_at": "2026-10-19T08:00:00Z", + "head": {"sha": MERGED, "repo": {"full_name": "o/r"}}} + response = json.dumps([[merged]] if history else [[]]) + result, calls = run_release_step(tmp_path, response=response, ancestor=ancestor) + assert result.returncode == 0, result.stderr + assert calls[-1][:2] == ["pr", "edit"] + body = (tmp_path / "body.md").read_text() + assert "new clean subject" in body + carried = history and ancestor == 0 + assert ("zqcanary7731" not in body) == carried + assert ("Commits since #5" in body) == carried