Skip to content

fix(rs-sdk-ffi)!: sanitize properties in dash_sdk_document_set_properties - #4927

Merged
QuantumExplorer merged 1 commit into
v4.2-devfrom
fix/ffi-document-set-properties-sanitize
Sep 23, 2026
Merged

QuantumExplorer merged 1 commit into
v4.2-devfrom
fix/ffi-document-set-properties-sanitize

Conversation

@QuantumExplorer

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

dash_sdk_document_set_properties stored the parsed JSON properties as they came in, without the schema sanitizer. dash_sdk_document_create, and the platform-wallet create_document_with_signer / replace_document_with_signer, all run DocumentType::sanitize_document_properties first. That step turns base58 or hex strings into identifiers, hex or base64 strings into bytes, and narrows integers, including inside protocol version 14 typed array elements.

The Swift SDK's documentReplace (used by the SwiftExampleApp state-transition builder in TransitionDetailView.swift) goes through set_properties. #4926 makes that builder send identifier and byte-array values as base58 and hex strings. On this path those strings stayed as text in the document and ended up in the replace transition as text, where the document type's schema expects byte arrays ("type": "array"). So the replace could not carry an identifier or byte-array property, whether top-level or inside a typed array.

Example: a note type with an identifier author, a byte array payload and a typed identifier array members, updated with

{"author":"US517G5965aydkZ46HS38QLi7UQiSojurfbQfKCELFx","payload":"deadbeef","members":["YMN9Qj5jPNp7j14VPcML1B6xGgcPWVZUGLFU3Mnyfaf","cGfHiC6Kgg3FpFZvgwGcswsCRtp4aBP2fzuXRQPizuN"]}

Before, the stored document properties:

author:  Text("US517G5965aydkZ46HS38QLi7UQiSojurfbQfKCELFx")
members: Array([Text("YMN9Qj5jPNp7j14VPcML1B6xGgcPWVZUGLFU3Mnyfaf"), Text("cGfHiC6Kgg3FpFZvgwGcswsCRtp4aBP2fzuXRQPizuN")])
payload: Text("deadbeef")

After:

author:  Identifier([7; 32])
members: Array([Identifier([8; 32]), Identifier([9; 32])])
payload: Bytes([0xde, 0xad, 0xbe, 0xef])

What was done?

  • dash_sdk_document_set_properties (packages/rs-sdk-ffi/src/document/util.rs) now takes the SDK handle, the base58 contract id and the document type name. It gets the contract from the trusted context provider the same way dash_sdk_document_create does, which is also the lookup dash_sdk_document_replace_on_platform makes with the same id. It then runs sanitize_document_properties for that document type before storing the map. If the contract or type cannot be resolved, it returns an error and leaves the document unchanged.
  • The Swift SDK's documentReplace (StateTransitionExtensions.swift) passes the SDK handle, contract id and document type it already holds. It used to discard the returned error (_ =). It now frees the error and throws SDKError.internalError.

The C signature had to change: a Document records neither its contract id nor its type name, so the handle alone cannot identify the schema to sanitize against. git grep finds no other caller (Swift, Kotlin, C or docs).

How Has This Been Tested?

  • New should_store_identifier_and_byte_properties_decoded_from_strings in rs-sdk-ffi: builds a contract at the latest protocol version with an identifier, a byte array and a typed identifier array. It serves that contract from an offline trusted context provider (an address-literal quorum URL, so nothing resolves or connects), sets the JSON above through dash_sdk_document_set_properties, and checks the stored values. Before the fix, this test failed with left: Some(Text("US517G59...")) right: Some(Identifier([7, ...])).
  • New should_leave_the_document_unchanged_when_the_document_type_cannot_be_resolved: with an unknown contract id and with an unknown type name, the call returns an InternalError and the document stays unchanged.
  • cargo test -p rs-sdk-ffi --lib document::: 75 passed.
  • cargo clippy -p rs-sdk-ffi and cargo clippy -p rs-sdk-ffi --all-targets: no warnings.
  • The Swift change was not compiled locally (that needs the iOS xcframework build); CI builds it.

Breaking Changes

dash_sdk_document_set_properties changes from (document_handle, properties_json) to (sdk_handle, document_handle, data_contract_id, document_type_name, properties_json). FFI callers must pass the SDK handle, the base58 contract id and the document type name. The contract must be in the trusted context provider, as it already must be for the dash_sdk_document_replace_on_platform* call that follows. The only in-repo caller, the Swift SDK's documentReplace, is updated here.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

…ties

dash_sdk_document_set_properties stored the parsed JSON as is, so a
base58 identifier or a hex byte string (top level or inside a typed
array) reached the replace transition as text. The create path and the
platform-wallet create/replace paths run
DocumentType::sanitize_document_properties first; this one did not.

The function now takes the SDK handle, the base58 contract id and the
document type name, looks the contract up in the trusted context
provider (the lookup dash_sdk_document_replace_on_platform makes with
the same id) and sanitizes before storing. On any error the document is
left unchanged.

BREAKING CHANGE: dash_sdk_document_set_properties takes
(sdk_handle, document_handle, data_contract_id, document_type_name,
properties_json). The Swift SDK's documentReplace is updated and now
surfaces the error instead of discarding it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c75af8fa-356b-4b22-82a2-a7be9513f741

📥 Commits

Reviewing files that changed from the base of the PR and between ba7b075 and 066464c.

📒 Files selected for processing (2)
  • packages/rs-sdk-ffi/src/document/util.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

⚠️ DEGRADED — Queued for automated review — 28th in line, estimated start in ~17 h (commit 066464c)
Estimated review time once started: ~1.3 h (two-phase automated review; median of recent runs).
The primary review models are currently out of quota; this review will run on stand-in models and be marked as degraded.

  • Request priority review — click to move this review to the front of the queue.

@QuantumExplorer
QuantumExplorer merged commit 4527e5f into v4.2-dev Sep 23, 2026
8 checks passed
@QuantumExplorer
QuantumExplorer deleted the fix/ffi-document-set-properties-sanitize branch September 23, 2026 00:28
@github-actions github-actions Bot added this to the v4.2.0 milestone Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants