fix(ci): resolve all zizmor findings and add zizmor pre-commit checks#1181
Open
gforsyth wants to merge 10 commits intoNVIDIA:mainfrom
Open
fix(ci): resolve all zizmor findings and add zizmor pre-commit checks#1181gforsyth wants to merge 10 commits intoNVIDIA:mainfrom
gforsyth wants to merge 10 commits intoNVIDIA:mainfrom
Conversation
This reverts commit 160e868.
gforsyth
added
non-breaking
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
/ok to test |
Contributor
Author
|
/ok to test |
33 tasks
📝 WalkthroughWalkthroughGitHub Actions workflows are updated with explicit permission blocks and action version pinning to specific commit hashes, replacing floating tags and broad secrets inheritance with granular, least-privilege access controls. Two new configuration files define and enforce action-pinning policies via the Zizmor tool and pre-commit integration. ChangesWorkflow Permissions & Action Pinning
Security Policy & Tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/self_hosted_service_test.yaml (1)
97-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
SCRIPTbefore executing it.Line 99 executes a caller-provided path directly. Add a strict allowlist and file-existence guard so only expected repo scripts run.
🔧 Suggested hardening
- name: Run tests env: SCRIPT: ${{ inputs.script }} - run: bash "$SCRIPT" + run: | + [[ "$SCRIPT" =~ ^ci/[A-Za-z0-9._/-]+\.sh$ ]] || { echo "Invalid script path: $SCRIPT"; exit 1; } + [[ -f "$SCRIPT" ]] || { echo "Script not found: $SCRIPT"; exit 1; } + bash "$SCRIPT"As per coding guidelines, in
.github/workflows/**anyrun:step invokingci/*.sh,python ci/*.py, or a repo-local binary must reference a file present in the PR or the base branch.🤖 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/self_hosted_service_test.yaml around lines 97 - 100, The workflow currently executes a caller-provided path via env SCRIPT and run: bash "$SCRIPT" which is unsafe; change the step to validate SCRIPT against a strict allowlist of permitted repo-local entries (e.g., explicit names matching ci/*.sh, python ci/*.py, or known repo binaries) and add a file-existence guard that fails the job if the resolved path is not present in the PR or base branch before running bash "$SCRIPT"; implement this by checking the env variable SCRIPT in the workflow (or a preceding run step) to ensure it matches allowed patterns and that the file exists in the repository, and reject any values that do not match the allowlist or are missing..github/workflows/cloud_ci.yaml (1)
6-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a literal branch filter or remove the
branchesstanza.GitHub does not evaluate shell parameter expansion in workflow triggers. The syntax
${GITHUB_REF##*/}is treated as a literal branch name, preventing this workflow from triggering on any normal push. If the intent is "trigger on any branch withcloud-scriptschanges", remove thebranches:block; otherwise, list explicit branch globs.Suggested fix
on: push: - branches: - - ${GITHUB_REF##*/} paths: - 'cloud-scripts'🤖 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/cloud_ci.yaml around lines 6 - 10, The push trigger currently uses a shell-expansion-looking branch value `${GITHUB_REF##*/}` which GitHub Actions treats as a literal branch name, so update the workflow trigger under the push stanza: either remove the entire branches: block to trigger on pushes to any branch matching the paths rule (paths: - 'cloud-scripts'), or replace the branches: entry with explicit branch names or globs (e.g., main, release/*) instead of `${GITHUB_REF##*/}`; ensure you edit the push -> branches -> entry that contains `${GITHUB_REF##*/}` so the workflow actually triggers when cloud-scripts files change..github/workflows/build_images.yaml (1)
70-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the
buildersDocker context as the Buildx endpoint, not a filesystem path.The
endpointparameter indocker/setup-buildx-actionexpects either a Docker socket address or a Docker context name fromdocker context ls. Using./ci/docker/context(a filesystem directory path) will fail because Buildx cannot connect to it as a Docker endpoint. Thebuilderscontext created in the previous step should be used instead.Suggested fix
- name: Set up Docker Buildx uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3.12.0 with: driver: docker - endpoint: ./ci/docker/context + endpoint: builders🤖 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/build_images.yaml around lines 70 - 78, The workflow currently creates a Docker context named "builders" (step id buildx-context) but then passes a filesystem path to docker/setup-buildx-action as the endpoint; change the action input for docker/setup-buildx-action (the endpoint parameter) to use the "builders" Docker context name instead of "./ci/docker/context" so Buildx connects to the created context (i.e., set endpoint to "builders" for the docker/setup-buildx-action step).
🤖 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/pr.yaml:
- Around line 77-80: The workflow is incorrectly reading the job output as
steps.check-lean-ci.outputs.lean_ci_enabled; update the env assignment to use
the job outputs via needs.check-lean-ci.outputs.lean_ci_enabled (i.e., set
LEAN_CI from needs.check-lean-ci.outputs.lean_ci_enabled) and similarly update
the other occurrence at the later block (the lines around the second env/run
block), so both jobs reference needs.check-lean-ci.outputs.lean_ci_enabled
instead of steps.check-lean-ci.outputs.lean_ci_enabled.
---
Outside diff comments:
In @.github/workflows/build_images.yaml:
- Around line 70-78: The workflow currently creates a Docker context named
"builders" (step id buildx-context) but then passes a filesystem path to
docker/setup-buildx-action as the endpoint; change the action input for
docker/setup-buildx-action (the endpoint parameter) to use the "builders" Docker
context name instead of "./ci/docker/context" so Buildx connects to the created
context (i.e., set endpoint to "builders" for the docker/setup-buildx-action
step).
In @.github/workflows/cloud_ci.yaml:
- Around line 6-10: The push trigger currently uses a shell-expansion-looking
branch value `${GITHUB_REF##*/}` which GitHub Actions treats as a literal branch
name, so update the workflow trigger under the push stanza: either remove the
entire branches: block to trigger on pushes to any branch matching the paths
rule (paths: - 'cloud-scripts'), or replace the branches: entry with explicit
branch names or globs (e.g., main, release/*) instead of `${GITHUB_REF##*/}`;
ensure you edit the push -> branches -> entry that contains `${GITHUB_REF##*/}`
so the workflow actually triggers when cloud-scripts files change.
In @.github/workflows/self_hosted_service_test.yaml:
- Around line 97-100: The workflow currently executes a caller-provided path via
env SCRIPT and run: bash "$SCRIPT" which is unsafe; change the step to validate
SCRIPT against a strict allowlist of permitted repo-local entries (e.g.,
explicit names matching ci/*.sh, python ci/*.py, or known repo binaries) and add
a file-existence guard that fails the job if the resolved path is not present in
the PR or base branch before running bash "$SCRIPT"; implement this by checking
the env variable SCRIPT in the workflow (or a preceding run step) to ensure it
matches allowed patterns and that the file exists in the repository, and reject
any values that do not match the allowlist or are missing.
🪄 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: Enterprise
Run ID: f42f0ddf-604e-4509-8d47-fc7cdd562ce7
📒 Files selected for processing (15)
.github/workflows/build.yaml.github/workflows/build_images.yaml.github/workflows/build_test_publish_images.yaml.github/workflows/cloud_ci.yaml.github/workflows/inactivity_reminder.yaml.github/workflows/issue_automation.yaml.github/workflows/nightly-summary.yaml.github/workflows/nightly.yaml.github/workflows/pr.yaml.github/workflows/self_hosted_service_test.yaml.github/workflows/test.yaml.github/workflows/test_images.yaml.github/workflows/trigger-breaking-change-alert.yaml.github/zizmor.yml.pre-commit-config.yaml
Comment on lines
+77
to
+80
| env: | ||
| LEAN_CI: ${{ steps.check-lean-ci.outputs.lean_ci_enabled }} | ||
| run: | | ||
| if [ "${{ needs.check-lean-ci.outputs.lean_ci_enabled }}" == "true" ]; then | ||
| if [ "$LEAN_CI" == "true" ]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/pr.yamlRepository: NVIDIA/cuopt
Length of output: 29587
Use needs.check-lean-ci.outputs.lean_ci_enabled, not steps.check-lean-ci.outputs.lean_ci_enabled.
Both jobs have needs: check-lean-ci but incorrectly reference the output as a step within their own context. In GitHub Actions, outputs from dependent jobs must be accessed through the needs context, not steps. As written, both $LEAN_CI variables are undefined, causing the checks to silently fail.
Suggested fixes
- name: Check lean-ci status
env:
- LEAN_CI: ${{ steps.check-lean-ci.outputs.lean_ci_enabled }}
+ LEAN_CI: ${{ needs.check-lean-ci.outputs.lean_ci_enabled }}
run: |
if [ "$LEAN_CI" == "true" ]; then - name: Set matrix filters
id: set-filters
env:
- LEAN_CI: ${{ steps.check-lean-ci.outputs.lean_ci_enabled }}
+ LEAN_CI: ${{ needs.check-lean-ci.outputs.lean_ci_enabled }}
run: |
if [ "$LEAN_CI" == "true" ]; thenAlso applies to: 105-108
🧰 Tools
🪛 actionlint (1.7.12)
[error] 78-78: property "check-lean-ci" is not defined in object type {}
(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/pr.yaml around lines 77 - 80, The workflow is incorrectly
reading the job output as steps.check-lean-ci.outputs.lean_ci_enabled; update
the env assignment to use the job outputs via
needs.check-lean-ci.outputs.lean_ci_enabled (i.e., set LEAN_CI from
needs.check-lean-ci.outputs.lean_ci_enabled) and similarly update the other
occurrence at the later block (the lines around the second env/run block), so
both jobs reference needs.check-lean-ci.outputs.lean_ci_enabled instead of
steps.check-lean-ci.outputs.lean_ci_enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similar to upstream changes in
shared-workflows, this PR cleans up and annotates all of the workflows and adds thezizmorlinter to make sure changes are checked.Part of rapidsai/build-planning#275