From 3a0d5e30127b007b0ae03e20a6d84493c2bd15fa Mon Sep 17 00:00:00 2001 From: Ubiratan Soares Date: Fri, 22 Nov 2024 16:59:12 +0100 Subject: [PATCH 1/5] feat: supporting ignores through inlined YAML comments --- src/main.rs | 5 ++-- src/models.rs | 8 +++-- src/registry.rs | 46 +++++++++++++++++++++++------ tests/acceptance.rs | 17 +++++++++++ tests/test-data/inlined-ignores.yml | 28 ++++++++++++++++++ 5 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 tests/test-data/inlined-ignores.yml diff --git a/src/main.rs b/src/main.rs index 430c7658..05e16414 100644 --- a/src/main.rs +++ b/src/main.rs @@ -177,12 +177,13 @@ fn run() -> Result { workflow = workflow.filename().cyan() )); for (name, audit) in audit_registry.iter_workflow_audits() { - results.extend(audit.audit(workflow).with_context(|| { + let findings = audit.audit(workflow).with_context(|| { format!( "{name} failed on {workflow}", workflow = workflow.filename() ) - })?); + })?; + results.extend(workflow, findings); bar.inc(1); } bar.println(format!( diff --git a/src/models.rs b/src/models.rs index 7222b6db..60e04693 100644 --- a/src/models.rs +++ b/src/models.rs @@ -17,6 +17,7 @@ use crate::finding::{Route, SymbolicLocation}; /// providing access to the underlying data model. pub(crate) struct Workflow { pub(crate) path: String, + pub(crate) contents: String, pub(crate) document: yamlpath::Document, inner: workflow::Workflow, } @@ -32,12 +33,12 @@ impl Deref for Workflow { impl Workflow { /// Load a workflow from the given file on disk. pub(crate) fn from_file>(p: P) -> Result { - let raw = std::fs::read_to_string(p.as_ref())?; + let contents = std::fs::read_to_string(p.as_ref())?; - let inner = serde_yaml::from_str(&raw) + let inner = serde_yaml::from_str(&contents) .with_context(|| format!("invalid GitHub Actions workflow: {:?}", p.as_ref()))?; - let document = yamlpath::Document::new(raw)?; + let document = yamlpath::Document::new(&contents)?; Ok(Self { path: p @@ -45,6 +46,7 @@ impl Workflow { .to_str() .ok_or_else(|| anyhow!("invalid workflow: path is not UTF-8"))? .to_string(), + contents, document, inner, }) diff --git a/src/registry.rs b/src/registry.rs index 778d96a9..a8d699cb 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -3,14 +3,13 @@ use std::{collections::HashMap, path::Path, process::ExitCode}; -use anyhow::{anyhow, Context, Result}; - use crate::{ audit::WorkflowAudit, config::Config, finding::{Finding, Severity}, models::Workflow, }; +use anyhow::{anyhow, Context, Result}; pub(crate) struct WorkflowRegistry { pub(crate) workflows: HashMap, @@ -129,21 +128,23 @@ impl<'a> FindingRegistry<'a> { /// Adds one or more findings to the current findings set, /// filtering with the configuration in the process. - pub(crate) fn extend(&mut self, results: Vec>) { + pub(crate) fn extend(&mut self, workflow: &Workflow, results: Vec>) { // TODO: is it faster to iterate like this, or do `find_by_max` // and then `extend`? - for result in results { - if self.config.ignores(&result) { - self.ignored.push(result); + for finding in results { + if self.config.ignores(&finding) + || self.ignored_from_inlined_comment(workflow, &finding) + { + self.ignored.push(finding); } else { if self .highest_severity - .map_or(true, |s| result.determinations.severity > s) + .map_or(true, |s| finding.determinations.severity > s) { - self.highest_severity = Some(result.determinations.severity); + self.highest_severity = Some(finding.determinations.severity); } - self.findings.push(result); + self.findings.push(finding); } } } @@ -157,6 +158,33 @@ impl<'a> FindingRegistry<'a> { pub(crate) fn ignored(&self) -> &[Finding<'a>] { &self.ignored } + + fn ignored_from_inlined_comment(&self, workflow: &Workflow, finding: &Finding) -> bool { + let document_lines = &workflow.contents.split("\n").collect::>(); + let line_ranges = finding + .locations + .iter() + .map(|l| { + ( + l.concrete.location.start_point.row, + l.concrete.location.end_point.row, + ) + }) + .collect::>(); + + let inlined_ignore = format!("zizmor: ignore[{}]", finding.ident); + for (start, end) in line_ranges { + for document_line in start..(end + 1) { + if let Some(line) = document_lines.get(document_line) { + if line.contains(&inlined_ignore) { + return true; + } + } + } + } + + false + } } impl From> for ExitCode { diff --git a/tests/acceptance.rs b/tests/acceptance.rs index dba00b5c..d1acd9e1 100644 --- a/tests/acceptance.rs +++ b/tests/acceptance.rs @@ -35,6 +35,23 @@ fn assert_value_match(json: &Value, path_pattern: &str, value: &str) { assert!(queried.to_string().contains(value)); } +#[test] +fn catches_inlined_ignore() -> anyhow::Result<()> { + let auditable = workflow_under_test("inlined-ignores.yml"); + + let cli_args = [&auditable]; + + let execution = zizmor().args(cli_args).output()?; + + assert_eq!(execution.status.code(), Some(0)); + + let findings = String::from_utf8(execution.stdout)?; + + assert_eq!(&findings, "[]"); + + Ok(()) +} + #[test] fn audit_artipacked() -> anyhow::Result<()> { let auditable = workflow_under_test("artipacked.yml"); diff --git a/tests/test-data/inlined-ignores.yml b/tests/test-data/inlined-ignores.yml new file mode 100644 index 00000000..38f4bdb6 --- /dev/null +++ b/tests/test-data/inlined-ignores.yml @@ -0,0 +1,28 @@ +on: + push: + branches: + - master + workflow_dispatch: + +jobs: + artipacked-ignored: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 # zizmor: ignore[artipacked] + + insecure-commands-ignored: + runs-on: ubuntu-latest + env: + ACTIONS_ALLOW_UNSECURE_COMMANDS: yes # zizmor: ignore[insecure-commands] + steps: + - run: echo "I shall pass!" + + hardcoded-credentials-ignored: + runs-on: ubuntu-latest + container: + image: fake.example.com/example + credentials: + username: user + password: hackme # zizmor: ignore[hardcoded-container-credentials] + steps: + - run: echo 'This is a honeypot actually!' From 0e76cf8b96ccea884f4d806efc0a5061c5daf441 Mon Sep 17 00:00:00 2001 From: Ubiratan Soares Date: Fri, 22 Nov 2024 21:02:51 +0100 Subject: [PATCH 2/5] feat: another take --- src/finding/mod.rs | 47 +++++++++++++++++++++++++++++++++++++++++----- src/main.rs | 2 +- src/registry.rs | 33 ++------------------------------ 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 3c8b80ca..c6a8b29f 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -222,6 +222,7 @@ pub(crate) struct Finding<'w> { pub(crate) url: &'static str, pub(crate) determinations: Determinations, pub(crate) locations: Vec>, + pub(crate) ignored_from_inlined_comment: bool, } pub(crate) struct FindingBuilder<'w> { @@ -261,6 +262,14 @@ impl<'w> FindingBuilder<'w> { } pub(crate) fn build(self, workflow: &'w Workflow) -> Result> { + let locations = self + .locations + .iter() + .map(|l| l.clone().concretize(workflow)) + .collect::>>()?; + + let should_ignore = self.ignored_from_inlined_comment(workflow, &locations, self.ident); + Ok(Finding { ident: self.ident, desc: self.desc, @@ -269,11 +278,39 @@ impl<'w> FindingBuilder<'w> { confidence: self.confidence, severity: self.severity, }, - locations: self - .locations - .into_iter() - .map(|l| l.concretize(workflow)) - .collect::>>()?, + locations, + ignored_from_inlined_comment: should_ignore, }) } + + fn ignored_from_inlined_comment( + &self, + workflow: &Workflow, + locations: &[Location], + id: &str, + ) -> bool { + let document_lines = &workflow.contents.lines().collect::>(); + let line_ranges = locations + .iter() + .map(|l| { + ( + l.concrete.location.start_point.row, + l.concrete.location.end_point.row, + ) + }) + .collect::>(); + + let inlined_ignore = format!("zizmor: ignore[{}]", id); + for (start, end) in line_ranges { + for document_line in start..(end + 1) { + if let Some(line) = document_lines.get(document_line) { + if line.contains(&inlined_ignore) { + return true; + } + } + } + } + + false + } } diff --git a/src/main.rs b/src/main.rs index 05e16414..cfcd14d8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -183,7 +183,7 @@ fn run() -> Result { workflow = workflow.filename() ) })?; - results.extend(workflow, findings); + results.extend(findings); bar.inc(1); } bar.println(format!( diff --git a/src/registry.rs b/src/registry.rs index a8d699cb..ff765715 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -128,13 +128,11 @@ impl<'a> FindingRegistry<'a> { /// Adds one or more findings to the current findings set, /// filtering with the configuration in the process. - pub(crate) fn extend(&mut self, workflow: &Workflow, results: Vec>) { + pub(crate) fn extend(&mut self, results: Vec>) { // TODO: is it faster to iterate like this, or do `find_by_max` // and then `extend`? for finding in results { - if self.config.ignores(&finding) - || self.ignored_from_inlined_comment(workflow, &finding) - { + if self.config.ignores(&finding) || finding.ignored_from_inlined_comment { self.ignored.push(finding); } else { if self @@ -158,33 +156,6 @@ impl<'a> FindingRegistry<'a> { pub(crate) fn ignored(&self) -> &[Finding<'a>] { &self.ignored } - - fn ignored_from_inlined_comment(&self, workflow: &Workflow, finding: &Finding) -> bool { - let document_lines = &workflow.contents.split("\n").collect::>(); - let line_ranges = finding - .locations - .iter() - .map(|l| { - ( - l.concrete.location.start_point.row, - l.concrete.location.end_point.row, - ) - }) - .collect::>(); - - let inlined_ignore = format!("zizmor: ignore[{}]", finding.ident); - for (start, end) in line_ranges { - for document_line in start..(end + 1) { - if let Some(line) = document_lines.get(document_line) { - if line.contains(&inlined_ignore) { - return true; - } - } - } - } - - false - } } impl From> for ExitCode { From 5cfd0ae5e8911f0a2293f27d3c49386b8486f0eb Mon Sep 17 00:00:00 2001 From: Ubiratan Soares Date: Sat, 23 Nov 2024 20:23:02 +0100 Subject: [PATCH 3/5] feat: some improvements --- src/finding/mod.rs | 6 +++--- src/main.rs | 5 ++--- src/models.rs | 2 -- src/registry.rs | 2 +- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/finding/mod.rs b/src/finding/mod.rs index c6a8b29f..2b2715d0 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -222,7 +222,7 @@ pub(crate) struct Finding<'w> { pub(crate) url: &'static str, pub(crate) determinations: Determinations, pub(crate) locations: Vec>, - pub(crate) ignored_from_inlined_comment: bool, + pub(crate) ignored: bool, } pub(crate) struct FindingBuilder<'w> { @@ -279,7 +279,7 @@ impl<'w> FindingBuilder<'w> { severity: self.severity, }, locations, - ignored_from_inlined_comment: should_ignore, + ignored: should_ignore, }) } @@ -289,7 +289,7 @@ impl<'w> FindingBuilder<'w> { locations: &[Location], id: &str, ) -> bool { - let document_lines = &workflow.contents.lines().collect::>(); + let document_lines = &workflow.document.source().lines().collect::>(); let line_ranges = locations .iter() .map(|l| { diff --git a/src/main.rs b/src/main.rs index cfcd14d8..430c7658 100644 --- a/src/main.rs +++ b/src/main.rs @@ -177,13 +177,12 @@ fn run() -> Result { workflow = workflow.filename().cyan() )); for (name, audit) in audit_registry.iter_workflow_audits() { - let findings = audit.audit(workflow).with_context(|| { + results.extend(audit.audit(workflow).with_context(|| { format!( "{name} failed on {workflow}", workflow = workflow.filename() ) - })?; - results.extend(findings); + })?); bar.inc(1); } bar.println(format!( diff --git a/src/models.rs b/src/models.rs index 60e04693..c5ed20bc 100644 --- a/src/models.rs +++ b/src/models.rs @@ -17,7 +17,6 @@ use crate::finding::{Route, SymbolicLocation}; /// providing access to the underlying data model. pub(crate) struct Workflow { pub(crate) path: String, - pub(crate) contents: String, pub(crate) document: yamlpath::Document, inner: workflow::Workflow, } @@ -46,7 +45,6 @@ impl Workflow { .to_str() .ok_or_else(|| anyhow!("invalid workflow: path is not UTF-8"))? .to_string(), - contents, document, inner, }) diff --git a/src/registry.rs b/src/registry.rs index ff765715..0ca5791e 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -132,7 +132,7 @@ impl<'a> FindingRegistry<'a> { // TODO: is it faster to iterate like this, or do `find_by_max` // and then `extend`? for finding in results { - if self.config.ignores(&finding) || finding.ignored_from_inlined_comment { + if self.config.ignores(&finding) || finding.ignored { self.ignored.push(finding); } else { if self From 0edaf5ba60f0ba0e850c5231b245546430d611fd Mon Sep 17 00:00:00 2001 From: Ubiratan Soares Date: Sat, 23 Nov 2024 23:14:41 +0100 Subject: [PATCH 4/5] feat: matching with rfind --- src/finding/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 2b2715d0..c1388286 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -304,7 +304,7 @@ impl<'w> FindingBuilder<'w> { for (start, end) in line_ranges { for document_line in start..(end + 1) { if let Some(line) = document_lines.get(document_line) { - if line.contains(&inlined_ignore) { + if line.rfind(&inlined_ignore).is_some() { return true; } } From 73e38bb8ecc7f36357a98bf55b299f6b01545ec8 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sat, 23 Nov 2024 17:23:32 -0500 Subject: [PATCH 5/5] Update src/finding/mod.rs --- src/finding/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/finding/mod.rs b/src/finding/mod.rs index c1388286..12407e71 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -300,7 +300,7 @@ impl<'w> FindingBuilder<'w> { }) .collect::>(); - let inlined_ignore = format!("zizmor: ignore[{}]", id); + let inlined_ignore = format!("# zizmor: ignore[{}]", id); for (start, end) in line_ranges { for document_line in start..(end + 1) { if let Some(line) = document_lines.get(document_line) {