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>
|
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_564263c8-fbd8-45fb-a052-aef81703fe2d) |
ApprovabilityVerdict: Needs human review Unable to check for correctness in 7ed1c87. New security scanning feature with ~345 lines of new scripts. Multiple unresolved review comments identify potential bypass paths (fork PRs supplying their own gate script, silent rule skipping). Security-related CI changes with substantive open concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Running ultrareview automatically — Adds a security gate over untrusted PR/issue/comment bodies with hand-rolled regex/allowlist logic; a missed bypass can leak internal details to a public repo, and a false positive can block every PR, so this CI change deserves multi-pass review.. I'll post findings when complete. |
PR Summary by QodoCI: scan PR/issue/comment bodies with 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.
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?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. | ||
| sparse-checkout: scripts/public-repo-guard | ||
| sparse-checkout-cone-mode: false |
There was a problem hiding this comment.
🔍 Fork PRs supply the gate script that judges them, and vars may be unavailable
body-guard runs on pull_request and checks out the PR's own copy of scripts/public-repo-guard, so a fork PR can modify body-policy.sh in the same PR and the gate that evaluates its body is the modified one. This mirrors the pre-existing arrangement for content-policy.sh in the guard job, so it is not a regression, but it means the body gate is advisory against a motivated author. Separately, vars.GUARD_PRIVATE_REPOS (.github/workflows/public-repo-guard.yml:163) may not be exposed to fork-originated pull_request runs; if it is empty the private-repo-ops check silently skips (scripts/public-repo-guard/body-policy.sh:117) while the job still reports a pass.
Was this helpful? React with 👍 or 👎 to provide feedback.
…al formats 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}
There was a problem hiding this comment.
Ultrareview completed in 10m 12s
3 issues found and verified against the latest diff
Confidence score: 2/5
- In
.github/workflows/public-repo-guard.yml, the body-content check runsscripts/public-repo-guard/body-policy.shfrom the PR’s own checkout, so an attacker can alter the scanner and bypass the guard on untrusted PR text; this undercuts the core protection intent — run the policy script from a trusted ref (or inline/pinned action code) instead of PR-controlled files. - In
.github/workflows/public-repo-guard.yml, skipping allpull_request.editedevents creates a validation gap where non-body/title edits can avoid tree scanning, which can let risky metadata changes slip through undetected — narrow the skip logic to only safe text-only edits or re-enable scans for edited events. - In
.github/workflows/public-repo-guard.yml, setup docs mention copying four files while the workflow requiresscripts/public-repo-guard/tests/body-policy.test.sh, so new adopters can get failing required checks from incomplete installation — align the install instructions with all required files (or make the self-test conditional).
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:20">
P2: The install header says to copy four files to set up public-repo-guard in a new repo, but the guard job unconditionally executes a fifth file, scripts/public-repo-guard/tests/body-policy.test.sh, as a required self-test step. A repo that follows the documented four-file install list will fail CI at this step since the test script won't exist. Update the header to include the test script (or gate/remove the self-test step for minimal installs).</violation>
<violation number="2" location=".github/workflows/public-repo-guard.yml:58">
P2: The tree scan now skips every `pull_request.edited` event, which creates a blind spot for edited actions that are not just body/title text (for example PR metadata changes that can affect what should be validated). Running `guard` for all PR actions avoids relying on `edited` being text-only and keeps the scan behavior fail-closed.</violation>
<violation number="3" location=".github/workflows/public-repo-guard.yml:132">
P1: The body-content gate can be bypassed on pull requests because it executes `scripts/public-repo-guard/body-policy.sh` from the PR checkout. Since this job is meant to validate untrusted PR text, running the scanner from the same untrusted ref allows a PR to weaken or disable the scan and still produce a green check. Using a trusted ref for the scanner (for PRs, the base SHA) keeps the policy enforcement tamper-resistant.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| 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 |
There was a problem hiding this comment.
P1: The body-content gate can be bypassed on pull requests because it executes scripts/public-repo-guard/body-policy.sh from the PR checkout. Since this job is meant to validate untrusted PR text, running the scanner from the same untrusted ref allows a PR to weaken or disable the scan and still produce a green check. Using a trusted ref for the scanner (for PRs, the base SHA) keeps the policy enforcement tamper-resistant.
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 132:
<comment>The body-content gate can be bypassed on pull requests because it executes `scripts/public-repo-guard/body-policy.sh` from the PR checkout. Since this job is meant to validate untrusted PR text, running the scanner from the same untrusted ref allows a PR to weaken or disable the scan and still produce a green check. Using a trusted ref for the scanner (for PRs, the base SHA) keeps the policy enforcement tamper-resistant.</comment>
<file context>
@@ -71,3 +92,72 @@ jobs:
+ 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
+
</file context>
| sparse-checkout: scripts/public-repo-guard | |
| ref: ${{ github.event.pull_request.base.sha || github.sha }} | |
| sparse-checkout: scripts/public-repo-guard |
| # and the org should not pay for a gitleaks run every time anyone posts one) and | ||
| # skips `edited` (a title or body edit does not change the tree). | ||
| if: >- | ||
| (github.event_name == 'pull_request' && github.event.action != 'edited') |
There was a problem hiding this comment.
P2: The tree scan now skips every pull_request.edited event, which creates a blind spot for edited actions that are not just body/title text (for example PR metadata changes that can affect what should be validated). Running guard for all PR actions avoids relying on edited being text-only and keeps the scan behavior fail-closed.
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 58:
<comment>The tree scan now skips every `pull_request.edited` event, which creates a blind spot for edited actions that are not just body/title text (for example PR metadata changes that can affect what should be validated). Running `guard` for all PR actions avoids relying on `edited` being text-only and keeps the scan behavior fail-closed.</comment>
<file context>
@@ -25,24 +26,44 @@ name: public-repo-guard
+ # and the org should not pay for a gitleaks run every time anyone posts one) and
+ # skips `edited` (a title or body edit does not change the tree).
+ if: >-
+ (github.event_name == 'pull_request' && github.event.action != 'edited')
+ || github.event_name == 'push'
+ || github.event_name == 'workflow_dispatch'
</file context>
| (github.event_name == 'pull_request' && github.event.action != 'edited') | |
| github.event_name == 'pull_request' |
| # .github/workflows/public-repo-guard.yml | ||
| # .gitleaks.toml | ||
| # scripts/public-repo-guard/content-policy.sh | ||
| # scripts/public-repo-guard/body-policy.sh |
There was a problem hiding this comment.
P2: The install header says to copy four files to set up public-repo-guard in a new repo, but the guard job unconditionally executes a fifth file, scripts/public-repo-guard/tests/body-policy.test.sh, as a required self-test step. A repo that follows the documented four-file install list will fail CI at this step since the test script won't exist. Update the header to include the test script (or gate/remove the self-test step for minimal installs).
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 20:
<comment>The install header says to copy four files to set up public-repo-guard in a new repo, but the guard job unconditionally executes a fifth file, scripts/public-repo-guard/tests/body-policy.test.sh, as a required self-test step. A repo that follows the documented four-file install list will fail CI at this step since the test script won't exist. Update the header to include the test script (or gate/remove the self-test step for minimal installs).</comment>
<file context>
@@ -13,10 +13,11 @@ name: public-repo-guard
# .github/workflows/public-repo-guard.yml
# .gitleaks.toml
# scripts/public-repo-guard/content-policy.sh
+# scripts/public-repo-guard/body-policy.sh
#
# Scan scope: the published working TREE (gitleaks --no-git), NOT git history. The
</file context>
| # scripts/public-repo-guard/body-policy.sh | |
| # scripts/public-repo-guard/body-policy.sh | |
| # scripts/public-repo-guard/tests/body-policy.test.sh |
…d not scan Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| body-guard: | ||
| name: Body content policy |
There was a problem hiding this comment.
🔍 New job's check name is not the required check, so a failing body scan may not block merge
The PR states "On a PR this PREVENTS the merge", but branch protection in these repos requires the check named Secrets + content policy (the guard job). The new job publishes a different check name, Body content policy. Unless the branch-protection rule is updated to also require Body content policy, a blocking body-policy violation will show as a red run but will not gate the merge. Worth confirming the required-checks configuration alongside this PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Branch protection required-checks are repository settings, not workflow code; this PR cannot change them. The observation is correct that "Body content policy" must be added to the required checks for the body scan to gate merges, but that is an admin configuration step outside the PR's files.
The primary scan failed closed on rg errors but both follow-up filters swallowed them with '|| true', collapsing a detection into an empty match list and a green run. The filters now treat rc >= 2 as exit 2, keeping rc 1 (every hit allowlisted) as clean. Also: comment events now scan only the comment body — the enclosing issue/PR body has its own opened/edited events, and re-scanning it on every comment would pin a permanent red run to main for one stale leak. And the changelog gets the Unreleased entry CONTRIBUTING.md requires. 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' | ||
| concurrency: | ||
| group: public-repo-guard-tree-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true |
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 the workflow now fires on body/title edits, while the guard job's if deliberately skips those runs. A job skipped via if produces a check run with conclusion skipped. GitHub branch protection normally treats a skipped required check as passing, but if "Secrets + content policy" is configured as a required status check it is worth confirming on one PR that an edited event does not leave the PR blocked or reset an already-green check.
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.
The \b before OPS_DETAIL could never match after a multi-part credential name's underscore, so name-then-detail order missed every multi-segment name; drop it and add a regression fixture. Key the body-guard concurrency group on comment.id first: review-comment payloads also carry pull_request, so PR-number-first funneled all inline comments into one group where GitHub keeps only one pending run, dropping intermediate comments unscanned. 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.
Two review findings: pull_request_review was the last unscanned body surface: a review's top-level summary lives at .review.body, and the old jq fallback would have scanned the PR body while reporting a verdict on the review. New trigger (submitted/edited), a review.id term in the concurrency chain so review bursts cannot drop a pending scan, and an explicit jq branch. An empty GUARD_PRIVATE_REPOS used to skip the private-repo rule silently, green-stamping bodies the main rule never examined. GitHub withholds configuration variables from fork pull_request runs, so the workflow marks the variable REQUIRED everywhere except fork PRs: required+empty exits 2 (fail closed), fork+empty skips the one rule with a visible warning, local runs stay quiet. Three new fixtures pin all three paths (27 pass). Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| # the private-repo rule with a visible warning. Everywhere else the | ||
| # variable must be set, and the script FAILS CLOSED on an empty one | ||
| # rather than green-stamping a body its main rule never examined. | ||
| GUARD_PRIVATE_REPOS_REQUIRED: ${{ github.event.pull_request.head.repo.fork != true && '1' || '' }} |
There was a problem hiding this comment.
🟡 Missing private-repo configuration silently downgrades to a warning on review comments from forks
The scan of a code-review comment or review summary on a fork's pull request is marked as "allowed to run without the private-repo list" (github.event.pull_request.head.repo.fork != true at .github/workflows/public-repo-guard.yml:205), even though that list is in fact available for those events, so a missing or renamed list turns the gate's main rule into a warning that passes instead of a red run.
Impact: If the private-repo configuration is ever unset or renamed, review comments naming a private repo alongside operational detail would be reported as clean instead of failing.
Why the fork check is wrong for review-family events
GUARD_PRIVATE_REPOS_REQUIRED is set to '1' only when github.event.pull_request.head.repo.fork != true. For pull_request_review and pull_request_review_comment the payload carries the PR object, so on a fork PR head.repo.fork is true and the flag becomes empty. But unlike pull_request, those events (like issue_comment) run in the base repository's context, where vars.GUARD_PRIVATE_REPOS is available — so an empty value there means real misconfiguration, not a platform limitation.
With the flag empty, scripts/public-repo-guard/body-policy.sh:154-160 takes the warn-and-continue branch instead of exit 2, and the private-repo-ops rule at scripts/public-repo-guard/body-policy.sh:161-186 never runs — the exact fail-open the comment at .github/workflows/public-repo-guard.yml:199-204 says it prevents. Note the inconsistency is visible within the same fork PR: an issue_comment on it has no .pull_request in the payload, so it does get '1' and fails closed.
The condition should only relax for the pull_request event itself, e.g. gate on github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork.
| GUARD_PRIVATE_REPOS_REQUIRED: ${{ github.event.pull_request.head.repo.fork != true && '1' || '' }} | |
| GUARD_PRIVATE_REPOS_REQUIRED: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) && '' || '1' }} |
Was this helpful? React with 👍 or 👎 to provide feedback.
| matches="$(printf '%s' "$raw" \ | ||
| | rg -vN -- 'guard:allow[[:space:]]+[^[:space:]]')"; frc=$? |
There was a problem hiding this comment.
🟨 Any author can disable the body leak gate by adding a guard:allow marker to their own text
scripts/public-repo-guard/body-policy.sh:60-61 drops every line containing guard:allow <reason> before any rule verdict is computed. Unlike the tree scanner, where such a marker lives in a reviewable diff, the scanned text here is the untrusted body of a PR, issue, or comment written by an arbitrary (possibly external) author. Appending guard:allow x to the same line neutralizes all rules on that line, including live-credential formats such as the AWS key ID and private-key header checks. The fixture at scripts/public-repo-guard/tests/body-policy.test.sh:88-89 confirms this bypass is exercised and expected.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - 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.
🟨 Body scanner script is executed from the pull request's own checkout, so a fork can neutralize the gate
The body-guard job checks out the event ref (for pull_request this is the fork's merge commit) at .github/workflows/public-repo-guard.yml:152-157 and then runs scripts/public-repo-guard/body-policy.sh from that checkout at .github/workflows/public-repo-guard.yml:206. A fork PR that edits scripts/public-repo-guard/body-policy.sh to exit 0 immediately makes the body gate report green while its own body carries a leak. The same pattern already exists for the tree job's content-policy.sh, so this is not newly introduced, but the new job extends it to the body surface.
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.
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 6cb1806. Configure here.
Note
Add CI scanning of PR/issue/comment bodies for internal-leak patterns
body-guardjob in public-repo-guard.yml that triggers on issue, comment, and review events and scans their text bodies.guard:allowmarkers and redacted annotations.guardjob runs as a self-test step on every CI run.guardjob gains per-PR concurrency with cancel-in-progress and skips body-only events to avoid redundant tree scans.Macroscope summarized 7ed1c87.