Repository: woodruffw/zizmor (rust)

Issue:
[BUG]: Ignoring results with inline comments on composite actions
### Pre-submission checks

- [x] I am **not** filing a feature request. These should be filed via the feature request form instead.
- [x] I have looked through the [open issues](https://github.com/woodruffw/zizmor/issues?q=is%3Aissue+is%3Aopen+) for a duplicate report.

### Expected behavior

Ignore results within composite actions using inline comments.



### Actual behavior

Results are not ignored when inline comments are used within composite actions.

### Reproduction steps

On a composite action that raises `error[template-injection]` for example, adding `# zizmor: ignore[template-injection]` has no effect.

### Logs

```plain text

```

### Additional context

Kudos for the great work being done @woodruffw.

zizmor is being used to tighten up our actions here: https://github.com/ansys/actions/pull/725
>[!NOTE]
>I am opening this as a bug and not as a feature request because the documentation implies that inline comments should be supported within composite actions. [Although on this page](https://woodruffw.github.io/zizmor/configuration/), it is stated that a `zizmor.yml` config cannot be used. I stand to be corrected though, maybe I missed something. Thanks!

Pull request:
tests: add repro case for #612
~~WIP.~~

Things I need to do here:

* Add the concept of an "invisible" span, which is used for evaluating ignore comments but not presented in outputs like SARIF or the terminal render.
* Add invisible spans to audits where appropriate.

Fixes #612.

Patch:
diff --git a/docs/development.md b/docs/development.md
index 1cd18b2..d95c7c6 100644
--- a/docs/development.md
+++ b/docs/development.md
@@ -291,6 +291,27 @@ The general procedure for adding a new audit can be described as:
   information about the underlying vulnerability
 - Open your Pull Request!
 
+#### Adding locations to an audit
+
+Locations can be added to a finding via the `FindingBuilder::add_location`
+method. Locations have a few different flavors that can be used in
+different situations:
+
+* "Primary" locations are subjectively the most important locations
+  for a finding. In general, a finding should have exactly one primary location.
+* "Related" locations are additional locations that are related to the
+  finding, but not as important as the primary location. A finding can
+  have multiple related locations.
+* "Hidden" locations are used to mark a span as relevant to a finding,
+  but are not shown in outputs like SARIF or the cargo-style "plain"
+  output. These are useful marking spans as included in a finding e.g.
+  so that `# zizmor: ignore` works in intuitive places.
+
+In general, audit authors shouldn't need hidden locations at all.
+They're only needed in specific cases where a finding's locations result
+in "gaps" in the finding's spans, resulting in ignore comments not
+working as expected.
+
 ### Changing an existing audit
 
 The general procedure for changing an existing audit is:
diff --git a/docs/release-notes.md b/docs/release-notes.md
index 9e1cad8..fc4f4e2 100644
--- a/docs/release-notes.md
+++ b/docs/release-notes.md
@@ -13,6 +13,8 @@ of `zizmor`.
 
 * Fixed a bug where `zizmor` would over-eagerly parse invalid and
   commented-out expressions, resulting in spurious warnings (#570)
+* Fixed a bug where `zizmor` would fail to honor `# zizmor: ignore[rule]`
+  comments in unintuitive cases (#612)
 
 ### Upcoming Changes 🚧
 
diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs
index dc9bf8d..c689e20 100644
--- a/src/audit/template_injection.rs
+++ b/src/audit/template_injection.rs
@@ -290,6 +290,7 @@ impl Audit for TemplateInjection {
                     .severity(severity)
                     .confidence(confidence)
                     .persona(persona)
+                    .add_location(step.location().hidden())
                     .add_location(step.location_with_name())
                     .add_location(
                         script_loc.clone().primary().annotated(format!(
@@ -318,6 +319,7 @@ impl Audit for TemplateInjection {
                     .severity(severity)
                     .confidence(confidence)
                     .persona(persona)
+                    .add_location(step.location().hidden())
                     .add_location(step.location_with_name())
                     .add_location(
                         script_loc.clone().primary().annotated(format!(
diff --git a/src/finding/mod.rs b/src/finding/mod.rs
index 49bd1d3..c9be499 100644
--- a/src/finding/mod.rs
+++ b/src/finding/mod.rs
@@ -119,10 +119,31 @@ impl<'w> Route<'w> {
     }
 }
 
-/// Represents a symbolic workflow location.
+/// Represents a location's type.
+#[derive(Serialize, Copy, Clone, Debug, Default)]
+pub(crate) enum LocationKind {
+    /// A location that is subjectively "primary" to a finding.
+    ///
+    /// This is used to distinguish between "primary" and "related" locations
+    /// in output formats like SARIF.
+    Primary,
+    #[default]
+    /// A location that is "related" to a finding.
+    ///
+    /// This is the default location type.
+    Related,
+    /// A hidden location.
+    ///
+    /// These locations are not rendered in output formats like SARIF or
+    /// the cargo-style output. Instead, they're used to provide spanning
+    /// information for checking things like ignore comments.
+    Hidden,
+}
+
+/// Represents a symbolic location.
 #[derive(Serialize, Clone, Debug)]
 pub(crate) struct SymbolicLocation<'w> {
-    /// The unique ID of the workflow, as it appears in the workflow registry.
+    /// The unique ID of the input, as it appears in the input registry.
     pub(crate) key: &'w InputKey,
 
     /// An annotation for this location.
@@ -137,12 +158,8 @@ 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,
+    /// The kind of location.
+    pub(crate) kind: LocationKind,
 }
 
 impl<'w> SymbolicLocation<'w> {
@@ -152,7 +169,7 @@ impl<'w> SymbolicLocation<'w> {
             annotation: self.annotation.clone(),
             link: None,
             route: self.route.with_keys(keys),
-            primary: self.primary,
+            kind: self.kind,
         }
     }
 
@@ -182,10 +199,24 @@ impl<'w> SymbolicLocation<'w> {
 
     /// Mark the current `SymbolicLocation` as a "primary" location.
     pub(crate) fn primary(mut self) -> SymbolicLocation<'w> {
-        self.primary = true;
+        self.kind = LocationKind::Primary;
+        self
+    }
+
+    /// Mark the current `SymbolicLocation` as a "hidden" location.
+    pub(crate) fn hidden(mut self) -> SymbolicLocation<'w> {
+        self.kind = LocationKind::Hidden;
         self
     }
 
+    pub(crate) fn is_primary(&self) -> bool {
+        matches!(self.kind, LocationKind::Primary)
+    }
+
+    pub(crate) fn is_hidden(&self) -> bool {
+        matches!(self.kind, LocationKind::Hidden)
+    }
+
     /// Concretize this `SymbolicLocation`, consuming it in the process.
     pub(crate) fn concretize(
         self,
@@ -403,6 +434,10 @@ impl Finding<'_> {
             url = self.url
         )
     }
+
+    pub(crate) fn visible_locations(&self) -> impl Iterator<Item = &Location<'_>> {
+        self.locations.iter().filter(|l| !l.symbolic.is_hidden())
+    }
 }
 
 pub(crate) struct FindingBuilder<'w> {
@@ -464,7 +499,7 @@ impl<'w> FindingBuilder<'w> {
 
         locations.extend(self.raw_locations);
 
-        if !locations.iter().any(|l| l.symbolic.primary) {
+        if !locations.iter().any(|l| l.symbolic.is_primary()) {
             return Err(anyhow!(
                 "API misuse: at least one location must be marked with primary()"
             ));
diff --git a/src/models.rs b/src/models.rs
index e617dbd..2203133 100644
--- a/src/models.rs
+++ b/src/models.rs
@@ -131,7 +131,7 @@ impl Workflow {
             annotation: "this workflow".to_string(),
             link: None,
             route: Route::new(),
-            primary: false,
+            kind: Default::default(),
         }
     }
 
@@ -777,7 +777,7 @@ impl Action {
             annotation: "this action".to_string(),
             link: None,
             route: Route::new(),
-            primary: false,
+            kind: Default::default(),
         }
     }
 
diff --git a/src/render.rs b/src/render.rs
index 309cbe7..0b241cc 100644
--- a/src/render.rs
+++ b/src/render.rs
@@ -33,6 +33,11 @@ pub(crate) fn finding_snippet<'w>(
     // by their enclosing workflow to generate each snippet correctly.
     let mut locations_by_workflow: HashMap<&InputKey, Vec<&Location<'w>>> = HashMap::new();
     for location in &finding.locations {
+        // Never include hidden locations in the output.
+        if location.symbolic.is_hidden() {
+            continue;
+        }
+
         match locations_by_workflow.entry(location.symbolic.key) {
             Entry::Occupied(mut e) => {
                 e.get_mut().push(location);
diff --git a/src/sarif.rs b/src/sarif.rs
index 65d7338..e390c09 100644
--- a/src/sarif.rs
+++ b/src/sarif.rs
@@ -94,9 +94,8 @@ fn build_results(findings: &[Finding]) -> Vec<SarifResult> {
 fn build_result(finding: &Finding<'_>) -> SarifResult {
     // NOTE: Safe unwrap because FindingBuilder::build ensures a primary location.
     let primary = finding
-        .locations
-        .iter()
-        .find(|l| l.symbolic.primary)
+        .visible_locations()
+        .find(|l| l.symbolic.is_primary())
         .unwrap();
 
     SarifResult::builder()
@@ -112,7 +111,9 @@ fn build_result(finding: &Finding<'_>) -> SarifResult {
         .message(&primary.symbolic.annotation)
         .locations(build_locations(std::iter::once(primary)))
         .related_locations(build_locations(
-            finding.locations.iter().filter(|l| !l.symbolic.primary),
+            finding
+                .visible_locations()
+                .filter(|l| !l.symbolic.is_primary()),
         ))
         // TODO: https://github.com/psastras/sarif-rs/pull/770
         .level(


Test patch:
diff --git a/tests/integration/e2e.rs b/tests/integration/e2e.rs
index 90b99b7..d94b2b4 100644
--- a/tests/integration/e2e.rs
+++ b/tests/integration/e2e.rs
@@ -169,3 +169,14 @@ fn progress_bar_tty() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn issue_612_repro() -> Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+            .input(input_under_test("issue-612-repro/action.yml"))
+            .run()?
+    );
+
+    Ok(())
+}
diff --git a/tests/integration/snapshots/integration__e2e__issue_612_repro.snap b/tests/integration/snapshots/integration__e2e__issue_612_repro.snap
new file mode 100644
index 0000000..46cd1cb
--- /dev/null
+++ b/tests/integration/snapshots/integration__e2e__issue_612_repro.snap
@@ -0,0 +1,5 @@
+---
+source: tests/integration/e2e.rs
+expression: "zizmor().input(input_under_test(\"issue-612-repro/action.yml\")).run()?"
+---
+No findings to report. Good job! (2 ignored)
diff --git a/tests/integration/test-data/issue-612-repro/action.yml b/tests/integration/test-data/issue-612-repro/action.yml
new file mode 100644
index 0000000..9b08c73
--- /dev/null
+++ b/tests/integration/test-data/issue-612-repro/action.yml
@@ -0,0 +1,31 @@
+# Repro case for #612.
+# Ensures that we don't unintuitively ignore ignore comments
+# when they occur on a line that's conceptually part of the finding's
+# span but is not included in the visible span rendering.
+#
+# This should emit no findings, only ignored findings.
+
+name: "Automerge dependabot and pre-commit.ci PRs"
+description: "Action merging dependabot and pre-commit.ci PRs."
+
+inputs:
+  approver:
+    description: "Approver GitHub ID."
+    required: true
+  approver-token:
+    description: "Approver GitHub token."
+    required: true
+
+runs:
+  using: "composite"
+  steps:
+    - name: Verify allowed PRs (dependabot and pre-commit.ci)
+      id: verify-allowed-prs # zizmor: ignore[template-injection] this works fine
+      shell: bash
+      run: |
+        # Check if the author is either dependabot or pre-commit.ci.
+        if [[ ${{ github.event.pull_request.user.login }} == 'dependabot[bot]' ]] || [[ ${{ github.event.pull_request.user.login }} == 'pre-commit-ci[bot]' ]]; then
+          true
+        else
+          exit 1
+        fi