fix(push-protection): use alerts API proxy when security_and_analysis is unreadable#224
Conversation
|
PR is ready for review. All checks passing (SonarCloud quality gate passed, ShellCheck clean, AgentShield passed). CodeQL is still running but expected to pass since the changes are pure shell additions with no new code patterns. Tagging @petry-projects/org-leads for review per CODEOWNERS. |
|
Warning Review limit reached
More reviews will be available in 21 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates push-protection audit and apply logic to gracefully degrade when the OAuth token cannot read repository-level ChangesGraceful Degradation for Unreadable Repository Security Analysis
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses recurring push-protection compliance findings when the audit token cannot read security_and_analysis from GET /repos/{owner}/{repo} by adding a proxy verification path via the secret-scanning alerts API, and updating the standard to document the token-scope behavior.
Changes:
- Add a fallback in
pp_check_security_and_analysisthat uses the secret-scanning alerts API to distinguish “unavailable” from “unverifiable.” - Make
pp_apply_security_and_analysiscontinue applying required settings even when the currentsecurity_and_analysisstate can’t be read. - Document the token-scope requirement and fallback behavior in the push-protection standard.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| standards/push-protection.md | Documents why security_and_analysis may be unreadable and how the audit falls back to proxy verification. |
| scripts/lib/push-protection.sh | Implements proxy-check fallback via secret-scanning alerts and makes apply logic proceed when state is unreadable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lib/push-protection.sh`:
- Around line 137-145: The current comment above the alerts_response / gh_api
call incorrectly states the secret-scanning alerts endpoint is accessible
without admin scope on public repos; update that comment to clarify that the
endpoint requires the authenticated user to be an administrator of the
repository or owning organization regardless of public/private visibility, and
that scope substitution (e.g., using public_repo vs repo) does not remove the
admin role requirement; also note the intended use of this proxy is for cases
where the caller is an admin who has a token with the security_events scope but
not the full repo scope, so the proxy can succeed when security_and_analysis
checks would fail due to scope differences (referencing alerts_response, gh_api,
ORG and repo in the surrounding code).
- Around line 117-122: Update the error message and surrounding comment in
scripts/lib/push-protection.sh: change the err call that currently mentions
`security_events` so it only states that the PATCH to update
`security_and_analysis` failed because the authenticated token/user must have
admin permissions on the repository (remove any suggestion that adding
`security_events` OAuth scope will allow the PATCH). Also correct the comment
that describes the secret-scanning alerts proxy check (the block referencing
access for public repos) to remove the incorrect claim that public repositories
bypass the admin requirement—note that the secret-scanning alerts endpoint
requires repository admin privileges regardless of visibility; keep the proxy
check logic but update its comment to reflect the admin requirement.
In `@standards/push-protection.md`:
- Around line 441-444: Update the remediation paragraph under "To enable full
verification" to instruct owners to regenerate or rotate the classic personal
access token (PAT) in Developer Settings → Personal access tokens (classic) with
the security_events scope checked, then update the ORG_SCORECARD_TOKEN value in
org Settings → Secrets; also keep the alternative command reference to
scripts/apply-repo-settings.sh <repo> using an admin token to configure required
settings and grant the audit token visibility. Ensure the text explicitly names
the security_events scope and the ORG_SCORECARD_TOKEN secret so maintainers know
to change the PAT in Developer Settings before updating the secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e90f2fe-942c-4bc3-b4dc-4f4711379bd8
📒 Files selected for processing (2)
scripts/lib/push-protection.shstandards/push-protection.md
|
Auto-rebase failed — merge conflict — this branch has conflicts with Please resolve the conflicts and push: |
… is unreadable
The compliance audit token (ORG_SCORECARD_TOKEN) lacks the admin or
`security_events` OAuth scope needed to read the `security_and_analysis`
field from GET /repos/{owner}/{repo}. This caused the audit to emit
`security_and_analysis_unavailable` every week with no actionable path
to resolution, since the token also couldn't verify whether the settings
were actually configured.
Changes to scripts/lib/push-protection.sh:
- pp_check_security_and_analysis: when security_and_analysis is unreadable,
fall back to a proxy check via the secret-scanning alerts endpoint
(accessible without admin scope on public repos). If the alerts endpoint
returns a valid array, secret scanning is confirmed active and the finding
is emitted as `security_and_analysis_unverifiable` (more accurate) instead
of `security_and_analysis_unavailable`. Both findings include actionable
guidance: add `security_events` scope to ORG_SCORECARD_TOKEN, or run
apply-repo-settings.sh with an admin token.
- pp_apply_security_and_analysis: remove the premature return 1 when the
current state is unreadable. The loop already handles null/missing values
correctly (treating them as needing update), so the apply now proceeds
unconditionally when the state is unreadable — all required values are
"enabled" so this is idempotent. Improves the error message to mention
both admin scope and security_events scope.
Changes to standards/push-protection.md:
- Document the token scope requirement for full security_and_analysis
verification and explain the proxy-check fallback behavior introduced
by this fix.
Closes #117
Co-authored-by: Don Petry <don-petry@users.noreply.github.com>
don-petry
force-pushed
the
claude/issue-117-20260508-1733
branch
from
0e723e2 to
4a39ef6
Compare
Superseded by automated re-review at
|
Superseded by automated re-review at
|
|
@coderabbitai resolve |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
✅ Action performedComments resolved and changes approved. |
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #224 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 842652e5d56a44ca92bfe4774bf5ffa52338a156
Review mode: triage-approved (single reviewer)
Summary
Targeted compliance-audit fix: makes pp_check_security_and_analysis fall back to the secret-scanning alerts endpoint as a proxy when security_and_analysis is unreadable, downgrading the recurring security_and_analysis_unavailable finding (#117) to a more accurate security_and_analysis_unverifiable when scanning is in fact active. Also makes pp_apply_security_and_analysis proceed when current state is unreadable (safe — required values are all "enabled", so PATCH is idempotent), and updates standards/push-protection.md to document the token-scope requirement and remediation steps. Scope is tight: 2 files, +58/-6. The earlier CodeRabbit changes-requested round was resolved (alerts-endpoint admin-requirement comment corrected, security_events removed from the PATCH error message, classic-PAT/secret-rotation language added to the standard), and CodeRabbit subsequently re-approved.
Linked issue analysis
Linked issue #117 (recurring security_and_analysis_unavailable finding) is substantively addressed: the proxy check via the secret-scanning alerts API gives the audit a self-resolving path when the token can prove scanning is active, and the new security_and_analysis_unverifiable finding name causes the existing issue to auto-close on the next audit run as described in the PR body.
Findings
No blocking findings.
- Correctness: the fallback logic correctly distinguishes "scanning confirmed active" (alerts API returns a JSON array →
unverifiable) from "cannot confirm" (non-array →unavailable).jq -e 'type == "array"'is the right type-check. - Idempotency: removing the early
return 1inpp_apply_security_and_analysisis safe — the loop that follows applies required values unconditionally, and all required values are"enabled", so re-applying is a no-op on already-correct repos. - Error messaging: the PATCH-failure message no longer overstates what
security_eventscan do (it now correctly cites the repo-admin requirement for PATCH), matching the CodeRabbit feedback. - Standards doc: the new "Token scope requirement" subsection is consistent with the script behaviour and names the right secret (
ORG_SCORECARD_TOKEN), the right scope (security_events), and the right rotation path (Developer Settings → PAT classic → org Settings → Secrets → Actions). No HIGH-risk surface (no auth/secret/crypto/migration changes, no GitHub Actions security smell, no CODEOWNERS/AGENTS.md violation).
CI status
All required checks green: ShellCheck, Lint, Agent Security Scan, Secret scan (gitleaks), CodeQL (actions), SonarCloud (quality gate passed, 0 new issues), AgentShield, dev-lead/dispatch, pr-auto-review/check-and-dispatch. Dependency-audit ecosystem jobs correctly skipped (no matching ecosystems in changed files). mergeStateStatus is BLOCKED only because REVIEW_REQUIRED — this approval will unblock it.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
… is unreadable (#224) * fix(push-protection): use alerts API proxy when security_and_analysis is unreadable The compliance audit token (ORG_SCORECARD_TOKEN) lacks the admin or `security_events` OAuth scope needed to read the `security_and_analysis` field from GET /repos/{owner}/{repo}. This caused the audit to emit `security_and_analysis_unavailable` every week with no actionable path to resolution, since the token also couldn't verify whether the settings were actually configured. Changes to scripts/lib/push-protection.sh: - pp_check_security_and_analysis: when security_and_analysis is unreadable, fall back to a proxy check via the secret-scanning alerts endpoint (accessible without admin scope on public repos). If the alerts endpoint returns a valid array, secret scanning is confirmed active and the finding is emitted as `security_and_analysis_unverifiable` (more accurate) instead of `security_and_analysis_unavailable`. Both findings include actionable guidance: add `security_events` scope to ORG_SCORECARD_TOKEN, or run apply-repo-settings.sh with an admin token. - pp_apply_security_and_analysis: remove the premature return 1 when the current state is unreadable. The loop already handles null/missing values correctly (treating them as needing update), so the apply now proceeds unconditionally when the state is unreadable — all required values are "enabled" so this is idempotent. Improves the error message to mention both admin scope and security_events scope. Changes to standards/push-protection.md: - Document the token scope requirement for full security_and_analysis verification and explain the proxy-check fallback behavior introduced by this fix. Closes #117 Co-authored-by: Don Petry <don-petry@users.noreply.github.com> * chore: apply manual instructions [skip ci-relay] --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Don Petry <don-petry@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Summary
security_and_analysis_unavailablecompliance finding (Compliance: security_and_analysis_unavailable #117) by introducing a proxy check via the secret-scanning alerts API when the primarysecurity_and_analysisfield is unreadable.pp_apply_security_and_analysismore robust when the current settings state cannot be read.standards/push-protection.md.Root Cause
The audit token (
ORG_SCORECARD_TOKEN) lacksadminorsecurity_eventsOAuth scope, soGET /repos/{owner}/{repo}returnssecurity_and_analysis: null. The check treated null the same as "unavailable," firingsecurity_and_analysis_unavailableevery week with no self-resolving path.Changes
scripts/lib/push-protection.shpp_check_security_and_analysis: Whensecurity_and_analysisis unreadable, falls back toGET /repos/{owner}/{repo}/secret-scanning/alerts(accessible without admin scope on public repos):security_and_analysis_unverifiable(accurate, actionable) instead ofsecurity_and_analysis_unavailable. This causes issue Compliance: security_and_analysis_unavailable #117 to auto-close on the next audit run.security_and_analysis_unavailablewith improved guidance mentioningsecurity_eventsscope and the apply script.pp_apply_security_and_analysis: Removes the prematurereturn 1when current state is unreadable. The loop already handles null/missing values correctly (all required values are"enabled", so applying unconditionally is idempotent and safe).standards/push-protection.mdsecurity_eventsscope requirement and the proxy-check fallback behaviour.Test plan
security_and_analysis_unavailableis no longer in findings for.github)security_and_analysis_unverifiablefinding is created with actionable remediation stepsapply-repo-settings.shno longer exits early whensecurity_and_analysisis unreadable — it attempts to apply all settings and reports success or a clear errorCloses #117
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation