ci: scan issue and comment bodies — this repo has never scanned one - #27
ci: scan issue and comment bodies — this repo has never scanned one#27yakimoto wants to merge 11 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_31b04dac-ddcf-4755-b62e-bd5e19d92c8d) |
ApprovabilityVerdict: Needs human review Unable to check for correctness in c284c89. This PR introduces a new security scanning capability with significant workflow changes. An unresolved review comment identifies a potential security bypass where a malicious PR could modify the scanner script to defeat the control. Human review is needed to address this concern. You can customize Macroscope's approvability policy. Learn more. |
PR Summary by QodoCI: scan issue/PR/comment bodies in public-repo-guard (with fixtures + safe concurrency)
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. PR-controlled body scanner
|
| - 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.
1. Pr-controlled body scanner 🐞 Bug ⛨ Security
The body-guard job runs on pull_request events but checks out repo code without pinning to a trusted base revision, then executes scripts from that checkout; a malicious PR can modify body-policy.sh to always pass (or change behavior) and defeat the new body leak gate.
Agent Prompt
### Issue description
`body-guard` runs on `pull_request` events but executes `scripts/public-repo-guard/body-policy.sh` from the checked-out repository contents. With `actions/checkout` not pinned to a trusted ref, a PR can change `body-policy.sh` in the same PR and make the body scan always pass, defeating the purpose of the new control.
### Issue Context
The workflow already has the untrusted body content in `$GITHUB_EVENT_PATH`; the only reason to checkout is to obtain the scanner scripts. For `pull_request` events, those scripts should come from a trusted source (base branch/commit), not from the PR.
### Fix Focus Areas
- .github/workflows/public-repo-guard.yml[113-163]
### Implementation notes
- In `body-guard`, set the checkout `ref` to a trusted base revision when `github.event_name == 'pull_request'` (e.g. `${{ github.event.pull_request.base.sha }}`), and otherwise to the default branch/`github.ref` for issue/comment events.
- Keep the sparse checkout, but ensure it pulls from the trusted ref.
- Optionally set `persist-credentials: false` since the job doesn’t need git auth after checkout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
Three review findings on the body gate: - ABOUT_THE_CONTROL exempted every rule, so a real credential on a line that mentioned the policy scanned clean. The allowlist is now opt-in per rule (prose flag) and only the heuristic rules (internal-marker, private-repo-ops) honour it; credential/infra formats always block. - A global (?i) leaked onto the SCREAMING_CASE credential-name branch of private-repo-ops, so lowercase code talk (session_token) near a repo name blocked ordinary prose. Case-insensitivity is now scoped to the repo names and the English phrase alternatives only. - Both policy scripts require rg -P, but Ubuntu's apt ripgrep is built without PCRE2, which would turn the required check permanently red. Both jobs now install the upstream binary pinned + SHA-256-verified (same pattern as gitleaks), and body-policy.sh refuses a PCRE2-less rg up front with a legible error. Fixtures added for all three regressions; suite passes 26/26. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…names Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…sede a failing one Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
… scan cannot match it Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…after the repo name Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…for the tree job Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| if: >- | ||
| github.event_name == 'pull_request' | ||
| || github.event_name == 'push' | ||
| || github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
📝 Info: Findings on issue/comment bodies land on the default branch, not the PR
For issues and issue_comment events GitHub runs the workflow in the default-branch context, so a failing body-guard run for those events produces a check run on main's head commit rather than anywhere visible on the PR. The job comment already frames this as detection-only, but it is worth noting that nobody is notified unless someone watches the Actions tab — a failure notification/alert path (or an org-level required review) would make the detection actionable.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ed review comments are all scanned Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Review events (pull_request_review, pull_request_review_comment) run in the PR's context and publish a check run on the PR head SHA; with the guard job skipping them, that run lands as 'skipped', which GitHub treats as passing while evaluating the most recent check run per name, so any review comment could supersede a failing tree scan with a green rubber stamp. The guard job now re-runs the tree scan on those events, same as pull_request: edited. body-policy.sh now fails closed (exit 2) in CI when GUARD_PRIVATE_REPOS is empty or contains no names: a missing or renamed org variable must go red, not silently skip the private-repo proximity rule and report a pass over an unscanned leak class. Local runs still skip the rule, and both behaviours are pinned by new fixtures. 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_86adf5e8-be25-4e09-b0ce-5d6efff24718) |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| steps: | ||
| - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 |
There was a problem hiding this comment.
🔍 Review-event tree scan may check out the base/default branch, not the PR head
The guard job now runs the full tree scan for pull_request_review and pull_request_review_comment events (.github/workflows/public-repo-guard.yml:82-88). The extensive comment at lines 68-77 asserts these events "run in the PR's context and publish a check run on the PR HEAD SHA." For GitHub pull_request_review / pull_request_review_comment events, github.sha/github.ref conventionally resolve to the repository's default branch rather than the PR head, so actions/checkout (line 94, no explicit ref) would scan the default branch tree and publish the check run against the default-branch commit — not the PR head. If that is the case, the review-event re-scan does not actually re-verify the PR head, and the concurrency reasoning (a review event superseding an in-flight PR-head tree scan under cancel-in-progress: true, line 91) could cancel the PR-head scan without landing an equivalent PR-head verdict. Worth verifying the actual github.sha semantics for these event types against current GitHub behavior before relying on this gate.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
GitHub's events-that-trigger-workflows reference documents GITHUB_REF for both pull_request_review and pull_request_review_comment as the PR merge branch refs/pull/N/merge (GITHUB_SHA = last merge commit on it), not the default branch, so the unqualified checkout scans the PR merge result exactly as the workflow comment states. The default-branch semantics this finding assumes apply to issue_comment/issues events, and the stale-merge-ref safeguards already landed on this PR rely on the same documented merge-ref behavior.
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 (new required-style checks, fail-closed scanner deps, and merge-blocking body rules); misconfiguration of
GUARD_PRIVATE_REPOSor missing PCRE2rgcan block merges, but scope is limited to the guard workflow and vendored scripts.Overview
Extends
public-repo-guardso world-readable titles and bodies (PRs, issues, comments, and review text—not only the checked-in tree) are scanned in CI, closing a gap where tree policy could block a file while the same leak sat in the PR body.The workflow is replaced and expanded: new triggers include
editedon PRs/issues/comments/reviews, plusissues,issue_comment,pull_request_review, andpull_request_review_comment.guardstill runs gitleaks +content-policy.shon the tree but with per-job concurrency (tree group can cancel in-flight runs on new pushes/edits) andactions/checkoutv7.0.1. It installs PCRE2-capable, pinnedripgrep(Ubuntu aptrgbreaks-Prules), runsbody-policy.test.shas a self-test, and no longer runs on pure issue/comment-only events (review events still re-run the tree so a skipped check cannot mask a failing scan).A new
body-guardjob materializes untrusted text from the event payload viajqinto a file (no shell interpolation), sparse-checkouts onlyscripts/public-repo-guard, and runsbody-policy.sh. Body concurrency keys on comment/review/issue/PR id withcancel-in-progress: falseso every edit gets a completed verdict.body-policy.shmirrors many tree leak classes (credentials, CFaccount_id, Tailscale IPs, operator paths) with redacted annotations,guard:allow, and prose rules that exempt discussion of the gate itself. Private-repo handling differs from the tree: bare cross-repo references are allowed; blocking requires a configured private repo name within ~140 chars of operational detail (credential names, secret bindings, secret counts).GUARD_PRIVATE_REPOSunset in CI fails closed.tests/body-policy.test.shadds fixture tests for positives, precision negatives, and fail-closed paths.Reviewed by Cursor Bugbot for commit 7e086ee. Configure here.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Note
Scan PR/issue/comment/review bodies for credential and infrastructure leaks in CI
body-guardCI job in public-repo-guard.yml triggered byissues,issue_comment,pull_request_review, andpull_request_review_commentevents, with per-event concurrency keyed to the most specific object ID.guardjob triggers to include review events and body-policy fixture tests; adds PCRE2-capable ripgrep installation to prevent false scanner failures.GUARD_PRIVATE_REPOSis unset in CI or when a non-PCRE2rgis detected, blocking the check run.Macroscope summarized c284c89.