diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index c7db7749610..b1843f09271 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -18,13 +18,13 @@ pub fn download(req: &mut dyn Request) -> CargoResult { let crate_name = &req.params()["crate_id"]; let version = &req.params()["version"]; - increment_download_counts(req, crate_name, version)?; + let crate_name = increment_download_counts(req, crate_name, version)?; let redirect_url = req .app() .config .uploader - .crate_location(crate_name, version); + .crate_location(&crate_name, version); if req.wants_json() { #[derive(Serialize)] @@ -39,7 +39,8 @@ pub fn download(req: &mut dyn Request) -> CargoResult { /// Increment the download counts for a given crate version. /// -/// Returns an error if we could not load the version ID from the database. +/// Returns the crate name as stored in the database, or an error if we could +/// not load the version ID from the database. /// /// This ignores any errors that occur updating the download count. Failure is /// expected if the application is in read only mode, or for API-only mirrors. @@ -49,20 +50,21 @@ fn increment_download_counts( req: &dyn Request, crate_name: &str, version: &str, -) -> CargoResult<()> { +) -> CargoResult { use self::versions::dsl::*; let conn = req.db_conn()?; - let version_id = versions - .select(id) - .filter(crate_id.eq_any(Crate::by_name(crate_name).select(crates::id))) + let (version_id, crate_name) = versions + .inner_join(crates::table) + .select((id, crates::name)) + .filter(Crate::with_name(crate_name)) .filter(num.eq(version)) .first(&*conn)?; // Wrap in a transaction so we don't poison the outer transaction if this // fails let _ = conn.transaction(|| VersionDownload::create_or_increment(version_id, &conn)); - Ok(()) + Ok(crate_name) } /// Handles the `GET /crates/:crate_id/:version/downloads` route. diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 84c623a7ea5..0ad371f5408 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1310,6 +1310,23 @@ fn download_nonexistent_version_of_existing_crate_404s() { .assert_not_found(); } +#[test] +fn download_noncanonical_crate_name() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + app.db(|conn| { + CrateBuilder::new("foo_download", user.id) + .version(VersionBuilder::new("1.0.0")) + .expect_build(conn); + }); + + // Request download for "foo-download" with a dash instead of an underscore, + // and assert that the correct download link is returned. + anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download") + .assert_redirect_ends_with("/crates/foo_download/foo_download-1.0.0.crate"); +} + #[test] fn dependencies() { let (app, anon, user) = TestApp::init().with_user(); diff --git a/src/tests/util.rs b/src/tests/util.rs index 8cd15f98bfe..9f9dc317bc7 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -556,6 +556,11 @@ where assert_eq!(status, self.response.status.0); self } + + pub fn assert_redirect_ends_with(&self, target: &str) -> &Self { + assert!(self.response.headers["Location"][0].ends_with(target)); + self + } } impl Response<()> {