From 17ae0b0b95c48851e6b50aaac899e975d49e4f5f Mon Sep 17 00:00:00 2001 From: Alex Matrosov Date: Mon, 6 Jul 2026 08:48:23 -0700 Subject: [PATCH 1/4] fix(quality): stop crypto-compliance false-passes (vacuous CNSA2/PQC, classical crypto, undocumented CBOM) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First cluster from the quality/compliance audit — the flagship problem: a tool advertising CNSA 2.0 / NIST PQC checking green-lit SBOMs it should reject, so a user could certify quantum-safety the tool never actually verified. The audit's threat model is a false regulatory claim; every fix here flips a false verdict in the correct direction, and the A/B confirms no over-correction (legitimate PQC crypto still passes). Vacuous pass on empty inventory: - check_cnsa2 / check_nist_pqc iterated components, `continue`d past every non-Cryptographic one, and emitted nothing — so a plain application SBOM (no CBOM) reported COMPLIANT for CNSA 2.0 and NIST PQC. They now emit an Error (SBOM-CNSA2-000 / SBOM-PQC-000) when zero cryptographic assets were evaluated: a PQC-readiness verdict must not be "compliant" having checked nothing. Classical quantum-vulnerable crypto passed PQC: - PQC-001 fired only when nistQuantumSecurityLevel == Some(0), and the broken-family list had no classical asymmetric algorithm — so an RSA / ECDSA / ECDH / DH / DSA CBOM with an unset quantum level (the common real-world case) passed NIST PQC. A new AlgorithmProperties::is_classical_quantum_vulnerable() flags these on the family alone (broken by Shor's algorithm regardless of key size); PQC-001 now fires on it, and the quantum_vulnerable_count metric counts it. Grade-A for undocumented crypto: - CryptographyMetrics incremented total_crypto_components BEFORE the crypto_properties guard, so a Cryptographic component with no cryptoProperties made has_data() true and the six CBOM sub-scores returned 100 on empty denominators (grade-A crypto for nothing documented). It now counts only documented assets, so the crypto score is N/A rather than 100. CNSA gate escapes + ECC false-fail: - The hash gate only matched family literally "SHA-2"/"SHA2" with param "256", so SHA-224 and family-form "SHA-256" escaped; it now catches both encodings. AES now requires an explicit 256 (via param or classical security level) instead of passing on an absent parameter. - Conversely, the key-size check false-FAILED every ECC key (flat <2048), but an ECC key size is the curve bit-length (P-256 → 256 ≈ 128-bit security). Strong ECC curve sizes (255/256/384/448/521) are now adequate while RSA/finite-field still needs ≥2048; 512 is deliberately excluded so a factorable 512-bit RSA/DSA key is not passed as "strong ECC". Verified: false-pass/false-fail A/B vs main (all four false-passes flip to fail; ML-KEM-1024/ML-DSA-87/SHA-384/AES-256 clean CBOM stays compliant; ECC P-256/P-384 no longer false-failed; RSA-1024 still flagged) and an over-correction audit (no PQC family or hybrid trips the classical list; it caught the 512-bit-RSA hole in the ECC allowlist, fixed here). 7 new tests; two compliance TUI snapshots correctly flip a crypto-less fixture from pass to fail. Full suite green (1649 tests), clippy -D warnings and fmt clean. Co-Authored-By: Claude Fable 5 --- src/model/crypto.rs | 20 +++ src/quality/compliance/crypto.rs | 135 ++++++++++++++---- src/quality/compliance/mod.rs | 82 +++++++++++ src/quality/compliance/registry.rs | 12 ++ src/quality/metrics.rs | 104 +++++++++++++- ...napshot_tests__diff_compliance_120x40.snap | 2 +- ...napshot_tests__view_compliance_120x40.snap | 2 +- 7 files changed, 321 insertions(+), 36 deletions(-) diff --git a/src/model/crypto.rs b/src/model/crypto.rs index de005372..191eb977 100644 --- a/src/model/crypto.rs +++ b/src/model/crypto.rs @@ -168,6 +168,26 @@ impl AlgorithmProperties { self.primitive == CryptoPrimitive::Combiner } + /// Returns `true` if this is a CLASSICAL public-key algorithm broken by a + /// cryptographically-relevant quantum computer (Shor's algorithm): RSA, + /// finite-field / elliptic-curve Diffie-Hellman, DSA/ECDSA/EdDSA, ElGamal. + /// + /// These are quantum-vulnerable regardless of key size or a declared + /// `nistQuantumSecurityLevel` — the family alone is authoritative. Matched + /// on `algorithm_family` (case-insensitive); the PQC families (ML-KEM, + /// ML-DSA, SLH-DSA, …) are not in the list and correctly return `false`. + #[must_use] + pub fn is_classical_quantum_vulnerable(&self) -> bool { + const CLASSICAL_PK: &[&str] = &[ + "RSA", "DSA", "DH", "DHE", "ECDH", "ECDHE", "ECDSA", "EDDSA", "ED25519", "ED448", + "X25519", "X448", "ELGAMAL", "ECIES", "ECMQV", + ]; + self.algorithm_family.as_deref().is_some_and(|f| { + let upper = f.to_uppercase(); + CLASSICAL_PK.iter().any(|c| upper == *c) + }) + } + /// Returns `true` if the algorithm is considered broken or weak. /// Checks `algorithm_family` first, then falls back to matching /// common weak names in the `parameter_set_identifier`. diff --git a/src/quality/compliance/crypto.rs b/src/quality/compliance/crypto.rs index f0396578..4b49538d 100644 --- a/src/quality/compliance/crypto.rs +++ b/src/quality/compliance/crypto.rs @@ -2,6 +2,25 @@ use super::*; +/// Whether a SHA-2 hash of digest size < 384 bits is indicated, given the +/// uppercased family string and the parameter set. +/// +/// CNSA 2.0 approves only SHA-384 and SHA-512. A weak SHA-2 digest can be +/// encoded as the family carrying the size (`SHA-224`, `SHA-256`) or as a +/// generic family (`SHA-2`/`SHA2`) with the size in the parameter. Both forms +/// (previously only the latter, and only for exactly "256") are caught. +fn is_weak_cnsa_hash(family_upper: &str, param: Option<&str>) -> bool { + // Family carries the digest size directly. + if matches!(family_upper, "SHA-224" | "SHA224" | "SHA-256" | "SHA256") { + return true; + } + // Generic SHA-2 family with a weak size in the parameter. + if matches!(family_upper, "SHA-2" | "SHA2") { + return matches!(param, Some("224") | Some("256")); + } + false +} + impl ComplianceChecker { // ════════════════════════════════════════════════════════════════════ // CNSA 2.0 compliance checks @@ -10,6 +29,8 @@ impl ComplianceChecker { pub(crate) fn check_cnsa2(&self, sbom: &NormalizedSbom, violations: &mut Vec) { use crate::model::{ComponentType, CryptoAssetType}; + let mut crypto_assets_evaluated = 0usize; + for comp in sbom.components.values() { if comp.component_type != ComponentType::Cryptographic { continue; @@ -17,6 +38,7 @@ impl ComplianceChecker { let Some(cp) = &comp.crypto_properties else { continue; }; + crypto_assets_evaluated += 1; match cp.asset_type { CryptoAssetType::Algorithm => { @@ -63,37 +85,51 @@ impl ComplianceChecker { } } - // CNSA2-ALG-001: symmetric must be AES-256 + // CNSA2-ALG-001: symmetric must be AES-256. The key + // size may be carried in parameter_set_identifier OR in + // a RelatedCryptoMaterial key size; treat AES as + // non-compliant unless it is explicitly 256, so AES-128 + // does not slip through on an absent parameter. if let Some(family) = &algo.algorithm_family { let upper = family.to_uppercase(); - if upper == "AES" - && let Some(param) = &algo.parameter_set_identifier - && param != "256" - { - violations.push(Violation { - severity: ViolationSeverity::Error, - category: ViolationCategory::CryptographyInfo, - message: format!( - "'{}' uses AES-{}, CNSA 2.0 requires AES-256 only", - comp.name, param - ), - element: Some(comp.name.clone()), - requirement: "CNSA 2.0 Symmetric".to_string(), - rule_id: "SBOM-CNSA2-ALG-001", - standard_refs: Vec::new(), - }); + if upper == "AES" { + let is_256 = algo.parameter_set_identifier.as_deref() + == Some("256") + || algo.classical_security_level == Some(256); + if !is_256 { + let shown = algo + .parameter_set_identifier + .as_deref() + .map(str::to_string) + .or_else(|| { + algo.classical_security_level.map(|b| b.to_string()) + }) + .unwrap_or_else(|| "unspecified".to_string()); + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::CryptographyInfo, + message: format!( + "'{}' uses AES-{shown}, CNSA 2.0 requires AES-256 only", + comp.name + ), + element: Some(comp.name.clone()), + requirement: "CNSA 2.0 Symmetric".to_string(), + rule_id: "SBOM-CNSA2-ALG-001", + standard_refs: Vec::new(), + }); + } } - // CNSA2-ALG-002: hash must be SHA-384+ - if (upper == "SHA-2" || upper == "SHA2") - && let Some(param) = &algo.parameter_set_identifier - && param == "256" - { + // CNSA2-ALG-002: hash must be SHA-384 or SHA-512. + // Recognize SHA-2 at any digest size ≤ 256 whether + // the size is in the family string ("SHA-256", + // "SHA-224") or the parameter ("SHA-2"/param 256). + if is_weak_cnsa_hash(&upper, algo.parameter_set_identifier.as_deref()) { violations.push(Violation { severity: ViolationSeverity::Error, category: ViolationCategory::CryptographyInfo, message: format!( - "'{}' uses SHA-256, CNSA 2.0 requires SHA-384 or SHA-512", + "'{}' uses a SHA-2 digest < 384 bits, CNSA 2.0 requires SHA-384 or SHA-512", comp.name ), element: Some(comp.name.clone()), @@ -197,6 +233,22 @@ impl ComplianceChecker { _ => {} } } + + // CNSA2-000: no cryptographic inventory to evaluate — a "compliant" + // verdict here would be a false CNSA 2.0 claim, so fail. + if crypto_assets_evaluated == 0 { + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::CryptographyInfo, + message: "No cryptographic inventory (CBOM) found; CNSA 2.0 compliance \ + cannot be asserted. Provide cryptographic asset components." + .to_string(), + element: None, + requirement: "CNSA 2.0: cryptographic inventory required".to_string(), + rule_id: "SBOM-CNSA2-000", + standard_refs: Vec::new(), + }); + } } // ════════════════════════════════════════════════════════════════════ @@ -212,6 +264,11 @@ impl ComplianceChecker { "CAST5", ]; + // Track whether we actually evaluated any cryptographic asset. A tool + // asserting PQC compliance must not report "compliant" when it found + // no cryptographic inventory to evaluate. + let mut crypto_assets_evaluated = 0usize; + for comp in sbom.components.values() { if comp.component_type != ComponentType::Cryptographic { continue; @@ -219,18 +276,26 @@ impl ComplianceChecker { let Some(cp) = &comp.crypto_properties else { continue; }; + crypto_assets_evaluated += 1; if cp.asset_type == CryptoAssetType::Algorithm && let Some(algo) = &cp.algorithm_properties { - // PQC-001: quantum-vulnerable algorithm - if algo.nist_quantum_security_level == Some(0) { + // PQC-001: quantum-vulnerable algorithm. A classical public-key + // primitive (RSA/ECDSA/ECDH/DH/DSA/EdDSA/ElGamal) is broken by + // Shor's algorithm regardless of key size, so flag it on the + // family alone — not only when nistQuantumSecurityLevel is an + // explicit 0 (which real-world CBOMs rarely populate). + if algo.nist_quantum_security_level == Some(0) + || algo.is_classical_quantum_vulnerable() + { violations.push(Violation { severity: ViolationSeverity::Error, category: ViolationCategory::CryptographyInfo, message: format!( - "'{}' has nistQuantumSecurityLevel=0, quantum-vulnerable (IR 8547)", - comp.name + "'{}' ({}) is quantum-vulnerable and must migrate to PQC (IR 8547)", + comp.name, + algo.algorithm_family.as_deref().unwrap_or("classical") ), element: Some(comp.name.clone()), requirement: "IR 8547: quantum-vulnerable".to_string(), @@ -349,5 +414,21 @@ impl ComplianceChecker { } } } + + // PQC-000: no cryptographic inventory. Reporting "compliant" when there + // was nothing to evaluate is a false PQC-readiness claim, so fail. + if crypto_assets_evaluated == 0 { + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::CryptographyInfo, + message: "No cryptographic inventory (CBOM) found; NIST PQC readiness \ + cannot be asserted. Provide cryptographic asset components." + .to_string(), + element: None, + requirement: "IR 8547: cryptographic inventory required".to_string(), + rule_id: "SBOM-PQC-000", + standard_refs: Vec::new(), + }); + } } } diff --git a/src/quality/compliance/mod.rs b/src/quality/compliance/mod.rs index 2ef52a4f..ea47e95a 100644 --- a/src/quality/compliance/mod.rs +++ b/src/quality/compliance/mod.rs @@ -1029,6 +1029,88 @@ mod tests { ); } + /// A plain SBOM with NO cryptographic inventory must NOT pass PQC/CNSA2 — + /// that was a vacuous false-pass. It now fails with an "inventory absent" + /// Error. + #[test] + fn crypto_standards_fail_on_empty_inventory() { + let mut sbom = NormalizedSbom::default(); + sbom.add_component(crate::model::Component::new( + "lodash".to_string(), + "lodash@4.17.21".to_string(), + )); + for level in [ComplianceLevel::NistPqc, ComplianceLevel::Cnsa2] { + let result = ComplianceChecker::new(level).check(&sbom); + assert!( + !result.is_compliant, + "{level:?} must not report compliant with no crypto inventory" + ); + assert!( + result + .violations + .iter() + .any(|v| v.severity == ViolationSeverity::Error + && v.message.contains("No cryptographic inventory")), + "{level:?} must emit an inventory-absent error" + ); + } + } + + /// A classical quantum-vulnerable algorithm (RSA/ECDSA/DH) must fail NIST + /// PQC even when nistQuantumSecurityLevel is UNSET (real CBOMs rarely set + /// it to an explicit 0). + #[test] + fn pqc_flags_classical_crypto_with_unset_quantum_level() { + for family in ["RSA", "ECDSA", "ECDH", "DH", "DSA"] { + let sbom = make_crypto_sbom(&[("classical", family, None, None)]); + let result = ComplianceChecker::new(ComplianceLevel::NistPqc).check(&sbom); + assert!( + !result.is_compliant, + "{family} with unset quantum level must fail PQC" + ); + assert!( + result + .violations + .iter() + .any(|v| v.severity == ViolationSeverity::Error && v.rule_id == "SBOM-PQC-001"), + "{family} must raise the quantum-vulnerable error" + ); + } + } + + /// SHA-224 and SHA-256 must fail CNSA 2.0 whether the size is in the family + /// string or the parameter (previously only family "SHA-2"/param "256" + /// was caught). + #[test] + fn cnsa2_flags_weak_sha2_in_either_encoding() { + for (family, param) in [ + ("SHA-256", None), + ("SHA-224", None), + ("SHA-2", Some("256")), + ("SHA-2", Some("224")), + ] { + let sbom = make_crypto_sbom(&[("hash", family, param, None)]); + let result = ComplianceChecker::new(ComplianceLevel::Cnsa2).check(&sbom); + assert!( + result + .violations + .iter() + .any(|v| v.rule_id == "SBOM-CNSA2-ALG-002"), + "{family}/{param:?} must fail CNSA 2.0 hash gate" + ); + } + // SHA-384 passes the hash gate. + let ok = make_crypto_sbom(&[("hash", "SHA-384", None, None)]); + let result = ComplianceChecker::new(ComplianceLevel::Cnsa2).check(&ok); + assert!( + !result + .violations + .iter() + .any(|v| v.rule_id == "SBOM-CNSA2-ALG-002"), + "SHA-384 must not trip the hash gate" + ); + } + #[test] fn test_pqc_approved_algorithm_info() { let sbom = make_crypto_sbom(&[("ML-DSA-65", "ML-DSA", Some("65"), Some(3))]); diff --git a/src/quality/compliance/registry.rs b/src/quality/compliance/registry.rs index 8c9faaa1..c8bf13b3 100644 --- a/src/quality/compliance/registry.rs +++ b/src/quality/compliance/registry.rs @@ -763,6 +763,12 @@ pub fn rule_meta(rule_id: &str) -> Option { remediation: REMEDIATION_GENERIC, }, // ---- CNSA 2.0 ---------------------------------------------------- + "SBOM-CNSA2-000" => RuleMeta { + sarif_id: "SBOM-CNSA2-000", + default_severity: ViolationSeverity::Error, + refs: &[(K::Cnsa2, "CNSA 2.0")], + remediation: REMEDIATION_GENERIC, + }, "SBOM-CNSA2-ALG-001" => RuleMeta { sarif_id: "SBOM-CNSA2-ALG-001", default_severity: ViolationSeverity::Error, @@ -806,6 +812,12 @@ pub fn rule_meta(rule_id: &str) -> Option { remediation: REMEDIATION_GENERIC, }, // ---- NIST PQC ---------------------------------------------------- + "SBOM-PQC-000" => RuleMeta { + sarif_id: "SBOM-PQC-000", + default_severity: ViolationSeverity::Error, + refs: &[], + remediation: REMEDIATION_GENERIC, + }, "SBOM-PQC-001" => RuleMeta { sarif_id: "SBOM-PQC-001", default_severity: ViolationSeverity::Error, diff --git a/src/quality/metrics.rs b/src/quality/metrics.rs index c323ea89..f93f2d2f 100644 --- a/src/quality/metrics.rs +++ b/src/quality/metrics.rs @@ -1619,11 +1619,15 @@ impl CryptographyMetrics { if comp.component_type != ComponentType::Cryptographic { continue; } - m.total_crypto_components += 1; - let Some(cp) = &comp.crypto_properties else { + // A Cryptographic component with no cryptoProperties carries no + // evaluable crypto data. Counting it toward total_crypto_ + // components would make has_data() true and let the CBOM + // sub-scores return 100 on empty denominators (grade A for + // undocumented crypto), so only count documented assets. continue; }; + m.total_crypto_components += 1; match cp.asset_type { CryptoAssetType::Algorithm => { @@ -1643,10 +1647,16 @@ impl CryptographyMetrics { { m.algorithms_with_security_level += 1; } - if algo.is_quantum_safe() { - m.quantum_safe_count += 1; - } else if algo.nist_quantum_security_level == Some(0) { + // A classical public-key family (RSA/ECDSA/DH/…) is + // quantum-vulnerable on the family alone — real CBOMs + // rarely set nistQuantumSecurityLevel=0, so counting + // only Some(0) let classical crypto escape the penalty. + if algo.is_classical_quantum_vulnerable() + || algo.nist_quantum_security_level == Some(0) + { m.quantum_vulnerable_count += 1; + } else if algo.is_quantum_safe() { + m.quantum_safe_count += 1; } if algo.is_weak_by_name(&comp.name) { m.weak_algorithm_count += 1; @@ -1691,14 +1701,35 @@ impl CryptographyMetrics { if mat.state == Some(CryptoMaterialState::Compromised) { m.compromised_keys += 1; } - // Flag inadequate key sizes + // Flag inadequate key sizes. A key size is a BIT-LENGTH, + // and its adequacy is key-type-dependent: an ECC curve + // bit-length (P-256 → 256) provides ~size/2-bit security, + // so 256-bit ECC is strong, whereas RSA/finite-field + // needs ≥2048. The old flat `<2048` rule false-failed + // every standard ECC key. Recognize the strong ECC curve + // sizes; otherwise apply the finite-field ≥2048 rule. if let Some(size) = mat.size { let is_symmetric = matches!( mat.material_type, crate::model::CryptoMaterialType::SymmetricKey | crate::model::CryptoMaterialType::SecretKey ); - if (is_symmetric && size < 128) || (!is_symmetric && size < 2048) { + // Curve bit-lengths giving ≥128-bit security: + // Curve25519(255), P-256, P-384, Curve448, P-521. + // 512 is deliberately EXCLUDED: a 512-bit RSA/DSA key + // is trivially factorable, and matching it here would + // false-PASS it; a 512-bit ECC curve (brainpoolP512r1, + // rare) instead takes the finite-field path and is + // flagged — an acceptable over-caution vs. a false pass. + const STRONG_ECC_SIZES: &[u32] = &[255, 256, 384, 448, 521]; + let inadequate = if is_symmetric { + size < 128 + } else if STRONG_ECC_SIZES.contains(&size) { + false + } else { + size < 2048 + }; + if inadequate { m.inadequate_key_sizes += 1; } } @@ -1994,6 +2025,65 @@ fn is_restrictive_license(expr: &str) -> bool { mod tests { use super::*; + /// A Cryptographic component with NO cryptoProperties must not count as + /// crypto inventory — otherwise has_data() is true and the CBOM sub-scores + /// return 100 (grade A) for undocumented crypto. + #[test] + fn property_less_crypto_component_is_not_crypto_inventory() { + use crate::model::{Component, ComponentType, NormalizedSbom}; + let mut sbom = NormalizedSbom::default(); + let mut c = Component::new("mystery-crypto".to_string(), "mc@1".to_string()); + c.component_type = ComponentType::Cryptographic; + // No crypto_properties set. + sbom.add_component(c); + + let m = CryptographyMetrics::from_sbom(&sbom); + assert_eq!( + m.total_crypto_components, 0, + "undocumented crypto is not inventory" + ); + assert!( + !m.has_data(), + "has_data must be false → CBOM scores are N/A, not 100" + ); + } + + /// Standard ECC keys (P-256/384/etc.) must NOT be flagged as inadequate — + /// their size is the curve bit-length, giving ~size/2-bit security. The old + /// flat `<2048` rule false-failed every ECC key. RSA-1024 stays flagged. + #[test] + fn ecc_key_sizes_are_not_falsely_inadequate() { + use crate::model::{ + Component, ComponentType, CryptoAssetType, CryptoMaterialType, CryptoProperties, + NormalizedSbom, RelatedCryptoMaterialProperties, + }; + let key = |name: &str, mtype: CryptoMaterialType, size: u32| { + let mut c = Component::new(name.to_string(), format!("{name}@1")); + c.component_type = ComponentType::Cryptographic; + let mat = RelatedCryptoMaterialProperties::new(mtype).with_size(size); + c.crypto_properties = Some( + CryptoProperties::new(CryptoAssetType::RelatedCryptoMaterial) + .with_related_crypto_material_properties(mat), + ); + c + }; + let mut sbom = NormalizedSbom::default(); + sbom.add_component(key("ecc-p256", CryptoMaterialType::PublicKey, 256)); + sbom.add_component(key("ecc-p384", CryptoMaterialType::PublicKey, 384)); + sbom.add_component(key("ecc-p521", CryptoMaterialType::PublicKey, 521)); + sbom.add_component(key("rsa-1024", CryptoMaterialType::PublicKey, 1024)); + // A 512-bit key must be inadequate — it would be a factorable RSA/DSA + // key; 512 must NOT be treated as a "strong ECC" size. + sbom.add_component(key("weak-512", CryptoMaterialType::PublicKey, 512)); + sbom.add_component(key("aes-256", CryptoMaterialType::SymmetricKey, 256)); + + let m = CryptographyMetrics::from_sbom(&sbom); + assert_eq!( + m.inadequate_key_sizes, 2, + "RSA-1024 and the 512-bit key are inadequate; strong ECC (256/384/521) is not" + ); + } + #[test] fn test_purl_validation() { assert!(is_valid_purl("pkg:npm/@scope/name@1.0.0")); diff --git a/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_compliance_120x40.snap b/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_compliance_120x40.snap index 59a8527e..3a8b5b7f 100644 --- a/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_compliance_120x40.snap +++ b/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_compliance_120x40.snap @@ -8,7 +8,7 @@ sbom-tools DIFF │ acme-webapp@1.0.0 ⟷ acme-webapp@2.0.0 ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Compliance Standards (←/→) - ✓ Min │ ✗ Std │ ✗ NTIA │ ✗ CRA-1 │ ✗ CRA-2 │ ✗ FDA │ ✗ SSDF │ ✗ EO14028 │ ✓ CNSA2 │ ✓ PQC │ ✗ BSI + ✓ Min │ ✗ Std │ ✗ NTIA │ ✗ CRA-1 │ ✗ CRA-2 │ ✗ FDA │ ✗ SSDF │ ✗ EO14028 │ ✗ CNSA2 │ ✗ PQC │ ✗ BSI ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ┌ EU CRA Phase 1 (2027): FAIL 54% → FAIL 54% unchanged ───────────────────────────────────────────────────────────────┐ │Errors: 3 Warnings: 14 Info: 26 │ New: 13 Resolved: 13 │ diff --git a/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_compliance_120x40.snap b/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_compliance_120x40.snap index 35777703..c957369f 100644 --- a/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_compliance_120x40.snap +++ b/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_compliance_120x40.snap @@ -8,7 +8,7 @@ sbom-tools VIEW │ acme-webapp │ CycloneDX 1.5 │ [SBOM] ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ┌ Compliance Standards (←/→ to switch) ────────────────────────────────────────────────────────────────────────────────┐ -│ ⚠ Min(10) ✗ Std(13) ✗ NTIA(20) ✗ CRA-1(43) ✗ CRA-2(47) ✗ FDA(33) ✗ SSDF(9) ✗ EO14028(7) ✓ CNSA2 ✓ PQC › │ +│ ⚠ Min(10) ✗ Std(13) ✗ NTIA(20) ✗ CRA-1(43) ✗ CRA-2(47) ✗ FDA(33) ✗ SSDF(9) ✗ EO14028(7) ✗ CNSA2(1) › │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ ┌ NTIA Minimum Elements ───────────────────────────────────────────────────────────┐┌ Summary ─────────────────────────┐ │ NON-COMPLIANT — Errors must be fixed ││ Errors: 11 │ From 3f996e5f2df032e4617b00a2053b9032f1b4b6d6 Mon Sep 17 00:00:00 2001 From: Alex Matrosov Date: Tue, 7 Jul 2026 01:38:18 -0700 Subject: [PATCH 2/4] fix(quality): make required compliance elements gate + reconcile quality exit code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second cluster from the quality/compliance audit — gate mis-mappings where a required element was declared mandatory but never actually failed the verdict, plus a score/verdict exit-code inconsistency. Threat model is the same false regulatory claim; each fix flips a false verdict in the correct direction, and the A/B confirms a fully-complete SBOM is not newly false-failed. Required elements now gate: - NTIA "Timestamp" (one of the seven required minimum data fields) was a literal no-op: the check was skipped on the assumption "we always set a timestamp." But a missing/invalid source timestamp is stored as the UNIX_EPOCH sentinel, so it now gates on has_known_timestamp() with an Error (SBOM-NTIA-TIMESTAMP) for NtiaMinimum/Comprehensive. - EO 14028 §4(e) Supplier Name — a required NTIA element — was a non-gating Warning behind a >30% missing threshold while version and unique-id were Errors. It is now a gating Error on any missing supplier, consistent with the other required elements. - BSI TR-03183-2 §5.3 mandatory component name was a comment-only no-op that claimed the name was "already enforced globally" — but the dedicated BSI checker never runs the generic component check, so a nameless component passed. It now enforces the name (SBOM-BSI-TR-03183-2-5-3). False-fail fixed: - CRA Art. 24 OSS-steward vulnerability-handling gate only inspected component external refs and the sidecar, ignoring the document-level vulnerability_disclosure_url / security_contact that its own doc comment says satisfy it. Those document fields now count (a strict OR-widening). Score/verdict exit code: - `quality` printed the compliance verdict ("NON-COMPLIANT (N errors)") but its only exit gate was --min-score, so a grade-A but non-compliant SBOM exited 0. Added an opt-in --fail-on-noncompliant flag that exits COMPLIANCE_ERRORS when the verdict is non-compliant; the default stays score-only so existing scripts are unaffected. Exit-code help text updated. N/A stays compliant (deliberate): a standard that does not apply to an SBOM (e.g. EU AI Act on a non-AI SBOM) is not a violation, so is_compliant is left true — unlike the crypto case, where no crypto inventory under a *crypto* standard means readiness cannot be asserted. The CRA "Default product class under-gates" behavior is the intended CRA-P3.2 severity-tiering-by-class, not a bug, and is left as-is. Also aligned two registry default_severity entries (EO14028-SUPPLIER, BSI-5-3) to the Error severity actually emitted, and expanded the Art.24 remediation message to mention the document-level satisfiers. Verified: verdict A/B vs the crypto-cluster commit (all four gates flip in the correct direction with the expected rule_ids; a fully-complete SBOM stays compliant under NTIA/EO/BSI; the flag gates only when set, confirmed through the built binary) and an over-correction audit (the widenings cannot introduce a false-fail; compliant golden fixtures still pass zero errors). 4 new tests; full suite green (1653 tests), clippy -D warnings and fmt clean; no golden/snapshot churn. Co-Authored-By: Claude Fable 5 --- src/cli/quality.rs | 19 ++++++ src/main.rs | 11 +++- src/quality/compliance/bsi.rs | 23 ++++++- src/quality/compliance/cra.rs | 29 +++++---- src/quality/compliance/eo14028.rs | 28 +++++---- src/quality/compliance/generic.rs | 21 +++++-- src/quality/compliance/mod.rs | 97 ++++++++++++++++++++++++++++++ src/quality/compliance/registry.rs | 10 ++- 8 files changed, 201 insertions(+), 37 deletions(-) diff --git a/src/cli/quality.rs b/src/cli/quality.rs index 87650394..5321ad18 100644 --- a/src/cli/quality.rs +++ b/src/cli/quality.rs @@ -21,6 +21,10 @@ pub struct QualityConfig { pub show_recommendations: bool, pub show_metrics: bool, pub min_score: Option, + /// Exit non-zero when the compliance verdict is non-compliant (opt-in; + /// the default gate is `--min-score` only, so existing scripts are + /// unaffected). + pub fail_on_noncompliant: bool, pub no_color: bool, /// Optional CRA sidecar metadata path (auto-discovered next to the SBOM /// when None). Supplements the embedded compliance check used by the @@ -47,6 +51,7 @@ pub fn run_quality( show_recommendations: bool, show_metrics: bool, min_score: Option, + fail_on_noncompliant: bool, no_color: bool, cra_sidecar_path: Option, cra_product_class: Option, @@ -60,6 +65,7 @@ pub fn run_quality( show_recommendations, show_metrics, min_score, + fail_on_noncompliant, no_color, cra_sidecar_path, cra_product_class, @@ -157,6 +163,18 @@ fn run_quality_impl(config: QualityConfig) -> Result { return Ok(exit_codes::QUALITY_BELOW_THRESHOLD); } + // Opt-in: fail the command when the compliance verdict is non-compliant, + // so the printed "NON-COMPLIANT" cannot be paired with a success exit. + // Off by default, so `quality --min-score` keeps its score-only contract. + if config.fail_on_noncompliant && !report.compliance.is_compliant { + tracing::error!( + "SBOM is non-compliant with {} ({} error(s))", + report.compliance.level.name(), + report.compliance.error_count + ); + return Ok(exit_codes::COMPLIANCE_ERRORS); + } + Ok(exit_codes::SUCCESS) } @@ -684,6 +702,7 @@ mod tests { show_recommendations: true, show_metrics: true, min_score, + fail_on_noncompliant: false, no_color: true, cra_sidecar_path: None, cra_product_class: None, diff --git a/src/main.rs b/src/main.rs index 2266358d..03c4b742 100644 --- a/src/main.rs +++ b/src/main.rs @@ -721,14 +721,15 @@ struct MatrixArgs { /// Arguments for the `quality` subcommand #[derive(Parser)] #[command(after_help = "EXIT CODES: - 0 Score meets the --min-score threshold (or no threshold set) - 1 Overall score below --min-score + 0 Score meets --min-score (or no threshold) and, with --fail-on-noncompliant, the SBOM is compliant + 1 Overall score below --min-score, OR (with --fail-on-noncompliant) the SBOM is non-compliant 3 Error occurred EXAMPLES: sbom-tools quality app.cdx.json # Score overview sbom-tools quality app.cdx.json --profile security --metrics # Detailed metrics sbom-tools quality app.cdx.json --min-score 70 -o json # CI gate with export + sbom-tools quality app.cdx.json --profile cra --fail-on-noncompliant # gate on compliance sbom-tools quality app.cdx.json --recommendations # Improvement suggestions")] struct QualityArgs { /// Path to the SBOM file @@ -758,6 +759,11 @@ struct QualityArgs { #[arg(long)] min_score: Option, + /// Exit non-zero when the SBOM is non-compliant with the report's + /// compliance standard (the score gate `--min-score` ignores compliance). + #[arg(long)] + fail_on_noncompliant: bool, + /// Path to a CRA sidecar metadata file (JSON or YAML). /// Consulted by the embedded CRA compliance check when running /// `--profile cra`. Auto-discovered next to the SBOM if omitted. @@ -1880,6 +1886,7 @@ fn main() -> Result<()> { args.recommendations, args.metrics, args.min_score, + args.fail_on_noncompliant, resolve_bool(cli.no_color, app.output.no_color), args.cra_sidecar, args.cra_product_class, diff --git a/src/quality/compliance/bsi.rs b/src/quality/compliance/bsi.rs index b73c60f7..a9fdf2f2 100644 --- a/src/quality/compliance/bsi.rs +++ b/src/quality/compliance/bsi.rs @@ -73,9 +73,26 @@ impl ComplianceChecker { }); } - // §5.3 — Component name (mandatory) — already enforced globally; - // we add a BSI-specific message only if many components are - // missing names (extreme case). + // §5.3 — Component name (mandatory). The BsiTr03183_2 level dispatches + // ONLY to this checker (not the generic component check), so the name + // requirement must be enforced here — it is not "already enforced + // globally" for this standard. + let nameless = sbom + .components + .values() + .filter(|c| c.name.trim().is_empty()) + .count(); + if nameless > 0 { + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::ComponentIdentification, + message: format!("{nameless} component(s) missing a name (BSI §5.3 mandatory)"), + element: None, + requirement: "BSI TR-03183-2 §5.3: Component name".to_string(), + rule_id: "SBOM-BSI-TR-03183-2-5-3", + standard_refs: Vec::new(), + }); + } // §5.3 — Component identifier: PURL or other recognised ID (mandatory). // Stricter than CRA: BSI requires a PURL where the ecosystem applies. diff --git a/src/quality/compliance/cra.rs b/src/quality/compliance/cra.rs index 1ebf443a..171b0b78 100644 --- a/src/quality/compliance/cra.rs +++ b/src/quality/compliance/cra.rs @@ -941,24 +941,27 @@ impl ComplianceChecker { // a vulnerability-disclosure URL on the document or a SecurityContact // / Advisories external reference on at least one component, OR // sidecar-supplied PSIRT URL. - let has_vuln_handling = sbom.components.values().any(|c| { - c.external_refs.iter().any(|r| { - matches!( - r.ref_type, - crate::model::ExternalRefType::SecurityContact - | crate::model::ExternalRefType::Advisories - | crate::model::ExternalRefType::VulnerabilityAssertion - ) + let has_vuln_handling = sbom.document.vulnerability_disclosure_url.is_some() + || sbom.document.security_contact.is_some() + || sbom.components.values().any(|c| { + c.external_refs.iter().any(|r| { + matches!( + r.ref_type, + crate::model::ExternalRefType::SecurityContact + | crate::model::ExternalRefType::Advisories + | crate::model::ExternalRefType::VulnerabilityAssertion + ) + }) }) - }) || self - .sidecar - .as_ref() - .is_some_and(|s| s.psirt_url.is_some() || s.vulnerability_disclosure_url.is_some()); + || self + .sidecar + .as_ref() + .is_some_and(|s| s.psirt_url.is_some() || s.vulnerability_disclosure_url.is_some()); if !has_vuln_handling { violations.push(Violation { severity: ViolationSeverity::Error, category: ViolationCategory::SecurityInfo, - message: "[CRA Art. 24 / Annex I Part II] OSS steward must operate a vulnerability-handling process — declare a SecurityContact / Advisories external reference, or set psirt_url / vulnerability_disclosure_url in the sidecar".to_string(), + message: "[CRA Art. 24 / Annex I Part II] OSS steward must operate a vulnerability-handling process — set a document-level security contact or vulnerability-disclosure URL, declare a SecurityContact / Advisories external reference, or set psirt_url / vulnerability_disclosure_url in the sidecar".to_string(), element: None, requirement: "CRA Art. 24: Vulnerability-handling process (steward floor)" .to_string(), diff --git a/src/quality/compliance/eo14028.rs b/src/quality/compliance/eo14028.rs index 2dcea10c..d1d5a6fc 100644 --- a/src/quality/compliance/eo14028.rs +++ b/src/quality/compliance/eo14028.rs @@ -177,20 +177,22 @@ impl ComplianceChecker { .filter(|c| c.supplier.is_none() && c.author.is_none()) .count(); if without_supplier > 0 { + // Supplier Name is a REQUIRED NTIA minimum element, and EO 14028 + // §4(e) mandates the NTIA minimum elements — so a missing supplier + // is a gating Error (as version and unique-id already are), not a + // Warning behind a >30% threshold. let pct = (without_supplier * 100) / total.max(1); - if pct > 30 { - violations.push(Violation { - severity: ViolationSeverity::Warning, - category: ViolationCategory::SupplierInfo, - message: format!( - "{without_supplier}/{total} components ({pct}%) missing supplier information" - ), - element: None, - requirement: "EO 14028 Sec 4(e): Supplier identification".to_string(), - rule_id: "SBOM-EO14028-SUPPLIER", - standard_refs: Vec::new(), - }); - } + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::SupplierInfo, + message: format!( + "{without_supplier}/{total} components ({pct}%) missing supplier information" + ), + element: None, + requirement: "EO 14028 Sec 4(e): Supplier identification".to_string(), + rule_id: "SBOM-EO14028-SUPPLIER", + standard_refs: Vec::new(), + }); } } } diff --git a/src/quality/compliance/generic.rs b/src/quality/compliance/generic.rs index 81550685..3bd8c3ab 100644 --- a/src/quality/compliance/generic.rs +++ b/src/quality/compliance/generic.rs @@ -387,13 +387,26 @@ impl ComplianceChecker { } } - // NTIA requires timestamp + // NTIA "Timestamp" is one of the seven required minimum data fields. + // `DocumentMetadata::created` is never None, but a missing/invalid + // source timestamp is stored as the UNIX_EPOCH sentinel (see + // `has_known_timestamp`), so gate on that rather than assuming it is + // always meaningful. if matches!( self.level, ComplianceLevel::NtiaMinimum | ComplianceLevel::Comprehensive - ) { - // Timestamp is always set in our model, but check if it's meaningful - // For now, we'll skip this check as we always set a timestamp + ) && !sbom.document.has_known_timestamp() + { + violations.push(Violation { + severity: ViolationSeverity::Error, + category: ViolationCategory::DocumentMetadata, + message: "SBOM is missing a creation timestamp (NTIA required data field)" + .to_string(), + element: None, + requirement: "NTIA Minimum Elements: Timestamp".to_string(), + rule_id: "SBOM-NTIA-TIMESTAMP", + standard_refs: Vec::new(), + }); } // Standard+ requires serial number/document ID diff --git a/src/quality/compliance/mod.rs b/src/quality/compliance/mod.rs index ea47e95a..a50fa9df 100644 --- a/src/quality/compliance/mod.rs +++ b/src/quality/compliance/mod.rs @@ -884,6 +884,103 @@ impl Default for ComplianceChecker { mod tests { use super::*; + /// NTIA lists Timestamp as a required data field; it must gate. A + /// timestamp-less SBOM (epoch sentinel) now fails NtiaMinimum. + #[test] + fn ntia_gates_on_missing_timestamp() { + use crate::model::{Component, DocumentMetadata, NormalizedSbom}; + let comp = |sbom: &mut NormalizedSbom| { + let c = Component::new("lib".to_string(), "lib@1".to_string()) + .with_version("1.0".to_string()) + .with_purl("pkg:cargo/lib@1.0".to_string()); + sbom.add_component(c); + }; + + // No real timestamp (epoch sentinel) → NTIA timestamp error. + let mut no_ts = NormalizedSbom::new(DocumentMetadata::default()); + no_ts.document.created = chrono::DateTime::UNIX_EPOCH; + comp(&mut no_ts); + let r = ComplianceChecker::new(ComplianceLevel::NtiaMinimum).check(&no_ts); + assert!( + r.violations + .iter() + .any(|v| v.rule_id == "SBOM-NTIA-TIMESTAMP" + && v.severity == ViolationSeverity::Error), + "missing timestamp must fail NTIA" + ); + + // A real timestamp → no timestamp error. + let mut with_ts = NormalizedSbom::new(DocumentMetadata::default()); // now() + comp(&mut with_ts); + let r = ComplianceChecker::new(ComplianceLevel::NtiaMinimum).check(&with_ts); + assert!( + !r.violations + .iter() + .any(|v| v.rule_id == "SBOM-NTIA-TIMESTAMP") + ); + } + + /// EO 14028 §4(e) requires Supplier Name (an NTIA element); a missing + /// supplier must be a gating Error, not a sub-threshold Warning. + #[test] + fn eo14028_gates_on_missing_supplier() { + use crate::model::{Component, DocumentMetadata, NormalizedSbom}; + let mut sbom = NormalizedSbom::new(DocumentMetadata::default()); + // Component with version + id but NO supplier. + sbom.add_component( + Component::new("lib".to_string(), "lib@1".to_string()) + .with_version("1.0".to_string()) + .with_purl("pkg:cargo/lib@1.0".to_string()), + ); + let r = ComplianceChecker::new(ComplianceLevel::Eo14028).check(&sbom); + assert!( + r.violations + .iter() + .any(|v| v.rule_id == "SBOM-EO14028-SUPPLIER" + && v.severity == ViolationSeverity::Error), + "missing supplier must be a gating Error under EO 14028" + ); + } + + /// BSI TR-03183-2 §5.3 makes component name mandatory; the dedicated BSI + /// checker must enforce it (it does not run the generic component check). + #[test] + fn bsi_gates_on_nameless_component() { + use crate::model::{Component, DocumentMetadata, NormalizedSbom}; + let mut sbom = NormalizedSbom::new(DocumentMetadata::default()); + let mut c = + Component::new(String::new(), "ref-1".to_string()).with_version("1.0".to_string()); + c.identifiers.purl = Some("pkg:cargo/x@1.0".to_string()); + sbom.add_component(c); + let r = ComplianceChecker::new(ComplianceLevel::BsiTr03183_2).check(&sbom); + assert!( + r.violations + .iter() + .any(|v| v.rule_id == "SBOM-BSI-TR-03183-2-5-3"), + "a nameless component must fail BSI §5.3" + ); + } + + /// CRA Art. 24 steward vuln-handling is satisfied by a DOCUMENT-level + /// disclosure URL — the check previously ignored doc fields (false-fail). + #[test] + fn cra_art24_honors_document_level_disclosure() { + use crate::model::{Component, DocumentMetadata, NormalizedSbom}; + let mut sbom = NormalizedSbom::new(DocumentMetadata::default()); + sbom.document.vulnerability_disclosure_url = + Some("https://example.org/security".to_string()); + sbom.add_component( + Component::new("lib".to_string(), "lib@1".to_string()) + .with_version("1.0".to_string()) + .with_purl("pkg:cargo/lib@1.0".to_string()), + ); + let r = ComplianceChecker::new(ComplianceLevel::CraOssSteward).check(&sbom); + assert!( + !r.violations.iter().any(|v| v.rule_id == "SBOM-CRA-ART-24"), + "a document-level disclosure URL must satisfy the Art.24 vuln-handling gate" + ); + } + #[test] fn test_compliance_level_names() { assert_eq!(ComplianceLevel::Minimum.name(), "Minimum"); diff --git a/src/quality/compliance/registry.rs b/src/quality/compliance/registry.rs index c8bf13b3..abacf1fc 100644 --- a/src/quality/compliance/registry.rs +++ b/src/quality/compliance/registry.rs @@ -499,6 +499,12 @@ pub fn rule_meta(rule_id: &str) -> Option { refs: &[(K::NtiaMinimum, "NTIA Minimum Elements")], remediation: REMEDIATION_GENERIC, }, + "SBOM-NTIA-TIMESTAMP" => RuleMeta { + sarif_id: "SBOM-NTIA-TIMESTAMP", + default_severity: ViolationSeverity::Error, + refs: &[(K::NtiaMinimum, "NTIA Minimum Elements")], + remediation: REMEDIATION_GENERIC, + }, "SBOM-NTIA-SUPPLIER" => RuleMeta { sarif_id: "SBOM-NTIA-SUPPLIER", default_severity: ViolationSeverity::Error, @@ -721,7 +727,7 @@ pub fn rule_meta(rule_id: &str) -> Option { }, "SBOM-EO14028-SUPPLIER" => RuleMeta { sarif_id: "SBOM-EO14028-SUPPLIER", - default_severity: ViolationSeverity::Warning, + default_severity: ViolationSeverity::Error, refs: &[(K::Eo14028, "EO 14028 §4")], remediation: REMEDIATION_EO14028, }, @@ -740,7 +746,7 @@ pub fn rule_meta(rule_id: &str) -> Option { }, "SBOM-BSI-TR-03183-2-5-3" => RuleMeta { sarif_id: "SBOM-BSI-TR-03183-2-5-3", - default_severity: ViolationSeverity::Warning, + default_severity: ViolationSeverity::Error, refs: &[(K::BsiTr03183_2, "§5.3")], remediation: REMEDIATION_GENERIC, }, From bbacf175d16fcc19df66be1027dc0cb22e0514cb Mon Sep 17 00:00:00 2001 From: Alex Matrosov Date: Tue, 7 Jul 2026 23:31:34 -0700 Subject: [PATCH 3/4] fix(quality): count license/identifier metrics per-component, not per-entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third cluster from the quality audit — metric-counting bugs that inflated scores by conflating per-entry counts with per-component denominators, plus a fake SPDX validator. All fixes are in how metrics COUNT; the A/B confirms a well-formed SBOM scores byte-identically before and after. NOASSERTION counted as licensed: - CompletenessMetrics and LicenseMetrics counted any non-empty declared list as "has license", but the CycloneDX parser emits declared=["NOASSERTION"] for empty license objects — so an SBOM with no real license data reported 100% licensed. A component now needs at least one non-NOASSERTION entry (SPDX NONE still counts as documented: asserting no license exists is information; making no assertion is not). A/B: 4-of-5 NOASSERTION SBOM drops from 100% to 20% licensed. SPDX validity was a substring check: - is_valid_spdx_license accepted any string containing " OR "/" AND "/ " WITH " — "GARBAGE OR NONSENSE" validated. Deleted; metrics now use the is_valid_spdx the model already computes in LicenseExpression::new via the spdx crate (LAX parse). Net direction is MORE accepting of real-world forms (GPL-2.0+, Zlib, MIT/Apache-2.0, case variants now valid; the old 25-id list rejected them) while garbage operands finally fail. - validate_spdx's NOASSERTION/NONE guard now matches whole tokens, so LicenseRef-NONEXCLUSIVE is no longer substring-rejected; and from_spdx_id validates instead of hardcoding is_valid_spdx=true now that scoring trusts the bit. Per-entry counts against per-component denominators: - valid_spdx_expressions counted per declared ENTRY but divided by the per-component with_declared, so one multi-licensed component pushed spdx_ratio to 3.0 and the 30-pt bonus through the 100 clamp (license score 100 at 50% coverage). All license counters are now per-component with valid+non_standard == with_declared, making the ratio <= 1.0 by construction. A/B: that SBOM now scores 60, not 100. - IdentifierMetrics counted CPEs per entry and summed per-type counts, so one component with 3 CPEs masked two components with no identifier at all (coverage 100%). New components_with_valid_id field is the coverage numerator; per-type counts are per-component. A/B: the masking SBOM drops to 33%. Lifecycle double-count: - Deprecated/archived components were counted twice — once for the StalenessLevel and once for the boolean flag, which the enrichment sets together (the normal case, not an edge). Each state now counts a component once; the audit's state-matrix check confirms NEW <= OLD in every combination. Verified: score A/B vs the gate-mapping commit (all five scenarios move by the exact expected amounts; clean SBOM byte-identical; 7-fixture sweep shows only one change, a 59.3->60.3 gain) and an adversarial audit (100+ license forms LAX-vs-old — no legitimate form flips invalid; all construction paths go through LicenseExpression::new; serde round-trip surfaces checked; the concluded-NOASSERTION guard is load-bearing via the SPDX3 path). 5 new tests; full suite green (1657), clippy -D warnings and fmt clean; zero snapshot churn. Co-Authored-By: Claude Fable 5 --- src/model/license.rs | 15 +- src/quality/metrics.rs | 334 ++++++++++++++++++++++++++++++----------- 2 files changed, 258 insertions(+), 91 deletions(-) diff --git a/src/model/license.rs b/src/model/license.rs index 6430857c..46972d69 100644 --- a/src/model/license.rs +++ b/src/model/license.rs @@ -29,10 +29,9 @@ impl LicenseExpression { /// Create from an SPDX license ID #[must_use] pub fn from_spdx_id(id: &str) -> Self { - Self { - expression: id.to_string(), - is_valid_spdx: true, - } + // Validate rather than trusting the caller: scoring now relies on + // `is_valid_spdx`, so a hardcoded `true` would be a footgun. + Self::new(id.to_string()) } /// Validate an SPDX expression using the spdx crate. @@ -40,7 +39,13 @@ impl LicenseExpression { /// Uses lax parsing mode to accept common non-standard expressions /// (e.g., "Apache2" instead of "Apache-2.0", "/" instead of "OR"). fn validate_spdx(expr: &str) -> bool { - if expr.is_empty() || expr.contains("NOASSERTION") || expr.contains("NONE") { + // Reject expressions with a NOASSERTION/NONE clause (no license + // information), matching whole tokens so that legitimate ids like + // `LicenseRef-NONEXCLUSIVE` are not caught by a substring test. + let has_no_info_token = expr + .split(|c: char| c.is_whitespace() || c == '(' || c == ')') + .any(|tok| tok == "NOASSERTION" || tok == "NONE"); + if expr.is_empty() || has_no_info_token { return false; } spdx::Expression::parse_mode(expr, spdx::ParseMode::LAX).is_ok() diff --git a/src/quality/metrics.rs b/src/quality/metrics.rs index f93f2d2f..c8265612 100644 --- a/src/quality/metrics.rs +++ b/src/quality/metrics.rs @@ -70,7 +70,22 @@ impl CompletenessMetrics { if !comp.hashes.is_empty() { with_hashes += 1; } - if !comp.licenses.declared.is_empty() || comp.licenses.concluded.is_some() { + // A NOASSERTION entry carries zero license information (the + // CycloneDX parser emits declared=["NOASSERTION"] for empty + // license objects), so it must not count as "has license". + // SPDX NONE *is* information (the author asserts no license + // exists) and still counts as documented. + let has_license_info = comp + .licenses + .declared + .iter() + .any(|l| l.expression != "NOASSERTION") + || comp + .licenses + .concluded + .as_ref() + .is_some_and(|c| c.expression != "NOASSERTION"); + if has_license_info { with_licenses += 1; } if comp.description.is_some() { @@ -385,12 +400,17 @@ pub struct IdentifierMetrics { pub valid_purls: usize, /// Components with invalid/malformed PURLs pub invalid_purls: usize, - /// Components with valid CPEs + /// Components with at least one valid CPE pub valid_cpes: usize, - /// Components with invalid/malformed CPEs + /// Components with at least one invalid/malformed CPE pub invalid_cpes: usize, /// Components with SWID tags pub with_swid: usize, + /// Components with at least one valid identifier of any kind. This is the + /// coverage numerator: summing the per-type counts would let one + /// multi-identifier component mask components with no identifier at all. + #[serde(default)] + pub components_with_valid_id: usize, /// Unique ecosystems identified pub ecosystems: Vec, /// Components missing all identifiers (only name) @@ -406,6 +426,7 @@ impl IdentifierMetrics { let mut valid_cpes = 0; let mut invalid_cpes = 0; let mut with_swid = 0; + let mut with_valid_id = 0; let mut missing_all = 0; let mut ecosystems = std::collections::HashSet::new(); @@ -414,8 +435,10 @@ impl IdentifierMetrics { let has_cpe = !comp.identifiers.cpe.is_empty(); let has_swid = comp.identifiers.swid.is_some(); + let mut purl_valid = false; if let Some(ref purl) = comp.identifiers.purl { if is_valid_purl(purl) { + purl_valid = true; valid_purls += 1; // Extract ecosystem from PURL if let Some(eco) = extract_ecosystem_from_purl(purl) { @@ -426,18 +449,25 @@ impl IdentifierMetrics { } } - for cpe in &comp.identifiers.cpe { - if is_valid_cpe(cpe) { - valid_cpes += 1; - } else { - invalid_cpes += 1; - } + // Per-COMPONENT, not per-entry: a component with several CPEs + // counts once, so it cannot mask components with no identifier. + let any_cpe_valid = comp.identifiers.cpe.iter().any(|c| is_valid_cpe(c)); + let any_cpe_invalid = comp.identifiers.cpe.iter().any(|c| !is_valid_cpe(c)); + if any_cpe_valid { + valid_cpes += 1; + } + if any_cpe_invalid { + invalid_cpes += 1; } if has_swid { with_swid += 1; } + if purl_valid || any_cpe_valid || has_swid { + with_valid_id += 1; + } + if !has_purl && !has_cpe && !has_swid { missing_all += 1; } @@ -452,6 +482,7 @@ impl IdentifierMetrics { valid_cpes, invalid_cpes, with_swid, + components_with_valid_id: with_valid_id, ecosystems: ecosystem_list, missing_all_identifiers: missing_all, } @@ -464,9 +495,12 @@ impl IdentifierMetrics { return 0.0; } - let with_valid_id = self.valid_purls + self.valid_cpes + self.with_swid; - let coverage = - (with_valid_id.min(total_components) as f32 / total_components as f32) * 100.0; + // Coverage over components with at least one valid identifier — NOT + // the sum of per-type counts, which a single PURL+CPE+SWID component + // would inflate 3x (masking identifier-less components). + let coverage = (self.components_with_valid_id.min(total_components) as f32 + / total_components as f32) + * 100.0; // Penalize invalid identifiers let invalid_count = self.invalid_purls + self.invalid_cpes; @@ -479,17 +513,19 @@ impl IdentifierMetrics { /// License quality metrics #[derive(Debug, Clone, Serialize, Deserialize)] pub struct LicenseMetrics { - /// Components with declared licenses + /// Components with at least one real (non-NOASSERTION) declared license pub with_declared: usize, /// Components with concluded licenses pub with_concluded: usize, - /// Components with valid SPDX expressions + /// Components whose declared licenses are all valid SPDX expressions + /// (subset of `with_declared`) pub valid_spdx_expressions: usize, - /// Components with non-standard license names + /// Components with at least one non-standard declared license name + /// (subset of `with_declared`; disjoint with `valid_spdx_expressions`) pub non_standard_licenses: usize, - /// Components with NOASSERTION license + /// Components with at least one NOASSERTION declared entry pub noassertion_count: usize, - /// Components with deprecated SPDX license identifiers + /// Components with at least one deprecated SPDX license identifier pub deprecated_licenses: usize, /// Components with restrictive/copyleft licenses (GPL family) pub restrictive_licenses: usize, @@ -513,28 +549,58 @@ impl LicenseMetrics { let mut licenses = HashSet::new(); let mut copyleft_ids = HashSet::new(); + // All counters are PER-COMPONENT (matching the field docs). The old + // per-entry counting let a component with several declared licenses + // push valid_spdx_expressions above with_declared, blowing the SPDX + // ratio past 1.0 — and a NOASSERTION-only component counted as + // license-documented. for comp in sbom.components.values() { - if !comp.licenses.declared.is_empty() { - with_declared += 1; - for lic in &comp.licenses.declared { - let expr = &lic.expression; - licenses.insert(expr.clone()); - - if expr == "NOASSERTION" { - noassertion += 1; - } else if is_valid_spdx_license(expr) { - valid_spdx += 1; - } else { - non_standard += 1; - } + let mut has_real_entry = false; + let mut has_noassertion = false; + let mut all_valid = true; + let mut any_deprecated = false; + let mut any_restrictive = false; + + for lic in &comp.licenses.declared { + let expr = &lic.expression; + licenses.insert(expr.clone()); + + if expr == "NOASSERTION" { + has_noassertion = true; + continue; + } + has_real_entry = true; - if is_deprecated_spdx_license(expr) { - deprecated += 1; - } - if is_restrictive_license(expr) { - restrictive += 1; - copyleft_ids.insert(expr.clone()); - } + // is_valid_spdx is computed at construction via the `spdx` + // crate (real expression parsing), unlike the old substring + // heuristic that accepted any string containing " OR ". + if !lic.is_valid_spdx { + all_valid = false; + } + if is_deprecated_spdx_license(expr) { + any_deprecated = true; + } + if is_restrictive_license(expr) { + any_restrictive = true; + copyleft_ids.insert(expr.clone()); + } + } + + if has_noassertion { + noassertion += 1; + } + if has_real_entry { + with_declared += 1; + if all_valid { + valid_spdx += 1; + } else { + non_standard += 1; + } + if any_deprecated { + deprecated += 1; + } + if any_restrictive { + restrictive += 1; } } @@ -1472,16 +1538,20 @@ impl LifecycleMetrics { } if let Some(ref stale_info) = comp.staleness { - match stale_info.level { - StalenessLevel::Stale | StalenessLevel::Abandoned => stale += 1, - StalenessLevel::Deprecated => deprecated += 1, - StalenessLevel::Archived => archived += 1, - _ => {} + if matches!( + stale_info.level, + StalenessLevel::Stale | StalenessLevel::Abandoned + ) { + stale += 1; } - if stale_info.is_deprecated { + // The enrichment sets level=Deprecated AND is_deprecated=true + // together (same for Archived), so counting both branches + // double-counted every deprecated/archived component in the + // normal case. Count each component at most once per state. + if stale_info.level == StalenessLevel::Deprecated || stale_info.is_deprecated { deprecated += 1; } - if stale_info.is_archived { + if stale_info.level == StalenessLevel::Archived || stale_info.is_archived { archived += 1; } if stale_info.latest_version.is_some() { @@ -1934,44 +2004,6 @@ fn is_valid_cpe(cpe: &str) -> bool { cpe.starts_with("cpe:2.3:") || cpe.starts_with("cpe:/") } -fn is_valid_spdx_license(expr: &str) -> bool { - // Common SPDX license identifiers - const COMMON_SPDX: &[&str] = &[ - "MIT", - "Apache-2.0", - "GPL-2.0", - "GPL-3.0", - "BSD-2-Clause", - "BSD-3-Clause", - "ISC", - "MPL-2.0", - "LGPL-2.1", - "LGPL-3.0", - "AGPL-3.0", - "Unlicense", - "CC0-1.0", - "0BSD", - "EPL-2.0", - "CDDL-1.0", - "Artistic-2.0", - "GPL-2.0-only", - "GPL-2.0-or-later", - "GPL-3.0-only", - "GPL-3.0-or-later", - "LGPL-2.1-only", - "LGPL-2.1-or-later", - "LGPL-3.0-only", - "LGPL-3.0-or-later", - ]; - - // Check for common licenses or expressions - let trimmed = expr.trim(); - COMMON_SPDX.contains(&trimmed) - || trimmed.contains(" AND ") - || trimmed.contains(" OR ") - || trimmed.contains(" WITH ") -} - /// Whether a license identifier is on the SPDX deprecated list. /// /// These are license IDs that SPDX has deprecated in favor of more specific @@ -2084,6 +2116,125 @@ mod tests { ); } + /// A NOASSERTION-only component must not count as "has license" — the + /// CycloneDX parser emits declared=["NOASSERTION"] for empty license + /// objects, which carries zero license information. + #[test] + fn noassertion_only_component_is_not_licensed() { + use crate::model::{Component, LicenseExpression, NormalizedSbom}; + let mut sbom = NormalizedSbom::default(); + let mut c = Component::new("no-info".to_string(), "ni@1".to_string()); + c.licenses + .add_declared(LicenseExpression::new("NOASSERTION".to_string())); + sbom.add_component(c); + let mut licensed = Component::new("real".to_string(), "real@1".to_string()); + licensed + .licenses + .add_declared(LicenseExpression::new("MIT".to_string())); + sbom.add_component(licensed); + + let completeness = CompletenessMetrics::from_sbom(&sbom); + assert!( + (completeness.components_with_licenses - 50.0).abs() < 0.01, + "1 of 2 components has real license info, got {}", + completeness.components_with_licenses + ); + + let lm = LicenseMetrics::from_sbom(&sbom); + assert_eq!( + lm.with_declared, 1, + "NOASSERTION-only must not count as declared" + ); + assert_eq!(lm.noassertion_count, 1); + assert_eq!(lm.valid_spdx_expressions, 1); + } + + /// spdx_ratio must never exceed 1.0: a component with several valid + /// declared licenses previously pushed the per-entry numerator above the + /// per-component denominator, blowing the 30-pt SPDX bonus past its cap. + #[test] + fn multi_license_component_does_not_inflate_spdx_bonus() { + use crate::model::{Component, LicenseExpression, NormalizedSbom}; + let mut sbom = NormalizedSbom::default(); + let mut multi = Component::new("multi".to_string(), "m@1".to_string()); + for id in ["MIT", "Apache-2.0", "BSD-3-Clause"] { + multi + .licenses + .add_declared(LicenseExpression::new(id.to_string())); + } + sbom.add_component(multi); + // A second component with NO license at all. + sbom.add_component(Component::new("bare".to_string(), "b@1".to_string())); + + let lm = LicenseMetrics::from_sbom(&sbom); + assert_eq!(lm.with_declared, 1); + assert_eq!(lm.valid_spdx_expressions, 1, "per-component, not per-entry"); + assert!( + lm.valid_spdx_expressions <= lm.with_declared, + "spdx ratio numerator must not exceed its denominator" + ); + // coverage 50% of 60 = 30, bonus 1.0*30 = 30 → 60. The old per-entry + // count gave ratio 3.0 → bonus 90 → clamped 100 despite 50% coverage. + let score = lm.quality_score(2); + assert!( + (score - 60.0).abs() < 0.01, + "expected 60 (half coverage + full SPDX bonus), got {score}" + ); + } + + /// One component with many CPEs must not mask components with no + /// identifier at all in the coverage score. + #[test] + fn multi_cpe_component_does_not_mask_identifierless_ones() { + use crate::model::{Component, NormalizedSbom}; + let mut sbom = NormalizedSbom::default(); + let mut multi = Component::new("multi-cpe".to_string(), "mc@1".to_string()); + for i in 0..3 { + multi + .identifiers + .cpe + .push(format!("cpe:2.3:a:vendor:product{i}:1.0:*:*:*:*:*:*:*")); + } + sbom.add_component(multi); + sbom.add_component(Component::new("bare-1".to_string(), "b1@1".to_string())); + sbom.add_component(Component::new("bare-2".to_string(), "b2@1".to_string())); + + let im = IdentifierMetrics::from_sbom(&sbom); + assert_eq!(im.components_with_valid_id, 1); + assert_eq!(im.valid_cpes, 1, "per-component CPE count"); + assert_eq!(im.missing_all_identifiers, 2); + let score = im.quality_score(3); + assert!( + (score - 33.33).abs() < 0.1, + "1/3 coverage expected, got {score} (old per-entry count gave 100)" + ); + } + + /// Deprecated/archived components are counted once, not twice, when the + /// enrichment sets both the StalenessLevel and the boolean flag. + #[test] + fn lifecycle_does_not_double_count_deprecated() { + use crate::model::{Component, NormalizedSbom, StalenessInfo, StalenessLevel}; + let mut sbom = NormalizedSbom::default(); + let mut c = Component::new("old-pkg".to_string(), "op@1".to_string()); + c.staleness = Some(StalenessInfo { + level: StalenessLevel::Deprecated, + last_published: None, + is_deprecated: true, // enrichment sets both together + is_archived: false, + deprecation_message: None, + days_since_update: None, + latest_version: None, + }); + sbom.add_component(c); + + let lm = LifecycleMetrics::from_sbom(&sbom); + assert_eq!( + lm.deprecated_components, 1, + "one deprecated component must count once, not twice" + ); + } + #[test] fn test_purl_validation() { assert!(is_valid_purl("pkg:npm/@scope/name@1.0.0")); @@ -2099,12 +2250,23 @@ mod tests { assert!(!is_valid_cpe("something:else")); } + /// License validity comes from the model's spdx-crate parse (stored in + /// `LicenseExpression.is_valid_spdx`), not the old substring heuristic + /// that accepted any string containing " OR "/" AND "/" WITH ". #[test] fn test_spdx_license_validation() { - assert!(is_valid_spdx_license("MIT")); - assert!(is_valid_spdx_license("Apache-2.0")); - assert!(is_valid_spdx_license("MIT AND Apache-2.0")); - assert!(is_valid_spdx_license("GPL-2.0 OR MIT")); + use crate::model::LicenseExpression; + let valid = |e: &str| LicenseExpression::new(e.to_string()).is_valid_spdx; + assert!(valid("MIT")); + assert!(valid("Apache-2.0")); + assert!(valid("MIT AND Apache-2.0")); + assert!(valid("GPL-2.0 OR MIT")); + assert!(valid("GPL-2.0-only WITH Classpath-exception-2.0")); + assert!(valid("Zlib")); + // The substring heuristic accepted these; real parsing must not. + assert!(!valid("GARBAGE OR NONSENSE")); + assert!(!valid("foo AND bar")); + assert!(!valid("NOASSERTION")); } #[test] From 80818d9c4d9718f3b8905c5157b7e7aa054f4db6 Mon Sep 17 00:00:00 2001 From: Alex Matrosov Date: Wed, 8 Jul 2026 00:00:31 -0700 Subject: [PATCH 4/4] fix(quality): make scoring monotonic and deterministic (vuln baseline, freshness, coverage clamp) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth and final cluster from the quality audit — score-math defects where the arithmetic itself produced perverse or unstable results. Disclosing a vulnerability lowered the score: - A bare VulnerabilityRef (no CVSS/CWE/remediation) scored the VulnDocs category 0, while an SBOM with NO vulnerability data got N/A and its weight redistributed — so disclosing was worse than stripping the data, punishing exactly the transparency an SBOM exists to provide. documentation_score now grants a 40-point disclosure baseline plus up to 60 for quality (CVSS 24 / CWE 18 / remediation 18 — the old 40:30:30 proportions preserved; fully documented still lands exactly on 100). A/B: the perverse gap halves (6.74 → 3.27 pts on the probe SBOM); the quality gradient survives (bare 40 < partial < full 100). The residual gap is deliberate: full parity would require rewarding disclosure above the SBOM's own average. Freshness made scores non-deterministic: - is_fresh (created < 90 days, live wall clock) carried 12 points of the provenance checklist, so identical SBOM bytes scored differently across midnight, and a future-dated document always counted fresh. Freshness is now display-only metadata: the checklist item is removed (denominator 105 → 93 for CycloneDX), making the quality score a pure function of the document — the same determinism contract as the parser content-hash work. Future-dated documents no longer read as fresh. NOTE: fresh documents lose a little provenance ((S+12)/105 → S/93), stale and timestamp-less ones gain — that redistribution is the point. The TUI score explanation no longer cites freshness, and the two quality snapshots move by exactly the renormalization (9→10 / 10%→11%). Cyclic graphs absorbed their own penalty: - Dependency coverage uses an N/(N-1) denominator, so a fully-cyclic graph reached ~125% raw coverage and swallowed the cycle penalty before the final clamp (ring graph scored a perfect 100). Coverage now clamps to 100 before penalties: the same ring scores 95 with its cycle visible; acyclic graphs are bit-identical. Wrong-field recommendation + hardcoded flag: - The "Add VCS URLs" recommendation derived missing-VCS from HASH coverage: it stayed silent for SBOMs with no VCS refs (all hashed) and fired for fully-VCS'd SBOMs missing hashes. AuditabilityMetrics is now plumbed into generate_recommendations and the count uses components_with_vcs. - CompletenessMetrics.has_timestamp was hardcoded true; it now reports has_known_timestamp() (score-neutral; TUI checklist and reports only). Verified: score A/B vs the metric-counting commit (all four fixes move by the measured amounts; a well-formed SBOM's full report JSON differs in exactly two fields — the provenance renormalization) and an adversarial audit (baseline-40 coefficients binary-exact at 100; no consumer special-cases score 0; the one-fake-CVE gaming vector is bounded and smaller than the pre-existing fully-documented-CVE one; both scorer paths checked — score_ai_readiness builds its own recommendations). 4 new tests, one prior test corrected (its "fresh" fixture was year-3999, silently relying on the future-dating bug). Full suite green (1660), clippy -D warnings and fmt clean. Co-Authored-By: Claude Fable 5 --- src/quality/metrics.rs | 170 +++++++++++++++--- src/quality/scorer.rs | 9 +- src/tui/shared/quality.rs | 7 +- ...r_snapshot_tests__diff_quality_120x40.snap | 2 +- ...r_snapshot_tests__view_quality_120x40.snap | 2 +- 5 files changed, 161 insertions(+), 29 deletions(-) diff --git a/src/quality/metrics.rs b/src/quality/metrics.rs index c8265612..56fbb2e6 100644 --- a/src/quality/metrics.rs +++ b/src/quality/metrics.rs @@ -104,7 +104,9 @@ impl CompletenessMetrics { components_with_licenses: pct(with_licenses), components_with_description: pct(with_description), has_creator_info: !sbom.document.creators.is_empty(), - has_timestamp: true, // Always set in our model + // A missing/invalid source timestamp is stored as the epoch + // sentinel — report it as absent, not hardcoded true. + has_timestamp: sbom.document.has_known_timestamp(), has_serial_number: sbom.document.serial_number.is_some(), total_components: total, } @@ -726,6 +728,13 @@ impl VulnerabilityMetrics { /// category should be excluded from the weighted score (N/A-aware). /// This prevents inflating the overall score when vulnerability assessment /// was not performed. + /// + /// Disclosing vulnerabilities at all earns a 40-point baseline; the + /// remaining 60 points reward per-vulnerability documentation quality + /// (CVSS 24, CWE 18, remediation 18). Without the baseline, a bare + /// disclosure scored 0 — LOWER than saying nothing (N/A redistributes the + /// category weight), which punished transparency: an author was better + /// off stripping vulnerability data than disclosing it undocumented. #[must_use] pub fn documentation_score(&self) -> Option { if self.total_vulnerabilities == 0 { @@ -736,11 +745,8 @@ impl VulnerabilityMetrics { let cwe_ratio = self.with_cwe as f32 / self.total_vulnerabilities as f32; let remediation_ratio = self.with_remediation as f32 / self.total_vulnerabilities as f32; - Some( - remediation_ratio - .mul_add(30.0, cvss_ratio.mul_add(40.0, cwe_ratio * 30.0)) - .min(100.0), - ) + let quality = remediation_ratio.mul_add(18.0, cvss_ratio.mul_add(24.0, cwe_ratio * 18.0)); + Some((40.0 + quality).min(100.0)) } } @@ -949,9 +955,12 @@ impl DependencyMetrics { return 0.0; } - // Score based on how many components have dependency info + // Score based on how many components have dependency info. Clamp to + // 100 BEFORE subtracting penalties: with an N/(N-1) denominator a + // fully-cyclic graph reaches ~125% coverage, which silently absorbed + // the cycle/orphan penalties below. let coverage = if total_components > 1 { - (self.components_with_deps as f32 / (total_components - 1) as f32) * 100.0 + ((self.components_with_deps as f32 / (total_components - 1) as f32) * 100.0).min(100.0) } else { 100.0 // Single component SBOM }; @@ -1306,7 +1315,9 @@ impl ProvenanceMetrics { let has_contact_email = doc.creators.iter().any(|c| c.email.is_some()); let timestamp_known = doc.has_known_timestamp(); - let age_days = (chrono::Utc::now() - doc.created).num_days().max(0) as u32; + // Signed age: negative means the document is dated in the future. + let age_days_signed = (chrono::Utc::now() - doc.created).num_days(); + let age_days = age_days_signed.max(0) as u32; Self { has_tool_creator, @@ -1319,10 +1330,12 @@ impl ProvenanceMetrics { // consumers gate the display on timestamp_known. timestamp_age_days: if timestamp_known { age_days } else { 0 }, timestamp_known, - // A missing timestamp is not fresh — this is the correctness fix: - // the old Utc::now() fallback silently granted freshness credit to - // every timestamp-less document. - is_fresh: timestamp_known && age_days < FRESHNESS_THRESHOLD_DAYS, + // A missing timestamp is not fresh, and neither is a FUTURE-dated + // document (negative signed age): a bogus forward date must not + // read as "recently generated". Display-only — freshness is + // deliberately NOT part of quality_score (see below). + is_fresh: timestamp_known + && (0..i64::from(FRESHNESS_THRESHOLD_DAYS)).contains(&age_days_signed), has_primary_component: sbom.primary_component_id.is_some(), lifecycle_phase: doc.lifecycle_phase.clone(), completeness_declaration: doc.completeness_declaration.clone(), @@ -1335,9 +1348,14 @@ impl ProvenanceMetrics { /// Calculate provenance quality score (0-100) /// /// Weighted checklist: tool creator (15%), tool version (5%), org creator (12%), - /// contact email (8%), serial number (8%), document name (5%), freshness (12%), + /// contact email (8%), serial number (8%), document name (5%), /// primary component (12%), completeness declaration (8%), signature (5%), /// lifecycle phase (10% CDX-only). + /// + /// Freshness (`is_fresh`) is deliberately NOT scored: it is computed from + /// the live wall clock, so identical SBOM bytes would score differently + /// across days (and flip at midnight). It remains available as display + /// metadata; the score itself is a pure function of the document. #[must_use] pub fn quality_score(&self, is_cyclonedx: bool) -> f32 { let mut score = 0.0; @@ -1353,7 +1371,6 @@ impl ProvenanceMetrics { (self.has_contact_email, 8.0), (self.has_serial_number, 8.0), (self.has_document_name, 5.0), - (self.is_fresh, 12.0), (self.has_primary_component, 12.0), (completeness_declared, 8.0), (self.has_signature, 5.0), @@ -2235,6 +2252,106 @@ mod tests { ); } + /// Disclosing a bare vulnerability (no CVSS/CWE/remediation) earns the + /// 40-point disclosure baseline, not 0 — scoring 0 made an SBOM better + /// off stripping vulnerability data than disclosing it (non-monotonic). + #[test] + fn bare_vuln_disclosure_earns_baseline_credit() { + use crate::model::{Component, NormalizedSbom, VulnerabilityRef, VulnerabilitySource}; + let mut sbom = NormalizedSbom::default(); + let mut c = Component::new("app".to_string(), "app@1".to_string()); + c.vulnerabilities.push(VulnerabilityRef::new( + "CVE-2024-0001".to_string(), + VulnerabilitySource::Osv, + )); + sbom.add_component(c); + + let vm = VulnerabilityMetrics::from_sbom(&sbom); + let score = vm.documentation_score().expect("vuln data present"); + assert!( + (score - 40.0).abs() < 0.01, + "bare disclosure must earn the 40-pt baseline, got {score}" + ); + + // No vulnerability data at all stays N/A (None), not 40. + let empty = NormalizedSbom::default(); + assert!( + VulnerabilityMetrics::from_sbom(&empty) + .documentation_score() + .is_none() + ); + } + + /// The provenance score must be a pure function of the document — no + /// wall-clock term. Freshness is display-only metadata. + #[test] + fn provenance_score_has_no_wall_clock_term() { + let fresh = ProvenanceMetrics { + is_fresh: true, + ..base_provenance() + }; + let stale = ProvenanceMetrics { + is_fresh: false, + ..base_provenance() + }; + assert!( + (fresh.quality_score(true) - stale.quality_score(true)).abs() < f32::EPSILON, + "is_fresh must not affect the score" + ); + } + + fn base_provenance() -> ProvenanceMetrics { + ProvenanceMetrics { + has_tool_creator: true, + has_tool_version: false, + has_org_creator: false, + has_contact_email: false, + has_serial_number: false, + has_document_name: false, + timestamp_age_days: 0, + timestamp_known: true, + is_fresh: false, + has_primary_component: false, + lifecycle_phase: None, + completeness_declaration: CompletenessDeclaration::Unknown, + has_signature: false, + has_citations: false, + citations_count: 0, + } + } + + /// A fully-cyclic dependency graph reaches >100% raw coverage via the + /// N/(N-1) denominator; the clamp must land BEFORE the penalties so the + /// cycle penalty is not silently absorbed. + #[test] + fn cyclic_graph_coverage_does_not_absorb_cycle_penalty() { + use crate::model::{Component, DependencyEdge, DependencyType, NormalizedSbom}; + let mut sbom = NormalizedSbom::default(); + let n = 5; + let mut ids = Vec::new(); + for i in 0..n { + let c = Component::new(format!("c{i}"), format!("c{i}@1")); + ids.push(c.canonical_id.clone()); + sbom.add_component(c); + } + // Ring: c0→c1→...→c4→c0 — every node has an outgoing edge. + for i in 0..n { + sbom.add_edge(DependencyEdge::new( + ids[i].clone(), + ids[(i + 1) % n].clone(), + DependencyType::DependsOn, + )); + } + let dm = DependencyMetrics::from_sbom(&sbom); + assert!(dm.cycle_count >= 1, "ring must be detected as a cycle"); + let score = dm.quality_score(n); + assert!( + score <= 95.0, + "cycle penalty must survive the clamp (raw coverage 125% \ + previously absorbed it), got {score}" + ); + } + #[test] fn test_purl_validation() { assert!(is_valid_purl("pkg:npm/@scope/name@1.0.0")); @@ -2807,12 +2924,25 @@ mod tests { "unknown age must not leak a huge number" ); - // A timestamped document stays fresh. - let cdx_ts = r#"{"bomFormat":"CycloneDX","specVersion":"1.5", - "metadata":{"timestamp":"3999-01-01T00:00:00Z"}, - "components":[{"type":"library","name":"a","version":"1.0"}]}"#; - let sbom_ts = crate::parsers::parse_sbom_str(cdx_ts).expect("parse"); + // A recently-timestamped document is fresh. + let now = chrono::Utc::now().to_rfc3339(); + let cdx_ts = format!( + r#"{{"bomFormat":"CycloneDX","specVersion":"1.5", + "metadata":{{"timestamp":"{now}"}}, + "components":[{{"type":"library","name":"a","version":"1.0"}}]}}"# + ); + let sbom_ts = crate::parsers::parse_sbom_str(&cdx_ts).expect("parse"); let prov_ts = ProvenanceMetrics::from_sbom(&sbom_ts); assert!(prov_ts.timestamp_known && prov_ts.is_fresh); + + // A FUTURE-dated document is known but must NOT read as fresh — + // a bogus forward date is not "recently generated". + let cdx_future = r#"{"bomFormat":"CycloneDX","specVersion":"1.5", + "metadata":{"timestamp":"3999-01-01T00:00:00Z"}, + "components":[{"type":"library","name":"a","version":"1.0"}]}"#; + let sbom_future = crate::parsers::parse_sbom_str(cdx_future).expect("parse"); + let prov_future = ProvenanceMetrics::from_sbom(&sbom_future); + assert!(prov_future.timestamp_known); + assert!(!prov_future.is_fresh, "future-dated must not be fresh"); } } diff --git a/src/quality/scorer.rs b/src/quality/scorer.rs index 8b76b6d8..1cbfeaf2 100644 --- a/src/quality/scorer.rs +++ b/src/quality/scorer.rs @@ -639,6 +639,7 @@ impl QualityScorer { &hash_quality_metrics, &provenance_metrics, &lifecycle_metrics, + &auditability_metrics, &compliance, total_components, ); @@ -1014,6 +1015,7 @@ impl QualityScorer { hashes: &HashQualityMetrics, provenance: &ProvenanceMetrics, lifecycle: &LifecycleMetrics, + auditability: &AuditabilityMetrics, compliance: &ComplianceResult, total_components: usize, ) -> Vec { @@ -1175,11 +1177,10 @@ impl QualityScorer { }); } - // Priority 3: VCS URL coverage + // Priority 3: VCS URL coverage — derived from the actual VCS + // reference count, not hash coverage (an unrelated field). if total_components > 0 { - let missing_vcs = total_components.saturating_sub( - ((completeness.components_with_hashes / 100.0) * total_components as f32) as usize, - ); + let missing_vcs = total_components.saturating_sub(auditability.components_with_vcs); if missing_vcs > total_components / 2 { recommendations.push(Recommendation { priority: 3, diff --git a/src/tui/shared/quality.rs b/src/tui/shared/quality.rs index 3d6749ae..3b7466ca 100644 --- a/src/tui/shared/quality.rs +++ b/src/tui/shared/quality.rs @@ -122,13 +122,14 @@ pub fn explain_integrity_score(report: &QualityReport) -> String { } pub fn explain_provenance_score(report: &QualityReport) -> String { + // Freshness is display-only metadata and deliberately NOT part of the + // provenance score, so the score explanation must not cite it — a + // stale-but-complete document scores full provenance. let m = &report.provenance_metrics; - if m.has_tool_creator && m.has_org_creator && m.is_fresh { + if m.has_tool_creator && m.has_org_creator { "Good provenance".to_string() } else if !m.has_tool_creator { "No tool info".to_string() - } else if !m.is_fresh { - "Stale SBOM".to_string() } else { "Partial provenance".to_string() } diff --git a/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_quality_120x40.snap b/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_quality_120x40.snap index f7262c73..0635ad9d 100644 --- a/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_quality_120x40.snap +++ b/src/tui/ui/snapshots/sbom_tools__tui__ui__render_snapshot_tests__diff_quality_120x40.snap @@ -17,7 +17,7 @@ sbom-tools DIFF │ acme-webapp@1.0.0 ⟷ acme-webapp@2.0.0 │ Identifiers █████████████████░░░ 89% 89% ×20% │ │ Licenses ████████████████░░░░ 83% 83% ×12% │ │ Completeness ██████████░░░░░░░░░░ 51% 51% ×25% │ -│ Provenance █░░░░░░░░░░░░░░░░░░░ 10% 10% ×10% ⚠low │ +│ Provenance ██░░░░░░░░░░░░░░░░░░ 11% 11% ×10% ⚠low │ │ Dependencies ░░░░░░░░░░░░░░░░░░░░ 0% 0% ×10% ✗ │ │ Integrity ░░░░░░░░░░░░░░░░░░░░ 0% 0% ×8% ✗ │ │ │ diff --git a/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_quality_120x40.snap b/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_quality_120x40.snap index 20ea7093..aba5c691 100644 --- a/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_quality_120x40.snap +++ b/src/tui/view/ui/snapshots/sbom_tools__tui__view__ui__render_snapshot_tests__view_quality_120x40.snap @@ -22,7 +22,7 @@ sbom-tools VIEW │ acme-webapp │ CycloneDX 1.5 │ [SBOM] │▇▇▇▇▇▇ ██████ ██████ ││ Versions 100% ██████████████████████████████ │ │██████ ██████ ██████ ││ PURLs 89% ███████████████████████████░░░ │ │██████ ██████ ██████ ││ Licenses 89% ███████████████████████████░░░ │ -│██50██ ██88██ ██83██ N/A 0 0 ▅▅9▅▅▅ N/A ││ Suppliers 0% ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ │ +│██50██ ██88██ ██83██ N/A 0 0 ▆▆10▆▆ N/A ││ Suppliers 0% ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ │ │ Cmpl IDs Lic VDoc Deps Hash Prov Life ││ Hashes 0% ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ │ └────────────────────────────────────────────────────────────────┘└────────────────────────────────────────────────────┘ ┌ Top Recommendations (7) [↑↓ select, Enter→detail] ───────────────────────────────────────────────────────────────────┐