Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bfba2ef
fix(rs-dapi): decode base64 CBOR message in Tenderdash error data field
lklimek Mar 16, 2026
7ae8846
Merge branch 'v3.1-dev' into fix/sdk-cbor-security-hardening
lklimek Mar 18, 2026
25de8d2
fix(rs-dapi): address PR review comments on decode_data_message
lklimek Mar 18, 2026
1b38d74
Merge branch 'v3.1-dev' into fix/sdk-cbor-security-hardening
lklimek Mar 20, 2026
91daf4b
Merge branch 'v3.1-dev' into fix/sdk-cbor-security-hardening
lklimek Mar 20, 2026
b18c173
feat(sdk): detect duplicate identity key errors from drive-error-data…
lklimek Mar 20, 2026
5e99219
Revert "feat(sdk): detect duplicate identity key errors from drive-er…
lklimek Mar 20, 2026
c27a959
feat(sdk): add DriveInternalError variant and decode drive-error-data…
lklimek Mar 20, 2026
a9a9fb9
Merge branch 'v3.1-dev' into fix/sdk-cbor-security-hardening
lklimek Mar 23, 2026
e32635f
fix(sdk): log drive-error-data-bin to_bytes() failures instead of sil…
lklimek Mar 23, 2026
8055ce2
Merge remote-tracking branch 'origin/v3.1-dev' into fix/sdk-cbor-secu…
lklimek Apr 13, 2026
5581447
test(sdk): add unit tests for drive-error-data-bin DriveInternalError…
lklimek Apr 13, 2026
f7833a2
fix(wasm-sdk): handle DriveInternalError variant in error mapping
lklimek Apr 13, 2026
d855555
feat(sdk): auto-detect protocol version from network response metadata
lklimek Apr 13, 2026
6928fdd
test(sdk): add TC-6 concurrent updates and TC-7 global DPP version sy…
lklimek Apr 13, 2026
a9fbe7c
fix(sdk-ffi): map DriveInternalError to dedicated FFI error code
lklimek Apr 13, 2026
3445940
fix(sdk): fix TC-7 test race and formatting
lklimek Apr 13, 2026
8c85455
Revert "fix(sdk): fix TC-7 test race and formatting"
lklimek Apr 13, 2026
32b661c
Revert "test(sdk): add TC-6 concurrent updates and TC-7 global DPP ve…
lklimek Apr 13, 2026
60c4d32
Revert "feat(sdk): auto-detect protocol version from network response…
lklimek Apr 13, 2026
86fbeea
Merge branch 'v3.1-dev' into fix/sdk-cbor-security-hardening
lklimek Apr 15, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 192 additions & 4 deletions packages/rs-dapi/src/services/platform_service/error_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,31 @@ pub(super) fn decode_consensus_error(info_base64: String) -> Option<Vec<u8>> {
Some(serialized_error)
}

