Skip to content

workflows/ci: fix template-injection zizmor findings#195318

Merged
bevanjkay merged 2 commits into
masterfrom
zizmor-ci-template-injection
Dec 27, 2024
Merged

workflows/ci: fix template-injection zizmor findings#195318
bevanjkay merged 2 commits into
masterfrom
zizmor-ci-template-injection

Conversation

@samford

@samford samford commented Dec 15, 2024

Copy link
Copy Markdown
Member

Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.

In the following questions <cask> is the token of the cask you're submitting.

After making any changes to a cask, existing or new, verify:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • Checked the cask was not already refused (add your cask's name to the end of the search field).
  • brew audit --cask --new <cask> worked successfully.
  • HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask> worked successfully.
  • brew uninstall --cask <cask> worked successfully.

This updates workflows/ci.yml to use environment variables to
address a template-injection error and similar info output from zizmor.

I've added # shellcheck disable=SC2086 comments in a few places where shellcheck wanted quotes but the strings consist of space-separated packages:

ci.yml:194:9: shellcheck reported issue in this script: SC2086:info:1:26: Double quote to prevent globbing and word splitting [shellcheck]
    |
194 |         run: |
    |         ^~~~
ci.yml:202:9: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting [shellcheck]
    |
202 |         run: |
    |         ^~~~
ci.yml:240:9: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting [shellcheck]
    |
240 |         run: |
    |         ^~~~

Adding quotes in those instances would cause brew to interpret something like "one two three" as one package with that name instead of three packages. If there's a better way to handle this, let me know.

As with my other recent actions PRs, I'm not very knowledgeable about GitHub Actions, so I've created this as a draft until more knowledgeable maintainers have a chance to review this and catch any mistakes.

@samford samford left a comment

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.

Seeing some CI failures with a few of these changes:

Error: The template is not valid. .github/workflows/ci.yml (Line: 193, Col: 30): Error reading JToken from JsonReader. Path '', line 0, position 0.

Error: The template is not valid. .github/workflows/ci.yml (Line: 202, Col: 27): Error reading JToken from JsonReader. Path '', line 0, position 0.

Error: The template is not valid. .github/workflows/ci.yml (Line: 241, Col: 30): Error reading JToken from JsonReader. Path '', line 0, position 0.

Comment thread .github/workflows/ci.yml Outdated
@bayandin

bayandin commented Dec 16, 2024

Copy link
Copy Markdown
Contributor

Seeing some CI failures with a few of these changes:

Error: The template is not valid. .github/workflows/ci.yml (Line: 193, Col: 30): Error reading JToken from JsonReader. Path '', line 0, position 0.

Error: The template is not valid. .github/workflows/ci.yml (Line: 202, Col: 27): Error reading JToken from JsonReader. Path '', line 0, position 0.

Error: The template is not valid. .github/workflows/ci.yml (Line: 241, Col: 30): Error reading JToken from JsonReader. Path '', line 0, position 0.

In https://github.com/Homebrew/homebrew-cask/actions/runs/12342912865/job/34443307830?pr=195318

I suspect "Gather cask information" step got skipped, so some variables weren't set, and in the next steps, they're used in env: (with proposed changes, the evaluation happens earlier)

@samford samford force-pushed the zizmor-ci-template-injection branch 7 times, most recently from 37dc7fd to cdd799b Compare December 17, 2024 02:28
@samford samford force-pushed the zizmor-ci-template-injection branch 8 times, most recently from ea18b8a to bd43a55 Compare December 26, 2024 16:08
This updates `workflows/ci.yml` to use an environment variable to
address a `template-injection` error from `zizmor`.
This updates `workflows/ci.yml` to use environment variables to
address `template-injection` info from `zizmor`.
@samford samford force-pushed the zizmor-ci-template-injection branch from bd43a55 to 458430e Compare December 26, 2024 17:31
@samford samford marked this pull request as ready for review December 26, 2024 17:33
@samford

samford commented Dec 26, 2024

Copy link
Copy Markdown
Member Author

Nothing blew up when I ran this with a version bump commit (https://github.com/Homebrew/homebrew-cask/actions/runs/12505907895), so I think this is finally in a working state. There's some precedent for using the HOMEBREW_ prefix to work around the environment filtering (https://github.com/Homebrew/homebrew-core/blob/c315e9ff988b75b7f0f21f28ca625ff6ad577032/.github/workflows/scheduled.yml#L172), so this is probably fine for now.

I've marked this as ready for review and it would be helpful for folks who are more familiar with the cask CI setup to look over these changes.

@p-linnane p-linnane requested a review from a team December 26, 2024 18:44

@bevanjkay bevanjkay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me

@samford we can test the SNAPSHOT_BEFORE logic by testing google-drive as it should always fail CI with the default.

@bevanjkay bevanjkay merged commit 9b563e7 into master Dec 27, 2024
@bevanjkay bevanjkay deleted the zizmor-ci-template-injection branch December 27, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants