fix: handle non-local shell definitions in obfuscation audit (#1418)

crates/zizmor/src/audit/github_env.rs

@@ -392,7 +392,7 @@ impl Audit for GitHubEnv {
         }
 
         if let StepBody::Run { run, .. } = &step.deref().body {
-            let shell = step.shell().unwrap_or_else(|| {
+            let shell = step.shell().map(|s| s.0).unwrap_or_else(|| {
                 tracing::warn!(
                     "github-env: couldn't determine shell type for {workflow}:{job} step {stepno}; assuming bash",
                     workflow = step.workflow().key.presentation_path(),
@@ -438,7 +438,7 @@ impl Audit for GitHubEnv {
             return Ok(findings);
         };
 
-        let shell = step.shell().unwrap_or_else(|| {
+        let shell = step.shell().map(|s| s.0).unwrap_or_else(|| {
             tracing::warn!(
                 "github-env: couldn't determine shell type for {action} step {stepno}; assuming bash",
                 action = step.action().key.presentation_path(),

crates/zizmor/src/audit/mod.rs

@@ -189,10 +189,9 @@ pub(crate) enum AuditLoadError {
 }
 
 #[derive(Error, Debug)]
-#[error("error in {ident}")]
+#[error("error in '{ident}' audit")]
 pub(crate) struct AuditError {
     ident: &'static str,
-    #[source]
     source: anyhow::Error,
 }
 

crates/zizmor/src/audit/obfuscation.rs

@@ -225,7 +225,10 @@ impl Obfuscation {
                 }
             }
             crate::models::StepBodyCommon::Run { .. } => {
-                if let Some("cmd" | "cmd.exe") = step.shell().map(utils::normalize_shell) {
+                if let Some(("cmd" | "cmd.exe", shell_loc)) = step
+                    .shell()
+                    .map(|(shell, loc)| (utils::normalize_shell(shell), loc))
+                {
                     // `shell: cmd` is basically impossible to analyze: it has no formal
                     // grammar and has several line continuation mechanisms that stymie
                     // naive matching. It also hasn't been the default shell on Windows
@@ -235,11 +238,10 @@ impl Obfuscation {
                             .confidence(Confidence::High)
                             .severity(Severity::Low)
                             .add_location(
-                                step.location()
-                                    .primary()
-                                    .with_keys(["shell".into()])
+                                step.location_with_grip()
                                     .annotated("Windows CMD shell limits analysis"),
                             )
+                            .add_location(shell_loc.primary())
                             .tip("use 'shell: pwsh' or 'shell: bash' for improved analysis")
                             .build(step)
                             .map_err(Self::err)?,

crates/zizmor/src/audit/template_injection.rs

@@ -185,7 +185,7 @@ impl TemplateInjection {
             return None;
         }
 
-        let shell = utils::normalize_shell(step.shell()?);
+        let shell = utils::normalize_shell(step.shell()?.0);
 
         match shell {
             "bash" | "sh" | "zsh" => Some(format!("${{{env_var}}}")),

crates/zizmor/src/audit/use_trusted_publishing.rs

@@ -430,7 +430,7 @@ impl Audit for UseTrustedPublishing {
         if let StepBodyCommon::Run { run, .. } = step.body()
             && !step.parent.has_id_token()
         {
-            let shell = step.shell().unwrap_or_else(|| {
+            let shell = step.shell().map(|s| s.0).unwrap_or_else(|| {
                 tracing::debug!(
                     "use-trusted-publishing: couldn't determine shell type for {workflow}:{job} step {stepno}",
                     workflow = step.workflow().key.filename(),

crates/zizmor/src/main.rs

@@ -30,6 +30,7 @@ use tracing_indicatif::{IndicatifLayer, span_ext::IndicatifSpanExt};
 use tracing_subscriber::{EnvFilter, layer::SubscriberExt as _, util::SubscriberInitExt as _};
 
 use crate::{
+    audit::AuditError,
     config::{Config, ConfigError, ConfigErrorInner},
     github::Client,
     models::AsDocument,
@@ -594,10 +595,10 @@ enum Error {
     #[error("failed to load audit rules")]
     AuditLoad(#[source] anyhow::Error),
     /// An error while running an audit.
-    #[error("{ident} failed on {input}")]
+    #[error("'{ident}' audit failed on {input}")]
     Audit {
         ident: &'static str,
-        source: anyhow::Error,
+        source: AuditError,
         input: String,
     },
     /// An error while rendering output.
@@ -794,7 +795,7 @@ async fn run(app: &mut App) -> Result<ExitCode, Error> {
             while let Some(findings) = completion_stream.next().await {
                 let findings = findings.map_err(|err| Error::Audit {
                     ident: err.ident(),
-                    source: err.into(),
+                    source: err,
                     input: input.key().to_string(),
                 })?;
 

crates/zizmor/src/models.rs

@@ -7,7 +7,7 @@ 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;
+use crate::finding::location::{Locatable, SymbolicLocation};
 use crate::models::inputs::HasInputs;
 
 pub(crate) mod action;
@@ -64,7 +64,7 @@ pub(crate) trait StepCommon<'doc>: Locatable<'doc> + HasInputs {
     ///
     /// Returns `None` if the shell cannot be statically determined, including
     /// if the shell is specified via an expression.
-    fn shell(&self) -> Option<&str>;
+    fn shell(&self) -> Option<(&str, SymbolicLocation<'doc>)>;
 }
 
 impl<'a, 'doc, T: StepCommon<'doc>> AsDocument<'a, 'doc> for T {

crates/zizmor/src/models/action.rs

@@ -232,14 +232,19 @@ impl<'doc> StepCommon<'doc> for CompositeStep<'doc> {
         self.action().as_document()
     }
 
-    fn shell(&self) -> Option<&str> {
+    fn shell(&self) -> Option<(&str, SymbolicLocation<'doc>)> {
         // 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)
+            Some((
+                shell,
+                self.location()
+                    .with_keys(["shell".into()])
+                    .annotated("shell defined here"),
+            ))
         } else {
             None
         }

crates/zizmor/src/models/workflow.rs

@@ -691,7 +691,7 @@ impl<'doc> StepCommon<'doc> for Step<'doc> {
         self.workflow().as_document()
     }
 
-    fn shell(&self) -> Option<&str> {
+    fn shell(&self) -> Option<(&str, SymbolicLocation<'doc>)> {
         // For workflow steps, we can use the existing shell() method
         self.shell()
     }
@@ -720,7 +720,7 @@ impl<'doc> Step<'doc> {
     /// if the shell can't be statically inferred.
     ///
     /// Invariant: panics if the step is not a `run:` step.
-    pub(crate) fn shell(&self) -> Option<&str> {
+    pub(crate) fn shell(&self) -> Option<(&str, SymbolicLocation<'doc>)> {
         let StepBody::Run {
             run: _,
             working_directory: _,
@@ -736,7 +736,12 @@ impl<'doc> Step<'doc> {
         // 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::Literal(shell)) => Some((
+                shell.as_str(),
+                self.location()
+                    .with_keys(["shell".into()])
+                    .annotated("shell defined here"),
+            )),
             Some(LoE::Expr(_)) => return None,
             None => match self
                 .job()
@@ -744,7 +749,13 @@ impl<'doc> Step<'doc> {
                 .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::Literal(shell)) => Some((
+                    shell.as_str(),
+                    self.job()
+                        .location()
+                        .with_keys(["defaults".into(), "run".into(), "shell".into()])
+                        .annotated("job default shell defined here"),
+                )),
                 Some(LoE::Expr(_)) => return None,
                 None => match self
                     .workflow()
@@ -752,14 +763,30 @@ impl<'doc> Step<'doc> {
                     .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::Literal(shell)) => Some((
+                        shell.as_str(),
+                        self.workflow()
+                            .location()
+                            .with_keys(["defaults".into(), "run".into(), "shell".into()])
+                            .annotated("workflow default shell defined here"),
+                    )),
                     Some(LoE::Expr(_)) => return None,
                     None => None,
                 },
             },
         };
 
-        shell.or_else(|| self.parent.runner_default_shell())
+        shell.or_else(|| {
+            self.parent.runner_default_shell().map(|shell| {
+                (
+                    shell,
+                    self.job()
+                        .location()
+                        .with_keys(["runs-on".into()])
+                        .annotated("shell implied by runner"),
+                )
+            })
+        })
     }
 }
 

crates/zizmor/tests/integration/audit/impostor_commit.rs

@@ -1,7 +1,9 @@
 use crate::common::{input_under_test, zizmor};
 
-#[cfg_attr(not(feature = "gh-token-tests"), ignore)]
-#[cfg_attr(not(feature = "slow-tests"), ignore)]
+#[cfg_attr(
+    any(not(feature = "gh-token-tests"), not(feature = "slow-tests")),
+    ignore
+)]
 #[test]
 fn test_regular_persona() -> anyhow::Result<()> {
     insta::assert_snapshot!(

crates/zizmor/tests/integration/audit/obfuscation.rs

@@ -237,3 +237,56 @@ fn test_issue_1177_repro_pedantic() -> Result<()> {
 
     Ok(())
 }
+
+/// Reproduces issue #1414: the obfuscation audit should not crash if the
+/// user has `shell: cmd` defined as a job or workflow default rather than
+/// at the step level.
+///
+/// See: https://github.com/zizmorcore/zizmor/issues/1414
+#[test]
+fn test_issue_1414_repro() -> Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+            .input(input_under_test("obfuscation/issue-1414-repro.yml"))
+            .run()?,
+        @r"
+    help[obfuscation]: obfuscated usage of GitHub Actions features
+      --> @@INPUT@@:13:9
+       |
+    13 |         shell: cmd
+       |         ^^^^^^^^^^ job default shell defined here
+    14 |     steps:
+    15 |       - name: say hi
+       |         ------------ Windows CMD shell limits analysis
+       |
+       = note: audit confidence → High
+       = tip: use 'shell: pwsh' or 'shell: bash' for improved analysis
+
+    3 findings (2 suppressed): 0 informational, 1 low, 0 medium, 0 high
+    "
+    );
+
+    // Like #1414, but with `shell: cmd` defined at the workflow level.
+    insta::assert_snapshot!(
+        zizmor()
+            .input(input_under_test("obfuscation/workflow-cmd-default-shell.yml"))
+            .run()?,
+        @r"
+    help[obfuscation]: obfuscated usage of GitHub Actions features
+      --> @@INPUT@@:10:5
+       |
+    10 |     shell: cmd
+       |     ^^^^^^^^^^ workflow default shell defined here
+    ...
+    16 |       - name: say hi
+       |         ------------ Windows CMD shell limits analysis
+       |
+       = note: audit confidence → High
+       = tip: use 'shell: pwsh' or 'shell: bash' for improved analysis
+
+    3 findings (2 suppressed): 0 informational, 1 low, 0 medium, 0 high
+    "
+    );
+
+    Ok(())
+}

crates/zizmor/tests/integration/e2e.rs

@@ -481,10 +481,10 @@ fn issue_1286() -> Result<()> {
         @r"
     🌈 zizmor v@@VERSION@@
     fatal: no audit was performed
-    ref-confusion failed on file://@@INPUT@@
+    'ref-confusion' audit failed on file://@@INPUT@@
 
     Caused by:
-        0: error in ref-confusion
+        0: error in 'ref-confusion' audit
         1: couldn't list branches for woodruffw-experiments/this-does-not-exist
         2: can't access woodruffw-experiments/this-does-not-exist: missing or you have no access
     ",

crates/zizmor/tests/integration/test-data/obfuscation/issue-1414-repro.yml

@@ -0,0 +1,16 @@
+name: issue-1414-repro
+
+on:
+  pull_request:
+
+permissions: {}
+
+jobs:
+  some-job:
+    runs-on: windows-latest
+    defaults:
+      run:
+        shell: cmd
+    steps:
+      - name: say hi
+        run: echo hi

crates/zizmor/tests/integration/test-data/obfuscation/workflow-cmd-default-shell.yml

@@ -0,0 +1,17 @@
+name: workflow-cmd-default-shell
+
+on:
+  pull_request:
+
+permissions: {}
+
+defaults:
+  run:
+    shell: cmd
+
+jobs:
+  some-job:
+    runs-on: windows-latest
+    steps:
+      - name: say hi
+        run: echo hi

docs/release-notes.md

@@ -31,6 +31,12 @@ of `zizmor`.
 * zizmor now produces more useful and less ambiguous spans for many findings,
   particularly those from the [anonymous-definition] audit (#1416)
 
+### Bug Fixes 🐛
+
+* Fixed a bug where the [obfuscation] audit would crash if it encountered
+  a CMD shell that was defined outside of the current step block (i.e. 
+  as a job or workflow default) (#1418)
+
 ## 1.18.0
 
 ### Enhancements 🌱