Flag missing concurrency when all jobs are reusable

crates/zizmor/src/audit/concurrency_limits.rs

@@ -58,48 +58,73 @@ impl Audit for ConcurrencyLimits {
                 );
             }
             None => {
-                for job in workflow.jobs() {
-                    let Job::NormalJob(job) = job else {
-                        continue;
-                    };
-                    match &job.concurrency {
-                        Some(Concurrency::Bare(_)) => {
-                            findings.push(
-                                Self::finding()
-                                    .confidence(Confidence::High)
-                                    .severity(Severity::Low)
-                                    .persona(Persona::Pedantic)
-                                    .add_location(
-                                        job.location()
-                                            .primary()
-                                            .with_keys(["concurrency".into()])
-                                            .annotated(
-                                                "job concurrency is missing cancel-in-progress",
-                                            ),
-                                    )
-                                    .build(workflow)?,
-                            );
-                        }
-                        None => {
-                            findings.push(
-                                Self::finding()
-                                    .confidence(Confidence::High)
-                                    .severity(Severity::Low)
-                                    .persona(Persona::Pedantic)
-                                    .add_location(
-                                        workflow
-                                            .location()
-                                            .primary()
-                                            .annotated("missing concurrency setting"),
-                                    )
-                                    .build(workflow)?,
-                            );
+                let all_jobs_are_reusable = workflow
+                    .jobs()
+                    .all(|job| matches!(job, Job::ReusableWorkflowCallJob(_)));
+
+                if all_jobs_are_reusable {
+                    // If all of the workflow's jobs are reusable workflow calls,
+                    // then the workflow itself should have a concurrency setting.
+                    findings.push(
+                        Self::finding()
+                            .confidence(Confidence::High)
+                            .severity(Severity::Low)
+                            .persona(Persona::Pedantic)
+                            .add_location(
+                                workflow
+                                    .location_with_grip()
+                                    .primary()
+                                    .annotated("workflow is missing a `concurrency` key"),
+                            )
+                            .add_location(workflow.location().hidden())
+                            .build(workflow)?,
+                    );
+                } else {
+                    // Otherwise, we flag each non-reusable job that lacks
+                    // a sufficient concurrency setting.
+                    for job in workflow.jobs() {
+                        let Job::NormalJob(job) = job else {
+                            continue;
+                        };
+                        match &job.concurrency {
+                            Some(Concurrency::Bare(_)) => {
+                                findings.push(
+                                    Self::finding()
+                                        .confidence(Confidence::High)
+                                        .severity(Severity::Low)
+                                        .persona(Persona::Pedantic)
+                                        .add_location(
+                                            job.location()
+                                                .primary()
+                                                .with_keys(["concurrency".into()])
+                                                .annotated(
+                                                    "job concurrency is missing cancel-in-progress",
+                                                ),
+                                        )
+                                        .build(workflow)?,
+                                );
+                            }
+                            None => {
+                                findings.push(
+                                    Self::finding()
+                                        .confidence(Confidence::High)
+                                        .severity(Severity::Low)
+                                        .persona(Persona::Pedantic)
+                                        .add_location(
+                                            workflow
+                                                .location()
+                                                .primary()
+                                                .annotated("missing a `concurrency` key"),
+                                        )
+                                        .build(workflow)?,
+                                );
+                            }
+                            // NOTE: Per #1302, we don't nag the user if they've explicitly set
+                            // `cancel-in-progress: false` or similar. This is like with the
+                            // artipacked audit, where `persist-credentials: true` is seen as
+                            // a positive signal of user intent.
+                            _ => {}
                         }
-                        // NOTE: Per #1302, we don't nag the user if they've explicitly set
-                        // `cancel-in-progress: false` or similar. This is like with the
-                        // artipacked audit, where `persist-credentials: true` is seen as
-                        // a positive signal of user intent.
-                        _ => {}
                     }
                 }
             }

crates/zizmor/src/models/workflow.rs

@@ -204,6 +204,15 @@ impl Workflow {
             kind: Default::default(),
         }
     }
+
+    /// Returns this workflow's location, with a grip on the "name" field if it exists.
+    pub fn location_with_grip(&self) -> SymbolicLocation<'_> {
+        if self.name.is_some() {
+            self.location().with_keys(["name".into()])
+        } else {
+            self.location()
+        }
+    }
 }
 
 /// Represents a single "normal" GitHub Actions job.

crates/zizmor/tests/integration/audit/concurrency_limits.rs

@@ -35,7 +35,7 @@ fn test_missing() -> anyhow::Result<()> {
     ...  |
     10 | |     - name: 1-ok
     11 | |       run: echo ok
-       | |___________________^ missing concurrency setting
+       | |___________________^ missing a `concurrency` key
        |
        = note: audit confidence → High
 
@@ -97,7 +97,7 @@ fn test_jobs_missing_no_cancel() -> anyhow::Result<()> {
     ...  |
     17 | |     - name: 2-ok
     18 | |       run: echo ok
-       | |___________________^ missing concurrency setting
+       | |___________________^ missing a `concurrency` key
        |
        = note: audit confidence → High
 
@@ -123,3 +123,28 @@ fn test_issue_1511() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_issue_1619() -> anyhow::Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+            .input(input_under_test(
+                "concurrency-limits/issue-1619-repro.yml"
+            ))
+            .args(["--persona=pedantic"])
+            .run()?,
+        @r"
+    help[concurrency-limits]: insufficient job-level concurrency limits
+     --> @@INPUT@@:5:1
+      |
+    5 | name: issue-1619-repro
+      | ^^^^^^^^^^^^^^^^^^^^^^ workflow is missing a `concurrency` key
+      |
+      = note: audit confidence → High
+
+    2 findings (1 ignored): 0 informational, 1 low, 0 medium, 0 high
+    "
+    );
+
+    Ok(())
+}

crates/zizmor/tests/integration/audit/excessive_permissions.rs

@@ -304,7 +304,7 @@ fn test_reusable_workflow_call() -> Result<()> {
        |
        = note: audit confidence → Medium
 
-    2 findings (1 suppressed): 0 informational, 0 low, 1 medium, 0 high
+    3 findings (2 suppressed): 0 informational, 0 low, 1 medium, 0 high
     "
     );
 

crates/zizmor/tests/integration/audit/secrets_inherit.rs

@@ -50,7 +50,7 @@ fn secrets_inherit() -> anyhow::Result<()> {
        |
        = note: audit confidence → High
 
-    5 findings: 0 informational, 0 low, 1 medium, 4 high
+    6 findings (1 suppressed): 0 informational, 0 low, 1 medium, 4 high
     "
     );
 

crates/zizmor/tests/integration/audit/unpinned_uses.rs

@@ -641,7 +641,7 @@ fn test_reusable_workflow_unpinned() -> Result<()> {
        |
        = note: audit confidence → High
 
-    2 findings: 0 informational, 0 low, 0 medium, 2 high
+    3 findings (1 suppressed): 0 informational, 0 low, 0 medium, 2 high
     "
     );
 

crates/zizmor/tests/integration/e2e.rs

@@ -661,7 +661,7 @@ fn test_github_output() -> Result<()> {
     ::error file=@@INPUT@@,line=11,title=excessive-permissions::several-vulnerabilities.yml:11: overly broad permissions: uses write-all permissions
     ::error file=@@INPUT@@,line=2,title=dangerous-triggers::several-vulnerabilities.yml:2: use of fundamentally insecure workflow trigger: pull_request_target is almost always used insecurely
     ::error file=@@INPUT@@,line=16,title=template-injection::several-vulnerabilities.yml:16: code injection via template expansion: may expand into attacker-controllable code
-    ::warning file=@@INPUT@@,line=1,title=concurrency-limits::several-vulnerabilities.yml:1: insufficient job-level concurrency limits: missing concurrency setting
+    ::warning file=@@INPUT@@,line=1,title=concurrency-limits::several-vulnerabilities.yml:1: insufficient job-level concurrency limits: missing a `concurrency` key
     "
     );
 

crates/zizmor/tests/integration/snapshots/integration__e2e__gha_hazmat.snap

@@ -1278,4 +1278,4 @@ error[dangerous-triggers]: use of fundamentally insecure workflow trigger
    |
    = note: audit confidence → Medium
 
-217 findings (104 suppressed): 7 informational, 0 low, 43 medium, 63 high
+218 findings (105 suppressed): 7 informational, 0 low, 43 medium, 63 high

crates/zizmor/tests/integration/test-data/concurrency-limits/issue-1619-repro.yml

@@ -0,0 +1,14 @@
+# Repro case for #1619: we should flag a workflow that is not reusable, but that
+# only has reusable jobs, as missing a concurrency key.
+# See: <https://github.com/zizmorcore/zizmor/issues/1619>
+
+name: issue-1619-repro
+on:
+  push:
+  pull_request:
+
+permissions: {}
+
+jobs:
+  test:
+    uses: org/repo/.github/workflows/reusable-tests.yml@main # zizmor: ignore[unpinned-uses]