diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 387984eb5fd..975ea713fa1 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, @@ -594,14 +586,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::validate_crate_name("dependency", &dep.name).map_err(cargo_err)?; for feature in &dep.features { Crate::validate_feature(feature).map_err(cargo_err)?; diff --git a/src/models/krate.rs b/src/models/krate.rs index c1dda837d9d..453c6615425 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -192,18 +192,59 @@ 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 + // Validates the name is a valid crate name. + // This is also used for validating the name of dependencies. + // So the `for_what` parameter is used to indicate what the name is used for. + // It can be "crate" or "dependency". + pub fn validate_crate_name(for_what: &str, name: &str) -> Result<(), InvalidCrateName> { + if name.chars().count() > MAX_NAME_LENGTH { + return Err(InvalidCrateName::TooLong { + what: for_what.into(), + name: name.into(), + }); + } + Crate::validate_create_ident(for_what, name) } - fn valid_ident(name: &str) -> bool { - Self::valid_feature_prefix(name) - && name - .chars() - .next() - .map(char::is_alphabetic) - .unwrap_or(false) + // 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 validate_create_ident(for_what: &str, name: &str) -> Result<(), InvalidCrateName> { + if name.is_empty() { + return Err(InvalidCrateName::Empty { + what: for_what.into(), + }); + } + let mut chars = name.chars(); + if let Some(ch) = chars.next() { + if ch.is_ascii_digit() { + return Err(InvalidCrateName::StartWithDigit { + what: for_what.into(), + name: name.into(), + }); + } + if !ch.is_ascii_alphabetic() { + return Err(InvalidCrateName::Start { + first_char: ch, + what: for_what.into(), + name: name.into(), + }); + } + } + + for ch in chars { + if !(ch.is_ascii_alphanumeric() || ch == '-' || ch == '_') { + return Err(InvalidCrateName::Char { + ch, + what: for_what.into(), + name: name.into(), + }); + } + } + + Ok(()) } pub fn validate_dependency_name(name: &str) -> Result<(), InvalidDependencyName> { @@ -281,15 +322,6 @@ impl Crate { } } - /// 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 == '-') - } - /// 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 { @@ -545,6 +577,37 @@ pub enum InvalidFeature { DependencyName(#[from] InvalidDependencyName), } +#[derive(Debug, Eq, PartialEq, thiserror::Error)] +pub enum InvalidCrateName { + #[error("the {what} name `{name}` is too long (max {MAX_NAME_LENGTH} characters)")] + TooLong { what: String, name: String }, + #[error("{what} name cannot be empty")] + Empty { what: String }, + #[error( + "the name `{name}` cannot be used as a {what} name, \ + the name cannot start with a digit" + )] + StartWithDigit { what: String, name: String }, + #[error( + "invalid character `{first_char}` in {what} name: `{name}`, \ + the first character must be an ASCII character" + )] + Start { + first_char: char, + what: String, + name: String, + }, + #[error( + "invalid character `{ch}` in {what} name: `{name}`, \ + characters must be an ASCII alphanumeric characters, `-`, or `_`" + )] + Char { + ch: char, + what: String, + name: String, + }, +} + #[derive(Debug, Eq, PartialEq, thiserror::Error)] pub enum InvalidDependencyName { #[error("the dependency name `{0}` is too long (max {MAX_NAME_LENGTH} characters)")] @@ -573,17 +636,72 @@ mod tests { use crate::models::Crate; #[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")); + fn validate_crate_name() { + use super::{InvalidCrateName, MAX_NAME_LENGTH}; + + assert_ok!(Crate::validate_crate_name("crate", "foo")); + assert_err_eq!( + Crate::validate_crate_name("crate", "京"), + InvalidCrateName::Start { + first_char: '京', + what: "crate".into(), + name: "京".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", ""), + InvalidCrateName::Empty { + what: "crate".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", "💝"), + InvalidCrateName::Start { + first_char: '💝', + what: "crate".into(), + name: "💝".into() + } + ); + assert_ok!(Crate::validate_crate_name("crate", "foo_underscore")); + assert_ok!(Crate::validate_crate_name("crate", "foo-dash")); + assert_err_eq!( + Crate::validate_crate_name("crate", "foo+plus"), + InvalidCrateName::Char { + ch: '+', + what: "crate".into(), + name: "foo+plus".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", "_foo"), + InvalidCrateName::Start { + first_char: '_', + what: "crate".into(), + name: "_foo".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", "-foo"), + InvalidCrateName::Start { + first_char: '-', + what: "crate".into(), + name: "-foo".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", "123"), + InvalidCrateName::StartWithDigit { + what: "crate".into(), + name: "123".into() + } + ); + assert_err_eq!( + Crate::validate_crate_name("crate", "o".repeat(MAX_NAME_LENGTH + 1).as_str()), + InvalidCrateName::TooLong { + what: "crate".into(), + name: "o".repeat(MAX_NAME_LENGTH + 1).as_str().into() + } + ); } #[test] diff --git a/src/models/token/scopes.rs b/src/models/token/scopes.rs index 2587d6ea197..8c426cc871d 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::validate_crate_name("crate", 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..e9ada2bf9ac 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 dependency name: `🦀`, the first character must be an ASCII character" } ] } 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..56f54a509f3 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 crate 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..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": "\"á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" } ] } 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..a92a742ab14 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": "crate name cannot be empty" } ] }