Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 55 additions & 28 deletions crates/zizmor/src/audit/concurrency_limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,42 +58,20 @@ impl Audit for ConcurrencyLimits {
);
}
None => {
// Collect all jobs that are missing concurrency or have incomplete settings
let mut jobs_missing_concurrency = vec![];
let mut jobs_missing_cancel_in_progress = vec![];

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)?,
);
jobs_missing_cancel_in_progress.push(job);
}
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)?,
);
jobs_missing_concurrency.push(job);
}
// 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
Expand All @@ -102,6 +80,55 @@ impl Audit for ConcurrencyLimits {
_ => {}
}
}

// Create a single finding for jobs missing cancel-in-progress
if !jobs_missing_cancel_in_progress.is_empty() {
let mut finding_builder = Self::finding()
.confidence(Confidence::High)
.severity(Severity::Low)
.persona(Persona::Pedantic)
.add_location(
workflow
.location()
.primary()
.with_keys(["on".into()])
.annotated("workflow is missing concurrency setting"),
);

for job in &jobs_missing_cancel_in_progress {
finding_builder = finding_builder.add_location(
job.location()
.with_keys(["concurrency".into()])
.annotated("job concurrency is missing cancel-in-progress"),
);
}

findings.push(finding_builder.build(workflow)?);
}

