Repository: woodruffw/zizmor (language: python)

Issue:
Return different codes based on findings
We'd like to use zizmor to gate PRs and prevent GHA code from being merged if it has serious security bugs. Currently, I am outputting the json and then using jq to parse the output and then checking the result in bash. 

What would be more ideal is to use zizmor's return code (`$?`). I think most other linters, scanners, etc return non-zero codes based on results. I might suggest returning 0 for any low/medium priority findings and 1 for high. And then maybe in pedantic mode, returning 1 for any findings.

Great tool by the way. We have been looking for something like this for a while and I'm trying to learn rust so I can contribute.


Discussion / hints:
Thanks for the issue @daviddavis, and for the kind words! 

I agree, this would be a great feature to add -- I'd be happy to have it return with the following codes:

1. `0`: no warnings/errors at all;
2. `11`: all warnings are "low" or below;
3. `12`: all warnings are "medium" or below;
4. `13`: there's at least one "high"

(The reason I suggest starting at `11` is because I don't want to shadow any existing - incidental - low error code behavior: I think currently we exit with `1` on any internal tool failures.)

If you'd be interested in sending a PR for this, I'd be happy to review it! Otherwise, I can tackle this myself in the next day or so.
(Also thinking out loud: I'm not 100% sure if exit codes should be used with `--format=sarif` -- some tools might treat an exit code in that case as an error within the tool itself. But we can start out by making exit codes universal and seeing what happens 🙂)
That all sounds good to me. 👍 

Fix patch:
diff --git a/docs/usage.md b/docs/usage.md
index 20bf3dc..6c17249 100644
--- a/docs/usage.md
+++ b/docs/usage.md
@@ -38,6 +38,22 @@ zizmor --format sarif
 
 See [Integration](#integration) for suggestions on when to use each format.
 
+## Exit codes
+
+`zizmor` uses various exit codes to summarize the results of a run:
+
+| Code | Meaning |
+| ---- | ------- |
+| 0    | Successful audit; no findings to report. |
+| 1    | Error during audit; consult output. |
+| 10   | One or more findings found; highest finding is "unknown" level. |
+| 11   | One or more findings found; highest finding is "informational" level. |
+| 12   | One or more findings found; highest finding is "low" level. |
+| 13   | One or more findings found; highest finding is "medium" level. |
+| 14   | One or more findings found; highest finding is "high" level. |
+
+All other exit codes are currently reserved.
+
 ## Ignoring results
 
 `zizmor`'s defaults are not always 100% right for every possible use case.
diff --git a/src/finding/mod.rs b/src/finding/mod.rs
index 8f32908..fb81695 100644
--- a/src/finding/mod.rs
+++ b/src/finding/mod.rs
@@ -22,7 +22,7 @@ pub(crate) enum Confidence {
     High,
 }
 
-#[derive(Copy, Clone, Debug, Default, Eq, Hash, PartialEq, Serialize)]
+#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialOrd, PartialEq, Serialize)]
 pub(crate) enum Severity {
     #[default]
     Unknown,
diff --git a/src/main.rs b/src/main.rs
index 53d9180..bd8f179 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,4 +1,4 @@
-use std::{io::stdout, path::PathBuf, time::Duration};
+use std::{io::stdout, path::PathBuf, process::ExitCode, time::Duration};
 
 use anyhow::{anyhow, Context, Result};
 use audit::WorkflowAudit;
@@ -6,7 +6,7 @@ use clap::{Parser, ValueEnum};
 use config::Config;
 use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle};
 use owo_colors::OwoColorize;
-use registry::{AuditRegistry, WorkflowRegistry};
+use registry::{AuditRegistry, FindingRegistry, WorkflowRegistry};
 use state::AuditState;
 
 mod audit;
@@ -66,7 +66,7 @@ pub(crate) enum OutputFormat {
     Sarif,
 }
 
-fn main() -> Result<()> {
+fn run() -> Result<ExitCode> {
     human_panic::setup_panic!();
 
     let args = App::parse();
@@ -156,7 +156,7 @@ fn main() -> Result<()> {
         );
     }
 
-    let mut results = vec![];
+    let mut results = FindingRegistry::new(&config);
     for (_, workflow) in workflow_registry.iter_workflows() {
         bar.set_message(format!(
             "auditing {workflow}",
@@ -177,10 +177,6 @@ fn main() -> Result<()> {
         ));
     }
 
-    // TODO: Suboptimal; should probably use `extract_if` once stabilized.
-    // See: https://github.com/rust-lang/rust/issues/43244
-    let (ignored, results): (Vec<_>, Vec<_>) = results.into_iter().partition(|r| config.ignores(r));
-
     bar.finish_and_clear();
 
     let format = match args.format {
@@ -189,11 +185,22 @@ fn main() -> Result<()> {
     };
 
     match format {
-        OutputFormat::Plain => render::render_findings(&workflow_registry, &results, &ignored),
-        OutputFormat::Json => serde_json::to_writer_pretty(stdout(), &results)?,
-        OutputFormat::Sarif => {
-            serde_json::to_writer_pretty(stdout(), &sarif::build(&workflow_registry, results))?
-        }
+        OutputFormat::Plain => render::render_findings(&workflow_registry, &results),
+        OutputFormat::Json => serde_json::to_writer_pretty(stdout(), &results.findings())?,
+        OutputFormat::Sarif => serde_json::to_writer_pretty(
+            stdout(),
+            &sarif::build(&workflow_registry, results.findings()),
+        )?,
     };
-    Ok(())
+
+    Ok(results.into())
+}
+
+fn main() -> ExitCode {
+    // This is a little silly, but returning an ExitCode like this ensures
+    // we always exit cleanly, rather than performing a hard process exit.
+    match run() {
+        Ok(exit) => exit,
+        Err(_) => ExitCode::FAILURE,
+    }
 }
diff --git a/src/registry.rs b/src/registry.rs
index 54fe490..06b5eb0 100644
--- a/src/registry.rs
+++ b/src/registry.rs
@@ -1,11 +1,16 @@
 //! Functionality for registering and managing the lifecycles of
 //! audits.
 
-use std::{collections::HashMap, path::Path};
+use std::{collections::HashMap, path::Path, process::ExitCode};
 
 use anyhow::{anyhow, Result};
 
-use crate::{audit::WorkflowAudit, models::Workflow};
+use crate::{
+    audit::WorkflowAudit,
+    config::Config,
+    finding::{Finding, Severity},
+    models::Workflow,
+};
 
 pub(crate) struct WorkflowRegistry {
     pub(crate) workflows: HashMap<String, Workflow>,
@@ -100,3 +105,68 @@ impl AuditRegistry {
         self.workflow_audits.iter_mut()
     }
 }
+
+/// A registry of all findings discovered during a `zizmor` run.
+pub(crate) struct FindingRegistry<'a> {
+    config: &'a Config,
+    ignored: Vec<Finding<'a>>,
+    findings: Vec<Finding<'a>>,
+    highest_severity: Option<Severity>,
+}
+
+impl<'a> FindingRegistry<'a> {
+    pub(crate) fn new(config: &'a Config) -> Self {
+        Self {
+            config,
+            ignored: Default::default(),
+            findings: Default::default(),
+            highest_severity: None,
+        }
+    }
+
+    /// 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<Finding<'a>>) {
+        // 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);
+            } else {
+                if self
+                    .highest_severity
+                    .map_or(true, |s| result.determinations.severity > s)
+                {
+                    self.highest_severity = Some(result.determinations.severity);
+                }
+
+                self.findings.push(result);
+            }
+        }
+    }
+
+    /// All non-filtered findings.
+    pub(crate) fn findings(&self) -> &[Finding<'a>] {
+        &self.findings
+    }
+
+    /// All filtered findings.
+    pub(crate) fn ignored(&self) -> &[Finding<'a>] {
+        &self.ignored
+    }
+}
+
+impl From<FindingRegistry<'_>> for ExitCode {
+    fn from(value: FindingRegistry<'_>) -> Self {
+        match value.highest_severity {
+            Some(sev) => match sev {
+                Severity::Unknown => ExitCode::from(10),
+                Severity::Informational => ExitCode::from(11),
+                Severity::Low => ExitCode::from(12),
+                Severity::Medium => ExitCode::from(13),
+                Severity::High => ExitCode::from(14),
+            },
+            None => ExitCode::SUCCESS,
+        }
+    }
+}
diff --git a/src/render.rs b/src/render.rs
index 021b06f..0d1f9c9 100644
--- a/src/render.rs
+++ b/src/render.rs
@@ -4,7 +4,7 @@ use std::collections::{hash_map::Entry, HashMap};
 
 use crate::{
     finding::{Finding, Location, Severity},
-    registry::WorkflowRegistry,
+    registry::{FindingRegistry, WorkflowRegistry},
 };
 use annotate_snippets::{Level, Renderer, Snippet};
 use anstream::println;
