From 29e086448587c5580ddf5dd07e9f8b53457c5a8d Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Fri, 31 Jul 2026 17:07:04 -0600 Subject: [PATCH] close the spl pin guard gaps the audit found The root workspace entry now rejects a path route, so a local override is caught in every manifest rather than only in members. The exact-version coverage the old policy test carried is restored as a derived property: each workspace declaration must carry a version and it must equal the resolved lockfile version, with no literal version in guard or test code. The inherited-exactly-once requirement is now stated over the workspace instead of a hardcoded consumer manifest path. Source-replacement and TOML-parse diagnostics now name what the guard actually checks: the Cargo configuration route and the specific file, rather than claiming a package-specific effect the guard does not establish. The wrong-query and wrong-fragment fixtures now derive a different revision dynamically instead of assuming the pin is never forty a characters. The suite now has 29 fixture cases. make ci is green, and the pinned revision and lockfile are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- crates/rust-release-manifest/src/spl_pin.rs | 97 ++++++++++---- crates/rust-release-manifest/tests/spl_pin.rs | 121 +++++++++++++++--- 2 files changed, 174 insertions(+), 44 deletions(-) diff --git a/crates/rust-release-manifest/src/spl_pin.rs b/crates/rust-release-manifest/src/spl_pin.rs index bce8c58..60a6fac 100644 --- a/crates/rust-release-manifest/src/spl_pin.rs +++ b/crates/rust-release-manifest/src/spl_pin.rs @@ -2,7 +2,7 @@ // Copyright (c) 2026 sol pbc use super::{Error, RepoRoot, Result}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::fs; use std::path::{Component, Path, PathBuf}; use std::process::Command; @@ -11,6 +11,11 @@ const SPL_SOURCE: &str = "https://github.com/solpbc/spl-rust"; const PACKAGES: [&str; 2] = ["spl-core", "spl-transport"]; const DEPENDENCY_TABLES: [&str; 3] = ["dependencies", "dev-dependencies", "build-dependencies"]; +struct WorkspacePins { + revisions: BTreeMap, + versions: BTreeMap, +} + fn error(message: String) -> Error { Error::new(message) } @@ -90,12 +95,13 @@ fn selector_actual(table: &toml::value::Table) -> String { } } -fn validate_workspace(root: &toml::Value) -> Result> { +fn validate_workspace(root: &toml::Value) -> Result { let dependencies = root .get("workspace") .and_then(|value| value.get("dependencies")) .and_then(toml::Value::as_table); let mut revisions = BTreeMap::new(); + let mut versions = BTreeMap::new(); for package in PACKAGES { let matches = dependencies .into_iter() @@ -117,6 +123,20 @@ fn validate_workspace(root: &toml::Value) -> Result> { "SPL package {package} source mismatch: expected {SPL_SOURCE}, actual non-table\nrepair: declare {package} from the approved SPL Git source in root Cargo.toml" )) })?; + if table.contains_key("path") { + return Err(error(format!( + "SPL package {package} path dependency mismatch: expected absent, actual Cargo.toml:{}\nrepair: remove the local path route for {package} from Cargo.toml", + matches[0].0 + ))); + } + let version = table + .get("version") + .and_then(toml::Value::as_str) + .ok_or_else(|| { + error(format!( + "SPL package {package} version mismatch: expected declared version, actual missing\nrepair: declare {package} with the resolved version in root Cargo.toml" + )) + })?; let source = table .get("git") .and_then(toml::Value::as_str) @@ -146,6 +166,7 @@ fn validate_workspace(root: &toml::Value) -> Result> { ))); } revisions.insert(package.to_owned(), revision.to_owned()); + versions.insert(package.to_owned(), version.to_owned()); } if revisions["spl-core"] != revisions["spl-transport"] { return Err(error(format!( @@ -165,12 +186,18 @@ fn validate_workspace(root: &toml::Value) -> Result> { } } } - Ok(revisions) + Ok(WorkspacePins { + revisions, + versions, + }) } fn validate_member_dependencies(manifests: &[(PathBuf, toml::Value)]) -> Result<()> { + let mut inherited = BTreeMap::from([ + ("spl-core", BTreeSet::new()), + ("spl-transport", BTreeSet::new()), + ]); for (path, manifest) in manifests { - let mut inherited = BTreeMap::from([("spl-core", 0_usize), ("spl-transport", 0_usize)]); for table in dependency_tables(manifest) { for (key, dependency) in table { let package = package_identity(key, dependency); @@ -184,7 +211,6 @@ fn validate_member_dependencies(manifests: &[(PathBuf, toml::Value)]) -> Result< continue; }; if PACKAGES.contains(&package.as_str()) { - *inherited.get_mut(package.as_str()).unwrap() += 1; if details.contains_key("path") { return Err(error(format!( "SPL package {package} path dependency mismatch: expected absent, actual {}:{key}\nrepair: remove the local path route for {package} from {}", @@ -209,6 +235,10 @@ fn validate_member_dependencies(manifests: &[(PathBuf, toml::Value)]) -> Result< path.display() ))); } + inherited + .get_mut(package.as_str()) + .unwrap() + .insert(path.clone()); } else if let Some(source) = details.get("git").and_then(toml::Value::as_str) { return Err(error(format!( "Cargo package {package} Git source mismatch: expected no unapproved member Git dependency, actual {}:{key}={source}\nrepair: remove the unapproved Git dependency from {}", @@ -218,15 +248,23 @@ fn validate_member_dependencies(manifests: &[(PathBuf, toml::Value)]) -> Result< } } } - if path == Path::new("crates/solstone-linux/Cargo.toml") { - for package in PACKAGES { - if inherited[package] != 1 { - return Err(error(format!( - "SPL package {package} leaf inheritance mismatch: expected workspace = true, actual missing\nrepair: inherit {package} from root [workspace.dependencies] in {}", - path.display() - ))); - } - } + } + for package in PACKAGES { + let manifests = &inherited[package]; + if manifests.is_empty() { + return Err(error(format!( + "SPL package {package} leaf inheritance mismatch: expected workspace = true, actual missing\nrepair: inherit {package} from root [workspace.dependencies] in a workspace member" + ))); + } + if manifests.len() > 1 { + let actual = manifests + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(","); + return Err(error(format!( + "SPL package {package} leaf inheritance mismatch: expected exactly one inheriting workspace member, actual {actual}\nrepair: inherit {package} from root [workspace.dependencies] in only one workspace member" + ))); } } Ok(()) @@ -248,6 +286,7 @@ fn validate_overrides(root: &toml::Value) -> Result<()> { } } if let Some(replacements) = root.get("replace").and_then(toml::Value::as_table) { + // Cargo identifies the replaced package in the package-ID key; there is no rename key. for package_id in replacements.keys() { let package = package_id.split(':').next().unwrap_or(package_id); if PACKAGES.contains(&package) { @@ -265,12 +304,12 @@ fn validate_config(repo: &Path) -> Result<()> { if !repo.join(relative).exists() { continue; } - let config = read_toml(repo, relative, "workspace manifest")?; + let config = read_toml(repo, relative, "Cargo configuration")?; if let Some(sources) = config.get("source").and_then(toml::Value::as_table) { for (source, value) in sources { if let Some(replacement) = value.get("replace-with").and_then(toml::Value::as_str) { return Err(error(format!( - "SPL package spl-core Cargo source replacement mismatch: expected absent, actual {}:{source}->{replacement}\nrepair: remove the source replace-with route affecting spl-core", + "Cargo configuration source replacement mismatch: expected absent, actual {}:{source}->{replacement}\nrepair: remove the replace-with route so the SPL packages resolve from the declared workspace source", relative.display() ))); } @@ -298,7 +337,7 @@ fn validate_in_tree(repo: &Path) -> Result<()> { .filter(|value| !value.is_empty()) { let relative = PathBuf::from(String::from_utf8(bytes.to_vec()).map_err(|cause| error(format!("tracked manifest inventory mismatch: expected UTF-8 paths, actual {cause}\nrepair: restore the Git checkout before validating SPL pins")))?); - let manifest = read_toml(repo, &relative, "workspace manifest")?; + let manifest = read_toml(repo, &relative, "tracked manifest")?; if let Some(package) = manifest .get("package") .and_then(|value| value.get("name")) @@ -314,7 +353,7 @@ fn validate_in_tree(repo: &Path) -> Result<()> { Ok(()) } -fn validate_lock(repo: &Path, revisions: &BTreeMap) -> Result<()> { +fn validate_lock(repo: &Path, pins: &WorkspacePins) -> Result<()> { let text = fs::read_to_string(repo.join("Cargo.lock")).map_err(|cause| error(format!("lockfile parse mismatch: expected valid TOML, actual {cause}\nrepair: restore a valid Cargo.lock before validating the SPL pin")))?; let lock: toml::Value = toml::from_str(&text).map_err(|cause| error(format!("lockfile parse mismatch: expected valid TOML, actual {cause}\nrepair: restore a valid Cargo.lock before validating the SPL pin")))?; let records = lock @@ -338,6 +377,16 @@ fn validate_lock(repo: &Path, revisions: &BTreeMap) -> Result<() matches.len() ))); } + let lock_version = matches[0] + .get("version") + .and_then(toml::Value::as_str) + .unwrap_or("missing"); + if lock_version != pins.versions[package] { + return Err(error(format!( + "SPL package {package} version mismatch: expected {lock_version}, actual {}\nrepair: declare {package} at the resolved version in root Cargo.toml", + pins.versions[package] + ))); + } let source = matches[0] .get("source") .and_then(toml::Value::as_str) @@ -372,16 +421,16 @@ fn validate_lock(repo: &Path, revisions: &BTreeMap) -> Result<() ))); } let query_revision = pairs[0].1; - if query_revision != revisions[package] { + if query_revision != pins.revisions[package] { return Err(error(format!( "SPL package {package} lockfile revision query mismatch: expected {}, actual {query_revision}\nrepair: regenerate Cargo.lock from the approved {package} workspace revision", - revisions[package] + pins.revisions[package] ))); } - if fragment != revisions[package] { + if fragment != pins.revisions[package] { return Err(error(format!( "SPL package {package} lockfile resolved revision mismatch: expected {}, actual {fragment}\nrepair: regenerate Cargo.lock so {package} resolves to the approved workspace revision", - revisions[package] + pins.revisions[package] ))); } } @@ -390,7 +439,7 @@ fn validate_lock(repo: &Path, revisions: &BTreeMap) -> Result<() pub fn validate_spl_pin(repo: &RepoRoot) -> Result<()> { let root = read_toml(repo.path(), Path::new("Cargo.toml"), "workspace manifest")?; - let revisions = validate_workspace(&root)?; + let pins = validate_workspace(&root)?; validate_overrides(&root)?; let members = root.get("workspace").and_then(|value| value.get("members")).and_then(toml::Value::as_array).ok_or_else(|| error("workspace manifest parse mismatch: expected valid TOML, actual Cargo.toml: missing workspace.members\nrepair: restore valid TOML in Cargo.toml".to_owned()))?; let mut manifests = Vec::new(); @@ -405,7 +454,7 @@ pub fn validate_spl_pin(repo: &RepoRoot) -> Result<()> { validate_member_dependencies(&manifests)?; validate_config(repo.path())?; validate_in_tree(repo.path())?; - validate_lock(repo.path(), &revisions) + validate_lock(repo.path(), &pins) } #[cfg(test)] diff --git a/crates/rust-release-manifest/tests/spl_pin.rs b/crates/rust-release-manifest/tests/spl_pin.rs index 0a6253b..f2be05b 100644 --- a/crates/rust-release-manifest/tests/spl_pin.rs +++ b/crates/rust-release-manifest/tests/spl_pin.rs @@ -96,6 +96,23 @@ fn root_revision(root: &Path, package: &str) -> String { .to_owned() } +fn root_version(root: &Path, package: &str) -> String { + let value: toml::Value = + toml::from_str(&fs::read_to_string(root.join("Cargo.toml")).unwrap()).unwrap(); + value["workspace"]["dependencies"][package]["version"] + .as_str() + .unwrap() + .to_owned() +} + +fn different_revision(revision: &str) -> String { + if revision == "a".repeat(40) { + "b".repeat(40) + } else { + "a".repeat(40) + } +} + fn package_block(lock: &str, package: &str) -> String { let marker = format!("[[package]]\nname = \"{package}\""); let start = lock.find(&marker).unwrap(); @@ -109,11 +126,7 @@ fn package_block(lock: &str, package: &str) -> String { fn spl_pin_accepts_consistent_different_revision() { let fixture = fixture(|root| { let old = root_revision(root, "spl-core"); - let new = if old == "a".repeat(40) { - "b".repeat(40) - } else { - "a".repeat(40) - }; + let new = different_revision(&old); for relative in ["Cargo.toml", "Cargo.lock"] { let path = root.join(relative); let text = fs::read_to_string(&path).unwrap(); @@ -146,6 +159,50 @@ fn spl_pin_rejects_unapproved_workspace_source() { ); } +#[test] +fn spl_pin_rejects_root_workspace_path_dependency() { + rejected( + |root| { + replace( + &root.join("Cargo.toml"), + "spl-core = {", + "spl-core = { path = \"local\",", + ) + }, + "repair: remove the local path route for spl-core from Cargo.toml", + ); +} + +#[test] +fn spl_pin_rejects_missing_workspace_version() { + rejected( + |root| { + let version = root_version(root, "spl-core"); + replace( + &root.join("Cargo.toml"), + &format!("version = \"{version}\", "), + "", + ); + }, + "repair: declare spl-core with the resolved version in root Cargo.toml", + ); +} + +#[test] +fn spl_pin_rejects_workspace_version_disagreeing_with_lock() { + rejected( + |root| { + let version = root_version(root, "spl-core"); + replace( + &root.join("Cargo.toml"), + &format!("version = \"{version}\""), + &format!("version = \"{version}.different\""), + ); + }, + "repair: declare spl-core at the resolved version in root Cargo.toml", + ); +} + #[test] fn spl_pin_rejects_non_rev_workspace_selector() { rejected( @@ -187,9 +244,10 @@ fn spl_pin_rejects_aliased_duplicate_workspace_declaration() { rejected( |root| { let revision = root_revision(root, "spl-core"); + let version = root_version(root, "spl-core"); let path = root.join("Cargo.toml"); let mut text = fs::read_to_string(&path).unwrap(); - text.push_str(&format!("\n[workspace.dependencies.spl_alias]\npackage = \"spl-core\"\nversion = \"0.1.0\"\ngit = \"https://github.com/solpbc/spl-rust\"\nrev = \"{revision}\"\n")); + text.push_str(&format!("\n[workspace.dependencies.spl_alias]\npackage = \"spl-core\"\nversion = \"{version}\"\ngit = \"https://github.com/solpbc/spl-rust\"\nrev = \"{revision}\"\n")); fs::write(path, text).unwrap(); }, "repair: declare spl-core once in root [workspace.dependencies]", @@ -201,11 +259,7 @@ fn spl_pin_rejects_different_workspace_revisions() { rejected( |root| { let old = root_revision(root, "spl-transport"); - let new = if old == "a".repeat(40) { - "b".repeat(40) - } else { - "a".repeat(40) - }; + let new = different_revision(&old); let path = root.join("Cargo.toml"); let text = fs::read_to_string(&path).unwrap(); let line = text @@ -236,10 +290,11 @@ fn spl_pin_rejects_leaf_keys_alongside_inheritance() { fn spl_pin_rejects_leaf_without_inheritance() { rejected( |root| { + let version = root_version(root, "spl-core"); replace( &root.join("crates/solstone-linux/Cargo.toml"), "spl-core.workspace = true", - "spl-core = \"0.1.0\"", + &format!("spl-core = \"{version}\""), ) }, "repair: inherit spl-core from root [workspace.dependencies] in crates/solstone-linux/Cargo.toml", @@ -247,14 +302,33 @@ fn spl_pin_rejects_leaf_without_inheritance() { } #[test] -fn spl_pin_rejects_missing_shipping_leaf() { +fn spl_pin_rejects_missing_workspace_consumer() { rejected( |root| { let path = root.join("crates/solstone-linux/Cargo.toml"); let text = fs::read_to_string(&path).unwrap(); fs::write(path, text.replace("spl-core.workspace = true\n", "")).unwrap(); }, - "repair: inherit spl-core from root [workspace.dependencies] in crates/solstone-linux/Cargo.toml", + "repair: inherit spl-core from root [workspace.dependencies] in a workspace member", + ); +} + +#[test] +fn spl_pin_rejects_duplicate_workspace_consumers() { + rejected( + |root| { + let path = root.join("crates/rust-release-manifest/Cargo.toml"); + let text = fs::read_to_string(&path).unwrap(); + fs::write( + path, + text.replace( + "[dev-dependencies]\n", + "[dev-dependencies]\nspl-core.workspace = true\n", + ), + ) + .unwrap(); + }, + "repair: inherit spl-core from root [workspace.dependencies] in only one workspace member", ); } @@ -336,7 +410,7 @@ fn spl_pin_rejects_wrong_lock_query_revision() { let block = package_block(&text, "spl-core"); let changed = block.replacen( &format!("rev={revision}"), - &format!("rev={}", "a".repeat(40)), + &format!("rev={}", different_revision(&revision)), 1, ); fs::write(path, text.replacen(&block, &changed, 1)).unwrap(); @@ -353,8 +427,11 @@ fn spl_pin_rejects_wrong_lock_resolved_revision() { let path = root.join("Cargo.lock"); let text = fs::read_to_string(&path).unwrap(); let block = package_block(&text, "spl-core"); - let changed = - block.replacen(&format!("#{revision}"), &format!("#{}", "a".repeat(40)), 1); + let changed = block.replacen( + &format!("#{revision}"), + &format!("#{}", different_revision(&revision)), + 1, + ); fs::write(path, text.replacen(&block, &changed, 1)).unwrap(); }, "repair: regenerate Cargo.lock so spl-core resolves to the approved workspace revision", @@ -391,9 +468,12 @@ fn spl_pin_rejects_crates_io_patch() { fn spl_pin_rejects_replace() { rejected( |root| { + let version = root_version(root, "spl-core"); let path = root.join("Cargo.toml"); let mut text = fs::read_to_string(&path).unwrap(); - text.push_str("\n[replace]\n\"spl-core:0.1.0\" = { path = \"local\" }\n"); + text.push_str(&format!( + "\n[replace]\n\"spl-core:{version}\" = {{ path = \"local\" }}\n" + )); fs::write(path, text).unwrap(); }, "repair: remove the spl-core replacement from root Cargo.toml", @@ -420,7 +500,7 @@ fn spl_pin_rejects_root_cargo_source_replacement() { fs::create_dir(root.join(".cargo")).unwrap(); fs::write(root.join(".cargo/config.toml"), "[source.crates-io]\nreplace-with = \"local\"\n[source.local]\ndirectory = \"vendor\"\n").unwrap(); }, - "repair: remove the source replace-with route affecting spl-core", + "repair: remove the replace-with route so the SPL packages resolve from the declared workspace source", ); } @@ -428,10 +508,11 @@ fn spl_pin_rejects_root_cargo_source_replacement() { fn spl_pin_rejects_tracked_in_tree_package() { rejected( |root| { + let version = root_version(root, "spl-core"); fs::create_dir(root.join("local-spl")).unwrap(); fs::write( root.join("local-spl/Cargo.toml"), - "[package]\nname = \"spl-core\"\nversion = \"0.1.0\"\n", + format!("[package]\nname = \"spl-core\"\nversion = \"{version}\"\n"), ) .unwrap(); }, -- 2.51.2