fix: allow expressions in shell: clauses (#1336)

woodruffw · web-flow · commit e134107e65ee · 2025-11-12T10:54:29.000-05:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -21,7 +21,7 @@ rust-version = "1.88.0"
 [workspace.dependencies]
 anyhow = "1.0.100"
 github-actions-expressions = { path = "crates/github-actions-expressions", version = "0.0.11" }
-github-actions-models = { path = "crates/github-actions-models", version = "0.38.0" }
+github-actions-models = { path = "crates/github-actions-models", version = "0.39.0" }
 itertools = "0.14.0"
 pest = "2.8.3"
 pest_derive = "2.8.3"
diff --git a/crates/github-actions-models/Cargo.toml b/crates/github-actions-models/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "github-actions-models"
-version = "0.38.0"
+version = "0.39.0"
 description = "Unofficial, high-quality data models for GitHub Actions workflows, actions, and related components"
 repository = "https://github.com/zizmorcore/zizmor/tree/main/crates/github-actions-models"
 keywords = ["github", "ci"]
diff --git a/crates/github-actions-models/src/action.rs b/crates/github-actions-models/src/action.rs
@@ -152,7 +152,7 @@ pub enum StepBody {
         run: String,
 
         /// The shell to run in.
-        shell: String,
+        shell: LoE<String>,
 
         /// An optional working directory to run from.
         working_directory: Option<String>,
diff --git a/crates/github-actions-models/src/workflow/job.rs b/crates/github-actions-models/src/workflow/job.rs
@@ -134,7 +134,7 @@ pub enum StepBody {
 
         /// An optional shell to run in. Defaults to the job or workflow's
         /// default shell.
-        shell: Option<String>,
+        shell: Option<LoE<String>>,
     },
 }
 
diff --git a/crates/github-actions-models/src/workflow/mod.rs b/crates/github-actions-models/src/workflow/mod.rs
@@ -74,7 +74,7 @@ pub struct Defaults {
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "kebab-case")]
 pub struct RunDefaults {
-    pub shell: Option<String>,
+    pub shell: Option<LoE<String>>,
     pub working_directory: Option<String>,
 }
 
diff --git a/crates/zizmor/src/audit/github_env.rs b/crates/zizmor/src/audit/github_env.rs
@@ -12,6 +12,7 @@ use crate::audit::AuditError;
 use crate::config::Config;
 use crate::finding::location::Locatable as _;
 use crate::finding::{Confidence, Finding, Severity};
+use crate::models::StepCommon;
 use crate::models::{workflow::JobExt as _, workflow::Step};
 use crate::state::AuditState;
 use crate::utils;
