From 6d27bfed0800c90c95fa5186c7b6bfd7b1f057d1 Mon Sep 17 00:00:00 2001 From: Nutomic Date: Tue, 17 Oct 2023 19:25:35 +0200 Subject: [PATCH] Handle invalid ban expires values (fixes #4045) (#4046) * Handle invalid ban expires values (fixes #4045) * Adding a few missing expire time checks. Fixing up time conversions. (#4051) * Adding a few missing expire time checks. Fixing up time conversions. * Increase settings export wait time. * get rid of RemoveCommunity.expires * fmt * tests --------- Co-authored-by: Dessalines --- crates/api/src/community/ban.rs | 6 +-- crates/api/src/local_user/ban_person.rs | 7 +-- crates/api_common/src/community.rs | 1 - crates/api_common/src/utils.rs | 52 ++++++++++++++++++- crates/api_crud/src/community/remove.rs | 7 +-- crates/apub/src/activities/block/mod.rs | 16 +++--- crates/apub/src/activities/deletion/delete.rs | 1 - .../src/activities/deletion/undo_delete.rs | 1 - crates/apub/src/objects/comment.rs | 6 +-- crates/apub/src/objects/community.rs | 9 ++-- crates/apub/src/objects/instance.rs | 5 +- crates/apub/src/objects/person.rs | 5 +- crates/apub/src/objects/post.rs | 5 +- crates/apub/src/objects/private_message.rs | 6 +-- crates/db_schema/src/impls/moderator.rs | 2 - crates/db_schema/src/schema.rs | 1 - crates/db_schema/src/source/moderator.rs | 2 - crates/utils/src/error.rs | 2 + crates/utils/src/utils/mod.rs | 1 - crates/utils/src/utils/time.rs | 12 ----- .../down.sql | 3 ++ .../up.sql | 3 ++ 22 files changed, 91 insertions(+), 62 deletions(-) delete mode 100644 crates/utils/src/utils/time.rs create mode 100644 migrations/2023-10-17-181800_drop_remove_community_expires/down.sql create mode 100644 migrations/2023-10-17-181800_drop_remove_community_expires/up.sql diff --git a/crates/api/src/community/ban.rs b/crates/api/src/community/ban.rs index 8e9aedbad..f662c4a08 100644 --- a/crates/api/src/community/ban.rs +++ b/crates/api/src/community/ban.rs @@ -4,7 +4,7 @@ use lemmy_api_common::{ community::{BanFromCommunity, BanFromCommunityResponse}, context::LemmyContext, send_activity::{ActivityChannel, SendActivityData}, - utils::{check_community_mod_action, remove_user_data_in_community}, + utils::{check_community_mod_action, check_expire_time, remove_user_data_in_community}, }; use lemmy_db_schema::{ source::{ @@ -22,7 +22,7 @@ use lemmy_db_views::structs::LocalUserView; use lemmy_db_views_actor::structs::PersonView; use lemmy_utils::{ error::{LemmyError, LemmyErrorExt, LemmyErrorType}, - utils::{time::naive_from_unix, validation::is_valid_body_field}, + utils::validation::is_valid_body_field, }; #[tracing::instrument(skip(context))] @@ -33,7 +33,7 @@ pub async fn ban_from_community( ) -> Result, LemmyError> { let banned_person_id = data.person_id; let remove_data = data.remove_data.unwrap_or(false); - let expires = data.expires.map(naive_from_unix); + let expires = check_expire_time(data.expires)?; // Verify that only mods or admins can ban check_community_mod_action( diff --git a/crates/api/src/local_user/ban_person.rs b/crates/api/src/local_user/ban_person.rs index 8ff203f0e..d7c47e619 100644 --- a/crates/api/src/local_user/ban_person.rs +++ b/crates/api/src/local_user/ban_person.rs @@ -4,7 +4,7 @@ use lemmy_api_common::{ context::LemmyContext, person::{BanPerson, BanPersonResponse}, send_activity::{ActivityChannel, SendActivityData}, - utils::{is_admin, remove_user_data}, + utils::{check_expire_time, is_admin, remove_user_data}, }; use lemmy_db_schema::{ source::{ @@ -18,8 +18,9 @@ use lemmy_db_views::structs::LocalUserView; use lemmy_db_views_actor::structs::PersonView; use lemmy_utils::{ error::{LemmyError, LemmyErrorExt, LemmyErrorType}, - utils::{time::naive_from_unix, validation::is_valid_body_field}, + utils::validation::is_valid_body_field, }; + #[tracing::instrument(skip(context))] pub async fn ban_from_site( data: Json, @@ -31,7 +32,7 @@ pub async fn ban_from_site( is_valid_body_field(&data.reason, false)?; - let expires = data.expires.map(naive_from_unix); + let expires = check_expire_time(data.expires)?; let person = Person::update( &mut context.pool(), diff --git a/crates/api_common/src/community.rs b/crates/api_common/src/community.rs index 7c96344b5..8e87ab750 100644 --- a/crates/api_common/src/community.rs +++ b/crates/api_common/src/community.rs @@ -180,7 +180,6 @@ pub struct RemoveCommunity { pub community_id: CommunityId, pub removed: bool, pub reason: Option, - pub expires: Option, } #[derive(Debug, Serialize, Deserialize, Clone, Default)] diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index df84adc16..b3dcd7558 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -6,6 +6,7 @@ use crate::{ }; use actix_web::cookie::{Cookie, SameSite}; use anyhow::Context; +use chrono::{DateTime, Days, Local, TimeZone, Utc}; use lemmy_db_schema::{ newtypes::{CommunityId, DbUrl, PersonId, PostId}, source::{ @@ -761,12 +762,40 @@ pub fn create_login_cookie(jwt: Sensitive) -> Cookie<'static> { cookie } +/// Ensure that ban/block expiry is in valid range. If its in past, throw error. If its more +/// than 10 years in future, convert to permanent ban. Otherwise return the same value. +pub fn check_expire_time(expires_unix_opt: Option) -> LemmyResult>> { + if let Some(expires_unix) = expires_unix_opt { + let expires = Utc + .timestamp_opt(expires_unix, 0) + .single() + .ok_or(LemmyErrorType::InvalidUnixTime)?; + + limit_expire_time(expires) + } else { + Ok(None) + } +} + +fn limit_expire_time(expires: DateTime) -> LemmyResult>> { + const MAX_BAN_TERM: Days = Days::new(10 * 365); + + if expires < Local::now() { + Err(LemmyErrorType::BanExpirationInPast)? + } else if expires > Local::now() + MAX_BAN_TERM { + Ok(None) + } else { + Ok(Some(expires)) + } +} + #[cfg(test)] mod tests { #![allow(clippy::unwrap_used)] #![allow(clippy::indexing_slicing)] - use crate::utils::{honeypot_check, password_length_check}; + use crate::utils::{honeypot_check, limit_expire_time, password_length_check}; + use chrono::{Days, Utc}; #[test] #[rustfmt::skip] @@ -784,4 +813,25 @@ mod tests { assert!(honeypot_check(&Some("1".to_string())).is_err()); assert!(honeypot_check(&Some("message".to_string())).is_err()); } + + #[test] + fn test_limit_ban_term() { + // Ban expires in past, should throw error + assert!(limit_expire_time(Utc::now() - Days::new(5)).is_err()); + + // Legitimate ban term, return same value + let fourteen_days = Utc::now() + Days::new(14); + assert_eq!( + limit_expire_time(fourteen_days).unwrap(), + Some(fourteen_days) + ); + let nine_years = Utc::now() + Days::new(365 * 9); + assert_eq!(limit_expire_time(nine_years).unwrap(), Some(nine_years)); + + // Too long ban term, changes to None (permanent ban) + assert_eq!( + limit_expire_time(Utc::now() + Days::new(365 * 11)).unwrap(), + None + ); + } } diff --git a/crates/api_crud/src/community/remove.rs b/crates/api_crud/src/community/remove.rs index 9604b0432..3c21c02b2 100644 --- a/crates/api_crud/src/community/remove.rs +++ b/crates/api_crud/src/community/remove.rs @@ -15,10 +15,7 @@ use lemmy_db_schema::{ traits::Crud, }; use lemmy_db_views::structs::LocalUserView; -use lemmy_utils::{ - error::{LemmyError, LemmyErrorExt, LemmyErrorType}, - utils::time::naive_from_unix, -}; +use lemmy_utils::error::{LemmyError, LemmyErrorExt, LemmyErrorType}; #[tracing::instrument(skip(context))] pub async fn remove_community( @@ -52,13 +49,11 @@ pub async fn remove_community( .with_lemmy_type(LemmyErrorType::CouldntUpdateCommunity)?; // Mod tables - let expires = data.expires.map(naive_from_unix); let form = ModRemoveCommunityForm { mod_person_id: local_user_view.person.id, community_id: data.community_id, removed: Some(removed), reason: data.reason.clone(), - expires, }; ModRemoveCommunity::create(&mut context.pool(), &form).await?; diff --git a/crates/apub/src/activities/block/mod.rs b/crates/apub/src/activities/block/mod.rs index 0d64aacd4..c6bef9a00 100644 --- a/crates/apub/src/activities/block/mod.rs +++ b/crates/apub/src/activities/block/mod.rs @@ -11,7 +11,12 @@ use activitypub_federation::{ traits::{Actor, Object}, }; use chrono::{DateTime, Utc}; -use lemmy_api_common::{community::BanFromCommunity, context::LemmyContext, person::BanPerson}; +use lemmy_api_common::{ + community::BanFromCommunity, + context::LemmyContext, + person::BanPerson, + utils::check_expire_time, +}; use lemmy_db_schema::{ newtypes::CommunityId, source::{community::Community, person::Person, site::Site}, @@ -19,10 +24,7 @@ use lemmy_db_schema::{ utils::DbPool, }; use lemmy_db_views::structs::SiteView; -use lemmy_utils::{ - error::{LemmyError, LemmyResult}, - utils::time::naive_from_unix, -}; +use lemmy_utils::error::{LemmyError, LemmyResult}; use serde::Deserialize; use url::Url; @@ -137,7 +139,7 @@ pub(crate) async fn send_ban_from_site( context: Data, ) -> Result<(), LemmyError> { let site = SiteOrCommunity::Site(SiteView::read_local(&mut context.pool()).await?.site.into()); - let expires = data.expires.map(naive_from_unix); + let expires = check_expire_time(data.expires)?; // if the action affects a local user, federate to other instances if banned_user.local { @@ -177,7 +179,7 @@ pub(crate) async fn send_ban_from_community( let community: ApubCommunity = Community::read(&mut context.pool(), community_id) .await? .into(); - let expires = data.expires.map(naive_from_unix); + let expires = check_expire_time(data.expires)?; if data.ban { BlockUser::send( diff --git a/crates/apub/src/activities/deletion/delete.rs b/crates/apub/src/activities/deletion/delete.rs index 28c4eace7..140c98665 100644 --- a/crates/apub/src/activities/deletion/delete.rs +++ b/crates/apub/src/activities/deletion/delete.rs @@ -115,7 +115,6 @@ pub(in crate::activities) async fn receive_remove_action( community_id: community.id, removed: Some(true), reason, - expires: None, }; ModRemoveCommunity::create(&mut context.pool(), &form).await?; Community::update( diff --git a/crates/apub/src/activities/deletion/undo_delete.rs b/crates/apub/src/activities/deletion/undo_delete.rs index 6572938dd..697153fc2 100644 --- a/crates/apub/src/activities/deletion/undo_delete.rs +++ b/crates/apub/src/activities/deletion/undo_delete.rs @@ -107,7 +107,6 @@ impl UndoDelete { community_id: community.id, removed: Some(false), reason: None, - expires: None, }; ModRemoveCommunity::create(&mut context.pool(), &form).await?; Community::update( diff --git a/crates/apub/src/objects/comment.rs b/crates/apub/src/objects/comment.rs index 4d57b50ee..ecee70724 100644 --- a/crates/apub/src/objects/comment.rs +++ b/crates/apub/src/objects/comment.rs @@ -29,7 +29,7 @@ use lemmy_db_schema::{ }; use lemmy_utils::{ error::{LemmyError, LemmyErrorType}, - utils::{markdown::markdown_to_html, slurs::remove_slurs, time::convert_datetime}, + utils::{markdown::markdown_to_html, slurs::remove_slurs}, }; use std::ops::Deref; use url::Url; @@ -113,8 +113,8 @@ impl Object for ApubComment { media_type: Some(MediaTypeMarkdownOrHtml::Html), source: Some(Source::new(self.content.clone())), in_reply_to, - published: Some(convert_datetime(self.published)), - updated: self.updated.map(convert_datetime), + published: Some(self.published), + updated: self.updated, tag: maa.tags, distinguished: Some(self.distinguished), language, diff --git a/crates/apub/src/objects/community.rs b/crates/apub/src/objects/community.rs index d88f457bd..69d6231c0 100644 --- a/crates/apub/src/objects/community.rs +++ b/crates/apub/src/objects/community.rs @@ -28,10 +28,7 @@ use lemmy_db_schema::{ traits::{ApubActor, Crud}, }; use lemmy_db_views_actor::structs::CommunityFollowerView; -use lemmy_utils::{ - error::LemmyError, - utils::{markdown::markdown_to_html, time::convert_datetime}, -}; +use lemmy_utils::{error::LemmyError, utils::markdown::markdown_to_html}; use std::ops::Deref; use tracing::debug; use url::Url; @@ -109,8 +106,8 @@ impl Object for ApubCommunity { }), public_key: self.public_key(), language, - published: Some(convert_datetime(self.published)), - updated: self.updated.map(convert_datetime), + published: Some(self.published), + updated: self.updated, posting_restricted_to_mods: Some(self.posting_restricted_to_mods), attributed_to: Some(generate_moderators_url(&self.actor_id)?.into()), }; diff --git a/crates/apub/src/objects/instance.rs b/crates/apub/src/objects/instance.rs index d6086fdc2..3044d77f2 100644 --- a/crates/apub/src/objects/instance.rs +++ b/crates/apub/src/objects/instance.rs @@ -34,7 +34,6 @@ use lemmy_utils::{ utils::{ markdown::markdown_to_html, slurs::{check_slurs, check_slurs_opt}, - time::convert_datetime, }, }; use std::ops::Deref; @@ -103,8 +102,8 @@ impl Object for ApubSite { outbox: Url::parse(&format!("{}/site_outbox", self.actor_id))?, public_key: self.public_key(), language, - published: convert_datetime(self.published), - updated: self.updated.map(convert_datetime), + published: self.published, + updated: self.updated, }; Ok(instance) } diff --git a/crates/apub/src/objects/person.rs b/crates/apub/src/objects/person.rs index 3ca473616..1102567d0 100644 --- a/crates/apub/src/objects/person.rs +++ b/crates/apub/src/objects/person.rs @@ -35,7 +35,6 @@ use lemmy_utils::{ utils::{ markdown::markdown_to_html, slurs::{check_slurs, check_slurs_opt}, - time::convert_datetime, }, }; use std::ops::Deref; @@ -107,13 +106,13 @@ impl Object for ApubPerson { icon: self.avatar.clone().map(ImageObject::new), image: self.banner.clone().map(ImageObject::new), matrix_user_id: self.matrix_user_id.clone(), - published: Some(convert_datetime(self.published)), + published: Some(self.published), outbox: generate_outbox_url(&self.actor_id)?.into(), endpoints: self.shared_inbox_url.clone().map(|s| Endpoints { shared_inbox: s.into(), }), public_key: self.public_key(), - updated: self.updated.map(convert_datetime), + updated: self.updated, inbox: self.inbox_url.clone().into(), }; Ok(person) diff --git a/crates/apub/src/objects/post.rs b/crates/apub/src/objects/post.rs index 6aba17554..a86d4342f 100644 --- a/crates/apub/src/objects/post.rs +++ b/crates/apub/src/objects/post.rs @@ -43,7 +43,6 @@ use lemmy_utils::{ utils::{ markdown::markdown_to_html, slurs::{check_slurs_opt, remove_slurs}, - time::convert_datetime, validation::check_url_scheme, }, }; @@ -127,8 +126,8 @@ impl Object for ApubPost { comments_enabled: Some(!self.locked), sensitive: Some(self.nsfw), language, - published: Some(convert_datetime(self.published)), - updated: self.updated.map(convert_datetime), + published: Some(self.published), + updated: self.updated, audience: Some(community.actor_id.into()), in_reply_to: None, }; diff --git a/crates/apub/src/objects/private_message.rs b/crates/apub/src/objects/private_message.rs index f683a989f..be60cc4fa 100644 --- a/crates/apub/src/objects/private_message.rs +++ b/crates/apub/src/objects/private_message.rs @@ -22,7 +22,7 @@ use lemmy_db_schema::{ }; use lemmy_utils::{ error::{LemmyError, LemmyErrorType}, - utils::{markdown::markdown_to_html, time::convert_datetime}, + utils::markdown::markdown_to_html, }; use std::ops::Deref; use url::Url; @@ -86,8 +86,8 @@ impl Object for ApubPrivateMessage { content: markdown_to_html(&self.content), media_type: Some(MediaTypeHtml::Html), source: Some(Source::new(self.content.clone())), - published: Some(convert_datetime(self.published)), - updated: self.updated.map(convert_datetime), + published: Some(self.published), + updated: self.updated, }; Ok(note) } diff --git a/crates/db_schema/src/impls/moderator.rs b/crates/db_schema/src/impls/moderator.rs index a4c300b2a..012e05394 100644 --- a/crates/db_schema/src/impls/moderator.rs +++ b/crates/db_schema/src/impls/moderator.rs @@ -651,7 +651,6 @@ mod tests { community_id: inserted_community.id, reason: None, removed: None, - expires: None, }; let inserted_mod_remove_community = ModRemoveCommunity::create(pool, &mod_remove_community_form) @@ -667,7 +666,6 @@ mod tests { mod_person_id: inserted_mod.id, reason: None, removed: true, - expires: None, when_: inserted_mod_remove_community.when_, }; diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index 2d6f221fc..440cb09fa 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -563,7 +563,6 @@ diesel::table! { community_id -> Int4, reason -> Nullable, removed -> Bool, - expires -> Nullable, when_ -> Timestamptz, } } diff --git a/crates/db_schema/src/source/moderator.rs b/crates/db_schema/src/source/moderator.rs index 7e2ff2867..181bdbab7 100644 --- a/crates/db_schema/src/source/moderator.rs +++ b/crates/db_schema/src/source/moderator.rs @@ -127,7 +127,6 @@ pub struct ModRemoveCommunity { pub community_id: CommunityId, pub reason: Option, pub removed: bool, - pub expires: Option>, pub when_: DateTime, } @@ -138,7 +137,6 @@ pub struct ModRemoveCommunityForm { pub community_id: CommunityId, pub reason: Option, pub removed: Option, - pub expires: Option>, } #[skip_serializing_none] diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index 3b5da1bc9..714fdfe56 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -221,6 +221,8 @@ pub enum LemmyErrorType { /// Thrown when an API call is submitted with more than 1000 array elements, see [[MAX_API_PARAM_ELEMENTS]] TooManyItems, CommunityHasNoFollowers, + BanExpirationInPast, + InvalidUnixTime, Unknown(String), } diff --git a/crates/utils/src/utils/mod.rs b/crates/utils/src/utils/mod.rs index 04be57d34..e9ba7f84d 100644 --- a/crates/utils/src/utils/mod.rs +++ b/crates/utils/src/utils/mod.rs @@ -1,5 +1,4 @@ pub mod markdown; pub mod mention; pub mod slurs; -pub mod time; pub mod validation; diff --git a/crates/utils/src/utils/time.rs b/crates/utils/src/utils/time.rs deleted file mode 100644 index 1f7ebe145..000000000 --- a/crates/utils/src/utils/time.rs +++ /dev/null @@ -1,12 +0,0 @@ -use chrono::{DateTime, TimeZone, Utc}; - -pub fn naive_from_unix(time: i64) -> DateTime { - Utc - .timestamp_opt(time, 0) - .single() - .expect("convert datetime") -} - -pub fn convert_datetime(datetime: DateTime) -> DateTime { - datetime -} diff --git a/migrations/2023-10-17-181800_drop_remove_community_expires/down.sql b/migrations/2023-10-17-181800_drop_remove_community_expires/down.sql new file mode 100644 index 000000000..048c3d7f1 --- /dev/null +++ b/migrations/2023-10-17-181800_drop_remove_community_expires/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE mod_remove_community + ADD COLUMN expires timestamp; + diff --git a/migrations/2023-10-17-181800_drop_remove_community_expires/up.sql b/migrations/2023-10-17-181800_drop_remove_community_expires/up.sql new file mode 100644 index 000000000..a94453096 --- /dev/null +++ b/migrations/2023-10-17-181800_drop_remove_community_expires/up.sql @@ -0,0 +1,3 @@ +ALTER TABLE mod_remove_community + DROP COLUMN expires; +