bugfix: special-case workflow_call in excessive-permissions (#473)

.github/workflows/zizmor.yml

@@ -6,6 +6,8 @@ on:
   pull_request:
     branches: ["*"]
 
+permissions: {}
+
 jobs:
   zizmor:
     name: zizmor latest via Cargo

Cargo.toml

@@ -23,7 +23,7 @@ clap-verbosity-flag = { version = "3.0.2", features = [
 ], default-features = false }
 etcetera = "0.8.0"
 flate2 = "1.0.35"
-github-actions-models = "0.21.0"
+github-actions-models = "0.22.0"
 http-cache-reqwest = "0.15.0"
 human-panic = "2.0.1"
 indexmap = "2.7.0"

docs/release-notes.md

@@ -11,6 +11,11 @@ of `zizmor`.
 
 Nothing to see here (yet!)
 
+### Bug Fixes 🐛
+
+* The [excessive-permissions] audit is now more precise about both
+  reusable workflows and reusable workflow calls (#473)
+
 ## v1.2.1
 
 This is a small corrective release for some SARIF behavior that

src/audit/excessive_permissions.rs

@@ -57,12 +57,9 @@ impl Audit for ExcessivePermissions {
 
         let all_jobs_have_permissions = workflow
             .jobs()
-            .filter_map(|job| {
-                let Job::NormalJob(job) = job else {
-                    return None;
-                };
-
-                Some(&job.permissions)
+            .map(|job| match job {
+                Job::NormalJob(job) => &job.permissions,
+                Job::ReusableWorkflowCallJob(job) => &job.permissions,
             })
             .all(|perm| !matches!(perm, Permissions::Base(BasePermission::Default)));
 
@@ -71,17 +68,21 @@ impl Audit for ExcessivePermissions {
             Permissions::Base(BasePermission::Default)
         );
 
-        // Top-level permissions are a minor issue if there's only one
-        // job in the workflow, since they're equivalent to job-level
-        // permissions in that case. Emit only pedantic findings in
-        // that case.
-        // Similarly, if all jobs in the workflow have their own explicit
-        // permissions, then any permissions set at the top-level are moot.
-        let persona = if workflow.jobs.len() == 1 || all_jobs_have_permissions {
-            Persona::Pedantic
-        } else {
-            Persona::Regular
-        };
+        let workflow_is_reusable_only =
+            workflow.has_workflow_call() && workflow.has_single_trigger();
+
+        // Top-level permissions are a pedantic finding under the following
+        // conditions:
+        //
+        // 1. The workflow has only one job.
+        // 2. All jobs in the workflow have their own explicit permissions.
+        // 3. The workflow is reusable and has only one trigger.
+        let workflow_finding_persona =
+            if workflow.jobs.len() == 1 || all_jobs_have_permissions || workflow_is_reusable_only {
+                Persona::Pedantic
+            } else {
+                Persona::Regular
+            };
 
         // Handle top-level permissions.
         let location = workflow.location().primary();
@@ -93,27 +94,43 @@ impl Audit for ExcessivePermissions {
                 Self::finding()
                     .severity(severity)
                     .confidence(confidence)
-                    .persona(persona)
+                    .persona(workflow_finding_persona)
                     .add_location(perm_location)
                     .build(workflow)?,
             );
         }
 
         for job in workflow.jobs() {
-            let Job::NormalJob(job) = &job else {
-                continue;
+            let (permissions, job_location, job_finding_persona) = match job {
+                Job::NormalJob(job) => {
+                    // For normal jobs: if the workflow is reusable-only, we
+                    // emit pedantic findings.
+                    let persona = if workflow_is_reusable_only {
+                        Persona::Pedantic
+                    } else {
+                        Persona::Regular
+                    };
+
+                    (&job.permissions, job.location(), persona)
+                }
+                Job::ReusableWorkflowCallJob(job) => {
+                    // For reusable jobs: the caller is always responsible for
+                    // permissions, so we emit regular findings even if
+                    // the workflow is reusable-only.
+                    (&job.permissions, job.location(), Persona::Regular)
+                }
             };
 
-            let job_location = job.location();
             if let Some((severity, confidence, perm_location)) = self.check_job_permissions(
-                &job.permissions,
+                permissions,
                 explicit_parent_permissions,
                 job_location.clone(),
             ) {
                 findings.push(
                     Self::finding()
                         .severity(severity)
                         .confidence(confidence)
+                        .persona(job_finding_persona)
                         .add_location(job_location)
                         .add_location(perm_location.primary())
                         .build(workflow)?,

src/models.rs

@@ -135,7 +135,7 @@ impl Workflow {
         Jobs::new(self)
     }
 
-    /// Whether this workflow's is triggered by pull_request_target.
+    /// Whether this workflow is triggered by pull_request_target.
     pub(crate) fn has_pull_request_target(&self) -> bool {
         match &self.on {
             Trigger::BareEvent(event) => *event == BareEvent::PullRequestTarget,
@@ -144,14 +144,32 @@ impl Workflow {
         }
     }
 
-    /// Whether this workflow's is triggered by workflow_run.
+    /// Whether this workflow is triggered by workflow_run.
     pub(crate) fn has_workflow_run(&self) -> bool {
         match &self.on {
             Trigger::BareEvent(event) => *event == BareEvent::WorkflowRun,
             Trigger::BareEvents(events) => events.contains(&BareEvent::WorkflowRun),
             Trigger::Events(events) => !matches!(events.workflow_run, OptionalBody::Missing),
         }
     }
+
+    /// Whether this workflow is triggered by workflow_call.
+    pub(crate) fn has_workflow_call(&self) -> bool {
+        match &self.on {
+            Trigger::BareEvent(event) => *event == BareEvent::WorkflowCall,
+            Trigger::BareEvents(events) => events.contains(&BareEvent::WorkflowCall),
+            Trigger::Events(events) => !matches!(events.workflow_call, OptionalBody::Missing),
+        }
+    }
+
+    /// Whether this workflow is triggered by exactly one event.
+    pub(crate) fn has_single_trigger(&self) -> bool {
+        match &self.on {
+            Trigger::BareEvent(_) => true,
+            Trigger::BareEvents(events) => events.len() == 1,
+            Trigger::Events(events) => events.count() == 1,
+        }
+    }
 }
 
 /// Common behavior across both normal and reusable jobs.

src/registry.rs

@@ -99,7 +99,7 @@ impl InputKey {
             InputKey::Local(local) => local
                 .prefix
                 .as_ref()
-                .and_then(|pfx| local.given_path.strip_prefix(dbg!(pfx)).ok())
+                .and_then(|pfx| local.given_path.strip_prefix(pfx).ok())
                 .unwrap_or_else(|| &local.given_path)
                 .as_str(),
             InputKey::Remote(remote) => remote.path.as_str(),

tests/snapshot.rs

@@ -436,6 +436,25 @@ fn excessive_permissions() -> Result<()> {
             "excessive-permissions/workflow-default-perms-all-jobs-explicit.yml"
         ))
         .run()?);
+
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test(
+            "excessive-permissions/issue-472-repro.yml"
+        ))
+        .run()?);
+
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test(
+            "excessive-permissions/reusable-workflow-call.yml"
+        ))
+        .run()?);
+
+    insta::assert_snapshot!(zizmor()
+        .workflow(workflow_under_test(
+            "excessive-permissions/reusable-workflow-other-triggers.yml"
+        ))
+        .run()?);
+
     Ok(())
 }
 

tests/snapshots/snapshot__excessive_permissions-10.snap

@@ -0,0 +1,21 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"excessive-permissions/issue-472-repro.yml\")).run()?"
+snapshot_kind: text
+---
+warning[excessive-permissions]: overly broad permissions
+  --> @@INPUT@@:19:3
+   |
+19 | /   job2:
+20 | |     # normal permissions finding here, since callers are always
+21 | |     # responsible for setting permissions, even if the workflow
+22 | |     # is reusable-only
+23 | |     uses: ./.github/workflows/fake.yml
+   | |                                       -
+   | |_______________________________________|
+   |                                         this job
+   |                                         default permissions used due to no permissions: block
+   |
+   = note: audit confidence → Medium
+
+3 findings (2 suppressed): 0 unknown, 0 informational, 0 low, 1 medium, 0 high

tests/snapshots/snapshot__excessive_permissions-11.snap

@@ -0,0 +1,19 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"excessive-permissions/reusable-workflow-call.yml\")).run()?"
+snapshot_kind: text
+---
+warning[excessive-permissions]: overly broad permissions
+ --> @@INPUT@@:7:3
+  |
+7 | /   job1:
+8 | |     # finding: reusable jobs should always specify their permissions
+9 | |     uses: ./.github/workflows/zizmor-child.yml
+  | |                                               -
+  | |_______________________________________________|
+  |                                                 this job
+  |                                                 default permissions used due to no permissions: block
+  |
+  = note: audit confidence → Medium
+
+2 findings (1 suppressed): 0 unknown, 0 informational, 0 low, 1 medium, 0 high

tests/snapshots/snapshot__excessive_permissions-12.snap

@@ -0,0 +1,47 @@
+---
+source: tests/snapshot.rs
+expression: "zizmor().workflow(workflow_under_test(\"excessive-permissions/reusable-workflow-other-triggers.yml\")).run()?"
+snapshot_kind: text
+---
+warning[excessive-permissions]: overly broad permissions
+  --> @@INPUT@@:1:1
+   |
+ 1 | / name: reusable-workflow-other-triggers
+ 2 | |
+...  |
+20 | |     # responsible for setting permissions
+21 | |     uses: ./.github/workflows/fake.yml
+   | |_______________________________________- default permissions used due to no permissions: block
+   |
+   = note: audit confidence → Medium
+
+warning[excessive-permissions]: overly broad permissions
+  --> @@INPUT@@:11:3
+   |
+11 | /   job1:
+12 | |     # regular job-level finding, since we can be triggered by
+...  |
+15 | |     steps:
+16 | |       - run: echo hello
+   | |                       -
+   | |_______________________|
+   |                         this job
+   |                         default permissions used due to no permissions: block
+   |
+   = note: audit confidence → Medium
+
+warning[excessive-permissions]: overly broad permissions
+  --> @@INPUT@@:18:3
+   |
+18 | /   job2:
+19 | |     # normal permissions finding here, since callers are always
+20 | |     # responsible for setting permissions
+21 | |     uses: ./.github/workflows/fake.yml
+   | |                                       -
+   | |_______________________________________|
+   |                                         this job
+   |                                         default permissions used due to no permissions: block
+   |
+   = note: audit confidence → Medium
+
+3 findings: 0 unknown, 0 informational, 0 low, 3 medium, 0 high

tests/test-data/excessive-permissions/issue-472-repro.yml

@@ -0,0 +1,23 @@
+# repro case for https://github.com/woodruffw/zizmor/issues/472
+
+name: issue-472-repro
+
+on:
+  workflow_call:
+
+# no non-pedantic top-level permissions finding, since
+# the workflow is reusable-only
+
+jobs:
+  job1:
+    # no non-pedantic job-level permissions finding, since
+    # the workflow is reusable-only
+    runs-on: ubuntu-24.04
+    steps:
+      - run: echo hello
+
+  job2:
+    # normal permissions finding here, since callers are always
+    # responsible for setting permissions, even if the workflow
+    # is reusable-only
+    uses: ./.github/workflows/fake.yml

tests/test-data/excessive-permissions/reusable-workflow-call.yml

@@ -0,0 +1,9 @@
+name: reusable-workflow-call
+
+on:
+  workflow_dispatch:
+
+jobs:
+  job1:
+    # finding: reusable jobs should always specify their permissions
+    uses: ./.github/workflows/zizmor-child.yml

tests/test-data/excessive-permissions/reusable-workflow-other-triggers.yml

@@ -0,0 +1,21 @@
+name: reusable-workflow-other-triggers
+
+on:
+  workflow_call:
+  push:
+
+# regular top-level finding, since we can be triggered by
+# either a workflow call or a push
+
+jobs:
+  job1:
+    # regular job-level finding, since we can be triggered by
+    # either a workflow call or a push
+    runs-on: ubuntu-24.04
+    steps:
+      - run: echo hello
+
+  job2:
+    # normal permissions finding here, since callers are always
+    # responsible for setting permissions
+    uses: ./.github/workflows/fake.yml