From 4712279c3fcee9f574c41cb337b33a2a89d9863e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 20:03:58 +0000 Subject: [PATCH 1/3] Start refactor for #707 Assisted-by: Claude Code:claude-opus-5-5 From 2388907ad64ddc40f8a9ac3362336f9debff72d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 20:04:37 +0000 Subject: [PATCH 2/3] Load manifest hashes as lowercase hex A manifest whose beforeHash/afterHash are spelled in uppercase hex passed blob download (which compares case-insensitively) but then failed apply and rollback verification, which compare the computed lowercase git-sha256 with ==. The same bytes were reported as a hash mismatch. Lowercase both hashes when a PatchFileInfo is deserialized, so every comparison site, blob name and vendored pin sees one spelling. A manifest written back after load carries the lowercase spelling. Fixes #707. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/manifest/schema.rs | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/crates/socket-patch-core/src/manifest/schema.rs b/crates/socket-patch-core/src/manifest/schema.rs index 7fd487e0a..ac88535e9 100644 --- a/crates/socket-patch-core/src/manifest/schema.rs +++ b/crates/socket-patch-core/src/manifest/schema.rs @@ -12,13 +12,31 @@ pub struct VulnerabilityInfo { } /// Hash information for a single patched file. +/// +/// Both hashes are lowercase hex once deserialized, whatever case the +/// manifest spells them in (#707): apply and rollback verification, blob +/// names and vendored pins then compare them to a computed (lowercase) +/// git-sha256 with plain `==`, and no comparison site needs its own case +/// rule. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "camelCase")] pub struct PatchFileInfo { + #[serde(deserialize_with = "deserialize_hash")] pub before_hash: String, + #[serde(deserialize_with = "deserialize_hash")] pub after_hash: String, } +/// The manifest's hash case policy: a loaded hash is lowercase hex. +fn deserialize_hash<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let mut hash = String::deserialize(deserializer)?; + hash.make_ascii_lowercase(); + Ok(hash) +} + /// A single patch record in the manifest. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] #[serde(rename_all = "camelCase")] @@ -565,4 +583,116 @@ mod tests { "a manifest without a `patches` field must be rejected" ); } + + /// #707: a hand-edited or third-party manifest may spell its hashes in + /// uppercase. They load as lowercase, the case every computed + /// git-sha256 and every blob name uses; an empty `beforeHash` (a + /// patch-added file) stays empty. + #[test] + fn test_patch_file_info_loads_hashes_lowercase() { + let upper = "ABCDEF0123456789".repeat(4); + let json = format!(r#"{{"beforeHash": "{upper}", "afterHash": "{upper}"}}"#); + let parsed: PatchFileInfo = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed.before_hash, upper.to_ascii_lowercase()); + assert_eq!(parsed.after_hash, upper.to_ascii_lowercase()); + + let added: PatchFileInfo = + serde_json::from_str(r#"{"beforeHash": "", "afterHash": "AB"}"#).unwrap(); + assert_eq!(added.before_hash, ""); + assert_eq!(added.after_hash, "ab"); + + // Re-serializing writes the normalized spelling. + let json = serde_json::to_string(&parsed).unwrap(); + assert!(!json.contains(&upper), "{json}"); + } + + /// #707 end to end: the same patch, with its hashes uppercased in the + /// manifest, verifies, applies and rolls back exactly like the + /// lowercase one. Before the load normalized them, apply and rollback + /// compared the computed (lowercase) hash with `==` and reported + /// `HashMismatch` for bytes that matched. + #[tokio::test] + async fn test_uppercase_manifest_hashes_apply_and_roll_back() { + use crate::hash::git_sha256::compute_git_sha256_from_bytes; + use crate::patch::apply::{ + apply_package_patch, verify_file_patch, MismatchPolicy, PatchSources, VerifyStatus, + }; + use crate::patch::rollback::{ + rollback_package_patch, verify_file_rollback, VerifyRollbackStatus, + }; + + let original = b"module.exports = 'vulnerable';\n"; + let patched = b"module.exports = 'fixed';\n"; + let before = compute_git_sha256_from_bytes(original); + let after = compute_git_sha256_from_bytes(patched); + + for upper in [false, true] { + let spell = |h: &str| { + if upper { + h.to_ascii_uppercase() + } else { + h.to_string() + } + }; + let manifest_json = format!( + r#"{{"patches": {{"pkg:npm/demo@1.0.0": {{ + "uuid": "11111111-1111-4111-8111-111111111111", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{"package/index.js": {{"beforeHash": "{}", "afterHash": "{}"}}}}, + "vulnerabilities": {{}}, + "description": "d", "license": "MIT", "tier": "free" + }}}}}}"#, + spell(&before), + spell(&after) + ); + let dir = tempfile::tempdir().unwrap(); + let manifest_path = dir.path().join("manifest.json"); + std::fs::write(&manifest_path, manifest_json).unwrap(); + let manifest = crate::manifest::operations::read_manifest(&manifest_path) + .await + .unwrap() + .unwrap(); + let files = &manifest.patches["pkg:npm/demo@1.0.0"].files; + let info = &files["package/index.js"]; + + let pkg = dir.path().join("pkg"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write(pkg.join("index.js"), original).unwrap(); + // Blobs are stored under the lowercase name blob download uses. + let blobs = dir.path().join("blobs"); + std::fs::create_dir_all(&blobs).unwrap(); + std::fs::write(blobs.join(&before), original).unwrap(); + std::fs::write(blobs.join(&after), patched).unwrap(); + + let verify = verify_file_patch(&pkg, "package/index.js", info).await; + assert_eq!( + verify.status, + VerifyStatus::Ready, + "upper={upper}: {verify:?}" + ); + let applied = apply_package_patch( + "pkg:npm/demo@1.0.0", + &pkg, + files, + &PatchSources::blobs_only(&blobs), + None, + false, + MismatchPolicy::Strict, + ) + .await; + assert!(applied.success, "upper={upper}: {:?}", applied.error); + assert_eq!(std::fs::read(pkg.join("index.js")).unwrap(), patched); + + let verify = verify_file_rollback(&pkg, "package/index.js", info, &blobs).await; + assert_eq!( + verify.status, + VerifyRollbackStatus::Ready, + "upper={upper}: {verify:?}" + ); + let rolled = + rollback_package_patch("pkg:npm/demo@1.0.0", &pkg, files, &blobs, false).await; + assert!(rolled.success, "upper={upper}: {:?}", rolled.error); + assert_eq!(std::fs::read(pkg.join("index.js")).unwrap(), original); + } + } } From b920a55e217d8e359e09543115496ea2efee4119 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 20:25:48 +0000 Subject: [PATCH 3/3] Port oracle selftest tempdir fix from #1162 The coverage job failed on e2e_vendored_production: tests/common is compiled twice into that binary, so its oracle selftests ran twice in one process and clobbered each other's PID-named /tmp scratch files. This is the same change as #1162 (per-test tempfile::tempdir()), so the flake stops blocking this PR; it no-ops once #1162 merges. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/tests/common/mod.rs | 34 +++++---------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/mod.rs b/crates/socket-patch-cli/tests/common/mod.rs index 50355d2d6..1cc10c5b2 100644 --- a/crates/socket-patch-cli/tests/common/mod.rs +++ b/crates/socket-patch-cli/tests/common/mod.rs @@ -575,10 +575,9 @@ mod oracle_selftests { // back and confirm it equals hashing the same bytes in memory, // and that distinct contents produce distinct hashes (i.e. it // isn't returning a constant or hashing the path). - let dir = std::env::temp_dir(); - let unique = format!("socket-patch-oracle-{}", std::process::id()); - let p1 = dir.join(format!("{unique}-a.bin")); - let p2 = dir.join(format!("{unique}-b.bin")); + let dir = tempfile::tempdir().expect("temp dir"); + let p1 = dir.path().join("a.bin"); + let p2 = dir.path().join("b.bin"); let content_a = b"alpha-content\n"; let content_b = b"beta-content\n"; std::fs::write(&p1, content_a).expect("write temp a"); @@ -591,21 +590,6 @@ mod oracle_selftests { git_sha256_file(&p2), "git_sha256_file must reflect file contents" ); - - let _ = std::fs::remove_file(&p1); - let _ = std::fs::remove_file(&p2); - } - - // Unique temp dir per (pid, callsite) so the fixture-builder self-tests - // never collide with each other or across parallel test binaries. - fn scratch_dir(tag: &str) -> PathBuf { - let d = std::env::temp_dir().join(format!( - "socket-patch-oracle-{}-{}", - std::process::id(), - tag - )); - let _ = std::fs::remove_dir_all(&d); - d } #[test] @@ -616,8 +600,8 @@ mod oracle_selftests { // the suites would pass while exercising nothing. Pin the exact shape // apply consumes: `patches..{uuid,files..{beforeHash, // afterHash}}`, all camelCase. - let root = scratch_dir("manifest"); - let socket_dir = root.join(".socket"); + let root = tempfile::tempdir().expect("temp dir"); + let socket_dir = root.path().join(".socket"); let purl = "pkg:npm/dummy@1.0.0"; let uuid = "11111111-1111-4111-8111-111111111111"; let path = write_minimal_manifest( @@ -669,8 +653,6 @@ mod oracle_selftests { !socket_dir.join("blobs").join("afterhash111").exists(), "write_minimal_manifest must not stage after_hash blobs" ); - - let _ = std::fs::remove_dir_all(&root); } #[test] @@ -679,8 +661,8 @@ mod oracle_selftests { // `/blobs/` and verifies their bytes. If write_blob // wrote the wrong path or mangled the bytes, "offline apply succeeds" // tests would silently fall back to a network path or fail to match. - let root = scratch_dir("blob"); - let socket_dir = root.join(".socket"); + let root = tempfile::tempdir().expect("temp dir"); + let socket_dir = root.path().join(".socket"); let hash = "deadbeefcafef00d"; let payload = &[0u8, 1, 2, 255, b'p', b'a', b't', b'c', b'h', 0, 42]; write_blob(&socket_dir, hash, payload); @@ -696,8 +678,6 @@ mod oracle_selftests { payload, "write_blob must stage the exact bytes, byte-for-byte" ); - - let _ = std::fs::remove_dir_all(&root); } #[test]