fix: Verify policy server signatures on all events, not just timeline ones

style: Clarifications

style: Clippy

src/service/rooms/event_handler/policy_server.rs

@@ -6,18 +6,18 @@
 use std::{collections::BTreeMap, time::Duration};
 
 use conduwuit::{
-	Err, Error, Event, PduEvent, Result, debug, debug_error, debug_info, debug_warn, error,
-	implement, info, state_res::EventTypeExt, trace, utils::to_canonical_object, warn,
+	debug, debug_error, debug_info, debug_warn, error, implement, info, state_res::EventTypeExt, trace, utils::to_canonical_object,
+	warn, Err, Error, Event, PduEvent, Result,
 };
 use http::StatusCode;
 use ruma::{
-	CanonicalJsonObject, CanonicalJsonValue, KeyId, RoomId, ServerName, SigningKeyAlgorithm,
-	api::error::ErrorKind,
-	canonical_json::redact,
-	events::{StateEventType, room::policy::RoomPolicyEventContent},
-	room_version_rules::{RedactionRules, RoomVersionRules},
-	serde::{Base64, base64::Standard},
-	signatures::{to_canonical_json_string_for_signing, verify_canonical_json_bytes},
+	api::error::ErrorKind, canonical_json::redact, events::{room::policy::RoomPolicyEventContent, StateEventType}, room_version_rules::{RedactionRules, RoomVersionRules}, serde::{base64::Standard, Base64}, signatures::{to_canonical_json_string_for_signing, verify_canonical_json_bytes},
+	CanonicalJsonObject,
+	CanonicalJsonValue,
+	KeyId,
+	RoomId,
+	ServerName,
+	SigningKeyAlgorithm,
 };
 use ruminuwuity::policy::policy_sign::unstable::Request as PolicySignRequest;
 use serde_json::value::RawValue;
