Repository: mozilla/cargo-vet (language: python)

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.


Discussion / hints:
Over to @mystor or @afranchuk to fix when one of them is back.
It took some searching to find where they are filtered out (having not written the code), but I did [find it](https://github.com/mozilla/cargo-vet/blob/main/src/resolver.rs#L2724-L2727). @mystor what was the reasoning behind this? If I read the code correctly, if someone prunes the store (e.g. removing a violation for `foo`) and then uses `foo` later, the live imports will have it so the `imports.lock` will be updated and an appropriate error will occur. So I think it will work correctly if we remove the unused violations? Or is there a case I'm missing?
I had written up a full comment about this but apparently never submitted it so it ended up disappearing. 

This was originally an intentional design decision with the first design where we wanted to pull in updates when needed, and always pulled in everything from a peer when needed. Because violations were considered important, we would forcefully import them every time similar to how we always imported criteria back then.

This wasn't changed when doing the other changes to only import what we need. Technically speaking we will never actually import a violation and store it in `imports.lock` as we only update imports.lock after a successful vet, and we'll never successfully vet with a relevant violation, so just switching this to always return `false` would roughly have the expected effect.

I'm not sure if that's the behaviour we'd want though. We might instead want to have it always import violations for third-party crates you're using whether or not they end up relevant for the audit.
thanks for filing this! noticed it also in our repos after we (well I) created the violation entry in our registry, but missed filing an issue on it.

Fix patch:
diff --git a/src/resolver.rs b/src/resolver.rs
index 4b986fe4..eb3cd0bb 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 7f7ec645..bd3c6e09 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 5a28d4d5..ec6f4b6b 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 6b4e3c4e..d3d07240 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 00000000..25b434f0
--- /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 00000000..72eb21d5
--- /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"
+