From e944ed5eb482572d6ef73fa8e415e21add40afc9 Mon Sep 17 00:00:00 2001 From: strawberry Date: Wed, 17 Jan 2024 02:08:07 -0500 Subject: [PATCH] fix `/report` endpoint a lot in short, the `/report` endpoint now checks if: - the reporting room in the URI matches the PDU/event reported - sender user is in the room reported - raises report reasoning to 750 characters (spec doesn't say to limit these, but thorough and informative reports for server admins are not a bad thing) - (hopefully) fixes some broken formatting - add a random short delay before sending a successful response to the client to make it more annoying to enumerate for events on our server (security by obscurity but spec suggests it) basically, secure reports better lol see https://spec.matrix.org/v1.9/client-server-api/#post_matrixclientv3roomsroomidreporteventid Signed-off-by: strawberry --- DIFFERENCES.md | 1 + src/api/client_server/report.rs | 45 +++++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/DIFFERENCES.md b/DIFFERENCES.md index f47c078f..6fe0a549 100644 --- a/DIFFERENCES.md +++ b/DIFFERENCES.md @@ -52,3 +52,4 @@ - Only follow 6 redirects total in our default reqwest ClientBuilder - Generate passwords with 25 characters instead of 15 - Add missing `reason` field to user ban events (`/ban`) +- For all [`/report`](https://spec.matrix.org/v1.9/client-server-api/#post_matrixclientv3roomsroomidreporteventid) requests: check if the reported event ID belongs to the reported room ID, raise report reasoning character limit to 750, fix broken formatting, make a small delayed random response per spec suggestion on privacy, and check if the sender user is in the reported room. \ No newline at end of file diff --git a/src/api/client_server/report.rs b/src/api/client_server/report.rs index e7503eac..85e3f543 100644 --- a/src/api/client_server/report.rs +++ b/src/api/client_server/report.rs @@ -1,9 +1,14 @@ +use std::time::Duration; + use crate::{services, utils::HtmlEscape, Error, Result, Ruma}; +use rand::Rng; use ruma::{ api::client::{error::ErrorKind, room::report_content}, events::room::message, int, }; +use tokio::time::sleep; +use tracing::{debug, info}; /// # `POST /_matrix/client/v3/rooms/{roomId}/report/{eventId}` /// @@ -12,18 +17,30 @@ use ruma::{ pub async fn report_event_route( body: Ruma, ) -> Result { + // user authentication let sender_user = body.sender_user.as_ref().expect("user is authenticated"); + info!("Received /report request by user {}", sender_user); + + // check if we know about the reported event ID or if it's invalid let pdu = match services().rooms.timeline.get_pdu(&body.event_id)? { Some(pdu) => pdu, _ => { return Err(Error::BadRequest( - ErrorKind::InvalidParam, - "Invalid Event ID", + ErrorKind::NotFound, + "Event ID is not known to us or Event ID is invalid", )) } }; + // check if the room ID from the URI matches the PDU's room ID + if body.room_id != pdu.room_id { + return Err(Error::BadRequest( + ErrorKind::NotFound, + "Event ID does not belong to the reported room", + )); + } + // check if reporting user is in the reporting room if !services() .rooms @@ -38,6 +55,7 @@ pub async fn report_event_route( )); } + // check if score is in valid range if let Some(true) = body.score.map(|s| s > int!(0) || s < int!(-100)) { return Err(Error::BadRequest( ErrorKind::InvalidParam, @@ -45,13 +63,15 @@ pub async fn report_event_route( )); }; - if let Some(true) = body.reason.clone().map(|s| s.chars().count() > 500) { + // check if report reasoning is less than or equal to 750 characters + if let Some(true) = body.reason.clone().map(|s| s.chars().count() >= 750) { return Err(Error::BadRequest( ErrorKind::InvalidParam, - "Reason too long, should be 500 characters or fewer", + "Reason too long, should be 750 characters or fewer", )); }; + // send admin room message that we received the report with an @room ping for urgency services().admin .send_message(message::RoomMessageEventContent::text_html( format!( @@ -59,18 +79,18 @@ pub async fn report_event_route( Event ID: {:?}\n\ Room ID: {:?}\n\ Sent By: {:?}\n\n\ - Report Score: {:?}\n\ + Report Score: {:#?}\n\ Report Reason: {:?}", - sender_user, pdu.event_id, pdu.room_id, pdu.sender, body.score, body.reason + sender_user.to_owned(), pdu.event_id, pdu.room_id, pdu.sender, body.score, body.reason ), format!( - "
Report received from: {0:?}\ + "
@room Report received from: {0}\
  • Event Info
  • \ Report Info
    • Report Score: {4:?}
    • Report Reason: {5}
    • \
", - sender_user, + sender_user.to_owned(), pdu.event_id, pdu.room_id, pdu.sender, @@ -79,5 +99,14 @@ pub async fn report_event_route( ), )); + // even though this is kinda security by obscurity, let's still make a small random delay sending a successful response + // per spec suggestion regarding enumerating for potential events existing in our server. + let time_to_wait = rand::thread_rng().gen_range(8..21); + debug!( + "Got successful /report request, waiting {} seconds before sending successful response.", + time_to_wait + ); + sleep(Duration::from_secs(time_to_wait)).await; + Ok(report_content::v3::Response {}) }