diff --git a/Cargo.lock b/Cargo.lock index 714d8083..82618f4b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -132,6 +132,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "similar", "tar", "tempfile", "textwrap", diff --git a/Cargo.toml b/Cargo.toml index 85a8e226..4ebaa759 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,3 +57,4 @@ features = [ [dev-dependencies] insta = "1.15.0" +similar = "2.1.0" diff --git a/src/errors.rs b/src/errors.rs index 56e352cd..8e524071 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -185,6 +185,12 @@ pub enum StoreAcquireError { #[error(transparent)] #[diagnostic(transparent)] Validate(#[from] StoreValidateErrors), + #[error(transparent)] + #[diagnostic(transparent)] + FetchAuditError(#[from] FetchAuditError), + #[diagnostic(transparent)] + #[error(transparent)] + CriteriaChange(#[from] CriteriaChangeErrors), } #[derive(Debug, Error, Diagnostic)] @@ -203,6 +209,26 @@ pub enum StoreCommitError { ), } +#[derive(Debug, Error, Diagnostic)] +#[error("Some of your imported audits changed their criteria descriptions")] +#[diagnostic()] +pub struct CriteriaChangeErrors { + #[related] + pub errors: Vec, +} +#[derive(Debug, Error, Diagnostic)] +// FIXME: it would be rad if this was a diff! +#[error( + "{import_name}'s '{criteria_name}' criteria changed from\n\n{old_desc}\n\nto\n\n{new_desc}\n" +)] +#[diagnostic(help("Run `cargo vet regenerate imports` to accept this new definition"))] +pub struct CriteriaChangeError { + pub import_name: ImportName, + pub criteria_name: ForeignCriteriaName, + pub old_desc: String, + pub new_desc: String, +} + //////////////////////////////////////////////////////////// // StoreValidateErrors //////////////////////////////////////////////////////////// @@ -436,38 +462,25 @@ pub enum FetchAuditError { #[source] error: url::ParseError, }, + #[error("{import_name}'s '{criteria_name}' criteria is missing a description")] + MissingCriteriaDescription { + import_name: ImportName, + criteria_name: ForeignCriteriaName, + }, + #[error("{import_name}'s '{criteria_name}' criteria description URI is invalid: '{url}'")] + InvalidCriteriaDescriptionUrl { + import_name: ImportName, + criteria_name: ForeignCriteriaName, + url: String, + #[source] + error: url::ParseError, + }, #[diagnostic(transparent)] #[error(transparent)] Download(#[from] DownloadError), #[diagnostic(transparent)] #[error(transparent)] Toml(#[from] LoadTomlError), - #[diagnostic(transparent)] - #[error(transparent)] - Validate(#[from] StoreValidateErrors), - #[diagnostic(transparent)] - #[error(transparent)] - CriteriaChange(#[from] CriteriaChangeErrors), -} - -#[derive(Debug, Error, Diagnostic)] -#[error("Some of your imported audits changed their criteria descriptions")] -#[diagnostic()] -pub struct CriteriaChangeErrors { - #[related] - pub errors: Vec, -} -#[derive(Debug, Error, Diagnostic)] -// FIXME: it would be rad if this was a diff! -#[error( - "{import_name}'s '{criteria_name}' criteria changed from\n\n{old_desc}\n\nto\n\n{new_desc}\n" -)] -#[diagnostic(help("Run `cargo vet regenerate imports` to accept this new definition"))] -pub struct CriteriaChangeError { - pub import_name: ImportName, - pub criteria_name: ForeignCriteriaName, - pub old_desc: String, - pub new_desc: String, } ////////////////////////////////////////////////////////// diff --git a/src/format.rs b/src/format.rs index 458a1102..02e03b0b 100644 --- a/src/format.rs +++ b/src/format.rs @@ -162,6 +162,14 @@ pub struct AuditEntry { pub criteria: Vec>, pub kind: AuditKind, pub notes: Option, + /// A non-serialized member which indicates whether this audit is a "fresh" + /// audit. This will be set for all audits imported found in the remote + /// audits file which aren't also found in the local `imports.lock` cache. + /// + /// This should almost always be `false`, and only set to `true` by the + /// import handling code. + #[serde(skip)] + pub is_fresh_import: bool, } /// Implement PartialOrd manually because the order we want for sorting is diff --git a/src/main.rs b/src/main.rs index 186e465d..ee00422f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -524,9 +524,9 @@ fn cmd_inspect( let package = &*sub_args.package; let fetched = { - let store = Store::acquire(cfg)?; - let cache = Cache::acquire(cfg)?; let network = Network::acquire(cfg); + let store = Store::acquire(cfg, network.as_ref(), false)?; + let cache = Cache::acquire(cfg)?; // Record this command for magic in `vet certify` cache.set_last_fetch(FetchCommand::Inspect { @@ -607,14 +607,18 @@ fn cmd_certify( sub_args: &CertifyArgs, ) -> Result<(), miette::Report> { // Certify that you have reviewed a crate's source for some version / delta - let mut store = Store::acquire(cfg)?; let network = Network::acquire(cfg); + let mut store = Store::acquire(cfg, network.as_ref(), false)?; // Grab the last fetch and immediately drop the cache let last_fetch = Cache::acquire(cfg)?.get_last_fetch(); do_cmd_certify(out, cfg, sub_args, &mut store, network.as_ref(), last_fetch)?; + // Re-run the resolver after adding the new `certify`. This will be used to + // potentially update imports. + store.maybe_update_imports_file(cfg, false); + store.commit()?; Ok(()) } @@ -707,7 +711,7 @@ fn do_cmd_certify( let criteria_mapper = CriteriaMapper::new( &store.audits.criteria, - &store.imports, + store.imported_audits(), &store.config.imports, ); @@ -914,6 +918,7 @@ fn do_cmd_certify( .collect(), who, notes, + is_fresh_import: false, }; store @@ -1087,7 +1092,7 @@ fn cmd_record_violation( sub_args: &RecordViolationArgs, ) -> Result<(), miette::Report> { // Mark a package as a violation - let mut store = Store::acquire(cfg)?; + let mut store = Store::acquire_offline(cfg)?; let kind = AuditKind::Violation { violation: sub_args.versions.clone(), @@ -1134,6 +1139,7 @@ fn cmd_record_violation( criteria, who, notes, + is_fresh_import: false, }; store @@ -1156,7 +1162,7 @@ fn cmd_add_exemption( sub_args: &AddExemptionArgs, ) -> Result<(), miette::Report> { // Add an exemption entry - let mut store = Store::acquire(cfg)?; + let mut store = Store::acquire_offline(cfg)?; let dependency_criteria = if sub_args.dependency_criteria.is_empty() { // TODO: look at the current audits to infer this? prompt? @@ -1229,8 +1235,8 @@ fn cmd_suggest( ) -> Result<(), miette::Report> { // Run the checker to validate that the current set of deps is covered by the current cargo vet store trace!("suggesting..."); - let suggest_store = Store::acquire(cfg)?.clone_for_suggest(); let network = Network::acquire(cfg); + let suggest_store = Store::acquire(cfg, network.as_ref(), false)?.clone_for_suggest(); // DO THE THING!!!! let report = resolver::resolve( @@ -1261,28 +1267,21 @@ fn cmd_regenerate_imports( ) -> Result<(), miette::Report> { trace!("regenerating imports..."); - let mut store = Store::acquire(cfg)?; - let network = Network::acquire(cfg); - - if let Some(network) = &network { - if !cfg.cli.locked { - // Literally the only difference between this command and fetch-imports - // is that we pass `accept_changes = true` - tokio::runtime::Handle::current() - .block_on(store.fetch_foreign_audits(network, true))?; - store.commit()?; - return Ok(()); - } + if cfg.cli.locked { + // ERRORS: just a warning that you're holding it wrong, unclear if immediate or buffered, + // or if this should be a hard error, or if we should ignore the --locked flag and + // just do it anyway + writeln!( + out, + "warning: ran `regenerate imports` with --locked, this won't do anything!" + ); + return Ok(()); } - // ERRORS: just a warning that you're holding it wrong, unclear if immediate or buffered, - // or if this should be a hard error, or if we should ignore the --locked flag and - // just do it anyway - writeln!( - out, - "warning: ran `regenerate imports` with --locked, this won't do anything!" - ); - + let network = Network::acquire(cfg); + let mut store = Store::acquire(cfg, network.as_ref(), true)?; + store.maybe_update_imports_file(cfg, true); + store.commit()?; Ok(()) } @@ -1292,7 +1291,7 @@ fn cmd_regenerate_audit_as( _sub_args: &RegenerateAuditAsCratesIoArgs, ) -> Result<(), miette::Report> { trace!("regenerating audit-as-crates-io..."); - let mut store = Store::acquire(cfg)?; + let mut store = Store::acquire_offline(cfg)?; fix_audit_as(cfg, &mut store)?; @@ -1347,8 +1346,8 @@ fn cmd_regenerate_exemptions( _sub_args: &RegenerateExemptionsArgs, ) -> Result<(), miette::Report> { trace!("regenerating exemptions..."); - let mut store = Store::acquire(cfg)?; let network = Network::acquire(cfg); + let mut store = Store::acquire(cfg, network.as_ref(), false)?; minimize_exemptions(cfg, &mut store, network.as_ref())?; @@ -1457,6 +1456,9 @@ pub fn minimize_exemptions( // Alright there's the new exemptions store.config.exemptions = new_exemptions; + // Re-vet and ensure that imports are updated after changing exemptions. + store.maybe_update_imports_file(cfg, false); + Ok(()) } @@ -1466,9 +1468,9 @@ fn cmd_diff(out: &Arc, cfg: &Config, sub_args: &DiffArgs) -> Result<(), let package = &*sub_args.package; let (fetched1, fetched2) = { - let store = Store::acquire(cfg)?; - let cache = Cache::acquire(cfg)?; let network = Network::acquire(cfg); + let store = Store::acquire(cfg, network.as_ref(), false)?; + let cache = Cache::acquire(cfg)?; // Record this command for magic in `vet certify` cache.set_last_fetch(FetchCommand::Diff { @@ -1552,16 +1554,10 @@ fn cmd_check(out: &Arc, cfg: &Config, sub_args: &CheckArgs) -> Result<( // Run the checker to validate that the current set of deps is covered by the current cargo vet store trace!("vetting..."); - let mut store = Store::acquire(cfg)?; let network = Network::acquire(cfg); + let mut store = Store::acquire(cfg, network.as_ref(), false)?; if !cfg.cli.locked { - // Try to update the foreign audits (imports) - if let Some(network) = &network { - tokio::runtime::Handle::current() - .block_on(store.fetch_foreign_audits(network, false))?; - } - // Check if any of our first-parties are in the crates.io registry check_audit_as_crates_io(cfg, &store)?; } @@ -1598,6 +1594,7 @@ fn cmd_check(out: &Arc, cfg: &Config, sub_args: &CheckArgs) -> Result<( // Err(eyre!("report contains errors"))?; panic_any(ExitPanic(-1)); } else { + store.imports = store.get_updated_imports_file(&report, false); store.commit()?; } @@ -1611,25 +1608,23 @@ fn cmd_fetch_imports( ) -> Result<(), miette::Report> { trace!("fetching imports..."); - let mut store = Store::acquire(cfg)?; - let network = Network::acquire(cfg); + if cfg.cli.locked { + // ERRORS: just a warning that you're holding it wrong, unclear if immediate or buffered, + // or if this should be a hard error, or if we should ignore the --locked flag and + // just do it anyway + writeln!( + out, + "warning: ran fetch-imports with --locked, this won't do anything!" + ); - if let Some(network) = &network { - if !cfg.cli.locked { - tokio::runtime::Handle::current() - .block_on(store.fetch_foreign_audits(network, false))?; - store.commit()?; - return Ok(()); - } + return Ok(()); } - // ERRORS: just a warning that you're holding it wrong, unclear if immediate or buffered, - // or if this should be a hard error, or if we should ignore the --locked flag and - // just do it anyway - writeln!( - out, - "warning: ran fetch-imports with --locked, this won't do anything!" - ); + let network = Network::acquire(cfg); + let mut store = Store::acquire(cfg, network.as_ref(), false)?; + + store.maybe_update_imports_file(cfg, true); + store.commit()?; Ok(()) } @@ -1656,7 +1651,8 @@ fn cmd_dump_graph( fn cmd_fmt(_out: &Arc, cfg: &Config, _sub_args: &FmtArgs) -> Result<(), miette::Report> { // Reformat all the files (just load and store them, formatting is implicit). trace!("formatting..."); - let store = Store::acquire(cfg)?; + // We don't need to fetch foreign audits to format files + let store = Store::acquire_offline(cfg)?; store.commit()?; Ok(()) } diff --git a/src/resolver.rs b/src/resolver.rs index ae935a6a..2dd6e20c 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -67,13 +67,15 @@ use futures_util::future::join_all; use miette::IntoDiagnostic; use serde::Serialize; use serde_json::json; +use std::cmp::Reverse; +use std::collections::BinaryHeap; use std::sync::Arc; use tracing::{error, trace, trace_span}; use crate::errors::SuggestError; use crate::format::{ - self, AuditKind, CriteriaName, CriteriaStr, Delta, DiffStat, ExemptedDependency, ImportName, - ImportsFile, PackageName, PackageStr, PolicyEntry, RemoteImport, SAFE_TO_DEPLOY, SAFE_TO_RUN, + self, AuditKind, AuditsFile, CriteriaName, CriteriaStr, Delta, DiffStat, ExemptedDependency, + ImportName, PackageName, PackageStr, PolicyEntry, RemoteImport, SAFE_TO_DEPLOY, SAFE_TO_RUN, }; use crate::format::{FastMap, FastSet, SortedMap, SortedSet}; use crate::network::Network; @@ -116,6 +118,8 @@ pub struct Success { pub vetted_partially: Vec, /// Third-party packages that were successfully vetted using only 'audits' pub vetted_fully: Vec, + /// Third-party packages that needed fresh imports to be successfully vetted + pub needed_fresh_imports: SortedSet, } #[derive(Debug, Clone)] @@ -291,14 +295,10 @@ pub struct DepGraph<'a> { pub struct ResolveResult<'a> { /// The set of criteria we validated for this package. pub validated_criteria: CriteriaSet, - /// The set of criteria we validated for this package without 'exemptions' entries. - pub fully_audited_criteria: CriteriaSet, /// Individual search results for each criteria. pub search_results: Vec>, /// Whether there was an exemption for this exact version. pub directly_exempted: bool, - /// Whether we ever needed the not-fully_audited_criteria for our reverse-deps. - pub needed_exemption: bool, } pub type PolicyFailures = SortedMap; @@ -315,8 +315,9 @@ pub struct AuditFailure { pub enum SearchResult<'a> { /// We found a path, criteria validated. Connected { - /// Whether we found a path to a fully_audited entry - fully_audited: bool, + /// Caveats which were required to build the audit chain for this + /// Criteria. + caveats: Caveats, }, /// We failed to find a *proper* path, criteria not valid, but adding in failing /// edges caused by our dependencies not meeting criteria created a connection! @@ -340,6 +341,29 @@ pub enum SearchResult<'a> { }, } +// NOTE: There's probably a more efficient representation for these in the +// general case. +/// Caveats which apply to the results of an audit. +#[derive(Debug, Clone, Default)] +pub struct Caveats { + /// The set of packages which required exemptions in order to successfully + /// audit. + pub needed_exemptions: SortedSet, + + /// The set of packages which required fresh imports in order to + /// successfully audit. + pub needed_fresh_imports: SortedSet, +} + +impl Caveats { + /// Union the given caveat set with this set of caveats, mutating `self`. + fn add(&mut self, other: &Caveats) { + self.needed_exemptions.extend(&other.needed_exemptions); + self.needed_fresh_imports + .extend(&other.needed_fresh_imports); + } +} + /// A graph of the audits for a package. /// /// The nodes of the graph are Versions and the edges are audits. @@ -367,6 +391,21 @@ pub enum SearchResult<'a> { /// used for `suggest`. pub type AuditGraph<'a> = SortedMap<&'a Version, Vec>>; +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +enum DeltaEdgeOrigin { + /// This edge represents an audit from the store, either within audits.toml + /// or within the cached imports.lock file. These edges will be tried first. + StoredAudit, + /// This edge represents an exemption. These edges will be tried after + /// stored audits, but before freshly-imported audits have been attempted. + Exemption, + /// This edge represents an imported audit from a peer which is only present + /// on the remote server, and not present in the cached imports.lock file. + /// These edges will be tried after all locally-available audits have been + /// attempted. + FreshImportedAudit, +} + /// 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. @@ -379,9 +418,9 @@ pub struct DeltaEdge<'a> { /// Requirements that dependencies must satisfy for the edge to be valid. /// If a dependency isn't mentioned, then it defaults to `criteria`. dependency_criteria: FastMap, CriteriaSet>, - /// Whether this edge represents an exemption. These will initially - /// be ignored, and then used only if we can't find a path. - is_exemption: bool, + /// The origin of this edge. See `DeltaEdgeOrigin`'s documentation for more + /// details. + origin: DeltaEdgeOrigin, } const NUM_BUILTINS: usize = 2; @@ -405,7 +444,7 @@ fn builtin_criteria() -> [CriteriaInfo; NUM_BUILTINS] { impl CriteriaMapper { pub fn new( criteria: &SortedMap, - imports: &ImportsFile, + imports: &SortedMap, mappings: &SortedMap, ) -> CriteriaMapper { // First, build a list of all our criteria @@ -416,7 +455,7 @@ impl CriteriaMapper { implies: v.implies.iter().map(|s| s.to_string()).collect::>(), }); let builtins = builtin_criteria(); - let foreigns = imports.audits.iter().flat_map(|(import, audit_file)| { + let foreigns = imports.iter().flat_map(|(import, audit_file)| { audit_file.criteria.iter().map(move |(k, v)| CriteriaInfo { namespace: CriteriaNamespace::Foreign(import.clone()), raw_name: k.clone(), @@ -800,33 +839,9 @@ impl CriteriaFailureSet { impl ResolveResult<'_> { fn with_no_criteria(empty: CriteriaSet) -> Self { Self { - validated_criteria: empty.clone(), - fully_audited_criteria: empty, + validated_criteria: empty, search_results: vec![], directly_exempted: false, - needed_exemption: false, - } - } - - fn contains(&mut self, other: &CriteriaSet) -> bool { - if self.fully_audited_criteria.contains(other) { - true - } else if self.validated_criteria.contains(other) { - self.needed_exemption = true; - true - } else { - false - } - } - - fn _has_criteria(&mut self, criteria_idx: usize) -> bool { - if self.fully_audited_criteria.has_criteria(criteria_idx) { - true - } else if self.validated_criteria.has_criteria(criteria_idx) { - self.needed_exemption = true; - true - } else { - false } } } @@ -1294,7 +1309,6 @@ impl<'a> DepGraph<'a> { // Dummy values for corner cases pub static ROOT_VERSION: Version = Version::new(0, 0, 0); -static NO_AUDITS: Vec = Vec::new(); #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub enum ResolveDepth { @@ -1317,7 +1331,7 @@ pub fn resolve<'a>( let criteria_mapper = CriteriaMapper::new( &store.audits.criteria, - &store.imports, + store.imported_audits(), &store.config.imports, ); trace!("built CriteriaMapper!"); @@ -1327,6 +1341,7 @@ pub fn resolve<'a>( vec![ResolveResult::with_no_criteria(criteria_mapper.no_criteria()); graph.nodes.len()]; let mut root_failures = RootFailures::new(); let mut violations = SortedMap::new(); + let mut root_caveats = Caveats::default(); // Actually vet the build graph for &pkgidx in &graph.topo_index { @@ -1364,6 +1379,7 @@ pub fn resolve<'a>( &mut results, &mut violations, &mut root_failures, + &mut root_caveats, pkgidx, ); } @@ -1404,6 +1420,7 @@ pub fn resolve<'a>( &mut results, &mut violations, &mut root_failures, + &mut root_caveats, pkgidx, ); } else { @@ -1499,7 +1516,7 @@ pub fn resolve<'a>( } let result = &results[pkgidx]; - if !result.needed_exemption { + if !root_caveats.needed_exemptions.contains(&pkgidx) { vetted_fully.push(pkgidx); } else if result.directly_exempted { vetted_with_exemptions.push(pkgidx); @@ -1516,6 +1533,7 @@ pub fn resolve<'a>( vetted_with_exemptions, vetted_partially, vetted_fully, + needed_fresh_imports: root_caveats.needed_fresh_imports, }), } } @@ -1532,19 +1550,41 @@ fn resolve_third_party<'a>( let package = &graph.nodes[pkgidx]; let exemptions = store.config.exemptions.get(package.name); - let own_audits = ( - CriteriaNamespace::Local, - store.audits.audits.get(package.name).unwrap_or(&NO_AUDITS), - ); - // FIXME: ideally we wouldn't clone the CriteriaNamespace every time we iterate all_criteria - // (which happens a lot when inspecting violations). - let foreign_audits = store.imports.audits.iter().map(|(import_name, audits)| { - ( - CriteriaNamespace::Foreign(import_name.clone()), - audits.audits.get(package.name).unwrap_or(&NO_AUDITS), - ) - }); - let all_audits = Some(own_audits).into_iter().chain(foreign_audits); + // Pre-build the namespaces for each audit so that we can take a reference + // to each one as-needed rather than cloning the name each time. + let foreign_namespaces: Vec = store + .imported_audits() + .keys() + .map(|import_name| CriteriaNamespace::Foreign(import_name.clone())) + .collect(); + + // Each of our own audits should be put into the "local" criteria namespace. + let own_audits = store + .audits + .audits + .get(package.name) + .map(|v| &v[..]) + .unwrap_or(&[]) + .iter() + .map(|audit| (&CriteriaNamespace::Local, audit)); + + // Each foreign audit should be put into a "foreign" criteria namespace. + let foreign_audits = store + .imported_audits() + .values() + .enumerate() + .flat_map(|(idx, audits)| { + let namespace = &foreign_namespaces[idx]; + audits + .audits + .get(package.name) + .map(|v| &v[..]) + .unwrap_or(&[]) + .iter() + .map(move |audit| (namespace, audit)) + }); + + let all_audits = own_audits.chain(foreign_audits); // See AuditGraph's docs for details on the lowering we do here let mut forward_audits = AuditGraph::new(); @@ -1552,49 +1592,53 @@ fn resolve_third_party<'a>( let mut violation_nodes = Vec::new(); // Collect up all the deltas, their criteria, and dependency_criteria - for (namespace, entries) in all_audits.clone() { - for entry in entries { - // For uniformity, model a Full Audit as `0.0.0 -> x.y.z` - let (from_ver, to_ver, dependency_criteria) = match &entry.kind { - AuditKind::Full { - version, - dependency_criteria, - } => (&ROOT_VERSION, version, dependency_criteria), - AuditKind::Delta { - delta, - dependency_criteria, - } => (&delta.from, &delta.to, dependency_criteria), - AuditKind::Violation { .. } => { - violation_nodes.push((namespace.clone(), entry)); - continue; - } - }; + for (namespace, entry) in all_audits.clone() { + // For uniformity, model a Full Audit as `0.0.0 -> x.y.z` + let (from_ver, to_ver, dependency_criteria) = match &entry.kind { + AuditKind::Full { + version, + dependency_criteria, + } => (&ROOT_VERSION, version, dependency_criteria), + AuditKind::Delta { + delta, + dependency_criteria, + } => (&delta.from, &delta.to, dependency_criteria), + AuditKind::Violation { .. } => { + violation_nodes.push((namespace.clone(), entry)); + continue; + } + }; - let criteria = criteria_mapper.criteria_from_namespaced_entry(&namespace, entry); - // Convert all the custom criteria to CriteriaSets - let dependency_criteria: FastMap<_, _> = dependency_criteria - .iter() - .map(|(pkg_name, criteria)| { - ( - &**pkg_name, - criteria_mapper.criteria_from_namespaced_list(&namespace, criteria), - ) - }) - .collect(); + let criteria = criteria_mapper.criteria_from_namespaced_entry(namespace, entry); + // Convert all the custom criteria to CriteriaSets + let dependency_criteria: FastMap<_, _> = dependency_criteria + .iter() + .map(|(pkg_name, criteria)| { + ( + &**pkg_name, + criteria_mapper.criteria_from_namespaced_list(namespace, criteria), + ) + }) + .collect(); - forward_audits.entry(from_ver).or_default().push(DeltaEdge { - version: to_ver, - criteria: criteria.clone(), - dependency_criteria: dependency_criteria.clone(), - is_exemption: false, - }); - backward_audits.entry(to_ver).or_default().push(DeltaEdge { - version: from_ver, - criteria, - dependency_criteria, - is_exemption: false, - }); - } + let origin = if entry.is_fresh_import { + DeltaEdgeOrigin::FreshImportedAudit + } else { + DeltaEdgeOrigin::StoredAudit + }; + + forward_audits.entry(from_ver).or_default().push(DeltaEdge { + version: to_ver, + criteria: criteria.clone(), + dependency_criteria: dependency_criteria.clone(), + origin, + }); + backward_audits.entry(to_ver).or_default().push(DeltaEdge { + version: from_ver, + criteria, + dependency_criteria, + origin, + }); } // Reject forbidden packages (violations) @@ -1652,47 +1696,42 @@ fn resolve_third_party<'a>( } // Note if this entry conflicts with any audits - for (namespace, audits) in all_audits.clone() { - for audit in audits { - let audit_criteria = - criteria_mapper.criteria_from_namespaced_entry(&namespace, audit); - let has_violation = violation_criterias - .iter() - .any(|v| audit_criteria.contains(v)); - if !has_violation { - continue; - } - match &audit.kind { - AuditKind::Full { version, .. } => { - if violation_range.matches(version) { - violations.entry(pkgidx).or_default().push( - ViolationConflict::AuditConflict { - violation_source: violation_source.clone(), - violation: (*violation_entry).clone(), - audit_source: namespace.clone(), - audit: audit.clone(), - }, - ); - } - } - AuditKind::Delta { delta, .. } => { - if violation_range.matches(&delta.from) - || violation_range.matches(&delta.to) - { - violations.entry(pkgidx).or_default().push( - ViolationConflict::AuditConflict { - violation_source: violation_source.clone(), - violation: (*violation_entry).clone(), - audit_source: namespace.clone(), - audit: audit.clone(), - }, - ); - } + for (namespace, audit) in all_audits.clone() { + let audit_criteria = criteria_mapper.criteria_from_namespaced_entry(namespace, audit); + let has_violation = violation_criterias + .iter() + .any(|v| audit_criteria.contains(v)); + if !has_violation { + continue; + } + match &audit.kind { + AuditKind::Full { version, .. } => { + if violation_range.matches(version) { + violations.entry(pkgidx).or_default().push( + ViolationConflict::AuditConflict { + violation_source: violation_source.clone(), + violation: (*violation_entry).clone(), + audit_source: namespace.clone(), + audit: audit.clone(), + }, + ); } - AuditKind::Violation { .. } => { - // don't care + } + AuditKind::Delta { delta, .. } => { + if violation_range.matches(&delta.from) || violation_range.matches(&delta.to) { + violations.entry(pkgidx).or_default().push( + ViolationConflict::AuditConflict { + violation_source: violation_source.clone(), + violation: (*violation_entry).clone(), + audit_source: namespace.clone(), + audit: audit.clone(), + }, + ); } } + AuditKind::Violation { .. } => { + // don't care + } } } @@ -1743,19 +1782,18 @@ fn resolve_third_party<'a>( version: to_ver, criteria: criteria.clone(), dependency_criteria: dependency_criteria.clone(), - is_exemption: true, + origin: DeltaEdgeOrigin::Exemption, }); backward_audits.entry(to_ver).or_default().push(DeltaEdge { version: from_ver, criteria, dependency_criteria, - is_exemption: true, + origin: DeltaEdgeOrigin::Exemption, }); } } let mut validated_criteria = criteria_mapper.no_criteria(); - let mut fully_audited_criteria = criteria_mapper.no_criteria(); let mut search_results = vec![]; for criteria in criteria_mapper.all_criteria_iter() { let result = search_for_path( @@ -1765,17 +1803,15 @@ fn resolve_third_party<'a>( &forward_audits, graph, criteria_mapper, - package, results, + pkgidx, + &package.normal_and_build_deps, ); match result { - SearchResult::Connected { fully_audited } => { - // We found a path, hooray, criteria validated! - if fully_audited { - fully_audited_criteria.unioned_with(criteria); - } + SearchResult::Connected { caveats } => { + // We found a patch to satisfy this criteria. validated_criteria.unioned_with(criteria); - search_results.push(SearchResult::Connected { fully_audited }); + search_results.push(SearchResult::Connected { caveats }); } SearchResult::PossiblyConnected { failed_deps } => { // We failed but found a possible solution if our dependencies were better. @@ -1795,8 +1831,9 @@ fn resolve_third_party<'a>( &backward_audits, graph, criteria_mapper, - package, results, + pkgidx, + &package.normal_and_build_deps, ); if let SearchResult::Disconnected { reachable_from_root: reachable_from_target, @@ -1824,14 +1861,69 @@ fn resolve_third_party<'a>( // We've completed our graph analysis for this package, now record the results results[pkgidx] = ResolveResult { validated_criteria, - fully_audited_criteria, - directly_exempted, search_results, - // Only gets found out later, for now, assume not. - needed_exemption: false, + directly_exempted, }; } +/// Updates `caveats`, and `failed_deps` to include any caveats or failed +/// dependencies involved in requiring `dependencies` to satisfy the given +/// criteria. +#[allow(clippy::too_many_arguments)] +fn get_dependency_criteria_caveats( + dep_graph: &DepGraph, + criteria_mapper: &CriteriaMapper, + results: &[ResolveResult<'_>], + dependencies: &[PackageIdx], + base_criteria: &CriteriaSet, + dependency_criteria: &FastMap, CriteriaSet>, + caveats: &mut Caveats, + failed_deps: &mut SortedMap, +) { + for &depidx in dependencies { + let dep_package = &dep_graph.nodes[depidx]; + let dep_results = &results[depidx]; + + // If no custom criteria is specified, then require our dependency to match + // the base criteria that we're trying to validate. This makes audits effectively + // break down their criteria into individually verifiable components instead of + // purely "all or nothing". + // + // e.g. a safe-to-deploy audit with some deps that are only safe-to-run + // still audits for safe-to-run, but not safe-to-deploy. Similarly so for + // `[safe-to-run, some-other-criteria]` validating each criteria individually. + let dep_req = dependency_criteria + .get(dep_package.name) + .unwrap_or(base_criteria); + + if !dep_results.validated_criteria.contains(dep_req) { + // This dependency's criteria is not satisfied, so add it to the + // failed deps map. + failed_deps + .entry(depidx) + .or_insert_with(|| criteria_mapper.no_criteria()) + .unioned_with(dep_req); + continue; + } + + // Only iterate the minimal set of indices to reduce the number of + // search results we check for caveats, as implied results will have a + // subset of the caveats of the stronger criteria. + for required_criteria_idx in criteria_mapper.minimal_indices(dep_req) { + // Check if the original search results succeeded here, and if it + // did record the relevant caveats. It's OK if we don't see + // `Connected` here, as that just means a policy overwrote our + // failure. + if let SearchResult::Connected { + caveats: dep_caveats, + } = &dep_results.search_results[required_criteria_idx] + { + caveats.add(dep_caveats); + } + } + } +} + #[allow(clippy::too_many_arguments)] fn search_for_path<'a>( cur_criteria: &CriteriaSet, @@ -1840,160 +1932,143 @@ fn search_for_path<'a>( audit_graph: &AuditGraph<'a>, dep_graph: &DepGraph<'a>, criteria_mapper: &CriteriaMapper, - package: &PackageNode<'a>, - results: &mut [ResolveResult], + results: &[ResolveResult], + pkgidx: PackageIdx, + dependencies: &[PackageIdx], ) -> SearchResult<'a> { - // Search for any path through the graph with edges that satisfy cur_criteria. - // Finding any path validates that we satisfy that criteria. All we're doing is - // basic depth-first search with a manual stack. + // Search for any path through the graph with edges that satisfy + // cur_criteria. Finding any path validates that we satisfy that criteria. // - // All full-audits and exemptions have been "desugarred" to a delta from 0.0.0, - // meaning our graph now has exactly one source and one sink, significantly simplifying - // the start and end conditions. + // All full-audits and exemptions have been "desugarred" to a delta from + // 0.0.0, meaning our graph now has exactly one source and one sink, + // significantly simplifying the start and end conditions. // - // Because we want to know if the validation can be done without ever using an - // exemption, we initially "defer" using those edges. This is accomplished - // by wrapping the entire algorithm in a loop, and only taking those edges on the - // next iteration of the outer loop. So if we find a path in the first iteration, - // then that's an unambiguous proof that we didn't need those edges. + // Some edges have caveats which we want to avoid requiring, so we defer + // edges with caveats to be checked later, after all edges without caveats + // have been visited. This is done by storing work to do in a BinaryHeap, + // sorted by the caveats which apply to each node. This means that if we + // find a patch without using a node with caveats, it's unambiguous proof we + // don't need edges with caveats. // - // We apply this same "deferring" trick to edges which fail because of our dependencies. - // Once we run out of both 'exemptions' entries and still don't have a path, we start - // speculatively allowing ourselves to follow those edges. If we find a path by doing that - // then we can reliably "blame" our deps for our own failings. Otherwise we there is - // no possible path, and we are absolutely just missing reviews for ourself. - - // Conclusions - let mut found_path = false; - let mut needed_exemption = false; - let mut needed_failed_edges = false; - let mut failed_deps = SortedMap::::new(); - - // Search State - let mut search_stack = vec![from_version]; + // These caveats extend all the way from exemptions (the least-important + // caveat) to fresh imports and even failed edges (the most-important + // caveat). + struct Node<'a> { + version: &'a Version, + caveats: Caveats, + failed_deps: SortedMap, + } + + impl<'a> Node<'a> { + fn key(&self) -> Reverse<(usize, usize, usize)> { + // Nodes are compared by the number of failed dependencies, fresh + // imports, and exemptions, in that order. Fewer caveats makes the + // node sort higher, as it will be stored in a max heap. + Reverse(( + self.failed_deps.len(), + self.caveats.needed_fresh_imports.len(), + self.caveats.needed_exemptions.len(), + )) + } + } + impl<'a> PartialEq for Node<'a> { + fn eq(&self, other: &Self) -> bool { + self.key() == other.key() + } + } + impl<'a> Eq for Node<'a> {} + impl<'a> PartialOrd for Node<'a> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } + } + impl<'a> Ord for Node<'a> { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.key().cmp(&other.key()) + } + } + + let mut queue = BinaryHeap::new(); + queue.push(Node { + version: from_version, + caveats: Caveats::default(), + failed_deps: SortedMap::new(), + }); + let mut visited = SortedSet::new(); - let mut deferred_exemptions_entries = vec![]; - let mut deferred_failed_edges = vec![]; - - // Loop until we find a path or run out of deferred edges. - loop { - // If there are any deferred edges (only possible on iteration 2+), try to follow them. - // Always prefer following 'exemptions' edges, so that we only dip into failed edges when - // we've completely run out of options. - if let Some(node) = deferred_exemptions_entries.pop() { - // Don't bother if we got to that node some other way. - if visited.contains(node) { + while let Some(Node { + version, + caveats, + failed_deps, + }) = queue.pop() + { + // If We've been to a version before, We're not going to get a better + // result revisiting it, as we visit the "best" edges first. + if !visited.insert(version) { + continue; + } + + // We found a path! Return a search result reflecting what we + // discovered. + if version == to_version { + return if failed_deps.is_empty() { + SearchResult::Connected { caveats } + } else { + SearchResult::PossiblyConnected { failed_deps } + }; + } + + // Apply deltas to move along to the next layer of the search, adding it + // to our queue. + let edges = audit_graph.get(version).map(|v| &v[..]).unwrap_or(&[]); + for edge in edges { + if !edge.criteria.contains(cur_criteria) { + // This edge never would have been useful to us. continue; } - // Ok at this point we officially "need" the exemptions edge. If the search still - // fails, then we won't mention that we used this, since the graph is just broken - // and we can't make any conclusions about whether anything is needed or not! - needed_exemption = true; - search_stack.push(node); - } else if let Some(node) = deferred_failed_edges.pop() { - // Don't bother if we got to that node some other way. - if visited.contains(node) { + if visited.contains(edge.version) { + // We've been to the target of this edge already. continue; } - // Ok at this point we officially "need" the failed edge. If the search still - // fails, then we won't mention that we used this, since the graph is just broken - // and we can't make any conclusions about whether anything is needed or not! - needed_failed_edges = true; - search_stack.push(node); - } - - // Do Depth-First-Search - while let Some(cur_version) = search_stack.pop() { - // Don't revisit nodes, there's never an advantage to doing so, and because deltas - // can go both forwards and backwards in time, cycles are a real concern! - visited.insert(cur_version); - if cur_version == to_version { - // Success! Nothing more to do. - found_path = true; - break; - } - - // Apply deltas to move along to the next "layer" of the search - if let Some(edges) = audit_graph.get(cur_version) { - for edge in edges { - if !edge.criteria.contains(cur_criteria) { - // This edge never would have been useful to us - continue; - } - if visited.contains(edge.version) { - // We've been to this node already - continue; - } - // Deltas should only apply if dependencies satisfy dep_criteria - let mut deps_satisfied = true; - for &dependency in &package.normal_and_build_deps { - let dep_package = &dep_graph.nodes[dependency]; - let dep_vet_result = &mut results[dependency]; - - // If no custom criteria is specified, then require our dependency to match - // the same criteria that we're trying to validate. This makes audits effectively - // break down their criteria into individually verifiable components instead of - // purely "all or nothing". - // - // e.g. a safe-to-deploy audit with some deps that are only safe-to-run - // still audits for safe-to-run, but not safe-to-deploy. Similarly so for - // `[safe-to-run, some-other-criteria]` validating each criteria individually. - let dep_req = edge - .dependency_criteria - .get(dep_package.name) - .unwrap_or(cur_criteria); - - if !dep_vet_result.contains(dep_req) { - failed_deps - .entry(dependency) - .or_insert_with(|| criteria_mapper.no_criteria()) - .unioned_with(dep_req); - deps_satisfied = false; - } - } + let mut edge_caveats = caveats.clone(); + let mut edge_failed_deps = failed_deps.clone(); - if deps_satisfied { - // Ok yep, this edge is usable! But defer it if it's an exemption. - if edge.is_exemption { - deferred_exemptions_entries.push(edge.version); - } else { - search_stack.push(edge.version); - } - } else { - // Remember this edge failed, if we can't find any path we'll speculatively - // re-enable it. - deferred_failed_edges.push(edge.version); - } + match edge.origin { + DeltaEdgeOrigin::Exemption => { + edge_caveats.needed_exemptions.insert(pkgidx); } - } - } + DeltaEdgeOrigin::FreshImportedAudit => { + edge_caveats.needed_fresh_imports.insert(pkgidx); + } + DeltaEdgeOrigin::StoredAudit => {} + } + + get_dependency_criteria_caveats( + dep_graph, + criteria_mapper, + results, + dependencies, + cur_criteria, + &edge.dependency_criteria, + &mut edge_caveats, + &mut edge_failed_deps, + ); - // Exit conditions - if found_path - || (deferred_exemptions_entries.is_empty() && deferred_failed_edges.is_empty()) - { - break; + queue.push(Node { + version: edge.version, + caveats: edge_caveats, + failed_deps: edge_failed_deps, + }); } } - // It's only a success if we found a path and used no 'failed' edges. - if found_path && !needed_failed_edges { - // Complete success! - SearchResult::Connected { - fully_audited: !needed_exemption, - } - } else if found_path { - // Failure, but it's clearly the fault of our deps. - SearchResult::PossiblyConnected { failed_deps } - } else { - // Complete failure, we need more audits of ourself, - // so all that matters is what nodes were reachable. - SearchResult::Disconnected { - reachable_from_root: visited, - // This will get filled in by the second pass - reachable_from_target: Default::default(), - } + // Complete failure, we need more audits for this package, so all that + // matters is what nodes were reachable. + SearchResult::Disconnected { + reachable_from_root: visited, + // This will get filled in by a second pass. + reachable_from_target: Default::default(), } } @@ -2028,28 +2103,26 @@ fn resolve_first_party<'a>( // Compute whether we have each criteria based on our dependencies let mut validated_criteria = criteria_mapper.no_criteria(); - let mut search_results = vec![]; + let mut search_results = Vec::with_capacity(criteria_mapper.len()); for criteria in criteria_mapper.all_criteria_iter() { // Find any build/normal dependencies that don't satisfy this criteria + let mut caveats = Caveats::default(); let mut failed_deps = SortedMap::new(); - for &depidx in &package.normal_and_build_deps { - // If we have an explicit policy for dependency, that's all that matters. - // Otherwise just use the current criteria to "inherit" the results of our deps. - let dep_name = graph.nodes[depidx].name; - let required_criteria = dep_criteria.get(dep_name).unwrap_or(criteria); - if !results[depidx].contains(required_criteria) { - failed_deps - .entry(depidx) - .or_insert_with(|| criteria_mapper.no_criteria()) - .unioned_with(required_criteria); - } - } + + get_dependency_criteria_caveats( + graph, + criteria_mapper, + results, + &package.normal_and_build_deps, + criteria, + &dep_criteria, + &mut caveats, + &mut failed_deps, + ); if failed_deps.is_empty() { // All our deps passed the test, so we have this criteria - search_results.push(SearchResult::Connected { - fully_audited: true, - }); + search_results.push(SearchResult::Connected { caveats }); validated_criteria.unioned_with(criteria); } else { // Some of our deps failed to satisfy this criteria, record this @@ -2064,10 +2137,48 @@ fn resolve_first_party<'a>( ); // Save the results - results[pkgidx].search_results = search_results; - results[pkgidx].validated_criteria = validated_criteria; + results[pkgidx] = ResolveResult { + validated_criteria, + search_results, + directly_exempted: false, + }; +} + +fn get_policy_caveats<'a>( + criteria_mapper: &CriteriaMapper, + search_results: &[SearchResult<'a>], + own_policy: &CriteriaSet, + pkgidx: PackageIdx, + root_caveats: &mut Caveats, + policy_failures: &mut PolicyFailures, +) { + for criteria_idx in own_policy.indices() { + match &search_results[criteria_idx] { + SearchResult::PossiblyConnected { failed_deps } => { + // Our children failed us + for (&dep, failed_criteria) in failed_deps { + policy_failures + .entry(dep) + .or_insert_with(|| criteria_mapper.no_criteria()) + .unioned_with(failed_criteria); + } + } + SearchResult::Disconnected { .. } => { + // We failed ourselves + policy_failures + .entry(pkgidx) + .or_insert_with(|| criteria_mapper.no_criteria()) + .set_criteria(criteria_idx) + } + SearchResult::Connected { caveats } => { + // A-OK + root_caveats.add(caveats); + } + } + } } +#[allow(clippy::too_many_arguments)] fn resolve_self_policy<'a>( store: &'a Store, graph: &DepGraph<'a>, @@ -2075,6 +2186,7 @@ fn resolve_self_policy<'a>( results: &mut [ResolveResult<'a>], _violations: &mut SortedMap>, root_failures: &mut RootFailures, + root_caveats: &mut Caveats, pkgidx: PackageIdx, ) { let package = &graph.nodes[pkgidx]; @@ -2094,30 +2206,19 @@ fn resolve_self_policy<'a>( }; let mut policy_failures = PolicyFailures::new(); - for criteria_idx in own_policy.indices() { - match &results[pkgidx].search_results[criteria_idx] { - SearchResult::PossiblyConnected { failed_deps } => { - // Our children failed us - for (&dep, failed_criteria) in failed_deps { - policy_failures - .entry(dep) - .or_insert_with(|| criteria_mapper.no_criteria()) - .unioned_with(failed_criteria); - } - } - SearchResult::Disconnected { .. } => { - // We failed ourselves - policy_failures - .entry(pkgidx) - .or_insert_with(|| criteria_mapper.no_criteria()) - .set_criteria(criteria_idx) - } - SearchResult::Connected { .. } => { - // A-OK - } - } - } + get_policy_caveats( + criteria_mapper, + &results[pkgidx].search_results, + &own_policy, + pkgidx, + root_caveats, + &mut policy_failures, + ); + + // NOTE: We don't update search results here, just `validated_criteria`. + // This is the only way that these two should get out of sync, but we want + // to keep around the old search results as it will be useful for blame. if policy_failures.is_empty() { // We had a policy and it passed, so now we're validated for all criteria // because our parents can never require anything else of us. No need @@ -2132,6 +2233,7 @@ fn resolve_self_policy<'a>( } } +#[allow(clippy::too_many_arguments)] fn resolve_dev<'a>( store: &'a Store, graph: &DepGraph<'a>, @@ -2139,6 +2241,7 @@ fn resolve_dev<'a>( results: &mut [ResolveResult<'a>], _violations: &mut SortedMap>, root_failures: &mut RootFailures, + root_caveats: &mut Caveats, pkgidx: PackageIdx, ) { // Check the dev-deps of this package. It is assumed to be a root in this context, @@ -2166,25 +2269,23 @@ fn resolve_dev<'a>( let mut search_results = vec![]; for criteria in criteria_mapper.all_criteria_iter() { // Find any build/normal dependencies that don't satisfy this criteria + let mut caveats = Caveats::default(); let mut failed_deps = SortedMap::new(); - for &depidx in &package.dev_deps { - // If we have an explicit policy for dependency, that's all that matters. - // Otherwise just use the current criteria to "inherit" the results of our deps. - let dep_name = graph.nodes[depidx].name; - let required_criteria = dep_criteria.get(dep_name).unwrap_or(criteria); - if !results[depidx].contains(required_criteria) { - failed_deps - .entry(depidx) - .or_insert_with(|| criteria_mapper.no_criteria()) - .unioned_with(required_criteria); - } - } + + get_dependency_criteria_caveats( + graph, + criteria_mapper, + results, + &package.dev_deps, + criteria, + &dep_criteria, + &mut caveats, + &mut failed_deps, + ); if failed_deps.is_empty() { // All our deps passed the test, so we have this criteria - search_results.push(SearchResult::Connected { - fully_audited: true, - }); + search_results.push(SearchResult::Connected { caveats }); validated_criteria.unioned_with(criteria); } else { // Some of our deps failed to satisfy this criteria, record this @@ -2197,10 +2298,8 @@ fn resolve_dev<'a>( .criteria_names(&validated_criteria) .collect::>() ); - // DON'T save the results, because we're analyzing this as a dev-node and anything that - // depends on us only cares about the "normal" results. - // results[pkgidx].search_results = search_results; - // results[pkgidx].validated_criteria = validated_criteria; + // NOTE: DON'T save the results, because we're analyzing this as a dev-node + // and anything that depends on us only cares about the "normal" results. // Now check that we pass our own policy let entry = store.config.policy.get(package.name); @@ -2213,29 +2312,15 @@ fn resolve_dev<'a>( }; let mut policy_failures = PolicyFailures::new(); - for criteria_idx in own_policy.indices() { - match &search_results[criteria_idx] { - SearchResult::PossiblyConnected { failed_deps } => { - // Our children failed us - for (&dep, failed_criteria) in failed_deps { - policy_failures - .entry(dep) - .or_insert_with(|| criteria_mapper.no_criteria()) - .unioned_with(failed_criteria); - } - } - SearchResult::Disconnected { .. } => { - // We failed ourselves - policy_failures - .entry(pkgidx) - .or_insert_with(|| criteria_mapper.no_criteria()) - .set_criteria(criteria_idx) - } - SearchResult::Connected { .. } => { - // A-OK - } - } - } + + get_policy_caveats( + criteria_mapper, + &search_results, + &own_policy, + pkgidx, + root_caveats, + &mut policy_failures, + ); if policy_failures.is_empty() { trace!(" passed dev policy"); diff --git a/src/serialization.rs b/src/serialization.rs index 64c874ec..d4c1ae7c 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -234,6 +234,9 @@ pub mod audit { notes: val.notes, criteria: val.criteria, kind: kind?, + // By default, always read entries as non-fresh. The import code + // will set this flag to true for imported entries. + is_fresh_import: false, }) } } @@ -605,6 +608,7 @@ mod test { dependency_criteria: dc_long, }, notes: Some("notes go here!".to_owned()), + is_fresh_import: false, // ignored }, AuditEntry { who: None, @@ -614,6 +618,7 @@ mod test { dependency_criteria: dc_short, }, notes: Some("notes go here!".to_owned()), + is_fresh_import: true, // ignored }, ], ); diff --git a/src/storage.rs b/src/storage.rs index 0ad1e863..cd843e7b 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -11,7 +11,7 @@ use std::{ use cargo_metadata::Version; use crates_index::Index; use flate2::read::GzDecoder; -use futures_util::future::{join_all, try_join_all}; +use futures_util::future::try_join_all; use miette::{NamedSource, SourceOffset}; use reqwest::Url; use serde::{Deserialize, Serialize}; @@ -21,18 +21,19 @@ use tracing::{error, info, log::warn, trace}; use crate::{ errors::{ CacheAcquireError, CacheCommitError, CommandError, CriteriaChangeError, - CriteriaChangeErrors, DiffError, FetchAndDiffError, FetchAuditError, FetchError, - FlockError, InvalidCriteriaError, JsonParseError, LoadJsonError, LoadTomlError, SourceFile, - StoreAcquireError, StoreCommitError, StoreCreateError, StoreJsonError, StoreTomlError, - StoreValidateError, StoreValidateErrors, TomlParseError, UnpackError, + CriteriaChangeErrors, DiffError, DownloadError, FetchAndDiffError, FetchAuditError, + FetchError, FlockError, InvalidCriteriaError, JsonParseError, LoadJsonError, LoadTomlError, + SourceFile, StoreAcquireError, StoreCommitError, StoreCreateError, StoreJsonError, + StoreTomlError, StoreValidateError, StoreValidateErrors, TomlParseError, UnpackError, }, flock::{FileLock, Filesystem}, format::{ - AuditsFile, CommandHistory, ConfigFile, CriteriaName, Delta, DiffCache, DiffStat, FastMap, - FetchCommand, ImportsFile, MetaConfig, PackageStr, SortedMap, SAFE_TO_DEPLOY, SAFE_TO_RUN, + AuditKind, AuditsFile, CommandHistory, ConfigFile, CriteriaName, Delta, DiffCache, + DiffStat, FastMap, FetchCommand, ImportName, ImportsFile, MetaConfig, PackageStr, + SortedMap, SAFE_TO_DEPLOY, SAFE_TO_RUN, }, network::Network, - resolver, + resolver::{self, Conclusion}, serialization::{spanned::Spanned, to_formatted_toml}, Config, PartialConfig, }; @@ -123,6 +124,10 @@ pub struct Store { pub imports: ImportsFile, pub audits: AuditsFile, + // The complete live set of imports fetched from the network. Will be + // initialized to `None` if `--locked` was passed. + pub live_imports: Option, + pub config_src: SourceFile, pub imports_src: SourceFile, pub audits_src: SourceFile, @@ -151,6 +156,7 @@ impl Store { criteria: SortedMap::new(), audits: SortedMap::new(), }, + live_imports: None, config_src: Arc::new(NamedSource::new(CONFIG_TOML, "")), audits_src: Arc::new(NamedSource::new(AUDITS_TOML, "")), imports_src: Arc::new(NamedSource::new(IMPORTS_LOCK, "")), @@ -162,8 +168,19 @@ impl Store { metacfg.store_path().as_path_unlocked().exists() } + pub fn acquire_offline(cfg: &Config) -> Result { + Self::acquire(cfg, None, false) + } + /// Acquire an existing store - pub fn acquire(cfg: &Config) -> Result { + /// + /// If `network` is passed and `!cfg.cli.locked`, this will fetch remote + /// imports to use for comparison purposes. + pub fn acquire( + cfg: &Config, + network: Option<&Network>, + allow_criteria_changes: bool, + ) -> Result { let root = cfg.metacfg.store_path(); // Before we do anything else, acquire an exclusive lock on the @@ -177,11 +194,24 @@ impl Store { let (imports_src, imports): (_, ImportsFile) = load_toml(IMPORTS_LOCK, lock.read_imports()?)?; + // If this command isn't locked, and the network is available, fetch the + // live state of imported audits. + let live_imports = if let (false, Some(network)) = (cfg.cli.locked, network) { + let imported_audits = tokio::runtime::Handle::current() + .block_on(fetch_imported_audits(network, &config))?; + let live_imports = + process_imported_audits(imported_audits, &imports, allow_criteria_changes)?; + Some(live_imports) + } else { + None + }; + let store = Self { lock: Some(lock), config, audits, imports, + live_imports, config_src, audits_src, imports_src, @@ -201,12 +231,37 @@ impl Store { config, imports, audits, + live_imports: None, config_src: Arc::new(NamedSource::new(CONFIG_TOML, "")), audits_src: Arc::new(NamedSource::new(AUDITS_TOML, "")), imports_src: Arc::new(NamedSource::new(IMPORTS_LOCK, "")), } } + /// Create a mock store, also mocking out the unlocked import fetching + /// process by providing the live values of imported AuditsFiles. + #[cfg(test)] + pub fn mock_online( + config: ConfigFile, + audits: AuditsFile, + imports: ImportsFile, + fetched_audits: Vec<(ImportName, AuditsFile)>, + allow_criteria_changes: bool, + ) -> Result { + let live_imports = + process_imported_audits(fetched_audits, &imports, allow_criteria_changes)?; + Ok(Self { + lock: None, + config, + imports, + audits, + live_imports: Some(live_imports), + config_src: Arc::new(NamedSource::new(CONFIG_TOML, "")), + audits_src: Arc::new(NamedSource::new(AUDITS_TOML, "")), + imports_src: Arc::new(NamedSource::new(IMPORTS_LOCK, "")), + }) + } + #[cfg(test)] pub fn mock_acquire( config: &str, @@ -222,6 +277,7 @@ impl Store { config, imports, audits, + live_imports: None, config_src, audits_src, imports_src, @@ -247,6 +303,7 @@ impl Store { config: self.config.clone(), imports: self.imports.clone(), audits: self.audits.clone(), + live_imports: self.live_imports.clone(), config_src: self.config_src.clone(), audits_src: self.audits_src.clone(), imports_src: self.imports_src.clone(), @@ -258,6 +315,18 @@ impl Store { clone } + /// Returns the set of audits which should be operated upon. + /// + /// If the store was acquired unlocked, this will include audits which are + /// not stored in imports.lock, otherwise it will only contain imports + /// stored locally. + pub fn imported_audits(&self) -> &SortedMap { + match &self.live_imports { + Some(live_imports) => &live_imports.audits, + None => &self.imports.audits, + } + } + /// Commit the store's contents back to disk pub fn commit(self) -> Result<(), StoreCommitError> { // TODO: make this truly transactional? @@ -273,6 +342,26 @@ impl Store { Ok(()) } + /// Mock `commit`. Returns the serialized value for each file in the store. + /// Doesn't take `self` by value so that it can continue to be used. + #[cfg(test)] + pub fn mock_commit(&self) -> SortedMap { + let mut audits = Vec::new(); + let mut config = Vec::new(); + let mut imports = Vec::new(); + store_audits(&mut audits, self.audits.clone()).unwrap(); + store_config(&mut config, self.config.clone()).unwrap(); + store_imports(&mut imports, self.imports.clone()).unwrap(); + + [ + (AUDITS_TOML.to_owned(), String::from_utf8(audits).unwrap()), + (CONFIG_TOML.to_owned(), String::from_utf8(config).unwrap()), + (IMPORTS_LOCK.to_owned(), String::from_utf8(imports).unwrap()), + ] + .into_iter() + .collect() + } + /// Validate the store's integrity #[allow(clippy::for_kv_map)] pub fn validate(&self) -> Result<(), StoreValidateErrors> { @@ -426,84 +515,203 @@ impl Store { Ok(()) } - /// Fetch foreign audits, only call this is we're not --locked - pub async fn fetch_foreign_audits( - &mut self, - network: &Network, - accept_changes: bool, - ) -> Result<(), FetchAuditError> { - let raw_new_imports = - try_join_all(self.config.imports.iter().map(|(name, import)| async { - let audit_file = fetch_foreign_audit(network, name, &import.url).await?; - // Fetch the descriptions to cache them and check that they haven't changed - // FIXME: this should probably treat failing to fetch as an error but eula_for_criteria - // hides errors... should we have two versions? Or make it the caller's problem? - let new_descs = join_all(audit_file.criteria.iter().map(|(criteria, _)| async { - ( - criteria.clone(), - crate::eula_for_criteria(Some(network), &audit_file.criteria, criteria) - .await, - ) - })) - .await; - Ok::<_, FetchAuditError>((name.clone(), audit_file, new_descs)) - })) - .await?; + /// Return an updated version of the `imports.lock` file taking into account + /// the result of running the resolver, to pick which audits need to be + /// vendored. + /// + /// If `force_update` is specified, audits for all crates will be updated, + /// even if the currently vendored audits would be sufficient. + #[must_use] + pub fn get_updated_imports_file( + &self, + report: &resolver::ResolveReport<'_>, + force_update: bool, + ) -> ImportsFile { + if self.live_imports.is_none() { + // We're locked, so can't update anything. + return self.imports.clone(); + } + + // Determine which packages were used. + let mut used_packages = SortedMap::new(); + for package in report.graph.nodes.iter() { + used_packages.insert(package.name, false); + } + + // If we succeeded, also record which packages need fresh imports. + if let Conclusion::Success(success) = &report.conclusion { + for &node_idx in &success.needed_fresh_imports { + let package = &report.graph.nodes[node_idx]; + used_packages.insert(package.name, true); + } + } let mut new_imports = ImportsFile { audits: SortedMap::new(), }; - let mut criteria_changes = vec![]; - for (import_name, mut audits_file, new_descs) in raw_new_imports { - for (criteria_name, new_desc) in new_descs { - if !accept_changes { - // Check that the new description doesn't modify an existing old one - if let Some(old_entry) = self - .imports - .audits - .get(&import_name) - .and_then(|file| file.criteria.get(&criteria_name)) - { + for import_name in self.config.imports.keys() { + let live_audit_file = self + .imported_audits() + .get(import_name) + .expect("Live audits missing for import?"); + + // Is this the first time we're doing this import? If it is we'll + // want to effectively force-update this peer. + let first_import = !self.imports.audits.contains_key(import_name); + + // Create the new audits file, and copy criteria into it on a + // per-package basis, only including used packages. + // We always use the live version of criteria in the new file. + let mut new_audits_file = AuditsFile { + criteria: live_audit_file.criteria.clone(), + audits: SortedMap::new(), + }; + for (&package, &need_fresh_imports) in &used_packages { + // Always pull audit entries from the live version to pull in + // revocations, violations, updated notes, etc. + // Filter out entries which are fresh unless we're updating + // entries for this package or it's a violation. + let update_package = first_import || need_fresh_imports || force_update; + let audits = live_audit_file + .audits + .get(package) + .map(|v| &v[..]) + .unwrap_or(&[]) + .iter() + .filter(|audit| { + update_package + || !audit.is_fresh_import + || matches!(audit.kind, AuditKind::Violation { .. }) + }) + .cloned() + .map(|mut audit| { + audit.is_fresh_import = false; + audit + }) + .collect::>(); + + // Record audits in the new audits file, if there are any. + if !audits.is_empty() { + new_audits_file.audits.insert(package.to_owned(), audits); + } + } + new_imports + .audits + .insert(import_name.to_owned(), new_audits_file); + } + + new_imports + } + + pub fn maybe_update_imports_file(&mut self, cfg: &Config, force_update: bool) { + let report = resolver::resolve( + &cfg.metadata, + cfg.cli.filter_graph.as_ref(), + self, + // Resolve depth only impacts error results, so we can use Shallow + // to do less work. + resolver::ResolveDepth::Shallow, + ); + self.imports = self.get_updated_imports_file(&report, force_update); + } +} + +/// Process imported audits from the network, generating a `LiveImports` +/// description of the live state of imported audits. +fn process_imported_audits( + fetched_audits: Vec<(ImportName, AuditsFile)>, + imports_lock: &ImportsFile, + allow_criteria_changes: bool, +) -> Result { + let mut new_imports = ImportsFile { + audits: SortedMap::new(), + }; + let mut changed_criteria = Vec::new(); + for (import_name, mut audits_file) in fetched_audits { + // By default all audits read from the network are fresh. + for audit_entry in audits_file.audits.values_mut().flat_map(|v| v.iter_mut()) { + audit_entry.is_fresh_import = true; + } + + // If we have an existing audits file for these imports, compare against it. + if let Some(existing_audits_file) = imports_lock.audits.get(&import_name) { + if !allow_criteria_changes { + // Compare the new criteria descriptions with existing criteria + // descriptions. If the description already exists, record a + // CriteriaChangeError. + for (criteria_name, old_entry) in &existing_audits_file.criteria { + if let Some(new_entry) = audits_file.criteria.get(criteria_name) { let old_desc = old_entry.description.as_ref().unwrap(); - if old_desc != &new_desc { - criteria_changes.push(CriteriaChangeError { + let new_desc = new_entry.description.as_ref().unwrap(); + if old_desc != new_desc { + changed_criteria.push(CriteriaChangeError { import_name: import_name.clone(), criteria_name: criteria_name.to_owned(), old_desc: old_desc.clone(), - new_desc, + new_desc: new_desc.clone(), }); - continue; } } } - - // Store the new fetched description - audits_file - .criteria - .get_mut(&criteria_name) - .unwrap() - .description = Some(new_desc); } - // Now add the new import - new_imports.audits.insert(import_name, audits_file); - } - if !criteria_changes.is_empty() { - Err(CriteriaChangeErrors { - errors: criteria_changes, - })?; + // Compare the new audits with existing audits. If an audit already + // existed in the existing audits file, mark it as non-fresh. + for (package, existing_audits) in &existing_audits_file.audits { + let new_audits = audits_file + .audits + .get_mut(package) + .map(|v| &mut v[..]) + .unwrap_or(&mut []); + for existing_audit in existing_audits { + for new_audit in &mut new_audits[..] { + // Ignore `who` and `notes` for comparison, as they + // are not relevant semantically and might have been + // updated uneventfully. + if new_audit.is_fresh_import + && new_audit.kind == existing_audit.kind + && new_audit.criteria == existing_audit.criteria + { + new_audit.is_fresh_import = false; + break; + } + } + } + } } - // Accept the new imports. These will only be committed if the current command succeeds. - self.imports = new_imports; + // Now add the new import + new_imports.audits.insert(import_name, audits_file); + } - // Now do one last validation to catch corrupt imports - self.validate()?; - Ok(()) + if !changed_criteria.is_empty() { + return Err(CriteriaChangeErrors { + errors: changed_criteria, + }); } + + // FIXME: Consider doing some additional validation on these audits + // before returning? + + Ok(new_imports) +} + +/// Fetch all declared imports from the network, filling in any criteria +/// descriptions. +async fn fetch_imported_audits( + network: &Network, + config: &ConfigFile, +) -> Result, FetchAuditError> { + try_join_all(config.imports.iter().map(|(name, import)| async { + let audit_file = fetch_imported_audit(network, name, &import.url).await?; + Ok::<_, FetchAuditError>((name.clone(), audit_file)) + })) + .await } -async fn fetch_foreign_audit( +/// Fetch a single AuditsFile from the network, filling in any criteria +/// descriptions. +async fn fetch_imported_audit( network: &Network, name: &str, url: &str, @@ -516,7 +724,7 @@ async fn fetch_foreign_audit( let audit_bytes = network.download(parsed_url).await?; let audit_string = String::from_utf8(audit_bytes).map_err(LoadTomlError::from)?; let audit_source = Arc::new(NamedSource::new(name, audit_string.clone())); - let audit_file: AuditsFile = toml::de::from_str(&audit_string) + let mut audit_file: AuditsFile = toml::de::from_str(&audit_string) .map_err(|error| { let (line, col) = error.line_col().unwrap_or((0, 0)); TomlParseError { @@ -526,6 +734,46 @@ async fn fetch_foreign_audit( } }) .map_err(LoadTomlError::from)?; + + // Eagerly fetch all descriptions for criteria in the imported audits file, + // and store them inline. We'll error out if any of these descriptions are + // unavailable. + try_join_all( + audit_file + .criteria + .iter_mut() + .map(|(criteria_name, criteria_entry)| async { + if criteria_entry.description.is_some() { + return Ok(()); + } + + let url_string = criteria_entry.description_url.as_ref().ok_or_else(|| { + FetchAuditError::MissingCriteriaDescription { + import_name: name.to_owned(), + criteria_name: criteria_name.clone(), + } + })?; + let url = Url::parse(url_string).map_err(|error| { + FetchAuditError::InvalidCriteriaDescriptionUrl { + import_name: name.to_owned(), + criteria_name: criteria_name.clone(), + url: url_string.clone(), + error, + } + })?; + let bytes = network.download(url.clone()).await?; + let description = + String::from_utf8(bytes).map_err(|error| DownloadError::InvalidText { + url: url.clone(), + error, + })?; + + criteria_entry.description = Some(description); + Ok::<(), FetchAuditError>(()) + }), + ) + .await?; + Ok(audit_file) } diff --git a/src/tests/import.rs b/src/tests/import.rs new file mode 100644 index 00000000..d53402f2 --- /dev/null +++ b/src/tests/import.rs @@ -0,0 +1,738 @@ +use super::*; + +// Helper function for imports tests. Performs a vet and updates imports based +// on it, returning a diff of the two. +fn get_imports_file_changes(metadata: &Metadata, store: &Store, force_updates: bool) -> String { + // Run the resolver before calling `get_imports_file` to compute the new + // imports file. + let report = crate::resolver::resolve(metadata, None, store, ResolveDepth::Shallow); + let new_imports = store.get_updated_imports_file(&report, force_updates); + + // Format the old and new files as TOML, and write out a diff using `similar`. + let old_imports = crate::serialization::to_formatted_toml(&store.imports) + .unwrap() + .to_string(); + let new_imports = crate::serialization::to_formatted_toml(new_imports) + .unwrap() + .to_string(); + + generate_diff(&old_imports, &new_imports) +} + +// Test cases: + +#[test] +fn new_peer_import() { + // (Pass) We import all audits for our third-party packages from a brand-new + // peer even though we are already fully audited. This won't force an import + // from existing peers. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [ + ( + "third-party2".to_owned(), + vec![delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY)], + ), + ( + "unused-package".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + ), + ] + .into_iter() + .collect(), + }; + + let old_other_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + let new_other_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![delta_audit(ver(200), ver(300), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + imports + .audits + .insert(OTHER_FOREIGN.to_owned(), old_other_foreign_audits); + + config.imports.insert( + OTHER_FOREIGN.to_owned(), + crate::format::RemoteImport { + url: OTHER_FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![ + (FOREIGN.to_owned(), new_foreign_audits), + (OTHER_FOREIGN.to_owned(), new_other_foreign_audits), + ], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_skip_import() { + // (Pass) If we've previously imported from a peer, we don't import + // audits for a package unless it's useful. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [ + ( + "third-party2".to_owned(), + vec![delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY)], + ), + ( + "unused-package".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + ), + ] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_remove_unused() { + // (Pass) We'll remove unused audits when unlocked, even if our peer hasn't + // changed. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [ + ( + "third-party2".to_owned(), + vec![delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY)], + ), + ( + "unused-package".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + ), + ] + .into_iter() + .collect(), + }; + + let new_foreign_audits = old_foreign_audits.clone(); + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_import_delta_audit() { + // (Pass) If a new delta audit from a peer is useful, we'll import it and + // all other audits for that crate, including from other peers. + + 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"); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![full_audit(ver(9), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [ + // A new audit for third-party2 should fix our audit, so we should + // import all of these. + ( + "third-party2".to_owned(), + vec![ + full_audit(ver(9), SAFE_TO_DEPLOY), + delta_audit(ver(9), ver(DEFAULT_VER), SAFE_TO_DEPLOY), + delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY), + ], + ), + // This audit won't change things for us, so we won't import it to + // avoid churn. + ( + "third-party1".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + ), + ] + .into_iter() + .collect(), + }; + + let old_other_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + let new_other_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + // We'll also import unrelated audits from other sources. + audits: [( + "third-party2".to_owned(), + vec![delta_audit(ver(200), ver(300), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + imports + .audits + .insert(OTHER_FOREIGN.to_owned(), old_other_foreign_audits); + + config.imports.insert( + OTHER_FOREIGN.to_owned(), + crate::format::RemoteImport { + url: OTHER_FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![ + (FOREIGN.to_owned(), new_foreign_audits), + (OTHER_FOREIGN.to_owned(), new_other_foreign_audits), + ], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_import_custom_criteria() { + // (Pass) We'll immediately import criteria changes wen unlocked, even if + // our peer hasn't changed or we aren't mapping them locally. This doesn't + // force an import of other crates. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let new_foreign_audits = AuditsFile { + criteria: [( + "fuzzed".to_string(), + CriteriaEntry { + implies: vec![], + description: Some("fuzzed".to_string()), + description_url: None, + }, + )] + .into_iter() + .collect(), + audits: [( + "third-party2".to_owned(), + vec![ + full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY), + delta_audit(ver(DEFAULT_VER), ver(11), SAFE_TO_DEPLOY), + ], + )] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + + insta::assert_snapshot!(output); +} + +#[test] +fn new_audit_for_unused_criteria_basic() { + // (Pass) If a peer adds an audit for an unused criteria, we shouldn't + // vendor in the changes unnecessarily. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + let old_foreign_audits = AuditsFile { + criteria: [( + "fuzzed".to_string(), + CriteriaEntry { + implies: vec![], + description: Some("fuzzed".to_string()), + description_url: None, + }, + )] + .into_iter() + .collect(), + audits: [( + "third-party2".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let mut new_foreign_audits = old_foreign_audits.clone(); + new_foreign_audits + .audits + .get_mut("third-party2") + .unwrap() + .push(full_audit(ver(DEFAULT_VER), "fuzzed")); + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + + insta::assert_snapshot!(output); +} + +#[test] +fn new_audit_for_unused_criteria_transitive() { + // (Pass) If a peer adds an audit for an unused criteria of a transitive + // dependency, we shouldn't vendor in the changes unnecessarily. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + let old_foreign_audits = AuditsFile { + criteria: [( + "fuzzed".to_string(), + CriteriaEntry { + implies: vec![], + description: Some("fuzzed".to_string()), + description_url: None, + }, + )] + .into_iter() + .collect(), + audits: [( + "third-party1".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let mut new_foreign_audits = old_foreign_audits.clone(); + new_foreign_audits + .audits + .get_mut("third-party1") + .unwrap() + .push(full_audit(ver(DEFAULT_VER), "fuzzed")); + new_foreign_audits.audits.insert( + "transitive-third-party1".to_owned(), + vec![full_audit(ver(DEFAULT_VER), "fuzzed")], + ); + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_revoked_audit() { + // (Pass) If a previously-imported audit is removed, we should also remove + // it locally, even if we don't use it. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn existing_peer_add_violation() { + // (Pass) If a peer adds a violation for any version of a crate we use, we + // should immediately import it. We won't immediately import other audits + // added for that crate, however. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_full_audited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![ + delta_audit(ver(100), ver(200), SAFE_TO_DEPLOY), + delta_audit(ver(200), ver(300), SAFE_TO_DEPLOY), + violation(VersionReq::parse("99.*").unwrap(), SAFE_TO_DEPLOY), + ], + )] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn peer_audits_exemption_no_minimize() { + // (Pass) We don't import audits for a package which would replace an + // exemption unless we're regenerating exemptions. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_inited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + let output = get_imports_file_changes(&metadata, &store, false); + insta::assert_snapshot!(output); +} + +#[test] +fn peer_audits_exemption_minimize() { + // (Pass) We do import audits for a package which would replace an exemption + // when we're regenerating exemptions. + + let _enter = TEST_RUNTIME.enter(); + let mock = MockMetadata::simple(); + + let metadata = mock.metadata(); + let (mut config, audits, mut imports) = builtin_files_inited(&metadata); + + let old_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: SortedMap::new(), + }; + + let new_foreign_audits = AuditsFile { + criteria: SortedMap::new(), + audits: [( + "third-party2".to_owned(), + vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)], + )] + .into_iter() + .collect(), + }; + + imports + .audits + .insert(FOREIGN.to_owned(), old_foreign_audits); + + config.imports.insert( + FOREIGN.to_owned(), + crate::format::RemoteImport { + url: FOREIGN_URL.to_owned(), + criteria_map: Vec::new(), + }, + ); + + let mut store = Store::mock_online( + config, + audits, + imports, + vec![(FOREIGN.to_owned(), new_foreign_audits)], + true, + ) + .unwrap(); + + // Capture the old imports before minimizing exemptions + let old = store.mock_commit(); + + crate::minimize_exemptions(&mock_cfg(&metadata), &mut store, None).unwrap(); + + // Capture after minimizing exemptions, and generate a diff. + let new = store.mock_commit(); + + let output = diff_store_commits(&old, &new); + insta::assert_snapshot!(output); +} + +// Other tests worth adding: +// +// - used edges with dependency-criteria should cause criteria to be imported diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 721507b0..9d7ac8ea 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -14,7 +14,7 @@ use crate::{ editor::Editor, format::{ AuditKind, CriteriaName, CriteriaStr, Delta, DependencyCriteria, FastMap, MetaConfig, - PackageName, PackageStr, PolicyEntry, VersionReq, SAFE_TO_DEPLOY, SAFE_TO_RUN, + PackageName, PackageStr, PolicyEntry, SortedSet, VersionReq, SAFE_TO_DEPLOY, SAFE_TO_RUN, }, init_files, out::Out, @@ -25,6 +25,7 @@ use crate::{ mod audit_as_crates_io; mod certify; +mod import; mod regenerate_unaudited; mod store_parsing; mod vet; @@ -299,6 +300,7 @@ fn delta_audit(from: Version, to: Version, criteria: CriteriaStr) -> AuditEntry delta, dependency_criteria: DependencyCriteria::default(), }, + is_fresh_import: false, } } @@ -331,6 +333,7 @@ fn delta_audit_dep( }) .collect(), }, + is_fresh_import: false, } } @@ -343,6 +346,7 @@ fn full_audit(version: Version, criteria: CriteriaStr) -> AuditEntry { version, dependency_criteria: DependencyCriteria::default(), }, + is_fresh_import: false, } } @@ -358,6 +362,7 @@ fn full_audit_m( version, dependency_criteria: DependencyCriteria::default(), }, + is_fresh_import: false, } } @@ -387,6 +392,7 @@ fn full_audit_dep( }) .collect(), }, + is_fresh_import: false, } } @@ -396,6 +402,7 @@ fn violation_hard(version: VersionReq) -> AuditEntry { notes: None, criteria: vec![SAFE_TO_RUN.to_string().into()], kind: AuditKind::Violation { violation: version }, + is_fresh_import: false, } } #[allow(dead_code)] @@ -405,6 +412,7 @@ fn violation(version: VersionReq, criteria: CriteriaStr) -> AuditEntry { notes: None, criteria: vec![criteria.to_string().into()], kind: AuditKind::Violation { violation: version }, + is_fresh_import: false, } } #[allow(dead_code)] @@ -417,6 +425,7 @@ fn violation_m( notes: None, criteria: criteria.into_iter().map(|s| s.into().into()).collect(), kind: AuditKind::Violation { violation: version }, + is_fresh_import: false, } } @@ -1152,6 +1161,32 @@ impl Out for BasicTestOutput { } } +/// Format a diff between the old and new strings for reporting. +fn generate_diff(old: &str, new: &str) -> String { + similar::utils::diff_lines(similar::Algorithm::Myers, old, new) + .into_iter() + .map(|(tag, line)| format!("{}{}", tag, line)) + .collect() +} + +/// Generate a diff between two values returned from `Store::mock_commit`. +fn diff_store_commits(old: &SortedMap, new: &SortedMap) -> String { + use std::fmt::Write; + let mut result = String::new(); + let keys = old.keys().chain(new.keys()).collect::>(); + for key in keys { + let old = old.get(key).map(|s| &s[..]).unwrap_or(""); + let new = new.get(key).map(|s| &s[..]).unwrap_or(""); + if old == new { + writeln!(&mut result, "{key}: (unchanged)").unwrap(); + continue; + } + let diff = generate_diff(old, new); + writeln!(&mut result, "{key}:\n{diff}").unwrap(); + } + result +} + // TESTING BACKLOG: // // * custom policies diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_add_violation.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_add_violation.snap new file mode 100644 index 00000000..873f9586 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_add_violation.snap @@ -0,0 +1,13 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [[audits.peer-company.audits.third-party2]] + criteria = "safe-to-deploy" + delta = "100.0.0 -> 200.0.0" ++ ++[[audits.peer-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++violation = "99.*" + diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_custom_criteria.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_custom_criteria.snap new file mode 100644 index 00000000..f67fa8de --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_custom_criteria.snap @@ -0,0 +1,12 @@ +--- +source: src/tests/import.rs +expression: output +--- + ++[audits.peer-company.criteria.fuzzed] ++description = "fuzzed" ++ + [[audits.peer-company.audits.third-party2]] + criteria = "safe-to-deploy" + version = "10.0.0" + diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_delta_audit.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_delta_audit.snap new file mode 100644 index 00000000..4574e737 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_import_delta_audit.snap @@ -0,0 +1,22 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [[audits.peer-company.audits.third-party2]] + criteria = "safe-to-deploy" + version = "9.0.0" + +-[audits.rival-company.audits] ++[[audits.peer-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++delta = "9.0.0 -> 10.0.0" ++ ++[[audits.peer-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++delta = "100.0.0 -> 200.0.0" ++ ++[[audits.rival-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++delta = "200.0.0 -> 300.0.0" + 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 new file mode 100644 index 00000000..ab84c697 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_remove_unused.snap @@ -0,0 +1,13 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [[audits.peer-company.audits.third-party2]] + criteria = "safe-to-deploy" + delta = "100.0.0 -> 200.0.0" +- +-[[audits.peer-company.audits.unused-package]] +-criteria = "safe-to-deploy" +-version = "10.0.0" + diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_revoked_audit.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_revoked_audit.snap new file mode 100644 index 00000000..cc9e5db4 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_revoked_audit.snap @@ -0,0 +1,10 @@ +--- +source: src/tests/import.rs +expression: output +--- + +-[[audits.peer-company.audits.third-party2]] +-criteria = "safe-to-deploy" +-delta = "100.0.0 -> 200.0.0" ++[audits.peer-company.audits] + diff --git a/src/tests/snapshots/cargo_vet__tests__import__existing_peer_skip_import.snap b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_skip_import.snap new file mode 100644 index 00000000..c24bb390 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__existing_peer_skip_import.snap @@ -0,0 +1,7 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [audits.peer-company.audits] + diff --git a/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_basic.snap b/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_basic.snap new file mode 100644 index 00000000..b37182c8 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_basic.snap @@ -0,0 +1,12 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [audits.peer-company.criteria.fuzzed] + description = "fuzzed" + + [[audits.peer-company.audits.third-party2]] + criteria = "safe-to-deploy" + version = "10.0.0" + diff --git a/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_transitive.snap b/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_transitive.snap new file mode 100644 index 00000000..9627fddc --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__new_audit_for_unused_criteria_transitive.snap @@ -0,0 +1,12 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [audits.peer-company.criteria.fuzzed] + description = "fuzzed" + + [[audits.peer-company.audits.third-party1]] + criteria = "safe-to-deploy" + version = "10.0.0" + diff --git a/src/tests/snapshots/cargo_vet__tests__import__new_peer_import.snap b/src/tests/snapshots/cargo_vet__tests__import__new_peer_import.snap new file mode 100644 index 00000000..43f221eb --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__new_peer_import.snap @@ -0,0 +1,11 @@ +--- +source: src/tests/import.rs +expression: output +--- + ++[[audits.peer-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++delta = "100.0.0 -> 200.0.0" ++ + [audits.rival-company.audits] + diff --git a/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_minimize.snap b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_minimize.snap new file mode 100644 index 00000000..f54b1187 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_minimize.snap @@ -0,0 +1,37 @@ +--- +source: src/tests/import.rs +expression: output +--- +audits.toml: (unchanged) +config.toml: + + # cargo-vet config file + + [imports.peer-company] + url = "https://peercompany.co.uk" + criteria-map = [] + + [[exemptions.third-party1]] + version = "10.0.0" + criteria = "safe-to-deploy" + +-[[exemptions.third-party2]] +-version = "10.0.0" +-criteria = "safe-to-deploy" +- + [[exemptions.transitive-third-party1]] + version = "10.0.0" + criteria = "safe-to-deploy" + + +imports.lock: + + # cargo-vet imports lock + +-[audits.peer-company.audits] ++[[audits.peer-company.audits.third-party2]] ++criteria = "safe-to-deploy" ++version = "10.0.0" + + + diff --git a/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_no_minimize.snap b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_no_minimize.snap new file mode 100644 index 00000000..c24bb390 --- /dev/null +++ b/src/tests/snapshots/cargo_vet__tests__import__peer_audits_exemption_no_minimize.snap @@ -0,0 +1,7 @@ +--- +source: src/tests/import.rs +expression: output +--- + + [audits.peer-company.audits] + diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap index 2ee5699c..5eb6a7e4 100644 --- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap +++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-default-root.snap @@ -2,5 +2,5 @@ source: src/tests/vet.rs expression: output --- -Vetting Succeeded (1 fully audited, 3 exempted) +Vetting Succeeded (4 exempted) diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap index 2ee5699c..5eb6a7e4 100644 --- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap +++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-audit-as-weaker-root.snap @@ -2,5 +2,5 @@ source: src/tests/vet.rs expression: output --- -Vetting Succeeded (1 fully audited, 3 exempted) +Vetting Succeeded (4 exempted) diff --git a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-deps-unaudited-adds-uneeded-criteria.snap b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-deps-unaudited-adds-uneeded-criteria.snap index 3c57c2e3..358cbdf2 100644 --- a/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-deps-unaudited-adds-uneeded-criteria.snap +++ b/src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-deps-unaudited-adds-uneeded-criteria.snap @@ -2,5 +2,5 @@ source: src/tests/vet.rs expression: output --- -Vetting Succeeded (5 fully audited, 1 partially audited) +Vetting Succeeded (6 fully audited)