Repository: mozilla/cargo-vet
Pull request #520 (2023-07-25 17:57:42)

Add an importable flag to audits
This adds a new option to audits, `importable`, which can be set to `false` to mark an audit such that it will be ignored when importing. This will be enabled by default for new audits containing git revisions, allowing those entries to be cleaned up when they are no longer necessary.

Currently this patch adds no mechanism to automatically collapse sequences of non-importable audits or similar, so cleanup will still need to be manual. This can be added in the future in a backwards-compatible way.


Patch:
diff --git a/src/format.rs b/src/format.rs
index 612e02a..6a48d71 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -281,6 +281,7 @@ pub struct AuditEntry {
     pub who: Vec<Spanned<String>>,
     pub criteria: Vec<Spanned<CriteriaName>>,
     pub kind: AuditKind,
+    pub importable: bool,
     pub notes: Option<String>,
     /// Chain of sources this audit was aggregated from, most recent last.
     pub aggregated_from: Vec<Spanned<String>>,
diff --git a/src/main.rs b/src/main.rs
index bbbaaf5..606f4b3 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -918,6 +918,7 @@ fn do_cmd_certify(
     let criteria = criteria_names.into_iter().map(|s| s.into()).collect();
     match kind {
         CertifyKind::Full { version } => {
+            let importable = version.git_rev.is_none();
             store
                 .audits
                 .audits
@@ -927,12 +928,14 @@ fn do_cmd_certify(
                     kind: AuditKind::Full { version },
                     criteria,
                     who,
+                    importable,
                     notes,
                     aggregated_from: vec![],
                     is_fresh_import: false,
                 });
         }
         CertifyKind::Delta { from, to } => {
+            let importable = from.git_rev.is_none() && to.git_rev.is_none();
             store
                 .audits
                 .audits
@@ -942,6 +945,7 @@ fn do_cmd_certify(
                     kind: AuditKind::Delta { from, to },
                     criteria,
                     who,
+                    importable,
                     notes,
                     aggregated_from: vec![],
                     is_fresh_import: false,
@@ -1561,6 +1565,7 @@ fn cmd_record_violation(
         kind,
         criteria,
         who,
+        importable: true,
         notes,
         aggregated_from: vec![],
         is_fresh_import: false,
@@ -2431,14 +2436,15 @@ fn do_aggregate_audits(sources: Vec<(String, AuditsFile)>) -> Result<AuditsFile,
             }
         }
         for (package_name, entries) in audit_file.audits {
-            aggregate
-                .audits
-                .entry(package_name)
-                .or_default()
-                .extend(entries.into_iter().map(|mut audit_entry| {
-                    audit_entry.aggregated_from.push(source.clone().into());
-                    audit_entry
-                }));
+            aggregate.audits.entry(package_name).or_default().extend(
+                entries
+                    .into_iter()
+                    .filter(|audit_entry| audit_entry.importable)
+                    .map(|mut audit_entry| {
+                        audit_entry.aggregated_from.push(source.clone().into());
+                        audit_entry
+                    }),
+            );
         }
         for (package_name, entries) in audit_file.wildcard_audits {
             aggregate
diff --git a/src/serialization.rs b/src/serialization.rs
index 5562e66..1efdef4 100644
--- a/src/serialization.rs
+++ b/src/serialization.rs
@@ -303,6 +303,7 @@ pub mod audit {
         version: Option<VetVersion>,
         delta: Option<Delta>,
         violation: Option<VersionReq>,
+        importable: Option<bool>,
         notes: Option<String>,
         #[serde(rename = "aggregated-from")]
         #[serde(skip_serializing_if = "Vec::is_empty")]
@@ -334,6 +335,7 @@ pub mod audit {
                 notes: val.notes,
                 criteria: val.criteria,
                 kind: kind?,
+                importable: val.importable.unwrap_or(true),
                 aggregated_from: val.aggregated_from,
                 // By default, always read entries as non-fresh. The import code
                 // will set this flag to true for imported entries.
@@ -363,6 +365,7 @@ pub mod audit {
                 version,
                 delta,
                 violation,
+                importable: if val.importable { None } else { Some(false) },
                 aggregated_from: val.aggregated_from,
             }
         }
diff --git a/src/storage.rs b/src/storage.rs
index 50ec3c7..4369838 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -1271,6 +1271,8 @@ pub(crate) fn foreign_audit_file_to_local(
                         None
                     }
                 })
+                // Filter out non-importable audits
+                .filter(|value| value.importable)
                 .collect();
             (package, parsed)
         })


Test patch:
diff --git a/src/tests/certify.rs b/src/tests/certify.rs
index ae05777..9c25d42 100644
--- a/src/tests/certify.rs
+++ b/src/tests/certify.rs
@@ -210,6 +210,68 @@ fn mock_delta_certify_flow() {
     insta::assert_snapshot!("mock-delta-certify-flow", result);
 }
 
+#[test]
+fn mock_delta_git_certify_flow() {
+    let mock = MockMetadata::simple();
+
+    let _enter = TEST_RUNTIME.enter();
+    let metadata = mock.metadata();
+
+    let (config, audits, imports) = files_inited(&metadata);
+
+    let mut store = Store::mock(config, audits, imports);
+
+    let output = BasicTestOutput::with_callbacks(
+        |_| Ok("\n".to_owned()),
+        |_| {
+            Ok("\
+            I, testing, certify that I have audited the changes from version 10.0.0 to 10.0.0@git:00112233445566778899aabbccddeeff00112233 of third-party1 in accordance with the above criteria.\n\
+            \n\
+            These are testing notes. They contain some\n\
+            newlines. Trailing whitespace        \n    \
+            and leading whitespace\n\
+            \n".to_owned())
+        },
+    );
+
+    let cfg = mock_cfg_args(
+        &metadata,
+        [
+            "cargo",
+            "vet",
+            "certify",
+            "third-party1",
+            "10.0.0",
+            "10.0.0@git:00112233445566778899aabbccddeeff00112233",
+            "--who",
+            "testing",
+            "--criteria",
+            "safe-to-deploy",
+        ],
+    );
+    let sub_args = if let Some(crate::cli::Commands::Certify(sub_args)) = &cfg.cli.command {
+        sub_args
+    } else {
+        unreachable!();
+    };
+
+    crate::do_cmd_certify(
+        &output.clone().as_dyn(),
+        &cfg,
+        sub_args,
+        &mut store,
+        None,
+        None,
+    )
+    .expect("do_cmd_certify failed");
+
+    let audits = crate::serialization::to_formatted_toml(&store.audits, None).unwrap();
+
+    let result = format!("OUTPUT:\n{output}\nAUDITS:\n{audits}");
+
+    insta::assert_snapshot!("mock-delta-git-certify-flow", result);
+}
+
 #[test]
 fn mock_wildcard_certify_flow() {
     let mock = MockMetadata::simple();
diff --git a/src/tests/import.rs b/src/tests/import.rs
index 1c1b96e..88051cb 100644
--- a/src/tests/import.rs
+++ b/src/tests/import.rs
@@ -1510,6 +1510,12 @@ fn foreign_audit_file_to_local() {
                         version = "20.0.0"
                         unknown = "invalid unknown field"
                     },
+                    toml::toml! {
+                        criteria = "safe-to-deploy"
+                        version = "10.0.0"
+                        importable = false
+                        notes = "parses correctly, but will be ignored"
+                    },
                 ],
             ),
             (
@@ -1545,6 +1551,15 @@ fn foreign_audit_file_to_local() {
                     },
                 ],
             ),