@@ -66,26 +66,22 @@ pub(crate) fn finding_snippet<'w>(
     snippets
 }
 
-pub(crate) fn render_findings(
-    registry: &WorkflowRegistry,
-    findings: &[Finding],
-    ignored: &[Finding],
-) {
-    for finding in findings {
+pub(crate) fn render_findings(registry: &WorkflowRegistry, findings: &FindingRegistry) {
+    for finding in findings.findings() {
         render_finding(registry, finding);
         println!();
     }
 
-    if findings.is_empty() {
+    if findings.findings().is_empty() {
         println!(
             "{no_findings} ({nignored} ignored)",
             no_findings = "No findings to report. Good job!".green(),
-            nignored = ignored.len().bright_yellow()
+            nignored = findings.ignored().len().bright_yellow()
         );
     } else {
         let mut findings_by_severity = HashMap::new();
 
-        for finding in findings {
+        for finding in findings.findings() {
             match findings_by_severity.entry(&finding.determinations.severity) {
                 Entry::Occupied(mut e) => {
                     *e.get_mut() += 1;
@@ -98,8 +94,8 @@ pub(crate) fn render_findings(
 
         println!(
             "{nfindings} findings ({nignored} ignored): {nunknown} unknown, {ninformational} informational, {nlow} low, {nmedium} medium, {nhigh} high",
-            nfindings = (findings.len() + ignored.len()).green(),
-            nignored = ignored.len().bright_yellow(),
+            nfindings = (findings.findings().len() + findings.ignored().len()).green(),
+            nignored = findings.ignored().len().bright_yellow(),
             nunknown = findings_by_severity.get(&Severity::Unknown).unwrap_or(&0),
             ninformational = findings_by_severity.get(&Severity::Informational).unwrap_or(&0).purple(),
             nlow = findings_by_severity.get(&Severity::Low).unwrap_or(&0).cyan(),
diff --git a/src/sarif.rs b/src/sarif.rs
index d15f98f..94b2c97 100644
--- a/src/sarif.rs
+++ b/src/sarif.rs
@@ -10,7 +10,7 @@ use crate::{
     registry::WorkflowRegistry,
 };
 
-pub(crate) fn build(registry: &WorkflowRegistry, findings: Vec<Finding<'_>>) -> Sarif {
+pub(crate) fn build(registry: &WorkflowRegistry, findings: &[Finding]) -> Sarif {
     Sarif::builder()
         .version("2.1.0")
         .schema("https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/schemas/sarif-external-property-file-schema-2.1.0.json")
@@ -18,7 +18,7 @@ pub(crate) fn build(registry: &WorkflowRegistry, findings: Vec<Finding<'_>>) ->
         .build()
 }
 
-fn build_run(registry: &WorkflowRegistry, findings: Vec<Finding<'_>>) -> Run {
+fn build_run(registry: &WorkflowRegistry, findings: &[Finding]) -> Run {
     Run::builder()
         .tool(
             Tool::builder()
@@ -37,7 +37,7 @@ fn build_run(registry: &WorkflowRegistry, findings: Vec<Finding<'_>>) -> Run {
         .build()
 }
 
-fn build_results(registry: &WorkflowRegistry, findings: Vec<Finding<'_>>) -> Vec<SarifResult> {
+fn build_results(registry: &WorkflowRegistry, findings: &[Finding]) -> Vec<SarifResult> {
     findings.iter().map(|f| build_result(registry, f)).collect()
 }
 


Test patch:
diff --git a/tests/acceptance.rs b/tests/acceptance.rs
index 87e0d42..0e125c0 100644
--- a/tests/acceptance.rs
+++ b/tests/acceptance.rs
@@ -40,9 +40,9 @@ fn audit_artipacked() -> anyhow::Result<()> {
     let auditable = workflow_under_test("artipacked.yml");
     let cli_args = [&auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(13));
 
     let findings = serde_json::from_slice(&execution.stdout)?;
 
@@ -61,9 +61,9 @@ fn audit_excessive_permission() -> anyhow::Result<()> {
     let auditable = workflow_under_test("excessive-permissions.yml");
     let cli_args = [&auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(14));
 
     let findings = serde_json::from_slice(&execution.stdout)?;
 
@@ -82,9 +82,9 @@ fn audit_hardcoded_credentials() -> anyhow::Result<()> {
     let auditable = workflow_under_test("hardcoded-credentials.yml");
     let cli_args = [&auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(14));
 
     let findings = serde_json::from_slice(&execution.stdout)?;
 
@@ -103,9 +103,9 @@ fn audit_template_injection() -> anyhow::Result<()> {
     let auditable = workflow_under_test("template-injection.yml");
     let cli_args = [&auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(14));
 
     let findings = serde_json::from_slice(&execution.stdout)?;
 
@@ -124,9 +124,9 @@ fn audit_use_trusted_publishing() -> anyhow::Result<()> {
     let auditable = workflow_under_test("use-trusted-publishing.yml");
     let cli_args = [&auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(11));
 
     let findings = serde_json::from_slice(&execution.stdout)?;
 
@@ -147,9 +147,9 @@ fn audit_self_hosted() -> anyhow::Result<()> {
     // Note : self-hosted audit is pedantic
     let cli_args = ["--pedantic", &auditable];
 
-    let execution = zizmor().args(cli_args).unwrap();
+    let execution = zizmor().args(cli_args).output()?;
 
-    assert!(execution.status.success());
+    assert_eq!(execution.status.code(), Some(10));
 
     let findings = serde_json::from_slice(&execution.stdout)?;