Repository: mozilla/cargo-vet
Pull request #367 (2022-12-23 18:45:07)

Relax parsing when importing remote audits
This will allow the audits files to still partially parse and provide audits when the cargo-vet version used upstream is newer than the local version in more cases. The goal is for this to avoid issues with organizations updating the version of cargo-vet they are using out-of-sync with one another.

This ended up requiring a few hacky things in order to avoid parsing issues around how `toml::Value` de-serialization works compared to normal toml string deserialization, and Spanned locations will be less accurate in remote code (however we don't need to report errors in that situation much, so I doubt it'll be an issue).

The rough approach taken is to perform basic de-serialization at the root level like normal, and deserialize each audit and criteria entry independently, so that a parsing or validation error only impacts that single audit/criteria entry, and the other entries can still be imported.


Patch:
diff --git a/src/format.rs b/src/format.rs
index 5eeb17e..069cd09 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -9,6 +9,7 @@ use std::path::PathBuf;
 use std::str::FromStr;
 
 use cargo_metadata::Version;
+use serde::de::Visitor;
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
 
 // Collections based on how we're using, so it's easier to swap them out.
@@ -135,6 +136,15 @@ pub struct AuditsFile {
     pub audits: AuditedDependencies,
 }
 
+/// Foreign audits.toml with unparsed entries and audits. Should have the same
+/// structure as `AuditsFile`, but with individual audits and criteria unparsed.
+#[derive(serde::Deserialize, Clone)]
+pub struct ForeignAuditsFile {
+    #[serde(default)]
+    pub criteria: SortedMap<CriteriaName, toml::Value>,
+    pub audits: SortedMap<PackageName, Vec<toml::Value>>,
+}
+
 /// Information on a Criteria
 #[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
 pub struct CriteriaEntry {
@@ -231,18 +241,34 @@ impl<'de> Deserialize<'de> for Delta {
     where
         D: Deserializer<'de>,
     {
-        let s = <&str>::deserialize(deserializer)?;
-        if let Some((from, to)) = s.split_once("->") {
-            Ok(Delta {
-                from: Some(Version::parse(from.trim()).map_err(de::Error::custom)?),
-                to: Version::parse(to.trim()).map_err(de::Error::custom)?,
-            })
-        } else {
-            Ok(Delta {
-                from: None,
-                to: Version::parse(s.trim()).map_err(de::Error::custom)?,
-            })
+        struct DeltaVisitor;
+
+        impl<'de> Visitor<'de> for DeltaVisitor {
+            type Value = Delta;
+
+            fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
+                f.write_str("version -> version delta")
+            }
+
+            fn visit_str<E>(self, string: &str) -> Result<Self::Value, E>
+            where
+                E: de::Error,
+            {
+                if let Some((from, to)) = string.split_once("->") {
+                    Ok(Delta {
+                        from: Some(Version::parse(from.trim()).map_err(de::Error::custom)?),
+                        to: Version::parse(to.trim()).map_err(de::Error::custom)?,
+                    })
+                } else {
+                    Ok(Delta {
+                        from: None,
+                        to: Version::parse(string.trim()).map_err(de::Error::custom)?,
+                    })
+                }
+            }
         }
+
+        deserializer.deserialize_str(DeltaVisitor)
     }
 }
 
diff --git a/src/serialization.rs b/src/serialization.rs
index ca956b3..2a41131 100644
--- a/src/serialization.rs
+++ b/src/serialization.rs
@@ -386,9 +386,24 @@ where
     Ok(toml_document)
 }
 
