From 8c0c1628e08f8db4f602f1b69a7f94ba60bfc2d5 Mon Sep 17 00:00:00 2001 From: Dessalines Date: Tue, 28 Feb 2023 06:34:50 -0500 Subject: [PATCH] Adding a vector indexing check to prevent panics. Fixes #2753 (#2754) * Adding a vector indexing check to prevent panics. Fixes #2753 * Fixing for new clippy lint. * Externalizing get_top_mod check. Adding get_first clippy lint * Removing unit tests get(0)s * Fixing some firsts manually --- .drone.yml | 5 ++++- crates/api/src/community/transfer.rs | 13 ++++-------- crates/api_common/src/request.rs | 11 ++++++---- crates/api_common/src/utils.rs | 17 ++++++++++++++- crates/api_crud/src/community/delete.rs | 6 ++---- .../src/collections/community_featured.rs | 5 ++++- .../apub/src/collections/community_outbox.rs | 5 ++++- crates/db_schema/src/impls/actor_language.rs | 4 ++-- crates/db_schema/src/impls/comment.rs | 21 +++++++++++-------- crates/utils/src/email.rs | 21 +++++++++---------- crates/utils/src/utils/mention.rs | 9 ++++---- scripts/fix-clippy.sh | 7 ++++++- src/api_routes_websocket.rs | 12 +++++++---- src/lib.rs | 2 +- 14 files changed, 85 insertions(+), 53 deletions(-) diff --git a/.drone.yml b/.drone.yml index 4c1eda4c..12630bd3 100644 --- a/.drone.yml +++ b/.drone.yml @@ -68,7 +68,10 @@ steps: -D clippy::manual_string_new -D clippy::redundant_closure_for_method_calls -D clippy::unused_self -A clippy::uninlined_format_args - - cargo clippy --workspace --features console -- -D clippy::unwrap_used + -D clippy::get_first + - cargo clippy --workspace --features console -- + -D clippy::unwrap_used + -D clippy::indexing_slicing - name: lemmy_api_common doesnt depend on diesel image: clux/muslrust:1.67.0 diff --git a/crates/api/src/community/transfer.rs b/crates/api/src/community/transfer.rs index 7ca174e4..e46ed351 100644 --- a/crates/api/src/community/transfer.rs +++ b/crates/api/src/community/transfer.rs @@ -4,7 +4,7 @@ use anyhow::Context; use lemmy_api_common::{ community::{GetCommunityResponse, TransferCommunity}, context::LemmyContext, - utils::get_local_user_view_from_jwt, + utils::{get_local_user_view_from_jwt, is_admin, is_top_mod}, }; use lemmy_db_schema::{ source::{ @@ -13,7 +13,7 @@ use lemmy_db_schema::{ }, traits::{Crud, Joinable}, }; -use lemmy_db_views_actor::structs::{CommunityModeratorView, CommunityView, PersonViewSafe}; +use lemmy_db_views_actor::structs::{CommunityModeratorView, CommunityView}; use lemmy_utils::{error::LemmyError, location_info, ConnectionId}; // TODO: we dont do anything for federation here, it should be updated the next time the community @@ -32,19 +32,14 @@ impl Perform for TransferCommunity { let local_user_view = get_local_user_view_from_jwt(&data.auth, context.pool(), context.secret()).await?; - let admins = PersonViewSafe::admins(context.pool()).await?; - // Fetch the community mods let community_id = data.community_id; let mut community_mods = CommunityModeratorView::for_community(context.pool(), community_id).await?; // Make sure transferrer is either the top community mod, or an admin - if local_user_view.person.id != community_mods[0].moderator.id - && !admins - .iter() - .map(|a| a.person.id) - .any(|x| x == local_user_view.person.id) + if !(is_top_mod(&local_user_view, &community_mods).is_ok() + || is_admin(&local_user_view).is_ok()) { return Err(LemmyError::from_message("not_an_admin")); } diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 5ddeea9c..260abce1 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -81,7 +81,7 @@ fn html_to_site_metadata(html_bytes: &[u8]) -> Result let og_image = page .opengraph .images - .get(0) + .first() .and_then(|ogo| Url::parse(&ogo.url).ok()); let og_embed_url = page .opengraph @@ -200,6 +200,9 @@ pub async fn fetch_site_data( // Warning, this may ignore SSL errors let metadata_option = fetch_site_metadata(client, url).await.ok(); + let missing_pictrs_file = + |r: PictrsResponse| r.files.first().expect("missing pictrs file").file.clone(); + // Fetch pictrs thumbnail let pictrs_hash = match &metadata_option { Some(metadata_res) => match &metadata_res.image { @@ -207,16 +210,16 @@ pub async fn fetch_site_data( // Try to generate a small thumbnail if there's a full sized one from post-links Some(metadata_image) => fetch_pictrs(client, settings, metadata_image) .await - .map(|r| r.files[0].file.clone()), + .map(missing_pictrs_file), // Metadata, but no image None => fetch_pictrs(client, settings, url) .await - .map(|r| r.files[0].file.clone()), + .map(missing_pictrs_file), }, // No metadata, try to fetch the URL as an image None => fetch_pictrs(client, settings, url) .await - .map(|r| r.files[0].file.clone()), + .map(missing_pictrs_file), }; // The full urls are necessary for federation diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 91902763..722aacc4 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -64,7 +64,7 @@ pub async fn is_mod_or_admin( pub async fn is_top_admin(pool: &DbPool, person_id: PersonId) -> Result<(), LemmyError> { let admins = PersonViewSafe::admins(pool).await?; let top_admin = admins - .get(0) + .first() .ok_or_else(|| LemmyError::from_message("no admins"))?; if top_admin.person.id != person_id { @@ -80,6 +80,21 @@ pub fn is_admin(local_user_view: &LocalUserView) -> Result<(), LemmyError> { Ok(()) } +pub fn is_top_mod( + local_user_view: &LocalUserView, + community_mods: &[CommunityModeratorView], +) -> Result<(), LemmyError> { + if local_user_view.person.id + != community_mods + .first() + .map(|cm| cm.moderator.id) + .unwrap_or(PersonId(0)) + { + return Err(LemmyError::from_message("not_top_mod")); + } + Ok(()) +} + #[tracing::instrument(skip_all)] pub async fn get_post(post_id: PostId, pool: &DbPool) -> Result { Post::read(pool, post_id) diff --git a/crates/api_crud/src/community/delete.rs b/crates/api_crud/src/community/delete.rs index 84de5547..4720d110 100644 --- a/crates/api_crud/src/community/delete.rs +++ b/crates/api_crud/src/community/delete.rs @@ -3,7 +3,7 @@ use actix_web::web::Data; use lemmy_api_common::{ community::{CommunityResponse, DeleteCommunity}, context::LemmyContext, - utils::get_local_user_view_from_jwt, + utils::{get_local_user_view_from_jwt, is_top_mod}, websocket::{send::send_community_ws_message, UserOperationCrud}, }; use lemmy_db_schema::{ @@ -33,9 +33,7 @@ impl PerformCrud for DeleteCommunity { CommunityModeratorView::for_community(context.pool(), community_id).await?; // Make sure deleter is the top mod - if local_user_view.person.id != community_mods[0].moderator.id { - return Err(LemmyError::from_message("no_community_edit_allowed")); - } + is_top_mod(&local_user_view, &community_mods)?; // Do the delete let community_id = data.community_id; diff --git a/crates/apub/src/collections/community_featured.rs b/crates/apub/src/collections/community_featured.rs index 7e1941ff..91abd43b 100644 --- a/crates/apub/src/collections/community_featured.rs +++ b/crates/apub/src/collections/community_featured.rs @@ -76,7 +76,10 @@ impl ApubObject for ApubCommunityFeatured { { let mut posts = apub.ordered_items; if posts.len() as i64 > FETCH_LIMIT_MAX { - posts = posts[0..(FETCH_LIMIT_MAX as usize)].to_vec(); + posts = posts + .get(0..(FETCH_LIMIT_MAX as usize)) + .unwrap_or_default() + .to_vec(); } // We intentionally ignore errors here. This is because the outbox might contain posts from old diff --git a/crates/apub/src/collections/community_outbox.rs b/crates/apub/src/collections/community_outbox.rs index 49140aab..c6d8bb44 100644 --- a/crates/apub/src/collections/community_outbox.rs +++ b/crates/apub/src/collections/community_outbox.rs @@ -101,7 +101,10 @@ impl ApubObject for ApubCommunityOutbox { ) -> Result { let mut outbox_activities = apub.ordered_items; if outbox_activities.len() as i64 > FETCH_LIMIT_MAX { - outbox_activities = outbox_activities[0..(FETCH_LIMIT_MAX as usize)].to_vec(); + outbox_activities = outbox_activities + .get(0..(FETCH_LIMIT_MAX as usize)) + .unwrap_or_default() + .to_vec(); } // We intentionally ignore errors here. This is because the outbox might contain posts from old diff --git a/crates/db_schema/src/impls/actor_language.rs b/crates/db_schema/src/impls/actor_language.rs index c0498241..d4b64061 100644 --- a/crates/db_schema/src/impls/actor_language.rs +++ b/crates/db_schema/src/impls/actor_language.rs @@ -288,8 +288,8 @@ pub async fn default_post_language( .get_results::(conn) .await?; - if intersection.len() == 1 { - Ok(Some(intersection[0])) + if let Some(i) = intersection.get(0) { + Ok(Some(*i)) } else { Ok(None) } diff --git a/crates/db_schema/src/impls/comment.rs b/crates/db_schema/src/impls/comment.rs index 21dc6c26..2dfae6dd 100644 --- a/crates/db_schema/src/impls/comment.rs +++ b/crates/db_schema/src/impls/comment.rs @@ -98,9 +98,13 @@ impl Comment { // left join comment c2 on c2.path <@ c.path and c2.path != c.path // group by c.id - let top_parent = format!("0.{}", parent_path.0.split('.').collect::>()[1]); - let update_child_count_stmt = format!( - " + let path_split = parent_path.0.split('.').collect::>(); + let parent_id = path_split.get(1); + + if let Some(parent_id) = parent_id { + let top_parent = format!("0.{}", parent_id); + let update_child_count_stmt = format!( + " update comment_aggregates ca set child_count = c.child_count from ( select c.id, c.path, count(c2.id) as child_count from comment c @@ -109,9 +113,10 @@ from ( group by c.id ) as c where ca.comment_id = c.id" - ); + ); - sql_query(update_child_count_stmt).execute(conn).await?; + sql_query(update_child_count_stmt).execute(conn).await?; + } } updated_comment } else { @@ -135,10 +140,8 @@ where ca.comment_id = c.id" let mut ltree_split: Vec<&str> = self.path.0.split('.').collect(); ltree_split.remove(0); // The first is always 0 if ltree_split.len() > 1 { - ltree_split[ltree_split.len() - 2] - .parse::() - .map(CommentId) - .ok() + let parent_comment_id = ltree_split.get(ltree_split.len() - 2); + parent_comment_id.and_then(|p| p.parse::().map(CommentId).ok()) } else { None } diff --git a/crates/utils/src/email.rs b/crates/utils/src/email.rs index 474fc38c..f036c2a6 100644 --- a/crates/utils/src/email.rs +++ b/crates/utils/src/email.rs @@ -30,18 +30,17 @@ pub fn send_email( let (smtp_server, smtp_port) = { let email_and_port = email_config.smtp_server.split(':').collect::>(); - if email_and_port.len() == 1 { - return Err(LemmyError::from_message( - "email.smtp_server needs a port, IE smtp.xxx.com:465", - )); - } + let email = *email_and_port + .first() + .ok_or_else(|| LemmyError::from_message("missing an email"))?; + let port = email_and_port + .get(1) + .ok_or_else(|| { + LemmyError::from_message("email.smtp_server needs a port, IE smtp.xxx.com:465") + })? + .parse::()?; - ( - email_and_port[0], - email_and_port[1] - .parse::() - .expect("email needs a port"), - ) + (email, port) }; // the message length before wrap, 78, is somewhat arbritary but looks good to me diff --git a/crates/utils/src/utils/mention.rs b/crates/utils/src/utils/mention.rs index d0383514..1dcace37 100644 --- a/crates/utils/src/utils/mention.rs +++ b/crates/utils/src/utils/mention.rs @@ -24,10 +24,11 @@ impl MentionData { pub fn scrape_text_for_mentions(text: &str) -> Vec { let mut out: Vec = Vec::new(); for caps in MENTIONS_REGEX.captures_iter(text) { - out.push(MentionData { - name: caps["name"].to_string(), - domain: caps["domain"].to_string(), - }); + if let Some(name) = caps.name("name").map(|c| c.as_str().to_string()) { + if let Some(domain) = caps.name("domain").map(|c| c.as_str().to_string()) { + out.push(MentionData { name, domain }); + } + } } out.into_iter().unique().collect() } diff --git a/scripts/fix-clippy.sh b/scripts/fix-clippy.sh index 6b8be28b..d8591378 100755 --- a/scripts/fix-clippy.sh +++ b/scripts/fix-clippy.sh @@ -13,6 +13,11 @@ cargo clippy --workspace --fix --allow-staged --allow-dirty --tests --all-target -D clippy::wildcard_imports -D clippy::cast_lossless \ -D clippy::manual_string_new -D clippy::redundant_closure_for_method_calls \ -D clippy::unused_self \ - -A clippy::uninlined_format_args + -A clippy::uninlined_format_args \ + -D clippy::get_first + +cargo clippy --workspace --features console -- \ + -D clippy::unwrap_used \ + -D clippy::indexing_slicing cargo +nightly fmt diff --git a/src/api_routes_websocket.rs b/src/api_routes_websocket.rs index 407bbde3..d0309806 100644 --- a/src/api_routes_websocket.rs +++ b/src/api_routes_websocket.rs @@ -248,10 +248,14 @@ async fn parse_json_message( context: LemmyContext, ) -> Result { let json: Value = serde_json::from_str(&msg)?; - let data = &json["data"].to_string(); - let op = &json["op"] - .as_str() - .ok_or_else(|| LemmyError::from_message("missing op"))?; + let data = &json + .get("data") + .ok_or_else(|| LemmyError::from_message("missing data"))? + .to_string(); + let op = &json + .get("op") + .ok_or_else(|| LemmyError::from_message("missing op"))? + .to_string(); // check if api call passes the rate limit, and generate future for later execution if let Ok(user_operation_crud) = UserOperationCrud::from_str(op) { diff --git a/src/lib.rs b/src/lib.rs index 3605f88c..ce44739c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,7 +47,7 @@ pub(crate) const REQWEST_TIMEOUT: Duration = Duration::from_secs(10); /// Placing the main function in lib.rs allows other crates to import it and embed Lemmy pub async fn start_lemmy_server() -> Result<(), LemmyError> { let args: Vec = env::args().collect(); - if args.len() == 2 && args[1] == "--print-config-docs" { + if args.get(1) == Some(&"--print-config-docs".to_string()) { let fmt = Formatting { auto_comments: AutoComments::none(), comments_style: CommentsStyle { -- 2.44.1