Check for job-level concurrency (#1338)

Co-authored-by: William Woodruff <william@yossarian.net>

crates/zizmor/src/audit/concurrency_limits.rs

@@ -1,4 +1,5 @@
-use super::{Audit, AuditLoadError, audit_meta};
+use super::{Audit, AuditLoadError, Job, audit_meta};
+use crate::finding::location::Locatable as _;
 use crate::{
     audit::AuditError,
     config::Config,
@@ -40,25 +41,56 @@ impl Audit for ConcurrencyLimits {
                                 .location()
                                 .primary()
                                 .with_keys(["concurrency".into()])
-                                .annotated("concurrency is missing cancel-in-progress"),
+                                .annotated("workflow 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)?,
-                );
+                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)?,
+                            );
+                        }
+                        // 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

crates/zizmor/tests/integration/acceptance.rs

@@ -324,6 +324,21 @@ fn concurrency_limits_cancel_true() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[test]
+fn concurrency_limits_cancel_job_true() -> anyhow::Result<()> {
+    let auditable = input_under_test("concurrency-limits/job-cancel-true.yml");
+
+    let cli_args = [&auditable];
+
+    let execution = zizmor().args(cli_args).output()?;
+    assert_eq!(execution.status.code(), Some(0));
+
+    let findings = String::from_utf8(execution.stdout)?;
+    assert_eq!(&findings, "[]");
+
+    Ok(())
+}
+
 #[test]
 fn concurrency_limits_cancel_expr() -> anyhow::Result<()> {
     let auditable = input_under_test("concurrency-limits/cancel-expr.yml");

crates/zizmor/tests/integration/snapshot.rs

@@ -1133,7 +1133,7 @@ fn concurrency_limits() -> Result<()> {
      --> @@INPUT@@:5:1
       |
     5 | concurrency: group
-      | ^^^^^^^^^^^^^^^^^^ concurrency is missing cancel-in-progress
+      | ^^^^^^^^^^^^^^^^^^ workflow concurrency is missing cancel-in-progress
       |
       = note: audit confidence → High
 
@@ -1141,5 +1141,38 @@ fn concurrency_limits() -> Result<()> {
     "
     );
 
+    insta::assert_snapshot!(
+        zizmor()
+            .input(input_under_test(
+                "concurrency-limits/jobs-missing-no-cancel.yml"
+            ))
+            .args(["--persona=pedantic"])
+            .run()?,
+        @r"
+  help[concurrency-limits]: insufficient job-level concurrency limits
+   --> @@INPUT@@:9:5
+    |
+  9 |     concurrency: group
+    |     ^^^^^^^^^^^^^^^^^^ job concurrency is missing cancel-in-progress
+    |
+    = note: audit confidence → High
+
+  help[concurrency-limits]: insufficient job-level concurrency limits
+    --> @@INPUT@@:1:1
+     |
+   1 | / name: Workflow with job 1 missing cancel-in-progress and job 2 missing concurrency
+   2 | | on: push
+   3 | | permissions: {}
+  ...  |
+  17 | |     - name: 2-ok
+  18 | |       run: echo ok
+     | |___________________^ missing concurrency setting
+     |
+     = note: audit confidence → High
+
+  2 findings: 0 informational, 2 low, 0 medium, 0 high
+    "
+    );
+
     Ok(())
 }

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

@@ -995,4 +995,4 @@ error[dangerous-triggers]: use of fundamentally insecure workflow trigger
    |
    = note: audit confidence → Medium
 
-183 findings (97 suppressed): 7 informational, 0 low, 29 medium, 50 high
+197 findings (111 suppressed): 7 informational, 0 low, 29 medium, 50 high

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

@@ -168,4 +168,4 @@ error[unpinned-uses]: unpinned action reference
    |
    = note: audit confidence → High
 
-97 findings (1 ignored, 78 suppressed): 0 informational, 1 low, 0 medium, 17 high
+98 findings (1 ignored, 79 suppressed): 0 informational, 1 low, 0 medium, 17 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__cache_poisoning-15.snap

@@ -16,4 +16,4 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
    |
    = note: audit confidence → Low
 
-2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
+3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__cache_poisoning-16.snap

@@ -28,4 +28,4 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
    = note: audit confidence → Low
    = note: this finding has an auto-fix
 
-3 findings (1 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
+4 findings (2 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__cache_poisoning-17.snap

@@ -37,4 +37,4 @@ error[cache-poisoning]: runtime artifacts potentially vulnerable to a cache pois
    |
    = note: audit confidence → Low
 
-4 findings (1 suppressed): 0 informational, 0 low, 0 medium, 3 high
+6 findings (3 suppressed): 0 informational, 0 low, 0 medium, 3 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-11.snap

@@ -17,4 +17,4 @@ warning[excessive-permissions]: overly broad permissions
    |
    = note: audit confidence → Medium
 
-3 findings (2 suppressed): 0 informational, 0 low, 1 medium, 0 high
+2 findings (1 suppressed): 0 informational, 0 low, 1 medium, 0 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-4.snap

@@ -11,4 +11,4 @@ warning[excessive-permissions]: overly broad permissions
   |
   = note: audit confidence → High
 
-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/snapshots/integration__snapshot__excessive_permissions-5.snap

@@ -11,4 +11,4 @@ error[excessive-permissions]: overly broad permissions
   |
   = note: audit confidence → High
 
-2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
+3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-6.snap

@@ -3,4 +3,4 @@ source: crates/zizmor/tests/integration/snapshot.rs
 assertion_line: 631
 expression: "zizmor().input(input_under_test(\"excessive-permissions/workflow-empty-perms.yml\")).run()?"
 ---
-No findings to report. Good job! (1 suppressed)
+No findings to report. Good job! (2 suppressed)

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-7.snap

@@ -33,4 +33,4 @@ error[excessive-permissions]: overly broad permissions
    |
    = note: audit confidence → High
 
-3 findings (1 suppressed): 0 informational, 0 low, 1 medium, 1 high
+4 findings (2 suppressed): 0 informational, 0 low, 1 medium, 1 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-8.snap

@@ -27,4 +27,4 @@ warning[excessive-permissions]: overly broad permissions
   |
   = note: audit confidence → High
 
-6 findings (3 suppressed): 0 informational, 0 low, 1 medium, 2 high
+7 findings (4 suppressed): 0 informational, 0 low, 1 medium, 2 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__excessive_permissions-9.snap

@@ -3,4 +3,4 @@ source: crates/zizmor/tests/integration/snapshot.rs
 assertion_line: 655
 expression: "zizmor().input(input_under_test(\"excessive-permissions/workflow-default-perms-all-jobs-explicit.yml\")).run()?"
 ---
-No findings to report. Good job! (4 suppressed)
+No findings to report. Good job! (5 suppressed)

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

@@ -1,5 +1,6 @@
 ---
 source: crates/zizmor/tests/integration/snapshot.rs
+assertion_line: 830
 expression: "zizmor().input(input_under_test(\"obfuscation.yml\")).run()?"
 ---
 help[obfuscation]: obfuscated usage of GitHub Actions features
@@ -189,4 +190,4 @@ help[obfuscation]: obfuscated usage of GitHub Actions features
    = note: audit confidence → High
    = note: this finding has an auto-fix
 
-38 findings (1 ignored, 17 suppressed, 19 fixable): 0 informational, 20 low, 0 medium, 0 high
+39 findings (1 ignored, 18 suppressed, 19 fixable): 0 informational, 20 low, 0 medium, 0 high

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

@@ -1,5 +1,6 @@
 ---
 source: crates/zizmor/tests/integration/snapshot.rs
+assertion_line: 730
 expression: "zizmor().input(input_under_test(\"secrets-inherit.yml\")).run()?"
 ---
 warning[secrets-inherit]: secrets unconditionally inherited by called workflow
@@ -13,4 +14,4 @@ warning[secrets-inherit]: secrets unconditionally inherited by called workflow
    |
    = note: audit confidence → High
 
-2 findings (1 suppressed): 0 informational, 0 low, 1 medium, 0 high
+1 finding: 0 informational, 0 low, 1 medium, 0 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__use_trusted_publishing-3.snap

@@ -1,5 +1,6 @@
 ---
 source: crates/zizmor/tests/integration/snapshot.rs
+assertion_line: 297
 expression: "zizmor().input(input_under_test(\"use-trusted-publishing/cargo-publish.yml\")).run()?"
 ---
 info[use-trusted-publishing]: prefer trusted publishing for authentication
@@ -91,4 +92,4 @@ info[use-trusted-publishing]: prefer trusted publishing for authentication
    |
    = note: audit confidence → High
 
-10 findings (2 suppressed): 8 informational, 0 low, 0 medium, 0 high
+11 findings (3 suppressed): 8 informational, 0 low, 0 medium, 0 high

crates/zizmor/tests/integration/snapshots/integration__snapshot__use_trusted_publishing-4.snap

@@ -1,5 +1,6 @@
 ---
 source: crates/zizmor/tests/integration/snapshot.rs
+assertion_line: 303
 expression: "zizmor().input(input_under_test(\"use-trusted-publishing/npm-publish.yml\")).run()?"
 ---
 info[use-trusted-publishing]: prefer trusted publishing for authentication
@@ -126,4 +127,4 @@ info[use-trusted-publishing]: prefer trusted publishing for authentication
     |
     = note: audit confidence → High
 
-14 findings (2 suppressed): 12 informational, 0 low, 0 medium, 0 high
+16 findings (4 suppressed): 12 informational, 0 low, 0 medium, 0 high

crates/zizmor/tests/integration/test-data/concurrency-limits/job-cancel-true.yml

@@ -0,0 +1,14 @@
+name: Workflow with cancel-in-progress=true in a job
+on: push
+permissions: {}
+
+jobs:
+  job:
+    name: some-job
+    runs-on: ubuntu-latest
+    concurrency:
+      group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+      cancel-in-progress: true
+    steps:
+    - name: 1-ok
+      run: echo ok

crates/zizmor/tests/integration/test-data/concurrency-limits/jobs-missing-no-cancel.yml

@@ -0,0 +1,18 @@
+name: Workflow with job 1 missing cancel-in-progress and job 2 missing concurrency
+on: push
+permissions: {}
+
+jobs:
+  job1:
+    name: job-1
+    runs-on: ubuntu-latest
+    concurrency: group
+    steps:
+    - name: 1-ok
+      run: echo ok
+  job2:
+    name: job-2
+    runs-on: ubuntu-latest
+    steps:
+    - name: 2-ok
+      run: echo ok

docs/release-notes.md

@@ -29,8 +29,11 @@ of `zizmor`.
   improving compatibility with user environments that perform TLS interception
   (#1328)
 
-* The [github-env] now falls back to assuming bash-like shell syntax in
+* The [github-env] audit now falls back to assuming bash-like shell syntax in
   `run:` blocks if it can't infer the shell being used (#1336)
+  
+* The [concurrency-limits] audit now correctly detects job-level `concurrency`
+  settings, in addition to workflow-level settings (#1338)
 
 ## 1.16.3