Refactor location handling (#2)

src/audit/artipacked.rs

@@ -1,3 +1,4 @@
+
 use anyhow::Result;
 use github_actions_models::{
     common::EnvValue,
@@ -6,11 +7,11 @@ use github_actions_models::{
 use itertools::Itertools;
 
 use crate::{
-    finding::{Confidence, Finding, Severity, StepLocation},
+    finding::{Confidence, Finding, Severity},
     models::AuditConfig,
 };
 use crate::{
-    finding::{Determinations, JobLocation, WorkflowLocation},
+    finding::{Determinations},
     models::Workflow,
 };
 
@@ -36,19 +37,19 @@ impl<'a> WorkflowAudit<'a> for Artipacked<'a> {
 
         let mut findings = vec![];
 
-        for (jobid, job) in workflow.jobs.iter() {
+        for job in workflow.jobs() {
             // Reusable workflows aren't checked, for now,
             // since we'd need to resolve their contents to determine
             // whether their interior steps are vulnerable.
-            let Job::NormalJob(job) = job else {
+            if !matches!(job.inner, Job::NormalJob(_)) {
                 continue;
-            };
+            }
 
             // First, collect all vulnerable checkouts and upload steps independently.
             let mut vulnerable_checkouts = vec![];
             let mut vulnerable_uploads = vec![];
-            for (stepno, step) in job.steps.iter().enumerate() {
-                let StepBody::Uses { uses, with } = &step.body else {
+            for step in job.steps() {
+                let StepBody::Uses { ref uses, ref with } = &step.inner.body else {
                     continue;
                 };
 
@@ -59,23 +60,21 @@ impl<'a> WorkflowAudit<'a> for Artipacked<'a> {
                             // If a user explicitly sets `persist-credentials: true`,
                             // they probably mean it. Only report if being pedantic.
                             if self.config.pedantic {
-                                vulnerable_checkouts.push(StepLocation::new(stepno, step))
+                                vulnerable_checkouts.push(step)
                             } else {
                                 continue;
                             }
                         }
                         // TODO: handle expressions and literal strings here.
                         // persist-credentials is true by default.
-                        _ => vulnerable_checkouts.push(StepLocation::new(stepno, step)),
+                        _ => vulnerable_checkouts.push(step),
                     }
-                }
-
-                if uses.starts_with("actions/upload-artifact") {
+                } else if uses.starts_with("actions/upload-artifact") {
                     match with.get("path") {
                         // TODO: This is pretty naive -- we should also flag on
                         // `${{ expressions }}` and absolute paths, etc.
                         Some(EnvValue::String(s)) if s == "." || s == ".." => {
-                            vulnerable_uploads.push(StepLocation::new(stepno, step))
+                            vulnerable_uploads.push(step)
                         }
                         _ => continue,
                     }
@@ -92,14 +91,7 @@ impl<'a> WorkflowAudit<'a> for Artipacked<'a> {
                             severity: Severity::Medium,
                             confidence: Confidence::Low,
                         },
-                        location: WorkflowLocation {
-                            name: workflow.filename.clone(),
-                            jobs: vec![JobLocation {
-                                id: jobid,
-                                name: job.name.as_deref(),
-                                steps: vec![checkout],
-                            }],
-                        },
+                        locations: vec![checkout.location().clone()],
                     })
                 }
             } else {
@@ -117,14 +109,7 @@ impl<'a> WorkflowAudit<'a> for Artipacked<'a> {
                                 severity: Severity::High,
                                 confidence: Confidence::High,
                             },
-                            location: WorkflowLocation {
-                                name: workflow.filename.clone(),
-                                jobs: vec![JobLocation {
-                                    id: jobid,
-                                    name: job.name.as_deref(),
-                                    steps: vec![checkout, upload],
-                                }],
-                            },
+                            locations: vec![checkout.location().clone(), upload.location().clone()],
                         });
                     }
                 }

src/audit/impostor_commit.rs

@@ -112,11 +112,11 @@ impl<'a> WorkflowAudit<'a> for ImpostorCommit<'a> {
 
         let mut findings = vec![];
 
