diff --git a/.changes/unreleased/library-Fixed-20260411-160000.yaml b/.changes/unreleased/library-Fixed-20260411-160000.yaml new file mode 100644 index 0000000..58ce034 --- /dev/null +++ b/.changes/unreleased/library-Fixed-20260411-160000.yaml @@ -0,0 +1,7 @@ +component: library +kind: Fixed +body: |- + Validate overlay names reject path separators in release builds + + `OverlayName` previously only checked for forward slashes via `debug_assert`, meaning invalid names with path separators (including backslashes) could slip through in release builds. Added `OverlayName::try_new` which returns an error for names containing `/` or `\`, and updated all user-input paths to use it. +time: 2026-04-11T16:00:00.000000000-07:00 diff --git a/src/overlay_name.rs b/src/overlay_name.rs index 18f1c7b..c47f8fd 100644 --- a/src/overlay_name.rs +++ b/src/overlay_name.rs @@ -4,6 +4,8 @@ use std::fmt; +use anyhow::bail; + /// A normalized overlay name, as stored in `.ccl` file stems. /// /// This newtype prevents accidental comparison between overlay names @@ -14,19 +16,31 @@ use std::fmt; pub(crate) struct OverlayName(String); impl OverlayName { - /// Create a new `OverlayName` from a string. + /// Create a new `OverlayName` from a string that is already known to be valid. /// - /// The name must be a simple overlay name (e.g., `"my-overlay"`), - /// not a path like `"org/repo/name"`. + /// Use this only when the name comes from a trusted source (e.g., file stems + /// from directory listing). For user-provided input, use [`try_new`](Self::try_new). pub(crate) fn new(name: impl Into) -> Self { let name = name.into(); debug_assert!( - !name.contains('/'), + !name.contains('/') && !name.contains('\\'), "OverlayName must not contain path separators: {name}" ); Self(name) } + /// Create a new `OverlayName` from user-provided input, validating that it + /// contains no path separators. + /// + /// Returns an error if the name contains `/` or `\`. + pub(crate) fn try_new(name: impl Into) -> anyhow::Result { + let name = name.into(); + if name.contains('/') || name.contains('\\') { + bail!("Overlay name must not contain path separators: {name}"); + } + Ok(Self(name)) + } + /// Get the underlying string slice. pub(crate) fn as_str(&self) -> &str { &self.0 @@ -110,9 +124,32 @@ mod tests { } #[test] - #[should_panic(expected = "must not contain path separators")] - fn overlay_name_rejects_paths_in_debug() { - let _ = OverlayName::new("org/repo/name"); + fn try_new_rejects_forward_slash() { + let result = OverlayName::try_new("org/repo/name"); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("path separators"), + "expected path separator error, got: {msg}" + ); + } + + #[test] + fn try_new_rejects_backslash() { + let result = OverlayName::try_new(r"org\repo\name"); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!( + msg.contains("path separators"), + "expected path separator error, got: {msg}" + ); + } + + #[test] + fn try_new_accepts_valid_name() { + let result = OverlayName::try_new("my-overlay"); + assert!(result.is_ok()); + assert_eq!(result.unwrap().as_str(), "my-overlay"); } #[test] diff --git a/src/resolve.rs b/src/resolve.rs index d306f6e..bdf714e 100644 --- a/src/resolve.rs +++ b/src/resolve.rs @@ -880,6 +880,7 @@ fn get_fuzzy_suggestions_multi_source( ) -> Vec { let available: Vec = manager .list_overlays_for_repo(org, repo) + .unwrap_or_default() .into_iter() .map(|n| n.to_string()) .collect(); diff --git a/src/sources.rs b/src/sources.rs index 3fb93d9..d243e20 100644 --- a/src/sources.rs +++ b/src/sources.rs @@ -307,8 +307,11 @@ impl SourceManager { /// List overlay names for a specific org/repo across all sources. /// /// Returns unique overlay names (deduplicated across sources). - #[must_use] - pub(crate) fn list_overlays_for_repo(&self, org: &str, repo: &str) -> Vec { + pub(crate) fn list_overlays_for_repo( + &self, + org: &str, + repo: &str, + ) -> anyhow::Result> { let mut names = std::collections::HashSet::new(); for ms in &self.sources { @@ -319,7 +322,7 @@ impl SourceManager { } if let Ok(overlays) = manager.list_overlays_for_repo(org, repo) { for overlay in overlays { - names.insert(OverlayName::new(overlay.name)); + names.insert(OverlayName::try_new(overlay.name)?); } } } @@ -329,7 +332,7 @@ impl SourceManager { if overlay.org.eq_ignore_ascii_case(org) && overlay.repo.eq_ignore_ascii_case(repo) { - names.insert(OverlayName::new(overlay.name)); + names.insert(OverlayName::try_new(overlay.name)?); } } } @@ -339,7 +342,7 @@ impl SourceManager { let mut result: Vec<_> = names.into_iter().collect(); result.sort(); - result + Ok(result) } /// Get the base path for a named source. @@ -1360,7 +1363,9 @@ mod tests { }; // Should find overlays from cloned source, skip uncloned source - let overlays = manager.list_overlays_for_repo("microsoft", "FluidFramework"); + let overlays = manager + .list_overlays_for_repo("microsoft", "FluidFramework") + .unwrap(); assert_eq!(overlays.len(), 2); assert!(overlays.contains(&OverlayName::new("claude-config"))); assert!(overlays.contains(&OverlayName::new("vscode-settings"))); @@ -1419,7 +1424,9 @@ mod tests { }; // Should deduplicate across sources - let overlays = manager.list_overlays_for_repo("microsoft", "FluidFramework"); + let overlays = manager + .list_overlays_for_repo("microsoft", "FluidFramework") + .unwrap(); assert_eq!(overlays.len(), 1); assert_eq!(overlays[0], "claude-config"); } @@ -1462,7 +1469,9 @@ mod tests { }; // Different repo should return empty - let overlays = manager.list_overlays_for_repo("google", "chromium"); + let overlays = manager + .list_overlays_for_repo("google", "chromium") + .unwrap(); assert!(overlays.is_empty()); } @@ -2059,7 +2068,9 @@ mod tests { let manager = SourceManager::new(sources, Some(repo_root)).unwrap(); - let overlays = manager.list_overlays_for_repo("microsoft", "FluidFramework"); + let overlays = manager + .list_overlays_for_repo("microsoft", "FluidFramework") + .unwrap(); assert_eq!(overlays.len(), 2); assert!(overlays.contains(&OverlayName::new("claude-config"))); assert!(overlays.contains(&OverlayName::new("vscode-settings")));