Review GitHub Actions with zizmor 1.0.1

[tcompa]

I am starting with a bunch of persist-credentials: false (note: in some cases it may actually be needed to persist credentials.. I'll revert changes in this PR when appropriate).

Checklist before merging

• I added an appropriate entry to CHANGELOG.md

[tcompa]

Here are the remaining findings:

warning[artipacked]: credential persistence through GitHub Actions artifacts
  --> .../fractal-web/.github/workflows/github_release.yaml:21:9
   |
21 |       - uses: actions/checkout@v4
   |         ------------------------- does not set persist-credentials: false
   |
   = note: audit confidence → Low

error[template-injection]: code injection via template expansion
  --> .../fractal-web/.github/workflows/github_release.yaml:41:9
   |
41 | ... - name: Repack the package removing parent folder
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this step
42 | ...   run: tar -C package -czf node-${{ matrix.node-version }}-fractal-web-${{ github.ref_name }}.tar.gz build package.json node_modules LICENS...
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ github.ref_name may expand into attacker-controllable code
   |
   = note: audit confidence → High

error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
  --> .../fractal-web/.github/workflows/github_release.yaml:2:1
   |
 2 | / on:
 3 | |   push:
...  |
 9 | |       - 'v[0-9]+.[0-9]+.[0-9]+beta[0-9]+'
10 | |       - 'v[0-9]+.[0-9]+.[0-9]+rc[0-9]+'
   | |_______________________________________^ generally used when publishing artifacts generated at runtime
11 |
...
24 |           uses: actions/setup-node@v4
25 | /         with:
26 | |           node-version: ${{ matrix.node-version }}
27 | |           cache: npm
   | |____________________^ opt-in for caching here
   |
   = note: audit confidence → Low

18 findings (15 suppressed): 0 unknown, 0 informational, 0 low, 1 medium, 2 high

The related explanations and suggested remediations are

• https://woodruffw.github.io/zizmor/audits/#template-injection
• https://woodruffw.github.io/zizmor/audits/#cache-poisoning

I'll propose a fix based on those

[zonia3000]

Just a comment on the following fix to highlight that, on one hand, it is an issue that is already mitigated upstream by the constraints on tag names, and on the other hand, it would still leaves room for other attacks in similar situations (without the contstraints), even though it removes the warning from the static analysis tool.

env:
  GITHUB_REF_NAME: ${{ github.ref_name }}
run: tar -C package -czf node-${{ matrix.node-version }}-fractal-web-${GITHUB_REF_NAME}.tar.gz build package.json node_modules LICENSE

Already mitigated upstream

Even without addressing the warning reported by zizmor, injection is not possible in this specific case because the action is executed only when the following constraints on tags are met:

tags:
  - 'v[0-9]+.[0-9]+.[0-9]+'
  - 'v[0-9]+.[0-9]+.[0-9]+[a-c][0-9]+'
  - 'v[0-9]+.[0-9]+.[0-9]+-[a-c][0-9]+'
  - 'v[0-9]+.[0-9]+.[0-9]+alpha[0-9]+'
  - 'v[0-9]+.[0-9]+.[0-9]+beta[0-9]+'
  - 'v[0-9]+.[0-9]+.[0-9]+rc[0-9]+'

Similar problematic case exploiting the issue title

Git tags already limit the characters that can be used (for example, spaces are not allowed), but a similar action based on the issue title (github.event.issue.title) could be exploited to modify the contents of the tar file.

In the following (unrealistic) example, the job creates an artifact based on the issue title. Zizmor does not detect any issues.

name: Open Issue Workflow

on:
  issues: 
    types: [opened]

jobs:
  job:
    runs-on: ubuntu-latest
    steps:
      - name: Create foo file
        run: touch foo.tar.gz

      - name: Create secret file
        run: echo "secret" > secret.txt

      - name: Test
        env:
          ISSUE_TITLE: ${{ github.event.issue.title }}
        run: tar -czf test-${ISSUE_TITLE}.tar.gz foo.tar.gz

      - name: Upload artifact
        uses: actions/upload-artifact@v4
        with:
          path: test-*.tar.gz

The purpose of the job is to create a file named test-issue-title.tar.gz containing the file foo.tar.gz. However, if a user creates an issue with the title "x.tar.gz secret.txt foo," they will succeed in generating a tar file that also contains the secret.txt file, which the action author likely did not intend to expose within the artifact.

To eliminate the problem, the filename should be escaped with quotes:

run: tar -czf "test-${ISSUE_TITLE}.tar.gz" foo.tar.gz

Similarly, to be cautious, we should escape in our action as well:

run: tar -C package -czf "node-${{ matrix.node-version }}-fractal-web-${GITHUB_REF_NAME}.tar.gz" build package.json node_modules LICENSE

[tcompa]

Thanks a lot for these clarifications!