-        for (jobid, job) in workflow.jobs.iter() {
-            match job {
-                Job::NormalJob(job) => {
-                    for (stepno, step) in job.steps.iter().enumerate() {
-                        let StepBody::Uses { uses, .. } = &step.body else {
+        for job in workflow.jobs() {
+            match job.inner {
+                Job::NormalJob(_) => {
+                    for step in job.steps() {
+                        let StepBody::Uses { uses, .. } = &step.inner.body else {
                             continue;
                         };
 
@@ -131,22 +131,16 @@ impl<'a> WorkflowAudit<'a> for ImpostorCommit<'a> {
                                     severity: Severity::High,
                                     confidence: Confidence::High,
                                 },
-                                location: WorkflowLocation {
-                                    name: workflow.filename.clone(),
-                                    jobs: vec![JobLocation {
-                                        id: jobid,
-                                        name: job.name.as_deref(),
-                                        steps: vec![StepLocation::new(stepno, step)],
-                                    }],
-                                },
+                                locations: vec![step.location()],
                             })
                         }
                     }
                 }
-                Job::ReusableWorkflowCallJob(job) => {
+                Job::ReusableWorkflowCallJob(reusable) => {
                     // Reusable workflows can also be commit pinned, meaning
                     // they can also be impersonated.
-                    let Some((owner, org, commit)) = reusable_workflow_components(&job.uses) else {
+                    let Some((owner, org, commit)) = reusable_workflow_components(&reusable.uses)
+                    else {
                         continue;
                     };
 
@@ -157,14 +151,7 @@ impl<'a> WorkflowAudit<'a> for ImpostorCommit<'a> {
                                 severity: Severity::High,
                                 confidence: Confidence::High,
                             },
-                            location: WorkflowLocation {
-                                name: workflow.filename.clone(),
-                                jobs: vec![JobLocation {
-                                    id: jobid,
-                                    name: job.name.as_deref(),
-                                    steps: vec![],
-                                }],
-                            },
+                            locations: vec![job.location()],
                         })
                     }
                 }

src/audit/pull_request_target.rs

@@ -2,7 +2,7 @@ use anyhow::Result;
 use github_actions_models::workflow::event::{BareEvent, OptionalBody};
 use github_actions_models::workflow::Trigger;
 
-use crate::finding::{Confidence, Determinations, Finding, Severity, WorkflowLocation};
+use crate::finding::{Confidence, Determinations, Finding, Severity};
 use crate::models::{AuditConfig, Workflow};
 
 use super::WorkflowAudit;
@@ -41,10 +41,7 @@ impl<'a> WorkflowAudit<'a> for PullRequestTarget<'a> {
                     confidence: Confidence::Medium,
                     severity: Severity::High,
                 },
-                location: WorkflowLocation {
-                    name: workflow.filename.clone(),
-                    jobs: vec![],
-                },
+                locations: vec![workflow.location()],
             })
         }
 

src/finding.rs

@@ -1,6 +1,7 @@
-use github_actions_models::workflow::job::Step;
 use serde::Serialize;
 
+use crate::models::{Job, Step};
+
 // TODO: Traits + more flexible models here.
 
 #[derive(Serialize)]
@@ -19,33 +20,67 @@ pub(crate) enum Severity {
 }
 
 #[derive(Serialize, Clone)]
-pub(crate) struct StepLocation {
+pub(crate) struct StepLocation<'w> {
     pub(crate) index: usize,
-    pub(crate) id: Option<String>,
-    pub(crate) name: Option<String>,
+    pub(crate) id: Option<&'w str>,
+    pub(crate) name: Option<&'w str>,
 }
 
-impl StepLocation {
-    pub(crate) fn new(index: usize, step: &Step) -> Self {
+impl<'w> From<&Step<'w>> for StepLocation<'w> {
+    fn from(step: &Step<'w>) -> Self {
         Self {
-            index,
-            id: step.id.clone(),
-            name: step.name.clone(),
+            index: step.index,
+            id: step.inner.id.as_deref(),
+            name: step.inner.name.as_deref(),
         }
     }
 }
 
-#[derive(Serialize)]
+#[derive(Serialize, Clone)]
 pub(crate) struct JobLocation<'w> {
     pub(crate) id: &'w str,
     pub(crate) name: Option<&'w str>,
-    pub(crate) steps: Vec<StepLocation>,
+    pub(crate) step: Option<StepLocation<'w>>,
 }
 
