From 19dee4e48913e486ac84d6f11131fb310502a318 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 4 Jun 2024 13:15:26 +0200 Subject: [PATCH] crates_io_tarball: Disallow paths differing only by case --- crates/crates_io_tarball/src/lib.rs | 51 ++++++++++++++----- src/controllers/krate/publish.rs | 20 ++------ ...publish__manifest__multiple_manifests.snap | 4 +- 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/crates/crates_io_tarball/src/lib.rs b/crates/crates_io_tarball/src/lib.rs index 732fc3eccfe..d89426536ba 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; +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), #[error(transparent)] IO(#[from] std::io::Error), } @@ -67,6 +67,7 @@ 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)?; @@ -93,6 +94,13 @@ 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) { @@ -116,13 +124,6 @@ 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 @@ -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") } } diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index b42b974bd61..5ec41567954 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -714,6 +714,11 @@ 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}")) } @@ -727,21 +732,6 @@ 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 03ac9313687..550f9213626 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.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`" } ] }