pre-commit: Add zizmor - #465
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRepository automation now applies a seven-day Dependabot cooldown, runs the pinned ChangesRepository automation hardening
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🤖 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 @.pre-commit-config.yaml:
- Around line 77-80: Update the zizmor hook configuration at rev v1.29.0 to add
a local files override that includes .pre-commit-config.yaml in its audit scope,
while preserving the existing hook id and repository configuration.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9f9ca41-ec40-436f-8672-cd7e2547244c
📒 Files selected for processing (2)
.github/dependabot.yml.pre-commit-config.yaml
| - repo: https://github.com/zizmorcore/zizmor-pre-commit | ||
| rev: v1.29.0 | ||
| hooks: | ||
| - id: zizmor |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
manifest="$(mktemp)"
trap 'rm -f "$manifest"' EXIT
curl -fsSL \
'https://raw.githubusercontent.com/zizmorcore/zizmor-pre-commit/v1.29.0/.pre-commit-hooks.yaml' \
> "$manifest"
grep -n '^files:' "$manifest"
if grep -Eq '\.pre-commit-config\.ya?ml' "$manifest"; then
echo "The upstream hook now includes pre-commit configuration files."
exit 1
fiRepository: cpplint/cpplint
Length of output: 153
🏁 Script executed:
set -u
echo '--- local configuration ---'
sed -n '65,90p' .pre-commit-config.yaml
echo '--- upstream hook manifest ---'
curl -fsSL \
'https://raw.githubusercontent.com/zizmorcore/zizmor-pre-commit/v1.29.0/.pre-commit-hooks.yaml' \
| sed -n '1,120p'Repository: cpplint/cpplint
Length of output: 884
🌐 Web query:
zizmor v1.29.0 supported file types .pre-commit-config.yaml
💡 Result:
In zizmor v1.29.0, experimental support was added for auditing pre-commit inputs, specifically including.pre-commit-config.yaml files and hook definitions [1][2]. When running zizmor, it can automatically infer the input type—such as a workflow, action, Dependabot configuration, or a pre-commit configuration—when reading from standard input [3]. For local directories or repositories, zizmor identifies and collects these auditable inputs automatically, and users can explicitly target or filter these inputs using the --collect=pre-commit option [3]. It is important to distinguish between using zizmor to audit a.pre-commit-config.yaml file and using zizmor itself as a hook within a.pre-commit-config.yaml file. The former refers to zizmor's new ability to analyze the security of pre-commit configurations [1][2], while the latter is a well-established integration where zizmor is installed as a hook in a repository's.pre-commit-config.yaml to run security audits on GitHub Actions workflows [4][5].
Citations:
- 1: https://newreleases.io/project/github/zizmorcore/zizmor/release/v1.29.0
- 2: https://docs.zizmor.sh/release-notes/
- 3: https://docs.zizmor.sh/usage/
- 4: https://docs.zizmor.sh/integrations/
- 5: https://github.com/zizmorcore/zizmor-pre-commit
🏁 Script executed:
set -u
echo '--- v1.29.0 release note ---'
curl -fsSL 'https://raw.githubusercontent.com/zizmorcore/zizmor/v1.29.0/CHANGELOG.md' \
| grep -in -A8 -B4 'pre-commit' || true
echo '--- usage documentation references ---'
for url in \
'https://raw.githubusercontent.com/zizmorcore/zizmor/v1.29.0/docs/usage.md' \
'https://raw.githubusercontent.com/zizmorcore/zizmor/v1.29.0/docs/integrations.md'
do
echo "URL: $url"
curl -fsSL "$url" | grep -in -A5 -B5 'pre-commit' || true
done
echo '--- proposed file pattern behavior ---'
python3 - <<'PY'
import re
pattern = r'^(\.github/(workflows/.*|dependabot\.ya?ml)|(?:.*/)?action\.ya?ml|\.pre-commit-config\.ya?ml)$'
for path in [
".pre-commit-config.yaml",
".github/workflows/ci.yml",
".github/dependabot.yml",
"sub/action.yaml",
"action.yml",
".github/actions/action.yml",
"README.md",
]:
print(f"{path}: {bool(re.fullmatch(pattern, path))}")
PYRepository: cpplint/cpplint
Length of output: 2941
Include .pre-commit-config.yaml in the hook file scope.
The v1.29.0 hook manifest excludes .pre-commit-config.yaml, so pre-commit skips it. zizmor v1.29.0 supports auditing pre-commit configurations. Add a local files override if this PR must audit that configuration.
🤖 Prompt for 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.
In @.pre-commit-config.yaml around lines 77 - 80, Update the zizmor hook
configuration at rev v1.29.0 to add a local files override that includes
.pre-commit-config.yaml in its audit scope, while preserving the existing hook
id and repository configuration.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
117-118: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the commented TestPyPI publisher to a commit SHA.
Line 118 still uses
@v1.14.2. If the TODO step is uncommented, it will not match the immutable commit-SHA policy used by the active PyPI step at Line 52. Use the same commit SHA and retain the version comment.Proposed fix
- # uses: pypa/gh-action-pypi-publish@v1.14.2 + # uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2🤖 Prompt for 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. In @.github/workflows/release.yml around lines 117 - 118, Update the commented TestPyPI publisher step to pin its uses reference to the same commit SHA as the active PyPI publishing step, while retaining the existing v1.14.2 version comment.
🤖 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.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 117-118: Update the commented TestPyPI publisher step to pin its
uses reference to the same commit SHA as the active PyPI publishing step, while
retaining the existing v1.14.2 version comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd46d639-79a4-495b-9765-f26f717f3f0d
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/release.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@zizmor.yml`:
- Around line 2-3: Remove the excessive-permissions configuration block from
zizmor.yml so the audit remains enabled; do not disable the complete audit, and
use only a documented targeted ignore for any confirmed false positive.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| excessive-permissions: | ||
| disable: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- zizmor.yml ---'
cat -n zizmor.yml
printf '%s\n' '--- zizmor hook ---'
sed -n '65,90p' .pre-commit-config.yaml
printf '%s\n' '--- local zizmor references ---'
rg -n -i 'excessive-permissions|zizmor' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: cpplint/cpplint
Length of output: 687
🌐 Web query:
zizmor v1.29.0 configuration rules disable excessive-permissions audit documentation
💡 Result:
In zizmor v1.29.0, you can disable the excessive-permissions audit by modifying your zizmor configuration file (typically zizmor.yml) to set the disable property to true for that specific rule [1][2]. To disable the audit, add the following to your configuration file: rules: excessive-permissions: disable: true The zizmor documentation emphasizes that disabling audits should be used as a measure of last resort, as disabled rules will not appear in ignored or suppressed finding counts, which may lead to missing important future findings [1][2]. Before disabling the audit entirely, the developers recommend considering these alternatives [1][2]: 1. Ignoring specific findings using the rules..ignore configuration [1][2]. 2. Adjusting your persona (e.g., removing --persona=pedantic or --persona=auditor) if you are currently using a more sensitive persona [1][2]. Configuration files can be placed where zizmor discovers them automatically or passed explicitly using the --config flag or the ZIZMOR_CONFIG environment variable [3][4].
Citations:
- 1: https://docs.zizmor.sh/configuration/
- 2: https://github.com/zizmorcore/zizmor/blob/main/docs/configuration.md
- 3: https://docs.zizmor.sh/usage/
- 4: https://github.com/zizmorcore/zizmor/blob/main/docs/usage.md
Keep the excessive-permissions audit enabled.
disable: true disables the complete audit. Remove this block. If a finding is a confirmed false positive, use a documented, targeted ignore instead.
🤖 Prompt for 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.
In `@zizmor.yml` around lines 2 - 3, Remove the excessive-permissions
configuration block from zizmor.yml so the audit remains enabled; do not disable
the complete audit, and use only a documented targeted ignore for any confirmed
false positive.
aaronliu0130
left a comment
There was a problem hiding this comment.
Wonderful tool! pre-commit does seem like the right place to add this. I don't like the configuration we used though.
I feel like excessive permissions should definitely be checked for and resolved, while I"m unconvinced about pinning hashes: the latter seems like too much labor for little benefit. (Think Manjaro:) Always using an old version prevents auto-updating to a newer patched version in the same major release. Remember that we're not pinning to a specific release, but the same major version number. Since we're only using widely used actions, a security patch release within the same major version from either the authors or GitHub would always be present in the "version contains a supply-chain attack" threat model we're thinking of. Restricting permissions is a far easier and efficacious way of defending against actions maliciously doing more than they're supposed to—
| excessive-permissions: | ||
| disable: true |
There was a problem hiding this comment.
—so this is the only change I ask for.
| excessive-permissions: | |
| disable: true | |
| unpinned-uses: | |
| config: | |
| policies: | |
| "*": ref-pin |
It's not as precise as I'd want it to be (allow v6 but not v6.1.0), but it'll do.
GitHub Actions Session (Maintainer Summit Re-cap)https://github.com/community/maintainers/discussions/827#discussioncomment-17983486 Immediate Actions hardening advice shared during the session
|
|
Can I just say that you are incredibly slow at understanding the value of incrementalism. This steady forward progress is the superpower of collaborative open source development. If this pull request is solid forward progress then review it and merge it so we have guardrails in place. if you have confidence and experience in dealing with excessive permissions then fix the config after this pull request is merged. I have made regrettable errors trying to fix excessive permissions so I am reluctant to fix them in this pull request which does other useful things. Solving 95% of the problem and getting it merged is better than waiting an extra 2 weeks without any guardrails in place for a 100% solution. |
|
The link you gave was dead and I couldn't find any reference to it. You do make a very good argument that this makes the security better than before and thus theoretically could prevent a lot of attacks if merged sooner than later. I do have quite a bit more thoughts on incrementalism when applied to every other area, and I'll type them out, but I'll merge this now. I think this conversation is important to have, and I'm glad we'll have an opportunity to discuss it :D |
|
When applied to things that don't prevent more security issues, incrementalism has these problems:
|
https://docs.zizmor.sh --
zizmoris a static analysis tool that can find and fix security issues in common CI/CD setups, including GitHub Actions, Dependabot, and pre-commit.Summary by CodeRabbit