+            (
+                "crate-d".to_string(),
+                vec![toml::toml! {
+                    criteria = "safe-to-deploy"
+                    version = "10.0.0"
+                    importable = false
+                    notes = "parses correctly, but will be ignored, along with the entire crate"
+                }],
+            ),
         ]
         .into_iter()
         .collect(),
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index adfff59..7895176 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -184,6 +184,7 @@ fn delta_audit(from: VetVersion, to: VetVersion, criteria: CriteriaStr) -> Audit
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Delta { from, to },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
@@ -195,6 +196,7 @@ fn full_audit(version: VetVersion, criteria: CriteriaStr) -> AuditEntry {
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Full { version },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
@@ -209,6 +211,7 @@ fn full_audit_m(
         notes: None,
         criteria: criteria.into_iter().map(|s| s.into().into()).collect(),
         kind: AuditKind::Full { version },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
@@ -220,6 +223,7 @@ fn violation_hard(version: VersionReq) -> AuditEntry {
         notes: None,
         criteria: vec![SAFE_TO_RUN.to_string().into()],
         kind: AuditKind::Violation { violation: version },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
@@ -231,6 +235,7 @@ fn violation(version: VersionReq, criteria: CriteriaStr) -> AuditEntry {
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Violation { violation: version },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
@@ -245,6 +250,7 @@ fn violation_m(
         notes: None,
         criteria: criteria.into_iter().map(|s| s.into().into()).collect(),
         kind: AuditKind::Violation { violation: version },
+        importable: true,
         aggregated_from: vec![],
         is_fresh_import: false,
     }
diff --git a/src/tests/snapshots/cargo_vet__tests__certify__mock-delta-git-certify-flow.snap b/src/tests/snapshots/cargo_vet__tests__certify__mock-delta-git-certify-flow.snap
new file mode 100644
index 0000000..38ce991
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__certify__mock-delta-git-certify-flow.snap
@@ -0,0 +1,75 @@
+---
+source: src/tests/certify.rs
+expression: result
+---
+OUTPUT:
+<<<EDITING VET_CERTIFY>>>
+# Please read the following criteria and then follow the instructions below:
+
+# === BEGIN CRITERIA "safe-to-deploy" ===
+#
+# This crate will not introduce a serious security vulnerability to production
+# software exposed to untrusted input.
+#
+# Auditors are not required to perform a full logic review of the entire crate.
+# Rather, they must review enough to fully reason about the behavior of all unsafe
+# blocks and usage of powerful imports. For any reasonable usage of the crate in
+# real-world software, an attacker must not be able to manipulate the runtime
+# behavior of these sections in an exploitable or surprising way.
+#
+# Ideally, all unsafe code is fully sound, and ambient capabilities (e.g.
+# filesystem access) are hardened against manipulation and consistent with the
+# advertised behavior of the crate. However, some discretion is permitted. In such
+# cases, the nature of the discretion should be recorded in the `notes` field of
+# the audit record.
+#
+# For crates which generate deployed code (e.g. build dependencies or procedural
+# macros), reasonable usage of the crate should output code which meets the above
+# criteria.
+#
+# === END CRITERIA ===
+#
+# Uncomment the following statement:
+
+# I, testing, certify that I have audited the changes from version 10.0.0 to 10.0.0@git:00112233445566778899aabbccddeeff00112233 of third-party1 in accordance with the above criteria.
+
+# Add any notes about your audit below this line:
+
+
+<<<EDIT OK>>>
+I, testing, certify that I have audited the changes from version 10.0.0 to 10.0.0@git:00112233445566778899aabbccddeeff00112233 of third-party1 in accordance with the above criteria.
+
+These are testing notes. They contain some
+newlines. Trailing whitespace        
+    and leading whitespace
+
+
+<<<END EDIT>>>
+
+AUDITS:
+
+[criteria.fuzzed]
+description = "fuzzed"
+
+[criteria.reviewed]
+description = "reviewed"
+implies = "weak-reviewed"
+
+[criteria.strong-reviewed]
+description = "strongly reviewed"
+implies = "reviewed"
+
+[criteria.weak-reviewed]
+description = "weakly reviewed"
+
+[[audits.third-party1]]
+who = "testing"
+criteria = "safe-to-deploy"
+delta = "10.0.0 -> 10.0.0@git:00112233445566778899aabbccddeeff00112233"
+importable = false
+notes = """
+These are testing notes. They contain some
+newlines. Trailing whitespace
+    and leading whitespace
+"""
+
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
index 3c373bd..fc40615 100644
--- 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
@@ -4,8 +4,8 @@ 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
+      `version`, `delta`, `violation`, `importable`, `notes`, `aggregated-
+      from` for key `audits.zzz` at line 4 column 1
    ╭─[audits.toml:3:1]
  3 │ 
  4 │ [[audits.zzz]]
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
index 4d99401..b9d6a10 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
index cf618e8..c085d3b 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
index cc67224..e422abb 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
index 9e64225..bc51782 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
@@ -17,6 +17,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -25,6 +26,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
index 230f1f0..5d7c18b 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=10",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
index 34e2aab..2c14ea7 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=10",
+            "importable": null,
             "notes": null
           },
           "exemptions": {
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
index 0128f75..bfd6088 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=5.0.0",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": null,
             "delta": "3.0.0 -> 5.0.0",
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
@@ -34,6 +36,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=5.0.0",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -42,6 +45,7 @@ expression: json
             "version": null,
             "delta": "5.0.0 -> 10.0.0",
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
index 0b9c9e3..5b1574a 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=3.0.0",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "3.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
@@ -34,6 +36,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "=3.0.0",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -42,6 +45,7 @@ expression: json
             "version": null,
             "delta": "3.0.0 -> 5.0.0",
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
index 51bf8ca..c406ac3 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
@@ -17,6 +17,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -25,6 +26,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
index 579eab5..f7060fe 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
@@ -14,6 +14,7 @@ expression: json
             "version": null,
             "delta": null,
             "violation": "*",
+            "importable": null,
             "notes": null
           },
           "audit_source": null,
@@ -22,6 +23,7 @@ expression: json
             "version": "10.0.0",
             "delta": null,
             "violation": null,
+            "importable": null,
             "notes": null
           }
         }