test(property): expand parser invariants (#3534)

* test(property): harden parser invariants for #3522

* test(property): satisfy clippy for #3522

Cargo.lock

@@ -9120,6 +9120,7 @@ dependencies = [
  "percent-encoding",
  "pin-project-lite",
  "pprof-pyroscope-fork",
+ "proptest",
  "rand 0.10.1",
  "rcgen",
  "reqwest",

crates/protocols/src/ftps/driver.rs

@@ -39,6 +39,22 @@ const EVENT_FTPS_DIRECTORY_STATE: &str = "ftps_directory_state";
 const EVENT_FTPS_CWD_FAILED: &str = "ftps_cwd_failed";
 const EVENT_FTPS_RENAME_STATE: &str = "ftps_rename_state";
 
+fn parse_s3_path(path_input: &str) -> std::result::Result<(String, Option<String>), String> {
+    if path_input.chars().any(char::is_control) {
+        return Err("control characters are not allowed in FTPS paths".to_string());
+    }
+
+    let cleaned_path = path::clean(path_input);
+    let (bucket, object) = path::path_to_bucket_object(&cleaned_path);
+
+    if object.contains(path::GLOBAL_DIR_SUFFIX) {
+        return Err("internal directory marker is not allowed in FTPS paths".to_string());
+    }
+
+    let key = if object.is_empty() { None } else { Some(object) };
+    Ok((bucket, key))
+}
+
 /// FTPS metadata implementation
 #[derive(Debug, Clone)]
 pub struct FtpsMetadata {
@@ -148,14 +164,6 @@ where
         }
     }
 
-    fn parse_s3_path(&self, path: &str) -> std::result::Result<(String, Option<String>), String> {
-        let cleaned_path = path::clean(path);
-        let (bucket, object) = path::path_to_bucket_object(&cleaned_path);
-        let key = if object.is_empty() { None } else { Some(object) };
-
-        Ok((bucket, key))
-    }
-
     /// Recursively delete all objects in a bucket, then delete the bucket itself.
     async fn delete_bucket_recursively(
         &self,
@@ -249,8 +257,7 @@ where
         let path_str = path.as_ref().to_string_lossy();
         let session_context = &user.session_context;
 
-        let (bucket, key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         if let Some(key) = key {
@@ -353,8 +360,7 @@ where
             return self.list_buckets(session_context).await;
         }
 
-        let (bucket, prefix) = self
-            .parse_s3_path(&path_str)
+        let (bucket, prefix) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         // Authorize the operation
@@ -485,8 +491,7 @@ where
         let session_context = &user.session_context;
         let masked_username = MaskedAccessKey(&user.username);
 
-        let (bucket, key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         let key = key.ok_or_else(|| Error::new(ErrorKind::PermanentFileNotAvailable, "Cannot get directory"))?;
@@ -566,8 +571,7 @@ where
         let session_context = &user.session_context;
         let masked_username = MaskedAccessKey(&user.username);
 
-        let (bucket, key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         let key = key.ok_or_else(|| Error::new(ErrorKind::PermanentFileNotAvailable, "Cannot put to directory"))?;
@@ -658,8 +662,7 @@ where
             "FTPS delete requested"
         );
 
-        let (bucket, key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         if let Some(key) = key {
@@ -726,8 +729,7 @@ where
             "ftps directory state changed"
         );
 
-        let (bucket, _key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, _key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         // Create bucket for directory
@@ -774,8 +776,7 @@ where
         let path_str = path.as_ref().to_string_lossy();
         let session_context = &user.session_context;
 
-        let (bucket, _key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, _key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         // Authorize delete bucket
@@ -831,8 +832,7 @@ where
         let path_str = path.as_ref().to_string_lossy();
         let session_context = &user.session_context;
 
-        let (bucket, _key) = self
-            .parse_s3_path(&path_str)
+        let (bucket, _key) = parse_s3_path(&path_str)
             .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?;
 
         // Authorize HeadBucket (CWD probes bucket existence)
@@ -886,3 +886,43 @@ where
         ))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::parse_s3_path;
+    use rustfs_utils::path;
+
+    proptest::proptest! {
+        #[test]
+        fn parse_s3_path_never_leaks_control_bytes_or_traversal_in_ok_output(
+            input in proptest::prelude::any::<String>(),
+        ) {
+            match parse_s3_path(&input) {
+                Err(_) => {}
+                Ok((bucket, key)) => {
+                    proptest::prop_assert!(!bucket.contains('/'));
+                    proptest::prop_assert!(!bucket.chars().any(char::is_control));
+
+                    if let Some(k) = key.as_deref() {
+                        proptest::prop_assert!(!k.chars().any(char::is_control));
+                        proptest::prop_assert!(!k.starts_with('/'));
+                        proptest::prop_assert!(!k.split('/').any(|segment| segment == ".."));
+                        proptest::prop_assert!(!k.contains(path::GLOBAL_DIR_SUFFIX));
+                    }
+                }
+            }
+        }
+    }
+
+    #[test]
+    fn parse_s3_path_rejects_control_bytes() {
+        assert!(parse_s3_path("/bucket/line\rfeed").is_err());
+        assert!(parse_s3_path("/bucket/line\nfeed").is_err());
+        assert!(parse_s3_path("/bucket/tab\tname").is_err());
+    }
+
+    #[test]
+    fn parse_s3_path_rejects_internal_directory_marker() {
+        assert!(parse_s3_path("/bucket/__XLDIR__").is_err());
+    }
+}

crates/protocols/src/swift/object.rs

@@ -111,6 +111,14 @@ impl ObjectKeyMapper {
             return Err(SwiftError::BadRequest("Object name cannot contain null bytes".to_string()));
         }
 
+        if object.chars().any(char::is_control) {
+            return Err(SwiftError::BadRequest("Object name cannot contain control characters".to_string()));
+        }
+
+        if object.starts_with('/') {
+            return Err(SwiftError::BadRequest("Object name cannot start with '/'".to_string()));
+        }
+
         // Check for directory traversal attempts
         if object.contains("..") {
             // Allow ".." as part of a filename, but not as a path segment
@@ -1115,6 +1123,7 @@ pub fn format_content_range(start: i64, end: i64, total: i64) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use proptest::prelude::*;
 
     #[test]
     fn test_validate_object_name_valid() {
@@ -1162,6 +1171,30 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_validate_object_name_control_character() {
+        let result = ObjectKeyMapper::validate_object_name("file\nname.txt");
+        assert!(result.is_err());
+        match result {
+            Err(SwiftError::BadRequest(msg)) => {
+                assert!(msg.contains("control"));
+            }
+            _ => panic!("Expected BadRequest error"),
+        }
+    }
+
+    #[test]
+    fn test_validate_object_name_leading_slash() {
+        let result = ObjectKeyMapper::validate_object_name("/file.txt");
+        assert!(result.is_err());
+        match result {
+            Err(SwiftError::BadRequest(msg)) => {
+                assert!(msg.contains("start with '/'"));
+            }
+            _ => panic!("Expected BadRequest error"),
+        }
+    }
+
     #[test]
     fn test_validate_object_name_directory_traversal() {
         // ".." as a path segment should be rejected
@@ -1240,6 +1273,41 @@ mod tests {
         assert_eq!(ObjectKeyMapper::normalize_path("path///to/file.txt"), "path/to/file.txt");
     }
 
+    proptest! {
+        #[test]
+        fn validate_object_name_ok_outputs_are_safe(input in any::<String>()) {
+            if let Ok(()) = ObjectKeyMapper::validate_object_name(&input) {
+                prop_assert!(!input.is_empty());
+                prop_assert!(input.len() <= 1024);
+                prop_assert!(!input.starts_with('/'));
+                prop_assert!(!input.chars().any(char::is_control));
+                prop_assert!(!input.split('/').any(|segment| segment == ".."));
+            }
+        }
+
+        #[test]
+        fn decode_object_from_url_round_trips_valid_input(input in any::<String>()) {
+            let encoded = ObjectKeyMapper::encode_object_for_url(&input);
+
+            match ObjectKeyMapper::decode_object_from_url(&encoded) {
+                Ok(decoded) => {
+                    prop_assert_eq!(decoded.as_str(), input.as_str());
+                    prop_assert!(ObjectKeyMapper::validate_object_name(&decoded).is_ok());
+                }
+                Err(_) => {
+                    prop_assert!(ObjectKeyMapper::validate_object_name(&input).is_err());
+                }
+            }
+        }
+
+        #[test]
+        fn normalize_path_is_idempotent(input in any::<String>()) {
+            let once = ObjectKeyMapper::normalize_path(&input);
+            let twice = ObjectKeyMapper::normalize_path(&once);
+            prop_assert_eq!(twice, once);
+        }
+    }
+
     #[test]
     fn test_parse_destination_header() {
         // Valid destination headers

crates/protocols/src/webdav/driver.rs

@@ -770,6 +770,11 @@ where
         let decoded_path = percent_decode_str(&path_str)
             .decode_utf8()
             .map_err(|_| FsError::GeneralFailure)?;
+
+        if decoded_path.chars().any(char::is_control) {
+            return Err(FsError::GeneralFailure);
+        }
+
         let cleaned_path = path::clean(&decoded_path);
         let (bucket, object) = path::path_to_bucket_object(&cleaned_path);
 
@@ -777,6 +782,10 @@ where
             return Ok((String::new(), None));
         }
 
+        if object.contains(path::GLOBAL_DIR_SUFFIX) {
+            return Err(FsError::GeneralFailure);
+        }
+
         let key = if object.is_empty() { None } else { Some(object) };
         Ok((bucket, key))
     }
@@ -1689,6 +1698,7 @@ mod tests {
     use dav_server::davpath::DavPath;
     use dav_server::fs::FsError;
     use futures_util::StreamExt;
+    use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode};
     use rustfs_credentials::Credentials;
     use rustfs_policy::auth::UserIdentity;
     use s3s::dto::*;
@@ -2186,6 +2196,58 @@ mod tests {
         assert_eq!(key.as_deref(), Some("file with spaces.txt"));
     }
 
+    #[test]
+    fn parse_path_rejects_control_bytes_after_decode() {
+        let driver = driver();
+        let path = DavPath::new("/bucket/report%0Aname.txt").expect("path should parse");
+
+        let err = driver.parse_path(&path).expect_err("control bytes should be rejected");
+
+        assert_eq!(err, FsError::GeneralFailure);
+    }
+
+    #[test]
+    fn parse_path_rejects_internal_directory_marker() {
+        let driver = driver();
+        let path = DavPath::new("/bucket/__XLDIR__").expect("path should parse");
+
+        let err = driver
+            .parse_path(&path)
+            .expect_err("internal directory marker should be rejected");
+
+        assert_eq!(err, FsError::GeneralFailure);
+    }
+
+    proptest::proptest! {
+        #[test]
+        fn parse_path_never_leaks_control_bytes_or_traversal_in_ok_output(
+            input in proptest::prelude::any::<String>(),
+        ) {
+            let driver = driver();
+            let encoded = utf8_percent_encode(&input, NON_ALPHANUMERIC).to_string();
+            let Ok(path) = DavPath::new(&format!("/{encoded}")) else {
+                return Ok(());
+            };
+
+            match driver.parse_path(&path) {
+                Err(err) => {
+                    proptest::prop_assert_eq!(err, FsError::GeneralFailure);
+                }
+                Ok((bucket, key)) => {
+                    proptest::prop_assert!(!bucket.contains('/'));
+                    proptest::prop_assert!(!bucket.chars().any(char::is_control));
+
+                    if let Some(k) = key.as_deref() {
+                        proptest::prop_assert!(!k.chars().any(char::is_control));
+                        proptest::prop_assert!(!k.starts_with('/'));
+                        proptest::prop_assert!(!k.split('/').any(|segment| segment == ".."));
+                        proptest::prop_assert!(!k.contains(rustfs_utils::path::GLOBAL_DIR_SUFFIX));
+                    }
+                }
+            }
+        }
+    }
+
     #[tokio::test]
     async fn directory_rename_returns_error_when_delete_fails_after_successful_copy() {
         let (driver, storage) = recording_driver(

rustfs/Cargo.toml

@@ -203,6 +203,7 @@ serial_test = { workspace = true }
 tempfile = { workspace = true }
 aws-config = { workspace = true }
 anyhow = { workspace = true }
+proptest = "1"
 tokio = { workspace = true, features = ["test-util"] }
 temp-env = { workspace = true, features = ["async_closure"] }
 tracing-subscriber = { workspace = true }

rustfs/src/storage/options.rs

@@ -617,9 +617,17 @@ pub fn parse_copy_source_range(range_str: &str) -> S3Result<HTTPRangeSpec> {
                 .parse::<i64>()
                 .map_err(|_| s3_error!(InvalidArgument, "Invalid range format"))?;
 
+            if length <= 0 {
+                return Err(s3_error!(InvalidArgument, "Invalid range format"));
+            }
+
+            let start = length
+                .checked_neg()
+                .ok_or_else(|| s3_error!(InvalidArgument, "Invalid range format"))?;
+
             Ok(HTTPRangeSpec {
                 is_suffix_length: true,
-                start: -length,
+                start,
                 end: -1,
             })
         } else {
@@ -777,6 +785,7 @@ fn get_content_sha256_cksum(headers: &HeaderMap<HeaderValue>, service_type: Serv
 
 #[cfg(test)]
 mod tests {
+    use proptest::prelude::*;
     use temp_env;
 
     use super::*;
@@ -1560,5 +1569,28 @@ mod tests {
         assert!(parse_copy_source_range("bytes=").is_err());
         assert!(parse_copy_source_range("bytes=abc-def").is_err());
         assert!(parse_copy_source_range("bytes=100-50").is_err()); // start > end
+        assert!(parse_copy_source_range("bytes=-0").is_err());
+        assert!(parse_copy_source_range("bytes=--9223372036854775808").is_err());
+    }
+
+    proptest! {
+        #[test]
+        fn parse_copy_source_range_never_panics_and_preserves_output_invariants(
+            input in any::<String>(),
+        ) {
+            match std::panic::catch_unwind(|| parse_copy_source_range(&input)) {
+                Ok(Ok(spec)) => {
+                    if spec.is_suffix_length {
+                        prop_assert_eq!(spec.end, -1);
+                        prop_assert!(spec.start < 0);
+                    } else {
+                        prop_assert!(spec.start >= 0);
+                        prop_assert!(spec.end == -1 || spec.end >= spec.start);
+                    }
+                }
+                Ok(Err(_)) => {}
+                Err(_) => prop_assert!(false, "parse_copy_source_range panicked for input {:?}", input),
+            }
+        }
     }
 }