Repository: mozilla/cargo-vet (rust)

Issue:
Imports should drop audits with unkown fields
Currently the import machinery silently drops audit fields it doesn't understand. This creates a risk that we might add some future audit field that serves as a caveat (only these features, only these targets, etc), and a consumer on an older version ends up with a more sweeping audit than the author intended. We should change the behavior so that an unknown field causes the entire entry to be dropped.

A downside of this change is that it would no longer be possible for projects to rewrite existing audits with new fields without potentially breaking consumers on older versions. However, this would merely result in the consumer getting some errors and deciding to upgrade, whereas the current situation risks unsoundness.

I _think_ we should do the same for all the tables (including audits and wildcards), but I haven't fully thought those through.

Pull request description:
Deny unknown fields in audit and criteria entries
Fixes #388

Patch:
diff --git a/src/format.rs b/src/format.rs
index aef46ab..c55cb77 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -225,6 +225,7 @@ pub struct ForeignAuditsFile {
 
 /// Information on a Criteria
 #[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
+#[serde(deny_unknown_fields)]
 pub struct CriteriaEntry {
     /// Summary of how you evaluate something by this criteria.
     pub description: Option<String>,
diff --git a/src/serialization.rs b/src/serialization.rs
index b0ed6c6..5a00c4b 100644
--- a/src/serialization.rs
+++ b/src/serialization.rs
@@ -187,6 +187,7 @@ pub mod audit {
     use crate::format::{AuditEntry, AuditKind, CriteriaName, Delta, VersionReq, VetVersion};
 
     #[derive(Serialize, Deserialize)]
+    #[serde(deny_unknown_fields)]
     pub struct AuditEntryAll {
         #[serde(default)]
         #[serde(skip_serializing_if = "Vec::is_empty")]


Test patch:
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 7b25f09..4ca4581 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -883,7 +883,6 @@ fn foreign_audit_file_to_local() {
                 toml::toml! {
                     description = "example2"
                     implies = ["safe-to-deploy", "will-not-parse"]
-                    unknown = "ignored unknown field"
                 },
             ),
             (
@@ -893,6 +892,14 @@ fn foreign_audit_file_to_local() {
                     description = "will be ignored"
                 },
             ),
+            (
+                "will-not-parse2".to_string(),
+                toml::toml! {
+                    description = "example2"
+                    implies = "safe-to-deploy"
+                    unknown = "invalid unknown field"
+                },
+            ),
         ]
         .into_iter()
         .collect(),
@@ -925,6 +932,11 @@ fn foreign_audit_file_to_local() {
                         violation = "invalid"
                         notes = "will be removed"
                     },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "20.0.0"
+                        unknown = "invalid unknown field"
+                    },
                 ],
             ),
             (
@@ -969,10 +981,13 @@ fn foreign_audit_file_to_local() {
     result.ignored_criteria.sort();
     result.ignored_audits.sort();
 
-    assert_eq!(result.ignored_criteria, &["will-not-parse"]);
+    assert_eq!(
+        result.ignored_criteria,
+        &["will-not-parse", "will-not-parse2"]
+    );
     assert_eq!(
         result.ignored_audits,
-        &["crate-a", "crate-a", "crate-a", "crate-a", "crate-b"]
+        &["crate-a", "crate-a", "crate-a", "crate-a", "crate-a", "crate-b"]
     );
 
     insta::assert_snapshot!(
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
index edfa6c6..e462c53 100644
--- a/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
@@ -35,16 +35,11 @@ Error:
   │ 
   │ --- old/audits.toml
   │ +++ new/audits.toml
-  │ @@ -4,14 +4,12 @@
-  │  [criteria.good]
-  │  description = "great"
-  │  implies = "safe-to-deploy"
-  │ -unrecognized-field = "hey there!"
+  │ @@ -7,9 +7,9 @@
   │ 
   │  [[audits.serde]]
   │  criteria = ["safe-to-deploy", "good"]
   │ -version = "2.0.0"
-  │ -note = "invalid field"
   │ +version = "1.0.0"
   │ +notes = "valid field"
   │ 
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_audit.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_audit.snap
new file mode 100644
index 0000000..6ddf72d
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_audit.snap
@@ -0,0 +1,17 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Failed to parse toml file
+  ╰─▶ unknown field `unknown-field`, expected one of `who`, `criteria`,
+      `version`, `delta`, `violation`, `notes`, `aggregated-from` for key
+      `audits.zzz` at line 4 column 1
+   ╭─[audits.toml:2:1]
+ 2 │ # cargo-vet audits file
+ 3 │ 
+   · ▲
+   · ╰── here
+ 4 │ [[audits.zzz]]
+   ╰────
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap
new file mode 100644
index 0000000..b0bde1a
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap
@@ -0,0 +1,25 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Your cargo-vet store (supply-chain) has consistency errors
+
+Error: 
+  × A file in the store is not correctly formatted:
+  │ 
+  │ --- old/config.toml
+  │ +++ new/config.toml
+  │ @@ -4,9 +4,7 @@
+  │  [imports.peer1]
+  │  url = "https://peer1.com"
+  │  exclude = ["zzz", "aaa"]
+  │ -unknown-field = "hi"
+  │ 
+  │  [[exemptions.zzz]]
+  │  version = "1.0.0"
+  │  criteria = "safe-to-deploy"
+  │ -unknown-field = "hi"
+  │ 
+  help: run `cargo vet` without --locked to reformat files in the store
+
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_criteria.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_criteria.snap
new file mode 100644
index 0000000..fd90ec8
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_criteria.snap
@@ -0,0 +1,17 @@
+---
+source: src/tests/store_parsing.rs
+expression: acquire_errors
+---
+
+  × Failed to parse toml file
+  ╰─▶ unknown field `unknown-field`, expected one of `description`,
+      `description-url`, `implies`, `aggregated-from` for key `criteria.good`
+      at line 9 column 1
+   ╭─[audits.toml:7:1]
+ 7 │ unknown-field = "invalid"
+ 8 │ 
+   · ▲
+   · ╰── here
+ 9 │ [audits]
+   ╰────
+
diff --git a/src/tests/store_parsing.rs b/src/tests/store_parsing.rs
index d4f2959..d3ccf87 100644
--- a/src/tests/store_parsing.rs
+++ b/src/tests/store_parsing.rs
@@ -225,6 +225,66 @@ version = "10.0.0"
     insta::assert_snapshot!(acquire_errors);
 }
 
+#[test]
+fn test_unknown_field_config() {
+    let config = r##"
+# cargo-vet config file
+
+[imports.peer1]
+url = "https://peer1.com"
+exclude = ["zzz", "aaa"]
+unknown-field = "hi"
+
+[[exemptions.zzz]]
+version = "1.0.0"
+criteria = "safe-to-deploy"
+unknown-field = "hi"
+"##;
+
+    let imports = r##"
+# cargo-vet imports lock
+
+[[audits.peer1.audits.third-party1]]
+criteria = "safe-to-deploy"
+version = "10.0.0"
+"##;
+
+    let acquire_errors = get_valid_store(config, EMPTY_AUDITS, imports);
+    insta::assert_snapshot!(acquire_errors);
+}
+
+#[test]
+fn test_unknown_field_criteria() {
+    let audits = r##"
+# cargo-vet audits file
+
+[criteria.good]
+description = "great"
+implies = "safe-to-deploy"
+unknown-field = "invalid"
+
+[audits]
+"##;
+
+    let acquire_errors = get_valid_store(EMPTY_CONFIG, audits, EMPTY_IMPORTS);
+    insta::assert_snapshot!(acquire_errors);
+}
+
+#[test]
+fn test_unknown_field_audit() {
+    let audits = r##"
+# cargo-vet audits file
+
+[[audits.zzz]]
+criteria = "safe-to-deploy"
+version = "2.0.0"
+unknown-field = "invalid"
+"##;
+
+    let acquire_errors = get_valid_store(EMPTY_CONFIG, audits, EMPTY_IMPORTS);
+    insta::assert_snapshot!(acquire_errors);
+}
+
 #[test]
 fn test_invalid_formatting() {
     let config = r##"
@@ -256,12 +316,10 @@ criteria = "safe-to-deploy"
 [criteria.good]
 description = "great"
 implies = "safe-to-deploy"
-unrecognized-field = "hey there!"
 
 [[audits.serde]]
 criteria = ["safe-to-deploy", "good"]
 version = "2.0.0"
-note = "invalid field"
 
 [[audits.serde]]
 criteria = ["safe-to-deploy", "good"]