Repository: mozilla/cargo-vet
Pull request #409 (2023-02-15 23:11:21)

Correctly handle importing audits for multiple versions
When we changed the way that imports were handled in #399, the new minimal imports logic didn't handle multiple versions of crates with the same name properly, meaning that we'd only import the required audits for one of the versions rather than all of them.

This patch adds a test to ensure that doesn't happen again, and fixes the underlying issue.


Patch:
diff --git a/book/src/install.md b/book/src/install.md
index 1d8fb68..4bc5a11 100644
--- a/book/src/install.md
+++ b/book/src/install.md
@@ -3,7 +3,7 @@
 Installing `cargo vet` can be done through Cargo:
 
 ```
-$ cargo install --locked cargo-vet
+$ cargo install cargo-vet
 ```
 
 Afterwards you can confirm that it's installed via:
diff --git a/src/resolver.rs b/src/resolver.rs
index da30585..5cca7c2 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -1832,6 +1832,12 @@ impl<'a> ResolveReport<'a> {
         false
     }
 
+    pub fn collect_required_entries(
+        &self,
+    ) -> SortedMap<PackageStr<'a>, Option<SortedSet<RequiredEntry>>> {
+        collect_required_entries(&self.graph, &self.results)
+    }
+
     pub fn compute_suggest(
         &self,
         cfg: &Config,
@@ -2489,6 +2495,38 @@ impl FailForViolationConflict {
     }
 }
 
+fn collect_required_entries<'a>(
+    graph: &DepGraph<'a>,
+    results: &[Option<ResolveResult<'_>>],
+) -> SortedMap<PackageStr<'a>, Option<SortedSet<RequiredEntry>>> {
+    let mut required_entries = SortedMap::new();
+    for (package, result) in graph.nodes.iter().zip(results.iter()) {
+        // We never need entries for non-third-party crates.
+        if !package.is_third_party {
+            continue;
+        }
+
+        // If any package with the name failed to vet, assume every package
+        // failed to vet, otherwise combine all sets together.
+        if let Some(ResolveResult {
+            required_entries: Some(req),
+            ..
+        }) = result
+        {
+            match required_entries
+                .entry(package.name)
+                .or_insert(Some(SortedSet::new()))
+            {
+                Some(entries) => entries.extend(req.iter().cloned()),
+                None => {}
+            }
+        } else {
+            required_entries.insert(package.name, None);
+        }
+    }
+    required_entries
+}
+
 /// Ensure that vet will pass on the store by adding new exemptions, as well as
 /// removing existing ones which are no longer necessary. Regenerating
 /// exemptions generally tries to avoid unnecessary changes, and to continue to
@@ -2520,7 +2558,8 @@ pub fn regenerate_exemptions(
     if !allow_new_exemptions {
         let (results, conclusion) = resolve_audits(&graph, store, &criteria_mapper, &requirements);
         if !matches!(conclusion, Conclusion::Success(_)) || cfg.cli.no_minimize_exemptions {
-            store.imports = store.get_updated_imports_file(&graph, &results);
+            store.imports =
+                store.get_updated_imports_file(&collect_required_entries(&graph, &results));
             return Ok(());
         }
     }
@@ -2578,14 +2617,16 @@ pub fn regenerate_exemptions(
                 // Also force an update even if we're seeing a violation
                 // conflict (to save the potentially-newly-imported violation),
                 // although our caller is unlikely to commit these changes.
-                store.imports = store.get_updated_imports_file(&graph, &results);
+                store.imports =
+                    store.get_updated_imports_file(&collect_required_entries(&graph, &results));
                 return Err(RegenerateExemptionsError::ViolationConflict);
             }
             Conclusion::Success(..) => {
                 // We succeeded! Whatever exemptions we've recorded so-far are
                 // suficient, so we're done. Record any imports which ended up
                 // being required for the vet to pass.
-                store.imports = store.get_updated_imports_file(&graph, &results);
+                store.imports =
+                    store.get_updated_imports_file(&collect_required_entries(&graph, &results));
                 return Ok(());
             }
         };
diff --git a/src/storage.rs b/src/storage.rs
index 4b52346..0de4d52 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -33,12 +33,12 @@ use crate::{
         CratesAPICrate, CratesPublisher, CriteriaEntry, CriteriaName, Delta, DiffCache, DiffStat,
         FastMap, FastSet, FetchCommand, ForeignAuditsFile, ImportName, ImportsFile, MetaConfig,
         PackageName, PackageStr, PublisherCache, PublisherCacheEntry, PublisherCacheUser,
-        PublisherCacheVersion, SortedMap, VetVersion, WildcardAudits, WildcardEntry,
+        PublisherCacheVersion, SortedMap, SortedSet, VetVersion, WildcardAudits, WildcardEntry,
         SAFE_TO_DEPLOY, SAFE_TO_RUN,
     },
     network::Network,
     out::{indeterminate_spinner, progress_bar, IncProgressOnDrop},
-    resolver,
+    resolver::{self, RequiredEntry},
     serialization::{parse_from_value, spanned::Spanned, to_formatted_toml},
     Config, PackageExt, PartialConfig, CARGO_ENV,
 };
