crates_io_tarball: Disallow paths differing only by case (#8788)

Author: Turbo87

crates/crates_io_tarball/src/lib.rs

@@ -9,7 +9,7 @@ use crate::manifest::validate_manifest;
 pub use crate::vcs_info::CargoVcsInfo;
 pub use cargo_manifest::{Manifest, StringOrBool};
 use flate2::read::GzDecoder;
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::io::Read;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
@@ -33,6 +33,8 @@ pub enum TarballError {
     Malformed(#[source] std::io::Error),
     #[error("invalid path found: {0}")]
     InvalidPath(String),
+    #[error("duplicate path found: {0}")]
+    DuplicatePath(String),
     #[error("unexpected symlink or hard link found: {0}")]
     UnexpectedSymlink(String),
     #[error("Cargo.toml manifest is missing")]
@@ -41,8 +43,6 @@ pub enum TarballError {
     InvalidManifest(#[from] cargo_manifest::Error),
     #[error("Cargo.toml manifest is incorrectly cased: {0:?}")]
     IncorrectlyCasedManifest(PathBuf),
-    #[error("more than one Cargo.toml manifest in tarball: {0:?}")]
-    TooManyManifests(Vec<PathBuf>),
     #[error(transparent)]
     IO(#[from] std::io::Error),
 }
@@ -67,6 +67,7 @@ pub fn process_tarball<R: Read>(
 
     let mut vcs_info = None;
     let mut manifests = BTreeMap::new();
+    let mut paths = HashSet::new();
 
     for entry in archive.entries()? {
         let mut entry = entry.map_err(TarballError::Malformed)?;
@@ -93,6 +94,13 @@ pub fn process_tarball<R: Read>(
             ));
         }
 
+        let lowercase_path = entry_path.as_os_str().to_ascii_lowercase();
+        if !paths.insert(lowercase_path) {
+            return Err(TarballError::DuplicatePath(
+                entry_path.display().to_string(),
+            ));
+        }
+
         // Let's go hunting for the VCS info and crate manifest. The only valid place for these is
         // in the package root in the tarball.
         if entry_path.parent() == Some(pkg_root) {
@@ -116,13 +124,6 @@ pub fn process_tarball<R: Read>(
         }
     }
 
-    if manifests.len() > 1 {
-        // There are no scenarios where we want to accept a crate file with multiple manifests.
-        return Err(TarballError::TooManyManifests(
-            manifests.into_keys().collect(),
-        ));
-    }
-
     // Although we're interested in all possible cases of `Cargo.toml` above to protect users
     // on case-insensitive filesystems, to match the behaviour of cargo we should only actually
     // accept `Cargo.toml` and (the now deprecated) `cargo.toml` as valid options for the
@@ -301,12 +302,36 @@ mod tests {
         };
 
         let err = assert_err!(process(vec!["cargo.toml", "Cargo.toml"]));
-        assert_snapshot!(err, @r###"more than one Cargo.toml manifest in tarball: ["foo-0.0.1/Cargo.toml", "foo-0.0.1/cargo.toml"]"###);
+        assert_snapshot!(err, @"duplicate path found: foo-0.0.1/Cargo.toml");
 
         let err = assert_err!(process(vec!["Cargo.toml", "Cargo.Toml"]));
-        assert_snapshot!(err, @r###"more than one Cargo.toml manifest in tarball: ["foo-0.0.1/Cargo.Toml", "foo-0.0.1/Cargo.toml"]"###);
+        assert_snapshot!(err, @"duplicate path found: foo-0.0.1/Cargo.Toml");
 
         let err = assert_err!(process(vec!["Cargo.toml", "cargo.toml", "CARGO.TOML"]));
-        assert_snapshot!(err, @r###"more than one Cargo.toml manifest in tarball: ["foo-0.0.1/CARGO.TOML", "foo-0.0.1/Cargo.toml", "foo-0.0.1/cargo.toml"]"###);
+        assert_snapshot!(err, @"duplicate path found: foo-0.0.1/cargo.toml");
+    }
+
+    #[test]
+    fn test_duplicate_paths() {
+        let tarball = TarballBuilder::new()
+            .add_file("foo-0.0.1/Cargo.toml", MANIFEST)
+            .add_file("foo-0.0.1/foo.rs", b"")
+            .add_file("foo-0.0.1/foo.rs", b"")
+            .build();
+
+        let err = assert_err!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
+        assert_snapshot!(err, @"duplicate path found: foo-0.0.1/foo.rs")
+    }
+
+    #[test]
+    fn test_case_insensitivity() {
+        let tarball = TarballBuilder::new()
+            .add_file("foo-0.0.1/Cargo.toml", MANIFEST)
+            .add_file("foo-0.0.1/foo.rs", b"")
+            .add_file("foo-0.0.1/FOO.rs", b"")
+            .build();
+
+        let err = assert_err!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
+        assert_snapshot!(err, @"duplicate path found: foo-0.0.1/FOO.rs")
     }
 }

src/controllers/krate/publish.rs

@@ -714,6 +714,11 @@ impl From<TarballError> for BoxedAppError {
                 "uploaded tarball is malformed or too large when decompressed",
             ),
             TarballError::InvalidPath(path) => bad_request(format!("invalid path found: {path}")),
+            TarballError::DuplicatePath(path) => {
+                bad_request(format!(
+                    "uploaded tarball contains more than one file with the same path: `{path}`"
+                ))
+            }
             TarballError::UnexpectedSymlink(path) => {
                 bad_request(format!("unexpected symlink or hard link found: {path}"))
             }
@@ -727,21 +732,6 @@ impl From<TarballError> for BoxedAppError {
                     name = name.to_string_lossy(),
                 ))
             }
-            TarballError::TooManyManifests(paths) => {
-                let paths = paths
-                    .into_iter()
-                    .map(|path| {
-                        path.file_name()
-                            .unwrap_or_default()
-                            .to_string_lossy()
-                            .into_owned()
-                    })
-                    .collect::<Vec<_>>()
-                    .join("`, `");
-                bad_request(format!(
-                    "uploaded tarball contains more than one `Cargo.toml` manifest file; found `{paths}`"
-                ))
-            }
             TarballError::InvalidManifest(err) => bad_request(format!(
                 "failed to parse `Cargo.toml` manifest file\n\n{err}"
             )),

src/tests/krate/publish/snapshots/all__krate__publish__manifest__multiple_manifests.snap

@@ -1,11 +1,11 @@
 ---
 source: src/tests/krate/publish/manifest.rs
-expression: response.into_json()
+expression: response.json()
 ---
 {
   "errors": [
     {
-      "detail": "uploaded tarball contains more than one `Cargo.toml` manifest file; found `Cargo.toml`, `cargo.toml`"
+      "detail": "uploaded tarball contains more than one file with the same path: `foo-1.0.0/cargo.toml`"
     }
   ]
 }