From 97cf57e7e55cc253f2cc73e8028074fcae97afdd Mon Sep 17 00:00:00 2001 From: Ashok Menon Date: Tue, 13 Aug 2024 20:50:28 +0100 Subject: [PATCH] fix(verify-source): Detect published-at = 0x0 ## Description Detect when the `published-at` field in `Move.toml` or `Move.lock` has been explicitly set to `0x0` and treat that as if it was not set at all. This is not commonly done by people, but it happens in our test set-up. This also required converting the field into an `ObjectID` earlier in the pipeline, which introduced some further changes in the codebase. ## Test plan ``` sui-source-validation$ cargo nextest run ``` --- crates/sui-move-build/src/lib.rs | 2 +- crates/sui-package-management/src/lib.rs | 91 ++++++++++++++++-------- crates/sui/tests/cli_tests.rs | 2 +- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/crates/sui-move-build/src/lib.rs b/crates/sui-move-build/src/lib.rs index 9584bcd4998fb..517808b39f104 100644 --- a/crates/sui-move-build/src/lib.rs +++ b/crates/sui-move-build/src/lib.rs @@ -645,7 +645,7 @@ pub struct PackageDependencies { pub invalid: BTreeMap, /// Set of dependencies that have conflicting `published-at` addresses. The key refers to /// the package, and the tuple refers to the address in the (Move.lock, Move.toml) respectively. - pub conflicting: BTreeMap, + pub conflicting: BTreeMap, } /// Partition packages in `resolution_graph` into one of four groups: diff --git a/crates/sui-package-management/src/lib.rs b/crates/sui-package-management/src/lib.rs index e472578c7746a..3cd7ddd025d47 100644 --- a/crates/sui-package-management/src/lib.rs +++ b/crates/sui-package-management/src/lib.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use anyhow::{bail, Context}; +use std::collections::HashMap; use std::fs::File; use std::path::{Path, PathBuf}; use std::str::FromStr; @@ -29,8 +30,8 @@ pub enum PublishedAtError { Invalid(String), NotPresent, Conflict { - id_lock: String, - id_manifest: String, + id_lock: ObjectID, + id_manifest: ObjectID, }, } @@ -140,45 +141,42 @@ pub fn resolve_published_id( ) -> Result { // Look up a valid `published-at` in the `Move.toml` first, which we'll // return if the Move.lock does not manage addresses. - let published_id_in_manifest = match published_at_property(package) { - Ok(v) => Some(v), - Err(PublishedAtError::NotPresent) => None, - Err(e) => return Err(e), // An existing but invalid `published-at` in `Move.toml` should fail early. - }; + let published_id_in_manifest = manifest_published_at(package); + + match published_id_in_manifest { + Ok(_) | Err(PublishedAtError::NotPresent) => { /* nop */ } + Err(e) => { + return Err(e); + } + } let lock = package.package_path.join(SourcePackageLayout::Lock.path()); let Ok(mut lock_file) = File::open(lock.clone()) else { - return match published_id_in_manifest { - Some(v) => { - ObjectID::from_str(v.as_str()).map_err(|_| PublishedAtError::Invalid(v.to_owned())) - } - None => Err(PublishedAtError::NotPresent), - }; + return published_id_in_manifest; }; - let managed_packages = ManagedPackage::read(&mut lock_file).ok(); + // Find the environment and ManagedPackage data for this chain_id. - let id_in_lock_for_chain_id = managed_packages.and_then(|m| { - let chain_id = chain_id.as_ref()?; - m.into_iter() - .find_map(|(_, v)| (v.chain_id == *chain_id).then_some(v.latest_published_id)) - }); - - let package_id = match (id_in_lock_for_chain_id, published_id_in_manifest) { - (Some(id_lock), Some(id_manifest)) if id_lock != id_manifest => { - return Err(PublishedAtError::Conflict { + let id_in_lock_for_chain_id = + lock_published_at(ManagedPackage::read(&mut lock_file).ok(), chain_id.as_ref()); + + match (id_in_lock_for_chain_id, published_id_in_manifest) { + (Ok(id_lock), Ok(id_manifest)) if id_lock != id_manifest => { + Err(PublishedAtError::Conflict { id_lock, id_manifest, }) } - (Some(id_lock), _) => id_lock, - (None, Some(id_manifest)) => id_manifest, /* No info in Move.lock: Fall back to manifest */ - _ => return Err(PublishedAtError::NotPresent), /* Neither in Move.toml nor Move.lock */ - }; - ObjectID::from_str(package_id.as_str()) - .map_err(|_| PublishedAtError::Invalid(package_id.to_owned())) + + (Ok(id), _) | (_, Ok(id)) => Ok(id), + + // We return early (above) if we failed to read the ID from the manifest for some reason + // other than it not being present, so at this point, we can defer to whatever error came + // from the lock file (Ok case is handled above). + (from_lock, Err(_)) => from_lock, + } } -fn published_at_property(package: &Package) -> Result { +fn manifest_published_at(package: &Package) -> Result { let Some(value) = package .source_package .package @@ -187,5 +185,36 @@ fn published_at_property(package: &Package) -> Result else { return Err(PublishedAtError::NotPresent); }; - Ok(value.to_string()) + + let id = + ObjectID::from_str(value.as_str()).map_err(|_| PublishedAtError::Invalid(value.clone()))?; + + if id == ObjectID::ZERO { + Err(PublishedAtError::NotPresent) + } else { + Ok(id) + } +} + +fn lock_published_at( + lock: Option>, + chain_id: Option<&String>, +) -> Result { + let (Some(lock), Some(chain_id)) = (lock, chain_id) else { + return Err(PublishedAtError::NotPresent); + }; + + let managed_package = lock + .into_values() + .find(|v| v.chain_id == *chain_id) + .ok_or(PublishedAtError::NotPresent)?; + + let id = ObjectID::from_str(managed_package.latest_published_id.as_str()) + .map_err(|_| PublishedAtError::Invalid(managed_package.latest_published_id.clone()))?; + + if id == ObjectID::ZERO { + Err(PublishedAtError::NotPresent) + } else { + Ok(id) + } } diff --git a/crates/sui/tests/cli_tests.rs b/crates/sui/tests/cli_tests.rs index cc0f05e41e3af..00c2f5034172e 100644 --- a/crates/sui/tests/cli_tests.rs +++ b/crates/sui/tests/cli_tests.rs @@ -2080,7 +2080,7 @@ async fn test_package_management_on_upgrade_command_conflict() -> Result<(), any let err_string = err_string.replace(&package.object_id().to_string(), ""); let expect = expect![[r#" - Conflicting published package address: `Move.toml` contains published-at address 0xbad but `Move.lock` file contains published-at address . You may want to: + Conflicting published package address: `Move.toml` contains published-at address 0x0000000000000000000000000000000000000000000000000000000000000bad but `Move.lock` file contains published-at address . You may want to: - delete the published-at address in the `Move.toml` if the `Move.lock` address is correct; OR - update the `Move.lock` address using the `sui manage-package` command to be the same as the `Move.toml`; OR