Skip to content

Commit 37cc1ef

Browse files
woodruffwclaudewoodruffw-bot
authored
Fix concurrency-limits audit to report at workflow level (#1793)
* fix(concurrency-limits): consolidate findings to workflow level (#1627) Rebase PR #1627 onto current main and update all integration test snapshots to reflect the consolidated concurrency-limits findings. The audit now collects affected jobs and produces a single workflow-level finding per issue type (missing concurrency / missing cancel-in-progress) instead of emitting one finding per job. https://claude.ai/code/session_019imdGGVzhxj9FwhK2nWhRa * update crater snapshots for consolidated concurrency-limits findings Regenerate all 5 crater test snapshots to reflect the consolidated workflow-level concurrency-limits findings instead of per-job findings. https://claude.ai/code/session_01PXxMnbkNKhN6f4dUVZnGW9 * Bump snapshots Signed-off-by: William Woodruff <[email protected]> --------- Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: William Woodruff <[email protected]>
1 parent de7bfc1 commit 37cc1ef

17 files changed

+310
-296
lines changed

‎crates/zizmor/src/audit/concurrency_limits.rs‎

Lines changed: 55 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -58,42 +58,20 @@ impl Audit for ConcurrencyLimits {
5858
);
5959
}
6060
None => {
61+
// Collect all jobs that are missing concurrency or have incomplete settings
62+
let mut jobs_missing_concurrency = vec![];
63+
let mut jobs_missing_cancel_in_progress = vec![];
64+
6165
for job in workflow.jobs() {
6266
let Job::NormalJob(job) = job else {
6367
continue;
6468
};
6569
match &job.concurrency {
6670
Some(Concurrency::Bare(_)) => {
67-
findings.push(
68-
Self::finding()
69-
.confidence(Confidence::High)
70-
.severity(Severity::Low)
71-
.persona(Persona::Pedantic)
72-
.add_location(
73-
job.location()
74-
.primary()
75-
.with_keys(["concurrency".into()])
76-
.annotated(
77-
"job concurrency is missing cancel-in-progress",
78-
),
79-
)
80-
.build(workflow)?,
81-
);
71+
jobs_missing_cancel_in_progress.push(job);
8272
}
8373
None => {
84-
findings.push(
85-
Self::finding()
86-
.confidence(Confidence::High)
87-
.severity(Severity::Low)
88-
.persona(Persona::Pedantic)
89-
.add_location(
90-
workflow
91-
.location()
92-
.primary()
93-
.annotated("missing concurrency setting"),
94-
)
95-
.build(workflow)?,
96-
);
74+
jobs_missing_concurrency.push(job);
9775
}
9876
// NOTE: Per #1302, we don't nag the user if they've explicitly set
9977
// `cancel-in-progress: false` or similar. This is like with the
@@ -102,6 +80,55 @@ impl Audit for ConcurrencyLimits {
10280
_ => {}
10381
}
10482
}
83+
84+
// Create a single finding for jobs missing cancel-in-progress
85+
if !jobs_missing_cancel_in_progress.is_empty() {
86+
let mut finding_builder = Self::finding()
87+
.confidence(Confidence::High)
88+
.severity(Severity::Low)
89+
.persona(Persona::Pedantic)
90+
.add_location(
91+
workflow
92+
.location()
93+
.primary()
94+
.with_keys(["on".into()])
95+
.annotated("workflow is missing concurrency setting"),
96+
);
97+
98+
for job in &jobs_missing_cancel_in_progress {
99+
finding_builder = finding_builder.add_location(
100+
job.location()
101+
.with_keys(["concurrency".into()])
102+
.annotated("job concurrency is missing cancel-in-progress"),
103+
);
104+
}
105+
106+
findings.push(finding_builder.build(workflow)?);
107+
}
108+
109+
// Create a single finding for jobs missing concurrency entirely
110+
if !jobs_missing_concurrency.is_empty() {
111+
let mut finding_builder = Self::finding()
112+
.confidence(Confidence::High)
113+
.severity(Severity::Low)
114+
.persona(Persona::Pedantic)
115+
.add_location(
116+
workflow
117+
.location()
118+
.primary()
119+
.with_keys(["on".into()])
120+
.annotated("workflow is missing concurrency setting"),
121+
);
122+
123+
for job in &jobs_missing_concurrency {
124+
finding_builder = finding_builder.add_location(
125+
job.location_with_grip()
126+
.annotated("job affected by missing workflow concurrency"),
127+
);
128+
}
129+
130+
findings.push(finding_builder.build(workflow)?);
131+
}
105132
}
106133
// NOTE: Per #1302, we don't nag the user if they've explicitly set
107134
// `cancel-in-progress: false` or similar. This is like with the

‎crates/zizmor/tests/integration/audit/cache_poisoning.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ fn test_issue_642() -> anyhow::Result<()> {
422422
|
423423
= note: audit confidence → Low
424424
425-
3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high
425+
2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
426426
"
427427
);
428428

@@ -466,7 +466,7 @@ fn test_issue_1081() -> anyhow::Result<()> {
466466
= note: audit confidence → Low
467467
= note: this finding has an auto-fix
468468
469-
4 findings (2 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
469+
3 findings (1 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
470470
"
471471
);
472472

@@ -519,7 +519,7 @@ fn test_issue_1152() -> anyhow::Result<()> {
519519
|
520520
= note: audit confidence → Low
521521
522-
6 findings (3 suppressed): 0 informational, 0 low, 0 medium, 3 high
522+
4 findings (1 suppressed): 0 informational, 0 low, 0 medium, 3 high
523523
"
524524
);
525525

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

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,17 +27,15 @@ fn test_missing() -> anyhow::Result<()> {
2727
.run()?,
2828
@"
2929
help[concurrency-limits]: insufficient job-level concurrency limits
30-
--> @@INPUT@@:1:1
31-
|
32-
1 | / name: Workflow without concurrency
33-
2 | | on: push
34-
3 | | permissions: {}
35-
... |
36-
10 | | - name: 1-ok
37-
11 | | run: echo ok
38-
| |___________________^ missing concurrency setting
39-
|
40-
= note: audit confidence → High
30+
--> @@INPUT@@:2:1
31+
|
32+
2 | on: push
33+
| ^^^^^^^^ workflow is missing concurrency setting
34+
...
35+
7 | name: some-job
36+
| -------------- job affected by missing workflow concurrency
37+
|
38+
= note: audit confidence → High
4139
4240
1 finding: 0 informational, 1 low, 0 medium, 0 high
4341
"
@@ -81,23 +79,24 @@ fn test_jobs_missing_no_cancel() -> anyhow::Result<()> {
8179
.run()?,
8280
@"
8381
help[concurrency-limits]: insufficient job-level concurrency limits
84-
--> @@INPUT@@:9:5
82+
--> @@INPUT@@:2:1
8583
|
84+
2 | on: push
85+
| ^^^^^^^^ workflow is missing concurrency setting
86+
...
8687
9 | concurrency: group
87-
| ^^^^^^^^^^^^^^^^^^ job concurrency is missing cancel-in-progress
88+
| ------------------ job concurrency is missing cancel-in-progress
8889
|
8990
= note: audit confidence → High
9091
9192
help[concurrency-limits]: insufficient job-level concurrency limits
92-
--> @@INPUT@@:1:1
93+
--> @@INPUT@@:2:1
9394
|
94-
1 | / name: Workflow with job 1 missing cancel-in-progress and job 2 missing concurrency
95-
2 | | on: push
96-
3 | | permissions: {}
97-
... |
98-
17 | | - name: 2-ok
99-
18 | | run: echo ok
100-
| |___________________^ missing concurrency setting
95+
2 | on: push
96+
| ^^^^^^^^ workflow is missing concurrency setting
97+
...
98+
14 | name: job-2
99+
| ----------- job affected by missing workflow concurrency
101100
|
102101
= note: audit confidence → High
103102

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ fn test_workflow_read_all() -> Result<()> {
104104
|
105105
= note: audit confidence → High
106106
107-
3 findings (2 suppressed): 0 informational, 0 low, 1 medium, 0 high
107+
2 findings (1 suppressed): 0 informational, 0 low, 1 medium, 0 high
108108
"
109109
);
110110

@@ -128,7 +128,7 @@ fn test_workflow_write_all() -> Result<()> {
128128
|
129129
= note: audit confidence → High
130130
131-
3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high
131+
2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
132132
"
133133
);
134134

@@ -143,7 +143,7 @@ fn test_workflow_empty_perms() -> Result<()> {
143143
"excessive-permissions/workflow-empty-perms.yml"
144144
))
145145
.run()?,
146-
@"No findings to report. Good job! (2 suppressed)"
146+
@"No findings to report. Good job! (1 suppressed)"
147147
);
148148

149149
Ok(())
@@ -188,7 +188,7 @@ fn test_jobs_broaden_permissions() -> Result<()> {
188188
|
189189
= note: audit confidence → High
190190
191-
4 findings (2 suppressed): 0 informational, 0 low, 1 medium, 1 high
191+
3 findings (1 suppressed): 0 informational, 0 low, 1 medium, 1 high
192192
"
193193
);
194194

@@ -228,7 +228,7 @@ fn test_workflow_write_explicit() -> Result<()> {
228228
|
229229
= note: audit confidence → High
230230
231-
7 findings (4 suppressed): 0 informational, 0 low, 1 medium, 2 high
231+
6 findings (3 suppressed): 0 informational, 0 low, 1 medium, 2 high
232232
"
233233
);
234234

@@ -243,7 +243,7 @@ fn test_workflow_default_perms_all_jobs_explicit() -> Result<()> {
243243
"excessive-permissions/workflow-default-perms-all-jobs-explicit.yml"
244244
))
245245
.run()?,
246-
@"No findings to report. Good job! (5 suppressed)"
246+
@"No findings to report. Good job! (4 suppressed)"
247247
);
248248

249249
Ok(())

‎crates/zizmor/tests/integration/audit/obfuscation.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ fn test_obfuscation() -> Result<()> {
195195
= note: audit confidence → High
196196
= note: this finding has an auto-fix
197197
198-
38 findings (1 ignored, 17 suppressed, 19 fixable): 0 informational, 20 low, 0 medium, 0 high
198+
37 findings (1 ignored, 16 suppressed, 19 fixable): 0 informational, 20 low, 0 medium, 0 high
199199
"
200200
);
201201

‎crates/zizmor/tests/integration/audit/template_injection.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,21 @@ fn test_issue_1664() -> Result<()> {
595595
|
596596
= note: audit confidence → High
597597
598-
3 findings (1 ignored, 1 suppressed): 0 informational, 1 low, 0 medium, 0 high
598+
help[concurrency-limits]: insufficient job-level concurrency limits
599+
--> @@INPUT@@:3:1
600+
|
601+
3 | / "on":
602+
4 | | push:
603+
5 | | branches:
604+
6 | | - main
605+
| |____________^ workflow is missing concurrency setting
606+
...
607+
12 | name: Repro
608+
| ----------- job affected by missing workflow concurrency
609+
|
610+
= note: audit confidence → High
611+
612+
3 findings (1 suppressed): 0 informational, 2 low, 0 medium, 0 high
599613
"#
600614
);
601615

‎crates/zizmor/tests/integration/audit/use_trusted_publishing.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ fn test_cargo_publish() -> Result<()> {
203203
|
204204
= note: audit confidence → High
205205
206-
11 findings (5 suppressed): 6 informational, 0 low, 0 medium, 0 high
206+
10 findings (4 suppressed): 6 informational, 0 low, 0 medium, 0 high
207207
"
208208
);
209209

@@ -341,7 +341,7 @@ fn test_npm_publish() -> Result<()> {
341341
|
342342
= note: audit confidence → High
343343
344-
31 findings (6 ignored, 13 suppressed): 12 informational, 0 low, 0 medium, 0 high
344+
29 findings (6 ignored, 11 suppressed): 12 informational, 0 low, 0 medium, 0 high
345345
"
346346
);
347347

@@ -400,7 +400,7 @@ fn test_nuget_push() -> Result<()> {
400400
|
401401
= note: audit confidence → High
402402
403-
7 findings (4 suppressed): 3 informational, 0 low, 0 medium, 0 high
403+
6 findings (3 suppressed): 3 informational, 0 low, 0 medium, 0 high
404404
"
405405
);
406406

@@ -560,7 +560,7 @@ fn test_bun_publish() -> Result<()> {
560560
|
561561
= note: audit confidence → High
562562
563-
8 findings (4 suppressed): 4 informational, 0 low, 0 medium, 0 high
563+
7 findings (3 suppressed): 4 informational, 0 low, 0 medium, 0 high
564564
"
565565
);
566566

‎crates/zizmor/tests/integration/e2e.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,7 @@ fn test_github_output() -> Result<()> {
673673
::error file=@@INPUT@@,line=11,title=excessive-permissions::several-vulnerabilities.yml:11: overly broad permissions: uses write-all permissions
674674
::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
675675
::error file=@@INPUT@@,line=16,title=template-injection::several-vulnerabilities.yml:16: code injection via template expansion: may expand into attacker-controllable code
676-
::warning file=@@INPUT@@,line=1,title=concurrency-limits::several-vulnerabilities.yml:1: insufficient job-level concurrency limits: missing concurrency setting
676+
::warning file=@@INPUT@@,line=2,title=concurrency-limits::several-vulnerabilities.yml:2: insufficient job-level concurrency limits: workflow is missing concurrency setting
677677
"
678678
);
679679

‎crates/zizmor/tests/integration/e2e/anchors.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ fn test_steps_list_alias() -> Result<()> {
215215
= note: audit confidence → High
216216
= note: this finding has an auto-fix
217217
218-
6 findings (4 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
218+
5 findings (3 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
219219
"#
220220
);
221221

@@ -363,7 +363,7 @@ fn test_dummy_job_anchors() -> Result<()> {
363363
= note: audit confidence → High
364364
= note: this finding has an auto-fix
365365
366-
7 findings (4 suppressed, 3 fixable): 0 informational, 1 low, 2 medium, 0 high
366+
6 findings (3 suppressed, 3 fixable): 0 informational, 1 low, 2 medium, 0 high
367367
"
368368
);
369369

0 commit comments

Comments
 (0)