Repository: mozilla/cargo-vet
Pull request #426 (2023-03-03 19:42:13)

Emit an error when trying to remap remote built-in criteria
This was never properly implemented, and is being changed to emit a useful error, rather than producing confusing behaviour, such as accidentally creating an implies cycle between the safe-to-run and safe-to-deploy criteria.

This patch also removes documentation suggesting support for multiple criteria in criteria-map entries. This has never been implemented, and would not be trivial to implement with the current infrastructure, so has been removed. Some internal code has also been simplified to produce a better error in that situation.


Patch:
diff --git a/book/src/config.md b/book/src/config.md
index 3ddeb01..de3791e 100644
--- a/book/src/config.md
+++ b/book/src/config.md
@@ -91,23 +91,26 @@ field is required.
 
 An inline table or array of inline tables specifying one or more mappings
 between the audit criteria of the imported and local sets. Each imported audit
-is matched against each mapping. If the imported audit certifies all of the
-criteria listed in the `theirs` key, it is associated with the local criteria
-specified in the `ours` key.
+is matched against each mapping. If the imported audit certifies the criteria
+listed in the `theirs` key, it will certify the local criteria specified in the
+`ours` key.
 
-This will generally be a 1:1 mapping:
-
-```
-criteria-map = { theirs: "a", ours: "x" }
 ```
+[[imports.peer.criteria-map]]
+ours = "x"
+theirs = "a"
 
-But can also be more complex:
-
-```
-criteria-map = [ { theirs: "b", ours: ["y", "z"] },
-                 { theirs: ["c", "d"], ours: "z" } ]
+[[imports.peer.criteria-map]]
+ours = "safe-to-deploy"
+theirs = "super-audited"
 ```
 
+Note that while you can map a remote criteria to the built-in criteria, you
+cannot remap a peer's built-in criteria to some other criteria. The
+`safe-to-run` and `safe-to-deploy` criteria will always be mapped directly.
+Please let us know in [#425](https://github.com/mozilla/cargo-vet/issues/425) if
+this limitation causes issues.
+
 #### `exclude`
 
 A list of crates whose audit entries should not be imported from this source.
diff --git a/src/errors.rs b/src/errors.rs
index f51dd90..f2f6803 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -393,6 +393,9 @@ pub enum StoreValidateError {
     #[diagnostic(transparent)]
     #[error(transparent)]
     BadWildcardEndDate(BadWildcardEndDateError),
+    #[diagnostic(transparent)]
+    #[error(transparent)]
+    BadBuiltInCriteriaMapping(BadBuiltInCriteriaMappingError),
     #[error("imports.lock is out-of-date with respect to configuration")]
     #[diagnostic(help("run `cargo vet` without --locked to update imports"))]
     ImportsLockOutdated,
@@ -429,6 +432,17 @@ pub struct BadWildcardEndDateError {
     pub max: chrono::NaiveDate,
 }
 
+#[derive(Debug, Error, Diagnostic)]
+#[error("Built-in criteria '{criteria_name}' cannot be re-mapped when importing")]
+#[diagnostic(help("built-in criteria are always imported unmodified (see issue #425)"))]
+pub struct BadBuiltInCriteriaMappingError {
+    #[source_code]
+    pub source_code: SourceFile,
+    #[label]
+    pub span: SourceSpan,
+    pub criteria_name: CriteriaName,
+}
+
 //////////////////////////////////////////////////////////
 // CacheErrors
 /////////////////////////////////////////////////////////
diff --git a/src/format.rs b/src/format.rs
index df0af7a..af42332 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -708,9 +708,8 @@ pub struct RemoteImport {
 pub struct CriteriaMapping {
     /// This local criteria is implied...
     pub ours: CriteriaName,
-    /// If all of these foreign criteria apply
-    #[serde(with = "serialization::string_or_vec")]
-    pub theirs: Vec<Spanned<ForeignCriteriaName>>,
+    /// If this foreign criteria applies
+    pub theirs: Spanned<ForeignCriteriaName>,
 }
 
 /// Semantically identical to a 'full audit' entry, but private to our project
diff --git a/src/resolver.rs b/src/resolver.rs
index 506a8cb..ac79dab 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -510,12 +510,8 @@ impl CriteriaMapper {
             let local = CriteriaNamespace::Local;
             for mapping in mappings {
                 // Add a bidirectional edge between these two criteria (they are now completely equivalent)
-                assert!(
-                    mapping.theirs.len() == 1,
-                    "criteria_map doesn't yet support multi-mapping, must be 1:1"
-                );
                 let our_idx = index[&local][&mapping.ours];
-                let their_idx = index[&foreign][&*mapping.theirs[0]];
+                let their_idx = index[&foreign][&*mapping.theirs];
                 direct_implies
                     .get_mut(&our_idx)
                     .unwrap()
diff --git a/src/storage.rs b/src/storage.rs
index cfad290..ff214ae 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -20,12 +20,12 @@ use tracing::{error, info, log::warn, trace};
 
 use crate::{
     errors::{
-        BadFormatError, BadWildcardEndDateError, CacheAcquireError, CacheCommitError, CertifyError,
-        CommandError, CriteriaChangeError, CriteriaChangeErrors, DiffError, DownloadError,
-        FetchAndDiffError, FetchAuditError, FetchError, FlockError, InvalidCriteriaError,
-        JsonParseError, LoadJsonError, LoadTomlError, SourceFile, StoreAcquireError,
-        StoreCommitError, StoreCreateError, StoreJsonError, StoreTomlError, StoreValidateError,
-        StoreValidateErrors, TomlParseError, UnpackCheckoutError, UnpackError,
+        BadBuiltInCriteriaMappingError, BadFormatError, BadWildcardEndDateError, CacheAcquireError,
+        CacheCommitError, CertifyError, CommandError, CriteriaChangeError, CriteriaChangeErrors,
+        DiffError, DownloadError, FetchAndDiffError, FetchAuditError, FetchError, FlockError,
+        InvalidCriteriaError, JsonParseError, LoadJsonError, LoadTomlError, SourceFile,
+        StoreAcquireError, StoreCommitError, StoreCreateError, StoreJsonError, StoreTomlError,
+        StoreValidateError, StoreValidateErrors, TomlParseError, UnpackCheckoutError, UnpackError,
     },
     flock::{FileLock, Filesystem},
     format::{
@@ -571,6 +571,20 @@ impl Store {
             }
         }
 
+        for (_name, import) in &self.config.imports {
+            for mapping in &import.criteria_map {
+                if &mapping.theirs[..] == SAFE_TO_DEPLOY || &mapping.theirs[..] == SAFE_TO_RUN {
+                    errors.push(StoreValidateError::BadBuiltInCriteriaMapping(
+                        BadBuiltInCriteriaMappingError {
+                            source_code: self.config_src.clone(),
+                            span: Spanned::span(&mapping.theirs),
+                            criteria_name: mapping.theirs[..].to_owned(),
+                        },
+                    ));
+                }
+            }
+        }
+
         // If requested, verify that files in the store are correctly formatted
         // and have no unrecognized fields. We don't want to be reformatting
         // them or dropping unused fields while in CI, as those changes will be


Test patch:
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_criteria_map.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_criteria_map.snap
new file mode 100644
index 0000000..ff1a8f4
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_criteria_map.snap
@@ -0,0 +1,16 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Your cargo-vet store (supply-chain) has consistency errors
+
+Error: 
+  × Built-in criteria 'safe-to-deploy' cannot be re-mapped when importing
+    ╭─[config.toml:12:1]
+ 12 │ ours = "safe-to-run"
+ 13 │ theirs = "safe-to-deploy"
+    ·          ────────────────
+    ╰────
+  help: built-in criteria are always imported unmodified (see issue #425)
+
diff --git a/src/tests/store_parsing.rs b/src/tests/store_parsing.rs
index 8f0de92..0bac050 100644
--- a/src/tests/store_parsing.rs
+++ b/src/tests/store_parsing.rs
@@ -386,3 +386,33 @@ version = "10.0.0"
     let acquire_errors = get_valid_store(config, audits, imports);
     insta::assert_snapshot!(acquire_errors);
 }
+
+#[test]
+fn test_invalid_criteria_map() {
+    let config = r##"
+# cargo-vet config file
+
+[imports.peer1]
+url = "https://peer1.com"
+
+[[imports.peer1.criteria-map]]
+ours = "safe-to-run"
+theirs = "fuzzed"
+
+[[imports.peer1.criteria-map]]
+ours = "safe-to-run"
+theirs = "safe-to-deploy"
+"##;
+
+    let imports = r##"
+# cargo-vet imports lock
+
+[audits.peer1.criteria.fuzzed]
+description = "fuzzed"
+
+[audits.peer1.audits]
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
diff --git a/src/tests/vet.rs b/src/tests/vet.rs
index 412ba72..95657eb 100644
--- a/src/tests/vet.rs
+++ b/src/tests/vet.rs
@@ -2478,7 +2478,7 @@ fn mock_simple_foreign_audited_pun_mapped() {
             url: FOREIGN_URL.to_owned(),
             criteria_map: vec![CriteriaMapping {
                 ours: DEFAULT_CRIT.to_owned(),
-                theirs: vec![DEFAULT_CRIT.to_owned().into()],
+                theirs: DEFAULT_CRIT.to_owned().into(),
             }],
             ..Default::default()
         },
@@ -2512,7 +2512,7 @@ fn mock_simple_foreign_audited_pun_wrong_mapped() {
             url: OTHER_FOREIGN_URL.to_owned(),
             criteria_map: vec![CriteriaMapping {
                 ours: DEFAULT_CRIT.to_owned(),
-                theirs: vec![DEFAULT_CRIT.to_owned().into()],
+                theirs: DEFAULT_CRIT.to_owned().into(),
             }],
             ..Default::default()
         },