From 27637ae40b467036ef955cc49783f4be22459b3e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Mon, 8 Apr 2024 15:53:07 -0700 Subject: [PATCH] Implement rate limiting for e-mail verifications. This applies a burst of 3 and a refill time of 30 minutes by default per user. (Not that I can imagine a scenario where we'd ever override this for a user, but using the same machinery as other user actions is obviously much simpler.) I don't love that this ends up essentially prop-drilling the rate limiter into a bunch of new places, but I don't see an alternative other than prop-drilling the whole app struct through, which would be worse. This doesn't address (sorry) per-address rate limiting, but is at least a reasonable starting point. --- src/controllers/user/me.rs | 47 +++++++++---- src/controllers/user/session.rs | 17 +++-- src/models/user.rs | 12 ++-- src/rate_limiter.rs | 69 ++++++++++++++------ src/tests/owners.rs | 7 +- src/tests/routes/crates/list.rs | 7 +- src/tests/routes/me/email_notifications.rs | 7 +- src/tests/routes/users/read.rs | 14 +++- src/tests/user.rs | 36 ++++++++-- src/tests/util/test_app.rs | 2 +- src/typosquat/test_util.rs | 13 ++-- src/worker/jobs/downloads/update_metadata.rs | 8 ++- 12 files changed, 177 insertions(+), 62 deletions(-) diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 8c10a0f799..71c851d9d6 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -1,4 +1,6 @@ use crate::auth::AuthCheck; +use crate::rate_limiter::LimitedAction; +use crate::rate_limiter::RateLimiter; use secrecy::{ExposeSecret, SecretString}; use std::collections::HashMap; @@ -159,11 +161,13 @@ pub async fn update_user( // an invalid email set in their GitHub profile, and we should let them sign in even though // we're trying to silently use their invalid address during signup and can't send them an // email. They'll then have to provide a valid email address. - let email = UserConfirmEmail { - user_name: &user.gh_login, - domain: &state.emails.domain, + let email = UserConfirmEmail::new( + &state.rate_limiter, + conn, + user, + &state.emails.domain, token, - }; + )?; let _ = state.emails.send(user_email, email); @@ -222,11 +226,13 @@ pub async fn regenerate_token_and_send( .optional()? .ok_or_else(|| bad_request("Email could not be found"))?; - let email1 = UserConfirmEmail { - user_name: &user.gh_login, - domain: &state.emails.domain, - token: email.token, - }; + let email1 = UserConfirmEmail::new( + &state.rate_limiter, + conn, + user, + &state.emails.domain, + email.token, + )?; state.emails.send(&email.email, email1).map_err(Into::into) })?; @@ -300,9 +306,26 @@ pub async fn update_email_notifications(app: AppState, req: BytesRequest) -> App } pub struct UserConfirmEmail<'a> { - pub user_name: &'a str, - pub domain: &'a str, - pub token: SecretString, + user_name: &'a str, + domain: &'a str, + token: SecretString, +} + +impl<'a> UserConfirmEmail<'a> { + pub fn new( + rate_limiter: &RateLimiter, + conn: &mut PgConnection, + user: &'a User, + domain: &'a str, + token: SecretString, + ) -> AppResult { + rate_limiter.check_rate_limit(user.id, LimitedAction::VerifyEmail, conn)?; + Ok(Self { + user_name: &user.gh_login, + domain, + token, + }) + } } impl crate::email::Email for UserConfirmEmail<'_> { diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 94bf65c08a..414cbaba04 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -1,4 +1,5 @@ use crate::controllers::frontend_prelude::*; +use crate::rate_limiter::RateLimiter; use axum::extract::{FromRequestParts, Query}; use oauth2::reqwest::http_client; @@ -108,8 +109,13 @@ pub async fn authorize( // Fetch the user info from GitHub using the access token we just got and create a user record let ghuser = Handle::current().block_on(app.github.current_user(token))?; - let user = - save_user_to_database(&ghuser, token.secret(), &app.emails, &mut *app.db_write()?)?; + let user = save_user_to_database( + &ghuser, + token.secret(), + &app.emails, + &app.rate_limiter, + &mut *app.db_write()?, + )?; // Log in by setting a cookie and the middleware authentication session.insert("user_id".to_string(), user.id.to_string()); @@ -125,6 +131,7 @@ fn save_user_to_database( user: &GithubUser, access_token: &str, emails: &Emails, + rate_limiter: &RateLimiter, conn: &mut PgConnection, ) -> AppResult { NewUser::new( @@ -134,7 +141,7 @@ fn save_user_to_database( user.avatar_url.as_deref(), access_token, ) - .create_or_update(user.email.as_deref(), emails, conn) + .create_or_update(user.email.as_deref(), emails, rate_limiter, conn) .map_err(Into::into) .or_else(|e: BoxedAppError| { // If we're in read only mode, we can't update their details @@ -165,6 +172,7 @@ mod tests { #[test] fn gh_user_with_invalid_email_doesnt_fail() { let emails = Emails::new_in_memory(); + let rate_limiter = RateLimiter::new(Default::default()); let (_test_db, conn) = &mut test_db_connection(); let gh_user = GithubUser { email: Some("String.Format(\"{0}.{1}@live.com\", FirstName, LastName)".into()), @@ -173,7 +181,8 @@ mod tests { id: -1, avatar_url: None, }; - let result = save_user_to_database(&gh_user, "arbitrary_token", &emails, conn); + let result = + save_user_to_database(&gh_user, "arbitrary_token", &emails, &rate_limiter, conn); assert!( result.is_ok(), diff --git a/src/models/user.rs b/src/models/user.rs index 542ffa373f..38bf4e2046 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -5,6 +5,7 @@ use secrecy::SecretString; use crate::app::App; use crate::controllers::user::me::UserConfirmEmail; use crate::email::Emails; +use crate::rate_limiter::RateLimiter; use crate::util::errors::AppResult; use crate::models::{ApiToken, Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights}; @@ -58,8 +59,9 @@ impl<'a> NewUser<'a> { &self, email: Option<&'a str>, emails: &Emails, + rate_limiter: &RateLimiter, conn: &mut PgConnection, - ) -> QueryResult { + ) -> AppResult { use diesel::dsl::sql; use diesel::insert_into; use diesel::pg::upsert::excluded; @@ -102,12 +104,10 @@ impl<'a> NewUser<'a> { .map(SecretString::new); if let Some(token) = token { + let email = + UserConfirmEmail::new(rate_limiter, conn, &user, &emails.domain, token)?; + // Swallows any error. Some users might insert an invalid email address here. - let email = UserConfirmEmail { - user_name: &user.gh_login, - domain: &emails.domain, - token, - }; let _ = emails.send(user_email, email); } } diff --git a/src/rate_limiter.rs b/src/rate_limiter.rs index 448af5160a..99a7ec3144 100644 --- a/src/rate_limiter.rs +++ b/src/rate_limiter.rs @@ -14,15 +14,17 @@ pg_enum! { PublishNew = 0, PublishUpdate = 1, YankUnyank = 2, + VerifyEmail = 3, } } impl LimitedAction { pub fn default_rate_seconds(&self) -> u64 { match self { - LimitedAction::PublishNew => 10 * 60, // 10 minutes - LimitedAction::PublishUpdate => 60, // 1 minute - LimitedAction::YankUnyank => 60, // 1 minute + LimitedAction::PublishNew => 10 * 60, // 10 minutes + LimitedAction::PublishUpdate => 60, // 1 minute + LimitedAction::YankUnyank => 60, // 1 minute + LimitedAction::VerifyEmail => 30 * 60, // 30 minutes } } @@ -31,6 +33,7 @@ impl LimitedAction { LimitedAction::PublishNew => 5, LimitedAction::PublishUpdate => 30, LimitedAction::YankUnyank => 100, + LimitedAction::VerifyEmail => 3, } } @@ -39,6 +42,7 @@ impl LimitedAction { LimitedAction::PublishNew => "PUBLISH_NEW", LimitedAction::PublishUpdate => "PUBLISH_UPDATE", LimitedAction::YankUnyank => "YANK_UNYANK", + LimitedAction::VerifyEmail => "VERIFY_EMAIL", } } @@ -53,6 +57,9 @@ impl LimitedAction { LimitedAction::YankUnyank => { "You have yanked or unyanked too many versions in a short period of time" } + LimitedAction::VerifyEmail => { + "You have attempted to verify too many e-mail addresses in a short period of time" + } } } } @@ -182,7 +189,7 @@ mod tests { use crate::test_util::*; #[test] - fn default_rate_limits() -> QueryResult<()> { + fn default_rate_limits() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -246,11 +253,26 @@ mod tests { } assert_eq!(expected_last_refill_times, last_refill_times); + // E-mail verification has a burst of 3 and a refill time of 30 minutes, which means we + // should be able to verify an e-mail address every 30 minutes, always have tokens + // remaining, and set the last_refill based on the refill time. + let action = LimitedAction::VerifyEmail; + let mut last_refill_times = vec![]; + let mut expected_last_refill_times = vec![]; + for publish_num in 1..=3 { + let publish_time = now + chrono::Duration::minutes(30 * publish_num); + let bucket = rate.take_token(user_id, action, publish_time, conn)?; + + last_refill_times.push(bucket.last_refill); + expected_last_refill_times.push(publish_time); + } + assert_eq!(expected_last_refill_times, last_refill_times); + Ok(()) } #[test] - fn take_token_with_no_bucket_creates_new_one() -> QueryResult<()> { + fn take_token_with_no_bucket_creates_new_one() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -297,7 +319,7 @@ mod tests { } #[test] - fn take_token_with_existing_bucket_modifies_existing_bucket() -> QueryResult<()> { + fn take_token_with_existing_bucket_modifies_existing_bucket() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -320,7 +342,7 @@ mod tests { } #[test] - fn take_token_after_delay_refills() -> QueryResult<()> { + fn take_token_after_delay_refills() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -344,7 +366,7 @@ mod tests { } #[test] - fn refill_subsecond_rate() -> QueryResult<()> { + fn refill_subsecond_rate() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); // Subsecond rates have floating point rounding issues, so use a known // timestamp that rounds fine @@ -372,7 +394,7 @@ mod tests { } #[test] - fn last_refill_always_advanced_by_multiple_of_rate() -> QueryResult<()> { + fn last_refill_always_advanced_by_multiple_of_rate() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -401,7 +423,7 @@ mod tests { } #[test] - fn zero_tokens_returned_when_user_has_no_tokens_left() -> QueryResult<()> { + fn zero_tokens_returned_when_user_has_no_tokens_left() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -427,7 +449,7 @@ mod tests { } #[test] - fn a_user_with_no_tokens_gets_a_token_after_exactly_rate() -> QueryResult<()> { + fn a_user_with_no_tokens_gets_a_token_after_exactly_rate() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -452,7 +474,7 @@ mod tests { } #[test] - fn tokens_never_refill_past_burst() -> QueryResult<()> { + fn tokens_never_refill_past_burst() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -477,7 +499,7 @@ mod tests { } #[test] - fn two_actions_dont_interfere_with_each_other() -> QueryResult<()> { + fn two_actions_dont_interfere_with_each_other() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -520,7 +542,7 @@ mod tests { } #[test] - fn override_is_used_instead_of_global_burst_if_present() -> QueryResult<()> { + fn override_is_used_instead_of_global_burst_if_present() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -550,7 +572,7 @@ mod tests { } #[test] - fn overrides_can_expire() -> QueryResult<()> { + fn overrides_can_expire() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); @@ -597,7 +619,7 @@ mod tests { } #[test] - fn override_is_different_for_each_action() -> QueryResult<()> { + fn override_is_different_for_each_action() -> AppResult<()> { let (_test_db, conn) = &mut test_db_connection(); let now = now(); let user_id = new_user(conn, "user")?; @@ -636,14 +658,19 @@ mod tests { Ok(()) } - fn new_user(conn: &mut PgConnection, gh_login: &str) -> QueryResult { + fn new_user(conn: &mut PgConnection, gh_login: &str) -> AppResult { use crate::models::NewUser; let user = NewUser { gh_login, ..NewUser::default() } - .create_or_update(None, &Emails::new_in_memory(), conn)?; + .create_or_update( + None, + &Emails::new_in_memory(), + &RateLimiter::new(Default::default()), + conn, + )?; Ok(user.id) } @@ -651,15 +678,15 @@ mod tests { conn: &mut PgConnection, tokens: i32, now: NaiveDateTime, - ) -> QueryResult { - diesel::insert_into(publish_limit_buckets::table) + ) -> AppResult { + Ok(diesel::insert_into(publish_limit_buckets::table) .values(Bucket { user_id: new_user(conn, "new_user")?, tokens, last_refill: now, action: LimitedAction::PublishNew, }) - .get_result(conn) + .get_result(conn)?) } struct SampleRateLimiter { diff --git a/src/tests/owners.rs b/src/tests/owners.rs index ca57a0d121..63d1b65b87 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -636,7 +636,12 @@ fn inactive_users_dont_get_invitations() { gh_avatar: None, gh_access_token: "some random token", } - .create_or_update(None, &app.as_inner().emails, conn) + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); CrateBuilder::new(krate_name, owner.id).expect_build(conn); }); diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 3ea054f719..31a6dd9f1f 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -20,7 +20,12 @@ fn index() { let krate = app.db(|conn| { let u = new_user("foo") - .create_or_update(None, &app.as_inner().emails, conn) + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); CrateBuilder::new("fooindex", u.id).expect_build(conn) }); diff --git a/src/tests/routes/me/email_notifications.rs b/src/tests/routes/me/email_notifications.rs index 9d177d44a9..79afe5445b 100644 --- a/src/tests/routes/me/email_notifications.rs +++ b/src/tests/routes/me/email_notifications.rs @@ -111,7 +111,12 @@ fn test_update_email_notifications_not_owned() { let not_my_crate = app.db(|conn| { let u = new_user("arbitrary_username") - .create_or_update(None, &app.as_inner().emails, conn) + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); CrateBuilder::new("test_package", u.id).expect_build(conn) }); diff --git a/src/tests/routes/users/read.rs b/src/tests/routes/users/read.rs index c7ad311c4c..31f471ffbe 100644 --- a/src/tests/routes/users/read.rs +++ b/src/tests/routes/users/read.rs @@ -39,7 +39,12 @@ fn show_latest_user_case_insensitively() { None, "bar" ) - .create_or_update(None, &app.as_inner().emails, conn)); + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn + )); assert_ok!(NewUser::new( 2, "FOOBAR", @@ -47,7 +52,12 @@ fn show_latest_user_case_insensitively() { None, "bar" ) - .create_or_update(None, &app.as_inner().emails, conn)); + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn + )); }); let json: UserShowPublicResponse = anon.get("/api/v1/users/fOObAr").good(); diff --git a/src/tests/user.rs b/src/tests/user.rs index a4fa54c8ca..84c338a057 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -29,6 +29,7 @@ fn updating_existing_user_doesnt_change_api_token() { NewUser::new(gh_id, "bar", None, None, "bar_token").create_or_update( None, &app.as_inner().emails, + &app.as_inner().rate_limiter, conn ) ); @@ -60,7 +61,12 @@ fn github_without_email_does_not_overwrite_email() { let user_without_github_email = app.db(|conn| { let u = new_user("arbitrary_username"); let u = u - .create_or_update(None, &app.as_inner().emails, conn) + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); MockCookieUser::new(&app, u) }); @@ -82,7 +88,12 @@ fn github_without_email_does_not_overwrite_email() { ..new_user("arbitrary_username") }; let u = u - .create_or_update(None, &app.as_inner().emails, conn) + .create_or_update( + None, + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); MockCookieUser::new(&app, u) }); @@ -117,7 +128,12 @@ fn github_with_email_does_not_overwrite_email() { ..new_user("arbitrary_username") }; let u = u - .create_or_update(Some(new_github_email), &app.as_inner().emails, conn) + .create_or_update( + Some(new_github_email), + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); MockCookieUser::new(&app, u) }); @@ -164,7 +180,12 @@ fn test_confirm_user_email() { ..new_user("arbitrary_username") }; let u = u - .create_or_update(Some(email), &app.as_inner().emails, conn) + .create_or_update( + Some(email), + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); MockCookieUser::new(&app, u) }); @@ -204,7 +225,12 @@ fn test_existing_user_email() { ..new_user("arbitrary_username") }; let u = u - .create_or_update(Some(email), &app.as_inner().emails, conn) + .create_or_update( + Some(email), + &app.as_inner().emails, + &app.as_inner().rate_limiter, + conn, + ) .unwrap(); update(Email::belonging_to(&u)) // Users created before we added verification will have diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index f30072b4a9..cbe4ceb641 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -127,7 +127,7 @@ impl TestApp { let email = "something@example.com"; let user = crate::new_user(username) - .create_or_update(None, &self.0.app.emails, conn) + .create_or_update(None, &self.0.app.emails, &self.0.app.rate_limiter, conn) .unwrap(); diesel::insert_into(emails::table) .values(( diff --git a/src/typosquat/test_util.rs b/src/typosquat/test_util.rs index 04520aca8f..0ed2b6a672 100644 --- a/src/typosquat/test_util.rs +++ b/src/typosquat/test_util.rs @@ -6,6 +6,7 @@ use crate::{ models::{ Crate, CrateOwner, NewCrate, NewTeam, NewUser, NewVersion, Owner, OwnerKind, User, Version, }, + rate_limiter::RateLimiter, schema::{crate_downloads, crate_owners}, Emails, }; @@ -13,6 +14,7 @@ use crate::{ pub struct Faker { emails: Emails, id: i32, + rate_limiter: RateLimiter, } impl Faker { @@ -20,6 +22,7 @@ impl Faker { Self { emails: Emails::new_in_memory(), id: Default::default(), + rate_limiter: RateLimiter::new(Default::default()), } } @@ -102,13 +105,9 @@ impl Faker { } pub fn user(&mut self, conn: &mut PgConnection, login: &str) -> anyhow::Result { - Ok( - NewUser::new(self.next_id(), login, None, None, "token").create_or_update( - None, - &self.emails, - conn, - )?, - ) + NewUser::new(self.next_id(), login, None, None, "token") + .create_or_update(None, &self.emails, &self.rate_limiter, conn) + .map_err(|e| anyhow::anyhow!("{e:?}")) } fn next_id(&mut self) -> i32 { diff --git a/src/worker/jobs/downloads/update_metadata.rs b/src/worker/jobs/downloads/update_metadata.rs index 9418802ee9..4f2ec394ea 100644 --- a/src/worker/jobs/downloads/update_metadata.rs +++ b/src/worker/jobs/downloads/update_metadata.rs @@ -170,13 +170,19 @@ mod tests { use super::*; use crate::email::Emails; use crate::models::{Crate, NewCrate, NewUser, NewVersion, User, Version}; + use crate::rate_limiter::RateLimiter; use crate::schema::{crate_downloads, crates, versions}; use crate::test_util::test_db_connection; use std::collections::BTreeMap; fn user(conn: &mut PgConnection) -> User { NewUser::new(2, "login", None, None, "access_token") - .create_or_update(None, &Emails::new_in_memory(), conn) + .create_or_update( + None, + &Emails::new_in_memory(), + &RateLimiter::new(Default::default()), + conn, + ) .unwrap() }