ci: scan issue and comment bodies — this repo has never scanned one - #29
ci: scan issue and comment bodies — this repo has never scanned one#29yakimoto wants to merge 12 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_895e2909-3829-40f4-93e0-63e37e037599) |
|
Running ultrareview automatically — This PR rewires the repo's merge-blocking secret scanner: new issue/comment triggers, per-job concurrency, and a fail-closed body regex engine whose false-positive or false-negative bugs could either stall every PR or let credentials leak in public issue and comment bodies.. I'll post findings when complete. |
ApprovabilityVerdict: Needs human review Unable to check for correctness in d2e4139. This PR introduces security-sensitive CI infrastructure with multiple unresolved review comments identifying potential bypass vulnerabilities and design inconsistencies. The concerns about gate scripts executing from PR checkout and the body scan being advisory-only 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.
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
![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.
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 9m 52s
2 issues found and verified against the latest diff
Confidence score: 2/5
- In
.github/workflows/public-repo-guard.yml, the PR body scanner executesscripts/public-repo-guard/body-policy.shfrom the PR’s own checkout, so an attacker can modify the script in the same PR to suppress findings and bypass the control; this weakens a security boundary for public-repo leak prevention — run the policy script from a trusted ref (default branch/pinned commit) instead of PR code. - In
.github/workflows/public-repo-guard.yml, leak detection appears fail-open becauseBody content policyis separate from the required merge-protection context, so merges can proceed even when the scanner fails or is bypassed; this creates a direct path for sensitive content to slip through — make the scanning check itself required (or block on its result in the required context).
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: The new body leak gate can be bypassed on pull requests because it executes `scripts/public-repo-guard/body-policy.sh` from the PR checkout itself. That means the same PR can change the scanner logic and force a green result even when the PR body should be blocked. Using the trusted base ref for PR events keeps enforcement code immutable during evaluation.</violation>
<violation number="2" location=".github/workflows/public-repo-guard.yml:114">
P1: Custom agent: **Flag Security Vulnerabilities**
PR body leak detection is currently fail-open for merge protection. The new scanner runs in a separate `Body content policy` check context, while the required context called out in this repo is still `Secrets + content policy`; that means a leaking PR body can fail `body-guard` without failing the required gate. This is especially visible on `pull_request` `edited`, where the required `guard` job is skipped and only the non-required body job runs. Consider routing body-scan failure into the required check context (or updating required checks) so PR body leaks actually block merges.</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: The new body leak gate can be bypassed on pull requests because it executes scripts/public-repo-guard/body-policy.sh from the PR checkout itself. That means the same PR can change the scanner logic and force a green result even when the PR body should be blocked. Using the trusted base ref for PR events keeps enforcement code immutable during evaluation.
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>The new body leak gate can be bypassed on pull requests because it executes `scripts/public-repo-guard/body-policy.sh` from the PR checkout itself. That means the same PR can change the scanner logic and force a green result even when the PR body should be blocked. Using the trusted base ref for PR events keeps enforcement code immutable during evaluation.</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>
| # detection — it tells us to go redact, fast. Only the client-side pre-write hook | ||
| # can stop that class before publication. | ||
| body-guard: | ||
| name: Body content policy |
There was a problem hiding this comment.
P1: Custom agent: Flag Security Vulnerabilities
PR body leak detection is currently fail-open for merge protection. The new scanner runs in a separate Body content policy check context, while the required context called out in this repo is still Secrets + content policy; that means a leaking PR body can fail body-guard without failing the required gate. This is especially visible on pull_request edited, where the required guard job is skipped and only the non-required body job runs. Consider routing body-scan failure into the required check context (or updating required checks) so PR body leaks actually block merges.
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 114:
<comment>PR body leak detection is currently fail-open for merge protection. The new scanner runs in a separate `Body content policy` check context, while the required context called out in this repo is still `Secrets + content policy`; that means a leaking PR body can fail `body-guard` without failing the required gate. This is especially visible on `pull_request` `edited`, where the required `guard` job is skipped and only the non-required body job runs. Consider routing body-scan failure into the required check context (or updating required checks) so PR body leaks actually block merges.</comment>
<file context>
@@ -71,3 +92,72 @@ jobs:
+ # detection — it tells us to go redact, fast. Only the client-side pre-write hook
+ # can stop that class before publication.
+ body-guard:
+ name: Body content policy
+ if: github.event_name == 'pull_request' || github.event_name == 'issues' || github.event_name == 'issue_comment'
+ concurrency:
</file context>
…E2 at install Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…public surface Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| # --- Infrastructure identifiers ---------------------------------------------- | ||
| # shellcheck disable=SC2016 # $CLOUDFLARE_ACCOUNT_ID is literal guidance text | ||
| check BLOCK cf-account-id 'account_id\s*[:=]\s*["'"'"']?[0-9a-f]{32}' 'Hardcoded Cloudflare account_id — reference the env var instead' | ||
| check BLOCK internal-ip '100\.(6[4-9]|[7-9][0-9]|1[01][0-9]|12[0-7])\.[0-9]{1,3}\.[0-9]{1,3}' 'Internal Tailscale-CGNAT IP (100.64.0.0/10) — internal fleet address' | ||
| # shellcheck disable=SC2016 # $HOME is literal guidance text | ||
| check BLOCK abs-user-path '/(Users|home)/(?!runner/)[a-z][a-z0-9._-]+/' 'Operator absolute home path — leaks identity and local layout' |
There was a problem hiding this comment.
🔍 Infrastructure rules will block bodies that merely discuss the gate's own patterns
internal-ip, cf-account-id and abs-user-path deliberately do not accept the ABOUT-THE-CONTROL allowlist. That means a PR/issue body describing this gate — e.g. quoting 100.64.0.0/10 or pasting /home/someone/repo from a repro — blocks the merge, and the only escape is an explicit guard:allow <reason> in the body. The internal-ip regex matches 100.64.0.0 inside the CIDR notation itself, so the design doc / release notes for this very feature would trip it. Worth confirming this friction is acceptable, since the file's own comments argue that a gate people fight gets switched off.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This friction is intentional and documented in the file: credential and infrastructure rules deliberately ignore the ABOUT-THE-CONTROL allowlist because an infra identifier is a leak even on a line discussing the gate, and the explicit, publicly visible guard:allow is the designed escape for docs and design discussions that must quote the patterns.
| - 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.
🔍 Gate scripts are executed from the pull request's own checkout
Both jobs check out the PR merge ref and then run bash scripts/public-repo-guard/body-policy.sh from that checkout, so a pull request that edits (or empties) the policy script is scanned by its own modified version and can report a pass. This is pre-existing behaviour for the tree job, but the new body job inherits it, and the body job is the one whose whole premise is that server-side scanning cannot be bypassed client-side. If the intent is tamper-resistance, the gate scripts would need to come from the base ref (e.g. a second checkout of github.event.pull_request.base.sha into a separate path, or a git checkout base -- scripts/public-repo-guard).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
On pull_request events GitHub executes the workflow definition itself from the PR merge ref, so a fork PR can already edit the YAML to bypass the gate; pinning the script checkout to a fixed ref gains nothing. The pull_request-over-pull_request_target trade (no write token or secrets for forks) is deliberate and documented, and true tamper-proofing must come from a source outside the repo checkout entirely.
…credential names 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. | ||
| # | ||
| # Names are NOT hardcoded (this file is public); CI injects them via the | ||
| # GUARD_PRIVATE_REPOS variable. Unset locally → this check is skipped. | ||
| if [[ -n "${GUARD_PRIVATE_REPOS:-}" ]]; then | ||
| OPS_DETAIL='(?:[A-Z][A-Z0-9]*_(?:SECRET|TOKEN|KEY|PASSWORD)|wrangler\s+secret|secret\s+(?:is\s+)?(?:bound|binding|list)|(?:is\s+)?bound\s+on|service\s+binding|\d{2,}\s+secrets)' |
There was a problem hiding this comment.
🔍 Proximity matching is line-scoped, so multi-line bodies split repo name and ops detail
The proximity pattern uses [^\n]{0,140}? between the repo name and OPS_DETAIL, and rg matches per line, so a real PR body that names a private repo on one line and its bound secret on the next line will not fire. That is a deliberate precision trade-off (bare mentions must stay silent), but it means the common markdown shape — a bullet list where each fact is its own line — is the exact shape that evades the rule. Worth confirming this matches the intended threat model.
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.
…e newline-separated lists Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
| _no_private_repo_config() { | ||
| if [[ "${GITHUB_ACTIONS:-}" == "true" ]]; then | ||
| echo "::error title=public-repo-guard (private-repo-ops)::GUARD_PRIVATE_REPOS is empty (the org variable is unset, renamed, or not visible to this run). Refusing to report a pass with the private-repo rule silently disabled." | ||
| exit 2 | ||
| fi | ||
| echo "body-policy: GUARD_PRIVATE_REPOS unset; private-repo-ops rule skipped (local run only)" | ||
| } | ||
| if [[ -z "${GUARD_PRIVATE_REPOS:-}" ]]; then | ||
| _no_private_repo_config |
There was a problem hiding this comment.
🔍 Fail-closed on empty GUARD_PRIVATE_REPOS may hard-fail every fork PR
The new fail-closed branch turns an empty GUARD_PRIVATE_REPOS into exit 2 whenever GITHUB_ACTIONS=true, and the same semantics were added to scripts/public-repo-guard/body-policy.sh:140-148. Both workflows inject the value from ${{ vars.GUARD_PRIVATE_REPOS }} (.github/workflows/public-repo-guard.yml:118, .github/workflows/public-repo-guard-body.yml:133). Worth confirming before rollout: whether org/repo configuration variables are exposed to pull_request runs originating from a fork, and whether the org variable's visibility list includes this repo. If either is not the case, Secrets + content policy — a required check — will hard-fail (exit 2, indistinguishable to a contributor from a real leak) on every external contribution to what is an examples repo whose main contribution path is forking. If that is a real risk, consider keeping the fail-closed behaviour only for push/workflow_dispatch/same-repo PRs, or asserting the variable's presence once in the workflow with a clearer error.
Was this helpful? React with 👍 or 👎 to provide feedback.
…e retarget 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.
…t body-guard merge-gate claim 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.
…r reports skipped A job-level 'if' skip still creates a check run (conclusion 'skipped') on the PR head SHA, required checks treat 'skipped' as satisfied, and the newest check run of a name wins: with 'edited' (and review events) in the tree gate's trigger list, editing a PR description or posting a review re-reported 'Secrets + content policy' as skipped, laundering an earlier genuine failure on the same commit into a mergeable PR. The tree workflow now triggers only on events it really scans, running a full scan on 'edited' (preserving base-retarget coverage), and body-guard moves to public-repo-guard-body.yml with the issue/comment/review triggers. 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_2443c9a7-d0b0-4ed4-b30f-22f4cdcdf321) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…d events cancelling tree scans 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.
… cased internal markers 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 | ||
| # The gate only reads the tree; no later step may find the workflow token | ||
| # in .git/config — matches every other workflow in this repo. | ||
| persist-credentials: false |
There was a problem hiding this comment.
🟨 Body-leak gate executes the pull request's own copy of the scanner, so a PR can weaken the check that scans it
On pull_request events actions/checkout defaults to the PR merge ref, so scripts/public-repo-guard/body-policy.sh executed at .github/workflows/public-repo-guard-body.yml:134 is the version proposed by the PR, not the trusted version from the default branch. A PR that both leaks internal detail in its body and modifies (or deletes) the policy script in the same change would be scanned by its own, potentially neutered, scanner. The token is read-only (persist-credentials: false, contents: read), so this is gate integrity rather than code-execution privilege escalation; the tree gate has the same property inherently because it must scan the PR tree, but the body gate has no such need — it only needs the trusted scanner.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
On pull_request / pull_request_review / pull_request_review_comment events GitHub executes the workflow YAML itself from the PR merge ref, so a PR that can neuter body-policy.sh can equally rewrite the job to emit a green check with no scan; pinning the script checkout to the default branch adds no integrity boundary. On issues/issue_comment events both the workflow and the checkout already come from the default branch, so the scanner is already trusted there.
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
Four 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 body gate lives in its OWN workflow file, not as a second job beside the tree gate: a job-level
ifskip still creates a check run (conclusionskipped) on the PR head SHA, required checks treatskippedas satisfied, and the newest check run of a name wins. If the tree gate's workflow also fired on issue/comment/review events only to skip theguardjob, any comment or body edit would launder an earlier genuineSecrets + content policyfailure on the same commit into a mergeable PR. Splitting the files means the required check never reports a skipped verdict.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
Medium Risk
Changes org merge-gate behavior and required-check semantics (skipped vs failure laundering); misconfigured
GUARD_PRIVATE_REPOSor missing PCRE2 ripgrep will fail CI until fixed.Overview
Closes the gap where only the published tree was scanned while PR/issue/comment/review text could leak the same material (e.g. private repo names plus operational detail in a body while
wrangler.tomlwas blocked).Adds a separate workflow
public-repo-guard-body.ymlwith aBody content policyjob onpull_request,issues,issue_comment,pull_request_review, andpull_request_review_comment(includingedited). It materializes untrusted title/body viajqinto a temp file, sparse-checkoutsscripts/public-repo-guard, verifies PCRE2-enabled ripgrep, and runs the newbody-policy.sh. Splitting workflows avoids a skipped requiredSecrets + content policyrun replacing a real tree-scan failure on the same commit.Updates
public-repo-guard.ymlso the treeguardjob stays on tree-changing events only, uses per-job concurrency, bumpsactions/checkoutto v7.0.1, adds PCRE2 checks for policy scripts, documents a six-file install bundle, and runstests/body-policy.test.shin CI.body-policy.shapplies credential/infra rules plus a proximity rule: private repo names fromGUARD_PRIVATE_REPOSonly block when paired with operational detail (not bare cross-refs). In CI, emptyGUARD_PRIVATE_REPOSfails closed (exit 2).content-policy.shnow parses multiline/newline-separatedGUARD_PRIVATE_REPOSthe same way as the body gate.Reviewed by Cursor Bugbot for commit 4a64de0. Configure here.
Note
Add issue and comment body scanning to the public repo guard CI
GUARD_PRIVATE_REPOS.GUARD_PRIVATE_REPOSvalues and fail closed in CI when the variable is empty or separator-only.GUARD_PRIVATE_REPOSorg/repo variable now causes the job to exit with code 2 (fail closed) instead of silently skipping the private-repo rule.Macroscope summarized d2e4139.