From: Nina Blanson Date: Tue, 27 Jun 2023 11:03:30 +0000 (-0500) Subject: Fixes #2900 - Checks slur regex to see if it is too permissive (#3146) X-Git-Url: http://these/git/%7BcanonicalUrl%7D?a=commitdiff_plain;h=e63aa80c3a3631f4713525d65c8223e1106355e8;p=lemmy.git Fixes #2900 - Checks slur regex to see if it is too permissive (#3146) * Fixes #2900 - Checks slur regex to see if it is too permissive along with small validation organization * Clean up variable names, add handler for valid empty string usecase * Update tests * Create validation function and add tests * Test clean up * Use payload value vs local site value to prevent stunlocking * Remove println added while testing * Fall back to local site regex if not provided from request * Attempt clean up of flaky comment_view tests * Pull in latest submodule * Move application, post check into functions, add more tests and improve test readability --------- Co-authored-by: Nutomic --- diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 4781ce92..e3e761c9 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -299,15 +299,6 @@ pub fn password_length_check(pass: &str) -> Result<(), LemmyError> { } } -/// Checks the site description length -pub fn site_description_length_check(description: &str) -> Result<(), LemmyError> { - if description.len() > 150 { - Err(LemmyError::from_message("site_description_length_overflow")) - } else { - Ok(()) - } -} - /// Checks for a honeypot. If this field is filled, fail the rest of the function pub fn honeypot_check(honeypot: &Option) -> Result<(), LemmyError> { if honeypot.is_some() && honeypot != &Some(String::new()) { diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index e7486e63..838d5bc4 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -1,4 +1,7 @@ -use crate::{site::check_application_question, PerformCrud}; +use crate::{ + site::{application_question_check, site_default_post_listing_type_check}, + PerformCrud, +}; use activitypub_federation::http_signatures::generate_actor_keypair; use actix_web::web::Data; use lemmy_api_common::{ @@ -8,9 +11,7 @@ use lemmy_api_common::{ generate_site_inbox_url, is_admin, local_site_rate_limit_to_rate_limit_config, - local_site_to_slur_regex, local_user_view_from_jwt, - site_description_length_check, }, }; use lemmy_db_schema::{ @@ -26,10 +27,16 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::SiteView; use lemmy_utils::{ - error::LemmyError, + error::{LemmyError, LemmyResult}, utils::{ slurs::{check_slurs, check_slurs_opt}, - validation::{check_site_visibility_valid, is_valid_body_field}, + validation::{ + build_and_check_regex, + check_site_visibility_valid, + is_valid_body_field, + site_description_length_check, + site_name_length_check, + }, }, }; use url::Url; @@ -41,57 +48,23 @@ impl PerformCrud for CreateSite { #[tracing::instrument(skip(context))] async fn perform(&self, context: &Data) -> Result { let data: &CreateSite = self; - - let local_site = LocalSite::read(context.pool()).await?; - - if local_site.site_setup { - return Err(LemmyError::from_message("site_already_exists")); - }; - let local_user_view = local_user_view_from_jwt(&data.auth, context).await?; + let local_site = LocalSite::read(context.pool()).await?; - // Make sure user is an admin + // Make sure user is an admin; other types of users should not create site data... is_admin(&local_user_view)?; - check_site_visibility_valid( - local_site.private_instance, - local_site.federation_enabled, - &data.private_instance, - &data.federation_enabled, - )?; - - let sidebar = diesel_option_overwrite(&data.sidebar); - let description = diesel_option_overwrite(&data.description); - let icon = diesel_option_overwrite_to_url(&data.icon)?; - let banner = diesel_option_overwrite_to_url(&data.banner)?; - - let slur_regex = local_site_to_slur_regex(&local_site); - check_slurs(&data.name, &slur_regex)?; - check_slurs_opt(&data.description, &slur_regex)?; - - if let Some(Some(desc)) = &description { - site_description_length_check(desc)?; - } - - is_valid_body_field(&data.sidebar, false)?; - - let application_question = diesel_option_overwrite(&data.application_question); - check_application_question( - &application_question, - data - .registration_mode - .unwrap_or(local_site.registration_mode), - )?; + validate_create_payload(&local_site, data)?; let actor_id: DbUrl = Url::parse(&context.settings().get_protocol_and_hostname())?.into(); let inbox_url = Some(generate_site_inbox_url(&actor_id)?); let keypair = generate_actor_keypair()?; let site_form = SiteUpdateForm::builder() .name(Some(data.name.clone())) - .sidebar(sidebar) - .description(description) - .icon(icon) - .banner(banner) + .sidebar(diesel_option_overwrite(&data.sidebar)) + .description(diesel_option_overwrite(&data.description)) + .icon(diesel_option_overwrite_to_url(&data.icon)?) + .banner(diesel_option_overwrite_to_url(&data.banner)?) .actor_id(Some(actor_id)) .last_refreshed_at(Some(naive_now())) .inbox_url(inbox_url) @@ -111,7 +84,7 @@ impl PerformCrud for CreateSite { .enable_nsfw(data.enable_nsfw) .community_creation_admin_only(data.community_creation_admin_only) .require_email_verification(data.require_email_verification) - .application_question(application_question) + .application_question(diesel_option_overwrite(&data.application_question)) .private_instance(data.private_instance) .default_theme(data.default_theme.clone()) .default_post_listing_type(data.default_post_listing_type) @@ -163,3 +136,449 @@ impl PerformCrud for CreateSite { }) } } + +fn validate_create_payload(local_site: &LocalSite, create_site: &CreateSite) -> LemmyResult<()> { + // Make sure the site hasn't already been set up... + if local_site.site_setup { + return Err(LemmyError::from_message("site_already_exists")); + }; + + // Check that the slur regex compiles, and returns the regex if valid... + // Prioritize using new slur regex from the request; if not provided, use the existing regex. + let slur_regex = build_and_check_regex( + &create_site + .slur_filter_regex + .as_deref() + .or(local_site.slur_filter_regex.as_deref()), + )?; + + site_name_length_check(&create_site.name)?; + check_slurs(&create_site.name, &slur_regex)?; + + if let Some(desc) = &create_site.description { + site_description_length_check(desc)?; + check_slurs_opt(&create_site.description, &slur_regex)?; + } + + site_default_post_listing_type_check(&create_site.default_post_listing_type)?; + + check_site_visibility_valid( + local_site.private_instance, + local_site.federation_enabled, + &create_site.private_instance, + &create_site.federation_enabled, + )?; + + // Ensure that the sidebar has fewer than the max num characters... + is_valid_body_field(&create_site.sidebar, false)?; + + application_question_check( + &local_site.application_question, + &create_site.application_question, + create_site + .registration_mode + .unwrap_or(local_site.registration_mode), + ) +} + +#[cfg(test)] +mod tests { + use crate::site::create::validate_create_payload; + use lemmy_api_common::site::CreateSite; + use lemmy_db_schema::{source::local_site::LocalSite, ListingType, RegistrationMode}; + + #[test] + fn test_validate_invalid_create_payload() { + let invalid_payloads = [ + ( + "CreateSite attempted on set up LocalSite", + "site_already_exists", + &generate_local_site( + true, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite name matches LocalSite slur filter", + "slurs", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("foo site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite name matches new slur filter", + "slurs", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("zeta site_name"), + None::, + None::, + None::, + Some(String::from("(zeta|alpha)")), + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite listing type is Subscribed, which is invalid", + "invalid_default_post_listing_type", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + Some(ListingType::Subscribed), + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite is both private and federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + Some(true), + Some(true), + None::, + None::, + ), + ), + ( + "LocalSite is private, but CreateSite also makes it federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + Some(true), + None::, + None::, + ), + ), + ( + "CreateSite requires application, but neither it nor LocalSite has an application question", + "application_question_required", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; + + invalid_payloads.iter().enumerate().for_each( + |( + idx, + &(reason, expected_err, local_site, create_site), + )| { + match validate_create_payload( + local_site, + create_site, + ) { + Ok(_) => { + panic!( + "Got Ok, but validation should have failed with error: {} for reason: {}. invalid_payloads.nth({})", + expected_err, reason, idx + ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})", + error.message, + expected_err, + reason, + idx + ) + } + } + }, + ); + } + + #[test] + fn test_validate_valid_create_payload() { + let valid_payloads = [ + ( + "No changes between LocalSite and CreateSite", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite allows clearing and changing values", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(String::new()), + Some(false), + Some(true), + Some(String::new()), + Some(RegistrationMode::Open), + ), + ), + ( + "CreateSite clears existing slur filter regex", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("foo site_name"), + None::, + None::, + None::, + Some(String::new()), + None::, + None::, + None::, + None::, + ), + ), + ( + "LocalSite has application question and CreateSite now requires applications,", + &generate_local_site( + false, + None::, + true, + false, + Some(String::from("question")), + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; + + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(reason, local_site, edit_site))| { + assert!( + validate_create_payload(local_site, edit_site).is_ok(), + "Got Err, but should have got Ok for reason: {}. valid_payloads.nth({})", + reason, + idx + ); + }) + } + + fn generate_local_site( + site_setup: bool, + site_slur_filter_regex: Option, + site_is_private: bool, + site_is_federated: bool, + site_application_question: Option, + site_registration_mode: RegistrationMode, + ) -> LocalSite { + LocalSite { + id: Default::default(), + site_id: Default::default(), + site_setup, + enable_downvotes: false, + enable_nsfw: false, + community_creation_admin_only: false, + require_email_verification: false, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: String::new(), + default_post_listing_type: ListingType::All, + legal_information: None, + hide_modlog_mod_names: false, + application_email_admins: false, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: 0, + federation_enabled: site_is_federated, + captcha_enabled: false, + captcha_difficulty: String::new(), + published: Default::default(), + updated: None, + registration_mode: site_registration_mode, + reports_email_admins: false, + } + } + + // Allow the test helper function to have too many arguments. + // It's either this or generate the entire struct each time for testing. + #[allow(clippy::too_many_arguments)] + fn generate_create_site( + site_name: String, + site_description: Option, + site_sidebar: Option, + site_listing_type: Option, + site_slur_filter_regex: Option, + site_is_private: Option, + site_is_federated: Option, + site_application_question: Option, + site_registration_mode: Option, + ) -> CreateSite { + CreateSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: None, + default_post_listing_type: site_listing_type, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: site_is_federated, + federation_debug: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: site_registration_mode, + auth: Default::default(), + } + } +} diff --git a/crates/api_crud/src/site/mod.rs b/crates/api_crud/src/site/mod.rs index d0c09b93..a98f2057 100644 --- a/crates/api_crud/src/site/mod.rs +++ b/crates/api_crud/src/site/mod.rs @@ -1,19 +1,95 @@ -use lemmy_db_schema::RegistrationMode; -use lemmy_utils::error::LemmyError; +use lemmy_db_schema::{ListingType, RegistrationMode}; +use lemmy_utils::error::{LemmyError, LemmyResult}; mod create; mod read; mod update; -pub fn check_application_question( - application_question: &Option>, +/// Checks whether the default post listing type is valid for a site. +pub fn site_default_post_listing_type_check( + default_post_listing_type: &Option, +) -> LemmyResult<()> { + if let Some(listing_type) = default_post_listing_type { + // Only allow all or local as default listing types... + if listing_type != &ListingType::All && listing_type != &ListingType::Local { + Err(LemmyError::from_message( + "invalid_default_post_listing_type", + )) + } else { + Ok(()) + } + } else { + Ok(()) + } +} + +/// Checks whether the application question and registration mode align. +pub fn application_question_check( + current_application_question: &Option, + new_application_question: &Option, registration_mode: RegistrationMode, -) -> Result<(), LemmyError> { +) -> LemmyResult<()> { + let has_no_question: bool = + current_application_question.is_none() && new_application_question.is_none(); + let is_nullifying_question: bool = new_application_question == &Some(String::new()); + if registration_mode == RegistrationMode::RequireApplication - && application_question.as_ref().unwrap_or(&None).is_none() + && (has_no_question || is_nullifying_question) { Err(LemmyError::from_message("application_question_required")) } else { Ok(()) } } + +#[cfg(test)] +mod tests { + use crate::site::{application_question_check, site_default_post_listing_type_check}; + use lemmy_db_schema::{ListingType, RegistrationMode}; + + #[test] + fn test_site_default_post_listing_type_check() { + assert!(site_default_post_listing_type_check(&None::).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::All)).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::Local)).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::Subscribed)).is_err()); + } + + #[test] + fn test_application_question_check() { + assert!( + application_question_check(&Some(String::from("q")), &Some(String::new()), RegistrationMode::RequireApplication).is_err(), + "Expected application to be invalid because an application is required, current question: {:?}, new question: {:?}", + "q", + String::new(), + ); + assert!( + application_question_check(&None, &None, RegistrationMode::RequireApplication).is_err(), + "Expected application to be invalid because an application is required, current question: {:?}, new question: {:?}", + None::, + None:: + ); + + assert!( + application_question_check(&None, &None, RegistrationMode::Open).is_ok(), + "Expected application to be valid because no application required, current question: {:?}, new question: {:?}, mode: {:?}", + None::, + None::, + RegistrationMode::Open + ); + assert!( + application_question_check(&None, &Some(String::from("q")), RegistrationMode::RequireApplication).is_ok(), + "Expected application to be valid because new application provided, current question: {:?}, new question: {:?}, mode: {:?}", + None::, + Some(String::from("q")), + RegistrationMode::RequireApplication + ); + assert!( + application_question_check(&Some(String::from("q")), &None, RegistrationMode::RequireApplication).is_ok(), + "Expected application to be valid because application existed, current question: {:?}, new question: {:?}, mode: {:?}", + Some(String::from("q")), + None::, + RegistrationMode::RequireApplication + ); + } +} diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index fa800a5a..c9f97e8d 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -1,15 +1,12 @@ -use crate::{site::check_application_question, PerformCrud}; +use crate::{ + site::{application_question_check, site_default_post_listing_type_check}, + PerformCrud, +}; use actix_web::web::Data; use lemmy_api_common::{ context::LemmyContext, site::{EditSite, SiteResponse}, - utils::{ - is_admin, - local_site_rate_limit_to_rate_limit_config, - local_site_to_slur_regex, - local_user_view_from_jwt, - site_description_length_check, - }, + utils::{is_admin, local_site_rate_limit_to_rate_limit_config, local_user_view_from_jwt}, }; use lemmy_db_schema::{ source::{ @@ -24,15 +21,20 @@ use lemmy_db_schema::{ }, traits::Crud, utils::{diesel_option_overwrite, diesel_option_overwrite_to_url, naive_now}, - ListingType, RegistrationMode, }; use lemmy_db_views::structs::SiteView; use lemmy_utils::{ - error::LemmyError, + error::{LemmyError, LemmyResult}, utils::{ slurs::check_slurs_opt, - validation::{check_site_visibility_valid, is_valid_body_field}, + validation::{ + build_and_check_regex, + check_site_visibility_valid, + is_valid_body_field, + site_description_length_check, + site_name_length_check, + }, }, }; @@ -48,51 +50,17 @@ impl PerformCrud for EditSite { let local_site = site_view.local_site; let site = site_view.site; - // Make sure user is an admin + // Make sure user is an admin; other types of users should not update site data... is_admin(&local_user_view)?; - check_site_visibility_valid( - local_site.private_instance, - local_site.federation_enabled, - &data.private_instance, - &data.federation_enabled, - )?; - - let slur_regex = local_site_to_slur_regex(&local_site); - - check_slurs_opt(&data.name, &slur_regex)?; - check_slurs_opt(&data.description, &slur_regex)?; - - if let Some(desc) = &data.description { - site_description_length_check(desc)?; - } - - is_valid_body_field(&data.sidebar, false)?; - - let application_question = diesel_option_overwrite(&data.application_question); - check_application_question( - &application_question, - data - .registration_mode - .unwrap_or(local_site.registration_mode), - )?; - - if let Some(listing_type) = &data.default_post_listing_type { - // only allow all or local as default listing types - if listing_type != &ListingType::All && listing_type != &ListingType::Local { - return Err(LemmyError::from_message( - "invalid_default_post_listing_type", - )); - } - } + validate_update_payload(&local_site, data)?; if let Some(discussion_languages) = data.discussion_languages.clone() { SiteLanguage::update(context.pool(), discussion_languages.clone(), &site).await?; } - let name = data.name.clone(); let site_form = SiteUpdateForm::builder() - .name(name) + .name(data.name.clone()) .sidebar(diesel_option_overwrite(&data.sidebar)) .description(diesel_option_overwrite(&data.description)) .icon(diesel_option_overwrite_to_url(&data.icon)?) @@ -112,7 +80,7 @@ impl PerformCrud for EditSite { .enable_nsfw(data.enable_nsfw) .community_creation_admin_only(data.community_creation_admin_only) .require_email_verification(data.require_email_verification) - .application_question(application_question) + .application_question(diesel_option_overwrite(&data.application_question)) .private_instance(data.private_instance) .default_theme(data.default_theme.clone()) .default_post_listing_type(data.default_post_listing_type) @@ -204,3 +172,411 @@ impl PerformCrud for EditSite { Ok(res) } } + +fn validate_update_payload(local_site: &LocalSite, edit_site: &EditSite) -> LemmyResult<()> { + // Check that the slur regex compiles, and return the regex if valid... + // Prioritize using new slur regex from the request; if not provided, use the existing regex. + let slur_regex = build_and_check_regex( + &edit_site + .slur_filter_regex + .as_deref() + .or(local_site.slur_filter_regex.as_deref()), + )?; + + if let Some(name) = &edit_site.name { + // The name doesn't need to be updated, but if provided it cannot be blanked out... + site_name_length_check(name)?; + check_slurs_opt(&edit_site.name, &slur_regex)?; + } + + if let Some(desc) = &edit_site.description { + site_description_length_check(desc)?; + check_slurs_opt(&edit_site.description, &slur_regex)?; + } + + site_default_post_listing_type_check(&edit_site.default_post_listing_type)?; + + check_site_visibility_valid( + local_site.private_instance, + local_site.federation_enabled, + &edit_site.private_instance, + &edit_site.federation_enabled, + )?; + + // Ensure that the sidebar has fewer than the max num characters... + is_valid_body_field(&edit_site.sidebar, false)?; + + application_question_check( + &local_site.application_question, + &edit_site.application_question, + edit_site + .registration_mode + .unwrap_or(local_site.registration_mode), + ) +} + +#[cfg(test)] +mod tests { + use crate::site::update::validate_update_payload; + use lemmy_api_common::site::EditSite; + use lemmy_db_schema::{source::local_site::LocalSite, ListingType, RegistrationMode}; + + #[test] + fn test_validate_invalid_update_payload() { + let invalid_payloads = [ + ( + "EditSite name matches LocalSite slur filter", + "slurs", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("foo site_name")), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "EditSite name matches new slur filter", + "slurs", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("zeta site_name")), + None::, + None::, + None::, + Some(String::from("(zeta|alpha)")), + None::, + None::, + None::, + None::, + ), + ), + ( + "EditSite listing type is Subscribed, which is invalid", + "invalid_default_post_listing_type", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + Some(ListingType::Subscribed), + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "EditSite is both private and federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + Some(true), + Some(true), + None::, + None::, + ), + ), + ( + "LocalSite is private, but EditSite also makes it federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + None::, + Some(true), + None::, + None::, + ), + ), + ( + "EditSite requires application, but neither it nor LocalSite has an application question", + "application_question_required", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; + + invalid_payloads.iter().enumerate().for_each( + |( + idx, + &(reason, expected_err, local_site, edit_site), + )| { + match validate_update_payload(local_site, edit_site) { + Ok(_) => { + panic!( + "Got Ok, but validation should have failed with error: {} for reason: {}. invalid_payloads.nth({})", + expected_err, reason, idx + ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})", + error.message, + expected_err, + reason, + idx + ) + } + } + }, + ); + } + + #[test] + fn test_validate_valid_update_payload() { + let valid_payloads = [ + ( + "No changes between LocalSite and EditSite", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "EditSite allows clearing and changing values", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(String::new()), + Some(false), + Some(true), + Some(String::new()), + Some(RegistrationMode::Open), + ), + ), + ( + "EditSite name passes slur filter regex", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("foo site_name")), + None::, + None::, + None::, + Some(String::new()), + None::, + None::, + None::, + None::, + ), + ), + ( + "LocalSite has application question and EditSite now requires applications,", + &generate_local_site( + None::, + true, + false, + Some(String::from("question")), + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; + + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(reason, local_site, edit_site))| { + assert!( + validate_update_payload(local_site, edit_site).is_ok(), + "Got Err, but should have got Ok for reason: {}. valid_payloads.nth({})", + reason, + idx + ); + }) + } + + fn generate_local_site( + site_slur_filter_regex: Option, + site_is_private: bool, + site_is_federated: bool, + site_application_question: Option, + site_registration_mode: RegistrationMode, + ) -> LocalSite { + LocalSite { + id: Default::default(), + site_id: Default::default(), + site_setup: true, + enable_downvotes: false, + enable_nsfw: false, + community_creation_admin_only: false, + require_email_verification: false, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: String::new(), + default_post_listing_type: ListingType::All, + legal_information: None, + hide_modlog_mod_names: false, + application_email_admins: false, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: 0, + federation_enabled: site_is_federated, + captcha_enabled: false, + captcha_difficulty: String::new(), + published: Default::default(), + updated: None, + registration_mode: site_registration_mode, + reports_email_admins: false, + } + } + + // Allow the test helper function to have too many arguments. + // It's either this or generate the entire struct each time for testing. + #[allow(clippy::too_many_arguments)] + fn generate_edit_site( + site_name: Option, + site_description: Option, + site_sidebar: Option, + site_listing_type: Option, + site_slur_filter_regex: Option, + site_is_private: Option, + site_is_federated: Option, + site_application_question: Option, + site_registration_mode: Option, + ) -> EditSite { + EditSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: None, + default_post_listing_type: site_listing_type, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: site_is_federated, + federation_debug: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: site_registration_mode, + reports_email_admins: None, + auth: Default::default(), + } + } +} diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs index 621543b4..7e09c5af 100644 --- a/crates/utils/src/utils/validation.rs +++ b/crates/utils/src/utils/validation.rs @@ -1,7 +1,7 @@ use crate::error::{LemmyError, LemmyResult}; use itertools::Itertools; use once_cell::sync::Lazy; -use regex::Regex; +use regex::{Regex, RegexBuilder}; use totp_rs::{Secret, TOTP}; use url::Url; @@ -17,9 +17,13 @@ static CLEAN_URL_PARAMS_REGEX: Lazy = Lazy::new(|| { Regex::new(r"^utm_source|utm_medium|utm_campaign|utm_term|utm_content|gclid|gclsrc|dclid|fbclid$") .expect("compile regex") }); + const BODY_MAX_LENGTH: usize = 10000; const POST_BODY_MAX_LENGTH: usize = 50000; const BIO_MAX_LENGTH: usize = 300; +const SITE_NAME_MAX_LENGTH: usize = 20; +const SITE_NAME_MIN_LENGTH: usize = 1; +const SITE_DESCRIPTION_MAX_LENGTH: usize = 150; fn has_newline(name: &str) -> bool { name.contains('\n') @@ -88,14 +92,83 @@ pub fn is_valid_body_field(body: &Option, post: bool) -> LemmyResult<()> } pub fn is_valid_bio_field(bio: &str) -> LemmyResult<()> { - let check = bio.chars().count() <= BIO_MAX_LENGTH; - if !check { - Err(LemmyError::from_message("bio_length_overflow")) + max_length_check(bio, BIO_MAX_LENGTH, String::from("bio_length_overflow")) +} + +/// Checks the site name length, the limit as defined in the DB. +pub fn site_name_length_check(name: &str) -> LemmyResult<()> { + min_max_length_check( + name, + SITE_NAME_MIN_LENGTH, + SITE_NAME_MAX_LENGTH, + String::from("site_name_required"), + String::from("site_name_length_overflow"), + ) +} + +/// Checks the site description length, the limit as defined in the DB. +pub fn site_description_length_check(description: &str) -> LemmyResult<()> { + max_length_check( + description, + SITE_DESCRIPTION_MAX_LENGTH, + String::from("site_description_length_overflow"), + ) +} + +fn max_length_check(item: &str, max_length: usize, msg: String) -> LemmyResult<()> { + if item.len() > max_length { + Err(LemmyError::from_message(&msg)) + } else { + Ok(()) + } +} + +fn min_max_length_check( + item: &str, + min_length: usize, + max_length: usize, + min_msg: String, + max_msg: String, +) -> LemmyResult<()> { + if item.len() > max_length { + Err(LemmyError::from_message(&max_msg)) + } else if item.len() < min_length { + Err(LemmyError::from_message(&min_msg)) } else { Ok(()) } } +/// Attempts to build a regex and check it for common errors before inserting into the DB. +pub fn build_and_check_regex(regex_str_opt: &Option<&str>) -> LemmyResult> { + regex_str_opt.map_or_else( + || Ok(None::), + |regex_str| { + if regex_str.is_empty() { + // If the proposed regex is empty, return as having no regex at all; this is the same + // behavior that happens downstream before the write to the database. + return Ok(None::); + } + + RegexBuilder::new(regex_str) + .case_insensitive(true) + .build() + .map_err(|e| LemmyError::from_error_message(e, "invalid_regex")) + .and_then(|regex| { + // NOTE: It is difficult to know, in the universe of user-crafted regex, which ones + // may match against any string text. To keep it simple, we'll match the regex + // against an innocuous string - a single number - which should help catch a regex + // that accidentally matches against all strings. + if regex.is_match("1") { + return Err(LemmyError::from_message("permissive_regex")); + } + + Ok(Some(regex)) + }) + }, + ) +} + pub fn clean_url_params(url: &Url) -> Url { let mut url_out = url.clone(); if url.query().is_some() { @@ -177,13 +250,20 @@ pub fn check_site_visibility_valid( mod tests { use super::build_totp_2fa; use crate::utils::validation::{ + build_and_check_regex, check_site_visibility_valid, clean_url_params, generate_totp_2fa_secret, is_valid_actor_name, + is_valid_bio_field, is_valid_display_name, is_valid_matrix_id, is_valid_post_title, + site_description_length_check, + site_name_length_check, + BIO_MAX_LENGTH, + SITE_DESCRIPTION_MAX_LENGTH, + SITE_NAME_MAX_LENGTH, }; use url::Url; @@ -252,6 +332,126 @@ mod tests { assert!(totp.is_ok()); } + #[test] + fn test_valid_site_name() { + let valid_names = [ + (0..SITE_NAME_MAX_LENGTH).map(|_| 'A').collect::(), + String::from("A"), + ]; + let invalid_names = [ + ( + &(0..SITE_NAME_MAX_LENGTH + 1) + .map(|_| 'A') + .collect::(), + "site_name_length_overflow", + ), + (&String::new(), "site_name_required"), + ]; + + valid_names.iter().for_each(|valid_name| { + assert!( + site_name_length_check(valid_name).is_ok(), + "Expected {} of length {} to be Ok.", + valid_name, + valid_name.len() + ) + }); + + invalid_names + .iter() + .for_each(|&(invalid_name, expected_err)| { + let result = site_name_length_check(invalid_name); + + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .message + .eq(&Some(String::from(expected_err))), + "Testing {}, expected error {}", + invalid_name, + expected_err + ); + }); + } + + #[test] + fn test_valid_bio() { + assert!(is_valid_bio_field(&(0..BIO_MAX_LENGTH).map(|_| 'A').collect::()).is_ok()); + + let invalid_result = + is_valid_bio_field(&(0..BIO_MAX_LENGTH + 1).map(|_| 'A').collect::()); + + assert!( + invalid_result.is_err() + && invalid_result + .unwrap_err() + .message + .eq(&Some(String::from("bio_length_overflow"))) + ); + } + + #[test] + fn test_valid_site_description() { + assert!(site_description_length_check( + &(0..SITE_DESCRIPTION_MAX_LENGTH) + .map(|_| 'A') + .collect::() + ) + .is_ok()); + + let invalid_result = site_description_length_check( + &(0..SITE_DESCRIPTION_MAX_LENGTH + 1) + .map(|_| 'A') + .collect::(), + ); + + assert!( + invalid_result.is_err() + && invalid_result + .unwrap_err() + .message + .eq(&Some(String::from("site_description_length_overflow"))) + ); + } + + #[test] + fn test_valid_slur_regex() { + let valid_regexes = [&None, &Some(""), &Some("(foo|bar)")]; + + valid_regexes.iter().for_each(|regex| { + let result = build_and_check_regex(regex); + + assert!(result.is_ok(), "Testing regex: {:?}", regex); + }); + } + + #[test] + fn test_too_permissive_slur_regex() { + let match_everything_regexes = [ + (&Some("["), "invalid_regex"), + (&Some("(foo|bar|)"), "permissive_regex"), + (&Some(".*"), "permissive_regex"), + ]; + + match_everything_regexes + .iter() + .for_each(|&(regex_str, expected_err)| { + let result = build_and_check_regex(regex_str); + + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .message + .eq(&Some(String::from(expected_err))), + "Testing regex {:?}, expected error {}", + regex_str, + expected_err + ); + }); + } + #[test] fn test_check_site_visibility_valid() { assert!(check_site_visibility_valid(true, true, &None, &None).is_err());