diff --git a/crates/zizmor/src/audit/ref_version_mismatch.rs b/crates/zizmor/src/audit/ref_version_mismatch.rs index 523f9d0f5..1e2c166d1 100644 --- a/crates/zizmor/src/audit/ref_version_mismatch.rs +++ b/crates/zizmor/src/audit/ref_version_mismatch.rs @@ -24,7 +24,7 @@ pub(crate) struct RefVersionMismatch { audit_meta!( RefVersionMismatch, "ref-version-mismatch", - "detects commit SHAs that don't match their version comment tags" + "action's hash pin does not match version comment" ); #[allow(clippy::unwrap_used)] diff --git a/crates/zizmor/src/output/sarif.rs b/crates/zizmor/src/output/sarif.rs index 6f63c09c9..b270921b1 100644 --- a/crates/zizmor/src/output/sarif.rs +++ b/crates/zizmor/src/output/sarif.rs @@ -3,9 +3,10 @@ use std::collections::HashSet; use serde_sarif::sarif::{ - ArtifactContent, ArtifactLocation, Invocation, Location as SarifLocation, LogicalLocation, - Message, MultiformatMessageString, PhysicalLocation, PropertyBag, Region, ReportingDescriptor, - Result as SarifResult, ResultKind, ResultLevel, Run, Sarif, Tool, ToolComponent, + ArtifactContent, ArtifactLocation, CodeFlow, Invocation, Location as SarifLocation, + LogicalLocation, Message, MultiformatMessageString, PhysicalLocation, PropertyBag, Region, + ReportingDescriptor, Result as SarifResult, ResultKind, ResultLevel, Run, Sarif, ThreadFlow, + ThreadFlowLocation, Tool, ToolComponent, }; use crate::finding::{Finding, Severity, location::Location}; @@ -97,6 +98,35 @@ fn build_results(findings: &[Finding]) -> Vec { fn build_result(finding: &Finding<'_>) -> SarifResult { let primary = finding.primary_location(); + // Build code flows for better visualization of location chains. + // GitHub renders these as step-by-step traces in security alerts. + let code_flows = { + let thread_flow_locations: Vec = finding + .visible_locations() + .map(|loc| { + let importance = if loc.symbolic.is_primary() { + "essential" + } else { + "important" + }; + ThreadFlowLocation::builder() + .location(build_location(loc, None)) + // See: + .importance(serde_json::Value::String(importance.into())) + .build() + }) + .collect(); + vec![ + CodeFlow::builder() + .thread_flows(vec![ + ThreadFlow::builder() + .locations(thread_flow_locations) + .build(), + ]) + .build(), + ] + }; + SarifResult::builder() .rule_id(format!("zizmor/{id}", id = finding.ident)) // NOTE: Between 1.4.0 and 1.9.0 we used the primary location's @@ -105,20 +135,9 @@ fn build_result(finding: &Finding<'_>) -> SarifResult { // code security alert titles when the primary annotation was // terse. So now we use the finding's description again, like // we did before 1.4.0. - .message(finding.desc) - .locations(build_locations(std::iter::once(primary))) - // TODO: Evaluate including the related locations via CodeFlows - // instead -- GitHub seems to do a better job of rendering these, - // but still doesn't do a great job of putting all of the locations - // into the same render. - // TODO: Give related locations IDs and back-link to them in the - // main location's message -- GitHub renders these as modals that - // users can click through to see more context. - .related_locations(build_locations( - finding - .visible_locations() - .filter(|l| !l.symbolic.is_primary()), - )) + .message(Message::builder().text(finding.desc).build()) + .locations(vec![build_location(primary, None)]) + .code_flows(code_flows) .level(ResultLevel::from(finding.determinations.severity)) .kind(ResultKind::from(finding.determinations.severity)) .properties( @@ -145,57 +164,67 @@ fn build_result(finding: &Finding<'_>) -> SarifResult { .build() } -fn build_locations<'a>(locations: impl Iterator>) -> Vec { - locations - .map(|location| { - SarifLocation::builder() - .logical_locations([LogicalLocation::builder() - .properties( - PropertyBag::builder() - .additional_properties([( - "symbolic".into(), - serde_json::value::to_value(location.symbolic.clone()) - .expect("failed to serialize symbolic location"), - )]) - .build(), - ) - .build()]) - .physical_location( - PhysicalLocation::builder() - .artifact_location( - ArtifactLocation::builder() - .uri(location.symbolic.key.sarif_path()) - .build(), - ) - .region( - Region::builder() - // NOTE: SARIF lines/columns are 1-based. - .start_line((location.concrete.location.start_point.row as i64) + 1) - .end_line((location.concrete.location.end_point.row as i64) + 1) - .start_column( - (location.concrete.location.start_point.column as i64) + 1, - ) - .end_column( - (location.concrete.location.end_point.column as i64) + 1, - ) - .source_language("yaml") - .snippet( - ArtifactContent::builder() - .text(location.concrete.feature) - .build(), - ) - .build(), - ) - .build(), - ) - .message( - Message::builder() - .text(location.symbolic.annotation.as_ref()) +fn build_physical_location(location: &Location<'_>) -> PhysicalLocation { + PhysicalLocation::builder() + .artifact_location( + ArtifactLocation::builder() + .uri(location.symbolic.key.sarif_path()) + .build(), + ) + .region( + Region::builder() + // NOTE: SARIF lines/columns are 1-based. + .start_line((location.concrete.location.start_point.row as i64) + 1) + .end_line((location.concrete.location.end_point.row as i64) + 1) + .start_column((location.concrete.location.start_point.column as i64) + 1) + .end_column((location.concrete.location.end_point.column as i64) + 1) + .source_language("yaml") + .snippet( + ArtifactContent::builder() + .text(location.concrete.feature) .build(), ) - .build() - }) - .collect() + .build(), + ) + .build() +} + +fn build_logical_locations(location: &Location<'_>) -> Vec { + vec![ + LogicalLocation::builder() + .properties( + PropertyBag::builder() + .additional_properties([( + "symbolic".into(), + serde_json::value::to_value(location.symbolic.clone()) + .expect("failed to serialize symbolic location"), + )]) + .build(), + ) + .build(), + ] +} + +fn build_location(location: &Location<'_>, id: Option) -> SarifLocation { + let message = Message::builder() + .text(location.symbolic.annotation.as_ref()) + .build(); + let physical = build_physical_location(location); + let logical = build_logical_locations(location); + + match id { + Some(id) => SarifLocation::builder() + .id(id) + .logical_locations(logical) + .physical_location(physical) + .message(message) + .build(), + None => SarifLocation::builder() + .logical_locations(logical) + .physical_location(physical) + .message(message) + .build(), + } } #[cfg(test)] diff --git a/crates/zizmor/tests/integration/audit/ref_version_mismatch.rs b/crates/zizmor/tests/integration/audit/ref_version_mismatch.rs index 924a20063..268f60f4a 100644 --- a/crates/zizmor/tests/integration/audit/ref_version_mismatch.rs +++ b/crates/zizmor/tests/integration/audit/ref_version_mismatch.rs @@ -22,7 +22,7 @@ fn test_ref_version_mismatch() -> Result<()> { = note: audit confidence → High = note: this finding has an auto-fix - warning[ref-version-mismatch]: detects commit SHAs that don't match their version comment tags + warning[ref-version-mismatch]: action's hash pin does not match version comment --> @@INPUT@@:22:77 | 22 | - uses: actions/setup-node@1a4442cacd436585916779262731d5b162bc6ec7 # v3.8.1 diff --git a/crates/zizmor/tests/integration/cli.rs b/crates/zizmor/tests/integration/cli.rs index a0863086a..9a67fa7eb 100644 --- a/crates/zizmor/tests/integration/cli.rs +++ b/crates/zizmor/tests/integration/cli.rs @@ -277,6 +277,69 @@ jobs: ], "results": [ { + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "importance": "essential", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "does not set persist-credentials: false", + "feature_kind": "Normal", + "key": { + "Stdin": {} + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "test" + }, + { + "Key": "steps" + }, + { + "Index": 0 + } + ] + } + } + } + } + ], + "message": { + "text": "does not set persist-credentials: false" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "" + }, + "region": { + "endColumn": 1, + "endLine": 7, + "snippet": { + "text": "uses: actions/checkout@v3\n" + }, + "sourceLanguage": "yaml", + "startColumn": 9, + "startLine": 6 + } + } + } + } + ] + } + ] + } + ], "kind": "fail", "level": "warning", "locations": [ @@ -339,10 +402,113 @@ jobs: "zizmor/persona": "Regular", "zizmor/severity": "Medium" }, - "relatedLocations": [], "ruleId": "zizmor/artipacked" }, { + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "importance": "important", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "this job", + "feature_kind": "Normal", + "key": { + "Stdin": {} + }, + "kind": "Related", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "test" + } + ] + } + } + } + } + ], + "message": { + "text": "this job" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "" + }, + "region": { + "endColumn": 1, + "endLine": 7, + "snippet": { + "text": " test:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v3\n" + }, + "sourceLanguage": "yaml", + "startColumn": 3, + "startLine": 3 + } + } + } + }, + { + "importance": "essential", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "default permissions used due to no permissions: block", + "feature_kind": "Normal", + "key": { + "Stdin": {} + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "test" + } + ] + } + } + } + } + ], + "message": { + "text": "default permissions used due to no permissions: block" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "" + }, + "region": { + "endColumn": 1, + "endLine": 7, + "snippet": { + "text": " test:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v3\n" + }, + "sourceLanguage": "yaml", + "startColumn": 3, + "startLine": 3 + } + } + } + } + ] + } + ] + } + ], "kind": "fail", "level": "warning", "locations": [ @@ -399,55 +565,82 @@ jobs: "zizmor/persona": "Regular", "zizmor/severity": "Medium" }, - "relatedLocations": [ + "ruleId": "zizmor/excessive-permissions" + }, + { + "codeFlows": [ { - "logicalLocations": [ + "threadFlows": [ { - "properties": { - "symbolic": { - "annotation": "this job", - "feature_kind": "Normal", - "key": { - "Stdin": {} - }, - "kind": "Related", - "route": { - "route": [ + "locations": [ + { + "importance": "essential", + "location": { + "logicalLocations": [ { - "Key": "jobs" + "properties": { + "symbolic": { + "annotation": "action is not pinned to a hash (required by blanket policy)", + "feature_kind": { + "Subfeature": { + "after": 0, + "fragment": { + "Raw": "actions/checkout@v3" + } + } + }, + "key": { + "Stdin": {} + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "test" + }, + { + "Key": "steps" + }, + { + "Index": 0 + }, + { + "Key": "uses" + } + ] + } + } + } + } + ], + "message": { + "text": "action is not pinned to a hash (required by blanket policy)" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "" }, - { - "Key": "test" + "region": { + "endColumn": 34, + "endLine": 6, + "snippet": { + "text": "actions/checkout@v3" + }, + "sourceLanguage": "yaml", + "startColumn": 15, + "startLine": 6 } - ] + } } } - } + ] } - ], - "message": { - "text": "this job" - }, - "physicalLocation": { - "artifactLocation": { - "uri": "" - }, - "region": { - "endColumn": 1, - "endLine": 7, - "snippet": { - "text": " test:\n runs-on: ubuntu-latest\n steps:\n - uses: actions/checkout@v3\n" - }, - "sourceLanguage": "yaml", - "startColumn": 3, - "startLine": 3 - } - } + ] } ], - "ruleId": "zizmor/excessive-permissions" - }, - { "kind": "fail", "level": "error", "locations": [ @@ -520,7 +713,6 @@ jobs: "zizmor/persona": "Regular", "zizmor/severity": "High" }, - "relatedLocations": [], "ruleId": "zizmor/unpinned-uses" } ], diff --git a/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__curl.snap b/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__curl.snap index e7d1275e7..2e42a09dc 100644 --- a/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__curl.snap +++ b/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__curl.snap @@ -21,7 +21,7 @@ expression: "zizmor().offline(false).output(OutputMode::Both).args([\"--persona= INFO audit: zizmor: 🌈 completed .github/workflows/macos.yml INFO audit: zizmor: 🌈 completed .github/workflows/non-native.yml INFO audit: zizmor: 🌈 completed .github/workflows/windows.yml -warning[ref-version-mismatch]: detects commit SHAs that don't match their version comment tags +warning[ref-version-mismatch]: action's hash pin does not match version comment --> .github/workflows/windows.yml:57:87 | 57 | - uses: cygwin/cygwin-install-action@f2009323764960f80959895c7bc3bb30210afe4d # v6 @@ -33,4 +33,3 @@ warning[ref-version-mismatch]: detects commit SHAs that don't match their versio = note: this finding has an auto-fix 47 findings (2 ignored, 44 suppressed): 0 informational, 0 low, 1 medium, 0 high - diff --git a/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__libssh2.snap b/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__libssh2.snap index f601dc55a..9ccba0896 100644 --- a/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__libssh2.snap +++ b/crates/zizmor/tests/integration/e2e/snapshots/integration__e2e__crater__libssh2.snap @@ -11,7 +11,7 @@ expression: "zizmor().offline(false).output(OutputMode::Both).args([\"--persona= INFO audit: zizmor: 🌈 completed .github/workflows/cifuzz.yml INFO audit: zizmor: 🌈 completed .github/workflows/codeql.yml INFO audit: zizmor: 🌈 completed .github/workflows/openssh_server.yml -warning[ref-version-mismatch]: detects commit SHAs that don't match their version comment tags +warning[ref-version-mismatch]: action's hash pin does not match version comment --> .github/workflows/ci.yml:658:87 | 658 | - uses: cygwin/cygwin-install-action@f2009323764960f80959895c7bc3bb30210afe4d # v6 @@ -23,4 +23,3 @@ warning[ref-version-mismatch]: detects commit SHAs that don't match their versio = note: this finding has an auto-fix 13 findings (2 ignored, 10 suppressed): 0 informational, 0 low, 1 medium, 0 high - diff --git a/crates/zizmor/tests/integration/snapshots/integration__e2e__sarif_zizmor_properties.snap b/crates/zizmor/tests/integration/snapshots/integration__e2e__sarif_zizmor_properties.snap index 7cefd8dff..8cb253956 100644 --- a/crates/zizmor/tests/integration/snapshots/integration__e2e__sarif_zizmor_properties.snap +++ b/crates/zizmor/tests/integration/snapshots/integration__e2e__sarif_zizmor_properties.snap @@ -13,6 +13,119 @@ expression: "zizmor().offline(true).input(input_under_test(\"several-vulnerabili ], "results": [ { + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "importance": "important", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "this job", + "feature_kind": "Normal", + "key": { + "Local": { + "given_path": "@@INPUT@@", + "prefix": null + } + }, + "kind": "Related", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "hackme" + } + ] + } + } + } + } + ], + "message": { + "text": "this job" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "@@INPUT@@" + }, + "region": { + "endColumn": 1, + "endLine": 17, + "snippet": { + "text": " hackme:\n name: hackme\n runs-on: ubuntu-latest\n permissions: write-all\n\n steps:\n - name: hackme\n run: |\n echo \"${{ github.event.pull_request.title }}\"\n" + }, + "sourceLanguage": "yaml", + "startColumn": 3, + "startLine": 8 + } + } + } + }, + { + "importance": "essential", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "uses write-all permissions", + "feature_kind": "Normal", + "key": { + "Local": { + "given_path": "@@INPUT@@", + "prefix": null + } + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "hackme" + }, + { + "Key": "permissions" + } + ] + } + } + } + } + ], + "message": { + "text": "uses write-all permissions" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "@@INPUT@@" + }, + "region": { + "endColumn": 27, + "endLine": 11, + "snippet": { + "text": " permissions: write-all" + }, + "sourceLanguage": "yaml", + "startColumn": 5, + "startLine": 11 + } + } + } + } + ] + } + ] + } + ], "kind": "fail", "level": "error", "locations": [ @@ -75,58 +188,66 @@ expression: "zizmor().offline(true).input(input_under_test(\"several-vulnerabili "zizmor/persona": "Regular", "zizmor/severity": "High" }, - "relatedLocations": [ + "ruleId": "zizmor/excessive-permissions" + }, + { + "codeFlows": [ { - "logicalLocations": [ + "threadFlows": [ { - "properties": { - "symbolic": { - "annotation": "this job", - "feature_kind": "Normal", - "key": { - "Local": { - "given_path": "@@INPUT@@", - "prefix": null - } - }, - "kind": "Related", - "route": { - "route": [ + "locations": [ + { + "importance": "essential", + "location": { + "logicalLocations": [ { - "Key": "jobs" + "properties": { + "symbolic": { + "annotation": "pull_request_target is almost always used insecurely", + "feature_kind": "Normal", + "key": { + "Local": { + "given_path": "@@INPUT@@", + "prefix": null + } + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "on" + } + ] + } + } + } + } + ], + "message": { + "text": "pull_request_target is almost always used insecurely" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "@@INPUT@@" }, - { - "Key": "hackme" + "region": { + "endColumn": 23, + "endLine": 3, + "snippet": { + "text": "on:\n pull_request_target:" + }, + "sourceLanguage": "yaml", + "startColumn": 1, + "startLine": 2 } - ] + } } } - } - } - ], - "message": { - "text": "this job" - }, - "physicalLocation": { - "artifactLocation": { - "uri": "@@INPUT@@" - }, - "region": { - "endColumn": 1, - "endLine": 17, - "snippet": { - "text": " hackme:\n name: hackme\n runs-on: ubuntu-latest\n permissions: write-all\n\n steps:\n - name: hackme\n run: |\n echo \"${{ github.event.pull_request.title }}\"\n" - }, - "sourceLanguage": "yaml", - "startColumn": 3, - "startLine": 8 + ] } - } + ] } ], - "ruleId": "zizmor/excessive-permissions" - }, - { "kind": "fail", "level": "error", "locations": [ @@ -183,10 +304,144 @@ expression: "zizmor().offline(true).input(input_under_test(\"several-vulnerabili "zizmor/persona": "Regular", "zizmor/severity": "High" }, - "relatedLocations": [], "ruleId": "zizmor/dangerous-triggers" }, { + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "importance": "essential", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "may expand into attacker-controllable code", + "feature_kind": { + "Subfeature": { + "after": 7, + "fragment": { + "Raw": "github.event.pull_request.title" + } + } + }, + "key": { + "Local": { + "given_path": "@@INPUT@@", + "prefix": null + } + }, + "kind": "Primary", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "hackme" + }, + { + "Key": "steps" + }, + { + "Index": 0 + }, + { + "Key": "run" + } + ] + } + } + } + } + ], + "message": { + "text": "may expand into attacker-controllable code" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "@@INPUT@@" + }, + "region": { + "endColumn": 52, + "endLine": 16, + "snippet": { + "text": "|\n echo \"${{ github.event.pull_request.title }}\"\n" + }, + "sourceLanguage": "yaml", + "startColumn": 21, + "startLine": 16 + } + } + } + }, + { + "importance": "important", + "location": { + "logicalLocations": [ + { + "properties": { + "symbolic": { + "annotation": "this run block", + "feature_kind": "KeyOnly", + "key": { + "Local": { + "given_path": "@@INPUT@@", + "prefix": null + } + }, + "kind": "Related", + "route": { + "route": [ + { + "Key": "jobs" + }, + { + "Key": "hackme" + }, + { + "Key": "steps" + }, + { + "Index": 0 + }, + { + "Key": "run" + } + ] + } + } + } + } + ], + "message": { + "text": "this run block" + }, + "physicalLocation": { + "artifactLocation": { + "uri": "@@INPUT@@" + }, + "region": { + "endColumn": 12, + "endLine": 15, + "snippet": { + "text": "run" + }, + "sourceLanguage": "yaml", + "startColumn": 9, + "startLine": 15 + } + } + } + } + ] + } + ] + } + ], "kind": "fail", "level": "error", "locations": [ @@ -262,64 +517,6 @@ expression: "zizmor().offline(true).input(input_under_test(\"several-vulnerabili "zizmor/persona": "Regular", "zizmor/severity": "High" }, - "relatedLocations": [ - { - "logicalLocations": [ - { - "properties": { - "symbolic": { - "annotation": "this run block", - "feature_kind": "KeyOnly", - "key": { - "Local": { - "given_path": "@@INPUT@@", - "prefix": null - } - }, - "kind": "Related", - "route": { - "route": [ - { - "Key": "jobs" - }, - { - "Key": "hackme" - }, - { - "Key": "steps" - }, - { - "Index": 0 - }, - { - "Key": "run" - } - ] - } - } - } - } - ], - "message": { - "text": "this run block" - }, - "physicalLocation": { - "artifactLocation": { - "uri": "@@INPUT@@" - }, - "region": { - "endColumn": 12, - "endLine": 15, - "snippet": { - "text": "run" - }, - "sourceLanguage": "yaml", - "startColumn": 9, - "startLine": 15 - } - } - } - ], "ruleId": "zizmor/template-injection" } ], diff --git a/docs/release-notes.md b/docs/release-notes.md index 523dc7c7a..3edb016c5 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -43,6 +43,12 @@ of `zizmor`. Many thanks to @deckstose for implementing this improvement! +* zizmor's SARIF output now uses codeflows instead of related locations, + improving its rendering behavior on GitHub Advanced Security (#1843) + +* The [ref-version-mismatch] audit now uses a more useful audit description + for its findings (#1843) + ### Bug Fixes 🐛 * Fixed a bug where the [concurrency-limits] audit reported findings