From 767823e4ffa234b6ee93103af7d8779838201165 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 6 Jun 2024 14:20:17 -0700 Subject: [PATCH] Revert "crates_io_tarball: Disallow paths differing only by case (#8788)" This reverts commit e476d90a2b66cb3ebcba2bc258a10c832711f15b. --- crates/crates_io_tarball/src/lib.rs | 51 +++++-------------- src/controllers/krate/publish.rs | 20 ++++++-- ...publish__manifest__multiple_manifests.snap | 4 +- 3 files changed, 30 insertions(+), 45 deletions(-) diff --git a/crates/crates_io_tarball/src/lib.rs b/crates/crates_io_tarball/src/lib.rs index d89426536b..732fc3eccf 100644 --- a/crates/crates_io_tarball/src/lib.rs +++ b/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, HashSet}; +use std::collections::BTreeMap; use std::io::Read; use std::path::{Path, PathBuf}; use std::str::FromStr; @@ -33,8 +33,6 @@ 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")] @@ -43,6 +41,8 @@ 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), #[error(transparent)] IO(#[from] std::io::Error), } @@ -67,7 +67,6 @@ pub fn process_tarball( 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)?; @@ -94,13 +93,6 @@ pub fn process_tarball( )); } - 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) { @@ -124,6 +116,13 @@ pub fn process_tarball( } } + 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 @@ -302,36 +301,12 @@ mod tests { }; let err = assert_err!(process(vec!["cargo.toml", "Cargo.toml"])); - assert_snapshot!(err, @"duplicate path found: foo-0.0.1/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"]"###); let err = assert_err!(process(vec!["Cargo.toml", "Cargo.Toml"])); - assert_snapshot!(err, @"duplicate path found: foo-0.0.1/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"]"###); let err = assert_err!(process(vec!["Cargo.toml", "cargo.toml", "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") + 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"]"###); } } diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 5ec4156795..b42b974bd6 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -714,11 +714,6 @@ impl From 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}")) } @@ -732,6 +727,21 @@ impl From 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::>() + .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}" )), diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__manifest__multiple_manifests.snap b/src/tests/krate/publish/snapshots/all__krate__publish__manifest__multiple_manifests.snap index 550f921362..03ac931368 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__manifest__multiple_manifests.snap +++ b/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.json() +expression: response.into_json() --- { "errors": [ { - "detail": "uploaded tarball contains more than one file with the same path: `foo-1.0.0/cargo.toml`" + "detail": "uploaded tarball contains more than one `Cargo.toml` manifest file; found `Cargo.toml`, `cargo.toml`" } ] }