secrets-outside-env: flag secrets in reusable workflow calls

crates/github-actions-expressions/src/identifier.rs

@@ -7,13 +7,13 @@
 #[derive(Debug)]
 pub struct Identifier<'src>(pub(crate) &'src str);
 
-impl Identifier<'_> {
+impl<'src> Identifier<'src> {
     /// Returns the identifier as a string slice, as it appears in the
     /// expression.
     ///
     /// Important: identifiers are case-insensitive, so this should not
     /// be used for comparisons.
-    pub fn as_str(&self) -> &str {
+    pub fn as_str(&self) -> &'src str {
         self.0
     }
 }

crates/zizmor/src/audit/secrets_outside_env.rs

@@ -6,9 +6,12 @@ use crate::{
     config::Config,
     finding::{
         Confidence, Finding, Persona, Severity,
-        location::{Feature, Locatable, Location},
+        location::{Feature, Locatable, Location, Routable},
+    },
+    models::{
+        AsDocument,
+        workflow::{JobCommon as _, NormalJob, ReusableWorkflowCallJob},
     },
-    models::workflow::{JobCommon as _, NormalJob},
     state::AuditState,
     utils::{once::warn_once, parse_fenced_expressions_from_routable},
 };
@@ -30,6 +33,44 @@ impl Audit for SecretsOutsideEnvironment {
         Ok(Self)
     }
 
+    async fn audit_reusable_job<'doc>(
+        &self,
+        job: &ReusableWorkflowCallJob<'doc>,
+        config: &Config,
+    ) -> Result<Vec<Finding<'doc>>, AuditError> {
+        // Unlike normal jobs, jobs that call reusable workflows can't
+        // activate an environment directly. This essentially means that any
+        // secrets referenced by the caller are guaranteed to be outside of a
+        // dedicated environment.
+        let mut findings = vec![];
+        for (secret, subfeature) in Self::secrets_in_routable(job) {
+            if config
+                .secrets_outside_env_policy
+                .allow
+                .contains(&secret.to_ascii_lowercase())
+            {
+                continue;
+            }
+
+            findings.push(
+                Self::finding()
+                    .persona(Persona::Auditor)
+                    .severity(Severity::Medium)
+                    .confidence(Confidence::High)
+                    .add_location(job.location().key_only())
+                    .add_raw_location(Location::new(
+                        job.location()
+                            .primary()
+                            .annotated("secret is accessed outside of a dedicated environment"),
+                        Feature::from_subfeature(&subfeature, job),
+                    ))
+                    .build(job)?,
+            );
+        }
+
+        Ok(findings)
+    }
+
     async fn audit_normal_job<'doc>(
         &self,
         job: &NormalJob<'doc>,
