Repository: woodruffw/zizmor
Pull request #343 (2024-12-21 09:51:41)

fix: setup-go's cache-control is OptOut
`setup-go`'s caching is on by default and opt-out with `cache=false`, https://github.com/actions/setup-go?tab=readme-ov-file#v4

Fixes [this](https://github.com/jmelahman/connections/blob/master/.github/workflows/release.yml) false positive,
![screenshot-2024-12-21-09-34-42](https://github.com/user-attachments/assets/166a6d0b-4e23-48f3-aab5-cfc6ce977cbf)


woodruffw: Thanks @jmelahman! I'll review this later today.
ubiratansoares: My bad here, @jmelahman, thank you so much for catching this one! 

Too easy got lost in the booleans when modelling the behaviour of those cache-aware Actions, I guess 🙈 

If I got everything correctly, the issue is `setup-go`  [actually defines opt-in via the `cache` input;](https://github.com/actions/setup-go/blob/main/action.yml#L16C1-L16C106) however the control value is actually a `boolean`, not a string-as-enum in `setup-node`, `setup-java` and others.

I gave a try on your Workflow locally, setting the control value to `ControlValue::Boolean` and it fixes the issue

```bash
➜  zizmor git:(ufs/issue-343) ✗ git diff && cargo run -- tests/test-data/cache-poisoning/issue-343-repro.yml

Δ src/audit/cache_poisoning.rs
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

───────────────────────────────────────────────────────────────────────────────────────┐
• 55: static KNOWN_CACHE_AWARE_ACTIONS: LazyLock<Vec<CacheAwareAction>> = LazyLock::ne │
───────────────────────────────────────────────────────────────────────────────────────┘
        CacheAwareAction {
            uses: Uses::from_step("actions/setup-go").unwrap(),
            cache_control: CacheControl::OptIn("cache"),
            control_value: ControlValue::String,
            control_value: ControlValue::Boolean,
            caching_by_default: true,
        },
        // https://github.com/actions/setup-node/blob/main/action.yml
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
     Running `target/debug/zizmor tests/test-data/cache-poisoning/issue-343-repro.yml`
2024-12-21T16:36:37.656862Z  WARN zizmor: skipping impostor-commit: can't run without a GitHub API token
2024-12-21T16:36:37.656919Z  WARN zizmor: skipping ref-confusion: can't run without a GitHub API token
2024-12-21T16:36:37.656941Z  WARN zizmor: skipping known-vulnerable-actions: can't run without a GitHub API token
2024-12-21T16:36:37.660063Z  INFO audit: zizmor: 🌈 completed /Users/lonewolf/RustroverProjects/zizmor/tests/test-data/cache-poisoning/issue-343-repro.yml
No findings to report. Good job! (6 suppressed)

```

I _think_ the way to go here is moving the control value to `ControlValue:Boolean`, since the description of the `cache` input in `setup-go` makes it clear that true == opt-in. 

Does that make sense?
woodruffw: Unless I'm misunderstanding, I think `setup-go` is indeed opt-out:

> The action will try to enable caching unless the cache input is explicitly set to false.

(From the README)

However, I agree that this should be `ControlValue::Boolean`.
woodruffw: Oh, this was my misunderstanding -- I forgot about the distinction between `CacheControl` and `caching_by_default`. @ubiratansoares's explanation makes sense to me!
woodruffw: Thanks for raising this @jmelahman!



Patch:
diff --git a/src/audit/cache_poisoning.rs b/src/audit/cache_poisoning.rs
index b50edde..a603937 100644
--- a/src/audit/cache_poisoning.rs
+++ b/src/audit/cache_poisoning.rs
@@ -55,7 +55,7 @@ static KNOWN_CACHE_AWARE_ACTIONS: LazyLock<Vec<CacheAwareAction>> = LazyLock::ne
         CacheAwareAction {
             uses: Uses::from_step("actions/setup-go").unwrap(),
             cache_control: CacheControl::OptIn("cache"),
-            control_value: ControlValue::String,
+            control_value: ControlValue::Boolean,
             caching_by_default: true,
         },
         // https://github.com/actions/setup-node/blob/main/action.yml


Test patch:
diff --git a/tests/snapshot.rs b/tests/snapshot.rs
index 74af2a4..1d62312 100644
--- a/tests/snapshot.rs
+++ b/tests/snapshot.rs
@@ -324,6 +324,10 @@ fn cache_poisoning() -> Result<()> {
         .workflow(workflow_under_test("cache-poisoning/publisher-step.yml"))
         .run()?);
 
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test("cache-poisoning/issue-343-repro.yml"))
+        .run()?);
+
     Ok(())
 }
 
