bugfix: generalize path prefix handling (#469)

docs/release-notes.md

@@ -11,6 +11,11 @@ of `zizmor`.
 
 Nothing to see here (yet!)
 
+### Bug Fixes 🐛
+
+* SARIF outputs now use relative paths again, but more correctly
+  than before [v1.2.0](#v120) (#469)
+
 ## v1.2.0
 
 This release comes with one new audit ([bot-conditions]), plus a handful

src/audit/bot_conditions.rs

@@ -1,13 +1,12 @@
 use github_actions_models::common::{expr::ExplicitExpr, If};
 
+use super::{audit_meta, Audit};
 use crate::{
     expr::{self, Expr},
     finding::{Confidence, Severity},
     models::JobExt,
 };
 
-use super::{audit_meta, Audit};
-
 pub(crate) struct BotConditions;
 
 audit_meta!(BotConditions, "bot-conditions", "spoofable bot actor check");
@@ -152,7 +151,7 @@ impl BotConditions {
             // We have a bot condition but it doesn't dominate the expression.
             (true, false) => Some(Confidence::Medium),
             // No bot condition.
-            (_, _) => None,
+            (..) => None,
         }
     }
 }

src/main.rs

@@ -160,17 +160,18 @@ fn tip(err: impl AsRef<str>, tip: impl AsRef<str>) -> String {
 
 #[instrument(skip(mode, registry))]
 fn collect_from_repo_dir(
-    repo_dir: &Utf8Path,
+    top_dir: &Utf8Path,
+    current_dir: &Utf8Path,
     mode: &CollectionMode,
     registry: &mut InputRegistry,
 ) -> Result<()> {
     // The workflow directory might not exist if we're collecting from
     // a repository that only contains actions.
     if mode.workflows() {
-        let workflow_dir = if repo_dir.ends_with(".github/workflows") {
-            repo_dir.into()
+        let workflow_dir = if current_dir.ends_with(".github/workflows") {
+            current_dir.into()
         } else {
-            repo_dir.join(".github/workflows")
+            current_dir.join(".github/workflows")
         };
 
         if workflow_dir.is_dir() {
@@ -180,7 +181,7 @@ fn collect_from_repo_dir(
                 match input_path.extension() {
                     Some(ext) if ext == "yml" || ext == "yaml" => {
                         registry
-                            .register_by_path(input_path)
+                            .register_by_path(input_path, Some(top_dir))
                             .with_context(|| format!("failed to register input: {input_path}"))?;
                     }
                     _ => continue,
@@ -192,18 +193,18 @@ fn collect_from_repo_dir(
     }
 
     if mode.actions() {
-        for entry in repo_dir.read_dir_utf8()? {
+        for entry in current_dir.read_dir_utf8()? {
             let entry = entry?;
             let entry_path = entry.path();
 
             if entry_path.is_file()
                 && matches!(entry_path.file_name(), Some("action.yml" | "action.yaml"))
             {
-                let action = Action::from_file(entry_path)?;
+                let action = Action::from_file(entry_path, Some(top_dir))?;
                 registry.register_input(action.into())?;
             } else if entry_path.is_dir() && !entry_path.ends_with(".github/workflows") {
                 // Recurse and limit the collection mode to only actions.
-                collect_from_repo_dir(entry_path, &CollectionMode::ActionsOnly, registry)?;
+                collect_from_repo_dir(top_dir, entry_path, &CollectionMode::ActionsOnly, registry)?;
             }
         }
     }
@@ -285,13 +286,13 @@ fn collect_inputs(
     for input in inputs {
         let input_path = Utf8Path::new(input);
         if input_path.is_file() {
+            // When collecting individual files, we don't know which part
+            // of the input path is the prefix.
             registry
-                .register_by_path(input_path)
+                .register_by_path(input_path, None)
                 .with_context(|| format!("failed to register input: {input_path}"))?;
         } else if input_path.is_dir() {
-            // TODO: walk directory to discover composite actions.
-            let absolute = input_path.canonicalize_utf8()?;
-            collect_from_repo_dir(&absolute, mode, &mut registry)?;
+            collect_from_repo_dir(input_path, input_path, mode, &mut registry)?;
         } else {
             // If this input isn't a file or directory, it's probably an
             // `owner/repo(@ref)?` slug.
@@ -387,7 +388,10 @@ fn run() -> Result<ExitCode> {
                 })?);
                 Span::current().pb_inc(1);
             }
-            tracing::info!("🌈 completed {input}", input = input.key().path());
+            tracing::info!(
+                "🌈 completed {input}",
+                input = input.key().best_effort_relative_path()
+            );
         }
     }
 

src/models.rs

@@ -101,7 +101,7 @@ impl Workflow {
             InputKey::Local(_) => None,
             InputKey::Remote(_) => {
                 // NOTE: InputKey's Display produces a URL, hence `key.to_string()`.
-                Some(Link::new(key.path(), &key.to_string()).to_string())
+                Some(Link::new(key.best_effort_relative_path(), &key.to_string()).to_string())
             }
         };
 
@@ -114,11 +114,9 @@ impl Workflow {
     }
 
     /// Load a workflow from the given file on disk.
-    pub(crate) fn from_file<P: AsRef<Utf8Path>>(p: P) -> Result<Self> {
-        let contents = std::fs::read_to_string(p.as_ref())?;
-        let path = p.as_ref().canonicalize_utf8()?;
-
-        Self::from_string(contents, InputKey::local(path)?)
+    pub(crate) fn from_file<P: AsRef<Utf8Path>>(path: P, prefix: Option<P>) -> Result<Self> {
+        let contents = std::fs::read_to_string(path.as_ref())?;
+        Self::from_string(contents, InputKey::local(path, prefix)?)
     }
 
     /// This workflow's [`SymbolicLocation`].
@@ -716,13 +714,10 @@ impl Debug for Action {
 
 impl Action {
     /// Load an action from the given file on disk.
-    pub(crate) fn from_file<P: AsRef<Utf8Path>>(p: P) -> Result<Self> {
+    pub(crate) fn from_file<P: AsRef<Utf8Path>>(path: P, prefix: Option<P>) -> Result<Self> {
         let contents =
-            std::fs::read_to_string(p.as_ref()).with_context(|| "couldn't read action file")?;
-
-        let path = p.as_ref().canonicalize_utf8()?;
-
-        Self::from_string(contents, InputKey::local(path)?)
+            std::fs::read_to_string(path.as_ref()).with_context(|| "couldn't read action file")?;
+        Self::from_string(contents, InputKey::local(path, prefix)?)
     }
 
     /// Load a workflow from a buffer, with an assigned name.
@@ -736,7 +731,7 @@ impl Action {
             InputKey::Local(_) => None,
             InputKey::Remote(_) => {
                 // NOTE: InputKey's Display produces a URL, hence `key.to_string()`.
-                Some(Link::new(key.path(), &key.to_string()).to_string())
+                Some(Link::new(key.best_effort_relative_path(), &key.to_string()).to_string())
             }
         };
 

src/registry.rs

@@ -20,7 +20,10 @@ use crate::{
 
 #[derive(Debug, Clone, Eq, Hash, PartialEq, Serialize)]
 pub(crate) struct LocalKey {
-    path: Utf8PathBuf,
+    /// The path's nondeterministic prefix, if any.
+    prefix: Option<Utf8PathBuf>,
+    /// The given path to the input. This can be absolute or relative.
+    given_path: Utf8PathBuf,
 }
 
 #[derive(Debug, Clone, Eq, Hash, PartialEq, Serialize)]
@@ -45,7 +48,7 @@ pub(crate) enum InputKey {
 impl Display for InputKey {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            InputKey::Local(local) => write!(f, "file://{path}", path = local.path),
+            InputKey::Local(local) => write!(f, "file://{path}", path = local.given_path),
             InputKey::Remote(remote) => {
                 // No ref means assume HEAD, i.e. whatever's on the default branch.
                 let git_ref = remote.git_ref.as_deref().unwrap_or("HEAD");
@@ -62,13 +65,16 @@ impl Display for InputKey {
 }
 
 impl InputKey {
-    pub(crate) fn local(path: Utf8PathBuf) -> Result<Self> {
+    pub(crate) fn local<P: AsRef<Utf8Path>>(path: P, prefix: Option<P>) -> Result<Self> {
         // All keys must have a filename component.
-        if path.file_name().is_none() {
+        if path.as_ref().file_name().is_none() {
             return Err(anyhow!("invalid local input: no filename component"));
         }
 
-        Ok(Self::Local(LocalKey { path }))
+        Ok(Self::Local(LocalKey {
+            prefix: prefix.map(|p| p.as_ref().to_path_buf()),
+            given_path: path.as_ref().to_path_buf(),
+        }))
     }
 
     pub(crate) fn remote(slug: &RepositoryUses, path: String) -> Result<Self> {
@@ -84,13 +90,18 @@ impl InputKey {
         }))
     }
 
-    /// Returns this [`InputKey`]'s filepath component.
+    /// Return a "best-effort" relative path for this [`InputKey`].
     ///
-    /// This will be an absolute path for local keys, and a relative
-    /// path for remote keys.
-    pub(crate) fn path(&self) -> &str {
+    /// This will always be a relative path for remote keys,
+    /// and will be a "best-effort" relative path for local keys.
+    pub(crate) fn best_effort_relative_path(&self) -> &str {
         match self {
-            InputKey::Local(local) => local.path.as_str(),
+            InputKey::Local(local) => local
+                .prefix
+                .as_ref()
+                .and_then(|pfx| local.given_path.strip_prefix(dbg!(pfx)).ok())
+                .unwrap_or_else(|| &local.given_path)
+                .as_str(),
             InputKey::Remote(remote) => remote.path.as_str(),
         }
     }
@@ -100,16 +111,14 @@ impl InputKey {
         // NOTE: Safe unwraps, since the presence of a filename component
         // is a construction invariant of all `InputKey` variants.
         match self {
-            InputKey::Local(local) => local.path.file_name().unwrap(),
+            InputKey::Local(local) => local.given_path.file_name().unwrap(),
             InputKey::Remote(remote) => remote.path.file_name().unwrap(),
         }
     }
 }
 
 pub(crate) struct InputRegistry {
     pub(crate) inputs: IndexMap<InputKey, AuditInput>,
-    // pub(crate) actions: IndexMap<InputKey, Action>,
-    // pub(crate) workflows: IndexMap<InputKey, Workflow>,
 }
 
 impl InputRegistry {
@@ -140,10 +149,14 @@ impl InputRegistry {
 
     /// Registers a workflow or action definition from its path on disk.
     #[instrument(skip(self))]
-    pub(crate) fn register_by_path(&mut self, path: &Utf8Path) -> Result<()> {
-        match Workflow::from_file(path) {
+    pub(crate) fn register_by_path(
+        &mut self,
+        path: &Utf8Path,
+        prefix: Option<&Utf8Path>,
+    ) -> Result<()> {
+        match Workflow::from_file(path, prefix) {
             Ok(workflow) => self.register_input(workflow.into()),
-            Err(we) => match Action::from_file(path) {
+            Err(we) => match Action::from_file(path, prefix) {
                 Ok(action) => self.register_input(action.into()),
                 Err(ae) => Err(anyhow!("failed to register input as workflow or action"))
                     .with_context(|| we)
@@ -289,8 +302,8 @@ mod tests {
     use super::InputKey;
 
     #[test]
-    fn test_workflow_key_display() {
-        let local = InputKey::local("/foo/bar/baz.yml".into()).unwrap();
+    fn test_input_key_display() {
+        let local = InputKey::local("/foo/bar/baz.yml", None).unwrap();
         assert_eq!(local.to_string(), "file:///foo/bar/baz.yml");
 
         // No ref
@@ -313,4 +326,26 @@ mod tests {
             "https://github.com/foo/bar/blob/v1/.github/workflows/baz.yml"
         );
     }
+
+    #[test]
+    fn test_input_key_local_path() {
+        let local = InputKey::local("/foo/bar/baz.yml", None).unwrap();
+        assert_eq!(local.best_effort_relative_path(), "/foo/bar/baz.yml");
+
+        let local = InputKey::local("/foo/bar/baz.yml", Some("/foo")).unwrap();
+        assert_eq!(local.best_effort_relative_path(), "bar/baz.yml");
+
+        let local = InputKey::local("/foo/bar/baz.yml", Some("/foo/bar/")).unwrap();
+        assert_eq!(local.best_effort_relative_path(), "baz.yml");
+
+        let local = InputKey::local(
+            "/home/runner/work/repo/repo/.github/workflows/baz.yml",
+            Some("/home/runner/work/repo/repo"),
+        )
+        .unwrap();
+        assert_eq!(
+            local.best_effort_relative_path(),
+            ".github/workflows/baz.yml"
+        );
+    }
 }

src/render.rs

@@ -50,7 +50,11 @@ pub(crate) fn finding_snippet<'w>(
             Snippet::source(input.document().source())
                 .fold(true)
                 .line_start(1)
-                .origin(input.link().unwrap_or(input_key.path()))
+                .origin(
+                    input
+                        .link()
+                        .unwrap_or(input_key.best_effort_relative_path()),
+                )
                 .annotations(locations.iter().map(|loc| {
                     let annotation = match loc.symbolic.link {
                         Some(ref link) => link,

src/sarif.rs

@@ -125,7 +125,8 @@ fn build_locations<'a>(locations: impl Iterator<Item = &'a Location<'a>>) -> Vec
                     PhysicalLocation::builder()
                         .artifact_location(
                             ArtifactLocation::builder()
-                                .uri(location.symbolic.key.path())
+                                .uri_base_id("%SRCROOT%")
+                                .uri(location.symbolic.key.best_effort_relative_path())
                                 .build(),
                         )
                         .region(