fix(skills): address PR 3219 review — case-insensitive deny, CLI policy, runtime warn

neo-wanderer · neo-wanderer · commit b6624ffec58f · 2026-04-26T16:07:35.000Z
- resolve_effective_passthrough lowercases both pattern and name before
  glob match, closing a bypass where 'aws_secret_access_key' would slip
  past the default 'AWS_*' deny pattern (Windows env vars are
  case-insensitive at the OS level). New unit test asserts mixed-case
  manifest entries are blocked.
- librefang skill test now loads [skills] from ~/.librefang/config.toml
  and applies the same EnvPassthroughPolicy the daemon does, falling
  back to SkillsConfig::default() when no config exists. Dev runs see
  the same gate prod will apply instead of silently passing every
  declared var.
- registry.load_skill emits a tracing::warn when env_passthrough is
  non-empty for Builtin / PromptOnly / Wasm runtimes, since the field
  only flows to subprocess-spawning runtimes (Python / Node / Shell)
  and was previously inert without feedback.
- Drop stale 'Returns None' sentence on EnvPassthroughPolicy::from_skills_config
  doc; the Optional-ness lives on KernelHandle::skill_env_passthrough_policy.
diff --git a/crates/librefang-cli/src/main.rs b/crates/librefang-cli/src/main.rs
@@ -1895,6 +1895,27 @@ fn load_update_channel_from_config() -> Option<librefang_types::config::UpdateCh
         .ok()
 }
 
