From 16a82862b834bed0a3a7e430057ece8216c26295 Mon Sep 17 00:00:00 2001 From: Dessalines Date: Thu, 6 Jun 2024 09:55:08 -0400 Subject: [PATCH] Allow empty string to clear URL-type DB fields. (#4780) * Allow empty string to clear URL-type DB fields. - To address difficulties with clearing URL-type fields like avatars, banners, site icons, this PR turns the URL type form fields into strings. - This allows an empty string to be used as a "clear data", as in the case with the regular text form fields. - Also includes various cleanups. - Fixes #4777 - Context: #2287 * Fixing comment. * Use Option<&str> and deref. --------- Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com> --- api_tests/src/user.spec.ts | 11 ++ crates/api/src/community/ban.rs | 5 +- crates/api/src/local_user/ban_person.rs | 4 +- crates/api/src/local_user/save_settings.rs | 26 ++-- crates/api/src/post/get_link_metadata.rs | 9 +- .../site/registration_applications/approve.rs | 4 +- crates/api_common/src/post.rs | 16 +-- crates/api_common/src/request.rs | 6 +- crates/api_common/src/utils.rs | 50 ++------ crates/api_crud/src/comment/create.rs | 2 +- crates/api_crud/src/comment/update.rs | 4 +- crates/api_crud/src/community/create.rs | 14 +- crates/api_crud/src/community/update.rs | 28 ++-- crates/api_crud/src/post/create.rs | 31 +++-- crates/api_crud/src/post/update.rs | 52 +++++--- crates/api_crud/src/private_message/create.rs | 2 +- crates/api_crud/src/private_message/update.rs | 2 +- crates/api_crud/src/site/create.rs | 32 +++-- crates/api_crud/src/site/update.rs | 37 ++++-- crates/apub/src/objects/post.rs | 5 +- crates/db_schema/src/utils.rs | 54 ++++---- crates/utils/src/utils/validation.rs | 120 ++++++++---------- 22 files changed, 279 insertions(+), 235 deletions(-) diff --git a/api_tests/src/user.spec.ts b/api_tests/src/user.spec.ts index 4f91cbd87..d008dcdc3 100644 --- a/api_tests/src/user.spec.ts +++ b/api_tests/src/user.spec.ts @@ -21,6 +21,7 @@ import { fetchFunction, alphaImage, unfollows, + saveUserSettingsBio, } from "./shared"; import { LemmyHttp, SaveUserSettings, UploadImage } from "lemmy-js-client"; import { GetPosts } from "lemmy-js-client/dist/types/GetPosts"; @@ -198,4 +199,14 @@ test("Set a new avatar, old avatar is deleted", async () => { // make sure only the new avatar is kept const listMediaRes3 = await alphaImage.listMedia(); expect(listMediaRes3.images.length).toBe(1); + + // Now try to save a user settings, with the icon missing, + // and make sure it doesn't clear the data, or delete the image + await saveUserSettingsBio(alpha); + let site = await getSite(alpha); + expect(site.my_user?.local_user_view.person.avatar).toBe(upload2.url); + + // make sure only the new avatar is kept + const listMediaRes4 = await alphaImage.listMedia(); + expect(listMediaRes4.images.length).toBe(1); }); diff --git a/crates/api/src/community/ban.rs b/crates/api/src/community/ban.rs index 93cf00415..877d9464f 100644 --- a/crates/api/src/community/ban.rs +++ b/crates/api/src/community/ban.rs @@ -43,7 +43,10 @@ pub async fn ban_from_community( &mut context.pool(), ) .await?; - is_valid_body_field(&data.reason, false)?; + + if let Some(reason) = &data.reason { + is_valid_body_field(reason, false)?; + } 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 c31940fba..49cd6893a 100644 --- a/crates/api/src/local_user/ban_person.rs +++ b/crates/api/src/local_user/ban_person.rs @@ -31,7 +31,9 @@ pub async fn ban_from_site( // Make sure user is an admin is_admin(&local_user_view)?; - is_valid_body_field(&data.reason, false)?; + if let Some(reason) = &data.reason { + is_valid_body_field(reason, false)?; + } let expires = check_expire_time(data.expires)?; diff --git a/crates/api/src/local_user/save_settings.rs b/crates/api/src/local_user/save_settings.rs index bdba817cc..193f9d269 100644 --- a/crates/api/src/local_user/save_settings.rs +++ b/crates/api/src/local_user/save_settings.rs @@ -21,7 +21,7 @@ use lemmy_db_schema::{ person::{Person, PersonUpdateForm}, }, traits::Crud, - utils::diesel_option_overwrite, + utils::{diesel_string_update, diesel_url_update}, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -42,18 +42,24 @@ pub async fn save_user_settings( let slur_regex = local_site_to_slur_regex(&site_view.local_site); let url_blocklist = get_url_blocklist(&context).await?; - let bio = diesel_option_overwrite( - process_markdown_opt(&data.bio, &slur_regex, &url_blocklist, &context).await?, + let bio = diesel_string_update( + process_markdown_opt(&data.bio, &slur_regex, &url_blocklist, &context) + .await? + .as_deref(), ); - replace_image(&data.avatar, &local_user_view.person.avatar, &context).await?; - replace_image(&data.banner, &local_user_view.person.banner, &context).await?; - let avatar = proxy_image_link_opt_api(&data.avatar, &context).await?; - let banner = proxy_image_link_opt_api(&data.banner, &context).await?; - let display_name = diesel_option_overwrite(data.display_name.clone()); - let matrix_user_id = diesel_option_overwrite(data.matrix_user_id.clone()); + let avatar = diesel_url_update(data.avatar.as_deref())?; + replace_image(&avatar, &local_user_view.person.avatar, &context).await?; + let avatar = proxy_image_link_opt_api(avatar, &context).await?; + + let banner = diesel_url_update(data.banner.as_deref())?; + replace_image(&banner, &local_user_view.person.banner, &context).await?; + let banner = proxy_image_link_opt_api(banner, &context).await?; + + let display_name = diesel_string_update(data.display_name.as_deref()); + let matrix_user_id = diesel_string_update(data.matrix_user_id.as_deref()); let email_deref = data.email.as_deref().map(str::to_lowercase); - let email = diesel_option_overwrite(email_deref.clone()); + let email = diesel_string_update(email_deref.as_deref()); if let Some(Some(email)) = &email { let previous_email = local_user_view.local_user.email.clone().unwrap_or_default(); diff --git a/crates/api/src/post/get_link_metadata.rs b/crates/api/src/post/get_link_metadata.rs index 17346790a..0669408aa 100644 --- a/crates/api/src/post/get_link_metadata.rs +++ b/crates/api/src/post/get_link_metadata.rs @@ -4,14 +4,19 @@ use lemmy_api_common::{ post::{GetSiteMetadata, GetSiteMetadataResponse}, request::fetch_link_metadata, }; -use lemmy_utils::error::LemmyResult; +use lemmy_utils::{ + error::{LemmyErrorExt, LemmyResult}, + LemmyErrorType, +}; +use url::Url; #[tracing::instrument(skip(context))] pub async fn get_link_metadata( data: Query, context: Data, ) -> LemmyResult> { - let metadata = fetch_link_metadata(&data.url, &context).await?; + let url = Url::parse(&data.url).with_lemmy_type(LemmyErrorType::InvalidUrl)?; + let metadata = fetch_link_metadata(&url, &context).await?; Ok(Json(GetSiteMetadataResponse { metadata })) } diff --git a/crates/api/src/site/registration_applications/approve.rs b/crates/api/src/site/registration_applications/approve.rs index 0fb55ffc8..823af54c4 100644 --- a/crates/api/src/site/registration_applications/approve.rs +++ b/crates/api/src/site/registration_applications/approve.rs @@ -10,7 +10,7 @@ use lemmy_db_schema::{ registration_application::{RegistrationApplication, RegistrationApplicationUpdateForm}, }, traits::Crud, - utils::diesel_option_overwrite, + utils::diesel_string_update, }; use lemmy_db_views::structs::{LocalUserView, RegistrationApplicationView}; use lemmy_utils::{error::LemmyResult, LemmyErrorType}; @@ -26,7 +26,7 @@ pub async fn approve_registration_application( is_admin(&local_user_view)?; // Update the registration with reason, admin_id - let deny_reason = diesel_option_overwrite(data.deny_reason.clone()); + let deny_reason = diesel_string_update(data.deny_reason.as_deref()); let app_form = RegistrationApplicationUpdateForm { admin_id: Some(Some(local_user_view.person.id)), deny_reason, diff --git a/crates/api_common/src/post.rs b/crates/api_common/src/post.rs index 49327dac1..3d1bc4078 100644 --- a/crates/api_common/src/post.rs +++ b/crates/api_common/src/post.rs @@ -10,7 +10,6 @@ use serde::{Deserialize, Serialize}; use serde_with::skip_serializing_none; #[cfg(feature = "full")] use ts_rs::TS; -use url::Url; #[skip_serializing_none] #[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)] @@ -20,8 +19,7 @@ use url::Url; pub struct CreatePost { pub name: String, pub community_id: CommunityId, - #[cfg_attr(feature = "full", ts(type = "string"))] - pub url: Option, + pub url: Option, /// An optional body for the post in markdown. pub body: Option, /// An optional alt_text, usable for image posts. @@ -30,9 +28,8 @@ pub struct CreatePost { pub honeypot: Option, pub nsfw: Option, pub language_id: Option, - #[cfg_attr(feature = "full", ts(type = "string"))] /// Instead of fetching a thumbnail, use a custom one. - pub custom_thumbnail: Option, + pub custom_thumbnail: Option, } #[derive(Debug, Serialize, Deserialize, Clone)] @@ -114,17 +111,15 @@ pub struct CreatePostLike { pub struct EditPost { pub post_id: PostId, pub name: Option, - #[cfg_attr(feature = "full", ts(type = "string"))] - pub url: Option, + pub url: Option, /// An optional body for the post in markdown. pub body: Option, /// An optional alt_text, usable for image posts. pub alt_text: Option, pub nsfw: Option, pub language_id: Option, - #[cfg_attr(feature = "full", ts(type = "string"))] /// Instead of fetching a thumbnail, use a custom one. - pub custom_thumbnail: Option, + pub custom_thumbnail: Option, } #[derive(Debug, Serialize, Deserialize, Clone, Copy, Default, PartialEq, Eq, Hash)] @@ -249,8 +244,7 @@ pub struct ListPostReportsResponse { #[cfg_attr(feature = "full", ts(export))] /// Get metadata for a given site. pub struct GetSiteMetadata { - #[cfg_attr(feature = "full", ts(type = "string"))] - pub url: Url, + pub url: String, } #[derive(Debug, Serialize, Deserialize, Clone)] diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 9bfb97b72..8a423ff7c 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -340,15 +340,15 @@ async fn is_image_content_type(client: &ClientWithMiddleware, url: &Url) -> Lemm /// When adding a new avatar, banner or similar image, delete the old one. pub async fn replace_image( - new_image: &Option, + new_image: &Option>, old_image: &Option, context: &Data, ) -> LemmyResult<()> { - if let (Some(new_image), Some(old_image)) = (new_image, old_image) { + if let (Some(Some(new_image)), Some(old_image)) = (new_image, old_image) { // Note: Oftentimes front ends will include the current image in the form. // In this case, deleting `old_image` would also be deletion of `new_image`, // so the deletion must be skipped for the image to be kept. - if new_image != old_image.as_str() { + if new_image != old_image { // Ignore errors because image may be stored externally. let image = LocalImage::delete_by_url(&mut context.pool(), old_image) .await diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index bad3b8180..ba5756998 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1004,26 +1004,25 @@ pub(crate) async fn proxy_image_link(link: Url, context: &LemmyContext) -> Lemmy } pub async fn proxy_image_link_opt_api( - link: &Option, + link: Option>, context: &LemmyContext, ) -> LemmyResult>> { - proxy_image_link_api(link, context).await.map(Some) + if let Some(Some(link)) = link { + proxy_image_link(link.into(), context) + .await + .map(Some) + .map(Some) + } else { + Ok(link) + } } pub async fn proxy_image_link_api( - link: &Option, + link: Option, context: &LemmyContext, ) -> LemmyResult> { - let link: Option = match link.as_ref().map(String::as_str) { - // An empty string is an erase - Some("") => None, - Some(str_url) => Url::parse(str_url) - .map(|u| Some(u.into())) - .with_lemmy_type(LemmyErrorType::InvalidUrl)?, - None => None, - }; - if let Some(l) = link { - proxy_image_link(l.into(), context).await.map(Some) + if let Some(link) = link { + proxy_image_link(link.into(), context).await.map(Some) } else { Ok(link) } @@ -1130,29 +1129,4 @@ mod tests { .is_ok() ); } - - #[tokio::test] - #[serial] - async fn test_diesel_option_overwrite_to_url() { - let context = LemmyContext::init_test_context().await; - - assert!(matches!( - proxy_image_link_api(&None, &context).await, - Ok(None) - )); - assert!(matches!( - proxy_image_link_opt_api(&Some(String::new()), &context).await, - Ok(Some(None)) - )); - assert!( - proxy_image_link_opt_api(&Some("invalid_url".to_string()), &context) - .await - .is_err() - ); - let example_url = "https://lemmy-alpha/image.png"; - assert!(matches!( - proxy_image_link_opt_api(&Some(example_url.to_string()), &context).await, - Ok(Some(Some(url))) if url == Url::parse(example_url).unwrap().into() - )); - } } diff --git a/crates/api_crud/src/comment/create.rs b/crates/api_crud/src/comment/create.rs index 2efd46964..636f83392 100644 --- a/crates/api_crud/src/comment/create.rs +++ b/crates/api_crud/src/comment/create.rs @@ -47,7 +47,7 @@ pub async fn create_comment( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?; - is_valid_body_field(&Some(content.clone()), false)?; + is_valid_body_field(&content, false)?; // 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 695ededfe..4c8cf9436 100644 --- a/crates/api_crud/src/comment/update.rs +++ b/crates/api_crud/src/comment/update.rs @@ -63,7 +63,9 @@ pub async fn update_comment( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; let content = process_markdown_opt(&data.content, &slur_regex, &url_blocklist, &context).await?; - is_valid_body_field(&content, false)?; + if let Some(content) = &content { + is_valid_body_field(content, false)?; + } let comment_id = data.comment_id; let form = CommentUpdateForm { diff --git a/crates/api_crud/src/community/create.rs b/crates/api_crud/src/community/create.rs index b0b6bea0e..4289b7d24 100644 --- a/crates/api_crud/src/community/create.rs +++ b/crates/api_crud/src/community/create.rs @@ -30,6 +30,7 @@ use lemmy_db_schema::{ }, }, traits::{ApubActor, Crud, Followable, Joinable}, + utils::diesel_url_create, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -61,11 +62,18 @@ pub async fn create_community( check_slurs(&data.title, &slur_regex)?; let description = process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context).await?; - let icon = proxy_image_link_api(&data.icon, &context).await?; - let banner = proxy_image_link_api(&data.banner, &context).await?; + + let icon = diesel_url_create(data.icon.as_deref())?; + let icon = proxy_image_link_api(icon, &context).await?; + + let banner = diesel_url_create(data.banner.as_deref())?; + let banner = proxy_image_link_api(banner, &context).await?; is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize)?; - is_valid_body_field(&data.description, false)?; + + if let Some(desc) = &data.description { + is_valid_body_field(desc, false)?; + } // 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 33c6a47dd..6190a0ca7 100644 --- a/crates/api_crud/src/community/update.rs +++ b/crates/api_crud/src/community/update.rs @@ -21,7 +21,7 @@ use lemmy_db_schema::{ local_site::LocalSite, }, traits::Crud, - utils::{diesel_option_overwrite, naive_now}, + utils::{diesel_string_update, diesel_url_update, naive_now}, }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::{ @@ -40,18 +40,28 @@ pub async fn update_community( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; check_slurs_opt(&data.title, &slur_regex)?; - let description = - process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context).await?; - is_valid_body_field(&data.description, false)?; + + let description = diesel_string_update( + process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context) + .await? + .as_deref(), + ); + + if let Some(Some(desc)) = &description { + is_valid_body_field(desc, false)?; + } + let old_community = Community::read(&mut context.pool(), data.community_id) .await? .ok_or(LemmyErrorType::CouldntFindCommunity)?; - replace_image(&data.icon, &old_community.icon, &context).await?; - replace_image(&data.banner, &old_community.banner, &context).await?; - let description = diesel_option_overwrite(description); - let icon = proxy_image_link_opt_api(&data.icon, &context).await?; - let banner = proxy_image_link_opt_api(&data.banner, &context).await?; + let icon = diesel_url_update(data.icon.as_deref())?; + replace_image(&icon, &old_community.icon, &context).await?; + let icon = proxy_image_link_opt_api(icon, &context).await?; + + let banner = diesel_url_update(data.banner.as_deref())?; + replace_image(&banner, &old_community.banner, &context).await?; + let banner = proxy_image_link_opt_api(banner, &context).await?; // Verify its a mod (only mods can edit it) check_community_mod_action( diff --git a/crates/api_crud/src/post/create.rs b/crates/api_crud/src/post/create.rs index 21a989d3f..0b0fad5dc 100644 --- a/crates/api_crud/src/post/create.rs +++ b/crates/api_crud/src/post/create.rs @@ -26,6 +26,7 @@ use lemmy_db_schema::{ post::{Post, PostInsertForm, PostLike, PostLikeForm, PostUpdateForm}, }, traits::{Crud, Likeable}, + utils::diesel_url_create, CommunityVisibility, }; use lemmy_db_views::structs::LocalUserView; @@ -37,7 +38,6 @@ use lemmy_utils::{ slurs::check_slurs, validation::{ check_url_scheme, - clean_url_params, is_url_blocked, is_valid_alt_text_field, is_valid_body_field, @@ -64,16 +64,27 @@ pub async fn create_post( let url_blocklist = get_url_blocklist(&context).await?; let body = process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context).await?; - let data_url = data.url.as_ref(); - let url = data_url.map(clean_url_params); // TODO no good way to handle a "clear" - let custom_thumbnail = data.custom_thumbnail.as_ref().map(clean_url_params); + let url = diesel_url_create(data.url.as_deref())?; + let custom_thumbnail = diesel_url_create(data.custom_thumbnail.as_deref())?; is_valid_post_title(&data.name)?; - is_valid_body_field(&body, true)?; - is_valid_alt_text_field(&data.alt_text)?; - is_url_blocked(&url, &url_blocklist)?; - check_url_scheme(&url)?; - check_url_scheme(&custom_thumbnail)?; + + if let Some(url) = &url { + is_url_blocked(url, &url_blocklist)?; + check_url_scheme(url)?; + } + + if let Some(custom_thumbnail) = &custom_thumbnail { + check_url_scheme(custom_thumbnail)?; + } + + if let Some(alt_text) = &data.alt_text { + is_valid_alt_text_field(alt_text)?; + } + + if let Some(body) = &body { + is_valid_body_field(body, true)?; + } check_community_user_action( &local_user_view.person, @@ -156,7 +167,7 @@ pub async fn create_post( generate_post_link_metadata( updated_post.clone(), - custom_thumbnail, + custom_thumbnail.map(Into::into), |post| Some(SendActivityData::CreatePost(post)), Some(local_site), context.reset_request_count(), diff --git a/crates/api_crud/src/post/update.rs b/crates/api_crud/src/post/update.rs index 4b4bd9845..9e665aed6 100644 --- a/crates/api_crud/src/post/update.rs +++ b/crates/api_crud/src/post/update.rs @@ -20,16 +20,15 @@ use lemmy_db_schema::{ post::{Post, PostUpdateForm}, }, traits::Crud, - utils::{diesel_option_overwrite, naive_now}, + utils::{diesel_string_update, diesel_url_update, naive_now}, }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::{ error::{LemmyErrorExt, LemmyErrorType, LemmyResult}, utils::{ - slurs::check_slurs_opt, + slurs::check_slurs, validation::{ check_url_scheme, - clean_url_params, is_url_blocked, is_valid_alt_text_field, is_valid_body_field, @@ -47,26 +46,43 @@ pub async fn update_post( ) -> LemmyResult> { let local_site = LocalSite::read(&mut context.pool()).await?; - // TODO No good way to handle a clear. - // Issue link: https://github.com/LemmyNet/lemmy/issues/2287 - let url = data.url.as_ref().map(clean_url_params); - let custom_thumbnail = data.custom_thumbnail.as_ref().map(clean_url_params); + let url = diesel_url_update(data.url.as_deref())?; + + let custom_thumbnail = diesel_url_update(data.custom_thumbnail.as_deref())?; let url_blocklist = get_url_blocklist(&context).await?; let slur_regex = local_site_to_slur_regex(&local_site); - check_slurs_opt(&data.name, &slur_regex)?; - let body = process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context).await?; + + let body = diesel_string_update( + process_markdown_opt(&data.body, &slur_regex, &url_blocklist, &context) + .await? + .as_deref(), + ); + + let alt_text = diesel_string_update(data.alt_text.as_deref()); if let Some(name) = &data.name { is_valid_post_title(name)?; + check_slurs(name, &slur_regex)?; } - is_valid_body_field(&body, true)?; - is_valid_alt_text_field(&data.alt_text)?; - is_url_blocked(&url, &url_blocklist)?; - check_url_scheme(&url)?; - check_url_scheme(&custom_thumbnail)?; + if let Some(Some(body)) = &body { + is_valid_body_field(body, true)?; + } + + if let Some(Some(alt_text)) = &alt_text { + is_valid_alt_text_field(alt_text)?; + } + + if let Some(Some(url)) = &url { + is_url_blocked(url, &url_blocklist)?; + check_url_scheme(url)?; + } + + if let Some(Some(custom_thumbnail)) = &custom_thumbnail { + check_url_scheme(custom_thumbnail)?; + } let post_id = data.post_id; let orig_post = Post::read(&mut context.pool(), post_id) @@ -95,9 +111,9 @@ pub async fn update_post( let post_form = PostUpdateForm { name: data.name.clone(), - url: Some(url.map(Into::into)), - body: diesel_option_overwrite(body), - alt_text: diesel_option_overwrite(data.alt_text.clone()), + url, + body, + alt_text, nsfw: data.nsfw, language_id: data.language_id, updated: Some(Some(naive_now())), @@ -111,7 +127,7 @@ pub async fn update_post( generate_post_link_metadata( updated_post.clone(), - custom_thumbnail, + custom_thumbnail.flatten().map(Into::into), |post| Some(SendActivityData::UpdatePost(post)), Some(local_site), context.reset_request_count(), diff --git a/crates/api_crud/src/private_message/create.rs b/crates/api_crud/src/private_message/create.rs index e977a6c86..0381d196c 100644 --- a/crates/api_crud/src/private_message/create.rs +++ b/crates/api_crud/src/private_message/create.rs @@ -39,7 +39,7 @@ pub async fn create_private_message( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?; - is_valid_body_field(&Some(content.clone()), false)?; + is_valid_body_field(&content, false)?; check_person_block( local_user_view.person.id, diff --git a/crates/api_crud/src/private_message/update.rs b/crates/api_crud/src/private_message/update.rs index 2842fea65..364d5c2e3 100644 --- a/crates/api_crud/src/private_message/update.rs +++ b/crates/api_crud/src/private_message/update.rs @@ -41,7 +41,7 @@ pub async fn update_private_message( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; let content = process_markdown(&data.content, &slur_regex, &url_blocklist, &context).await?; - is_valid_body_field(&Some(content.clone()), false)?; + is_valid_body_field(&content, false)?; let private_message_id = data.private_message_id; PrivateMessage::update( diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 466c7ff1d..6b1909966 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -11,7 +11,7 @@ use lemmy_api_common::{ local_site_rate_limit_to_rate_limit_config, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_opt_api, + proxy_image_link_api, }, }; use lemmy_db_schema::{ @@ -23,7 +23,7 @@ use lemmy_db_schema::{ tagline::Tagline, }, traits::Crud, - utils::{diesel_option_overwrite, naive_now}, + utils::{diesel_string_update, diesel_url_create, naive_now}, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -61,21 +61,25 @@ pub async fn create_site( let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?; - let icon = proxy_image_link_opt_api(&data.icon, &context).await?; - let banner = proxy_image_link_opt_api(&data.banner, &context).await?; + + let icon = diesel_url_create(data.icon.as_deref())?; + let icon = proxy_image_link_api(icon, &context).await?; + + let banner = diesel_url_create(data.banner.as_deref())?; + let banner = proxy_image_link_api(banner, &context).await?; let site_form = SiteUpdateForm { name: Some(data.name.clone()), - sidebar: diesel_option_overwrite(sidebar), - description: diesel_option_overwrite(data.description.clone()), - icon, - banner, + sidebar: diesel_string_update(sidebar.as_deref()), + description: diesel_string_update(data.description.as_deref()), + icon: Some(icon), + banner: Some(banner), actor_id: Some(actor_id), last_refreshed_at: Some(naive_now()), inbox_url, private_key: Some(Some(keypair.private_key)), public_key: Some(keypair.public_key), - content_warning: diesel_option_overwrite(data.content_warning.clone()), + content_warning: diesel_string_update(data.content_warning.as_deref()), ..Default::default() }; @@ -91,16 +95,16 @@ pub async fn create_site( enable_nsfw: data.enable_nsfw, community_creation_admin_only: data.community_creation_admin_only, require_email_verification: data.require_email_verification, - application_question: diesel_option_overwrite(data.application_question.clone()), + application_question: diesel_string_update(data.application_question.as_deref()), private_instance: data.private_instance, default_theme: data.default_theme.clone(), default_post_listing_type: data.default_post_listing_type, default_sort_type: data.default_sort_type, - legal_information: diesel_option_overwrite(data.legal_information.clone()), + legal_information: diesel_string_update(data.legal_information.as_deref()), application_email_admins: data.application_email_admins, hide_modlog_mod_names: data.hide_modlog_mod_names, updated: Some(Some(naive_now())), - slur_filter_regex: diesel_option_overwrite(data.slur_filter_regex.clone()), + slur_filter_regex: diesel_string_update(data.slur_filter_regex.as_deref()), actor_name_max_length: data.actor_name_max_length, federation_enabled: data.federation_enabled, captcha_enabled: data.captcha_enabled, @@ -179,7 +183,9 @@ fn validate_create_payload(local_site: &LocalSite, create_site: &CreateSite) -> )?; // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&create_site.sidebar, false)?; + if let Some(body) = &create_site.sidebar { + is_valid_body_field(body, false)?; + } application_question_check( &local_site.application_question, diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 7efa9b568..f6377038d 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -27,7 +27,7 @@ use lemmy_db_schema::{ tagline::Tagline, }, traits::Crud, - utils::{diesel_option_overwrite, naive_now}, + utils::{diesel_string_update, diesel_url_update, naive_now}, RegistrationMode, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; @@ -67,22 +67,29 @@ pub async fn update_site( SiteLanguage::update(&mut context.pool(), discussion_languages.clone(), &site).await?; } - replace_image(&data.icon, &site.icon, &context).await?; - replace_image(&data.banner, &site.banner, &context).await?; - let slur_regex = local_site_to_slur_regex(&local_site); let url_blocklist = get_url_blocklist(&context).await?; - let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?; - let icon = proxy_image_link_opt_api(&data.icon, &context).await?; - let banner = proxy_image_link_opt_api(&data.banner, &context).await?; + let sidebar = diesel_string_update( + process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context) + .await? + .as_deref(), + ); + + let icon = diesel_url_update(data.icon.as_deref())?; + replace_image(&icon, &site.icon, &context).await?; + let icon = proxy_image_link_opt_api(icon, &context).await?; + + let banner = diesel_url_update(data.banner.as_deref())?; + replace_image(&banner, &site.banner, &context).await?; + let banner = proxy_image_link_opt_api(banner, &context).await?; let site_form = SiteUpdateForm { name: data.name.clone(), - sidebar: diesel_option_overwrite(sidebar), - description: diesel_option_overwrite(data.description.clone()), + sidebar, + description: diesel_string_update(data.description.as_deref()), icon, banner, - content_warning: diesel_option_overwrite(data.content_warning.clone()), + content_warning: diesel_string_update(data.content_warning.as_deref()), updated: Some(Some(naive_now())), ..Default::default() }; @@ -99,16 +106,16 @@ pub async fn update_site( enable_nsfw: data.enable_nsfw, community_creation_admin_only: data.community_creation_admin_only, require_email_verification: data.require_email_verification, - application_question: diesel_option_overwrite(data.application_question.clone()), + application_question: diesel_string_update(data.application_question.as_deref()), private_instance: data.private_instance, default_theme: data.default_theme.clone(), default_post_listing_type: data.default_post_listing_type, default_sort_type: data.default_sort_type, - legal_information: diesel_option_overwrite(data.legal_information.clone()), + legal_information: diesel_string_update(data.legal_information.as_deref()), application_email_admins: data.application_email_admins, hide_modlog_mod_names: data.hide_modlog_mod_names, updated: Some(Some(naive_now())), - slur_filter_regex: diesel_option_overwrite(data.slur_filter_regex.clone()), + slur_filter_regex: diesel_string_update(data.slur_filter_regex.as_deref()), actor_name_max_length: data.actor_name_max_length, federation_enabled: data.federation_enabled, captcha_enabled: data.captcha_enabled, @@ -229,7 +236,9 @@ fn validate_update_payload(local_site: &LocalSite, edit_site: &EditSite) -> Lemm )?; // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&edit_site.sidebar, false)?; + if let Some(body) = &edit_site.sidebar { + is_valid_body_field(body, false)?; + } application_question_check( &local_site.application_question, diff --git a/crates/apub/src/objects/post.rs b/crates/apub/src/objects/post.rs index 59f4920d3..7e4254840 100644 --- a/crates/apub/src/objects/post.rs +++ b/crates/apub/src/objects/post.rs @@ -219,7 +219,10 @@ impl Object for ApubPost { } else { None }; - check_url_scheme(&url)?; + + if let Some(url) = &url { + check_url_scheme(url)?; + } let alt_text = first_attachment.cloned().and_then(Attachment::alt_text); diff --git a/crates/db_schema/src/utils.rs b/crates/db_schema/src/utils.rs index c6170fcd4..4267bb360 100644 --- a/crates/db_schema/src/utils.rs +++ b/crates/db_schema/src/utils.rs @@ -29,6 +29,7 @@ use i_love_jesus::CursorKey; use lemmy_utils::{ error::{LemmyErrorExt, LemmyErrorType, LemmyResult}, settings::SETTINGS, + utils::validation::clean_url_params, }; use once_cell::sync::Lazy; use regex::Regex; @@ -287,37 +288,35 @@ pub fn is_email_regex(test: &str) -> bool { EMAIL_REGEX.is_match(test) } -pub fn diesel_option_overwrite(opt: Option) -> Option> { +/// Takes an API text input, and converts it to an optional diesel DB update. +pub fn diesel_string_update(opt: Option<&str>) -> Option> { match opt { // An empty string is an erase - Some(unwrapped) => { - if !unwrapped.eq("") { - Some(Some(unwrapped)) - } else { - Some(None) - } - } + Some("") => Some(None), + Some(str) => Some(Some(str.into())), None => None, } } -pub fn diesel_option_overwrite_to_url(opt: &Option) -> LemmyResult>> { - match opt.as_ref().map(String::as_str) { +/// Takes an optional API URL-type input, and converts it to an optional diesel DB update. +/// Also cleans the url params. +pub fn diesel_url_update(opt: Option<&str>) -> LemmyResult>> { + match opt { // An empty string is an erase Some("") => Ok(Some(None)), Some(str_url) => Url::parse(str_url) - .map(|u| Some(Some(u.into()))) + .map(|u| Some(Some(clean_url_params(&u).into()))) .with_lemmy_type(LemmyErrorType::InvalidUrl), None => Ok(None), } } -pub fn diesel_option_overwrite_to_url_create(opt: &Option) -> LemmyResult> { - match opt.as_ref().map(String::as_str) { - // An empty string is nothing - Some("") => Ok(None), +/// Takes an optional API URL-type input, and converts it to an optional diesel DB create. +/// Also cleans the url params. +pub fn diesel_url_create(opt: Option<&str>) -> LemmyResult> { + match opt { Some(str_url) => Url::parse(str_url) - .map(|u| Some(u.into())) + .map(|u| Some(clean_url_params(&u).into())) .with_lemmy_type(LemmyErrorType::InvalidUrl), None => Ok(None), } @@ -569,7 +568,6 @@ impl Queries { } #[cfg(test)] -#[allow(clippy::unwrap_used)] #[allow(clippy::indexing_slicing)] mod tests { @@ -593,26 +591,24 @@ mod tests { #[test] fn test_diesel_option_overwrite() { - assert_eq!(diesel_option_overwrite(None), None); - assert_eq!(diesel_option_overwrite(Some(String::new())), Some(None)); + assert_eq!(diesel_string_update(None), None); + assert_eq!(diesel_string_update(Some("")), Some(None)); assert_eq!( - diesel_option_overwrite(Some("test".to_string())), + diesel_string_update(Some("test")), Some(Some("test".to_string())) ); } #[test] - fn test_diesel_option_overwrite_to_url() { - assert!(matches!(diesel_option_overwrite_to_url(&None), Ok(None))); - assert!(matches!( - diesel_option_overwrite_to_url(&Some(String::new())), - Ok(Some(None)) - )); - assert!(diesel_option_overwrite_to_url(&Some("invalid_url".to_string())).is_err()); + fn test_diesel_option_overwrite_to_url() -> LemmyResult<()> { + assert!(matches!(diesel_url_update(None), Ok(None))); + assert!(matches!(diesel_url_update(Some("")), Ok(Some(None)))); + assert!(diesel_url_update(Some("invalid_url")).is_err()); let example_url = "https://example.com"; assert!(matches!( - diesel_option_overwrite_to_url(&Some(example_url.to_string())), - Ok(Some(Some(url))) if url == Url::parse(example_url).unwrap().into() + diesel_url_update(Some(example_url)), + Ok(Some(Some(url))) if url == Url::parse(example_url)?.into() )); + Ok(()) } } diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs index a913e6243..c07a129b4 100644 --- a/crates/utils/src/utils/validation.rs +++ b/crates/utils/src/utils/validation.rs @@ -158,14 +158,12 @@ pub fn is_valid_post_title(title: &str) -> LemmyResult<()> { } /// This could be post bodies, comments, or any description field -pub fn is_valid_body_field(body: &Option, post: bool) -> LemmyResult<()> { - if let Some(body) = body { - if post { - max_length_check(body, POST_BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?; - } else { - max_length_check(body, BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?; - }; - } +pub fn is_valid_body_field(body: &str, post: bool) -> LemmyResult<()> { + if post { + max_length_check(body, POST_BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?; + } else { + max_length_check(body, BODY_MAX_LENGTH, LemmyErrorType::InvalidBodyField)?; + }; Ok(()) } @@ -173,16 +171,14 @@ pub fn is_valid_bio_field(bio: &str) -> LemmyResult<()> { max_length_check(bio, BIO_MAX_LENGTH, LemmyErrorType::BioLengthOverflow) } -pub fn is_valid_alt_text_field(alt_text: &Option) -> LemmyResult<()> { - if let Some(alt_text) = alt_text { - max_length_check( - alt_text, - ALT_TEXT_MAX_LENGTH, - LemmyErrorType::AltTextLengthOverflow, - ) - } else { - Ok(()) - } +pub fn is_valid_alt_text_field(alt_text: &str) -> LemmyResult<()> { + max_length_check( + alt_text, + ALT_TEXT_MAX_LENGTH, + LemmyErrorType::AltTextLengthOverflow, + )?; + + Ok(()) } /// Checks the site name length, the limit as defined in the DB. @@ -287,23 +283,17 @@ pub fn check_site_visibility_valid( } } -pub fn check_url_scheme(url: &Option) -> LemmyResult<()> { - if let Some(url) = url { - if !ALLOWED_POST_URL_SCHEMES.contains(&url.scheme()) { - Err(LemmyErrorType::InvalidUrlScheme.into()) - } else { - Ok(()) - } - } else { - Ok(()) +pub fn check_url_scheme(url: &Url) -> LemmyResult<()> { + if !ALLOWED_POST_URL_SCHEMES.contains(&url.scheme()) { + Err(LemmyErrorType::InvalidUrlScheme)? } + + Ok(()) } -pub fn is_url_blocked(url: &Option, blocklist: &RegexSet) -> LemmyResult<()> { - if let Some(url) = url { - if blocklist.is_match(url.as_str()) { - Err(LemmyErrorType::BlockedUrl)? - } +pub fn is_url_blocked(url: &Url, blocklist: &RegexSet) -> LemmyResult<()> { + if blocklist.is_match(url.as_str()) { + Err(LemmyErrorType::BlockedUrl)? } Ok(()) @@ -350,12 +340,11 @@ pub fn build_url_str_without_scheme(url_str: &str) -> LemmyResult { } #[cfg(test)] -#[allow(clippy::unwrap_used)] #[allow(clippy::indexing_slicing)] mod tests { use crate::{ - error::LemmyErrorType, + error::{LemmyErrorType, LemmyResult}, utils::validation::{ build_and_check_regex, check_site_visibility_valid, @@ -379,15 +368,17 @@ mod tests { use url::Url; #[test] - fn test_clean_url_params() { - let url = Url::parse("https://example.com/path/123?utm_content=buffercf3b2&utm_medium=social&username=randomuser&id=123").unwrap(); + fn test_clean_url_params() -> LemmyResult<()> { + let url = Url::parse("https://example.com/path/123?utm_content=buffercf3b2&utm_medium=social&username=randomuser&id=123")?; let cleaned = clean_url_params(&url); - let expected = Url::parse("https://example.com/path/123?username=randomuser&id=123").unwrap(); + let expected = Url::parse("https://example.com/path/123?username=randomuser&id=123")?; assert_eq!(expected.to_string(), cleaned.to_string()); - let url = Url::parse("https://example.com/path/123").unwrap(); + let url = Url::parse("https://example.com/path/123")?; let cleaned = clean_url_params(&url); assert_eq!(url.to_string(), cleaned.to_string()); + + Ok(()) } #[test] @@ -465,7 +456,7 @@ mod tests { } #[test] - fn test_valid_site_name() { + fn test_valid_site_name() -> LemmyResult<()> { let valid_names = [ (0..SITE_NAME_MAX_LENGTH).map(|_| 'A').collect::(), String::from("A"), @@ -496,12 +487,13 @@ mod tests { assert!(result.is_err()); assert!( - result.unwrap_err().error_type.eq(&expected_err.clone()), + result.is_err_and(|e| e.error_type.eq(&expected_err.clone())), "Testing {}, expected error {}", invalid_name, expected_err ); }); + Ok(()) } #[test] @@ -513,10 +505,7 @@ mod tests { assert!( invalid_result.is_err() - && invalid_result - .unwrap_err() - .error_type - .eq(&LemmyErrorType::BioLengthOverflow) + && invalid_result.is_err_and(|e| e.error_type.eq(&LemmyErrorType::BioLengthOverflow)) ); } @@ -537,10 +526,9 @@ mod tests { assert!( invalid_result.is_err() - && invalid_result - .unwrap_err() + && invalid_result.is_err_and(|e| e .error_type - .eq(&LemmyErrorType::SiteDescriptionLengthOverflow) + .eq(&LemmyErrorType::SiteDescriptionLengthOverflow)) ); } @@ -570,7 +558,7 @@ mod tests { assert!(result.is_err()); assert!( - result.unwrap_err().error_type.eq(&expected_err.clone()), + result.is_err_and(|e| e.error_type.eq(&expected_err.clone())), "Testing regex {:?}, expected error {}", regex_str, expected_err @@ -591,38 +579,38 @@ mod tests { } #[test] - fn test_check_url_scheme() { - assert!(check_url_scheme(&None).is_ok()); - assert!(check_url_scheme(&Some(Url::parse("http://example.com").unwrap())).is_ok()); - assert!(check_url_scheme(&Some(Url::parse("https://example.com").unwrap())).is_ok()); - assert!(check_url_scheme(&Some(Url::parse("https://example.com").unwrap())).is_ok()); - assert!(check_url_scheme(&Some(Url::parse("ftp://example.com").unwrap())).is_err()); - assert!(check_url_scheme(&Some(Url::parse("javascript:void").unwrap())).is_err()); + fn test_check_url_scheme() -> LemmyResult<()> { + assert!(check_url_scheme(&Url::parse("http://example.com")?).is_ok()); + assert!(check_url_scheme(&Url::parse("https://example.com")?).is_ok()); + assert!(check_url_scheme(&Url::parse("https://example.com")?).is_ok()); + assert!(check_url_scheme(&Url::parse("ftp://example.com")?).is_err()); + assert!(check_url_scheme(&Url::parse("javascript:void")?).is_err()); let magnet_link="magnet:?xt=urn:btih:4b390af3891e323778959d5abfff4b726510f14c&dn=Ravel%20Complete%20Piano%20Sheet%20Music%20-%20Public%20Domain&tr=udp%3A%2F%2Fopen.tracker.cl%3A1337%2Fannounce"; - assert!(check_url_scheme(&Some(Url::parse(magnet_link).unwrap())).is_ok()); + assert!(check_url_scheme(&Url::parse(magnet_link)?).is_ok()); + + Ok(()) } #[test] - fn test_url_block() { + fn test_url_block() -> LemmyResult<()> { let set = regex::RegexSet::new(vec![ r"(https://)?example\.org/page/to/article", r"(https://)?example\.net/?", r"(https://)?example\.com/?", - ]) - .unwrap(); + ])?; - assert!(is_url_blocked(&Some(Url::parse("https://example.blog").unwrap()), &set).is_ok()); + assert!(is_url_blocked(&Url::parse("https://example.blog")?, &set).is_ok()); - assert!(is_url_blocked(&Some(Url::parse("https://example.org").unwrap()), &set).is_ok()); + assert!(is_url_blocked(&Url::parse("https://example.org")?, &set).is_ok()); - assert!(is_url_blocked(&None, &set).is_ok()); + assert!(is_url_blocked(&Url::parse("https://example.com")?, &set).is_err()); - assert!(is_url_blocked(&Some(Url::parse("https://example.com").unwrap()), &set).is_err()); + Ok(()) } #[test] - fn test_url_parsed() { + fn test_url_parsed() -> LemmyResult<()> { // Make sure the scheme is removed, and uniques also assert_eq!( &check_urls_are_valid(&vec![ @@ -630,8 +618,7 @@ mod tests { "http://example.com".to_string(), "https://example.com".to_string(), "https://example.com/test?q=test2&q2=test3#test4".to_string(), - ]) - .unwrap(), + ])?, &vec![ "example.com".to_string(), "example.com/test?q=test2&q2=test3#test4".to_string() @@ -639,5 +626,6 @@ mod tests { ); assert!(check_urls_are_valid(&vec!["https://example .com".to_string()]).is_err()); + Ok(()) } }