From 3ae3390cda73a3c2106185103601bf5f8e0add74 Mon Sep 17 00:00:00 2001 From: Philip Kannegaard Hayes Date: Wed, 2 Sep 2026 02:18:19 +0000 Subject: [PATCH] pg: fix list_key_versions interpreting keys as patterns Using `key LIKE $3` unescaped tells Postgres to interpret the requested key prefix as a pattern, which means %, _, and \ are interpreted as wildcards and character escapes in the original prefix rather the actual intended literal prefix. This means a `list_key_versions(key="foo_bar", ..)` actually includes `foo1bar`, which definitely does not seem intentional. A better approach is to just use `starts_with(key, $3)`, which the query planner understands better and doesn't require us to do any escaping. --- api/src/kv_store_tests.rs | 46 +++++++++++++++++++++++++++++++++++++ impls/src/postgres_store.rs | 43 +++++++++++++++++----------------- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/api/src/kv_store_tests.rs b/api/src/kv_store_tests.rs index 6d3792b..15a4a7b 100644 --- a/api/src/kv_store_tests.rs +++ b/api/src/kv_store_tests.rs @@ -51,6 +51,7 @@ macro_rules! define_kv_store_tests { create_test!(get_should_return_correct_value_when_key_exists); create_test!(list_should_return_paginated_response); create_test!(list_should_honour_page_size_and_key_prefix_if_provided); + create_test!(list_should_treat_key_prefix_as_a_literal_string); create_test!(list_should_return_zero_global_version_when_global_versioning_not_enabled); create_test!(list_should_limit_max_page_size); create_test!(list_should_return_results_ordered_by_creation_time); @@ -467,6 +468,51 @@ pub trait KvStoreTestSuite { Ok(()) } + async fn list_should_treat_key_prefix_as_a_literal_string() -> Result<(), VssError> { + let kv_store = Self::create_store().await; + let ctx = TestContext::new(&kv_store); + + let stored_keys = [ + "percent%older", + "percent-false-match", + "percent%newer", + "underscore_older", + "underscoreXfalse-match", + "underscore_newer", + "backslash\\older", + "backslash%", + "backslash\\newer", + ]; + ctx.put_objects(Some(0), stored_keys.into_iter().map(|key| kv(key, "v1", 0)).collect()) + .await?; + + async fn assert_list_eq(ctx: &TestContext<'_>, prefix: &str, expected: &[&str]) { + for page_size in [100, 1] { + let mut page_token = None; + let mut actual = Vec::new(); + loop { + let resp = ctx + .list(page_token, Some(page_size), Some(prefix.to_owned())) + .await + .unwrap(); + assert!(resp.key_versions.len() <= page_size as usize); + actual.extend(resp.key_versions.into_iter().map(|key_version| key_version.key)); + page_token = resp.next_page_token.filter(|token| !token.is_empty()); + if page_token.is_none() { + break; + } + } + assert_eq!(actual, expected, "prefix='{prefix}', page_size={page_size}"); + } + } + + assert_list_eq(&ctx, "percent%", &["percent%newer", "percent%older"]).await; + assert_list_eq(&ctx, "underscore_", &["underscore_newer", "underscore_older"]).await; + assert_list_eq(&ctx, "backslash\\", &["backslash\\newer", "backslash\\older"]).await; + + Ok(()) + } + async fn list_should_return_zero_global_version_when_global_versioning_not_enabled() -> Result<(), VssError> { let kv_store = Self::create_store().await; diff --git a/impls/src/postgres_store.rs b/impls/src/postgres_store.rs index 41dd921..efa4792 100644 --- a/impls/src/postgres_store.rs +++ b/impls/src/postgres_store.rs @@ -657,7 +657,7 @@ where &self, user_token: String, request: ListKeyVersionsRequest, ) -> Result { let store_id = &request.store_id; - let key_prefix = &request.key_prefix; + let key_prefix = request.key_prefix.as_deref().unwrap_or_default(); let page_token = &request.page_token; let page_size = request.page_size.unwrap_or(i32::MAX); @@ -691,29 +691,30 @@ where let conn = self.pool.get().await?; - let key_like = format!("{}%", key_prefix.as_deref().unwrap_or_default()); - let rows = if let Some(token) = page_token { let page_sort_order = decode_page_token(token)?; - let stmt = "SELECT key, version, sort_order FROM vss_db WHERE user_token = $1 AND store_id = $2 AND sort_order < $3 AND key LIKE $4 AND key != $5 ORDER BY sort_order DESC LIMIT $6"; - let params: Vec<&(dyn tokio_postgres::types::ToSql + Sync)> = vec![ - &user_token, - &store_id, - &page_sort_order, - &key_like, - &GLOBAL_VERSION_KEY, - &fetch_limit, - ]; - conn.query(stmt, ¶ms) - .await - .map_err(|e| Error::new(ErrorKind::Other, format!("Query error: {}", e)))? + let stmt = "SELECT key, version, sort_order FROM vss_db WHERE user_token = $1 AND store_id = $2 AND sort_order < $3 AND starts_with(key, $4) AND key != $5 ORDER BY sort_order DESC LIMIT $6"; + conn.query( + stmt, + &[ + &user_token, + &store_id, + &page_sort_order, + &key_prefix, + &GLOBAL_VERSION_KEY, + &fetch_limit, + ], + ) + .await + .map_err(|e| Error::new(ErrorKind::Other, format!("Query error: {}", e)))? } else { - let stmt = "SELECT key, version, sort_order FROM vss_db WHERE user_token = $1 AND store_id = $2 AND key LIKE $3 AND key != $4 ORDER BY sort_order DESC LIMIT $5"; - let params: Vec<&(dyn tokio_postgres::types::ToSql + Sync)> = - vec![&user_token, &store_id, &key_like, &GLOBAL_VERSION_KEY, &fetch_limit]; - conn.query(stmt, ¶ms) - .await - .map_err(|e| Error::new(ErrorKind::Other, format!("Query error: {}", e)))? + let stmt = "SELECT key, version, sort_order FROM vss_db WHERE user_token = $1 AND store_id = $2 AND starts_with(key, $3) AND key != $4 ORDER BY sort_order DESC LIMIT $5"; + conn.query( + stmt, + &[&user_token, &store_id, &key_prefix, &GLOBAL_VERSION_KEY, &fetch_limit], + ) + .await + .map_err(|e| Error::new(ErrorKind::Other, format!("Query error: {}", e)))? }; let limit_usize = limit as usize;