From 12163701e718a02b949b52e7ff7bf78dc6a620ce Mon Sep 17 00:00:00 2001 From: Nutomic Date: Tue, 30 Apr 2024 12:33:37 +0200 Subject: [PATCH] Avoid crash when handling urls without domain (#4676) * Avoid crash when handling urls without domain * Add some extra checks --- crates/apub/src/activities/block/block_user.rs | 11 +++++++++-- .../src/activities/create_or_update/comment.rs | 3 ++- .../create_or_update/private_message.rs | 3 ++- crates/apub/src/lib.rs | 15 ++++++++++++--- crates/apub/src/mentions.rs | 5 ++++- crates/apub/src/objects/instance.rs | 11 +++++++++-- crates/utils/src/error.rs | 1 + 7 files changed, 39 insertions(+), 10 deletions(-) diff --git a/crates/apub/src/activities/block/block_user.rs b/crates/apub/src/activities/block/block_user.rs index f68301be1..48408c4fb 100644 --- a/crates/apub/src/activities/block/block_user.rs +++ b/crates/apub/src/activities/block/block_user.rs @@ -39,7 +39,10 @@ use lemmy_db_schema::{ }, traits::{Bannable, Crud, Followable}, }; -use lemmy_utils::error::{LemmyError, LemmyResult}; +use lemmy_utils::{ + error::{LemmyError, LemmyResult}, + LemmyErrorType, +}; use url::Url; impl BlockUser { @@ -129,7 +132,11 @@ impl ActivityHandler for BlockUser { verify_is_public(&self.to, &self.cc)?; match self.target.dereference(context).await? { SiteOrCommunity::Site(site) => { - let domain = self.object.inner().domain().expect("url needs domain"); + let domain = self + .object + .inner() + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)?; if context.settings().hostname == domain { return Err( anyhow!("Site bans from remote instance can't affect user's home instance").into(), diff --git a/crates/apub/src/activities/create_or_update/comment.rs b/crates/apub/src/activities/create_or_update/comment.rs index 7f1532087..b86e32d49 100644 --- a/crates/apub/src/activities/create_or_update/comment.rs +++ b/crates/apub/src/activities/create_or_update/comment.rs @@ -19,7 +19,7 @@ use activitypub_federation::{ config::Data, fetch::object_id::ObjectId, kinds::public, - protocol::verification::verify_domains_match, + protocol::verification::{verify_domains_match, verify_urls_match}, traits::{ActivityHandler, Actor, Object}, }; use lemmy_api_common::{ @@ -133,6 +133,7 @@ impl ActivityHandler for CreateOrUpdateNote { verify_domains_match(self.actor.inner(), self.object.id.inner())?; check_community_deleted_or_removed(&community)?; check_post_deleted_or_removed(&post)?; + verify_urls_match(self.actor.inner(), self.object.attributed_to.inner())?; ApubComment::verify(&self.object, self.actor.inner(), context).await?; Ok(()) diff --git a/crates/apub/src/activities/create_or_update/private_message.rs b/crates/apub/src/activities/create_or_update/private_message.rs index 950f4ae99..6bba4e374 100644 --- a/crates/apub/src/activities/create_or_update/private_message.rs +++ b/crates/apub/src/activities/create_or_update/private_message.rs @@ -9,7 +9,7 @@ use crate::{ }; use activitypub_federation::{ config::Data, - protocol::verification::verify_domains_match, + protocol::verification::{verify_domains_match, verify_urls_match}, traits::{ActivityHandler, Actor, Object}, }; use lemmy_api_common::context::LemmyContext; @@ -61,6 +61,7 @@ impl ActivityHandler for CreateOrUpdateChatMessage { verify_person(&self.actor, context).await?; verify_domains_match(self.actor.inner(), self.object.id.inner())?; verify_domains_match(self.to[0].inner(), self.object.to[0].inner())?; + verify_urls_match(self.actor.inner(), self.object.attributed_to.inner())?; ApubPrivateMessage::verify(&self.object, self.actor.inner(), context).await?; Ok(()) } diff --git a/crates/apub/src/lib.rs b/crates/apub/src/lib.rs index f500c41ee..a5643b95c 100644 --- a/crates/apub/src/lib.rs +++ b/crates/apub/src/lib.rs @@ -78,7 +78,10 @@ impl UrlVerifier for VerifyUrlData { /// - URL not being in the blocklist (if it is active) #[tracing::instrument(skip(local_site_data))] fn check_apub_id_valid(apub_id: &Url, local_site_data: &LocalSiteData) -> LemmyResult<()> { - let domain = apub_id.domain().expect("apud id has domain").to_string(); + let domain = apub_id + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)? + .to_string(); if !local_site_data .local_site @@ -158,7 +161,10 @@ pub(crate) async fn check_apub_id_valid_with_strictness( is_strict: bool, context: &LemmyContext, ) -> LemmyResult<()> { - let domain = apub_id.domain().expect("apud id has domain").to_string(); + let domain = apub_id + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)? + .to_string(); let local_instance = context .settings() .get_hostname_without_port() @@ -185,7 +191,10 @@ pub(crate) async fn check_apub_id_valid_with_strictness( .expect("local hostname is valid"); allowed_and_local.push(local_instance); - let domain = apub_id.domain().expect("apud id has domain").to_string(); + let domain = apub_id + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)? + .to_string(); if !allowed_and_local.contains(&domain) { Err(LemmyErrorType::FederationDisabledByStrictAllowList)? } diff --git a/crates/apub/src/mentions.rs b/crates/apub/src/mentions.rs index 4f4edc76d..de472bd8a 100644 --- a/crates/apub/src/mentions.rs +++ b/crates/apub/src/mentions.rs @@ -54,7 +54,10 @@ pub async fn collect_non_local_mentions( name: Some(format!( "@{}@{}", &parent_creator.name, - &parent_creator.id().domain().expect("has domain") + &parent_creator + .id() + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)? )), kind: MentionType::Mention, }; diff --git a/crates/apub/src/objects/instance.rs b/crates/apub/src/objects/instance.rs index 145dc63c2..ddfeb8ca2 100644 --- a/crates/apub/src/objects/instance.rs +++ b/crates/apub/src/objects/instance.rs @@ -45,6 +45,7 @@ use lemmy_utils::{ markdown::markdown_to_html, slurs::{check_slurs, check_slurs_opt}, }, + LemmyErrorType, }; use std::ops::Deref; use tracing::debug; @@ -137,7 +138,11 @@ impl Object for ApubSite { #[tracing::instrument(skip_all)] async fn from_json(apub: Self::Kind, context: &Data) -> LemmyResult { - let domain = apub.id.inner().domain().expect("group id has domain"); + let domain = apub + .id + .inner() + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)?; let instance = DbInstance::read_or_create(&mut context.pool(), domain.to_string()).await?; let local_site = LocalSite::read(&mut context.pool()).await.ok(); @@ -210,7 +215,9 @@ pub(in crate::objects) async fn fetch_instance_actor_for_object + C Err(e) => { // Failed to fetch instance actor, its probably not a lemmy instance debug!("Failed to dereference site for {}: {}", &instance_id, e); - let domain = instance_id.domain().expect("has domain"); + let domain = instance_id + .domain() + .ok_or(LemmyErrorType::UrlWithoutDomain)?; Ok( DbInstance::read_or_create(&mut context.pool(), domain.to_string()) .await? diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index a687f5136..6cfd0aaed 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -176,6 +176,7 @@ pub enum LemmyErrorType { InvalidUnixTime, InvalidBotAction, CantBlockLocalInstance, + UrlWithoutDomain, Unknown(String), }