You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "QuakeWang (via GitHub)" <gi...@apache.org> on 2026/07/12 03:11:53 UTC
[PR] fix: preserve raw query parameters during signing [opendal-reqsign]
QuakeWang opened a new pull request, #782:
URL: https://github.com/apache/opendal-reqsign/pull/782
Closes #778.
`SigningRequest` previously parsed existing URI queries and rebuilt them from decoded pairs. This could change percent-encoded values and even alter query parameter boundaries during signing.
This PR preserves unchanged raw query parameters when applying signed requests. Affected signers now use separate canonical query representations for signature calculation without rewriting the query sent over the wire.
Regression tests cover header signing and query signing across affected services
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: preserve raw query parameters during signing [opendal-reqsign]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #782:
URL: https://github.com/apache/opendal-reqsign/pull/782#issuecomment-5042894159
Thank you @QuakeWang for working on this, I have refactor the oerall design in https://github.com/apache/opendal-reqsign/issues/784
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: preserve raw query parameters during signing [opendal-reqsign]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #782: fix: preserve raw query parameters during signing
URL: https://github.com/apache/opendal-reqsign/pull/782
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: preserve raw query parameters during signing [opendal-reqsign]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #782:
URL: https://github.com/apache/opendal-reqsign/pull/782#discussion_r3588087623
##########
core/src/request.rs:
##########
@@ -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.
Review Comment:
This is a bit random in this docstring. A proper documentation can be:
1. Why we apply signing context (probably already in documentation somewhere), one sentence explanation in the first paragraph.
2. What rules do we apply?
3. Special conditions: - ...
Either use paragraphs or sections.
##########
services/volcengine-tos/src/sign_request.rs:
##########
@@ -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";
Review Comment:
nitpick: just copy the raw query into assert_eq
##########
core/src/request.rs:
##########
@@ -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
Review Comment:
If we want to use original query, we could store them. And the other parts can be a slice.
##########
core/src/request.rs:
##########
@@ -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);
Review Comment:
What is this size?
##########
core/src/request.rs:
##########
@@ -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(
Review Comment:
The previous string operations become function invocations. This is a bit difficult to read.
##########
services/google/src/sign_request.rs:
##########
@@ -456,7 +456,10 @@ fn hex_encode_upper(bytes: &[u8]) -> String {
out
}
-fn canonical_request_string(req: &mut SigningRequest) -> Result<String> {
+fn canonical_request_string(
Review Comment:
Maybe test this function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org