diff --git a/crates/zizmor/src/audit/dependabot_cooldown.rs b/crates/zizmor/src/audit/dependabot_cooldown.rs index afec8f62..db2e46cf 100644 --- a/crates/zizmor/src/audit/dependabot_cooldown.rs +++ b/crates/zizmor/src/audit/dependabot_cooldown.rs @@ -1,7 +1,8 @@ use crate::{ audit::{Audit, audit_meta}, - finding::{Confidence, Severity, location::Locatable as _}, + finding::{Confidence, Fix, FixDisposition, Severity, location::Locatable as _}, }; +use yamlpatch::{Op, Patch}; audit_meta!( DependabotCooldown, @@ -11,6 +12,67 @@ audit_meta!( pub(crate) struct DependabotCooldown; +impl DependabotCooldown { + /// Creates a fix that adds default-days to an existing cooldown block + fn create_add_default_days_fix<'doc>( + update: crate::models::dependabot::Update<'doc>, + ) -> Fix<'doc> { + Fix { + title: "add default-days to cooldown".to_string(), + key: update.location().key, + disposition: FixDisposition::Safe, + patches: vec![Patch { + route: update.location().route.with_keys(["cooldown".into()]), + operation: Op::Add { + key: "default-days".to_string(), + value: serde_yaml::Value::Number(7.into()), + }, + }], + } + } + + /// Creates a fix that increases an insufficient default-days value + fn create_increase_default_days_fix<'doc>( + update: crate::models::dependabot::Update<'doc>, + ) -> Fix<'doc> { + Fix { + title: "increase default-days to 7".to_string(), + key: update.location().key, + disposition: FixDisposition::Safe, + patches: vec![Patch { + route: update + .location() + .route + .with_keys(["cooldown".into(), "default-days".into()]), + operation: Op::Replace(serde_yaml::Value::Number(7.into())), + }], + } + } + + /// Creates a fix that adds a cooldown block with default-days + fn create_add_cooldown_fix<'doc>(update: crate::models::dependabot::Update<'doc>) -> Fix<'doc> { + Fix { + title: "add cooldown configuration".to_string(), + key: update.location().key, + disposition: FixDisposition::Safe, + patches: vec![Patch { + route: update.location().route, + operation: Op::Add { + key: "cooldown".to_string(), + value: serde_yaml::Value::Mapping({ + let mut map = serde_yaml::Mapping::new(); + map.insert( + serde_yaml::Value::String("default-days".to_string()), + serde_yaml::Value::Number(7.into()), + ); + map + }), + }, + }], + } + } +} + impl Audit for DependabotCooldown { fn new(_state: &crate::state::AuditState) -> Result where @@ -45,6 +107,7 @@ impl Audit for DependabotCooldown { ) .confidence(Confidence::High) .severity(Severity::Medium) + .fix(Self::create_add_default_days_fix(update)) .build(dependabot)?, ), // We currently (arbitrarily) consider cooldowns under 4 days @@ -63,6 +126,7 @@ impl Audit for DependabotCooldown { ) .confidence(Confidence::Medium) .severity(Severity::Low) + .fix(Self::create_increase_default_days_fix(update)) .build(dependabot)?, ), Some(_) => {} @@ -77,6 +141,7 @@ impl Audit for DependabotCooldown { ) .confidence(Confidence::High) .severity(Severity::Medium) + .fix(Self::create_add_cooldown_fix(update)) .build(dependabot)?, ), } @@ -85,3 +150,245 @@ impl Audit for DependabotCooldown { Ok(findings) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + config::Config, + models::{AsDocument, dependabot::Dependabot}, + registry::input::InputKey, + state::AuditState, + }; + + /// Macro for testing dependabot audits with common boilerplate + macro_rules! test_dependabot_audit { + ($audit_type:ty, $filename:expr, $dependabot_content:expr, $test_fn:expr) => {{ + let key = InputKey::local("fakegroup".into(), $filename, None::<&str>).unwrap(); + let dependabot = Dependabot::from_string($dependabot_content.to_string(), key).unwrap(); + let audit_state = AuditState::default(); + let audit = <$audit_type>::new(&audit_state).unwrap(); + let findings = audit + .audit_dependabot(&dependabot, &Config::default()) + .unwrap(); + + $test_fn(&dependabot, findings) + }}; + } + + #[test] + fn test_fix_missing_cooldown() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny +"#; + + test_dependabot_audit!( + DependabotCooldown, + "test_fix_missing_cooldown.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert!(!findings.is_empty(), "Expected findings but got none"); + let finding = &findings[0]; + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + + let fix = &finding.fixes[0]; + let fixed_document = fix.apply(dependabot.as_document()).unwrap(); + insta::assert_snapshot!(fixed_document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny + cooldown: + default-days: 7 + "); + } + ); + } + + #[test] + fn test_fix_missing_default_days() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + cooldown: {} + schedule: + interval: daily + insecure-external-code-execution: deny +"#; + + test_dependabot_audit!( + DependabotCooldown, + "test_fix_missing_default_days.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert!(!findings.is_empty(), "Expected findings but got none"); + let finding = &findings[0]; + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + + let fix = &finding.fixes[0]; + let fixed_document = fix.apply(dependabot.as_document()).unwrap(); + insta::assert_snapshot!(fixed_document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + cooldown: { default-days: 7 } + schedule: + interval: daily + insecure-external-code-execution: deny + "); + } + ); + } + + #[test] + fn test_fix_insufficient_default_days() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + cooldown: + default-days: 2 + schedule: + interval: daily + insecure-external-code-execution: deny +"#; + + test_dependabot_audit!( + DependabotCooldown, + "test_fix_insufficient_default_days.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert!(!findings.is_empty(), "Expected findings but got none"); + let finding = &findings[0]; + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + + let fix = &finding.fixes[0]; + let fixed_document = fix.apply(dependabot.as_document()).unwrap(); + insta::assert_snapshot!(fixed_document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + cooldown: + default-days: 7 + schedule: + interval: daily + insecure-external-code-execution: deny + "); + } + ); + } + + #[test] + fn test_fix_multiple_updates() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + + - package-ecosystem: npm + directory: / + cooldown: + default-days: 1 + schedule: + interval: weekly +"#; + + test_dependabot_audit!( + DependabotCooldown, + "test_fix_multiple_updates.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert_eq!(findings.len(), 2, "Expected 2 findings"); + + // Apply all fixes and snapshot the result + let mut document = dependabot.as_document().clone(); + for finding in &findings { + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + for fix in &finding.fixes { + document = fix.apply(&document).unwrap(); + } + } + + insta::assert_snapshot!(document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + cooldown: + default-days: 7 + + - package-ecosystem: npm + directory: / + cooldown: + default-days: 7 + schedule: + interval: weekly + "); + } + ); + } + + #[test] + fn test_no_fix_needed_for_sufficient_cooldown() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + cooldown: + default-days: 7 + schedule: + interval: daily +"#; + + test_dependabot_audit!( + DependabotCooldown, + "test_no_fix_needed.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert_eq!(findings.len(), 0, "Expected no findings"); + + // Verify the document remains unchanged + insta::assert_snapshot!(dependabot.as_document().source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + cooldown: + default-days: 7 + schedule: + interval: daily + "); + } + ); + } +} diff --git a/crates/zizmor/src/audit/dependabot_execution.rs b/crates/zizmor/src/audit/dependabot_execution.rs index f7f2ba03..e115b9a4 100644 --- a/crates/zizmor/src/audit/dependabot_execution.rs +++ b/crates/zizmor/src/audit/dependabot_execution.rs @@ -2,8 +2,9 @@ use github_actions_models::dependabot::v2::AllowDeny; use crate::{ audit::{Audit, audit_meta}, - finding::location::Locatable as _, + finding::{Fix, FixDisposition, location::Locatable as _}, }; +use yamlpatch::{Op, Patch}; audit_meta!( DependabotExecution, @@ -13,6 +14,24 @@ audit_meta!( pub(crate) struct DependabotExecution; +impl DependabotExecution { + /// Creates a fix that changes insecure-external-code-execution from allow to deny + fn create_set_deny_fix<'doc>(update: crate::models::dependabot::Update<'doc>) -> Fix<'doc> { + Fix { + title: "set insecure-external-code-execution to deny".to_string(), + key: update.location().key, + disposition: FixDisposition::Unsafe, + patches: vec![Patch { + route: update + .location() + .route + .with_keys(["insecure-external-code-execution".into()]), + operation: Op::Replace(serde_yaml::Value::String("deny".to_string())), + }], + } + } +} + impl Audit for DependabotExecution { fn new(_state: &crate::state::AuditState) -> Result where @@ -42,6 +61,7 @@ impl Audit for DependabotExecution { .annotated("enabled here"), ) .add_location(update.location_with_name()) + .fix(Self::create_set_deny_fix(update)) .build(dependabot)?, ); } @@ -50,3 +70,190 @@ impl Audit for DependabotExecution { Ok(findings) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + config::Config, + models::{AsDocument, dependabot::Dependabot}, + registry::input::InputKey, + state::AuditState, + }; + + /// Macro for testing dependabot audits with common boilerplate + macro_rules! test_dependabot_audit { + ($audit_type:ty, $filename:expr, $dependabot_content:expr, $test_fn:expr) => {{ + let key = InputKey::local("fakegroup".into(), $filename, None::<&str>).unwrap(); + let dependabot = Dependabot::from_string($dependabot_content.to_string(), key).unwrap(); + let audit_state = AuditState::default(); + let audit = <$audit_type>::new(&audit_state).unwrap(); + let findings = audit + .audit_dependabot(&dependabot, &Config::default()) + .unwrap(); + + $test_fn(&dependabot, findings) + }}; + } + + #[test] + fn test_fix_allow_to_deny() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: allow +"#; + + test_dependabot_audit!( + DependabotExecution, + "test_fix_allow_to_deny.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert!(!findings.is_empty(), "Expected findings but got none"); + let finding = &findings[0]; + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + + let fix = &finding.fixes[0]; + let fixed_document = fix.apply(dependabot.as_document()).unwrap(); + insta::assert_snapshot!(fixed_document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny + "); + } + ); + } + + #[test] + fn test_no_fix_needed_for_deny() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny +"#; + + test_dependabot_audit!( + DependabotExecution, + "test_no_fix_needed_for_deny.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert_eq!(findings.len(), 0, "Expected no findings"); + + // Verify the document remains unchanged + insta::assert_snapshot!(dependabot.as_document().source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny + "); + } + ); + } + + #[test] + fn test_no_fix_needed_when_omitted() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily +"#; + + test_dependabot_audit!( + DependabotExecution, + "test_no_fix_needed_when_omitted.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert_eq!(findings.len(), 0, "Expected no findings"); + + // Verify the document remains unchanged + insta::assert_snapshot!(dependabot.as_document().source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + "); + } + ); + } + + #[test] + fn test_fix_multiple_updates() { + let dependabot_content = r#" +version: 2 + +updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: allow + + - package-ecosystem: npm + directory: / + schedule: + interval: weekly + insecure-external-code-execution: allow +"#; + + test_dependabot_audit!( + DependabotExecution, + "test_fix_multiple_updates.yml", + dependabot_content, + |dependabot: &Dependabot, findings: Vec| { + assert_eq!(findings.len(), 2, "Expected 2 findings"); + + // Apply all fixes and snapshot the result + let mut document = dependabot.as_document().clone(); + for finding in &findings { + assert!(!finding.fixes.is_empty(), "Expected fixes but got none"); + for fix in &finding.fixes { + document = fix.apply(&document).unwrap(); + } + } + + insta::assert_snapshot!(document.source(), @r" + version: 2 + + updates: + - package-ecosystem: pip + directory: / + schedule: + interval: daily + insecure-external-code-execution: deny + + - package-ecosystem: npm + directory: / + schedule: + interval: weekly + insecure-external-code-execution: deny + "); + } + ); + } +} diff --git a/crates/zizmor/tests/integration/snapshot.rs b/crates/zizmor/tests/integration/snapshot.rs index 5bdffc18..5fa384c9 100644 --- a/crates/zizmor/tests/integration/snapshot.rs +++ b/crates/zizmor/tests/integration/snapshot.rs @@ -979,8 +979,9 @@ fn dependabot_execution() -> Result<()> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ enabled here | = note: audit confidence → High + = note: this finding has an auto-fix - 1 finding: 0 informational, 0 low, 0 medium, 1 high + 1 findings (1 fixable): 0 informational, 0 low, 0 medium, 1 high " ); @@ -1003,8 +1004,9 @@ fn dependabot_cooldown() -> Result<()> { | ^^^^^^^^^^^^^^^^^^^^^^ missing cooldown configuration | = note: audit confidence → High + = note: this finding has an auto-fix - 1 finding: 0 informational, 0 low, 1 medium, 0 high + 1 findings (1 fixable): 0 informational, 0 low, 1 medium, 0 high " ); @@ -1022,8 +1024,9 @@ fn dependabot_cooldown() -> Result<()> { | ^^^^^^^^^^^^ no default-days configured | = note: audit confidence → High + = note: this finding has an auto-fix - 1 finding: 0 informational, 0 low, 1 medium, 0 high + 1 findings (1 fixable): 0 informational, 0 low, 1 medium, 0 high "); insta::assert_snapshot!( @@ -1040,8 +1043,9 @@ fn dependabot_cooldown() -> Result<()> { | ^^^^^^^^^^^^^^^ insufficient default-days configured | = note: audit confidence → Medium + = note: this finding has an auto-fix - 1 finding: 0 informational, 1 low, 0 medium, 0 high + 1 findings (1 fixable): 0 informational, 1 low, 0 medium, 0 high "); Ok(()) diff --git a/docs/audits.md b/docs/audits.md index 520ecb1f..f955a355 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -377,7 +377,7 @@ Some general pointers: | Type | Examples | Introduced in | Works offline | Auto-fixes available | Configurable | |----------|-------------------------|---------------|----------------|--------------------| ---------------| -| Dependabot | [dependabot-cooldown/] | v1.15.0 | ✅ | ❌ | ❌ | +| Dependabot | [dependabot-cooldown/] | v1.15.0 | ✅ | ✅ | ❌ | [dependabot-cooldown/]: https://github.com/zizmorcore/zizmor/blob/main/crates/zizmor/tests/integration/test-data/dependabot-cooldown/ @@ -443,7 +443,7 @@ enforces the following minimums: | Type | Examples | Introduced in | Works offline | Auto-fixes available | Configurable | |----------|-------------------------|---------------|----------------|--------------------| ---------------| -| Dependabot | [dependabot-execution/] | v1.15.0 | ✅ | ❌ | ❌ | +| Dependabot | [dependabot-execution/] | v1.15.0 | ✅ | ✅ | ❌ | [dependabot-execution/]: https://github.com/zizmorcore/zizmor/blob/main/crates/zizmor/tests/integration/test-data/dependabot-execution/ diff --git a/docs/release-notes.md b/docs/release-notes.md index c9eec5b1..db4f4b58 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -32,6 +32,16 @@ of `zizmor`. Many thanks to @cnaples79 for implementing this improvement! +* The [dependabot-cooldown] audit now supports auto-fixes for many findings + (#1229) + + Many thanks to @mostafa for implementing this improvement! + +* The [dependabot-execution] audit now supports auto-fixes for many findings + (#1229) + + Many thanks to @mostafa for implementing this improvement! + ## 1.15.2 ### Bug Fixes 🐛