ci: scan issue and comment bodies — this repo has never scanned one - #9
ci: scan issue and comment bodies — this repo has never scanned one#9yakimoto wants to merge 6 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_b1a68da0-c375-4440-a170-684f98768fae) |
|
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 2250a51. This PR adds new CI security scanning for PR/issue/comment bodies. Open review comments identify a significant security design issue: the body-guard job runs scanner scripts from the PR itself, allowing potential bypass. This security infrastructure change with unresolved concerns warrants human review. You can customize Macroscope's approvability policy. Learn more. |
PR Summary by Qodoci: scan PR/issue/comment bodies with public-repo-guard
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| FILE="${1:-}" | ||
| [[ -n "$FILE" && -f "$FILE" ]] || { echo "::error::body-policy: usage: body-policy.sh <file>"; exit 2; } | ||
| command -v rg >/dev/null 2>&1 || { echo "::error::body-policy: ripgrep (rg) required"; exit 2; } |
There was a problem hiding this comment.
🔍 Fixture suite verified end-to-end (all 28 cases pass under a PCRE2 engine)
I executed scripts/public-repo-guard/tests/body-policy.test.sh locally. The system ripgrep here is the Ubuntu 22.04 package (13.0.0-2ubuntu0.1) which is built WITHOUT PCRE2, so every fixture failed with exit 2 via the new preflight at scripts/public-repo-guard/body-policy.sh:31. Re-running with a PCRE2-capable shim, all 28 fixtures pass, including the tricky use-vs-mention lookarounds and the proximity rule in both orders. Two consequences worth noting: (a) the preflight message asserting "the apt/brew packages are" PCRE2-enabled is not true for Ubuntu 22.04's ripgrep 13, so a developer running the suite locally on 22.04 (or a self-hosted 22.04 runner) gets a hard exit 2 for the whole gate; (b) the same dependency already exists in scripts/public-repo-guard/content-policy.sh:52, so if the hosted runner image's ripgrep lacked PCRE2 the pre-existing guard job would already be permanently red — which is good evidence the hosted image is fine. Worth confirming the runner label stays on an image whose apt ripgrep has PCRE2.
Was this helpful? React with 👍 or 👎 to provide feedback.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| - 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. |
There was a problem hiding this comment.
2. Pr can modify scanner 🐞 Bug ⛨ Security
The new body-guard job runs on pull_request events but checks out and executes scripts/public-repo-guard/body-policy.sh from that checkout, so a PR can change the scanner to always succeed and bypass PR-body enforcement. This directly undermines the intended “PREVENTS the merge” behavior for PR body leaks.
Agent Prompt
### Issue description
`body-guard` executes `bash scripts/public-repo-guard/body-policy.sh ...` after an `actions/checkout` that does not pin a trusted repository ref for `pull_request` events. That means the PR can change `body-policy.sh` (or related scripts) and effectively disable the scan.
### Issue Context
This job is explicitly intended to gate PR bodies (and detect issue/comment leaks). For issue/comment events, checking out the default branch is fine, but for `pull_request` the scanner code must be trusted (base branch), not PR-controlled.
### Fix Focus Areas
- .github/workflows/public-repo-guard.yml[31-36]
- .github/workflows/public-repo-guard.yml[113-116]
- .github/workflows/public-repo-guard.yml[128-134]
- .github/workflows/public-repo-guard.yml[160-163]
### Suggested fix direction
- In `body-guard`, make the checkout explicitly pull a **trusted ref** for the scanner scripts:
- Example:
- uses: actions/checkout@<pin>
with:
ref: ${{ github.event.pull_request.base.sha || github.sha }}
sparse-checkout: scripts/public-repo-guard
sparse-checkout-cone-mode: false
- Then continue to run `bash scripts/public-repo-guard/body-policy.sh ...` as before.
- If you later need to scan PR tree content in this job, do a **second checkout** into a different path for the untrusted PR content and run the trusted scanner from the base checkout against that path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #10 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
|
…e comment on comment events Three review findings, each with a regression fixture: - The about-the-control allowlist filtered every rule's output, so a line mentioning SECURITY.md exempted a live AWS key on the same line. It now applies only to the narrative rules (internal-marker, private-repo-ops); credential/infra rules keep guard:allow as their only exemption. - A leading (?i) on the private-repo-ops pattern spilled into OPS_DETAIL, blocking ordinary lowercase prose like 'api_key' near a repo name. The flag is now scoped to the repo names via (?i:...). - issue_comment payloads carry the parent issue body, so one bad issue body failed every future comment on the thread. Comment events now scan only the comment; the parent got its own verdict from opened/edited runs. Also preflights rg --pcre2-version with a diagnosis, so a ripgrep build without PCRE2 fails with 'install a PCRE2-enabled build' instead of a per-rule 'ripgrep failed (exit 2)' on every fixture. 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.
… invented ones The fixture file is public and exempt from both tree scanners (content-policy.sh excludes the directory, .gitleaks.toml allowlists it), so pinning real private repo names published exactly the class of information the gate exists to block. The tests inject their own GUARD_PRIVATE_REPOS, so invented names exercise the same code paths. 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eued body scan Three review findings on the body gate: - pull_request_review and pull_request_review_comment were the one public text surface nothing scanned: diff review comments are not issue_comment events. Review-comment payloads carry .comment so the existing branch covers them; review submissions get their own .review.body branch, which must precede the pull_request fallback or the PR body would be re-scanned under the review's check name. - The shared concurrency group could drop an intermediate body version: GitHub keeps at most one pending run per group, so a burst of edits let a queued version go unscanned. The group is now keyed on run_id, i.e. never shared. - The install list said four files but the guard job runs the fixture suite, so a repo installed without it goes red; it now names all five. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…order Underscore is a word character, so in WAVE_API_TOKEN the only \b sits before the W, where [A-Z][A-Z0-9]* cannot reach the required _TOKEN — the \b-anchored OPS_DETAIL silently missed every multi-underscore credential name when it appeared AFTER the repo name (the detail-then-name order had no anchor and worked). Drop the \b so both orders match mid-token, with a regression fixture for the previously-missed order. Also make the 'talking about the control' fixture actually exercise the about-the-control allowlist: the body now names a configured repo next to SECRET_TOKEN, so the test fails if the allowlist is deleted or its rule scoping breaks. 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>
| - 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 | ||
|
|
||
| - name: Install ripgrep | ||
| run: command -v rg >/dev/null || (sudo apt-get update -qq && sudo apt-get install -y -qq ripgrep) | ||
|
|
||
| # 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")' "$GITHUB_EVENT_PATH")" != "true" ]; then | ||
| echo "::error title=public-repo-guard (body-guard)::Event payload contains no pull_request/issue/comment object — refusing to report a pass on an unscanned body." | ||
| exit 1 | ||
| fi | ||
| # On a comment event the payload also carries the parent issue/PR title | ||
| # and body. Re-scanning those here would make ONE bad issue body fail | ||
| # every future comment on the thread, however clean — so a comment run | ||
| # scans only the comment (this covers issue_comment AND diff review | ||
| # comments: pull_request_review_comment payloads carry `comment` too). | ||
| # The parent body already got its own verdict from the issues/ | ||
| # pull_request opened+edited runs. | ||
| if [ "$(jq -r 'has("comment")' "$GITHUB_EVENT_PATH")" = "true" ]; then | ||
| jq -r '.comment.body // empty' \ | ||
| "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/bodyscan/body.txt" | ||
| # A review SUBMISSION scans only the review body (often empty for a bare | ||
| # approve — an empty scan is a pass, not an error). Its inline diff | ||
| # comments each arrive as their own pull_request_review_comment event. | ||
| # This branch must come before the pull_request fallback: the review | ||
| # payload carries `pull_request` too, and falling through would re-scan | ||
| # the PR body under the review's check name instead of the review. | ||
| elif [ "$(jq -r 'has("review")' "$GITHUB_EVENT_PATH")" = "true" ]; then | ||
| jq -r '.review.body // empty' \ | ||
| "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/bodyscan/body.txt" | ||
| else | ||
| jq -r '[.pull_request.title, .pull_request.body, | ||
| .issue.title, .issue.body] | ||
| | map(select(. != null)) | join("\n")' \ | ||
| "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/bodyscan/body.txt" | ||
| fi | ||
| 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" |
There was a problem hiding this comment.
🟨 Body gate runs the gate script from the untrusted PR branch, so a fork PR can neuter its own scan
On pull_request events the new body-guard job checks out the PR's own code (actions/checkout at .github/workflows/public-repo-guard.yml:140) and then executes scripts/public-repo-guard/body-policy.sh from that checkout (.github/workflows/public-repo-guard.yml:195). A contributor (including a fork) can include a commit that edits body-policy.sh to exit 0 unconditionally, so the body leak gate reports a pass on the very PR that disables it. The same pattern already exists for the pre-existing guard job's content-policy.sh step, so this extends an existing weakness rather than introducing a new class.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Extended in review: the last unscanned text surface, and a queueing gap
Review feedback (cubic) identified that diff review comments and review bodies are not
issue_commentevents, so they were the one public text surface still scanned by nothing. The workflow now also triggers onpull_request_reviewandpull_request_review_comment; review-comment payloads carrycommentso the existing branch covers them, and review submissions scan.review.body(a bare approve scans empty and passes). It also found that the shared concurrency group could drop an intermediate body version, since GitHub queues at most one pending run per group; the group is now keyed onrun_id, i.e. never shared. The fixture suite is also now listed as the fifth file of the install bundle, since theguardjob runs it.Refs
wave-av/claude-workstation#1747.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Note
Cursor Bugbot is generating a summary for commit 944f18b. Configure here.
Note
Scan issue and PR comment/review bodies for credential and policy violations in CI
body-guardjob in public-repo-guard.yml that triggers onissues,issue_comment,pull_request_review, andpull_request_review_commentevents, covering event types that were never previously scanned.guardjob and fails CI on rule regressions or annotation redaction failures.guard:allow <reason>suppressions.guardjob no longer runs for PReditedevents; that case is now handled bybody-guardinstead.Macroscope summarized 2250a51.