feat(MSC4277): Unify reporting endpoint behaviours #919

Open
nex wants to merge 2 commits from nex/feat/4277-harmonized-reporting-endpoints into main
Owner

MSC4277

  • reporting rooms now always returns 200 OK
  • reporting an event returns OK if we don't know about the reported event
  • removed the score parameter (needs a followup ruwuma update)
[MSC4277](https://github.com/matrix-org/matrix-spec-proposals/pull/4277) * reporting rooms now always returns 200 OK * reporting an event returns OK if we don't know about the reported event * removed the score parameter (needs a followup ruwuma update)
nex added the
Enhancement
Matrix/Client
Matrix/MSC
labels 2025-08-05 20:04:18 +00:00
nex added 1 commit 2025-08-05 20:04:18 +00:00
feat(MSC4277): Unify reporting endpoint behaviours
Some checks failed
Release Docker Image / merge (push) Blocked by required conditions
Release Docker Image / define-variables (push) Successful in 2s
Checks / Prefligit / prefligit (push) Successful in 15s
Checks / Rust / Format (push) Successful in 42s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (push) Has been cancelled
Checks / Rust / Cargo Test (push) Has been cancelled
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (push) Has been cancelled
Checks / Rust / Clippy (push) Has been cancelled
Checks / Prefligit / prefligit (pull_request) Successful in 20s
Documentation / Build and Deploy Documentation (pull_request) Successful in 38s
b1be499973
* reporting rooms now always returns 200 OK
* reporting an event returns OK if we don't know about the reported event
* removed the score parameter (needs a followup ruwuma update)
nex added 1 commit 2025-08-05 20:06:57 +00:00
style(MSC4277): Run lints to satisfy checks
All checks were successful
Release Docker Image / define-variables (push) Successful in 4s
Checks / Prefligit / prefligit (push) Successful in 19s
Checks / Rust / Format (push) Successful in 50s
Documentation / Build and Deploy Documentation (pull_request) Successful in 38s
Checks / Prefligit / prefligit (pull_request) Successful in 19s
Checks / Rust / Clippy (push) Successful in 2m58s
Checks / Rust / Cargo Test (push) Successful in 4m26s
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (push) Successful in 13m5s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (push) Successful in 13m35s
Release Docker Image / merge (push) Successful in 26s
3c35fb61c9
requested review from Jade 2025-08-05 20:14:01 +00:00
Jade requested changes 2025-08-05 22:38:57 +00:00
Jade left a comment
Owner

Wondering if this is the correct action, we already have visibility checks in /event

Wondering if this is the correct action, we already have visibility checks in /event
@ -59,3 +66,1 @@
return Err!(Request(NotFound(
"Room does not exist to us, no local users have joined at all"
)));
// return 200 as to not reveal if the room exists, preventing enumeration.
Owner

Surely room peaking already has this issue? Either way checking if the room or event is visible to the user would be better

Surely room peaking already has this issue? Either way checking if the room or event is visible to the user would be better
Author
Owner

Not all rooms can be peeked per visibility settings and whatnot, so it's hard to differentiate between a room that does not exist and a room that cannot be peeked (aside from the different error types ig). Plus, can't peeking go over federation too?
I'm not sure why the user needs to know if a reported room exists or not, the only reason it wouldn't (aside from us having never joined) is if they are deliberately reporting random room IDs.

Not all rooms can be peeked per visibility settings and whatnot, so it's hard to differentiate between a room that does not exist and a room that cannot be peeked (aside from the different error types ig). Plus, can't peeking go over federation too? I'm not sure why the user needs to know if a reported room exists or not, the only reason it wouldn't (aside from us having never joined) is if they are deliberately reporting random room IDs.
Owner

We don't do federated peeking yet. Perhaps erroring on invalid ID formats but accepting otherwise and always pushing to admin room is OK?

We don't do federated peeking yet. Perhaps erroring on invalid ID formats but accepting otherwise and always pushing to admin room is OK?
@ -101,3 +101,3 @@
// check if we know about the reported event ID or if it's invalid
let Ok(pdu) = services.rooms.timeline.get_pdu(&body.event_id).await else {
return Err!(Request(NotFound("Event ID is not known to us or Event ID is invalid")));
info!(
Owner

Same

Same
Author
Owner

I can kinda see that argument for this better, but also, I don't see how reporting an event we just don't have (remember soft-failed events pass this check) benefits anybody

I can kinda see that argument for this better, but also, I don't see how reporting an event we just don't have (remember soft-failed events pass this check) benefits anybody
Owner

Checking visibility using that check wouldn't allow events that we can't run that check on through at least

Checking visibility using that check wouldn't allow events that we can't run that check on through at least
Owner

Re both threads, my reasoning is basically: dropping reports silently is bad - especially with user provided reason text.

Re both threads, my reasoning is basically: dropping reports silently is bad - especially with user provided reason text.
Author
Owner

@Jade wrote in #919 (comment):

Re both threads, my reasoning is basically: dropping reports silently is bad - especially with user provided reason text.

counterpoint, dropping reports noisily, especially without ratelimiting (although kinda alleviated by the random delay) opens up the potential for enumeration. I can't think of a situation where an invalid report can even accidentally be made, since all implementations currently require you to have the thing you're reporting in front of you, as such invalid reports have to be deliberate and should be treated without inherent trust or respect.

@Jade wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/919#issuecomment-17067: > Re both threads, my reasoning is basically: dropping reports silently is bad - especially with user provided reason text. counterpoint, dropping reports noisily, especially without ratelimiting (although kinda alleviated by the random delay) opens up the potential for enumeration. I can't think of a situation where an invalid report can even accidentally be made, since all implementations currently require you to have the thing you're reporting in front of you, as such invalid reports have to be deliberate and should be treated without inherent trust or respect.
All checks were successful
Release Docker Image / define-variables (push) Successful in 4s
Checks / Prefligit / prefligit (push) Successful in 19s
Required
Details
Checks / Rust / Format (push) Successful in 50s
Required
Details
Documentation / Build and Deploy Documentation (pull_request) Successful in 38s
Checks / Prefligit / prefligit (pull_request) Successful in 19s
Required
Details
Checks / Rust / Clippy (push) Successful in 2m58s
Required
Details
Checks / Rust / Cargo Test (push) Successful in 4m26s
Required
Details
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (push) Successful in 13m5s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (push) Successful in 13m35s
Release Docker Image / merge (push) Successful in 26s
This pull request has changes requested by an official reviewer.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin nex/feat/4277-harmonized-reporting-endpoints:nex/feat/4277-harmonized-reporting-endpoints
git checkout nex/feat/4277-harmonized-reporting-endpoints
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: continuwuation/continuwuity#919
No description provided.