From aad603457551ce3f131a02ffbbd4d3254216582d Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Fri, 1 Oct 2021 12:28:39 -0700 Subject: [PATCH 1/3] controllers::krate::publish: extract pkg_name from verify_tarball Will make it easier to test. --- src/controllers/krate/publish.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 1d492573f95..2a9522a39f8 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -193,7 +193,8 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { let mut tarball = Vec::new(); LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut tarball)?; let hex_cksum: String = Sha256::digest(&tarball).encode_hex(); - verify_tarball(&krate, vers, &tarball, maximums.max_unpack_size)?; + let pkg_name = format!("{}-{}", krate.name, vers); + verify_tarball(&pkg_name, &tarball, maximums.max_unpack_size)?; if let Some(readme) = new_crate.readme { render::render_and_upload_readme( @@ -363,12 +364,7 @@ pub fn add_dependencies( Ok(git_deps) } -fn verify_tarball( - krate: &Crate, - vers: &semver::Version, - tarball: &[u8], - max_unpack: u64, -) -> AppResult<()> { +fn verify_tarball(pkg_name: &str, tarball: &[u8], max_unpack: u64) -> AppResult<()> { // All our data is currently encoded with gzip let decoder = GzDecoder::new(tarball); @@ -378,7 +374,6 @@ fn verify_tarball( // Use this I/O object now to take a peek inside let mut archive = tar::Archive::new(decoder); - let prefix = format!("{}-{}", krate.name, vers); for entry in archive.entries()? { let entry = entry.map_err(|err| { err.chain(cargo_err( @@ -391,7 +386,7 @@ fn verify_tarball( // upload a tarball that contains both `foo-0.1.0/` source code as well // as `bar-0.1.0/` source code, and this could overwrite other crates in // the registry! - if !entry.path()?.starts_with(&prefix) { + if !entry.path()?.starts_with(&pkg_name) { return Err(cargo_err("invalid tarball uploaded")); } From 8caecae2f28d3a973c00c81e5e7418f3890bb81d Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Fri, 1 Oct 2021 12:09:50 -0700 Subject: [PATCH 2/3] controllers::krate::publish: Add test for verify_tarball --- src/admin/render_readmes.rs | 4 ++-- src/controllers/krate/publish.rs | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/admin/render_readmes.rs b/src/admin/render_readmes.rs index 62e51eb937f..5b140b3395d 100644 --- a/src/admin/render_readmes.rs +++ b/src/admin/render_readmes.rs @@ -262,13 +262,13 @@ fn find_file_by_path( } #[cfg(test)] -mod tests { +pub mod tests { use std::io::Write; use tar; use super::render_pkg_readme; - fn add_file(pkg: &mut tar::Builder, path: &str, content: &[u8]) { + pub fn add_file(pkg: &mut tar::Builder, path: &str, content: &[u8]) { let mut header = tar::Header::new_gnu(); header.set_size(content.len() as u64); header.set_cksum(); diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 2a9522a39f8..483a72a76a1 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -405,7 +405,10 @@ fn verify_tarball(pkg_name: &str, tarball: &[u8], max_unpack: u64) -> AppResult< #[cfg(test)] mod tests { - use super::missing_metadata_error_message; + use super::{missing_metadata_error_message, verify_tarball}; + use crate::admin::render_readmes::tests::add_file; + use flate2::read::GzEncoder; + use std::io::Read; #[test] fn missing_metadata_error_message_test() { @@ -413,4 +416,16 @@ mod tests { assert_eq!(missing_metadata_error_message(&["a", "b"]), "missing or empty metadata fields: a, b. Please see https://doc.rust-lang.org/cargo/reference/manifest.html for how to upload metadata"); assert_eq!(missing_metadata_error_message(&["a", "b", "c"]), "missing or empty metadata fields: a, b, c. Please see https://doc.rust-lang.org/cargo/reference/manifest.html for how to upload metadata"); } + + #[test] + fn verify_tarball_test() { + let mut pkg = tar::Builder::new(vec![]); + add_file(&mut pkg, "foo-0.0.1/.cargo_vcs_info.json", br#"{}"#); + let mut serialized_archive = vec![]; + GzEncoder::new(pkg.into_inner().unwrap().as_slice(), Default::default()) + .read_to_end(&mut serialized_archive) + .unwrap(); + verify_tarball("foo-0.0.1", &serialized_archive, 512 * 1024 * 1024).unwrap(); + verify_tarball("bar-0.0.1", &serialized_archive, 512 * 1024 * 1024).unwrap_err(); + } } From b25ad21f5ac2952b225f27be2dc48193ad936f2f Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 28 Oct 2021 11:52:06 +0200 Subject: [PATCH 3/3] controllers::krate::publish: Use `assert_ok/err!()` for `verify_tarball()` result assertions --- src/controllers/krate/publish.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 483a72a76a1..79049c03b59 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -425,7 +425,9 @@ mod tests { GzEncoder::new(pkg.into_inner().unwrap().as_slice(), Default::default()) .read_to_end(&mut serialized_archive) .unwrap(); - verify_tarball("foo-0.0.1", &serialized_archive, 512 * 1024 * 1024).unwrap(); - verify_tarball("bar-0.0.1", &serialized_archive, 512 * 1024 * 1024).unwrap_err(); + + let limit = 512 * 1024 * 1024; + assert_ok!(verify_tarball("foo-0.0.1", &serialized_archive, limit)); + assert_err!(verify_tarball("bar-0.0.1", &serialized_archive, limit)); } }