fix(dpp): bound document value validation depth#4115
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a versioned maximum document value depth, iterative nested-value traversal, and consensus validation that rejects documents exceeding the configured limit. Tests cover protocol configuration, depth boundaries, recursive map keys, oversized data, and validation errors. ChangesDocument depth validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit aa3edcf) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4115 +/- ##
=========================================
Coverage 87.42% 87.42%
=========================================
Files 2643 2643
Lines 333632 333685 +53
=========================================
+ Hits 291684 291735 +51
- Misses 41948 41950 +2
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact HEAD 22a91fc, the new depth check remains unreachable for some under-limit attacker payloads because recursive state-transition decoding can abort first; this is a blocking denial of service. The field-size helper also retains a direct root-array clone failure, but the current DPP caller reports a shallow map key and applies the v12 depth guard first, so that issue is nonblocking. Targeted tests passed, and isolated probes against the actual crates reproduced both aborts.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs:50-59: Enforce the depth limit during decoding
This validation runs after attacker-controlled document values have already undergone recursive processing. An actual document-batch state transition containing 4,000 single-element arrays encoded to 8,149 bytes, below the 20,480-byte transition limit, and aborted with a stack overflow inside `StateTransition::deserialize_from_bytes`. `Value` derives recursive `Decode`, so the transition never reaches this deterministic `ValueError`. Values that survive decoding are also recursively cloned while create/replace actions are built and again when `self.data().into()` prepares validation. Enforce the depth budget in a custom or iterative decoder, then check borrowed transition data before action transformation performs recursive clones.
In `packages/rs-platform-value/src/lib.rs`:
- [SUGGESTION] packages/rs-platform-value/src/lib.rs:1401: Avoid cloning a recursive root-array result
For a root array, `reported_value` is its first child. If an oversized scalar occurs beneath that child, `reported_value.cloned()` recursively clones the remaining subtree and defeats the helper's stack-independent traversal. An actual 8,000-level `Value::Array` ending in oversized text aborted here, while the new deep test uses `Null` and never enters this result path. The current DPP production caller wraps document properties in a root map, preserving a shallow field key, and v12 depth-checks first, so this is not a second blocking document-validation exploit. It still violates the changed public helper's stated behavior; represent the reported field without cloning a recursive `Value` and add a deeply nested oversized root-array test.
| if let Some(max_depth) = platform_version.system_limits.max_document_value_depth { | ||
| if let Some(actual_depth) = value.first_depth_exceeding(max_depth as usize) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| ConsensusError::BasicError(BasicError::ValueError( | ||
| ValueError::new_from_string(format!( | ||
| "document value depth {actual_depth} exceeds system maximum {max_depth}" | ||
| )), | ||
| )), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Enforce the depth limit during decoding
This validation runs after attacker-controlled document values have already undergone recursive processing. An actual document-batch state transition containing 4,000 single-element arrays encoded to 8,149 bytes, below the 20,480-byte transition limit, and aborted with a stack overflow inside StateTransition::deserialize_from_bytes. Value derives recursive Decode, so the transition never reaches this deterministic ValueError. Values that survive decoding are also recursively cloned while create/replace actions are built and again when self.data().into() prepares validation. Enforce the depth budget in a custom or iterative decoder, then check borrowed transition data before action transformation performs recursive clones.
source: ['codex']
| }; | ||
|
|
||
| if let Some(actual_size) = actual_size { | ||
| return Some((reported_value.cloned(), actual_size)); |
There was a problem hiding this comment.
🟡 Suggestion: Avoid cloning a recursive root-array result
For a root array, reported_value is its first child. If an oversized scalar occurs beneath that child, reported_value.cloned() recursively clones the remaining subtree and defeats the helper's stack-independent traversal. An actual 8,000-level Value::Array ending in oversized text aborted here, while the new deep test uses Null and never enters this result path. The current DPP production caller wraps document properties in a root map, preserving a shallow field key, and v12 depth-checks first, so this is not a second blocking document-validation exploit. It still violates the changed public helper's stated behavior; represent the reported field without cloning a recursive Value and add a deeply nested oversized root-array test.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head aa3edcf, both prior findings are fixed: decoding is iterative and version-scoped, and oversized-field reporting no longer clones recursive values. One blocking consensus-path inconsistency remains: over-depth documents are charged and nonce-bumped only when their nested Value depth remains within the decoder ceiling; deeper instances become unpaid invalid encodings.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/state_transition/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/mod.rs:800-807: Keep all over-depth documents on the same consensus path
The decoder applies `max_document_value_depth` to each `Value`, while document validation includes the enclosing `BTreeMap<String, Value>` as depth one. A property containing 256 nested arrays therefore has document depth 257 but Value depth 256: it decodes, reaches `first_data_depth_exceeding`, returns `BasicError::ValueError`, and on protocol v12 produces a `BumpIdentityDataContractNonce` action and paid error. With 257 nested arrays, the decoder instead raises `DecodeError::OtherString`; deserialization maps that to `PlatformDeserializationError`, raw-transition decoding maps it to `SerializedObjectParsingError`, and processing returns an unpaid error without the nonce action. `prepare_proposal` consequently retains the first transition but removes the second. This contradicts the PR's stated deterministic `ValueError` behavior and lets equivalent violations select different fee and nonce semantics. Preserve the iterative safety boundary, but coordinate it with document-depth validation through a typed depth failure or a separate defensive ceiling so every protocol depth violation follows the intended consensus path.
| let max_value_depth = platform_version | ||
| .system_limits | ||
| .max_document_value_depth | ||
| .map(usize::from); | ||
| let state_transition = | ||
| platform_value::with_value_decode_depth_limit(max_value_depth, || { | ||
| StateTransition::deserialize_from_bytes(bytes) | ||
| })?; |
There was a problem hiding this comment.
🔴 Blocking: Keep all over-depth documents on the same consensus path
The decoder applies max_document_value_depth to each Value, while document validation includes the enclosing BTreeMap<String, Value> as depth one. A property containing 256 nested arrays therefore has document depth 257 but Value depth 256: it decodes, reaches first_data_depth_exceeding, returns BasicError::ValueError, and on protocol v12 produces a BumpIdentityDataContractNonce action and paid error. With 257 nested arrays, the decoder instead raises DecodeError::OtherString; deserialization maps that to PlatformDeserializationError, raw-transition decoding maps it to SerializedObjectParsingError, and processing returns an unpaid error without the nonce action. prepare_proposal consequently retains the first transition but removes the second. This contradicts the PR's stated deterministic ValueError behavior and lets equivalent violations select different fee and nonce semantics. Preserve the iterative safety boundary, but coordinate it with document-depth validation through a typed depth failure or a separate defensive ceiling so every protocol depth violation follows the intended consensus path.
source: ['codex']
2 participants
Summary
Value::has_data_larger_thanas an iterative traversal while preserving outer field reportingRoot cause
Document create and replace validation passed decoded, attacker-controlled properties to
has_data_larger_thanbefore JSON/schema validation. Arrays and maps recursively called the helper once per nesting level, and the oversized-array error path repeatedly cloned nested subtrees while unwinding.Impact
Over-depth document values now return a deterministic consensus
ValueErrorbefore the formerly recursive consumers run. Protocol versions before v12 retain their prior consensus rule, while the shared field-size helper is iterative for all callers.Validation
DocumentFieldMaxSizeExceededErrorcargo test -p platform-value— 1,362 passedcargo test -p platform-version -q— 8 passedcargo test -p dpp --all-features -q— 3,867 passed, 11 ignoredcargo clippy -p platform-value --all-targets -- -D warnings— passedcargo clippy -p platform-version --all-targets -- -D warnings— passedcargo clippy -p dpp --all-features --lib -- -D warnings— passedcargo clippy -p dpp --all-features --tests -- -D warnings -A clippy::needless_borrows_for_generic_args— passedcargo fmt --all -- --checkandgit diff --check— passedThe unmodified strict DPP test-target Clippy command is currently blocked by an unrelated pre-existing
needless_borrows_for_generic_argswarning indata_contract/factory/v0/mod.rs.Summary by CodeRabbit
New Features
Bug Fixes