Merge pull request #7185 from Turbo87/publish-validation

models/krate: Move URL and name validation out of `create_or_update()`

Author: Turbo87

src/controllers/krate/publish.rs

@@ -4,10 +4,12 @@ use crate::auth::AuthCheck;
 use crate::background_jobs::{Job, PRIORITY_RENDER_README};
 use axum::body::Bytes;
 use crates_io_tarball::{process_tarball, TarballError};
+use diesel::dsl::{exists, select};
 use hex::ToHex;
 use hyper::body::Buf;
 use sha2::{Digest, Sha256};
 use tokio::runtime::Handle;
+use url::Url;
 
 use crate::controllers::cargo_prelude::*;
 use crate::models::{
@@ -19,6 +21,7 @@ use crate::middleware::log_request::RequestLogExt;
 use crate::models::token::EndpointScope;
 use crate::rate_limiter::LimitedAction;
 use crate::schema::*;
+use crate::sql::canon_crate_name;
 use crate::util::errors::{cargo_err, internal, AppResult};
 use crate::util::Maximums;
 use crate::views::{
@@ -142,6 +145,15 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
             };
 
             let license_file = metadata.license_file.as_deref();
+
+            validate_url(persist.homepage, "homepage")?;
+            validate_url(persist.documentation, "documentation")?;
+            validate_url(persist.repository, "repository")?;
+
+            if is_reserved_name(persist.name, conn)? {
+                return Err(cargo_err("cannot upload a crate with a reserved name"));
+            }
+
             let krate = persist.create_or_update(conn, user.id)?;
 
             let owners = krate.owners(conn)?;
@@ -332,6 +344,32 @@ fn split_body(mut bytes: Bytes) -> AppResult<(Bytes, Bytes)> {
     Ok((json_bytes, tarball_bytes))
 }
 
+fn is_reserved_name(name: &str, conn: &mut PgConnection) -> QueryResult<bool> {
+    select(exists(reserved_crate_names::table.filter(
+        canon_crate_name(reserved_crate_names::name).eq(canon_crate_name(name)),
+    )))
+    .get_result(conn)
+}
+
+fn validate_url(url: Option<&str>, field: &str) -> AppResult<()> {
+    let Some(url) = url else {
+        return Ok(());
+    };
+
+    // Manually check the string, as `Url::parse` may normalize relative URLs
+    // making it difficult to ensure that both slashes are present.
+    if !url.starts_with("http://") && !url.starts_with("https://") {
+        return Err(cargo_err(&format_args!(
+            "URL for field `{field}` must begin with http:// or https:// (url: {url})"
+        )));
+    }
+
+    // Ensure the entire URL parses as well
+    Url::parse(url)
+        .map_err(|_| cargo_err(&format_args!("`{field}` is not a valid url: `{url}`")))?;
+    Ok(())
+}
+
 fn missing_metadata_error_message(missing: &[&str]) -> String {
     format!(
         "missing or empty metadata fields: {}. Please \
@@ -438,7 +476,12 @@ impl From<TarballError> for BoxedAppError {
 
 #[cfg(test)]
 mod tests {
-    use super::missing_metadata_error_message;
+    use super::{missing_metadata_error_message, validate_url};
+
+    #[test]
+    fn deny_relative_urls() {
+        assert_err!(validate_url(Some("https:/example.com/home"), "homepage"));
+    }
 
     #[test]
     fn missing_metadata_error_message_test() {

src/models/krate.rs

@@ -5,7 +5,6 @@ use diesel::associations::Identifiable;
 use diesel::pg::Pg;
 use diesel::prelude::*;
 use diesel::sql_types::{Bool, Text};
-use url::Url;
 
 use crate::app::App;
 use crate::controllers::helpers::pagination::*;
@@ -105,9 +104,6 @@ impl<'a> NewCrate<'a> {
     pub fn create_or_update(self, conn: &mut PgConnection, uploader: i32) -> AppResult<Crate> {
         use diesel::update;
 
-        self.validate()?;
-        self.ensure_name_not_reserved(conn)?;
-
         conn.transaction(|conn| {
             // To avoid race conditions, we try to insert
             // first so we know whether to add an owner
@@ -124,49 +120,6 @@ impl<'a> NewCrate<'a> {
         })
     }
 
-    fn validate(&self) -> AppResult<()> {
-        fn validate_url(url: Option<&str>, field: &str) -> AppResult<()> {
-            let url = match url {
-                Some(s) => s,
-                None => return Ok(()),
-            };
-
-            // Manually check the string, as `Url::parse` may normalize relative URLs
-            // making it difficult to ensure that both slashes are present.
-            if !url.starts_with("http://") && !url.starts_with("https://") {
-                return Err(cargo_err(&format_args!(
-                    "URL for field `{field}` must begin with http:// or https:// (url: {url})"
-                )));
-            }
-
-            // Ensure the entire URL parses as well
-            Url::parse(url)
-                .map_err(|_| cargo_err(&format_args!("`{field}` is not a valid url: `{url}`")))?;
-            Ok(())
-        }
-
-        validate_url(self.homepage, "homepage")?;
-        validate_url(self.documentation, "documentation")?;
-        validate_url(self.repository, "repository")?;
-        Ok(())
-    }
-
-    fn ensure_name_not_reserved(&self, conn: &mut PgConnection) -> AppResult<()> {
-        use crate::schema::reserved_crate_names::dsl::*;
-        use diesel::dsl::exists;
-        use diesel::select;
-
-        let reserved_name: bool = select(exists(
-            reserved_crate_names.filter(canon_crate_name(name).eq(canon_crate_name(self.name))),
-        ))
-        .get_result(conn)?;
-        if reserved_name {
-            Err(cargo_err("cannot upload a crate with a reserved name"))
-        } else {
-            Ok(())
-        }
-    }
-
     fn save_new_crate(&self, conn: &mut PgConnection, user_id: i32) -> QueryResult<Option<Crate>> {
         use crate::schema::crates::dsl::*;
 
@@ -521,21 +474,7 @@ impl Crate {
 
 #[cfg(test)]
 mod tests {
-    use crate::models::{Crate, NewCrate};
-
-    #[test]
-    fn deny_relative_urls() {
-        let krate = NewCrate {
-            name: "name",
-            description: None,
-            homepage: Some("https:/example.com/home"),
-            documentation: None,
-            readme: None,
-            repository: None,
-            max_upload_size: None,
-        };
-        assert_err!(krate.validate());
-    }
+    use crate::models::Crate;
 
     #[test]
     fn valid_name() {

src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_urls.snap

@@ -0,0 +1,11 @@
+---
+source: src/tests/krate/publish/validation.rs
+expression: response.into_json()
+---
+{
+  "errors": [
+    {
+      "detail": "URL for field `documentation` must begin with http:// or https:// (url: javascript:alert('boom'))"
+    }
+  ]
+}

src/tests/krate/publish/validation.rs

@@ -86,3 +86,16 @@ fn license_and_description_required() {
 
     assert!(app.stored_files().is_empty());
 }
+
+#[test]
+fn invalid_urls() {
+    let (app, _, _, token) = TestApp::full().with_token();
+
+    let response = token.publish_crate(
+        PublishBuilder::new("foo", "1.0.0").documentation("javascript:alert('boom')"),
+    );
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_json_snapshot!(response.into_json());
+
+    assert!(app.stored_files().is_empty());
+}