From 19addcdb5697f47e47da7d028a0177b92a801a98 Mon Sep 17 00:00:00 2001 From: Adam Date: Sat, 1 Aug 2026 12:32:15 -0400 Subject: [PATCH 1/2] test(sccm): prove spine accepts overlapping ranges `validate_all_evidence_references` keys on (artifact_id, entry_id) and only rejects one identity carrying two ranges. Two references over the same physical lines under different entry ids pass, so one record can be cited twice and `compare_evidence_refs` picks between them arbitrarily. Adds the failing contract across all three citation surfaces plus the serde boundaries, and pins the shapes that must keep validating: adjacent spans, equal spans in different artifacts, and references that assert no extent at all. The `OverlappingEvidenceReference` variant lands inert here so the contract compiles; nothing reads it yet. Refs #418 Co-Authored-By: Claude Fable 5 --- .../cmtraceopen-parser/src/sccm/findings.rs | 1 + .../tests/sccm_spine_contract.rs | 217 ++++++++++++++++++ 2 files changed, 218 insertions(+) diff --git a/crates/cmtraceopen-parser/src/sccm/findings.rs b/crates/cmtraceopen-parser/src/sccm/findings.rs index 3e4fb3284..43a42db70 100644 --- a/crates/cmtraceopen-parser/src/sccm/findings.rs +++ b/crates/cmtraceopen-parser/src/sccm/findings.rs @@ -457,6 +457,7 @@ pub enum SccmFindingValidationError { InvalidRole, InvalidEvidenceReference, ConflictingEvidenceReference, + OverlappingEvidenceReference, MissingEvidenceOrCoverageGap, MissingTerminalEvidence, InvalidTerminalEvidence, diff --git a/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs b/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs index d8b6f7cef..75cbd68b6 100644 --- a/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs +++ b/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs @@ -1250,6 +1250,223 @@ fn finding_serialization_prioritizes_conflicting_evidence_identity_ranges() { assert!(error.contains("ConflictingEvidenceReference"), "{error}"); } +/// Builds one finding per citation surface, carrying `second` on that surface +/// only, so a validator that scans a single surface cannot pass by accident. +fn evidence_surface_findings( + label: &str, + first: &SccmEvidenceRef, + second: &SccmEvidenceRef, +) -> Vec<(String, Result)> { + fn scaffold(finding_id: String) -> SccmFindingBuilder { + SccmFindingBuilder::new(finding_id) + .class(SccmFindingClass::Symptom) + .phase(SccmPhase::Policy) + .role(SccmRole::Client) + .severity(Severity::Warning) + .confidence(SccmConfidence::Low) + } + + vec![ + ( + format!("{label}/top-level"), + scaffold(format!("{label}-top-level")) + .evidence(vec![first.clone(), second.clone()]) + .build(), + ), + ( + format!("{label}/terminal"), + scaffold(format!("{label}-terminal")) + .evidence(vec![first.clone()]) + .terminal_evidence(vec![SccmTerminalEvidence::observed_failure(second.clone())]) + .build(), + ), + ( + format!("{label}/correlation-key"), + scaffold(format!("{label}-correlation-key")) + .evidence(vec![first.clone()]) + .correlation_keys(vec![finding_key( + SccmCorrelationKeyKind::AssignmentId, + "{ABCDEFAB-0000-0000-0000-000000000001}", + "abcdefab-0000-0000-0000-000000000001", + SccmKeyConfidence::Low, + Some("sccm-keys-experimental-v1"), + second.clone(), + )]) + .build(), + ), + ] +} + +#[test] +fn finding_rejects_overlapping_ranges_across_distinct_evidence_identities() { + let first = SccmEvidenceRef { + artifact_id: "artifact-a".into(), + entry_id: "entry-a".into(), + line_start: Some(4), + line_end: Some(6), + }; + // Every case claims at least one physical line of `first` under a second + // entry id, so the two references cannot both be the record they claim. + let cases = [ + ("identical-span", Some(4), Some(6)), + ("shared-start-line", Some(1), Some(4)), + ("shared-end-line", Some(6), Some(9)), + ("contained-span", Some(5), Some(5)), + ("containing-span", Some(1), Some(9)), + ("straddling-span", Some(5), Some(9)), + ]; + + for (label, line_start, line_end) in cases { + let second = SccmEvidenceRef { + entry_id: "entry-b".into(), + line_start, + line_end, + ..first.clone() + }; + for (surface, result) in evidence_surface_findings(label, &first, &second) { + assert_eq!( + result.unwrap_err(), + SccmFindingValidationError::OverlappingEvidenceReference, + "{surface}" + ); + } + } +} + +#[test] +fn finding_overlap_rejection_survives_evidence_serde_round_trips() { + let first = SccmEvidenceRef { + artifact_id: "artifact-a".into(), + entry_id: "entry-a".into(), + line_start: Some(4), + line_end: Some(6), + }; + let mut finding = SccmFindingBuilder::new("overlapping-serde-ranges") + .class(SccmFindingClass::Symptom) + .phase(SccmPhase::Policy) + .role(SccmRole::Client) + .severity(Severity::Warning) + .confidence(SccmConfidence::Low) + .evidence(vec![ + first.clone(), + SccmEvidenceRef { + entry_id: "entry-b".into(), + line_start: Some(7), + line_end: Some(9), + ..first + }, + ]) + .build() + .unwrap(); + + let mut json = serde_json::to_value(&finding).unwrap(); + json["evidence"][1]["lineStart"] = serde_json::json!(6); + let error = serde_json::from_value::(json) + .unwrap_err() + .to_string(); + assert!(error.contains("OverlappingEvidenceReference"), "{error}"); + + finding.evidence[1].line_start = Some(6); + assert_eq!( + finding.validate().unwrap_err(), + SccmFindingValidationError::OverlappingEvidenceReference + ); + let error = serde_json::to_string(&finding).unwrap_err().to_string(); + assert!(error.contains("OverlappingEvidenceReference"), "{error}"); +} + +#[test] +fn finding_accepts_disjoint_and_unbounded_evidence_ranges() { + let anchor = SccmEvidenceRef { + artifact_id: "artifact-a".into(), + entry_id: "entry-a".into(), + line_start: Some(4), + line_end: Some(6), + }; + // Overlap is a claim about physical extent within one artifact. Adjacent + // spans, other artifacts, and references that assert no extent at all are + // all citations nine lanes already emit, and must keep validating. + let cases = [ + ( + "adjacent-below", + SccmEvidenceRef { + entry_id: "entry-b".into(), + line_start: Some(1), + line_end: Some(3), + ..anchor.clone() + }, + ), + ( + "adjacent-above", + SccmEvidenceRef { + entry_id: "entry-b".into(), + line_start: Some(7), + line_end: Some(9), + ..anchor.clone() + }, + ), + ( + "same-span-other-artifact", + SccmEvidenceRef { + artifact_id: "artifact-b".into(), + entry_id: "entry-b".into(), + ..anchor.clone() + }, + ), + ( + "unbounded-second", + SccmEvidenceRef { + entry_id: "entry-b".into(), + line_start: None, + line_end: None, + ..anchor.clone() + }, + ), + ]; + + for (label, second) in cases { + let finding = SccmFindingBuilder::new(format!("disjoint-{label}")) + .class(SccmFindingClass::Symptom) + .phase(SccmPhase::Policy) + .role(SccmRole::Client) + .severity(Severity::Warning) + .confidence(SccmConfidence::Low) + .evidence(vec![anchor.clone(), second]) + .build() + .unwrap_or_else(|error| panic!("{label}: {error:?}")); + let json = serde_json::to_value(&finding).unwrap(); + assert_eq!( + serde_json::from_value::(json).unwrap(), + finding, + "{label}" + ); + } + + // Two references that both assert no extent stay indistinguishable by span + // and must not be treated as claiming the same lines. + let unbounded = SccmEvidenceRef { + artifact_id: "artifact-a".into(), + entry_id: "entry-a".into(), + line_start: None, + line_end: None, + }; + SccmFindingBuilder::new("disjoint-both-unbounded") + .class(SccmFindingClass::Symptom) + .phase(SccmPhase::Policy) + .role(SccmRole::Client) + .severity(Severity::Warning) + .confidence(SccmConfidence::Low) + .evidence(vec![ + unbounded.clone(), + SccmEvidenceRef { + entry_id: "entry-b".into(), + ..unbounded + }, + ]) + .build() + .expect("unbounded references assert no physical extent"); +} + #[test] fn finding_rejects_top_level_evidence_identity_whitespace_aliases() { assert_evidence_alias_is_rejected(FindingEvidenceAliasSurface::TopLevel); From 8b487a3586ad6cccfe2a9df4b274451bcebc31b0 Mon Sep 17 00:00:00 2001 From: Adam Date: Sat, 1 Aug 2026 12:39:19 -0400 Subject: [PATCH 2/2] fix(sccm): reject overlapping evidence ranges in spine Two references over the same physical lines under different entry ids double-count one record and leave `compare_evidence_refs` ranking them on span width alone. Identity equality never saw it, so each reducer had to grow its own guard. The overlap predicate now lives beside the validator every finding already passes through, and `validate_all_evidence_references` sorts each artifact's spans and sweeps them, so the rule holds no matter which reducer, or none, assembled the finding. Semantics are the ones both existing copies already agreed on: inclusive bounds, so equal spans overlap and abutting spans do not; scoped to one artifact; and a reference carrying no bounds asserts no extent and can overlap nothing. An inverted range reads as empty under an inclusive test, so validity is checked first: every reference clears `validate_evidence_reference` before any pair reaches the predicate, and `InvalidEvidenceReference` still wins over the new rejection. The management-point copy is deleted in favour of the spine predicate. The bodies were byte-identical, so admission there is unchanged; its inverted-range handling in particular is preserved, because the reducer compares candidates that have not passed its own safety gate. `finding_rejects_key_or_terminal_refs_that_are_not_cited` built its uncited reference from a helper that pins every span to line 1, so `policy:2-2` claimed line 1 and contradicted its own entry id. The span is now spelled out. Closes #418 Co-Authored-By: Claude Fable 5 --- .../cmtraceopen-parser/src/sccm/findings.rs | 106 ++++++++++++++++-- .../sccm/server/windows/management_point.rs | 17 +-- .../tests/sccm_spine_contract.rs | 11 +- 3 files changed, 106 insertions(+), 28 deletions(-) diff --git a/crates/cmtraceopen-parser/src/sccm/findings.rs b/crates/cmtraceopen-parser/src/sccm/findings.rs index 43a42db70..d14a2afda 100644 --- a/crates/cmtraceopen-parser/src/sccm/findings.rs +++ b/crates/cmtraceopen-parser/src/sccm/findings.rs @@ -645,11 +645,9 @@ fn validate_roles(finding: &SccmFinding) -> Result<(), SccmFindingValidationErro Ok(()) } -fn validate_all_evidence_references( - finding: &SccmFinding, -) -> Result<(), SccmFindingValidationError> { - let mut ranges_by_identity = BTreeMap::new(); - for reference in finding +/// Every reference a finding cites, across all three citation surfaces. +fn cited_evidence_references(finding: &SccmFinding) -> impl Iterator { + finding .evidence .iter() .chain( @@ -664,17 +662,103 @@ fn validate_all_evidence_references( .iter() .filter_map(|key| key.evidence.as_ref()), ) - { +} + +fn validate_all_evidence_references( + finding: &SccmFinding, +) -> Result<(), SccmFindingValidationError> { + let mut references_by_identity: BTreeMap<(&str, &str), &SccmEvidenceRef> = BTreeMap::new(); + for reference in cited_evidence_references(finding) { validate_evidence_reference(reference)?; - let identity = (reference.artifact_id.as_str(), reference.entry_id.as_str()); - let range = (reference.line_start, reference.line_end); - if ranges_by_identity - .insert(identity, range) - .is_some_and(|existing| existing != range) + if references_by_identity + .insert(evidence_identity(reference), reference) + .is_some_and(|existing| { + (existing.line_start, existing.line_end) + != (reference.line_start, reference.line_end) + }) { return Err(SccmFindingValidationError::ConflictingEvidenceReference); } } + validate_disjoint_evidence_spans(&references_by_identity) +} + +/// Whether two references claim overlapping physical lines of one source. +/// +/// Physical extent, not an identity tuple, is the question. Two logical +/// records of one artifact occupy disjoint lines, so any overlap means at +/// least one of them is not the record it claims to be. Bounds are inclusive, +/// so equal spans are the degenerate overlap and abutting spans (`1-2` beside +/// `3-4`) are not one. A reference that carries no bounds asserts no extent and +/// therefore cannot be shown to claim another reference's lines. +/// +/// This presumes bounds that already passed a validity gate. An inverted range +/// reads as empty under an inclusive test and would be silently judged disjoint +/// from everything, so the gate has to run first, not after: the spine calls +/// [`validate_evidence_reference`] on every reference before any pair reaches +/// this predicate, and a reducer that hands over unvalidated references is +/// responsible for its own gate. +pub(crate) fn evidence_references_overlap(left: &SccmEvidenceRef, right: &SccmEvidenceRef) -> bool { + if left.artifact_id != right.artifact_id { + return false; + } + matches!( + ( + left.line_start, + left.line_end, + right.line_start, + right.line_end, + ), + (Some(left_start), Some(left_end), Some(right_start), Some(right_end)) + if left_start <= right_end && right_start <= left_end + ) +} + +/// Rejects a finding whose citations claim the same physical records twice. +/// +/// Identity equality only catches an exact repeat of one range. It leaves the +/// wider hole open: `1-2` and `1-1` are different entry ids over the same +/// physical line, so both survive, each cites the same record, and +/// [`compare_evidence_refs`] then ranks one above the other on nothing but +/// span width. Both the management-point and client-policy reducers had to +/// close this in their own code; enforcing it here closes it for every finding +/// regardless of which reducer, or none, assembled it. +/// +/// References arrive keyed by identity, so each entry id contributes exactly +/// one span and a repeated identity is already a +/// [`SccmFindingValidationError::ConflictingEvidenceReference`]. Sorting each +/// artifact's spans by start line lets one pass answer the question: a span +/// that clears the widest span seen so far clears every earlier one, because +/// no earlier span starts later or ends further. +fn validate_disjoint_evidence_spans( + references_by_identity: &BTreeMap<(&str, &str), &SccmEvidenceRef>, +) -> Result<(), SccmFindingValidationError> { + let mut by_artifact: BTreeMap<&str, Vec<&SccmEvidenceRef>> = BTreeMap::new(); + for reference in references_by_identity.values() { + if reference.line_start.is_some() && reference.line_end.is_some() { + by_artifact + .entry(reference.artifact_id.as_str()) + .or_default() + .push(reference); + } + } + + for mut spans in by_artifact.into_values() { + spans.sort_by(|left, right| { + left.line_start + .cmp(&right.line_start) + .then_with(|| left.line_end.cmp(&right.line_end)) + }); + let mut widest: Option<&SccmEvidenceRef> = None; + for span in spans { + if widest.is_some_and(|widest| evidence_references_overlap(widest, span)) { + return Err(SccmFindingValidationError::OverlappingEvidenceReference); + } + if widest.is_none_or(|widest| widest.line_end < span.line_end) { + widest = Some(span); + } + } + } Ok(()) } diff --git a/crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs b/crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs index b22bc08ac..2eb312dc7 100644 --- a/crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs +++ b/crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs @@ -15,6 +15,7 @@ use std::collections::{BTreeMap, BTreeSet}; use serde::Serialize; use crate::models::log_entry::Severity; +use crate::sccm::findings::evidence_references_overlap; use crate::sccm::{ classify_artifact_name, SccmArtifact, SccmArtifactFamily, SccmArtifactRequest, SccmConfidence, SccmCoverageState, SccmEvidence, SccmEvidenceRef, SccmFinding, SccmFindingBuilder, @@ -1127,22 +1128,6 @@ fn evidence_identity_is_unique( == 1 } -fn evidence_references_overlap(left: &SccmEvidenceRef, right: &SccmEvidenceRef) -> bool { - if left.artifact_id != right.artifact_id { - return false; - } - matches!( - ( - left.line_start, - left.line_end, - right.line_start, - right.line_end, - ), - (Some(left_start), Some(left_end), Some(right_start), Some(right_end)) - if left_start <= right_end && right_start <= left_end - ) -} - fn safe_evidence_reference(reference: &SccmEvidenceRef) -> bool { safe_opaque_id(&reference.artifact_id) && safe_opaque_id(&reference.entry_id) diff --git a/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs b/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs index 75cbd68b6..5320aa881 100644 --- a/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs +++ b/crates/cmtraceopen-parser/tests/sccm_spine_contract.rs @@ -908,7 +908,16 @@ fn finding_unregistered_strong_or_exact_key_profiles_are_rejected() { #[test] fn finding_rejects_key_or_terminal_refs_that_are_not_cited() { let cited = finding_evidence_ref("client-policy-agent", "policy:1-1"); - let missing = finding_evidence_ref("client-policy-agent", "policy:2-2"); + // `finding_evidence_ref` pins every span to line 1, so this reference used + // to claim line 1 while calling itself `policy:2-2`. Two entry ids over one + // physical line is the shape this suite now rejects outright, and it would + // mask the uncited-reference rule under test. The span is spelled out to + // match the entry id it advertises. + let missing = SccmEvidenceRef { + line_start: Some(2), + line_end: Some(2), + ..finding_evidence_ref("client-policy-agent", "policy:2-2") + }; let key_result = SccmFindingBuilder::new("uncited-key") .class(SccmFindingClass::Symptom)