+/// Load the `[skills]` config block and derive the `EnvPassthroughPolicy`
+/// the daemon would apply. Falls back to `SkillsConfig::default()` so the
+/// conservative built-in deny patterns still apply when no config exists —
+/// otherwise `librefang skill test` would silently allow vars that
+/// production strips. Errors during read/parse degrade to default; this is
+/// a dev-time gate, not a security boundary, but its job is to mirror
+/// what prod will do.
+fn load_skill_env_policy_from_config() -> librefang_types::config::EnvPassthroughPolicy {
+    let cfg = (|| -> Option<librefang_types::config::SkillsConfig> {
+        let config_path = dirs::home_dir()?.join(".librefang").join("config.toml");
+        let content = std::fs::read_to_string(&config_path).ok()?;
+        let value: toml::Value = toml::from_str(&content).ok()?;
+        let skills = value.get("skills")?.clone();
+        skills
+            .try_into::<librefang_types::config::SkillsConfig>()
+            .ok()
+    })()
+    .unwrap_or_default();
+    librefang_types::config::EnvPassthroughPolicy::from_skills_config(&cfg)
+}
+
 /// Load just the `log_dir` field from config.toml without fully deserializing.
 /// Returns the configured custom log directory, or `None` to use the default.
 fn load_log_dir_from_config() -> Option<PathBuf> {
@@ -6917,12 +6938,13 @@ fn cmd_skill_test(path: Option<PathBuf>, tool: Option<String>, input: Option<Str
     };
 
     let rt = tokio::runtime::Runtime::new().unwrap();
+    let env_policy = load_skill_env_policy_from_config();
     let result = rt.block_on(librefang_skills::loader::execute_skill_tool(
         &prepared.manifest,
         &prepared.source_dir,
         &tool_name,
         &input_json,
-        None,
+        Some(&env_policy),
     ));
     match result {
         Ok(result) => {
diff --git a/crates/librefang-skills/src/loader.rs b/crates/librefang-skills/src/loader.rs
@@ -120,9 +120,18 @@ pub fn resolve_effective_passthrough(
                 );
                 return false;
             }
-            let blocked_by_deny = denied
-                .iter()
-                .any(|pattern| librefang_types::capability::glob_matches(pattern, name));
+            // Match deny patterns case-insensitively. `glob_matches` itself
+            // is case-sensitive, but env-var names are conventionally
+            // upper-case and case-insensitive on Windows; lowercasing both
+            // sides closes a bypass where `aws_secret_access_key` would slip
+            // past the default `AWS_*` deny pattern.
+            let name_lower = name.to_ascii_lowercase();
+            let blocked_by_deny = denied.iter().any(|pattern| {
+                librefang_types::capability::glob_matches(
+                    &pattern.to_ascii_lowercase(),
+                    &name_lower,
+                )
+            });
             if blocked_by_deny {
                 let allowed_by_override = overrides
                     .map(|v| v.iter().any(|n| name_matches(n, name)))
@@ -703,6 +712,26 @@ mod tests {
         assert_eq!(resolved, vec!["GOG_KEYRING_PASSWORD".to_string()]);
     }
 
+    #[test]
+    fn test_resolve_passthrough_deny_pattern_is_case_insensitive() {
+        // Default deny includes `AWS_*` and `*_KEY`. Lowercase requests must
+        // still be blocked — Windows env-var names are case-insensitive at
+        // the OS level, so `aws_secret_access_key` resolves to the same
+        // value as `AWS_SECRET_ACCESS_KEY`.
+        let policy = EnvPassthroughPolicy {
+            denied_patterns: vec!["AWS_*".to_string(), "*_KEY".to_string()],
+            per_skill_overrides: std::collections::HashMap::new(),
+        };
+        let manifest = vec![
+            "aws_secret_access_key".to_string(),
+            "openai_api_key".to_string(),
+            "Aws_Region".to_string(),
+            "GOG_KEYRING_PASSWORD".to_string(),
+        ];
+        let resolved = resolve_effective_passthrough(&manifest, "any-skill", Some(&policy));
+        assert_eq!(resolved, vec!["GOG_KEYRING_PASSWORD".to_string()]);
+    }
+
     #[test]
     fn test_resolve_passthrough_per_skill_override_unblocks() {
         let mut overrides = std::collections::HashMap::new();
diff --git a/crates/librefang-skills/src/registry.rs b/crates/librefang-skills/src/registry.rs
@@ -286,6 +286,27 @@ impl SkillRegistry {
             }
         }
 
+        // env_passthrough only flows to subprocess-spawning runtimes
+        // (Python / Node / Shell). For other runtimes the field is silently
+        // inert; warn so authors don't think they've granted access.
+        if !manifest.env_passthrough.is_empty()
+            && !matches!(
+                manifest.runtime.runtime_type,
+                crate::SkillRuntime::Python
+                    | crate::SkillRuntime::Node
+                    | crate::SkillRuntime::Shell
+            )
+        {
+            warn!(
+                skill = %manifest.skill.name,
+                runtime = ?manifest.runtime.runtime_type,
+                vars = ?manifest.env_passthrough,
+                "skill declares env_passthrough but runtime does not spawn a \
+                 subprocess; field will be ignored. Move credentials to \
+                 [skill.config] or remove env_passthrough"
+            );
+        }
+
         let name = manifest.skill.name.clone();
 
         // Canonicalize the skill directory path so entry-point resolution
diff --git a/crates/librefang-types/src/config/types.rs b/crates/librefang-types/src/config/types.rs
@@ -1269,10 +1269,12 @@ pub struct EnvPassthroughPolicy {
 }
 
 impl EnvPassthroughPolicy {
-    /// Construct a policy from a `[skills]` config block. Returns `None`
-    /// when the operator has explicitly disabled the deny check (empty
-    /// patterns *and* empty overrides) — callers can skip the resolve step
-    /// in that case, since only the built-in hard blocks remain.
+    /// Construct a policy from a `[skills]` config block. Always returns a
+    /// populated `Self`; callers that want to skip applying the operator
+    /// gate entirely (e.g. an operator with empty deny patterns and no
+    /// per-skill overrides) should drop down to passing `None` for the
+    /// `env_policy` argument instead — that's `KernelHandle::skill_env_passthrough_policy`'s
+    /// job, not this constructor's.
     pub fn from_skills_config(cfg: &SkillsConfig) -> Self {
         Self {
             denied_patterns: cfg.env_passthrough_denied_patterns.clone(),
