fix(governance): split credentialed live policy audit - #685
fix(governance): split credentialed live policy audit#685hyperpolymath wants to merge 4 commits into
Conversation
|
Warning Review limit reachedNext included review available in 8 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While this PR successfully decouples the tree-based and live governance audits, it introduces several critical issues that should prevent merging. Most significantly, renaming the primary governance job is a breaking change that will block merges across all repositories using these reusable workflows due to branch protection requirements.
Additionally, there are fundamental logic errors in how the 'live' policy check is gated; using environment variables within 'if' conditions before they are initialized ensures that the security check is always skipped and the advisory notice is always shown. There is also a violation of the bootstrap logic, meaning changes to the auditing scripts cannot be verified within this repository's own Pull Requests. Finally, several redundant steps were left in the 'preflight' jobs that fetch scripts no longer used in those contexts.
About this PR
- The PR leaves behind redundant logic in the 'preflight' jobs. Sparse-checkout, staging, and copy operations for 'scripts/check-actions-policy.sh' are still present even though that script is now handled exclusively by the new 'actions-policy-live' jobs. These should be removed to reduce maintenance surface and improve performance.
Test suggestions
- The tree-based allowlist (preflight) job continues to block execution on policy violations.
- The live policy job reports an advisory notice and succeeds when the HYPATIA_SCAN_PAT secret is missing.
- The live policy job executes the administration API check when the HYPATIA_SCAN_PAT secret is provided.
- The live policy job fails when the PAT is provided but the repository configuration is non-compliant.
- The standards repository tests local changes to the audit scripts in its own CI workflows (bootstrap logic).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. The standards repository tests local changes to the audit scripts in its own CI workflows (bootstrap logic).
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| # endpoint. When a caller deliberately supplies this credential, an | ||
| # API/authentication failure or real policy violation remains red. | ||
| GH_TOKEN: ${{ secrets.HYPATIA_SCAN_PAT }} | ||
| if: ${{ env.GH_TOKEN != '' }} |
There was a problem hiding this comment.
🔴 HIGH RISK
The 'if' condition is evaluated before the step-level 'env' block is applied. This causes the security gate to be skipped even when the required secret is provided. Reference the secret directly in the condition.
| env: | ||
| GH_TOKEN: ${{ secrets.HYPATIA_SCAN_PAT || github.token }} | ||
| GH_TOKEN: ${{ secrets.HYPATIA_SCAN_PAT }} | ||
| if: ${{ env.GH_TOKEN != '' }} |
There was a problem hiding this comment.
🔴 HIGH RISK
The 'if' condition is evaluated before the step-level 'env' block is applied. This means 'env.GH_TOKEN' will be empty, and this security check will always be skipped. Use the secret context directly to ensure the policy check runs when intended:
if: ${{ secrets.HYPATIA_SCAN_PAT != '' }}| jobs: | ||
| preflight: | ||
| name: Actions policy (tree + live) | ||
| name: Actions policy (tree) |
There was a problem hiding this comment.
🔴 HIGH RISK
Renaming the job from Actions policy (tree + live) to Actions policy (tree) is a breaking change. Any repository using this reusable workflow with the old name specified as a 'Required' status check in branch protection will have its merges blocked.
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| repository: hyperpolymath/standards | ||
| ref: main |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The actions-policy-live job ignores the 'Bootstrap rule' by hardcoding ref: main. This prevents testing changes to check-actions-policy.sh within the standards repository's own PRs.
| - name: Report unavailable live policy credential | ||
| env: | ||
| GH_TOKEN: ${{ secrets.HYPATIA_SCAN_PAT }} | ||
| if: ${{ env.GH_TOKEN == '' }} |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The 'if' condition is evaluated before the step-level 'env' block is applied. This means 'env.GH_TOKEN' will be empty, and this notice will always be displayed regardless of secret availability. Use secrets.HYPATIA_SCAN_PAT directly.
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| repository: hyperpolymath/standards | ||
| ref: main |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This checkout hardcodes ref: main, which violates the 'Bootstrap rule' where the standards repository should test its own local scripts during PRs. When the repository is hyperpolymath/standards, it should use the current SHA to ensure changes to check-actions-policy.sh are verified before merging.
| - name: Report unavailable live policy credential | ||
| env: | ||
| GH_TOKEN: ${{ secrets.HYPATIA_SCAN_PAT }} | ||
| if: ${{ env.GH_TOKEN == '' }} |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The 'if' condition is evaluated before the step-level 'env' block is applied. This means 'env.GH_TOKEN' will be empty at the time of check, and this notice will always be displayed. Reference the secret context directly:
if: ${{ secrets.HYPATIA_SCAN_PAT == '' }}
6b67ada to
541214b
Compare
|
…688) ## Summary - removes the four high-severity Hypatia findings without weakening the scanner - records the remaining 61 reviewed medium findings as exact-file, severity-preserving baseline entries tracked by #687 - makes the baseline schema and executable validator support Hypatia rule IDs such as `HYP-S009` - separates credentialed live Actions-policy auditing from the deterministic pull-request gate, with explicit degraded-mode reporting - validates pinned workflow-staleness sources and adds regression tests for both policy paths - patches the Dependabot-reported `gix-packetline` denial of service and the additional audited `h2` denial of service in the certifier lockfile - adds optional, exact-version Zig installation to the reusable Rust workflow so native Zig-backed crates can use the central CI without ad hoc setup - refreshes only the derived registry hashes affected by these source changes ## Verification - exact Zig 0.15.2: `zig build test` - baseline tests: 8/8 passed - Actions-policy regression tests: 12/12 passed - workflow-staleness tests: 20/20 passed - authoritative strict Hypatia baseline gate: 0 kept, 61 suppressed - baseline ratchet with the actual base: 104 to 129 entries accepted only through the declared, issue-linked exception - `cargo audit --no-fetch`: 0 vulnerabilities after the two lockfile updates - `gh actions-lock --no-fix`: clean across 44 workflows - `actionlint .github/workflows/rust-ci-reusable.yml`: clean - registry/topology check: clean ## Boundaries and follow-up - no broad path exclusion or severity downgrade is introduced - four warning-class dependency advisories remain (two unsoundness warnings and two yanked crates); this PR does not misrepresent them as resolved vulnerabilities - the incomplete `rsr-certifier` workspace is tracked separately in #689 instead of being papered over with placeholder modules or features - the 61 reviewed medium Hypatia findings remain explicit debt in #687 This PR now supersedes #685 by containing its live/deterministic policy split as well as the security and reusable-CI work.
1 participant
Summary
HYPATIA_SCAN_PATwas not supplied, without claiming the live policy passedpreserving consumer pin semantics and allowing Standards PRs to test their
own fixes
the canonical Standards reusable to publish centrally managed results
Why
The census in #656 established that 331 of 332 governance callers cannot pass the combined check: the ordinary job token cannot read the Administration endpoint, while the PAT was neither distributed nor inherited. A permanently red advisory gives no useful policy signal.
This implements option C from #656. It does not silently equate a tree check with a live settings audit. The two capabilities now have distinct contexts and conclusions.
Verification
bash scripts/tests/actions-policy-486-test.sh— 12/12 positive and negative cases pass, including an API/authentication-unavailable controlbash scripts/tests/check-workflow-staleness-test.sh— 20/20 positive and negative cases pass, including consumer direct-SARIF rejection and canonical-publisher acceptanceactionlint -ignore 'SC2086' -ignore 'property "workflow_sha" is not defined' .github/workflows/governance-reusable.yml .github/workflows/allowlist-preflight-reusable.yml— pass; the second suppression is limited to the current actionlint schema lag for GitHub's documentedjob.workflow_shacontextgit diff --check— passgh actions-lock --no-fixstill reaches the pre-existing unsupported local-action error insigned-push-smoke.yml; this change adds no new action identity or refCloses #656