Skip to content

fix(kvstore): percent-encode keys and list items in request paths #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mohith26 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(kvstore): percent-encode keys and list items in request paths #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
119 changes: 82 additions & 37 deletions crates/core/src/kvstore/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,21 @@ impl ResolvedKvStoreConfig {
}
Ok(Self { base_url })
}

/// Builds an endpoint URL from path segments. Each segment is
/// percent-encoded, so a key or list item containing `/`, `?`, `#` or
/// spaces stays a single path segment instead of changing the path,
/// starting a query string or being cut off.
fn url(&self, segments: &[&str]) -> Result<reqwest::Url, SdkError> {
let mut url = self.base_url.clone();
url.path_segments_mut()
.map_err(|()| {
SdkError::Config("KV store base URL does not support path segments".into())
})?
.pop_if_empty()
.extend(segments);
Ok(url)
}
}

// ── Request types ──────────────────────────────────────────────────────────
Expand Down Expand Up @@ -403,11 +418,7 @@ impl KvStoreApiClient {

/// Returns the string value stored for a single set by key.
pub async fn get_set(&self, key: &str) -> Result<GetSetResponse, SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("sets/{key}"))?;
let url = self.config.kvstore().url(&["sets", key])?;
let resp = self
.config
.http_client()
Expand Down Expand Up @@ -447,11 +458,7 @@ impl KvStoreApiClient {

/// Removes a single set by key.
pub async fn delete_set(&self, key: &str) -> Result<(), SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("sets/{key}"))?;
let url = self.config.kvstore().url(&["sets", key])?;
let resp = self
.config
.http_client()
Expand Down Expand Up @@ -523,11 +530,7 @@ impl KvStoreApiClient {
key: &str,
params: &GetListParams,
) -> Result<GetListResponse, SdkError> {
let mut url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}"))?;
let mut url = self.config.kvstore().url(&["lists", key])?;
{
let mut pairs = url.query_pairs_mut();
if let Some(v) = params.limit {
Expand Down Expand Up @@ -555,11 +558,7 @@ impl KvStoreApiClient {
/// Updates an existing list by adding and/or removing items in a single
/// operation. Either `add_items`, `remove_items`, or both may be supplied.
pub async fn update_list(&self, key: &str, params: &UpdateListParams) -> Result<(), SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}"))?;
let url = self.config.kvstore().url(&["lists", key])?;
let resp = self
.config
.http_client()
Expand All @@ -582,11 +581,7 @@ impl KvStoreApiClient {
key: &str,
params: &AddListItemParams,
) -> Result<(), SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}/items"))?;
let url = self.config.kvstore().url(&["lists", key, "items"])?;
let resp = self
.config
.http_client()
Expand All @@ -612,8 +607,7 @@ impl KvStoreApiClient {
let url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}/contains/{item}"))?;
.url(&["lists", key, "contains", item])?;
let resp = self
.config
.http_client()
Expand All @@ -633,11 +627,7 @@ impl KvStoreApiClient {

/// Removes a specific item from the list identified by `key`.
pub async fn delete_list_item(&self, key: &str, item: &str) -> Result<(), SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}/items/{item}"))?;
let url = self.config.kvstore().url(&["lists", key, "items", item])?;
let resp = self
.config
.http_client()
Expand All @@ -655,11 +645,7 @@ impl KvStoreApiClient {

/// Removes a list and all of its items by key.
pub async fn delete_list(&self, key: &str) -> Result<(), SdkError> {
let url = self
.config
.kvstore()
.base_url
.join(&format!("lists/{key}"))?;
let url = self.config.kvstore().url(&["lists", key])?;
let resp = self
.config
.http_client()
Expand Down Expand Up @@ -853,6 +839,23 @@ mod tests {
assert_eq!(resp.value, "my-value");
}

#[tokio::test]
async fn get_set_key_is_one_encoded_path_segment() {
let server = MockServer::start().await;
Mock::given(method("GET"))
.and(path("/sets/user:1%2Fprofile%3Fv=2"))
.respond_with(
ResponseTemplate::new(200)
.set_body_json(serde_json::json!({"data": {"value": "my-value"}})),
)
.expect(1)
.mount(&server)
.await;
let sdk = make_sdk(format!("{}/", server.uri()));
let resp = sdk.kvstore.get_set("user:1/profile?v=2").await.unwrap();
assert_eq!(resp.value, "my-value");
}

#[tokio::test]
async fn get_set_not_found() {
let server = MockServer::start().await;
Expand Down Expand Up @@ -1401,6 +1404,29 @@ mod tests {
assert!(resp.exists);
}

#[tokio::test]
async fn list_contains_item_encodes_key_and_item() {
let server = MockServer::start().await;
Mock::given(method("GET"))
.and(path(
"/lists/my%20list/contains/https:%2F%2Fexample.com%2Fa%3Fb=1%23top",
))
.respond_with(
ResponseTemplate::new(200)
.set_body_json(serde_json::json!({"data": {"exists": true}})),
)
.expect(1)
.mount(&server)
.await;
let sdk = make_sdk(format!("{}/", server.uri()));
let resp = sdk
.kvstore
.list_contains_item("my list", "https://example.com/a?b=1#top")
.await
.unwrap();
assert!(resp.exists);
}

#[tokio::test]
async fn list_contains_item_not_found() {
let server = MockServer::start().await;
Expand Down Expand Up @@ -1453,6 +1479,25 @@ mod tests {
.unwrap();
}

#[tokio::test]
async fn delete_list_item_cannot_escape_the_list() {
let server = MockServer::start().await;
Mock::given(method("DELETE"))
.and(path("/lists/my-list/items/..%2F..%2Fsets%2Fsecret"))
.respond_with(
ResponseTemplate::new(200)
.set_body_json(serde_json::json!({"code": 0, "msg": "ok", "data": null})),
)
.expect(1)
.mount(&server)
.await;
let sdk = make_sdk(format!("{}/", server.uri()));
sdk.kvstore
.delete_list_item("my-list", "../../sets/secret")
.await
.unwrap();
}

#[tokio::test]
async fn delete_list_item_not_found() {
let server = MockServer::start().await;
Expand Down