From 695fe57145aecd4fc42a50251730aff748256659 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 6 Sep 2026 00:38:32 +0000 Subject: [PATCH] Fail closed on glued Bitwarden refs from bash 0.3.0 refresh va refresh on the bash launcher captured each VAR=name:KEY line with $(...), which strips the trailing newline, then concatenated. Launch sent the blob to the vault and reported no secret matched the glued name. validate now rejects that shape and prints the recovered one-mapping-per-line form. refresh splits those lines even when every secret already appears as a substring, so a second merge is not a no-op. --- AGENTS.md | 2 +- MIGRATION.md | 21 +++ README.md | 1 + src/commands.rs | 28 +++- src/refs.rs | 199 ++++++++++++++++++++++++++--- src/validate.rs | 48 +++++++ tests/cli_refresh_glued.rs | 67 ++++++++++ tests/cli_validate_auth_workdir.rs | 33 +++++ 8 files changed, 377 insertions(+), 22 deletions(-) create mode 100644 tests/cli_refresh_glued.rs 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