ci: use zizmor to lint for CI security issues - #721
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthrough
ChangesWorkflow Security Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 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 |
6cdb7e8 to
c3ed221
Compare
561ccfc to
ee6400f
Compare
5b6a738 to
cf9547e
Compare
|
zizmor failed in CI because we don't use dependabot cooldowns. i added a |
| @@ -0,0 +1,7 @@ | |||
| # zizmor configuration | |||
There was a problem hiding this comment.
we do not run dependabot consistently, and we also don't do merges of dependabot without proper review. Technically right now we are not merging any dependabot pr until we go over package updates with care and manual testing.
so to ask: could we resolve this finding in dependabot.yml instead of suppressing it here? Adding a cooldown directly means we fix the actual finding rather than mute it, and we can then delete this zizmor.yml entirely (no standing exception to maintain).
Since cooldown doesn't apply to security updates and we batch-review Dependabot PRs manually anyway, a short default is essentially free and even trims PR churn:
schedule:
interval: 'weekly'
cooldown:
default-days: 7
There was a problem hiding this comment.
Sure thing! That's done now.
c68c5dc to
99b2688
Compare
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 @.github/workflows/publish-docker.yaml:
- Around line 27-29: The concurrency group currently uses only github.ref which
causes manual dispatches with different inputs to collide; update the
concurrency group expression (the "group" key under concurrency) to include the
workflow_dispatch input (e.g., the "version" input) or alternatively detect
workflow_dispatch and disable cancel-in-progress for that event; locate the
concurrency block (group: publish-docker-${{ github.ref }} / cancel-in-progress:
true) and modify the group to incorporate the input (for example append -${{
github.event.inputs.version }}), or set cancel-in-progress to false when
github.event_name == 'workflow_dispatch'.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88d5b59b-081d-4cf3-b11a-06aceba0581e
📒 Files selected for processing (9)
.github/dependabot.yml.github/workflows/apply_pr_checks.yaml.github/workflows/check_if_directory_changed.yaml.github/workflows/checklist.yaml.github/workflows/mdbook.yml.github/workflows/publish-db-migrator.yaml.github/workflows/publish-docker.yaml.github/workflows/publish-types.yaml.github/workflows/zizmor.yml
✅ Files skipped from review due to trivial changes (1)
- .github/dependabot.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/checklist.yaml
- .github/workflows/publish-db-migrator.yaml
- .github/workflows/zizmor.yml
- .github/workflows/check_if_directory_changed.yaml
- .github/workflows/mdbook.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 @.github/workflows/publish-docker.yaml:
- Around line 27-29: The concurrency group currently uses only github.ref which
causes manual dispatches with different inputs to collide; update the
concurrency group expression (the "group" key under concurrency) to include the
workflow_dispatch input (e.g., the "version" input) or alternatively detect
workflow_dispatch and disable cancel-in-progress for that event; locate the
concurrency block (group: publish-docker-${{ github.ref }} / cancel-in-progress:
true) and modify the group to incorporate the input (for example append -${{
github.event.inputs.version }}), or set cancel-in-progress to false when
github.event_name == 'workflow_dispatch'.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88d5b59b-081d-4cf3-b11a-06aceba0581e
📒 Files selected for processing (9)
.github/dependabot.yml.github/workflows/apply_pr_checks.yaml.github/workflows/check_if_directory_changed.yaml.github/workflows/checklist.yaml.github/workflows/mdbook.yml.github/workflows/publish-db-migrator.yaml.github/workflows/publish-docker.yaml.github/workflows/publish-types.yaml.github/workflows/zizmor.yml
✅ Files skipped from review due to trivial changes (1)
- .github/dependabot.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/checklist.yaml
- .github/workflows/publish-db-migrator.yaml
- .github/workflows/zizmor.yml
- .github/workflows/check_if_directory_changed.yaml
- .github/workflows/mdbook.yml
🛑 Comments failed to post (1)
.github/workflows/publish-docker.yaml (1)
27-29:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
workflow_dispatchinputs in the concurrency key.Line 28 keys manual backfills only by
github.ref, so two dispatches frommainfor differentversionvalues will cancel each other mid-publish. That breaks the new backfill path the input was added for. Include the version input in the group, or disable cancellation forworkflow_dispatch.Suggested fix
concurrency: - group: publish-docker-${{ github.ref }} - cancel-in-progress: true + group: publish-docker-${{ github.ref }}-${{ github.event_name == 'workflow_dispatch' && github.event.inputs.version || github.run_id }} + cancel-in-progress: ${{ github.event_name != 'workflow_dispatch' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.concurrency: group: publish-docker-${{ github.ref }}-${{ github.event_name == 'workflow_dispatch' && github.event.inputs.version || github.run_id }} cancel-in-progress: ${{ github.event_name != 'workflow_dispatch' }}🤖 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/publish-docker.yaml around lines 27 - 29, The concurrency group currently uses only github.ref which causes manual dispatches with different inputs to collide; update the concurrency group expression (the "group" key under concurrency) to include the workflow_dispatch input (e.g., the "version" input) or alternatively detect workflow_dispatch and disable cancel-in-progress for that event; locate the concurrency block (group: publish-docker-${{ github.ref }} / cancel-in-progress: true) and modify the group to incorporate the input (for example append -${{ github.event.inputs.version }}), or set cancel-in-progress to false when github.event_name == 'workflow_dispatch'.
Fix zizmor findings across the workflows: - Move `github.base_ref`/`inputs.ref`/step-output expressions out of `run:` blocks into `env:` vars to avoid template injection. - Scope `checks: write` down from the workflow level to only the `check_api_server` job in apply_pr_checks.yaml. - Add `persist-credentials: false` to checkout steps. - Add a zizmor CI workflow to run the audit on push and PRs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zizmor's cache-poisoning auto-fix added `lookup-only: true` to the mdbook cache step. Because the install step was gated on `cache-hit != 'true'`, a cache hit then skipped both the download and the install, leaving mdbook-bin/ empty and breaking the build. The original win (PR roostorg#14 / issue roostorg#3) was switching from compiling mdbook from source to downloading the prebuilt release; the cache was only a marginal optimization on top. Removing it keeps that win, fixes the break, and resolves the cache-poisoning finding cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
99b2688 to
14d994c
Compare
* Harden GitHub Actions workflows per zizmor audit Fix zizmor findings across the workflows: - Move `github.base_ref`/`inputs.ref`/step-output expressions out of `run:` blocks into `env:` vars to avoid template injection. - Scope `checks: write` down from the workflow level to only the `check_api_server` job in apply_pr_checks.yaml. - Add `persist-credentials: false` to checkout steps. - Add a zizmor CI workflow to run the audit on push and PRs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * mdbook: drop broken cache step, install binary unconditionally zizmor's cache-poisoning auto-fix added `lookup-only: true` to the mdbook cache step. Because the install step was gated on `cache-hit != 'true'`, a cache hit then skipped both the download and the install, leaving mdbook-bin/ empty and breaking the build. The original win (PR #14 / issue #3) was switching from compiling mdbook from source to downloading the prebuilt release; the cache was only a marginal optimization on top. Removing it keeps that win, fixes the break, and resolves the cache-poisoning finding cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * address PR comments * ignore dependabot cooldowns in zizmor * add dependency cooldown to dependabot --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Context & Requests for Reviewers
This PR introduces zizmor, a static analysis tool for GitHub Actions.
I suggest we do this because it's an automated and low-friction way to ensure that our CI follows security best practices.
There are two parts to this:
zizmorlocally and fixed its findings. Most of these were automatic fixes, with the exception of:github.base_refintoenv:variables to avoid template injectionchecks:writepermission from theapply_pr_checksworkflow, and instead giving this permission to only thecheck_api_serverjob where it's actually needed.mdbookworkflow.Tests
Basically, ensure that CI continues to pass :-)
(Optional) Rollout Plan
There's a slight challenge with this, in that some workflows (mdbook and publishing packages/images) only run on
main. I can trigger these manually viaworkflow_dispatchonce they are merged tomain. But, they have side effects (like publishing a package!).So I think the best approach there is just to be ready for potential issues the next time we run these, so we can fix them quickly.
Summary by CodeRabbit