From: Felix Ableitner <me@nutomic.com>
Date: Thu, 6 Aug 2020 19:44:47 +0000 (+0200)
Subject: Various adjustments after review
X-Git-Url: http://these/git/%7B%60%24%7BwebArchiveUrl%7D/%22%7B%7D/%22https:/nerdica.net/%7Biframely.url%7D?a=commitdiff_plain;h=313f31589626823584e8d999b801c4874aa83899;p=lemmy.git

Various adjustments after review
---

diff --git a/docs/src/contributing_federation_development.md b/docs/src/contributing_federation_development.md
index 143ae9f8..8af38a07 100644
--- a/docs/src/contributing_federation_development.md
+++ b/docs/src/contributing_federation_development.md
@@ -68,3 +68,16 @@ cd /lemmy/
 sudo docker-compose pull
 sudo docker-compose up -d
 ```
+
+## Security Model
+
+- HTTP signature verify: This ensures that activity really comes from the activity that it claims
+- check_is_apub_valid : Makes sure its in our allowed instances list
+- Lower level checks: To make sure that the user that creates/updates/removes a post is actually on the same instance as that post
+
+For the last point, note that we are *not* checking whether the actor that sends the create activity for a post is
+actually identical to the post's creator, or that the user that removes a post is a mod/admin. These things are checked
+by the API code, and its the responsibility of each instance to check user permissions. This does not leave any attack
+vector, as a normal instance user cant do actions that violate the API rules. The only one who could do that is the
+admin (and the software deployed by the admin). But the admin can do anything on the instance, including send activities
+from other user accounts. So we wouldnt actually gain any security by checking mod permissions or similar.
\ No newline at end of file
diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs
index 7c5eeb2f..8124cd4a 100644
--- a/server/src/api/mod.rs
+++ b/server/src/api/mod.rs
@@ -55,7 +55,7 @@ pub trait Perform {
   ) -> Result<Self::Response, LemmyError>;
 }
 
-pub async fn is_mod_or_admin(
+pub(in crate::api) async fn is_mod_or_admin(
   pool: &DbPool,
   user_id: i32,
   community_id: i32,
@@ -65,8 +65,7 @@ pub async fn is_mod_or_admin(
   })
   .await?;
   if !is_mod_or_admin {
-    // TODO: more accurately, not_a_mod_or_admin?
-    return Err(APIError::err("not_an_admin").into());
+    return Err(APIError::err("not_a_mod_or_admin").into());
   }
   Ok(())
 }
diff --git a/server/src/apub/comment.rs b/server/src/apub/comment.rs
index 1aa3790c..fbec5905 100644
--- a/server/src/apub/comment.rs
+++ b/server/src/apub/comment.rs
@@ -1,5 +1,4 @@
 use crate::{
-  api::check_slurs,
   apub::{
     activities::{generate_activity_id, send_activity_to_community},
     check_actor_domain,
@@ -50,7 +49,7 @@ use lemmy_db::{
   user::User_,
   Crud,
 };
-use lemmy_utils::{convert_datetime, scrape_text_for_mentions, MentionData};
+use lemmy_utils::{convert_datetime, remove_slurs, scrape_text_for_mentions, MentionData};
 use log::debug;
 use serde::Deserialize;
 use serde_json::Error;
@@ -174,13 +173,13 @@ impl FromApub for CommentForm {
       .as_single_xsd_string()
       .unwrap()
       .to_string();
-    check_slurs(&content)?;
+    let content_slurs_removed = remove_slurs(&content);
 
     Ok(CommentForm {
       creator_id: creator.id,
       post_id: post.id,
       parent_id,
-      content,
+      content: content_slurs_removed,
       removed: None,
       read: None,
       published: note.published().map(|u| u.to_owned().naive_local()),
diff --git a/server/src/apub/inbox/activities/create.rs b/server/src/apub/inbox/activities/create.rs
index 6e201ff3..b45f489a 100644
--- a/server/src/apub/inbox/activities/create.rs
+++ b/server/src/apub/inbox/activities/create.rs
@@ -24,7 +24,6 @@ use crate::{
 };
 use activitystreams::{activity::Create, base::AnyBase, object::Note, prelude::*};
 use actix_web::{client::Client, HttpResponse};
-use anyhow::anyhow;
 use lemmy_db::{
   comment::{Comment, CommentForm},
   comment_view::CommentView,
@@ -63,10 +62,6 @@ async fn receive_create_post(
   let page = PageExt::from_any_base(create.object().to_owned().one().unwrap())?.unwrap();
 
   let post = PostForm::from_apub(&page, client, pool, Some(user.actor_id()?)).await?;
-  // TODO: not sure if it makes sense to check for the exact user, seeing as we already check the domain
-  if post.creator_id != user.id {
-    return Err(anyhow!("Actor for create activity and post creator need to be identical").into());
-  }
 
   let inserted_post = blocking(pool, move |conn| Post::create(conn, &post)).await??;
 
@@ -99,11 +94,6 @@ async fn receive_create_comment(
   let note = Note::from_any_base(create.object().to_owned().one().unwrap())?.unwrap();
 
   let comment = CommentForm::from_apub(&note, client, pool, Some(user.actor_id()?)).await?;
-  if comment.creator_id != user.id {
-    return Err(
-      anyhow!("Actor for create activity and comment creator need to be identical").into(),
-    );
-  }
 
   let inserted_comment = blocking(pool, move |conn| Comment::create(conn, &comment)).await??;
 
diff --git a/server/src/apub/inbox/activities/remove.rs b/server/src/apub/inbox/activities/remove.rs
index 3db019fe..b81927af 100644
--- a/server/src/apub/inbox/activities/remove.rs
+++ b/server/src/apub/inbox/activities/remove.rs
@@ -1,10 +1,5 @@
 use crate::{
-  api::{
-    comment::CommentResponse,
-    community::CommunityResponse,
-    is_mod_or_admin,
-    post::PostResponse,
-  },
+  api::{comment::CommentResponse, community::CommunityResponse, post::PostResponse},
   apub::{
     fetcher::{get_or_fetch_and_insert_comment, get_or_fetch_and_insert_post},
     inbox::shared_inbox::{
@@ -29,6 +24,7 @@ use crate::{
 };
 use activitystreams::{activity::Remove, base::AnyBase, object::Note, prelude::*};
 use actix_web::{client::Client, HttpResponse};
+use anyhow::anyhow;
 use lemmy_db::{
   comment::{Comment, CommentForm},
   comment_view::CommentView,
@@ -49,8 +45,9 @@ pub async fn receive_remove(
   let remove = Remove::from_any_base(activity)?.unwrap();
   let actor = get_user_from_activity(&remove, client, pool).await?;
   let community = get_community_from_activity(&remove, client, pool).await?;
-  // TODO: we dont federate remote admins at all, and remote mods arent federated properly
-  is_mod_or_admin(pool, actor.id, community.id).await?;
+  if actor.actor_id()?.domain() != community.actor_id()?.domain() {
+    return Err(anyhow!("Remove activities are only allowed on local objects").into());
+  }
 
   match remove.object().as_single_kind_str() {
     Some("Page") => receive_remove_post(remove, client, pool, chat_server).await,
diff --git a/server/src/apub/inbox/activities/undo.rs b/server/src/apub/inbox/activities/undo.rs
index 238b1bb0..49e70830 100644
--- a/server/src/apub/inbox/activities/undo.rs
+++ b/server/src/apub/inbox/activities/undo.rs
@@ -67,8 +67,8 @@ where
   let inner_actor = inner_activity.actor()?;
   let inner_actor_uri = inner_actor.as_single_xsd_any_uri().unwrap();
 
-  if outer_actor_uri != inner_actor_uri {
-    Err(anyhow!("An actor can only undo its own activities").into())
+  if outer_actor_uri.domain() != inner_actor_uri.domain() {
+    Err(anyhow!("Cant undo activities from a different instance").into())
   } else {
     Ok(())
   }
diff --git a/server/src/apub/inbox/activities/update.rs b/server/src/apub/inbox/activities/update.rs
index d669e5be..6f2a6d56 100644
--- a/server/src/apub/inbox/activities/update.rs
+++ b/server/src/apub/inbox/activities/update.rs
@@ -25,7 +25,6 @@ use crate::{
 };
 use activitystreams::{activity::Update, base::AnyBase, object::Note, prelude::*};
 use actix_web::{client::Client, HttpResponse};
-use anyhow::anyhow;
 use lemmy_db::{
   comment::{Comment, CommentForm},
   comment_view::CommentView,
@@ -64,16 +63,11 @@ async fn receive_update_post(
   let page = PageExt::from_any_base(update.object().to_owned().one().unwrap())?.unwrap();
 
   let post = PostForm::from_apub(&page, client, pool, Some(user.actor_id()?)).await?;
-  if post.creator_id != user.id {
-    return Err(anyhow!("Actor for update activity and post creator need to be identical").into());
-  }
 
-  let original_post = get_or_fetch_and_insert_post(&post.get_ap_id()?, client, pool).await?;
-  if post.ap_id != original_post.ap_id {
-    return Err(anyhow!("Updated post ID needs to be identical to the original ID").into());
-  }
+  let original_post_id = get_or_fetch_and_insert_post(&post.get_ap_id()?, client, pool)
+    .await?
+    .id;
 
-  let original_post_id = original_post.id;
   blocking(pool, move |conn| {
     Post::update(conn, original_post_id, &post)
   })
@@ -107,17 +101,11 @@ async fn receive_update_comment(
   let user = get_user_from_activity(&update, client, pool).await?;
 
   let comment = CommentForm::from_apub(&note, client, pool, Some(user.actor_id()?)).await?;
-  if comment.creator_id != user.id {
-    return Err(anyhow!("Actor for update activity and post creator need to be identical").into());
-  }
 
-  let original_comment =
-    get_or_fetch_and_insert_comment(&comment.get_ap_id()?, client, pool).await?;
-  if comment.ap_id != original_comment.ap_id {
-    return Err(anyhow!("Updated post ID needs to be identical to the original ID").into());
-  }
+  let original_comment_id = get_or_fetch_and_insert_comment(&comment.get_ap_id()?, client, pool)
+    .await?
+    .id;
 
-  let original_comment_id = original_comment.id;
   let updated_comment = blocking(pool, move |conn| {
     Comment::update(conn, original_comment_id, &comment)
   })
diff --git a/server/src/apub/inbox/community_inbox.rs b/server/src/apub/inbox/community_inbox.rs
index 37e7c289..8b257d47 100644
--- a/server/src/apub/inbox/community_inbox.rs
+++ b/server/src/apub/inbox/community_inbox.rs
@@ -69,14 +69,16 @@ pub async fn community_inbox(
 
   verify(&request, &user)?;
 
-  insert_activity(user.id, activity.clone(), false, &db).await?;
-
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().unwrap();
-  match kind {
-    ValidTypes::Follow => handle_follow(any_base, user, community, &client, db).await,
-    ValidTypes::Undo => handle_undo_follow(any_base, user, community, db).await,
-  }
+  let user_id = user.id;
+  let res = match kind {
+    ValidTypes::Follow => handle_follow(any_base, user, community, &client, &db).await,
+    ValidTypes::Undo => handle_undo_follow(any_base, user, community, &db).await,
+  };
+
+  insert_activity(user_id, activity.clone(), false, &db).await?;
+  res
 }
 
 /// Handle a follow request from a remote user, adding it to the local database and returning an
@@ -86,7 +88,7 @@ async fn handle_follow(
   user: User_,
   community: Community,
   client: &Client,
-  db: DbPoolParam,
+  db: &DbPoolParam,
 ) -> Result<HttpResponse, LemmyError> {
   let follow = Follow::from_any_base(activity)?.unwrap();
   let community_follower_form = CommunityFollowerForm {
@@ -95,12 +97,12 @@ async fn handle_follow(
   };
 
   // This will fail if they're already a follower, but ignore the error.
-  blocking(&db, move |conn| {
+  blocking(db, move |conn| {
     CommunityFollower::follow(&conn, &community_follower_form).ok()
   })
   .await?;
 
-  community.send_accept_follow(follow, &client, &db).await?;
+  community.send_accept_follow(follow, &client, db).await?;
 
   Ok(HttpResponse::Ok().finish())
 }
@@ -109,7 +111,7 @@ async fn handle_undo_follow(
   activity: AnyBase,
   user: User_,
   community: Community,
-  db: DbPoolParam,
+  db: &DbPoolParam,
 ) -> Result<HttpResponse, LemmyError> {
   let _undo = Undo::from_any_base(activity)?.unwrap();
 
@@ -119,7 +121,7 @@ async fn handle_undo_follow(
   };
 
   // This will fail if they aren't a follower, but ignore the error.
-  blocking(&db, move |conn| {
+  blocking(db, move |conn| {
     CommunityFollower::unfollow(&conn, &community_follower_form).ok()
   })
   .await?;
diff --git a/server/src/apub/inbox/shared_inbox.rs b/server/src/apub/inbox/shared_inbox.rs
index 5406d111..d5ed9312 100644
--- a/server/src/apub/inbox/shared_inbox.rs
+++ b/server/src/apub/inbox/shared_inbox.rs
@@ -77,12 +77,9 @@ pub async fn shared_inbox(
   check_is_apub_id_valid(sender)?;
   verify(&request, actor.as_ref())?;
 
-  // TODO: probably better to do this after, so we dont store activities that fail a check somewhere
-  insert_activity(actor.user_id(), activity.clone(), false, &pool).await?;
-
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().unwrap();
-  match kind {
+  let res = match kind {
     ValidTypes::Announce => receive_announce(any_base, &client, &pool, chat_server).await,
     ValidTypes::Create => receive_create(any_base, &client, &pool, chat_server).await,
     ValidTypes::Update => receive_update(any_base, &client, &pool, chat_server).await,
@@ -91,7 +88,10 @@ pub async fn shared_inbox(
     ValidTypes::Remove => receive_remove(any_base, &client, &pool, chat_server).await,
     ValidTypes::Delete => receive_delete(any_base, &client, &pool, chat_server).await,
     ValidTypes::Undo => receive_undo(any_base, &client, &pool, chat_server).await,
-  }
+  };
+
+  insert_activity(actor.user_id(), activity.clone(), false, &pool).await?;
+  res
 }
 
 pub(in crate::apub::inbox) fn receive_unhandled_activity<A>(
diff --git a/server/src/apub/inbox/user_inbox.rs b/server/src/apub/inbox/user_inbox.rs
index 53f1fad2..b443b51c 100644
--- a/server/src/apub/inbox/user_inbox.rs
+++ b/server/src/apub/inbox/user_inbox.rs
@@ -65,11 +65,9 @@ pub async fn user_inbox(
   let actor = get_or_fetch_and_upsert_actor(actor_uri, &client, &pool).await?;
   verify(&request, actor.as_ref())?;
 
-  insert_activity(actor.user_id(), activity.clone(), false, &pool).await?;
-
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().unwrap();
-  match kind {
+  let res = match kind {
     ValidTypes::Accept => receive_accept(any_base, username, &client, &pool).await,
     ValidTypes::Create => {
       receive_create_private_message(any_base, &client, &pool, chat_server).await
@@ -83,7 +81,10 @@ pub async fn user_inbox(
     ValidTypes::Undo => {
       receive_undo_delete_private_message(any_base, &client, &pool, chat_server).await
     }
-  }
+  };
+
+  insert_activity(actor.user_id(), activity.clone(), false, &pool).await?;
+  res
 }
 
 /// Handle accepted follows.
diff --git a/server/src/apub/post.rs b/server/src/apub/post.rs
index a2a83f45..8bd8364f 100644
--- a/server/src/apub/post.rs
+++ b/server/src/apub/post.rs
@@ -1,5 +1,5 @@
 use crate::{
-  api::{check_slurs, check_slurs_opt},
+  api::check_slurs,
   apub::{
     activities::{generate_activity_id, send_activity_to_community},
     check_actor_domain,
@@ -44,7 +44,7 @@ use lemmy_db::{
   user::User_,
   Crud,
 };
-use lemmy_utils::convert_datetime;
+use lemmy_utils::{convert_datetime, remove_slurs};
 use serde::Deserialize;
 use url::Url;
 
@@ -225,11 +225,11 @@ impl FromApub for PostForm {
       .as_ref()
       .map(|c| c.as_single_xsd_string().unwrap().to_string());
     check_slurs(&name)?;
-    check_slurs_opt(&body)?;
+    let body_slurs_removed = body.map(|b| remove_slurs(&b));
     Ok(PostForm {
       name,
       url,
-      body,
+      body: body_slurs_removed,
       creator_id: creator.id,
       community_id: community.id,
       removed: None,