ci: resolve a few zizmor findings#204
Conversation
📝 WalkthroughWalkthroughRemoves ChangesCI Security Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 |
![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
🤖 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/CI.yml:
- Line 67: Remove the `if: "${{ !matrix.disable-gil }}"` condition line from the
workflow step. The `disable-gil` key is not defined in the matrix object
declaration, making this condition invalid and causing workflow validation to
fail. Simply delete this entire line to resolve the issue.
- Line 61: The rust-cache action at line 61 (and also at lines 74, 115, and 121)
currently allows cache saves on all pull_request runs, which is a security risk.
Add a `save-if` conditional to each of these cache steps that restricts cache
saves to trusted push events only. Use `if: github.event_name == 'push'` as the
save condition while keeping the restore functionality available for all events.
This follows the pattern of using the restore-only behavior on PRs and splitting
to a separate cache step with explicit event gating for saves.
🪄 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: CHILL
Plan: Pro
Run ID: 5a4478a5-0780-46b3-9bc1-73811dd79f51
📒 Files selected for processing (3)
.github/dependabot.yml.github/workflows/CI.ymlrenovate.json
💤 Files with no reviewable changes (1)
- renovate.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/dependabot.yml
| - id: cache-rust | ||
| name: cache rust | ||
| uses: Swatinem/rust-cache@v2 | ||
| uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CI.yml" -o -name "ci.yml" | head -20Repository: dimastbk/python-calamine
Length of output: 95
🏁 Script executed:
cat -n .github/workflows/CI.yml | sed -n '50,130p'Repository: dimastbk/python-calamine
Length of output: 2995
🏁 Script executed:
cat -n .github/workflows/CI.yml | head -50Repository: dimastbk/python-calamine
Length of output: 1656
🏁 Script executed:
cat -n .github/workflows/CI.yml | sed -n '1,100p' | grep -A 20 "^.*on:"Repository: dimastbk/python-calamine
Length of output: 2752
🌐 Web query:
Swatinem/rust-cache v2.9.1 default behavior pull_request save behavior
💡 Result:
In Swatinem/rust-cache v2.9.1, the default behavior for saving the cache is to always save it, regardless of whether the workflow run is triggered by a pull request or a push [1][2]. The action provides an input option called save-if that controls this behavior [1]. By default, save-if is set to "true" [1][3]. When this default is active, the action will restore the cache (if available) and then save the cache at the end of the job run [1][4]. If you wish to change this behavior—for example, to prevent pull requests from overwriting the main branch's cache or to only save cache when running on specific branches—you can explicitly configure the save-if input [1][3]. A common use case is to restrict saves to the default branch using a GitHub Actions expression [1]: save-if: ${{ github.ref == 'refs/heads/main' }} It is important to note that GitHub Actions has its own native caching scope restrictions: caches created in a pull request are generally only accessible to that specific pull request or the base branch from which it originated, meaning they cannot "pollute" or overwrite the primary cache of other branches unless specifically managed [5]. If you notice that cache is not being "found" in a PR, it is often due to these native GitHub cache scoping rules rather than the save-if configuration [5].
Citations:
- 1: https://github.com/Swatinem/rust-cache/blob/master/README.md
- 2: https://github.com/Swatinem/rust-cache/tree/refs/tags/v2.9.1
- 3: https://github.com/marketplace/actions/rust-cache
- 4: https://github.com/Swatinem/rust-cache/tree/refs/tags/v2
- 5: "No cache found." on first push in PR, subsequent pushes are cached. Swatinem/rust-cache#221
🌐 Web query:
actions/cache v5.0.5 GitHub Actions pull_request write behavior default
💡 Result:
In GitHub Actions, the actions/cache@v5.0.5 action does not rely on the permissions block defined in your workflow file to determine its ability to write to the GitHub Actions cache [1][2]. Instead, cache operations are managed by GitHub's internal runner-scoped tokens, which automatically grant write access for workflows running on pull_request events within the repository [3][2]. Key behaviors regarding pull_request and cache write access include: 1. Automatic Write Capability: By default, jobs triggered by pull_request events have permission to create and write to caches within the scope of that specific pull request [3][4]. This allows workflows to "warm" a cache during a PR run. 2. Scope Isolation: Caches created during a pull_request run are restricted to that specific PR (specifically, the merge ref, refs/pull/.../merge) [3][4]. They cannot be restored by other pull requests or by the base branch, preventing unauthorized cache poisoning across different branches [3][4]. 3. Security Considerations: Because cache write operations bypass standard permissions: contents: read limitations, they can pose a security risk if a workflow runs in an untrusted context (e.g., pull_request_target or workflows that check out external PR head code) [1][2]. A malicious pull request could potentially "poison" the cache, which might then be restored by a subsequent trusted run on the main branch [1][2]. To mitigate these security risks, it is recommended to restrict cache writes for untrusted PR builds [1][2]. You can achieve this by using the actions/cache/restore@v5 sub-action instead of the full actions/cache@v5 action, or by adding a conditional check to ensure that the cache save step is skipped for pull_request events [1][2]: if: github.event_name!= 'pull_request' && github.event_name!= 'pull_request_target' uses: actions/cache@v5 with:...
Citations:
- 1: alandtse/open-shaders@8bb775b
- 2: https://saschb2b.de/skills/audit-actions
- 3: https://docs.github.com/en/actions/reference/workflows-and-actions/dependency-caching
- 4: Same cache key not shared between different pull requests actions/cache#79
Restrict cache saves to trusted push events.
These cache steps currently enable save behavior on all pull_request runs. While GitHub's cache scoping prevents PR caches from overwriting the main branch cache, explicitly gating cache saves to trusted events (push) is a security best practice. Gate cache save using save-if conditionals, and allow restore for all events.
Mitigation pattern
- - id: cache-rust
- name: cache rust
- uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1
+ - id: cache-rust
+ name: cache rust
+ uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1
with:
key: v3
+ save-if: ${{ github.event_name == 'push' }}For actions/cache, use the restore-only action on PRs and split to separate save step with event gating on trusted pushes.
Applies to lines: 61, 74, 115, 121
🧰 Tools
🪛 zizmor (1.25.2)
[error] 61-61: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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/CI.yml at line 61, The rust-cache action at line 61 (and
also at lines 74, 115, and 121) currently allows cache saves on all pull_request
runs, which is a security risk. Add a `save-if` conditional to each of these
cache steps that restricts cache saves to trusted push events only. Use `if:
github.event_name == 'push'` as the save condition while keeping the restore
functionality available for all events. This follows the pattern of using the
restore-only behavior on PRs and splitting to a separate cache step with
explicit event gating for saves.
Source: Linters/SAST tools
| - name: set up python | ||
| uses: actions/setup-python@v6 | ||
| uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 | ||
| if: "${{ !matrix.disable-gil }}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/CI.yml | head -80Repository: dimastbk/python-calamine
Length of output: 2789
🏁 Script executed:
# Check if disable-gil appears anywhere else in the workflow
rg "disable-gil" .github/workflows/CI.yml
# Also check the full context around line 67 to see if there's more matrix definition
cat -n .github/workflows/CI.yml | sed -n '23,42p'Repository: dimastbk/python-calamine
Length of output: 865
🏁 Script executed:
# Check all jobs in the workflow to see if any other job defines disable-gil
rg -A 20 "jobs:" .github/workflows/CI.yml | head -100
# Also check the entire workflow for all matrix definitions
rg "matrix:" .github/workflows/CI.yml -A 15Repository: dimastbk/python-calamine
Length of output: 2130
🏁 Script executed:
# Get the entire CI.yml file to see all jobs and confirm disable-gil is not defined anywhere
wc -l .github/workflows/CI.yml
# Let's see the full file structure to be thorough
cat -n .github/workflows/CI.yml | tail -100Repository: dimastbk/python-calamine
Length of output: 4169
Remove the undefined matrix key guard at Line 67.
matrix.disable-gil is not part of the declared matrix object, so this condition is invalid and will fail workflow validation. Remove the if statement since it references a non-existent matrix variable.
Proposed fix
- name: set up python
uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0
- if: "${{ !matrix.disable-gil }}"
with:
python-version: ${{ matrix.python-version }}
allow-prereleases: true📝 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.
| if: "${{ !matrix.disable-gil }}" | |
| - name: set up python | |
| uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 | |
| with: | |
| python-version: ${{ matrix.python-version }} | |
| allow-prereleases: true |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 67-67: property "disable-gil" is not defined in object type {pandas-version: string; python-version: string}
(expression)
🤖 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/CI.yml at line 67, Remove the `if: "${{
!matrix.disable-gil }}"` condition line from the workflow step. The
`disable-gil` key is not defined in the matrix object declaration, making this
condition invalid and causing workflow validation to fail. Simply delete this
entire line to resolve the issue.
Source: Linters/SAST tools
1 participant
Summary by CodeRabbit
Release Notes