fix(sccm): harden client intake and CCM ambiguity (#319) - #435
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR improves CCM nested-opener recovery and adds a 4096-artifact limit to SCCM client intake deserialization and validation. Regression tests cover parser ambiguity, boundary acceptance, and oversized input rejection. ChangesCCM opener recovery
SCCM intake artifact limit
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
github-actions
Bot
added
bug
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens SCCM client intake bundle handling and CCM multiline opener ambiguity so SCCM evidence extraction fails closed under adversarial or malformed inputs, while preserving the existing public CCM LogEntry projection behavior.
Changes:
- Introduces an authoritative
MAX_SCCM_CLIENT_INTAKE_ARTIFACTS(4,096) limit and enforces it both during validation and during serde deserialization of intake bundles. - Updates the CCM multiline scanner to treat nested physical-line openers inside a complete-looking record as ambiguity (coverage-only) for SCCM evidence mode while keeping public parsing compatible.
- Adds targeted tests covering bundle-size limits and CCM ambiguity cases for LF/CRLF and same-line literal openers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/tests/sccm_client_intake.rs | Adds tests for bundle artifact count hard limits (validation + wire deserialization). |
| crates/cmtraceopen-parser/src/sccm/client/intake.rs | Defines and enforces the 4,096 artifact ceiling, including bounded seq deserialization and early validation. |
| crates/cmtraceopen-parser/src/parser/ccm.rs | Adds ambiguity detection for nested line-start CCM openers in SCCM evidence scanning and corresponding unit tests. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/tests/sccm_client_intake.rs (1)
446-467: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a text-based JSON deserialization test to cover the loop fallback path.
This test builds inputs through
serde_json::to_valueand decodes them withserde_json::from_value. AValue-based deserializer knows the exact array length ahead of time, so the oversized case is rejected by the earlysize_hint()check indeserialize_bounded_client_artifacts(intake.rs Lines 237-244), never by the loop-plus-IgnoredAnyfallback at intake.rs Lines 246-262.Text-based JSON parsing (
serde_json::from_str,from_slice,from_reader) does not provide a size hint before reading the array, so production code that deserializes bundles from raw JSON text relies on the loop fallback, not the early check. Add a test that serializes an oversized bundle to a JSON string and deserializes it withserde_json::from_str, to confirm the fallback path independently rejects an oversized sequence.#[test] fn intake_wire_rejects_more_than_the_v1_artifact_limit_from_json_text() { let artifact = synthetic_artifact("wire-limit-text", "PolicyAgent.log"); let oversized = serde_json::json!({ "artifacts": vec![artifact; MAX_SCCM_CLIENT_INTAKE_ARTIFACTS + 1], }); let text = oversized.to_string(); let error = serde_json::from_str::<SccmClientIntakeBundle>(&text) .expect_err("text-based JSON parsing must also reject an oversized artifact sequence"); assert!( error.to_string().contains("artifact count exceeds"), "the streaming fallback must report the bounded-contract violation: {error}" ); }Please confirm with a web search or the
serde_jsondocumentation that text-based deserializers (from_str/from_slice/from_reader) do not provideSeqAccess::size_hint, whilefrom_valuedoes, since this determines which branch ofdeserialize_bounded_client_artifactseach caller actually exercises.🤖 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/tests/sccm_client_intake.rs` around lines 446 - 467, Add a separate test near intake_wire_rejects_more_than_the_v1_artifact_limit_while_deserializing that builds an oversized bundle, serializes it to JSON text, and deserializes it with serde_json::from_str; assert deserialization fails and the error contains “artifact count exceeds,” thereby exercising the loop fallback in deserialize_bounded_client_artifacts rather than the size_hint path.crates/cmtraceopen-parser/src/sccm/client/intake.rs (1)
233-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
SccmClientIntakeError::ArtifactLimitExceededinstead of duplicating its message text.Lines 241-243 and 259-261 hardcode
"client intake artifact count exceeds the supported limit"as a separate literal from theSccmClientIntakeError::ArtifactLimitExceededvariant defined at Line 582. Three copies of the same message text now exist. If one copy changes, the wire deserializer and the nativevalidate_bundlepath can silently report inconsistent text. Pass the error enum directly, sinceA::Error::customaccepts anyDisplayandthiserroralready implementsDisplayforSccmClientIntakeError::ArtifactLimitExceeded.♻️ Proposed fix to remove the duplicated literal
if sequence .size_hint() .is_some_and(|size| size > MAX_SCCM_CLIENT_INTAKE_ARTIFACTS) { - return Err(A::Error::custom( - "client intake artifact count exceeds the supported limit", - )); + return Err(A::Error::custom( + crate::sccm::client::intake::SccmClientIntakeError::ArtifactLimitExceeded, + )); } @@ if sequence.next_element::<IgnoredAny>()?.is_some() { - return Err(A::Error::custom( - "client intake artifact count exceeds the supported limit", - )); + return Err(A::Error::custom( + crate::sccm::client::intake::SccmClientIntakeError::ArtifactLimitExceeded, + )); }🤖 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/client/intake.rs` around lines 233 - 266, Update both limit-exceeded branches in the `visit_seq` implementation to pass `SccmClientIntakeError::ArtifactLimitExceeded` directly to `A::Error::custom` instead of duplicating the message literal, preserving the existing deserialization behavior and shared error text.
🤖 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/client/intake.rs`:
- Around line 233-266: Update both limit-exceeded branches in the `visit_seq`
implementation to pass `SccmClientIntakeError::ArtifactLimitExceeded` directly
to `A::Error::custom` instead of duplicating the message literal, preserving the
existing deserialization behavior and shared error text.
In `@crates/cmtraceopen-parser/tests/sccm_client_intake.rs`:
- Around line 446-467: Add a separate test near
intake_wire_rejects_more_than_the_v1_artifact_limit_while_deserializing that
builds an oversized bundle, serializes it to JSON text, and deserializes it with
serde_json::from_str; assert deserialization fails and the error contains
“artifact count exceeds,” thereby exercising the loop fallback in
deserialize_bounded_client_artifacts rather than the size_hint path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5af3c8f6-1e14-44eb-898f-913cab568be5
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/parser/ccm.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/tests/sccm_client_intake.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
Scope
Issue #319 pure hardening only:
MAX_SCCM_CLIENT_INTAKE_ARTIFACTSthe authoritative 4,096-entry boundary for pure and native client intake;LogEntryprojection for LF/CRLF multiline messages;The rejected normalized text-payload bridge is deliberately absent. No
ingest.rschange, native I/O, Windows collection, Tauri command, workflow reducer, or live-lab claim is included.Dependency state
Based on reviewed SCCM integration head
0b37e7a842991fd9cc9341f7602cf05186d03587. This small contract hardening must merge before the native #319 adapter is restacked so native capture imports the single authoritative limit.TDD and adversarial matrix
LogEntry;Verification
cargo test --locked -p cmtraceopen-parser --test sccm_client_intake— 50 passedcargo test --locked -p cmtraceopen-parser— passedcargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— passedrustup run 1.88.0 cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passedgit diff --check— passedRepository-wide formatting still reports the documented unrelated baseline drift; all three changed files are clean.
Remaining #319 work
Native discovery/capture is still local and rejected on bounded enumeration, early reader cardinality, raw-path debug/error surfaces, direct-writer Windows ACL validation, and honest rollback failure reporting. Real Windows acceptance remains pending. Keep #319 open.
Summary by CodeRabbit
New Features
Bug Fixes
Tests