feat(MSC4277): Unify reporting endpoint behaviours #919
No reviewers
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/MSC
Matrix/Media
Meta
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: continuwuation/continuwuity#919
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "nex/feat/4277-harmonized-reporting-endpoints"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
MSC4277
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.
Surely room peaking already has this issue? Either way checking if the room or event is visible to the user would be better
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.
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!(
Same
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
Checking visibility using that check wouldn't allow events that we can't run that check on through at least
Re both threads, my reasoning is basically: dropping reports silently is bad - especially with user provided reason text.
@Jade wrote in #919 (comment):
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.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.