Merge pull request #7234 from Turbo87/validate-dependencies

publish: Move dependency validation to `validate_dependency()` fn

Author: Turbo87

src/controllers/krate/publish.rs

@@ -14,6 +14,7 @@ use tokio::runtime::Handle;
 use url::Url;
 
 use crate::controllers::cargo_prelude::*;
+use crate::models::krate::MAX_NAME_LENGTH;
 use crate::models::{
     insert_version_owner_action, Category, Crate, DependencyKind, Keyword, NewCrate, NewVersion,
     Rights, VersionAction,
@@ -283,6 +284,10 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
                 VersionAction::Publish,
             )?;
 
+            for dep in &metadata.deps {
+                validate_dependency(dep)?;
+            }
+
             // Link this new version to all dependencies
             add_dependencies(conn, &metadata.deps, version.id)?;
 
@@ -429,6 +434,60 @@ fn missing_metadata_error_message(missing: &[&str]) -> String {
     )
 }
 
+pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
+    if !Crate::valid_name(&dep.name) {
+        return Err(cargo_err(&format_args!(
+            "\"{}\" is an invalid dependency name (dependency names must \
+            start with a letter, contain only letters, numbers, hyphens, \
+            or underscores and have at most {MAX_NAME_LENGTH} characters)",
+            dep.name
+        )));
+    }
+
+    for feature in &dep.features {
+        if !Crate::valid_feature(feature) {
+            return Err(cargo_err(&format_args!(
+                "\"{feature}\" is an invalid feature name",
+            )));
+        }
+    }
+
+    if let Some(registry) = &dep.registry {
+        if !registry.is_empty() {
+            return Err(cargo_err(&format_args!("Dependency `{}` is hosted on another registry. Cross-registry dependencies are not permitted on crates.io.", dep.name)));
+        }
+    }
+
+    match semver::VersionReq::parse(&dep.version_req) {
+        Err(_) => {
+            return Err(cargo_err(&format_args!(
+                "\"{}\" is an invalid version requirement",
+                dep.version_req
+            )));
+        }
+        Ok(req) if req == semver::VersionReq::STAR => {
+            return Err(cargo_err(&format_args!("wildcard (`*`) dependency constraints are not allowed \
+                on crates.io. Crate with this problem: `{}` See https://doc.rust-lang.org/cargo/faq.html#can-\
+                libraries-use--as-a-version-for-their-dependencies for more \
+                information", dep.name)));
+        }
+        _ => {}
+    }
+
+    if let Some(toml_name) = &dep.explicit_name_in_toml {
+        if !Crate::valid_dependency_name(toml_name) {
+            return Err(cargo_err(&format_args!(
+                "\"{toml_name}\" is an invalid dependency name (dependency \
+                names must start with a letter or underscore, contain only \
+                letters, numbers, hyphens, or underscores and have at most \
+                {MAX_NAME_LENGTH} characters)"
+            )));
+        }
+    }
+
+    Ok(())
+}
+
 #[instrument(skip_all)]
 pub fn add_dependencies(
     conn: &mut PgConnection,
@@ -439,33 +498,21 @@ pub fn add_dependencies(
 
     let crate_ids = crates::table
         .select((crates::name, crates::id))
-        .filter(crates::name.eq_any(deps.iter().map(|d| &d.name.0)))
+        .filter(crates::name.eq_any(deps.iter().map(|d| &d.name)))
         .load_iter::<(String, i32), DefaultLoadingMode>(conn)?
         .collect::<QueryResult<HashMap<_, _>>>()?;
 
     let new_dependencies = deps
         .iter()
         .map(|dep| {
-            if let Some(registry) = &dep.registry {
-                if !registry.is_empty() {
-                    return Err(cargo_err(&format_args!("Dependency `{}` is hosted on another registry. Cross-registry dependencies are not permitted on crates.io.", &*dep.name)));
-                }
-            }
-
             // Match only identical names to ensure the index always references the original crate name
-            let Some(&crate_id) = crate_ids.get(&dep.name.0) else {
-                return Err(cargo_err(&format_args!("no known crate named `{}`", &*dep.name)));
+            let Some(&crate_id) = crate_ids.get(&dep.name) else {
+                return Err(cargo_err(&format_args!(
+                    "no known crate named `{}`",
+                    dep.name
+                )));
             };
 
-            if let Ok(version_req) = semver::VersionReq::parse(&dep.version_req.0) {
-                 if version_req == semver::VersionReq::STAR {
-                    return Err(cargo_err(&format_args!("wildcard (`*`) dependency constraints are not allowed \
-                        on crates.io. Crate with this problem: `{}` See https://doc.rust-lang.org/cargo/faq.html#can-\
-                        libraries-use--as-a-version-for-their-dependencies for more \
-                        information", &*dep.name)));
-                }
-            }
-
             Ok((
                 dependencies::version_id.eq(version_id),
                 dependencies::crate_id.eq(crate_id),
@@ -475,7 +522,7 @@ pub fn add_dependencies(
                 dependencies::default_features.eq(dep.default_features),
                 dependencies::features.eq(&dep.features),
                 dependencies::target.eq(dep.target.as_deref()),
-                dependencies::explicit_name.eq(dep.explicit_name_in_toml.as_deref())
+                dependencies::explicit_name.eq(dep.explicit_name_in_toml.as_deref()),
             ))
         })
         .collect::<Result<Vec<_>, _>>()?;

src/tests/builders/dependency.rs

@@ -2,10 +2,11 @@ use crates_io::views::krate_publish as u;
 
 /// A builder for constructing a dependency of another crate.
 pub struct DependencyBuilder {
-    explicit_name_in_toml: Option<u::EncodableDependencyName>,
+    explicit_name_in_toml: Option<String>,
     name: String,
+    features: Vec<String>,
     registry: Option<String>,
-    version_req: u::EncodableCrateVersionReq,
+    version_req: String,
 }
 
 impl DependencyBuilder {
@@ -14,14 +15,15 @@ impl DependencyBuilder {
         DependencyBuilder {
             explicit_name_in_toml: None,
             name: name.to_string(),
+            features: vec![],
             registry: None,
-            version_req: u::EncodableCrateVersionReq("> 0".to_string()),
+            version_req: "> 0".to_string(),
         }
     }
 
     /// Rename this dependency.
     pub fn rename(mut self, new_name: &str) -> Self {
-        self.explicit_name_in_toml = Some(u::EncodableDependencyName(new_name.to_string()));
+        self.explicit_name_in_toml = Some(new_name.to_string());
         self
     }
 
@@ -38,18 +40,23 @@ impl DependencyBuilder {
     /// Panics if the `version_req` string specified isn't a valid `semver::VersionReq`.
     #[track_caller]
     pub fn version_req(mut self, version_req: &str) -> Self {
-        self.version_req = u::EncodableCrateVersionReq(version_req.to_string());
+        self.version_req = version_req.to_string();
+        self
+    }
+
+    pub fn add_feature<T: Into<String>>(mut self, feature: T) -> Self {
+        self.features.push(feature.into());
         self
     }
 
     /// Consume this builder to create a `u::CrateDependency`. If the dependent crate doesn't
     /// already exist, publishing a crate with this dependency will fail.
     pub fn build(self) -> u::EncodableCrateDependency {
         u::EncodableCrateDependency {
-            name: u::EncodableCrateName(self.name),
+            name: self.name,
             optional: false,
             default_features: true,
-            features: Vec::new(),
+            features: self.features,
             version_req: self.version_req,
             target: None,
             kind: None,

src/tests/builders/publish.rs

@@ -236,17 +236,10 @@ fn convert_dependency(
         ),
     };
 
-    let features = encoded
-        .features
-        .iter()
-        .map(|f| f.to_string())
-        .collect::<Vec<_>>()
-        .none_or_filled();
-
     let dependency = DependencyDetail {
         version: Some(encoded.version_req.to_string()),
         registry: encoded.registry.clone(),
-        features,
+        features: encoded.features.clone().none_or_filled(),
         optional: match encoded.optional {
             true => Some(true),
             false => None,

src/tests/krate/publish/dependencies.rs

@@ -3,6 +3,18 @@ use crate::util::{RequestHelper, TestApp};
 use http::StatusCode;
 use insta::assert_json_snapshot;
 
+#[test]
+fn invalid_dependency_name() {
+    let (app, _, _, token) = TestApp::full().with_token();
+
+    let response = token.publish_crate(
+        PublishBuilder::new("foo", "1.0.0").dependency(DependencyBuilder::new("🦀")),
+    );
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_json_snapshot!(response.into_json());
+    assert!(app.stored_files().is_empty());
+}
+
 #[test]
 fn new_with_renamed_dependency() {
     let (app, _, user, token) = TestApp::full().with_token();
@@ -211,3 +223,16 @@ fn new_krate_sorts_deps() {
     let crates = app.crates_from_index_head("two-deps");
     assert_json_snapshot!(crates);
 }
+
+#[test]
+fn invalid_feature_name() {
+    let (app, _, _, token) = TestApp::full().with_token();
+
+    let response = token.publish_crate(
+        PublishBuilder::new("foo", "1.0.0")
+            .dependency(DependencyBuilder::new("bar").add_feature("🍺")),
+    );
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_json_snapshot!(response.into_json());
+    assert!(app.stored_files().is_empty());
+}

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

@@ -0,0 +1,11 @@
+---
+source: src/tests/krate/publish/dependencies.rs
+expression: response.into_json()
+---
+{
+  "errors": [
+    {
+      "detail": "\"🦀\" is an invalid dependency name (dependency names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)"
+    }
+  ]
+}

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

@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "invalid upload request: invalid value: string \"💩\", expected a valid dependency name to start with a letter or underscore, contain only letters, numbers, hyphens, or underscores and have at most 64 characters at line 1 column 197"
+      "detail": "\"💩\" is an invalid dependency name (dependency names must start with a letter or underscore, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)"
     }
   ]
 }

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

@@ -0,0 +1,11 @@
+---
+source: src/tests/krate/publish/dependencies.rs
+expression: response.into_json()
+---
+{
+  "errors": [
+    {
+      "detail": "\"🍺\" is an invalid feature name"
+    }
+  ]
+}

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

@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "invalid upload request: invalid value: string \"broken\", expected a valid version req at line 1 column 136"
+      "detail": "\"broken\" is an invalid version requirement"
     }
   ]
 }

src/views/krate_publish.rs

@@ -3,9 +3,6 @@
 //! to and from structs. The serializing is only utilised in
 //! integration tests.
 
-use diesel::pg::Pg;
-use diesel::serialize::{self, Output, ToSql};
-use diesel::sql_types::Text;
 use serde::{de, Deserialize, Deserializer, Serialize};
 
 use crate::models::krate::MAX_NAME_LENGTH;
@@ -26,12 +23,12 @@ pub struct PublishMetadata {
 pub struct EncodableCrateDependency {
     pub optional: bool,
     pub default_features: bool,
-    pub name: EncodableCrateName,
-    pub features: Vec<EncodableFeature>,
-    pub version_req: EncodableCrateVersionReq,
+    pub name: String,
+    pub features: Vec<String>,
+    pub version_req: String,
     pub target: Option<String>,
     pub kind: Option<DependencyKind>,
-    pub explicit_name_in_toml: Option<EncodableDependencyName>,
+    pub explicit_name_in_toml: Option<String>,
     pub registry: Option<String>,
 }
 
@@ -54,47 +51,6 @@ impl<'de> Deserialize<'de> for EncodableCrateName {
     }
 }
 
-#[derive(Serialize, Clone, Debug, Deref)]
-pub struct EncodableDependencyName(pub String);
-
-impl<'de> Deserialize<'de> for EncodableDependencyName {
-    fn deserialize<D: Deserializer<'de>>(d: D) -> Result<EncodableDependencyName, D::Error> {
-        let s = String::deserialize(d)?;
-        if !Crate::valid_dependency_name(&s) {
-            let value = de::Unexpected::Str(&s);
-            let expected = format!(
-                "a valid dependency name to start with a letter or underscore, contain only letters, \
-                 numbers, hyphens, or underscores and have at most {MAX_NAME_LENGTH} characters"
-            );
-            Err(de::Error::invalid_value(value, &expected.as_ref()))
-        } else {
-            Ok(EncodableDependencyName(s))
-        }
-    }
-}
-
-#[derive(Serialize, Clone, Debug, Deref)]
-pub struct EncodableFeature(pub String);
-
-impl<'de> Deserialize<'de> for EncodableFeature {
-    fn deserialize<D: Deserializer<'de>>(d: D) -> Result<EncodableFeature, D::Error> {
-        let s = String::deserialize(d)?;
-        if !Crate::valid_feature(&s) {
-            let value = de::Unexpected::Str(&s);
-            let expected = "a valid feature name";
-            Err(de::Error::invalid_value(value, &expected))
-        } else {
-            Ok(EncodableFeature(s))
-        }
-    }
-}
-
-impl ToSql<Text, Pg> for EncodableFeature {
-    fn to_sql(&self, out: &mut Output<'_, '_, Pg>) -> serialize::Result {
-        ToSql::<Text, Pg>::to_sql(&**self, &mut out.reborrow())
-    }
-}
-
 #[derive(Serialize, Debug, Deref)]
 pub struct EncodableCrateVersion(pub semver::Version);
 
@@ -111,32 +67,3 @@ impl<'de> Deserialize<'de> for EncodableCrateVersion {
         }
     }
 }
