Skip to content

models/krate: Improve feature validation #7537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 16, 2023
Merged

models/krate: Improve feature validation #7537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd1d40a
models/krate: Use `assert_ok/err!()` assertion macros
Turbo87 Nov 15, 2023
efc598b
models/krate: Adjust naming of fns that return `Result`
Turbo87 Nov 15, 2023
6ffd5dd
models/krate: Extract custom error types
Turbo87 Nov 15, 2023
0e0feda
models/krate: Adjust `InvalidFeature::Empty` error message
Turbo87 Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/controllers/krate/publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use tokio::runtime::Handle;
use url::Url;

use crate::controllers::cargo_prelude::*;
use crate::models::krate::MAX_NAME_LENGTH;
use crate::models::krate::{InvalidDependencyName, MAX_NAME_LENGTH};
use crate::models::{
insert_version_owner_action, Category, Crate, DependencyKind, Keyword, NewCrate, NewVersion,
Rights, VersionAction,
Expand Down Expand Up @@ -238,7 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
}

for (key, values) in features.iter() {
Crate::valid_feature_name(key)?;
Crate::validate_feature_name(key).map_err(|error| cargo_err(&error))?;

let num_features = values.len();
if num_features > max_features {
Expand All @@ -253,7 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
}

for value in values.iter() {
Crate::valid_feature(value)?;
Crate::validate_feature(value).map_err(|error| cargo_err(&error))?;
}
}

Expand Down Expand Up @@ -596,7 +596,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
}

for feature in &dep.features {
Crate::valid_feature(feature)?;
Crate::validate_feature(feature).map_err(|error| cargo_err(&error))?;
}

if let Some(registry) = &dep.registry {
Expand All @@ -623,7 +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(&Crate::invalid_dependency_name_msg(toml_name)));
return Err(cargo_err(&InvalidDependencyName(toml_name.into())));
}
}

Expand Down
139 changes: 89 additions & 50 deletions src/models/krate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,32 +220,18 @@ impl Crate {
.unwrap_or(false)
}

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<()> {
pub fn validate_feature_name(name: &str) -> Result<(), InvalidFeature> {
if name.is_empty() {
return Err(cargo_err("feature cannot be an empty"));
return Err(InvalidFeature::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,
)));
return Err(InvalidFeature::Start(ch, name.into()));
}
}
for ch in chars {
Expand All @@ -254,33 +240,30 @@ impl Crate {
|| ch == '-'
|| ch == '.')
{
return Err(cargo_err(&format!(
"invalid character `{}` in feature `{}`, \
characters must be Unicode XID characters, `+`, `-`, or `.` \
(numbers, `+`, `-`, `_`, `.`, or most letters)",
ch, name,
)));
return Err(InvalidFeature::Char(ch, name.into()));
}
}

Ok(())
}

/// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`.
pub fn valid_feature(name: &str) -> AppResult<()> {
pub fn validate_feature(name: &str) -> Result<(), InvalidFeature> {
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)));
let err = InvalidDependencyName(dep.into());
return Err(InvalidFeature::DependencyName(err));
}
Crate::valid_feature_name(dep_feat)
Crate::validate_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)));
let err = InvalidDependencyName(dep.into());
return Err(InvalidFeature::DependencyName(err));
}
return Ok(());
} else {
Crate::valid_feature_name(name)
Crate::validate_feature_name(name)
}
}

Expand Down Expand Up @@ -528,6 +511,34 @@ impl CrateVersions for [Crate] {
}
}

#[derive(Debug, Eq, PartialEq, thiserror::Error)]
pub enum InvalidFeature {
#[error("feature cannot be empty")]
Empty,
#[error(
"invalid character `{0}` in feature `{1}`, the first character must be \
a Unicode XID start character or digit (most letters or `_` or `0` to \
`9`)"
)]
Start(char, String),
#[error(
"invalid character `{0}` in feature `{1}`, characters must be Unicode \
XID characters, `+`, `-`, or `.` (numbers, `+`, `-`, `_`, `.`, or most \
letters)"
)]
Char(char, String),
#[error(transparent)]
DependencyName(#[from] InvalidDependencyName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncertain about placing it here, as it is also used in validate_dependency. It's not intended for validating the feature name. But I can help to improve it in my next PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidDependencyName can also be used directly :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Got it. It's nice. Thanks!

}

#[derive(Debug, Eq, PartialEq, thiserror::Error)]
#[error(
"\"{0}\" 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)"
)]
pub struct InvalidDependencyName(pub String);

#[cfg(test)]
mod tests {
use crate::models::Crate;
Expand Down Expand Up @@ -559,29 +570,57 @@ mod tests {
assert!(Crate::valid_dependency_name("_foo"));
assert!(!Crate::valid_dependency_name("-foo"));
}

#[test]
fn valid_feature_names() {
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());
use super::InvalidDependencyName;
use super::InvalidFeature::*;

assert_ok!(Crate::validate_feature("foo"));
assert_ok!(Crate::validate_feature("1foo"));
assert_ok!(Crate::validate_feature("_foo"));
assert_ok!(Crate::validate_feature("_foo-_+.1"));
assert_ok!(Crate::validate_feature("_foo-_+.1"));
assert_err_eq!(Crate::validate_feature(""), Empty);
assert_err_eq!(
Crate::validate_feature("/"),
InvalidDependencyName("".into()).into()
);
assert_err_eq!(
Crate::validate_feature("%/%"),
InvalidDependencyName("%".into()).into()
);
assert_ok!(Crate::validate_feature("a/a"));
assert_ok!(Crate::validate_feature("32-column-tables"));
assert_ok!(Crate::validate_feature("c++20"));
assert_ok!(Crate::validate_feature("krate/c++20"));
assert_err_eq!(
Crate::validate_feature("c++20/wow"),
InvalidDependencyName("c++20".into()).into()
);
assert_ok!(Crate::validate_feature("foo?/bar"));
assert_ok!(Crate::validate_feature("dep:foo"));
assert_err_eq!(
Crate::validate_feature("dep:foo?/bar"),
InvalidDependencyName("dep:foo".into()).into()
);
assert_err_eq!(
Crate::validate_feature("foo/?bar"),
Start('?', "?bar".into())
);
assert_err_eq!(
Crate::validate_feature("foo?bar"),
Char('?', "foo?bar".into())
);
assert_ok!(Crate::validate_feature("bar.web"));
assert_ok!(Crate::validate_feature("foo/bar.web"));
assert_err_eq!(
Crate::validate_feature("dep:0foo"),
InvalidDependencyName("0foo".into()).into()
);
assert_err_eq!(
Crate::validate_feature("0foo?/bar.web"),
InvalidDependencyName("0foo".into()).into()
);
}
}
2 changes: 1 addition & 1 deletion src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ expression: response.into_json()
{
"errors": [
{
"detail": "feature cannot be an empty"
"detail": "feature cannot be empty"
}
]
}