+/// Deserialize the given data structure from a toml::Value, without falling
+/// over due to Spanned failing to parse.
+pub fn parse_from_value<T>(value: toml::Value) -> Result<T, toml::de::Error>
+where
+    T: for<'a> Deserialize<'a>,
+{
+    spanned::DISABLE_SPANNED_DESERIALIZATION.with(|disabled| {
+        let prev = disabled.replace(true);
+        let rv = T::deserialize(value);
+        disabled.set(prev);
+        rv
+    })
+}
+
 pub mod spanned {
     use std::{
         borrow::Borrow,
+        cell::Cell,
         cmp::Ordering,
         fmt::{self, Display},
         hash::{Hash, Hasher},
@@ -396,11 +411,16 @@ pub mod spanned {
     };
 
     use miette::SourceSpan;
-    use serde::{ser, Deserialize};
+    use serde::{de, ser};
+
+    thread_local! {
+        /// Hack to work around `toml::Spanned` failing to be deserialized when
+        /// used with the `toml::Value` deserializer.
+        pub(super) static DISABLE_SPANNED_DESERIALIZATION: Cell<bool> = Cell::new(false);
+    }
 
     /// A spanned value, indicating the range at which it is defined in the source.
-    #[derive(Clone, Default, Deserialize)]
-    #[serde(from = "toml::Spanned<T>")]
+    #[derive(Clone, Default)]
     pub struct Spanned<T> {
         start: usize,
         end: usize,
@@ -572,6 +592,19 @@ pub mod spanned {
         }
     }
 
+    impl<'de, T: de::Deserialize<'de>> de::Deserialize<'de> for Spanned<T> {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: de::Deserializer<'de>,
+        {
+            Ok(if DISABLE_SPANNED_DESERIALIZATION.with(|d| d.get()) {
+                T::deserialize(deserializer)?.into()
+            } else {
+                toml::Spanned::<T>::deserialize(deserializer)?.into()
+            })
+        }
+    }
+
     impl<T: ser::Serialize> ser::Serialize for Spanned<T> {
         fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where
diff --git a/src/storage.rs b/src/storage.rs
index f40f93b..ee8ed9c 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -28,14 +28,15 @@ use crate::{
     },
     flock::{FileLock, Filesystem},
     format::{
-        AuditKind, AuditsFile, CommandHistory, ConfigFile, CriteriaName, Delta, DiffCache,
-        DiffStat, FastMap, FetchCommand, ImportName, ImportsFile, MetaConfig, PackageStr,
-        SortedMap, SAFE_TO_DEPLOY, SAFE_TO_RUN,
+        self, AuditEntry, AuditKind, AuditedDependencies, AuditsFile, CommandHistory, ConfigFile,
+        CriteriaEntry, CriteriaName, Delta, DiffCache, DiffStat, FastMap, FetchCommand,
+        ForeignAuditsFile, ImportName, ImportsFile, MetaConfig, PackageName, PackageStr, SortedMap,
+        SAFE_TO_DEPLOY, SAFE_TO_RUN,
     },
     network::Network,
     out::{indeterminate_spinner, progress_bar, IncProgressOnDrop},
     resolver::{self, Conclusion},
-    serialization::{spanned::Spanned, to_formatted_toml},
+    serialization::{parse_from_value, spanned::Spanned, to_formatted_toml},
     Config, PartialConfig,
 };
 
@@ -257,6 +258,19 @@ impl Store {
         fetched_audits: Vec<(ImportName, AuditsFile)>,
         allow_criteria_changes: bool,
     ) -> Result<Self, CriteriaChangeErrors> {
+        // For extra checking of import serialization, serialize the fetched
+        // audits, convert them to a ForeignAuditsFile, and then deserialize
+        // them back into an AuditsFile to ensure they round-trip through
+        // that process successfully, as the parsing codepaths are different.
+        for (_, original_file) in &fetched_audits {
+            let orig_toml = to_formatted_toml(original_file).unwrap().to_string();
+            let result = foreign_audit_file_to_local(toml::de::from_str(&orig_toml).unwrap());
+            assert_eq!(result.ignored_criteria, Vec::<String>::new());
+            assert_eq!(result.ignored_audits, Vec::<String>::new());
+            let new_toml = to_formatted_toml(&result.audit_file).unwrap().to_string();
+            assert_eq!(new_toml, orig_toml);
+        }
+
         let live_imports =
             process_imported_audits(fetched_audits, &config, &imports, allow_criteria_changes)?;
         Ok(Self {
@@ -827,7 +841,13 @@ async fn fetch_imported_audit(
     let audit_bytes = network.download(parsed_url).await?;
     let audit_string = String::from_utf8(audit_bytes).map_err(LoadTomlError::from)?;
     let audit_source = SourceFile::new(name, audit_string.clone());
-    let mut audit_file: AuditsFile = toml::de::from_str(&audit_string)
+
+    // Attempt to parse each criteria and audit independently, to allow
+    // recovering from parsing or validation errors on a per-entry basis when
+    // importing audits. This reduces the risk of an upstream vendor adopting a
+    // new cargo-vet feature breaking projects still using an older version of
+    // cargo-vet.
+    let foreign_audit_file: ForeignAuditsFile = toml::de::from_str(&audit_string)
         .map_err(|error| {
             let (line, col) = error.line_col().unwrap_or((0, 0));
             TomlParseError {
@@ -837,6 +857,35 @@ async fn fetch_imported_audit(
             }
         })
         .map_err(LoadTomlError::from)?;
+    let ForeignAuditFileToLocalResult {
+        mut audit_file,
+        ignored_criteria,
+        ignored_audits,
+    } = foreign_audit_file_to_local(foreign_audit_file);
+    if !ignored_criteria.is_empty() {
+        warn!(
+            "Ignored {} invalid criteria entries when importing from '{}'\n\
+            These criteria may have been made with a more recent version of cargo-vet",
+            ignored_criteria.len(),
+            name
+        );
+        info!(
+            "The following criteria were ignored when importing from '{}': {:?}",
+            name, ignored_criteria
+        );
+    }
+    if !ignored_audits.is_empty() {
+        warn!(
+            "Ignored {} invalid audits when importing from '{}'\n\
+            These audits may have been made with a more recent version of cargo-vet",
+            ignored_audits.len(),
+            name
+        );
+        info!(
+            "Audits for the following packages were ignored when importing from '{}': {:?}",
+            name, ignored_audits
+        );
+    }
 
     // Eagerly fetch all descriptions for criteria in the imported audits file,
     // and store them inline. We'll error out if any of these descriptions are
@@ -880,6 +929,120 @@ async fn fetch_imported_audit(
     Ok(audit_file)
 }
 
+pub(crate) struct ForeignAuditFileToLocalResult {
+    pub audit_file: AuditsFile,
+    pub ignored_criteria: Vec<CriteriaName>,
+    pub ignored_audits: Vec<PackageName>,
+}
+
+fn is_known_criteria(valid_criteria: &[CriteriaName], criteria_name: &CriteriaName) -> bool {
+    criteria_name == format::SAFE_TO_RUN
+        || criteria_name == format::SAFE_TO_DEPLOY
+        || valid_criteria.contains(criteria_name)
+}
+
+/// Convert a foreign audits file into a local audits file, ignoring any entries
+/// which could not be interpreted, due to issues such as being created with a
+/// newer version of cargo-vet.
+pub(crate) fn foreign_audit_file_to_local(
+    foreign_audit_file: ForeignAuditsFile,
+) -> ForeignAuditFileToLocalResult {
+    let mut ignored_criteria = Vec::new();
+    let mut criteria: SortedMap<CriteriaName, CriteriaEntry> = foreign_audit_file
+        .criteria
+        .into_iter()
+        .filter_map(|(criteria, value)| match parse_imported_criteria(value) {
+            Some(entry) => Some((criteria, entry)),
+            None => {
+                ignored_criteria.push(criteria);
+                None
+            }
+        })
+        .collect();
+    let valid_criteria: Vec<CriteriaName> = criteria.keys().cloned().collect();
+
+    // Remove any unknown criteria from implies sets, to ensure we don't run
+    // into errors later on in the resolver.
+    for entry in criteria.values_mut() {
+        entry
+            .implies
+            .retain(|criteria_name| is_known_criteria(&valid_criteria, criteria_name));
+    }
+
+    let mut ignored_audits = Vec::new();
+    let audits: AuditedDependencies = foreign_audit_file
+        .audits
+        .into_iter()
+        .map(|(package, audits)| {
+            let parsed: Vec<_> = audits
+                .into_iter()
+                .filter_map(|value| match parse_imported_audit(&valid_criteria, value) {
+                    Some(audit) => Some(audit),
+                    None => {
+                        ignored_audits.push(package.clone());
+                        None
+                    }
+                })
+                .collect();
+            (package, parsed)
+        })
+        .filter(|(_, audits)| !audits.is_empty())
+        .collect();
+
+    ForeignAuditFileToLocalResult {
+        audit_file: AuditsFile { criteria, audits },
+        ignored_criteria,
+        ignored_audits,
+    }
+}
+
+/// Parse an unparsed criteria entry, validating and returning it.
+fn parse_imported_criteria(value: toml::Value) -> Option<CriteriaEntry> {
+    parse_from_value(value)
+        .map_err(|err| info!("imported criteria parsing failed due to {err}"))
+        .ok()
+}
+
+/// Parse an unparsed audit entry, validating and returning it.
+fn parse_imported_audit(valid_criteria: &[CriteriaName], value: toml::Value) -> Option<AuditEntry> {
+    let mut audit: AuditEntry = parse_from_value(value)
+        .map_err(|err| info!("imported audit parsing failed due to {err}"))
+        .ok()?;
+
+    // Remove any unrecognized criteria to avoid later errors caused by being
+    // unable to find criteria, and ignore the entry if it names no known
+    // criteria.
+    audit
+        .criteria
+        .retain(|criteria_name| is_known_criteria(valid_criteria, criteria_name));
+    if audit.criteria.is_empty() {
+        info!("imported audit parsing failed due to no known criteria");
+        return None;
+    }
+
+    // If any dependency criteria name an unknown criteria, ignore the audit.
+    match &audit.kind {
+        AuditKind::Delta {
+            dependency_criteria,
+            ..
+        }
+        | AuditKind::Full {
+            dependency_criteria,
+            ..
+        } => {
+            for criteria_name in dependency_criteria.values().flatten() {
+                if !is_known_criteria(valid_criteria, criteria_name) {
+                    info!("imported audit parsing failed due to unknown dependency criteria: {criteria_name}");
+                    return None;
+                }
+            }
+        }
+        _ => {}
+    }
+
+    Some(audit)
+}
+
 /// A Registry in CARGO_HOME (usually the crates.io one)
 pub struct CargoRegistry {
     /// The queryable index


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 71564fb..4f9f874 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -859,6 +859,148 @@ fn existing_peer_updated_description() {
     insta::assert_snapshot!("existing_peer_updated_description", format!("{:?}", error));
 }
 
+#[test]
+fn foreign_audit_file_to_local() {
+    let _enter = TEST_RUNTIME.enter();
+
+    let foreign_audit_file = crate::format::ForeignAuditsFile {
+        criteria: [
+            (
+                "example".to_string(),
+                toml::toml! {
+                    description = "Example criteria description"
+                },
+            ),
+            (
+                "example2".to_string(),
+                toml::toml! {
+                    implies = "unknown-criteria"
+                    description = "example2"
+                },
+            ),
+            (
+                "example3".to_string(),
+                toml::toml! {
+                    description = "example2"
+                    implies = ["safe-to-deploy", "will-not-parse"]
+                    unknown = "ignored unknown field"
+                },
+            ),
+            (
+                "will-not-parse".to_string(),
+                toml::toml! {
+                    implies = [{ not = "a string" }]
+                    description = "will be ignored"
+                },
+            ),
+        ]
+        .into_iter()
+        .collect(),
+        audits: [
+            (
+                "crate-a".to_string(),
+                vec![
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "10.0.0"
+                        notes = "should parse correctly"
+                    },
+                    toml::toml! {
+                        criteria = "unknown-criteria"
+                        version = "10.0.0"
+                        notes = "will be removed"
+                    },
+                    toml::toml! {
+                        criteria = "example"
+                        version = "invalid"
+                        notes = "will be removed"
+                    },
+                    toml::toml! {
+                        criteria = "will-not-parse"
+                        version = "10.0.0"
+                        notes = "will be removed"
+                    },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "10.0.0"
+                        dependency-criteria = { foo = "unknown-criteria" }
+                        notes = "will be removed"
+                    },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "10.0.0"
+                        dependency-criteria = { foo = ["example", "unknown-criteria"] }
+                        notes = "will be removed"
+                    },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "10.0.0"
+                        dependency-criteria = { foo = "example" }
+                        notes = "should parse correctly"
+                    },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        violation = "invalid"
+                        notes = "will be removed"
+                    },
+                ],
+            ),
+            (
+                "crate-b".to_string(),
+                vec![toml::toml! {
+                    criteria = "example"
+                    version = "invalid"
+                    notes = "will be removed, along with the entire crate"
+                }],
+            ),
+            (
+                "crate-c".to_string(),
+                vec![
+                    toml::toml! {
+                        criteria = "example2"
+                        version = "10.0.0"
+                        notes = "will not be removed"
+                    },
+                    toml::toml! {
+                        criteria = ["example2", "example3"]
+                        version = "10.0.0"
+                        notes = "will not be removed"
+                    },
+                    toml::toml! {
+                        criteria = "example2"
+                        delta = "1.0.0 -> 10.0.0"
+                        notes = "will not be removed"
+                    },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        violation = "=5.0.0"
+                        notes = "will not be removed"
+                    },
+                ],
+            ),
+        ]
+        .into_iter()
+        .collect(),
+    };
+
+    let mut result = crate::storage::foreign_audit_file_to_local(foreign_audit_file);
+    result.ignored_criteria.sort();
+    result.ignored_audits.sort();
+
+    assert_eq!(result.ignored_criteria, &["will-not-parse"]);
+    assert_eq!(
+        result.ignored_audits,
+        &["crate-a", "crate-a", "crate-a", "crate-a", "crate-a", "crate-a", "crate-b"]
+    );
+
+    insta::assert_snapshot!(
+        "foreign_audit_file_to_local",
+        crate::serialization::to_formatted_toml(&result.audit_file)
+            .unwrap()
+            .to_string()
+    );
+}
+
 // Other tests worth adding:
 //
 // - used edges with dependency-criteria should cause criteria to be imported
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index 04360ba..e5e077e 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -75,12 +75,10 @@ lazy_static::lazy_static! {
         })).expect("Failed to initialize error handler");
 
         tracing_subscriber::fmt::fmt()
-            .with_max_level(tracing::level_filters::LevelFilter::OFF)
-            // Toggle this on for tracing in tests
-            // .with_max_level(tracing::level_filters::LevelFilter::TRACE)
+            .with_max_level(tracing::level_filters::LevelFilter::TRACE)
             .with_target(false)
             .without_time()
-            .with_writer(std::io::stderr)
+            .with_writer(tracing_subscriber::fmt::writer::TestWriter::new())
             .init();
 
         tokio::runtime::Runtime::new().unwrap()
diff --git a/src/tests/snapshots/cargo_vet__tests__import__foreign_audit_file_to_local.snap b/src/tests/snapshots/cargo_vet__tests__import__foreign_audit_file_to_local.snap
new file mode 100644
index 0000000..e477811
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__foreign_audit_file_to_local.snap
@@ -0,0 +1,46 @@
+---
+source: src/tests/import.rs
+expression: "crate::serialization::to_formatted_toml(&result.audit_file).unwrap().to_string()"
+---
+
+[criteria.example]
+description = "Example criteria description"
+
+[criteria.example2]
+description = "example2"
+
+[criteria.example3]
+description = "example2"
+implies = "safe-to-deploy"
+
+[[audits.crate-a]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+notes = "should parse correctly"
+
+[[audits.crate-a]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+dependency-criteria = { foo = "example" }
+notes = "should parse correctly"
+
+[[audits.crate-c]]
+criteria = "example2"
+version = "10.0.0"
+notes = "will not be removed"
+
+[[audits.crate-c]]
+criteria = ["example2", "example3"]
+version = "10.0.0"
+notes = "will not be removed"
+
+[[audits.crate-c]]
+criteria = "example2"
+delta = "1.0.0 -> 10.0.0"
+notes = "will not be removed"
+
+[[audits.crate-c]]
+criteria = "safe-to-deploy"
+violation = "=5.0.0"
+notes = "will not be removed"
+