From 8e0b6b80323ebd2389aa71d5ef6bd15ce7e8caf2 Mon Sep 17 00:00:00 2001 From: Piotr Orzechowski Date: Sun, 25 Feb 2024 09:35:19 +0100 Subject: [PATCH] Reject malformed subscriber data --- Cargo.lock | 134 +++++++++++++++++++++++++++++++- Cargo.toml | 5 ++ src/domain/mod.rs | 7 ++ src/domain/new_subscriber.rs | 8 ++ src/domain/subscriber_email.rs | 136 +++++++++++++++++++++++++++++++++ src/domain/subscriber_name.rs | 115 ++++++++++++++++++++++++++++ src/lib.rs | 1 + src/routes/subscriptions.rs | 36 +++++++-- tests/subscriptions.rs | 42 +++++++++- 9 files changed, 473 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 b0412b3..852e482 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -163,6 +163,21 @@ version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8c3c1a368f70d6cf7302d78f8f7093da241fb8e8807c05cc9e51a125895a6d5b" +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -220,6 +235,15 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[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" @@ -367,6 +391,12 @@ dependencies = [ "serde", ] +[[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" @@ -445,6 +475,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" @@ -858,6 +898,16 @@ dependencies = [ "tokio", ] +[[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" @@ -1404,6 +1454,32 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proptest" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "31b476131c3c86cb68032fdc5cb6d5a1045e3e42d96b69fa599fd77701e1f5bf" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags 2.4.2", + "lazy_static", + "num-traits", + "rand", + "rand_chacha", + "rand_xorshift", + "regex-syntax 0.8.2", + "rusty-fork", + "tempfile", + "unarray", +] + +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quote" version = "1.0.35" @@ -1443,6 +1519,15 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rand_xorshift" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d25bf25ec5ae4a3f1b92f929810509a2f53d7dca2f50b794ff57e3face536c8f" +dependencies = [ + "rand_core", +] + [[package]] name = "redox_syscall" version = "0.4.1" @@ -1612,6 +1697,18 @@ version = "1.0.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7ffc183a10b4478d04cbbbfc96d0873219d962dd5accaff2ffbd4ceb7df837f4" +[[package]] +name = "rusty-fork" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb3dcc6e454c328bb824492db107ab7c0ae8fcffe4ad210136ef014458c1bc4f" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.16" @@ -2462,6 +2559,12 @@ version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed646292ffc8188ef8ea4d1e0e0150fb15a5c2e12ad9b8fc191ae7a8a7f3c4b9" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicode-bidi" version = "0.3.15" @@ -2502,7 +2605,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "31e6302e3bb753d46e83516cae55ae196fc0c309407cf11ab35cc51a4c2a4633" dependencies = [ "form_urlencoded", - "idna", + "idna 0.5.0", "percent-encoding", ] @@ -2521,6 +2624,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" @@ -2539,6 +2657,15 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" +[[package]] +name = "wait-timeout" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f200f5b12eb75f8c1ed65abd4b2db8a6e1b138a20de009dacee265a2498f3f6" +dependencies = [ + "libc", +] + [[package]] name = "want" version = "0.3.1" @@ -2823,8 +2950,11 @@ name = "zero2prod" version = "0.1.0" dependencies = [ "axum", + "claims", "config", + "fake", "once_cell", + "proptest", "reqwest", "secrecy", "serde", @@ -2838,7 +2968,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 6eb4196..a5907c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,12 @@ tracing = "0.1.40" tracing-bunyan-formatter = "0.3.9" tracing-log = "0.2.0" tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } +unicode-segmentation = "1.11.0" uuid = { version = "1.7.0", features = ["v4"] } +validator = "0.16.1" [dev-dependencies] +claims = "0.7.1" +fake = "2.9.2" +proptest = "1.4.0" reqwest = "0.11.24" diff --git a/src/domain/mod.rs b/src/domain/mod.rs new file mode 100644 index 0000000..e771a2e --- /dev/null +++ b/src/domain/mod.rs @@ -0,0 +1,7 @@ +mod new_subscriber; +mod subscriber_email; +mod subscriber_name; + +pub use new_subscriber::NewSubscriber; +pub use subscriber_email::SubscriberEmail; +pub use subscriber_name::SubscriberName; diff --git a/src/domain/new_subscriber.rs b/src/domain/new_subscriber.rs new file mode 100644 index 0000000..c76a7d2 --- /dev/null +++ b/src/domain/new_subscriber.rs @@ -0,0 +1,8 @@ +use super::{SubscriberEmail, SubscriberName}; +use serde::Deserialize; + +#[derive(Deserialize)] +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..8ec2f63 --- /dev/null +++ b/src/domain/subscriber_email.rs @@ -0,0 +1,136 @@ +use serde::Deserialize; +use validator::validate_email; + +#[derive(Debug, Deserialize)] +pub struct SubscriberEmail(String); + +impl SubscriberEmail { + pub fn parse(s: String) -> Result { + if validate_email(&s) { + Ok(Self(s)) + } else { + Err(format!("`{s}` email has invalid format")) + } + } +} + +impl AsRef for SubscriberEmail { + fn as_ref(&self) -> &str { + &self.0 + } +} + +#[cfg(test)] +mod tests { + use super::SubscriberEmail; + use claims::{assert_err, assert_ok}; + use proptest::prelude::proptest; + use valid_emails::valid_emails; + + proptest! { + #[test] + fn valid_emails_are_parsed_successfully(valid_email in valid_emails()) { + // when + let result = SubscriberEmail::parse(valid_email); + + // then + assert_ok!(result); + } + } + + #[test] + fn empty_string_is_rejected() { + // given + let email = "".to_string(); + + // when + let result = SubscriberEmail::parse(email); + + // then + assert_err!(result); + } + + #[test] + fn email_missing_at_symbol_is_rejected() { + // given + let email = "imie.nazwiskoexample.com".to_string(); + + // when + let result = SubscriberEmail::parse(email); + + // then + assert_err!(result); + } + + #[test] + fn email_missing_subject_is_rejected() { + // given + let email = "@xample.com".to_string(); + + // when + let result = SubscriberEmail::parse(email); + + // then + assert_err!(result); + } + + mod valid_emails { + use fake::{ + faker::internet::en::{FreeEmail, SafeEmail}, + Fake, + }; + use proptest::{ + prelude::Strategy, + prop_oneof, + strategy::{NewTree, ValueTree}, + test_runner::TestRunner, + }; + + pub fn valid_emails() -> impl Strategy { + // using just `SafeEmailStrategy` would be enough + prop_oneof![FreeEmailStrategy, SafeEmailStrategy] + } + + #[derive(Debug)] + struct FreeEmailStrategy; + + impl Strategy for FreeEmailStrategy { + type Tree = ValidEmailValueTree; + type Value = String; + + fn new_tree(&self, runner: &mut TestRunner) -> NewTree { + Ok(ValidEmailValueTree(FreeEmail().fake_with_rng(runner.rng()))) + } + } + + #[derive(Debug)] + struct SafeEmailStrategy; + + impl Strategy for SafeEmailStrategy { + type Tree = ValidEmailValueTree; + type Value = String; + + fn new_tree(&self, runner: &mut TestRunner) -> NewTree { + Ok(ValidEmailValueTree(SafeEmail().fake_with_rng(runner.rng()))) + } + } + + struct ValidEmailValueTree(String); + + impl ValueTree for ValidEmailValueTree { + type Value = String; + + fn current(&self) -> Self::Value { + self.0.clone() + } + + fn simplify(&mut self) -> bool { + false + } + + fn complicate(&mut self) -> bool { + false + } + } + } +} diff --git a/src/domain/subscriber_name.rs b/src/domain/subscriber_name.rs new file mode 100644 index 0000000..f9304bc --- /dev/null +++ b/src/domain/subscriber_name.rs @@ -0,0 +1,115 @@ +use once_cell::sync::Lazy; +use serde::Deserialize; +use unicode_segmentation::UnicodeSegmentation; + +#[derive(Debug, Deserialize)] +pub struct SubscriberName(String); + +static FORBIDDEN_CHARS: [char; 10] = ['<', '>', '\'', '"', '\\', '(', ')', '{', '}', '/']; +static FORBIDDEN_CHARS_STRING: Lazy = Lazy::new(|| String::from_iter(FORBIDDEN_CHARS)); + +impl SubscriberName { + pub fn parse(s: String) -> Result { + match s { + _ if s.trim().is_empty() => Err(format!( + "Subscriber name is empty or contains whitespace only: `{s}`" + )), + _ if s.graphemes(true).count() > 256 => { + Err(format!("`{s}` is longer than 256 graphemes")) + } + _ if s.chars().any(|c| FORBIDDEN_CHARS.contains(&c)) => Err(format!( + "`{s}` contains at least one of forbidden characters: {}", + *FORBIDDEN_CHARS_STRING + )), + _ => 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}; + + use super::FORBIDDEN_CHARS; + + #[test] + fn a_valid_name_is_parsed_successfully() { + // given + let name = "Imię Nazwisko".to_string(); + + // when + let result = SubscriberName::parse(name); + + // then + assert_ok!(result); + } + + #[test] + fn empty_string_is_rejected() { + // given + let name = "".to_string(); + + // when + let result = SubscriberName::parse(name); + + // then + assert_err!(result); + } + + #[test] + fn whitespace_only_names_are_rejected() { + // given + let name = " ".repeat(10); + + // when + let result = SubscriberName::parse(name); + + // then + assert_err!(result); + } + + #[test] + fn a_256_grapheme_long_name_is_valid() { + // given + let name = "ę".repeat(256); + + // when + let result = SubscriberName::parse(name); + + // then + assert_ok!(result); + } + + #[test] + fn a_name_longer_than_256_graphemes_is_rejected() { + // given + let name = "ę".repeat(257); + + // when + let result = SubscriberName::parse(name); + + // then + assert_err!(result); + } + + #[test] + fn names_containing_invalid_characters_are_rejected() { + // given + for name in FORBIDDEN_CHARS { + let name = name.to_string(); + + // when + let result = SubscriberName::parse(name); + + // then + assert_err!(result); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 24ea30d..d0c0948 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ pub mod configuration; +pub mod domain; pub mod request_id; pub mod routes; pub mod startup; diff --git a/src/routes/subscriptions.rs b/src/routes/subscriptions.rs index 6ebf633..f8514fa 100644 --- a/src/routes/subscriptions.rs +++ b/src/routes/subscriptions.rs @@ -1,3 +1,4 @@ +use crate::domain::{NewSubscriber, SubscriberEmail, SubscriberName}; use axum::{extract::State, http::StatusCode, routing::post, Form, Router}; use serde::Deserialize; use sqlx::PgPool; @@ -16,8 +17,16 @@ pub fn router() -> Router { subscriber_name = %form.name ) )] -async fn subscribe(State(db_pool): State, Form(form): Form) -> StatusCode { - match insert_subscriber(&form, &db_pool).await { +async fn subscribe(State(db_pool): State, Form(form): Form) -> StatusCode { + let new_subscriber = match form.try_into() { + Ok(subscriber) => subscriber, + Err(e) => { + tracing::info!(e); + return StatusCode::BAD_REQUEST; + } + }; + + match insert_subscriber(&new_subscriber, &db_pool).await { Ok(_) => StatusCode::OK, Err(_) => StatusCode::INTERNAL_SERVER_ERROR, } @@ -25,17 +34,20 @@ async fn subscribe(State(db_pool): State, Form(form): Form #[tracing::instrument( name = "Saving new subscriber details in the database", - skip(form, db_pool) + skip(new_subscriber, db_pool) )] -pub async fn insert_subscriber(form: &Subscription, db_pool: &PgPool) -> Result<(), sqlx::Error> { +async fn insert_subscriber( + new_subscriber: &NewSubscriber, + db_pool: &PgPool, +) -> 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(), OffsetDateTime::now_utc(), ) .execute(db_pool) @@ -49,7 +61,17 @@ pub async fn insert_subscriber(form: &Subscription, db_pool: &PgPool) -> Result< } #[derive(Deserialize)] -struct Subscription { +struct FormData { name: String, email: String, } + +impl TryFrom for NewSubscriber { + type Error = String; + + fn try_from(value: FormData) -> Result { + let email = SubscriberEmail::parse(value.email)?; + let name = SubscriberName::parse(value.name)?; + Ok(NewSubscriber { email, name }) + } +} diff --git a/tests/subscriptions.rs b/tests/subscriptions.rs index e2e4f96..c4047c7 100644 --- a/tests/subscriptions.rs +++ b/tests/subscriptions.rs @@ -4,7 +4,7 @@ mod util; async fn subscribe_returns_a_200_for_valid_form_data() { // given let client = reqwest::Client::new(); - let body = "name=Imi%C4%99%20Nazwisko&email=imi%C4%99.nazwisko%40example.com"; + let body = "name=Imi%C4%99%20Nazwisko&email=imie.nazwisko%40example.com"; let app = util::spawn_app().await; let url = util::url(app.address, "subscriptions"); @@ -24,7 +24,43 @@ async fn subscribe_returns_a_200_for_valid_form_data() { .await .expect("Failed to fetch saved subscription"); assert_eq!(saved.name, "Imię Nazwisko"); - assert_eq!(saved.email, "imię.nazwisko@example.com"); + assert_eq!(saved.email, "imie.nazwisko@example.com"); +} + +#[tokio::test] +async fn subscribe_returns_a_400_when_fields_are_present_but_empty() { + // given + let client = reqwest::Client::new(); + let app = util::spawn_app().await; + let url = util::url(app.address, "subscriptions"); + let test_cases = vec![ + ("name=Imi%C4%99%20Nazwisko&email=", "empty email"), + ( + "name=Imi%C4%99%20Nazwisko&email=definitely-not-an-email", + "invalid email", + ), + ("name=&email=imie.nazwisko%40example.com", "empty name"), + ("name=&email=", "empty both name and email"), + ]; + + for (body, description) in test_cases { + // when + let response = client + .post(&url) + .header("Content-Type", "application/x-www-form-urlencoded") + .body(body) + .send() + .await + .expect("Failed to execute request"); + + // then + assert_eq!( + 400, + response.status().as_u16(), + "The API did not return a 400 BAD_REQUEST when the payload was {}", + description + ); + } } #[tokio::test] @@ -35,7 +71,7 @@ async fn subscribe_returns_a_400_when_data_is_missing() { let url = util::url(app.address, "subscriptions"); let test_cases = vec![ ("name=Imi%C4%99%20Nazwisko", "missing the email"), - ("email=imi%C4%99.nazwisko%40example.com", "missing the name"), + ("email=imie.nazwisko%40example.com", "missing the name"), ("", "missing both name and email"), ];