Merge pull request #1201 from LemmyNet/verify-activity-domains

Add method verify_activity_domains_valid() (ref #1196)

docs/src/contributing_apub_api_outline.md

@@ -597,7 +597,7 @@ Sent to: User
     "type": "Delete",
     "actor": "https://ds9.lemmy.ml/u/sisko",
     "to": "https://enterprise.lemmy.ml/u/riker/inbox",
-    "object": "https://enterprise.lemmy.ml/private_message/341"
+    "object": "https://ds9.lemmy.ml/private_message/341"
 }
 ```
 

lemmy_apub/src/activities/receive/mod.rs

@@ -4,7 +4,8 @@ use crate::{
 };
 use activitystreams::{
   activity::{ActorAndObjectRef, ActorAndObjectRefExt},
-  base::{AsBase, Extends, ExtendsExt},
+  base::{AsBase, BaseExt, Extends, ExtendsExt},
+  error::DomainError,
   object::{AsObject, ObjectExt},
 };
 use actix_web::HttpResponse;
@@ -121,3 +122,36 @@ pub(crate) async fn find_by_id(
 
   return Err(NotFound.into());
 }
+
+pub(crate) fn verify_activity_domains_valid<T, Kind>(
+  activity: &T,
+  actor_id: Url,
+  object_domain_must_match: bool,
+) -> Result<(), LemmyError>
+where
+  T: AsBase<Kind> + ActorAndObjectRef,
+{
+  let expected_domain = actor_id.domain().context(location_info!())?;
+
+  activity.id(expected_domain)?;
+
+  let object_id = match activity.object().to_owned().single_xsd_any_uri() {
+    // object is just an ID
+    Some(id) => id,
+    // object is something like an activity, a comment or a post
+    None => activity
+      .object()
+      .to_owned()
+      .one()
+      .context(location_info!())?
+      .id()
+      .context(location_info!())?
+      .to_owned(),
+  };
+
+  if object_domain_must_match && object_id.domain() != Some(expected_domain) {
+    return Err(DomainError.into());
+  }
+
+  Ok(())
+}

lemmy_apub/src/extensions/signatures.rs

@@ -63,7 +63,7 @@ pub async fn sign_and_send(
   Ok(response)
 }
 
-pub fn verify(request: &HttpRequest, actor: &dyn ActorType) -> Result<(), LemmyError> {
+pub fn verify_signature(request: &HttpRequest, actor: &dyn ActorType) -> Result<(), LemmyError> {
   let public_key = actor.public_key().context(location_info!())?;
   let verified = CONFIG2
     .begin_verify(

lemmy_apub/src/fetcher.rs

@@ -93,7 +93,7 @@ pub async fn search_by_apub_id(
 ) -> Result<SearchResponse, LemmyError> {
   // Parse the shorthand query url
   let query_url = if query.contains('@') {
-    debug!("{}", query);
+    debug!("Search for {}", query);
     let split = query.split('@').collect::<Vec<&str>>();
 
     // User type will look like ['', username, instance]

lemmy_apub/src/inbox/community_inbox.rs

@@ -1,6 +1,7 @@
 use crate::{
+  activities::receive::verify_activity_domains_valid,
   check_is_apub_id_valid,
-  extensions::signatures::verify,
+  extensions::signatures::verify_signature,
   fetcher::get_or_fetch_and_upsert_user,
   insert_activity,
   ActorType,
@@ -48,15 +49,15 @@ pub async fn community_inbox(
   })
   .await??;
 
-  if !community.local {
+  let to = activity
-    return Err(
+    .to()
-      anyhow!(
+    .context(location_info!())?
-        "Received activity is addressed to remote community {}",
+    .to_owned()
-        &community.actor_id
+    .single_xsd_any_uri();
-      )
+  if Some(community.actor_id()?) != to {
-      .into(),
+    return Err(anyhow!("Activity delivered to wrong community").into());
-    );
   }
+
   info!(
     "Community {} received activity {:?}",
     &community.name, &activity
@@ -75,7 +76,7 @@ pub async fn community_inbox(
 
   let user = get_or_fetch_and_upsert_user(&user_uri, &context).await?;
 
-  verify(&request, &user)?;
+  verify_signature(&request, &user)?;
 
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().context(location_info!())?;
@@ -98,6 +99,8 @@ async fn handle_follow(
   context: &LemmyContext,
 ) -> Result<HttpResponse, LemmyError> {
   let follow = Follow::from_any_base(activity)?.context(location_info!())?;
+  verify_activity_domains_valid(&follow, user.actor_id()?, false)?;
+
   let community_follower_form = CommunityFollowerForm {
     community_id: community.id,
     user_id: user.id,
@@ -120,7 +123,12 @@ async fn handle_undo_follow(
   community: Community,
   context: &LemmyContext,
 ) -> Result<HttpResponse, LemmyError> {
-  let _undo = Undo::from_any_base(activity)?.context(location_info!())?;
+  let undo = Undo::from_any_base(activity)?.context(location_info!())?;
+  verify_activity_domains_valid(&undo, user.actor_id()?, true)?;
+
+  let object = undo.object().to_owned().one().context(location_info!())?;
+  let follow = Follow::from_any_base(object)?.context(location_info!())?;
+  verify_activity_domains_valid(&follow, user.actor_id()?, false)?;
 
   let community_follower_form = CommunityFollowerForm {
     community_id: community.id,

lemmy_apub/src/inbox/shared_inbox.rs

@@ -10,7 +10,7 @@ use crate::{
     update::receive_update,
   },
   check_is_apub_id_valid,
-  extensions::signatures::verify,
+  extensions::signatures::verify_signature,
   fetcher::get_or_fetch_and_upsert_actor,
   insert_activity,
 };
@@ -62,7 +62,7 @@ pub async fn shared_inbox(
   check_is_apub_id_valid(&actor)?;
 
   let actor = get_or_fetch_and_upsert_actor(&actor, &context).await?;
-  verify(&request, actor.as_ref())?;
+  verify_signature(&request, actor.as_ref())?;
 
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().context(location_info!())?;

lemmy_apub/src/inbox/user_inbox.rs

@@ -1,19 +1,20 @@
 use crate::{
+  activities::receive::verify_activity_domains_valid,
   check_is_apub_id_valid,
-  extensions::signatures::verify,
+  extensions::signatures::verify_signature,
   fetcher::{get_or_fetch_and_upsert_actor, get_or_fetch_and_upsert_community},
   insert_activity,
+  ActorType,
   FromApub,
 };
 use activitystreams::{
-  activity::{Accept, ActorAndObject, Create, Delete, Undo, Update},
+  activity::{Accept, ActorAndObject, Create, Delete, Follow, Undo, Update},
   base::AnyBase,
-  error::DomainError,
   object::Note,
   prelude::*,
 };
 use actix_web::{web, HttpRequest, HttpResponse};
-use anyhow::Context;
+use anyhow::{anyhow, Context};
 use lemmy_db::{
   community::{CommunityFollower, CommunityFollowerForm},
   private_message::{PrivateMessage, PrivateMessageForm},
@@ -50,6 +51,19 @@ pub async fn user_inbox(
 ) -> Result<HttpResponse, LemmyError> {
   let activity = input.into_inner();
   let username = path.into_inner();
+  let user = blocking(&context.pool(), move |conn| {
+    User_::read_from_name(&conn, &username)
+  })
+  .await??;
+
+  let to = activity
+    .to()
+    .context(location_info!())?
+    .to_owned()
+    .single_xsd_any_uri();
+  if Some(user.actor_id()?) != to {
+    return Err(anyhow!("Activity delivered to wrong user").into());
+  }
 
   let actor_uri = activity
     .actor()?
@@ -57,7 +71,7 @@ pub async fn user_inbox(
     .context(location_info!())?;
   debug!(
     "User {} inbox received activity {:?} from {}",
-    username,
+    user.name,
     &activity.id_unchecked(),
     &actor_uri
   );
@@ -65,16 +79,18 @@ pub async fn user_inbox(
   check_is_apub_id_valid(actor_uri)?;
 
   let actor = get_or_fetch_and_upsert_actor(actor_uri, &context).await?;
-  verify(&request, actor.as_ref())?;
+  verify_signature(&request, actor.as_ref())?;
 
   let any_base = activity.clone().into_any_base()?;
   let kind = activity.kind().context(location_info!())?;
   let res = match kind {
-    ValidTypes::Accept => receive_accept(any_base, username, &context).await,
+    ValidTypes::Accept => receive_accept(&context, any_base, actor.as_ref(), user).await,
-    ValidTypes::Create => receive_create_private_message(any_base, &context).await,
+    ValidTypes::Create => receive_create_private_message(&context, any_base, actor.as_ref()).await,
-    ValidTypes::Update => receive_update_private_message(any_base, &context).await,
+    ValidTypes::Update => receive_update_private_message(&context, any_base, actor.as_ref()).await,
-    ValidTypes::Delete => receive_delete_private_message(any_base, &context).await,
+    ValidTypes::Delete => receive_delete_private_message(&context, any_base, actor.as_ref()).await,
-    ValidTypes::Undo => receive_undo_delete_private_message(any_base, &context).await,
+    ValidTypes::Undo => {
+      receive_undo_delete_private_message(&context, any_base, actor.as_ref()).await
+    }
   };
 
   insert_activity(actor.user_id(), activity.clone(), false, context.pool()).await?;
@@ -83,11 +99,20 @@ pub async fn user_inbox(
 
 /// Handle accepted follows.
 async fn receive_accept(
-  activity: AnyBase,
-  username: String,
   context: &LemmyContext,
+  activity: AnyBase,
+  actor: &dyn ActorType,
+  user: User_,
 ) -> Result<HttpResponse, LemmyError> {
   let accept = Accept::from_any_base(activity)?.context(location_info!())?;
+  verify_activity_domains_valid(&accept, actor.actor_id()?, false)?;
+
+  // TODO: we should check that we actually sent this activity, because the remote instance
+  //       could just put a fake Follow
+  let object = accept.object().to_owned().one().context(location_info!())?;
+  let follow = Follow::from_any_base(object)?.context(location_info!())?;
+  verify_activity_domains_valid(&follow, user.actor_id()?, false)?;
+
   let community_uri = accept
     .actor()?
     .to_owned()
@@ -96,11 +121,6 @@ async fn receive_accept(
 
   let community = get_or_fetch_and_upsert_community(&community_uri, context).await?;
 
-  let user = blocking(&context.pool(), move |conn| {
-    User_::read_from_name(conn, &username)
-  })
-  .await??;
-
   // Now you need to add this to the community follower
   let community_follower_form = CommunityFollowerForm {
     community_id: community.id,
@@ -113,15 +133,17 @@ async fn receive_accept(
   })
   .await?;
 
-  // TODO: make sure that we actually requested a follow
   Ok(HttpResponse::Ok().finish())
 }
 
 async fn receive_create_private_message(
-  activity: AnyBase,
   context: &LemmyContext,
+  activity: AnyBase,
+  actor: &dyn ActorType,
 ) -> Result<HttpResponse, LemmyError> {
   let create = Create::from_any_base(activity)?.context(location_info!())?;
+  verify_activity_domains_valid(&create, actor.actor_id()?, true)?;
+
   let note = Note::from_any_base(
     create
       .object()
@@ -131,18 +153,8 @@ async fn receive_create_private_message(
   )?
   .context(location_info!())?;
 
-  let actor = create
+  let private_message =
-    .actor()?
+    PrivateMessageForm::from_apub(&note, context, Some(actor.actor_id()?)).await?;
-    .to_owned()
-    .single_xsd_any_uri()
-    .context(location_info!())?;
-  let domain = Some(
-    create
-      .id(actor.domain().context(location_info!())?)?
-      .context(location_info!())?
-      .to_owned(),
-  );
-  let private_message = PrivateMessageForm::from_apub(&note, context, domain).await?;
 
   let inserted_private_message = blocking(&context.pool(), move |conn| {
     PrivateMessage::create(conn, &private_message)
@@ -169,31 +181,22 @@ async fn receive_create_private_message(
 }
 
 async fn receive_update_private_message(
-  activity: AnyBase,
   context: &LemmyContext,
+  activity: AnyBase,
+  actor: &dyn ActorType,
 ) -> Result<HttpResponse, LemmyError> {
   let update = Update::from_any_base(activity)?.context(location_info!())?;
-  let note = Note::from_any_base(
+  verify_activity_domains_valid(&update, actor.actor_id()?, true)?;
-    update
+
+  let object = update
     .object()
     .as_one()
     .context(location_info!())?
-      .to_owned(),
+    .to_owned();
-  )?
+  let note = Note::from_any_base(object)?.context(location_info!())?;
-  .context(location_info!())?;
 
-  let actor = update
+  let private_message_form =
-    .actor()?
+    PrivateMessageForm::from_apub(&note, context, Some(actor.actor_id()?)).await?;
-    .to_owned()
-    .single_xsd_any_uri()
-    .context(location_info!())?;
-  let domain = Some(
-    update
-      .id(actor.domain().context(location_info!())?)?
-      .context(location_info!())?
-      .to_owned(),
-  );
-  let private_message_form = PrivateMessageForm::from_apub(&note, context, domain).await?;
 
   let private_message_ap_id = private_message_form
     .ap_id
@@ -232,27 +235,18 @@ async fn receive_update_private_message(
 }
 
 async fn receive_delete_private_message(
-  activity: AnyBase,
   context: &LemmyContext,
+  activity: AnyBase,
+  actor: &dyn ActorType,
 ) -> Result<HttpResponse, LemmyError> {
   let delete = Delete::from_any_base(activity)?.context(location_info!())?;
+  verify_activity_domains_valid(&delete, actor.actor_id()?, true)?;
+
   let private_message_id = delete
     .object()
     .to_owned()
     .single_xsd_any_uri()
     .context(location_info!())?;
-  let actor = delete
-    .actor()?
-    .to_owned()
-    .single_xsd_any_uri()
-    .context(location_info!())?;
-  let delete_id = delete
-    .id(actor.domain().context(location_info!())?)?
-    .map(|i| i.domain())
-    .flatten();
-  if private_message_id.domain() != delete_id {
-    return Err(DomainError.into());
-  }
   let private_message = blocking(context.pool(), move |conn| {
     PrivateMessage::read_from_apub_id(conn, private_message_id.as_str())
   })
@@ -280,30 +274,21 @@ async fn receive_delete_private_message(
 }
 
 async fn receive_undo_delete_private_message(
-  activity: AnyBase,
   context: &LemmyContext,
+  activity: AnyBase,
+  actor: &dyn ActorType,
 ) -> Result<HttpResponse, LemmyError> {
   let undo = Undo::from_any_base(activity)?.context(location_info!())?;
-  let delete = Delete::from_any_base(undo.object().as_one().context(location_info!())?.to_owned())?
+  verify_activity_domains_valid(&undo, actor.actor_id()?, true)?;
-    .context(location_info!())?;
+  let object = undo.object().to_owned().one().context(location_info!())?;
+  let delete = Delete::from_any_base(object)?.context(location_info!())?;
+  verify_activity_domains_valid(&delete, actor.actor_id()?, true)?;
+
   let private_message_id = delete
     .object()
     .to_owned()
     .single_xsd_any_uri()
     .context(location_info!())?;
-  let actor = undo
-    .actor()?
-    .to_owned()
-    .single_xsd_any_uri()
-    .context(location_info!())?;
-  let undo_id = undo
-    .id(actor.domain().context(location_info!())?)?
-    .map(|i| i.domain())
-    .flatten();
-  if private_message_id.domain() != undo_id {
-    return Err(DomainError.into());
-  }
-
   let private_message = blocking(context.pool(), move |conn| {
     PrivateMessage::read_from_apub_id(conn, private_message_id.as_str())
   })
@@ -319,9 +304,7 @@ async fn receive_undo_delete_private_message(
   .await??;
 
   let res = PrivateMessageResponse { message };
-
   let recipient_id = res.message.recipient_id;
-
   context.chat_server().do_send(SendUserRoomMessage {
     op: UserOperation::EditPrivateMessage,
     response: res,

lemmy_apub/src/lib.rs

@@ -17,10 +17,8 @@ use crate::extensions::{
 use activitystreams::{
   activity::Follow,
   actor::{ApActor, Group, Person},
-  base::{AnyBase, AsBase},
+  base::AnyBase,
-  markers::Base,
   object::{Page, Tombstone},
-  prelude::*,
 };
 use activitystreams_ext::{Ext1, Ext2};
 use anyhow::{anyhow, Context};
@@ -132,24 +130,6 @@ pub trait ApubObjectType {
   async fn send_undo_remove(&self, mod_: &User_, context: &LemmyContext) -> Result<(), LemmyError>;
 }
 
-pub(in crate) fn check_actor_domain<T, Kind>(
-  apub: &T,
-  expected_domain: Option<Url>,
-) -> Result<String, LemmyError>
-where
-  T: Base + AsBase<Kind>,
-{
-  let actor_id = if let Some(url) = expected_domain {
-    let domain = url.domain().context(location_info!())?;
-    apub.id(domain)?.context(location_info!())?
-  } else {
-    let actor_id = apub.id_unchecked().context(location_info!())?;
-    check_is_apub_id_valid(&actor_id)?;
-    actor_id
-  };
-  Ok(actor_id.to_string())
-}
-
 #[async_trait::async_trait(?Send)]
 pub trait ApubLikeableType {
   async fn send_like(&self, creator: &User_, context: &LemmyContext) -> Result<(), LemmyError>;

lemmy_apub/src/objects/comment.rs

@@ -1,11 +1,10 @@
 use crate::{
-  check_actor_domain,
   fetcher::{
     get_or_fetch_and_insert_comment,
     get_or_fetch_and_insert_post,
     get_or_fetch_and_upsert_user,
   },
-  objects::create_tombstone,
+  objects::{check_object_domain, create_tombstone},
   FromApub,
   ToApub,
 };
@@ -140,7 +139,7 @@ impl FromApub for CommentForm {
       published: note.published().map(|u| u.to_owned().naive_local()),
       updated: note.updated().map(|u| u.to_owned().naive_local()),
       deleted: None,
-      ap_id: Some(check_actor_domain(note, expected_domain)?),
+      ap_id: Some(check_object_domain(note, expected_domain)?),
       local: false,
     })
   }

lemmy_apub/src/objects/community.rs

@@ -1,8 +1,7 @@
 use crate::{
-  check_actor_domain,
   extensions::group_extensions::GroupExtension,
   fetcher::get_or_fetch_and_upsert_user,
-  objects::create_tombstone,
+  objects::{check_object_domain, create_tombstone},
   ActorType,
   FromApub,
   GroupExt,
@@ -189,7 +188,7 @@ impl FromApub for CommunityForm {
       updated: group.inner.updated().map(|u| u.to_owned().naive_local()),
       deleted: None,
       nsfw: group.ext_one.sensitive,
-      actor_id: Some(check_actor_domain(group, expected_domain)?),
+      actor_id: Some(check_object_domain(group, expected_domain)?),
       local: false,
       private_key: None,
       public_key: Some(group.ext_two.to_owned().public_key.public_key_pem),

lemmy_apub/src/objects/mod.rs

@@ -1,10 +1,13 @@
+use crate::check_is_apub_id_valid;
 use activitystreams::{
-  base::BaseExt,
+  base::{AsBase, BaseExt},
+  markers::Base,
   object::{Tombstone, TombstoneExt},
 };
-use anyhow::anyhow;
+use anyhow::{anyhow, Context};
 use chrono::NaiveDateTime;
-use lemmy_utils::{utils::convert_datetime, LemmyError};
+use lemmy_utils::{location_info, utils::convert_datetime, LemmyError};
+use url::Url;
 
 pub mod comment;
 pub mod community;
@@ -36,3 +39,22 @@ where
     Err(anyhow!("Cant convert object to tombstone if it wasnt deleted").into())
   }
 }
+
+pub(in crate::objects) fn check_object_domain<T, Kind>(
+  apub: &T,
+  expected_domain: Option<Url>,
+) -> Result<String, LemmyError>
+where
+  T: Base + AsBase<Kind>,
+{
+  let actor_id = if let Some(url) = expected_domain {
+    check_is_apub_id_valid(&url)?;
+    let domain = url.domain().context(location_info!())?;
+    apub.id(domain)?.context(location_info!())?
+  } else {
+    let actor_id = apub.id_unchecked().context(location_info!())?;
+    check_is_apub_id_valid(&actor_id)?;
+    actor_id
+  };
+  Ok(actor_id.to_string())
+}

lemmy_apub/src/objects/post.rs

@@ -1,8 +1,7 @@
 use crate::{
-  check_actor_domain,
   extensions::page_extension::PageExtension,
   fetcher::{get_or_fetch_and_upsert_community, get_or_fetch_and_upsert_user},
-  objects::create_tombstone,
+  objects::{check_object_domain, create_tombstone},
   FromApub,
   PageExt,
   ToApub,
@@ -193,7 +192,7 @@ impl FromApub for PostForm {
       embed_description: iframely_description,
       embed_html: iframely_html,
       thumbnail_url: pictrs_thumbnail,
-      ap_id: Some(check_actor_domain(page, expected_domain)?),
+      ap_id: Some(check_object_domain(page, expected_domain)?),
       local: false,
     })
   }

lemmy_apub/src/objects/private_message.rs

@@ -1,8 +1,7 @@
 use crate::{
-  check_actor_domain,
   check_is_apub_id_valid,
   fetcher::get_or_fetch_and_upsert_user,
-  objects::create_tombstone,
+  objects::{check_object_domain, create_tombstone},
   FromApub,
   ToApub,
 };
@@ -96,7 +95,7 @@ impl FromApub for PrivateMessageForm {
       updated: note.updated().map(|u| u.to_owned().naive_local()),
       deleted: None,
       read: None,
-      ap_id: Some(check_actor_domain(note, expected_domain)?),
+      ap_id: Some(check_object_domain(note, expected_domain)?),
       local: false,
     })
   }

lemmy_apub/src/objects/user.rs

@@ -1,4 +1,4 @@
-use crate::{check_actor_domain, ActorType, FromApub, PersonExt, ToApub};
+use crate::{objects::check_object_domain, ActorType, FromApub, PersonExt, ToApub};
 use activitystreams::{
   actor::{ApActor, Endpoints, Person},
   object::{Image, Tombstone},
@@ -145,7 +145,7 @@ impl FromApub for UserForm {
       show_avatars: false,
       send_notifications_to_email: false,
       matrix_user_id: None,
-      actor_id: Some(check_actor_domain(person, expected_domain)?),
+      actor_id: Some(check_object_domain(person, expected_domain)?),
       bio: Some(bio),
       local: false,
       private_key: None,