🧪 Integrate Zizmor checks into GHA CI/CD 🌈#16268
🧪 Integrate Zizmor checks into GHA CI/CD 🌈#16268webknjaz wants to merge 1 commit intoansible:develfrom
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
webknjaz
force-pushed
the
maintenance/gha-security-zizmor
branch
from
38a8fe3 to
7a16c63
Compare
|
Checks aren't working, try rebase And I looked at the overview: https://github.com/ansible/awx/security/code-scanning?query=pr%3A16268+is%3Aopen and correct me if I'm wrong, but it seems to be the same as the SonarCloud output? Is that the intent? We already see these regularly. @chrismeyersfsu started addressing some of the issues, but seems to be a manual process, which makes no sense. I'm fine to get a patch for each of the issues, approve+merge or reject. Because I see no reason that can't be automated.... but I guess I don't see what this does that helps? |
This was because of GitHub's outage yesterday. Rebased now to re-trigger.
No idea, that page is 404 for me as I don't have access to see it. FTR, I've never seen SonarCloud assessing security and common issues with setting up GHA, nor have I seen it being configurable. It's a black box that does something but it was never useful to me. Maybe @woodruffw knows of a better comparison.
This basically helps avoid using commonly known insecure practices in GH Actions+Workflows. And yes, it automates checking for typical mistakes. You aren't limited to seeing the reports in GH UI — it can be invoked locally, I just wanted to start w/ the integration that would surface new problems in PRs right away. It also has an auto-fixing mode where possible: https://docs.zizmor.sh/usage/#auto-fixing-results Here's the list of rules in currently checks with well-written explanation for each: https://docs.zizmor.sh/audits/ |
webknjaz
force-pushed
the
maintenance/gha-security-zizmor
branch
from
7a16c63 to
b1bd8d2
Compare
Yeah, I'm not aware of a commercial product that has a similar footprint. I know a few commercial tools have begun to integrate or use zizmor internally though, so I suppose it isn't out of the question that SonarCloud does that for zizmor. But I wouldn't know for certain 🙂 (The closest equivalents I'm aware of are GitHub's own CodeQL, plus poutine from Boost Security. But I feel somewhat confident in saying that zizmor has the best coverage + actionability defaults of the major tools.) |
|
I see, it does seem different. Zizmor gives 138 items, SonarCloud gives 1,146, and when I dive in, some are qualitatively reporting the same thing (like unpinned github action reference) but word differently. |
|
@AlanCoding they may be some overlap. However, I haven't seen SonarCloud explaining things well anyway. Plus I can't run it locally. I don't understand what's the problem with having a superrior tool integrated — it's non-blocking. |
webknjaz
force-pushed
the
maintenance/gha-security-zizmor
branch
from
b1bd8d2 to
9fb085e
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new GitHub Actions job Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
454-460: Consider addingtimeout-minutesto guard against runaway executions.Every other job in this workflow caps execution time (60 min, 30 min, or 10 min). The
zizmorjob has notimeout-minutes, so if the reusable workflow hangs, it defaults to GitHub's 6-hour maximum and blocks the PR for that duration.✏️ Proposed addition
zizmor: name: 🌈 zizmor + timeout-minutes: 10 permissions: security-events: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 454 - 460, The zizmor job lacks a timeout-minutes setting which can allow the reusable workflow (uses: zizmorcore/workflow/.github/workflows/reusable-zizmor.yml@...) to run up to GitHub's 6-hour default; add a timeout-minutes: 30 (or another appropriate cap like 10/60 consistent with other jobs) directly under the zizmor job definition so the job is aborted if it exceeds the expected runtime.
🤖 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/ci.yml:
- Around line 454-460: The job-level permissions for the zizmor job currently
only set security-events: write, which will leave contents disabled and break
steps (like actions/checkout) inside the reusable workflow; update the zizmor
job's permissions block to include contents: read (e.g., add contents: read
alongside security-events: write) so the reusable workflow can perform
repository checkout/reads when it invokes actions/checkout inside the referenced
reusable workflow.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 454-460: The zizmor job lacks a timeout-minutes setting which can
allow the reusable workflow (uses:
zizmorcore/workflow/.github/workflows/reusable-zizmor.yml@...) to run up to
GitHub's 6-hour default; add a timeout-minutes: 30 (or another appropriate cap
like 10/60 consistent with other jobs) directly under the zizmor job definition
so the job is aborted if it exceeds the expected runtime.
|
@webknjaz so does Zizmor only look at Github Actions? It found many issues in this PR, but it looks like only in the workflows folder. |
|
@pb82 yes, it's heavily focused on GHA which is mostly the workflows dir. Some of the checks it implements are based on GitHub's security section of their blog, while others are William's original ideas (I think there's one that's a result of collaboration w/ me), AFAIK. But I'd say that its checks reach beyond GHA while still in that category, for example: https://docs.zizmor.sh/audits/#impostor-commit. |
webknjaz
force-pushed
the
maintenance/gha-security-zizmor
branch
from
9fb085e to
1a6c4bc
Compare
|
Confirming everything @webknjaz said 🙂 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
454-460: Consider addingtimeout-minutesto bound the job.Every other job in this workflow sets an explicit
timeout-minutes. Reusable workflow calls default to 360 minutes without one; a security-scanning job like this should finish in under a minute.⏱️ Suggested addition
zizmor: name: 🌈 zizmor + timeout-minutes: 10 permissions: security-events: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 454 - 460, The zizmor job lacks an explicit timeout-minutes setting; add a timeout-minutes (e.g., 5) to the zizmor job definition so the reusable workflow call doesn't inherit the 360-minute default—place timeout-minutes directly under the zizmor job stanza near permissions/uses to bound the job runtime.
459-460: The pinned SHA1e20adb0862e932363a4d85d68c92e5cc6fcb5d4is legitimate and trustworthy. It's a recent commit (Feb 9, 2026) by Sviatoslav Sydorenko that updates dependencies, including the SARIF uploader to a non-deprecated API. No supply-chain concerns.Consider adding
timeout-minutes: 10to thezizmorjob; a security scan should complete quickly, and an explicit timeout prevents runaway jobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 459 - 460, The pinned reusable workflow SHA is fine; add an explicit job timeout to prevent runaway runs by adding timeout-minutes: 10 to the zizmor job definition in the CI workflow (the job with id/name "zizmor" that uses the reusable-zizmor.yml via the uses: zizmorcore/... line); ensure the timeout-minutes key is placed at the same indentation level as other top-level job fields (e.g., runs-on/uses/steps) so the runner enforces a 10-minute timeout for that job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 454-460: The zizmor job lacks an explicit timeout-minutes setting;
add a timeout-minutes (e.g., 5) to the zizmor job definition so the reusable
workflow call doesn't inherit the 360-minute default—place timeout-minutes
directly under the zizmor job stanza near permissions/uses to bound the job
runtime.
- Around line 459-460: The pinned reusable workflow SHA is fine; add an explicit
job timeout to prevent runaway runs by adding timeout-minutes: 10 to the zizmor
job definition in the CI workflow (the job with id/name "zizmor" that uses the
reusable-zizmor.yml via the uses: zizmorcore/... line); ensure the
timeout-minutes key is placed at the same indentation level as other top-level
job fields (e.g., runs-on/uses/steps) so the runner enforces a 10-minute timeout
for that job.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
.github/workflows/ci.yml
|
|
@pb82 @AlanCoding in case you're curious what cind of exploits Zizmor is capable of catching: https://blog.yossarian.net/2024/12/06/zizmor-ultralytics-injection. |
This linter guards against common insecure setups in GitHub Actions and Workflows. It is authored and maintained by a member of the PyPA, contributor to PyPI, former employee of the Trail Of Bits. Ref: https://zizmor.sh
webknjaz
force-pushed
the
maintenance/gha-security-zizmor
branch
from
1a6c4bc to
2b40ade
Compare
4 participants
This linter guards against common insecure setups in GitHub Actions and Workflows. It is authored and maintained by a member of the PyPA, contributor to PyPI, former employee of the Trail Of Bits.
Ref: https://zizmor.sh
Summary by CodeRabbit