fix(s3s-fs)!: reject path aliases in object keys

crates/s3s-fs/src/fs.rs

@@ -9,10 +9,11 @@ use s3s::crypto::Checksum;
 use s3s::crypto::Md5;
 use s3s::dto;
 use s3s::dto::PartNumber;
+use s3s::{S3Result, s3_error};
 
 use std::env;
 use std::ops::Not;
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::time::SystemTime;
 
@@ -150,10 +151,23 @@ impl FileSystem {
     }
 
     /// resolve object path under the virtual root
-    pub(crate) fn get_object_path(&self, bucket: &str, key: &str) -> Result<PathBuf> {
+    pub(crate) fn get_object_path(&self, bucket: &str, key: &str) -> S3Result<PathBuf> {
         let dir = Path::new(&bucket);
         let file_path = Path::new(&key);
-        self.resolve_abs_path(dir.join(file_path))
+        // Filesystem normalization would make these keys refer to other objects.
+        let path_key = key.strip_suffix('/').unwrap_or(key);
+        if path_key
+            .split(std::path::is_separator)
+            .any(|part| matches!(part, "" | "." | ".."))
+            || (cfg!(windows) && key.contains('\\'))
+            || file_path.components().any(|part| !matches!(part, Component::Normal(_)))
+        {
+            return Err(s3_error!(
+                InvalidArgument,
+                "Object key cannot be represented as a literal filesystem path"
+            ));
+        }
+        self.resolve_abs_path(dir.join(file_path)).map_err(Into::into)
     }
 
     /// resolve bucket path under the virtual root
@@ -514,6 +528,48 @@ mod tests {
         }
     }
 
