diff --git a/Cargo.lock b/Cargo.lock index 4d814ecd..f93939a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -545,10 +545,11 @@ checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" [[package]] name = "github-actions-models" -version = "0.11.0" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1106863433991623eb597aea98e60623ce119dbc190c9c2b3166cdb75c751b1f" +checksum = "0102922a92566de8ff25ff79144d6b30516efe941bc34ff849f01b4979add8e2" dependencies = [ + "indexmap", "serde", "serde_yaml", ] @@ -865,12 +866,13 @@ dependencies = [ [[package]] name = "indexmap" -version = "2.6.0" +version = "2.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "707907fe3c25f5424cce2cb7e1cbcafee6bdbe735ca90ef77c29e84591e5b9da" +checksum = "62f822373a4fe84d4bb149bf54e584a7f4abec90e072ed49cda0edea5b95471f" dependencies = [ "equivalent", "hashbrown", + "serde", ] [[package]] @@ -2578,6 +2580,7 @@ dependencies = [ "env_logger", "github-actions-models", "human-panic", + "indexmap", "indicatif", "insta", "itertools", diff --git a/Cargo.toml b/Cargo.toml index 28167374..1280ad1c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,8 +19,9 @@ anyhow = "1.0.93" clap = { version = "4.5.21", features = ["derive", "env"] } clap-verbosity-flag = "3.0.0" env_logger = "0.11.5" -github-actions-models = "0.11.0" +github-actions-models = "0.12.0" human-panic = "2.0.1" +indexmap = "2.7.0" indicatif = "0.17.9" itertools = "0.13.0" log = "0.4.22" diff --git a/docs/snippets/help.txt b/docs/snippets/help.txt index 8689ab39..9aa7890d 100644 --- a/docs/snippets/help.txt +++ b/docs/snippets/help.txt @@ -3,34 +3,69 @@ Static analysis for GitHub Actions Usage: zizmor [OPTIONS] ... Arguments: - ... The workflow filenames or directories to audit + ... + The workflow filenames or directories to audit Options: -p, --pedantic - Emit findings even when the context suggests an explicit security decision made by the user + Emit 'pedantic' findings. + + This is an alias for --persona=pedantic. + + --persona + The persona to use while auditing + + [default: regular] + + Possible values: + - auditor: The "auditor" persona (false positives OK) + - pedantic: The "pedantic" persona (code smells OK) + - regular: The "regular" persona (minimal false positives) + -o, --offline Only perform audits that don't require network access + -v, --verbose... Increase logging verbosity + -q, --quiet... Decrease logging verbosity + -n, --no-progress Disable the progress bar. This is useful primarily when running with a high verbosity level, as the two will fight for stderr + --gh-token - The GitHub API token to use [env: GH_TOKEN=] + The GitHub API token to use + + [env: GH_TOKEN=] + --format - The output format to emit. By default, plain text will be emitted [possible values: plain, json, sarif] + The output format to emit. By default, plain text will be emitted + + [default: plain] + [possible values: plain, json, sarif] + -c, --config The configuration file to load. By default, any config will be discovered relative to $CWD + --no-config Disable all configuration loading + --no-exit-codes Disable all error codes besides success and tool failure + --min-severity - Filter all results below this severity [possible values: unknown, informational, low, medium, high] + Filter all results below this severity + + [possible values: unknown, informational, low, medium, high] + --min-confidence - Filter all results below this confidence [possible values: unknown, low, medium, high] + Filter all results below this confidence + + [possible values: unknown, low, medium, high] + -h, --help - Print help + Print help (see a summary with '-h') + -V, --version Print version diff --git a/docs/usage.md b/docs/usage.md index e4bd68a2..93779ef8 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -69,6 +69,94 @@ See [Integration](#integration) for suggestions on when to use each format. All other exit codes are currently reserved. +## Using personas + +!!! tip + + `--persona=...` is available in `v0.7.0` and later. + +`zizmor` comes with three pre-defined "personas," which dictate how +sensitive `zizmor`'s analyses are: + +* The _regular persona_: the user wants high-signal, low-noise, actionable + security findings. This persona is best for ordinary local use as well as use + in most CI/CD setups, which is why it's the default. + + !!! note + + This persona can be made explicit with `--persona=regular`, + although this is not required. + + +* The _pedantic persona_, enabled by `--persona=pedantic`: the user wants + *code smells* in addition to regular, actionable security findings. + + This persona is ideal for finding things that are a good idea + to clean up or resolve, but are likely not immediately actionable + security findings (or are actionable, but indicate a intentional + security decision by the workflow author). + + For example, using the pedantic persona will flag the following + with an `unpinned-uses` finding, since it uses a symbolic reference + as its pin instead of a hashed pin: + + ```yaml + uses: actions/checkout@v3 + ``` + + produces: + + ```console + $ zizmor --pedantic tests/test-data/unpinned-uses.yml + help[unpinned-uses]: unpinned action reference + --> tests/test-data/unpinned-uses.yml:14:9 + | + 14 | - uses: actions/checkout@v3 + | ------------------------- help: action is not pinned to a hash ref + | + = note: audit confidence → High + ``` + + !!! tip + + This persona can also be enabled with `--pedantic`, which is + an alias for `--persona=pedantic`. + +* The _auditor persona_, enabled by `--persona=auditor`: the user wants + *everything* flagged by `zizmor`, including findings that are likely + to be false positives. + + This persona is ideal for security auditors and code reviewers, who + want to go through `zizmor`'s findings manually with a fine-toothed comb. + + Some audits, notably `self-hosted-runner`, *only* produce auditor-level + results. This is because these audits require runtime context that `zizmor` + lacks access to by design, meaning that their results are always + subject to false positives. + + For example, with the default persona: + + ```console + $ zizmor tests/test-data/self-hosted.yml + 🌈 completed self-hosted.yml + No findings to report. Good job! (1 suppressed) + ``` + + and with `--persona=auditor`: + + ```console + $ zizmor --persona=auditor tests/test-data/self-hosted.yml + note[self-hosted-runner]: runs on a self-hosted runner + --> tests/test-data/self-hosted.yml:8:5 + | + 8 | runs-on: [self-hosted, my-ubuntu-box] + | ------------------------------------- note: self-hosted runner used here + | + = note: audit confidence → High + + 1 finding: 1 unknown, 0 informational, 0 low, 0 medium, 0 high + ``` + ## Filtering results There are two straightforward ways to filter `zizmor`'s results: diff --git a/src/audit/artipacked.rs b/src/audit/artipacked.rs index 24ea5606..65aaddf1 100644 --- a/src/audit/artipacked.rs +++ b/src/audit/artipacked.rs @@ -9,14 +9,12 @@ use itertools::Itertools; use super::{audit_meta, WorkflowAudit}; use crate::{ - finding::{Confidence, Finding, Severity}, + finding::{Confidence, Finding, Persona, Severity}, state::AuditState, }; use crate::{models::Workflow, utils::split_patterns}; -pub(crate) struct Artipacked { - pub(crate) state: AuditState, -} +pub(crate) struct Artipacked; audit_meta!( Artipacked, @@ -47,8 +45,8 @@ impl Artipacked { } impl WorkflowAudit for Artipacked { - fn new(state: AuditState) -> Result { - Ok(Self { state }) + fn new(_state: AuditState) -> Result { + Ok(Self) } fn audit<'w>(&self, workflow: &'w Workflow) -> Result>> { @@ -76,15 +74,11 @@ impl WorkflowAudit for Artipacked { Some(EnvValue::Boolean(true)) => { // If a user explicitly sets `persist-credentials: true`, // they probably mean it. Only report if being pedantic. - if self.state.pedantic { - vulnerable_checkouts.push(step) - } else { - continue; - } + vulnerable_checkouts.push((step, Persona::Pedantic)) } // TODO: handle expressions and literal strings here. // persist-credentials is true by default. - _ => vulnerable_checkouts.push(step), + _ => vulnerable_checkouts.push((step, Persona::default())), } } else if uses.starts_with("actions/upload-artifact") { let Some(EnvValue::String(path)) = with.get("path") else { @@ -102,11 +96,12 @@ impl WorkflowAudit 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 in vulnerable_checkouts { + for (checkout, persona) in vulnerable_checkouts { findings.push( Self::finding() .severity(Severity::Medium) .confidence(Confidence::Low) + .persona(persona) .add_location( checkout .location() @@ -119,7 +114,7 @@ impl WorkflowAudit for Artipacked { // Select only pairs where the vulnerable checkout precedes the // vulnerable upload. There are more efficient ways to do this than // a cartesian product, but this way is simple. - for (checkout, upload) in vulnerable_checkouts + for ((checkout, persona), upload) in vulnerable_checkouts .into_iter() .cartesian_product(vulnerable_uploads.into_iter()) { @@ -128,6 +123,7 @@ impl WorkflowAudit for Artipacked { Self::finding() .severity(Severity::High) .confidence(Confidence::High) + .persona(persona) .add_location( checkout .location() diff --git a/src/audit/github_env.rs b/src/audit/github_env.rs index 5b50b286..ccfd2dd7 100644 --- a/src/audit/github_env.rs +++ b/src/audit/github_env.rs @@ -172,7 +172,6 @@ mod tests { ("something | tee \"${$OTHER_ENV}\" # not $GITHUB_ENV", false), ] { let audit_state = AuditState { - pedantic: false, offline: false, gh_token: None, caches: Caches::new(), diff --git a/src/audit/self_hosted_runner.rs b/src/audit/self_hosted_runner.rs index d3e3fb5c..b9066d11 100644 --- a/src/audit/self_hosted_runner.rs +++ b/src/audit/self_hosted_runner.rs @@ -2,11 +2,11 @@ //! which are frequently unsafe to use in public repositories //! due to the potential for persistence between workflow runs. //! -//! This audit is "pedantic" only, since zizmor can't detect +//! This audit is "auditor" only, since zizmor can't detect //! whether self-hosted runners are ephemeral or not. use crate::{ - finding::{Confidence, Severity}, + finding::{Confidence, Persona, Severity}, AuditState, }; @@ -18,9 +18,7 @@ use github_actions_models::{ use super::{audit_meta, WorkflowAudit}; -pub(crate) struct SelfHostedRunner { - pub(crate) state: AuditState, -} +pub(crate) struct SelfHostedRunner; audit_meta!( SelfHostedRunner, @@ -29,11 +27,11 @@ audit_meta!( ); impl WorkflowAudit for SelfHostedRunner { - fn new(state: AuditState) -> anyhow::Result + fn new(_state: AuditState) -> anyhow::Result where Self: Sized, { - Ok(Self { state }) + Ok(Self) } fn audit<'w>( @@ -42,11 +40,6 @@ impl WorkflowAudit for SelfHostedRunner { ) -> Result>> { let mut results = vec![]; - if !self.state.pedantic { - log::info!("skipping self-hosted runner checks"); - return Ok(results); - } - for job in workflow.jobs() { let Job::NormalJob(normal) = *job else { continue; @@ -66,6 +59,7 @@ impl WorkflowAudit for SelfHostedRunner { Self::finding() .confidence(Confidence::High) .severity(Severity::Unknown) + .persona(Persona::Auditor) .add_location( job.location() .with_keys(&["runs-on".into()]) @@ -82,6 +76,7 @@ impl WorkflowAudit for SelfHostedRunner { Self::finding() .confidence(Confidence::Low) .severity(Severity::Unknown) + .persona(Persona::Auditor) .add_location( job.location().with_keys(&["runs-on".into()]).annotated( "expression may expand into a self-hosted runner", @@ -101,6 +96,7 @@ impl WorkflowAudit for SelfHostedRunner { Self::finding() .confidence(Confidence::Low) .severity(Severity::Unknown) + .persona(Persona::Auditor) .add_location( job.location() .with_keys(&["runs-on".into()]) @@ -114,6 +110,7 @@ impl WorkflowAudit for SelfHostedRunner { Self::finding() .confidence(Confidence::Low) .severity(Severity::Unknown) + .persona(Persona::Auditor) .add_location( job.location() .with_keys(&["runs-on".into()]) diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs index efd44604..09603914 100644 --- a/src/audit/template_injection.rs +++ b/src/audit/template_injection.rs @@ -20,14 +20,12 @@ use github_actions_models::{ use super::{audit_meta, WorkflowAudit}; use crate::{ expr::{BinOp, Expr, UnOp}, - finding::{Confidence, Severity}, + finding::{Confidence, Persona, Severity}, state::AuditState, utils::extract_expressions, }; -pub(crate) struct TemplateInjection { - pub(crate) state: AuditState, -} +pub(crate) struct TemplateInjection; audit_meta!( TemplateInjection, @@ -165,22 +163,27 @@ impl TemplateInjection { &self, run: &str, job: &NormalJob, - ) -> Vec<(String, Severity, Confidence)> { + ) -> Vec<(String, Severity, Confidence, Persona)> { let mut bad_expressions = vec![]; for expr in extract_expressions(run) { - let Ok(expr) = Expr::parse(expr.as_bare()) else { + let Ok(parsed) = Expr::parse(expr.as_bare()) else { log::warn!("couldn't parse expression: {expr}", expr = expr.as_bare()); continue; }; - // Filter "safe" expressions (ones that might expand to code, - // but not arbitrary code) by default, unless we're operating - // in pedantic mode. - if Self::expr_is_safe(&expr) && !self.state.pedantic { + if Self::expr_is_safe(&parsed) { + // Emit a pedantic finding for all expressions, since + // all template injections are code smells, even if unexploitable. + bad_expressions.push(( + expr.as_raw().into(), + Severity::Unknown, + Confidence::Unknown, + Persona::Pedantic, + )); continue; } - for context in expr.contexts() { + for context in parsed.contexts() { if context.starts_with("secrets.") { // While not ideal, secret expansion is typically not exploitable. continue; @@ -191,14 +194,29 @@ impl TemplateInjection { // input's type. In the future, we should index back into // the workflow's triggers and exclude input expansions // from innocuous types, e.g. booleans. - bad_expressions.push((context.into(), Severity::High, Confidence::Low)); + bad_expressions.push(( + context.into(), + Severity::High, + Confidence::Low, + Persona::default(), + )); } else if context.starts_with("env.") { // Almost never exploitable. - bad_expressions.push((context.into(), Severity::Low, Confidence::High)); + bad_expressions.push(( + context.into(), + Severity::Low, + Confidence::High, + Persona::default(), + )); } else if context.starts_with("github.event.") || context == "github.ref_name" { // TODO: Filter these more finely; not everything in the event // context is actually attacker-controllable. - bad_expressions.push((context.into(), Severity::High, Confidence::High)); + bad_expressions.push(( + context.into(), + Severity::High, + Confidence::High, + Persona::default(), + )); } else if context.starts_with("matrix.") || context == "matrix" { if let Some(Strategy { matrix, .. }) = &job.strategy { let matrix_is_static = match matrix { @@ -218,6 +236,7 @@ impl TemplateInjection { context.into(), Severity::Medium, Confidence::Medium, + Persona::default(), )); } } @@ -229,6 +248,7 @@ impl TemplateInjection { context.into(), Severity::Informational, Confidence::Low, + Persona::default(), )); } } @@ -239,11 +259,11 @@ impl TemplateInjection { } impl WorkflowAudit for TemplateInjection { - fn new(state: AuditState) -> anyhow::Result + fn new(_state: AuditState) -> anyhow::Result where Self: Sized, { - Ok(Self { state }) + Ok(Self) } fn audit_step<'w>(&self, step: &super::Step<'w>) -> anyhow::Result>> { @@ -266,12 +286,14 @@ impl WorkflowAudit for TemplateInjection { StepBody::Run { run, .. } => (run, step.location().with_keys(&["run".into()])), }; - for (expr, severity, confidence) in self.injectable_template_expressions(script, step.job()) + for (expr, severity, confidence, persona) in + self.injectable_template_expressions(script, step.job()) { findings.push( Self::finding() .severity(severity) .confidence(confidence) + .persona(persona) .add_location(step.location_with_name()) .add_location( script_loc.clone().annotated(format!( diff --git a/src/audit/unpinned_uses.rs b/src/audit/unpinned_uses.rs index 7ea50225..752f9b0e 100644 --- a/src/audit/unpinned_uses.rs +++ b/src/audit/unpinned_uses.rs @@ -1,19 +1,17 @@ -use crate::finding::{Confidence, Severity}; +use crate::finding::{Confidence, Persona, Severity}; use super::{audit_meta, AuditState, Finding, Step, WorkflowAudit}; -pub(crate) struct UnpinnedUses { - state: AuditState, -} +pub(crate) struct UnpinnedUses; audit_meta!(UnpinnedUses, "unpinned-uses", "unpinned action reference"); impl WorkflowAudit for UnpinnedUses { - fn new(state: AuditState) -> anyhow::Result + fn new(_state: AuditState) -> anyhow::Result where Self: Sized, { - Ok(Self { state }) + Ok(Self) } fn audit_step<'w>(&self, step: &Step<'w>) -> anyhow::Result>> { @@ -23,13 +21,18 @@ impl WorkflowAudit for UnpinnedUses { return Ok(vec![]); }; - let (annotation, severity) = if uses.unpinned() { + let (annotation, severity, persona) = if uses.unpinned() { ( "action is not pinned to a tag, branch, or hash ref", Severity::Medium, + Persona::default(), + ) + } else if uses.unhashed() { + ( + "action is not pinned to a hash ref", + Severity::Low, + Persona::Pedantic, ) - } else if uses.unhashed() && self.state.pedantic { - ("action is not pinned to a hash ref", Severity::Low) } else { return Ok(vec![]); }; @@ -38,6 +41,7 @@ impl WorkflowAudit for UnpinnedUses { Self::finding() .confidence(Confidence::High) .severity(severity) + .persona(persona) .add_location( step.location() .with_keys(&["uses".into()]) diff --git a/src/audit/use_trusted_publishing.rs b/src/audit/use_trusted_publishing.rs index 546b058b..81d9335c 100644 --- a/src/audit/use_trusted_publishing.rs +++ b/src/audit/use_trusted_publishing.rs @@ -1,7 +1,8 @@ -use std::{collections::HashMap, ops::Deref}; +use std::ops::Deref; use anyhow::Ok; use github_actions_models::{common::EnvValue, workflow::job::StepBody}; +use indexmap::IndexMap; use super::{audit_meta, WorkflowAudit}; use crate::{ @@ -28,7 +29,7 @@ audit_meta!( ); impl UseTrustedPublishing { - fn pypi_publish_uses_manual_credentials(&self, with: &HashMap) -> bool { + fn pypi_publish_uses_manual_credentials(&self, with: &IndexMap) -> bool { // `password` implies the step isn't using Trusted Publishing, // but we also need to check `repository-url` to prevent false-positives // on third-party indices. @@ -46,7 +47,7 @@ impl UseTrustedPublishing { } } - fn release_gem_uses_manual_credentials(&self, with: &HashMap) -> bool { + fn release_gem_uses_manual_credentials(&self, with: &IndexMap) -> bool { match with.get("setup-trusted-publisher") { Some(v) if v.to_string() == "true" => false, // Anything besides `true` means to *not* use trusted publishing. @@ -58,7 +59,7 @@ impl UseTrustedPublishing { fn rubygems_credential_uses_manual_credentials( &self, - with: &HashMap, + with: &IndexMap, ) -> bool { with.contains_key("api-token") } diff --git a/src/finding/mod.rs b/src/finding/mod.rs index b6acb859..4c8819bc 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -13,7 +13,32 @@ use crate::models::{Job, Step, Workflow}; pub(crate) mod locate; -// TODO: Traits + more flexible models here. +/// Represents the expected "persona" that would be interested in a given +/// finding. This is used to model the sensitivity of different use-cases +/// to false positives. +#[derive( + Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialOrd, PartialEq, Serialize, ValueEnum, +)] +pub(crate) enum Persona { + /// The "auditor" persona (false positives OK). + /// + /// This persona wants all results, including results that are likely + /// to be false positives. + Auditor, + + /// The "pedantic" persona (code smells OK). + /// + /// This persona wants findings that may or may not be problems, + /// but are potential "code smells". + Pedantic, + + /// The "regular" persona (minimal false positives). + /// + /// This persona wants actionable findings, and is sensitive to + /// false positives. + #[default] + Regular, +} #[derive( Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialOrd, PartialEq, Serialize, ValueEnum, @@ -240,11 +265,12 @@ pub(crate) struct Location<'w> { pub(crate) concrete: Feature<'w>, } -/// A finding's "determination," i.e. its confidence and severity classifications. +/// A finding's "determination," i.e. its various classifications. #[derive(Serialize)] pub(crate) struct Determinations { pub(crate) confidence: Confidence, pub(crate) severity: Severity, + pub(super) persona: Persona, } #[derive(Serialize)] @@ -263,6 +289,7 @@ pub(crate) struct FindingBuilder<'w> { url: &'static str, severity: Severity, confidence: Confidence, + persona: Persona, locations: Vec>, } @@ -274,6 +301,7 @@ impl<'w> FindingBuilder<'w> { url, severity: Default::default(), confidence: Default::default(), + persona: Default::default(), locations: vec![], } } @@ -288,6 +316,11 @@ impl<'w> FindingBuilder<'w> { self } + pub(crate) fn persona(mut self, persona: Persona) -> Self { + self.persona = persona; + self + } + pub(crate) fn add_location(mut self, location: SymbolicLocation<'w>) -> Self { self.locations.push(location); self @@ -309,6 +342,7 @@ impl<'w> FindingBuilder<'w> { determinations: Determinations { confidence: self.confidence, severity: self.severity, + persona: self.persona, }, locations, ignored: should_ignore, diff --git a/src/main.rs b/src/main.rs index 6da2e0f8..0d8b217d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,7 +5,7 @@ use anyhow::{anyhow, Context, Result}; use audit::WorkflowAudit; use clap::{Parser, ValueEnum}; use config::Config; -use finding::{Confidence, Severity}; +use finding::{Confidence, Persona, Severity}; use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle}; use owo_colors::OwoColorize; use registry::{AuditRegistry, FindingRegistry, WorkflowRegistry}; @@ -27,10 +27,16 @@ mod utils; #[derive(Parser)] #[command(about, version)] struct App { - /// Emit findings even when the context suggests an explicit security decision made by the user. - #[arg(short, long)] + /// Emit 'pedantic' findings. + /// + /// This is an alias for --persona=pedantic. + #[arg(short, long, group = "_persona")] pedantic: bool, + /// The persona to use while auditing. + #[arg(long, group = "_persona", value_enum, default_value_t)] + persona: Persona, + /// Only perform audits that don't require network access. #[arg(short, long)] offline: bool, @@ -48,8 +54,8 @@ struct App { gh_token: Option, /// The output format to emit. By default, plain text will be emitted - #[arg(long, value_enum)] - format: Option, + #[arg(long, value_enum, default_value_t)] + format: OutputFormat, /// The configuration file to load. By default, any config will be /// discovered relative to $CWD. @@ -77,8 +83,9 @@ struct App { inputs: Vec, } -#[derive(Debug, Copy, Clone, ValueEnum)] +#[derive(Debug, Default, Copy, Clone, ValueEnum)] pub(crate) enum OutputFormat { + #[default] Plain, Json, Sarif, @@ -87,7 +94,12 @@ pub(crate) enum OutputFormat { fn run() -> Result { human_panic::setup_panic!(); - let app = App::parse(); + let mut app = App::parse(); + + // `--pedantic` is a shortcut for `--persona=pedantic`. + if app.pedantic { + app.persona = Persona::Pedantic; + } env_logger::Builder::new() .filter_level(app.verbose.log_level_filter()) @@ -203,12 +215,7 @@ fn run() -> Result { bar.finish_and_clear(); - let format = match app.format { - None => OutputFormat::Plain, - Some(f) => f, - }; - - match format { + match app.format { 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( @@ -217,7 +224,7 @@ fn run() -> Result { )?, }; - if app.no_exit_codes || matches!(format, OutputFormat::Sarif) { + if app.no_exit_codes || matches!(app.format, OutputFormat::Sarif) { Ok(ExitCode::SUCCESS) } else { Ok(results.into()) diff --git a/src/models.rs b/src/models.rs index a0ffa5ad..d55b3847 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,7 +1,7 @@ //! Enriching/context-bearing wrappers over GitHub Actions models //! from the `github-actions-models` crate. -use std::{collections::hash_map, iter::Enumerate, ops::Deref, path::Path}; +use std::{iter::Enumerate, ops::Deref, path::Path}; use crate::finding::{Route, SymbolicLocation}; use anyhow::{anyhow, Context, Result}; @@ -177,7 +177,7 @@ impl<'w> Job<'w> { /// An iterable container for jobs within a [`Workflow`]. pub(crate) struct Jobs<'w> { parent: &'w Workflow, - inner: hash_map::Iter<'w, String, workflow::Job>, + inner: indexmap::map::Iter<'w, String, workflow::Job>, } impl<'w> Jobs<'w> { diff --git a/src/registry.rs b/src/registry.rs index 6d67086d..142ba701 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -6,7 +6,7 @@ use std::{collections::HashMap, path::Path, process::ExitCode}; use crate::{ audit::WorkflowAudit, config::Config, - finding::{Confidence, Finding, Severity}, + finding::{Confidence, Finding, Persona, Severity}, models::Workflow, App, }; @@ -114,6 +114,8 @@ pub(crate) struct FindingRegistry<'a> { config: &'a Config, minimum_severity: Option, minimum_confidence: Option, + persona: Persona, + suppressed: Vec>, ignored: Vec>, findings: Vec>, highest_seen_severity: Option, @@ -125,6 +127,8 @@ impl<'a> FindingRegistry<'a> { config, minimum_severity: app.min_severity, minimum_confidence: app.min_confidence, + persona: app.persona, + suppressed: Default::default(), ignored: Default::default(), findings: Default::default(), highest_seen_severity: None, @@ -137,7 +141,9 @@ 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 finding.ignored + if self.persona > finding.determinations.persona { + self.suppressed.push(finding); + } else if finding.ignored || self .minimum_severity .map_or(false, |min| min > finding.determinations.severity) @@ -160,15 +166,25 @@ impl<'a> FindingRegistry<'a> { } } - /// All non-filtered findings. + /// The total count of all findings, regardless of status. + pub(crate) fn count(&self) -> usize { + self.findings.len() + self.ignored.len() + self.suppressed.len() + } + + /// All non-ignored and non-suppressed findings. pub(crate) fn findings(&self) -> &[Finding<'a>] { &self.findings } - /// All filtered findings. + /// All ignored findings. pub(crate) fn ignored(&self) -> &[Finding<'a>] { &self.ignored } + + /// All persona-suppressed findings. + pub(crate) fn suppressed(&self) -> &[Finding<'a>] { + &self.suppressed + } } impl From> for ExitCode { diff --git a/src/render.rs b/src/render.rs index ae6b192b..61a4b6fb 100644 --- a/src/render.rs +++ b/src/render.rs @@ -72,14 +72,28 @@ pub(crate) fn render_findings(registry: &WorkflowRegistry, findings: &FindingReg println!(); } + let mut qualifiers = vec![]; + if !findings.ignored().is_empty() { + qualifiers.push(format!( + "{nignored} ignored", + nignored = findings.ignored().len().bright_yellow() + )); + } + if !findings.suppressed().is_empty() { + qualifiers.push(format!( + "{nsuppressed} suppressed", + nsuppressed = findings.suppressed().len().bright_yellow() + )); + } + if findings.findings().is_empty() { - if findings.ignored().is_empty() { + if qualifiers.is_empty() { println!("{}", "No findings to report. Good job!".green()); } else { println!( - "{no_findings} ({nignored} ignored)", + "{no_findings} ({qualifiers})", no_findings = "No findings to report. Good job!".green(), - nignored = findings.ignored().len().bright_yellow() + qualifiers = qualifiers.join(", ").bold(), ); } } else { @@ -96,8 +110,8 @@ pub(crate) fn render_findings(registry: &WorkflowRegistry, findings: &FindingReg } } - if findings.ignored().is_empty() { - let nfindings = findings.findings().len(); + if qualifiers.is_empty() { + let nfindings = findings.count(); print!( "{nfindings} finding{s}: ", nfindings = nfindings.green(), @@ -105,9 +119,9 @@ pub(crate) fn render_findings(registry: &WorkflowRegistry, findings: &FindingReg ); } else { print!( - "{nfindings} findings ({nignored} ignored): ", - nfindings = (findings.findings().len() + findings.ignored().len()).green(), - nignored = findings.ignored().len().bright_yellow() + "{nfindings} findings ({qualifiers}): ", + nfindings = findings.count().green(), + qualifiers = qualifiers.join(", ").bold(), ); } diff --git a/src/state.rs b/src/state.rs index 3ea9540b..9c2d3c7c 100644 --- a/src/state.rs +++ b/src/state.rs @@ -9,7 +9,6 @@ use crate::{ #[derive(Clone)] pub(crate) struct AuditState { - pub(crate) pedantic: bool, pub(crate) offline: bool, pub(crate) gh_token: Option, pub(crate) caches: Caches, @@ -19,7 +18,6 @@ impl AuditState { pub(crate) fn new(app: &App) -> Self { Self { caches: Caches::new(), - pedantic: app.pedantic, offline: app.offline, gh_token: app.gh_token.clone(), } diff --git a/tests/acceptance.rs b/tests/acceptance.rs index b7a35f4a..70386c7d 100644 --- a/tests/acceptance.rs +++ b/tests/acceptance.rs @@ -152,8 +152,8 @@ fn audit_use_trusted_publishing() -> anyhow::Result<()> { fn audit_self_hosted() -> anyhow::Result<()> { let auditable = workflow_under_test("self-hosted.yml"); - // Note : self-hosted audit is pedantic - let cli_args = ["--pedantic", &auditable]; + // Note: self-hosted audit is auditor-only + let cli_args = ["--persona=auditor", &auditable]; let execution = zizmor().args(cli_args).output()?; diff --git a/tests/snapshot.rs b/tests/snapshot.rs index 22842893..f1575995 100644 --- a/tests/snapshot.rs +++ b/tests/snapshot.rs @@ -70,11 +70,25 @@ pub(crate) fn zizmor() -> Zizmor { Zizmor::new() } +#[test] +fn artipacked() -> Result<()> { + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("artipacked.yml")) + .args(["--persona=pedantic"]) + .run()?); + + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("artipacked.yml")) + .run()?); + + Ok(()) +} + #[test] fn self_hosted() -> Result<()> { insta::assert_snapshot!(zizmor() .workflow(workflow_under_test("self-hosted.yml")) - .args(["--pedantic"]) + .args(["--persona=auditor"]) .run()?); insta::assert_snapshot!(zizmor() diff --git a/tests/snapshots/snapshot__artipacked-2.snap b/tests/snapshots/snapshot__artipacked-2.snap new file mode 100644 index 00000000..d78a5b6a --- /dev/null +++ b/tests/snapshots/snapshot__artipacked-2.snap @@ -0,0 +1,14 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"artipacked.yml\")).run()?" +snapshot_kind: text +--- +warning[artipacked]: credential persistence through GitHub Actions artifacts + --> @@INPUT@@:13:9 + | +13 | - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + | ---------------------------------------------------------------------------- does not set persist-credentials: false + | + = note: audit confidence → Low + +2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 1 medium, 0 high diff --git a/tests/snapshots/snapshot__artipacked.snap b/tests/snapshots/snapshot__artipacked.snap new file mode 100644 index 00000000..b5b03084 --- /dev/null +++ b/tests/snapshots/snapshot__artipacked.snap @@ -0,0 +1,25 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"artipacked.yml\")).args([\"--persona=pedantic\"]).run()?" +snapshot_kind: text +--- +warning[artipacked]: credential persistence through GitHub Actions artifacts + --> @@INPUT@@:13:9 + | +13 | - 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@@:18:9 + | +18 | - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + | _________- +19 | | with: +20 | | 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/tests/snapshots/snapshot__self_hosted-2.snap b/tests/snapshots/snapshot__self_hosted-2.snap index eb3f5dcd..500ee044 100644 --- a/tests/snapshots/snapshot__self_hosted-2.snap +++ b/tests/snapshots/snapshot__self_hosted-2.snap @@ -1,6 +1,6 @@ --- source: tests/snapshot.rs -expression: "zizmor(&workflow_under_test(\"self-hosted.yml\"), &[], false)?" +expression: "zizmor().workflow(workflow_under_test(\"self-hosted.yml\")).run()?" snapshot_kind: text --- -No findings to report. Good job! +No findings to report. Good job! (1 suppressed) diff --git a/tests/snapshots/snapshot__unpinned_uses-2.snap b/tests/snapshots/snapshot__unpinned_uses-2.snap index 1f926f6e..fc3799f3 100644 --- a/tests/snapshots/snapshot__unpinned_uses-2.snap +++ b/tests/snapshots/snapshot__unpinned_uses-2.snap @@ -1,6 +1,6 @@ --- source: tests/snapshot.rs -expression: "zizmor(Some(&workflow_under_test(\"unpinned-uses.yml\")), &[], false)?" +expression: "zizmor().workflow(workflow_under_test(\"unpinned-uses.yml\")).run()?" snapshot_kind: text --- warning[unpinned-uses]: unpinned action reference @@ -35,4 +35,4 @@ warning[unpinned-uses]: unpinned action reference | = note: audit confidence → High -4 findings: 0 unknown, 0 informational, 0 low, 4 medium, 0 high +5 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 4 medium, 0 high diff --git a/tests/test-data/artipacked.yml b/tests/test-data/artipacked.yml index 6893b833..aa4f8f1b 100644 --- a/tests/test-data/artipacked.yml +++ b/tests/test-data/artipacked.yml @@ -11,3 +11,10 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + + pedantic: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 + with: + persist-credentials: true