ci: Add static analysis of GitHub Actions with zizmor#2693
ci: Add static analysis of GitHub Actions with zizmor#2693matthewfeickert wants to merge 16 commits into
Conversation
matthewfeickert
added
CI
| permissions: | ||
| contents: write # for Git to git push | ||
| runs-on: ubuntu-latest | ||
| environment: ci |
There was a problem hiding this comment.
@henryiii is https://docs.zizmor.sh/audits/#secrets-outside-env going to mean that every GitHub action run triggers a huge number of "deployments" now because this exists in an environment?
There was a problem hiding this comment.
I'm not sure, but I think so. This check is only triggered with the "auditor" level or something like that, right?
There was a problem hiding this comment.
That can be avoided à la data-apis/array-api-extra#699
There was a problem hiding this comment.
There was a problem hiding this comment.
but now the CI isn't running.
what do you mean by this? The required status checks are showing as pending because the job names have been changed
There was a problem hiding this comment.
The required status checks are showing as pending because the job names have been changed
@lucascolley you're (in retrospect, obviously) very correct
test (ubuntu-latest, 3.13)
vs. now
CI/CD / CI (ubuntu-latest, 3.13) (pull_request)
Thanks for pointing this out. Apologies for not having taken the time to think before posting. I didn't realize that in
jobs:
test:
name: CI
runs-on: ${{ matrix.os }}
environment:
name: ci
deployment: false
...that name: CI would override everything else — I thought it was internal naming.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2693 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 65 65
Lines 4305 4305
Branches 465 465
=======================================
Hits 4231 4231
Misses 46 46
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
matthewfeickert
force-pushed
the
feat/add-zizmor
branch
from
366b9bc to
f9c4a87
Compare
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request introduces security hardening and static analysis tooling across GitHub Actions workflows. Changes include disabling credential persistence in checkout steps, adding GitHub Actions environment configurations, introducing Zizmor security scanning via pre-commit hooks, implementing a Dependabot cooldown policy, and refactoring the version bump workflow to use environment variables instead of direct context references. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 27 seconds.Comment |
|
On the Scientific Python Discord, Seth Larson pointed out
and @tupi helpfully mentioned that in addition to permissions:
security-events: writeunder https://docs.zizmor.sh/integrations/#manual-integration you can write out a sarif file. |
matthewfeickert
force-pushed
the
feat/add-zizmor
branch
from
661ad3b to
a5c3f78
Compare
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request_target: # zizmor: ignore[dangerous-triggers] |
There was a problem hiding this comment.
I'm not sure if there is anything that can be done about this given https://github.com/amannn/action-semantic-pull-request/tree/45b9ed7cf24087a2f7785bf55be97394ba87e1c2#installation still shows uing pull_request_target.
|
|
||
| - name: Update the Git tag annotation | ||
| if: ${{ github.event.inputs.dry_run }} == 'false' | ||
| if: ${{ github.event.inputs.dry_run }} == 'false' # zizmor: ignore[unsound-condition] |
There was a problem hiding this comment.
This is one of two ignores, but in this particular case I think this is acceptable?
Without this I get https://docs.zizmor.sh/audits/#unsound-condition
error[unsound-condition]: unsound conditional expression
--> .github/workflows/bump-version.yml:238:7
|
238 | if: ${{ github.event.inputs.dry_run }} == 'false'
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ condition always evaluates to true
|
= note: audit confidence → High
22 findings (1 ignored, 20 suppressed): 0 informational, 0 low, 0 medium, 1 high
but dry_run can be set by the user.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.pre-commit-config.yaml (1)
82-89: 🧹 Nitpick | 🔵 TrivialAdd zizmor to CI with SARIF output for centralized GitHub Security tab visibility.
Currently zizmor runs only in pre-commit locally. Pair it with a CI workflow step using zizmor's
--format=sarifoutput andgithub/codeql-action/upload-sarifto expose findings centrally in the Security tab. This complements the existing SARIF infrastructure (scorecard.yml, codeql-analysis.yml) and provides auditable, persistent records of workflow security checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 82 - 89, Add a CI workflow step that runs the existing zizmor pre-commit hook in CI and emits SARIF so findings appear in the GitHub Security tab: run the zizmor hook (the hook id "zizmor" from the .pre-commit-config entry) in the workflow instead of relying only on local pre-commit, invoke it with the --format=sarif argument (replace or extend the current args array that contains --persona=regular), save the generated SARIF artifact, and add the github/codeql-action/upload-sarif step to upload that SARIF file; ensure the workflow step names and paths reference the same hook id "zizmor" and produce a deterministic SARIF filename for upload..github/workflows/bump-version.yml (1)
52-57:⚠️ Potential issue | 🟠 MajorRe-authenticate before the final
git push.With
persist-credentials: falseat line 57, the PAT token is not persisted to Git config, so thegit pushcommand at line 284 will fail with an authentication error. The workflow needs explicit credential setup before the push step.Suggested fix
- name: Push new tag back to GitHub shell: bash run: | - if [ ${GITHUB_EVENT_INPUTS_DRY_RUN} == 'true' ]; then + if [ "${GITHUB_EVENT_INPUTS_DRY_RUN}" == 'true' ]; then echo "# DRY RUN" else - git push origin ${GITHUB_EVENT_INPUTS_TARGET_BRANCH} --tags + git remote set-url origin "https://x-access-token:${ACCESS_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" + git push origin "${GITHUB_EVENT_INPUTS_TARGET_BRANCH}" --tags fi env: + ACCESS_TOKEN: ${{ secrets.ACCESS_TOKEN }} GITHUB_EVENT_INPUTS_DRY_RUN: ${{ github.event.inputs.dry_run }} GITHUB_EVENT_INPUTS_TARGET_BRANCH: ${{ github.event.inputs.target_branch }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bump-version.yml around lines 52 - 57, The workflow currently sets actions/checkout@v6.0.0 with persist-credentials: false which prevents the PAT being available for the later git push; add an explicit authentication step before the final git push (or change persist-credentials to true) so the push will succeed. Concretely, either set persist-credentials: true on the actions/checkout invocation or add a step prior to the git push that configures credentials (for example, set the remote URL to include the token or run git config/credential helper with the token from secrets.ACCESS_TOKEN) so the git push command can authenticate; update the job that performs the push (the step invoking git push) to run after this authentication step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/semantic-pr-check.yml:
- Line 4: The workflow suppresses the dangerous-triggers audit for
pull_request_target but uses a mutable action tag; update the action reference
amannn/action-semantic-pull-request@v6 to a specific commit SHA (full 40-char
hash) so the workflow is pinned before keeping the suppression for
pull_request_target; locate the action usage string
"amannn/action-semantic-pull-request@v6" and replace the tag with the exact
commit SHA for that release.
In @.github/zizmor.yml:
- Around line 1-3: Replace the global suppression rules.unpinned-uses.disable:
true with a scoped ignore list so the unpinned-uses check remains active
everywhere except trusted workflows; modify the YAML key
rules.unpinned-uses.disable to rules.unpinned-uses.ignore and set it to an array
of specific workflows/locations (e.g., ["ci.yml:100", "tests.yml"]) to suppress
only those findings, leaving the audit enabled for all other workflows.
---
Outside diff comments:
In @.github/workflows/bump-version.yml:
- Around line 52-57: The workflow currently sets actions/checkout@v6.0.0 with
persist-credentials: false which prevents the PAT being available for the later
git push; add an explicit authentication step before the final git push (or
change persist-credentials to true) so the push will succeed. Concretely, either
set persist-credentials: true on the actions/checkout invocation or add a step
prior to the git push that configures credentials (for example, set the remote
URL to include the token or run git config/credential helper with the token from
secrets.ACCESS_TOKEN) so the git push command can authenticate; update the job
that performs the push (the step invoking git push) to run after this
authentication step.
In @.pre-commit-config.yaml:
- Around line 82-89: Add a CI workflow step that runs the existing zizmor
pre-commit hook in CI and emits SARIF so findings appear in the GitHub Security
tab: run the zizmor hook (the hook id "zizmor" from the .pre-commit-config
entry) in the workflow instead of relying only on local pre-commit, invoke it
with the --format=sarif argument (replace or extend the current args array that
contains --persona=regular), save the generated SARIF artifact, and add the
github/codeql-action/upload-sarif step to upload that SARIF file; ensure the
workflow step names and paths reference the same hook id "zizmor" and produce a
deterministic SARIF filename for upload.
🪄 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: 44cb0253-1189-4b27-a811-61998cc7f356
📒 Files selected for processing (17)
.github/dependabot.yml.github/workflows/bump-version.yml.github/workflows/ci-windows.yml.github/workflows/ci.yml.github/workflows/codeql-analysis.yml.github/workflows/dependencies-head.yml.github/workflows/docker.yml.github/workflows/docs.yml.github/workflows/lint.yml.github/workflows/lower-bound-requirements.yml.github/workflows/merged.yml.github/workflows/notebooks.yml.github/workflows/publish-package.yml.github/workflows/release_tests.yml.github/workflows/semantic-pr-check.yml.github/zizmor.yml.pre-commit-config.yaml
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request_target: # zizmor: ignore[dangerous-triggers] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Finding pull_request_target workflows and non-SHA-pinned actions..."
for wf in $(fd -e yml -e yaml .github/workflows); do
if rg -n '^\s*pull_request_target\s*:' "$wf" >/dev/null; then
echo "== $wf =="
rg -n '^\s*uses:\s+\S+@' "$wf" || true
echo "-- non SHA-pinned uses --"
rg -nP '^\s*uses:\s+\S+@(?![0-9a-f]{40}\b)\S+' "$wf" || true
fi
doneRepository: scikit-hep/pyhf
Length of output: 519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Correctly search .github/workflows directory
echo "=== Checking .github/workflows directory ==="
fd . '.github/workflows' -e yml -e yaml
echo -e "\n=== Reading semantic-pr-check.yml ==="
cat -n '.github/workflows/semantic-pr-check.yml' 2>/dev/null || echo "File not found"
echo -e "\n=== Checking for actions with non-SHA pins in pull_request_target workflows ==="
for wf in $(fd . '.github/workflows' -e yml -e yaml); do
if rg -q '^\s*pull_request_target\s*:' "$wf"; then
echo "== Workflow: $wf =="
rg -A 50 '^\s*pull_request_target\s*:' "$wf" | head -60
echo ""
fi
doneRepository: scikit-hep/pyhf
Length of output: 2494
Pin action to commit SHA before suppressing dangerous-triggers warning.
Line 4 suppresses the dangerous-triggers audit on pull_request_target, but the workflow runs with an unpinned action tag (amannn/action-semantic-pull-request@v6 on line 28). Replace the mutable tag with a full commit SHA to mitigate supply-chain risk, then the suppression is justified.
🔧 Suggested changes
- pull_request_target: # zizmor: ignore[dangerous-triggers]
+ pull_request_target:- uses: amannn/action-semantic-pull-request@v6
+ uses: amannn/action-semantic-pull-request@<FULL_40_CHAR_COMMIT_SHA>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/semantic-pr-check.yml at line 4, The workflow suppresses
the dangerous-triggers audit for pull_request_target but uses a mutable action
tag; update the action reference amannn/action-semantic-pull-request@v6 to a
specific commit SHA (full 40-char hash) so the workflow is pinned before keeping
the suppression for pull_request_target; locate the action usage string
"amannn/action-semantic-pull-request@v6" and replace the tag with the exact
commit SHA for that release.
|
cc @jarrodmillman @drammock given our recent discussions, for examples of what applying zizmor looks like. |
3 participants
Description
Resolves #2685
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees:
Summary by CodeRabbit