Add option to disable strict allowlist (fixes #1486) (#1581)

* Add option to disable strict allowlist (fixes #1486)

* adjust docs

config/config.hjson

@@ -65,11 +65,13 @@
     # Allows and blocks are described here:
     # https://join.lemmy.ml/docs/en/federation/administration.html#instance-allowlist-and-blocklist
     #
-    # comma separated list of instances with which federation is allowed
-    # Only one of these blocks should be uncommented
+    # list of instances with which federation is allowed
     # allowed_instances: ["instance1.tld","instance2.tld"]
-    # comma separated list of instances which are blocked from federating
+    # instances which we never federate anything with (but previously federated objects are unaffected)
     # blocked_instances: []
+    # If true, only federate with instances on the allowlist and block everything else. If false,
+    # use allowlist only for remote communities, and posts/comments in local communities.
+    # strict_allowlist: true
   }
   captcha: {
     enabled: true

crates/apub/src/activities/send/community.rs

@@ -290,7 +290,7 @@ impl CommunityType for Community {
       .map(|i| i.into_inner())
       .unique()
       // Don't send to blocked instances
-      .filter(|inbox| check_is_apub_id_valid(inbox).is_ok())
+      .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok())
       .collect();
 
     Ok(inboxes)

crates/apub/src/activity_queue.rs

@@ -46,7 +46,7 @@ where
   Kind: Serialize,
   <T as Extends<Kind>>::Error: From<serde_json::Error> + Send + Sync + 'static,
 {
-  if check_is_apub_id_valid(&inbox).is_ok() {
+  if check_is_apub_id_valid(&inbox, false).is_ok() {
     debug!(
       "Sending activity {:?} to {}",
       &activity.id_unchecked(),
@@ -83,7 +83,7 @@ where
   .flatten()
   .unique()
   .filter(|inbox| inbox.host_str() != Some(&Settings::get().hostname()))
-  .filter(|inbox| check_is_apub_id_valid(inbox).is_ok())
+  .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok())
   .map(|inbox| inbox.to_owned())
   .collect();
   debug!(
@@ -124,7 +124,7 @@ where
       .await?;
   } else {
     let inbox = community.get_shared_inbox_or_inbox_url();
-    check_is_apub_id_valid(&inbox)?;
+    check_is_apub_id_valid(&inbox, false)?;
     debug!(
       "Sending activity {:?} to community {}",
       &activity.id_unchecked(),
@@ -160,7 +160,7 @@ where
   );
   let mentions = mentions
     .iter()
-    .filter(|inbox| check_is_apub_id_valid(inbox).is_ok())
+    .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok())
     .map(|i| i.to_owned())
     .collect();
   send_activity_internal(

crates/apub/src/fetcher/fetch.rs

@@ -59,7 +59,7 @@ where
   if *recursion_counter > MAX_REQUEST_NUMBER {
     return Err(LemmyError::from(anyhow!("Maximum recursion depth reached")).into());
   }
-  check_is_apub_id_valid(&url)?;
+  check_is_apub_id_valid(&url, false)?;
 
   let timeout = Duration::from_secs(60);
 

crates/apub/src/lib.rs

@@ -65,8 +65,7 @@ pub static APUB_JSON_CONTENT_TYPE: &str = "application/activity+json";
 /// - URL being in the allowlist (if it is active)
 /// - URL not being in the blocklist (if it is active)
 ///
-/// Note that only one of allowlist and blacklist can be enabled, not both.
-pub fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> {
+pub fn check_is_apub_id_valid(apub_id: &Url, use_strict_allowlist: bool) -> Result<(), LemmyError> {
   let settings = Settings::get();
   let domain = apub_id.domain().context(location_info!())?.to_string();
   let local_instance = settings.get_hostname_without_port()?;
@@ -95,30 +94,33 @@ pub fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> {
     return Err(anyhow!("invalid apub id scheme {}: {}", apub_id.scheme(), apub_id).into());
   }
 
-  let allowed_instances = Settings::get().get_allowed_instances();
-  let blocked_instances = Settings::get().get_blocked_instances();
-
-  if allowed_instances.is_none() && blocked_instances.is_none() {
-    Ok(())
-  } else if let Some(mut allowed) = allowed_instances {
-    // need to allow this explicitly because apub receive might contain objects from our local
-    // instance. split is needed to remove the port in our federation test setup.
-    allowed.push(local_instance);
-
-    if allowed.contains(&domain) {
-      Ok(())
-    } else {
-      Err(anyhow!("{} not in federation allowlist", domain).into())
-    }
-  } else if let Some(blocked) = blocked_instances {
+  // TODO: might be good to put the part above in one method, and below in another
+  //       (which only gets called in apub::objects)
+  //        -> no that doesnt make sense, we still need the code below for blocklist and strict allowlist
+  if let Some(blocked) = Settings::get().get_blocked_instances() {
     if blocked.contains(&domain) {
-      Err(anyhow!("{} is in federation blocklist", domain).into())
-    } else {
-      Ok(())
+      return Err(anyhow!("{} is in federation blocklist", domain).into());
     }
-  } else {
-    panic!("Invalid config, both allowed_instances and blocked_instances are specified");
   }
+
+  if let Some(mut allowed) = Settings::get().get_allowed_instances() {
+    // Only check allowlist if this is a community, or strict allowlist is enabled.
+    let strict_allowlist = Settings::get()
+      .federation()
+      .strict_allowlist
+      .unwrap_or(true);
+    if use_strict_allowlist || strict_allowlist {
+      // need to allow this explicitly because apub receive might contain objects from our local
+      // instance.
+      allowed.push(local_instance);
+
+      if !allowed.contains(&domain) {
+        return Err(anyhow!("{} not in federation allowlist", domain).into());
+      }
+    }
+  }
+
+  Ok(())
 }
 
 /// Common functions for ActivityPub objects, which are implemented by most (but not all) objects

crates/apub/src/objects/comment.rs

@@ -1,6 +1,7 @@
 use crate::{
   extensions::context::lemmy_context,
   fetcher::objects::{get_or_fetch_and_insert_comment, get_or_fetch_and_insert_post},
+  get_community_from_to_or_cc,
   objects::{
     check_object_domain,
     check_object_for_community_or_site_ban,
@@ -145,6 +146,8 @@ impl FromApubToForm<NoteExt> for CommentForm {
     request_counter: &mut i32,
     _mod_action_allowed: bool,
   ) -> Result<CommentForm, LemmyError> {
+    let community = get_community_from_to_or_cc(note, context, request_counter).await?;
+    let ap_id = Some(check_object_domain(note, expected_domain, community.local)?);
     let creator_actor_id = &note
       .attributed_to()
       .context(location_info!())?
@@ -202,7 +205,7 @@ impl FromApubToForm<NoteExt> 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_object_domain(note, expected_domain)?),
+      ap_id,
       local: Some(false),
     })
   }

crates/apub/src/objects/community.rs

@@ -203,7 +203,7 @@ impl FromApubToForm<GroupExt> for CommunityForm {
       updated: group.inner.updated().map(|u| u.to_owned().naive_local()),
       deleted: None,
       nsfw: Some(group.ext_one.sensitive.unwrap_or(false)),
-      actor_id: Some(check_object_domain(group, expected_domain)?),
+      actor_id: Some(check_object_domain(group, expected_domain, true)?),
       local: Some(false),
       private_key: None,
       public_key: Some(group.ext_two.to_owned().public_key.public_key_pem),

crates/apub/src/objects/mod.rs

@@ -98,13 +98,14 @@ where
 pub(in crate::objects) fn check_object_domain<T, Kind>(
   apub: &T,
   expected_domain: Url,
+  use_strict_allowlist: bool,
 ) -> Result<DbUrl, LemmyError>
 where
   T: Base + AsBase<Kind>,
 {
   let domain = expected_domain.domain().context(location_info!())?;
   let object_id = apub.id(domain)?.context(location_info!())?;
-  check_is_apub_id_valid(object_id)?;
+  check_is_apub_id_valid(object_id, use_strict_allowlist)?;
   Ok(object_id.to_owned().into())
 }
 

crates/apub/src/objects/person.rs

@@ -189,7 +189,7 @@ impl FromApubToForm<PersonExt> for PersonForm {
       banner: banner.map(|o| o.map(|i| i.into())),
       published: person.inner.published().map(|u| u.to_owned().naive_local()),
       updated: person.updated().map(|u| u.to_owned().naive_local()),
-      actor_id: Some(check_object_domain(person, expected_domain)?),
+      actor_id: Some(check_object_domain(person, expected_domain, false)?),
       bio: Some(bio),
       local: Some(false),
       admin: Some(false),

crates/apub/src/objects/post.rs

@@ -143,12 +143,13 @@ impl FromApubToForm<PageExt> for PostForm {
     request_counter: &mut i32,
     mod_action_allowed: bool,
   ) -> Result<PostForm, LemmyError> {
+    let community = get_community_from_to_or_cc(page, context, request_counter).await?;
     let ap_id = if mod_action_allowed {
       let id = page.id_unchecked().context(location_info!())?;
-      check_is_apub_id_valid(id)?;
+      check_is_apub_id_valid(id, community.local)?;
       id.to_owned().into()
     } else {
-      check_object_domain(page, expected_domain)?
+      check_object_domain(page, expected_domain, community.local)?
     };
     let ext = &page.ext_one;
     let creator_actor_id = page
@@ -162,8 +163,6 @@ impl FromApubToForm<PageExt> for PostForm {
     let creator =
       get_or_fetch_and_upsert_person(creator_actor_id, context, request_counter).await?;
 
-    let community = get_community_from_to_or_cc(page, context, request_counter).await?;
-
     let thumbnail_url: Option<Url> = match &page.inner.image() {
       Some(any_image) => Image::from_any_base(
         any_image

crates/apub/src/objects/private_message.rs

@@ -1,5 +1,4 @@
 use crate::{
-  check_is_apub_id_valid,
   extensions::context::lemmy_context,
   fetcher::person::get_or_fetch_and_upsert_person,
   objects::{
@@ -116,8 +115,7 @@ impl FromApubToForm<NoteExt> for PrivateMessageForm {
       .context(location_info!())?;
     let recipient =
       get_or_fetch_and_upsert_person(&recipient_actor_id, context, request_counter).await?;
-    let ap_id = note.id_unchecked().context(location_info!())?.to_string();
-    check_is_apub_id_valid(&Url::parse(&ap_id)?)?;
+    let ap_id = Some(check_object_domain(note, expected_domain, false)?);
 
     let content = get_source_markdown_value(note)?.context(location_info!())?;
 
@@ -129,7 +127,7 @@ impl FromApubToForm<NoteExt> for PrivateMessageForm {
       updated: note.updated().map(|u| u.to_owned().naive_local()),
       deleted: None,
       read: None,
-      ap_id: Some(check_object_domain(note, expected_domain)?),
+      ap_id,
       local: Some(false),
     })
   }

crates/apub_receive/src/activities/receive/private_message.rs

@@ -220,7 +220,7 @@ where
     .to_owned()
     .single_xsd_any_uri()
     .context(location_info!())?;
-  check_is_apub_id_valid(&person_id)?;
+  check_is_apub_id_valid(&person_id, false)?;
   // check that the sender is a person, not a community
   get_or_fetch_and_upsert_person(&person_id, &context, request_counter).await?;
 

crates/apub_receive/src/inbox/mod.rs

@@ -85,7 +85,7 @@ where
     .to_owned()
     .single_xsd_any_uri()
     .context(location_info!())?;
-  check_is_apub_id_valid(&actor_id)?;
+  check_is_apub_id_valid(&actor_id, false)?;
   let actor = get_or_fetch_and_upsert_actor(&actor_id, &context, request_counter).await?;
   verify_signature(&request, actor.as_ref())?;
   Ok(actor)

crates/apub_receive/src/inbox/person_inbox.rs

@@ -302,7 +302,7 @@ pub async fn receive_announce(
     .context(location_info!())?;
 
   let inner_id = inner_activity.id().context(location_info!())?.to_owned();
-  check_is_apub_id_valid(&inner_id)?;
+  check_is_apub_id_valid(&inner_id, false)?;
   if is_activity_already_known(context.pool(), &inner_id).await? {
     return Ok(());
   }

crates/utils/src/settings/defaults.rs

@@ -49,6 +49,7 @@ impl Default for FederationConfig {
       enabled: false,
       allowed_instances: None,
       blocked_instances: None,
+      strict_allowlist: Some(true),
     }
   }
 }

crates/utils/src/settings/structs.rs

@@ -77,6 +77,7 @@ pub struct FederationConfig {
   pub enabled: bool,
   pub allowed_instances: Option<Vec<String>>,
   pub blocked_instances: Option<Vec<String>>,
+  pub strict_allowlist: Option<bool>,
 }
 
 #[derive(Debug, Deserialize, Clone)]