Merge pull request #7500 from hi-rustin/rustin-patch-feature-name

Use the same feature name validation rule from Cargo

Author: Turbo87

Cargo.lock

@@ -100,6 +100,7 @@ tracing = "=0.1.40"
 tracing-subscriber = { version = "=0.3.18", features = ["env-filter"] }
 typomania = { version = "=0.1.2", default-features = false }
 url = "=2.4.1"
+unicode-xid = "0.2.4"
 
 [dev-dependencies]
 bytes = "=1.5.0"

Cargo.toml

@@ -238,11 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
         }
 
         for (key, values) in features.iter() {
-            if !Crate::valid_feature_name(key) {
-                return Err(cargo_err(&format!(
-                    "\"{key}\" is an invalid feature name (feature names must contain only letters, numbers, '-', '+', or '_')"
-                )));
-            }
+            Crate::valid_feature_name(key)?;
 
             let num_features = values.len();
             if num_features > max_features {
@@ -257,9 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
             }
 
             for value in values.iter() {
-                if !Crate::valid_feature(value) {
-                    return Err(cargo_err(&format!("\"{value}\" is an invalid feature name")));
-                }
+                Crate::valid_feature(value)?;
             }
         }
 
@@ -602,11 +596,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
     }
 
     for feature in &dep.features {
-        if !Crate::valid_feature(feature) {
-            return Err(cargo_err(&format_args!(
-                "\"{feature}\" is an invalid feature name",
-            )));
-        }
+        Crate::valid_feature(feature)?;
     }
 
     if let Some(registry) = &dep.registry {
@@ -633,12 +623,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
 
     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)"
-            )));
+            return Err(cargo_err(&Crate::invalid_dependency_name_msg(toml_name)));
         }
     }
 

src/controllers/krate/publish.rs

@@ -220,12 +220,68 @@ impl Crate {
                 .unwrap_or(false)
     }
 
