Add Fix for cache-poisoning audit rule (#923)

Co-authored-by: William Woodruff <william@yossarian.net>
This commit is contained in:
Mostafa Moradian 2025-07-09 02:39:55 +02:00 committed by GitHub
parent cc92548a3d
commit c3706e2d84
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 327 additions and 43 deletions

View file

@ -327,8 +327,7 @@ fn apply_single_patch(
Some(idx) => (idx + bias, idx + bias + from.len()),
None => {
return Err(Error::InvalidOperation(format!(
"no match for '{}' in feature",
from
"no match for '{from}' in feature"
)));
}
};
@ -437,8 +436,7 @@ fn apply_single_patch(
let style = Style::from_feature(&existing_feature, document);
if !matches!(style, Style::BlockMapping | Style::FlowMapping) {
return Err(Error::InvalidOperation(format!(
"can't perform merge against non-mapping at {:?}",
existing_key_route
"can't perform merge against non-mapping at {existing_key_route:?}"
)));
}
@ -465,8 +463,7 @@ fn apply_single_patch(
)
.map_err(|e| {
Error::InvalidOperation(format!(
"MergeInto: failed to parse existing mapping at {:?}: {e}",
existing_key_route
"MergeInto: failed to parse existing mapping at {existing_key_route:?}: {e}"
))
})?;
@ -500,8 +497,7 @@ fn apply_single_patch(
// The key exists, but has an empty body.
// TODO: Support this.
Ok(None) => Err(Error::InvalidOperation(format!(
"MergeInto: cannot merge into empty key at {:?}",
existing_key_route
"MergeInto: cannot merge into empty key at {existing_key_route:?}"
))),
// The key does not exist.
Err(Error::Query(yamlpath::QueryError::ExhaustedMapping(_))) => apply_single_patch(
@ -632,8 +628,7 @@ pub fn serialize_flow(value: &serde_yaml::Value) -> Result<String, Error> {
}
if !matches!(key, serde_yaml::Value::String(_)) {
return Err(Error::InvalidOperation(format!(
"mapping keys must be strings, found: {:?}",
key
"mapping keys must be strings, found: {key:?}"
)));
}
serialize_inner(key, buf)?;
@ -649,8 +644,7 @@ pub fn serialize_flow(value: &serde_yaml::Value) -> Result<String, Error> {
Ok(())
}
serde_yaml::Value::Tagged(tagged_value) => Err(Error::InvalidOperation(format!(
"cannot serialize tagged value: {:?}",
tagged_value
"cannot serialize tagged value: {tagged_value:?}"
))),
}
}
@ -961,7 +955,7 @@ fn apply_value_replacement(
if string_content.contains('\n') {
// For multiline literal blocks, use the raw string content
let leading_whitespace = extract_leading_whitespace(doc, feature);
let content_indent = format!("{} ", leading_whitespace); // Key indent + 2 spaces for content
let content_indent = format!("{leading_whitespace} "); // Key indent + 2 spaces for content
// Format as: key: |\n content\n more content
let indented_content = string_content
@ -1027,15 +1021,15 @@ fn handle_flow_mapping_value_replacement(
if value_part.is_empty() {
// Case: { key: } -> { key: value }
let key_part = before_colon.trim_start_matches('{').trim();
Ok(format!("{{ {}: {} }}", key_part, val_str))
Ok(format!("{{ {key_part}: {val_str} }}"))
} else {
// Case: { key: oldvalue } -> { key: newvalue }
let key_part = before_colon.trim_start_matches('{').trim();
Ok(format!("{{ {}: {} }}", key_part, val_str))
Ok(format!("{{ {key_part}: {val_str} }}"))
}
} else {
// Case 2: { key } - no colon, bare key -> { key: value }
let key_part = trimmed.trim_start_matches('{').trim_end_matches('}').trim();
Ok(format!("{{ {}: {} }}", key_part, val_str))
Ok(format!("{{ {key_part}: {val_str} }}"))
}
}

View file

@ -4,13 +4,16 @@ use github_actions_models::workflow::Trigger;
use github_actions_models::workflow::event::{BareEvent, BranchFilters, OptionalBody};
use crate::audit::{Audit, audit_meta};
use crate::finding::location::Locatable as _;
use crate::finding::{Confidence, Finding, Severity};
use crate::finding::location::{Locatable as _, Routable};
use crate::finding::{Confidence, Finding, Fix, FixDisposition, Severity};
use crate::models::StepCommon;
use crate::models::coordinate::{ActionCoordinate, ControlExpr, ControlFieldType, Toggle, Usage};
use crate::models::workflow::{JobExt as _, NormalJob, Step, Steps};
use crate::state::AuditState;
use indexmap::IndexMap;
use yamlpatch::{Op, Patch};
use super::AuditLoadError;
/// The list of know cache-aware actions
@ -278,10 +281,84 @@ impl CachePoisoning {
))
}
fn evaluate_cache_usage<'doc>(&self, step: &impl StepCommon<'doc>) -> Option<Usage> {
fn evaluate_cache_usage<'doc>(
&self,
step: &impl StepCommon<'doc>,
) -> Option<(&'static ActionCoordinate, Usage)> {
KNOWN_CACHE_AWARE_ACTIONS
.iter()
.find_map(|coord| coord.usage(step))
.find_map(|coord| coord.usage(step).map(|usage| (coord, usage)))
}
fn create_cache_disable_fix<'doc>(
&self,
coord: &ActionCoordinate,
step: &Step<'doc>,
) -> Option<Fix<'doc>> {
match coord {
ActionCoordinate::NotConfigurable(_pattern) => {
// For non-configurable actions, we can't provide automatic fixes
None
}
ActionCoordinate::Configurable {
uses_pattern,
control,
} => self.create_configurable_action_fix(uses_pattern, control, step),
}
}
fn create_configurable_action_fix<'doc>(
&self,
_uses_pattern: &crate::models::uses::RepositoryUsesPattern,
control: &ControlExpr,
step: &Step<'doc>,
) -> Option<Fix<'doc>> {
match control {
ControlExpr::Single {
toggle,
field_name,
field_type,
..
} => {
let (field_value, title, _description) = match (toggle, field_type) {
(Toggle::OptOut, ControlFieldType::Boolean) => (
serde_yaml::Value::Bool(true),
format!("Set {field_name}: true to disable caching"),
format!(
"Set '{field_name}' to 'true' to disable cache writes in this publishing workflow."
),
),
(Toggle::OptIn, ControlFieldType::Boolean) => (
serde_yaml::Value::Bool(false),
format!("Set {field_name}: false to disable caching"),
format!(
"Set '{field_name}' to 'false' to disable caching in this publishing workflow."
),
),
// String control fields are action-specific and we can't reliably know
// what value disables caching (e.g., setup-node expects '' not 'false')
(Toggle::OptIn, ControlFieldType::String)
| (Toggle::OptOut, ControlFieldType::String) => {
return None;
}
};
Some(Fix {
title,
key: step.location().key,
disposition: FixDisposition::default(),
patches: vec![Patch {
route: step.route().into(),
operation: Op::MergeInto {
key: "with".to_string(),
updates: IndexMap::from([(field_name.to_string(), field_value)]),
},
}],
})
}
// For complex control expressions (All/Any), don't provide automatic fixes for now
ControlExpr::All(_) | ControlExpr::Any(_) => None,
}
}
fn uses_cache_aware_step<'doc>(
@ -289,7 +366,7 @@ impl CachePoisoning {
step: &Step<'doc>,
scenario: &PublishingArtifactsScenario<'doc>,
) -> Option<Finding<'doc>> {
let cache_usage = self.evaluate_cache_usage(step)?;
let (coord, cache_usage) = self.evaluate_cache_usage(step)?;
let (yaml_key, annotation) = match cache_usage {
Usage::Always => ("uses", "caching always restored here"),
@ -298,7 +375,7 @@ impl CachePoisoning {
Usage::ConditionalOptIn => ("with", "opt-in for caching might happen here"),
};
let finding = match scenario {
let mut finding_builder = match scenario {
PublishingArtifactsScenario::UsingTypicalWorkflowTrigger => Self::finding()
.confidence(Confidence::Low)
.severity(Severity::High)
@ -313,8 +390,7 @@ impl CachePoisoning {
.primary()
.with_keys(&[yaml_key.into()])
.annotated(annotation),
)
.build(step.workflow()),
),
PublishingArtifactsScenario::UsingWellKnowPublisherAction(publisher) => Self::finding()
.confidence(Confidence::Low)
.severity(Severity::High)
@ -329,11 +405,15 @@ impl CachePoisoning {
.primary()
.with_keys(&[yaml_key.into()])
.annotated(annotation),
)
.build(step.workflow()),
),
};
finding.ok()
// Add fix if available
if let Some(fix) = self.create_cache_disable_fix(coord, step) {
finding_builder = finding_builder.fix(fix);
}
finding_builder.build(step.workflow()).ok()
}
}
@ -363,3 +443,200 @@ impl Audit for CachePoisoning {
Ok(findings)
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::{
github_api::GitHubHost, models::workflow::Workflow, registry::InputKey, state::AuditState,
};
/// Macro for testing workflow audits with common boilerplate
///
/// Usage: `test_workflow_audit!(AuditType, "filename.yml", workflow_yaml, |findings| { ... })`
///
/// This macro:
/// 1. Creates a test workflow from the provided YAML with the specified filename
/// 2. Sets up the audit state
/// 3. Creates and runs the audit
/// 4. Executes the provided test closure with the findings
macro_rules! test_workflow_audit {
($audit_type:ty, $filename:expr, $workflow_content:expr, $test_fn:expr) => {{
let key = InputKey::local($filename, None::<&str>).unwrap();
let workflow = Workflow::from_string($workflow_content.to_string(), key).unwrap();
let audit_state = AuditState {
config: &Default::default(),
no_online_audits: false,
cache_dir: "/tmp/zizmor".into(),
gh_token: None,
gh_hostname: GitHubHost::Standard("github.com".into()),
};
let audit = <$audit_type>::new(&audit_state).unwrap();
let findings = audit.audit_workflow(&workflow).unwrap();
$test_fn(findings)
}};
}
/// Helper function to apply a fix and return the result for snapshot testing
fn apply_fix_for_snapshot(workflow_content: &str, findings: Vec<Finding>) -> String {
assert!(!findings.is_empty(), "Expected findings but got none");
let finding = &findings[0];
assert!(!finding.fixes.is_empty(), "Expected fixes but got none");
let fix = &finding.fixes[0];
// Parse the workflow content as a document
let document = yamlpath::Document::new(workflow_content).unwrap();
// Apply the fix and get the new document
let fixed_document = fix.apply(&document).unwrap();
// Return the source content
fixed_document.source().to_string()
}
#[test]
fn test_cache_disable_fix_opt_out_boolean() {
let workflow_content = r#"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/cache@v4
with:
path: |
~/.cargo/registry
~/.cargo/git
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- uses: softprops/action-gh-release@v1
"#;
test_workflow_audit!(
CachePoisoning,
"test_cache_disable_fix_opt_out_boolean.yml",
workflow_content,
|findings: Vec<Finding>| {
let fixed_content = apply_fix_for_snapshot(workflow_content, findings);
insta::assert_snapshot!(fixed_content, @r"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/cache@v4
with:
path: |
~/.cargo/registry
~/.cargo/git
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
lookup-only: true
- uses: softprops/action-gh-release@v1
");
}
);
}
#[test]
fn test_cache_disable_fix_opt_in_boolean() {
let workflow_content = r#"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/setup-go@v4
with:
go-version: '1.21'
cache: true
- uses: softprops/action-gh-release@v1
"#;
test_workflow_audit!(
CachePoisoning,
"test_cache_disable_fix_opt_in_boolean.yml",
workflow_content,
|findings: Vec<Finding>| {
let fixed_content = apply_fix_for_snapshot(workflow_content, findings);
insta::assert_snapshot!(fixed_content, @r"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/setup-go@v4
with:
go-version: '1.21'
cache: false
- uses: softprops/action-gh-release@v1
");
}
);
}
#[test]
fn test_cache_disable_fix_opt_in_string() {
let workflow_content = r#"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/setup-java@v4
with:
distribution: 'temurin'
java-version: '17'
cache: 'gradle'
- uses: softprops/action-gh-release@v1
"#;
test_workflow_audit!(
CachePoisoning,
"test_cache_disable_fix_opt_in_string.yml",
workflow_content,
|findings: Vec<Finding>| {
let finding = &findings[0];
// String control fields should not have fixes since we can't reliably
// know what value disables caching for different actions
assert!(finding.fixes.is_empty());
}
);
}
#[test]
fn test_cache_disable_fix_non_configurable() {
let workflow_content = r#"
name: Test Workflow
on: release
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: Mozilla-Actions/sccache-action@v1
- uses: softprops/action-gh-release@v1
"#;
test_workflow_audit!(
CachePoisoning,
"test_cache_disable_fix_non_configurable.yml",
workflow_content,
|findings: Vec<Finding>| {
let finding = &findings[0];
// Non-configurable actions should not have fixes
assert!(finding.fixes.is_empty());
}
);
}
}

View file

@ -274,18 +274,16 @@ impl ControlExpr {
ControlExpr::Single {
toggle,
field_name,
field_type,
field_type: _,
satisfied_by_default: enabled_by_default,
} => {
// If the controlling field is not present, the default dictates the semantics.
if let Some(field_value) = with.get(*field_name) {
match field_value.to_string().as_str() {
"false" if matches!(field_type, ControlFieldType::Boolean) => {
match toggle {
Toggle::OptIn => ControlEvaluation::NotSatisfied,
Toggle::OptOut => ControlEvaluation::Satisfied,
}
}
"false" => match toggle {
Toggle::OptIn => ControlEvaluation::NotSatisfied,
Toggle::OptOut => ControlEvaluation::Satisfied,
},
other => match ExplicitExpr::from_curly(other) {
None => match toggle {
Toggle::OptIn => ControlEvaluation::Satisfied,

View file

@ -517,7 +517,7 @@ impl<'doc> Matrix<'doc> {
fn expand(values: HashMap<String, serde_json::Value>) -> Vec<(String, String)> {
values
.iter()
.flat_map(|(key, value)| Matrix::walk_path(value, format!("matrix.{}", key)))
.flat_map(|(key, value)| Matrix::walk_path(value, format!("matrix.{key}")))
.collect()
}

View file

@ -91,7 +91,7 @@ pub fn apply_fixes(
failed_fixes.push((
finding.ident,
file_path,
format!("conflict after applying previous fixes: {}", e),
format!("conflict after applying previous fixes: {e}"),
));
}
}

View file

@ -1,6 +1,7 @@
---
source: crates/zizmor/tests/integration/e2e.rs
expression: "zizmor().offline(false).output(OutputMode::Both).args([\"--no-online-audits\"]).input(\"woodruffw/gha-hazmat@33cd22cdd7823a5795768388aff977fe992b5aad\").run()?"
snapshot_kind: text
---
INFO collect_inputs: zizmor: collected 22 inputs from woodruffw/gha-hazmat
INFO zizmor::registry: skipping impostor-commit: offline audits only requested
@ -269,6 +270,7 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
error[excessive-permissions]: overly broad permissions
--> .github/workflows/excessive-permissions.yml:19:3

View file

@ -14,5 +14,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ runtime artifacts usually published here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
1 finding: 0 unknown, 0 informational, 0 low, 0 medium, 1 high
1 findings (1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -17,6 +17,7 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
--> @@INPUT@@:5:1
@ -37,6 +38,7 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| |______________________________________________________^ opt-in for caching here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache poisoning attack
--> @@INPUT@@:5:1
@ -55,5 +57,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| |________________________^ opt-in for caching here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
3 findings: 0 unknown, 0 informational, 0 low, 0 medium, 3 high
3 findings (3 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 3 high

View file

@ -17,5 +17,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed, 1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -14,5 +14,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed, 1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -16,5 +16,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| |_____________________^ opt-in for caching here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed, 1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -17,5 +17,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cache enabled by default here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed, 1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -17,5 +17,6 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
| |__________________________^ opt-in for caching here
|
= note: audit confidence → Low
= note: this finding has an auto-fix
1 finding: 0 unknown, 0 informational, 0 low, 0 medium, 1 high
1 findings (1 fixable): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

View file

@ -9,6 +9,10 @@ of `zizmor`.
## Next (UNRELEASED)
### Enhancements 🌱
* The [cache-poisoning] audit now supports auto-fixes for many findings (#923)
### Bug Fixes 🐛
* Fixed a bug where `--fix` would fail to preserve comments when modifying