Repository: woodruffw/zizmor
Pull request #317 (2024-12-17 06:26:08)

fix: expands_to_static_values considers expressions inside strings
This tweaks #298 slightly, following the regression noted in https://github.com/woodruffw/zizmor/issues/22#issuecomment-2543128489 -- the previous `expands_to_static_values` only checked whether the expanded matrix value was *precisely* an expression, but we also need to handle the case where an expanded matrix value *contains* an expression.

By way of example:

```yaml
matrix:
  include:
    - foo: 123
      bar: ${{ expression }} # correctly fails expands_to_static_values
    - foo: 456
      bar: prefix-${{ expression }} # incorrectly passes expands_to_static_values
```

I'll work on a testcase for this as well. CC @ubiratansoares for viz 🙂 


Patch:
diff --git a/src/models.rs b/src/models.rs
index bb866cd..6cca05c 100644
--- a/src/models.rs
+++ b/src/models.rs
@@ -3,6 +3,7 @@
 
 use crate::finding::{Route, SymbolicLocation};
 use crate::registry::WorkflowKey;
+use crate::utils::extract_expressions;
 use anyhow::{bail, Context, Result};
 use camino::Utf8Path;
 use github_actions_models::common::expr::LoE;
@@ -275,8 +276,12 @@ impl<'w> Matrix<'w> {
     /// Checks whether some expanded path leads to an expression
     pub(crate) fn expands_to_static_values(&self, context: &str) -> bool {
         let expands_to_expression = self.expanded_values.iter().any(|(path, expansion)| {
-            let expanded_to_expression = expansion.starts_with("${{") && expansion.ends_with("}}");
-            context == path && expanded_to_expression
+            // Each expanded value in the matrix might be an expression, or contain
+            // one or more expressions (e.g. `foo-${{ bar }}-${{ baz }}`). So we
+            // need to check for *any* expression in the expanded value,
+            // not just that it starts and ends with the expression delimiters.
+            let expansion_contains_expression = !extract_expressions(expansion).is_empty();
+            context == path && expansion_contains_expression
         });
 
         !expands_to_expression


Test patch:
diff --git a/tests/snapshot.rs b/tests/snapshot.rs
index 746ac9a..8b94e42 100644
--- a/tests/snapshot.rs
+++ b/tests/snapshot.rs
@@ -251,5 +251,9 @@ fn template_injection() -> Result<()> {
         .workflow(workflow_under_test("template-injection/issue-22-repro.yml"))
         .run()?);
 
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test("template-injection/pr-317-repro.yml"))
+        .run()?);
+
     Ok(())
 }
diff --git a/tests/snapshots/snapshot__template_injection-4.snap b/tests/snapshots/snapshot__template_injection-4.snap
new file mode 100644
index 0000000..4e1eaf9
--- /dev/null
+++ b/tests/snapshots/snapshot__template_injection-4.snap
@@ -0,0 +1,19 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"template-injection/pr-317-repro.yml\")).run()?"
+snapshot_kind: text
+---
+warning[template-injection]: code injection via template expansion
+  --> @@INPUT@@:25:9
+   |
+25 |         - run: |
+   |  _________-
+26 | |           echo ${{ matrix.bar }}
+   | |                                 -
+   | |_________________________________|
+   |                                   this step
+   |                                   matrix.bar may expand into attacker-controllable code
+   |
+   = note: audit confidence → Medium
+
+1 finding: 0 unknown, 0 informational, 0 low, 1 medium, 0 high
diff --git a/tests/test-data/template-injection/pr-317-repro.yml b/tests/test-data/template-injection/pr-317-repro.yml
new file mode 100644
index 0000000..829ad44
--- /dev/null
+++ b/tests/test-data/template-injection/pr-317-repro.yml
@@ -0,0 +1,26 @@
+# reproduction case for https://github.com/woodruffw/zizmor/pull/317
+
+name: PR-317-REPRO
+on:
+  pull_request:
+
+jobs:
+  PR-317-REPRO:
+    name: PR-317-REPRO
+    runs-on: ubuntu-latest
+
+    strategy:
+      matrix:
+        include:
+          - foo: 123
+            bar: abc
+
+          - foo: 456
+            # BUG: this should be detected as a matrix expansion
+            # candidate containing an expression, but was not
+            # before #317.
+            bar: prefix-${{ github.repository_owner }}
+
+    steps:
+      - run: |
+          echo ${{ matrix.bar }}