From 97511cb66754931b4bef925eeaf48b16f6d9f57a Mon Sep 17 00:00:00 2001 From: tonythethompson Date: Mon, 10 Aug 2026 05:30:42 -0700 Subject: [PATCH 1/2] fix(install): verify cached artifacts before extract Cache hits previously skipped integrity::verify_and_report, so a corrupted cache file could reach extraction while provisional installs still claimed integrity checks passed. Always re-verify the on-disk cache file, and cover provisional warn-once plus corrupted-cache paths in install integration tests. Co-authored-by: Cursor --- src/install/transaction.rs | 5 + tests/install_test.rs | 228 ++++++++++++++++++++++++++++++++++++- 2 files changed, 232 insertions(+), 1 deletion(-) diff --git a/src/install/transaction.rs b/src/install/transaction.rs index 732c758..5a386aa 100644 --- a/src/install/transaction.rs +++ b/src/install/transaction.rs @@ -226,6 +226,11 @@ pub fn install_package( println!("{} Using cached download", console::style("✓").green()); } + // Re-check the promoted cache file so cache hits cannot bypass integrity. + if let Some(ref expected_sha) = artifact_sha256 { + integrity::verify_and_report(&cache_file, expected_sha, &lock_key)?; + } + // Observed SHA-256 of the downloaded archive — distinct from the // registry-declared `artifact_sha256` used for integrity verification. let payload_sha256 = { diff --git a/tests/install_test.rs b/tests/install_test.rs index ae80fe5..87675f7 100644 --- a/tests/install_test.rs +++ b/tests/install_test.rs @@ -2,7 +2,8 @@ use ed25519_dalek::{Signer, SigningKey}; use numan_cli::core::nu_version::NuVersion; use numan_cli::core::official_registry::RegistrySignature; use numan_cli::core::package::{ - Artifact, Package, PackageType, RegistryIndex, ScopedId, TargetArtifact, VersionEntry, + Artifact, EvidenceTier, Package, PackageType, RegistryIndex, ScopedId, TargetArtifact, + VersionEntry, }; use numan_cli::core::platform::{Arch, Env, Os, Platform}; use numan_cli::install::transaction::{self, InstallOptions}; @@ -560,3 +561,228 @@ fn integration_snapshot_before_install() { let snapshots = list_snapshots(&root).unwrap(); assert_eq!(snapshots.len(), 1, "Should have exactly one snapshot"); } + +#[test] +fn integration_install_provisional_warns_only_on_new_install() { + const CHILD_MARKER: &str = "NUMAN_PROVISIONAL_INSTALL_TEST_CHILD"; + + if std::env::var_os(CHILD_MARKER).is_none() { + let output = Command::new(std::env::current_exe().unwrap()) + .arg("integration_install_provisional_warns_only_on_new_install") + .arg("--exact") + .arg("--nocapture") + .env(CHILD_MARKER, "1") + .output() + .unwrap(); + assert!( + output.status.success(), + "Provisional-install child test failed.\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + let parts: Vec<&str> = stdout.split("===SECOND_INSTALL===").collect(); + assert_eq!( + parts.len(), + 2, + "expected delimiter between installs\nstdout:\n{stdout}" + ); + assert!( + parts[0].contains( + "This package has not been lifecycle-tested. It passed integrity checks. (reason not recorded)" + ), + "new provisional install must warn with fallback reason\nfirst half:\n{}", + parts[0] + ); + assert!( + !parts[1].contains("lifecycle-tested"), + "already-installed no-op must not emit provisional warning\nsecond half:\n{}", + parts[1] + ); + return; + } + + let tmp = TempDir::new().unwrap(); + let root = tmp.path().to_path_buf(); + let signing_key = setup_trusted_key(&root, "test"); + + let artifacts_dir = root.join("artifacts"); + std::fs::create_dir_all(&artifacts_dir).unwrap(); + let (zip_url, zip_sha) = + create_plugin_zip(&artifacts_dir, "nu_plugin_prov.exe", b"provisional plugin"); + + let package = Package { + id: ScopedId::new("test", "prov"), + description: "Provisional plugin".to_string(), + repo: "https://github.com/test/prov".to_string(), + package_type: PackageType::Plugin, + tags: vec![], + versions: vec![VersionEntry { + version: semver::Version::new(1, 0, 0), + nu_version: "*".to_string(), + verified_with: vec![], + artifact: Artifact { + kind: "binary".to_string(), + url: None, + sha256: None, + targets: { + let mut m = HashMap::new(); + m.insert( + "x86_64-pc-windows-msvc".to_string(), + TargetArtifact { + url: zip_url, + sha256: zip_sha, + executable_path: "nu_plugin_prov.exe".to_string(), + }, + ); + m + }, + archive_root: None, + include: None, + entry: None, + }, + source: None, + dependencies: BTreeMap::new(), + activation: None, + provenance: None, + evidence_tier: Some(EvidenceTier::Provisional), + deferral_reason: None, + }], + }; + + create_signed_registry(&root, "test", vec![package], &signing_key); + std::fs::write( + root.join("config.toml"), + "[general]\ndefault_registry = \"test\"\n", + ) + .unwrap(); + + let platform = Platform { + os: Os::Windows, + arch: Arch::X86_64, + env: Env::Msvc, + triple: "x86_64-pc-windows-msvc".to_string(), + }; + let nu_version = NuVersion::parse("0.113.1").unwrap(); + let options = InstallOptions { + root: &root, + platform: &platform, + nu_version: &nu_version, + force: false, + verbose: false, + registry_name: None, + snapshot_trigger: SnapshotTrigger::Install, + }; + + let first = transaction::install_package("test/prov", None, &options).unwrap(); + assert!(first.installed); + assert!(!first.already_existed); + + println!("===SECOND_INSTALL==="); + + let second = transaction::install_package("test/prov", None, &options).unwrap(); + assert!(!second.installed); + assert!(second.already_existed); +} + +#[test] +fn integration_install_rejects_corrupted_cached_artifact() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().to_path_buf(); + let signing_key = setup_trusted_key(&root, "test"); + + let artifacts_dir = root.join("artifacts"); + std::fs::create_dir_all(&artifacts_dir).unwrap(); + let (zip_url, zip_sha) = create_plugin_zip( + &artifacts_dir, + "nu_plugin_cache.exe", + b"cache integrity plugin", + ); + + let package = Package { + id: ScopedId::new("test", "cache"), + description: "Cache integrity plugin".to_string(), + repo: "https://github.com/test/cache".to_string(), + package_type: PackageType::Plugin, + tags: vec![], + versions: vec![VersionEntry { + version: semver::Version::new(1, 0, 0), + nu_version: "*".to_string(), + verified_with: vec![], + artifact: Artifact { + kind: "binary".to_string(), + url: None, + sha256: None, + targets: { + let mut m = HashMap::new(); + m.insert( + "x86_64-pc-windows-msvc".to_string(), + TargetArtifact { + url: zip_url, + sha256: zip_sha.clone(), + executable_path: "nu_plugin_cache.exe".to_string(), + }, + ); + m + }, + archive_root: None, + include: None, + entry: None, + }, + source: None, + dependencies: BTreeMap::new(), + activation: None, + provenance: None, + evidence_tier: None, + deferral_reason: None, + }], + }; + + create_signed_registry(&root, "test", vec![package], &signing_key); + std::fs::write( + root.join("config.toml"), + "[general]\ndefault_registry = \"test\"\n", + ) + .unwrap(); + + // Plant a corrupted cache hit under the expected content-addressed key. + let cache_dir = root.join("cache/downloads"); + std::fs::create_dir_all(&cache_dir).unwrap(); + std::fs::write( + cache_dir.join(format!("{zip_sha}.bin")), + b"not the real artifact", + ) + .unwrap(); + + let platform = Platform { + os: Os::Windows, + arch: Arch::X86_64, + env: Env::Msvc, + triple: "x86_64-pc-windows-msvc".to_string(), + }; + let nu_version = NuVersion::parse("0.113.1").unwrap(); + let options = InstallOptions { + root: &root, + platform: &platform, + nu_version: &nu_version, + force: false, + verbose: false, + registry_name: None, + snapshot_trigger: SnapshotTrigger::Install, + }; + + let err = transaction::install_package("test/cache", None, &options) + .expect_err("corrupted cache must fail integrity before extract"); + let msg = err.to_string(); + assert!( + msg.contains("Integrity check failed"), + "expected integrity failure, got: {msg}" + ); + assert!( + !root.join("packages").exists() + || std::fs::read_dir(root.join("packages")) + .map(|mut d| d.next().is_none()) + .unwrap_or(true), + "corrupted cache must not produce an extracted package payload" + ); +} From f201ce60ba69003dd89eb25409ea8de55ab24356 Mon Sep 17 00:00:00 2001 From: tonythethompson Date: Mon, 10 Aug 2026 05:46:57 -0700 Subject: [PATCH 2/2] fix(install): self-heal corrupt cache and reuse digest On cache-hit integrity failure, delete the bad entry and re-download once instead of sticky-failing. Have verify_and_report return the observed digest so install can set payload_sha256 without a second full-file hash pass. Co-authored-by: Cursor --- src/core/integrity.rs | 28 ++++++++++++++++-- src/install/transaction.rs | 59 ++++++++++++++++++++++++++++---------- tests/install_test.rs | 32 +++++++++------------ 3 files changed, 83 insertions(+), 36 deletions(-) diff --git a/src/core/integrity.rs b/src/core/integrity.rs index 1cc3421..10685ae 100644 --- a/src/core/integrity.rs +++ b/src/core/integrity.rs @@ -40,7 +40,9 @@ pub fn verify_bytes(data: &[u8], expected_sha256: &str) -> Result { Ok(hash == expected_sha256) } -pub fn verify_and_report(path: &Path, expected_sha256: &str, pkg_name: &str) -> Result<()> { +/// Verify `path` against `expected_sha256`, returning the observed digest on +/// success so callers can reuse it without hashing the file again. +pub fn verify_and_report(path: &Path, expected_sha256: &str, pkg_name: &str) -> Result { let actual = hash_file(path)?; if actual != expected_sha256 { bail!( @@ -50,7 +52,7 @@ pub fn verify_and_report(path: &Path, expected_sha256: &str, pkg_name: &str) -> This may indicate a corrupted download or tampered artifact." ); } - Ok(()) + Ok(actual) } #[cfg(test)] @@ -89,4 +91,26 @@ mod tests { fn verify_bytes_matches() { assert!(verify_bytes(b"test", &compute_sha256(b"test")).unwrap()); } + + #[test] + fn verify_and_report_returns_digest_on_success() { + let file = NamedTempFile::new().unwrap(); + std::fs::write(file.path(), b"report me").unwrap(); + let expected = compute_sha256(b"report me"); + let actual = verify_and_report(file.path(), &expected, "pkg").unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn verify_and_report_rejects_mismatch() { + let file = NamedTempFile::new().unwrap(); + std::fs::write(file.path(), b"report me").unwrap(); + let err = verify_and_report( + file.path(), + "0000000000000000000000000000000000000000000000000000000000000000", + "pkg", + ) + .unwrap_err(); + assert!(err.to_string().contains("Integrity check failed")); + } } diff --git a/src/install/transaction.rs b/src/install/transaction.rs index 5a386aa..4f0d938 100644 --- a/src/install/transaction.rs +++ b/src/install/transaction.rs @@ -203,7 +203,41 @@ pub fn install_package( let cache_file = cache_dir.join(format!("{cache_key}.bin")); let cache_part = cache_dir.join(format!("{cache_key}.part")); - if !cache_file.exists() || options.force { + // Observed archive digest after integrity verification (or a fallback hash + // when the registry entry has no expected SHA). + let mut payload_sha256: Option = None; + let mut need_download = !cache_file.exists() || options.force; + + if !need_download { + if let Some(ref expected_sha) = artifact_sha256 { + match integrity::verify_and_report(&cache_file, expected_sha, &lock_key) { + Ok(digest) => { + if options.verbose { + println!("{} Using cached download", console::style("✓").green()); + } + payload_sha256 = Some(digest); + } + Err(_) => { + // One-shot self-heal: drop the bad cache entry and re-download. + if options.verbose { + println!( + "{} Cached download failed integrity; re-downloading", + console::style("!").yellow() + ); + } + let _ = std::fs::remove_file(&cache_file); + if cache_part.exists() { + let _ = std::fs::remove_file(&cache_part); + } + need_download = true; + } + } + } else if options.verbose { + println!("{} Using cached download", console::style("✓").green()); + } + } + + if need_download { if options.verbose { println!( "{} Downloading {}@{}...", @@ -213,32 +247,27 @@ pub fn install_package( ); } download::download_file(&artifact_url, &cache_part)?; - // Verify before promoting from .part to final + // Verify before promoting from .part to final; reuse digest as payload hash. if let Some(ref expected_sha) = artifact_sha256 { - integrity::verify_and_report(&cache_part, expected_sha, &lock_key)?; + payload_sha256 = Some(integrity::verify_and_report( + &cache_part, + expected_sha, + &lock_key, + )?); } // Atomic promote if cache_file.exists() { std::fs::remove_file(&cache_file)?; } std::fs::rename(&cache_part, &cache_file)?; - } else if options.verbose { - println!("{} Using cached download", console::style("✓").green()); } - // Re-check the promoted cache file so cache hits cannot bypass integrity. - if let Some(ref expected_sha) = artifact_sha256 { - integrity::verify_and_report(&cache_file, expected_sha, &lock_key)?; - } - - // Observed SHA-256 of the downloaded archive — distinct from the - // registry-declared `artifact_sha256` used for integrity verification. - let payload_sha256 = { + if payload_sha256.is_none() { let bytes = std::fs::read(&cache_file).with_context(|| { format!("Failed to read cached payload at {}", cache_file.display()) })?; - Some(integrity::compute_sha256(&bytes)) - }; + payload_sha256 = Some(integrity::compute_sha256(&bytes)); + } // 8. Extract to staging dir on same volume as install target let parent_dir = install_dir.parent().unwrap_or(options.root); diff --git a/tests/install_test.rs b/tests/install_test.rs index 87675f7..2538db1 100644 --- a/tests/install_test.rs +++ b/tests/install_test.rs @@ -686,7 +686,7 @@ fn integration_install_provisional_warns_only_on_new_install() { } #[test] -fn integration_install_rejects_corrupted_cached_artifact() { +fn integration_install_self_heals_corrupted_cached_artifact() { let tmp = TempDir::new().unwrap(); let root = tmp.path().to_path_buf(); let signing_key = setup_trusted_key(&root, "test"); @@ -748,11 +748,8 @@ fn integration_install_rejects_corrupted_cached_artifact() { // Plant a corrupted cache hit under the expected content-addressed key. let cache_dir = root.join("cache/downloads"); std::fs::create_dir_all(&cache_dir).unwrap(); - std::fs::write( - cache_dir.join(format!("{zip_sha}.bin")), - b"not the real artifact", - ) - .unwrap(); + let cache_file = cache_dir.join(format!("{zip_sha}.bin")); + std::fs::write(&cache_file, b"not the real artifact").unwrap(); let platform = Platform { os: Os::Windows, @@ -771,18 +768,15 @@ fn integration_install_rejects_corrupted_cached_artifact() { snapshot_trigger: SnapshotTrigger::Install, }; - let err = transaction::install_package("test/cache", None, &options) - .expect_err("corrupted cache must fail integrity before extract"); - let msg = err.to_string(); - assert!( - msg.contains("Integrity check failed"), - "expected integrity failure, got: {msg}" - ); - assert!( - !root.join("packages").exists() - || std::fs::read_dir(root.join("packages")) - .map(|mut d| d.next().is_none()) - .unwrap_or(true), - "corrupted cache must not produce an extracted package payload" + let result = transaction::install_package("test/cache", None, &options) + .expect("corrupted cache should self-heal via one re-download"); + assert!(result.installed); + assert!(root.join(&result.path).join("nu_plugin_cache.exe").exists()); + + let repaired = std::fs::read(&cache_file).unwrap(); + assert_eq!( + numan_cli::core::integrity::compute_sha256(&repaired), + zip_sha, + "cache entry must be replaced with the verified artifact" ); }