From 38d4429ae7cef27089263cc39dc0fa9647263d6d Mon Sep 17 00:00:00 2001 From: Dessalines Date: Sat, 15 Apr 2023 10:45:11 -0400 Subject: [PATCH] Adding check to description and body length fields. (#2805) * Adding check to description and body length fields. - Also making the checks return `LemmyError` - Fixes #1747 * Address PR comments. * PR comments 2 --- crates/api/src/community/ban.rs | 7 +- crates/api/src/local_user/ban_person.rs | 8 +- crates/api/src/local_user/save_settings.rs | 15 +-- crates/api_crud/src/comment/create.rs | 7 +- crates/api_crud/src/comment/update.rs | 9 +- crates/api_crud/src/community/create.rs | 7 +- crates/api_crud/src/community/update.rs | 7 +- crates/api_crud/src/post/create.rs | 7 +- crates/api_crud/src/post/update.rs | 8 +- crates/api_crud/src/private_message/create.rs | 7 +- crates/api_crud/src/private_message/update.rs | 8 +- crates/api_crud/src/site/create.rs | 7 +- crates/api_crud/src/site/update.rs | 8 +- crates/api_crud/src/user/create.rs | 4 +- crates/utils/src/utils/validation.rs | 117 ++++++++++++------ 15 files changed, 154 insertions(+), 72 deletions(-) diff --git a/crates/api/src/community/ban.rs b/crates/api/src/community/ban.rs index d3341340..c19df91f 100644 --- a/crates/api/src/community/ban.rs +++ b/crates/api/src/community/ban.rs @@ -23,7 +23,11 @@ use lemmy_db_schema::{ traits::{Bannable, Crud, Followable}, }; use lemmy_db_views_actor::structs::PersonView; -use lemmy_utils::{error::LemmyError, utils::time::naive_from_unix, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{time::naive_from_unix, validation::is_valid_body_field}, + ConnectionId, +}; #[async_trait::async_trait(?Send)] impl Perform for BanFromCommunity { @@ -46,6 +50,7 @@ impl Perform for BanFromCommunity { // Verify that only mods or admins can ban is_mod_or_admin(context.pool(), local_user_view.person.id, community_id).await?; + is_valid_body_field(&data.reason)?; let community_user_ban_form = CommunityPersonBanForm { community_id: data.community_id, diff --git a/crates/api/src/local_user/ban_person.rs b/crates/api/src/local_user/ban_person.rs index 55069742..d2b78043 100644 --- a/crates/api/src/local_user/ban_person.rs +++ b/crates/api/src/local_user/ban_person.rs @@ -14,7 +14,11 @@ use lemmy_db_schema::{ traits::Crud, }; use lemmy_db_views_actor::structs::PersonView; -use lemmy_utils::{error::LemmyError, utils::time::naive_from_unix, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{time::naive_from_unix, validation::is_valid_body_field}, + ConnectionId, +}; #[async_trait::async_trait(?Send)] impl Perform for BanPerson { @@ -33,6 +37,8 @@ impl Perform for BanPerson { // Make sure user is an admin is_admin(&local_user_view)?; + is_valid_body_field(&data.reason)?; + let ban = data.ban; let banned_person_id = data.person_id; let expires = data.expires.map(naive_from_unix); diff --git a/crates/api/src/local_user/save_settings.rs b/crates/api/src/local_user/save_settings.rs index e3c95a3d..0221d0ce 100644 --- a/crates/api/src/local_user/save_settings.rs +++ b/crates/api/src/local_user/save_settings.rs @@ -21,6 +21,7 @@ use lemmy_utils::{ utils::validation::{ build_totp_2fa, generate_totp_2fa_secret, + is_valid_bio_field, is_valid_display_name, is_valid_matrix_id, }, @@ -67,24 +68,18 @@ impl Perform for SaveUserSettings { } if let Some(Some(bio)) = &bio { - if bio.chars().count() > 300 { - return Err(LemmyError::from_message("bio_length_overflow")); - } + is_valid_bio_field(bio)?; } if let Some(Some(display_name)) = &display_name { - if !is_valid_display_name( + is_valid_display_name( display_name.trim(), site_view.local_site.actor_name_max_length as usize, - ) { - return Err(LemmyError::from_message("invalid_username")); - } + )?; } if let Some(Some(matrix_user_id)) = &matrix_user_id { - if !is_valid_matrix_id(matrix_user_id) { - return Err(LemmyError::from_message("invalid_matrix_id")); - } + is_valid_matrix_id(matrix_user_id)?; } let local_user_id = local_user_view.local_user.id; diff --git a/crates/api_crud/src/comment/create.rs b/crates/api_crud/src/comment/create.rs index 70997378..511059ac 100644 --- a/crates/api_crud/src/comment/create.rs +++ b/crates/api_crud/src/comment/create.rs @@ -27,7 +27,11 @@ use lemmy_db_schema::{ }; use lemmy_utils::{ error::LemmyError, - utils::{mention::scrape_text_for_mentions, slurs::remove_slurs}, + utils::{ + mention::scrape_text_for_mentions, + slurs::remove_slurs, + validation::is_valid_body_field, + }, ConnectionId, }; @@ -50,6 +54,7 @@ impl PerformCrud for CreateComment { &data.content.clone(), &local_site_to_slur_regex(&local_site), ); + is_valid_body_field(&Some(content_slurs_removed.clone()))?; // Check for a community ban let post_id = data.post_id; diff --git a/crates/api_crud/src/comment/update.rs b/crates/api_crud/src/comment/update.rs index 1d5f1c5a..1d6d0869 100644 --- a/crates/api_crud/src/comment/update.rs +++ b/crates/api_crud/src/comment/update.rs @@ -18,7 +18,11 @@ use lemmy_db_schema::{ use lemmy_db_views::structs::CommentView; use lemmy_utils::{ error::LemmyError, - utils::{mention::scrape_text_for_mentions, slurs::remove_slurs}, + utils::{ + mention::scrape_text_for_mentions, + slurs::remove_slurs, + validation::is_valid_body_field, + }, ConnectionId, }; @@ -65,6 +69,9 @@ impl PerformCrud for EditComment { .content .as_ref() .map(|c| remove_slurs(c, &local_site_to_slur_regex(&local_site))); + + is_valid_body_field(&content_slurs_removed)?; + let comment_id = data.comment_id; let form = CommentUpdateForm::builder() .content(content_slurs_removed) diff --git a/crates/api_crud/src/community/create.rs b/crates/api_crud/src/community/create.rs index 907cf1b2..e2e80b84 100644 --- a/crates/api_crud/src/community/create.rs +++ b/crates/api_crud/src/community/create.rs @@ -36,7 +36,7 @@ use lemmy_utils::{ error::LemmyError, utils::{ slurs::{check_slurs, check_slurs_opt}, - validation::is_valid_actor_name, + validation::{is_valid_actor_name, is_valid_body_field}, }, ConnectionId, }; @@ -72,9 +72,8 @@ impl PerformCrud for CreateCommunity { check_slurs(&data.title, &slur_regex)?; check_slurs_opt(&data.description, &slur_regex)?; - if !is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize) { - return Err(LemmyError::from_message("invalid_community_name")); - } + is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize)?; + is_valid_body_field(&data.description)?; // Double check for duplicate community actor_ids let community_actor_id = generate_local_apub_endpoint( diff --git a/crates/api_crud/src/community/update.rs b/crates/api_crud/src/community/update.rs index 9628615b..200c5f2c 100644 --- a/crates/api_crud/src/community/update.rs +++ b/crates/api_crud/src/community/update.rs @@ -17,7 +17,11 @@ use lemmy_db_schema::{ utils::{diesel_option_overwrite, diesel_option_overwrite_to_url, naive_now}, }; use lemmy_db_views_actor::structs::CommunityModeratorView; -use lemmy_utils::{error::LemmyError, utils::slurs::check_slurs_opt, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{slurs::check_slurs_opt, validation::is_valid_body_field}, + ConnectionId, +}; #[async_trait::async_trait(?Send)] impl PerformCrud for EditCommunity { @@ -41,6 +45,7 @@ impl PerformCrud for EditCommunity { let slur_regex = local_site_to_slur_regex(&local_site); check_slurs_opt(&data.title, &slur_regex)?; check_slurs_opt(&data.description, &slur_regex)?; + is_valid_body_field(&data.description)?; // Verify its a mod (only mods can edit it) let community_id = data.community_id; diff --git a/crates/api_crud/src/post/create.rs b/crates/api_crud/src/post/create.rs index 2ceade21..1a885184 100644 --- a/crates/api_crud/src/post/create.rs +++ b/crates/api_crud/src/post/create.rs @@ -31,7 +31,7 @@ use lemmy_utils::{ error::LemmyError, utils::{ slurs::{check_slurs, check_slurs_opt}, - validation::{clean_url_params, is_valid_post_title}, + validation::{clean_url_params, is_valid_body_field, is_valid_post_title}, }, ConnectionId, }; @@ -62,9 +62,8 @@ impl PerformCrud for CreatePost { let data_url = data.url.as_ref(); let url = data_url.map(clean_url_params).map(Into::into); // TODO no good way to handle a "clear" - if !is_valid_post_title(&data.name) { - return Err(LemmyError::from_message("invalid_post_title")); - } + is_valid_post_title(&data.name)?; + is_valid_body_field(&data.body)?; check_community_ban(local_user_view.person.id, data.community_id, context.pool()).await?; check_community_deleted_or_removed(data.community_id, context.pool()).await?; diff --git a/crates/api_crud/src/post/update.rs b/crates/api_crud/src/post/update.rs index 27cfabe5..8f5c442f 100644 --- a/crates/api_crud/src/post/update.rs +++ b/crates/api_crud/src/post/update.rs @@ -20,7 +20,7 @@ use lemmy_utils::{ error::LemmyError, utils::{ slurs::check_slurs_opt, - validation::{clean_url_params, is_valid_post_title}, + validation::{clean_url_params, is_valid_body_field, is_valid_post_title}, }, ConnectionId, }; @@ -52,11 +52,11 @@ impl PerformCrud for EditPost { check_slurs_opt(&data.body, &slur_regex)?; if let Some(name) = &data.name { - if !is_valid_post_title(name) { - return Err(LemmyError::from_message("invalid_post_title")); - } + is_valid_post_title(name)?; } + is_valid_body_field(&data.body)?; + let post_id = data.post_id; let orig_post = Post::read(context.pool(), post_id).await?; diff --git a/crates/api_crud/src/private_message/create.rs b/crates/api_crud/src/private_message/create.rs index 28c243f6..48fa8402 100644 --- a/crates/api_crud/src/private_message/create.rs +++ b/crates/api_crud/src/private_message/create.rs @@ -22,7 +22,11 @@ use lemmy_db_schema::{ traits::Crud, }; use lemmy_db_views::structs::LocalUserView; -use lemmy_utils::{error::LemmyError, utils::slurs::remove_slurs, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{slurs::remove_slurs, validation::is_valid_body_field}, + ConnectionId, +}; #[async_trait::async_trait(?Send)] impl PerformCrud for CreatePrivateMessage { @@ -43,6 +47,7 @@ impl PerformCrud for CreatePrivateMessage { &data.content.clone(), &local_site_to_slur_regex(&local_site), ); + is_valid_body_field(&Some(content_slurs_removed.clone()))?; check_person_block(local_user_view.person.id, data.recipient_id, context.pool()).await?; diff --git a/crates/api_crud/src/private_message/update.rs b/crates/api_crud/src/private_message/update.rs index 06bd349f..6a6bfa21 100644 --- a/crates/api_crud/src/private_message/update.rs +++ b/crates/api_crud/src/private_message/update.rs @@ -14,7 +14,11 @@ use lemmy_db_schema::{ traits::Crud, utils::naive_now, }; -use lemmy_utils::{error::LemmyError, utils::slurs::remove_slurs, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{slurs::remove_slurs, validation::is_valid_body_field}, + ConnectionId, +}; #[async_trait::async_trait(?Send)] impl PerformCrud for EditPrivateMessage { @@ -40,6 +44,8 @@ impl PerformCrud for EditPrivateMessage { // Doing the update let content_slurs_removed = remove_slurs(&data.content, &local_site_to_slur_regex(&local_site)); + is_valid_body_field(&Some(content_slurs_removed.clone()))?; + let private_message_id = data.private_message_id; PrivateMessage::update( context.pool(), diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index bda7eadc..ecafa9b2 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -26,7 +26,10 @@ use lemmy_db_schema::{ use lemmy_db_views::structs::SiteView; use lemmy_utils::{ error::LemmyError, - utils::slurs::{check_slurs, check_slurs_opt}, + utils::{ + slurs::{check_slurs, check_slurs_opt}, + validation::is_valid_body_field, + }, ConnectionId, }; use url::Url; @@ -68,6 +71,8 @@ impl PerformCrud for CreateSite { site_description_length_check(desc)?; } + is_valid_body_field(&data.sidebar)?; + let application_question = diesel_option_overwrite(&data.application_question); check_application_question( &application_question, diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 3922fdab..eb29a6da 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -28,7 +28,11 @@ use lemmy_db_schema::{ ListingType, }; use lemmy_db_views::structs::SiteView; -use lemmy_utils::{error::LemmyError, utils::slurs::check_slurs_opt, ConnectionId}; +use lemmy_utils::{ + error::LemmyError, + utils::{slurs::check_slurs_opt, validation::is_valid_body_field}, + ConnectionId, +}; use std::str::FromStr; #[async_trait::async_trait(?Send)] @@ -60,6 +64,8 @@ impl PerformCrud for EditSite { site_description_length_check(desc)?; } + is_valid_body_field(&data.sidebar)?; + let application_question = diesel_option_overwrite(&data.application_question); check_application_question( &application_question, diff --git a/crates/api_crud/src/user/create.rs b/crates/api_crud/src/user/create.rs index 0554da05..f90393c2 100644 --- a/crates/api_crud/src/user/create.rs +++ b/crates/api_crud/src/user/create.rs @@ -96,9 +96,7 @@ impl PerformCrud for Register { check_slurs_opt(&data.answer, &slur_regex)?; let actor_keypair = generate_actor_keypair()?; - if !is_valid_actor_name(&data.username, local_site.actor_name_max_length as usize) { - return Err(LemmyError::from_message("invalid_username")); - } + is_valid_actor_name(&data.username, local_site.actor_name_max_length as usize)?; let actor_id = generate_local_apub_endpoint( EndpointType::Person, &data.username, diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs index 37838866..c4feb467 100644 --- a/crates/utils/src/utils/validation.rs +++ b/crates/utils/src/utils/validation.rs @@ -1,4 +1,4 @@ -use crate::error::LemmyError; +use crate::error::{LemmyError, LemmyResult}; use itertools::Itertools; use once_cell::sync::Lazy; use regex::Regex; @@ -17,32 +17,77 @@ 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 BIO_MAX_LENGTH: usize = 300; fn has_newline(name: &str) -> bool { name.contains('\n') } -pub fn is_valid_actor_name(name: &str, actor_name_max_length: usize) -> bool { - name.chars().count() <= actor_name_max_length +pub fn is_valid_actor_name(name: &str, actor_name_max_length: usize) -> LemmyResult<()> { + let check = name.chars().count() <= actor_name_max_length && VALID_ACTOR_NAME_REGEX.is_match(name) - && !has_newline(name) + && !has_newline(name); + if !check { + Err(LemmyError::from_message("invalid_name")) + } else { + Ok(()) + } } // Can't do a regex here, reverse lookarounds not supported -pub fn is_valid_display_name(name: &str, actor_name_max_length: usize) -> bool { - !name.starts_with('@') +pub fn is_valid_display_name(name: &str, actor_name_max_length: usize) -> LemmyResult<()> { + let check = !name.starts_with('@') && !name.starts_with('\u{200b}') && name.chars().count() >= 3 && name.chars().count() <= actor_name_max_length - && !has_newline(name) + && !has_newline(name); + if !check { + Err(LemmyError::from_message("invalid_username")) + } else { + Ok(()) + } +} + +pub fn is_valid_matrix_id(matrix_id: &str) -> LemmyResult<()> { + let check = VALID_MATRIX_ID_REGEX.is_match(matrix_id) && !has_newline(matrix_id); + if !check { + Err(LemmyError::from_message("invalid_matrix_id")) + } else { + Ok(()) + } } -pub fn is_valid_matrix_id(matrix_id: &str) -> bool { - VALID_MATRIX_ID_REGEX.is_match(matrix_id) && !has_newline(matrix_id) +pub fn is_valid_post_title(title: &str) -> LemmyResult<()> { + let check = VALID_POST_TITLE_REGEX.is_match(title) && !has_newline(title); + if !check { + Err(LemmyError::from_message("invalid_post_title")) + } else { + Ok(()) + } } -pub fn is_valid_post_title(title: &str) -> bool { - VALID_POST_TITLE_REGEX.is_match(title) && !has_newline(title) +/// This could be post bodies, comments, or any description field +pub fn is_valid_body_field(body: &Option) -> LemmyResult<()> { + if let Some(body) = body { + let check = body.chars().count() <= BODY_MAX_LENGTH; + if !check { + Err(LemmyError::from_message("invalid_body_field")) + } else { + Ok(()) + } + } else { + Ok(()) + } +} + +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")) + } else { + Ok(()) + } } pub fn clean_url_params(url: &Url) -> Url { @@ -63,7 +108,7 @@ pub fn check_totp_2fa_valid( totp_token: &Option, site_name: &str, username: &str, -) -> Result<(), LemmyError> { +) -> LemmyResult<()> { // Check only if they have a totp_secret in the DB if let Some(totp_secret) = totp_secret { // Throw an error if their token is missing @@ -131,52 +176,48 @@ mod tests { #[test] fn regex_checks() { - assert!(!is_valid_post_title("hi")); - assert!(is_valid_post_title("him")); - assert!(!is_valid_post_title("n\n\n\n\nanother")); - assert!(!is_valid_post_title("hello there!\n this is a test.")); - assert!(is_valid_post_title("hello there! this is a test.")); + assert!(is_valid_post_title("hi").is_err()); + assert!(is_valid_post_title("him").is_ok()); + assert!(is_valid_post_title("n\n\n\n\nanother").is_err()); + assert!(is_valid_post_title("hello there!\n this is a test.").is_err()); + assert!(is_valid_post_title("hello there! this is a test.").is_ok()); } #[test] fn test_valid_actor_name() { let actor_name_max_length = 20; - assert!(is_valid_actor_name("Hello_98", actor_name_max_length)); - assert!(is_valid_actor_name("ten", actor_name_max_length)); - assert!(!is_valid_actor_name("Hello-98", actor_name_max_length)); - assert!(!is_valid_actor_name("a", actor_name_max_length)); - assert!(!is_valid_actor_name("", actor_name_max_length)); + assert!(is_valid_actor_name("Hello_98", actor_name_max_length).is_ok()); + assert!(is_valid_actor_name("ten", actor_name_max_length).is_ok()); + assert!(is_valid_actor_name("Hello-98", actor_name_max_length).is_err()); + assert!(is_valid_actor_name("a", actor_name_max_length).is_err()); + assert!(is_valid_actor_name("", actor_name_max_length).is_err()); } #[test] fn test_valid_display_name() { let actor_name_max_length = 20; - assert!(is_valid_display_name("hello @there", actor_name_max_length)); - assert!(!is_valid_display_name( - "@hello there", - actor_name_max_length - )); + assert!(is_valid_display_name("hello @there", actor_name_max_length).is_ok()); + assert!(is_valid_display_name("@hello there", actor_name_max_length).is_err()); // Make sure zero-space with an @ doesn't work - assert!(!is_valid_display_name( - &format!("{}@my name is", '\u{200b}'), - actor_name_max_length - )); + assert!( + is_valid_display_name(&format!("{}@my name is", '\u{200b}'), actor_name_max_length).is_err() + ); } #[test] fn test_valid_post_title() { - assert!(is_valid_post_title("Post Title")); - assert!(is_valid_post_title(" POST TITLE 😃😃😃😃😃")); - assert!(!is_valid_post_title("\n \n \n \n ")); // tabs/spaces/newlines + assert!(is_valid_post_title("Post Title").is_ok()); + assert!(is_valid_post_title(" POST TITLE 😃😃😃😃😃").is_ok()); + assert!(is_valid_post_title("\n \n \n \n ").is_err()); // tabs/spaces/newlines } #[test] fn test_valid_matrix_id() { - assert!(is_valid_matrix_id("@dess:matrix.org")); - assert!(!is_valid_matrix_id("dess:matrix.org")); - assert!(!is_valid_matrix_id(" @dess:matrix.org")); - assert!(!is_valid_matrix_id("@dess:matrix.org t")); + assert!(is_valid_matrix_id("@dess:matrix.org").is_ok()); + assert!(is_valid_matrix_id("dess:matrix.org").is_err()); + assert!(is_valid_matrix_id(" @dess:matrix.org").is_err()); + assert!(is_valid_matrix_id("@dess:matrix.org t").is_err()); } #[test] -- 2.44.1