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

Issue:
prefer local wildcard audits to remote regular audits
Over on [mozilla-central](https://phabricator.services.mozilla.com/D197955), @jrmuizel and @glandium identified an interesting issue where Jeff was trying to replace a regular audit with a wildcard audit. Rather than reifying the local wildcard audit, cargo-vet instead re-fetches the locally-deleted regular audit from the [aggregated mozilla-wide set](https://raw.githubusercontent.com/mozilla/supply-chain/main/audits.toml) and re-inserts it as an import.

I'm assuming this is because the precedence order is currently:
1. local regular audits
2. remote regular audits
3. local wildcard audits
4. remote wildcard audits

Ideally we'd swap 2 and 3.

@afranchuk Is my hypothesis correct?


Discussion / hints:
I believe so after reading the code, and will verify with a potential fix shortly. As it currently exists, local and remote wildcard audits are put together in a single enum variant when ordering, so that may explain the resulting behavior.
Another option here might be for us to include a mechanism by which you can identify your repository's "public url" (for m-c that'd be `"https://hg.mozilla.org/mozilla-central/raw-file/tip/supply-chain/audits.toml"`), which would prevent an audit with that `aggregated-from` URI from being imported, avoiding the cyclic import issue when an entry is removed (though generally removing entries is unsupported).

This issue only comes up when you're removing an audit but are still using it (causing it to be re-imported). I think if we just make sure to always prioritize local audits over remote ones, we should never hit the case where we re-import an audit that could otherwise be dropped.
The reason why it came up in this situation is that a fresh wildcard audit is always somewhat remote (as it requires importing a `publisher` entry), and there is no differentiation in the resolver right now between a wildcard audit which is partially remote vs. fully remote (which I am guessing is what @afranchuk is proposing adding).

I just figured I'd mention the `aggregated-from` option because otherwise it would be possible for someone to remove an audit (which they shouldn't be doing, but i'm sure someone will do from time to time), and then have their audits start failing later when aggregation is complete.
I see. That's a fair point, but I'd still opt not to add a user-facing configuration field for such an edge case.
I have a script which easily reproduces this problem, and have a set of changes which fixes it. Or half-fixes it: after a `cargo vet certify --wildcard` it'll still have the old import (which is not "fresh" at that point, having been imported when the `mozilla` import was added), but subsequent operations prompt the user to `prune` (and it does remove the unneeded import). I'm wondering whether this is acceptable and desirable behavior? If so, I can convert the state of my test script into a unit test and push up a PR.
That sound fine to me, I think that's consistent with other behavior around e.g. exemptions (certify doesn't implicitly prune).

Fix patch:
diff --git a/src/resolver.rs b/src/resolver.rs
index 46b5bee7..b9e55e34 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -341,6 +341,34 @@ pub enum RequiredEntry {
     },
 }
 
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+enum DeltaEdgeFreshness {
+    // All information requried for this delta edge is already stored within the store's
+    // supply-chain. Using this edge will require no changes to imports.lock.
+    Stale,
+    // This edge originates from an already-imported or local wildcard audit, however will require
+    // importing fresh publisher information into imports.lock.
+    FreshPublisher,
+    // This edge is fully fresh, and will require adding the audit entry to imports.lock to use.
+    Fresh,
+}
+
+impl DeltaEdgeFreshness {
+    fn new(is_fresh_audit: bool, is_fresh_publisher: bool) -> Self {
+        if is_fresh_audit {
+            DeltaEdgeFreshness::Fresh
+        } else if is_fresh_publisher {
+            DeltaEdgeFreshness::FreshPublisher
+        } else {
+            DeltaEdgeFreshness::Stale
+        }
+    }
+
+    fn is_fresh(&self) -> bool {
+        self != &DeltaEdgeFreshness::Stale
+    }
+}
+
 /// A directed edge in the graph of audits. This may be forward or backwards,
 /// depending on if we're searching from "roots" (forward) or the target (backward).
 /// The source isn't included because that's implicit in the Node.
@@ -355,7 +383,7 @@ struct DeltaEdge<'a> {
     origin: DeltaEdgeOrigin,
     /// Whether or not the edge is a "fresh import", and should be
     /// de-prioritized to avoid unnecessary imports.lock updates.