// Create a single finding for jobs missing concurrency entirely
if !jobs_missing_concurrency.is_empty() {
let mut finding_builder = Self::finding()
.confidence(Confidence::High)
.severity(Severity::Low)
.persona(Persona::Pedantic)
.add_location(
workflow
.location()
.primary()
.with_keys(["on".into()])
.annotated("workflow is missing concurrency setting"),
);

for job in &jobs_missing_concurrency {
finding_builder = finding_builder.add_location(
job.location_with_grip()
.annotated("job affected by missing workflow concurrency"),
);
}

findings.push(finding_builder.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
Expand Down
6 changes: 3 additions & 3 deletions crates/zizmor/tests/integration/audit/cache_poisoning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ fn test_issue_642() -> anyhow::Result<()> {
|
= note: audit confidence → Low

3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
"
);

Expand Down Expand Up @@ -466,7 +466,7 @@ fn test_issue_1081() -> anyhow::Result<()> {
= note: audit confidence → Low
= note: this finding has an auto-fix

4 findings (2 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
3 findings (1 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
"
);

Expand Down Expand Up @@ -519,7 +519,7 @@ fn test_issue_1152() -> anyhow::Result<()> {
|
= note: audit confidence → Low

6 findings (3 suppressed): 0 informational, 0 low, 0 medium, 3 high
4 findings (1 suppressed): 0 informational, 0 low, 0 medium, 3 high
"
);

Expand Down
41 changes: 20 additions & 21 deletions crates/zizmor/tests/integration/audit/concurrency_limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,15 @@ fn test_missing() -> anyhow::Result<()> {
.run()?,
@"
help[concurrency-limits]: insufficient job-level concurrency limits
--> @@INPUT@@:1:1
|
1 | / name: Workflow without concurrency
2 | | on: push
3 | | permissions: {}
... |
10 | | - name: 1-ok
11 | | run: echo ok
| |___________________^ missing concurrency setting
|
= note: audit confidence → High
--> @@INPUT@@:2:1
|
2 | on: push
| ^^^^^^^^ workflow is missing concurrency setting
...
7 | name: some-job
| -------------- job affected by missing workflow concurrency
|
= note: audit confidence → High

1 finding: 0 informational, 1 low, 0 medium, 0 high
"
Expand Down Expand Up @@ -81,23 +79,24 @@ fn test_jobs_missing_no_cancel() -> anyhow::Result<()> {
.run()?,
@"
help[concurrency-limits]: insufficient job-level concurrency limits
--> @@INPUT@@:9:5
--> @@INPUT@@:2:1
|
2 | on: push
| ^^^^^^^^ workflow is missing concurrency setting
...
9 | concurrency: group
| ^^^^^^^^^^^^^^^^^^ job concurrency is missing cancel-in-progress
| ------------------ job concurrency is missing cancel-in-progress
|
= note: audit confidence → High

help[concurrency-limits]: insufficient job-level concurrency limits
--> @@INPUT@@:1:1
--> @@INPUT@@:2: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
2 | on: push
| ^^^^^^^^ workflow is missing concurrency setting
...
14 | name: job-2
| ----------- job affected by missing workflow concurrency
|
= note: audit confidence → High

Expand Down
12 changes: 6 additions & 6 deletions crates/zizmor/tests/integration/audit/excessive_permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ fn test_workflow_read_all() -> Result<()> {
|
= note: audit confidence → High

3 findings (2 suppressed): 0 informational, 0 low, 1 medium, 0 high
2 findings (1 suppressed): 0 informational, 0 low, 1 medium, 0 high
"
);

Expand All @@ -128,7 +128,7 @@ fn test_workflow_write_all() -> Result<()> {
|
= note: audit confidence → High

3 findings (2 suppressed): 0 informational, 0 low, 0 medium, 1 high
2 findings (1 suppressed): 0 informational, 0 low, 0 medium, 1 high
"
);

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

Ok(())
Expand Down Expand Up @@ -188,7 +188,7 @@ fn test_jobs_broaden_permissions() -> Result<()> {
|
= note: audit confidence → High

4 findings (2 suppressed): 0 informational, 0 low, 1 medium, 1 high
3 findings (1 suppressed): 0 informational, 0 low, 1 medium, 1 high
"
);

Expand Down Expand Up @@ -228,7 +228,7 @@ fn test_workflow_write_explicit() -> Result<()> {
|
= note: audit confidence → High

7 findings (4 suppressed): 0 informational, 0 low, 1 medium, 2 high
6 findings (3 suppressed): 0 informational, 0 low, 1 medium, 2 high
"
);

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

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion crates/zizmor/tests/integration/audit/obfuscation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ fn test_obfuscation() -> Result<()> {
= 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
37 findings (1 ignored, 16 suppressed, 19 fixable): 0 informational, 20 low, 0 medium, 0 high
"
);

Expand Down
16 changes: 15 additions & 1 deletion crates/zizmor/tests/integration/audit/template_injection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,21 @@ fn test_issue_1664() -> Result<()> {
|
= note: audit confidence → High

3 findings (1 ignored, 1 suppressed): 0 informational, 1 low, 0 medium, 0 high
help[concurrency-limits]: insufficient job-level concurrency limits
--> @@INPUT@@:3:1
|
3 | / "on":
4 | | push:
5 | | branches:
6 | | - main
| |____________^ workflow is missing concurrency setting
...
12 | name: Repro
| ----------- job affected by missing workflow concurrency
|
= note: audit confidence → High

3 findings (1 suppressed): 0 informational, 2 low, 0 medium, 0 high
"#
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ fn test_cargo_publish() -> Result<()> {
|
= note: audit confidence → High

11 findings (5 suppressed): 6 informational, 0 low, 0 medium, 0 high
10 findings (4 suppressed): 6 informational, 0 low, 0 medium, 0 high
"
);

Expand Down Expand Up @@ -341,7 +341,7 @@ fn test_npm_publish() -> Result<()> {
|
= note: audit confidence → High

31 findings (6 ignored, 13 suppressed): 12 informational, 0 low, 0 medium, 0 high
29 findings (6 ignored, 11 suppressed): 12 informational, 0 low, 0 medium, 0 high
"
);

Expand Down Expand Up @@ -400,7 +400,7 @@ fn test_nuget_push() -> Result<()> {
|
= note: audit confidence → High

7 findings (4 suppressed): 3 informational, 0 low, 0 medium, 0 high
6 findings (3 suppressed): 3 informational, 0 low, 0 medium, 0 high
"
);

Expand Down Expand Up @@ -560,7 +560,7 @@ fn test_bun_publish() -> Result<()> {
|
= note: audit confidence → High

8 findings (4 suppressed): 4 informational, 0 low, 0 medium, 0 high
7 findings (3 suppressed): 4 informational, 0 low, 0 medium, 0 high
"
);

Expand Down
2 changes: 1 addition & 1 deletion crates/zizmor/tests/integration/e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,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=2,title=concurrency-limits::several-vulnerabilities.yml:2: insufficient job-level concurrency limits: workflow is missing concurrency setting
"
);

Expand Down
4 changes: 2 additions & 2 deletions crates/zizmor/tests/integration/e2e/anchors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ fn test_steps_list_alias() -> Result<()> {
= note: audit confidence → High
= note: this finding has an auto-fix

6 findings (4 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
5 findings (3 suppressed, 2 fixable): 0 informational, 0 low, 0 medium, 2 high
"#
);

Expand Down Expand Up @@ -363,7 +363,7 @@ fn test_dummy_job_anchors() -> Result<()> {
= note: audit confidence → High
= note: this finding has an auto-fix

7 findings (4 suppressed, 3 fixable): 0 informational, 1 low, 2 medium, 0 high
6 findings (3 suppressed, 3 fixable): 0 informational, 1 low, 2 medium, 0 high
"
);

Expand Down
Loading