-    /// Validates the THIS parts of `features = ["THIS", "and/THIS"]`.
-    pub fn valid_feature_name(name: &str) -> bool {
-        !name.is_empty()
-            && name
-                .chars()
-                .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '+')
+    pub fn invalid_dependency_name_msg(dep: &str) -> String {
+        format!(
+            "\"{dep}\" 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)"
+        )
+    }
+
+    /// Validates the THIS parts of `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`.
+    /// 1. The name must be non-empty.
+    /// 2. The first character must be a Unicode XID start character, `_`, or a digit.
+    /// 3. The remaining characters must be Unicode XID characters, `_`, `+`, `-`, or `.`.
+    pub fn valid_feature_name(name: &str) -> AppResult<()> {
+        if name.is_empty() {
+            return Err(cargo_err("feature cannot be an empty"));
+        }
+        let mut chars = name.chars();
+        if let Some(ch) = chars.next() {
+            if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_ascii_digit()) {
+                return Err(cargo_err(&format!(
+                    "invalid character `{}` in feature `{}`, \
+                the first character must be a Unicode XID start character or digit \
+                (most letters or `_` or `0` to `9`)",
+                    ch, name,
+                )));
+            }
+        }
+        for ch in chars {
+            if !(unicode_xid::UnicodeXID::is_xid_continue(ch)
+                || ch == '+'
+                || ch == '-'
+                || ch == '.')
+            {
+                return Err(cargo_err(&format!(
+                    "invalid character `{}` in feature `{}`, \
+                characters must be Unicode XID characters, `+`, `-`, or `.` \
+                (numbers, `+`, `-`, `_`, `.`, or most letters)",
+                    ch, name,
+                )));
+            }
+        }
+
+        Ok(())
+    }
+
+    /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`.
+    pub fn valid_feature(name: &str) -> AppResult<()> {
+        if let Some((dep, dep_feat)) = name.split_once('/') {
+            let dep = dep.strip_suffix('?').unwrap_or(dep);
+            if !Crate::valid_dependency_name(dep) {
+                return Err(cargo_err(&Crate::invalid_dependency_name_msg(dep)));
+            }
+            Crate::valid_feature_name(dep_feat)
+        } else if let Some((_, dep)) = name.split_once("dep:") {
+            if !Crate::valid_dependency_name(dep) {
+                return Err(cargo_err(&Crate::invalid_dependency_name_msg(dep)));
+            }
+            return Ok(());
+        } else {
+            Crate::valid_feature_name(name)
+        }
     }
 
     /// Validates the prefix in front of the slash: `features = ["THIS/feature"]`.
@@ -237,17 +293,6 @@ impl Crate {
                 .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
     }
 
-    /// Validates a whole feature string, `features = ["THIS", "ALL/THIS"]`.
-    pub fn valid_feature(name: &str) -> bool {
-        match name.split_once('/') {
-            Some((dep, dep_feat)) => {
-                let dep = dep.strip_suffix('?').unwrap_or(dep);
-                Crate::valid_feature_prefix(dep) && Crate::valid_feature_name(dep_feat)
-            }
-            None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)),
-        }
-    }
-
     /// Return both the newest (most recently updated) and
     /// highest version (in semver order) for the current crate.
     pub fn top_versions(&self, conn: &mut PgConnection) -> QueryResult<TopVersions> {
@@ -514,22 +559,29 @@ mod tests {
         assert!(Crate::valid_dependency_name("_foo"));
         assert!(!Crate::valid_dependency_name("-foo"));
     }
-
     #[test]
     fn valid_feature_names() {
-        assert!(Crate::valid_feature("foo"));
-        assert!(!Crate::valid_feature(""));
-        assert!(!Crate::valid_feature("/"));
-        assert!(!Crate::valid_feature("%/%"));
-        assert!(Crate::valid_feature("a/a"));
-        assert!(Crate::valid_feature("32-column-tables"));
-        assert!(Crate::valid_feature("c++20"));
-        assert!(Crate::valid_feature("krate/c++20"));
-        assert!(!Crate::valid_feature("c++20/wow"));
-        assert!(Crate::valid_feature("foo?/bar"));
-        assert!(Crate::valid_feature("dep:foo"));
-        assert!(!Crate::valid_feature("dep:foo?/bar"));
-        assert!(!Crate::valid_feature("foo/?bar"));
-        assert!(!Crate::valid_feature("foo?bar"));
+        assert!(Crate::valid_feature("foo").is_ok());
+        assert!(Crate::valid_feature("1foo").is_ok());
+        assert!(Crate::valid_feature("_foo").is_ok());
+        assert!(Crate::valid_feature("_foo-_+.1").is_ok());
+        assert!(Crate::valid_feature("_foo-_+.1").is_ok());
+        assert!(Crate::valid_feature("").is_err());
+        assert!(Crate::valid_feature("/").is_err());
+        assert!(Crate::valid_feature("%/%").is_err());
+        assert!(Crate::valid_feature("a/a").is_ok());
+        assert!(Crate::valid_feature("32-column-tables").is_ok());
+        assert!(Crate::valid_feature("c++20").is_ok());
+        assert!(Crate::valid_feature("krate/c++20").is_ok());
+        assert!(Crate::valid_feature("c++20/wow").is_err());
+        assert!(Crate::valid_feature("foo?/bar").is_ok());
+        assert!(Crate::valid_feature("dep:foo").is_ok());
+        assert!(Crate::valid_feature("dep:foo?/bar").is_err());
+        assert!(Crate::valid_feature("foo/?bar").is_err());
+        assert!(Crate::valid_feature("foo?bar").is_err());
+        assert!(Crate::valid_feature("bar.web").is_ok());
+        assert!(Crate::valid_feature("foo/bar.web").is_ok());
+        assert!(Crate::valid_feature("dep:0foo").is_err());
+        assert!(Crate::valid_feature("0foo?/bar.web").is_err());
     }
 }

src/models/krate.rs

@@ -26,7 +26,46 @@ fn features_version_2() {
 }
 
 #[test]
-fn invalid_feature_name() {
+fn feature_name_with_dot() {
+    let (app, _, _, token) = TestApp::full().with_token();
+    let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("foo.bar", &[]);
+    token.publish_crate(crate_to_publish).good();
+    let crates = app.crates_from_index_head("foo");
+    assert_json_snapshot!(crates);
+}
+
+#[test]
+fn feature_name_start_with_number_and_underscore() {
+    let (app, _, _, token) = TestApp::full().with_token();
+    let crate_to_publish = PublishBuilder::new("foo", "1.0.0")
+        .feature("0foo1.bar", &[])
+        .feature("_foo2.bar", &[]);
+    token.publish_crate(crate_to_publish).good();
+    let crates = app.crates_from_index_head("foo");
+    assert_json_snapshot!(crates);
+}
+
+#[test]
+fn feature_name_with_unicode_chars() {
+    let (app, _, _, token) = TestApp::full().with_token();
+    let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("foo.你好世界", &[]);
+    token.publish_crate(crate_to_publish).good();
+    let crates = app.crates_from_index_head("foo");
+    assert_json_snapshot!(crates);
+}
+
+#[test]
+fn empty_feature_name() {
+    let (app, _, _, token) = TestApp::full().with_token();
+    let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("", &[]);
+    let response = token.publish_crate(crate_to_publish);
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_json_snapshot!(response.into_json());
+    assert!(app.stored_files().is_empty());
+}
+
+#[test]
+fn invalid_feature_name1() {
     let (app, _, _, token) = TestApp::full().with_token();
 
     let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("~foo", &[]);
@@ -37,7 +76,7 @@ fn invalid_feature_name() {
 }
 
 #[test]
-fn invalid_feature() {
+fn invalid_feature_name2() {
     let (app, _, _, token) = TestApp::full().with_token();
 
     let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("foo", &["!bar"]);
@@ -47,6 +86,16 @@ fn invalid_feature() {
     assert_that!(app.stored_files(), empty());
 }
 
+#[test]
+fn invalid_feature_name_start_with_hyphen() {
+    let (app, _, _, token) = TestApp::full().with_token();
+    let crate_to_publish = PublishBuilder::new("foo", "1.0.0").feature("-foo1.bar", &[]);
+    let response = token.publish_crate(crate_to_publish);
+    assert_eq!(response.status(), StatusCode::OK);
+    assert_json_snapshot!(response.into_json());
+    assert!(app.stored_files().is_empty());
+}
+
 #[test]
 fn too_many_features() {
     let (app, _, _, token) = TestApp::full()

src/tests/krate/publish/features.rs

@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "\"🍺\" is an invalid feature name"
+      "detail": "invalid character `🍺` in feature `🍺`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
     }
   ]
 }

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

@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "\"!bar\" is an invalid feature name"
+      "detail": "feature cannot be an empty"
     }
   ]
 }

src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap renamed to src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap

@@ -0,0 +1,17 @@
+---
+source: src/tests/krate/publish/features.rs
+expression: crates
+---
+[
+  {
+    "name": "foo",
+    "vers": "1.0.0",
+    "deps": [],
+    "cksum": "6f73fad556c46cdb740173ccc7a5f5bf64b8d954966be16963a08eb138e3c69c",
+    "features": {
+      "0foo1.bar": [],
+      "_foo2.bar": []
+    },
+    "yanked": false
+  }
+]

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

@@ -0,0 +1,16 @@
+---
+source: src/tests/krate/publish/features.rs
+expression: crates
+---
+[
+  {
+    "name": "foo",
+    "vers": "1.0.0",
+    "deps": [],
+    "cksum": "d0bfdbcd4905a15b3dc6db5ce23e206ac413b4d780053fd38e145a75197fb1e1",
+    "features": {
+      "foo.bar": []
+    },
+    "yanked": false
+  }
+]

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

@@ -0,0 +1,16 @@
+---
+source: src/tests/krate/publish/features.rs
+expression: crates
+---
+[
+  {
+    "name": "foo",
+    "vers": "1.0.0",
+    "deps": [],
+    "cksum": "493720846371607438c1a4eb90c9cc7d7286600ca9c4e2ca04151aad9563b47a",
+    "features": {
+      "foo.你好世界": []
+    },
+    "yanked": false
+  }
+]