@@ -383,8 +384,8 @@ impl Audit for GitHubEnv {
         if let StepBody::Run { run, .. } = &step.deref().body {
             let shell = step.shell().unwrap_or_else(|| {
                 tracing::warn!(
-                    "github-env: couldn't determine shell type for {workflow}:{job} step {stepno}",
-                    workflow = step.workflow().key.filename(),
+                    "github-env: couldn't determine shell type for {workflow}:{job} step {stepno}; assuming bash",
+                    workflow = step.workflow().key.presentation_path(),
                     job = step.parent.id(),
                     stepno = step.index
                 );
@@ -423,10 +424,23 @@ impl Audit for GitHubEnv {
     ) -> Result<Vec<Finding<'doc>>, AuditError> {
         let mut findings = vec![];
 
-        let action::StepBody::Run { run, shell, .. } = &step.body else {
+        let action::StepBody::Run { run, .. } = &step.body else {
             return Ok(findings);
         };
 
+        let shell = step.shell().unwrap_or_else(|| {
+            tracing::warn!(
+                "github-env: couldn't determine shell type for {action} step {stepno}; assuming bash",
+                action = step.action().key.presentation_path(),
+                stepno = step.index
+            );
+
+            // The only way shell inference can fail for a `run:` in a
+            // composition action is if a user specifies an expression instead
+            // of a string literal. In that case, assume bash.
+            "bash"
+        });
+
         // TODO: actually use the spanning information here.
         for (dest, _span) in self.uses_github_env(run, shell)? {
             findings.push(
diff --git a/crates/zizmor/src/models.rs b/crates/zizmor/src/models.rs
@@ -4,6 +4,7 @@
 use github_actions_expressions::context;
 use github_actions_models::common;
 use github_actions_models::common::Env;
+use github_actions_models::common::expr::LoE;
 use github_actions_models::workflow::job::Strategy;
 
 use crate::finding::location::Locatable;
@@ -30,7 +31,7 @@ pub(crate) enum StepBodyCommon<'s> {
     Run {
         run: &'s str,
         _working_directory: Option<&'s str>,
-        _shell: Option<&'s str>,
+        _shell: Option<&'s LoE<String>>,
     },
 }
 
@@ -59,7 +60,10 @@ pub(crate) trait StepCommon<'doc>: Locatable<'doc> + HasInputs {
 
     /// Returns the effective shell for this step, if it can be determined.
     /// This includes the step's explicit shell, job defaults, workflow defaults,
-    /// and runner defaults. Returns `None` if the shell cannot be statically determined.
+    /// and runner defaults.
+    ///
+    /// Returns `None` if the shell cannot be statically determined, including
+    /// if the shell is specified via an expression.
     fn shell(&self) -> Option<&str>;
 }
 
diff --git a/crates/zizmor/src/models/action.rs b/crates/zizmor/src/models/action.rs
@@ -4,7 +4,11 @@
 //! providing higher-level APIs for zizmor to use.
 
 use github_actions_expressions::context;
-use github_actions_models::{action, common, workflow::job::Strategy};
+use github_actions_models::{
+    action,
+    common::{self, expr::LoE},
+    workflow::job::Strategy,
+};
 use terminal_link::Link;
 
 use crate::{
@@ -226,8 +230,12 @@ impl<'doc> StepCommon<'doc> for CompositeStep<'doc> {
     }
 
     fn shell(&self) -> Option<&str> {
-        // For composite action steps, shell is always explicitly specified in the YAML
-        if let action::StepBody::Run { shell, .. } = &self.inner.body {
+        // For composite action steps, shell is always explicitly specified in the YAML.
+        if let action::StepBody::Run {
+            shell: LoE::Literal(shell),
+            ..
+        } = &self.inner.body
+        {
             Some(shell)
         } else {
             None
diff --git a/crates/zizmor/src/models/workflow.rs b/crates/zizmor/src/models/workflow.rs
@@ -662,7 +662,7 @@ impl<'doc> StepCommon<'doc> for Step<'doc> {
             } => StepBodyCommon::Run {
                 run,
                 _working_directory: working_directory.as_deref(),
-                _shell: shell.as_deref(),
+                _shell: shell.as_ref(),
             },
         }
     }
@@ -713,21 +713,33 @@ impl<'doc> Step<'doc> {
         // The steps's own `shell:` takes precedence, followed by the
         // job's default, followed by the entire workflow's default,
         // followed by the runner's default.
-        shell
-            .as_deref()
-            .or_else(|| {
-                self.job()
+        // If any of these is an expression, we can't infer the shell
+        // statically, so we terminate early with `None`.
+        let shell = match shell {
+            Some(LoE::Literal(shell)) => Some(shell.as_str()),
+            Some(LoE::Expr(_)) => return None,
+            None => match self
+                .job()
+                .defaults
+                .as_ref()
+                .and_then(|d| d.run.as_ref().and_then(|r| r.shell.as_ref()))
+            {
+                Some(LoE::Literal(shell)) => Some(shell.as_str()),
+                Some(LoE::Expr(_)) => return None,
+                None => match self
+                    .workflow()
                     .defaults
                     .as_ref()
-                    .and_then(|d| d.run.as_ref().and_then(|r| r.shell.as_deref()))
-            })
-            .or_else(|| {
-                self.workflow()
-                    .defaults
-                    .as_ref()
-                    .and_then(|d| d.run.as_ref().and_then(|r| r.shell.as_deref()))
-            })
-            .or_else(|| self.parent.runner_default_shell())
+                    .and_then(|d| d.run.as_ref().and_then(|r| r.shell.as_ref()))
+                {
+                    Some(LoE::Literal(shell)) => Some(shell.as_str()),
+                    Some(LoE::Expr(_)) => return None,
+                    None => None,
+                },
+            },
+        };
+
+        shell.or_else(|| self.parent.runner_default_shell())
     }
 }
 
diff --git a/crates/zizmor/tests/integration/acceptance.rs b/crates/zizmor/tests/integration/acceptance.rs
@@ -1,13 +1,13 @@
 use crate::common::input_under_test;
-use assert_cmd::Command;
+use assert_cmd::{Command, cargo};
 use serde_json::Value;
 use serde_json_path::JsonPath;
 
 // Acceptance tests for zizmor, on top of Json output
 // For now we don't cover tests that depends on GitHub API under the hood
 
 fn zizmor() -> Command {
-    let mut cmd = Command::cargo_bin("zizmor").expect("Cannot create executable command");
+    let mut cmd = Command::new(cargo::cargo_bin!());
     // All tests are currently offline, and we always need JSON output.
     cmd.args(["--offline", "--format", "json"]);
     cmd
diff --git a/crates/zizmor/tests/integration/common.rs b/crates/zizmor/tests/integration/common.rs
@@ -3,7 +3,7 @@ use camino::Utf8PathBuf;
 use regex::{Captures, Regex};
 use std::{env::current_dir, io::ErrorKind, sync::LazyLock};
 
-use assert_cmd::Command;
+use assert_cmd::{Command, cargo};
 
 static TEST_PREFIX: LazyLock<Utf8PathBuf> = LazyLock::new(|| {
     let current_dir = current_dir().expect("Cannot figure out current directory");
@@ -51,7 +51,7 @@ pub struct Zizmor {
 impl Zizmor {
     /// Create a new zizmor runner.
     pub fn new() -> Self {
-        let cmd = Command::cargo_bin("zizmor").unwrap();
+        let cmd = Command::new(cargo::cargo_bin!());
 
         Self {
             cmd,
diff --git a/crates/zizmor/tests/integration/snapshot.rs b/crates/zizmor/tests/integration/snapshot.rs
@@ -3,7 +3,7 @@
 //! TODO: This file is too big; break it into multiple
 //! modules, one per audit/conceptual group.
 
-use crate::common::{input_under_test, zizmor};
+use crate::common::{OutputMode, input_under_test, zizmor};
 use anyhow::Result;
 
 #[test]
@@ -707,6 +707,21 @@ fn github_env() -> Result<()> {
             .run()?
     );
 
+    // Ensures that we produce a reasonable warning if the user gives us a
+    // `shell:` clause containing an expression.
+    insta::assert_snapshot!(
+        zizmor()
+            .output(OutputMode::Both)
+            .setenv("RUST_LOG", "warn")
+            .input(input_under_test("github-env/issue-1333/action.yml"))
+            .run()?,
+        @r"
+    🌈 zizmor v@@VERSION@@
+     WARN zizmor::audit::github_env: github-env: couldn't determine shell type for @@INPUT@@ step 0; assuming bash
+    No findings to report. Good job!
+    "
+    );
+
     Ok(())
 }
 
diff --git a/crates/zizmor/tests/integration/test-data/github-env/issue-1333/action.yml b/crates/zizmor/tests/integration/test-data/github-env/issue-1333/action.yml
@@ -0,0 +1,15 @@
+---
+name: Test
+description: |
+  Composite action for testing
+inputs:
+  shell:
+    description: The shell to use
+    default: bash --noprofile {0}
+    required: false
+runs:
+  using: composite
+  steps:
+    - run: |
+        echo "Path is: $PATH"
+      shell: ${{ inputs.shell }}
diff --git a/docs/release-notes.md b/docs/release-notes.md
@@ -29,6 +29,9 @@ of `zizmor`.
   improving compatibility with user environments that perform TLS interception
   (#1328)
 
+* The [github-env] now falls back to assuming bash-like shell syntax in
+  `run:` blocks if it can't infer the shell being used (#1336)
+
 ## 1.16.3
 
 ### Bug Fixes 🐛

Original file line number	Diff line number	Diff line change
`@@ -134,7 +134,7 @@ pub enum StepBody {`
`134`	`134`
`135`	`135`	`/// An optional shell to run in. Defaults to the job or workflow's`
`136`	`136`	`/// default shell.`
`137`		`- shell: Option<String>,`
	`137`	`+ shell: Option<LoE<String>>,`
`138`	`138`	`},`
`139`	`139`	`}`
`140`	`140`
Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ pub struct Defaults {`
`74`	`74`	`#[derive(Deserialize, Debug)]`
`75`	`75`	`#[serde(rename_all = "kebab-case")]`
`76`	`76`	`pub struct RunDefaults {`
`77`		`- pub shell: Option<String>,`
	`77`	`+ pub shell: Option<LoE<String>>,`
`78`	`78`	`pub working_directory: Option<String>,`
`79`	`79`	`}`
`80`	`80`