@@ -72,15 +72,13 @@ pub(super) fn verify_policy_signature(
 
 /// Asks a remote policy server if the event is allowed.
 ///
-/// If the event is the `org.matrix.msc4284.policy` configuration state event,
+/// If the event is the `m.room.policy` configuration state event,
 /// this check is skipped. Similarly, if there is no policy server configured in
 /// the PDU's room, or the configured server is not present in the room, the
 /// check is also skipped.
 ///
-/// If the policy server marks the event as spam, Ok(false) is returned,
-/// otherwise Ok(true) allows the event. If the policy server cannot be
-/// contacted for whatever reason, Err(e) is returned, which generally is a
-/// fail-open operation.
+/// If the policy server marks the event as spam, the relevant error is returned. Otherwise,
+/// the incoming PDU JSON is mutated to include the new policy server signature.
 #[implement(super::Service)]
 #[tracing::instrument(skip(self, pdu, pdu_json, room_version_rules), level = "info")]
 pub async fn policy_server_allows_event(
@@ -152,6 +150,7 @@ pub async fn policy_server_allows_event(
 			);
 			return Ok(());
 		}
+		// N.B. In a future room version, this will be a soft failure specifically.
 		debug_info!(
 			via = %ps.via,
 			"Event is incoming but does not have a valid policy server signature; asking policy \
@@ -183,6 +182,10 @@ pub async fn policy_server_allows_event(
 	self.fetch_policy_server_signature(pdu, pdu_json, &ps.via, outgoing, room_id, ps_key, 0)
 		.await
 }
+
+/// Handles an error returned by the policy server. If the error is one that should be returned to
+/// the user, it is propagated, otherwise the request may be retried (for example, when
+/// rate-limited).
 #[allow(clippy::too_many_arguments)]
 #[implement(super::Service)]
 async fn handle_policy_server_error(
@@ -290,7 +293,8 @@ async fn handle_policy_server_error(
 }
 
 /// Asks a remote policy server for a signature on this event.
-/// If the policy server signs this event, the original data is mutated.
+/// If the policy server signs this event, the original data is mutated. Otherwise, the error is
+/// handled and potentially returned.
 #[allow(clippy::too_many_arguments)]
 #[implement(super::Service)]
 #[tracing::instrument(skip_all, fields(event_id=%pdu.event_id(), via=%via), level = "info")]
@@ -358,7 +362,7 @@ pub async fn fetch_policy_server_signature(
 			response.signatures
 		);
 		return Err!(BadServerResponse(
-			"Policy server did not include expected server name in signatures"
+			"Policy server did not sign the event"
 		));
 	}
 	// Unwraps are safe here because we checked both in the above if statement

src/service/rooms/event_handler/upgrade_outlier_pdu.rs

@@ -1,18 +1,17 @@
 use std::{borrow::Borrow, collections::BTreeMap, sync::Arc, time::Instant};
 
 use conduwuit::{
-	Err, Result, debug, debug_info, debug_warn, err, implement, is_equal_to,
-	matrix::{Event, EventTypeExt, PduEvent, StateKey, state_res},
-	trace,
+	debug, debug_info, debug_warn, err, implement, is_equal_to, matrix::{state_res, Event, EventTypeExt, PduEvent, StateKey}, trace,
 	utils::{
 		IterStream,
 		stream::{BroadbandExt, ReadyExt},
 	},
-	warn,
+	Err,
+	Result,
 };
-use futures::{FutureExt, StreamExt, future::ready};
+use futures::{future::ready, FutureExt, StreamExt};
 use ruma::{
-	CanonicalJsonValue, RoomId, ServerName, api::error::ErrorKind, events::StateEventType,
+	api::error::ErrorKind, events::StateEventType, CanonicalJsonValue, RoomId, ServerName,
 };
 use tokio::join;
 
@@ -249,36 +248,34 @@ where
 	if !soft_fail {
 		// Don't call the below checks on events that have already soft-failed, there's
 		// no reason to re-calculate that.
-		// 14-pre. If the event is not a state event, ask the policy server about it
-		if incoming_pdu.state_key.is_none() {
-			debug!(event_id = %incoming_pdu.event_id, "Checking policy server for event");
-			if let Err(e) = self
-				.policy_server_allows_event(
-					&incoming_pdu,
-					&mut incoming_pdu.to_canonical_object(),
-					room_id,
-					&room_version_rules,
-					true,
-				)
-				.await
-			{
-				if matches!(e.kind(), ErrorKind::Forbidden) {
-					warn!(
-						event_id = %incoming_pdu.event_id,
-						error = %e,
-						"Event has been marked as spam by policy server: {}",
-						e.message(),
-					);
-					soft_fail = true;
-				} else {
-					return Err(e);
-				}
-			} else {
-				debug!(
+		// 14-pre. ask the policy server to sign the event, if possible
+		debug!(event_id = %incoming_pdu.event_id, "Checking policy server for event");
+		if let Err(e) = self
+			.policy_server_allows_event(
+				&incoming_pdu,
+				&mut incoming_pdu.to_canonical_object(),
+				room_id,
+				&room_version_rules,
+				true,
+			)
+			.await
+		{
+			if matches!(e.kind(), ErrorKind::Forbidden) {
+				info!(
 					event_id = %incoming_pdu.event_id,
-					"Event has passed policy server check."
+					error = %e,
+					"Event has been marked as spam by policy server: {}",
+					e.message(),
 				);
+				soft_fail = true;
+			} else {
+				return Err(e);
 			}
+		} else {
+			debug!(
+				event_id = %incoming_pdu.event_id,
+				"Event has passed policy server check."
+			);
 		}
 
 		// Additionally, if this is a redaction for a soft-failed event, we soft-fail it
@@ -298,9 +295,7 @@ where
 				.is_event_soft_failed(&redact_id)
 				.await
 			{
-				// TODO: This should avoid pushing the event to the timeline instead of using
-				// soft-fails as a hack
-				warn!(
+				info!(
 					redact_id = %redact_id,
 					"Redaction is for a soft-failed event"
 				);
@@ -367,9 +362,11 @@ where
 		self.services
 			.pdu_metadata
 			.mark_event_soft_failed(incoming_pdu.event_id());
-		debug_warn!(
+
+		info!(
 			elapsed = ?timer.elapsed(),
-			"Event has been soft-failed",
+			event_id = %incoming_pdu.event_id,
+			"Event was soft failed"
 		);
 	} else {
 		debug_info!(