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 <me@nutomic.com>

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<String>) -> Result<(), LemmyError> {
   if honeypot.is_some() && honeypot != &Some(String::new()) {

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<LemmyContext>) -> Result<SiteResponse, LemmyError> {
     let data: &CreateSite = self;
-
+    let local_user_view = local_user_view_from_jwt(&data.auth, context).await?;
     let local_site = LocalSite::read(context.pool()).await?;
 
-    if local_site.site_setup {
+    // Make sure user is an admin; other types of users should not create site data...
-      return Err(LemmyError::from_message("site_already_exists"));
-    };
-
-    let local_user_view = local_user_view_from_jwt(&data.auth, context).await?;
-
-    // Make sure user is an admin
     is_admin(&local_user_view)?;
 
-    check_site_visibility_valid(
+    validate_create_payload(&local_site, data)?;
-      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),
-    )?;
 
     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)
+      .sidebar(diesel_option_overwrite(&data.sidebar))
-      .description(description)
+      .description(diesel_option_overwrite(&data.description))
-      .icon(icon)
+      .icon(diesel_option_overwrite_to_url(&data.icon)?)
-      .banner(banner)
+      .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::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite name matches LocalSite slur filter",
+        "slurs",
+        &generate_local_site(
+          false,
+          Some(String::from("(foo|bar)")),
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("foo site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite name matches new slur filter",
+        "slurs",
+        &generate_local_site(
+          false,
+          Some(String::from("(foo|bar)")),
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("zeta site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          Some(String::from("(zeta|alpha)")),
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite listing type is Subscribed, which is invalid",
+        "invalid_default_post_listing_type",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          Some(ListingType::Subscribed),
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite is both private and federated",
+        "cant_enable_private_instance_and_federation_together",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          Some(true),
+          Some(true),
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "LocalSite is private, but CreateSite also makes it federated",
+        "cant_enable_private_instance_and_federation_together",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          Some(true),
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite requires application, but neither it nor LocalSite has an application question",
+        "application_question_required",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          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::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "CreateSite allows clearing and changing values",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          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::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("foo site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          Some(String::new()),
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "LocalSite has application question and CreateSite now requires applications,",
+        &generate_local_site(
+          false,
+          None::<String>,
+          true,
+          false,
+          Some(String::from("question")),
+          RegistrationMode::Open,
+        ),
+        &generate_create_site(
+          String::from("site_name"),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          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<String>,
+    site_is_private: bool,
+    site_is_federated: bool,
+    site_application_question: Option<String>,
+    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<String>,
+    site_sidebar: Option<String>,
+    site_listing_type: Option<ListingType>,
+    site_slur_filter_regex: Option<String>,
+    site_is_private: Option<bool>,
+    site_is_federated: Option<bool>,
+    site_application_question: Option<String>,
+    site_registration_mode: Option<RegistrationMode>,
+  ) -> 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(),
+    }
+  }
+}

crates/api_crud/src/site/mod.rs

@@ -1,19 +1,95 @@
-use lemmy_db_schema::RegistrationMode;
+use lemmy_db_schema::{ListingType, RegistrationMode};
-use lemmy_utils::error::LemmyError;
+use lemmy_utils::error::{LemmyError, LemmyResult};
 
 mod create;
 mod read;
 mod update;
 
-pub fn check_application_question(
+/// Checks whether the default post listing type is valid for a site.
-  application_question: &Option<Option<String>>,
+pub fn site_default_post_listing_type_check(
+  default_post_listing_type: &Option<ListingType>,
+) -> 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<String>,
+  new_application_question: &Option<String>,
   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::<ListingType>).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::<String>,
+      None::<String>
+    );
+
+    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::<String>,
+      None::<String>,
+      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::<String>,
+      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::<String>,
+      RegistrationMode::RequireApplication
+    );
+  }
+}

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::{
+  utils::{is_admin, local_site_rate_limit_to_rate_limit_config, local_user_view_from_jwt},
-    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::{
   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(
+    validate_update_payload(&local_site, data)?;
-      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",
-        ));
-      }
-    }
 
     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::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("foo site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "EditSite name matches new slur filter",
+        "slurs",
+        &generate_local_site(
+          Some(String::from("(foo|bar)")),
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("zeta site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          Some(String::from("(zeta|alpha)")),
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "EditSite listing type is Subscribed, which is invalid",
+        "invalid_default_post_listing_type",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("site_name")),
+          None::<String>,
+          None::<String>,
+          Some(ListingType::Subscribed),
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "EditSite is both private and federated",
+        "cant_enable_private_instance_and_federation_together",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          Some(true),
+          Some(true),
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "LocalSite is private, but EditSite also makes it federated",
+        "cant_enable_private_instance_and_federation_together",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          Some(true),
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "EditSite requires application, but neither it nor LocalSite has an application question",
+        "application_question_required",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          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::<String>,
+          true,
+          false,
+          None::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          None::<String>,
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "EditSite allows clearing and changing values",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          None::<String>,
+          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::<String>,
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("foo site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          Some(String::new()),
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          None::<RegistrationMode>,
+        ),
+      ),
+      (
+        "LocalSite has application question and EditSite now requires applications,",
+        &generate_local_site(
+          None::<String>,
+          true,
+          false,
+          Some(String::from("question")),
+          RegistrationMode::Open,
+        ),
+        &generate_edit_site(
+          Some(String::from("site_name")),
+          None::<String>,
+          None::<String>,
+          None::<ListingType>,
+          None::<String>,
+          None::<bool>,
+          None::<bool>,
+          None::<String>,
+          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<String>,
+    site_is_private: bool,
+    site_is_federated: bool,
+    site_application_question: Option<String>,
+    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<String>,
+    site_description: Option<String>,
+    site_sidebar: Option<String>,
+    site_listing_type: Option<ListingType>,
+    site_slur_filter_regex: Option<String>,
+    site_is_private: Option<bool>,
+    site_is_federated: Option<bool>,
+    site_application_question: Option<String>,
+    site_registration_mode: Option<RegistrationMode>,
+  ) -> 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(),
+    }
+  }
+}

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<Regex> = 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<String>, post: bool) -> LemmyResult<()>
 }
 
 pub fn is_valid_bio_field(bio: &str) -> LemmyResult<()> {
-  let check = bio.chars().count() <= BIO_MAX_LENGTH;
+  max_length_check(bio, BIO_MAX_LENGTH, String::from("bio_length_overflow"))
-  if !check {
+}
-    Err(LemmyError::from_message("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<Option<Regex>> {
+  regex_str_opt.map_or_else(
+    || Ok(None::<Regex>),
+    |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::<Regex>);
+      }
+
+      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>(),
+      String::from("A"),
+    ];
+    let invalid_names = [
+      (
+        &(0..SITE_NAME_MAX_LENGTH + 1)
+          .map(|_| 'A')
+          .collect::<String>(),
+        "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::<String>()).is_ok());
+
+    let invalid_result =
+      is_valid_bio_field(&(0..BIO_MAX_LENGTH + 1).map(|_| 'A').collect::<String>());
+
+    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::<String>()
+    )
+    .is_ok());
+
+    let invalid_result = site_description_length_check(
+      &(0..SITE_DESCRIPTION_MAX_LENGTH + 1)
+        .map(|_| 'A')
+        .collect::<String>(),
+    );
+
+    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());