diff --git a/tests/snapshots/snapshot__cache_poisoning-11.snap b/tests/snapshots/snapshot__cache_poisoning-11.snap
new file mode 100644
index 0000000..b6c69f8
--- /dev/null
+++ b/tests/snapshots/snapshot__cache_poisoning-11.snap
@@ -0,0 +1,60 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"cache-poisoning/issue-343-repro.yml\")).run()?"
+snapshot_kind: text
+---
+error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
+  --> @@INPUT@@:5:1
+   |
+ 5 | / on:
+ 6 | |   push:
+ 7 | |     tags:
+ 8 | |       - "v*.*.*"
+   | |________________^ generally used when publishing artifacts generated at runtime
+ 9 |
+...
+24 |         - name: true-positive-2
+25 |           uses: actions/setup-go@v5
+   |           ^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
+   |
+   = note: audit confidence → Low
+
+error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
+  --> @@INPUT@@:5:1
+   |
+ 5 | / on:
+ 6 | |   push:
+ 7 | |     tags:
+ 8 | |       - "v*.*.*"
+   | |________________^ generally used when publishing artifacts generated at runtime
+ 9 |
+...
+31 |           uses: actions/setup-go@v5
+32 | /         with:
+33 | |           go-version: stable
+34 | |           cache: true
+35 | |
+36 | |       # Finding because setup enables cache explicitly
+   | |______________________________________________________^ opt-in for caching here
+   |
+   = note: audit confidence → Low
+
+error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
+  --> @@INPUT@@:5:1
+   |
+ 5 | / on:
+ 6 | |   push:
+ 7 | |     tags:
+ 8 | |       - "v*.*.*"
+   | |________________^ generally used when publishing artifacts generated at runtime
+ 9 |
+...
+38 |           uses: actions/setup-go@v5
+39 | /         with:
+40 | |           go-version: stable
+41 | |           cache: "true"
+   | |________________________^ opt-in for caching here
+   |
+   = note: audit confidence → Low
+
+7 findings (4 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 3 high
diff --git a/tests/test-data/cache-poisoning/issue-343-repro.yml b/tests/test-data/cache-poisoning/issue-343-repro.yml
new file mode 100644
index 0000000..3f8f1cf
--- /dev/null
+++ b/tests/test-data/cache-poisoning/issue-343-repro.yml
@@ -0,0 +1,41 @@
+# minimized from https://github.com/woodruffw/zizmor/pull/343
+
+name: Release
+
+on:
+  push:
+    tags:
+      - "v*.*.*"
+
+jobs:
+  goreleaser:
+    runs-on: ubuntu-latest
+    permissions:
+      contents: write
+    steps:
+      # No finding, since cache is explicitly disabled.
+      - name: true-negative-1
+        uses: actions/setup-go@v5
+        with:
+          go-version: stable
+          cache: false
+
+      # Finding because setup-go enables cache by default
+      - name: true-positive-2
+        uses: actions/setup-go@v5
+        with:
+          go-version: stable
+
+      # Finding because setup enables cache explicitly
+      - name: true-positive-2
+        uses: actions/setup-go@v5
+        with:
+          go-version: stable
+          cache: true
+
+      # Finding because setup enables cache explicitly
+      - name: true-positive-3
+        uses: actions/setup-go@v5
+        with:
+          go-version: stable
+          cache: "true"