-
-#[derive(Serialize, Clone, Debug, Deref)]
-pub struct EncodableCrateVersionReq(pub String);
-
-impl<'de> Deserialize<'de> for EncodableCrateVersionReq {
-    fn deserialize<D: Deserializer<'de>>(d: D) -> Result<EncodableCrateVersionReq, D::Error> {
-        let s = String::deserialize(d)?;
-        match semver::VersionReq::parse(&s) {
-            Ok(_) => Ok(EncodableCrateVersionReq(s)),
-            Err(..) => {
-                let value = de::Unexpected::Str(&s);
-                let expected = "a valid version req";
-                Err(de::Error::invalid_value(value, &expected))
-            }
-        }
-    }
-}
-
-#[test]
-fn feature_deserializes_for_valid_features() {
-    use serde_json as json;
-
-    assert_ok!(json::from_str::<EncodableFeature>("\"foo\""));
-    assert_err!(json::from_str::<EncodableFeature>("\"\""));
-    assert_err!(json::from_str::<EncodableFeature>("\"/\""));
-    assert_err!(json::from_str::<EncodableFeature>("\"%/%\""));
-    assert_ok!(json::from_str::<EncodableFeature>("\"a/a\""));
-    assert_ok!(json::from_str::<EncodableFeature>("\"32-column-tables\""));
-}