feat: improve forbidden-uses error message on invalid config (#1381)

crates/zizmor/src/audit/forbidden_uses.rs

@@ -2,7 +2,7 @@ use github_actions_models::common::Uses;
 
 use super::{Audit, AuditLoadError, AuditState, audit_meta};
 use crate::audit::AuditError;
-use crate::config::{Config, ForbiddenUsesConfig};
+use crate::config::{Config, ForbiddenUsesConfigInner};
 use crate::finding::{Confidence, Finding, Persona, Severity};
 use crate::models::{StepCommon, action::CompositeStep, workflow::Step};
 
@@ -11,7 +11,7 @@ pub(crate) struct ForbiddenUses;
 audit_meta!(ForbiddenUses, "forbidden-uses", "forbidden action used");
 
 impl ForbiddenUses {
-    fn use_denied(&self, uses: &Uses, config: &ForbiddenUsesConfig) -> bool {
+    fn use_denied(&self, uses: &Uses, config: &ForbiddenUsesConfigInner) -> bool {
         match uses {
             // Local uses are never denied.
             Uses::Local(_) => false,
@@ -23,10 +23,10 @@ impl ForbiddenUses {
                 false
             }
             Uses::Repository(uses) => match config {
-                ForbiddenUsesConfig::Allow { allow } => {
+                ForbiddenUsesConfigInner::Allow(allow) => {
                     !allow.iter().any(|pattern| pattern.matches(uses))
                 }
-                ForbiddenUsesConfig::Deny { deny } => {
+                ForbiddenUsesConfigInner::Deny(deny) => {
                     deny.iter().any(|pattern| pattern.matches(uses))
                 }
             },

crates/zizmor/src/config.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, fs, num::NonZeroUsize, str::FromStr};
+use std::{collections::HashMap, fs, num::NonZeroUsize, ops::Deref, str::FromStr};
 
 use anyhow::{Context as _, anyhow};
 use camino::Utf8Path;
@@ -168,11 +168,33 @@ impl Default for DependabotCooldownConfig {
     }
 }
 
+/// Slightly annoying wrapper for [`ForbiddenUsesConfigInner`], which is our
+/// real configuration type for the `forbidden-uses` rule.
+///
+/// We need this wrapper type so that we can apply the `singleton_map`
+/// deserializer to the inner type, ensuring that we deserialize from a
+/// mapping with an explicit key discriminant (i.e. `allow:` or `deny:`)
+/// rather than a YAML tag. We could work around this by using serde's
+/// `untagged` instead, but this produces suboptimal user-facing error messages.
 #[derive(Clone, Debug, Deserialize)]
-#[serde(rename_all = "kebab-case", untagged)]
-pub(crate) enum ForbiddenUsesConfig {
-    Allow { allow: Vec<RepositoryUsesPattern> },
-    Deny { deny: Vec<RepositoryUsesPattern> },
+#[serde(transparent)]
+pub(crate) struct ForbiddenUsesConfig(
+    #[serde(with = "serde_yaml::with::singleton_map")] pub(crate) ForbiddenUsesConfigInner,
+);
+
+impl Deref for ForbiddenUsesConfig {
+    type Target = ForbiddenUsesConfigInner;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+#[derive(Clone, Debug, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+pub(crate) enum ForbiddenUsesConfigInner {
+    Allow(Vec<RepositoryUsesPattern>),
+    Deny(Vec<RepositoryUsesPattern>),
 }
 
 /// Config for the `unpinned-uses` rule.

crates/zizmor/tests/integration/audit/forbidden_uses.rs

@@ -1,4 +1,4 @@
-use crate::common::{input_under_test, zizmor};
+use crate::common::{OutputMode, input_under_test, zizmor};
 use anyhow::Result;
 
 #[test]
@@ -172,3 +172,61 @@ fn test_allow_some_refs() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_config_invalid_pattern() -> Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+            .expects_failure(true)
+            .input(input_under_test("neutral.yml"))
+            .config(input_under_test(
+                "forbidden-uses/configs/invalid-pattern.yml"
+            ))
+            .output(OutputMode::Stderr)
+            .run()?,
+        @r"
+    🌈 zizmor v@@VERSION@@
+    fatal: no audit was performed
+    error: configuration error in @@CONFIG@@
+      |
+      = help: check the configuration for the 'forbidden-uses' rule
+      = help: see: https://docs.zizmor.sh/audits/#forbidden-uses-configuration
+
+    Caused by:
+        0: configuration error in @@CONFIG@@
+        1: invalid syntax for audit `forbidden-uses`
+        2: invalid pattern: */*
+    "
+    );
+
+    Ok(())
+}
+
+#[test]
+fn test_config_invalid_variant() -> Result<()> {
+    insta::assert_snapshot!(
+        zizmor()
+            .expects_failure(true)
+            .input(input_under_test("neutral.yml"))
+            .config(input_under_test(
+                "forbidden-uses/configs/invalid-variant.yml"
+            ))
+            .output(OutputMode::Stderr)
+            .run()?,
+        @r"
+    🌈 zizmor v@@VERSION@@
+    fatal: no audit was performed
+    error: configuration error in @@CONFIG@@
+      |
+      = help: check the configuration for the 'forbidden-uses' rule
+      = help: see: https://docs.zizmor.sh/audits/#forbidden-uses-configuration
+
+    Caused by:
+        0: configuration error in @@CONFIG@@
+        1: invalid syntax for audit `forbidden-uses`
+        2: unknown variant `mystery-variant`, expected `allow` or `deny`
+    "
+    );
+
+    Ok(())
+}

crates/zizmor/tests/integration/config.rs

@@ -279,31 +279,6 @@ fn test_invalid_configs() -> anyhow::Result<()> {
     "
     );
 
-    // forbidden-uses audit config is invalid.
-    insta::assert_snapshot!(
-        zizmor()
-            .expects_failure(true)
-            .input(input_under_test("neutral.yml"))
-            .config(input_under_test(
-                "config-scenarios/zizmor.invalid-schema-2.yml"
-            ))
-            .output(OutputMode::Stderr)
-            .run()?,
-        @r"
-    🌈 zizmor v@@VERSION@@
-    fatal: no audit was performed
-    error: configuration error in @@CONFIG@@
-      |
-      = help: check the configuration for the 'forbidden-uses' rule
-      = help: see: https://docs.zizmor.sh/audits/#forbidden-uses-configuration
-
-    Caused by:
-        0: configuration error in @@CONFIG@@
-        1: invalid syntax for audit `forbidden-uses`
-        2: data did not match any variant of untagged enum ForbiddenUsesConfig
-    "
-    );
-
     // unpinned-uses audit config is invalid.
     insta::assert_snapshot!(
         zizmor()

crates/zizmor/tests/integration/test-data/forbidden-uses/configs/invalid-pattern.yml

@@ -1,4 +1,4 @@
-# zizmor.yml config file with an invalid schema
+# zizmor.yml config file with an invalid schema for the 'forbidden-uses' rule.
 
 rules:
   forbidden-uses:

crates/zizmor/tests/integration/test-data/forbidden-uses/configs/invalid-variant.yml

@@ -0,0 +1,7 @@
+# zizmor.yml config file with an invalid schema for the 'forbidden-uses' rule.
+
+rules:
+  forbidden-uses:
+    config:
+      mystery-variant:
+        - "actions/*"

docs/release-notes.md

@@ -20,6 +20,9 @@ of `zizmor`.
 * The [dependabot-cooldown] audit can now be configured with a custom
   minimum cooldown period via `rules.dependabot-cooldown.config.days`
   (#1377)
+  
+* `zizmor` now produces slightly more useful error messages when the user supplies
+  an invalid configuration for the [forbidden-uses] audit (#1381)
 
 ### Bug Fixes 🐛