fix(dpp): add additionalProperties: false to document meta-schema#3475
Merged
QuantumExplorer merged 11 commits intov3.1-devfrom Apr 12, 2026
Merged
fix(dpp): add additionalProperties: false to document meta-schema#3475QuantumExplorer merged 11 commits intov3.1-devfrom
QuantumExplorer merged 11 commits intov3.1-devfrom
Conversation
Locks down the document type meta-schema to reject unknown top-level properties. This prevents pre-v12 contracts from smuggling in v12-only keys (like documentsCountable/rangeCountable) that would change storage semantics after a protocol upgrade. Added missing standard JSON Schema properties to the allowed list: - required, $comment, description - minProperties, maxProperties, dependentRequired TODO: On v12 activation, consider scanning existing contracts in state and stripping any unknown top-level keys from document schemas as a safety migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a v1 document meta-schema and compiles it, selects meta-schema version during document-type validation, extends platform/contract versioning to expose the schema feature flag (introducing CONTRACT_VERSIONS_V4), exposes a mutable accessor for document_schemas, and adds a protocol-upgrade migration that strips unknown top-level schema properties from stored contracts. Changes
Sequence Diagram(s)sequenceDiagram
participant Upgrade as PlatformUpgrade
participant DB as GroveDB
participant Reader as ContractReader
participant Deser as Deserializer
participant Strip as SchemaStripper
participant Writer as ContractWriter
Upgrade->>DB: iterate DataContractDocuments entries
DB->>Reader: fetch element bytes
Reader->>Deser: deserialize to DataContractInSerializationFormat
alt deserialization succeeds
Deser->>Strip: get & iterate document_schemas
Strip->>Strip: strip_unknown_properties_from_document_schema(...)
alt schema modified
Strip->>Writer: serialize updated contract
Writer->>DB: write back updated bincode value
end
else deserialization fails
Reader->>Upgrade: log & skip entry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
|
✅ Review complete (commit f0ccff7) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json`:
- Around line 627-628: The inline "dependentRequired" definition is too
permissive; replace it with the Draft 2020-12 validation meta-schema for
dependentRequired so values are enforced as arrays of strings (or explicitly set
"type":"object" plus "additionalProperties":
{"type":"array","items":{"type":"string"}}). Locate the "dependentRequired"
property in the document-meta.json and either change its value to a $ref that
points to the Draft 2020-12 dependentRequired meta-schema or update the object
to include "type":"object" and "additionalProperties":
{"type":"array","items":{"type":"string"}} to match the rest of the file's
meta-schema pattern. Ensure the new definition mirrors other properties' use of
Draft 2020-12 vocabularies.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36caaa25-3498-49eb-9c07-f7e9c6ae155c
📒 Files selected for processing (1)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR correctly adds additionalProperties: false to the document-type meta-schema, closing a property-smuggling vector. The allowlist of newly-defined top-level properties (required, $comment, description, minProperties, maxProperties, dependentRequired) is appropriate for enabling the constraint without breaking existing schemas. One minor schema consistency issue and a test-coverage gap remain.
Reviewed commit: 582af94
🟡 2 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/schema/meta_schemas/document/v0/document-meta.json`:
- [SUGGESTION] lines 627-629: Top-level `dependentRequired` definition is weaker than the inner one
The `dependentRequired` added at line 627 only validates `"type": "object"`, while the equivalent definition inside `documentSchema` (line 112-117) also enforces `minProperties: 1` and constrains each value to be a string array via `$ref: ".../$defs/stringArray"`. Per JSON Schema spec, `dependentRequired` values must be arrays of property-name strings. With the current top-level definition, a document type could pass meta-schema validation with `"dependentRequired": {"foo": 42}`, which would cause unpredictable behavior at runtime.
- [SUGGESTION] line 637: No test verifying that unknown top-level properties are now rejected
The PR's stated purpose is to block property smuggling (e.g., a pre-v12 contract including `documentsCountable` that would later be honored). While 2451 existing tests pass (proving no regressions), none explicitly submits a document type schema with an unknown top-level property and asserts rejection. A targeted test like `{"type": "object", ..., "unknownProp": true}` would directly verify the security fix and guard against accidental removal of the `additionalProperties: false` constraint.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3475 +/- ##
============================================
+ Coverage 84.77% 84.79% +0.02%
============================================
Files 2473 2476 +3
Lines 266762 267709 +947
============================================
+ Hits 226139 227000 +861
- Misses 40623 40709 +86
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Apr 11, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new meta-schema and regression test harden validation for newly checked schemas, but the PR still does not mitigate the stated pre-upgrade smuggling case for contracts already persisted in state.
Reviewed commit: 831d973
🔴 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/schema/meta_schemas/document/v0/document-meta.json`:
- [BLOCKING] lines 627-644: Persisted contracts still bypass the new top-level schema-key restriction
This closes the hole only for schemas that go through full meta-schema validation. `DocumentType::try_from_schema()` applies `DOCUMENT_META_SCHEMA_V0` only inside `if full_validation` (`packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:94-140`), but contracts loaded from state are still deserialized with `DataContract::versioned_deserialize(..., false, ...)` in Drive (`packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-84`), and both serialization paths pass that `full_validation = false` through when rebuilding document types (`packages/rs-dpp/src/data_contract/v0/serialization/mod.rs:94-105`, `packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:94-105`). So any contract that already smuggled an unknown top-level document-schema key before upgrade will continue to load unchanged after this patch, which is exactly the threat model described in the PR. Please add a load-time rejection/migration path for persisted contracts, or this fix remains incomplete for the targeted security case.
QuantumExplorer
force-pushed
the
fix/document-schema-additional-properties
branch
from
831d973 to
f9b8ab6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/rs-dpp/src/validation/meta_validators/mod.rs (1)
148-199: Consider extracting a shared document meta-schema builder.
DOCUMENT_META_SCHEMA_V0andDOCUMENT_META_SCHEMA_V1are almost identical; centralizing the common options/docs setup will reduce drift and make future changes safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/validation/meta_validators/mod.rs` around lines 148 - 199, DOCUMENT_META_SCHEMA_V0 and DOCUMENT_META_SCHEMA_V1 duplicate the same JSONSchema::options() setup; extract the repeated chain into a shared builder function (e.g., build_document_meta_schema_options or document_meta_schema_base) that configures the common options and .with_document entries (including the regex engine, Draft, and all DRAFT202012_* documents) and returns the configured builder or options object; then replace the duplicated chains in the constants (DOCUMENT_META_SCHEMA_V0 and DOCUMENT_META_SCHEMA_V1) by calling that shared builder and only applying schema-specific tweaks (like .compile(&DOCUMENT_META_JSON_V1) / .compile(&DOCUMENT_META_JSON_V0) or any version-specific overrides) to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json`:
- Around line 343-346: The "$schema" property in the v1 document meta-schema
currently requires the v0 URI; update the const value for the "$schema" schema
node to the v1 URI
"https://github.com/dashpay/platform/blob/master/packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json"
so the meta-schema's own "$schema" check matches its "$id" (i.e., change the
const on the "$schema" property in the document-meta.json from the v0 path to
the v1 path).
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 776-805: The test currently uses PlatformVersion::latest(), which
is unstable for version-sensitive assertions; replace that call with an
explicit, fixed PlatformVersion instance (e.g., construct the concrete version
used by these tests — PlatformVersion::new(<concrete_major_or_index>) or the
same explicit platform version constant used elsewhere in tests) and use that
value when calling DataContractConfig::default_for_version(...) and
DocumentTypeV1::try_from_schema(...). Update both occurrences noted (around the
first PlatformVersion::latest() and the other block at 824-854) so the
assertions are pinned to a specific platform version rather than latest().
In `@packages/rs-dpp/src/validation/meta_validators/mod.rs`:
- Around line 153-160: The duplicate builder call to with_patterns_regex_engine
overwrites the earlier size-limited RegexEngine configuration and removes the 5
MB size_limit; remove the second
with_patterns_regex_engine(RegexEngine::Regex(Default::default())) so the
RegexOptions with size_limit (set via RegexEngine::Regex(RegexOptions {
size_limit: Some(5 * (1 << 20)), ..Default::default() })) remains effective,
retaining the ReDoS mitigation while leaving the other builder calls
(should_ignore_unknown_formats, should_validate_formats, with_draft) unchanged.
---
Nitpick comments:
In `@packages/rs-dpp/src/validation/meta_validators/mod.rs`:
- Around line 148-199: DOCUMENT_META_SCHEMA_V0 and DOCUMENT_META_SCHEMA_V1
duplicate the same JSONSchema::options() setup; extract the repeated chain into
a shared builder function (e.g., build_document_meta_schema_options or
document_meta_schema_base) that configures the common options and .with_document
entries (including the regex engine, Draft, and all DRAFT202012_* documents) and
returns the configured builder or options object; then replace the duplicated
chains in the constants (DOCUMENT_META_SCHEMA_V0 and DOCUMENT_META_SCHEMA_V1) by
calling that shared builder and only applying schema-specific tweaks (like
.compile(&DOCUMENT_META_JSON_V1) / .compile(&DOCUMENT_META_JSON_V0) or any
version-specific overrides) to avoid drift.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c25d9692-23e4-4798-acce-0a5beb3b60ea
📒 Files selected for processing (10)
packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/validation/meta_validators/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
QuantumExplorer
force-pushed
the
fix/document-schema-additional-properties
branch
from
f9b8ab6 to
b52aed1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (2)
1054-1119: Consider adding test coverage for the property stripping logic.The existing test verifies that shielded pool trees are created but doesn't exercise the
strip_unknown_document_schema_propertieslogic. Consider adding a test that:
- Creates a contract with an unknown top-level property in its document schema
- Runs
transition_to_version_12- Verifies the unknown property was stripped and the contract was re-serialized
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs` around lines 1054 - 1119, Add a new test alongside test_transition_to_version_12_creates_shielded_pool_trees that constructs a contract document containing an unexpected top-level property, inserts/commits that contract into the platform state before calling transition_to_version_12, invokes platform.transition_to_version_12(&transaction, platform_version), then reads back the contract document and asserts the unknown property is removed (verifying strip_unknown_document_schema_properties ran) and that the contract was re-serialized/updated in storage; reference the existing test function name for placement and use transition_to_version_12 and strip_unknown_document_schema_properties as the behaviors under test.
686-713: Add a comment linking this allowlist to the v1 meta-schema source.This allowlist must stay synchronized with the v1 document meta-schema's top-level properties. Consider adding a comment pointing to
schema/meta_schemas/document/v1/document-meta.jsonso future maintainers know to update both in sync.📝 Suggested documentation
+ /// The set of top-level property names that the v1 document meta-schema allows on + /// a document type schema object. + /// + /// IMPORTANT: This list must match the `properties` keys defined in + /// `packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json`. const ALLOWED_DOCUMENT_SCHEMA_PROPERTIES: &[&str] = &[🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs` around lines 686 - 713, Add a brief comment immediately above the ALLOWED_DOCUMENT_SCHEMA_PROPERTIES constant that points to the canonical v1 meta-schema source (schema/meta_schemas/document/v1/document-meta.json) and states that this allowlist must be kept in sync with that file; reference the constant name ALLOWED_DOCUMENT_SCHEMA_PROPERTIES so maintainers know which list to update when the v1 document-meta.json changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- Around line 1054-1119: Add a new test alongside
test_transition_to_version_12_creates_shielded_pool_trees that constructs a
contract document containing an unexpected top-level property, inserts/commits
that contract into the platform state before calling transition_to_version_12,
invokes platform.transition_to_version_12(&transaction, platform_version), then
reads back the contract document and asserts the unknown property is removed
(verifying strip_unknown_document_schema_properties ran) and that the contract
was re-serialized/updated in storage; reference the existing test function name
for placement and use transition_to_version_12 and
strip_unknown_document_schema_properties as the behaviors under test.
- Around line 686-713: Add a brief comment immediately above the
ALLOWED_DOCUMENT_SCHEMA_PROPERTIES constant that points to the canonical v1
meta-schema source (schema/meta_schemas/document/v1/document-meta.json) and
states that this allowlist must be kept in sync with that file; reference the
constant name ALLOWED_DOCUMENT_SCHEMA_PROPERTIES so maintainers know which list
to update when the v1 document-meta.json changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0f6ed05-6e23-44e6-b448-999597ab0ea7
📒 Files selected for processing (12)
packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-dpp/src/validation/meta_validators/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs
- packages/rs-dpp/src/validation/meta_validators/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
QuantumExplorer
force-pushed
the
fix/document-schema-additional-properties
branch
2 times, most recently
from
9b66880 to
fe631a8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs`:
- Around line 45-50: The retain closure currently keeps non-string keys by
returning true; change it to drop non-text keys so malformed non-text keys are
removed. In the map.retain(|(key, _)| { ... }) closure, replace the `_ => return
true` branch with `_ => return false` (or equivalently treat only Value::Text(s)
and check ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES.contains(&s.as_str()) while
returning false for all other variants) so non-text keys are not preserved by
the filter; ensure the logic still uses ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES to
allow valid text keys.
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- Around line 724-835: The migration currently silently skips contracts on
unexpected storage layouts or deserialization failures (branches around
maybe_element/maybe_history_element and the Err(e) from
bincode::borrow_decode_from_slice), which can leave the node in a mixed state;
change those continue paths to surface failures: replace the tracing::warn +
continue behavior in the match arms that handle unexpected reference types,
unresolved historical elements, unexpected element types, and bincode
deserialization Err(e) to instead return a descriptive error (including
contract_id_bytes hex and the underlying error or a specific enum/variant), or
accumulate and then return an aggregated error from the outer function so the
protocol upgrade aborts rather than proceeding silently; update any callers of
this function to propagate the error (or handle the aggregated error) so
migration fails fast on any problematic contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cc04a72-64ad-4dee-b1a7-821276e4b62c
📒 Files selected for processing (14)
packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rspackages/rs-dpp/src/data_contract/document_type/schema/mod.rspackages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-dpp/src/validation/meta_validators/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (4)
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rs
- packages/rs-dpp/src/data_contract/serialized_version/mod.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs
- packages/rs-dpp/src/validation/meta_validators/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json
QuantumExplorer
force-pushed
the
fix/document-schema-additional-properties
branch
4 times, most recently
from
862bd4b to
a006d20
Compare
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I re-reviewed the pinned a006d20 commit directly after all six reviewer ACP lanes and a codex verifier lane stalled before first output. The new v12 migration closes the earlier stored-contract gap by stripping unknown top-level document schema keys during protocol upgrade, the version switch now selects the strict v1 document meta-schema at protocol 12, and the targeted DPP + drive-abci tests pass on this checkout. I did not find additional actionable issues in the current diff.
Reviewed commit: a006d20
…ies from v12+ Introduces a versioned document meta-schema to prevent pre-v12 contracts from smuggling unknown top-level properties that could change storage semantics after a protocol upgrade. - v0 meta-schema (pre-v12): allows any top-level properties (unchanged) - v1 meta-schema (v12+): adds additionalProperties: false, plus all standard JSON Schema keywords used by existing contracts (required, $comment, description, minProperties, maxProperties, dependentRequired) Changes: - New: schema/meta_schemas/document/v1/document-meta.json - New: DOCUMENT_META_SCHEMA_V1 compiled validator - New: document_type_schema version field in DocumentTypeSchemaVersions - CONTRACT_VERSIONS_V4 uses document_type_schema: 1 - try_from_schema v0/v1 dispatch to correct validator based on version - 3 tests: v0 allows unknown props, v1 rejects unknown, v1 accepts known Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/document-schema-additional-properties
branch
from
a006d20 to
538c847
Compare
thepastaclaw
suggested changes
Apr 11, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Direct verification against 538c847e9b6126b3923cad23978d0d5d98401cbf confirms two blocking issues remain. The PR does change the active pre-v12 document/v0 meta-schema in this checkout, and the new v12 migration only rewrites the latest stored revision for keeps-history contracts even though history fetch/verification still deserializes every historical snapshot with the current platform version. The remaining non-blocking findings are minor cleanup and test-coverage concerns.
Reviewed commit: 538c847
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: The PR relaxes pre-v12 document-type validation on the live v0 schema path
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json (lines 602-612)
This checkout removes the root-level additionalProperties: false guard from document/v0/document-meta.json, so protocol versions that still dispatch document_type_schema version 0 now accept unknown top-level document schema keys that older nodes reject. DocumentTypeV1::try_from_schema selects DOCUMENT_META_SCHEMA_V0 for the pre-v12 path, and the new v0_schema_allows_unknown_properties test proves the behavior changed. That is not just a v12-only migration aid: it alters consensus validation before the fork boundary and creates mixed-version rollout/replay risk.
🤖 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/schema/meta_schemas/document/v0/document-meta.json`:
- [BLOCKING] lines 602-612: The PR relaxes pre-v12 document-type validation on the live v0 schema path
This checkout removes the root-level `additionalProperties: false` guard from `document/v0/document-meta.json`, so protocol versions that still dispatch `document_type_schema` version `0` now accept unknown top-level document schema keys that older nodes reject. `DocumentTypeV1::try_from_schema` selects `DOCUMENT_META_SCHEMA_V0` for the pre-v12 path, and the new `v0_schema_allows_unknown_properties` test proves the behavior changed. That is not just a v12-only migration aid: it alters consensus validation before the fork boundary and creates mixed-version rollout/replay risk.
In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- [BLOCKING] lines 753-799: The v12 migration rewrites only the latest historical contract revision
When the contract root contains a history tree, the migration resolves key `[0]` to a single sibling timestamp and rewrites only that one `Element::Item`. Older timestamped revisions in the same history tree are never visited. That leaves pre-v12 revisions with smuggled top-level schema keys on disk, but both history fetch and proof verification still deserialize each historical entry with `DataContract::versioned_deserialize(..., platform_version)`, so querying or verifying contract history after protocol12 can still fail on those untouched older snapshots.
In `packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs`:
- [SUGGESTION] lines 9-34: The migration allowlist is not checked against the v1 JSON schema
`ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES` is the source of truth for stripping unknown keys during the v12 migration, but there is no test that compares this list to the top-level `properties` declared in `schema/meta_schemas/document/v1/document-meta.json`. If a later change updates one side without the other, the migration can silently delete a field that the validator accepts or preserve one that the validator rejects.
…cker, fix review items - Move strip_unknown_document_schema_properties from Platform<C> in drive-abci to a method on Drive in rs-drive - Add check-contract-properties script in rs-scripts that fetches all contracts from Platform Explorer API, discovers DAPI nodes from validators, and checks for unknown top-level document schema properties - Remove duplicate applicator registration in DRAFT_202012, V0, and V1 meta-schema validators - Add allowlist_matches_v1_meta_schema_properties test ensuring the migration allowlist stays in sync with the v1 JSON meta-schema - Fix script error counting to distinguish fetch errors from checked contracts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert the migration's use of fetch_contract_ids (which will be in a separate PR) back to the original inline grove_get_raw_path_query approach. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Apr 11, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The checkout matches pinned head 66b862d32a63b43c4ccf050540a5297fe18970e0, and the earlier pre-v12 concerns called out in the coordinator notes do appear resolved. One blocking issue remains: the v12 migration rewrites only the latest history-enabled contract revision, while older timestamped revisions are still reachable through the history fetch/proof APIs and remain unsanitized.
Reviewed commit: 66b862d
🔴 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [BLOCKING] lines 83-140: v12 migration sanitizes only the latest revision of history-enabled contracts
When `keeps_history` contracts are encountered, this migration resolves the `[contract]/[0]/[0]` sibling reference and rewrites only the single timestamped item it points to. Older revisions remain stored under other timestamp keys in the same history subtree, and `fetch_contract_history_query` still ranges over those keys directly while `fetch_contract_with_history_v0` and `prove_contract_history_v0` expose them to callers. That leaves reachable historical contract revisions with the pre-v12 schema shape after the upgrade, so the migration does not actually sanitize all observable contract state.
thepastaclaw
suggested changes
Apr 11, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The checkout matches pinned HEAD 27e50d9f1f74faee524669d61b96df05210a9d7d, and the only substantive candidate issue remains valid. The v12 migration still sanitizes only the timestamp currently targeted by the [0] reference for history-enabled contracts, while the history query/proof APIs continue to expose older timestamped revisions unchanged.
Reviewed commit: 27e50d9
🔴 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [BLOCKING] lines 81-124: Historical contract migration rewrites only the latest stored revision
In the `Element::Tree(..)` branch, the migration reads `[DataContractDocuments, contract_id,0,0]`, resolves that sibling reference to a single `timestamp_key`, and rewrites only that one item. History-enabled contract updates append a new item at each encoded timestamp and then move the `[0]` reference to the latest entry (`packages/rs-drive/src/drive/contract/insert/add_contract_to_storage/v0/mod.rs:93-126`), so older revisions remain stored under the same history subtree. The history APIs query the whole `[DataContractDocuments, contract_id,0]` subtree by timestamp range rather than following the `[0]` reference (`packages/rs-drive/src/drive/contract/queries.rs:201-224`, `packages/rs-drive/src/drive/contract/get_fetch/fetch_contract_with_history/v0/mod.rs:62-89`), which means pre-latest revisions with unknown schema properties remain reachable after the migration.
…ma-additional-properties
Replace inline grove_get_raw_path_query with fetch_contract_ids_v0 for enumerating contracts during the v12 migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The v12 migration now iterates every revision stored in a historical contract's history subtree (keyed by encoded timestamps), stripping unknown top-level properties from each one. Previously only the latest revision (via the [0] reference) was cleaned, leaving older revisions with smuggled properties reachable through history queries. Extract shared deserialization/strip/rewrite logic into strip_and_rewrite_contract_element helper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test that the v12 migration strips unknown properties from ALL revisions of a historical contract, not just the latest. Creates 3 revisions each with a different smuggled property, runs migration, verifies all 3 are cleaned and the [0] reference is preserved. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…visions Give each historical revision distinct element flags and assert they are preserved after migration strips unknown properties. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The v1 document meta-schema's $schema const now correctly points to the v1 path. A new enrich_with_base_schema v1 injects the v1 URI into document schemas during enrichment. CONTRACT_VERSIONS_V4 now uses enrich_with_base_schema: 1. Pre-v12 contracts continue using the v0 URI via enrich_with_base_schema v0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify that enrich_with_base_schema injects v0 URI for pre-v12 protocol versions and v1 URI for v12+. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's migration logic and test coverage are solid. The only finding — that fetch_contract_ids_v0 is called with u16::MAX without a pagination loop — is technically correct but practically irrelevant given mainnet's ~46 contracts and the economic cost of creating 65,000+ contracts before a v12 upgrade. Codex agents independently confirmed no issues, and all test assertions are correct.
Reviewed commit: 0ade3fe
🟡 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [SUGGESTION] lines 28-29: Migration fetches at most u16::MAX contracts without pagination
The migration calls `fetch_contract_ids_v0(None, u16::MAX, ...)` as a single unpaginated query. Since `limit` is `u16`, the absolute maximum is 65,535 contracts. If more exist at migration time, the remainder would be silently skipped and never cleaned.
Mainnet currently has ~46 contracts, and creating ~65,490 more before the v12 upgrade activates is economically prohibitive given per-contract fees. The `fetch_contract_ids_v0` function already supports pagination via its `start_at` parameter (with full test coverage for pagination in the v0 module tests), so wrapping this in a pagination loop would be a low-cost hardening measure for a one-shot consensus-critical migration.
Comment on lines
+28
to
+29
| let contract_ids = | ||
| self.fetch_contract_ids_v0(None, u16::MAX, Some(transaction), drive_version)?; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Migration fetches at most u16::MAX contracts without pagination
The migration calls fetch_contract_ids_v0(None, u16::MAX, ...) as a single unpaginated query. Since limit is u16, the absolute maximum is 65,535 contracts. If more exist at migration time, the remainder would be silently skipped and never cleaned.
Mainnet currently has ~46 contracts, and creating ~65,490 more before the v12 upgrade activates is economically prohibitive given per-contract fees. The fetch_contract_ids_v0 function already supports pagination via its start_at parameter (with full test coverage for pagination in the v0 module tests), so wrapping this in a pagination loop would be a low-cost hardening measure for a one-shot consensus-critical migration.
source: ['claude']
🤖 Fix this 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [SUGGESTION] lines 28-29: Migration fetches at most u16::MAX contracts without pagination
The migration calls `fetch_contract_ids_v0(None, u16::MAX, ...)` as a single unpaginated query. Since `limit` is `u16`, the absolute maximum is 65,535 contracts. If more exist at migration time, the remainder would be silently skipped and never cleaned.
Mainnet currently has ~46 contracts, and creating ~65,490 more before the v12 upgrade activates is economically prohibitive given per-contract fees. The `fetch_contract_ids_v0` function already supports pagination via its `start_at` parameter (with full test coverage for pagination in the v0 module tests), so wrapping this in a pagination loop would be a low-cost hardening measure for a one-shot consensus-critical migration.
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All agent findings converge on a single pre-existing defense-in-depth gap: the v12 migration fetches contract IDs in one u16::MAX-bounded call without pagination. The head commit (f0ccff7) itself is test-only and correct — the schema URI version dispatch, CONTRACT_VERSIONS_V4, and enrich_with_base_schema_v1 are all properly wired. No blocking issues found.
Reviewed commit: f0ccff7
🟡 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [SUGGESTION] lines 27-29: Migration fetches at most 65,535 contract IDs without pagination
`strip_unknown_document_schema_properties()` calls `fetch_contract_ids_v0(None, u16::MAX, ...)` once and never pages through the result set. Because `fetch_contract_ids_v0` wraps the limit in `SizedQuery::new(query, Some(limit), None)`, the query genuinely stops at 65,535 entries — any contracts beyond that boundary are silently skipped and left unsanitized.
This matters because the migration is specifically designed to strip unknown top-level document schema properties before protocol v12 starts interpreting them. Contract creation is permissionless, so the total count is unbounded in principle. If the contract set ever exceeds 65,535, the migration's invariant ("all persisted schemas are normalized") breaks silently — no error, no log, just contracts that retain properties the migration was supposed to remove.
The function already supports cursor-based pagination via its `start_at: Option<([u8; 32], bool)>` parameter. A paged loop using the last returned ID as the next cursor would close the gap without changing the function signature. Current mainnet has ~46 contracts, so this is not urgent, but it's the kind of silent truncation that becomes very hard to diagnose if it ever triggers.
Comment on lines
+27
to
+29
| // 1. Fetch all contract IDs. | ||
| let contract_ids = | ||
| self.fetch_contract_ids_v0(None, u16::MAX, Some(transaction), drive_version)?; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Migration fetches at most 65,535 contract IDs without pagination
strip_unknown_document_schema_properties() calls fetch_contract_ids_v0(None, u16::MAX, ...) once and never pages through the result set. Because fetch_contract_ids_v0 wraps the limit in SizedQuery::new(query, Some(limit), None), the query genuinely stops at 65,535 entries — any contracts beyond that boundary are silently skipped and left unsanitized.
This matters because the migration is specifically designed to strip unknown top-level document schema properties before protocol v12 starts interpreting them. Contract creation is permissionless, so the total count is unbounded in principle. If the contract set ever exceeds 65,535, the migration's invariant ("all persisted schemas are normalized") breaks silently — no error, no log, just contracts that retain properties the migration was supposed to remove.
The function already supports cursor-based pagination via its start_at: Option<([u8; 32], bool)> parameter. A paged loop using the last returned ID as the next cursor would close the gap without changing the function signature. Current mainnet has ~46 contracts, so this is not urgent, but it's the kind of silent truncation that becomes very hard to diagnose if it ever triggers.
source: ['claude', 'codex']
🤖 Fix this 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [SUGGESTION] lines 27-29: Migration fetches at most 65,535 contract IDs without pagination
`strip_unknown_document_schema_properties()` calls `fetch_contract_ids_v0(None, u16::MAX, ...)` once and never pages through the result set. Because `fetch_contract_ids_v0` wraps the limit in `SizedQuery::new(query, Some(limit), None)`, the query genuinely stops at 65,535 entries — any contracts beyond that boundary are silently skipped and left unsanitized.
This matters because the migration is specifically designed to strip unknown top-level document schema properties before protocol v12 starts interpreting them. Contract creation is permissionless, so the total count is unbounded in principle. If the contract set ever exceeds 65,535, the migration's invariant ("all persisted schemas are normalized") breaks silently — no error, no log, just contracts that retain properties the migration was supposed to remove.
The function already supports cursor-based pagination via its `start_at: Option<([u8; 32], bool)>` parameter. A paged loop using the last returned ID as the next cursor would close the gap without changing the function signature. Current mainnet has ~46 contracts, so this is not urgent, but it's the kind of silent truncation that becomes very hard to diagnose if it ever triggers.
shumkov
reviewed
Apr 12, 2026
| ], | ||
| "additionalProperties": false | ||
| }, | ||
| "countable": { |
Collaborator
There was a problem hiding this comment.
Are we adding countable in this PR?
Member
Author
|
there's countable in here, next PR will fix this. |
shumkov
approved these changes
Apr 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a versioned document meta-schema and a v12 migration to prevent pre-v12 contracts from carrying unknown top-level properties that could change storage semantics after a protocol upgrade.
Motivation
A scan of all 46 data contracts currently deployed on mainnet confirmed that at least one contract carries an unknown top-level property (
"mutable"on a document type schema, which should be"documentsMutable"). Because the v0 document meta-schema did not enforceadditionalProperties: false, these invalid keys were silently accepted and persisted. If left in place, future protocol versions that read new schema keys (e.g.documentsCountable,rangeCountablefrom PR #3457) could misinterpret properties that were never intended by the contract author, leading to storage-semantic mismatches such as creating CountTree structures where NormalTree was expected.Approach
document/v0/document-meta.jsondocument/v1/document-meta.jsonadditionalProperties: false— rejects unknown keysv1 meta-schema additions
Standard JSON Schema keywords added to the allowed list:
required,$comment,descriptionminProperties,maxPropertiesdependentRequiredNote:
documentsCountableandrangeCountableare NOT added here — they come in PR #3457.v12 Migration
On activation of protocol version 12,
transition_to_version_12now:The migration logic lives as a method on
Drive(strip_unknown_document_schema_properties) inrs-drive, called by the drive-abci protocol upgrade handler.Script: check-contract-properties
A new
rs-scriptsbinary (check-contract-properties) was added to verify contracts on any network. It:Usage:
cargo run -p rs-scripts --bin check-contract-propertiesChanges
schema/meta_schemas/document/v1/document-meta.jsonDOCUMENT_META_SCHEMA_V1compiled validatordocument_type_schemaversion field inDocumentTypeSchemaVersionsDrive::strip_unknown_document_schema_propertiesmigration method inrs-drivedocument_schemas_mut()accessor onDataContractInSerializationFormatcheck-contract-propertiesscript inrs-scriptsCONTRACT_VERSIONS_V4setsdocument_type_schema: 1try_from_schemav0/v1 dispatch to correct validator based on versionTest plan
v0_schema_allows_unknown_properties— pre-v12 behavior preservedv1_schema_rejects_unknown_properties— v12+ rejects unknown keysv1_schema_accepts_known_properties— standard keys still passtest_v12_migration_strips_unknown_document_schema_properties— non-historical contractstest_v12_migration_strips_unknown_properties_from_historical_contract— historical contracts with cache verification"mutable"(stripped by migration)cargo check -p drive-abcicleancargo fmt --allclean🤖 Generated with Claude Code