diff --git a/crates/zizmor/src/audit/github_env.rs b/crates/zizmor/src/audit/github_env.rs index ff7dfe04..48a2ca6b 100644 --- a/crates/zizmor/src/audit/github_env.rs +++ b/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(), diff --git a/crates/zizmor/src/audit/mod.rs b/crates/zizmor/src/audit/mod.rs index c2158757..f1704388 100644 --- a/crates/zizmor/src/audit/mod.rs +++ b/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, } diff --git a/crates/zizmor/src/audit/obfuscation.rs b/crates/zizmor/src/audit/obfuscation.rs index ae7d02e1..1b02baa7 100644 --- a/crates/zizmor/src/audit/obfuscation.rs +++ b/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)?, diff --git a/crates/zizmor/src/audit/template_injection.rs b/crates/zizmor/src/audit/template_injection.rs index 2d8cf81f..259a7f82 100644 --- a/crates/zizmor/src/audit/template_injection.rs +++ b/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}}}")), diff --git a/crates/zizmor/src/audit/use_trusted_publishing.rs b/crates/zizmor/src/audit/use_trusted_publishing.rs index 33a170ff..7c1bb86d 100644 --- a/crates/zizmor/src/audit/use_trusted_publishing.rs +++ b/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(), diff --git a/crates/zizmor/src/main.rs b/crates/zizmor/src/main.rs index f9e9da1c..614cf626 100644 --- a/crates/zizmor/src/main.rs +++ b/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 { 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(), })?; diff --git a/crates/zizmor/src/models.rs b/crates/zizmor/src/models.rs index a888d2b9..505d720c 100644 --- a/crates/zizmor/src/models.rs +++ b/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 { diff --git a/crates/zizmor/src/models/action.rs b/crates/zizmor/src/models/action.rs index aad696da..6afc4f4f 100644 --- a/crates/zizmor/src/models/action.rs +++ b/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 } diff --git a/crates/zizmor/src/models/workflow.rs b/crates/zizmor/src/models/workflow.rs index fb33c13d..4c6b6f7d 100644 --- a/crates/zizmor/src/models/workflow.rs +++ b/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"), + ) + }) + }) } } diff --git a/crates/zizmor/tests/integration/audit/impostor_commit.rs b/crates/zizmor/tests/integration/audit/impostor_commit.rs index 92b9d1fd..5bada037 100644 --- a/crates/zizmor/tests/integration/audit/impostor_commit.rs +++ b/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!( diff --git a/crates/zizmor/tests/integration/audit/obfuscation.rs b/crates/zizmor/tests/integration/audit/obfuscation.rs index 4d8c7bfd..e7149117 100644 --- a/crates/zizmor/tests/integration/audit/obfuscation.rs +++ b/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(()) +} diff --git a/crates/zizmor/tests/integration/e2e.rs b/crates/zizmor/tests/integration/e2e.rs index 7d4c0a50..1ce49aec 100644 --- a/crates/zizmor/tests/integration/e2e.rs +++ b/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 ", diff --git a/crates/zizmor/tests/integration/test-data/obfuscation/issue-1414-repro.yml b/crates/zizmor/tests/integration/test-data/obfuscation/issue-1414-repro.yml new file mode 100644 index 00000000..7d78a85e --- /dev/null +++ b/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 diff --git a/crates/zizmor/tests/integration/test-data/obfuscation/workflow-cmd-default-shell.yml b/crates/zizmor/tests/integration/test-data/obfuscation/workflow-cmd-default-shell.yml new file mode 100644 index 00000000..19bd79d0 --- /dev/null +++ b/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 diff --git a/docs/release-notes.md b/docs/release-notes.md index 7761b02b..9f54073d 100644 --- a/docs/release-notes.md +++ b/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 🌱