From 3c1309ca9dd2eed0d2c71f30de4300256f31dd2a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sat, 18 Jan 2025 23:19:30 -0500 Subject: [PATCH] bugfix: special-case workflow_call in excessive-permissions (#473) --- .github/workflows/zizmor.yml | 2 + Cargo.lock | 4 +- Cargo.toml | 2 +- docs/release-notes.md | 5 ++ src/audit/excessive_permissions.rs | 61 ++++++++++++------- src/models.rs | 22 ++++++- src/registry.rs | 2 +- tests/snapshot.rs | 19 ++++++ .../snapshot__excessive_permissions-10.snap | 21 +++++++ .../snapshot__excessive_permissions-11.snap | 19 ++++++ .../snapshot__excessive_permissions-12.snap | 47 ++++++++++++++ .../excessive-permissions/issue-472-repro.yml | 23 +++++++ .../reusable-workflow-call.yml | 9 +++ .../reusable-workflow-other-triggers.yml | 21 +++++++ 14 files changed, 229 insertions(+), 28 deletions(-) create mode 100644 tests/snapshots/snapshot__excessive_permissions-10.snap create mode 100644 tests/snapshots/snapshot__excessive_permissions-11.snap create mode 100644 tests/snapshots/snapshot__excessive_permissions-12.snap create mode 100644 tests/test-data/excessive-permissions/issue-472-repro.yml create mode 100644 tests/test-data/excessive-permissions/reusable-workflow-call.yml create mode 100644 tests/test-data/excessive-permissions/reusable-workflow-other-triggers.yml diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index d1d12b8..bc1fc24 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -6,6 +6,8 @@ on: pull_request: branches: ["*"] +permissions: {} + jobs: zizmor: name: zizmor latest via Cargo diff --git a/Cargo.lock b/Cargo.lock index 7ca5bd9..26479b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -616,9 +616,9 @@ checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" [[package]] name = "github-actions-models" -version = "0.21.0" +version = "0.22.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "768ea1269c648e6eb2e2fc9143e609609a1587150f0ad3342305ae2ae2d217ca" +checksum = "ea4c30fa8bf11e002d3ca72233e7a7bac33ffce4dc50877d63a8f5a161e0cd84" dependencies = [ "indexmap", "serde", diff --git a/Cargo.toml b/Cargo.toml index 1d95162..a513037 100644 --- a/Cargo.toml +++ b/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" diff --git a/docs/release-notes.md b/docs/release-notes.md index 31e14ee..1309ba3 100644 --- a/docs/release-notes.md +++ b/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 diff --git a/src/audit/excessive_permissions.rs b/src/audit/excessive_permissions.rs index 29f403f..b266780 100644 --- a/src/audit/excessive_permissions.rs +++ b/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,20 +94,35 @@ 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(), ) { @@ -114,6 +130,7 @@ impl Audit for ExcessivePermissions { Self::finding() .severity(severity) .confidence(confidence) + .persona(job_finding_persona) .add_location(job_location) .add_location(perm_location.primary()) .build(workflow)?, diff --git a/src/models.rs b/src/models.rs index f68d75e..9c22852 100644 --- a/src/models.rs +++ b/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,7 +144,7 @@ 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, @@ -152,6 +152,24 @@ impl Workflow { 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. diff --git a/src/registry.rs b/src/registry.rs index ff1b3de..d0eda40 100644 --- a/src/registry.rs +++ b/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(), diff --git a/tests/snapshot.rs b/tests/snapshot.rs index 142fda2..632c29d 100644 --- a/tests/snapshot.rs +++ b/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(()) } diff --git a/tests/snapshots/snapshot__excessive_permissions-10.snap b/tests/snapshots/snapshot__excessive_permissions-10.snap new file mode 100644 index 0000000..8d6183c --- /dev/null +++ b/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 diff --git a/tests/snapshots/snapshot__excessive_permissions-11.snap b/tests/snapshots/snapshot__excessive_permissions-11.snap new file mode 100644 index 0000000..17b6fbc --- /dev/null +++ b/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 diff --git a/tests/snapshots/snapshot__excessive_permissions-12.snap b/tests/snapshots/snapshot__excessive_permissions-12.snap new file mode 100644 index 0000000..203bf0f --- /dev/null +++ b/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 diff --git a/tests/test-data/excessive-permissions/issue-472-repro.yml b/tests/test-data/excessive-permissions/issue-472-repro.yml new file mode 100644 index 0000000..2f4c867 --- /dev/null +++ b/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 diff --git a/tests/test-data/excessive-permissions/reusable-workflow-call.yml b/tests/test-data/excessive-permissions/reusable-workflow-call.yml new file mode 100644 index 0000000..c1dc021 --- /dev/null +++ b/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 diff --git a/tests/test-data/excessive-permissions/reusable-workflow-other-triggers.yml b/tests/test-data/excessive-permissions/reusable-workflow-other-triggers.yml new file mode 100644 index 0000000..c5c4c77 --- /dev/null +++ b/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