ci: scan issue and comment bodies — this repo has never scanned one - #22
ci: scan issue and comment bodies — this repo has never scanned one#22yakimoto wants to merge 8 commits into
Conversation
…ment body Measured across all 28 public wave-av repos (claude-workstation#1747, #1794): TWO coverage shapes satisfy the one required check name `Secrets + content policy`. 27 repos triggers: pull_request, push, workflow_dispatch jobs: guard 1 repo triggers: + issues, issue_comment jobs: + body-guard This repo is in the 27. All 28 report the same green check. The outlier is wave-moq-edge, and its own comment says why it matters: "`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." A PR/issue/comment BODY is exactly as world-readable as the tree, and until now it was scanned by nothing server-side. That gap was not theoretical on wave-moq-edge: 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. WHAT LANDS HERE — the bundle the workflow's own header names, minus what this repo already has (.gitleaks.toml and content-policy.sh are already vendored): .github/workflows/public-repo-guard.yml replaced (73 -> 163 lines) scripts/public-repo-guard/body-policy.sh new, mode 100755 scripts/public-repo-guard/tests/body-policy.test.sh new, mode 100755 Copied from wave-moq-edge, which has run this shape in production. Modes preserved via the git trees API — the contents API would have created both scripts 100644. HONEST ABOUT WHAT IT CAN 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 says go redact, fast. Only a client-side pre-write hook stops that class before publication. Also inherited from the reference: concurrency moves from workflow-level to PER JOB, because the two jobs want opposite behaviour. A workflow-level group forced one policy on both, and rapid body edits cancelled the tree job repeatedly — every cancelled check-run stays attached to the commit, so the PR reported UNSTABLE while the live runs were green. The body gate ships with its own fixtures and runs them in CI. Its NEGATIVE cases are the load-bearing half: a leak gate that blocks legitimate cross-repo references gets switched off, and then it protects nothing. Refs wave-av/claude-workstation#1747. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_47cccff1-abba-499a-96ce-a7fd30b8397a) |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
ApprovabilityVerdict: Needs human review Unable to check for correctness in 743a99f. This PR adds new security scanning infrastructure with ~400 lines of new code. Multiple unresolved review comments identify potential security bypass vectors (fork PRs can modify the scanner judging them, review event triggers may run fork-supplied scripts with base-repo context). Security-related changes with outstanding security concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
PR Summary by Qodoci: scan PR/issue/comment bodies in public-repo-guard
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #23 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 1 fixed
|
… repo names Review findings from #22: - The ABOUT_THE_CONTROL allowlist applied to every rule, so a credential pasted on a line that happened to name the gate was reported clean. It now applies only to the self-referential prose rules (internal-marker, private-repo-ops); credential-format and infrastructure rules honour only the explicit guard:allow marker. - A global (?i) leaked into OPS_DETAIL, letting lowercase prose like api_key count as a SCREAMING_CASE credential name next to a private repo. Case-insensitivity is now scoped to the repo-name alternation and the prose verbs. - GUARD_PRIVATE_REPOS parsing read only the first line; newlines are now normalized to spaces before splitting. - Both jobs install a pinned, checksum-verified PCRE2-enabled ripgrep release instead of relying on apt (Ubuntu's package lacks PCRE2, so rg -P exits 2 and every check fails closed). Adds regression fixtures for all of the above; suite is 24/24. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…t with || true The initial rg -nP scan already failed closed on exit >=2, but the guard:allow and about-the-control filter pipelines appended || true, so a filter error would be treated as an empty match set and report a clean scan. The filters now distinguish exit 1 (every line filtered, fine) from >=2 (scanner error, exit 2). Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| - 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 |
There was a problem hiding this comment.
🔍 Fork PRs run their own copy of the gate script
The body-guard job checks out the PR merge ref (default actions/checkout behaviour under pull_request) and then executes scripts/public-repo-guard/body-policy.sh from that checkout, so a PR can modify the scanner itself in the same PR whose body is being scanned. This mirrors the pre-existing behaviour of the tree guard job and is contained by permissions: contents: read / no secrets other than vars.GUARD_PRIVATE_REPOS, but if the intent is that the body gate cannot be disarmed by the change under review, the body job would need to run the scripts from the base ref (e.g. a second checkout at github.event.pull_request.base.sha into a separate path).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
HOLD — do not merge. This PR is now a canary that found a defect in its own change. It was the first of 18 planned repos. The other 17 have not been touched, and will not be until this is resolved. What went wrongThe bundle adds if: >-
(github.event_name == 'pull_request' && github.event.action != 'edited')
|| github.event_name == 'push'
|| github.event_name == 'workflow_dispatch'That job's So an The intent is sound — editing a body should not re-scan the whole tree — but the implementation expresses it by skipping the job that owns the required check name, rather than by having that job run and do nothing. Why this matters more than one PRShipping it to all 18 would mean: on every public repo, editing a PR title or body can leave the required check unsatisfied, blocking that PR until an unrelated push re-triggers it. That is a merge-blocking regression on every repo that publishes our npm packages. Shape of the fix (not applied here yet)Two candidates, both keeping the body scanning:
(1) is the safer shape: it means the fan-out adds a file instead of replacing a required gate's definition in 18 repos at once. Worth checking separately
Refs |
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 2/5
- In
.github/workflows/public-repo-guard.yml, thepull_requestbody-check job appears to run against the PR merge commit/default ref, so a forked PR can alterscripts/public-repo-guard/body...to self-approve without actually scanning its own text; that weakens the repository’s guardrail and could let disallowed PR content slip through — run the check from a trusted ref (base/default branch) and treat PR text as untrusted input in the workflow.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/public-repo-guard.yml">
<violation number="1" location=".github/workflows/public-repo-guard.yml:66">
P1: A fork PR can make the new body check pass without scanning its text. This `pull_request` job checks out the event's default ref, which is the PR merge commit, and line 163 then executes `scripts/public-repo-guard/body-policy.sh` from that checkout; the PR author can replace that script (or the workflow) with a no-op. The sparse checkout limits paths but does not make the scanner trusted. The body gate should execute an immutable base-branch copy from a trusted workflow/context, without executing files supplied by the PR.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 |
There was a problem hiding this comment.
P1: A fork PR can make the new body check pass without scanning its text. This pull_request job checks out the event's default ref, which is the PR merge commit, and line 163 then executes scripts/public-repo-guard/body-policy.sh from that checkout; the PR author can replace that script (or the workflow) with a no-op. The sparse checkout limits paths but does not make the scanner trusted. The body gate should execute an immutable base-branch copy from a trusted workflow/context, without executing files supplied by the PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/public-repo-guard.yml, line 66:
<comment>A fork PR can make the new body check pass without scanning its text. This `pull_request` job checks out the event's default ref, which is the PR merge commit, and line 163 then executes `scripts/public-repo-guard/body-policy.sh` from that checkout; the PR author can replace that script (or the workflow) with a no-op. The sparse checkout limits paths but does not make the scanner trusted. The body gate should execute an immutable base-branch copy from a trusted workflow/context, without executing files supplied by the PR.</comment>
<file context>
@@ -25,24 +26,44 @@ name: public-repo-guard
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
# gitleaks' GitHub Action requires a paid license for organizations; the CLI
</file context>
… rules A body has no reviewable diff, so a guard:allow marker there is one edit away, not one review away: the same untrusted author who writes a leaking line could append the marker and turn every rule green, including the live-credential rules. Scope guard:allow (like the about-the-control list) to the self-referential prose rules only; credential-format and infrastructure rules now have no author-controlled escape in a body, with a regression fixture proving a guard:allow'd AWS key still blocks. Also port body-policy's newline normalization of GUARD_PRIVATE_REPOS to content-policy.sh: read stops at the first newline, so a one-name-per-line org variable silently configured only the first name and the tree scanner passed over the unscanned rest. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
Correction — I was wrong, and the HOLD above is withdrawn on the technical grounds I gave. I claimed that skipping the
So the behaviour I flagged is the design working correctly: on an Two things I should have done before writing that: read a repo already running this shape ( Also self-inflicted, and worth noting for anyone reading the run history: several of the cancelled and re-queued runs above are caused by my own comments. The Still true and unchanged from the original description: this repo had no issue or comment body scanning, and 27 of 28 public repos are in the same state. |
Retargeting a PR fires only `edited` (never `synchronize`), so the blanket edited-skip let a base-branch change reach merge with the old merge tree's green check; the tree job now runs on edited events whose changes carry a base object. Inline diff review comments are a separate event from issue_comment and were world-readable text no job ever scanned; add the pull_request_review_comment trigger to body-guard and key its concurrency group on the comment id first so two rapid review comments on one PR cannot collapse into a single group and drop a verdict. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| 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' |
There was a problem hiding this comment.
🔍 The body gate only blocks a merge if branch protection is updated to require it
The new job's check name is Body content policy, distinct from the existing required Secrets + content policy. The PR states "On a PR this PREVENTS the merge", but that only holds once branch protection lists the new name as a required status check. Until then the body gate is detection-only on PRs too — the same status the PR assigns to issues/comments. Worth confirming the repo's branch-protection config is updated alongside this merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
…range designation A job skipped by a job-level if still publishes a check run with conclusion skipped on the PR head SHA, branch protection treats skipped as passing, and the newest check run for a name wins. So skipping the tree scan on edited (or on review/review-comment events) let a title edit or an inline comment flip an already-failed required check green. The guard job now skips only issues and issue_comment events, whose runs attach to the default branch and cannot mask a PR verdict. Also: scan top-level review bodies via pull_request_review (trigger, if, concurrency key, jq .review.body), set persist-credentials: false on both checkouts to match the repo's other workflows, and stop the internal-ip rule matching the range designation 100.64.0.0(/nn), which is the public NAME of the CGNAT range, appears in the gate's own docs, and had no allowlist escape. Fixtures added for the designation (pass) and an adjacent real host (block). Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
The guard job executes scripts/public-repo-guard/tests/body-policy.test.sh, so a repo installed from the four-file list fails every run on the missing script (bash exits 127 under the default -e shell), and a permanently-red required gate gets switched off. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| 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 |
There was a problem hiding this comment.
🔍 Body-gate verdicts for issue/comment events land on the default branch, not the PR
issues and issue_comment runs execute in the default-branch context, so the Body content policy check run produced by those events attaches to the latest default-branch commit rather than to any PR head. A leak in a comment therefore leaves a persistent red check on main's commit (and repeats on every subsequent comment, because .issue.body is re-scanned each time at .github/workflows/public-repo-guard.yml:216-220). Worth confirming that nothing downstream (release gates, badges, deploy checks) treats a red check on the default-branch commit as blocking, since a comment leak that has already been redacted will not clear it until another run overwrites the same check name.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate observation of an intended, documented property: the workflow explicitly states issue/comment scans are detection-only ("the text is already public the moment it posts"), and where check runs attach on those events is GitHub behavior, not something this workflow controls. Confirming downstream consumers of default-branch checks is an ops verification task, not a code change in this PR.
…cripts A CRLF-stored org variable glued an invisible \r to every name after the LF normalization, so the built regex matched nothing and the private-repo gate fail-opened with no diagnostic. Normalize \r alongside \n in both scripts and add a CRLF regression fixture. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| concurrency: | ||
| group: public-repo-guard-tree-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
🔴 Posting a review comment can cancel a pull request's required secret scan, leaving it stuck
The tree scan for review-triggered events reuses the same cancellation group as the pull request's own scan (group: public-repo-guard-tree-${{ github.event.pull_request.number || github.ref }} with cancel-in-progress: true at .github/workflows/public-repo-guard.yml:76-78), so a review or review comment posted while the pull request's scan is running kills it.
Impact: The required secret/content check on the pull request ends as cancelled and never turns green, blocking the merge until someone manually re-runs it.
Why the review-triggered run cannot replace the cancelled one
The guard job now runs for pull_request_review and pull_request_review_comment (.github/workflows/public-repo-guard.yml:38-46, .github/workflows/public-repo-guard.yml:75). For those events GitHub sets GITHUB_REF/GITHUB_SHA to the default branch, so actions/checkout checks out main, the scan examines the default-branch tree rather than the PR, and the resulting check run attaches to the default-branch commit — not the PR head SHA.
But the payload of those events does contain pull_request.number, so the concurrency group evaluates to public-repo-guard-tree-<PR number>, identical to the group used by the real pull_request-event run for that PR. With cancel-in-progress: true, the review event's run cancels the PR's in-flight tree scan. The PR head SHA is then left with a cancelled check run for "Secrets + content policy", and the replacement run's verdict lands on a different commit — precisely the "cancelled check-run stays attached to the commit" failure mode the comment at .github/workflows/public-repo-guard.yml:56-59 says the per-job concurrency was introduced to avoid.
Possible fixes: key the tree job's group on github.event.pull_request.head.sha || github.ref, exclude review events from the group (or from the job entirely, since they scan the wrong tree), or set cancel-in-progress: false for the review-triggered path.
Prompt for agents
In .github/workflows/public-repo-guard.yml the `guard` (tree scan) job runs for pull_request, pull_request_review and pull_request_review_comment events, and its job-level concurrency group is keyed on `github.event.pull_request.number || github.ref` with cancel-in-progress: true. For pull_request_review and pull_request_review_comment, GitHub runs the workflow against the DEFAULT branch (GITHUB_REF/GITHUB_SHA point at the default branch) but the payload still carries pull_request.number, so those runs land in the same concurrency group as the PR's real tree scan and cancel it. The cancelled check run stays attached to the PR head SHA while the replacement run scans main and reports on the default-branch commit, so the PR's required check never goes green. Decide whether the tree job should run at all for review events (it cannot scan the PR tree), and if it should, make its concurrency group distinguish the actual scanned ref/commit (e.g. key on github.event.pull_request.head.sha or github.sha) so review activity cannot cancel the PR's own scan.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
False positive built on the same incorrect context claim: review-triggered runs execute on the PR merge ref, so a run that cancels the PR's in-flight scan checks out the same merge tree and reports the same check name on the same head SHA; the newest completed run supplies the verdict and the PR is not left stuck. Sharing the concurrency group is the intended collapse of redundant scans of the same tree.
| # 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' |
There was a problem hiding this comment.
🔍 Rationale for running the tree scan on review events rests on an incorrect assumption
The comment argues the tree job must run (not be skipped) for pull_request_review and pull_request_review_comment because a skipped job "still publishes a check run ... on the PR head SHA" and would mask an earlier failure. For those two events GitHub runs the workflow in the DEFAULT-branch context (GITHUB_REF/GITHUB_SHA = last commit on the default branch), exactly like issues/issue_comment, which the same if excludes for that very reason. So the check runs from review events attach to the default-branch commit, not the PR head, and could not mask a failed PR check. The practical effect is a full clone + gitleaks install + tree scan of main on every review comment, and (see the reported concurrency finding) collateral cancellation of the PR's own scan. Worth re-deriving the intended trigger matrix.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Incorrect premise: per GitHub's events reference, pull_request_review and pull_request_review_comment set GITHUB_REF to the PR merge branch (refs/pull/N/merge) and GITHUB_SHA to its last merge commit, not the default branch. Their check runs therefore attach to the PR context, which is precisely why the tree scan must run rather than publish a maskable skipped check.
This repo's
public-repo-guardhas never scanned a single issue or comment body.Measured across all 28 public wave-av repos (
wave-av/claude-workstation#1747,#1794): two coverage shapes satisfy the one required check nameSecrets + content policy.pull_request, push, workflow_dispatchguardissues,issue_commentbody-guardThis repo is in the 27. All 28 report the same green check — because a required check asserts that something named X passed, never what X examined.
The outlier is
wave-moq-edge, and its own comment says why it matters:That gap was not theoretical there: a PR was blocked for naming a private repo in
wrangler.tomlwhile the very same name, with more operational detail attached, sat unchallenged in its body.What lands
Three files — the bundle the workflow's own header names, minus what this repo already has (
.gitleaks.tomlandcontent-policy.share already vendored, and are checked as prerequisites; a repo missing either is refused rather than half-installed):The workflow's header names four files as the install unit but executes a fifth —
tests/body-policy.test.sh, in its own self-test step. Omitting it installs a workflow that fails on a step nobody read, so the manifest ships it. Modes are preserved via the git trees API; the contents API creates100644regardless, which would silently break running these scripts as executables.Planned by
governance/lib/vendor-bundle.mjs(claude-workstation#1850) against a checked-in manifest, not by ad-hoc shell.One deliberate divergence from the reference, stated rather than silent
The shipped workflow is
wave-moq-edge's withactions/checkoutbumped from v5.0.1 to v7.0.1 (3d3c42e5aac5ba805825da76410c181273ba90b1), the pin already used byclaude-workstation's own gate.Copying verbatim was checked first and rejected on evidence: of the 18 target repos, 17 carry a byte-identical guard, and
wave-realtime-edgealready runs v7.0.0 — so a verbatim copy would have downgraded it, and shipped a stale pin to the other 17. A separate PR brings the reference itself up to the same pin.Honest about what this 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 says go redact, fast. Only a client-side pre-write hook stops that class before publication.
Also inherited from the reference: concurrency moves from workflow-level to per job, because the two jobs want opposite behaviour. A workflow-level group forced one policy on both, and rapid body edits cancelled the tree job repeatedly — every cancelled check-run stays attached to the commit, so the PR reported UNSTABLE while the live runs were green.
The body gate ships with its own fixtures and runs them in CI. Its negative cases are the load-bearing half: a leak gate that blocks legitimate cross-repo references gets switched off, and then it protects nothing.
Refs
wave-av/claude-workstation#1747.Note
Scan PR/issue/comment/review body text for credential and policy violations in CI
body-guardCI job in public-repo-guard.yml that scans PR, issue, and comment/review body text using a new body-policy.sh script; blocks on violations for PRs and warns for issues/comments post-publication.body-policy.shchecks for credential formats (Stripe, Anthropic, GitHub PATs, AWS AKIDs, private key blocks, etc.) and private repo identifiers usingrg -P(PCRE2), emitting GitHub Actions annotations with line numbers only to avoid re-publishing secrets.GUARD_PRIVATE_REPOSparsing in content-policy.sh to handle newline- and CRLF-separated repo name lists.guardjob and will fail CI on regressions.Macroscope summarized 743a99f.