GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — pre-commit: Add zizmor by cclauss · Pull Request #465 · cpplint/cpplint 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9f9ca41-ec40-436f-8672-cd7e2547244c
.github/dependabot.yml.pre-commit-config.yaml
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| - repo: https://github.com/zizmorcore/zizmor-pre-commit | ||
| rev: v1.29.0 | ||
| hooks: | ||
| - id: zizmor |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🏁 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 fi
Repository: 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:
🏁 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))}") PY
Repository: 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
.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/[email protected] + # 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.
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.
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd46d639-79a4-495b-9765-f26f717f3f0d
.github/workflows/ci.yml.github/workflows/release.yml
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| excessive-permissions: | ||
| disable: true |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🏁 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 -200
Repository: 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:
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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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—
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| excessive-permissions: | ||
| disable: true |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
—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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
GitHub Actions Session (Maintainer Summit Re-cap)https://github.com/community/maintainers/discussions/827#discussioncomment-17983486 Immediate Actions hardening advice shared during the session
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
When applied to things that don't prevent more security issues, incrementalism has these problems:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
71677398
Deployed
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.