From 969825a13f54a98aa4e6fba259a3204dab593882 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 16 Aug 2026 12:30:00 +0000 Subject: [PATCH 1/2] fix: validate secondary-index keys on UpdateItem UpdateItem never validated the item it produced against the table's secondary index keys, so an update expression could set an index key to an empty string or binary value, or to a value of the wrong type, and the write silently succeeded. The transactional Update path already performed exactly these checks, so plain UpdateItem was the only way through. The validator itself was already correct and already named the offending value's type; only the caller was missing. UpdateItem now runs the same two checks the transactional path runs, against the evaluated post-update image rather than the expression, which is what covers if_not_exists and list_append whose result is not knowable from the expression alone. index_key_refs becomes pub(crate) so the two paths share one construction of the index key references. Verified against a live server: setting a GSI hash key to an empty binary value via an update expression previously succeeded and now returns ValidationException with the update-expression message variant. tests/rust/src/index_key_validation.rs gains the two UpdateItem cases, a rejection and a positive control that a valid value is still accepted, so a change that rejected everything could not pass. All six tests in that file pass. --- .../storage-postgres/src/data/transactions.rs | 2 +- .../storage-postgres/src/data/update_item.rs | 16 ++++ tests/rust/src/index_key_validation.rs | 74 +++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) diff --git a/crates/storage-postgres/src/data/transactions.rs b/crates/storage-postgres/src/data/transactions.rs index 5e0980466..e3192b564 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -255,7 +255,7 @@ fn transact_op_table_id<'a>(op: &'a TransactWriteOp<'_>) -> &'a str { /// cancellation reasons (BLOCKER #3 fix). /// Build [`validation::IndexKeyRef`] views over the table's indexes for /// secondary-index key validation. -fn index_key_refs(indexes: &[IndexMeta]) -> Vec> { +pub(crate) fn index_key_refs(indexes: &[IndexMeta]) -> Vec> { indexes .iter() .map(|idx| validation::IndexKeyRef { diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index b374465fa..897e3c2aa 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -158,6 +158,22 @@ impl PostgresEngine { validation::validate_item_size(&item, self.max_item_size_bytes) .map_err(|e| StorageError::Validation(e.to_string()))?; + // Secondary-index key validation on the post-update item, matching + // the transactional Update path: an update expression must not set + // an index key to a mismatched type, nor to an empty string or + // binary value. Validating the evaluated image rather than the + // expression is what covers if_not_exists and list_append, whose + // result is not knowable from the expression alone. + let idx_refs = super::transactions::index_key_refs(&indexes); + validation::validate_index_key_types(&item, &idx_refs, &key_info.attribute_definitions) + .map_err(|e| StorageError::Validation(e.to_string()))?; + validation::validate_index_key_not_empty( + &item, + &idx_refs, + validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + let new_item = if return_new { Some(item.clone()) } else { None }; let item_json = diff --git a/tests/rust/src/index_key_validation.rs b/tests/rust/src/index_key_validation.rs index f0b4e25e4..ca645e665 100644 --- a/tests/rust/src/index_key_validation.rs +++ b/tests/rust/src/index_key_validation.rs @@ -161,3 +161,77 @@ async fn put_item_rejects_empty_binary_index_key_reports_binary() { "expected type-correct 'empty binary value' message, got: {m}" ); } + +/// Control for the rejection above: a normal update that leaves the index key +/// valid must still succeed. +#[tokio::test] +async fn update_item_accepts_setting_an_index_key_to_a_valid_value() { + let c = client(); + let t = tables().await; + let key_value = format!("idxupdok_{}", ts()); + + let mut item: HashMap = HashMap::new(); + item.insert(HASH_KEY_S.into(), s(&key_value)); + item.insert(GSI_HASH_KEY.into(), s("h")); + item.insert(GSI_RANGE_KEY.into(), s("r")); + c.put_item() + .table_name(&t.simple_key_string_gsi) + .set_item(Some(item)) + .send() + .await + .expect("seed item must be accepted"); + + c.update_item() + .table_name(&t.simple_key_string_gsi) + .key(HASH_KEY_S, s(&key_value)) + .update_expression("SET #g = :v") + .expression_attribute_names("#g", GSI_HASH_KEY) + .expression_attribute_values(":v", s("h2")) + .send() + .await + .expect("a valid index-key update must be accepted"); +} + +/// An update expression that sets a secondary-index key to an empty value must +/// be rejected, the same as writing that value with PutItem. Before this was +/// enforced, UpdateItem accepted the write and stored an unindexable key. +#[tokio::test] +async fn update_item_rejects_setting_an_index_key_to_an_empty_value() { + let c = client(); + let t = tables().await; + let key_value = format!("idxupde_{}", ts()); + + // Seed a row with a valid index key so the update targets an existing item. + let mut item: HashMap = HashMap::new(); + item.insert(HASH_KEY_S.into(), s(&key_value)); + item.insert(GSI_HASH_KEY.into(), s("h")); + item.insert(GSI_RANGE_KEY.into(), s("r")); + c.put_item() + .table_name(&t.simple_key_string_gsi) + .set_item(Some(item)) + .send() + .await + .expect("seed item must be accepted"); + + let err = c + .update_item() + .table_name(&t.simple_key_string_gsi) + .key(HASH_KEY_S, s(&key_value)) + .update_expression("SET #g = :empty") + .expression_attribute_names("#g", GSI_HASH_KEY) + .expression_attribute_values(":empty", s("")) + .send() + .await + .expect_err("setting an index key to an empty value must be rejected"); + assert_eq!( + err_code(&err), + Some("ValidationException"), + "{}", + err_msg(&err) + ); + let m = err_msg(&err); + assert!( + m.contains("update expression attempted to update a secondary index key"), + "expected the update-expression index-key message, got: {m}" + ); +} From 44af361d8cc4a5ba6c12fbc38da1c33184c90a7f Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 16 Aug 2026 17:04:58 +0000 Subject: [PATCH 2/2] fix: report the update-expression message on MongoDB index-key rejections mongodb-rust failed update_item_rejects_setting_an_index_key_to_an_empty_value. The MongoDB backend did reject the write, so this was not a missing check: it reported the wrong message. validate_index_keys hardcodes SecondaryIndexEmptyContext::Item, and MongoDB used it on paths that validate the evaluated result of an update expression, so the rejection named the attribute and index as though the caller had supplied the item directly. DynamoDB words the two cases differently. An item supplied directly reports the attribute and index by name; a value produced by an update expression reports that the update expression attempted the change and names neither. A path that evaluates an expression and then validates the result has to say the second thing or it rejects for the right reason with the wrong message. validate_index_keys_in_context takes the variant explicitly, and validate_index_keys keeps its signature by delegating with the Item context, so the PostgreSQL and SQLite put paths are untouched. MongoDB's two update paths, UpdateItem and the Update leg of TransactWriteItems, now pass UpdateExpression; its two put paths stay on Item. The transact leg was not covered by a failing test but is the same defect, and the PostgreSQL equivalent is already correct. Verified on both backends that run tests/rust in CI, 6/6 each, including the positive control that a valid value is still accepted: mongodb via devtools/run-mongodb-tests, the path CI uses postgres unchanged, confirming the delegation preserved its behaviour 808 workspace unit tests pass with 0 filtered out, fmt and clippy --all-targets clean. --- crates/core/src/validation/mod.rs | 21 +++++++++++++++- crates/storage-mongodb/src/data_engine.rs | 30 +++++++++++++++++------ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index 7e4ddd8c8..fa75c6f39 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -997,9 +997,28 @@ pub fn validate_index_keys( item: &Item, indexes: &[IndexKeyRef<'_>], attr_defs: &[AttributeDefinition], +) -> Result<(), DynamoDbError> { + validate_index_keys_in_context(item, indexes, attr_defs, SecondaryIndexEmptyContext::Item) +} + +/// [`validate_index_keys`] with the caller choosing the empty-value message +/// variant. +/// +/// DynamoDB words the rejection differently depending on how the offending value +/// arrived. An item supplied directly reports the attribute and index by name; a +/// value produced by an update expression reports that the update expression +/// attempted the change and names neither. A path that evaluates an update +/// expression and then validates the resulting item must therefore pass +/// [`SecondaryIndexEmptyContext::UpdateExpression`], or it rejects for the right +/// reason with the wrong message. +pub fn validate_index_keys_in_context( + item: &Item, + indexes: &[IndexKeyRef<'_>], + attr_defs: &[AttributeDefinition], + context: SecondaryIndexEmptyContext, ) -> Result<(), DynamoDbError> { validate_index_key_types(item, indexes, attr_defs)?; - validate_index_key_not_empty(item, indexes, SecondaryIndexEmptyContext::Item) + validate_index_key_not_empty(item, indexes, context) } /// Indexes sorted by name, so the alphabetically-first index that uses a diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index eb58495dd..6f7c735fa 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -252,7 +252,12 @@ impl MongoEngine { // work so the caller sees a top-level ValidationException on // wrong-type or empty index-key attributes (D-M10, RFC-0003 // §2.3). - self.validate_index_keys_for_item(key_info, &item).await?; + self.validate_index_keys_for_item( + key_info, + &item, + extenddb_core::validation::SecondaryIndexEmptyContext::Item, + ) + .await?; let coll_name = data_collection_name(&key_info.table_id); let coll = self.data_db.collection::(&coll_name); @@ -816,9 +821,13 @@ impl MongoEngine { // the resulting item — D-M10, RFC-0003 §2.3. Same // shape as put_item's up-front check, but the // post-update item is what actually gets written. - self.validate_index_keys_for_item(key_info, &new_item) - .await - .map_err(TxErr::Fatal)?; + self.validate_index_keys_for_item( + key_info, + &new_item, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .await + .map_err(TxErr::Fatal)?; let mut new_doc = item_to_document( &new_item, @@ -1702,6 +1711,7 @@ impl MongoEngine { &self, key_info: &TableKeyInfo, item: &Item, + context: extenddb_core::validation::SecondaryIndexEmptyContext, ) -> Result<(), StorageError> { let idx_pairs = self.fetch_index_key_schemas(&key_info.table_id).await?; if idx_pairs.is_empty() { @@ -1714,8 +1724,13 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys(item, &refs, &key_info.attribute_definitions) - .map_err(|e| StorageError::Validation(e.to_string())) + extenddb_core::validation::validate_index_keys_in_context( + item, + &refs, + &key_info.attribute_definitions, + context, + ) + .map_err(|e| StorageError::Validation(e.to_string())) } async fn sync_indexes_in_session( @@ -2542,10 +2557,11 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys( + extenddb_core::validation::validate_index_keys_in_context( &item, &idx_refs, &key_info.attribute_definitions, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, ) .map_err(|e| { TransactOpError::Cancel(CancellationReason::validation_error(e.to_string()))