Skip to content

Fix: Env-mediate expressions in run blocks - #135

Merged
tykeal merged 1 commit into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:fix/zizmor-template-injection
Jul 28, 2026
Merged

Fix: Env-mediate expressions in run blocks#135
tykeal merged 1 commit into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:fix/zizmor-template-injection

Conversation

@ModeSevenIndustrialSolutions

Copy link
Copy Markdown
Contributor

Why

The org-wide zizmor workflow (lfreleng-actions/.github/workflows/zizmor.yaml) was changed today:

2026-07-28T10:46  Feat: Surface all zizmor findings at any level

It runs --persona auditor --min-severity informational and now fails a run on any finding at any level. This repo carries 3 pre-existing template-injection findings, so every new PR here is blocked until they're cleared — currently #134.

What

.github/workflows/testing.yaml expanded action outputs directly inside a run: block:

run: |
  dynamic='${{ steps.testing.outputs.dynamic_version }}'

GitHub expands ${{ }} before the shell sees the script, so a crafted value becomes shell source text rather than data. Routing through env: makes the value opaque to the shell parser:

env:
  DYNAMIC_VERSION: ${{ steps.testing.outputs.dynamic_version }}
run: |
  dynamic="$DYNAMIC_VERSION"

Same shape python-workflows uses (see its "Fix: Env-mediate expressions in run blocks"), and the env:-before-run: ordering already used elsewhere here.

⚠️ Why this was written by hand

zizmor offers an auto-fix for this audit. It is marked unsafe, and on inspection it is — it rewrites the expression in place and leaves it inside the original single quotes:

- dynamic='${{ steps.testing.outputs.dynamic_version }}'
+ dynamic='${STEPS_TESTING_OUTPUTS_DYNAMIC_VERSION}'

The shell does not expand ${...} inside single quotes. The original was correct precisely because GitHub expanded the template before the shell ran; once the value moves to env:, expansion becomes the shell's job and the quoting has to change with it. Applied as-is, dynamic would hold the literal string ${STEPS_...} and the assertion below would fail.

All three edits here use double quotes and were verified by re-running the audit.

Validation

  • zizmor --persona auditor --min-severity informationalNo findings to report
  • pre-commit run --all-files → all hooks pass, including yamllint and actionlint
  • No behavioural change: the same values reach the same commands

Unblocks #134.

zizmor's auditor persona reported 3 template-injection findings in
testing.yaml: GitHub expressions expanded directly inside a 'run:'
block, where expansion happens before the shell sees the script and a
crafted value could inject commands.

Pass the three action outputs through 'env:' instead and read them as
shell variables in the validation step.

The org-wide zizmor workflow now fails a run on any finding at any
level, so these informational findings block every new pull request
in this repository.

zizmor offers an auto-fix for this audit, but it is marked unsafe and
on inspection it is: it rewrites the expression in place, leaving it
inside the original single quotes, where the shell will not expand it.
The original was correct because GitHub expanded the template before
the shell ever saw it.  These edits were therefore written by hand,
with double quotes, and follow the env-before-run shape already used
elsewhere in this repository.

No behavioural change: the same values reach the same commands.
Zizmor now reports no findings for the repository.

Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses GitHub Actions template-injection findings by avoiding direct ${{ ... }} expression expansion inside run: scripts and instead passing action outputs through env: variables, keeping shell parsing separate from expression evaluation.

Changes:

  • Move steps.testing.outputs.* references from inline shell-script assignments into env: for the “Validate action” step.
  • Update the bash script to read those values via $DYNAMIC_VERSION, $DYNAMIC_PROVIDER, and $DYNAMIC_SOURCE.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zxiiro zxiiro 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.

🤖 Auto-approved by agent: reviewed workflow/code change for security and CI/CD impact, found low risk. Mediates GitHub Actions expressions (step outputs) through env: vars instead of direct ${{ }} interpolation in shell run: steps, in test/CI workflow only; standard injection-hardening pattern, no new permissions or secrets.

@tykeal
tykeal merged commit bba34db into lfreleng-actions:main Jul 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants