From cae140bb8cb965e1283bf057373c87e7aa4fb023 Mon Sep 17 00:00:00 2001 From: QuakeWang Date: Wed, 8 Jul 2026 20:06:08 +0800 Subject: [PATCH 1/3] fix(services): encode version id for cos and tos Signed-off-by: QuakeWang --- core/services/cos/src/core.rs | 274 +++++++++++++++++++++++++++++++++- core/services/tos/src/core.rs | 99 +++++++++++- 2 files changed, 365 insertions(+), 8 deletions(-) diff --git a/core/services/cos/src/core.rs b/core/services/cos/src/core.rs index 20dcb6af3ca7..36ed6201bd03 100644 --- a/core/services/cos/src/core.rs +++ b/core/services/cos/src/core.rs @@ -16,10 +16,12 @@ // under the License. use std::fmt::Debug; +use std::str::FromStr; use bytes::Bytes; use http::Request; use http::Response; +use http::Uri; use http::header::CACHE_CONTROL; use http::header::CONTENT_DISPOSITION; use http::header::CONTENT_LENGTH; @@ -76,6 +78,7 @@ impl CosCore { } pub async fn sign(&self, ctx: &OperationContext, req: Request) -> Result> { + let encoded_query = req.uri().query().map(str::to_string); let (mut parts, body) = req.into_parts(); self.signer(ctx) @@ -83,6 +86,8 @@ impl CosCore { .await .map_err(|e| new_request_sign_error(e.into()))?; + Self::restore_encoded_query(&mut parts.uri, encoded_query.as_deref(), false)?; + Ok(Request::from_parts(parts, body)) } @@ -92,6 +97,7 @@ impl CosCore { req: Request, duration: Duration, ) -> Result> { + let encoded_query = req.uri().query().map(str::to_string); let (mut parts, body) = req.into_parts(); self.signer(ctx) @@ -99,9 +105,72 @@ impl CosCore { .await .map_err(|e| new_request_sign_error(e.into()))?; + Self::restore_encoded_query(&mut parts.uri, encoded_query.as_deref(), true)?; + Ok(Request::from_parts(parts, body)) } + fn restore_encoded_query( + uri: &mut Uri, + encoded_query: Option<&str>, + keep_signed_query: bool, + ) -> Result<()> { + let Some(encoded_query) = encoded_query else { + return Ok(()); + }; + + let Some(path_and_query) = uri.path_and_query() else { + return Ok(()); + }; + let Some(signed_query) = path_and_query.query() else { + return Ok(()); + }; + + let rest = if keep_signed_query { + let decoded_query = Self::decode_signed_query(encoded_query); + let rest = signed_query + .strip_prefix(&decoded_query) + .ok_or_else(|| Error::new(ErrorKind::Unexpected, "signed query changed"))?; + rest.strip_prefix('&').filter(|v| !v.is_empty()) + } else { + None + }; + + let query = if let Some(rest) = rest { + format!("{encoded_query}&{rest}") + } else { + encoded_query.to_string() + }; + + let path_and_query = format!("{}?{}", path_and_query.path(), query); + + let mut parts = std::mem::take(uri).into_parts(); + parts.path_and_query = Some(http::uri::PathAndQuery::from_str(&path_and_query).map_err( + |e| Error::new(ErrorKind::Unexpected, "invalid signed query").set_source(e), + )?); + *uri = Uri::from_parts(parts).map_err(|e| { + Error::new(ErrorKind::Unexpected, "failed to restore signed query").set_source(e) + })?; + + Ok(()) + } + + fn decode_signed_query(encoded_query: &str) -> String { + encoded_query + .split('&') + .map(|pair| { + let mut kv = pair.splitn(2, '='); + let key = percent_decode_path(kv.next().unwrap_or_default()); + if let Some(value) = kv.next() { + format!("{key}={}", percent_decode_path(value)) + } else { + key + } + }) + .collect::>() + .join("&") + } + #[inline] pub async fn send( &self, @@ -141,7 +210,7 @@ impl CosCore { query_args.push(format!( "{}={}", constants::COS_QUERY_VERSION_ID, - percent_decode_path(version) + percent_encode_path(version) )) } if !query_args.is_empty() { @@ -255,7 +324,7 @@ impl CosCore { query_args.push(format!( "{}={}", constants::COS_QUERY_VERSION_ID, - percent_decode_path(version) + percent_encode_path(version) )) } if !query_args.is_empty() { @@ -287,6 +356,17 @@ impl CosCore { path: &str, args: &OpDelete, ) -> Result> { + let req = self.cos_delete_object_request(path, args)?; + let req = self.sign(ctx, req).await?; + + self.send(ctx, req).await + } + + pub fn cos_delete_object_request( + &self, + path: &str, + args: &OpDelete, + ) -> Result> { let p = build_abs_path(&self.root, path); let mut url = format!("{}/{}", self.endpoint, percent_encode_path(&p)); @@ -296,7 +376,7 @@ impl CosCore { query_args.push(format!( "{}={}", constants::COS_QUERY_VERSION_ID, - percent_decode_path(version) + percent_encode_path(version) )) } if !query_args.is_empty() { @@ -310,9 +390,8 @@ impl CosCore { .extension(ServiceOperation("DeleteObject")); let req = req.body(Buffer::new()).map_err(new_request_build_error)?; - let req = self.sign(ctx, req).await?; - self.send(ctx, req).await + Ok(req) } pub fn cos_append_object_request( @@ -733,10 +812,195 @@ pub struct ListObjectVersionsOutputDeleteMarker { #[cfg(test)] mod tests { + use std::time::Duration; + use bytes::Buf; + use reqsign_core::ProvideCredentialChain; + use reqsign_tencent_cos::RequestSigner; + use reqsign_tencent_cos::StaticCredentialProvider; use super::*; + const VERSION_WITH_QUERY_RESERVED_CHARS: &str = "a+b=c%25&e"; + const ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS: &str = "a%2Bb%3Dc%2525%26e"; + + fn assert_version_id_query_encoded(uri: &http::Uri) { + let query = uri.query().unwrap_or_default(); + assert!( + query.split('&').any(|pair| { + pair == format!("versionId={ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS}") + }), + "uri query should keep versionId encoded: {uri}" + ); + assert!( + !query.split('&').any(|pair| pair == "e"), + "uri query should not expose versionId suffix as a query parameter: {uri}" + ); + } + + fn test_core() -> CosCore { + CosCore { + info: ServiceInfo::new("cos", "", "test"), + capability: Capability::default(), + bucket: "test".to_string(), + root: "/".to_string(), + endpoint: "https://test.cos.ap-beijing.myqcloud.com".to_string(), + signer: Signer::new( + Context::new(), + ProvideCredentialChain::::new() + .push(StaticCredentialProvider::new("secret_id", "secret_key")), + RequestSigner::new(), + ), + } + } + + #[test] + fn test_get_object_encodes_version_id() { + let req = test_core() + .cos_get_object_request( + "test.txt", + BytesRange::default(), + &OpRead::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.cos.ap-beijing.myqcloud.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } + + #[test] + fn test_head_object_encodes_version_id() { + let req = test_core() + .cos_head_object_request( + "test.txt", + &OpStat::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.cos.ap-beijing.myqcloud.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } + + #[test] + fn test_delete_object_encodes_version_id() { + let req = test_core() + .cos_delete_object_request( + "test.txt", + &OpDelete::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.cos.ap-beijing.myqcloud.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } + + #[tokio::test] + async fn test_signed_get_object_keeps_version_id_encoded() { + let core = test_core(); + let req = core + .cos_get_object_request( + "test.txt", + BytesRange::default(), + &OpRead::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + let req = core + .sign(&OperationContext::new(), req) + .await + .expect("request must be signed"); + + assert_version_id_query_encoded(req.uri()); + } + + #[tokio::test] + async fn test_signed_head_object_keeps_version_id_encoded() { + let core = test_core(); + let req = core + .cos_head_object_request( + "test.txt", + &OpStat::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + let req = core + .sign(&OperationContext::new(), req) + .await + .expect("request must be signed"); + + assert_version_id_query_encoded(req.uri()); + } + + #[tokio::test] + async fn test_signed_delete_object_keeps_version_id_encoded() { + let core = test_core(); + let req = core + .cos_delete_object_request( + "test.txt", + &OpDelete::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + let req = core + .sign(&OperationContext::new(), req) + .await + .expect("request must be signed"); + + assert_version_id_query_encoded(req.uri()); + } + + #[tokio::test] + async fn test_presigned_get_object_keeps_version_id_encoded() { + let core = test_core(); + let req = core + .cos_get_object_request( + "test.txt", + BytesRange::default(), + &OpRead::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + let req = core + .sign_query(&OperationContext::new(), req, Duration::from_secs(3600)) + .await + .expect("request must be presigned"); + + assert_version_id_query_encoded(req.uri()); + } + + #[tokio::test] + async fn test_presigned_head_object_keeps_version_id_encoded() { + let core = test_core(); + let req = core + .cos_head_object_request( + "test.txt", + &OpStat::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + let req = core + .sign_query(&OperationContext::new(), req, Duration::from_secs(3600)) + .await + .expect("request must be presigned"); + + assert_version_id_query_encoded(req.uri()); + } + #[test] fn test_parse_xml() { let bs = bytes::Bytes::from( diff --git a/core/services/tos/src/core.rs b/core/services/tos/src/core.rs index f32066cd35aa..7c0bc31040e3 100644 --- a/core/services/tos/src/core.rs +++ b/core/services/tos/src/core.rs @@ -245,7 +245,7 @@ impl TosCore { query_args.push(format!( "{}={}", constants::TOS_QUERY_VERSION_ID, - percent_decode_path(version) + percent_encode_query(version) )); } if !query_args.is_empty() { @@ -365,7 +365,7 @@ impl TosCore { query_args.push(format!( "{}={}", constants::TOS_QUERY_VERSION_ID, - percent_decode_path(version) + percent_encode_query(version) )); } if !query_args.is_empty() { @@ -419,6 +419,15 @@ impl TosCore { path: &str, args: &OpDelete, ) -> Result> { + let req = self.tos_delete_object_request(path, args)?; + self.send(ctx, req).await + } + + pub fn tos_delete_object_request( + &self, + path: &str, + args: &OpDelete, + ) -> Result> { let p = build_abs_path(&self.root, path); let mut url = format!( @@ -450,7 +459,7 @@ impl TosCore { .body(Buffer::new()) .map_err(new_request_build_error)?; - self.send(ctx, req).await + Ok(req) } pub async fn tos_delete_objects( @@ -1198,6 +1207,90 @@ mod error { pub(super) use error::*; +#[cfg(test)] +mod tests { + use reqsign_core::ProvideCredentialChain; + use reqsign_volcengine_tos::RequestSigner; + + use super::*; + + const VERSION_WITH_QUERY_RESERVED_CHARS: &str = "a+b=c%25&e"; + const ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS: &str = "a%2Bb%3Dc%2525%26e"; + + fn test_core() -> TosCore { + TosCore { + info: ServiceInfo::new("tos", "", "test"), + capability: Capability::default(), + bucket: "test".to_string(), + endpoint: "https://tos-cn-beijing.volces.com".to_string(), + endpoint_domain: "tos-cn-beijing.volces.com".to_string(), + root: "/".to_string(), + default_storage_class: None, + skip_signature: true, + signer: Signer::new( + Context::new(), + ProvideCredentialChain::::new(), + RequestSigner::new("cn-beijing"), + ), + } + } + + #[test] + fn test_get_object_encodes_version_id() { + let req = test_core() + .tos_get_object_request( + "test.txt", + BytesRange::default(), + &OpRead::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.tos-cn-beijing.volces.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } + + #[test] + fn test_head_object_encodes_version_id() { + let req = test_core() + .tos_head_object_request( + "test.txt", + OpStat::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.tos-cn-beijing.volces.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } + + #[test] + fn test_delete_object_encodes_version_id() { + let req = test_core() + .tos_delete_object_request( + "test.txt", + &OpDelete::default().with_version(VERSION_WITH_QUERY_RESERVED_CHARS), + ) + .expect("request must be built"); + + assert_eq!( + req.uri().to_string(), + format!( + "https://test.tos-cn-beijing.volces.com/test.txt?versionId={}", + ENCODED_VERSION_WITH_QUERY_RESERVED_CHARS + ) + ); + } +} + mod utils { use http::HeaderMap; use http::header::ETAG; From 59c25cc1ae9db052e09e2a667943b2b861652b75 Mon Sep 17 00:00:00 2001 From: QuakeWang Date: Wed, 8 Jul 2026 23:52:00 +0800 Subject: [PATCH 2/3] test(behavior): cover versioned object operations Signed-off-by: QuakeWang --- core/tests/behavior/async_delete.rs | 37 ++++++++++++++++++++++------- core/tests/behavior/async_read.rs | 31 ++++++++++++++++-------- core/tests/behavior/async_stat.rs | 9 ++++++- 3 files changed, 58 insertions(+), 19 deletions(-) diff --git a/core/tests/behavior/async_delete.rs b/core/tests/behavior/async_delete.rs index ae441cc54970..933721acb0f8 100644 --- a/core/tests/behavior/async_delete.rs +++ b/core/tests/behavior/async_delete.rs @@ -246,26 +246,47 @@ pub async fn test_delete_with_version(op: Operator) -> Result<()> { op.write(path.as_str(), content) .await .expect("write must success"); - let meta = op.stat(path.as_str()).await.expect("stat must success"); - let version = meta.version().expect("must have version"); + let first_meta = op.stat(path.as_str()).await.expect("stat must success"); + let first_version = first_meta.version().expect("must have version"); + + op.write(path.as_str(), b"second version".to_vec()) + .await + .expect("write must success"); + let second_meta = op.stat(path.as_str()).await.expect("stat must success"); + let second_version = second_meta.version().expect("must have version"); + assert_ne!(first_version, second_version); op.delete(path.as_str()).await.expect("delete must success"); assert!(!op.exists(path.as_str()).await?); - // After a simple delete, the data can still be accessed using its version. + // After a simple delete, both versions can still be accessed using their versions. + let meta = op + .stat_with(path.as_str()) + .version(first_version) + .await + .expect("stat must success"); + assert_eq!(first_version, meta.version().expect("must have version")); let meta = op .stat_with(path.as_str()) - .version(version) + .version(second_version) .await .expect("stat must success"); - assert_eq!(version, meta.version().expect("must have version")); + assert_eq!(second_version, meta.version().expect("must have version")); + + // After deleting with the version, the data is removed permanently. + op.delete_with(path.as_str()) + .version(first_version) + .await + .expect("delete must success"); + let ret = op.stat_with(path.as_str()).version(first_version).await; + assert!(ret.is_err()); + assert_eq!(ret.unwrap_err().kind(), ErrorKind::NotFound); - // After deleting with the version, the data is removed permanently op.delete_with(path.as_str()) - .version(version) + .version(second_version) .await .expect("delete must success"); - let ret = op.stat_with(path.as_str()).version(version).await; + let ret = op.stat_with(path.as_str()).version(second_version).await; assert!(ret.is_err()); assert_eq!(ret.unwrap_err().kind(), ErrorKind::NotFound); diff --git a/core/tests/behavior/async_read.rs b/core/tests/behavior/async_read.rs index feee7c73079c..09d3356c377b 100644 --- a/core/tests/behavior/async_read.rs +++ b/core/tests/behavior/async_read.rs @@ -1069,31 +1069,42 @@ pub async fn test_read_with_version(op: Operator) -> anyhow::Result<()> { return Ok(()); } - let (path, content, _) = TEST_FIXTURE.new_file(op.clone()); - op.write(path.as_str(), content.clone()) + let (path, first_content, _) = TEST_FIXTURE.new_file(op.clone()); + op.write(path.as_str(), first_content.clone()) .await .expect("write must success"); - let meta = op.stat(path.as_str()).await.expect("stat must success"); - let version = meta.version().expect("must have version"); + let first_meta = op.stat(path.as_str()).await.expect("stat must success"); + let first_version = first_meta.version().expect("must have version"); let data = op .read_with(path.as_str()) - .version(version) + .version(first_version) .await .expect("read must success"); - assert_eq!(content, data.to_vec()); + assert_eq!(first_content, data.to_vec()); - op.write(path.as_str(), "1") + let second_content = b"second version"; + op.write(path.as_str(), second_content.to_vec()) .await .expect("write must success"); + let second_meta = op.stat(path.as_str()).await.expect("stat must success"); + let second_version = second_meta.version().expect("must have version"); + assert_ne!(first_version, second_version); // After writing new data, we can still read the first version data - let second_data = op + let first_version_data = op + .read_with(path.as_str()) + .version(first_version) + .await + .expect("read must success"); + assert_eq!(first_content, first_version_data.to_vec()); + + let second_version_data = op .read_with(path.as_str()) - .version(version) + .version(second_version) .await .expect("read must success"); - assert_eq!(content, second_data.to_vec()); + assert_eq!(second_content.to_vec(), second_version_data.to_vec()); Ok(()) } diff --git a/core/tests/behavior/async_stat.rs b/core/tests/behavior/async_stat.rs index 14120089453a..a204db09d0bb 100644 --- a/core/tests/behavior/async_stat.rs +++ b/core/tests/behavior/async_stat.rs @@ -570,7 +570,7 @@ pub async fn test_stat_with_version(op: Operator) -> Result<()> { .expect("stat must success"); assert_eq!(first_meta, first_versioning_meta); - op.write(path.as_str(), content) + op.write(path.as_str(), b"second version".to_vec()) .await .expect("write must success"); let second_meta = op.stat(path.as_str()).await.expect("stat must success"); @@ -585,6 +585,13 @@ pub async fn test_stat_with_version(op: Operator) -> Result<()> { .expect("stat must success"); assert_eq!(first_meta, meta); + let meta = op + .stat_with(path.as_str()) + .version(second_version) + .await + .expect("stat must success"); + assert_eq!(second_meta, meta); + Ok(()) } From 17767977fb9d6e21a544f13cc88393f136ce542c Mon Sep 17 00:00:00 2001 From: QuakeWang Date: Thu, 9 Jul 2026 13:48:47 +0800 Subject: [PATCH 3/3] fix(services): simplify cos query restoration Signed-off-by: QuakeWang --- core/services/cos/src/core.rs | 37 ++++++++++++++--------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/core/services/cos/src/core.rs b/core/services/cos/src/core.rs index 36ed6201bd03..9407c92f4f14 100644 --- a/core/services/cos/src/core.rs +++ b/core/services/cos/src/core.rs @@ -86,7 +86,7 @@ impl CosCore { .await .map_err(|e| new_request_sign_error(e.into()))?; - Self::restore_encoded_query(&mut parts.uri, encoded_query.as_deref(), false)?; + parts.uri = Self::restore_encoded_query(parts.uri, encoded_query.as_deref())?; Ok(Request::from_parts(parts, body)) } @@ -105,36 +105,29 @@ impl CosCore { .await .map_err(|e| new_request_sign_error(e.into()))?; - Self::restore_encoded_query(&mut parts.uri, encoded_query.as_deref(), true)?; + parts.uri = Self::restore_encoded_query(parts.uri, encoded_query.as_deref())?; Ok(Request::from_parts(parts, body)) } - fn restore_encoded_query( - uri: &mut Uri, - encoded_query: Option<&str>, - keep_signed_query: bool, - ) -> Result<()> { + fn restore_encoded_query(uri: Uri, encoded_query: Option<&str>) -> Result { let Some(encoded_query) = encoded_query else { - return Ok(()); + return Ok(uri); }; let Some(path_and_query) = uri.path_and_query() else { - return Ok(()); + return Ok(uri); }; let Some(signed_query) = path_and_query.query() else { - return Ok(()); + return Ok(uri); }; - let rest = if keep_signed_query { - let decoded_query = Self::decode_signed_query(encoded_query); - let rest = signed_query - .strip_prefix(&decoded_query) - .ok_or_else(|| Error::new(ErrorKind::Unexpected, "signed query changed"))?; - rest.strip_prefix('&').filter(|v| !v.is_empty()) - } else { - None - }; + let decoded_query = Self::decode_signed_query(encoded_query); + let rest = signed_query + .strip_prefix(&decoded_query) + .ok_or_else(|| Error::new(ErrorKind::Unexpected, "signed query changed"))? + .strip_prefix('&') + .filter(|v| !v.is_empty()); let query = if let Some(rest) = rest { format!("{encoded_query}&{rest}") @@ -144,15 +137,15 @@ impl CosCore { let path_and_query = format!("{}?{}", path_and_query.path(), query); - let mut parts = std::mem::take(uri).into_parts(); + let mut parts = uri.into_parts(); parts.path_and_query = Some(http::uri::PathAndQuery::from_str(&path_and_query).map_err( |e| Error::new(ErrorKind::Unexpected, "invalid signed query").set_source(e), )?); - *uri = Uri::from_parts(parts).map_err(|e| { + let uri = Uri::from_parts(parts).map_err(|e| { Error::new(ErrorKind::Unexpected, "failed to restore signed query").set_source(e) })?; - Ok(()) + Ok(uri) } fn decode_signed_query(encoded_query: &str) -> String {