@@ -657,8 +657,7 @@ impl Store {
     #[must_use]
     pub fn get_updated_imports_file(
         &self,
-        graph: &resolver::DepGraph<'_>,
-        results: &[Option<resolver::ResolveResult>],
+        required_entries: &SortedMap<PackageStr<'_>, Option<SortedSet<RequiredEntry>>>,
     ) -> ImportsFile {
         let Some(live_imports) = &self.live_imports else {
             // We're locked, so can't update anything.
@@ -677,28 +676,12 @@ impl Store {
                 wildcard_audits: SortedMap::new(),
                 audits: SortedMap::new(),
             };
-            for (pkgidx, result) in results.iter().enumerate() {
-                let package = &graph.nodes[pkgidx];
-
-                // Don't import audits for first-party packages.
-                if !package.is_third_party {
-                    continue;
-                }
-
-                // If the audit succeeded, get the set of required audits.
-                let required_entries = match result {
-                    Some(resolver::ResolveResult {
-                        required_entries: Some(required_entries),
-                        ..
-                    }) => Some(required_entries),
-                    _ => None,
-                };
-
+            for (&pkgname, required_entries) in required_entries {
                 // Filter the set of audits from the live audits file to
                 // only required audits.
                 let audits = live_audits_file
                     .audits
-                    .get(package.name)
+                    .get(pkgname)
                     .map(|v| &v[..])
                     .unwrap_or(&[])
                     .iter()
@@ -730,7 +713,7 @@ impl Store {
 
                 let wildcard_audits = live_audits_file
                     .wildcard_audits
-                    .get(package.name)
+                    .get(pkgname)
                     .map(|v| &v[..])
                     .unwrap_or(&[])
                     .iter()
@@ -757,14 +740,12 @@ impl Store {
 
                 // Record audits in the new audits file, if there are any.
                 if !audits.is_empty() {
-                    new_audits_file
-                        .audits
-                        .insert(package.name.to_owned(), audits);
+                    new_audits_file.audits.insert(pkgname.to_owned(), audits);
                 }
                 if !wildcard_audits.is_empty() {
                     new_audits_file
                         .wildcard_audits
-                        .insert(package.name.to_owned(), wildcard_audits);
+                        .insert(pkgname.to_owned(), wildcard_audits);
                 }
             }
             new_imports
@@ -772,26 +753,10 @@ impl Store {
                 .insert(import_name.to_owned(), new_audits_file);
         }
 
-        for (pkgidx, result) in results.iter().enumerate() {
-            let package = &graph.nodes[pkgidx];
-
-            // Don't import publisher information for first-party packages.
-            if !package.is_third_party {
-                continue;
-            }
-
-            // If the audit succeeded, get the set of required audits.
-            let required_entries = match result {
-                Some(resolver::ResolveResult {
-                    required_entries: Some(required_entries),
-                    ..
-                }) => Some(required_entries),
-                _ => None,
-            };
-
+        for (&pkgname, required_entries) in required_entries {
             let publishers = live_imports
                 .publisher
-                .get(package.name)
+                .get(pkgname)
                 .map(|v| &v[..])
                 .unwrap_or(&[])
                 .iter()
@@ -814,9 +779,7 @@ impl Store {
                 })
                 .collect::<Vec<_>>();
             if !publishers.is_empty() {
-                new_imports
-                    .publisher
-                    .insert(package.name.to_owned(), publishers);
+                new_imports.publisher.insert(pkgname.to_owned(), publishers);
             }
         }
 


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 5b7bc6b..f42342e 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -8,7 +8,7 @@ fn get_imports_file_changes(metadata: &Metadata, store: &Store) -> String {
     // Run the resolver before calling `get_imports_file` to compute the new
     // imports file.
     let report = crate::resolver::resolve(metadata, None, store);
-    let new_imports = store.get_updated_imports_file(&report.graph, &report.results);
+    let new_imports = store.get_updated_imports_file(&report.collect_required_entries());
 
     // Format the old and new files as TOML, and write out a diff using `similar`.
     let old_imports = crate::serialization::to_formatted_toml(&store.imports)
@@ -1060,6 +1060,53 @@ fn equal_length_preferred_audits() {
     insta::assert_snapshot!(output);
 }
 
+#[test]
+fn import_multiple_versions() {
+    // (Pass) If multiple versions of a crate in the graph need to import
+    // audits, we need to import the required audits for all versions, not just
+    // one of them.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::complex();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-core");
+
+    let new_foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [(
+            "third-core".to_owned(),
+            vec![
+                full_audit(ver(5), "safe-to-deploy"),
+                full_audit(ver(DEFAULT_VER), "safe-to-deploy"),
+            ],
+        )]
+        .into_iter()
+        .collect(),
+        wildcard_audits: SortedMap::new(),
+    };
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: FOREIGN_URL.to_owned(),
+            ..Default::default()
+        },
+    );
+
+    let cfg = mock_cfg(&metadata);
+
+    let mut network = Network::new_mock();
+    network.mock_serve_toml(FOREIGN_URL, &new_foreign_audits);
+
+    let store = Store::mock_online(&cfg, config, audits, imports, &network, true).unwrap();
+
+    let output = get_imports_file_changes(&metadata, &store);
+    insta::assert_snapshot!(output);
+}
+
 #[test]
 fn foreign_audit_file_to_local() {
     let _enter = TEST_RUNTIME.enter();
diff --git a/src/tests/snapshots/cargo_vet__tests__import__import_multiple_versions.snap b/src/tests/snapshots/cargo_vet__tests__import__import_multiple_versions.snap
new file mode 100644
index 0000000..a30635f
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__import_multiple_versions.snap
@@ -0,0 +1,13 @@
+---
+source: src/tests/import.rs
+expression: output
+---
++
++[[audits.peer-company.audits.third-core]]
++criteria = "safe-to-deploy"
++version = "5.0.0"
++
++[[audits.peer-company.audits.third-core]]
++criteria = "safe-to-deploy"
++version = "10.0.0"
+