feat(MSC4277): Unify reporting endpoint behaviours #919

Merged
Jade merged 3 commits from nex/feat/4277-harmonized-reporting-endpoints into main 2025-09-10 16:35:03 +00:00
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)
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)
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
nex requested review from Jade 2025-08-05 20:14:01 +00:00
Jade requested changes 2025-08-05 22:38:57 +00:00
Dismissed
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?
nex marked this conversation as resolved
@ -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
nex marked this conversation as resolved
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.
nex added this to the v0.5.0-rc.8 milestone 2025-08-16 16:10:00 +00:00
Author
Owner

Blocked - see open comment threads.

Blocked - see open comment threads.
nex force-pushed nex/feat/4277-harmonized-reporting-endpoints from 3c35fb61c9
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
to de275dc8e9
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Successful in 24s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m4s
Release Docker Image / define-variables (pull_request) Successful in 2s
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (pull_request) Successful in 6m28s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (pull_request) Successful in 5m40s
Release Docker Image / merge (pull_request) Successful in 7s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 11m22s
2025-09-07 22:26:13 +00:00
Compare
fix(MSC4277): Undo refuted response changes
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Successful in 31s
Release Docker Image / define-variables (pull_request) Successful in 3s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 50s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (pull_request) Successful in 5m27s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m51s
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (pull_request) Successful in 4m8s
Release Docker Image / merge (pull_request) Successful in 7s
3df097c2a7
nex requested review from Jade 2025-09-07 22:36:49 +00:00
Jade approved these changes 2025-09-07 22:39:23 +00:00
nex force-pushed nex/feat/4277-harmonized-reporting-endpoints from 3df097c2a7
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Successful in 31s
Release Docker Image / define-variables (pull_request) Successful in 3s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 50s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (pull_request) Successful in 5m27s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m51s
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (pull_request) Successful in 4m8s
Release Docker Image / merge (pull_request) Successful in 7s
to baa89586e2
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Successful in 39s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 47s
Release Docker Image / define-variables (pull_request) Successful in 1s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 1m28s
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (pull_request) Failing after 1m2s
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (pull_request) Failing after 1m8s
Release Docker Image / merge (pull_request) Has been skipped
Release Docker Image / define-variables (push) Waiting to run
Release Docker Image / build-image (linux/amd64, release, linux-amd64, base) (push) Blocked by required conditions
Release Docker Image / build-image (linux/arm64, release, linux-arm64, base) (push) Blocked by required conditions
Release Docker Image / merge (push) Blocked by required conditions
Documentation / Build and Deploy Documentation (push) Successful in 31s
Checks / Prek / Clippy and Cargo Tests (push) Has been cancelled
Checks / Prek / Pre-commit & Formatting (push) Has been cancelled
2025-09-10 16:25:09 +00:00
Compare
Jade merged commit baa89586e2 into main 2025-09-10 16:35:03 +00:00
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.