fix(sccm): bound opaque ids and harden finding deserialization - #404
Conversation
RED: deserialized findings currently accept arbitrarily long finding ids, evidence artifact and entry ids, titles, summaries, correlation-key raw and normalized values, and extraction profile ids. Only the coverage-gap artifact id is bounded today. Refs #317
is_canonical_opaque_id checked only non-empty and trimmed, so finding ids, evidence artifact and entry ids, artifact request logical ids, and extraction profile ids were unbounded on the wire. Only the coverage-gap artifact id carried a length bound. Rename the constant to MAX_SCCM_OPAQUE_ID_CHARS and enforce it inside is_canonical_opaque_id so no identifier path can skip it, then drop the now-redundant explicit check in validate_coverage_gaps. Bound title, summary, and correlation-key raw and normalized values, following the existing request-reason precedent of trimming first and counting chars, not bytes. Fail-closed tightening: previously accepted values stay accepted. Refs #317
RED: SccmFindingCoverageGap and SccmArtifactRequest are public and derive Deserialize directly, so they accept unknown fields and skip every validation SccmFinding applies to the same payloads. Captured coverage, empty, untrimmed, overlong, and undeclared ids, and unbounded reasons all deserialize today. Refs #317
SccmFindingCoverageGap and SccmArtifactRequest derived Deserialize directly, so standalone payloads bypassed deny_unknown_fields and every check SccmFinding applies to the same values. Replace the derive with manual impls over the existing wire structs and validators, matching how SccmFinding::deserialize already works. Both types still implement Deserialize, so this is source compatible for consumers; only payloads the spine already considered invalid stop deserializing. Nothing outside the finding wire path deserializes either type: SccmServerIntakeAssessment is Serialize only and no caller names them in a Deserialize position. Refs #317
|
@coderabbitai full review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds shared SCCM length limits, validates nested finding types during serialization and deserialization, validates evidence references before citation checks, and records oversized correlation-key candidates as malformed extraction gaps. Contract tests cover boundary, wire-format, nested-validation, and extraction behavior. ChangesSCCM validation hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WirePayload
participant SccmDeserializer
participant SccmValidator
WirePayload->>SccmDeserializer: deserialize nested wire type
SccmDeserializer->>SccmValidator: validate references, bounds, roles, states, and confidence
SccmValidator-->>SccmDeserializer: validated SCCM model or error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
github-actions
Bot
added
bug
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 54 minutes. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Exact-head review of f4d30bc. Worktree detached at f4d30bc, clean before and
after; all probes removed.
VERDICT: BLOCK on one item. The change is correct, non-regressive, and every
claim in the PR body is verified true. The block is scope completion on the
shared spine, not a correctness defect.
BATTERY
- cargo test --locked -p cmtraceopen-parser --test sccm_spine_contract:
142 passed, 0 failed. - cargo test --locked -p cmtraceopen-parser: 918 passed, 0 failed,
14 test binaries plus doctests. - cargo test --locked --workspace: 1672 passed, 0 failed, 30 test binaries.
- cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings: clean.
- cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown: clean.
- rustfmt --check on the two changed files only: clean.
- git diff --check 7655826 HEAD: clean.
All 12 SCCM contract binaries green, 344 tests: client deployment 8,
client health 3, client intake 3, client inventory/compliance/metering 47,
server distribution point 48, server hierarchy and replication 28,
server intake 11, server intake fixture 1, server provider and admin service 30,
server software update point 18, site core 5, spine 142.
RED DISCIPLINE, BOTH PAIRS
- RED 10616a0: 137 passed, 3 failed. Reported mismatches included
"builder accepted an overlong finding ID", "builder accepted an overlong
evidence artifact ID", "serializer accepted an overlong finding ID", and
"deserializer accepted an overlong evidence artifactId". - GREEN ba95b4d: 140 passed, 0 failed.
- RED ae62275: 140 passed, 2 failed. Reported mismatches included
"coverage gap deserializer accepted an unknown field", "coverage gap
deserializer accepted captured coverage", and "artifact request deserializer
accepted an undeclared logical ID". - GREEN f4d30bc: 142 passed, 0 failed.
BACKWARD COMPATIBILITY
No merged lane regresses. Longest values measured across all 141 shipped
expected.json plus every fixture JSON in the corpus:
findingId 39 / 256 (15%)
evidence artifactId 67 / 256 (26%)
evidence entryId 46 / 256 (18%)
coverage gap artifactId 67 / 256 (26%)
request logicalId 34 / 256 (13%)
extractionProfileId 35 / 256 (14%)
correlation key value 36 / 256 (14%)
title 53 / 512 (10%)
summary 53 / 2048 (3%)
request reason 122 / 240 (51%, bound unchanged by this PR)
Nothing is within 20% of a new limit. Two structural notes worth recording:
SccmFindingBuilder::new defaults title and summary to finding_id, so the default
path is bounded by the 256 id limit and cannot reach 512 or 2048; and
normalize_server_host self-caps at 253 characters, so the theoretical worst-case
ServerHost correlation key is 254 against a 256 limit, about 99% of the bound.
That is safe today but it is the tightest constraint in the change, and
MAX_SCCM_CORRELATION_KEY_VALUE_CHARS should not be lowered.
BOUND PROBES
Exactly-at-limit accepted and limit+1 rejected on every gated field: finding id,
evidence artifact id, evidence entry id, coverage gap artifact id, extraction
profile id, and correlation key value at 256/257; title at 512/513; summary at
2048/2049. Characters rather than bytes confirmed: 256 four-byte scalars
(1024 bytes) accepted, 257 rejected, and the value round-trips unchanged.
Trim-then-count confirmed: 512 characters wrapped in six spaces accepted and
normalized to 512, 513 wrapped in the same padding rejected, whitespace-only
rejected. Opaque ids treat padding as rejection rather than normalization, which
matches is_canonical_opaque_id. No builder versus deserialize asymmetry was found
on any gated path.
DESERIALIZE ROUTING
Both manual impls behave as claimed. Unknown fields are rejected with the wire
struct's own serde error listing the permitted names. Invalid values are rejected
by the corresponding validator. Valid payloads round-trip for both types
individually and inside a full SccmFinding carrying both. Serialized field names
are unchanged (artifactId, role, coverage; logicalId, role, reason) and
optionality is unchanged: removing a required field still produces
"missing field". Independent greps confirm no from_value, from_str, from_slice,
or from_reader of either type outside the test files, that
SccmServerIntakeAssessment derives Serialize only, and that src-tauri/src has no
SCCM type usage at all (its single "SCCM" hit is a comment string).
EVIDENCE.RS OVERLAP
Confirmed already fixed at the base, not by this PR. git blame attributes the
sort, dedup, and merge loop to 59af7b5, which is outside this PR's four commits,
and the regression test is inside the green 918.
BLOCKING GAP
-
Three more public, re-exported SccmFinding member types still bypass the wire
contract, and their deny_unknown_fields wire structs already exist.Observed at f4d30bc via serde_json::from_value:
- SccmEvidenceRef accepted a 5000-character artifactId, an untrimmed entryId,
and an unknown field. - SccmTerminalEvidence accepted a 5000-character nested artifactId and an
unknown field. - SccmCorrelationKey accepted a 5000-character raw, an incoherent span
(start 99999, end 1), an unknown field, and confidence "exact".
The confidence case is why this is blocking rather than follow-up.
REGISTERED_STABLE_CORRELATION_PROFILE_IDS is deliberately empty, with a comment
stating that adding an entry requires contract review, and
validate_correlation_key_evidence therefore requires every key to be Low.
Standalone deserialization forges past that gate. That is a trust-signal
bypass, materially more consequential than the length bypasses this PR closed,
and it is exactly the class of hole the PR set out to close. Nine lanes stack
on this spine, so closing it after they land is considerably more expensive
than closing it now, and the remedy is the same ten-line pattern already
applied twice in this PR: route through SccmEvidenceRefWire,
SccmTerminalEvidenceWire, and SccmCorrelationKeyWire.To resolve: route those three types, or state on the PR why the spine
deliberately stops at two. - SccmEvidenceRef accepted a 5000-character artifactId, an untrimmed entryId,
NON-BLOCKING OBSERVATIONS
-
Collection cardinality is unbounded. A finding with 50,000 evidence refs and
50,000 coverage gaps built, validated, serialized to roughly 8 MB, and
deserialized cleanly. Only next_artifacts has a cardinality bound, at 16.
The per-element strings are now bounded, but the collections are not. This is
pre-existing and outside the stated thesis; worth a tracking issue. -
The ServerHost correlation key headroom noted above (254 of 256) is the
tightest ratio introduced by this change. Recommend a comment on the constant
so it is not lowered later.
Everything else in this PR is clean. The RED/GREEN discipline is exemplary, the
bounds are placed structurally rather than at call sites, the display-string
handling follows the is_bounded_request_reason precedent exactly, and no
serialized field name or optionality changed.
RED: SccmEvidenceRef, SccmTerminalEvidence, and SccmCorrelationKey are public, re-exported SccmFinding members that still derive Deserialize directly, so standalone payloads bypass deny_unknown_fields and every check the finding applies. Proven accepted today: 5000-char artifact ids, untrimmed and empty entry ids, incoherent line ranges, unknown fields at both the outer and nested level, non-failure terminal kinds, 5000-char key values, an incoherent 99999..1 span, and overlong profile ids. The serious case is confidence forging. Because REGISTERED_STABLE_CORRELATION_PROFILE_IDS is deliberately empty, validate_correlation_key_evidence holds every key at Low, yet a standalone payload deserializes confidence exact or strong and forges a trust signal no registered profile can authorize. Refs #317
SccmEvidenceRef, SccmTerminalEvidence, and SccmCorrelationKey were the remaining public SccmFinding members whose derived Deserialize skipped deny_unknown_fields and every finding-level check. Route all three through their existing wire structs and validators, matching the coverage-gap and artifact-request pattern. Standalone payloads have no surrounding finding, so a terminal evidence and a correlation key stand as their own citation set; that satisfies the citation rule while leaving every other gate in force. The gate that matters is confidence. While REGISTERED_STABLE_CORRELATION_PROFILE_IDS stays empty, no payload can now deserialize a Strong or Exact key and forge corroboration strength that no registered profile authorizes. SccmEvidenceRef and SccmCorrelationKey are declared in models.rs, so their impls live in findings.rs beside the validators they must satisfy. This also tightens SccmEvidence, SccmExtractionGap, and SccmKeyExtractionResult, which embed them; extraction output still round trips because extract_keys already downgrades every emitted key to Low. Also record that MAX_SCCM_CORRELATION_KEY_VALUE_CHARS must not be lowered: a worst-case ServerHost key sits at about 99% of it. Refs #317
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 29 minutes. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the SCCM findings wire contract in cmtraceopen-parser by bounding previously-unbounded identifier/text fields and ensuring public Deserialize entry points route through the same deny_unknown_fields wire structs and validators as SccmFinding.
Changes:
- Centralizes opaque-id length bounds in
is_canonical_opaque_idand adds bounded validation for finding title/summary and correlation-key values. - Replaces derived
Deserializewith manualDeserializeimpls for several public SCCM types to enforce the same wire strictness + validation when deserialized standalone. - Adds contract tests to assert reject/accept behavior across builder, direct validation, serialization, and deserialization boundaries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/sccm/findings.rs | Adds shared bounds + manual Deserialize routing through strict wire structs and validators for standalone deserialization paths. |
| crates/cmtraceopen-parser/src/sccm/models.rs | Removes derived Deserialize for SccmEvidenceRef and SccmCorrelationKey, documenting that deserialization is enforced via findings.rs. |
| crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | Expands contract tests to cover overlong fields, unknown-field rejection, and standalone deserialization strictness. |
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/findings.rs:2262
- The new length bound checks use
chars().count(), which iterates the entire string. For adversarially large JSON strings this turns the validation path into an avoidable O(n) scan per field; switching to an early-exit check keeps the bound fail-fast.
fn is_canonical_opaque_id(value: &str) -> bool {
!value.is_empty() && value.trim() == value && value.chars().count() <= MAX_SCCM_OPAQUE_ID_CHARS
}
Code reviewFive independent reviewers examined this change from different angles (CLAUDE.md compliance, focused bug scan, git-blame history, prior-PR feedback, and code-comment guidance). Each candidate finding was then independently verified and confidence-scored; anything below 80 is reported separately as unconfirmed. 1. SccmCorrelationKey::deserialize leaves its nested evidence ref unvalidated — the exact door the PR set out to close
Fix is one line mirroring findings.rs:354: run validate_evidence_reference over key.evidence before validate_correlation_key_evidence, or retype SccmCorrelationKeyWire.evidence as Option. 2. SccmCorrelationKey::deserialize never validates its nested evidence reference — the one door the PR left open
Also verified: correlation_key_deserializes_through_the_same_wire_contract_as_a_finding (tests/sccm_spine_contract.rs:1863-1939) has no nested-reference case, unlike its terminal-evidence sibling at 1808-1851. 3. New standalone SccmCorrelationKey deserializer never validates its nested evidence reference
Fix: retype the wire field or add key.evidence.as_ref().map_or(Ok(()), validate_evidence_reference) before validate_correlation_key_evidence. Unconfirmed findings (scored 50-79, verify before acting)
|
SccmCorrelationKeyWire carries evidence as Option<SccmEvidenceRefWire>, so a nested reference never reaches the SccmEvidenceRef deserializer. validate_correlation_key_evidence only checks that the citation set contains the reference, and the key's own reference is that set, so the check is self-satisfying and the reference itself is never validated. Share one noncanonical reference payload list across the standalone evidence-ref door, the terminal-evidence door, and the correlation-key door so no door can be tested against a weaker list than its siblings, and cover the SccmKeyExtractionResult path that nests keys. Also cover the nested role each of the coverage-gap and artifact-request wire doors carries. Refs #317
validate_correlation_key_evidence now validates the reference itself before the containment check, so every door reaches the same bar: the standalone key deserializer, the SccmKeyExtractionResult path that nests keys, SccmFinding::validate, the builder, and the serializer. Placed inside the validator rather than at the deserializer call site because the defect was a call site that omitted the check. A caller cannot omit a check the validator performs. Refs #317
MAX_SCCM_CORRELATION_KEY_VALUE_CHARS is enforced at the validator but not at the producer, so extract_keys can emit a key that fails the crate's own contract and no longer round trips. Covers the raw value crossing the bound, and a KB id whose raw stays inside the bound and only crosses it once normalization prepends "KB", so the producer has to weigh the normalized value too. Also pins the out-of-bound candidate as a recorded gap rather than a silent drop. Refs #317
extract_keys now weighs an out-of-bound value the same way it weighs a value that fails to normalize: the candidate becomes a MalformedCandidate gap instead of a key, so it stays visible and the producer stops emitting keys the crate's own validator rejects. Weighs the normalized value as well as the raw one, since normalize_kb_id prepends "KB" and can push a bounded raw past the bound. MAX_SCCM_CORRELATION_KEY_VALUE_CHARS becomes crate-visible so the producer and the validator share one number rather than two that can drift. It stays out of the public surface: it is an internal agreement, not wire surface. Refs #317
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 6 minutes. |
|
@coderabbitai full review Please review exact head 2fc676b. The three prior actionable threads have been fixed, replied to, and resolved; local CodeRabbit returned zero findings. |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/src/sccm/findings.rs`:
- Around line 215-235: Update SccmArtifactRequest::serialize to invoke the same
validate_artifact_requests validator used by Deserialize before constructing
SccmArtifactRequestSerializeWire. Propagate validation failures through
S::Error, while preserving the existing trimmed reason serialization and wire
representation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccedb243-2ee1-4304-b1ae-49d1655a56ab
📒 Files selected for processing (4)
crates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/keys.rscrates/cmtraceopen-parser/src/sccm/models.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/keys.rs:215
is_bounded_key_valueuseschars().count()to enforce the 256-char bound. For oversized candidate values this walks the entire string, which is avoidable and inconsistent with the fail-fasthas_at_most_chars(..)approach used infindings.rs. Switching to annth(MAX)check prevents scanning very large inputs during key extraction.
fn is_bounded_key_value(key: &SccmCorrelationKey) -> bool {
key.raw.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS
&& key.normalized.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS
}
Exact-head review checkpoint
PR #404 remains draft with CHANGES_REQUESTED by design. The repair is locally green, but it is not merge-ready until #335 resolves the contract and the exact-head review cycle is rerun. |
|
Exact-head repair checkpoint for
Verification on the exact committed head:
Fresh hosted CodeRabbit and Copilot reviews were requested on this exact head. The existing artifact-request thread remains unresolved until the hosted review and required CI are green. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/keys.rs:215
is_bounded_key_valueuseschars().count()for both raw and normalized values. Since the regex patterns can capture arbitrarily longvaluesubstrings (e.g.,[0-9]+[A-Za-z0-9_.-]*), this will fully scan oversized inputs; using the same fail-fastchars().nth(MAX).is_none()pattern as the validator avoids unnecessary work on malicious/degenerate evidence messages.
fn is_bounded_key_value(key: &SccmCorrelationKey) -> bool {
key.raw.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS
&& key.normalized.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS
}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/sccm/keys.rs (1)
209-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared character-bound helper.
findings.rsaddedhas_at_most_chars, which stops aftermaximum + 1characters instead of counting the whole string. This producer-side check counts every character of both values. The two sides now express the same bound through two different mechanisms, and only one of them fails fast.Make
has_at_most_charspub(crate)and call it here, so the producer and the validator share one implementation of the bound.♻️ Proposed refactor
In
crates/cmtraceopen-parser/src/sccm/findings.rs:-fn has_at_most_chars(value: &str, maximum: usize) -> bool { +pub(crate) fn has_at_most_chars(value: &str, maximum: usize) -> bool { value.chars().nth(maximum).is_none() }In
crates/cmtraceopen-parser/src/sccm/keys.rs:-use super::findings::MAX_SCCM_CORRELATION_KEY_VALUE_CHARS; +use super::findings::{MAX_SCCM_CORRELATION_KEY_VALUE_CHARS, has_at_most_chars};fn is_bounded_key_value(key: &SccmCorrelationKey) -> bool { - key.raw.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS - && key.normalized.chars().count() <= MAX_SCCM_CORRELATION_KEY_VALUE_CHARS + has_at_most_chars(&key.raw, MAX_SCCM_CORRELATION_KEY_VALUE_CHARS) + && has_at_most_chars(&key.normalized, MAX_SCCM_CORRELATION_KEY_VALUE_CHARS) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/keys.rs` around lines 209 - 215, Expose the existing has_at_most_chars helper in findings.rs as pub(crate), then update is_bounded_key_value in keys.rs to validate both key.raw and key.normalized through that shared helper with MAX_SCCM_CORRELATION_KEY_VALUE_CHARS. Remove the direct chars().count() checks while preserving the requirement that both values satisfy the bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/keys.rs`:
- Around line 209-215: Expose the existing has_at_most_chars helper in
findings.rs as pub(crate), then update is_bounded_key_value in keys.rs to
validate both key.raw and key.normalized through that shared helper with
MAX_SCCM_CORRELATION_KEY_VALUE_CHARS. Remove the direct chars().count() checks
while preserving the requirement that both values satisfy the bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3723537-ee63-4844-8480-34aca48c97fa
📒 Files selected for processing (4)
crates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/keys.rscrates/cmtraceopen-parser/src/sccm/models.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
|
Review follow-up pushed at exact head The CodeRabbit nitpick and Copilot suppressed observation were valid: producer-side extracted-key bounds used full Verification:
Requesting final hosted CodeRabbit and Copilot reviews on this exact head. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
* test(sccm): prove finding text and id fields are unbounded RED: deserialized findings currently accept arbitrarily long finding ids, evidence artifact and entry ids, titles, summaries, correlation-key raw and normalized values, and extraction profile ids. Only the coverage-gap artifact id is bounded today. Refs #317 * fix(sccm): bound every finding id and display string is_canonical_opaque_id checked only non-empty and trimmed, so finding ids, evidence artifact and entry ids, artifact request logical ids, and extraction profile ids were unbounded on the wire. Only the coverage-gap artifact id carried a length bound. Rename the constant to MAX_SCCM_OPAQUE_ID_CHARS and enforce it inside is_canonical_opaque_id so no identifier path can skip it, then drop the now-redundant explicit check in validate_coverage_gaps. Bound title, summary, and correlation-key raw and normalized values, following the existing request-reason precedent of trimming first and counting chars, not bytes. Fail-closed tightening: previously accepted values stay accepted. Refs #317 * test(sccm): prove gap and request deser skip the wire contract RED: SccmFindingCoverageGap and SccmArtifactRequest are public and derive Deserialize directly, so they accept unknown fields and skip every validation SccmFinding applies to the same payloads. Captured coverage, empty, untrimmed, overlong, and undeclared ids, and unbounded reasons all deserialize today. Refs #317 * fix(sccm): route gap and request deser through the wire contract SccmFindingCoverageGap and SccmArtifactRequest derived Deserialize directly, so standalone payloads bypassed deny_unknown_fields and every check SccmFinding applies to the same values. Replace the derive with manual impls over the existing wire structs and validators, matching how SccmFinding::deserialize already works. Both types still implement Deserialize, so this is source compatible for consumers; only payloads the spine already considered invalid stop deserializing. Nothing outside the finding wire path deserializes either type: SccmServerIntakeAssessment is Serialize only and no caller names them in a Deserialize position. Refs #317 * test(sccm): prove three more public types skip the wire contract RED: SccmEvidenceRef, SccmTerminalEvidence, and SccmCorrelationKey are public, re-exported SccmFinding members that still derive Deserialize directly, so standalone payloads bypass deny_unknown_fields and every check the finding applies. Proven accepted today: 5000-char artifact ids, untrimmed and empty entry ids, incoherent line ranges, unknown fields at both the outer and nested level, non-failure terminal kinds, 5000-char key values, an incoherent 99999..1 span, and overlong profile ids. The serious case is confidence forging. Because REGISTERED_STABLE_CORRELATION_PROFILE_IDS is deliberately empty, validate_correlation_key_evidence holds every key at Low, yet a standalone payload deserializes confidence exact or strong and forges a trust signal no registered profile can authorize. Refs #317 * fix(sccm): close the last three unvalidated deser doors SccmEvidenceRef, SccmTerminalEvidence, and SccmCorrelationKey were the remaining public SccmFinding members whose derived Deserialize skipped deny_unknown_fields and every finding-level check. Route all three through their existing wire structs and validators, matching the coverage-gap and artifact-request pattern. Standalone payloads have no surrounding finding, so a terminal evidence and a correlation key stand as their own citation set; that satisfies the citation rule while leaving every other gate in force. The gate that matters is confidence. While REGISTERED_STABLE_CORRELATION_PROFILE_IDS stays empty, no payload can now deserialize a Strong or Exact key and forge corroboration strength that no registered profile authorizes. SccmEvidenceRef and SccmCorrelationKey are declared in models.rs, so their impls live in findings.rs beside the validators they must satisfy. This also tightens SccmEvidence, SccmExtractionGap, and SccmKeyExtractionResult, which embed them; extraction output still round trips because extract_keys already downgrades every emitted key to Low. Also record that MAX_SCCM_CORRELATION_KEY_VALUE_CHARS must not be lowered: a worst-case ServerHost key sits at about 99% of it. Refs #317 * test(sccm): prove nested key evidence skips the contract SccmCorrelationKeyWire carries evidence as Option<SccmEvidenceRefWire>, so a nested reference never reaches the SccmEvidenceRef deserializer. validate_correlation_key_evidence only checks that the citation set contains the reference, and the key's own reference is that set, so the check is self-satisfying and the reference itself is never validated. Share one noncanonical reference payload list across the standalone evidence-ref door, the terminal-evidence door, and the correlation-key door so no door can be tested against a weaker list than its siblings, and cover the SccmKeyExtractionResult path that nests keys. Also cover the nested role each of the coverage-gap and artifact-request wire doors carries. Refs #317 * fix(sccm): validate nested correlation key evidence validate_correlation_key_evidence now validates the reference itself before the containment check, so every door reaches the same bar: the standalone key deserializer, the SccmKeyExtractionResult path that nests keys, SccmFinding::validate, the builder, and the serializer. Placed inside the validator rather than at the deserializer call site because the defect was a call site that omitted the check. A caller cannot omit a check the validator performs. Refs #317 * test(sccm): prove extract_keys emits rejected keys MAX_SCCM_CORRELATION_KEY_VALUE_CHARS is enforced at the validator but not at the producer, so extract_keys can emit a key that fails the crate's own contract and no longer round trips. Covers the raw value crossing the bound, and a KB id whose raw stays inside the bound and only crosses it once normalization prepends "KB", so the producer has to weigh the normalized value too. Also pins the out-of-bound candidate as a recorded gap rather than a silent drop. Refs #317 * fix(sccm): bound correlation key values at the producer extract_keys now weighs an out-of-bound value the same way it weighs a value that fails to normalize: the candidate becomes a MalformedCandidate gap instead of a key, so it stays visible and the producer stops emitting keys the crate's own validator rejects. Weighs the normalized value as well as the raw one, since normalize_kb_id prepends "KB" and can push a bounded raw past the bound. MAX_SCCM_CORRELATION_KEY_VALUE_CHARS becomes crate-visible so the producer and the validator share one number rather than two that can drift. It stays out of the public surface: it is an internal agreement, not wire surface. Refs #317 * fix(sccm): bound stored finding request text * fix(sccm): validate standalone artifact requests * fix(sccm): validate standalone citation serialization * perf(sccm): fail fast on oversized extracted keys
What this changes
Two foundation-level hardening items in
crates/cmtraceopen-parser/src/sccm/findings.rs, both surfaced by an exact-range CodeRabbit review of the spine as it sits oncodex/parser-family-skeleton. Both are fail-closed tightenings; no public serialized field names change.1. One shared opaque-id bound (RED
10616a0e/ GREENba95b4d8).Before:
validate_coverage_gapsboundedgap.artifact_idto 256 chars, butvalidate_evidence_referenceapplied no length bound at all, andis_canonical_opaque_idchecked only non-empty and trimmed. A deserialized finding could therefore carry arbitrarily longartifactIdandentryIdvalues throughevidence,terminalEvidence, and correlation-key evidence, use them asBTreeMapkeys, and re-serialize them.findingId,title,summary, and the correlation-key raw and normalized fields were likewise unbounded.After: the bound lives inside
is_canonical_opaque_id, which structurally gates finding ids, evidence artifact and entry ids, coverage-gap artifact ids, request logical ids, and extraction profile ids. The correlation-key bound sits insidehas_canonical_value, which every key must pass, so low-confidence keys have no bypass. Title and summary bounds follow the existingis_bounded_request_reasonprecedent exactly: trim first, countchars()rather than bytes.2. Public
Deserializeno longer bypasses the wire contract (REDae622752/ GREENf4d30bc3).Before:
SccmFindingCoverageGapandSccmArtifactRequestare public and re-exported bypub use findings::*, and both derivedDeserializewithoutdeny_unknown_fieldsand without validation, while the parallel wire structs setdeny_unknown_fieldsandSccmFinding::deserializeroutes through them. Any caller deserializing those two types directly got neither strictness nor validation.After: both keep implementing
Deserialize, now via manual impls over the existingdeny_unknown_fieldswire structs plusvalidate_coverage_gaps/validate_artifact_requests. This was chosen over removing the derive because removal is a breaking API change; grep evidence that routing is safe is in the review notes below.Why routing rather than removing
DeserializeNothing outside the finding wire path deserializes these types: no
from_value::<>,from_str::<>, orfrom_slice::<>of either type anywhere in the tree; the onlyDeserialize-deriving structs naming them are the wire structs infindings.rs;SccmServerIntakeAssessmentholdsVec<SccmArtifactRequest>but derivesSerializeonly;src-tauri/src/has zerosccmreferences. The change is source-compatible, and the only payloads that stop deserializing are ones the spine already considered invalid.Verified as already fixed, no change made
The overlapping-identity-range class (a
Users\DOMAIN\alicevalue leaving\alicein public output) is already closed at the spine head by the merged #332 work: the sort, dedup, and merge logic is atevidence.rs:262-271from59af7b5f, andexport_merges_overlapping_identity_ranges_without_mutating_raw_snapshotcovers exactly the standalone-matcher plus user-path-matcher overlap, asserting neitherADMINnorsecretsurvives into exported JSON.Verification
cargo test --locked -p cmtraceopen-parser --test sccm_spine_contract: 142 passed, 0 failed (137 baseline plus 5 new)cargo test --locked -p cmtraceopen-parser: 918 passed, 0 failedcargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings: cleancargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown: cleangit diff --check 7655826f HEAD: clean;rustfmt --checkclean on both changed filesCargo.tomluntouched; camelCase wire names preservedAssumptions and notes
cargo fmt -p cmtraceopen-parserreformats four unrelated ESP files (esp/redaction.rs,esp/reducer.rs,esp/timeline.rs,tests/esp_diagnostics.rs) because this machine's rustfmt disagrees with their committed formatting. Format only the files you changed.Refs #317. Not claiming native Windows acceptance; this is pure parser-layer work.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests