From 3c44b2198678bdcfd2fb396e470afd77f10fdb37 Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 13:34:50 +1100 Subject: [PATCH 1/8] explicitly panic on unknown errors during a validation callback. This prevents confusion between validation failures and actual errors --- crates/core/src/nucleus/actions/run_validation_callback.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/core/src/nucleus/actions/run_validation_callback.rs b/crates/core/src/nucleus/actions/run_validation_callback.rs index 7609270234..ee9b3af829 100644 --- a/crates/core/src/nucleus/actions/run_validation_callback.rs +++ b/crates/core/src/nucleus/actions/run_validation_callback.rs @@ -48,10 +48,12 @@ pub async fn run_validation_callback( String::from("JSON object does not match entry schema").into(), )) } else { - Err(ValidationError::Error(error_string.into())) + // an unknown error from the ribosome should panic rather than + // silently failing validation + panic!(error_string) } } - Err(error) => Err(ValidationError::Error(error.to_string().into())), + Err(error) => panic!(error.to_string()), // same here }; lax_send_sync( From 865142d34959c650fb3b639910e3656138cec638 Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 15:35:10 +1100 Subject: [PATCH 2/8] add verify function to dna with naive implementation --- crates/conductor_lib/src/conductor/base.rs | 4 ++- crates/core_types/src/dna/mod.rs | 32 +++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/conductor_lib/src/conductor/base.rs b/crates/conductor_lib/src/conductor/base.rs index d7bfbd24df..490f5bf9ff 100644 --- a/crates/conductor_lib/src/conductor/base.rs +++ b/crates/conductor_lib/src/conductor/base.rs @@ -1208,7 +1208,9 @@ impl Conductor { let mut f = File::open(file)?; let mut contents = String::new(); f.read_to_string(&mut contents)?; - Dna::try_from(JsonString::from_json(&contents)).map_err(|err| err.into()) + let dna: Dna = Dna::try_from(JsonString::from_json(&contents))?; + dna.verify()?; + Ok(dna) } /// Default KeyLoader that actually reads files from the filesystem diff --git a/crates/core_types/src/dna/mod.rs b/crates/core_types/src/dna/mod.rs index 653b71c171..1767f958f0 100644 --- a/crates/core_types/src/dna/mod.rs +++ b/crates/core_types/src/dna/mod.rs @@ -39,7 +39,7 @@ use crate::{ fn_declarations::{FnDeclaration, TraitFns}, }, entry::entry_type::EntryType, - error::{DnaError, HolochainError}, + error::{DnaError, HcResult, HolochainError}, }; use holochain_persistence_api::cas::content::{AddressableContent, Content}; @@ -257,6 +257,36 @@ impl Dna { .flatten() .collect() } + + // Check that all the zomes in the DNA have code with the required callbacks + pub fn verify(&self) -> HcResult<()> { + let errors: Vec = self + .zomes + .iter() + .map(|(zome_name, zome)| { + if zome.code.code.len() > 0 { + Ok(()) + } else { + Err(HolochainError::ErrorGeneric(format!( + "Zome {} has no code!", + zome_name + ))) + } + }) + .filter_map(|r| match r { + Ok(_) => None, + Err(e) => Some(e.to_owned()), + }) + .collect(); + if errors.len() == 0 { + Ok(()) + } else { + Err(HolochainError::ErrorGeneric(String::from(format!( + "invalid DNA: {:?}", + errors + )))) + } + } } impl Hash for Dna { From 17cd9a4428ae2927199c0b9ebc180e99d0bb3b95 Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 15:57:38 +1100 Subject: [PATCH 3/8] adds verify function to DNA --- crates/core_types/src/dna/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/core_types/src/dna/mod.rs b/crates/core_types/src/dna/mod.rs index 1767f958f0..e60f6e0783 100644 --- a/crates/core_types/src/dna/mod.rs +++ b/crates/core_types/src/dna/mod.rs @@ -259,11 +259,13 @@ impl Dna { } // Check that all the zomes in the DNA have code with the required callbacks + // TODO: Add more advanced checks that actually try and call required functions pub fn verify(&self) -> HcResult<()> { let errors: Vec = self .zomes .iter() .map(|(zome_name, zome)| { + // currently just check the zome has some code if zome.code.code.len() > 0 { Ok(()) } else { @@ -281,10 +283,10 @@ impl Dna { if errors.len() == 0 { Ok(()) } else { - Err(HolochainError::ErrorGeneric(String::from(format!( + Err(HolochainError::ErrorGeneric(format!( "invalid DNA: {:?}", errors - )))) + ))) } } } From 4b7b49564b4940d099aea7d4f53a0e1483057046 Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 16:05:55 +1100 Subject: [PATCH 4/8] f,t --- CHANGELOG-UNRELEASED.md | 1 + crates/cli/src/cli/package.rs | 12 ++++++------ crates/cli/src/main.rs | 11 +++++------ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/CHANGELOG-UNRELEASED.md b/CHANGELOG-UNRELEASED.md index 4dcc0a0303..7725fa6d9c 100644 --- a/CHANGELOG-UNRELEASED.md +++ b/CHANGELOG-UNRELEASED.md @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm * The sim2h switch-board server is now caching if a node is missing data and periodically checks back in. This makes it more resilient against unforseen problems like connection drops which otherwise could only be recovered through an explicit reconnection of the node. [#1834](https://github.com/holochain/holochain-rust/pull/1834) ### Changed +* DNA is now checked for invalid zome artifacts. Validation callbacks that fail unexpectedly will now panic rather than fail validation. `hc package` `--strip-meta` flag is now `--include-meta`. [#1838](https://github.com/holochain/holochain-rust/pull/1838) ### Deprecated diff --git a/crates/cli/src/cli/package.rs b/crates/cli/src/cli/package.rs index 8108283f17..9cda7c04de 100644 --- a/crates/cli/src/cli/package.rs +++ b/crates/cli/src/cli/package.rs @@ -56,15 +56,15 @@ fn hdk_version_compare(hdk_version: &HDKVersion, cargo_toml: &str) -> DefaultRes } struct Packager { - strip_meta: bool, + include_meta: bool, } impl Packager { - fn new(strip_meta: bool) -> Packager { - Packager { strip_meta } + fn new(include_meta: bool) -> Packager { + Packager { include_meta } } - pub fn package(strip_meta: bool, output: PathBuf, properties: Value) -> DefaultResult<()> { + pub fn package(include_meta: bool, output: PathBuf, properties: Value) -> DefaultResult<()> { // First, check whether they have `cargo` installed, since it will be needed for packaging // TODO: in the future, don't check for this here, since other build tools and languages // could be used @@ -81,7 +81,7 @@ impl Packager { return Ok(()); } - Packager::new(strip_meta).run(&output, properties) + Packager::new(include_meta).run(&output, properties) } fn run(&self, output: &PathBuf, mut properties: Value) -> DefaultResult<()> { @@ -295,7 +295,7 @@ impl Packager { } } - if !self.strip_meta { + if self.include_meta { if !meta_tree.is_empty() { meta_section.insert(META_TREE_SECTION_NAME.into(), meta_tree.into()); } diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 190bb3b173..6ffa050103 100755 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -41,8 +41,8 @@ enum Cli { /// Builds DNA source files into a single .dna.json DNA file Package { #[structopt(long)] - /// Strips all __META__ sections off the target bundle. Makes unpacking of the bundle impossible - strip_meta: bool, + /// Adds __META__ fields in the dna.json which allow it to be unpacked + include_meta: bool, #[structopt(long, short, parse(from_os_str))] output: Option, #[structopt(long, short)] @@ -167,7 +167,7 @@ fn run() -> HolochainResult<()> { match args { // If using default path, we'll create if necessary; otherwise, target dir must exist Cli::Package { - strip_meta, + include_meta, output, properties: properties_string, } => { @@ -182,9 +182,8 @@ fn run() -> HolochainResult<()> { .unwrap_or_else(|| Ok(json!({}))); match properties { - Ok(properties) => { - cli::package(strip_meta, output, properties).map_err(HolochainError::Default)? - } + Ok(properties) => cli::package(include_meta, output, properties) + .map_err(HolochainError::Default)?, Err(e) => { return Err(HolochainError::Default(format_err!( "Failed to parse properties argument as JSON: {:?}", From 98de7d6d6e42ed5246302811aa3b770d5af6c74a Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 16:29:04 +1100 Subject: [PATCH 5/8] adds back strip meta to keep test commands happy --- crates/cli/src/main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 6ffa050103..e03666ed15 100755 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -43,6 +43,9 @@ enum Cli { #[structopt(long)] /// Adds __META__ fields in the dna.json which allow it to be unpacked include_meta: bool, + #[structopt(long)] + /// Included for backward compatibility. Does nothing as stripping meta is now the default + strip_meta: bool, #[structopt(long, short, parse(from_os_str))] output: Option, #[structopt(long, short)] From ed3fe862a65eb134b8f5914c15b6491a55abc153 Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 16:34:48 +1100 Subject: [PATCH 6/8] adds simple test for dna verify --- crates/core_types/src/dna/mod.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/core_types/src/dna/mod.rs b/crates/core_types/src/dna/mod.rs index e60f6e0783..d365541934 100644 --- a/crates/core_types/src/dna/mod.rs +++ b/crates/core_types/src/dna/mod.rs @@ -453,6 +453,28 @@ pub mod tests { ); } + #[test] + fn test_dna_verify() { + let dna = test_dna(); + assert!(dna.verify().is_ok()) + } + + #[test] + fn test_dna_verify_fail() { + // should error because code is empty + let dna = Dna::try_from(JsonString::from_json( + r#"{ + "zomes": { + "my_zome": { + "code": {"code": ""} + } + } + }"#, + )) + .unwrap(); + assert!(dna.verify().is_err()) + } + static UNIT_UUID: &'static str = "00000000-0000-0000-0000-000000000000"; fn test_empty_dna() -> Dna { From d5cc3a7376cf9128c161d69b3c76370188a2dcaa Mon Sep 17 00:00:00 2001 From: Willem Olding Date: Thu, 7 Nov 2019 20:11:43 +1100 Subject: [PATCH 7/8] clean up --- crates/core_types/src/dna/mod.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/core_types/src/dna/mod.rs b/crates/core_types/src/dna/mod.rs index d365541934..6734824dcd 100644 --- a/crates/core_types/src/dna/mod.rs +++ b/crates/core_types/src/dna/mod.rs @@ -275,12 +275,9 @@ impl Dna { ))) } }) - .filter_map(|r| match r { - Ok(_) => None, - Err(e) => Some(e.to_owned()), - }) + .filter_map(|r| r.err()) .collect(); - if errors.len() == 0 { + if errors.is_empty() { Ok(()) } else { Err(HolochainError::ErrorGeneric(format!( From fcf5502a1d96e423435567ad6ce4ee1093608156 Mon Sep 17 00:00:00 2001 From: Nicolas Luck Date: Thu, 7 Nov 2019 14:07:42 +0100 Subject: [PATCH 8/8] Fix build --- crates/cli/src/main.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index e03666ed15..1b64f1a6af 100755 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -173,7 +173,9 @@ fn run() -> HolochainResult<()> { include_meta, output, properties: properties_string, + strip_meta, } => { + let _ = strip_meta; let output = if output.is_some() { output.unwrap() } else {