From 1283d337ac1383f996a9188b41588d32c8045a59 Mon Sep 17 00:00:00 2001 From: alexsavio Date: Tue, 2 Apr 2024 15:57:13 +0200 Subject: [PATCH] add subscriber data validation --- Cargo.lock | 95 ++++++++++++++++++++++++++++++++-- Cargo.toml | 7 +++ justfile | 1 + spec.yaml | 3 -- src/domain/mod.rs | 7 +++ src/domain/new_subscriber.rs | 8 +++ src/domain/subscriber_email.rs | 64 +++++++++++++++++++++++ src/domain/subscriber_name.rs | 70 +++++++++++++++++++++++++ src/lib.rs | 1 + src/routes/subscriptions.rs | 20 +++++-- tests/health_check.rs | 32 ++++++++++++ 11 files changed, 297 insertions(+), 11 deletions(-) create mode 100644 src/domain/mod.rs create mode 100644 src/domain/new_subscriber.rs create mode 100644 src/domain/subscriber_email.rs create mode 100644 src/domain/subscriber_name.rs diff --git a/Cargo.lock b/Cargo.lock index bb6092e..7d18cf2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -417,6 +417,15 @@ dependencies = [ "windows-targets 0.52.0", ] +[[package]] +name = "claims" +version = "0.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6995bbe186456c36307f8ea36be3eefe42f49d106896414e18efc4fb2f846b5" +dependencies = [ + "autocfg", +] + [[package]] name = "config" version = "0.14.0" @@ -602,6 +611,12 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "deunicode" +version = "1.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6e854126756c496b8c81dec88f9a706b15b875c5849d4097a3854476b9fdf94" + [[package]] name = "digest" version = "0.10.7" @@ -647,6 +662,16 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "env_logger" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a19187fea3ac7e84da7dacf48de0c45d63c6a76f9490dae389aead16c243fce3" +dependencies = [ + "log", + "regex", +] + [[package]] name = "equivalent" version = "1.0.1" @@ -680,6 +705,16 @@ version = "2.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0206175f82b8d6bf6652ff7d71a1e27fd2e4efde587fd368662814d6ec1d9ce0" +[[package]] +name = "fake" +version = "2.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c25829bde82205da46e1823b2259db6273379f626fc211f126f65654a2669be" +dependencies = [ + "deunicode", + "rand", +] + [[package]] name = "fastrand" version = "2.0.1" @@ -1099,6 +1134,16 @@ dependencies = [ "cc", ] +[[package]] +name = "idna" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d20d6b07bfbc108882d88ed8e37d39636dcc260e15e30c45e6ba089610b917c" +dependencies = [ + "unicode-bidi", + "unicode-normalization", +] + [[package]] name = "idna" version = "0.5.0" @@ -1679,6 +1724,28 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "quickcheck" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "588f6378e4dd99458b60ec275b4477add41ce4fa9f64dcba6f15adccb19b50d6" +dependencies = [ + "env_logger", + "log", + "rand", +] + +[[package]] +name = "quickcheck_macros" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b22a693222d716a9587786f37ac3f6b4faedb5b80c23914e7303ff5a1d8016e9" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "quote" version = "1.0.35" @@ -2815,9 +2882,9 @@ dependencies = [ [[package]] name = "unicode-segmentation" -version = "1.10.1" +version = "1.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1dd624098567895118886609431a7c3b8f516e41d30e0643f03d94592a147e36" +checksum = "d4c87d22b6e3f4a18d4d40ef354e97c90fcb14dd91d7dc0aa9d8a1172ebf7202" [[package]] name = "unicode_categories" @@ -2838,7 +2905,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "31e6302e3bb753d46e83516cae55ae196fc0c309407cf11ab35cc51a4c2a4633" dependencies = [ "form_urlencoded", - "idna", + "idna 0.5.0", "percent-encoding", ] @@ -2857,6 +2924,21 @@ dependencies = [ "getrandom", ] +[[package]] +name = "validator" +version = "0.16.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b92f40481c04ff1f4f61f304d61793c7b56ff76ac1469f1beb199b1445b253bd" +dependencies = [ + "idna 0.4.0", + "lazy_static", + "regex", + "serde", + "serde_derive", + "serde_json", + "url", +] + [[package]] name = "valuable" version = "0.1.0" @@ -3185,8 +3267,13 @@ version = "0.1.0" dependencies = [ "actix-web", "chrono", + "claims", "config", + "fake", "once_cell", + "quickcheck", + "quickcheck_macros", + "rand", "reqwest", "secrecy", "serde", @@ -3198,7 +3285,9 @@ dependencies = [ "tracing-bunyan-formatter", "tracing-log 0.2.0", "tracing-subscriber", + "unicode-segmentation", "uuid", + "validator", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 264ca6c..c29e50a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,8 @@ tracing-actix-web = "0.7" tracing-subscriber = { version = "0.3", features = ["registry", "env-filter"] } tracing-bunyan-formatter = "0.3" serde-aux = "4.5.0" +unicode-segmentation = "1.11.0" +validator = "0.16.0" [dependencies.sqlx] version = "0.7" @@ -41,3 +43,8 @@ features = [ [dev-dependencies] reqwest = "0.12.2" once_cell = "1" +claims = "0.7" +fake = "2.9.2" +quickcheck = "1.0.3" +quickcheck_macros = "1.0.0" +rand = "0.8.5" diff --git a/justfile b/justfile index ba07e97..5e4bcb6 100644 --- a/justfile +++ b/justfile @@ -49,6 +49,7 @@ testv: ## Run the linter lint: + cargo check cargo clippy ## Authenticate against DigitalOcean diff --git a/spec.yaml b/spec.yaml index 72027dc..8a7d61a 100644 --- a/spec.yaml +++ b/spec.yaml @@ -27,9 +27,6 @@ services: routes: - path: / envs: - - key: APP_APPLICATION__BASE_URL - scope: RUN_TIME - value: ${APP_URL} - key: APP_DATABASE__USERNAME scope: RUN_TIME value: ${newsletter.USERNAME} diff --git a/src/domain/mod.rs b/src/domain/mod.rs new file mode 100644 index 0000000..49b35d6 --- /dev/null +++ b/src/domain/mod.rs @@ -0,0 +1,7 @@ +mod subscriber_email; +mod subscriber_name; +mod new_subscriber; + +pub use subscriber_email::SubscriberEmail; +pub use subscriber_name::SubscriberName; +pub use new_subscriber::NewSubscriber; diff --git a/src/domain/new_subscriber.rs b/src/domain/new_subscriber.rs new file mode 100644 index 0000000..1b78516 --- /dev/null +++ b/src/domain/new_subscriber.rs @@ -0,0 +1,8 @@ + +use crate::domain::subscriber_name::SubscriberName; +use crate::domain::subscriber_email::SubscriberEmail; + +pub struct NewSubscriber { + pub email: SubscriberEmail, + pub name: SubscriberName, +} diff --git a/src/domain/subscriber_email.rs b/src/domain/subscriber_email.rs new file mode 100644 index 0000000..edaf7c0 --- /dev/null +++ b/src/domain/subscriber_email.rs @@ -0,0 +1,64 @@ +use validator::validate_email; + +#[derive(Debug)] +pub struct SubscriberEmail(String); + +impl SubscriberEmail { + pub fn parse(s: String) -> Result { + if validate_email(&s) { + Ok(Self(s)) + } + else { + Err(format!("{} is not a valid email address.", s)) + } + } +} + +impl AsRef for SubscriberEmail { + fn as_ref(&self) -> &str { + &self.0 + } +} + +#[cfg(test)] +mod tests { + use fake::faker::internet::en::SafeEmail; + use fake::Fake; + use rand::{rngs::StdRng, SeedableRng}; + use claims::assert_err; + use crate::domain::SubscriberEmail; + + #[test] + fn empty_string_is_rejected() { + let email = "".to_string(); + assert_err!(SubscriberEmail::parse(email)); + } + + #[test] + fn email_missing_at_symbol_is_rejected() { + let email = "name.example.com".to_string(); + assert_err!(SubscriberEmail::parse(email)); + } + + #[test] + fn email_missing_subject_is_rejected() { + let email = "@example.com".to_string(); + assert_err!(SubscriberEmail::parse(email)); + } + + #[derive(Debug, Clone)] + struct ValidEmailFixture(pub String); + + impl quickcheck::Arbitrary for ValidEmailFixture { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + let mut rng = StdRng::seed_from_u64(u64::arbitrary(g)); + let email = SafeEmail().fake_with_rng(&mut rng); + Self(email) + } + } + + #[quickcheck_macros::quickcheck] + fn valid_emails_are_parsed_successfully(valid_email: ValidEmailFixture) -> bool { + SubscriberEmail::parse(valid_email.0).is_ok() + } +} diff --git a/src/domain/subscriber_name.rs b/src/domain/subscriber_name.rs new file mode 100644 index 0000000..40ef7d0 --- /dev/null +++ b/src/domain/subscriber_name.rs @@ -0,0 +1,70 @@ +use unicode_segmentation::UnicodeSegmentation; + +#[derive(Debug)] +pub struct SubscriberName(String); + +impl SubscriberName { + pub fn parse(s: String) -> Result { + let is_empty_or_whitespace = s.trim().is_empty(); + let is_too_long = s.graphemes(true).count() > 256; + let forbidden_characters = ['/', '(', ')', '"', '<', '>', '\\', '{', '}']; + let contains_forbidden_characters = s.chars().any(|c| forbidden_characters.contains(&c)); + if is_empty_or_whitespace || is_too_long || contains_forbidden_characters { + Err(format!("{} is not a valid subscriber name.", s)) + } + else { + Ok(Self(s)) + } + } +} + +impl AsRef for SubscriberName { + fn as_ref(&self) -> &str { + &self.0 + } +} + + +#[cfg(test)] +mod tests { + use crate::domain::SubscriberName; + use claims::{assert_err, assert_ok}; + + #[test] + fn a_256_grapheme_long_name_is_valid() { + let name = "a".repeat(256); + assert_ok!(SubscriberName::parse(name)); + } + + #[test] + fn a_257_grapheme_long_name_is_rejected() { + let name = "a".repeat(257); + assert_err!(SubscriberName::parse(name)); + } + + #[test] + fn a_whitespaces_only_name_is_rejected() { + let name = " ".repeat(10); + assert_err!(SubscriberName::parse(name)); + } + + #[test] + fn a_name_with_forbidden_characters_is_rejected() { + let name = "a/b".to_string(); + assert_err!(SubscriberName::parse(name)); + } + + #[test] + fn an_empty_name_is_rejected() { + let name = "".to_string(); + assert_err!(SubscriberName::parse(name)); + } + + #[test] + fn names_containing_an_invalid_character_are_rejected() { + for name in &['/', '(', ')', '"', '<', '>', '\\', '{', '}'] { + let name = format!("a{}b", name); + assert_err!(SubscriberName::parse(name)); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 9079ef9..19fce70 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ pub mod configuration; +pub mod domain; pub mod routes; pub mod startup; pub mod telemetry; diff --git a/src/routes/subscriptions.rs b/src/routes/subscriptions.rs index 2663414..f74c995 100644 --- a/src/routes/subscriptions.rs +++ b/src/routes/subscriptions.rs @@ -2,6 +2,7 @@ use actix_web::{web, HttpResponse}; use sqlx::PgPool; use chrono::Utc; use uuid::Uuid; +use crate::domain::{NewSubscriber, SubscriberName, SubscriberEmail}; #[derive(serde::Deserialize)] pub struct FormData { @@ -19,7 +20,16 @@ pub struct FormData { ) )] pub async fn subscribe(form: web::Form, pool: web::Data) -> HttpResponse { - match insert_subscriber(&pool, &form).await { + let name = match SubscriberName::parse(form.0.name) { + Ok(name) => name, + Err(_) => return HttpResponse::BadRequest().finish(), + }; + let email = match SubscriberEmail::parse(form.0.email) { + Ok(email) => email, + Err(_) => return HttpResponse::BadRequest().finish(), + }; + let new_subscriber = NewSubscriber {email, name}; + match insert_subscriber(&pool, &new_subscriber).await { Ok(_) => HttpResponse::Ok().finish(), Err(_) => HttpResponse::InternalServerError().finish(), } @@ -27,17 +37,17 @@ pub async fn subscribe(form: web::Form, pool: web::Data) -> Ht #[tracing::instrument( name = "Saving new subscriber details in the database", - skip(form, pool) + skip(new_subscriber, pool) )] -pub async fn insert_subscriber(pool: &PgPool, form: &FormData) -> Result<(), sqlx::Error> { +pub async fn insert_subscriber(pool: &PgPool, new_subscriber: &NewSubscriber) -> Result<(), sqlx::Error> { sqlx::query!( r#" INSERT INTO subscriptions (id, email, name, subscribed_at) VALUES ($1, $2, $3, $4) "#, Uuid::new_v4(), - form.email, - form.name, + new_subscriber.email.as_ref(), + new_subscriber.name.as_ref(), Utc::now() ) .execute(pool) diff --git a/tests/health_check.rs b/tests/health_check.rs index 2488617..b6fa405 100644 --- a/tests/health_check.rs +++ b/tests/health_check.rs @@ -148,3 +148,35 @@ async fn subscribe_returns_a_400_when_data_is_missing() { ); } } + + +#[tokio::test] +async fn subscribe_returns_a_400_when_fields_are_present_but_invalid() { + // Arrange + let app = spawn_app().await; + let client = reqwest::Client::new(); + let test_cases = vec![ + ("name=&email=ursula_le_guin%40gmail.com", "empty name"), + ("name=Ursula&email=", "empty email"), + ("name=&email=", "empty name and email"), + ]; + + for (body, description) in test_cases { + // Act + let response = client + .post(&format!("{}/subscriptions", &app.address)) + .header("Content-Type", "application/x-www-form-urlencoded") + .body(body) + .send() + .await + .expect("Failed to execute request."); + + // Assert + assert_eq!( + 400, + response.status().as_u16(), + "The API returned a 400 when the payload was {}.", + description + ); + } +}