diff --git a/AGENTS.md b/AGENTS.md index 77eabaa..ced54ea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,7 @@ elevated launches always read the machine config dir). | **This launch only: other manifest** | `va -m readonly.env.tpl claude` | | Interactive pick + optional -m | `va -m narrow.env.tpl pick` | | One-shot command | `va run -m REFS --backend bitwarden -- cmd…` | -| Map new vault secrets into refs | `va refresh` / `va refresh --backend onepassword` | +| Map new vault secrets into refs | `va refresh` / `va refresh --backend onepassword` (also splits 0.3.0 glued `VAR=name:KEY` lines) | | Skip fields by name pattern (1P) | `va refresh --exclude '*_USERNAME'` | | Remove dangling refs / repair renamed refs | `va refresh --prune` (repair is bitwarden only) | | Edit a refs file (with checks) | `va edit-manifest` / `va edit-manifest name.env.tpl` | diff --git a/MIGRATION.md b/MIGRATION.md index 45b9a8a..028804f 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -1,3 +1,24 @@ +# Migration: glued Bitwarden refs from bash 0.3.0 `va refresh` (unreleased) + +`va refresh` on the bash launcher (v0.3.0) captured each new `VAR=name:KEY` +line with `$(…)`, which strips the trailing newline, then concatenated. A +refs file could end up with one physical line: + +``` +META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEY… +``` + +Launch used to send that blob to the vault and fail with `no secret matched +'name:META_AI_API_KEYFIREWORKS_API_KEY=name:…'`. + +**What to do.** `va refresh` now splits those mappings onto their own lines +(even when every secret already appears as a substring, so a second refresh +is not a no-op). `va doctor` and `va secrets validate --offline` fail closed +on the glued shape and print the recovered lines. + +A host still running the 0.3.0 bash binary will re-glue on the next merge +refresh. `va update` (or a reinstall) is the way off that writer. + # Migration: `va update` replaces the installed binary (unreleased) `va update` downloads a GitHub release asset for this OS/arch and overwrites diff --git a/README.md b/README.md index 546973e..faea606 100644 --- a/README.md +++ b/README.md @@ -200,6 +200,7 @@ va run -m openai.env.refs --backend bitwarden -p -- \ | Rotated a value | Nothing — next launch fetches live | | Added a secret you want mapped | `va refresh` (merge) or `va refresh --replace --all` | | Renamed or removed a secret | `va refresh --prune` (removes mappings nothing resolves; repairs renamed ones on Bitwarden) | +| Launch fails on a huge `name:KEYOTHER=name:OTHER` ref | `va refresh` (splits the bash 0.3.0 glued line) | | Removed or fixed a mapping | `va edit-manifest` (checks on save) or edit the refs file by hand | **Install options** (clone, shared host, flags): [Install details](#install-details). diff --git a/src/commands.rs b/src/commands.rs index f0c7b38..0855aa4 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1199,17 +1199,27 @@ fn refresh_bitwarden( println!("Wrote refs file (replace): {}", path.display()); } _ => { - let added = refs::write_refs_merge( + let merge = refs::write_refs_merge( &path, &secrets, indices.as_deref(), "vaulted-agent refresh", )?; - if added == 0 { - println!("No new mappings to add: {}", path.display()); + if merge.recovered > 0 { + println!( + "Split {} mapping(s) that were glued onto one line (va 0.3.0 refresh): {}", + merge.recovered, + path.display() + ); + } + if merge.added == 0 { + if merge.recovered == 0 { + println!("No new mappings to add: {}", path.display()); + } } else { println!( - "Updated refs file (+{added} mapping(s)): {}", + "Updated refs file (+{} mapping(s)): {}", + merge.added, path.display() ); } @@ -1936,8 +1946,14 @@ fn setup_bitwarden(paths: &Paths, mode: AuthMode, set_token: bool) -> Result<()> let man_path = default_bitwarden_manifest(paths)?; fs::create_dir_all(&paths.manifest_dir).ok(); if man_path.is_file() { - let added = refs::write_refs_merge(&man_path, &secrets, None, "vaulted-agent setup")?; - println!("Merged into {} (+{added})", man_path.display()); + let merge = refs::write_refs_merge(&man_path, &secrets, None, "vaulted-agent setup")?; + if merge.recovered > 0 { + println!( + "Split {} mapping(s) that were glued onto one line (va 0.3.0 refresh)", + merge.recovered + ); + } + println!("Merged into {} (+{})", man_path.display(), merge.added); } else { refs::write_refs_replace(&man_path, &secrets, None, "vaulted-agent setup")?; println!("Wrote {}", man_path.display()); diff --git a/src/refs.rs b/src/refs.rs index 8cac219..e46dc0e 100644 --- a/src/refs.rs +++ b/src/refs.rs @@ -202,12 +202,95 @@ pub fn write_refs_replace( Ok(()) } +/// How many mappings `write_refs_merge` added, and how many it recovered from +/// a 0.3.0 glued line. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct RefsMerge { + pub added: usize, + pub recovered: usize, +} + +/// Recover the one-mapping-per-line form of a 0.3.0 glued Bitwarden refs line. +/// +/// `va refresh` on the bash launcher captured each `VAR=name:KEY\n` with +/// `$(…)`, which strips the trailing newline, then concatenated. When the SM +/// secret key was already env-shaped, VAR equals KEY and the file held: +/// +/// ```text +/// META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEY +/// ``` +/// +/// KEY and the next VAR share the `[A-Z0-9_]` charset, so there is no +/// delimiter between them. The writer always emitted `VAR=name:VAR` in this +/// case, and that identity is what makes the split unambiguous. +/// +/// Returns `None` when the line is a single mapping (or not this shape). +pub fn split_glued_bitwarden_line(line: &str) -> Option> { + let mut rest = line.trim(); + if rest.is_empty() || rest.starts_with('#') { + return None; + } + let mut parts = Vec::new(); + while !rest.is_empty() { + let (var, after_eq) = rest.split_once('=')?; + if !crate::validate::validate_var_name(var) { + return None; + } + let after_form = after_eq.strip_prefix("name:")?; + if !after_form.starts_with(var) { + return None; + } + let after_key = &after_form[var.len()..]; + if !after_key.is_empty() && !starts_with_name_assignment(after_key) { + return None; + } + parts.push(format!("{var}=name:{var}")); + rest = after_key; + } + (parts.len() >= 2).then_some(parts) +} + +fn starts_with_name_assignment(s: &str) -> bool { + let Some((var, after_eq)) = s.split_once('=') else { + return false; + }; + crate::validate::validate_var_name(var) && after_eq.starts_with("name:") +} + +/// Split every glued mapping in a refs file. Unchanged bytes when there is +/// nothing to split, so a healthy merge stays byte-identical. +pub fn split_glued_bitwarden_text(text: &str) -> (String, usize) { + let mut recovered = 0usize; + let mut out = String::new(); + let mut changed = false; + for (i, line) in text.lines().enumerate() { + if i > 0 { + out.push('\n'); + } + if let Some(parts) = split_glued_bitwarden_line(line) { + recovered += parts.len(); + changed = true; + out.push_str(&parts.join("\n")); + } else { + out.push_str(line); + } + } + if text.ends_with('\n') && !out.ends_with('\n') { + out.push('\n'); + } + if changed { + (out, recovered) + } else { + (text.to_string(), 0) + } +} + pub fn write_refs_merge( path: &Path, secrets: &[(String, String, String)], indices: Option<&[usize]>, source: &str, -) -> Result { +) -> Result { let existing = if path.is_file() { fs::read_to_string(path).map_err(|e| Error::Io { path: path.to_path_buf(), @@ -216,6 +299,7 @@ pub fn write_refs_merge( } else { String::new() }; + let (existing, recovered) = split_glued_bitwarden_text(&existing); let mut new_lines = String::new(); let mut added = 0usize; // Track VARs newly claimed this pass (existing checked via text_has_var). @@ -239,10 +323,12 @@ pub fn write_refs_merge( new_lines.push_str(&line); added += 1; } - if added == 0 { - return Ok(0); + if added == 0 && recovered == 0 { + return Ok(RefsMerge::default()); } - let body = if existing.is_empty() { + let body = if added == 0 { + existing + } else if existing.is_empty() { format!( "# Bitwarden Secrets Manager refs (no secret values). Generated by {source}.\n\n{new_lines}" ) @@ -263,7 +349,7 @@ pub fn write_refs_merge( path: path.to_path_buf(), source: e, })?; - Ok(added) + Ok(RefsMerge { added, recovered }) } /// The separator `write_refs_merge` puts above the mappings it appends. @@ -1760,13 +1846,16 @@ mod tests { "OPENAI_API_KEY".to_string(), String::new(), )]; - assert_eq!(write_refs_merge(&p, &first, None, "test").unwrap(), 1); + assert_eq!(write_refs_merge(&p, &first, None, "test").unwrap().added, 1); let second = vec![( "id2".to_string(), "META_AI_API_KEY".to_string(), String::new(), )]; - assert_eq!(write_refs_merge(&p, &second, None, "test").unwrap(), 1); + assert_eq!( + write_refs_merge(&p, &second, None, "test").unwrap().added, + 1 + ); let body = fs::read_to_string(&p).unwrap(); assert_eq!( @@ -1774,12 +1863,22 @@ mod tests { 1, "{body}" ); - assert!( - body.contains("OPENAI_API_KEY=name:OPENAI_API_KEY"), - "{body}" - ); - assert!( - body.contains("META_AI_API_KEY=name:META_AI_API_KEY"), + // `contains` would pass if both mappings were glued onto one line, which + // is the 0.3.0 bash refresh bug. Each mapping has to be its own line. + let mappings: Vec<&str> = body + .lines() + .filter(|l| { + let t = l.trim(); + !t.is_empty() && !t.starts_with('#') && t.contains('=') + }) + .collect(); + assert_eq!( + mappings, + vec![ + "OPERATOR_PINNED=name:PINNED", + "OPENAI_API_KEY=name:OPENAI_API_KEY", + "META_AI_API_KEY=name:META_AI_API_KEY", + ], "{body}" ); // The operator's line is still above the separator, untouched. @@ -1799,7 +1898,10 @@ mod tests { ) .unwrap(); let secrets = vec![("id".to_string(), "NEW_KEY".to_string(), String::new())]; - assert_eq!(write_refs_merge(&p, &secrets, None, "test").unwrap(), 1); + assert_eq!( + write_refs_merge(&p, &secrets, None, "test").unwrap().added, + 1 + ); let body = fs::read_to_string(&p).unwrap(); assert_eq!( body.matches("# --- appended by test ---").count(), @@ -1877,7 +1979,74 @@ mod tests { "tools".to_string(), )]; let added = write_refs_merge(&p, &secrets, None, "t").unwrap(); - assert_eq!(added, 0, "{}", fs::read_to_string(&p).unwrap()); + assert_eq!(added.added, 0, "{}", fs::read_to_string(&p).unwrap()); + } + + #[test] + fn split_glued_line_recovers_the_0_3_0_refresh_blob() { + // The exact shape that landed on disk: one physical line, 13 mappings. + let line = "META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEYELEVENLABS_API_KEY=name:ELEVENLABS_API_KEYMUREKA_API_KEY=name:MUREKA_API_KEYGEMINI_API_KEY=name:GEMINI_API_KEYASSEMBLY_AI_API_KEY=name:ASSEMBLY_AI_API_KEYANTHROPIC_API_KEY=name:ANTHROPIC_API_KEYBASETEN_API_KEY=name:BASETEN_API_KEYTOGETHER_AI_API_KEY=name:TOGETHER_AI_API_KEYDEEPINFRA_API_KEY=name:DEEPINFRA_API_KEYGROQ_API_KEY=name:GROQ_API_KEYHUME_API_KEY=name:HUME_API_KEYHUME_SECRET_KEY=name:HUME_SECRET_KEY"; + let parts = split_glued_bitwarden_line(line).expect("glued"); + assert_eq!( + parts, + vec![ + "META_AI_API_KEY=name:META_AI_API_KEY", + "FIREWORKS_API_KEY=name:FIREWORKS_API_KEY", + "ELEVENLABS_API_KEY=name:ELEVENLABS_API_KEY", + "MUREKA_API_KEY=name:MUREKA_API_KEY", + "GEMINI_API_KEY=name:GEMINI_API_KEY", + "ASSEMBLY_AI_API_KEY=name:ASSEMBLY_AI_API_KEY", + "ANTHROPIC_API_KEY=name:ANTHROPIC_API_KEY", + "BASETEN_API_KEY=name:BASETEN_API_KEY", + "TOGETHER_AI_API_KEY=name:TOGETHER_AI_API_KEY", + "DEEPINFRA_API_KEY=name:DEEPINFRA_API_KEY", + "GROQ_API_KEY=name:GROQ_API_KEY", + "HUME_API_KEY=name:HUME_API_KEY", + "HUME_SECRET_KEY=name:HUME_SECRET_KEY", + ] + ); + assert!(split_glued_bitwarden_line("OPENAI_API_KEY=name:OPENAI_API_KEY").is_none()); + } + + #[test] + fn merge_splits_a_glued_line_even_when_every_secret_looks_already_mapped() { + // Substring search on the glued line matches `name:META_AI_API_KEY` + // inside the blob, so merge used to report "nothing to add" and leave + // the file broken. Repair has to run first, and has to write even + // when added == 0. + let dir = tempfile::tempdir().unwrap(); + let p = dir.path().join("bws.refs"); + fs::write( + &p, + "OPENAI_API_KEY=name:OPENAI_API_KEY\n\ + META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEY\n", + ) + .unwrap(); + let secrets = vec![ + ("id-o".into(), "OPENAI_API_KEY".into(), String::new()), + ("id-m".into(), "META_AI_API_KEY".into(), String::new()), + ("id-f".into(), "FIREWORKS_API_KEY".into(), String::new()), + ]; + let out = write_refs_merge(&p, &secrets, None, "test").unwrap(); + assert_eq!(out.added, 0, "already mapped once split"); + assert_eq!(out.recovered, 2, "{out:?}"); + let body = fs::read_to_string(&p).unwrap(); + let mappings: Vec<&str> = body + .lines() + .filter(|l| { + let t = l.trim(); + !t.is_empty() && !t.starts_with('#') && t.contains('=') + }) + .collect(); + assert_eq!( + mappings, + vec![ + "OPENAI_API_KEY=name:OPENAI_API_KEY", + "META_AI_API_KEY=name:META_AI_API_KEY", + "FIREWORKS_API_KEY=name:FIREWORKS_API_KEY", + ], + "{body}" + ); } #[test] diff --git a/src/validate.rs b/src/validate.rs index c9c720d..abd7cc6 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -83,6 +83,29 @@ pub fn validate_bitwarden_ref(var: &str, r: &str) -> Result<()> { if r.is_empty() { return Err(Error::Message(format!("empty reference for {var}"))); } + // None of the four Bitwarden reference forms contain `=`. A second + // `VAR=name:KEY` glued onto this one is the bash 0.3.0 refresh merge + // (command substitution strips the trailing newline). Fail closed with + // the recovered lines rather than sending the blob to the vault. + if r.contains('=') { + let glued = format!("{var}={r}"); + if let Some(parts) = crate::refs::split_glued_bitwarden_line(&glued) { + let listed = parts + .iter() + .map(|p| format!(" {p}")) + .collect::>() + .join("\n"); + return Err(Error::Message(format!( + "{var} looks like several mappings glued onto one line \ + (va 0.3.0 refresh merge dropped the newlines). \ + Split each onto its own line:\n{listed}\n\ + Or run: vaulted-agent refresh" + ))); + } + return Err(Error::Message(format!( + "{var} bad bitwarden ref {r} (a reference cannot contain '=')" + ))); + } if let Some(rest) = r.strip_prefix("uuid:") { if !is_uuid(rest) { return Err(Error::Message(format!( @@ -431,4 +454,29 @@ GOOD=op://Vault/item/field\n\ let text = "A=name: # uuid:11111111-1111-1111-1111-111111111111\n"; assert!(validate_manifest_text(text, Backend::Bitwarden).is_err()); } + + #[test] + fn a_glued_0_3_0_refresh_line_fails_closed_with_the_recovered_mappings() { + // The launch used to send the whole blob to the vault and report + // `no secret matched 'name:META_AI_API_KEYFIREWORKS_API_KEY=name:…'`. + // Shape is validate's job (CONTEXT.md: malformed ref). + let text = "META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEYELEVENLABS_API_KEY=name:ELEVENLABS_API_KEY\n"; + let err = validate_manifest_text(text, Backend::Bitwarden) + .unwrap_err() + .to_string(); + assert!(err.contains("glued onto one line"), "{err}"); + assert!( + err.contains("META_AI_API_KEY=name:META_AI_API_KEY"), + "{err}" + ); + assert!( + err.contains("FIREWORKS_API_KEY=name:FIREWORKS_API_KEY"), + "{err}" + ); + assert!( + err.contains("ELEVENLABS_API_KEY=name:ELEVENLABS_API_KEY"), + "{err}" + ); + assert!(err.contains("vaulted-agent refresh"), "{err}"); + } } diff --git a/tests/cli_refresh_glued.rs b/tests/cli_refresh_glued.rs new file mode 100644 index 0000000..75cfeea --- /dev/null +++ b/tests/cli_refresh_glued.rs @@ -0,0 +1,67 @@ +//! `va refresh` splits Bitwarden refs that bash 0.3.0 glued onto one line. + +mod common; + +use common::CliSeam; +use std::fs; + +#[test] +fn refresh_splits_glued_name_refs_even_when_every_secret_is_already_named() { + // The substring `name:META_AI_API_KEY` sits inside the glued blob, so + // merge used to treat every secret as mapped and write nothing. + let seam = CliSeam::new(); + let map = seam.write_secrets_json( + "vault.json", + r#"{"OPENAI_API_KEY": "a", "META_AI_API_KEY": "b", "FIREWORKS_API_KEY": "c"}"#, + ); + seam.install_fake_bws(&map); + fs::write( + seam.config_dir.join("manifests/bws.refs"), + "OPENAI_API_KEY=name:OPENAI_API_KEY\n\ + META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEY\n", + ) + .unwrap(); + fs::write( + seam.config_dir.join("harnesses.d/grok.conf"), + "backend = bitwarden\nmanifest = bws.refs\ncommand = true\n", + ) + .unwrap(); + fs::write( + seam.config_dir.join("bws.env"), + "BWS_ACCESS_TOKEN=test-token\n", + ) + .unwrap(); + + let out = seam + .vaulted_agent() + .args(["refresh", "--all"]) + .env("VAULTED_AGENT_AUTH_MODE", "file") + .env("VAULTED_AGENT_NO_REEXEC", "1") + .output() + .expect("run refresh"); + let text = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + assert!(out.status.success(), "{text}"); + assert!(text.contains("glued onto one line"), "{text}"); + + let body = fs::read_to_string(seam.config_dir.join("manifests/bws.refs")).unwrap(); + let mappings: Vec<&str> = body + .lines() + .filter(|l| { + let t = l.trim(); + !t.is_empty() && !t.starts_with('#') && t.contains('=') + }) + .collect(); + assert_eq!( + mappings, + vec![ + "OPENAI_API_KEY=name:OPENAI_API_KEY", + "META_AI_API_KEY=name:META_AI_API_KEY", + "FIREWORKS_API_KEY=name:FIREWORKS_API_KEY", + ], + "{body}" + ); +} diff --git a/tests/cli_validate_auth_workdir.rs b/tests/cli_validate_auth_workdir.rs index 4528f53..2b7fc76 100644 --- a/tests/cli_validate_auth_workdir.rs +++ b/tests/cli_validate_auth_workdir.rs @@ -98,6 +98,39 @@ fn secrets_validate_accepts_name_ref() { ); } +#[test] +fn secrets_validate_offline_rejects_glued_name_refs() { + // Bash 0.3.0 refresh glued VAR=name:KEY lines together. Offline validate + // must fail closed on the shape, not send the blob to the vault. + let seam = CliSeam::new(); + fs::write( + seam.config_dir.join("manifests/glued.env.refs"), + "META_AI_API_KEY=name:META_AI_API_KEYFIREWORKS_API_KEY=name:FIREWORKS_API_KEY\n", + ) + .unwrap(); + write_plain_harness( + &seam, + "grok", + "backend = bitwarden\nmanifest = glued.env.refs\ncommand = true\n", + ); + let out = seam + .vaulted_agent() + .args(["secrets", "validate", "grok", "--offline"]) + .output() + .expect("run"); + assert!(!out.status.success()); + let err = format!( + "{}{}", + String::from_utf8_lossy(&out.stderr), + String::from_utf8_lossy(&out.stdout) + ); + assert!(err.contains("glued onto one line"), "{err}"); + assert!( + err.contains("FIREWORKS_API_KEY=name:FIREWORKS_API_KEY"), + "{err}" + ); +} + #[test] fn secrets_validate_without_a_token_fails_rather_than_passing_blind() { // The behaviour change that matters: a gate that cannot reach the vault