From 7aa9251b385f62ba63e6d68146afbdbba3fd00e9 Mon Sep 17 00:00:00 2001 From: Jake Fineman Date: Tue, 8 Sep 2026 11:50:48 -0400 Subject: [PATCH] fix(ci): split public-repo-guard into two workflow files to stop the required tree-scan check from wedging on comment/review bursts The `guard` job (check-run name "Secrets + content policy", the required status context in this repos ruleset) previously shared one workflow file, and one `on:` trigger set, with the body-scan job. That shared trigger set had to include pull_request_review and pull_request_review_comment for the body scan, which meant every review-bot comment also re-fired the tree-scan job under a required-name check-run, and the job's cancel-in-progress:true concurrency group killed the previous run each time. Observed live: a burst of 4 bot comments within 68 seconds produced 7 check-runs named "Secrets + content policy" on one commit (5 cancelled, 2 success) - the checks tab showed the latest as green, but the commits status-check rollup reported FAILURE and the PR was permanently blocked from merging despite the gate genuinely passing. Splitting the tree scan and body scan into separate workflow files removes the shared trigger set entirely: the tree-scan workflows `on:` block now lists only events that can change the published tree (pull_request open/reopen/sync, push, workflow_dispatch). A review comment no longer matches this workflows trigger at all, so no check-run - skipped, cancelled, or otherwise - is ever published under the required name for that event. Coverage is unchanged: every tree-changing event still gets a real gitleaks + content-policy run, and every PR/issue/comment/review body is still scanned by the body-guard job in the new public-repo-guard-body.yml, unchanged from before. See the block comment at the top of public-repo-guard.yml for the full incident writeup. --- .github/workflows/public-repo-guard-body.yml | 122 +++++++++++++ .github/workflows/public-repo-guard.yml | 183 ++++++------------- 2 files changed, 181 insertions(+), 124 deletions(-) create mode 100644 .github/workflows/public-repo-guard-body.yml diff --git a/.github/workflows/public-repo-guard-body.yml b/.github/workflows/public-repo-guard-body.yml new file mode 100644 index 0000000..060e94d --- /dev/null +++ b/.github/workflows/public-repo-guard-body.yml @@ -0,0 +1,122 @@ +name: public-repo-guard-body + +# The other half of public-repo-guard.yml's coverage, deliberately in its OWN +# workflow file — see the long comment block at the top of public-repo-guard.yml +# for the incident (wave-av/cli PR #68) that caused the split and why it is a +# file-level split, not just a job-level one. +# +# `guard` (in public-repo-guard.yml) scans the published TREE and produces the +# REQUIRED check "Secrets + content policy". This job scans a PR/issue/comment +# BODY, which is just as world-readable and, until this job existed, was scanned +# by nothing server-side. That gap was real, not theoretical: a PR was blocked +# for naming a private repo in wrangler.toml while the very same name, with more +# operational detail attached, sat unchallenged in its body. +# +# This job's check-run name ("Body content policy") is NOT a required status +# context in this repo's ruleset, so it can safely trigger on every comment/review +# event without any risk of masking or wedging the required tree-scan context — +# that is the entire reason it lives in a separate file from the tree scan. +# +# Honest about what it can and cannot do. On a PR this PREVENTS the merge. On an +# issue or comment the text is already public the moment it posts, so this is +# detection — it tells us to go redact, fast. Only the client-side pre-write hook +# can stop that class before publication. +on: + # `edited` matters as much as `opened`: a body can be made to leak long after + # the PR is first raised, and until this job covered it, nothing re-scanned it. + pull_request: + types: [opened, edited, reopened, synchronize] + issues: + types: [opened, edited] + issue_comment: + types: [created, edited] + # Inline review comments on a diff are a SEPARATE event from issue_comment — + # without this trigger they are world-readable text that no job ever scans. + pull_request_review_comment: + types: [created, edited] + # A submitted review's top-level body (the free-text field above any inline + # comments) is yet another world-readable payload, separate from BOTH comment + # events — without this trigger nothing ever scans it. + pull_request_review: + types: [submitted, edited] + +# `pull_request`, deliberately NOT `pull_request_target`: a fork PR must never get +# a write token or repo secrets just because a gate wanted to read its body. +permissions: + contents: read + +jobs: + body-guard: + name: Body content policy + concurrency: + # Keyed on the specific comment / review / PR / issue rather than github.ref, + # because issue events all report the default branch and a ref-keyed group + # would let two comments cancel each other, leaving one unscanned. The comment + # and review ids come FIRST: those payloads also carry the PR number, and + # keying them on the PR would collapse two rapid comments into one group, + # dropping a verdict. + # + # cancel-in-progress is deliberately FALSE. Every version of a body deserves a + # verdict, the job is seconds long, and a cancelled check-run lingers on the + # commit. Since this check-run name is not required, a lingering cancelled + # run here cannot wedge a merge the way the tree scan's could — but a dropped + # verdict on a body would still be a real coverage gap, so the same "let it + # finish" policy applies. + group: public-repo-guard-body-${{ github.event.comment.id || github.event.review.id || github.event.pull_request.number || github.event.issue.number || github.ref }} + cancel-in-progress: false + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Only the gate's own scripts are needed — no reason to pay for the whole + # tree on every comment. + sparse-checkout: scripts/public-repo-guard + sparse-checkout-cone-mode: false + # This job only reads the scripts — never leave the token sitting in + # .git/config while repo-supplied scripts execute in the workspace. + persist-credentials: false + + # Same rationale as the tree job: body-policy.sh needs a PCRE2-enabled rg, + # and Ubuntu's apt package has none. + - name: Install ripgrep (pinned + checksum-verified, PCRE2 build) + env: + RIPGREP_VERSION: "14.1.1" + RIPGREP_SHA256: "4cf9f2741e6c465ffdb7c26f38056a59e2a2544b51f7cc128ef28337eeae4d8e" + run: | + if command -v rg >/dev/null && rg --pcre2-version >/dev/null 2>&1; then + echo "using preinstalled $(rg --version | head -n1) with PCRE2"; exit 0 + fi + curl -fsSL --proto '=https' --tlsv1.2 -o ripgrep.tar.gz \ + "https://github.com/BurntSushi/ripgrep/releases/download/${RIPGREP_VERSION}/ripgrep-${RIPGREP_VERSION}-x86_64-unknown-linux-musl.tar.gz" + echo "${RIPGREP_SHA256} ripgrep.tar.gz" | sha256sum -c - + tar -xzf ripgrep.tar.gz --strip-components=1 "ripgrep-${RIPGREP_VERSION}-x86_64-unknown-linux-musl/rg" + sudo install -m 0755 rg /usr/local/bin/rg + rm -f rg ripgrep.tar.gz + rg --pcre2-version + + # The body is read straight out of the event payload FILE and written to + # another file. It is never interpolated into a run: block and never placed + # in an environment variable, so shell metacharacters in a hostile PR body + # have nothing to act on. jq is preinstalled on the GitHub-hosted images. + - name: Materialize the untrusted title/body to a file + run: | + set -euo pipefail + mkdir -p "$RUNNER_TEMP/bodyscan" + # An UNRECOGNIZED payload shape must fail, never quietly scan nothing and + # report a pass. If the event schema ever moves, this job must go red + # rather than become a green rubber stamp over an unscanned body. + if [ "$(jq -r 'has("pull_request") or has("issue") or has("comment") or has("review")' "$GITHUB_EVENT_PATH")" != "true" ]; then + echo "::error title=public-repo-guard-body::Event payload contains no pull_request/issue/comment/review object — refusing to report a pass on an unscanned body." + exit 1 + fi + jq -r '[.pull_request.title, .pull_request.body, + .issue.title, .issue.body, + .comment.body, .review.body] + | map(select(. != null)) | join("\n")' \ + "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/bodyscan/body.txt" + echo "scanning $(wc -l < "$RUNNER_TEMP/bodyscan/body.txt") line(s) of body text" + + - name: body policy (PR / issue / comment text) + env: + GUARD_PRIVATE_REPOS: ${{ vars.GUARD_PRIVATE_REPOS }} + run: bash scripts/public-repo-guard/body-policy.sh "$RUNNER_TEMP/bodyscan/body.txt" diff --git a/.github/workflows/public-repo-guard.yml b/.github/workflows/public-repo-guard.yml index 7b2da26..73f9685 100644 --- a/.github/workflows/public-repo-guard.yml +++ b/.github/workflows/public-repo-guard.yml @@ -1,6 +1,7 @@ name: public-repo-guard -# Pre-publication content gate for WAVE public repos. Two complementary checks: +# Pre-publication content gate for WAVE public repos. Two complementary checks, +# split across TWO workflow files (this one, plus public-repo-guard-body.yml): # 1. gitleaks — formatted secrets (API keys, tokens, private keys) in the tree. # 2. content-policy.sh — WAVE-specific leaks gitleaks misses: live Stripe account # IDs, hardcoded Cloudflare account_ids, developer absolute paths, references @@ -13,8 +14,9 @@ name: public-repo-guard # wave-av/.github must not be able to alter another repo's secret scanner). The # gitleaks binary is version-pinned AND SHA-256-verified before it runs. # -# To install on a new repo, copy all five files together: +# To install on a new repo, copy all six files together: # .github/workflows/public-repo-guard.yml +# .github/workflows/public-repo-guard-body.yml # .gitleaks.toml # scripts/public-repo-guard/content-policy.sh # scripts/public-repo-guard/body-policy.sh @@ -25,25 +27,53 @@ name: public-repo-guard # # Allowlisting: annotate a verified-safe line with `# guard:allow `, add a # path glob to a repo-root `.guardignore`, or extend the repo-local `.gitleaks.toml`. - +# +# WHY THIS IS A SEPARATE WORKFLOW FROM public-repo-guard-body.yml (this used to be +# one file with two jobs — see PR history for the incident that split it): +# +# The `guard` job below produces the check-run named "Secrets + content policy", +# which is the REQUIRED status context in this repo's branch-protection ruleset. +# Two failure modes are possible for any job that produces a required check-run +# from a shared, comment/review-triggered `on:` block, and BOTH were hit in +# production before this split: +# +# (a) MASKING: if this job's `on:` trigger set includes pull_request_review / +# pull_request_review_comment (needed by the BODY scan, not the tree scan) +# and the job then uses a job-level `if:` to skip those events (because the +# tree hasn't changed), GitHub still publishes a check-run named "Secrets + +# content policy" with conclusion `skipped` on that event. Branch protection +# treats `skipped` as passing, and the newest check-run for a name wins — so +# a bare review comment could flip an already-FAILED required tree scan +# green with nothing re-examining the tree. Real incident: this repo shipped +# a version that closed (a) by making the job run for real (never skip) on +# every PR-context event instead of skipping. +# +# (b) CHURN / FALSE BLOCK: making the job run for real on every review comment, +# combined with `cancel-in-progress: true` (needed so a genuine new commit +# supersedes a stale scan promptly), means a burst of review-bot comments — +# which do not change the tree at all — repeatedly re-fires and cancels the +# SAME job. Every cancelled run leaves a `cancelled` check-run attached to +# the commit under the required name. Observed live: wave-av/cli PR #68, +# head 90b00ded3 — 3 CodeRabbit + 1 gitar-bot comment within 68s produced 7 +# check-runs named "Secrets + content policy" (5 cancelled, 2 success); the +# checks tab showed the latest as green, but the commit's status-check +# rollup reported FAILURE and the PR was permanently MERGEABLE/BLOCKED even +# though the gate had genuinely passed. +# +# (a) and (b) are the SAME structural problem: this job's required check-run name +# was reachable from an `on:` trigger set that also had to serve comment/review +# events for the (unrelated) body scan. Splitting into two workflow FILES — not +# just two jobs — removes the shared trigger set entirely: this file's `on:` block +# now lists ONLY events that can change the tree (pull_request open/reopen/sync, +# push, workflow_dispatch). A review comment or a title/body edit never matches +# this workflow's trigger at all, so GitHub never runs it and never publishes ANY +# check-run — skipped, cancelled, or otherwise — under the required name for that +# event. There is nothing left to mask and nothing left to cancel. Coverage is +# unchanged: every event that can actually alter the published tree still gets a +# real, non-skippable gitleaks + content-policy run, exactly as before. on: - # `edited` matters as much as `opened`: a body can be made to leak long after the - # PR is first raised, and until this workflow covered it, nothing ever re-scanned. pull_request: - types: [opened, edited, reopened, synchronize] - issues: - types: [opened, edited] - issue_comment: - types: [created, edited] - # Inline review comments on a diff are a SEPARATE event from issue_comment — - # without this trigger they are world-readable text that no job ever scans. - pull_request_review_comment: - types: [created, edited] - # A submitted review's top-level body (the free-text field above any inline - # comments) is yet another world-readable payload, separate from BOTH comment - # events — without this trigger nothing ever scans it. - pull_request_review: - types: [submitted, edited] + types: [opened, reopened, synchronize] push: branches: [main, master] workflow_dispatch: @@ -53,27 +83,20 @@ on: permissions: contents: read -# Concurrency is per JOB, not per workflow: the two jobs want opposite behaviour. -# A workflow-level group would force one policy on both, and it showed: rapid body -# edits cancelled the tree job over and over, and every cancelled check-run stays -# attached to the commit, so the PR reported UNSTABLE while the live runs were green. - jobs: guard: name: Secrets + content policy - # Skips ONLY issues/issue_comment events: the tree scan has nothing to say - # about a comment, and those events run against the DEFAULT branch, so their - # skipped check runs cannot attach to any PR head. Every event that runs in a - # PR's context (pull_request INCLUDING `edited`, pull_request_review, - # pull_request_review_comment) must run the scan for real: a job skipped by a - # job-level `if` still publishes a check run named "Secrets + content policy" - # with conclusion `skipped` on the PR head SHA, branch protection treats - # skipped as passing, and the newest check run for a name wins — so a mere - # title edit or review comment would flip an already-FAILED required tree - # scan green with nothing re-examining the tree. Re-scanning an unchanged - # tree costs minutes; a maskable required check costs the gate. - if: github.event_name != 'issues' && github.event_name != 'issue_comment' + # No job-level `if:` needed: the `on:` block above already scopes this job to + # exactly the tree-changing events, so every triggering event is a real run — + # never skipped, never a candidate for the masking bug described above. concurrency: + # cancel-in-progress is TRUE here and it is now safe: the only rapid + # retrigger this workflow can see is `synchronize` (a new commit landing + # while a prior scan of the OLD tree is still running), and superseding a + # stale in-flight scan with a fresh one for the new tree is exactly the + # right behaviour. Comment/review bursts cannot reach this workflow at all + # (see the block comment above), so this can no longer produce the + # cancelled-run pileup that caused PR #68 to wedge. group: public-repo-guard-tree-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true runs-on: ubuntu-latest @@ -136,91 +159,3 @@ jobs: # rather than by a leak. - name: body policy self-test (fixtures) run: bash scripts/public-repo-guard/tests/body-policy.test.sh - - # The other half of a public repo's surface. `guard` above scans the published - # TREE; a PR/issue/comment BODY is just as world-readable and, until this job, - # was scanned by nothing server-side. That gap was real, not theoretical: a PR - # was blocked for naming a private repo in wrangler.toml while the very same - # name, with more operational detail attached, sat unchallenged in its body. - # - # Honest about what it can and cannot do. On a PR this PREVENTS the merge. On an - # issue or comment the text is already public the moment it posts, so this is - # detection — it tells us to go redact, fast. Only the client-side pre-write hook - # can stop that class before publication. - body-guard: - name: Body content policy - if: >- - github.event_name == 'pull_request' - || github.event_name == 'issues' - || github.event_name == 'issue_comment' - || github.event_name == 'pull_request_review_comment' - || github.event_name == 'pull_request_review' - concurrency: - # Keyed on the specific comment / review / PR / issue rather than github.ref, - # because issue events all report the default branch and a ref-keyed group - # would let two comments cancel each other, leaving one unscanned. The comment - # and review ids come FIRST: those payloads also carry the PR number, and - # keying them on the PR would collapse two rapid comments into one group, - # dropping a verdict. - # - # cancel-in-progress is deliberately FALSE. Every version of a body deserves a - # verdict, the job is seconds long, and a cancelled check-run lingers on the - # commit and makes an otherwise-green PR look broken. - group: public-repo-guard-body-${{ github.event.comment.id || github.event.review.id || github.event.pull_request.number || github.event.issue.number || github.ref }} - cancel-in-progress: false - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - # Only the gate's own scripts are needed — no reason to pay for the whole - # tree on every comment. - sparse-checkout: scripts/public-repo-guard - sparse-checkout-cone-mode: false - # This job only reads the scripts — never leave the token sitting in - # .git/config while repo-supplied scripts execute in the workspace. - persist-credentials: false - - # Same rationale as the tree job: body-policy.sh needs a PCRE2-enabled rg, - # and Ubuntu's apt package has none. - - name: Install ripgrep (pinned + checksum-verified, PCRE2 build) - env: - RIPGREP_VERSION: "14.1.1" - RIPGREP_SHA256: "4cf9f2741e6c465ffdb7c26f38056a59e2a2544b51f7cc128ef28337eeae4d8e" - run: | - if command -v rg >/dev/null && rg --pcre2-version >/dev/null 2>&1; then - echo "using preinstalled $(rg --version | head -n1) with PCRE2"; exit 0 - fi - curl -fsSL --proto '=https' --tlsv1.2 -o ripgrep.tar.gz \ - "https://github.com/BurntSushi/ripgrep/releases/download/${RIPGREP_VERSION}/ripgrep-${RIPGREP_VERSION}-x86_64-unknown-linux-musl.tar.gz" - echo "${RIPGREP_SHA256} ripgrep.tar.gz" | sha256sum -c - - tar -xzf ripgrep.tar.gz --strip-components=1 "ripgrep-${RIPGREP_VERSION}-x86_64-unknown-linux-musl/rg" - sudo install -m 0755 rg /usr/local/bin/rg - rm -f rg ripgrep.tar.gz - rg --pcre2-version - - # The body is read straight out of the event payload FILE and written to - # another file. It is never interpolated into a run: block and never placed - # in an environment variable, so shell metacharacters in a hostile PR body - # have nothing to act on. jq is preinstalled on the GitHub-hosted images. - - name: Materialize the untrusted title/body to a file - run: | - set -euo pipefail - mkdir -p "$RUNNER_TEMP/bodyscan" - # An UNRECOGNIZED payload shape must fail, never quietly scan nothing and - # report a pass. If the event schema ever moves, this job must go red - # rather than become a green rubber stamp over an unscanned body. - if [ "$(jq -r 'has("pull_request") or has("issue") or has("comment") or has("review")' "$GITHUB_EVENT_PATH")" != "true" ]; then - echo "::error title=public-repo-guard (body-guard)::Event payload contains no pull_request/issue/comment/review object — refusing to report a pass on an unscanned body." - exit 1 - fi - jq -r '[.pull_request.title, .pull_request.body, - .issue.title, .issue.body, - .comment.body, .review.body] - | map(select(. != null)) | join("\n")' \ - "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/bodyscan/body.txt" - echo "scanning $(wc -l < "$RUNNER_TEMP/bodyscan/body.txt") line(s) of body text" - - - name: body policy (PR / issue / comment text) - env: - GUARD_PRIVATE_REPOS: ${{ vars.GUARD_PRIVATE_REPOS }} - run: bash scripts/public-repo-guard/body-policy.sh "$RUNNER_TEMP/bodyscan/body.txt"