diff --git a/core/src/request.rs b/core/src/request.rs index 2265cfea..634a19f9 100644 --- a/core/src/request.rs +++ b/core/src/request.rs @@ -47,10 +47,36 @@ pub struct SigningRequest { pub headers: HeaderMap, } +fn parse_query(query: &str) -> Vec<(String, String)> { + form_urlencoded::parse(query.as_bytes()) + .map(|(k, v)| (k.into_owned(), v.into_owned())) + .collect() +} + +fn append_query_pair(output: &mut String, key: &str, value: &str) { + output.push_str(key); + if !value.is_empty() { + output.push('='); + output.push_str(value); + } +} + +fn append_query_pairs(output: &mut String, query: &[(String, String)], mut needs_separator: bool) { + for (key, value) in query { + if needs_separator { + output.push('&'); + } + + append_query_pair(output, key, value); + needs_separator = true; + } +} + impl SigningRequest { /// Build a signing context from http::request::Parts. pub fn build(parts: &mut http::request::Parts) -> Result { - let uri = mem::take(&mut parts.uri).into_parts(); + // Keep the original URI in parts so apply can preserve its raw query. + let uri = parts.uri.clone().into_parts(); let paq = uri .path_and_query .unwrap_or_else(|| PathAndQuery::from_static("/")); @@ -62,14 +88,7 @@ impl SigningRequest { Error::request_invalid("request without authority is invalid for signing") })?, path: paq.path().to_string(), - query: paq - .query() - .map(|v| { - form_urlencoded::parse(v.as_bytes()) - .map(|(k, v)| (k.into_owned(), v.into_owned())) - .collect() - }) - .unwrap_or_default(), + query: paq.query().map(parse_query).unwrap_or_default(), // Take the headers out of the request to avoid copy. // We will return it back when apply the context. @@ -78,7 +97,15 @@ impl SigningRequest { } /// Apply the signing context back to http::request::Parts. + /// + /// Existing query parameters preserve their raw URI representation when unchanged. + /// Appended query parameters do not cause the existing query to be rebuilt. + /// Modifying, removing, or reordering existing parameters rebuilds the query. pub fn apply(mut self, parts: &mut http::request::Parts) -> Result<()> { + let original_query = parts + .uri + .query() + .map(|raw_query| (raw_query.to_string(), parse_query(raw_query))); let query_size = self.query_size(); // Return headers back. @@ -91,34 +118,38 @@ impl SigningRequest { // Return authority back. uri_parts.authority = Some(self.authority); // Build path and query. - uri_parts.path_and_query = - { - let paq = if query_size == 0 { - self.path - } else { + uri_parts.path_and_query = { + let paq = match original_query { + Some((raw_query, parsed_query)) if self.query.starts_with(&parsed_query) => { let mut s = self.path; - s.reserve(query_size + 1); - + s.reserve(raw_query.len() + query_size + self.query.len() + 1); s.push('?'); - for (i, (k, v)) in self.query.iter().enumerate() { - if i > 0 { - s.push('&'); - } - - s.push_str(k); - if !v.is_empty() { - s.push('='); - s.push_str(v); - } - } - + s.push_str(&raw_query); + + let needs_separator = !raw_query.is_empty() && !raw_query.ends_with('&'); + append_query_pairs( + &mut s, + &self.query[parsed_query.len()..], + needs_separator, + ); + s + } + _ if query_size == 0 => self.path, + _ => { + let mut s = self.path; + s.reserve(query_size + self.query.len() + 1); + s.push('?'); + append_query_pairs(&mut s, &self.query, false); s - }; + } + }; - Some(PathAndQuery::from_str(&paq).map_err(|e| { + Some( + PathAndQuery::from_str(&paq).map_err(|e| { Error::request_invalid("invalid path and query").with_source(e) - })?) - }; + })?, + ) + }; Uri::from_parts(uri_parts) .map_err(|e| Error::request_invalid("failed to build URI").with_source(e))? }; @@ -306,3 +337,115 @@ pub enum SigningMethod { /// Signing with query. Query(Duration), } + +#[cfg(test)] +mod tests { + use super::*; + + const RAW_QUERY: &str = "versionId=a%2Bb%3Dc%2525%26e"; + + fn build_signing_request(raw_query: &str) -> Result<(SigningRequest, http::request::Parts)> { + let req = http::Request::get(format!("https://example.com/object?{raw_query}")).body(())?; + let (mut parts, _) = req.into_parts(); + let signing_req = SigningRequest::build(&mut parts)?; + + Ok((signing_req, parts)) + } + + #[test] + fn test_apply_preserves_existing_raw_query() -> Result<()> { + let (signing_req, mut parts) = build_signing_request(RAW_QUERY)?; + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some(RAW_QUERY)); + Ok(()) + } + + #[test] + fn test_apply_preserves_existing_raw_query_when_appending() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request(RAW_QUERY)?; + signing_req.query_push("signature", "value"); + signing_req.apply(&mut parts)?; + + assert_eq!( + parts.uri.query(), + Some("versionId=a%2Bb%3Dc%2525%26e&signature=value") + ); + Ok(()) + } + + #[test] + fn test_apply_preserves_form_query_wire_representation_when_appending() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("value=a+b&empty=&flag")?; + signing_req.query_push("signature", "value"); + signing_req.apply(&mut parts)?; + + assert_eq!( + parts.uri.query(), + Some("value=a+b&empty=&flag&signature=value") + ); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_explicitly_updated_query() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request(RAW_QUERY)?; + signing_req.query[0].1 = "updated".to_string(); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("versionId=updated")); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_all_query_pairs_when_updating() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("keep=%41&change=old")?; + signing_req.query[1].1 = "new".to_string(); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("keep=A&change=new")); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_query_after_removing() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("remove=1&keep=%41")?; + signing_req.query.remove(0); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("keep=A")); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_ambiguous_duplicate_query_after_removing() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("x=%41&x=A")?; + signing_req.query.remove(0); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("x=A")); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_query_after_inserting() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("keep=%41")?; + signing_req + .query + .insert(0, ("insert".to_string(), "1".to_string())); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("insert=1&keep=A")); + Ok(()) + } + + #[test] + fn test_apply_rebuilds_query_after_sorting() -> Result<()> { + let (mut signing_req, mut parts) = build_signing_request("z=%41&a=1")?; + signing_req.query.sort(); + signing_req.apply(&mut parts)?; + + assert_eq!(parts.uri.query(), Some("a=1&z=A")); + Ok(()) + } +} diff --git a/services/aliyun-oss/src/sign_request.rs b/services/aliyun-oss/src/sign_request.rs index 4f528f9f..4f21112f 100644 --- a/services/aliyun-oss/src/sign_request.rs +++ b/services/aliyun-oss/src/sign_request.rs @@ -486,7 +486,7 @@ impl RequestSigner { let mut signing_req = SigningRequest::build(req)?; self.canonicalize_v4_headers(&mut signing_req, cred, signing_time, expires_in.is_some())?; let additional_headers = self.v4_additional_headers(&signing_req, expires_in.is_some())?; - self.canonicalize_v4_query( + let canonical_query = self.canonicalize_v4_query( &mut signing_req, cred, signing_time, @@ -497,6 +497,7 @@ impl RequestSigner { let canonical_request = self.build_v4_canonical_request( &signing_req, + &canonical_query, expires_in.is_some(), &additional_headers, )?; @@ -570,7 +571,7 @@ impl RequestSigner { expires_in: Option, region: &str, additional_headers: &[String], - ) -> Result<()> { + ) -> Result> { let mut query_pairs = req .query .iter() @@ -586,64 +587,54 @@ impl RequestSigner { if let Some(expires) = expires_in { let scope = self.v4_scope(signing_time, region); - let mut next_idx = query_pairs.len(); - query_pairs.push(( - next_idx, - X_OSS_SIGNATURE_VERSION.to_string(), - OSS_V4_ALGORITHM.to_string(), - )); - next_idx += 1; - query_pairs.push(( - next_idx, - X_OSS_CREDENTIAL.to_string(), - utf8_percent_encode( - &format!("{}/{}", cred.access_key_id, scope), - &OSS_V4_QUERY_ENCODE_SET, - ) - .to_string(), - )); - next_idx += 1; - query_pairs.push(( - next_idx, - X_OSS_DATE.to_string(), - signing_time.format_iso8601(), - )); - next_idx += 1; - query_pairs.push(( - next_idx, - X_OSS_EXPIRES.to_string(), - expires.as_secs().to_string(), - )); - next_idx += 1; - query_pairs.push(( - next_idx, - X_OSS_ADDITIONAL_HEADERS.to_string(), - utf8_percent_encode(&additional_headers.join(";"), &OSS_V4_QUERY_ENCODE_SET) + let mut signing_query = vec![ + ( + X_OSS_SIGNATURE_VERSION.to_string(), + OSS_V4_ALGORITHM.to_string(), + ), + ( + X_OSS_CREDENTIAL.to_string(), + utf8_percent_encode( + &format!("{}/{}", cred.access_key_id, scope), + &OSS_V4_QUERY_ENCODE_SET, + ) .to_string(), - )); - next_idx += 1; + ), + (X_OSS_DATE.to_string(), signing_time.format_iso8601()), + (X_OSS_EXPIRES.to_string(), expires.as_secs().to_string()), + ( + X_OSS_ADDITIONAL_HEADERS.to_string(), + utf8_percent_encode(&additional_headers.join(";"), &OSS_V4_QUERY_ENCODE_SET) + .to_string(), + ), + ]; if let Some(token) = &cred.security_token { - query_pairs.push(( - next_idx, + signing_query.push(( X_OSS_SECURITY_TOKEN.to_string(), utf8_percent_encode(token, &OSS_V4_QUERY_ENCODE_SET).to_string(), )); } + + let next_idx = query_pairs.len(); + query_pairs.extend( + signing_query + .iter() + .enumerate() + .map(|(idx, (k, v))| (next_idx + idx, k.clone(), v.clone())), + ); + signing_query.sort_by(|a, b| a.0.cmp(&b.0)); + req.query.extend(signing_query); } query_pairs.sort_by(|a, b| a.1.cmp(&b.1).then(a.0.cmp(&b.0))); - req.query = query_pairs - .into_iter() - .map(|(_, k, v)| (k, v)) - .collect::>(); - - Ok(()) + Ok(query_pairs.into_iter().map(|(_, k, v)| (k, v)).collect()) } fn build_v4_canonical_request( &self, req: &SigningRequest, + canonical_query: &[(String, String)], is_presign: bool, additional_headers: &[String], ) -> Result { @@ -680,7 +671,7 @@ impl RequestSigner { writeln!( &mut s, "{}", - req.query + canonical_query .iter() .map(|(k, v)| { if v.is_empty() { @@ -1683,7 +1674,7 @@ mod tests { let additional_headers = signer .v4_additional_headers(&signing_req, false) .expect("additional headers must build"); - signer + let canonical_query = signer .canonicalize_v4_query( &mut signing_req, &credential, @@ -1694,7 +1685,7 @@ mod tests { ) .expect("canonical query must build"); let canonical_request = signer - .build_v4_canonical_request(&signing_req, false, &additional_headers) + .build_v4_canonical_request(&signing_req, &canonical_query, false, &additional_headers) .expect("canonical request must build"); assert_eq!( canonical_request, @@ -1745,6 +1736,35 @@ mod tests { ); } + #[test] + fn test_v4_header_signing_preserves_existing_raw_query() { + let credential = Credential { + access_key_id: "testid".to_string(), + access_key_secret: "yourAccessKeySecret".to_string(), + security_token: None, + expires_in: None, + }; + let time = Timestamp::from_second(1_744_353_684).expect("timestamp must build"); + let signer = RequestSigner::new("examplebucket") + .with_region("cn-hangzhou") + .with_signing_version(SigningVersion::V4) + .with_time(time); + let raw_query = "b=2&versionId=a%2Bb%3Dc%2525%26e&empty=&flag"; + let mut req = http::Request::get(format!( + "https://examplebucket.oss-cn-hangzhou.aliyuncs.com/exampleobject?{raw_query}" + )) + .body(()) + .expect("request must build") + .into_parts() + .0; + + signer + .sign_v4(&mut req, &credential, time, None) + .expect("v4 header signing must succeed"); + + assert_eq!(req.uri.query(), Some(raw_query)); + } + #[test] fn test_v4_presign_signature_matches_golden_output() { let credential = Credential { @@ -1773,7 +1793,7 @@ mod tests { let additional_headers = signer .v4_additional_headers(&signing_req, true) .expect("additional headers must build"); - signer + let canonical_query = signer .canonicalize_v4_query( &mut signing_req, &credential, @@ -1784,7 +1804,7 @@ mod tests { ) .expect("canonical query must build"); let canonical_request = signer - .build_v4_canonical_request(&signing_req, true, &additional_headers) + .build_v4_canonical_request(&signing_req, &canonical_query, true, &additional_headers) .expect("canonical request must build"); assert_eq!( canonical_request, @@ -1825,28 +1845,56 @@ mod tests { expires_in: None, }; let time = Timestamp::from_second(1_733_196_187).expect("timestamp must build"); - let mut req = http::Request::get( - "https://examplebucket.oss-cn-hangzhou.aliyuncs.com/exampleobject?prefix=b&acl&prefix=a%20value", - ) - .body(()) - .expect("request must build") - .into_parts() - .0; - - RequestSigner::new("examplebucket") + let request_uri = "https://examplebucket.oss-cn-hangzhou.aliyuncs.com/exampleobject?prefix=b&acl&prefix=a%20value"; + let signer = RequestSigner::new("examplebucket") .with_region("cn-hangzhou") .with_signing_version(SigningVersion::V4) - .with_time(time) + .with_time(time); + + let mut canonical_req = http::Request::get(request_uri) + .body(()) + .expect("request must build") + .into_parts() + .0; + let mut signing_req = + SigningRequest::build(&mut canonical_req).expect("request must build"); + signer + .canonicalize_v4_headers(&mut signing_req, &credential, time, true) + .expect("canonical headers must build"); + let additional_headers = signer + .v4_additional_headers(&signing_req, true) + .expect("additional headers must build"); + let canonical_query = signer + .canonicalize_v4_query( + &mut signing_req, + &credential, + time, + Some(Duration::from_secs(60)), + "cn-hangzhou", + &additional_headers, + ) + .expect("canonical query must build"); + assert_eq!(canonical_query[0], ("acl".to_string(), String::new())); + assert_eq!(canonical_query[1], ("prefix".to_string(), "b".to_string())); + assert_eq!( + canonical_query[2], + ("prefix".to_string(), "a%20value".to_string()) + ); + + let mut req = http::Request::get(request_uri) + .body(()) + .expect("request must build") + .into_parts() + .0; + signer .sign_v4(&mut req, &credential, time, Some(Duration::from_secs(60))) .expect("v4 presign must succeed"); let query = req.uri.query().expect("query must exist"); - let acl = query.find("acl").expect("acl must exist"); - let prefix_b = query.find("prefix=b").expect("prefix=b must exist"); - let prefix_a = query - .find("prefix=a%20value") - .expect("encoded prefix must exist"); - assert!(acl < prefix_a); - assert!(prefix_b < prefix_a); + assert!( + query.starts_with("prefix=b&acl&prefix=a%20value&x-oss-additional-headers=host"), + "signed URI does not preserve the existing raw query: {}", + req.uri + ); } } diff --git a/services/aws-v4/src/sign_request.rs b/services/aws-v4/src/sign_request.rs index cb22badb..1602c38c 100644 --- a/services/aws-v4/src/sign_request.rs +++ b/services/aws-v4/src/sign_request.rs @@ -83,7 +83,7 @@ impl SignRequest for RequestSigner { // canonicalize context canonicalize_header(&mut signed_req, cred, expires_in, now)?; - canonicalize_query( + let canonical_query = canonicalize_query( &mut signed_req, cred, expires_in, @@ -93,7 +93,7 @@ impl SignRequest for RequestSigner { )?; // build canonical request and string to sign. - let creq = canonical_request_string(&mut signed_req)?; + let creq = canonical_request_string(&signed_req, &canonical_query)?; let encoded_req = hex_sha256(creq.as_bytes()); // Scope: "20220313///aws4_request" @@ -160,7 +160,10 @@ impl SignRequest for RequestSigner { } } -fn canonical_request_string(ctx: &mut SigningRequest) -> Result { +fn canonical_request_string( + ctx: &SigningRequest, + canonical_query: &[(String, String)], +) -> Result { // 256 is specially chosen to avoid reallocation for most requests. let mut f = String::with_capacity(256); @@ -181,7 +184,7 @@ fn canonical_request_string(ctx: &mut SigningRequest) -> Result { writeln!( f, "{}", - ctx.query + canonical_query .iter() .map(|(k, v)| { format!("{k}={v}") }) .collect::>() @@ -298,7 +301,9 @@ fn canonicalize_query( now: Timestamp, service: &str, region: &str, -) -> Result<()> { +) -> Result> { + let original_query_len = ctx.query.len(); + if let Some(expire) = expires_in { ctx.query .push(("X-Amz-Algorithm".into(), "AWS4-HMAC-SHA256".into())); @@ -326,26 +331,28 @@ fn canonicalize_query( } } - // Return if query is empty. - if ctx.query.is_empty() { - return Ok(()); - } - - // Sort by param name - ctx.query.sort(); - - ctx.query = ctx - .query - .iter() + let mut canonical_query = ctx.query.clone(); + canonical_query.sort(); + canonical_query = canonical_query + .into_iter() .map(|(k, v)| { ( - utf8_percent_encode(k, &AWS_QUERY_ENCODE_SET).to_string(), - utf8_percent_encode(v, &AWS_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&k, &AWS_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&v, &AWS_QUERY_ENCODE_SET).to_string(), ) }) .collect(); - Ok(()) + let mut appended_query = ctx.query.split_off(original_query_len); + appended_query.sort(); + ctx.query.extend(appended_query.into_iter().map(|(k, v)| { + ( + utf8_percent_encode(&k, &AWS_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&v, &AWS_QUERY_ENCODE_SET).to_string(), + ) + })); + + Ok(canonical_query) } fn generate_signing_key(secret: &str, time: Timestamp, region: &str, service: &str) -> Vec { @@ -380,6 +387,8 @@ mod tests { use reqsign_file_read_tokio::TokioFileRead; use reqsign_http_send_reqwest::ReqwestHttpSend; + const RAW_QUERY: &str = "b=2&versionId=a%2Bb%3Dc%2525%26e&empty=&flag"; + /// (name, request_builder) type TestCase = (&'static str, fn() -> Request<&'static str>); @@ -388,6 +397,10 @@ mod tests { ("get_request", test_get_request), ("get_request_with_sse", test_get_request_with_sse), ("get_request_with_query", test_get_request_with_query), + ( + "get_request_with_literal_plus", + test_get_request_with_literal_plus, + ), ("get_request_virtual_host", test_get_request_virtual_host), ( "get_request_with_query_virtual_host", @@ -452,6 +465,16 @@ mod tests { req } + fn test_get_request_with_literal_plus() -> Request<&'static str> { + let mut req = Request::new(""); + *req.method_mut() = http::Method::GET; + *req.uri_mut() = "http://127.0.0.1:9000/hello?value=a+b" + .parse() + .expect("url must be valid"); + + req + } + fn test_get_request_virtual_host() -> Request<&'static str> { let mut req = Request::new(""); *req.method_mut() = http::Method::GET; @@ -562,6 +585,38 @@ mod tests { assert_eq!(format_query(l), format_query(r), "{name} query mismatch"); } + #[tokio::test] + async fn test_signing_preserves_existing_raw_query() -> Result<()> { + let credential = Credential { + access_key_id: "access_key_id".to_string(), + secret_access_key: "secret_access_key".to_string(), + ..Default::default() + }; + let signer = RequestSigner::new("s3", "test").with_time("2026-07-10T00:00:00Z".parse()?); + + for expires_in in [None, Some(Duration::from_secs(3600))] { + let req = Request::get(format!("https://example.com/object?{RAW_QUERY}")).body(())?; + let (mut parts, _) = req.into_parts(); + + signer + .sign_request(&Context::new(), &mut parts, Some(&credential), expires_in) + .await?; + + let query = parts.uri.query().expect("signed URI must have a query"); + if expires_in.is_some() { + assert!( + query.starts_with(&format!("{RAW_QUERY}&X-Amz-Algorithm=")), + "signed URI does not preserve the existing raw query: {}", + parts.uri + ); + } else { + assert_eq!(query, RAW_QUERY); + } + } + + Ok(()) + } + #[tokio::test] async fn test() -> Result<()> { for (name, req) in test_cases() { diff --git a/services/azure-storage/src/sign_request.rs b/services/azure-storage/src/sign_request.rs index b35d23c6..3ceb2b2b 100644 --- a/services/azure-storage/src/sign_request.rs +++ b/services/azure-storage/src/sign_request.rs @@ -201,6 +201,7 @@ impl SignRequest for RequestSigner { }; let mut sctx = SigningRequest::build(req)?; + let original_query_len = sctx.query.len(); // Handle different credential types match cred { @@ -425,7 +426,7 @@ impl SignRequest for RequestSigner { } // Apply percent encoding for query parameters - for (_, v) in sctx.query.iter_mut() { + for (_, v) in sctx.query.iter_mut().skip(original_query_len) { *v = percent_encode(v.as_bytes(), &AZURE_QUERY_ENCODE_SET).to_string(); } @@ -670,7 +671,7 @@ fn canonicalize_resource(ctx: &mut SigningRequest, account_name: &str) -> String "/{}{}\n{}", account_name, ctx.path, - SigningRequest::query_to_percent_decoded_string(query, ":", "\n") + SigningRequest::query_to_string(query, ":", "\n") ) } @@ -685,6 +686,45 @@ mod tests { use std::str::FromStr; use std::time::Duration; + const RAW_QUERY: &str = "b=2&versionId=a%2Bb%3Dc%2525%26e&empty=&flag"; + + #[test] + fn test_canonicalize_resource_decodes_query_once() { + let req = Request::get( + "https://account.blob.core.windows.net/container/blob?versionId=a%2Bb%3Dc%2525%26e", + ) + .body(()) + .unwrap(); + let (mut parts, _) = req.into_parts(); + let mut signing_req = SigningRequest::build(&mut parts).unwrap(); + + assert_eq!( + canonicalize_resource(&mut signing_req, "account"), + "/account/container/blob\nversionid:a+b=c%25&e" + ); + } + + #[tokio::test] + async fn test_header_signing_preserves_existing_raw_query() { + let now = Timestamp::from_str("2022-03-01T08:12:34Z").unwrap(); + let key = reqsign_core::hash::base64_encode("key".as_bytes()); + let credential = Credential::with_shared_key("account", &key); + let signer = RequestSigner::new().with_time(now); + let req = Request::get(format!( + "https://account.blob.core.windows.net/container/blob?{RAW_QUERY}" + )) + .body(()) + .unwrap(); + let (mut parts, _) = req.into_parts(); + + signer + .sign_request(&Context::new(), &mut parts, Some(&credential), None) + .await + .unwrap(); + + assert_eq!(parts.uri.query(), Some(RAW_QUERY)); + } + #[tokio::test] async fn test_sas_token() { let _ = env_logger::builder().is_test(true).try_init(); @@ -782,7 +822,9 @@ mod tests { .with_service_sas_permissions("r"); let req = Request::builder() - .uri("https://account.blob.core.windows.net/container/path/to/blob.txt") + .uri(format!( + "https://account.blob.core.windows.net/container/path/to/blob.txt?{RAW_QUERY}" + )) .body(()) .unwrap(); let (mut parts, _) = req.into_parts(); @@ -799,7 +841,9 @@ mod tests { assert_eq!( parts.uri.to_string(), - "https://account.blob.core.windows.net/container/path/to/blob.txt?sv=2020-12-06&se=2022-03-01T08%3A17%3A34Z&sp=r&sr=b&sig=CP9a2LIrR9zeG4I4jZjqPetJSXWJ77QeUA7c3GMypyM%3D" + format!( + "https://account.blob.core.windows.net/container/path/to/blob.txt?{RAW_QUERY}&sv=2020-12-06&se=2022-03-01T08%3A17%3A34Z&sp=r&sr=b&sig=CP9a2LIrR9zeG4I4jZjqPetJSXWJ77QeUA7c3GMypyM%3D" + ) ); } diff --git a/services/google/src/sign_request.rs b/services/google/src/sign_request.rs index 9122c139..2447cd84 100644 --- a/services/google/src/sign_request.rs +++ b/services/google/src/sign_request.rs @@ -217,7 +217,7 @@ impl RequestSigner { ) -> Result { canonicalize_header(req)?; - canonicalize_query( + let canonical_query = canonicalize_query( req, SigningMethod::Query(expires_in), client_email, @@ -226,7 +226,7 @@ impl RequestSigner { &self.region, )?; - let creq = canonical_request_string(req)?; + let creq = canonical_request_string(req, &canonical_query)?; let encoded_req = hex_sha256(creq.as_bytes()); let scope = format!( @@ -456,7 +456,10 @@ fn hex_encode_upper(bytes: &[u8]) -> String { out } -fn canonical_request_string(req: &mut SigningRequest) -> Result { +fn canonical_request_string( + req: &SigningRequest, + canonical_query: &[(String, String)], +) -> Result { // 256 is specially chosen to avoid reallocation for most requests. let mut f = String::with_capacity(256); @@ -473,7 +476,7 @@ fn canonical_request_string(req: &mut SigningRequest) -> Result { // Insert query f.push_str(&SigningRequest::query_to_string( - req.query.clone(), + canonical_query.to_vec(), "=", "&", )); @@ -522,7 +525,9 @@ fn canonicalize_query( now: Timestamp, service: &str, region: &str, -) -> Result<()> { +) -> Result> { + let original_query_len = req.query.len(); + if let SigningMethod::Query(expire) = method { req.query .push(("X-Goog-Algorithm".into(), "GOOG4-RSA-SHA256".into())); @@ -545,26 +550,28 @@ fn canonicalize_query( )); } - // Return if query is empty. - if req.query.is_empty() { - return Ok(()); - } - - // Sort by param name - req.query.sort(); - - req.query = req - .query - .iter() + let mut canonical_query = req.query.clone(); + canonical_query.sort(); + canonical_query = canonical_query + .into_iter() .map(|(k, v)| { ( - utf8_percent_encode(k, &GOOG_QUERY_ENCODE_SET).to_string(), - utf8_percent_encode(v, &GOOG_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&k, &GOOG_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&v, &GOOG_QUERY_ENCODE_SET).to_string(), ) }) .collect(); - Ok(()) + let mut appended_query = req.query.split_off(original_query_len); + appended_query.sort(); + req.query.extend(appended_query.into_iter().map(|(k, v)| { + ( + utf8_percent_encode(&k, &GOOG_QUERY_ENCODE_SET).to_string(), + utf8_percent_encode(&v, &GOOG_QUERY_ENCODE_SET).to_string(), + ) + })); + + Ok(canonical_query) } #[cfg(test)] @@ -573,6 +580,9 @@ mod tests { use bytes::Bytes; use http::header; use reqsign_core::HttpSend; + + const RAW_QUERY: &str = "b=2&versionId=a%2Bb%3Dc%2525%26e&empty=&flag"; + use std::sync::{Arc, Mutex}; #[derive(Debug, Default)] @@ -661,7 +671,9 @@ mod tests { let mut builder = http::Request::builder(); builder = builder.method(http::Method::GET); - builder = builder.uri("https://storage.googleapis.com/test-bucket/test-object"); + builder = builder.uri(format!( + "https://storage.googleapis.com/test-bucket/test-object?{RAW_QUERY}" + )); let req = builder.body(Bytes::new()).expect("request must build"); let (mut parts, _body) = req.into_parts(); @@ -670,6 +682,11 @@ mod tests { .await?; let query = parts.uri.query().expect("signed url must have query"); + assert!( + query.starts_with(&format!("{RAW_QUERY}&X-Goog-Algorithm=")), + "signed URL does not preserve the existing raw query: {}", + parts.uri + ); assert_eq!( query_get(query, "X-Goog-Signature").expect("signature must exist"), "010203" @@ -680,7 +697,9 @@ mod tests { let mut builder = http::Request::builder(); builder = builder.method(http::Method::GET); - builder = builder.uri("https://storage.googleapis.com/test-bucket/test-object"); + builder = builder.uri(format!( + "https://storage.googleapis.com/test-bucket/test-object?{RAW_QUERY}" + )); let req = builder.body(Bytes::new()).expect("request must build"); let (mut parts_for_rebuild, _body) = req.into_parts(); diff --git a/services/tencent-cos/src/sign_request.rs b/services/tencent-cos/src/sign_request.rs index 8e1384a9..df574621 100644 --- a/services/tencent-cos/src/sign_request.rs +++ b/services/tencent-cos/src/sign_request.rs @@ -197,3 +197,47 @@ fn build_signature( cred.secret_id, key_time, key_time, header_list, param_list, signature ) } + +#[cfg(test)] +mod tests { + use super::*; + + const RAW_QUERY: &str = "versionId=a%2Bb%3Dc%2525%26e"; + + async fn sign_request(expires_in: Option) -> Result { + let req = http::Request::get(format!("https://example.com/object?{RAW_QUERY}")).body(())?; + let (mut parts, _) = req.into_parts(); + let credential = Credential { + secret_id: "secret_id".to_string(), + secret_key: "secret_key".to_string(), + ..Default::default() + }; + let signer = RequestSigner::new().with_time("2026-07-10T00:00:00Z".parse()?); + + signer + .sign_request(&Context::new(), &mut parts, Some(&credential), expires_in) + .await?; + + Ok(parts.uri) + } + + #[tokio::test] + async fn test_header_signing_preserves_existing_raw_query() -> Result<()> { + let uri = sign_request(None).await?; + + assert_eq!(uri.query(), Some(RAW_QUERY)); + Ok(()) + } + + #[tokio::test] + async fn test_query_signing_preserves_existing_raw_query() -> Result<()> { + let uri = sign_request(Some(Duration::from_secs(3600))).await?; + let query = uri.query().expect("signed URI must have a query"); + + assert!( + query.split('&').any(|pair| pair == RAW_QUERY), + "signed URI does not preserve the existing raw query: {uri}" + ); + Ok(()) + } +} diff --git a/services/volcengine-tos/src/sign_request.rs b/services/volcengine-tos/src/sign_request.rs index 8a6f32b5..68ae7531 100644 --- a/services/volcengine-tos/src/sign_request.rs +++ b/services/volcengine-tos/src/sign_request.rs @@ -107,8 +107,8 @@ impl SignRequest for RequestSigner { .insert(&*HEADER_TOS_SECURITY_TOKEN, token.parse()?); } - canonicalize_query(&mut signing_req); - let (canonical_request_hash, _) = canonical_request_hash(&mut signing_req)?; + let canonical_query = canonicalize_query(&signing_req); + let (canonical_request_hash, _) = canonical_request_hash(&signing_req, &canonical_query)?; // Scope: "//tos/request" let credential_scope = format!("{}/{}/tos/request", date_only, self.region); @@ -150,17 +150,20 @@ impl SignRequest for RequestSigner { } } -fn canonicalize_query(ctx: &mut SigningRequest) { - ctx.query = ctx +fn canonicalize_query(ctx: &SigningRequest) -> Vec<(String, String)> { + let mut query = ctx .query .iter() .map(|(k, v)| (percent_encode_query(k), percent_encode_query(v))) - .collect(); - // Sort by param name - ctx.query.sort(); + .collect::>(); + query.sort(); + query } -fn canonical_request_hash(ctx: &mut SigningRequest) -> Result<(String, String)> { +fn canonical_request_hash( + ctx: &SigningRequest, + canonical_query: &[(String, String)], +) -> Result<(String, String)> { let mut canonical_request = String::with_capacity(256); // Insert method @@ -176,8 +179,7 @@ fn canonical_request_hash(ctx: &mut SigningRequest) -> Result<(String, String)> canonical_request.push('\n'); // Insert encoded query - let query_string = ctx - .query + let query_string = canonical_query .iter() .map(|(k, v)| format!("{}={}", k, v)) .collect::>() @@ -289,11 +291,16 @@ mod tests { .with_env(OsEnv); let signer = Signer::new(ctx, loader, signer); - let req = http::Request::get("https://bucket.tos-cn-beijing.volces.com?list-type=2&prefix=abc&delimiter=%2F&max-keys=5&continuation-token=whvFnl2rE5vm9cWvQSgxwpc7QXHY7dgUGQ7nxlsVxFymg2%2BK227j5IHQZ32h").body(())?; + let raw_query = "list-type=2&prefix=abc&delimiter=%2F&max-keys=5&continuation-token=whvFnl2rE5vm9cWvQSgxwpc7QXHY7dgUGQ7nxlsVxFymg2%2BK227j5IHQZ32h"; + let req = http::Request::get(format!( + "https://bucket.tos-cn-beijing.volces.com?{raw_query}" + )) + .body(())?; let (mut parts, _) = req.into_parts(); signer.sign(&mut parts, None).await?; + assert_eq!(parts.uri.query(), Some(raw_query)); let headers = parts.headers; let auth = headers.get("Authorization").unwrap();