diff --git a/docs/package-manager-hardening-framing.md b/docs/package-manager-hardening-framing.md index 30019fa..8c120cf 100644 --- a/docs/package-manager-hardening-framing.md +++ b/docs/package-manager-hardening-framing.md @@ -115,13 +115,19 @@ CI. Landed as framed; all five in `src/packages/` (F-011 also touches `lua_bindings.rs` + acceptance tests via the rename). No serde/wire change. -- **F-005** (`resolver.rs`): new `ResolveError::BasenameCollision { - basename, names }`; a free `find_basename_collision(names)` (testable, - used by `into_plan`) groups the plan's names by `package_basename` and - returns the first basename with >1 distinct name. Two unit tests; the - existing `roster_lookup_picks_most_recent_on_basename_collision` loader - test still passes, confirming the intended cross-scope override is - untouched. +- **F-005** (`resolver.rs` + `lockfile.rs`): new + `ResolveError::BasenameCollision { basename, names }`; a `pub(crate)` + `find_basename_collision(names)` groups the plan's names by + `package_basename` and returns the first basename with >1 distinct name. + **Guards *both* plan-construction paths** (review follow-up): `into_plan` + (fresh / `UpdateOne`) *and* `Lockfile::to_resolve_plan` (the frozen / + lockfile-derived path, which returns its own + `LockfileError::BasenameCollision`). The frozen check runs up front, + before any fetch, so a hand-edited or pre-existing lockfile with two + same-basename packages fails fast. Three unit tests (detection, negative + cases, and the frozen-path rejection); the existing + `roster_lookup_picks_most_recent_on_basename_collision` loader test still + passes, confirming the intended cross-scope override is untouched. - **F-009** (`fetcher.rs`): `fnv1a_hex` → `sha256_hex` (`sha2::Sha256`, 64-char hex) for the bare-mirror dir/lock key; `normalize_url` unchanged in front. Hash test asserts stability, normalization, 64-hex shape, and @@ -142,8 +148,9 @@ Landed as framed; all five in `src/packages/` (F-011 also touches states the actual algorithm. Validated: `cargo fmt` clean; `clippy --all-targets` clean under **both** -Lua flavors (luajit + lua54); 1436 lib unit tests pass, incl. the new -F-005/F-009 tests, the F-010 timeout test, and the loader override test. +Lua flavors (luajit + lua54); 1437 lib unit tests pass, incl. the new +F-005 tests (fresh + frozen paths), the F-009 hash test, the F-010 timeout +test, and the loader override test. ## Deferred (named) diff --git a/src/packages/lockfile.rs b/src/packages/lockfile.rs index 1cb8fc0..5214968 100644 --- a/src/packages/lockfile.rs +++ b/src/packages/lockfile.rs @@ -88,7 +88,9 @@ use super::address::Address; use super::fetcher::{FetchError, Fetcher, RefSpec}; use super::installer::InstallPin; use super::manifest::PackageName; -use super::resolver::{ResolvePlan, ResolvedPackage}; +use super::resolver::{ + ResolvePlan, ResolvedPackage, find_basename_collision, format_package_name_list, +}; /// Schema version emitted by [`Lockfile::to_bytes`]. Bumped only on /// incompatible format changes. @@ -489,6 +491,16 @@ impl Lockfile { /// (reproducible installs across machines) on the install path /// itself, not just at lockfile-write time. pub fn to_resolve_plan(&self, fetcher: &Fetcher) -> Result { + // Reject basename collisions up front (audit F-005), before any + // fetch: a hand-edited or pre-existing lockfile with two distinct + // packages sharing a basename (e.g. `owner/magit` + `other/magit`) + // would otherwise install both to `/`. The + // fresh-resolve path guards this identically in `into_plan`. + if let Some((basename, names)) = + find_basename_collision(self.packages.iter().map(|e| e.name.clone())) + { + return Err(LockfileError::BasenameCollision { basename, names }); + } let mut packages = Vec::with_capacity(self.packages.len()); for entry in &self.packages { // Verify before reading the manifest. `verify_entry` @@ -816,6 +828,24 @@ pub enum LockfileError { /// The name that was passed to `UpdateOne`. name: PackageName, }, + /// Two or more distinct lockfile entries share an install basename + /// (audit F-005). A hand-edited or pre-existing lockfile with e.g. + /// `owner/magit` and `other/magit` would install both to + /// `/magit`. The frozen/lockfile plan path must reject this just + /// like the fresh-resolve path does. + #[error( + "lockfile has packages [{names}] sharing the install basename \ + `{basename}`: pmacs installs and requires by basename, so these \ + distinct packages would collide on disk. Regenerate the lockfile \ + after renaming one, or install them in separate roots.", + names = format_package_name_list(.names) + )] + BasenameCollision { + /// The colliding last-path-segment. + basename: String, + /// The distinct package names that map to it. + names: Vec, + }, } // --------------------------------------------------------------------------- @@ -826,6 +856,42 @@ pub enum LockfileError { mod tests { use super::*; + #[test] + fn to_resolve_plan_rejects_basename_collision_before_fetching() { + // A hand-edited / pre-existing lockfile with two distinct packages + // that share an install basename (audit F-005). The frozen path + // must reject it just like the fresh-resolve path, and it does so + // *before* any fetch — so the throwaway cache dir is never touched. + let entry = |name: &str, url: &str| LockfileEntry { + name: PackageName::new(name).unwrap(), + url: url.into(), + commit: "0".repeat(40), + version: Version::new(1, 0, 0), + content_hash: ContentHash::sha256_of(name.as_bytes()), + top_level_pin: None, + dependencies: vec![], + }; + let lock = Lockfile { + schema_version: LOCKFILE_SCHEMA_VERSION, + generator: "pmacs test".into(), + packages: vec![ + entry("owner/magit", "https://example.com/owner/magit.git"), + entry("other/magit", "https://example.com/other/magit.git"), + ], + }; + let fetcher = Fetcher::with_cache_dir(std::env::temp_dir()); + let err = lock + .to_resolve_plan(&fetcher) + .expect_err("colliding lockfile must be rejected"); + match err { + LockfileError::BasenameCollision { basename, names } => { + assert_eq!(basename, "magit"); + assert_eq!(names.len(), 2); + } + other => panic!("expected BasenameCollision, got {other:?}"), + } + } + #[test] fn content_hash_sha256_known_vector() { // RFC 6234 §8.5: SHA-256 of "abc" diff --git a/src/packages/resolver.rs b/src/packages/resolver.rs index 00bcd2a..31873cc 100644 --- a/src/packages/resolver.rs +++ b/src/packages/resolver.rs @@ -1329,9 +1329,9 @@ pub enum ResolveError { }, } -/// Comma-join package names for a [`ResolveError::BasenameCollision`] -/// message. -fn format_package_name_list(names: &[PackageName]) -> String { +/// Comma-join package names for a basename-collision message. Shared with +/// [`super::lockfile`], which guards the frozen/lockfile plan path. +pub(crate) fn format_package_name_list(names: &[PackageName]) -> String { names .iter() .map(|n| n.as_str().to_string()) @@ -1342,9 +1342,10 @@ fn format_package_name_list(names: &[PackageName]) -> String { /// Find an install basename shared by more than one distinct package name /// (audit F-005). Returns the colliding basename and its names (sorted by /// the `BTreeMap`/first-seen order), or `None` when every basename is -/// unique. Used by [`into_plan`] to reject a plan that would collide on -/// disk before anything is installed. -fn find_basename_collision( +/// unique. Guards both plan-construction paths: `into_plan` (fresh / +/// `UpdateOne` resolves) and [`super::lockfile::Lockfile::to_resolve_plan`] +/// (the frozen / lockfile-derived path). +pub(crate) fn find_basename_collision( names: impl Iterator, ) -> Option<(String, Vec)> { let mut by_basename: BTreeMap> = BTreeMap::new();