From 594ed342abe482ae5541ff709e3ac9d04ceafd7e Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Wed, 7 Oct 2026 09:48:02 +0400 Subject: [PATCH] feat: point config errors to the line of the invalid entry A half-deleted secret, e.g. a `- kind: env` line left without `name` and `path`, reported only `secret at profile 'p' index 3 missing 'name' field`: a 0-based index, no line, and only the first missing field. Config errors for profiles and secrets missing fields now give the line, the 1-based position or the name, the fields that are there, and every missing field: secret #4 in profile 'personal' (line 17, kind: env) is missing 'name' and 'path' serde_yaml_ng keeps no positions, so on such an error the YAML is read again up to the entry, and an error raised there on purpose carries its line. Closes #35 --- src/vault/config.rs | 235 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 208 insertions(+), 27 deletions(-) diff --git a/src/vault/config.rs b/src/vault/config.rs index 3a4373d..5973ccf 100644 --- a/src/vault/config.rs +++ b/src/vault/config.rs @@ -84,7 +84,7 @@ impl Config { /// Parses and validates configuration content. pub fn parse(data: &str) -> Result { let spec: Spec = serde_yaml_ng::from_str(data)?; - Self::try_from(spec) + Self::from_spec(spec, data) } /// Returns the default profile: the one marked `default: true`, or else the first one. @@ -119,10 +119,9 @@ impl Config { } } -impl TryFrom for Config { - type Error = anyhow::Error; - - fn try_from(spec: Spec) -> Result { +impl Config { + /// Validates `spec`, parsed from `data`, which is used to point errors to their line. + fn from_spec(spec: Spec, data: &str) -> Result { let version = match spec.version { Some(serde_yaml_ng::Value::Number(n)) => n.to_string(), Some(serde_yaml_ng::Value::String(s)) => s, @@ -140,10 +139,19 @@ impl TryFrom for Config { let mut profiles = Vec::new(); for (i, profile) in spec.profiles.into_iter().enumerate() { - let name = profile - .name - .filter(|s| !s.is_empty()) - .ok_or_else(|| anyhow!("profile at index {i} missing 'name' field"))?; + let line = || line_of(data, &[Step::Key("profiles"), Step::Index(i)]); + let Some(name) = profile.name.filter(|s| !s.is_empty()) else { + let mut missing = vec!["name"]; + if profile.provider.is_none() { + missing.push("provider"); + } + bail!( + "profile #{}{} is missing {}", + i + 1, + details(line(), &[]), + fields(&missing) + ); + }; if !is_profile_name(&name) { bail!("profile name '{name}' may only contain letters, digits, '.', '_' and '-'"); } @@ -153,19 +161,55 @@ impl TryFrom for Config { if profile.account.is_some() { bail!("profile '{name}' has 'account': move it into its provider (provider: {{ type: 1password, account: ... }})"); } - let provider: Provider = profile - .provider - .ok_or_else(|| anyhow!("profile '{name}' missing 'provider' field"))? - .into(); + let Some(provider) = profile.provider else { + bail!( + "profile '{name}'{} is missing 'provider'", + details(line(), &[]) + ); + }; + let provider: Provider = provider.into(); let mut secrets = Vec::new(); for (j, secret) in profile.secrets.unwrap_or_default().into_iter().enumerate() { - let kind = secret.kind.filter(|s| !s.is_empty()).ok_or_else(|| { - anyhow!("secret at profile '{name}' index {j} missing 'kind' field") - })?; - let secret_name = secret.name.filter(|s| !s.is_empty()).ok_or_else(|| { - anyhow!("secret at profile '{name}' index {j} missing 'name' field") - })?; + let fields_of = ( + secret.kind.filter(|s| !s.is_empty()), + secret.name.filter(|s| !s.is_empty()), + secret.path.filter(|s| !s.is_empty()), + ); + let (kind, secret_name, path) = match fields_of { + (Some(kind), Some(secret_name), Some(path)) => (kind, secret_name, path), + (kind, secret_name, path) => { + let missing: Vec<&str> = + [("kind", &kind), ("name", &secret_name), ("path", &path)] + .into_iter() + .filter(|(_, value)| value.is_none()) + .map(|(field, _)| field) + .collect(); + // Name the secret if it has a name, else say where it is and what it has + let label = match &secret_name { + Some(secret_name) => format!("secret '{secret_name}'"), + None => format!("secret #{}", j + 1), + }; + let mut present = Vec::new(); + if let Some(kind) = &kind { + present.push(format!("kind: {kind}")); + } + if let (None, Some(path)) = (&secret_name, &path) { + present.push(format!("path: {path}")); + } + let path = [ + Step::Key("profiles"), + Step::Index(i), + Step::Key("secrets"), + Step::Index(j), + ]; + bail!( + "{label} in profile '{name}'{} is missing {}", + details(line_of(data, &path), &present), + fields(&missing) + ); + } + }; let kind = kind.parse::().map_err(|_| { anyhow!( "secret '{secret_name}' has invalid kind: {kind} (valid kinds: env, ssh, file)" @@ -178,9 +222,6 @@ impl TryFrom for Config { "{kind} secret '{secret_name}' must use a valid environment variable name" ); } - let path = secret.path.filter(|s| !s.is_empty()).ok_or_else(|| { - anyhow!("secret '{secret_name}' in profile '{name}' missing 'path' field") - })?; let scheme = provider.scheme(); if !path.starts_with(scheme) { bail!("secret '{secret_name}' has invalid path: {path} (path must start with '{scheme}' for provider {provider})"); @@ -237,6 +278,100 @@ fn is_variable_name(name: &str) -> bool { && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') } +/// Returns the parenthesized `line` and `present` fields of an invalid entry, or nothing if +/// there are none. +fn details(line: Option, present: &[String]) -> String { + let details: Vec = line + .map(|line| format!("line {line}")) + .into_iter() + .chain(present.iter().cloned()) + .collect(); + match details.is_empty() { + true => String::new(), + false => format!(" ({})", details.join(", ")), + } +} + +/// Returns the quoted field `names`, e.g. `'name' and 'path'`. +fn fields(names: &[&str]) -> String { + let quoted: Vec = names.iter().map(|name| format!("'{name}'")).collect(); + match quoted.split_last() { + Some((last, rest)) if !rest.is_empty() => format!("{} and {last}", rest.join(", ")), + _ => quoted.join(""), + } +} + +/// A step of the path to a node in the config: a mapping key or a sequence index. +enum Step<'a> { + Key(&'a str), + Index(usize), +} + +/// Returns the line of the node at `path` in the YAML `data`, if there is one. +/// +/// serde_yaml_ng doesn't keep the position of what it parses, but it does stamp errors with +/// the position of the mapping or sequence they are raised in. So `data` is read again and an +/// error is raised on purpose once the node is reached. +fn line_of(data: &str, path: &[Step]) -> Option { + use serde::de::DeserializeSeed; + + let deserializer = serde_yaml_ng::Deserializer::from_str(data); + match Locate(path).deserialize(deserializer) { + Ok(()) => None, + Err(err) => err.location().map(|location| location.line()), + } +} + +/// Walks the rest of a [`Step`] path and fails at its end. +struct Locate<'a, 'p>(&'p [Step<'a>]); + +impl<'de> serde::de::DeserializeSeed<'de> for Locate<'_, '_> { + type Value = (); + + fn deserialize>(self, deserializer: D) -> Result<(), D::Error> { + match self.0.first() { + Some(Step::Index(_)) => deserializer.deserialize_seq(self), + _ => deserializer.deserialize_map(self), + } + } +} + +impl<'de> serde::de::Visitor<'de> for Locate<'_, '_> { + type Value = (); + + fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "a mapping or a sequence") + } + + fn visit_map>(self, mut map: A) -> Result<(), A::Error> { + let Some((Step::Key(key), rest)) = self.0.split_first() else { + return Err(serde::de::Error::custom("found")); + }; + while let Some(found) = map.next_key::()? { + if found.as_str() == Some(*key) { + map.next_value_seed(Locate(rest))?; + } else { + map.next_value::()?; + } + } + Ok(()) + } + + fn visit_seq>(self, mut seq: A) -> Result<(), A::Error> { + let Some((Step::Index(index), rest)) = self.0.split_first() else { + return Err(serde::de::Error::custom("found")); + }; + for _ in 0..*index { + if seq.next_element::()?.is_none() { + return Ok(()); + } + } + seq.next_element_seed(Locate(rest))?; + while seq.next_element::()?.is_some() {} + Ok(()) + } +} + /// A profile: a provider and the secrets loaded from it. #[derive(Debug, Clone)] pub struct Profile { @@ -443,7 +578,7 @@ mod tests { fn parse_fails_when_account_name_is_missing() { assert_eq!( parse_err("version: 1\nprofiles:\n - account: a.1password.com\n"), - "profile at index 0 missing 'name' field" + "profile #1 (line 3) is missing 'name' and 'provider'" ); } @@ -543,7 +678,7 @@ mod tests { fn parse_fails_when_provider_is_missing() { assert_eq!( parse_err("version: 1\nprofiles:\n - name: p\n"), - "profile 'p' missing 'provider' field" + "profile 'p' (line 3) is missing 'provider'" ); } @@ -552,7 +687,7 @@ mod tests { let data = with_secrets(" - name: A\n path: op://v/i/f\n"); assert_eq!( parse_err(&data), - "secret at profile 'p' index 0 missing 'kind' field" + "secret 'A' in profile 'p' (line 8) is missing 'kind'" ); } @@ -570,7 +705,7 @@ mod tests { let data = with_secrets(" - kind: env\n path: op://v/i/f\n"); assert_eq!( parse_err(&data), - "secret at profile 'p' index 0 missing 'name' field" + "secret #1 in profile 'p' (line 8, kind: env, path: op://v/i/f) is missing 'name'" ); } @@ -579,8 +714,54 @@ mod tests { let data = with_secrets(" - kind: env\n name: A\n"); assert_eq!( parse_err(&data), - "secret 'A' in profile 'p' missing 'path' field" + "secret 'A' in profile 'p' (line 8, kind: env) is missing 'path'" + ); + } + + #[test] + fn parse_points_to_a_half_deleted_secret() { + let data = with_secrets(concat!( + " - kind: env\n name: A\n path: op://v/i/a\n", + " # B was here\n", + " - kind: env\n name: C\n path: op://v/i/c\n", + "\n", + " - kind: env\n", + " - kind: env\n name: D\n path: op://v/i/d\n", + )); + assert_eq!( + parse_err(&data), + "secret #3 in profile 'p' (line 16, kind: env) is missing 'name' and 'path'" + ); + } + + #[test] + fn parse_reports_every_missing_field() { + let data = with_secrets( + " - {} +", + ); + assert_eq!( + parse_err(&data), + "secret #1 in profile 'p' (line 8) is missing 'kind', 'name' and 'path'" + ); + } + + #[test] + fn line_of_finds_nodes_in_any_style() { + let data = "a: 1 +list: [{x: 1}, + {y: 2}] +map: + key: + - 1 +"; + assert_eq!(line_of(data, &[Step::Key("list"), Step::Index(1)]), Some(3)); + assert_eq!( + line_of(data, &[Step::Key("map"), Step::Key("key")]), + Some(6) ); + assert_eq!(line_of(data, &[Step::Key("list"), Step::Index(5)]), None); + assert_eq!(line_of(data, &[Step::Key("missing")]), None); } #[test]