diff --git a/Cargo.lock b/Cargo.lock index 4d2c92dd325..66a15583811 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -983,6 +983,7 @@ dependencies = [ "tracing", "tracing-subscriber", "typomania", + "unicode-xid", "url", ] @@ -4307,6 +4308,12 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e51733f11c9c4f72aa0c160008246859e340b00807569a0da0e7a1079b27ba85" +[[package]] +name = "unicode-xid" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f962df74c8c05a667b5ee8bcf162993134c104e96440b663c8daa176dc772d8c" + [[package]] name = "unicode_categories" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 40b6b5c5be0..657745f3ed0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 4d07390cba9..05c76e407b3 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -238,11 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult max_features { @@ -257,9 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult 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))); } } diff --git a/src/models/krate.rs b/src/models/krate.rs index be034d36b7c..30cb3381546 100644 --- a/src/models/krate.rs +++ b/src/models/krate.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 { @@ -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()); } } diff --git a/src/tests/krate/publish/features.rs b/src/tests/krate/publish/features.rs index 77eb8a26002..14cfe59963d 100644 --- a/src/tests/krate/publish/features.rs +++ b/src/tests/krate/publish/features.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() diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap index dda7e3da25f..f9566fa6ba2 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap @@ -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`)" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap similarity index 68% rename from src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap rename to src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap index 8abeeb76659..508aa219266 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_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" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_start_with_number_and_underscore.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_start_with_number_and_underscore.snap new file mode 100644 index 00000000000..534f706643c --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_start_with_number_and_underscore.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 + } +] diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_dot.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_dot.snap new file mode 100644 index 00000000000..7f63ceefb5e --- /dev/null +++ b/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": "d0bfdbcd4905a15b3dc6db5ce23e206ac413b4d780053fd38e145a75197fb1e1", + "features": { + "foo.bar": [] + }, + "yanked": false + } +] diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_unicode_chars.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_unicode_chars.snap new file mode 100644 index 00000000000..6c81d7a7bc5 --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_unicode_chars.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 + } +] diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name.snap deleted file mode 100644 index 1f5b30cc585..00000000000 --- a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name.snap +++ /dev/null @@ -1,11 +0,0 @@ ---- -source: src/tests/krate/publish/features.rs -expression: response.into_json() ---- -{ - "errors": [ - { - "detail": "\"~foo\" is an invalid feature name (feature names must contain only letters, numbers, '-', '+', or '_')" - } - ] -} diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name1.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name1.snap new file mode 100644 index 00000000000..3df54b3eb26 --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name1.snap @@ -0,0 +1,11 @@ +--- +source: src/tests/krate/publish/features.rs +expression: response.into_json() +--- +{ + "errors": [ + { + "detail": "invalid character `~` in feature `~foo`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)" + } + ] +} diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name2.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name2.snap new file mode 100644 index 00000000000..8d5ba6c9916 --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name2.snap @@ -0,0 +1,11 @@ +--- +source: src/tests/krate/publish/features.rs +expression: response.into_json() +--- +{ + "errors": [ + { + "detail": "invalid character `!` in feature `!bar`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)" + } + ] +} diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name_start_with_hyphen.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name_start_with_hyphen.snap new file mode 100644 index 00000000000..28d85852bd3 --- /dev/null +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name_start_with_hyphen.snap @@ -0,0 +1,11 @@ +--- +source: src/tests/krate/publish/features.rs +expression: response.into_json() +--- +{ + "errors": [ + { + "detail": "invalid character `-` in feature `-foo1.bar`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)" + } + ] +}