fix(bundles): harden the install pipeline for cross-platform correctness
Windows review findings on the bundle provenance code: - clones now pin core.autocrlf=false and core.eol=lf so recorded sha256 values reflect repository bytes, not the machine's git config (autocrlf on Windows previously made every text file a false conflict on update), plus core.longpaths=true for deep bundle trees - is_safe_relative_path additionally rejects NTFS alternate data stream colons, reserved device names (con, nul, COM1..), and trailing dots or spaces; such names never come from a valid checkout and previously desynced or failed on Windows - file ownership dedupe compares paths case-insensitively on Windows and macOS where case variants denote one physical file (uninstalling one bundle could previously delete another bundle's file) - a failed git clone no longer leaks its partial tree in the temp dir, and temp cleanup failures are logged instead of swallowed - recording a bundle file outside the config dir (asset dir override) now warns instead of silently producing an undeletable record
This commit is contained in:
+66
-2
@@ -442,7 +442,9 @@ impl BundleStore {
|
||||
self.ensure_bundle_exists(bundle)?;
|
||||
for (name, record) in self.bundles.iter_mut() {
|
||||
if name != bundle {
|
||||
record.files.retain(|owned| owned.path != file.path);
|
||||
record
|
||||
.files
|
||||
.retain(|owned| !same_installed_path(&owned.path, &file.path));
|
||||
}
|
||||
}
|
||||
let record = self
|
||||
@@ -450,7 +452,9 @@ impl BundleStore {
|
||||
.get_mut(bundle)
|
||||
.expect("bundle existence checked above");
|
||||
|
||||
record.files.retain(|owned| owned.path != file.path);
|
||||
record
|
||||
.files
|
||||
.retain(|owned| !same_installed_path(&owned.path, &file.path));
|
||||
record.files.push(file);
|
||||
|
||||
self.save()
|
||||
@@ -577,6 +581,17 @@ pub(crate) struct BundleListRow {
|
||||
pub(crate) drift: DriftSummary,
|
||||
}
|
||||
|
||||
/// NTFS and default APFS resolve file names case-insensitively, so records
|
||||
/// differing only in case denote the same physical file there. Linux keeps
|
||||
/// exact matching because case variants are genuinely distinct files.
|
||||
fn same_installed_path(a: &str, b: &str) -> bool {
|
||||
if cfg!(any(windows, target_os = "macos")) {
|
||||
a.eq_ignore_ascii_case(b)
|
||||
} else {
|
||||
a == b
|
||||
}
|
||||
}
|
||||
|
||||
/// An unreadable file counts as locally modified: it exists but its integrity
|
||||
/// cannot be verified.
|
||||
pub(crate) fn bundle_list_rows(store: &BundleStore, config_dir: &Path) -> Vec<BundleListRow> {
|
||||
@@ -1489,4 +1504,53 @@ mod tests {
|
||||
|
||||
assert!(rows.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn same_installed_path_matches_filesystem_case_semantics() {
|
||||
assert!(same_installed_path("macros/a.yaml", "macros/a.yaml"));
|
||||
assert!(!same_installed_path("macros/a.yaml", "macros/b.yaml"));
|
||||
assert_eq!(
|
||||
same_installed_path("macros/Foo.yaml", "macros/foo.yaml"),
|
||||
cfg!(any(windows, target_os = "macos"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn record_file_transfers_case_variant_ownership_on_case_insensitive_hosts() {
|
||||
let dir = TempStoreDir::new("bundles-case-variant");
|
||||
let mut store = dir.store();
|
||||
store
|
||||
.upsert_bundle("alpha", metadata("https://x/a", "aaa"))
|
||||
.unwrap();
|
||||
store
|
||||
.upsert_bundle("beta", metadata("https://x/b", "bbb"))
|
||||
.unwrap();
|
||||
store
|
||||
.record_file("alpha", file_record("macros/Shared.yaml", "one"))
|
||||
.unwrap();
|
||||
|
||||
store
|
||||
.record_file("beta", file_record("macros/shared.yaml", "two"))
|
||||
.unwrap();
|
||||
|
||||
let alpha_still_owns = store
|
||||
.get("alpha")
|
||||
.unwrap()
|
||||
.files
|
||||
.iter()
|
||||
.any(|f| f.path == "macros/Shared.yaml");
|
||||
assert_eq!(
|
||||
alpha_still_owns,
|
||||
!cfg!(any(windows, target_os = "macos")),
|
||||
"case-variant paths are one physical file on case-insensitive hosts"
|
||||
);
|
||||
assert!(
|
||||
store
|
||||
.get("beta")
|
||||
.unwrap()
|
||||
.files
|
||||
.iter()
|
||||
.any(|f| f.path == "macros/shared.yaml")
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
+125
-21
@@ -752,9 +752,29 @@ fn select_uninstall_candidate(store: &BundleStore, spec: &str) -> Result<Option<
|
||||
fn is_safe_relative_path(path: &str) -> bool {
|
||||
let recorded = Path::new(path);
|
||||
!recorded.is_absolute()
|
||||
&& recorded
|
||||
.components()
|
||||
.all(|c| matches!(c, Component::Normal(_)))
|
||||
&& recorded.components().all(|c| match c {
|
||||
Component::Normal(name) => is_safe_component(&name.to_string_lossy()),
|
||||
_ => false,
|
||||
})
|
||||
}
|
||||
|
||||
/// Rejects names Windows refuses or silently rewrites (alternate data stream
|
||||
/// colons, reserved device names, trailing dots or spaces) so a recorded path
|
||||
/// denotes the same regular file on every platform.
|
||||
fn is_safe_component(name: &str) -> bool {
|
||||
!name.contains(':')
|
||||
&& !name.ends_with('.')
|
||||
&& !name.ends_with(' ')
|
||||
&& !is_windows_reserved_name(name)
|
||||
}
|
||||
|
||||
fn is_windows_reserved_name(name: &str) -> bool {
|
||||
let stem = name.split('.').next().unwrap_or("");
|
||||
let lower = stem.to_ascii_lowercase();
|
||||
matches!(lower.as_str(), "con" | "prn" | "aux" | "nul")
|
||||
|| (lower.len() == 4
|
||||
&& (lower.starts_with("com") || lower.starts_with("lpt"))
|
||||
&& matches!(lower.as_bytes()[3], b'1'..=b'9'))
|
||||
}
|
||||
|
||||
fn uninstall_owned_files(
|
||||
@@ -1067,7 +1087,12 @@ impl TempRepoDir {
|
||||
|
||||
impl Drop for TempRepoDir {
|
||||
fn drop(&mut self) {
|
||||
let _ = fs::remove_dir_all(&self.path);
|
||||
if let Err(error) = fs::remove_dir_all(&self.path) {
|
||||
log::warn!(
|
||||
"failed to remove temp clone {}: {error}",
|
||||
self.path.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1079,13 +1104,37 @@ fn is_commit_sha(reference: &str) -> bool {
|
||||
|
||||
fn clone_to_temp(url: &str, reference: Option<&str>) -> Result<TempRepoDir> {
|
||||
let dest = utils::temp_file("coyote-remote-install-", "");
|
||||
match clone_into(&dest, url, reference) {
|
||||
Ok(head_sha) => Ok(TempRepoDir {
|
||||
path: dest,
|
||||
head_sha,
|
||||
}),
|
||||
Err(error) => {
|
||||
let _ = fs::remove_dir_all(&dest);
|
||||
Err(error)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Checked-out bytes must not depend on the machine's git configuration:
|
||||
/// recorded sha256 provenance would otherwise drift with autocrlf settings.
|
||||
/// Long paths are opted into for deep bundle trees on Windows.
|
||||
fn git_content_config() -> Vec<OsString> {
|
||||
["core.autocrlf=false", "core.eol=lf", "core.longpaths=true"]
|
||||
.iter()
|
||||
.flat_map(|setting| ["-c".into(), (*setting).into()])
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn clone_into(dest: &Path, url: &str, reference: Option<&str>) -> Result<String> {
|
||||
let dest_arg: OsString = dest.as_os_str().into();
|
||||
|
||||
let is_sha = reference.is_some_and(is_commit_sha);
|
||||
|
||||
match reference {
|
||||
Some(r) if !is_sha => {
|
||||
run_git(vec![
|
||||
let mut args = git_content_config();
|
||||
args.extend([
|
||||
"clone".into(),
|
||||
"--depth".into(),
|
||||
"1".into(),
|
||||
@@ -1094,26 +1143,28 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result<TempRepoDir> {
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg,
|
||||
])?;
|
||||
]);
|
||||
run_git(args)?;
|
||||
}
|
||||
Some(r) => {
|
||||
run_git(vec![
|
||||
"clone".into(),
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg.clone(),
|
||||
])?;
|
||||
run_git(vec!["-C".into(), dest_arg, "checkout".into(), r.into()])?;
|
||||
let mut args = git_content_config();
|
||||
args.extend(["clone".into(), "--".into(), url.into(), dest_arg.clone()]);
|
||||
run_git(args)?;
|
||||
let mut args = git_content_config();
|
||||
args.extend(["-C".into(), dest_arg, "checkout".into(), r.into()]);
|
||||
run_git(args)?;
|
||||
}
|
||||
None => {
|
||||
run_git(vec![
|
||||
let mut args = git_content_config();
|
||||
args.extend([
|
||||
"clone".into(),
|
||||
"--depth".into(),
|
||||
"1".into(),
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg,
|
||||
])?;
|
||||
]);
|
||||
run_git(args)?;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1123,11 +1174,7 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result<TempRepoDir> {
|
||||
"rev-parse".into(),
|
||||
"HEAD".into(),
|
||||
])?;
|
||||
|
||||
Ok(TempRepoDir {
|
||||
path: dest,
|
||||
head_sha,
|
||||
})
|
||||
Ok(head_sha)
|
||||
}
|
||||
|
||||
fn run_git(args: Vec<OsString>) -> Result<()> {
|
||||
@@ -1816,7 +1863,17 @@ fn record_written_file(
|
||||
}
|
||||
|
||||
fn provenance_path(dst: &Path) -> String {
|
||||
let rel = dst.strip_prefix(paths::config_dir()).unwrap_or(dst);
|
||||
let rel = match dst.strip_prefix(paths::config_dir()) {
|
||||
Ok(rel) => rel,
|
||||
Err(_) => {
|
||||
log::warn!(
|
||||
"bundle file {} lies outside the config dir (an asset dir override?); \
|
||||
it will not be uninstallable and drift checks may misreport it",
|
||||
dst.display()
|
||||
);
|
||||
dst
|
||||
}
|
||||
};
|
||||
rel.to_string_lossy().replace('\\', "/")
|
||||
}
|
||||
|
||||
@@ -2242,6 +2299,53 @@ mod tests {
|
||||
use std::env;
|
||||
use std::time::{SystemTime, UNIX_EPOCH};
|
||||
|
||||
#[test]
|
||||
fn safe_relative_path_accepts_plain_portable_components() {
|
||||
assert!(is_safe_relative_path("macros/a.yaml"));
|
||||
assert!(is_safe_relative_path("skills/deep/nested/file.md"));
|
||||
assert!(is_safe_relative_path("functions/tools/console.sh"));
|
||||
assert!(is_safe_relative_path("roles/common.md"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn safe_relative_path_rejects_escapes_and_windows_hazards() {
|
||||
assert!(!is_safe_relative_path("../outside.yaml"));
|
||||
assert!(!is_safe_relative_path("/abs/path.yaml"));
|
||||
assert!(!is_safe_relative_path("macros/../../evil.yaml"));
|
||||
assert!(!is_safe_relative_path("macros/a.yaml:stream"));
|
||||
assert!(!is_safe_relative_path("macros/trailing."));
|
||||
assert!(!is_safe_relative_path("macros/trailing "));
|
||||
assert!(!is_safe_relative_path("macros/nul"));
|
||||
assert!(!is_safe_relative_path("macros/NUL.yaml"));
|
||||
assert!(!is_safe_relative_path("con/a.yaml"));
|
||||
assert!(!is_safe_relative_path("macros/COM1.txt"));
|
||||
assert!(!is_safe_relative_path("macros/lpt9"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn windows_reserved_name_check_is_stem_based() {
|
||||
assert!(is_windows_reserved_name("nul"));
|
||||
assert!(is_windows_reserved_name("NUL.txt"));
|
||||
assert!(is_windows_reserved_name("com1"));
|
||||
assert!(!is_windows_reserved_name("com0"));
|
||||
assert!(!is_windows_reserved_name("com10"));
|
||||
assert!(!is_windows_reserved_name("console"));
|
||||
assert!(!is_windows_reserved_name("nullable.yaml"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn git_content_config_pins_line_endings_and_long_paths() {
|
||||
let args = git_content_config();
|
||||
let rendered: Vec<String> = args
|
||||
.iter()
|
||||
.map(|a| a.to_string_lossy().into_owned())
|
||||
.collect();
|
||||
assert_eq!(rendered.len(), 6);
|
||||
assert!(rendered.contains(&"core.autocrlf=false".to_string()));
|
||||
assert!(rendered.contains(&"core.eol=lf".to_string()));
|
||||
assert!(rendered.contains(&"core.longpaths=true".to_string()));
|
||||
}
|
||||
|
||||
struct TestVaultConfigGuard {
|
||||
dir_key: String,
|
||||
file_key: String,
|
||||
|
||||
Reference in New Issue
Block a user