Adding check for requests with no id or name, adding max limit. (#2265)

* Adding check for requests with no id or name, adding max limit.

* Consolidating a few functions.

* Fix page min

* Adding more websocket rate limits.

* Add check to GetCommunity

* Use a default message rate limit check.

* Adding a page and limit checker

* Fix clippy

* Fix clippy again

* Adding check for requests with no id or name, adding max limit.

* Consolidating a few functions.

* Fix page min

* Adding more websocket rate limits.

* Add check to GetCommunity

* Use a default message rate limit check.

* Adding a page and limit checker

* Fix clippy

* Fix clippy again

* Fix limit request.

* Move checks to inside limit_and_offset

* Fixing API tests.

* Change NotFound diesel errors to QueryBuilderError

api_tests/src/shared.ts

@@ -586,7 +586,6 @@ export async function listPrivateMessages(
   let form: GetPrivateMessages = {
     auth: api.auth,
     unread_only: false,
-    limit: 999,
   };
   return api.client.getPrivateMessages(form);
 }

api_tests/src/user.spec.ts

@@ -7,7 +7,6 @@ import {
   saveUserSettings,
   getSite,
   createPost,
-  gamma,
   resolveCommunity,
   createComment,
   resolveBetaCommunity,

crates/api/src/local_user/notifications/mark_all_read.rs

@@ -34,7 +34,7 @@ impl Perform for MarkAllAsRead {
         .recipient_id(person_id)
         .unread_only(true)
         .page(1)
-        .limit(999)
+        .limit(std::i64::MAX)
         .list()
     })
     .await??;

crates/api_common/src/utils.rs

@@ -15,6 +15,7 @@ use lemmy_db_schema::{
   },
   traits::{Crud, Readable},
   utils::DbPool,
+  ListingType,
 };
 use lemmy_db_views::{
   comment_view::CommentQueryBuilder,
@@ -34,6 +35,7 @@ use lemmy_utils::{
 };
 use reqwest_middleware::ClientWithMiddleware;
 use rosetta_i18n::{Language, LanguageId};
+use std::str::FromStr;
 use tracing::warn;
 
 pub async fn blocking<F, T>(pool: &DbPool, f: F) -> Result<T, LemmyError>
@@ -716,3 +718,16 @@ pub async fn delete_user_account(
 
   Ok(())
 }
+
+pub async fn listing_type_with_site_default(
+  listing_type: Option<ListingType>,
+  pool: &DbPool,
+) -> Result<ListingType, LemmyError> {
+  Ok(match listing_type {
+    Some(l) => l,
+    None => {
+      let site = blocking(pool, Site::read_local_site).await??;
+      ListingType::from_str(&site.default_post_listing_type)?
+    }
+  })
+}

crates/api_crud/src/comment/list.rs

@@ -2,7 +2,12 @@ use crate::PerformCrud;
 use actix_web::web::Data;
 use lemmy_api_common::{
   comment::{GetComments, GetCommentsResponse},
-  utils::{blocking, check_private_instance, get_local_user_view_from_jwt_opt},
+  utils::{
+    blocking,
+    check_private_instance,
+    get_local_user_view_from_jwt_opt,
+    listing_type_with_site_default,
+  },
 };
 use lemmy_apub::{fetcher::resolve_actor_identifier, objects::community::ApubCommunity};
 use lemmy_db_schema::{source::community::Community, traits::DeleteableOrRemoveable};
@@ -33,6 +38,8 @@ impl PerformCrud for GetComments {
     let person_id = local_user_view.map(|u| u.person.id);
 
     let community_id = data.community_id;
+    let listing_type = listing_type_with_site_default(data.type_, context.pool()).await?;
+
     let community_actor_id = if let Some(name) = &data.community_name {
       resolve_actor_identifier::<ApubCommunity, Community>(name, context)
         .await
@@ -42,7 +49,6 @@ impl PerformCrud for GetComments {
       None
     };
     let sort = data.sort;
-    let listing_type = data.type_;
     let saved_only = data.saved_only;
     let page = data.page;
     let limit = data.limit;

crates/api_crud/src/community/read.rs

@@ -31,6 +31,10 @@ impl PerformCrud for GetCommunity {
       get_local_user_view_from_jwt_opt(data.auth.as_ref(), context.pool(), context.secret())
         .await?;
 
+    if data.name.is_none() && data.id.is_none() {
+      return Err(LemmyError::from_message("no_id_given"));
+    }
+
     check_private_instance(&local_user_view, context.pool()).await?;
 
     let person_id = local_user_view.map(|u| u.person.id);

crates/api_crud/src/post/list.rs

@@ -2,18 +2,18 @@ use crate::PerformCrud;
 use actix_web::web::Data;
 use lemmy_api_common::{
   post::{GetPosts, GetPostsResponse},
-  utils::{blocking, check_private_instance, get_local_user_view_from_jwt_opt},
+  utils::{
+    blocking,
+    check_private_instance,
+    get_local_user_view_from_jwt_opt,
+    listing_type_with_site_default,
+  },
 };
 use lemmy_apub::{fetcher::resolve_actor_identifier, objects::community::ApubCommunity};
-use lemmy_db_schema::{
-  source::{community::Community, site::Site},
-  traits::DeleteableOrRemoveable,
-  ListingType,
-};
+use lemmy_db_schema::{source::community::Community, traits::DeleteableOrRemoveable};
 use lemmy_db_views::post_view::PostQueryBuilder;
 use lemmy_utils::{error::LemmyError, ConnectionId};
 use lemmy_websocket::LemmyContext;
-use std::str::FromStr;
 
 #[async_trait::async_trait(?Send)]
 impl PerformCrud for GetPosts {
@@ -43,13 +43,8 @@ impl PerformCrud for GetPosts {
       .map(|t| t.local_user.show_read_posts);
 
     let sort = data.sort;
-    let listing_type: ListingType = match data.type_ {
-      Some(l) => l,
-      None => {
-        let site = blocking(context.pool(), Site::read_local_site).await??;
-        ListingType::from_str(&site.default_post_listing_type)?
-      }
-    };
+    let listing_type = listing_type_with_site_default(data.type_, context.pool()).await?;
+
     let page = data.page;
     let limit = data.limit;
     let community_id = data.community_id;

crates/api_crud/src/post/read.rs

@@ -50,7 +50,7 @@ impl PerformCrud for GetPost {
         .my_person_id(person_id)
         .show_bot_accounts(show_bot_accounts)
         .post_id(id)
-        .limit(9999)
+        .limit(std::i64::MAX)
         .list()
     })
     .await??;

crates/api_crud/src/user/read.rs

@@ -22,6 +22,12 @@ impl PerformCrud for GetPersonDetails {
     _websocket_id: Option<ConnectionId>,
   ) -> Result<GetPersonDetailsResponse, LemmyError> {
     let data: &GetPersonDetails = self;
+
+    // Check to make sure a person name or an id is given
+    if data.username.is_none() && data.person_id.is_none() {
+      return Err(LemmyError::from_message("no_id_given"));
+    }
+
     let local_user_view =
       get_local_user_view_from_jwt_opt(data.auth.as_ref(), context.pool(), context.secret())
         .await?;

crates/db_schema/src/impls/post.rs

@@ -11,7 +11,7 @@ use crate::{
     PostSavedForm,
   },
   traits::{Crud, DeleteableOrRemoveable, Likeable, Readable, Saveable},
-  utils::naive_now,
+  utils::{naive_now, FETCH_LIMIT_MAX},
 };
 use diesel::{dsl::*, result::Error, ExpressionMethods, PgConnection, QueryDsl, RunQueryDsl, *};
 use url::Url;
@@ -54,7 +54,7 @@ impl Post {
       .filter(removed.eq(false))
       .then_order_by(published.desc())
       .then_order_by(stickied.desc())
-      .limit(20)
+      .limit(FETCH_LIMIT_MAX)
       .load::<Self>(conn)
   }
 

crates/db_schema/src/utils.rs

@@ -4,6 +4,7 @@ use chrono::NaiveDateTime;
 use diesel::{
   backend::Backend,
   deserialize::FromSql,
+  result::Error::QueryBuilderError,
   serialize::{Output, ToSql},
   sql_types::Text,
   Connection,
@@ -15,6 +16,9 @@ use regex::Regex;
 use std::{env, env::VarError, io::Write};
 use url::Url;
 
+const FETCH_LIMIT_DEFAULT: i64 = 10;
+pub const FETCH_LIMIT_MAX: i64 = 50;
+
 pub type DbPool = diesel::r2d2::Pool<diesel::r2d2::ConnectionManager<diesel::PgConnection>>;
 
 pub fn get_database_url_from_env() -> Result<String, VarError> {
@@ -26,10 +30,39 @@ pub fn fuzzy_search(q: &str) -> String {
   format!("%{}%", replaced)
 }
 
-pub fn limit_and_offset(page: Option<i64>, limit: Option<i64>) -> (i64, i64) {
-  let page = page.unwrap_or(1);
-  let limit = limit.unwrap_or(10);
+pub fn limit_and_offset(
+  page: Option<i64>,
+  limit: Option<i64>,
+) -> Result<(i64, i64), diesel::result::Error> {
+  let page = match page {
+    Some(page) => {
+      if page < 1 {
+        return Err(QueryBuilderError("Page is < 1".into()));
+      } else {
+        page
+      }
+    }
+    None => 1,
+  };
+  let limit = match limit {
+    Some(limit) => {
+      if !(1..=FETCH_LIMIT_MAX).contains(&limit) {
+        return Err(QueryBuilderError(
+          format!("Fetch limit is > {}", FETCH_LIMIT_MAX).into(),
+        ));
+      } else {
+        limit
+      }
+    }
+    None => FETCH_LIMIT_DEFAULT,
+  };
   let offset = limit * (page - 1);
+  Ok((limit, offset))
+}
+
+pub fn limit_and_offset_unlimited(page: Option<i64>, limit: Option<i64>) -> (i64, i64) {
+  let limit = limit.unwrap_or(FETCH_LIMIT_DEFAULT);
+  let offset = limit * (page.unwrap_or(1) - 1);
   (limit, offset)
 }
 

crates/db_views/src/comment_report_view.rs

@@ -256,7 +256,7 @@ impl<'a> CommentReportQueryBuilder<'a> {
       query = query.filter(comment_report::resolved.eq(false));
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     query = query
       .order_by(comment_report::published.desc())

crates/db_views/src/comment_view.rs

@@ -1,5 +1,9 @@
 use crate::structs::CommentView;
-use diesel::{dsl::*, result::Error, *};
+use diesel::{
+  dsl::*,
+  result::{Error, Error::QueryBuilderError},
+  *,
+};
 use lemmy_db_schema::{
   aggregates::structs::CommentAggregates,
   newtypes::{CommentId, CommunityId, DbUrl, PersonId, PostId},
@@ -26,7 +30,7 @@ use lemmy_db_schema::{
     post::Post,
   },
   traits::{MaybeOptional, ToSafe, ViewToVec},
-  utils::{functions::hot_rank, fuzzy_search, limit_and_offset},
+  utils::{functions::hot_rank, fuzzy_search, limit_and_offset_unlimited},
   ListingType,
   SortType,
 };
@@ -414,14 +418,6 @@ impl<'a> CommentQueryBuilder<'a> {
       query = query.filter(comment::creator_id.eq(creator_id));
     };
 
-    if let Some(community_id) = self.community_id {
-      query = query.filter(post::community_id.eq(community_id));
-    }
-
-    if let Some(community_actor_id) = self.community_actor_id {
-      query = query.filter(community::actor_id.eq(community_actor_id))
-    }
-
     if let Some(post_id) = self.post_id {
       query = query.filter(comment::post_id.eq(post_id));
     };
@@ -449,9 +445,21 @@ impl<'a> CommentQueryBuilder<'a> {
               .or(community_follower::person_id.eq(person_id_join)),
           )
         }
-        ListingType::Community => {}
-      };
-    }
+        ListingType::Community => {
+          if self.community_actor_id.is_none() && self.community_id.is_none() {
+            return Err(QueryBuilderError("No community actor or id given".into()));
+          } else {
+            if let Some(community_id) = self.community_id {
+              query = query.filter(post::community_id.eq(community_id));
+            }
+
+            if let Some(community_actor_id) = self.community_actor_id {
+              query = query.filter(community::actor_id.eq(community_actor_id))
+            }
+          }
+        }
+      }
+    };
 
     if self.saved_only.unwrap_or(false) {
       query = query.filter(comment_saved::id.is_not_null());
@@ -489,7 +497,8 @@ impl<'a> CommentQueryBuilder<'a> {
       query = query.filter(person_block::person_id.is_null());
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    // Don't use the regular error-checking one, many more comments must ofter be fetched.
+    let (limit, offset) = limit_and_offset_unlimited(self.page, self.limit);
 
     // Note: deleted and removed comments are done on the front side
     let res = query

crates/db_views/src/post_report_view.rs

@@ -241,7 +241,7 @@ impl<'a> PostReportQueryBuilder<'a> {
       query = query.filter(post_report::resolved.eq(false));
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     query = query
       .order_by(post_report::published.desc())

crates/db_views/src/post_view.rs

@@ -1,5 +1,10 @@
 use crate::structs::PostView;
-use diesel::{dsl::*, pg::Pg, result::Error, *};
+use diesel::{
+  dsl::*,
+  pg::Pg,
+  result::{Error, Error::QueryBuilderError},
+  *,
+};
 use lemmy_db_schema::{
   aggregates::structs::PostAggregates,
   newtypes::{CommunityId, DbUrl, PersonId, PostId},
@@ -358,21 +363,25 @@ impl<'a> PostQueryBuilder<'a> {
           )
         }
         ListingType::Community => {
-          if let Some(community_id) = self.community_id {
-            query = query
-              .filter(post::community_id.eq(community_id))
-              .then_order_by(post_aggregates::stickied.desc());
+          if self.community_actor_id.is_none() && self.community_id.is_none() {
+            return Err(QueryBuilderError("No community actor or id given".into()));
+          } else {
+            if let Some(community_id) = self.community_id {
+              query = query
+                .filter(post::community_id.eq(community_id))
+                .then_order_by(post_aggregates::stickied.desc());
+            }
+
+            if let Some(community_actor_id) = self.community_actor_id {
+              query = query
+                .filter(community::actor_id.eq(community_actor_id))
+                .then_order_by(post_aggregates::stickied.desc());
+            }
           }
         }
       }
     }
 
-    if let Some(community_actor_id) = self.community_actor_id {
-      query = query
-        .filter(community::actor_id.eq(community_actor_id))
-        .then_order_by(post_aggregates::stickied.desc());
-    }
-
     if let Some(url_search) = self.url_search {
       query = query.filter(post::url.eq(url_search));
     }
@@ -455,7 +464,7 @@ impl<'a> PostQueryBuilder<'a> {
         .then_order_by(post_aggregates::published.desc()),
     };
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     query = query
       .limit(limit)

crates/db_views/src/private_message_view.rs

@@ -107,7 +107,7 @@ impl<'a> PrivateMessageQueryBuilder<'a> {
       )
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     query = query
       .filter(private_message::deleted.eq(false))

crates/db_views/src/registration_application_view.rs

@@ -134,7 +134,7 @@ impl<'a> RegistrationApplicationQueryBuilder<'a> {
       query = query.filter(local_user::email_verified.eq(true))
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     query = query
       .limit(limit)

crates/db_views_actor/src/community_view.rs

@@ -242,7 +242,7 @@ impl<'a> CommunityQueryBuilder<'a> {
       }
     }
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
     let res = query
       .limit(limit)
       .offset(offset)

crates/db_views_actor/src/person_mention_view.rs

@@ -311,7 +311,7 @@ impl<'a> PersonMentionQueryBuilder<'a> {
         .order_by(comment_aggregates::score.desc()),
     };
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_actor/src/person_view.rs

@@ -124,7 +124,7 @@ impl<'a> PersonQueryBuilder<'a> {
         .order_by(person_aggregates::comment_score.desc()),
     };
 
-    let (limit, offset) = limit_and_offset(self.page, self.limit);
+    let (limit, offset) = limit_and_offset(self.page, self.limit)?;
     query = query.limit(limit).offset(offset);
 
     let res = query.load::<PersonViewSafeTuple>(self.conn)?;

crates/db_views_moderator/src/admin_purge_comment_view.rs

@@ -35,7 +35,7 @@ impl AdminPurgeCommentView {
       query = query.filter(admin_purge_comment::admin_person_id.eq(admin_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/admin_purge_community_view.rs

@@ -32,7 +32,7 @@ impl AdminPurgeCommunityView {
       query = query.filter(admin_purge_community::admin_person_id.eq(admin_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/admin_purge_person_view.rs

@@ -32,7 +32,7 @@ impl AdminPurgePersonView {
       query = query.filter(admin_purge_person::admin_person_id.eq(admin_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/admin_purge_post_view.rs

@@ -35,7 +35,7 @@ impl AdminPurgePostView {
       query = query.filter(admin_purge_post::admin_person_id.eq(admin_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_add_community_view.rs

@@ -44,7 +44,7 @@ impl ModAddCommunityView {
       query = query.filter(mod_add_community::community_id.eq(community_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_add_view.rs

@@ -34,7 +34,7 @@ impl ModAddView {
       query = query.filter(mod_add::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_ban_from_community_view.rs

@@ -49,7 +49,7 @@ impl ModBanFromCommunityView {
       query = query.filter(mod_ban_from_community::community_id.eq(community_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_ban_view.rs

@@ -34,7 +34,7 @@ impl ModBanView {
       query = query.filter(mod_ban::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_hide_community_view.rs

@@ -41,7 +41,7 @@ impl ModHideCommunityView {
       query = query.filter(mod_hide_community::mod_person_id.eq(admin_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_lock_post_view.rs

@@ -43,7 +43,7 @@ impl ModLockPostView {
       query = query.filter(mod_lock_post::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_remove_comment_view.rs

@@ -55,7 +55,7 @@ impl ModRemoveCommentView {
       query = query.filter(mod_remove_comment::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_remove_community_view.rs

@@ -35,7 +35,7 @@ impl ModRemoveCommunityView {
       query = query.filter(mod_remove_community::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_remove_post_view.rs

@@ -43,7 +43,7 @@ impl ModRemovePostView {
       query = query.filter(mod_remove_post::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_sticky_post_view.rs

@@ -43,7 +43,7 @@ impl ModStickyPostView {
       query = query.filter(mod_sticky_post::mod_person_id.eq(mod_person_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/db_views_moderator/src/mod_transfer_community_view.rs

@@ -49,7 +49,7 @@ impl ModTransferCommunityView {
       query = query.filter(mod_transfer_community::community_id.eq(community_id));
     };
 
-    let (limit, offset) = limit_and_offset(page, limit);
+    let (limit, offset) = limit_and_offset(page, limit)?;
 
     let res = query
       .limit(limit)

crates/websocket/src/chat_server.rs

@@ -479,7 +479,7 @@ impl ChatServer {
           UserOperationCrud::CreatePost => rate_limiter.post().check(ip),
           UserOperationCrud::CreateCommunity => rate_limiter.register().check(ip),
           UserOperationCrud::CreateComment => rate_limiter.comment().check(ip),
-          _ => true,
+          _ => rate_limiter.message().check(ip),
         };
         let fut = (message_handler_crud)(context, msg.id, user_operation_crud, data);
         (passed, fut)
@@ -488,7 +488,7 @@ impl ChatServer {
         let passed = match user_operation {
           UserOperation::GetCaptcha => rate_limiter.post().check(ip),
           UserOperation::Search => rate_limiter.search().check(ip),
-          _ => true,
+          _ => rate_limiter.message().check(ip),
         };
         let fut = (message_handler)(context, msg.id, user_operation, data);
         (passed, fut)