-    is_fresh_import: bool,
+    freshness: DeltaEdgeFreshness,
 }
 
 #[derive(Debug, Copy, Clone, Eq, PartialEq)]
@@ -1142,12 +1170,13 @@ impl<'a> AuditGraph<'a> {
             };
 
             let criteria = criteria_mapper.criteria_from_list(&entry.criteria);
+            let freshness = DeltaEdgeFreshness::new(entry.is_fresh_import, false);
 
             forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                 version: Some(to_ver),
                 criteria: criteria.clone(),
                 origin: origin.clone(),
-                is_fresh_import: entry.is_fresh_import,
+                freshness,
             });
             backward_audits
                 .entry(Some(to_ver))
@@ -1156,7 +1185,7 @@ impl<'a> AuditGraph<'a> {
                     version: from_ver,
                     criteria,
                     origin,
-                    is_fresh_import: entry.is_fresh_import,
+                    freshness,
                 });
         }
 
@@ -1177,19 +1206,20 @@ impl<'a> AuditGraph<'a> {
                         audit_index,
                         publisher_index,
                     };
-                    let is_fresh_import = entry.is_fresh_import || publisher.is_fresh_import;
+                    let freshness =
+                        DeltaEdgeFreshness::new(entry.is_fresh_import, publisher.is_fresh_import);
 
                     forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                         version: to_ver,
                         criteria: criteria.clone(),
                         origin: origin.clone(),
-                        is_fresh_import,
+                        freshness,
                     });
                     backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                         version: from_ver,
                         criteria,
                         origin,
-                        is_fresh_import,
+                        freshness,
                     });
                 }
             }
@@ -1203,18 +1233,23 @@ impl<'a> AuditGraph<'a> {
                     let to_ver = Some(&publisher.version);
                     let criteria = criteria_mapper.criteria_from_list(&entry.criteria);
                     let origin = DeltaEdgeOrigin::Trusted { publisher_index };
+                    // While the import freshness is technically based on the publisher being a
+                    // fresh import, we use this as the _audit_ freshness here because a trusted
+                    // entry should be put at the same caveat level ordering as other audits (which
+                    // is determined from freshness later on).
+                    let freshness = DeltaEdgeFreshness::new(publisher.is_fresh_import, false);
 
                     forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                         version: to_ver,
                         criteria: criteria.clone(),
                         origin: origin.clone(),
-                        is_fresh_import: publisher.is_fresh_import,
+                        freshness,
                     });
                     backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                         version: from_ver,
                         criteria,
                         origin,
-                        is_fresh_import: publisher.is_fresh_import,
+                        freshness,
                     });
                 }
             }
@@ -1226,19 +1261,19 @@ impl<'a> AuditGraph<'a> {
             let to_ver = Some(&unpublished.version);
             let criteria = criteria_mapper.all_criteria();
             let origin = DeltaEdgeOrigin::Unpublished { unpublished_index };
-            let is_fresh_import = unpublished.is_fresh_import;
+            let freshness = DeltaEdgeFreshness::new(unpublished.is_fresh_import, false);
 
             forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                 version: to_ver,
                 criteria: criteria.clone(),
                 origin: origin.clone(),
-                is_fresh_import,
+                freshness,
             });
             backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                 version: from_ver,
                 criteria,
                 origin,
-                is_fresh_import,
+                freshness,
             });
         }
 
@@ -1255,13 +1290,13 @@ impl<'a> AuditGraph<'a> {
                     version: to_ver,
                     criteria: criteria.clone(),
                     origin: origin.clone(),
-                    is_fresh_import: false,
+                    freshness: DeltaEdgeFreshness::Stale,
                 });
                 backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                     version: from_ver,
                     criteria,
                     origin,
-                    is_fresh_import: false,
+                    freshness: DeltaEdgeFreshness::Stale,
                 });
             }
         }
