bugfix: proper handing of whitespace char in url with Form-decoding instead of Percent-decoding

Author: HJLebbink

common/src/cleanup_guard.rs

@@ -37,18 +37,18 @@ impl CleanupGuard {
 
 pub async fn cleanup(client: Client, bucket_name: &str) {
     tokio::select!(
-                _ = tokio::time::sleep(std::time::Duration::from_secs(60)) => {
-                    eprintln!("Cleanup timeout after 60s while removing bucket {}", bucket_name);
-                },
-                outcome = client.delete_and_purge_bucket(bucket_name) => {
-                    match outcome {
-                        Ok(_) => {
-                            eprintln!("Bucket {} removed successfully", bucket_name);
-                        }
-                        Err(e) => {
-                            eprintln!("Error removing bucket {}: {:?}", bucket_name, e);
-                        }
-                    }
+        _ = tokio::time::sleep(std::time::Duration::from_secs(60)) => {
+            eprintln!("Cleanup timeout after 60s while removing bucket {}", bucket_name);
+        },
+        outcome = client.delete_and_purge_bucket(bucket_name) => {
+            match outcome {
+                Ok(_) => {
+                    //eprintln!("Bucket {} removed successfully", bucket_name);
                 }
+                Err(e) => {
+                    eprintln!("Error removing bucket {}: {:?}", bucket_name, e);
+                }
+            }
+        }
     );
 }

src/s3/builders/copy_object.rs

@@ -26,7 +26,7 @@ use crate::s3::response::{
 use crate::s3::sse::{Sse, SseCustomerKey};
 use crate::s3::types::{Directive, PartInfo, Retention, S3Api, S3Request, ToS3Request};
 use crate::s3::utils::{
-    UtcTime, check_bucket_name, check_object_name, to_http_header_value, to_iso8601utc, urlencode,
+    UtcTime, check_bucket_name, check_object_name, to_http_header_value, to_iso8601utc, url_encode,
 };
 use async_recursion::async_recursion;
 use http::Method;
@@ -254,9 +254,9 @@ impl ToS3Request for CopyObjectInternal {
                     if !tagging.is_empty() {
                         tagging.push('&');
                     }
-                    tagging.push_str(&urlencode(key));
+                    tagging.push_str(&url_encode(key));
                     tagging.push('=');
-                    tagging.push_str(&urlencode(value));
+                    tagging.push_str(&url_encode(value));
                 }
                 if !tagging.is_empty() {
                     headers.add("x-amz-tagging", tagging);
@@ -285,7 +285,7 @@ impl ToS3Request for CopyObjectInternal {
             copy_source.push_str(&self.source.object);
             if let Some(v) = &self.source.version_id {
                 copy_source.push_str("?versionId=");
-                copy_source.push_str(&urlencode(v));
+                copy_source.push_str(&url_encode(v));
             }
             headers.add("x-amz-copy-source", copy_source);
 
@@ -1032,7 +1032,7 @@ impl ComposeSource {
         copy_source.push_str(&self.object);
         if let Some(v) = &self.version_id {
             copy_source.push_str("?versionId=");
-            copy_source.push_str(&urlencode(v));
+            copy_source.push_str(&url_encode(v));
         }
         headers.add("x-amz-copy-source", copy_source);
 
@@ -1155,9 +1155,9 @@ fn into_headers_copy_object(
             if !tagging.is_empty() {
                 tagging.push('&');
             }
-            tagging.push_str(&urlencode(key));
+            tagging.push_str(&url_encode(key));
             tagging.push('=');
-            tagging.push_str(&urlencode(value));
+            tagging.push_str(&url_encode(value));
         }
 
         if !tagging.is_empty() {

src/s3/builders/put_object.rs

@@ -29,7 +29,7 @@ use crate::s3::{
     },
     sse::Sse,
     types::{PartInfo, Retention, S3Api, S3Request, ToS3Request},
-    utils::{check_bucket_name, md5sum_hash, to_iso8601utc, urlencode},
+    utils::{check_bucket_name, md5sum_hash, to_iso8601utc, url_encode},
 };
 use bytes::{Bytes, BytesMut};
 use http::Method;
@@ -201,7 +201,7 @@ impl ToS3Request for AbortMultipartUpload {
 
         let headers: Multimap = self.extra_headers.unwrap_or_default();
         let mut query_params: Multimap = self.extra_query_params.unwrap_or_default();
-        query_params.add("uploadId", urlencode(&self.upload_id).to_string());
+        query_params.add("uploadId", url_encode(&self.upload_id).to_string());
 
         Ok(S3Request::new(self.client, Method::DELETE)
             .region(self.region)
@@ -885,9 +885,9 @@ fn into_headers_put_object(
             if !tagging.is_empty() {
                 tagging.push('&');
             }
-            tagging.push_str(&urlencode(key));
+            tagging.push_str(&url_encode(key));
             tagging.push('=');
-            tagging.push_str(&urlencode(value));
+            tagging.push_str(&url_encode(value));
         }
 
         if !tagging.is_empty() {

src/s3/client/delete_bucket.rs

@@ -16,7 +16,7 @@
 use super::Client;
 use crate::s3::builders::{DeleteBucket, DeleteObject, ObjectToDelete};
 use crate::s3::error::{Error, ErrorCode};
-use crate::s3::response::DeleteResult;
+use crate::s3::response::{BucketExistsResponse, DeleteResult};
 use crate::s3::response::{
     DeleteBucketResponse, DeleteObjectResponse, DeleteObjectsResponse, PutObjectLegalHoldResponse,
 };
@@ -57,6 +57,17 @@ impl Client {
         bucket: S,
     ) -> Result<DeleteBucketResponse, Error> {
         let bucket: String = bucket.into();
+
+        let resp: BucketExistsResponse = self.bucket_exists(&bucket).send().await?;
+        if !resp.exists {
+            // if the bucket does not exist, we can return early
+            return Ok(DeleteBucketResponse {
+                request: Default::default(), //TODO consider how to handle this
+                body: Bytes::new(),
+                headers: Default::default(),
+            });
+        }
+
         let is_express = self.is_minio_express().await;
 
         let mut stream = self

src/s3/multimap.rs

@@ -13,12 +13,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::s3::utils::urlencode;
+use crate::s3::utils::url_encode;
 use lazy_static::lazy_static;
 use multimap::MultiMap;
 use regex::Regex;
 use std::collections::BTreeMap;
-pub use urlencoding::decode as urldecode;
 
 /// Multimap for string key and string value
 pub type Multimap = MultiMap<String, String>;
@@ -71,9 +70,9 @@ impl MultimapExt for Multimap {
                 if !query.is_empty() {
                     query.push('&');
                 }
-                query.push_str(&urlencode(key));
+                query.push_str(&url_encode(key));
                 query.push('=');
-                query.push_str(&urlencode(value));
+                query.push_str(&url_encode(value));
             }
         }
         query
@@ -94,9 +93,9 @@ impl MultimapExt for Multimap {
                         if !query.is_empty() {
                             query.push('&');
                         }
-                        query.push_str(&urlencode(key.as_str()));
+                        query.push_str(&url_encode(key.as_str()));
                         query.push('=');
-                        query.push_str(&urlencode(value));
+                        query.push_str(&url_encode(value));
                     }
                 }
                 None => todo!(), // This never happens.

src/s3/response/list_objects.rs

@@ -15,26 +15,26 @@ use crate::s3::error::Error;
 use crate::s3::response::a_response_traits::HasS3Fields;
 use crate::s3::types::{FromS3Response, ListEntry, S3Request};
 use crate::s3::utils::xml::{Element, MergeXmlElements};
-use crate::s3::utils::{from_iso8601utc, parse_tags, urldecode};
+use crate::s3::utils::{from_iso8601utc, parse_tags, url_decode};
 use async_trait::async_trait;
 use bytes::{Buf, Bytes};
 use reqwest::header::HeaderMap;
 use std::collections::HashMap;
 use std::mem;
 
-fn url_decode(
+fn url_decode_w_enc(
     encoding_type: &Option<String>,
-    prefix: Option<String>,
+    s: Option<String>,
 ) -> Result<Option<String>, Error> {
     if let Some(v) = encoding_type.as_ref() {
         if v == "url" {
-            if let Some(v) = prefix {
-                return Ok(Some(urldecode(&v)?.to_string()));
+            if let Some(raw) = s {
+                return Ok(Some(url_decode(&raw).to_string()));
             }
         }
     }
 
-    if let Some(v) = prefix.as_ref() {
+    if let Some(v) = s.as_ref() {
         return Ok(Some(v.to_string()));
     }
 
@@ -56,7 +56,7 @@ fn parse_common_list_objects_response(
     Error,
 > {
     let encoding_type = root.get_child_text("EncodingType");
-    let prefix = url_decode(
+    let prefix = url_decode_w_enc(
         &encoding_type,
         Some(root.get_child_text("Prefix").unwrap_or_default()),
     )?;
@@ -90,7 +90,7 @@ fn parse_list_objects_contents(
     let merged = MergeXmlElements::new(&children1, &children2);
     for content in merged {
         let etype = encoding_type.as_ref().cloned();
-        let key = url_decode(&etype, Some(content.get_child_text_or_error("Key")?))?.unwrap();
+        let key = url_decode_w_enc(&etype, Some(content.get_child_text_or_error("Key")?))?.unwrap();
         let last_modified = Some(from_iso8601utc(
             &content.get_child_text_or_error("LastModified")?,
         )?);
@@ -156,7 +156,7 @@ fn parse_list_objects_common_prefixes(
 ) -> Result<(), Error> {
     for (_, common_prefix) in root.get_matching_children("CommonPrefixes") {
         contents.push(ListEntry {
-            name: url_decode(
+            name: url_decode_w_enc(
                 encoding_type,
                 Some(common_prefix.get_child_text_or_error("Prefix")?),
             )?
@@ -214,8 +214,8 @@ impl FromS3Response for ListObjectsV1Response {
         let root = Element::from(&xmltree_root);
         let (name, encoding_type, prefix, delimiter, is_truncated, max_keys) =
             parse_common_list_objects_response(&root)?;
-        let marker = url_decode(&encoding_type, root.get_child_text("Marker"))?;
-        let mut next_marker = url_decode(&encoding_type, root.get_child_text("NextMarker"))?;
+        let marker = url_decode_w_enc(&encoding_type, root.get_child_text("Marker"))?;
+        let mut next_marker = url_decode_w_enc(&encoding_type, root.get_child_text("NextMarker"))?;
         let mut contents: Vec<ListEntry> = Vec::new();
         parse_list_objects_contents(&mut contents, &root, "Contents", &encoding_type, false)?;
         if is_truncated && next_marker.is_none() {
@@ -281,7 +281,7 @@ impl FromS3Response for ListObjectsV2Response {
             .get_child_text("KeyCount")
             .map(|x| x.parse::<u16>())
             .transpose()?;
-        let start_after = url_decode(&encoding_type, root.get_child_text("StartAfter"))?;
+        let start_after = url_decode_w_enc(&encoding_type, root.get_child_text("StartAfter"))?;
         let continuation_token = root.get_child_text("ContinuationToken");
         let next_continuation_token = root.get_child_text("NextContinuationToken");
         let mut contents: Vec<ListEntry> = Vec::new();
@@ -344,8 +344,9 @@ impl FromS3Response for ListObjectVersionsResponse {
         let root = Element::from(&xmltree_root);
         let (name, encoding_type, prefix, delimiter, is_truncated, max_keys) =
             parse_common_list_objects_response(&root)?;
-        let key_marker = url_decode(&encoding_type, root.get_child_text("KeyMarker"))?;
-        let next_key_marker = url_decode(&encoding_type, root.get_child_text("NextKeyMarker"))?;
+        let key_marker = url_decode_w_enc(&encoding_type, root.get_child_text("KeyMarker"))?;
+        let next_key_marker =
+            url_decode_w_enc(&encoding_type, root.get_child_text("NextKeyMarker"))?;
         let version_id_marker = root.get_child_text("VersionIdMarker");
         let next_version_id_marker = root.get_child_text("NextVersionIdMarker");
         let mut contents: Vec<ListEntry> = Vec::new();

src/s3/utils.rs

@@ -32,15 +32,41 @@ use regex::Regex;
 use ring::digest::{Context, SHA256};
 #[cfg(not(feature = "ring"))]
 use sha2::{Digest, Sha256};
+use std::borrow::Cow;
 use std::collections::HashMap;
+use std::string::FromUtf8Error;
 use std::sync::Arc;
-pub use urlencoding::decode as urldecode;
-pub use urlencoding::encode as urlencode;
 use xmltree::Element;
 
 /// Date and time with UTC timezone
 pub type UtcTime = DateTime<Utc>;
 
+use url::form_urlencoded;
+
+// Great stuff to get confused about.
+// String "a b+c" in Percent-Encoding (RFC 3986) becomes "a%20b%2Bc".
+// S3 sometimes returns Form-Encoding (application/x-www-form-urlencoded) rendering string "a%20b%2Bc" into "a+b%2Bc"
+// If you were to do Percent-Decoding on "a+b%2Bc" you would get "a+b+c", which is wrong.
+// If you use Form-Decoding on "a+b%2Bc" you would get "a b+c", which is correct.
+
+/// Decodes a URL-encoded string, TODO
+pub fn url_decode_old(s: &str) -> Result<Cow<str>, FromUtf8Error> {
+    urlencoding::decode(s) //NOTE: Unencoded + is preserved literally, and not changed to a space.
+}
+
+/// Decodes a URL-encoded string, TODO
+pub fn url_decode(s: &str) -> Cow<str> {
+    let result: String = form_urlencoded::parse(s.as_bytes())
+        .map(|(k, _)| k)
+        .collect();
+    Cow::Owned(result)
+}
+
+/// Encodes a string using URL encoding, TODO
+pub fn url_encode(s: &str) -> Cow<str> {
+    urlencoding::encode(s)
+}
+
 /// Encodes data using base64 algorithm
 pub fn b64encode(input: impl AsRef<[u8]>) -> String {
     BASE64.encode(input)
@@ -245,7 +271,7 @@ pub fn match_region(value: &str) -> bool {
         || value.ends_with('_')
 }
 
-/// Validates given bucket name
+/// Validates given bucket name. TODO S3Express has slightly different rules for bucket names
 pub fn check_bucket_name(bucket_name: impl AsRef<str>, strict: bool) -> Result<(), Error> {
     let bucket_name: &str = bucket_name.as_ref().trim();
     let bucket_name_len = bucket_name.len();
@@ -302,6 +328,7 @@ pub fn check_bucket_name(bucket_name: impl AsRef<str>, strict: bool) -> Result<(
     Ok(())
 }
 
+/// Validates given object name. TODO S3Express has slightly different rules for object names
 pub fn check_object_name(object_name: impl AsRef<str>) -> Result<(), Error> {
     let object_name: &str = object_name.as_ref();
     let object_name_n_bytes = object_name.len();