From ddeedb9a22c85dd64eaeb6971ad880f3ab1d0db8 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sun, 29 Oct 2023 14:56:16 +0800 Subject: [PATCH 1/8] Allow `.` in feature name Signed-off-by: hi-rustin --- src/models/krate.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/models/krate.rs b/src/models/krate.rs index be034d36b7c..5ccf1b443dd 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -225,7 +225,7 @@ impl Crate { !name.is_empty() && name .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '+') + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '+' || c == '.') } /// Validates the prefix in front of the slash: `features = ["THIS/feature"]`. @@ -531,5 +531,7 @@ mod tests { assert!(!Crate::valid_feature("dep:foo?/bar")); assert!(!Crate::valid_feature("foo/?bar")); assert!(!Crate::valid_feature("foo?bar")); + assert!(Crate::valid_feature("bar.web")); + assert!(Crate::valid_feature("foo/bar.web")); } } From 3eb810757f0c1427cfc169811338cc74ffe57687 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sun, 29 Oct 2023 15:06:01 +0800 Subject: [PATCH 2/8] Add an integration test for feature name with dot Signed-off-by: hi-rustin --- src/tests/krate/publish/features.rs | 9 +++++++++ ...publish__features__feature_name_with_dot.snap | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 src/tests/krate/publish/snapshots/all__krate__publish__features__feature_name_with_dot.snap diff --git a/src/tests/krate/publish/features.rs b/src/tests/krate/publish/features.rs index 5ba029f9c29..268ccee7c4c 100644 --- a/src/tests/krate/publish/features.rs +++ b/src/tests/krate/publish/features.rs @@ -24,6 +24,15 @@ fn features_version_2() { assert_json_snapshot!(crates); } +#[test] +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 invalid_feature_name() { let (app, _, _, token) = TestApp::full().with_token(); 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 + } +] From 72356e4fcce8ba16372b3b5ea555e8226b084da1 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Mon, 6 Nov 2023 10:26:58 +0800 Subject: [PATCH 3/8] Add unicode-xid crate to dependencies Signed-off-by: hi-rustin --- Cargo.lock | 7 +++++++ Cargo.toml | 1 + 2 files changed, 8 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index cac630522ae..8cd46c08fb5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -965,6 +965,7 @@ dependencies = [ "tower-service", "tracing", "tracing-subscriber", + "unicode-xid", "url", ] @@ -4261,6 +4262,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 34e700b3c9c..ffc756b7e2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,6 +97,7 @@ tower-http = { version = "=0.4.4", features = ["add-extension", "fs", "catch-pan tracing = "=0.1.40" tracing-subscriber = { version = "=0.3.17", features = ["env-filter"] } url = "=2.4.1" +unicode-xid = "0.2.4" [dev-dependencies] bytes = "=1.5.0" From 22a9589c34f18497189b473281d298ccb9bd5506 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 7 Nov 2023 20:21:01 +0800 Subject: [PATCH 4/8] Use the same feature name validation rule from Cargo Signed-off-by: hi-rustin --- src/controllers/krate/publish.rs | 7 +-- src/models/krate.rs | 49 ++++++++++++++++--- ...blish__features__invalid_feature_name.snap | 2 +- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 9e520793de9..070d6b78156 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -238,12 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult max_features { return Err(cargo_err(&format!( diff --git a/src/models/krate.rs b/src/models/krate.rs index 5ccf1b443dd..272998d986e 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -220,12 +220,41 @@ 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 == '+' || c == '.') + /// 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 the prefix in front of the slash: `features = ["THIS/feature"]`. @@ -242,9 +271,9 @@ impl Crate { 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) + Crate::valid_feature_prefix(dep) && Crate::valid_feature_name(dep_feat).is_ok() } - None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)), + None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)).is_ok(), } } @@ -518,6 +547,10 @@ mod tests { #[test] fn valid_feature_names() { assert!(Crate::valid_feature("foo")); + assert!(Crate::valid_feature("1foo")); + assert!(Crate::valid_feature("_foo")); + assert!(Crate::valid_feature("_foo-_+.1")); + assert!(Crate::valid_feature("_foo-_+.1")); assert!(!Crate::valid_feature("")); assert!(!Crate::valid_feature("/")); assert!(!Crate::valid_feature("%/%")); 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 index 1f5b30cc585..3df54b3eb26 100644 --- 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 @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"~foo\" is an invalid feature name (feature names must contain only letters, numbers, '-', '+', or '_')" + "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`)" } ] } From 0b5f2625b8a08fd579bbc03dbbb8afa0dd574ad5 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 7 Nov 2023 21:23:19 +0800 Subject: [PATCH 5/8] Use the same crate name validation rule from Cargo Signed-off-by: hi-rustin --- src/controllers/krate/publish.rs | 28 +--- src/models/krate.rs | 123 ++++++++++-------- src/models/token/scopes.rs | 2 +- ...dependencies__invalid_dependency_name.snap | 2 +- ...pendencies__invalid_dependency_rename.snap | 2 +- ..._publish__validation__invalid_names-2.snap | 2 +- ..._publish__validation__invalid_names-3.snap | 2 +- ..._publish__validation__invalid_names-4.snap | 2 +- ..._publish__validation__invalid_names-5.snap | 2 +- ...e__publish__validation__invalid_names.snap | 2 +- 10 files changed, 83 insertions(+), 84 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 070d6b78156..4b1399fc80c 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -16,7 +16,6 @@ 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, @@ -53,14 +52,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult parsed, @@ -582,14 +574,7 @@ fn convert_dependency( } 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 - ))); - } + Crate::valid_name(&dep.name)?; for feature in &dep.features { if !Crate::valid_feature(feature) { @@ -622,14 +607,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)" - ))); - } + Crate::valid_dependency_name(toml_name)?; } Ok(()) diff --git a/src/models/krate.rs b/src/models/krate.rs index 272998d986e..ce6e261c86c 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -192,32 +192,63 @@ impl Crate { }) } - pub fn valid_name(name: &str) -> bool { - let under_max_length = name.chars().take(MAX_NAME_LENGTH + 1).count() <= MAX_NAME_LENGTH; - Crate::valid_ident(name) && under_max_length + pub fn valid_name(name: &str) -> AppResult<()> { + if name.chars().count() > MAX_NAME_LENGTH { + return Err(cargo_err(&format!( + "the name `{}` is too long (max {} characters)", + name, MAX_NAME_LENGTH + ))); + } + Crate::valid_ident(name, "crate name") } - fn valid_ident(name: &str) -> bool { - Self::valid_feature_prefix(name) - && name - .chars() - .next() - .map(char::is_alphabetic) - .unwrap_or(false) + pub fn valid_dependency_name(name: &str) -> AppResult<()> { + if name.chars().count() > MAX_NAME_LENGTH { + return Err(cargo_err(&format!( + "the name `{}` is too long (max {} characters)", + name, MAX_NAME_LENGTH + ))); + } + Crate::valid_ident(name, "dependency name") } - pub fn valid_dependency_name(name: &str) -> bool { - let under_max_length = name.chars().take(MAX_NAME_LENGTH + 1).count() <= MAX_NAME_LENGTH; - Crate::valid_dependency_ident(name) && under_max_length - } + // Checks that the name is a valid identifier. + // 1. The name must be non-empty. + // 2. The first character must be an ASCII character or `_`. + // 3. The remaining characters must be ASCII alphanumerics or `-` or `_`. + fn valid_ident(name: &str, what: &str) -> AppResult<()> { + if name.is_empty() { + return Err(cargo_err(&format!("the {} cannot be an empty", what))); + } + let mut chars = name.chars(); + if let Some(ch) = chars.next() { + if ch.is_ascii_digit() { + return Err(cargo_err(&format!( + "the name `{}` cannot be used as a {}, \ + the name cannot start with a digit", + name, what, + ))); + } + if !(ch.is_ascii_alphabetic() || ch == '_') { + return Err(cargo_err(&format!( + "invalid character `{}` in {}: `{}`, \ + the first character must be an ASCII character, or `_`", + ch, what, name + ))); + } + } - fn valid_dependency_ident(name: &str) -> bool { - Self::valid_feature_prefix(name) - && name - .chars() - .next() - .map(|n| n.is_alphabetic() || n == '_') - .unwrap_or(false) + for ch in chars { + if !(ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') { + return Err(cargo_err(&format!( + "invalid character `{}` in {}: `{}`, \ + characters must be an ASCII alphanumeric characters, `-`, or `_`", + ch, what, name + ))); + } + } + + Ok(()) } /// Validates the THIS parts of `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`. @@ -257,21 +288,13 @@ impl Crate { Ok(()) } - /// Validates the prefix in front of the slash: `features = ["THIS/feature"]`. - /// Normally this corresponds to the crate name of a dependency. - fn valid_feature_prefix(name: &str) -> bool { - !name.is_empty() - && name - .chars() - .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).is_ok() + Crate::valid_dependency_name(dep).is_ok() + && Crate::valid_feature_name(dep_feat).is_ok() } None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)).is_ok(), } @@ -518,30 +541,28 @@ mod tests { #[test] fn valid_name() { - assert!(Crate::valid_name("foo")); - assert!(!Crate::valid_name("京")); - assert!(!Crate::valid_name("")); - assert!(!Crate::valid_name("💝")); - assert!(Crate::valid_name("foo_underscore")); - assert!(Crate::valid_name("foo-dash")); - assert!(!Crate::valid_name("foo+plus")); - // Starting with an underscore is an invalid crate name. - assert!(!Crate::valid_name("_foo")); - assert!(!Crate::valid_name("-foo")); + assert!(Crate::valid_name("foo").is_ok()); + assert!(Crate::valid_name("京").is_err()); + assert!(Crate::valid_name("").is_err()); + assert!(Crate::valid_name("💝").is_err()); + assert!(Crate::valid_name("foo_underscore").is_ok()); + assert!(Crate::valid_name("foo-dash").is_ok()); + assert!(Crate::valid_name("foo+plus").is_err()); + assert!(Crate::valid_name("_foo").is_ok()); + assert!(Crate::valid_name("-foo").is_err()); } #[test] fn valid_dependency_name() { - assert!(Crate::valid_dependency_name("foo")); - assert!(!Crate::valid_dependency_name("京")); - assert!(!Crate::valid_dependency_name("")); - assert!(!Crate::valid_dependency_name("💝")); - assert!(Crate::valid_dependency_name("foo_underscore")); - assert!(Crate::valid_dependency_name("foo-dash")); - assert!(!Crate::valid_dependency_name("foo+plus")); - // Starting with an underscore is a valid dependency name. - assert!(Crate::valid_dependency_name("_foo")); - assert!(!Crate::valid_dependency_name("-foo")); + assert!(Crate::valid_dependency_name("foo").is_ok()); + assert!(Crate::valid_dependency_name("京").is_err()); + assert!(Crate::valid_dependency_name("").is_err()); + assert!(Crate::valid_dependency_name("💝").is_err()); + assert!(Crate::valid_dependency_name("foo_underscore").is_ok()); + assert!(Crate::valid_dependency_name("foo-dash").is_ok()); + assert!(Crate::valid_dependency_name("foo+plus").is_err()); + assert!(Crate::valid_dependency_name("_foo").is_ok()); + assert!(Crate::valid_dependency_name("-foo").is_err()); } #[test] diff --git a/src/models/token/scopes.rs b/src/models/token/scopes.rs index 2a4127c39ca..67c4b94ccb0 100644 --- a/src/models/token/scopes.rs +++ b/src/models/token/scopes.rs @@ -108,7 +108,7 @@ impl CrateScope { } let name_without_wildcard = pattern.strip_suffix('*').unwrap_or(pattern); - Crate::valid_name(name_without_wildcard) + Crate::valid_name(name_without_wildcard).is_ok() } pub fn matches(&self, crate_name: &str) -> bool { diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap index 0e8461c375f..5cfc1602868 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap @@ -5,7 +5,7 @@ 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)" + "detail": "invalid character `🦀` in crate name: `🦀`, the first character must be an ASCII character, or `_`" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_rename.snap b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_rename.snap index 0fcc4196e97..d4d08413d9e 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_rename.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_rename.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "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)" + "detail": "invalid character `💩` in dependency name: `💩`, the first character must be an ASCII character, or `_`" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-2.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-2.snap index a550286d9af..1ebd0bfd064 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-2.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-2.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"foo bar\" is an invalid crate name (crate names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)" + "detail": "invalid character ` ` in crate name: `foo bar`, characters must be an ASCII alphanumeric characters, `-`, or `_`" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-3.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-3.snap index 4c5037fb9e7..75fc2ec30a2 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-3.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-3.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" is an invalid crate name (crate names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)" + "detail": "the name `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa` is too long (max 64 characters)" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-4.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-4.snap index e089171f337..9d8c103d692 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-4.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-4.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"snow☃\" is an invalid crate name (crate names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)" + "detail": "invalid character `☃` in crate name: `snow☃`, characters must be an ASCII alphanumeric characters, `-`, or `_`" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap index 0975911a29f..58222d36ce1 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"áccênts\" is an invalid crate name (crate names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)" + "detail": "invalid character `á` in crate name: `áccênts`, the first character must be an ASCII character, or `_`" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names.snap index 895e92e797f..89555ae1a8e 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"\" is an invalid crate name (crate names must start with a letter, contain only letters, numbers, hyphens, or underscores and have at most 64 characters)" + "detail": "the crate name cannot be an empty" } ] } From 028a23513208fdf425b5448266479d75c302a225 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 7 Nov 2023 21:31:40 +0800 Subject: [PATCH 6/8] Refactor valid_feature function to return AppResult. Signed-off-by: hi-rustin --- src/controllers/krate/publish.rs | 10 +--- src/models/krate.rs | 50 +++++++++---------- ...h__dependencies__invalid_feature_name.snap | 2 +- ...e__publish__features__invalid_feature.snap | 2 +- 4 files changed, 29 insertions(+), 35 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 4b1399fc80c..4a9f9cea38f 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -244,9 +244,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult AppResult<()> { Crate::valid_name(&dep.name)?; 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 { diff --git a/src/models/krate.rs b/src/models/krate.rs index ce6e261c86c..12b9ef047b9 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -288,15 +288,15 @@ impl Crate { Ok(()) } - /// Validates a whole feature string, `features = ["THIS", "ALL/THIS"]`. - pub fn valid_feature(name: &str) -> bool { + /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`. + pub fn valid_feature(name: &str) -> AppResult<()> { match name.split_once('/') { Some((dep, dep_feat)) => { let dep = dep.strip_suffix('?').unwrap_or(dep); - Crate::valid_dependency_name(dep).is_ok() - && Crate::valid_feature_name(dep_feat).is_ok() + Crate::valid_dependency_name(dep)?; + Crate::valid_feature_name(dep_feat) } - None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)).is_ok(), + None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)), } } @@ -567,25 +567,25 @@ mod tests { #[test] fn valid_feature_names() { - assert!(Crate::valid_feature("foo")); - assert!(Crate::valid_feature("1foo")); - assert!(Crate::valid_feature("_foo")); - assert!(Crate::valid_feature("_foo-_+.1")); - assert!(Crate::valid_feature("_foo-_+.1")); - 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("bar.web")); - assert!(Crate::valid_feature("foo/bar.web")); + 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()); } } 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__invalid_feature.snap index 8abeeb76659..8d5ba6c9916 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "\"!bar\" is an invalid feature name" + "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`)" } ] } From b23fe0737a5e97fbedbdbcfd1f250ac554bc58da Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 8 Nov 2023 20:35:28 +0800 Subject: [PATCH 7/8] Fix the validation for optional dep Signed-off-by: hi-rustin --- src/models/krate.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/models/krate.rs b/src/models/krate.rs index 12b9ef047b9..220f8d60017 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -290,13 +290,14 @@ impl Crate { /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`. pub fn valid_feature(name: &str) -> AppResult<()> { - match name.split_once('/') { - Some((dep, dep_feat)) => { - let dep = dep.strip_suffix('?').unwrap_or(dep); - Crate::valid_dependency_name(dep)?; - Crate::valid_feature_name(dep_feat) - } - None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)), + if let Some((dep, dep_feat)) = name.split_once('/') { + let dep = dep.strip_suffix('?').unwrap_or(dep); + Crate::valid_dependency_name(dep)?; + Crate::valid_feature_name(dep_feat) + } else if let Some((_, dep)) = name.split_once("dep:") { + Crate::valid_dependency_name(dep) + } else { + Crate::valid_feature_name(name) } } @@ -587,5 +588,7 @@ mod tests { 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()); } } From f47425f1714b33b15b9021e0df92e7ef213cd203 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Nov 2023 21:01:42 +0800 Subject: [PATCH 8/8] Do not allow create name starts with `_` Signed-off-by: hi-rustin --- src/models/krate.rs | 64 +++++++++++++++---- ...dependencies__invalid_dependency_name.snap | 2 +- ..._publish__validation__invalid_names-5.snap | 2 +- 3 files changed, 54 insertions(+), 14 deletions(-) diff --git a/src/models/krate.rs b/src/models/krate.rs index 220f8d60017..37082856fd2 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -199,7 +199,47 @@ impl Crate { name, MAX_NAME_LENGTH ))); } - Crate::valid_ident(name, "crate name") + Crate::valid_create_ident(name) + } + + // Checks that the name is a valid crate name. + // 1. The name must be non-empty. + // 2. The first character must be an ASCII character. + // 3. The remaining characters must be ASCII alphanumerics or `-` or `_`. + // Note: This differs from `valid_dependency_name`, which allows `_` as the first character. + fn valid_create_ident(name: &str) -> AppResult<()> { + if name.is_empty() { + return Err(cargo_err("the crate name cannot be an empty")); + } + let mut chars = name.chars(); + if let Some(ch) = chars.next() { + if ch.is_ascii_digit() { + return Err(cargo_err(&format!( + "the name `{}` cannot be used as a crate name, \ + the name cannot start with a digit", + name, + ))); + } + if !ch.is_ascii_alphabetic() { + return Err(cargo_err(&format!( + "invalid character `{}` in crate name: `{}`, \ + the first character must be an ASCII character", + ch, name + ))); + } + } + + for ch in chars { + if !(ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') { + return Err(cargo_err(&format!( + "invalid character `{}` in crate name: `{}`, \ + characters must be an ASCII alphanumeric characters, `-`, or `_`", + ch, name + ))); + } + } + + Ok(()) } pub fn valid_dependency_name(name: &str) -> AppResult<()> { @@ -209,31 +249,31 @@ impl Crate { name, MAX_NAME_LENGTH ))); } - Crate::valid_ident(name, "dependency name") + Crate::valid_dependency_ident(name) } - // Checks that the name is a valid identifier. + // Checks that the name is a valid dependency name. // 1. The name must be non-empty. // 2. The first character must be an ASCII character or `_`. // 3. The remaining characters must be ASCII alphanumerics or `-` or `_`. - fn valid_ident(name: &str, what: &str) -> AppResult<()> { + fn valid_dependency_ident(name: &str) -> AppResult<()> { if name.is_empty() { - return Err(cargo_err(&format!("the {} cannot be an empty", what))); + return Err(cargo_err("the dependency name cannot be an empty")); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if ch.is_ascii_digit() { return Err(cargo_err(&format!( - "the name `{}` cannot be used as a {}, \ + "the name `{}` cannot be used as a dependency name, \ the name cannot start with a digit", - name, what, + name, ))); } if !(ch.is_ascii_alphabetic() || ch == '_') { return Err(cargo_err(&format!( - "invalid character `{}` in {}: `{}`, \ + "invalid character `{}` in dependency name: `{}`, \ the first character must be an ASCII character, or `_`", - ch, what, name + ch, name ))); } } @@ -241,9 +281,9 @@ impl Crate { for ch in chars { if !(ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') { return Err(cargo_err(&format!( - "invalid character `{}` in {}: `{}`, \ + "invalid character `{}` in dependency name: `{}`, \ characters must be an ASCII alphanumeric characters, `-`, or `_`", - ch, what, name + ch, name ))); } } @@ -549,7 +589,7 @@ mod tests { assert!(Crate::valid_name("foo_underscore").is_ok()); assert!(Crate::valid_name("foo-dash").is_ok()); assert!(Crate::valid_name("foo+plus").is_err()); - assert!(Crate::valid_name("_foo").is_ok()); + assert!(Crate::valid_name("_foo").is_err()); assert!(Crate::valid_name("-foo").is_err()); } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap index 5cfc1602868..a49f9a50b7e 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_dependency_name.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "invalid character `🦀` in crate name: `🦀`, the first character must be an ASCII character, or `_`" + "detail": "invalid character `🦀` in crate name: `🦀`, the first character must be an ASCII character" } ] } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap index 58222d36ce1..f0e3553b45c 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__validation__invalid_names-5.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "invalid character `á` in crate name: `áccênts`, the first character must be an ASCII character, or `_`" + "detail": "invalid character `á` in crate name: `áccênts`, the first character must be an ASCII character" } ] }