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 <dessalines@users.noreply.github.com>

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<Json<BanFromCommunityResponse>, 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(

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<BanPerson>,
@@ -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(),

crates/api_common/src/community.rs

@@ -180,7 +180,6 @@ pub struct RemoveCommunity {
   pub community_id: CommunityId,
   pub removed: bool,
   pub reason: Option<String>,
-  pub expires: Option<i64>,
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone, Default)]

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<String>) -> 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<i64>) -> LemmyResult<Option<DateTime<Utc>>> {
+  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<Utc>) -> LemmyResult<Option<DateTime<Utc>>> {
+  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
+    );
+  }
 }

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?;
 

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<LemmyContext>,
 ) -> 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(

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(

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(

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,

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()),
     };

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)
   }

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)

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,
     };

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)
   }

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_,
     };
 

crates/db_schema/src/schema.rs

@@ -563,7 +563,6 @@ diesel::table! {
         community_id -> Int4,
         reason -> Nullable<Text>,
         removed -> Bool,
-        expires -> Nullable<Timestamptz>,
         when_ -> Timestamptz,
     }
 }

crates/db_schema/src/source/moderator.rs

@@ -127,7 +127,6 @@ pub struct ModRemoveCommunity {
   pub community_id: CommunityId,
   pub reason: Option<String>,
   pub removed: bool,
-  pub expires: Option<DateTime<Utc>>,
   pub when_: DateTime<Utc>,
 }
 
@@ -138,7 +137,6 @@ pub struct ModRemoveCommunityForm {
   pub community_id: CommunityId,
   pub reason: Option<String>,
   pub removed: Option<bool>,
-  pub expires: Option<DateTime<Utc>>,
 }
 
 #[skip_serializing_none]

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),
 }
 

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;

crates/utils/src/utils/time.rs

@@ -1,12 +0,0 @@
-use chrono::{DateTime, TimeZone, Utc};
-
-pub fn naive_from_unix(time: i64) -> DateTime<Utc> {
-  Utc
-    .timestamp_opt(time, 0)
-    .single()
-    .expect("convert datetime")
-}
-
-pub fn convert_datetime(datetime: DateTime<Utc>) -> DateTime<Utc> {
-  datetime
-}

migrations/2023-10-17-181800_drop_remove_community_expires/down.sql

@@ -0,0 +1,3 @@
+ALTER TABLE mod_remove_community
+    ADD COLUMN expires timestamp;
+

migrations/2023-10-17-181800_drop_remove_community_expires/up.sql

@@ -0,0 +1,3 @@
+ALTER TABLE mod_remove_community
+    DROP COLUMN expires;
+