ci: scan issue and comment bodies — this repo has never scanned one - #87
ci: scan issue and comment bodies — this repo has never scanned one#87yakimoto wants to merge 10 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>
|
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 |
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_a69bf77d-fdb9-448c-8bb2-13ba80c8a653) |
|
Running ultrareview automatically — Running ultrareview automatically — this adds a new fail-closed secret/body scanner and reworks CI gating/concurrency; a subtle regex or workflow bug could block every PR or miss credential leaks on the whole public surface, so it needs a deeper pass.. I'll post findings when complete. |
ApprovabilityVerdict: Needs human review Unable to check for correctness in d87f627. Introduces substantial new CI security scanning infrastructure (new workflow job, 400+ lines of new scripts) that will run on every PR, issue, and comment. While well-designed with comprehensive tests, new security-adjacent capabilities of this scope 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-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #88 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
|
… comments Two review findings: 1. ABOUT_THE_CONTROL was applied inside check() for every rule, so a credential-format hit on a line that also named the gate (or SECURITY.md) was silently dropped — a same-line bypass. The allowlist is now opt-in per rule via a 'prose' tag; only internal-marker and private-repo-ops consult it. guard:allow remains the universal, visible escape hatch. 2. pull_request_review_comment was never subscribed, so inline review comment bodies were never scanned. Added the trigger, extended body-guard's condition, and moved comment.id first in the concurrency key: review-comment payloads carry the pull_request object, so keying on PR number would fold every review comment into one group. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…nstall; probe rg for PCRE2 Three review findings: 1. The leading (?i) on private-repo-ops bled into OPS_DETAIL, turning its deliberate SCREAMING_CASE requirement into a match on everyday lowercase words (docs/setup_key.md, process.env.api_token) — blocking exactly the bare cross-references the rule promises to leave alone. (?i:...) now wraps the repo-name alternation alone. 2. The install header said 'copy all four files' but the guard job runs a fifth (tests/body-policy.test.sh); a repo installed per the docs would fail on every run. The header now lists five. 3. Every rule uses rg -P, and not every distro builds ripgrep with PCRE2 (Ubuntu jammy's apt package does not). Both install steps now probe -P support up front so a runner-image change fails with a named cause instead of an opaque scanner error blocking every PR. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A review's summary body (the text submitted with an approval or changes-requested) is a fourth world-readable surface, distinct from both issue_comment and pull_request_review_comment. Subscribe to submitted/edited, extract .review.body in the materialize step, and key concurrency on review.id ahead of the PR number so two rapid reviews cannot fold into one group. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| if: >- | ||
| (github.event_name == 'pull_request' && github.event.action != 'edited') | ||
| || github.event_name == 'push' | ||
| || github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
🔍 Tree job is now skipped on PR body edits — confirm branch protection treats that as passing
Adding edited to the pull_request types means every body edit produces a run in which the required-looking check Secrets + content policy is skipped by the job-level if. GitHub treats a job skipped by if as a created check-run with conclusion skipped, which branch protection counts as satisfied, so this should be fine — but it is worth a one-time confirmation on this repo's protection settings, since the tree scan is the gate the org relies on.
Was this helpful? React with 👍 or 👎 to provide feedback.
…scan only the object the event touched; surface an unconfigured private-repo rule Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…xtures Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| # So a bare mention stays silent. What fires is a private repo name within ~140 | ||
| # characters of INTERNAL OPERATIONAL DETAIL — a SCREAMING_CASE credential NAME, a | ||
| # secret-binding verb, a service binding, or a secret COUNT. That is the topology | ||
| # of what is wired to what, and it is the shape that actually leaked. |
There was a problem hiding this comment.
🔍 Proximity rule is line-scoped, so a leak split across two lines is not detected
rg matches per line and [^\n]{0,140} further pins both halves to the same line, so a body that names a private repo on one line and the credential/binding detail on the next (a very common markdown bullet-list shape) will not fire. The comment says "within ~140 characters", which reads as character distance rather than same-line-only; worth documenting or moving to a multiline scan if bullet lists are a realistic leak shape.
Was this helpful? React with 👍 or 👎 to provide feedback.
…me-then-detail order 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 must come BEFORE the PR number: review-comment and | ||
| # review payloads carry the pull_request object too, and keying those on the | ||
| # PR number would fold every comment on a PR into one group. | ||
| # | ||
| # 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 |
There was a problem hiding this comment.
🔍 Comment/issue-triggered runs attach their check to the default branch, not the PR
issues, issue_comment, pull_request_review_comment and pull_request_review runs execute in the context of the default branch and their check-runs attach to the default branch head, not the PR head. Two consequences worth confirming against branch protection: (1) a leak in a PR comment or review body will NOT block that PR's merge (the header comment acknowledges this is detection-only), and (2) a failing body-guard run for a comment will surface as a red run on the default branch's commit, which can be mistaken for a broken main. Also note these events always use the default branch's copy of body-policy.sh, so changes to the gate only take effect for comments after merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.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_5ddc1058-e529-40ca-9740-00f8ea33d801) |
| 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' |
There was a problem hiding this comment.
🔍 The new body check runs under a new check-run name, so it blocks nothing until branch protection is updated
The PR description states "On a PR this PREVENTS the merge." That is only true if Body content policy is added to the branch's required status checks. The PR's own evidence table says the single required check name today is Secrets + content policy, which is the guard job's name (.github/workflows/public-repo-guard.yml:63). body-guard ships with a distinct name (:135), so out of the box a body leak on a PR produces a red-but-not-required check and merge is still permitted.
Second, related point: guard is now skipped on pull_request edited (:67-70). A body edit therefore produces a fresh Secrets + content policy check run with conclusion skipped on the same head SHA. GitHub treats a skipped required check as passing, so this should be benign, but it is worth confirming against this org's branch-protection settings before relying on the required check to mean the tree was actually scanned for that SHA.
Both are configuration follow-ups outside the diff, not defects in the code.
Was this helpful? React with 👍 or 👎 to provide feedback.
…fixtures The body profile's internal-ip rule matched the documentation form of the range itself (100.64.0.0/10), so any security discussion naming the range — including quotes of the gate's own comments — blocked with no escape short of guard:allow. The tree gate never hits this because it excludes the guard's own directory; body text has no such exclusion. Exempt the two range-talk shapes (all-zero host portion and CIDR-suffixed subnets) while a concrete host like 100.71.4.19 still blocks, with fixtures for all three. The fixture suite also now probes for PCRE2 support up front, mirroring the workflow's install step: on a PCRE2-less rg every fixture degraded to the scanner's fail-closed exit 2, producing dozens of opaque 'want exit 1, got 2' failures indistinguishable from a broken gate. 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 | ||
| # This job runs repo-checked-out scripts over attacker-controlled body | ||
| # text; nothing here pushes or calls the API, so the token must not | ||
| # linger in .git/config for those scripts to find. | ||
| persist-credentials: false |
There was a problem hiding this comment.
🟨 Body-leak gate can be neutralized by the pull request it is scanning
The body gate checks out the repository at the pull request's merge ref and then executes scripts/public-repo-guard/body-policy.sh from that checkout (.github/workflows/public-repo-guard.yml:157-166 and .github/workflows/public-repo-guard.yml:213). Because the script executed is the PR's own copy, a pull request (including one from a fork) that edits body-policy.sh — e.g. weakening a rule or making it exit 0 unconditionally — is scanned by its own modified gate, so the leak check for that PR's title/body becomes advisory rather than authoritative. The same structural property already applies to the pre-existing tree gate (content-policy.sh), so this PR extends rather than introduces the pattern. No token or secret exposure is involved: permissions: contents: read, pull_request (not pull_request_target) is used, and persist-credentials: false is set, which are all correct choices.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is the gate's deliberate trust model, shared with the pre-existing tree gate and ruled on in prior passes: on pull_request events the workflow file itself is equally PR-editable, so pinning the script to base adds no boundary. The comment itself notes the pattern is pre-existing and all token-safety choices (contents: read, pull_request, persist-credentials: false) are correct.
1 participant
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
Medium Risk
Changes security CI behavior and merge blocking on PR bodies; implementation is defensive (no credential echo, fail-closed scanners), but mis-tuned rules could block legitimate discussion or miss edge cases.
Overview
Extends
public-repo-guardso GitHub title/body text is scanned in CI, not only the published tree. A newbody-guardjob runs on PR, issue, comment, review-comment, and review events (includingedited), materializes untrusted text viajqinto a file, and runsbody-policy.shwithpersist-credentials: falseand sparse checkout of guard scripts.The existing
guardjob now skips PReditedand comment-only events, uses per-job concurrency (tree job can cancel in progress; body job does not), bumpsactions/checkoutto v7.0.1, verifies ripgrep PCRE2 support, and runsbody-policy.test.shfixtures in CI.body-policy.shis a fail-closed companion tocontent-policy.sh: it blocks credential formats, infra identifiers, internal markers, and private-repo names only when paired with operational detail on the same line (unlike the file gate’s bare private-repo block). Matches are redacted in annotations; prose rules allow discussion of the control itself.tests/body-policy.test.shadds hermetic fixtures for block/pass cases and filter fail-closed behavior.Reviewed by Cursor Bugbot for commit dbb3b63. Configure here.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Note
Scan issue and PR comment bodies for policy violations in the public-repo-guard workflow
body-guardjob to public-repo-guard.yml that materializes event titles, bodies, and comment text into a file and runs body-policy.sh against them, covering issues, PR comments, reviews, and review comments.guard:allow <reason>markers opt out of specific rules; prose-scoped rules additionally suppress self-referential discussion of the control itself.guardjob on every scan.guardjob gains ripgrep PCRE2 verification, a new concurrency group keyed by PR number/ref, andactions/checkoutis upgraded to v7.0.1 withpersist-credentials: false.Macroscope summarized d87f627.