/// Try to decode a Tenderdash `data` field as base64 → CBOR and extract the
/// human-readable `message` text. Returns `None` if the string is not
/// base64-encoded CBOR or does not contain a `message` key, allowing the
/// caller to fall back to the raw string.
fn decode_data_message(data: &str) -> Option<String> {
// Silently try base64 — failure is expected for plain-text data fields,
// so we intentionally avoid `base64_decode()` which logs at debug level.
let decoded_bytes = engine::GeneralPurpose::new(
&base64::alphabet::STANDARD,
engine::GeneralPurposeConfig::new()
.with_decode_allow_trailing_bits(true)
.with_decode_padding_mode(engine::DecodePaddingMode::Indifferent),
)
.decode(data)
.ok()?;

Comment on lines +280 to +291
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decode_data_message duplicates the base64 engine configuration that already exists in base64_decode(). To avoid config drift and make future changes safer, consider reusing the same engine (e.g., a shared static/helper) while still keeping this path silent (no debug logging) for expected non-base64 inputs.

Suggested change
fn decode_data_message(data: &str) -> Option<String> {
// Silently try base64 — failure is expected for plain-text data fields,
// so we intentionally avoid `base64_decode()` which logs at debug level.
let decoded_bytes = engine::GeneralPurpose::new(
&base64::alphabet::STANDARD,
engine::GeneralPurposeConfig::new()
.with_decode_allow_trailing_bits(true)
.with_decode_padding_mode(engine::DecodePaddingMode::Indifferent),
)
.decode(data)
.ok()?;
/// Shared base64 engine configuration for Tenderdash error payloads.
/// This mirrors the configuration used by other base64 helpers, so that
/// future changes only need to be made in one place.
fn tenderdash_base64_engine() -> engine::GeneralPurpose {
engine::GeneralPurpose::new(
&base64::alphabet::STANDARD,
engine::GeneralPurposeConfig::new()
.with_decode_allow_trailing_bits(true)
.with_decode_padding_mode(engine::DecodePaddingMode::Indifferent),
)
}
fn decode_data_message(data: &str) -> Option<String> {
// Silently try base64 — failure is expected for plain-text data fields,
// so we intentionally avoid `base64_decode()` which logs at debug level.
let decoded_bytes = tenderdash_base64_engine().decode(data).ok()?;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline engine in decode_data_message() is intentional — it avoids the debug! logging in base64_decode() which would fire on every plain-text data field (an expected/normal case). A shared tenderdash_base64_engine() helper as suggested could work, but the two call sites have different logging requirements, so extracting a shared engine without also sharing the silence/log behavior doesn't buy much. Leaving as-is for now; the duplication is minimal (one config block) and well-commented.

🤖 Co-authored by Claudius the Magnificent AI Agent

let raw_value: ciborium::Value = ciborium::de::from_reader(decoded_bytes.as_slice())
.inspect_err(|e| {
tracing::trace!("data field is not CBOR: {}", e);
})
Comment thread
lklimek marked this conversation as resolved.
.ok()?;

walk_cbor_for_key(&raw_value, &["message"]).and_then(|v| v.as_text().map(|s| s.to_string()))
}

impl From<serde_json::Value> for TenderdashStatus {
// Convert from a JSON error object returned by Tenderdash RPC, typically in the `error` field of a JSON-RPC response.
fn from(value: serde_json::Value) -> Self {
Expand All @@ -295,11 +320,10 @@ impl From<serde_json::Value> for TenderdashStatus {
object
.get("data")
.and_then(|d| d.as_str())
.filter(|s| s.is_ascii())
.map(|s| decode_data_message(s).unwrap_or_else(|| s.to_string()))
} else {
raw_message
}
.map(|s| s.to_string());
raw_message.map(|s| s.to_string())
};

// info contains additional error details, possibly including consensus error
let consensus_error = object
Expand Down Expand Up @@ -678,4 +702,168 @@ mod tests {
// "tx already exists in cache" maps to AlreadyExists, which maps to already_exists
assert_eq!(tonic_status.code(), tonic::Code::AlreadyExists);
}

// -- decode_data_message tests --

#[test]
fn decode_data_message_plain_text_returns_none() {
// Plain text that is not base64 CBOR → returns None so the caller
// can fall back to using the raw string.
assert!(super::decode_data_message("just plain text").is_none());
}

#[test]
fn decode_data_message_base64_cbor_with_message() {
// CBOR: {"message": "hello world"}
let cbor_bytes = {
let mut buf = Vec::new();
ciborium::ser::into_writer(
&ciborium::Value::Map(vec![(
ciborium::Value::Text("message".to_string()),
ciborium::Value::Text("hello world".to_string()),
)]),
&mut buf,
)
.unwrap();
buf
};
let b64 = base64::prelude::BASE64_STANDARD.encode(&cbor_bytes);
assert_eq!(
super::decode_data_message(&b64),
Some("hello world".to_string())
);
}

#[test]
fn decode_data_message_base64_cbor_without_message_key() {
// CBOR: {"data": {"serializedError": [1, 2, 3]}} — no "message" key
let cbor_bytes = {
let mut buf = Vec::new();
ciborium::ser::into_writer(
&ciborium::Value::Map(vec![(
ciborium::Value::Text("data".to_string()),
ciborium::Value::Map(vec![(
ciborium::Value::Text("serializedError".to_string()),
ciborium::Value::Array(vec![
ciborium::Value::Integer(1.into()),
ciborium::Value::Integer(2.into()),
ciborium::Value::Integer(3.into()),
]),
)]),
)]),
&mut buf,
)
.unwrap();
buf
};
let b64 = base64::prelude::BASE64_STANDARD.encode(&cbor_bytes);
assert!(super::decode_data_message(&b64).is_none());
}

// -- Real-world DET log fixture tests --

#[test]
fn from_json_value_decodes_cbor_data_field_non_unique_key() {
setup_tracing();
// Real fixture from DET logs: code 13 Internal, data is base64 CBOR
// containing {"message": "storage: identity: a unique key ... non unique set [...]"}
let data_b64 = concat!(
"oWdtZXNzYWdleMVzdG9yYWdlOiBpZGVudGl0eTogYSB1bmlxdWUga2V5IHdpdGggdGhh",
"dCBoYXNoIGFscmVhZHkgZXhpc3RzOiB0aGUga2V5IGFscmVhZHkgZXhpc3RzIGluIHRo",
"ZSBub24gdW5pcXVlIHNldCBbMTM1LCAyMDIsIDE3MiwgNTMsIDE3NiwgNDUsIDE5MSwg",
"MjcsIDUwLCAxMiwgNTAsIDIxNSwgNjUsIDEyNCwgMTQ3LCAzLCAyMDgsIDYsIDIyNiwg",
"MTUxXQ==",
);

let value = serde_json::json!({
"code": 13,
"message": "Internal error",
"data": data_b64,
"info": ""
});

let status = TenderdashStatus::from(value);
assert_eq!(status.code, 13);
assert!(
status
.message
.as_deref()
.expect("message should be decoded")
.starts_with("storage: identity: a unique key with that hash already exists"),
"expected decoded message, got: {:?}",
status.message
);
assert!(
status
.message
.as_deref()
.unwrap()
.contains("non unique set"),
);
assert!(status.consensus_error.is_none());
}

#[test]
fn from_json_value_decodes_cbor_data_field_unique_key() {
setup_tracing();
// Real fixture from DET logs: code 13 Internal, "unique set" variant
let data_b64 = concat!(
"oWdtZXNzYWdleMdzdG9yYWdlOiBpZGVudGl0eTogYSB1bmlxdWUga2V5IHdpdGggdGhh",
"dCBoYXNoIGFscmVhZHkgZXhpc3RzOiB0aGUga2V5IGFscmVhZHkgZXhpc3RzIGluIHRo",
"ZSB1bmlxdWUgc2V0IFsyMzIsIDQ4LCAxMTksIDEzNywgMTYxLCAxNDMsIDE1LCAxNzks",
"IDIzNSwgOTgsIDEwMSwgMjUxLCAyNTEsIDExMCwgMTMyLCAzNSwgMTE5LCA4NCwgMTQ3",
"LCAxMjRd",
);

let value = serde_json::json!({
"code": 13,
"message": "Internal error",
"data": data_b64,
"info": ""
});

let status = TenderdashStatus::from(value);
assert_eq!(status.code, 13);
assert!(
status
.message
.as_deref()
.expect("message should be decoded")
.starts_with("storage: identity: a unique key with that hash already exists"),
"expected decoded message, got: {:?}",
status.message
);
assert!(status.message.as_deref().unwrap().contains("unique set"),);
assert!(status.consensus_error.is_none());
}

#[test]
fn from_json_value_preserves_plain_text_data() {
// When data is plain text (not base64 CBOR), preserve it as-is
let value = serde_json::json!({
"code": 13,
"message": "Internal error",
"data": "plain text error detail"
});

let status = TenderdashStatus::from(value);
assert_eq!(status.message.as_deref(), Some("plain text error detail"));
}
Comment thread
lklimek marked this conversation as resolved.

#[test]
fn from_json_value_preserves_base64_non_cbor_data() {
// data field that is valid base64 but decodes to non-CBOR bytes.
// decode_data_message should return None → fall back to raw string.
let raw_bytes = b"this is not CBOR at all";
let b64 = base64::prelude::BASE64_STANDARD.encode(raw_bytes);

let value = serde_json::json!({
"code": 13,
"message": "Internal error",
"data": b64
});

let status = TenderdashStatus::from(value);
assert_eq!(status.message.as_deref(), Some(b64.as_str()));
}
}
43 changes: 43 additions & 0 deletions packages/rs-sdk/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ pub enum Error {
#[error("Identity nonce not found on platform: {0}")]
IdentityNonceNotFound(String),

/// Drive returned an internal error that was not classified as a consensus
/// error. Contains the decoded human-readable message from the
/// `drive-error-data-bin` gRPC metadata (CBOR → message extraction).
///
/// This typically indicates a storage-level error (e.g., GroveDB constraint
/// violation) that bypassed the consensus validation layer. If pre-validation
/// is working correctly, these should be rare.
#[error("Drive internal error: {0}")]
DriveInternalError(String),
Comment on lines +94 to +102
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Adding DriveInternalError here leaves the FFI bridge returning NetworkError

rs-sdk-ffi/src/error.rs still classifies FFIError::SDKError by substring matching on sdk_err.to_string(). The new DriveInternalError variant formats as "Drive internal error: ...", which matches none of the existing branches and therefore falls through to the default DashSDKErrorCode::NetworkError / "Failed to fetch balances: ..." path. That means the exact duplicate-key / storage errors this PR is trying to decode cleanly are still mislabeled once they cross the C/Swift boundary. Please update the FFI mapper for the new variant (or match the enum directly instead of stringifying it) so all SDK surfaces report the same error class.

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-sdk/src/error.rs`:
- [SUGGESTION] lines 94-102: Adding `DriveInternalError` here leaves the FFI bridge returning `NetworkError`
  `rs-sdk-ffi/src/error.rs` still classifies `FFIError::SDKError` by substring matching on `sdk_err.to_string()`. The new `DriveInternalError` variant formats as `"Drive internal error: ..."`, which matches none of the existing branches and therefore falls through to the default `DashSDKErrorCode::NetworkError` / `"Failed to fetch balances: ..."` path. That means the exact duplicate-key / storage errors this PR is trying to decode cleanly are still mislabeled once they cross the C/Swift boundary. Please update the FFI mapper for the new variant (or match the enum directly instead of stringifying it) so all SDK surfaces report the same error class.


Comment thread
lklimek marked this conversation as resolved.
Comment thread
lklimek marked this conversation as resolved.
/// Generic error
// TODO: Use domain specific errors instead of generic ones
#[error("SDK error: {0}")]
Expand Down Expand Up @@ -184,6 +194,20 @@ impl From<DapiClientError> for Error {
Self::Generic(format!("Invalid consensus error encoding: {e}"))
});
}
// Check drive-error-data-bin for decoded Drive error messages
if status.code() == Code::Internal {
if let Some(drive_error_value) = status
.metadata()
.get_bin("drive-error-data-bin")
{
if let Ok(bytes) = drive_error_value.to_bytes() {
if let Some(message) = extract_drive_error_message(&bytes) {
Comment thread
lklimek marked this conversation as resolved.
Outdated
return Self::DriveInternalError(message);
}
}
}
}
Comment thread
lklimek marked this conversation as resolved.

Comment thread
lklimek marked this conversation as resolved.
// Otherwise we parse the error code and act accordingly
if status.code() == Code::AlreadyExists {
return Self::AlreadyExists(status.message().to_string());
Expand All @@ -195,6 +219,25 @@ impl From<DapiClientError> for Error {
}
}

/// Extract the human-readable `message` field from CBOR-encoded `drive-error-data-bin` metadata.
///
/// The metadata contains a CBOR map with optional fields: `code`, `message`, `consensus_error`.
/// Returns `Some(message)` if the CBOR decodes and contains a non-empty `message` string.
fn extract_drive_error_message(bytes: &[u8]) -> Option<String> {
let value: ciborium::Value = ciborium::from_reader(bytes).ok()?;
let map = value.as_map()?;
for (key, val) in map {
if key.as_text() == Some("message") {
if let Some(msg) = val.as_text() {
if !msg.is_empty() {
return Some(msg.to_string());
}
}
}
}
None
}

impl From<PlatformVersionError> for Error {
fn from(value: PlatformVersionError) -> Self {
Self::Protocol(value.into())
Expand Down
Loading