From 2f4c874499f7d3324836da1810dfa37304aec431 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 5 Jun 2025 13:38:59 -0400 Subject: [PATCH] feat: artipacked now supported composite actions (#896) --- crates/zizmor/src/audit/artipacked.rs | 169 +++++++++--------- crates/zizmor/src/audit/cache_poisoning.rs | 2 +- crates/zizmor/src/audit/dangerous_triggers.rs | 1 - crates/zizmor/src/audit/forbidden_uses.rs | 4 +- .../src/audit/known_vulnerable_actions.rs | 5 +- crates/zizmor/src/audit/mod.rs | 9 +- crates/zizmor/src/audit/obfuscation.rs | 6 +- .../src/audit/overprovisioned_secrets.rs | 2 +- crates/zizmor/src/audit/stale_action_refs.rs | 5 +- crates/zizmor/src/audit/template_injection.rs | 16 +- crates/zizmor/src/audit/unpinned_uses.rs | 4 +- crates/zizmor/src/audit/unredacted_secrets.rs | 2 +- crates/zizmor/src/finding/location.rs | 8 +- crates/zizmor/src/models.rs | 74 +++++--- crates/zizmor/src/models/coordinate.rs | 10 +- crates/zizmor/tests/integration/snapshot.rs | 7 + .../integration__snapshot__artipacked-5.snap | 27 +++ .../artipacked/demo-action/action.yml | 15 ++ docs/release-notes.md | 5 + 19 files changed, 218 insertions(+), 153 deletions(-) create mode 100644 crates/zizmor/tests/integration/snapshots/integration__snapshot__artipacked-5.snap create mode 100644 crates/zizmor/tests/integration/test-data/artipacked/demo-action/action.yml diff --git a/crates/zizmor/src/audit/artipacked.rs b/crates/zizmor/src/audit/artipacked.rs index 093755fa..73688d1e 100644 --- a/crates/zizmor/src/audit/artipacked.rs +++ b/crates/zizmor/src/audit/artipacked.rs @@ -1,20 +1,11 @@ -use std::ops::Deref as _; - use anyhow::Result; -use github_actions_models::{ - common::{EnvValue, Uses, expr::ExplicitExpr}, - workflow::job::StepBody, -}; +use github_actions_models::common::{EnvValue, Uses, expr::ExplicitExpr}; use itertools::Itertools as _; use super::{Audit, AuditLoadError, audit_meta}; use crate::{ - finding::{ - Confidence, Finding, Fix, Persona, Severity, - location::{Locatable as _, Route, RouteComponent}, - }, - models::{JobExt, Step, uses::RepositoryUsesExt as _}, - route, + finding::{Confidence, Finding, Fix, Persona, Severity, location::Routable as _}, + models::{StepBodyCommon, StepCommon, uses::RepositoryUsesExt as _}, state::AuditState, utils::split_patterns, yaml_patch::{Op, Patch}, @@ -29,74 +20,20 @@ audit_meta!( ); impl Artipacked { - fn dangerous_artifact_patterns<'b>(&self, path: &'b str) -> Vec<&'b str> { - let mut patterns = vec![]; - for path in split_patterns(path) { - match path { - // TODO: this could be even more generic. - "." | "./" | ".." | "../" => patterns.push(path), - path => match ExplicitExpr::from_curly(path) { - Some(expr) if expr.as_bare().contains("github.workspace") => { - patterns.push(path) - } - // TODO: Other expressions worth flagging here? - Some(_) => continue, - _ => continue, - }, - } - } - - patterns - } - - /// Create a Fix for setting persist-credentials: false - fn create_persist_credentials_fix<'doc>(step: &Step<'doc>) -> Fix<'doc> { - let job_id = step.parent.id(); - let checkout_index = step.index; - - Fix { - title: "Set persist-credentials: false".to_string(), - description: "To prevent credential persistence, set 'persist-credentials: false' in this checkout step. \ - When 'persist-credentials' is true (the default), the GITHUB_TOKEN persists in the local git config \ - after checkout, which may be inadvertently leaked through subsequent actions like artifact uploads. \ - Setting 'persist-credentials: false' ensures that credentials don't persist beyond the checkout step itself.".to_string(), - _key: step.location().key, - patches: vec![ - Patch { - route: route!("jobs", job_id, "steps", checkout_index), - operation: Op::MergeInto { - key: "with".to_string(), - value: { - let mut with_map = serde_yaml::Mapping::new(); - with_map.insert( - serde_yaml::Value::String("persist-credentials".to_string()), - serde_yaml::Value::Bool(false), - ); - serde_yaml::Value::Mapping(with_map) - }, - }, - } - ], - } - } -} - -impl Audit for Artipacked { - fn new(_state: &AuditState<'_>) -> Result { - Ok(Self) - } - - fn audit_normal_job<'doc>(&self, job: &super::NormalJob<'doc>) -> Result>> { + fn process_steps<'doc>( + &self, + steps: impl Iterator>, + ) -> anyhow::Result>> { let mut findings = vec![]; // First, collect all vulnerable checkouts and upload steps independently. let mut vulnerable_checkouts = vec![]; let mut vulnerable_uploads = vec![]; - for step in job.steps() { - let StepBody::Uses { + for step in steps { + let StepBodyCommon::Uses { uses: Uses::Repository(uses), with, - } = &step.deref().body + } = &step.body() else { continue; }; @@ -133,20 +70,20 @@ impl Audit for Artipacked { if vulnerable_uploads.is_empty() { // If we have no vulnerable uploads, then emit lower-confidence // findings for just the checkout steps. - for (checkout, persona) in vulnerable_checkouts { + for (checkout, persona) in &vulnerable_checkouts { findings.push( Self::finding() .severity(Severity::Medium) .confidence(Confidence::Low) - .persona(persona) + .persona(*persona) .add_location( checkout .location() .primary() .annotated("does not set persist-credentials: false"), ) - .fix(Self::create_persist_credentials_fix(&checkout)) - .build(job.parent())?, + .fix(Self::create_persist_credentials_fix(checkout)) + .build(checkout)?, ); } } else { @@ -154,15 +91,15 @@ impl Audit for Artipacked { // vulnerable upload. There are more efficient ways to do this than // a cartesian product, but this way is simple. for ((checkout, persona), upload) in vulnerable_checkouts - .into_iter() - .cartesian_product(vulnerable_uploads.into_iter()) + .iter() + .cartesian_product(vulnerable_uploads.iter()) { - if checkout.index < upload.index { + if checkout.index() < upload.index() { findings.push( Self::finding() .severity(Severity::High) .confidence(Confidence::High) - .persona(persona) + .persona(*persona) .add_location( checkout .location() @@ -174,8 +111,8 @@ impl Audit for Artipacked { .location() .annotated("may leak the credentials persisted above"), ) - .fix(Self::create_persist_credentials_fix(&checkout)) - .build(job.parent())?, + .fix(Self::create_persist_credentials_fix(checkout)) + .build(checkout)?, ); } } @@ -183,6 +120,72 @@ impl Audit for Artipacked { Ok(findings) } + + fn dangerous_artifact_patterns<'b>(&self, path: &'b str) -> Vec<&'b str> { + let mut patterns = vec![]; + for path in split_patterns(path) { + match path { + // TODO: this could be even more generic. + "." | "./" | ".." | "../" => patterns.push(path), + path => match ExplicitExpr::from_curly(path) { + Some(expr) if expr.as_bare().contains("github.workspace") => { + patterns.push(path) + } + // TODO: Other expressions worth flagging here? + Some(_) => continue, + _ => continue, + }, + } + } + + patterns + } + + /// Create a Fix for setting persist-credentials: false + fn create_persist_credentials_fix<'doc>(step: &impl StepCommon<'doc>) -> Fix<'doc> { + Fix { + title: "Set persist-credentials: false".to_string(), + description: "To prevent credential persistence, set 'persist-credentials: false' in this checkout step. \ + When 'persist-credentials' is true (the default), the GITHUB_TOKEN persists in the local git config \ + after checkout, which may be inadvertently leaked through subsequent actions like artifact uploads. \ + Setting 'persist-credentials: false' ensures that credentials don't persist beyond the checkout step itself.".to_string(), + _key: step.location().key, + patches: vec![ + Patch { + route: step.route(), + operation: Op::MergeInto { + key: "with".to_string(), + value: { + let mut with_map = serde_yaml::Mapping::new(); + with_map.insert( + serde_yaml::Value::String("persist-credentials".to_string()), + serde_yaml::Value::Bool(false), + ); + serde_yaml::Value::Mapping(with_map) + }, + }, + } + ], + } + } +} + +impl Audit for Artipacked { + fn new(_state: &AuditState<'_>) -> Result { + Ok(Self) + } + + fn audit_action<'doc>( + &self, + action: &'doc crate::models::Action, + ) -> anyhow::Result>> { + let steps = action.steps(); + self.process_steps(steps) + } + + fn audit_normal_job<'doc>(&self, job: &super::NormalJob<'doc>) -> Result>> { + self.process_steps(job.steps()) + } } #[cfg(test)] diff --git a/crates/zizmor/src/audit/cache_poisoning.rs b/crates/zizmor/src/audit/cache_poisoning.rs index 3981f300..9a5e9c19 100644 --- a/crates/zizmor/src/audit/cache_poisoning.rs +++ b/crates/zizmor/src/audit/cache_poisoning.rs @@ -277,7 +277,7 @@ impl CachePoisoning { )) } - fn evaluate_cache_usage<'a, 'doc>(&self, step: &impl StepCommon<'a, 'doc>) -> Option { + fn evaluate_cache_usage<'doc>(&self, step: &impl StepCommon<'doc>) -> Option { KNOWN_CACHE_AWARE_ACTIONS .iter() .find_map(|coord| coord.usage(step)) diff --git a/crates/zizmor/src/audit/dangerous_triggers.rs b/crates/zizmor/src/audit/dangerous_triggers.rs index b08b4bc6..69973c38 100644 --- a/crates/zizmor/src/audit/dangerous_triggers.rs +++ b/crates/zizmor/src/audit/dangerous_triggers.rs @@ -1,7 +1,6 @@ use anyhow::Result; use super::{Audit, AuditLoadError, audit_meta}; -use crate::finding::location::Locatable as _; use crate::finding::{Confidence, Finding, Severity}; use crate::models::Workflow; use crate::state::AuditState; diff --git a/crates/zizmor/src/audit/forbidden_uses.rs b/crates/zizmor/src/audit/forbidden_uses.rs index b037bd68..d936aebf 100644 --- a/crates/zizmor/src/audit/forbidden_uses.rs +++ b/crates/zizmor/src/audit/forbidden_uses.rs @@ -36,9 +36,9 @@ impl ForbiddenUses { } } - fn process_step<'a, 'doc>( + fn process_step<'doc>( &self, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> anyhow::Result>> { let mut findings = vec![]; diff --git a/crates/zizmor/src/audit/known_vulnerable_actions.rs b/crates/zizmor/src/audit/known_vulnerable_actions.rs index edc4311b..c845d70e 100644 --- a/crates/zizmor/src/audit/known_vulnerable_actions.rs +++ b/crates/zizmor/src/audit/known_vulnerable_actions.rs @@ -116,10 +116,7 @@ impl KnownVulnerableActions { Ok(results) } - fn process_step<'a, 'doc>( - &self, - step: &'a impl StepCommon<'a, 'doc>, - ) -> Result>> { + fn process_step<'doc>(&self, step: &impl StepCommon<'doc>) -> Result>> { let mut findings = vec![]; let Some(Uses::Repository(uses)) = step.uses() else { diff --git a/crates/zizmor/src/audit/mod.rs b/crates/zizmor/src/audit/mod.rs index fac3253a..426efb95 100644 --- a/crates/zizmor/src/audit/mod.rs +++ b/crates/zizmor/src/audit/mod.rs @@ -7,10 +7,7 @@ use tracing::instrument; use yamlpath::Document; use crate::{ - finding::{ - Finding, FindingBuilder, - location::{Locatable, SymbolicLocation}, - }, + finding::{Finding, FindingBuilder, location::SymbolicLocation}, models::{ Action, AsDocument, CompositeStep, Job, NormalJob, ReusableWorkflowCallJob, Step, Workflow, }, @@ -69,10 +66,8 @@ impl AuditInput { AuditInput::Action(action) => action.link.as_deref(), } } -} -impl<'a> Locatable<'a, 'a> for AuditInput { - fn location(&'a self) -> SymbolicLocation<'a> { + pub(crate) fn location(&self) -> SymbolicLocation<'_> { match self { AuditInput::Workflow(workflow) => workflow.location(), AuditInput::Action(action) => action.location(), diff --git a/crates/zizmor/src/audit/obfuscation.rs b/crates/zizmor/src/audit/obfuscation.rs index 01eab13b..39f5b23d 100644 --- a/crates/zizmor/src/audit/obfuscation.rs +++ b/crates/zizmor/src/audit/obfuscation.rs @@ -5,7 +5,7 @@ use crate::{ Confidence, Severity, finding::{ Finding, - location::{Feature, Locatable as _, Location}, + location::{Feature, Location}, }, models::{CompositeStep, Step, StepCommon}, utils::parse_expressions_from_input, @@ -72,9 +72,9 @@ impl Obfuscation { annotations } - fn process_step<'a, 'doc>( + fn process_step<'doc>( &self, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> anyhow::Result>> { let mut findings = vec![]; diff --git a/crates/zizmor/src/audit/overprovisioned_secrets.rs b/crates/zizmor/src/audit/overprovisioned_secrets.rs index 9e110bc5..4ff7b23a 100644 --- a/crates/zizmor/src/audit/overprovisioned_secrets.rs +++ b/crates/zizmor/src/audit/overprovisioned_secrets.rs @@ -3,7 +3,7 @@ use github_actions_expressions::Expr; use crate::{ finding::{ Confidence, Severity, - location::{Feature, Locatable as _, Location}, + location::{Feature, Location}, }, utils::parse_expressions_from_input, }; diff --git a/crates/zizmor/src/audit/stale_action_refs.rs b/crates/zizmor/src/audit/stale_action_refs.rs index ff17ea97..f2547eab 100644 --- a/crates/zizmor/src/audit/stale_action_refs.rs +++ b/crates/zizmor/src/audit/stale_action_refs.rs @@ -34,10 +34,7 @@ impl StaleActionRefs { Ok(tag.is_none()) } - fn process_step<'a, 'doc>( - &self, - step: &'a impl StepCommon<'a, 'doc>, - ) -> Result>> { + fn process_step<'doc>(&self, step: &impl StepCommon<'doc>) -> Result>> { let mut findings = vec![]; let Some(Uses::Repository(uses)) = step.uses() else { diff --git a/crates/zizmor/src/audit/template_injection.rs b/crates/zizmor/src/audit/template_injection.rs index 6d514c0a..1d0ffc11 100644 --- a/crates/zizmor/src/audit/template_injection.rs +++ b/crates/zizmor/src/audit/template_injection.rs @@ -104,8 +104,8 @@ impl TemplateInjection { .unwrap_or(&[]) } - fn scripts_with_location<'a, 'doc>( - step: &'a impl StepCommon<'a, 'doc>, + fn scripts_with_location<'doc>( + step: &impl StepCommon<'doc>, ) -> Vec<(String, SymbolicLocation<'doc>)> { match step.body() { models::StepBodyCommon::Uses { @@ -196,11 +196,11 @@ impl TemplateInjection { } /// Attempts to produce a `Fix` for a given expression. - fn attempt_fix<'a, 'doc>( + fn attempt_fix<'doc>( &self, raw: &ExplicitExpr, parsed: &Expr, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> Option> { // We can only fix `run:` steps for now. if !matches!(step.body(), models::StepBodyCommon::Run { .. }) { @@ -260,10 +260,10 @@ impl TemplateInjection { }) } - fn injectable_template_expressions<'a, 'doc>( + fn injectable_template_expressions<'doc>( &self, script: &str, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> Vec<(String, Option>, Severity, Confidence, Persona)> { let mut bad_expressions = vec![]; for (expr, _) in extract_expressions(script) { @@ -412,9 +412,9 @@ impl TemplateInjection { bad_expressions } - fn process_step<'a, 'doc>( + fn process_step<'doc>( &self, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> anyhow::Result>> { let mut findings = vec![]; diff --git a/crates/zizmor/src/audit/unpinned_uses.rs b/crates/zizmor/src/audit/unpinned_uses.rs index 95d84cbb..0d784d24 100644 --- a/crates/zizmor/src/audit/unpinned_uses.rs +++ b/crates/zizmor/src/audit/unpinned_uses.rs @@ -87,9 +87,9 @@ impl UnpinnedUses { } } - fn process_step<'a, 'doc>( + fn process_step<'doc>( &self, - step: &'a impl StepCommon<'a, 'doc>, + step: &impl StepCommon<'doc>, ) -> anyhow::Result>> { let mut findings = vec![]; diff --git a/crates/zizmor/src/audit/unredacted_secrets.rs b/crates/zizmor/src/audit/unredacted_secrets.rs index a6f265dc..c14d70a1 100644 --- a/crates/zizmor/src/audit/unredacted_secrets.rs +++ b/crates/zizmor/src/audit/unredacted_secrets.rs @@ -2,7 +2,7 @@ use github_actions_expressions::{Expr, context::Context}; use crate::{ Confidence, Severity, - finding::location::{Feature, Locatable as _, Location}, + finding::location::{Feature, Location}, utils::parse_expressions_from_input, }; diff --git a/crates/zizmor/src/finding/location.rs b/crates/zizmor/src/finding/location.rs index b0a06bdb..89502ab9 100644 --- a/crates/zizmor/src/finding/location.rs +++ b/crates/zizmor/src/finding/location.rs @@ -214,9 +214,9 @@ impl<'doc> SymbolicLocation<'doc> { } /// Gives models (e.g. workflow steps) the ability to express their symbolic location. -pub(crate) trait Locatable<'a, 'doc> { +pub(crate) trait Locatable<'doc> { /// Returns the symbolic location of this model. - fn location(&'a self) -> SymbolicLocation<'doc>; + fn location(&self) -> SymbolicLocation<'doc>; /// Returns an "enriched" symbolic location of this model, /// when the model is of a type that has a name. Otherwise, @@ -224,7 +224,7 @@ pub(crate) trait Locatable<'a, 'doc> { /// /// For example, a GitHub Actions workflow step has an optional name, /// which is included in this symbolic location if present. - fn location_with_name(&'a self) -> SymbolicLocation<'doc> { + fn location_with_name(&self) -> SymbolicLocation<'doc> { self.location() } } @@ -233,7 +233,7 @@ pub(crate) trait Routable<'a, 'doc> { fn route(&'a self) -> Route<'doc>; } -impl<'a, 'doc, T: Locatable<'a, 'doc>> Routable<'a, 'doc> for T { +impl<'a, 'doc, T: Locatable<'doc>> Routable<'a, 'doc> for T { fn route(&'a self) -> Route<'doc> { self.location().route } diff --git a/crates/zizmor/src/models.rs b/crates/zizmor/src/models.rs index 72cf29d5..13290922 100644 --- a/crates/zizmor/src/models.rs +++ b/crates/zizmor/src/models.rs @@ -44,7 +44,10 @@ pub(crate) enum StepBodyCommon<'s> { } /// Common interfaces between workflow and action steps. -pub(crate) trait StepCommon<'a, 'doc>: Locatable<'a, 'doc> { +pub(crate) trait StepCommon<'doc>: Locatable<'doc> { + /// Returns the step's index within its parent job or action. + fn index(&self) -> usize; + /// Returns whether the given `env.name` environment access is "static," /// i.e. is not influenced by another expression. fn env_is_static(&self, name: &str) -> bool; @@ -64,7 +67,7 @@ pub(crate) trait StepCommon<'a, 'doc>: Locatable<'a, 'doc> { fn document(&self) -> &'doc yamlpath::Document; } -impl<'a, 'doc, T: StepCommon<'a, 'doc>> AsDocument<'a, 'doc> for T { +impl<'a, 'doc, T: StepCommon<'doc>> AsDocument<'a, 'doc> for T { fn as_document(&'a self) -> &'doc yamlpath::Document { self.document() } @@ -171,11 +174,15 @@ impl Workflow { Trigger::Events(events) => events.count() == 1, } } -} -impl<'a> Locatable<'a, 'a> for Workflow { - /// This workflow's [`SymbolicLocation`]. - fn location(&'a self) -> SymbolicLocation<'a> { + /// Returns this workflow's [`SymbolicLocation`]. + /// + /// NOTE: This is intentionally implemented directly on the `Workflow` type + /// rather than through the [`Locatable`] trait, since introducing + /// this through [`Locatable`] would require a split lifetime between + /// `'self` and `'doc` for just this and [`Action`], i.e. the owning + /// container types rather than the borrowing subtypes. + pub fn location(&self) -> SymbolicLocation<'_> { SymbolicLocation { key: &self.key, annotation: "this workflow".to_string(), @@ -195,9 +202,9 @@ pub(crate) trait JobExt<'doc> { fn parent(&self) -> &'doc Workflow; } -impl<'a, 'doc, T: JobExt<'doc>> Locatable<'a, 'doc> for T { +impl<'doc, T: JobExt<'doc>> Locatable<'doc> for T { /// Returns this job's [`SymbolicLocation`]. - fn location(&'a self) -> SymbolicLocation<'doc> { + fn location(&self) -> SymbolicLocation<'doc> { self.parent() .location() .annotated("this job") @@ -542,16 +549,16 @@ impl<'doc> Deref for Step<'doc> { } } -impl<'a, 'doc> Locatable<'a, 'doc> for Step<'doc> { +impl<'doc> Locatable<'doc> for Step<'doc> { /// This step's [`SymbolicLocation`]. - fn location(&'a self) -> SymbolicLocation<'doc> { + fn location(&self) -> SymbolicLocation<'doc> { self.parent .location() .with_keys(&["steps".into(), self.index.into()]) .annotated("this step") } - fn location_with_name(&'a self) -> SymbolicLocation<'doc> { + fn location_with_name(&self) -> SymbolicLocation<'doc> { match self.inner.name { Some(_) => self.location().with_keys(&["name".into()]), None => self.location(), @@ -560,7 +567,11 @@ impl<'a, 'doc> Locatable<'a, 'doc> for Step<'doc> { } } -impl<'doc> StepCommon<'_, 'doc> for Step<'doc> { +impl<'doc> StepCommon<'doc> for Step<'doc> { + fn index(&self) -> usize { + self.index + } + fn env_is_static(&self, name: &str) -> bool { utils::env_is_static(name, &[&self.env, &self.job().env, &self.workflow().env]) } @@ -699,19 +710,6 @@ impl<'a> AsDocument<'a, 'a> for Action { } } -impl<'a> Locatable<'a, 'a> for Action { - /// This actions's [`SymbolicLocation`]. - fn location(&'a self) -> SymbolicLocation<'a> { - SymbolicLocation { - key: &self.key, - annotation: "this action".to_string(), - link: None, - route: Route::new(), - kind: Default::default(), - } - } -} - impl Deref for Action { type Target = action::Action; @@ -757,6 +755,20 @@ impl Action { pub(crate) fn steps(&self) -> CompositeSteps<'_> { CompositeSteps::new(self) } + + /// Returns this action's [`SymbolicLocation`]. + /// + /// See [`Workflow::location`] for an explanation of why this isn't + /// implemented through the [`Locatable`] trait. + pub(crate) fn location(&self) -> SymbolicLocation<'_> { + SymbolicLocation { + key: &self.key, + annotation: "this action".to_string(), + link: None, + route: Route::new(), + kind: Default::default(), + } + } } /// An iterable container for steps within a [`Job`]. @@ -810,14 +822,14 @@ impl<'a> Deref for CompositeStep<'a> { } } -impl<'a, 'doc> Locatable<'a, 'doc> for CompositeStep<'doc> { - fn location(&'a self) -> SymbolicLocation<'doc> { +impl<'doc> Locatable<'doc> for CompositeStep<'doc> { + fn location(&self) -> SymbolicLocation<'doc> { self.parent .location() .with_keys(&["runs".into(), "steps".into(), self.index.into()]) } - fn location_with_name(&'a self) -> SymbolicLocation<'doc> { + fn location_with_name(&self) -> SymbolicLocation<'doc> { match self.inner.name { Some(_) => self.location().with_keys(&["name".into()]), None => self.location(), @@ -826,7 +838,11 @@ impl<'a, 'doc> Locatable<'a, 'doc> for CompositeStep<'doc> { } } -impl<'doc> StepCommon<'_, 'doc> for CompositeStep<'doc> { +impl<'doc> StepCommon<'doc> for CompositeStep<'doc> { + fn index(&self) -> usize { + self.index + } + fn env_is_static(&self, name: &str) -> bool { utils::env_is_static(name, &[&self.env]) } diff --git a/crates/zizmor/src/models/coordinate.rs b/crates/zizmor/src/models/coordinate.rs index a346564c..954e2e0e 100644 --- a/crates/zizmor/src/models/coordinate.rs +++ b/crates/zizmor/src/models/coordinate.rs @@ -44,7 +44,7 @@ impl ActionCoordinate { /// while the `Some(_)` variants indicate various (potential) usages (such as being implicitly /// enabled, or explicitly enabled, or potentially enabled by a template expansion that /// can't be directly analyzed). - pub(crate) fn usage<'a, 'doc>(&self, step: &impl StepCommon<'a, 'doc>) -> Option { + pub(crate) fn usage<'doc>(&self, step: &impl StepCommon<'doc>) -> Option { let uses_pattern = self.uses_pattern(); let StepBodyCommon::Uses { @@ -336,7 +336,7 @@ mod tests { }; // Test-only trait impls. - impl<'a, 'doc> Locatable<'a, 'doc> for Step { + impl<'doc> Locatable<'doc> for Step { fn location(&self) -> crate::models::SymbolicLocation<'doc> { unreachable!() } @@ -346,7 +346,11 @@ mod tests { } } - impl<'a, 'doc> StepCommon<'a, 'doc> for Step { + impl<'doc> StepCommon<'doc> for Step { + fn index(&self) -> usize { + unreachable!() + } + fn env_is_static(&self, _name: &str) -> bool { unreachable!() } diff --git a/crates/zizmor/tests/integration/snapshot.rs b/crates/zizmor/tests/integration/snapshot.rs index b8b8c192..1386aab7 100644 --- a/crates/zizmor/tests/integration/snapshot.rs +++ b/crates/zizmor/tests/integration/snapshot.rs @@ -58,6 +58,13 @@ fn artipacked() -> Result<()> { .run()? ); + insta::assert_snapshot!( + zizmor() + .input(input_under_test("artipacked/demo-action/action.yml")) + .args(["--persona=auditor"]) + .run()? + ); + Ok(()) } diff --git a/crates/zizmor/tests/integration/snapshots/integration__snapshot__artipacked-5.snap b/crates/zizmor/tests/integration/snapshots/integration__snapshot__artipacked-5.snap new file mode 100644 index 00000000..ff64658a --- /dev/null +++ b/crates/zizmor/tests/integration/snapshots/integration__snapshot__artipacked-5.snap @@ -0,0 +1,27 @@ +--- +source: crates/zizmor/tests/integration/snapshot.rs +expression: "zizmor().input(input_under_test(\"artipacked/demo-action/action.yml\")).args([\"--persona=auditor\"]).run()?" +--- +warning[artipacked]: credential persistence through GitHub Actions artifacts + --> @@INPUT@@:9:7 + | + 9 | - name: true-positive-1 + | _______- +10 | | uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + | |__________________________________________________________________________________- does not set persist-credentials: false + | + = note: audit confidence → Low + +warning[artipacked]: credential persistence through GitHub Actions artifacts + --> @@INPUT@@:12:7 + | +12 | - name: true-positive-2-pedantic + | _______- +13 | | uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 +14 | | with: +15 | | persist-credentials: true + | |__________________________________- does not set persist-credentials: false + | + = note: audit confidence → Low + +2 findings: 0 unknown, 0 informational, 0 low, 2 medium, 0 high diff --git a/crates/zizmor/tests/integration/test-data/artipacked/demo-action/action.yml b/crates/zizmor/tests/integration/test-data/artipacked/demo-action/action.yml new file mode 100644 index 00000000..10cf3abd --- /dev/null +++ b/crates/zizmor/tests/integration/test-data/artipacked/demo-action/action.yml @@ -0,0 +1,15 @@ +# demo of a composite action being flagged by artipacked + +name: artipacked-composite-action +description: artipacked-composite-action + +runs: + using: composite + steps: + - name: true-positive-1 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + + - name: true-positive-2-pedantic + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + with: + persist-credentials: true diff --git a/docs/release-notes.md b/docs/release-notes.md index 43d8266f..bbf25003 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -9,6 +9,11 @@ of `zizmor`. ## Next (UNRELEASED) +### Enhancements 🌱 + +* The [artipacked] audit now produces findings on composite action definitions, + rather than just workflow definitions (#896) + ### Bug Fixes 🐛 * The [template-injection] audit no longer crashes when attempting to