Skip to content

[security] Apply zizmor template-injection suggested fixes to workflow files - #8275

Merged
ScottTodd merged 13 commits into
mainfrom
users/scotttodd/zizmor-workflow-fixes
Sep 18, 2026
Merged

ScottTodd merged 13 commits into
mainfrom
users/scotttodd/zizmor-workflow-fixes

Conversation

@ScottTodd

@ScottTodd ScottTodd commented Sep 16, 2026 •

Copy link
Copy Markdown
Member

Motivation

This resolves all template-injection findings reported by https://docs.zizmor.sh/audits/#template-injection by applying the suggested remediation except in a few situations where a # zizmor: ignore[template-injection] exception is warranted.

Technical Details

GitHub's template expansions ${{ ... }} can result in unintended shell injection with user-controlled inputs due to how they evaluate expressions. While many of our uses are not user-controlled inputs, hardening all sites by routing through environment variables helps keep our workflows safer by default.

  • ~350 template-injection findings are fixed here
  • 20 findings were suppressed
  • ~275 findings from other rules remain across our workflow files

I made a few style choices while working through these findings/fixes:

  • env: at the job level or step level: move up to job level for ${{ inputs.* }} (especially if others are there already)
  • Use simple names where it makes sense, qualify further as needed like EVENT_SCHEDULE: ${{ github.event.schedule }}

Test Plan

  • CI workflows on this PR should pass
  • Manually trigger a dev release and check that workflows work as expected: https://github.com/ROCm/TheRock/actions/runs/35160568024 ("build python" fixed after triggering that)
  • Watch other scheduled and on-demand workflows after merge for issues
  • Catch backslide/regressions in other edits to workflow files after merge as part of enforcing zizmor on PRs

Submission Checklist

Pass dynamic maintenance inputs and outputs through environment variables to keep their contents out of generated shell source.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Pass build inputs and CI summaries through environment variables. Preserve intentional executable argument fragments with documented zizmor exceptions, and forward required values into WSL and Docker.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Use shared job environment values and quoted arguments for ROCm packaging, publication, and tests. Document the intentional component test-script exception.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Pass PyTorch inputs and outputs through quoted shell variables, including delayed expansion for CMD steps. Preserve trusted optional argument fragments with documented zizmor exceptions.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Use shared job environment values for JAX inputs and explicitly forward the checkout commit into Docker. Preserve supplied and computed package-version paths.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Document local zizmor checks, safe Python argument passing, and the trust assumptions required for template-injection exceptions.

Validation: pre-commit checks passed on the complete change set; offline zizmor reports no unsuppressed template-injection findings. Full CI remains pending.

Generated with Codex
Preserve the new PyTorch test_level input and pass it through TEST_LEVEL in the Linux and Windows test-matrix configuration jobs.

Validation: merge-conflict pre-commit checks and actionlint passed; offline zizmor reports no unsuppressed template-injection findings.

Generated with Codex
Use quoted percent expansion for controlled package paths in sanity and version-reporting steps. Explain why the two build steps retain delayed expansion.

Validation: actionlint, whitespace checks, and the focused zizmor scan passed.

Generated with Codex
@therock-pr-bot

therock-pr-bot Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

Pass the fetch output root through an environment variable so container path translation stays consistent with artifact and package directories. Use the same naming in Windows packaging and artifact structure checks.

Validated with actionlint and git diff --check.

Generated with Codex
Preserve external repository configuration and workflow environment aliases. Pass new compiler ASAN run URLs through environment variables to avoid template injection.

Validation: actionlint passed; zizmor reports zero template-injection findings.

Generated with Codex
@ScottTodd
ScottTodd marked this pull request as ready for review September 17, 2026 22:11
@ScottTodd
ScottTodd requested a review from cgoea September 17, 2026 22:12
@ScottTodd

Copy link
Copy Markdown
Member Author

This is likely to have merge conflicts the longer the review stays open, and several new issues have already slipped through since I last synced. I'm open to splitting this into a sequence of smaller PRs (see the individual commits) if it helps with review. There will be similar PRs for other scanner findings (zizmor and others).

@nunnikri nunnikri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and ci is passing

Comment thread .github/workflows/bump_submodules.yml Outdated
- name: Run scheduled bump automation
if: github.event_name == 'schedule' && steps.schedule.outputs.submodules != ''
env:
SUBMODULES: ${{ steps.schedule.outputs.submodules }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue more of a readability issue.
Similar names ENV variables. SUBMODULE and SUBMODULES

Also reuse of same variable SYSTEMS_TOKEN and LIBRARIES_TOKEN in the same file. Again not a bug, but good to use different variables.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to INPUT_SUBMODULE and SCHEDULE_SUBMODULES here and also adjusted

  • TEST_CONFIGS: ${{ inputs.test_configs }}
  • TEST_CONFIG: ${{ matrix.test_config }}

to

  • INPUT_TEST_CONFIGS: ${{ inputs.test_configs }}
  • MATRIX_TEST_CONFIG: ${{ matrix.test_config }}

As for reusing SYSTEMS_TOKEN in the same file, that happens in a few places now where we use the output of a prior step. We can't share that across steps without splitting across jobs or exporting through $GITHUB_ENV :/

Preserve updated requirements installation and reproduction arguments while keeping values in environment variables. Actionlint passed; seven template-injection findings remain in the new upstream code coverage workflow.

Generated with Codex
Rename SUBMODULE to INPUT_SUBMODULE and SUBMODULES to SCHEDULE_SUBMODULES so their sources and uses are clear.

Generated with Codex
Use INPUT_TEST_CONFIGS for the input list and MATRIX_TEST_CONFIG for each selected configuration. Pass the selected value through the test runner command-line option.

Generated with Codex

@jayhawk-commits jayhawk-commits left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ScottTodd
ScottTodd merged commit c70678e into main Sep 18, 2026
45 of 46 checks passed
@ScottTodd
ScottTodd deleted the users/scotttodd/zizmor-workflow-fixes branch September 18, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants