From 80d906e6ea69463e701784ef48a4485027f254aa Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 22 Jan 2025 20:46:01 -0500 Subject: [PATCH 1/9] feat: "raw" audit support --- src/audit/mod.rs | 17 ++++++++++-- src/audit/overprovisioned_secrets.rs | 41 ++++++++++++++++++++++++++++ src/audit/template_injection.rs | 2 +- src/main.rs | 1 + src/utils.rs | 36 ++++++++++++------------ 5 files changed, 77 insertions(+), 20 deletions(-) create mode 100644 src/audit/overprovisioned_secrets.rs diff --git a/src/audit/mod.rs b/src/audit/mod.rs index 743fb5e8..a818a5f2 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -21,6 +21,7 @@ pub(crate) mod hardcoded_container_credentials; pub(crate) mod impostor_commit; pub(crate) mod insecure_commands; pub(crate) mod known_vulnerable_actions; +pub(crate) mod overprovisioned_secrets; pub(crate) mod ref_confusion; pub(crate) mod secrets_inherit; pub(crate) mod self_hosted_runner; @@ -147,6 +148,10 @@ pub(crate) use audit_meta; /// 2. [`Audit::audit_composite_step`]: runs on each composite step within the /// action (most specific) /// +/// For both: +/// +/// 1. [`Audit::audit_raw`]: runs on the raw, unparsed YAML document source +/// /// Picking a higher specificity means that the lower methods are shadowed. /// In other words, if an audit chooses to implement [`Audit::audit`], it should implement /// **only** [`Audit::audit`] and not [`Audit::audit_normal_job`] or @@ -208,15 +213,23 @@ pub(crate) trait Audit: AuditCore { Ok(results) } + fn audit_raw<'w>(&self, _raw: &'w str) -> Result>> { + Ok(vec![]) + } + /// The top-level auditing function for both workflows and actions. /// /// Implementors **should not** override this blanket implementation, /// since it's marked with tracing instrumentation. #[instrument(skip(self))] fn audit<'w>(&self, input: &'w AuditInput) -> Result>> { - match input { + let mut results = match input { AuditInput::Workflow(workflow) => self.audit_workflow(workflow), AuditInput::Action(action) => self.audit_action(action), - } + }?; + + results.extend(self.audit_raw(input.document().source())?); + + Ok(results) } } diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs new file mode 100644 index 00000000..37006b26 --- /dev/null +++ b/src/audit/overprovisioned_secrets.rs @@ -0,0 +1,41 @@ +use std::ops::Range; + +use crate::{expr::Expr, utils::extract_expressions}; + +use super::{audit_meta, Audit}; + +pub(crate) struct OverprovisionedSecrets; + +audit_meta!( + OverprovisionedSecrets, + "overprovisioned-secrets", + "detects secrets that are overprovisioned" +); + +impl Audit for OverprovisionedSecrets { + fn new(_state: super::AuditState) -> anyhow::Result + where + Self: Sized, + { + Ok(Self) + } + + fn audit_raw<'w>(&self, raw: &'w str) -> anyhow::Result>> { + for (expr, span) in extract_expressions(raw) { + let Ok(parsed) = Expr::parse(expr.as_bare()) else { + tracing::warn!("couldn't parse expression: {expr}", expr = expr.as_bare()); + continue; + }; + + todo!() + } + + Ok(vec![]) + } +} + +impl OverprovisionedSecrets { + fn secrets_expansions(span: Range, expr: &Expr) -> Vec> { + todo!() + } +} diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs index 155fe0e8..368d8ab1 100644 --- a/src/audit/template_injection.rs +++ b/src/audit/template_injection.rs @@ -168,7 +168,7 @@ impl TemplateInjection { step: &impl StepCommon<'s>, ) -> Vec<(String, Severity, Confidence, Persona)> { let mut bad_expressions = vec![]; - for expr in extract_expressions(run) { + for (expr, _) in extract_expressions(run) { let Ok(parsed) = Expr::parse(expr.as_bare()) else { tracing::warn!("couldn't parse expression: {expr}", expr = expr.as_bare()); continue; diff --git a/src/main.rs b/src/main.rs index 84749d78..9237d57a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -377,6 +377,7 @@ fn run() -> Result { register_audit!(audit::cache_poisoning::CachePoisoning); register_audit!(audit::secrets_inherit::SecretsInherit); register_audit!(audit::bot_conditions::BotConditions); + register_audit!(audit::overprovisioned_secrets::OverprovisionedSecrets); let mut results = FindingRegistry::new(&app, &config); { diff --git a/src/utils.rs b/src/utils.rs index ae15874f..9f42d5de 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,5 +1,7 @@ //! Helper routines. +use std::ops::Range; + use camino::Utf8Path; use github_actions_models::common::{ expr::{ExplicitExpr, LoE}, @@ -38,14 +40,14 @@ pub(crate) fn split_patterns(patterns: &str) -> impl Iterator { } /// Parse an expression from the given free-form text, returning the -/// expression and the next offset at which to resume parsing. +/// expression and the expression's byte span (relative to the input). /// /// Returns `None` if no expression is found, or an index past /// the end of the text if parsing is successful but exhausted. /// /// Adapted roughly from GitHub's `parseScalar`: /// See: -fn extract_expression(text: &str) -> Option<(ExplicitExpr, usize)> { +fn extract_expression(text: &str) -> Option<(ExplicitExpr, Range)> { let start = text.find("${{")?; let mut end = None; @@ -63,23 +65,23 @@ fn extract_expression(text: &str) -> Option<(ExplicitExpr, usize)> { end.map(|end| { ( ExplicitExpr::from_curly(&text[start..=end]).unwrap(), - end + 1, + start..end + 1, ) }) } /// Extract zero or more expressions from the given free-form text. -pub(crate) fn extract_expressions(text: &str) -> Vec { +pub(crate) fn extract_expressions(text: &str) -> Vec<(ExplicitExpr, Range)> { let mut exprs = vec![]; let mut view = text; - while let Some((expr, next)) = extract_expression(view) { - exprs.push(expr); + while let Some((expr, span)) = extract_expression(view) { + exprs.push((expr, (span.start..span.end))); - if next >= text.len() { + if span.end >= text.len() { break; } else { - view = &view[next..]; + view = &view[span.end..]; } } @@ -161,21 +163,21 @@ mod tests { #[test] fn test_parse_expression() { let exprs = &[ - ("${{ foo }}", "foo", 10), - ("${{ foo }}${{ bar }}", "foo", 10), - ("leading ${{ foo }} trailing", "foo", 18), + ("${{ foo }}", "foo", 0..10), + ("${{ foo }}${{ bar }}", "foo", 0..10), + ("leading ${{ foo }} trailing", "foo", 8..18), ( "leading ${{ '${{ quoted! }}' }} trailing", "'${{ quoted! }}'", - 31, + 8..31, ), - ("${{ 'es''cape' }}", "'es''cape'", 17), + ("${{ 'es''cape' }}", "'es''cape'", 0..17), ]; - for (text, expected_expr, expected_idx) in exprs { - let (actual_expr, actual_idx) = extract_expression(text).unwrap(); + for (text, expected_expr, expected_span) in exprs { + let (actual_expr, actual_span) = extract_expression(text).unwrap(); assert_eq!(*expected_expr, actual_expr.as_bare()); - assert_eq!(*expected_idx, actual_idx); + assert_eq!(*expected_span, actual_span); } } @@ -184,7 +186,7 @@ mod tests { let expressions = r#"echo "OSSL_PATH=${{ github.workspace }}/osslcache/${{ matrix.PYTHON.OPENSSL.TYPE }}-${{ matrix.PYTHON.OPENSSL.VERSION }}-${OPENSSL_HASH}" >> $GITHUB_ENV"#; let exprs = extract_expressions(expressions) .into_iter() - .map(|e| e.as_curly().to_string()) + .map(|(e, _)| e.as_curly().to_string()) .collect::>(); assert_eq!( From b98190b34d9e58826e1bcd6105255aed440f0e4e Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 22 Jan 2025 20:53:37 -0500 Subject: [PATCH 2/9] remove unused parent fields --- src/finding/locate.rs | 14 +++----------- src/finding/mod.rs | 8 -------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/src/finding/locate.rs b/src/finding/locate.rs index 038f734b..67633ef8 100644 --- a/src/finding/locate.rs +++ b/src/finding/locate.rs @@ -21,8 +21,8 @@ impl Locator { // If we don't have a path into the workflow, all // we have is the workflow itself. - let (feature, parent_feature) = if location.route.components.is_empty() { - (document.root(), document.root()) + let feature = if location.route.components.is_empty() { + document.root() } else { let mut builder = yamlpath::QueryBuilder::new(); @@ -35,25 +35,17 @@ impl Locator { let query = builder.build(); - let parent_feature = if let Some(parent) = query.parent() { - document.query(&parent)? - } else { - document.root() - }; - - (document.query(&query)?, parent_feature) + document.query(&query)? }; Ok(Feature { location: ConcreteLocation::from(&feature.location), - parent_location: ConcreteLocation::from(&parent_feature.location), feature: document.extract_with_leading_whitespace(&feature), comments: document .feature_comments(&feature) .into_iter() .map(Comment) .collect(), - parent_feature: document.extract_with_leading_whitespace(&parent_feature), }) } } diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 368f2343..8920a7c4 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -265,19 +265,11 @@ pub(crate) struct Feature<'w> { /// The feature's concrete location, as both an offset range and point span. pub(crate) location: ConcreteLocation, - /// The feature's concrete parent location. - /// This can be the same as the feature's own location, if the feature - /// is the document root. - pub(crate) parent_location: ConcreteLocation, - /// The feature's textual content. pub(crate) feature: &'w str, /// Any comments within the feature's span. pub(crate) comments: Vec>, - - /// The feature's parent's textual content. - pub(crate) parent_feature: &'w str, } /// A location within a GitHub Actions workflow, with both symbolic and concrete components. From ec31dd61350f5c1a9d29dd50e462500e0ef19fa5 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 22 Jan 2025 21:15:31 -0500 Subject: [PATCH 3/9] refactor/simplify concretization --- src/audit/overprovisioned_secrets.rs | 2 +- src/finding/locate.rs | 51 ---------------------------- src/finding/mod.rs | 34 ++++++++++++++++--- 3 files changed, 30 insertions(+), 57 deletions(-) delete mode 100644 src/finding/locate.rs diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index 37006b26..7f0ef24b 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -27,7 +27,7 @@ impl Audit for OverprovisionedSecrets { continue; }; - todo!() + // todo!() } Ok(vec![]) diff --git a/src/finding/locate.rs b/src/finding/locate.rs deleted file mode 100644 index 67633ef8..00000000 --- a/src/finding/locate.rs +++ /dev/null @@ -1,51 +0,0 @@ -//! `tree-sitter` helpers for extracting and locating concrete features -//! in the original YAML. - -use anyhow::Result; - -use super::{Comment, ConcreteLocation, Feature, SymbolicLocation}; - -pub(crate) struct Locator {} - -impl Locator { - pub(crate) fn new() -> Self { - Self {} - } - - pub(crate) fn concretize<'w>( - &self, - document: &'w impl AsRef, - location: &SymbolicLocation, - ) -> Result> { - let document = document.as_ref(); - - // If we don't have a path into the workflow, all - // we have is the workflow itself. - let feature = if location.route.components.is_empty() { - document.root() - } else { - let mut builder = yamlpath::QueryBuilder::new(); - - for component in &location.route.components { - builder = match component { - super::RouteComponent::Key(key) => builder.key(key.clone()), - super::RouteComponent::Index(idx) => builder.index(*idx), - } - } - - let query = builder.build(); - - document.query(&query)? - }; - - Ok(Feature { - location: ConcreteLocation::from(&feature.location), - feature: document.extract_with_leading_whitespace(&feature), - comments: document - .feature_comments(&feature) - .into_iter() - .map(Comment) - .collect(), - }) - } -} diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 8920a7c4..c85c9bb4 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -4,7 +4,6 @@ use std::{borrow::Cow, sync::LazyLock}; use anyhow::{anyhow, Result}; use clap::ValueEnum; -use locate::Locator; use regex::Regex; use serde::Serialize; use terminal_link::Link; @@ -14,8 +13,6 @@ use crate::{ registry::InputKey, }; -pub(crate) mod locate; - /// 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. @@ -192,11 +189,38 @@ impl<'w> SymbolicLocation<'w> { self, document: &'w impl AsRef, ) -> Result> { - let feature = Locator::new().concretize(document, &self)?; + let document = document.as_ref(); + + // If we don't have a path into the workflow, all + // we have is the workflow itself. + let feature = if self.route.components.is_empty() { + document.root() + } else { + let mut builder = yamlpath::QueryBuilder::new(); + + for component in &self.route.components { + builder = match component { + RouteComponent::Key(key) => builder.key(key.clone()), + RouteComponent::Index(idx) => builder.index(*idx), + } + } + + let query = builder.build(); + + document.query(&query)? + }; Ok(Location { symbolic: self, - concrete: feature, + concrete: Feature { + location: ConcreteLocation::from(&feature.location), + feature: document.extract_with_leading_whitespace(&feature), + comments: document + .feature_comments(&feature) + .into_iter() + .map(Comment) + .collect(), + }, }) } } From efae5cd06becb94ed8a631fa720ed3a9754b26b4 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 22 Jan 2025 23:06:50 -0500 Subject: [PATCH 4/9] fill in more bits --- src/audit/mod.rs | 20 ++++++- src/audit/overprovisioned_secrets.rs | 88 +++++++++++++++++++++++++--- src/finding/mod.rs | 13 ++++ 3 files changed, 109 insertions(+), 12 deletions(-) diff --git a/src/audit/mod.rs b/src/audit/mod.rs index a818a5f2..d4abd48f 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -3,9 +3,10 @@ use anyhow::Result; use github_actions_models::action; use tracing::instrument; +use yamlpath::Document; use crate::{ - finding::{Finding, FindingBuilder}, + finding::{Finding, FindingBuilder, SymbolicLocation}, models::{Action, CompositeStep, Job, NormalJob, ReusableWorkflowCallJob, Step, Workflow}, registry::InputKey, state::AuditState, @@ -56,6 +57,19 @@ impl AuditInput { AuditInput::Action(action) => action.link.as_deref(), } } + + pub(crate) fn location(&self) -> SymbolicLocation { + match self { + AuditInput::Workflow(workflow) => workflow.location(), + AuditInput::Action(action) => action.location(), + } + } +} + +impl AsRef for AuditInput { + fn as_ref(&self) -> &Document { + self.document() + } } impl From for AuditInput { @@ -213,7 +227,7 @@ pub(crate) trait Audit: AuditCore { Ok(results) } - fn audit_raw<'w>(&self, _raw: &'w str) -> Result>> { + fn audit_raw<'w>(&self, _input: &'w AuditInput) -> Result>> { Ok(vec![]) } @@ -228,7 +242,7 @@ pub(crate) trait Audit: AuditCore { AuditInput::Action(action) => self.audit_action(action), }?; - results.extend(self.audit_raw(input.document().source())?); + results.extend(self.audit_raw(input)?); Ok(results) } diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index 7f0ef24b..dcbf8785 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -1,8 +1,10 @@ -use std::ops::Range; +use crate::{ + expr::Expr, + finding::{Confidence, Feature, Location, Severity}, + utils::extract_expressions, +}; -use crate::{expr::Expr, utils::extract_expressions}; - -use super::{audit_meta, Audit}; +use super::{audit_meta, Audit, AuditInput}; pub(crate) struct OverprovisionedSecrets; @@ -20,22 +22,90 @@ impl Audit for OverprovisionedSecrets { Ok(Self) } - fn audit_raw<'w>(&self, raw: &'w str) -> anyhow::Result>> { + fn audit_raw<'w>(&self, input: &'w AuditInput) -> anyhow::Result>> { + let mut findings = vec![]; + let raw = input.document().source(); + for (expr, span) in extract_expressions(raw) { let Ok(parsed) = Expr::parse(expr.as_bare()) else { tracing::warn!("couldn't parse expression: {expr}", expr = expr.as_bare()); continue; }; - // todo!() + for _ in Self::secrets_expansions(&parsed) { + findings.push( + Self::finding() + .confidence(Confidence::High) + .severity(Severity::Medium) + .add_raw_location(Location::new( + input.location(), + Feature { + location: todo!(), + feature: todo!(), + comments: vec![], // TODO: extract comments + }, + )) + .build(input)?, + ); + } } - Ok(vec![]) + Ok(findings) } } impl OverprovisionedSecrets { - fn secrets_expansions(span: Range, expr: &Expr) -> Vec> { - todo!() + fn secrets_expansions(expr: &Expr) -> Vec<()> { + let mut results = vec![]; + + match expr { + Expr::Call { func, args } => { + // TODO: Consider any function call that accepts bare `secrets` + // to be a finding? Are there any other functions that users + // would plausible call with the entire `secrets` object? + if func == "toJSON" + && args + .iter() + .any(|arg| matches!(arg, Expr::Context { raw, components: _ } if raw == "secrets")) + { + results.push(()); + } else { + results.extend(args.iter().flat_map(Self::secrets_expansions)); + } + } + Expr::Index(expr) => results.extend(Self::secrets_expansions(expr)), + Expr::Context { raw: _, components } => { + results.extend(components.iter().flat_map(Self::secrets_expansions)) + } + Expr::BinOp { lhs, op: _, rhs } => { + results.extend(Self::secrets_expansions(lhs)); + results.extend(Self::secrets_expansions(rhs)); + } + Expr::UnOp { op: _, expr } => results.extend(Self::secrets_expansions(expr)), + _ => (), + } + + results + } +} + +#[cfg(test)] +mod tests { + #[test] + fn test_secrets_expansions() { + for (expr, count) in &[ + ("secrets", 0), + ("toJSON(secrets.foo)", 0), + ("toJSON(secrets)", 1), + ("false || toJSON(secrets)", 1), + ("toJSON(secrets) || toJSON(secrets)", 2), + ("format('{0}', toJSON(secrets))", 1), + ] { + let expr = crate::expr::Expr::parse(expr).unwrap(); + assert_eq!( + super::OverprovisionedSecrets::secrets_expansions(&expr).len(), + *count + ); + } } } diff --git a/src/finding/mod.rs b/src/finding/mod.rs index c85c9bb4..8b6e3164 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -305,6 +305,12 @@ pub(crate) struct Location<'w> { pub(crate) concrete: Feature<'w>, } +impl<'w> Location<'w> { + pub(crate) fn new(symbolic: SymbolicLocation<'w>, concrete: Feature<'w>) -> Self { + Self { symbolic, concrete } + } +} + /// A finding's "determination," i.e. its various classifications. #[derive(Serialize)] pub(crate) struct Determinations { @@ -330,6 +336,7 @@ pub(crate) struct FindingBuilder<'w> { severity: Severity, confidence: Confidence, persona: Persona, + raw_locations: Vec>, locations: Vec>, } @@ -342,6 +349,7 @@ impl<'w> FindingBuilder<'w> { severity: Default::default(), confidence: Default::default(), persona: Default::default(), + raw_locations: vec![], locations: vec![], } } @@ -361,6 +369,11 @@ impl<'w> FindingBuilder<'w> { self } + pub(crate) fn add_raw_location(mut self, location: Location<'w>) -> Self { + self.raw_locations.push(location); + self + } + pub(crate) fn add_location(mut self, location: SymbolicLocation<'w>) -> Self { self.locations.push(location); self From 929e92a2d24c9e204ece15028dc82bb44e6e4bfb Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 23 Jan 2025 00:10:27 -0500 Subject: [PATCH 5/9] make it work --- Cargo.lock | 23 ++++++++++++ Cargo.toml | 1 + src/audit/mod.rs | 8 +++++ src/audit/overprovisioned_secrets.rs | 24 ++++++++++--- src/finding/mod.rs | 36 ++++++++++++++----- src/models.rs | 9 +++++ src/render.rs | 5 ++- src/utils.rs | 27 +++++++------- tests/snapshot.rs | 9 +++++ .../snapshot__overprovisioned_secrets.snap | 22 ++++++++++++ tests/test-data/overprovisioned-secrets.yml | 21 +++++++++++ 11 files changed, 158 insertions(+), 27 deletions(-) create mode 100644 tests/snapshots/snapshot__overprovisioned_secrets.snap create mode 100644 tests/test-data/overprovisioned-secrets.yml diff --git a/Cargo.lock b/Cargo.lock index c434cd6b..2055a596 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1072,6 +1072,16 @@ dependencies = [ "redox_syscall", ] +[[package]] +name = "line-index" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e27e0ed5a392a7f5ba0b3808a2afccff16c64933312c84b57618b49d1209bd2" +dependencies = [ + "nohash-hasher", + "text-size", +] + [[package]] name = "linked-hash-map" version = "0.5.6" @@ -1176,6 +1186,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "nohash-hasher" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bf50223579dc7cdcfb3bfcacf7069ff68243f8c363f62ffa99cf000a6b9c451" + [[package]] name = "nom" version = "7.1.3" @@ -2083,6 +2099,12 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3369f5ac52d5eb6ab48c6b4ffdc8efbcad6b89c765749064ba298f2c68a16a76" +[[package]] +name = "text-size" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f18aa187839b2bdb1ad2fa35ead8c4c2976b64e4363c386d45ac0f7ee85c9233" + [[package]] name = "thiserror" version = "1.0.69" @@ -3126,6 +3148,7 @@ dependencies = [ "indicatif", "insta", "itertools", + "line-index", "owo-colors", "pest", "pest_derive", diff --git a/Cargo.toml b/Cargo.toml index b0edefe9..469fcbe7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ human-panic = "2.0.1" indexmap = "2.7.1" indicatif = "0.17.9" itertools = "0.14.0" +line-index = "0.1.2" owo-colors = "4.1.0" pest = "2.7.15" pest_derive = "2.7.15" diff --git a/src/audit/mod.rs b/src/audit/mod.rs index d4abd48f..ff62fb33 100644 --- a/src/audit/mod.rs +++ b/src/audit/mod.rs @@ -2,6 +2,7 @@ use anyhow::Result; use github_actions_models::action; +use line_index::LineIndex; use tracing::instrument; use yamlpath::Document; @@ -51,6 +52,13 @@ impl AuditInput { } } + pub(crate) fn line_index(&self) -> &LineIndex { + match self { + AuditInput::Workflow(workflow) => &workflow.line_index, + AuditInput::Action(action) => &action.line_index, + } + } + pub(crate) fn link(&self) -> Option<&str> { match self { AuditInput::Workflow(workflow) => workflow.link.as_deref(), diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index dcbf8785..e1b10ab6 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -1,6 +1,6 @@ use crate::{ expr::Expr, - finding::{Confidence, Feature, Location, Severity}, + finding::{ConcreteLocation, Confidence, Feature, Location, Severity}, utils::extract_expressions, }; @@ -11,7 +11,7 @@ pub(crate) struct OverprovisionedSecrets; audit_meta!( OverprovisionedSecrets, "overprovisioned-secrets", - "detects secrets that are overprovisioned" + "excessively provisioned secrets" ); impl Audit for OverprovisionedSecrets { @@ -32,16 +32,28 @@ impl Audit for OverprovisionedSecrets { continue; }; + dbg!(expr.as_curly()); + for _ in Self::secrets_expansions(&parsed) { + let start_point = input.line_index().line_col((span.start as u32).into()); + let end_point = input.line_index().line_col((span.end as u32).into()); + findings.push( Self::finding() .confidence(Confidence::High) .severity(Severity::Medium) .add_raw_location(Location::new( - input.location(), + input + .location() + .annotated("injects the entire secrets context into the runner") + .primary(), Feature { - location: todo!(), - feature: todo!(), + location: ConcreteLocation::new( + start_point.into(), + end_point.into(), + span.start..span.end, + ), + feature: dbg!(&raw[span.start..span.end]), comments: vec![], // TODO: extract comments }, )) @@ -50,6 +62,8 @@ impl Audit for OverprovisionedSecrets { } } + dbg!(findings.len()); + Ok(findings) } } diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 8b6e3164..43fb7e4f 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -1,9 +1,10 @@ //! Models and APIs for handling findings and their locations. -use std::{borrow::Cow, sync::LazyLock}; +use std::{borrow::Cow, ops::Range, sync::LazyLock}; use anyhow::{anyhow, Result}; use clap::ValueEnum; +use line_index::LineCol; use regex::Regex; use serde::Serialize; use terminal_link::Link; @@ -232,6 +233,15 @@ pub(crate) struct Point { pub(crate) column: usize, } +impl From for Point { + fn from(value: LineCol) -> Self { + Self { + row: value.line as usize, + column: value.col as usize, + } + } +} + /// A "concrete" location for some feature. /// Every concrete location contains two spans: a line-and-column span, /// and an offset range. @@ -239,8 +249,17 @@ pub(crate) struct Point { pub(crate) struct ConcreteLocation { pub(crate) start_point: Point, pub(crate) end_point: Point, - pub(crate) start_offset: usize, - pub(crate) end_offset: usize, + pub(crate) offset_span: Range, +} + +impl ConcreteLocation { + pub(crate) fn new(start_point: Point, end_point: Point, offset_span: Range) -> Self { + Self { + start_point, + end_point, + offset_span, + } + } } impl From<&yamlpath::Location> for ConcreteLocation { @@ -254,8 +273,7 @@ impl From<&yamlpath::Location> for ConcreteLocation { row: value.point_span.1 .0, column: value.point_span.1 .1, }, - start_offset: value.byte_span.0, - end_offset: value.byte_span.1, + offset_span: value.byte_span.0..value.byte_span.1, } } } @@ -380,19 +398,21 @@ impl<'w> FindingBuilder<'w> { } pub(crate) fn build(self, document: &'w impl AsRef) -> Result> { - let locations = self + let mut locations = self .locations .iter() .map(|l| l.clone().concretize(document)) .collect::>>()?; + locations.extend(self.raw_locations); + if !locations.iter().any(|l| l.symbolic.primary) { return Err(anyhow!( "API misuse: at least one location must be marked with primary()" )); } - let should_ignore = self.ignored_from_inlined_comment(&locations, self.ident); + let should_ignore = Self::ignored_from_inlined_comment(&locations, self.ident); Ok(Finding { ident: self.ident, @@ -408,7 +428,7 @@ impl<'w> FindingBuilder<'w> { }) } - fn ignored_from_inlined_comment(&self, locations: &[Location], id: &str) -> bool { + fn ignored_from_inlined_comment(locations: &[Location], id: &str) -> bool { locations .iter() .flat_map(|l| &l.concrete.comments) diff --git a/src/models.rs b/src/models.rs index 9c228524..6461050d 100644 --- a/src/models.rs +++ b/src/models.rs @@ -14,6 +14,7 @@ use github_actions_models::workflow::job::{RunsOn, Strategy}; use github_actions_models::workflow::{self, job, job::StepBody, Trigger}; use github_actions_models::{action, common}; use indexmap::IndexMap; +use line_index::LineIndex; use serde_json::{json, Value}; use terminal_link::Link; @@ -66,6 +67,7 @@ pub(crate) struct Workflow { /// A clickable (OSC 8) link to this workflow, if remote. pub(crate) link: Option, pub(crate) document: yamlpath::Document, + pub(crate) line_index: LineIndex, inner: workflow::Workflow, } @@ -97,6 +99,8 @@ impl Workflow { let document = yamlpath::Document::new(&contents)?; + let line_index = LineIndex::new(&contents); + let link = match key { InputKey::Local(_) => None, InputKey::Remote(_) => { @@ -109,6 +113,7 @@ impl Workflow { link, key, document, + line_index, inner, }) } @@ -707,6 +712,7 @@ pub(crate) struct Action { pub(crate) key: InputKey, pub(crate) link: Option, pub(crate) document: yamlpath::Document, + pub(crate) line_index: LineIndex, inner: action::Action, } @@ -745,6 +751,8 @@ impl Action { let document = yamlpath::Document::new(&contents)?; + let line_index = LineIndex::new(&contents); + let link = match key { InputKey::Local(_) => None, InputKey::Remote(_) => { @@ -757,6 +765,7 @@ impl Action { key, link, document, + line_index, inner, }) } diff --git a/src/render.rs b/src/render.rs index 3c859d77..3f8391fe 100644 --- a/src/render.rs +++ b/src/render.rs @@ -62,7 +62,10 @@ pub(crate) fn finding_snippet<'w>( }; Level::from(&finding.determinations.severity) - .span(loc.concrete.location.start_offset..loc.concrete.location.end_offset) + .span( + loc.concrete.location.offset_span.start + ..loc.concrete.location.offset_span.end, + ) .label(annotation) })), ); diff --git a/src/utils.rs b/src/utils.rs index 9f42d5de..acfe9689 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -39,24 +39,25 @@ pub(crate) fn split_patterns(patterns: &str) -> impl Iterator { .filter(|line| !line.is_empty() && !line.starts_with('#')) } -/// Parse an expression from the given free-form text, returning the -/// expression and the expression's byte span (relative to the input). +/// Parse an expression from the given free-form text, starting +/// at the given offset. The returned span is absolute. /// -/// Returns `None` if no expression is found, or an index past +/// Returns `None` if no expression is found, or an span past /// the end of the text if parsing is successful but exhausted. /// /// Adapted roughly from GitHub's `parseScalar`: /// See: -fn extract_expression(text: &str) -> Option<(ExplicitExpr, Range)> { - let start = text.find("${{")?; +fn extract_expression(text: &str, offset: usize) -> Option<(ExplicitExpr, Range)> { + let view = &text[offset..]; + let start = view.find("${{")?; let mut end = None; let mut in_string = false; - for (idx, char) in text.bytes().enumerate().skip(start) { + for (idx, char) in view.bytes().enumerate().skip(start) { if char == b'\'' { in_string = !in_string; - } else if !in_string && text.as_bytes()[idx] == b'}' && text.as_bytes()[idx - 1] == b'}' { + } else if !in_string && view.as_bytes()[idx] == b'}' && view.as_bytes()[idx - 1] == b'}' { end = Some(idx); break; } @@ -64,8 +65,8 @@ fn extract_expression(text: &str) -> Option<(ExplicitExpr, Range)> { end.map(|end| { ( - ExplicitExpr::from_curly(&text[start..=end]).unwrap(), - start..end + 1, + ExplicitExpr::from_curly(&view[start..=end]).unwrap(), + start + offset..end + offset + 1, ) }) } @@ -73,15 +74,15 @@ fn extract_expression(text: &str) -> Option<(ExplicitExpr, Range)> { /// Extract zero or more expressions from the given free-form text. pub(crate) fn extract_expressions(text: &str) -> Vec<(ExplicitExpr, Range)> { let mut exprs = vec![]; - let mut view = text; + let mut offset = 0; - while let Some((expr, span)) = extract_expression(view) { + while let Some((expr, span)) = extract_expression(text, offset) { exprs.push((expr, (span.start..span.end))); if span.end >= text.len() { break; } else { - view = &view[span.end..]; + offset = span.end; } } @@ -175,7 +176,7 @@ mod tests { ]; for (text, expected_expr, expected_span) in exprs { - let (actual_expr, actual_span) = extract_expression(text).unwrap(); + let (actual_expr, actual_span) = extract_expression(text, 0).unwrap(); assert_eq!(*expected_expr, actual_expr.as_bare()); assert_eq!(*expected_span, actual_span); } diff --git a/tests/snapshot.rs b/tests/snapshot.rs index d7853ec0..37f9d43a 100644 --- a/tests/snapshot.rs +++ b/tests/snapshot.rs @@ -503,3 +503,12 @@ fn bot_conditions() -> Result<()> { Ok(()) } + +#[test] +fn overprovisioned_secrets() -> Result<()> { + insta::assert_snapshot!(zizmor() + .workflow(workflow_under_test("overprovisioned-secrets.yml")) + .run()?); + + Ok(()) +} diff --git a/tests/snapshots/snapshot__overprovisioned_secrets.snap b/tests/snapshots/snapshot__overprovisioned_secrets.snap new file mode 100644 index 00000000..43400038 --- /dev/null +++ b/tests/snapshots/snapshot__overprovisioned_secrets.snap @@ -0,0 +1,22 @@ +--- +source: tests/snapshot.rs +expression: "zizmor().workflow(workflow_under_test(\"overprovisioned-secrets.yml\")).run()?" +snapshot_kind: text +--- +warning[overprovisioned-secrets]: excessively provisioned secrets + --> @@INPUT@@:12:18 + | +12 | stuff: ${{ format('{0}', toJSON(secrets)) }} + | ------------------------------------- injects the entire secrets context into the runner + | + = note: audit confidence → High + +warning[overprovisioned-secrets]: excessively provisioned secrets + --> @@INPUT@@:21:25 + | +21 | secrets_json: ${{ toJSON(secrets) }} + | ---------------------- injects the entire secrets context into the runner + | + = note: audit confidence → High + +2 findings: 0 unknown, 0 informational, 0 low, 2 medium, 0 high diff --git a/tests/test-data/overprovisioned-secrets.yml b/tests/test-data/overprovisioned-secrets.yml new file mode 100644 index 00000000..582bc145 --- /dev/null +++ b/tests/test-data/overprovisioned-secrets.yml @@ -0,0 +1,21 @@ +on: push + +permissions: {} + +jobs: + test: + runs-on: ubuntu-latest + steps: + - run: echo "${stuff} ${otherstuff} ${morestuff}" + env: + # NOT OK: injects the entire secrets context into the runner + stuff: ${{ format('{0}', toJSON(secrets)) }} + # OK: selectively injects a single secret + otherstuff: ${{ secrets.otherstuff }} + # OK: weird, but still selectively injects only a single secret + morestuff: ${{ toJSON(secrets.morestuff) }} + + - run: | + some-random-command <<< "${secrets_json}" + env: + secrets_json: ${{ toJSON(secrets) }} From dca477a5afc7f01ac045a5e37d6b635e8d7061bb Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 23 Jan 2025 00:29:48 -0500 Subject: [PATCH 6/9] WIP docs --- docs/audits.md | 61 ++++++++++++++++++++++++++++ src/audit/overprovisioned_secrets.rs | 7 +++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/docs/audits.md b/docs/audits.md index 96be68b6..57b11bcb 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -873,6 +873,67 @@ not using `pull_request_target` for auto-merge workflows. GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} ``` +## `overprovisioned-secrets` + +| Type | Examples | Introduced in | Works offline | Enabled by default | +|----------|-------------------------|---------------|----------------|--------------------| +| Workflow | [overprovisioned-secrets.yml] | v1.3.0 | ✅ | ✅ | + +[overprovisioned-secrets.yml]: https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/overprovisioned-secrets.yml + +Detects excessive sharing of the `secrets` context. + +Typically, users access the `secrets` context via its individual members: + +```yaml +env: + SECRET_ONE: ${{ secrets.SECRET_ONE }} + SECRET_TWO: ${{ secrets.SECRET_TWO }} +``` + +This allows the Actions runner to only expose the secrets actually used by +the workflow to the job environment. + +However, if the user instead accesses the *entire* `secrets` context: + +```yaml +env: + SECRETS: ${{ toJson(secrets) }} +``` + +...then the entire `secrets` context is exposed to the runner, even if +only a single secret is actually needed. + +### Remediation + +In general, users should avoid loading the entire `secrets` context. +Secrets should be accessed individually by name. + +=== "Before :warning:" + + ```yaml title="overprovisioned.yml" hl_lines="7" + jobs: + deploy: + runs-on: ubuntu-latest + steps: + - run: ./deploy.sh + env: + SECRETS: ${{ toJSON(secrets) }} + ``` + +=== "After :white_check_mark:" + + ```yaml title="overprovisioned.yml" hl_lines="7-8" + jobs: + deploy: + runs-on: ubuntu-latest + steps: + - run: ./deploy.sh + env: + SECRET_ONE: ${{ secrets.SECRET_ONE }} + SECRET_TWO: ${{ secrets.SECRET_TWO }} + ``` + [ArtiPACKED: Hacking Giants Through a Race Condition in GitHub Actions Artifacts]: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/ [Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests]: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ [What the fork? Imposter commits in GitHub Actions and CI/CD]: https://www.chainguard.dev/unchained/what-the-fork-imposter-commits-in-github-actions-and-ci-cd diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index e1b10ab6..a09cc007 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -77,10 +77,10 @@ impl OverprovisionedSecrets { // TODO: Consider any function call that accepts bare `secrets` // to be a finding? Are there any other functions that users // would plausible call with the entire `secrets` object? - if func == "toJSON" + if func.eq_ignore_ascii_case("toJSON") && args .iter() - .any(|arg| matches!(arg, Expr::Context { raw, components: _ } if raw == "secrets")) + .any(|arg| matches!(arg, Expr::Context { raw, components: _ } if raw.eq_ignore_ascii_case("secrets"))) { results.push(()); } else { @@ -111,6 +111,9 @@ mod tests { ("secrets", 0), ("toJSON(secrets.foo)", 0), ("toJSON(secrets)", 1), + ("tojson(secrets)", 1), + ("toJSON(SECRETS)", 1), + ("tOjSoN(sECrEtS)", 1), ("false || toJSON(secrets)", 1), ("toJSON(secrets) || toJSON(secrets)", 2), ("format('{0}', toJSON(secrets))", 1), From 21de83145e1872f86943fff69fdd9cfd4b8ba54a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 23 Jan 2025 00:40:44 -0500 Subject: [PATCH 7/9] remove dbgs, ban dbgs in CI --- .github/workflows/ci.yml | 2 +- src/audit/overprovisioned_secrets.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d82499b9..007b0980 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,7 @@ jobs: - uses: Swatinem/rust-cache@f0deed1e0edfc6a9be95417288c0e1099b1eeec3 # v2 - name: Lint - run: cargo clippy -- -D warnings + run: cargo clippy -- -D warnings -D clippy::dbg_macro test: runs-on: ubuntu-latest diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index a09cc007..3950dd63 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -32,7 +32,7 @@ impl Audit for OverprovisionedSecrets { continue; }; - dbg!(expr.as_curly()); + expr.as_curly(); for _ in Self::secrets_expansions(&parsed) { let start_point = input.line_index().line_col((span.start as u32).into()); @@ -53,7 +53,7 @@ impl Audit for OverprovisionedSecrets { end_point.into(), span.start..span.end, ), - feature: dbg!(&raw[span.start..span.end]), + feature: &raw[span.start..span.end], comments: vec![], // TODO: extract comments }, )) @@ -62,7 +62,7 @@ impl Audit for OverprovisionedSecrets { } } - dbg!(findings.len()); + findings.len(); Ok(findings) } From fa006844e99cc848302272768df6bc4e9f45c5f6 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 26 Jan 2025 12:43:46 -0500 Subject: [PATCH 8/9] handle comments in raw spans --- src/audit/overprovisioned_secrets.rs | 15 +---- src/finding/mod.rs | 55 +++++++++++++++++-- .../snapshot__overprovisioned_secrets.snap | 2 +- tests/test-data/overprovisioned-secrets.yml | 6 ++ 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs index 3950dd63..ac5cd3b9 100644 --- a/src/audit/overprovisioned_secrets.rs +++ b/src/audit/overprovisioned_secrets.rs @@ -1,6 +1,6 @@ use crate::{ expr::Expr, - finding::{ConcreteLocation, Confidence, Feature, Location, Severity}, + finding::{Confidence, Feature, Location, Severity}, utils::extract_expressions, }; @@ -35,9 +35,6 @@ impl Audit for OverprovisionedSecrets { expr.as_curly(); for _ in Self::secrets_expansions(&parsed) { - let start_point = input.line_index().line_col((span.start as u32).into()); - let end_point = input.line_index().line_col((span.end as u32).into()); - findings.push( Self::finding() .confidence(Confidence::High) @@ -47,15 +44,7 @@ impl Audit for OverprovisionedSecrets { .location() .annotated("injects the entire secrets context into the runner") .primary(), - Feature { - location: ConcreteLocation::new( - start_point.into(), - end_point.into(), - span.start..span.end, - ), - feature: &raw[span.start..span.end], - comments: vec![], // TODO: extract comments - }, + Feature::from_span(&span, input), )) .build(input)?, ); diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 43fb7e4f..1f58df73 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -4,12 +4,13 @@ use std::{borrow::Cow, ops::Range, sync::LazyLock}; use anyhow::{anyhow, Result}; use clap::ValueEnum; -use line_index::LineCol; +use line_index::{LineCol, TextSize}; use regex::Regex; use serde::Serialize; use terminal_link::Link; use crate::{ + audit::AuditInput, models::{CompositeStep, JobExt, Step}, registry::InputKey, }; @@ -278,11 +279,13 @@ impl From<&yamlpath::Location> for ConcreteLocation { } } +static ANY_COMMENT: LazyLock = LazyLock::new(|| Regex::new(r"#.*$").unwrap()); + static IGNORE_EXPR: LazyLock = - LazyLock::new(|| Regex::new(r"^# zizmor: ignore\[(.+)\]\s*$").unwrap()); + LazyLock::new(|| Regex::new(r"# zizmor: ignore\[(.+)\]\s*$").unwrap()); /// Represents a single source comment. -#[derive(Serialize)] +#[derive(Debug, Serialize)] #[serde(transparent)] pub(crate) struct Comment<'w>(&'w str); @@ -310,10 +313,54 @@ pub(crate) struct Feature<'w> { /// The feature's textual content. pub(crate) feature: &'w str, - /// Any comments within the feature's span. + /// Any comments within the feature's line span. pub(crate) comments: Vec>, } +impl<'w> Feature<'w> { + pub(crate) fn from_span(span: &Range, input: &'w AuditInput) -> Self { + let raw = input.document().source(); + let start = TextSize::new(span.start as u32); + let end = TextSize::new(span.end as u32); + + let start_point = input.line_index().line_col(start); + let end_point = input.line_index().line_col(end); + + // Extract any comments within the feature's line span. + // + // This is slightly less precise than comment extraction + // when concretizing a symbolic location, since we're operating + // on a raw span rather than an AST-aware YAML path. + // + // NOTE: We can't use LineIndex::lines() to extract the comment-eligible + // lines, because it doesn't include full line spans if the input + // span is a strict subset of a single line. + let comments = (start_point.line..=end_point.line) + .into_iter() + .flat_map(|line| { + // NOTE: We don't really expect this to fail, since this + // line range comes from the line index itself. + let line = input.line_index().line(line)?; + // Chomp the trailing newline rather than enabling + // multi-line mode in ANY_COMMENT, on the theory that + // chomping is a little faster. + let line = &raw[line].trim_end(); + ANY_COMMENT.is_match(line).then(|| Comment(line)) + }) + .collect(); + + Feature { + location: ConcreteLocation::new( + start_point.into(), + end_point.into(), + span.start..span.end, + ), + feature: &raw[span.start..span.end], + comments, + } + } +} + /// A location within a GitHub Actions workflow, with both symbolic and concrete components. #[derive(Serialize)] pub(crate) struct Location<'w> { diff --git a/tests/snapshots/snapshot__overprovisioned_secrets.snap b/tests/snapshots/snapshot__overprovisioned_secrets.snap index 43400038..b1947703 100644 --- a/tests/snapshots/snapshot__overprovisioned_secrets.snap +++ b/tests/snapshots/snapshot__overprovisioned_secrets.snap @@ -19,4 +19,4 @@ warning[overprovisioned-secrets]: excessively provisioned secrets | = note: audit confidence → High -2 findings: 0 unknown, 0 informational, 0 low, 2 medium, 0 high +3 findings (1 ignored): 0 unknown, 0 informational, 0 low, 2 medium, 0 high diff --git a/tests/test-data/overprovisioned-secrets.yml b/tests/test-data/overprovisioned-secrets.yml index 582bc145..33752404 100644 --- a/tests/test-data/overprovisioned-secrets.yml +++ b/tests/test-data/overprovisioned-secrets.yml @@ -19,3 +19,9 @@ jobs: some-random-command <<< "${secrets_json}" env: secrets_json: ${{ toJSON(secrets) }} + + - run: | + some-random-command <<< "${secrets_json}" + env: + # tests that we handle ignore comments within raw spans correctly + secrets_json: ${{ toJSON(secrets) }} # zizmor: ignore[overprovisioned-secrets] From 6b3988fc8b32a8ab60cfd1af799a2f017c205ce3 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 26 Jan 2025 12:44:19 -0500 Subject: [PATCH 9/9] clippy fixes --- src/finding/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/finding/mod.rs b/src/finding/mod.rs index 1f58df73..b9a3b6b2 100644 --- a/src/finding/mod.rs +++ b/src/finding/mod.rs @@ -336,7 +336,6 @@ impl<'w> Feature<'w> { // lines, because it doesn't include full line spans if the input // span is a strict subset of a single line. let comments = (start_point.line..=end_point.line) - .into_iter() .flat_map(|line| { // NOTE: We don't really expect this to fail, since this // line range comes from the line index itself. @@ -345,7 +344,7 @@ impl<'w> Feature<'w> { // multi-line mode in ANY_COMMENT, on the theory that // chomping is a little faster. let line = &raw[line].trim_end(); - ANY_COMMENT.is_match(line).then(|| Comment(line)) + ANY_COMMENT.is_match(line).then_some(Comment(line)) }) .collect();