From 5278fbc9da6fc5d3cf28fffff9b627e49bb2b5a6 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 14:20:43 -0700 Subject: [PATCH 1/2] feat(groom): extract CI-privileged patch path policy into tested patch_policy.py + close lockfile/.husky/action.yml gaps (BE-4404) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The groom auto-builder's Capture patch step enforced its CI-privileged deny-list as an untestable inline `grep -E`, and still missed live execution vectors — the biggest being dependency lockfiles (a rewritten `resolved`+`integrity` pair runs an attacker tarball's postinstall in credentialed CI before human review). Extract the deny-list into `.github/groom/patch_policy.py` (denied_paths + a NUL-delimited stdin main(), exit 0 always), port the existing patterns verbatim, and add lockfiles, `.husky/`, composite-action manifests, `.gitmodules`, and the common build/test config across JS/Python/Rust/Ruby/Swift/Gradle/Bazel/CMake. Add a unit suite (positive root+nested, anchored negatives, git raw-byte quote/ newline regressions). Wire the step to `python3 "$GROOM_ASSETS/patch_policy.py"`, trim the stale comment to a pointer, and document the policy + its structural limit (source still executes in caller test CI) in the README and header. No change to bail semantics or the builder default; no caller touched. --- .github/groom/README.md | 39 +++++ .github/groom/patch_policy.py | 172 ++++++++++++++++++ .github/groom/tests/test_patch_policy.py | 212 +++++++++++++++++++++++ .github/workflows/groom.yml | 64 ++++--- 4 files changed, 452 insertions(+), 35 deletions(-) create mode 100644 .github/groom/patch_policy.py create mode 100644 .github/groom/tests/test_patch_policy.py diff --git a/.github/groom/README.md b/.github/groom/README.md index 22de4ee..62e6913 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -28,6 +28,20 @@ ledger's PR-state stops that finding from being re-proposed. The builder holds n credentials — it can only produce a *patch*, never push. Default off: the finds-only groomer (issues) stays the default. +A builder patch that touches a **CI-privileged path** — workflow/action +definitions, dependency **lockfiles** (`package-lock.json`, `pnpm-lock.yaml`, +`Cargo.lock`, …), package manifests, or build/test config — is downgraded from a +PR to a filed issue: on a same-repo branch push that code executes in +credentialed CI *before* a human reads the diff (review gates merge, not CI +exec). The deny-list is the tested [`patch_policy.py`](patch_policy.py) (BE-4404) +— a conservative default, not a proof of completeness, so read it before setting +`builder: true` on a repo whose CI runs something else privileged. **Structural +limit:** the policy guards privileged-*config* surfaces, but any patch's source +code still executes when the caller's CI runs its *tests* — a review-gated PR is +untrusted code running pre-merge. Callers enabling the builder should avoid +exposing secrets to test steps and consider `npm ci --ignore-scripts` (or +equivalents) where viable. + These two files are the **single source of truth** for the groom prompts, the same way [`.github/cursor-review/`](../cursor-review) is for the review panel. The core thesis of the groom initiative is *collaborate on the prompt, not the @@ -172,3 +186,28 @@ python3 .github/groom/ledger.py --repo owner/name --check "" ```bash python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v ``` + +## `patch_policy.py` — the CI-privileged patch deny-list (BE-4404) + +The auto-builder's `Capture patch` step must never open an auto-PR whose patch +touches a path the caller's CI *executes* before a human reviews the merge — +that would run builder-authored (untrusted) code with repository secrets. The +patterns that decide this were an untestable inline `grep -E`; `patch_policy.py` +extracts them so they carry a unit-test suite, and closes the biggest live gap +(dependency **lockfiles** — `npm ci` re-resolves and runs their tarballs' install +scripts) plus `.husky/`, composite-action manifests, `.gitmodules`, and the +common build files across the JS/Python/Rust/Ruby/Swift/Gradle/Bazel/CMake +ecosystems. + +- `denied_paths(paths)` returns the CI-privileged subset of the changed paths. +- `main()` reads NUL-delimited paths from stdin (matching `git diff --cached + --name-only -z`) and prints the matches, **exit 0 always** — the caller tests + non-emptiness. NUL delimiting is mandatory: git C-quotes exotic paths in its + default output, slipping them past the anchors; `-z` emits raw bytes. +- The list is a conservative **default, not a proof of completeness** — over-block + is safe (a false positive only downgrades a PR to an issue), under-block is the + hole. A repo whose CI runs something else privileged must add it here first. + +```bash +python3 -m unittest discover -s .github/groom/tests -p test_patch_policy.py -v +``` diff --git a/.github/groom/patch_policy.py b/.github/groom/patch_policy.py new file mode 100644 index 0000000..e149599 --- /dev/null +++ b/.github/groom/patch_policy.py @@ -0,0 +1,172 @@ +#!/usr/bin/env python3 +r"""CI-privileged patch-path policy for the groom auto-builder (BE-4404). + +The groom `build` job hands a prompt-injectable, credential-free builder agent a +finding and lets it edit a checkout; a later job opens a **review-gated** PR from +that patch. Review gates the *merge*, not CI *execution* — a same-repo branch +push runs the caller's CI (with repository secrets + a writable token) BEFORE any +human reads the diff. So a patch that touches a path the caller's pre-review CI +*executes* is a code-execution primitive, not a doc change. This module is the +deny-list that downgrades such a patch from an auto-PR to a filed issue (a human +authors the privileged change instead). + +`denied_paths(paths)` returns the subset of changed paths that are CI-privileged; +`main()` reads NUL-delimited paths from stdin (the `git diff --cached +--name-only -z` producer in groom.yml's `Capture patch` step) and prints the +matches, exit 0 always — the caller tests non-emptiness, not the exit code. + +Two load-bearing invariants (see also the trimmed comment in groom.yml): + +1. **Conservative DEFAULT, not proof of completeness.** The list covers the paths + a *typical* repo's CI executes (workflow/action defs, package manifests, + lockfiles, build/test config across ecosystems). It is target-repo-specific: + a repo whose CI runs something else privileged (a checked-in `scripts/` + entrypoint) must add it here. Erring WIDE is the safe direction — a false + positive only downgrades a legit refactor from a PR to an issue, and the + finding still lands in the ledger + a `groom` issue; nothing is dropped. Never + under-block. + +2. **NUL-delimited comparison is mandatory.** git C-quotes and double-quote-wraps + any path containing a quote, backslash or control byte in its DEFAULT output, + so `.github/workflows/ev"il.yml` is emitted as `".github/workflows/ev\"il.yml"` + whose leading quote slips past a `^\.github/` anchor. `-z` emits raw bytes with + no quoting; this module reads those bytes. A path may itself contain a newline + (`-z` does not escape it), so each path is split on newlines and EVERY line is + tested — mirroring the old `tr '\0' '\n' | grep` line semantics; that + over-blocks a newline-bearing path, never under-blocks (the safe direction). + +Run/test: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +""" + +import re +import sys +from collections.abc import Iterable + +# --- Patterns --------------------------------------------------------------- +# Ported verbatim from the inline `grep -E` the `Capture patch` step used before +# BE-4404 (root-anchored `.github/` prefixes + the basename set), then extended. + +# Path prefixes matched ROOT-anchored only (verbatim port — `^\.github/...`). +_ROOT_PREFIXES = ( + r"\.github/workflows/", + r"\.github/actions/", +) + +# Directory segments privileged wherever they appear in the path +# (`.husky/pre-commit`, `frontend/.husky/...`, `android/gradle/wrapper/...`). +_SEGMENT_PREFIXES = ( + r"\.husky/", + r"gradle/wrapper/", +) + +# Exact basenames, privileged at ANY depth (`package.json`, `sub/package.json`). +_BASENAMES = ( + # --- ported verbatim (pre-BE-4404) --- + "package.json", + "Makefile", + "GNUmakefile", + "conftest.py", + "noxfile.py", + "tox.ini", + "pytest.ini", + "setup.py", + "setup.cfg", + "pyproject.toml", + "Dockerfile", + ".pre-commit-config.yaml", + # --- added by BE-4404: lockfiles --- + # `npm ci` / `yarn install` / `pip install -r` re-resolve dependency tarballs + # from these on every CI run, so a rewritten `resolved` URL + `integrity` pair + # executes an attacker tarball's postinstall in credentialed CI before review + # (the BE-4012 headline gap — the pilot caller runs `npm ci` on every push). + "package-lock.json", + "npm-shrinkwrap.json", + "yarn.lock", + "pnpm-lock.yaml", + "bun.lock", + "bun.lockb", + "poetry.lock", + "uv.lock", + "Pipfile.lock", + "Cargo.lock", + "Gemfile.lock", + # --- added by BE-4404: other build/test config that runs in pre-review CI --- + "action.yml", + "action.yaml", + ".gitmodules", + "Pipfile", + "Cargo.toml", + "build.rs", + "Package.swift", + "settings.gradle", # also covered by the *.gradle glob; explicit for clarity + "Gemfile", + "Rakefile", + "CMakeLists.txt", + "WORKSPACE", + "WORKSPACE.bazel", + "BUILD", + "BUILD.bazel", + "Jenkinsfile", + "Taskfile.yml", + "justfile", + "Justfile", +) + +# Basename globs (`*` = any run of non-`/` chars), privileged at ANY depth. +_BASENAME_GLOBS = ( + "requirements*.txt", # requirements.txt, requirements-dev.txt, ... + "*.pbxproj", + "*.gradle", + "*.gradle.kts", + "*.gemspec", + "*.cmake", + "*.bzl", +) + + +def _glob_to_regex(glob: str) -> str: + """Translate a basename glob to a regex fragment (`*` never crosses `/`).""" + return re.escape(glob).replace(r"\*", r"[^/]*") + + +_PATTERN = re.compile( + "|".join( + [rf"^{p}" for p in _ROOT_PREFIXES] + + [rf"(?:^|/){p}" for p in _SEGMENT_PREFIXES] + + [rf"(?:^|/){re.escape(b)}$" for b in _BASENAMES] + + [rf"(?:^|/){_glob_to_regex(g)}$" for g in _BASENAME_GLOBS] + ) +) + + +def _is_denied(path: str) -> bool: + """True if `path` (or any of its newline-split lines) is CI-privileged.""" + # Split on newlines and test every line: git -z can emit a path containing a + # raw newline, and the old grep matched line-by-line. Testing every line + # over-blocks such a path, never under-blocks (the safe direction). + return any(_PATTERN.search(line) for line in path.split("\n")) + + +def denied_paths(paths: Iterable[str]) -> list[str]: + """Return the subset of `paths` that touch a CI-privileged surface.""" + return [p for p in paths if _is_denied(p)] + + +def parse_nul_delimited(data: bytes) -> list[str]: + """Decode `git ... -z` output (NUL-delimited raw path bytes) to str paths. + + `-z` emits paths verbatim with NO C-quoting, so bytes are decoded with + `surrogateescape` (which never raises) to preserve exotic/non-UTF-8 paths for + matching. Empty fields (e.g. the trailing one after the final NUL) are dropped. + """ + return [chunk.decode("utf-8", "surrogateescape") for chunk in data.split(b"\x00") if chunk] + + +def main() -> int: + for path in denied_paths(parse_nul_delimited(sys.stdin.buffer.read())): + sys.stdout.write(path + "\n") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/groom/tests/test_patch_policy.py b/.github/groom/tests/test_patch_policy.py new file mode 100644 index 0000000..19d210a --- /dev/null +++ b/.github/groom/tests/test_patch_policy.py @@ -0,0 +1,212 @@ +#!/usr/bin/env python3 +"""Tests for the groom CI-privileged patch-path policy (BE-4404). + +The property the policy must hold: a patch that touches a path the caller's +pre-review CI would EXECUTE is denied (downgraded from an auto-PR to a filed +issue), and the deny must survive git's raw `-z` output — including paths that +carry an embedded quote or newline. Over-blocking is safe; under-blocking is the +security hole, so every "must NOT match" case anchors a basename exactly. + +Run: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +""" + +import importlib.util +import os +import unittest + +_MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "patch_policy.py") +_spec = importlib.util.spec_from_file_location("groom_patch_policy", _MODULE_PATH) +policy = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(policy) + + +def denied(path): + """True if a single path is denied by the policy.""" + return policy.denied_paths([path]) == [path] + + +class PortedPatternsTest(unittest.TestCase): + """The pre-BE-4404 patterns must still match, at root and nested — no regression.""" + + def test_github_workflows_and_actions(self): + for p in ( + ".github/workflows/ci.yml", + ".github/workflows/nested/deploy.yaml", + ".github/actions/build/action.yml", + ): + self.assertTrue(denied(p), p) + + def test_manifests_and_build_config_root_and_nested(self): + for name in ( + "package.json", + "Makefile", + "GNUmakefile", + "conftest.py", + "noxfile.py", + "tox.ini", + "pytest.ini", + "setup.py", + "setup.cfg", + "pyproject.toml", + "Dockerfile", + ".pre-commit-config.yaml", + ): + self.assertTrue(denied(name), name) + self.assertTrue(denied("sub/pkg/" + name), "sub/pkg/" + name) + + +class LockfilesTest(unittest.TestCase): + """The BE-4404 headline gap: dependency lockfiles re-resolved by CI installs.""" + + LOCKFILES = ( + "package-lock.json", + "npm-shrinkwrap.json", + "yarn.lock", + "pnpm-lock.yaml", + "bun.lock", + "bun.lockb", + "poetry.lock", + "uv.lock", + "Pipfile.lock", + "Cargo.lock", + "Gemfile.lock", + ) + + def test_lockfiles_root_and_nested(self): + for name in self.LOCKFILES: + self.assertTrue(denied(name), name) + self.assertTrue(denied("packages/app/" + name), "nested " + name) + + +class AddedPatternsTest(unittest.TestCase): + """The rest of the BE-4404 additions, each root and nested where meaningful.""" + + def test_husky_any_segment(self): + for p in (".husky/pre-commit", "frontend/.husky/pre-push", "a/b/.husky/commit-msg"): + self.assertTrue(denied(p), p) + + def test_action_manifests(self): + for p in ("action.yml", "action.yaml", "tools/action.yml", "deep/dir/action.yaml"): + self.assertTrue(denied(p), p) + + def test_gitmodules(self): + self.assertTrue(denied(".gitmodules")) + self.assertTrue(denied("vendor/.gitmodules")) + + def test_requirements_glob(self): + for p in ( + "requirements.txt", + "requirements-dev.txt", + "requirements_test.txt", + "svc/requirements.txt", + ): + self.assertTrue(denied(p), p) + + def test_python_and_rust_and_swift(self): + for p in ("Pipfile", "Cargo.toml", "build.rs", "Package.swift", "crates/core/Cargo.toml"): + self.assertTrue(denied(p), p) + + def test_xcode_and_gradle(self): + for p in ( + "MyApp.xcodeproj/project.pbxproj", + "build.gradle", + "app/build.gradle", + "build.gradle.kts", + "settings.gradle", + "gradle/wrapper/gradle-wrapper.properties", + "android/gradle/wrapper/gradle-wrapper.jar", + ): + self.assertTrue(denied(p), p) + + def test_ruby(self): + for p in ("Gemfile", "Rakefile", "myproj.gemspec", "gems/foo.gemspec"): + self.assertTrue(denied(p), p) + + def test_cmake_and_bazel(self): + for p in ( + "CMakeLists.txt", + "src/CMakeLists.txt", + "cmake/toolchain.cmake", + "WORKSPACE", + "WORKSPACE.bazel", + "BUILD", + "pkg/BUILD", + "BUILD.bazel", + "rules/defs.bzl", + ): + self.assertTrue(denied(p), p) + + def test_task_runners(self): + for p in ("Jenkinsfile", "Taskfile.yml", "justfile", "Justfile", "ci/Jenkinsfile"): + self.assertTrue(denied(p), p) + + +class NegativeCasesTest(unittest.TestCase): + """Anchoring: near-misses must NOT match, or the deny-list would over-fire noise.""" + + def test_plain_source_and_docs_are_allowed(self): + for p in ( + "src/foo.py", + "README.md", + "docs/guide.md", + "src/index.ts", + "test/data.json", + ): + self.assertFalse(denied(p), p) + + def test_basename_prefixes_and_suffixes_do_not_match(self): + # The ticket's named anchoring regressions, plus a few more. + for p in ( + "packages.json", # not package.json + "Dockerfile.md", # not Dockerfile + "mypackage.json", # not package.json + "app/gradlew", # not *.gradle + "BUILD.md", # not BUILD / BUILD.bazel + "myrequirements.txt", # requirements* glob is basename-anchored + "notpyproject.toml", # not pyproject.toml + ): + self.assertFalse(denied(p), p) + + def test_husky_substring_is_not_a_segment(self): + # `.husky` must be a full path SEGMENT, not a substring of one. + self.assertFalse(denied("not.husky.file"), "not.husky.file") + + +class RawByteRegressionTest(unittest.TestCase): + """git C-quotes exotic paths in DEFAULT output; the policy reads raw `-z` bytes.""" + + def test_embedded_quote_caught_from_raw_bytes(self): + # `.github/workflows/ev"il.yml` — default git output would quote-wrap this, + # slipping the leading quote past a `^\.github/` anchor. Raw -z bytes don't. + raw = b'.github/workflows/ev"il.yml' + paths = policy.parse_nul_delimited(raw) + self.assertEqual(policy.denied_paths(paths), ['.github/workflows/ev"il.yml']) + + def test_embedded_newline_split_both_lines_tested(self): + # A single path carrying a raw newline arrives (via -z) as one field; the + # policy splits it and tests BOTH lines. Match on either => denied. + # (a) match on the first line + paths = policy.parse_nul_delimited(b".github/workflows/x.yml\nsecond-line") + self.assertEqual(len(paths), 1) + self.assertEqual(policy.denied_paths(paths), paths) + # (b) match on the SECOND line — proves both are tested, not just the first + paths = policy.parse_nul_delimited(b"innocent-first\npackage.json") + self.assertEqual(len(paths), 1) + self.assertEqual(policy.denied_paths(paths), paths) + + def test_nul_delimited_multiple_paths_partition(self): + raw = b".github/workflows/ci.yml\x00README.md\x00package-lock.json\x00" + paths = policy.parse_nul_delimited(raw) + self.assertEqual(paths, [".github/workflows/ci.yml", "README.md", "package-lock.json"]) + self.assertEqual( + policy.denied_paths(paths), + [".github/workflows/ci.yml", "package-lock.json"], + ) + + def test_empty_input_denies_nothing(self): + self.assertEqual(policy.parse_nul_delimited(b""), []) + self.assertEqual(policy.denied_paths([]), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index c846955..12574f3 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -19,6 +19,16 @@ name: Groom (reusable) # human review — they are NEVER auto-merged. The remaining findings (DOWNGRADE, # security, beyond-cap) are still filed as `groom` issues (hybrid). Default off. # +# Patch-path policy (BE-4404): a builder patch that touches a CI-privileged path +# (workflow/action defs, dependency lockfiles, package manifests, build/test +# config) is downgraded from a PR to a filed issue — that code would execute in +# credentialed pre-review CI. The deny-list is the tested `.github/groom/ +# patch_policy.py`. STRUCTURAL LIMIT: the deny-list guards privileged-CONFIG +# surfaces, but ANY patch's source still executes when the caller's CI runs its +# tests — a review-gated PR is still untrusted code running before merge. Callers +# enabling `builder: true` should avoid exposing secrets to test steps and +# consider `npm ci --ignore-scripts` (or equivalents) where viable. +# # Topology mirrors cursor-review.yml (this repo is the single source of truth — # the finder/verifier BRIEFS + the dedup ledger live under .github/groom/, so a # consumer carries only a thin caller and there's no logic to keep in sync). @@ -1535,50 +1545,34 @@ jobs: exit 0 fi CHANGED=$(git -C repo diff --cached --numstat | awk '{a+=($1=="-"?0:$1); d+=($2=="-"?0:$2)} END{print a+d+0}') - # Path policy: a patch touching `.github/workflows/` would, on the + # Path policy: a patch touching a CI-privileged path would, on the # same-repo branch push, run in CI with repository secrets + a writable # token BEFORE any human reviews the merge — defeating the RFC's # "review-gated, patch-only" boundary (review gates merge, not CI exec). - # A prompt-injected builder must never self-author CI. Refuse such a - # patch and file an issue so a human authors the workflow change. - # - # The deny-list covers the paths that actually execute in that pre-review CI: - # the workflow + composite-action definitions (`.github/workflows/`, - # `.github/actions/`) AND the build/test config the CI of a typical repo - # invokes — npm lifecycle scripts (`package.json`), `Makefile`, and the - # Python test/build config (`conftest.py`, `noxfile.py`, `tox.ini`, - # `pytest.ini`, `setup.py`, `setup.cfg`, `pyproject.toml`), plus - # `Dockerfile` and `.pre-commit-config.yaml`. Each is a "runs with - # repository secrets before a human reads the diff" window, exactly like a - # workflow file. Earlier revisions blocked only the first two and left the - # rest as a comment-guarded KNOWN GAP; a comment is not a control. - # - # STILL read this BEFORE setting `builder: true` on any caller: the precise - # list is target-repo-specific and this is a conservative DEFAULT, not a - # proof of completeness. A repo whose CI runs something else privileged - # (a checked-in `scripts/` entrypoint, a Gradle/Bazel build file) must add - # it here first. Erring wide is the safe direction — a false positive only - # downgrades a legitimate refactor from a PR to an issue, and nothing is - # dropped either way. - # - # Paths are compared NUL-delimited (`--name-only -z`), not via the default - # output: git C-quotes and double-quote-wraps any path containing a quote, - # backslash or control byte, so `.github/workflows/ev"il.yml` is emitted as - # `".github/workflows/ev\"il.yml"` — whose leading quote slips straight past - # the `^\.github/` anchor. `-c core.quotePath=false` did NOT fix this; it - # only suppresses quoting of high (non-ASCII) bytes. `-z` emits raw bytes - # with no quoting at all. Translating NUL to newline can only SPLIT a path - # that itself contains a newline, and both halves are then tested — that - # over-blocks, never under-blocks, which is the safe direction here. - TOUCHED_CI=$(git -C repo diff --cached --name-only -z | tr '\0' '\n' \ - | grep -E '^\.github/(workflows|actions)/|(^|/)(package\.json|Makefile|GNUmakefile|conftest\.py|noxfile\.py|tox\.ini|pytest\.ini|setup\.py|setup\.cfg|pyproject\.toml|Dockerfile|\.pre-commit-config\.yaml)$' || true) + # A prompt-injected builder must never self-author code that pre-review CI + # executes. The deny-list lives in the tested `patch_policy.py` (BE-4404) + # so the pattern set is unit-tested, not an untestable inline grep. Two + # load-bearing notes survive here: + # (a) it is a conservative DEFAULT, not a proof of completeness — the + # precise list is target-repo-specific, so STILL read patch_policy.py + # BEFORE setting `builder: true` on any caller (a repo whose CI runs + # something else privileged must add it there). Erring wide is safe: + # a false positive only downgrades a legit refactor from a PR to an + # issue, and nothing is dropped either way. + # (b) NUL-delimited comparison is mandatory (`--name-only -z`): git + # C-quotes and double-quote-wraps any path with a quote/backslash/ + # control byte in default output, slipping the path past a `^\.github/` + # anchor; `-z` emits raw bytes so patch_policy.py sees the true path. + # $GROOM_ASSETS already points into the _groom_assets checkout (workflow + # env, same var the prompt-build steps use). + TOUCHED_CI=$(git -C repo diff --cached --name-only -z | python3 "$GROOM_ASSETS/patch_policy.py" || true) if [ "$STATUS" != "patched" ] || [ ! -s /tmp/out/patch.diff ]; then echo "Build $IDX produced no patch (status=$STATUS) — will file as an issue." bail "${SUMMARY:-builder produced no patch}" elif [ -n "$TOUCHED_CI" ]; then echo "::warning::Build $IDX patch edits CI-privileged file(s) [$(echo "$TOUCHED_CI" | tr '\n' ' ')] — refusing to auto-open a PR that would run in credentialed CI before human review; filing an issue instead." - bail "patch modifies a CI-privileged path (.github/workflows|actions, or build/test config that executes in pre-review CI) — a human must author these changes" + bail "patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes" elif [ "$CHANGED" -gt "$PR_SIZE_LIMIT" ]; then echo "::warning::Build $IDX patch changed $CHANGED lines (> $PR_SIZE_LIMIT) — filing an issue instead of a giant PR." bail "patch too large: ${CHANGED} lines changed (> ${PR_SIZE_LIMIT})" From ef119a87b0a74f535f66e12bac2c68effd893775 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 14:51:47 -0700 Subject: [PATCH 2/2] fix(groom): close patch_policy under-blocks + fail-open flagged by review panel (BE-4404) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the cursor-review panel findings on the CI-privileged patch deny-list: - groom.yml: drop `|| true` on the patch_policy.py call. main() always exits 0 now, so `|| true` only masked real failures (missing $GROOM_ASSETS, bad interpreter, crash) — an empty TOUCHED_CI then fails OPEN. Under the step's `set -euo pipefail`, letting a failure abort fails CLOSED (no PR). - groom.yml: strip C0 control bytes from denied paths before interpolating them into the `::warning::` line — a path named `…\r::stop-commands::…` could inject workflow commands (the runner treats bare `\r` as a line terminator). - patch_policy.py main(): write raw bytes via surrogateescape so a non-UTF-8 denied path is emitted (and blocked), not crashed on by strict text stdout. - patch_policy.py: close under-blocks in the module's own "never under-block" direction — lowercase `makefile`/`rakefile`, `gradlew`/`gradlew.bat`, Go (`go.mod`/`go.sum`/`go.work`/`go.work.sum`), `Taskfile.yaml`+lowercase, `Package.resolved`, `.pnpmfile.cjs`, `.cargo/config[.toml]`, multi-Dockerfile globs. Match case-insensitively (macOS/Windows CI runners are). - patch_policy.py: `denied_paths` rejects a bare str/bytes (char-iteration footgun). - Tests + README updated to match. Co-Authored-By: Claude Opus 4.8 --- .github/groom/README.md | 5 +- .github/groom/patch_policy.py | 58 ++++++++++++- .github/groom/tests/test_patch_policy.py | 103 ++++++++++++++++++++++- .github/workflows/groom.yml | 17 +++- 4 files changed, 173 insertions(+), 10 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 62e6913..de4f9e1 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -196,8 +196,9 @@ patterns that decide this were an untestable inline `grep -E`; `patch_policy.py` extracts them so they carry a unit-test suite, and closes the biggest live gap (dependency **lockfiles** — `npm ci` re-resolves and runs their tarballs' install scripts) plus `.husky/`, composite-action manifests, `.gitmodules`, and the -common build files across the JS/Python/Rust/Ruby/Swift/Gradle/Bazel/CMake -ecosystems. +common build files across the JS/Python/Rust/Ruby/Swift/Go/Gradle/Bazel/CMake +ecosystems. Matching is **case-insensitive** — macOS/Windows CI runners resolve +`PACKAGE.JSON` to the real file, so the Linux checker must too. - `denied_paths(paths)` returns the CI-privileged subset of the changed paths. - `main()` reads NUL-delimited paths from stdin (matching `git diff --cached diff --git a/.github/groom/patch_policy.py b/.github/groom/patch_policy.py index e149599..87c0d3d 100644 --- a/.github/groom/patch_policy.py +++ b/.github/groom/patch_policy.py @@ -65,6 +65,8 @@ "package.json", "Makefile", "GNUmakefile", + "makefile", # GNU Make searches GNUmakefile, makefile, Makefile — and PREFERS + # lowercase `makefile` over `Makefile`, so it must be denied too. "conftest.py", "noxfile.py", "tox.ini", @@ -101,6 +103,7 @@ "settings.gradle", # also covered by the *.gradle glob; explicit for clarity "Gemfile", "Rakefile", + "rakefile", # `rake` accepts the lowercase form too. "CMakeLists.txt", "WORKSPACE", "WORKSPACE.bazel", @@ -108,8 +111,33 @@ "BUILD.bazel", "Jenkinsfile", "Taskfile.yml", + "Taskfile.yaml", + "taskfile.yml", # go-task also loads the lowercase spellings. + "taskfile.yaml", "justfile", "Justfile", + # Gradle wrapper scripts: Gradle CI invokes `./gradlew` (or `gradlew.bat` on + # Windows) directly, so an edited wrapper runs arbitrary code. These are at the + # repo root with no extension, so the `*.gradle` glob and `gradle/wrapper/` + # segment (which cover the jar/properties) do NOT reach them — deny by name. + "gradlew", + "gradlew.bat", + # Go: `go build`/`go test` in CI re-fetch modules from these; a `replace` + # directive in go.mod can redirect a dependency to attacker-controlled code. + "go.mod", + "go.sum", + "go.work", + "go.work.sum", + "Package.resolved", # SwiftPM pins resolved from this on `swift build`. + ".pnpmfile.cjs", # pnpm executes this hook during install. +) + +# Path suffixes anchored on a full segment boundary (basenames won't do — these +# carry a directory component). `.cargo/config[.toml]` can set a build `runner` or +# linker that executes during `cargo build`/`cargo test` in pre-review CI. +_PATH_SUFFIXES = ( + r"\.cargo/config\.toml", + r"\.cargo/config", ) # Basename globs (`*` = any run of non-`/` chars), privileged at ANY depth. @@ -121,6 +149,12 @@ "*.gemspec", "*.cmake", "*.bzl", + # Multi-Dockerfile conventions CI builds via `docker build -f`: the + # `Dockerfile.` suffix form (Dockerfile.prod) and the `.Dockerfile` + # extension form (prod.Dockerfile). `Dockerfile*` also over-catches + # `Dockerfile.md`-style docs — a safe over-block (downgrades to an issue). + "Dockerfile*", + "*.Dockerfile", ) @@ -129,13 +163,22 @@ def _glob_to_regex(glob: str) -> str: return re.escape(glob).replace(r"\*", r"[^/]*") +# IGNORECASE: macOS and Windows CI runners use case-INSENSITIVE filesystems, so a +# builder can commit `PACKAGE.JSON` / `MAKEFILE` / `DOCKERFILE` which a +# case-sensitive Linux checker would miss but which resolves to the real, +# CI-executed file once checked out on the runner. The policy explicitly targets +# Swift/Xcode (macOS) callers, so match case-insensitively. This also over-blocks +# some lowercase near-collisions (e.g. a file literally named `build`) — the safe +# direction per the module contract (over-block only downgrades a PR to an issue). _PATTERN = re.compile( "|".join( [rf"^{p}" for p in _ROOT_PREFIXES] + [rf"(?:^|/){p}" for p in _SEGMENT_PREFIXES] + [rf"(?:^|/){re.escape(b)}$" for b in _BASENAMES] + [rf"(?:^|/){_glob_to_regex(g)}$" for g in _BASENAME_GLOBS] - ) + + [rf"(?:^|/){s}$" for s in _PATH_SUFFIXES] + ), + re.IGNORECASE, ) @@ -149,6 +192,10 @@ def _is_denied(path: str) -> bool: def denied_paths(paths: Iterable[str]) -> list[str]: """Return the subset of `paths` that touch a CI-privileged surface.""" + # A bare `str` is iterable; without this guard a caller that passes one path as + # a string would iterate its characters and silently under-block. Fail loud. + if isinstance(paths, (str, bytes)): + raise TypeError("denied_paths expects an iterable of paths, not a single str/bytes") return [p for p in paths if _is_denied(p)] @@ -163,8 +210,15 @@ def parse_nul_delimited(data: bytes) -> list[str]: def main() -> int: + # Write raw bytes, not text: `parse_nul_delimited` decodes with `surrogateescape`, + # so a denied path carrying non-UTF-8 bytes holds lone surrogates that the default + # strict `sys.stdout` (text) would reject with UnicodeEncodeError — crashing on the + # very path we must report. Round-trip through the same codec so a non-UTF-8 + # CI-privileged path is still emitted (and thus still blocked), never dropped. + out = sys.stdout.buffer for path in denied_paths(parse_nul_delimited(sys.stdin.buffer.read())): - sys.stdout.write(path + "\n") + out.write(path.encode("utf-8", "surrogateescape") + b"\n") + out.flush() return 0 diff --git a/.github/groom/tests/test_patch_policy.py b/.github/groom/tests/test_patch_policy.py index 19d210a..59d8c08 100644 --- a/.github/groom/tests/test_patch_policy.py +++ b/.github/groom/tests/test_patch_policy.py @@ -115,11 +115,15 @@ def test_xcode_and_gradle(self): "settings.gradle", "gradle/wrapper/gradle-wrapper.properties", "android/gradle/wrapper/gradle-wrapper.jar", + # Wrapper scripts CI runs via `./gradlew` — root, no extension. + "gradlew", + "gradlew.bat", + "android/gradlew", ): self.assertTrue(denied(p), p) def test_ruby(self): - for p in ("Gemfile", "Rakefile", "myproj.gemspec", "gems/foo.gemspec"): + for p in ("Gemfile", "Rakefile", "rakefile", "myproj.gemspec", "gems/foo.gemspec"): self.assertTrue(denied(p), p) def test_cmake_and_bazel(self): @@ -137,10 +141,101 @@ def test_cmake_and_bazel(self): self.assertTrue(denied(p), p) def test_task_runners(self): - for p in ("Jenkinsfile", "Taskfile.yml", "justfile", "Justfile", "ci/Jenkinsfile"): + for p in ( + "Jenkinsfile", + "Taskfile.yml", + "Taskfile.yaml", + "taskfile.yml", + "taskfile.yaml", + "justfile", + "Justfile", + "ci/Jenkinsfile", + ): + self.assertTrue(denied(p), p) + + +class PanelHardeningTest(unittest.TestCase): + """Under-blocks closed after the BE-4404 cursor-review panel — each an + executed-in-pre-review-CI surface the conservative default first missed.""" + + def test_lowercase_makefile(self): + # GNU Make prefers lowercase `makefile` over `Makefile`. + for p in ("makefile", "sub/makefile"): + self.assertTrue(denied(p), p) + + def test_go_modules(self): + for p in ("go.mod", "go.sum", "go.work", "go.work.sum", "svc/api/go.mod"): + self.assertTrue(denied(p), p) + + def test_dockerfile_variants(self): + for p in ( + "Dockerfile", # plain (regression guard) + "Dockerfile.prod", # suffix form + "Dockerfile.dev", + "prod.Dockerfile", # extension form + "docker/api.Dockerfile", + ): + self.assertTrue(denied(p), p) + + def test_cargo_config(self): + for p in (".cargo/config.toml", ".cargo/config", "sub/.cargo/config.toml"): + self.assertTrue(denied(p), p) + + def test_swift_and_pnpm_extras(self): + for p in ("Package.resolved", "ios/Package.resolved", ".pnpmfile.cjs", "app/.pnpmfile.cjs"): + self.assertTrue(denied(p), p) + + def test_case_insensitive_on_case_insensitive_runners(self): + # macOS/Windows CI checks out `PACKAGE.JSON` as the real `package.json`. + for p in ("PACKAGE.JSON", "MAKEFILE", "DOCKERFILE", "sub/Package-Lock.JSON"): self.assertTrue(denied(p), p) +class ApiGuardTest(unittest.TestCase): + """`denied_paths` must reject a bare str/bytes (a silent character-iteration footgun).""" + + def test_bare_str_raises(self): + with self.assertRaises(TypeError): + policy.denied_paths("package.json") + + def test_bare_bytes_raises(self): + with self.assertRaises(TypeError): + policy.denied_paths(b"package.json") + + def test_list_is_accepted(self): + self.assertEqual(policy.denied_paths(["package.json"]), ["package.json"]) + + +class MainStdoutTest(unittest.TestCase): + """`main()` must emit a non-UTF-8 denied path without crashing (surrogateescape).""" + + def test_non_utf8_denied_path_is_emitted_not_crashed(self): + import io + + # A denied path (package.json) whose DIRECTORY segment carries a raw non-UTF-8 + # byte (0xff) — the basename still anchors, so it is denied. git -z emits it + # verbatim; parse_nul_delimited holds the byte as a lone surrogate. + raw = b"p\xffkg/package.json\x00package.json\x00" + stdin = io.BytesIO(raw) + stdout_buf = io.BytesIO() + + class _Stdin: + buffer = stdin + + class _Stdout: + buffer = stdout_buf + + orig_in, orig_out = policy.sys.stdin, policy.sys.stdout + try: + policy.sys.stdin, policy.sys.stdout = _Stdin(), _Stdout() + rc = policy.main() + finally: + policy.sys.stdin, policy.sys.stdout = orig_in, orig_out + self.assertEqual(rc, 0) + # Both paths denied; the non-UTF-8 one survives round-trip as raw bytes. + self.assertEqual(stdout_buf.getvalue(), b"p\xffkg/package.json\npackage.json\n") + + class NegativeCasesTest(unittest.TestCase): """Anchoring: near-misses must NOT match, or the deny-list would over-fire noise.""" @@ -158,12 +253,12 @@ def test_basename_prefixes_and_suffixes_do_not_match(self): # The ticket's named anchoring regressions, plus a few more. for p in ( "packages.json", # not package.json - "Dockerfile.md", # not Dockerfile "mypackage.json", # not package.json - "app/gradlew", # not *.gradle "BUILD.md", # not BUILD / BUILD.bazel "myrequirements.txt", # requirements* glob is basename-anchored "notpyproject.toml", # not pyproject.toml + "gocode/foo.go", # a .go source file is not go.mod/go.sum + "cargofile.toml", # not the `.cargo/config.toml` path suffix ): self.assertFalse(denied(p), p) diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 12574f3..7ca2d00 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -1565,13 +1565,26 @@ jobs: # anchor; `-z` emits raw bytes so patch_policy.py sees the true path. # $GROOM_ASSETS already points into the _groom_assets checkout (workflow # env, same var the prompt-build steps use). - TOUCHED_CI=$(git -C repo diff --cached --name-only -z | python3 "$GROOM_ASSETS/patch_policy.py" || true) + # NO `|| true`: patch_policy.py always exits 0 on success (a no-match is + # empty output, not a nonzero exit, unlike the old `grep`), so `|| true` + # would only mask a REAL failure (missing $GROOM_ASSETS, wrong path, bad + # interpreter, uncaught exception). Under this step's `set -euo pipefail`, + # letting such a failure abort the step fails CLOSED — no PR is opened when + # the gate can't run — instead of fail-open (empty TOUCHED_CI => PR opened). + TOUCHED_CI=$(git -C repo diff --cached --name-only -z | python3 "$GROOM_ASSETS/patch_policy.py") if [ "$STATUS" != "patched" ] || [ ! -s /tmp/out/patch.diff ]; then echo "Build $IDX produced no patch (status=$STATUS) — will file as an issue." bail "${SUMMARY:-builder produced no patch}" elif [ -n "$TOUCHED_CI" ]; then - echo "::warning::Build $IDX patch edits CI-privileged file(s) [$(echo "$TOUCHED_CI" | tr '\n' ' ')] — refusing to auto-open a PR that would run in credentialed CI before human review; filing an issue instead." + # Strip control bytes before interpolating attacker-influenced paths into + # a workflow-command line: `tr '\n' ' '` alone leaves CR (and other C0 + # controls) intact, and the Actions runner treats a bare `\r` as a line + # terminator — a denied path named `…\r::stop-commands::…` would inject + # arbitrary workflow commands. `tr -d '\000-\037'` drops all C0 controls + # (newlines are already spaces by then); this line is human-readable only, + # the authoritative deny already happened above. + echo "::warning::Build $IDX patch edits CI-privileged file(s) [$(echo "$TOUCHED_CI" | tr '\n' ' ' | tr -d '\000-\037')] — refusing to auto-open a PR that would run in credentialed CI before human review; filing an issue instead." bail "patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes" elif [ "$CHANGED" -gt "$PR_SIZE_LIMIT" ]; then echo "::warning::Build $IDX patch changed $CHANGED lines (> $PR_SIZE_LIMIT) — filing an issue instead of a giant PR."