Repository: mozilla/cargo-vet (rust)

Issue:
Violations should not appear in imports.lock if the crate is not used
Right now on m-c, cargo-vet wants to pull in a violation entry from embark for a crate (`epaint`) we don't use. I'm guessing that when we modified `imports.lock` to be more selective we missed violations somehow. We should fix that.

Pull request:
Remove violations when pruning if no crates in the graph match the violation crate
If a crate does match, keep all violations (regardless of version).

Closes #486.

Patch:
diff --git a/src/resolver.rs b/src/resolver.rs
index 4b986fe..eb3cd0b 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -2712,25 +2712,27 @@ pub(crate) fn get_store_updates(
                 .iter()
                 .map(|(pkgname, audits)| {
                     let prune_imports = mode(&pkgname[..]).prune_imports;
-                    let required_entries = required_entries
-                        .get(&pkgname[..])
-                        .unwrap_or(&no_required_entries);
+                    let (uses_package, required_entries) = match required_entries.get(&pkgname[..])
+                    {
+                        Some(e) => (true, e),
+                        None => (false, &no_required_entries),
+                    };
                     (
                         pkgname,
                         audits
                             .iter()
                             .enumerate()
                             .filter(|&(audit_index, entry)| {
-                                // Always keep any violations.
-                                if matches!(entry.kind, AuditKind::Violation { .. }) {
-                                    return true;
-                                }
-
                                 // Keep existing if we're not pruning imports.
                                 if !prune_imports && !entry.is_fresh_import {
                                     return true;
                                 }
 
+                                // Keep violations if the package is used in the graph.
+                                if matches!(entry.kind, AuditKind::Violation { .. }) {
+                                    return uses_package;
+                                }
+
                                 if let Some(required_entries) = required_entries {
                                     required_entries.contains_key(&RequiredEntry::Audit {
                                         import_index,


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 7f7ec64..bd3c6e0 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -220,8 +220,9 @@ fn existing_peer_skip_import() {
 
 #[test]
 fn existing_peer_remove_unused() {
-    // (Pass) When pruning, we'll remove unused audits when unlocked, even if
-    // our peer hasn't changed. These audits will be preserved when not pruning.
+    // (Pass) When pruning, we'll remove unused audits (including violations)
+    // when unlocked, even if our peer hasn't changed. These audits will be
+    // preserved when not pruning.
 
     let _enter = TEST_RUNTIME.enter();
     let mock = MockMetadata::simple();
@@ -248,6 +249,10 @@ fn existing_peer_remove_unused() {
                 "unused-package".to_owned(),
                 vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
             ),
+            (
+                "unused-violation".to_owned(),
+                vec![violation(VersionReq::parse("1.*").unwrap(), SAFE_TO_DEPLOY)],
+            ),
         ]
         .into_iter()
         .collect(),
@@ -723,6 +728,105 @@ fn existing_peer_add_violation() {
     insta::assert_snapshot!(output);
 }
 
+#[test]
+fn new_audit_needed_violation() {
+    // (Pass) If a peer provides a violation for a crate we use (even if there are no related
+    // audits), we should import it.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-party2");
+
+    let new_foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        wildcard_audits: SortedMap::new(),
+        audits: [(
+            "third-party2".to_owned(),
+            vec![violation(
+                VersionReq::parse("10.*").unwrap(),
+                SAFE_TO_DEPLOY,
+            )],
+        )]
+        .into_iter()
+        .collect(),
+        trusted: SortedMap::new(),
+    };
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: vec![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_prune(&metadata, &store);
+    insta::assert_snapshot!(output);
+}
+
+#[test]
+fn new_audit_unneeded_violation() {
+    // (Pass) If a peer provides a violation for a crate we don't use, we should not import it.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-party2");
+
+    let new_foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        wildcard_audits: SortedMap::new(),
+        audits: [
+            (
+                "third-party2".to_owned(),
+                vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+            ),
+            (
+                "third-party3".to_owned(),
+                vec![violation(
+                    VersionReq::parse("10.*").unwrap(),
+                    SAFE_TO_DEPLOY,
+                )],
+            ),
+        ]
+        .into_iter()
+        .collect(),
+        trusted: SortedMap::new(),
+    };
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: vec![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_prune(&metadata, &store);
+    insta::assert_snapshot!(output);
+}
+
 #[test]
 fn peer_audits_exemption_no_minimize() {
     // (Pass) We don't import audits for a package which would replace an
diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused.snap
index 5a28d4d..ec6f4b6 100644
--- a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused.snap
+++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused.snap
@@ -22,4 +22,8 @@ expression: output
 -[[audits.peer-company.audits.unused-package]]
 -criteria = "safe-to-deploy"
 -version = "10.0.0"
+-
+-[[audits.peer-company.audits.unused-violation]]
+-criteria = "safe-to-deploy"
+-violation = "1.*"
 
diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused_noprune.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused_noprune.snap
index 6b4e3c4..d3d0724 100644
--- a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused_noprune.snap
+++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused_noprune.snap
@@ -22,4 +22,8 @@ expression: output
  [[audits.peer-company.audits.unused-package]]
  criteria = "safe-to-deploy"
  version = "10.0.0"
+ 
+ [[audits.peer-company.audits.unused-violation]]
+ criteria = "safe-to-deploy"
+ violation = "1.*"
 
diff --git a/src/tests/snapshots/cargo_vet__tests__import__new_audit_needed_violation.snap b/src/tests/snapshots/cargo_vet__tests__import__new_audit_needed_violation.snap
new file mode 100644
index 0000000..25b434f
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__new_audit_needed_violation.snap
@@ -0,0 +1,9 @@
+---
+source: src/tests/import.rs
+expression: output
+---
++
++[[audits.peer-company.audits.third-party2]]
++criteria = "safe-to-deploy"
++violation = "10.*"
+
diff --git a/src/tests/snapshots/cargo_vet__tests__import__new_audit_unneeded_violation.snap b/src/tests/snapshots/cargo_vet__tests__import__new_audit_unneeded_violation.snap
new file mode 100644
index 0000000..72eb21d
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__new_audit_unneeded_violation.snap
@@ -0,0 +1,9 @@
+---
+source: src/tests/import.rs
+expression: output
+---
++
++[[audits.peer-company.audits.third-party2]]
++criteria = "safe-to-deploy"
++version = "10.0.0"
+