-#[derive(Serialize)]
+impl<'w> JobLocation<'w> {
+    fn with_step(&self, step: &Step<'w>) -> JobLocation<'w> {
+        JobLocation {
+            id: self.id,
+            name: self.name,
+            step: Some(step.into()),
+        }
+    }
+}
+
+#[derive(Serialize, Clone)]
 pub(crate) struct WorkflowLocation<'w> {
-    pub(crate) name: String,
-    pub(crate) jobs: Vec<JobLocation<'w>>,
+    pub(crate) name: &'w str,
+    /// The job location within this workflow, if present.
+    pub(crate) job: Option<JobLocation<'w>>,
+}
+
+impl<'w> WorkflowLocation<'w> {
+    pub(crate) fn with_job(&self, job: &Job<'w>) -> WorkflowLocation<'w> {
+        WorkflowLocation {
+            name: self.name,
+            job: Some(JobLocation {
+                id: job.id,
+                name: job.inner.name(),
+                step: None,
+            }),
+        }
+    }
+
+    pub(crate) fn with_step(&self, step: &Step<'w>) -> WorkflowLocation<'w> {
+        match &self.job {
+            None => panic!("API misuse: can't set step without parent job"),
+            Some(job) => WorkflowLocation {
+                name: self.name,
+                job: Some(job.with_step(step)),
+            },
+        }
+    }
 }
 
 #[derive(Serialize)]
@@ -58,5 +93,5 @@ pub(crate) struct Determinations {
 pub(crate) struct Finding<'w> {
     pub(crate) ident: &'static str,
     pub(crate) determinations: Determinations,
-    pub(crate) location: WorkflowLocation<'w>,
+    pub(crate) locations: Vec<WorkflowLocation<'w>>,
 }

src/main.rs

@@ -1,10 +1,10 @@
-use std::{io::stdout, path::PathBuf};
+use std::{path::PathBuf};
 
 use anyhow::{anyhow, Result};
 use audit::WorkflowAudit;
 use clap::Parser;
 use models::AuditConfig;
-use serde_jsonlines::{AsyncJsonLinesWriter, JsonLinesWriter};
+use serde_jsonlines::{AsyncJsonLinesWriter};
 
 mod audit;
 mod finding;

src/models.rs

@@ -1,8 +1,10 @@
 use anyhow::{Context, Ok, Result};
-use std::{ops::Deref, path::Path};
+use std::{collections::hash_map, iter::Enumerate, ops::Deref, path::Path};
 
 use github_actions_models::workflow;
 
+use crate::finding::WorkflowLocation;
+
 pub(crate) struct Workflow {
     pub(crate) filename: String,
     inner: workflow::Workflow,
@@ -33,6 +35,122 @@ impl Workflow {
             inner,
         })
     }
+
+    pub(crate) fn location(&self) -> WorkflowLocation {
+        WorkflowLocation {
+            name: &self.filename,
+            job: None,
+        }
+    }
+
+    pub(crate) fn jobs(&self) -> Jobs<'_> {
+        Jobs::new(self)
+    }
+}
+
+pub(crate) struct Job<'w> {
+    pub(crate) id: &'w str,
+    pub(crate) inner: &'w workflow::Job,
+    parent: WorkflowLocation<'w>,
+}
+
+impl<'w> Job<'w> {
+    pub(crate) fn new(id: &'w str, inner: &'w workflow::Job, parent: WorkflowLocation<'w>) -> Self {
+        Self { id, inner, parent }
+    }
+
+    pub(crate) fn location(&self) -> WorkflowLocation<'w> {
+        self.parent.with_job(self)
+    }
+
+    pub(crate) fn steps(&self) -> Steps<'w> {
+        Steps::new(self)
+    }
+}
+
+pub(crate) struct Jobs<'w> {
+    inner: hash_map::Iter<'w, String, workflow::Job>,
+    location: WorkflowLocation<'w>,
+}
+
+impl<'w> Jobs<'w> {
+    pub(crate) fn new(workflow: &'w Workflow) -> Self {
+        Self {
+            inner: workflow.jobs.iter(),
+            location: workflow.location(),
+        }
+    }
+}
+
+impl<'w> Iterator for Jobs<'w> {
+    type Item = Job<'w>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let item = self.inner.next();
+
+        match item {
+            Some((id, job)) => Some(Job::new(id, job, self.location.clone())),
+            None => None,
+        }
+    }
+}
+
+#[derive(Clone)]
+pub(crate) struct Step<'w> {
+    pub(crate) index: usize,
+    pub(crate) inner: &'w workflow::job::Step,
+    parent: WorkflowLocation<'w>,
+}
+
+impl<'w> Step<'w> {
+    pub(crate) fn new(
+        index: usize,
+        inner: &'w workflow::job::Step,
+        parent: WorkflowLocation<'w>,
+    ) -> Self {
+        Self {
+            index,
+            inner,
+            parent,
+        }
+    }
+
+    pub(crate) fn location(&self) -> WorkflowLocation<'w> {
+        self.parent.with_step(self)
+    }
+}
+
+pub(crate) struct Steps<'w> {
+    inner: Enumerate<std::slice::Iter<'w, github_actions_models::workflow::job::Step>>,
+    location: WorkflowLocation<'w>,
+}
+
+impl<'w> Steps<'w> {
+    pub(crate) fn new(job: &Job<'w>) -> Self {
+        // TODO: do something less silly here.
+        match &job.inner {
+            workflow::Job::ReusableWorkflowCallJob(_) => {
+                panic!("API misuse: can't call steps() on a reusable job")
+            }
+            workflow::Job::NormalJob(ref n) => Self {
+                inner: n.steps.iter().enumerate(),
+                location: job.location(),
+            },
+        }
+    }
+}
+
+impl<'w> Iterator for Steps<'w> {
+    type Item = Step<'w>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let item = self.inner.next();
+
+        match item {
+            Some((idx, step)) => Some(Step::new(idx, step, self.location.clone())),
+            None => None,
+        }
+    }
 }
 
 #[derive(Copy, Clone)]