Auto merge of #1758 - smarnach:download-non-standard-name, r=sgrif

Return correct download link for requests for non-standard crate names.

Fixes #1687.

Crates are uploaded to S3 under the name they were first published as, but the download endpoint always uses the name as written in the request. If these names differ in their use of dashes vs. underscores, the download endpoint returns an invalid link.

To avoid adding another database request for every single crate download, this fix changes the database request that updates the download count to also return the original crate name. We will need to confirm that the performance impact of this change is negligible.

Author: bors

src/controllers/version/downloads.rs

@@ -18,13 +18,13 @@ pub fn download(req: &mut dyn Request) -> CargoResult<Response> {
     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<Response> {
 
 /// 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<String> {
     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.

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();

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<()> {