From ff980ea956575476076e7c1dd8c8b6bc4f34ed80 Mon Sep 17 00:00:00 2001 From: Alpha Chen Date: Sat, 25 Apr 2026 20:22:17 -0700 Subject: [PATCH] Scope SecretString cache to the file variant The OnceLock was vestigial on Plain secrets. Bundles two related fixes: strip_suffix preserves all but one trailing newline (Docker convention), and from_plain/from_file are now public. Assisted-by: Claude Opus 4.7 via Claude Code --- src/secret.rs | 123 +++++++++++++++++++++++++++++--------------------- 1 file changed, 72 insertions(+), 51 deletions(-) diff --git a/src/secret.rs b/src/secret.rs index f3d9f5c..e439673 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -12,26 +12,30 @@ use std::sync::OnceLock; /// from file contents (Docker secrets convention). /// /// The [`std::fmt::Debug`] impl redacts the value. -pub struct SecretString { - source: SecretSource, - resolved: OnceLock>, +#[derive(Clone)] +pub struct SecretString(SecretSource); + +enum SecretSource { + Plain(String), + File { + path: PathBuf, + resolved: OnceLock>, + }, } -impl Clone for SecretString { +impl Clone for SecretSource { fn clone(&self) -> Self { - Self { - source: self.source.clone(), - resolved: OnceLock::new(), + match self { + Self::Plain(s) => Self::Plain(s.clone()), + // File clones get a fresh OnceLock — they re-read from disk on next reveal. + Self::File { path, .. } => Self::File { + path: path.clone(), + resolved: OnceLock::new(), + }, } } } -#[derive(Clone)] -enum SecretSource { - Plain(String), - File(PathBuf), -} - impl SecretString { /// The resolved secret value. /// @@ -39,16 +43,18 @@ impl SecretString { /// result. Returns a typed error if the file is missing or unreadable. /// Errors are also cached — subsequent calls return the same error. pub fn reveal(&self) -> crate::Result<&str> { - self.resolved - .get_or_init(|| match &self.source { - SecretSource::Plain(s) => Ok(s.clone()), - SecretSource::File(path) => fs_err::read_to_string(path) - .map(|s| s.trim_end_matches('\n').to_string()) - .map_err(|e| format!("{}: {e}", path.display())), - }) - .as_ref() - .map(|s| s.as_str()) - .map_err(|msg| crate::Error::SecretResolve(msg.clone())) + match &self.0 { + SecretSource::Plain(s) => Ok(s.as_str()), + SecretSource::File { path, resolved } => resolved + .get_or_init(|| { + fs_err::read_to_string(path) + .map(|s| s.strip_suffix('\n').unwrap_or(&s).to_string()) + .map_err(|e| format!("{}: {e}", path.display())) + }) + .as_ref() + .map(|s| s.as_str()) + .map_err(|msg| crate::Error::SecretResolve(msg.clone())), + } } } @@ -58,6 +64,23 @@ impl std::fmt::Debug for SecretString { } } +impl SecretString { + /// Build from a plain string literal. + pub fn from_plain(value: impl Into) -> Self { + Self(SecretSource::Plain(value.into())) + } + + /// Build from a file path. Contents are read lazily on first [`reveal`]. + /// + /// [`reveal`]: SecretString::reveal + pub fn from_file(path: impl Into) -> Self { + Self(SecretSource::File { + path: path.into(), + resolved: OnceLock::new(), + }) + } +} + impl<'de> serde::Deserialize<'de> for SecretString { fn deserialize(deserializer: D) -> Result where @@ -73,38 +96,20 @@ impl<'de> serde::Deserialize<'de> for SecretString { let raw = Raw::deserialize(deserializer)?; let source = match raw { Raw::Plain(s) => SecretSource::Plain(s), - Raw::File { file } => SecretSource::File(file), + Raw::File { file } => SecretSource::File { + path: file, + resolved: OnceLock::new(), + }, }; - Ok(Self { - source, - resolved: OnceLock::new(), - }) + Ok(Self(source)) } } #[cfg(test)] mod tests { - use std::path::Path; - use super::*; - impl SecretString { - fn from_plain(value: &str) -> Self { - Self { - source: SecretSource::Plain(value.to_string()), - resolved: OnceLock::new(), - } - } - - fn from_file(path: &Path) -> Self { - Self { - source: SecretSource::File(path.to_path_buf()), - resolved: OnceLock::new(), - } - } - } - #[test] fn debug_redacts_value() { let secret = SecretString::from_plain("super_secret_password"); @@ -146,6 +151,18 @@ mod tests { assert_eq!(secret.reveal().unwrap(), "line1\nline2"); } + #[test] + fn reveal_strips_only_one_trailing_newline() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("secret"); + // Docker secrets convention: strip exactly one trailing newline. + // Any additional trailing newlines are part of the secret. + fs_err::write(&path, "value\n\n\n").expect("write"); + + let secret = SecretString::from_file(&path); + assert_eq!(secret.reveal().unwrap(), "value\n\n"); + } + #[test] fn reveal_errors_on_missing_file() { let secret = SecretString::from_file(PathBuf::from("/no/such/file/ever").as_path()); @@ -157,17 +174,21 @@ mod tests { } #[test] - fn clone_resolves_independently() { + fn clone_resets_cache_and_rereads_from_disk() { let dir = tempfile::tempdir().expect("tempdir"); let path = dir.path().join("pw"); - fs_err::write(&path, "secret\n").expect("write"); + fs_err::write(&path, "initial\n").expect("write"); let original = SecretString::from_file(&path); - assert_eq!(original.reveal().unwrap(), "secret"); + assert_eq!(original.reveal().unwrap(), "initial"); - // Clone gets a fresh OnceLock — it re-reads from disk. + // Overwrite after the original cached "initial". The clone gets a fresh + // OnceLock, so it re-reads the current file contents. + fs_err::write(&path, "changed\n").expect("overwrite"); let cloned = original.clone(); - assert_eq!(cloned.reveal().unwrap(), "secret"); + assert_eq!(cloned.reveal().unwrap(), "changed"); + // Original's cache is untouched. + assert_eq!(original.reveal().unwrap(), "initial"); } #[test] -- 2.54.0