+    #[test]
+    fn object_paths_reject_normalization_aliases() {
+        let fs = FileSystem {
+            root: env::temp_dir(),
+            tmp_file_counter: AtomicU64::new(0),
+        };
+        for key in [
+            "",
+            ".",
+            "..",
+            "../victim",
+            "allowed/../victim",
+            "allowed/./victim",
+            "allowed//victim",
+            "allowed//",
+            "/victim",
+        ] {
+            let error = fs.get_object_path("bucket", key).unwrap_err();
+            assert_eq!(error.code(), &s3s::S3ErrorCode::InvalidArgument, "key {key:?}");
+        }
+        #[cfg(windows)]
+        for key in [
+            r"allowed\victim",
+            r"allowed\..\victim",
+            r"allowed\.\victim",
+            r"C:\victim",
+            r"C:victim",
+            r"\victim",
+        ] {
+            let error = fs.get_object_path("bucket", key).unwrap_err();
+            assert_eq!(error.code(), &s3s::S3ErrorCode::InvalidArgument, "key {key:?}");
+        }
+        for key in ["allowed/ok", "allowed/.config", "allowed/report..txt", "allowed/directory/"] {
+            assert_eq!(fs.get_object_path("bucket", key).unwrap(), fs.root.join("bucket").join(key));
+        }
+        #[cfg(not(windows))]
+        assert_eq!(
+            fs.get_object_path("bucket", r"allowed\literal").unwrap(),
+            fs.root.join("bucket").join(r"allowed\literal")
+        );
+    }
+
     #[test]
     fn concurrent_upload_directory_deletion_is_idempotent() -> Result<()> {
         let runtime = tokio::runtime::Builder::new_current_thread()

crates/s3s-fs/src/s3.rs

@@ -496,8 +496,20 @@ impl S3 for FileSystem {
         let input = req.input;
 
         let mut deleted_objects: Vec<DeletedObject> = Vec::new();
+        let mut errors = Vec::new();
         for object in input.delete.objects {
-            let path = self.get_object_path(&input.bucket, &object.key)?;
+            let path = match self.get_object_path(&input.bucket, &object.key) {
+                Ok(path) => path,
+                Err(error) => {
+                    errors.push(s3s::dto::Error {
+                        key: Some(object.key),
+                        version_id: object.version_id,
+                        code: Some(error.code().as_str().to_owned()),
+                        message: error.message().map(str::to_owned),
+                    });
+                    continue;
+                }
+            };
             if object.key.ends_with('/') {
                 match fs::read_dir(&path).await {
                     Ok(mut dir) => {
@@ -532,6 +544,7 @@ impl S3 for FileSystem {
 
         let output = DeleteObjectsOutput {
             deleted: Some(deleted_objects),
+            errors: (!errors.is_empty()).then_some(errors),
             ..Default::default()
         };
         Ok(S3Response::new(output))
@@ -2198,6 +2211,86 @@ mod tests {
         }
     }
 
+    #[tokio::test]
+    async fn batch_delete_rejects_path_aliases_and_preserves_other_objects() {
+        let root = env::temp_dir().join(format!("s3s-fs-delete-path-{}", Uuid::new_v4()));
+        std::fs::create_dir_all(root.join("bucket/allowed")).unwrap();
+        std::fs::create_dir_all(root.join("other-bucket")).unwrap();
+        let _root = TestRoot(root.clone());
+        let fs = FileSystem::new(&root).unwrap();
+        for (key, content) in [
+            ("bucket/allowed/ok", "delete me"),
+            ("bucket/allowed/after", "delete me too"),
+            ("bucket/victim", "keep victim"),
+            ("bucket/allowed/sibling", "keep sibling"),
+            ("bucket/allowed/repeated", "keep repeated-separator victim"),
+            ("other-bucket/secret", "keep other bucket"),
+        ] {
+            std::fs::write(root.join(key), content).unwrap();
+        }
+
+        let invalid_keys = [
+            "allowed//repeated",
+            "allowed/../victim",
+            "allowed/./sibling",
+            "../other-bucket/secret",
+            "/other-bucket/secret",
+        ];
+        let objects = std::iter::once("allowed/ok")
+            .chain(invalid_keys)
+            .chain(std::iter::once("allowed/after"))
+            .map(|key| ObjectIdentifier {
+                key: key.to_owned(),
+                ..Default::default()
+            })
+            .collect();
+        let input = DeleteObjectsInput {
+            bucket: "bucket".to_owned(),
+            delete: Delete {
+                objects,
+                ..Default::default()
+            },
+            bypass_governance_retention: None,
+            checksum_algorithm: None,
+            expected_bucket_owner: None,
+            mfa: None,
+            request_payer: None,
+        };
+        let request = S3Request {
+            input,
+            method: http::Method::POST,
+            uri: http::Uri::from_static("/bucket?delete"),
+            headers: http::HeaderMap::new(),
+            extensions: http::Extensions::new(),
+            credentials: None,
+            region: None,
+            service: None,
+            trailing_headers: None,
+        };
+        let result = fs.delete_objects(request).await;
+
+        assert_eq!(std::fs::read(root.join("bucket/victim")).unwrap(), b"keep victim");
+        assert_eq!(std::fs::read(root.join("bucket/allowed/sibling")).unwrap(), b"keep sibling");
+        assert_eq!(
+            std::fs::read(root.join("bucket/allowed/repeated")).unwrap(),
+            b"keep repeated-separator victim"
+        );
+        assert_eq!(std::fs::read(root.join("other-bucket/secret")).unwrap(), b"keep other bucket");
+        assert!(!root.join("bucket/allowed/ok").exists());
+        assert!(!root.join("bucket/allowed/after").exists());
+        let output = result.unwrap().output;
+        let deleted = output.deleted.unwrap();
+        assert_eq!(deleted.len(), 2);
+        assert_eq!(deleted[0].key.as_deref(), Some("allowed/ok"));
+        assert_eq!(deleted[1].key.as_deref(), Some("allowed/after"));
+        let errors = output.errors.unwrap();
+        assert_eq!(errors.len(), invalid_keys.len());
+        for (error, key) in errors.iter().zip(invalid_keys) {
+            assert_eq!(error.key.as_deref(), Some(key));
+            assert_eq!(error.code.as_deref(), Some("InvalidArgument"));
+        }
+    }
+
     #[tokio::test]
     async fn a_stat_failure_is_skipped_only_when_the_entry_is_gone() {
         let root = env::temp_dir().join(format!("s3s-fs-vanished-{}", Uuid::new_v4()));