@@ -1449,6 +1484,7 @@ fn search_for_path(
         NonImportableAudit,
         PreferredExemption,
         PreferredUnpublished,
+        FreshPublisher,
         FreshImport,
         Exemption,
         Unpublished,
@@ -1579,16 +1615,19 @@ fn search_for_path(
                 DeltaEdgeOrigin::Unpublished { .. } => match mode {
                     // When preferring exemptions, prefer existing unpublished
                     // entries to avoid imports.lock churn.
-                    SearchMode::PreferExemptions if !edge.is_fresh_import => {
+                    SearchMode::PreferExemptions if !edge.freshness.is_fresh() => {
                         CaveatLevel::PreferredUnpublished
                     }
                     SearchMode::PreferExemptions => CaveatLevel::Unpublished,
                     // Otherwise, prefer fresh to avoid outdated versions.
-                    _ if edge.is_fresh_import => CaveatLevel::PreferredUnpublished,
+                    _ if edge.freshness.is_fresh() => CaveatLevel::PreferredUnpublished,
                     _ => CaveatLevel::Unpublished,
                 },
-                _ if edge.is_fresh_import => CaveatLevel::FreshImport,
-                _ => CaveatLevel::None,
+                _ => match edge.freshness {
+                    DeltaEdgeFreshness::Stale => CaveatLevel::None,
+                    DeltaEdgeFreshness::FreshPublisher => CaveatLevel::FreshPublisher,
+                    DeltaEdgeFreshness::Fresh => CaveatLevel::FreshImport,
+                },
             };
 
             queue.push(Node {


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 26381a42..bc1de9a0 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -1934,3 +1934,123 @@ fn import_criteria_map_aggregated_error() {
 
     insta::assert_snapshot!(output);
 }
+
+#[test]
+fn existing_import_kept_despite_local_wildcard_audit() {
+    // (Pass) An existing imported audit is still kept if a local wildcard audit accounts for a
+    // crate.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, mut imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-party2");
+
+    audits.wildcard_audits.insert(
+        "third-party2".to_owned(),
+        vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+    );
+
+    let foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [(
+            "third-party2".to_owned(),
+            vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        wildcard_audits: SortedMap::new(),
+        trusted: SortedMap::new(),
+    };
+
+    imports
+        .audits
+        .insert(FOREIGN.to_owned(), foreign_audits.clone());
+
+    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();
+    MockRegistryBuilder::new()
+        .user(1, "user1", "User One")
+        .package(
+            "third-party2",
+            &[reg_published_by(ver(DEFAULT_VER), Some(1), "2022-12-15")],
+        )
+        .serve(&mut network);
+    network.mock_serve_toml(FOREIGN_URL, &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 local_wildcard_audit_preferred_to_fresh_import() {
+    // (Pass) If a local wildcard audit accounts for a crate, a freshly imported audit should not
+    // be preferred.
+
+    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");
+
+    audits.wildcard_audits.insert(
+        "third-party2".to_owned(),
+        vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+    );
+
+    let foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [(
+            "third-party2".to_owned(),
+            vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        // This isn't necessary (and overlaps with the imported audit), but we have it to also show
+        // that local wildcard audits are preferred to remote ones.
+        wildcard_audits: [(
+            "third-party2".to_owned(),
+            vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        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();
+    MockRegistryBuilder::new()
+        .user(1, "user1", "User One")
+        .package(
+            "third-party2",
+            &[reg_published_by(ver(DEFAULT_VER), Some(1), "2022-12-15")],
+        )
+        .serve(&mut network);
+    network.mock_serve_toml(FOREIGN_URL, &foreign_audits);
+
+    let store = Store::mock_online(&cfg, config, audits, imports, &network, true).unwrap();
+
+    let output = get_imports_file_changes_noprune(&metadata, &store);
+    insta::assert_snapshot!(output);
+}
diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_import_kept_despite_local_wildcard_audit.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_import_kept_despite_local_wildcard_audit.snap
new file mode 100644
index 00000000..ca417f8d
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__existing_import_kept_despite_local_wildcard_audit.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"
+
diff --git a/src/tests/snapshots/cargo_vet__tests__import__local_wildcard_audit_preferred_to_fresh_import.snap b/src/tests/snapshots/cargo_vet__tests__import__local_wildcard_audit_preferred_to_fresh_import.snap
new file mode 100644
index 00000000..f1e7c72a
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__import__local_wildcard_audit_preferred_to_fresh_import.snap
@@ -0,0 +1,14 @@
+---
+source: src/tests/import.rs
+expression: output
+---
++
++[[publisher.third-party2]]
++version = "10.0.0"
++when = "2022-12-15"
++user-id = 1
++user-login = "user1"
++user-name = "User One"
++
++[audits.peer-company.audits]
+