@@ -60,48 +101,62 @@ impl Audit for SecretsOutsideEnvironment {
         // GitHub Actions doesn't require fencing on expressions. In practice however GitHub Actions
         // doesn't allow users to reference secrets in `if:` clauses.
         let mut findings = vec![];
-        for (expr, span) in parse_fenced_expressions_from_routable(job) {
-            let Ok(parsed) = Expr::parse(expr.as_bare()) else {
-                warn_once!("couldn't parse expression: {expr}", expr = expr.as_bare());
+        for (secret, subfeature) in Self::secrets_in_routable(job) {
+            if config
+                .secrets_outside_env_policy
+                .allow
+                .contains(&secret.to_ascii_lowercase())
+            {
                 continue;
-            };
-
-            for (context, origin) in parsed.contexts() {
-                if !context.child_of("secrets") {
-                    continue;
-                }
-
-                // Check to see whether we allow this secret. The policy always includes
-                // GITHUB_TOKEN, since it's always latently available.
-                if let Some(secret_name) = context.single_tail()
-                    && config
-                        .secrets_outside_env_policy
-                        .allow
-                        .contains(&secret_name.to_ascii_lowercase())
-                {
-                    continue;
-                }
-
-                let after = span.start + origin.span.start;
-                let subfeature = Subfeature::new(after, origin.raw);
-
-                findings.push(
-                    Self::finding()
-                        .persona(Persona::Auditor)
-                        .severity(Severity::Medium)
-                        .confidence(Confidence::High)
-                        .add_location(job.location().key_only())
-                        .add_raw_location(Location::new(
-                            job.location()
-                                .primary()
-                                .annotated("secret is accessed outside of a dedicated environment"),
-                            Feature::from_subfeature(&subfeature, job),
-                        ))
-                        .build(job)?,
-                );
             }
+
+            findings.push(
+                Self::finding()
+                    .persona(Persona::Auditor)
+                    .severity(Severity::Medium)
+                    .confidence(Confidence::High)
+                    .add_location(job.location().key_only())
+                    .add_raw_location(Location::new(
+                        job.location()
+                            .primary()
+                            .annotated("secret is accessed outside of a dedicated environment"),
+                        Feature::from_subfeature(&subfeature, job),
+                    ))
+                    .build(job)?,
+            );
         }
 
         Ok(findings)
     }
 }
+
+impl SecretsOutsideEnvironment {
+    fn secrets_in_routable<'a, 'doc>(
+        routable: &'a (impl Locatable<'doc> + AsDocument<'a, 'doc> + Routable<'a, 'doc>),
+    ) -> Vec<(String, Subfeature<'doc>)> {
+        parse_fenced_expressions_from_routable(routable)
+            .iter()
+            .flat_map(|(expr, span)| {
+                let Ok(parsed) = Expr::parse(expr.as_bare()) else {
+                    warn_once!("couldn't parse expression: {expr}", expr = expr.as_bare());
+                    return vec![];
+                };
+
+                parsed
+                    .contexts()
+                    .into_iter()
+                    .filter_map(|(context, origin)| {
+                        if context.child_of("secrets") {
+                            // TODO(ww): Minor, but it would be nice to avoid this clone.
+                            let name = context.single_tail()?.to_string();
+                            let after = span.start + origin.span.start;
+                            Some((name, Subfeature::new(after, origin.raw)))
+                        } else {
+                            None
+                        }
+                    })
+                    .collect::<Vec<_>>()
+            })
+            .collect()
+    }
+}

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/secrets_outside_env.rs

@@ -151,3 +151,39 @@ fn test_config_allow_some() -> Result<()> {
     );
     Ok(())
 }
+
+#[test]
+fn test_issue_1773() -> Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+        .args(["--persona=auditor"])
+        .input(input_under_test("secrets-outside-env/issue-1773-repro.yml"))
+        .run()?,
+        @"
+    warning[secrets-outside-env]: secrets referenced without a dedicated environment
+      --> @@INPUT@@:13:28
+       |
+    10 |   pull:
+       |   ---- this job
+    ...
+    13 |       zulip-api-token: ${{ secrets.ZULIP_API_TOKEN }}
+       |                            ^^^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
+       |
+       = note: audit confidence → High
+
+    warning[secrets-outside-env]: secrets referenced without a dedicated environment
+      --> @@INPUT@@:14:30
+       |
+    10 |   pull:
+       |   ---- this job
+    ...
+    14 |       github-app-secret: ${{ secrets.APP_PRIVATE_KEY }}
+       |                              ^^^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
+       |
+       = note: audit confidence → High
+
+    2 findings: 0 informational, 0 low, 2 medium, 0 high
+    "
+    );
+    Ok(())
+}

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

@@ -1124,4 +1124,4 @@ error[dangerous-triggers]: use of fundamentally insecure workflow trigger
    |
    = note: audit confidence → Medium
 
-217 findings (118 suppressed): 7 informational, 0 low, 29 medium, 63 high
+216 findings (117 suppressed): 7 informational, 0 low, 29 medium, 63 high

crates/zizmor/tests/integration/test-data/secrets-outside-env/issue-1773-repro.yml

@@ -0,0 +1,14 @@
+name: ISSUE-1773-REPRO
+permissions: {}
+on:
+  workflow_dispatch:
+  schedule:
+    # Run at 04:00 UTC every Monday and Thursday
+    - cron: "0 4 * * 1,4"
+
+jobs:
+  pull:
+    uses: rust-lang/josh-sync/.github/workflows/rustc-pull.yml@a097aeb82f49801051560482307ceaca7e58bf97 # main
+    secrets:
+      zulip-api-token: ${{ secrets.ZULIP_API_TOKEN }}
+      github-app-secret: ${{ secrets.APP_PRIVATE_KEY }}

docs/release-notes.md

@@ -33,6 +33,9 @@ of `zizmor`.
   it encounters a cooldown used with a multi-ecosystem-group, as the two
   do not interact well (#1780)
 
+* The [secrets-outside-env] audit now flags secret usages on jobs that
+  call reusable workflows (#1792)
+
 ### Bug Fixes 🐛
 
 * Fixed a bug where auto-fixes for the [template-injection] audit would fail