refactor: add 'primary' locations (#328)

Signed-off-by: William Woodruff <william@yossarian.net>

src/audit/artipacked.rs

@@ -105,6 +105,7 @@ impl WorkflowAudit for Artipacked {
                             .add_location(
                                 checkout
                                     .location()
+                                    .primary()
                                     .annotated("does not set persist-credentials: false"),
                             )
                             .build(workflow)?,
@@ -127,6 +128,7 @@ impl WorkflowAudit for Artipacked {
                                 .add_location(
                                     checkout
                                         .location()
+                                        .primary()
                                         .annotated("does not set persist-credentials: false"),
                                 )
                                 .add_location(

src/audit/dangerous_triggers.rs

@@ -28,6 +28,7 @@ impl WorkflowAudit for DangerousTriggers {
                     .add_location(
                         workflow
                             .location()
+                            .primary()
                             .with_keys(&["on".into()])
                             .annotated("pull_request_target is almost always used insecurely"),
                     )
@@ -42,6 +43,7 @@ impl WorkflowAudit for DangerousTriggers {
                     .add_location(
                         workflow
                             .location()
+                            .primary()
                             .with_keys(&["on".into()])
                             .annotated("workflow_run is almost always used insecurely"),
                     )

src/audit/excessive_permissions.rs

@@ -65,6 +65,7 @@ impl WorkflowAudit for ExcessivePermissions {
                     .add_location(
                         workflow
                             .location()
+                            .primary()
                             .with_keys(&["permissions".into()])
                             .annotated(note),
                     )
@@ -86,6 +87,7 @@ impl WorkflowAudit for ExcessivePermissions {
                         .confidence(confidence)
                         .add_location(
                             job.location()
+                                .primary()
                                 .with_keys(&["permissions".into()])
                                 .annotated(note),
                         )

src/audit/github_env.rs

@@ -183,6 +183,7 @@ impl WorkflowAudit for GitHubEnv {
                         .confidence(Confidence::Low)
                         .add_location(
                             step.location()
+                                .primary()
                                 .with_keys(&["run".into()])
                                 .annotated("GITHUB_ENV write may allow code execution"),
                         )

src/audit/hardcoded_container_credentials.rs

@@ -59,6 +59,7 @@ impl WorkflowAudit for HardcodedContainerCredentials {
                             .confidence(Confidence::High)
                             .add_location(
                                 job.location()
+                                    .primary()
                                     .with_keys(&["container".into(), "credentials".into()])
                                     .annotated("container registry password is hard-coded"),
                             )
@@ -85,6 +86,7 @@ impl WorkflowAudit for HardcodedContainerCredentials {
                                 .confidence(Confidence::High)
                                 .add_location(
                                     job.location()
+                                        .primary()
                                         .with_keys(&[
                                             "services".into(),
                                             service.as_str().into(),

src/audit/impostor_commit.rs

@@ -141,7 +141,9 @@ impl WorkflowAudit for ImpostorCommit {
                                 Self::finding()
                                     .severity(Severity::High)
                                     .confidence(Confidence::High)
-                                    .add_location(step.location().annotated(IMPOSTOR_ANNOTATION))
+                                    .add_location(
+                                        step.location().primary().annotated(IMPOSTOR_ANNOTATION),
+                                    )
                                     .build(workflow)?,
                             );
                         }
@@ -159,7 +161,9 @@ impl WorkflowAudit for ImpostorCommit {
                             Self::finding()
                                 .severity(Severity::High)
                                 .confidence(Confidence::High)
-                                .add_location(job.location().annotated(IMPOSTOR_ANNOTATION))
+                                .add_location(
+                                    job.location().primary().annotated(IMPOSTOR_ANNOTATION),
+                                )
                                 .build(workflow)?,
                         );
                     }

src/audit/insecure_commands.rs

@@ -30,7 +30,7 @@ impl InsecureCommands {
             .severity(Severity::High)
             .persona(Persona::Auditor)
             .add_location(
-                location.with_keys(&["env".into()]).annotated(
+                location.primary().with_keys(&["env".into()]).annotated(
                     "non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS",
                 ),
             )
@@ -47,6 +47,7 @@ impl InsecureCommands {
             .severity(Severity::High)
             .add_location(
                 location
+                    .primary()
                     .with_keys(&["env".into()])
                     .annotated("insecure commands enabled here"),
             )

src/audit/known_vulnerable_actions.rs

@@ -150,6 +150,7 @@ impl WorkflowAudit for KnownVulnerableActions {
                     .severity(severity)
                     .add_location(
                         step.location()
+                            .primary()
                             .with_keys(&["uses".into()])
                             .annotated(&id)
                             .with_url(format!("https://github.com/advisories/{id}")),

src/audit/ref_confusion.rs

@@ -84,6 +84,7 @@ impl WorkflowAudit for RefConfusion {
                                     .confidence(Confidence::High)
                                     .add_location(
                                         step.location()
+                                            .primary()
                                             .with_keys(&["uses".into()])
                                             .annotated(REF_CONFUSION_ANNOTATION),
                                     )
@@ -102,7 +103,9 @@ impl WorkflowAudit for RefConfusion {
                             Self::finding()
                                 .severity(Severity::Medium)
                                 .confidence(Confidence::High)
-                                .add_location(job.location().annotated(REF_CONFUSION_ANNOTATION))
+                                .add_location(
+                                    job.location().primary().annotated(REF_CONFUSION_ANNOTATION),
+                                )
                                 .build(workflow)?,
                         )
                     }

src/audit/self_hosted_runner.rs

@@ -62,6 +62,7 @@ impl WorkflowAudit for SelfHostedRunner {
                                     .persona(Persona::Auditor)
                                     .add_location(
                                         job.location()
+                                            .primary()
                                             .with_keys(&["runs-on".into()])
                                             .annotated("self-hosted runner used here"),
                                     )
@@ -78,9 +79,12 @@ impl WorkflowAudit for SelfHostedRunner {
                                     .severity(Severity::Unknown)
                                     .persona(Persona::Auditor)
                                     .add_location(
-                                        job.location().with_keys(&["runs-on".into()]).annotated(
-                                            "expression may expand into a self-hosted runner",
-                                        ),
+                                        job.location()
+                                            .primary()
+                                            .with_keys(&["runs-on".into()])
+                                            .annotated(
+                                                "expression may expand into a self-hosted runner",
+                                            ),
                                     )
                                     .build(workflow)?,
                             );
@@ -99,6 +103,7 @@ impl WorkflowAudit for SelfHostedRunner {
                         .persona(Persona::Auditor)
                         .add_location(
                             job.location()
+                                .primary()
                                 .with_keys(&["runs-on".into()])
                                 .annotated("runner group implies self-hosted runner"),
                         )
@@ -129,9 +134,12 @@ impl WorkflowAudit for SelfHostedRunner {
                                         .annotated("matrix declares self-hosted runner"),
                                 )
                                 .add_location(
-                                    job.location().with_keys(&["runs-on".into()]).annotated(
-                                        "expression may expand into a self-hosted runner",
-                                    ),
+                                    job.location()
+                                        .primary()
+                                        .with_keys(&["runs-on".into()])
+                                        .annotated(
+                                            "expression may expand into a self-hosted runner",
+                                        ),
                                 )
                                 .build(workflow)?,
                         )

src/audit/template_injection.rs

@@ -266,7 +266,7 @@ impl WorkflowAudit for TemplateInjection {
                     .persona(persona)
                     .add_location(step.location_with_name())
                     .add_location(
-                        script_loc.clone().annotated(format!(
+                        script_loc.clone().primary().annotated(format!(
                             "{expr} may expand into attacker-controllable code"
                         )),
                     )

src/audit/unpinned_uses.rs

@@ -44,6 +44,7 @@ impl WorkflowAudit for UnpinnedUses {
                 .persona(persona)
                 .add_location(
                     step.location()
+                        .primary()
                         .with_keys(&["uses".into()])
                         .annotated(annotation),
                 )

src/audit/use_trusted_publishing.rs

@@ -87,6 +87,7 @@ impl WorkflowAudit for UseTrustedPublishing {
             .confidence(Confidence::High)
             .add_location(
                 step.location()
+                    .primary()
                     .with_keys(&["uses".into()])
                     .annotated("this step"),
             );
@@ -98,6 +99,7 @@ impl WorkflowAudit for UseTrustedPublishing {
                 candidate
                     .add_location(
                         step.location()
+                            .primary()
                             .with_keys(&["with".into(), "password".into()])
                             .annotated(USES_MANUAL_CREDENTIAL),
                     )
@@ -110,7 +112,7 @@ impl WorkflowAudit for UseTrustedPublishing {
         {
             findings.push(
                 candidate
-                    .add_location(step.location().annotated(USES_MANUAL_CREDENTIAL))
+                    .add_location(step.location().primary().annotated(USES_MANUAL_CREDENTIAL))
                     .build(step.workflow())?,
             );
         }

src/finding/mod.rs

@@ -2,7 +2,7 @@
 
 use std::{borrow::Cow, sync::LazyLock};
 
-use anyhow::Result;
+use anyhow::{anyhow, Result};
 use clap::ValueEnum;
 use locate::Locator;
 use regex::Regex;
@@ -137,6 +137,13 @@ pub(crate) struct SymbolicLocation<'w> {
 
     /// A symbolic route (of keys and indices) to the final location.
     pub(crate) route: Route<'w>,
+
+    /// Whether this location is subjectively "primary" to a finding,
+    /// or merely a "supporting" location.
+    ///
+    /// This distinction only matters in output formats like SARIF,
+    /// where locations are split between locations and "related" locations.
+    pub(crate) primary: bool,
 }
 
 impl<'w> SymbolicLocation<'w> {
@@ -146,6 +153,7 @@ impl<'w> SymbolicLocation<'w> {
             annotation: self.annotation.clone(),
             link: None,
             route: self.route.with_keys(keys),
+            primary: self.primary,
         }
     }
 
@@ -169,6 +177,12 @@ impl<'w> SymbolicLocation<'w> {
         self
     }
 
+    /// Mark the current `SymbolicLocation` as a "primary" location.
+    pub(crate) fn primary(mut self) -> SymbolicLocation<'w> {
+        self.primary = true;
+        self
+    }
+
     /// Concretize this `SymbolicLocation`, consuming it in the process.
     pub(crate) fn concretize(self, workflow: &'w Workflow) -> Result<Location<'w>> {
         let feature = Locator::new().concretize(workflow, &self)?;
@@ -336,6 +350,12 @@ impl<'w> FindingBuilder<'w> {
             .map(|l| l.clone().concretize(workflow))
             .collect::<Result<Vec<_>>>()?;
 
+        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);
 
         Ok(Finding {

src/models.rs

@@ -95,6 +95,7 @@ impl Workflow {
             annotation: "this workflow".to_string(),
             link: None,
             route: Route::new(),
+            primary: false,
         }
     }
 

src/sarif.rs

@@ -72,7 +72,14 @@ fn build_result(registry: &WorkflowRegistry, finding: &Finding<'_>) -> SarifResu
     SarifResult::builder()
         .message(finding.ident)
         .rule_id(finding.ident)
-        .locations(build_locations(registry, &finding.locations))
+        .locations(build_locations(
+            registry,
+            finding.locations.iter().filter(|l| l.symbolic.primary),
+        ))
+        .related_locations(build_locations(
+            registry,
+            finding.locations.iter().filter(|l| !l.symbolic.primary),
+        ))
         .level(
             serde_json::to_value(ResultLevel::from(finding.determinations.severity))
                 .expect("failed to serialize SARIF result level"),
@@ -84,9 +91,11 @@ fn build_result(registry: &WorkflowRegistry, finding: &Finding<'_>) -> SarifResu
         .build()
 }
 
-fn build_locations(registry: &WorkflowRegistry, locations: &[Location<'_>]) -> Vec<SarifLocation> {
+fn build_locations<'a>(
+    registry: &WorkflowRegistry,
+    locations: impl Iterator<Item = &'a Location<'a>>,
+) -> Vec<SarifLocation> {
     locations
-        .iter()
         .map(|location| {
             SarifLocation::builder()
                 .logical_locations([LogicalLocation::builder()