fix(backend): validate ansible vault_id entries before config generation (#9681)

* fix(backend): validate ansible vault_id entries before config generation

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(backend): cover ansible.cfg write-boundary vault_id validation

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

backend/parsers/windmill-parser-yaml/src/lib.rs

@@ -50,7 +50,7 @@ pub fn parse_ansible_sig(inner_content: &str) -> anyhow::Result<MainArgSignature
                                         has_default: default.is_some(),
                                         default,
                                         oidx: None,
-                                    otyp_inferred: false,
+                                        otyp_inferred: false,
                                     })
                                 }
                             }
@@ -69,7 +69,7 @@ pub fn parse_ansible_sig(inner_content: &str) -> anyhow::Result<MainArgSignature
                             has_default: inv.default.is_some(),
                             default: inv.default.map(|v| json!(format!("$res:{}", v))),
                             oidx: None,
-                        otyp_inferred: false,
+                            otyp_inferred: false,
                         });
                     }
                 }
@@ -83,7 +83,7 @@ pub fn parse_ansible_sig(inner_content: &str) -> anyhow::Result<MainArgSignature
                                 has_default: false,
                                 default: None,
                                 oidx: None,
-                            otyp_inferred: false,
+                                otyp_inferred: false,
                             });
                         }
                     }
@@ -435,6 +435,25 @@ pub fn parse_delegate_to_git_repo(inner_content: &str) -> anyhow::Result<Delegat
     Ok(DelegateWithSSHAuth { delegate_to_git_repo_details: None, git_ssh_identity })
 }
 
+/// Each `vault_id` entry is interpolated verbatim into the generated `ansible.cfg`
+/// (`vault_identity_list = <a>,<b>,...`). A newline or other config-meaningful
+/// character would let a script inject arbitrary `[defaults]` directives (e.g.
+/// `library`, `action_plugins`) and execute attacker-controlled code on the worker,
+/// and a `,` would smuggle in an extra entry. Restrict entries to the `label@source`
+/// charset so neither is possible.
+pub fn validate_vault_id(value: &str) -> anyhow::Result<()> {
+    let is_valid = !value.is_empty()
+        && value
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | '/' | '@'));
+    if !is_valid {
+        return Err(anyhow!(
+            "Invalid vault_id `{value}`: expected `label@filename` using only letters, digits and the characters `.`, `_`, `-`, `/`, `@`"
+        ));
+    }
+    Ok(())
+}
+
 pub fn parse_ansible_reqs(
     inner_content: &str,
 ) -> anyhow::Result<(String, Option<AnsibleRequirements>, String)> {
@@ -528,6 +547,7 @@ pub fn parse_ansible_reqs(
                         let Yaml::String(filename) = f else {
                             return Err(anyhow!("The elements of the vault_id field should be strings in the format: `label@filename`"));
                         };
+                        validate_vault_id(filename)?;
                         ret.vault_id.push(filename.to_string());
                     }
                 }
@@ -1051,4 +1071,55 @@ delegate_to_git_repo:
             Some("inventories/{{ env }}")
         );
     }
+
+    #[test]
+    fn test_parse_vault_id_valid() {
+        let p = r#"
+---
+vault_id:
+  - dev@vault_pass_dev.txt
+  - prod@./secrets/prod-pass
+---
+- name: Test
+  hosts: all
+"#;
+        let (_, reqs, _) = parse_ansible_reqs(p).unwrap();
+        assert_eq!(
+            reqs.unwrap().vault_id,
+            vec![
+                "dev@vault_pass_dev.txt".to_string(),
+                "prod@./secrets/prod-pass".to_string()
+            ]
+        );
+    }
+
+    #[test]
+    fn test_parse_vault_id_rejects_newline_injection() {
+        let p = "---\nvault_id:\n  - \"default@/tmp/wm/x\\nlibrary = /tmp/wm/evil_modules\"\n---\n- name: Test\n  hosts: all\n";
+        assert!(parse_ansible_reqs(p).is_err());
+    }
+
+    #[test]
+    fn test_parse_vault_id_rejects_comma() {
+        let p = r#"
+---
+vault_id:
+  - "a@b,c@d"
+---
+- name: Test
+  hosts: all
+"#;
+        assert!(parse_ansible_reqs(p).is_err());
+    }
+
+    #[test]
+    fn test_validate_vault_id() {
+        assert!(validate_vault_id("default@/tmp/wm/pass").is_ok());
+        assert!(validate_vault_id("dev@pass.txt").is_ok());
+        assert!(validate_vault_id("").is_err());
+        assert!(validate_vault_id("a@b\nlibrary = /evil").is_err());
+        assert!(validate_vault_id("a@b,c@d").is_err());
+        assert!(validate_vault_id("a@b c").is_err());
+        assert!(validate_vault_id("a@b=c").is_err());
+    }
 }

backend/windmill-worker/src/ansible_executor.rs

@@ -22,7 +22,8 @@ use windmill_common::{
 use windmill_queue::MiniPulledJob;
 
 use windmill_parser_yaml::{
-    AnsibleRequirements, GitRepo, PreexistingAnsibleInventory, ResourceOrVariablePath,
+    validate_vault_id, AnsibleRequirements, GitRepo, PreexistingAnsibleInventory,
+    ResourceOrVariablePath,
 };
 use windmill_queue::{append_logs, CanceledBy};
 
@@ -910,6 +911,11 @@ pub fn create_ansible_cfg(
     }
     if let Some(vault_ids) = reqs.as_ref().map(|r| &r.vault_id) {
         if !vault_ids.is_empty() {
+            // Defense in depth: entries are validated at parse time, but re-check here
+            // since they are interpolated raw into ansible.cfg (config-directive injection).
+            for vault_id in vault_ids {
+                validate_vault_id(vault_id)?;
+            }
             let password_files = vault_ids.join(",");
 
             passwords_cfg.push_str(&format!("vault_identity_list = {password_files}\n"));
@@ -1799,4 +1805,31 @@ mod tests {
         assert!(validate_relative_path("", "playbook").is_err());
         assert!(validate_relative_path("   ", "playbook").is_err());
     }
+
+    #[test]
+    fn test_create_ansible_cfg_writes_valid_vault_id() {
+        let dir = tempfile::tempdir().unwrap();
+        let job_dir = dir.path().to_str().unwrap();
+        let reqs = AnsibleRequirements {
+            vault_id: vec!["dev@vault_pass.txt".to_string()],
+            ..Default::default()
+        };
+        create_ansible_cfg(Some(&reqs), job_dir, false).unwrap();
+        let cfg = std::fs::read_to_string(dir.path().join("ansible.cfg")).unwrap();
+        assert!(cfg.contains("vault_identity_list = dev@vault_pass.txt"));
+        assert!(!cfg.contains("library"));
+    }
+
+    #[test]
+    fn test_create_ansible_cfg_rejects_vault_id_injection() {
+        let dir = tempfile::tempdir().unwrap();
+        let job_dir = dir.path().to_str().unwrap();
+        let reqs = AnsibleRequirements {
+            vault_id: vec!["default@/tmp/wm/x\nlibrary = /tmp/wm/evil_modules".to_string()],
+            ..Default::default()
+        };
+        // Defense-in-depth boundary: a poisoned entry must error before any config is written.
+        assert!(create_ansible_cfg(Some(&reqs), job_dir, false).is_err());
+        assert!(!dir.path().join("ansible.cfg").exists());
+    }
 }