MSC4277: Harmonizing the reporting endpoints #4277
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # MSC4277: Harmonizing the reporting endpoints | ||
|
|
||
| Matrix currently has three reporting endpoints: | ||
|
|
||
| - [`/_matrix/client/v3/rooms/{roomId}/report/{eventId}`] ([added] in Matrix | ||
Johennes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 0.4.0, no MSC known) | ||
| - [`/_matrix/client/v3/rooms/{roomId}/report`] ([added][1] in Matrix 1.13 as per | ||
| [MSC4151]) | ||
| - `/_matrix/client/v3/users/{userId}/report` (to be [added][2] to Matrix as per | ||
| [MSC4260]) | ||
|
|
||
| The spec contains a number of subtle differences for these endpoints: | ||
|
|
||
| 1. The `reason` parameter on the event reporting endpoint was made optional by | ||
| [MSC2414]. The other two reporting endpoints didn't pick up on this, | ||
| however, and still require `reason` to be present while allowing it to be | ||
| blank. | ||
| 2. The user reporting endpoint [allows] servers to respond with 200 even if the | ||
| user doesn't exist to deny enumerating users. This option is not allowed in | ||
| the event and room reporting endpoints. | ||
| 3. The user and event reporting endpoints allow servers to add a random delay | ||
| when generating responses. This is a more sophisticated measure against | ||
| enumeration attacks. While the spec doesn't explicit forbid this technique | ||
| on the room reporting endpoint it doesn't explicitly mention or recommend | ||
| it either. | ||
|
|
||
| These differences seem unnecessary and were likely introduced by accident only. | ||
| The present proposal, therefore, seeks to align the three endpoints. | ||
|
|
||
| ## Proposal | ||
|
|
||
| On all three endpoints: | ||
|
|
||
| 1. The `reason` parameter is made optional. | ||
Johennes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 2. Servers MAY respond with 200 and no content regardless of whether the | ||
| reported subject exists or not to combat enumeration attacks. | ||
|
|
||
| 3. Servers MAY add a random delay or use constant time functions when | ||
| processing responses to combat enumeration attacks. | ||
|
|
||
| All of these changes appear applicable regardless of the reported subject and it | ||
| is not clear why the spec should differentiate the endpoints here. | ||
|
|
||
| ## Potential issues | ||
|
|
||
| The `reason` field on the room and user reporting endpoints is currently | ||
|
There was a problem hiding this comment. Honestly, I find it bizarre that reason is required but may be empty string. I suppose this might not be the MSC to change it, but I'd rather we just made it optional. There was a problem hiding this comment. I think I understand the value of requiring it to be filled for spam protection. The combination of required but allowed to be empty strikes me as slightly odd, too, but it's hard to judge without knowing T&S's future plans for the field. Generally though, I suppose if we later go from required but possibly empty to required and not empty, it'll be just as much a breaking change for clients as if we went from optional to required and not empty. There was a problem hiding this comment. It's hard to say. Even if the field was required people could just keyboard smash the field. I'm not really going to die on this hill though, I just think we should unify it in one direction. My feeling for T&S that any report is a useful report, and that tooling is smart enough these days that we can make use of the room_id / event_id to do preliminary investigations even if the |
||
| required. Making it optional, therefore, is a breaking change. Clients should | ||
| either act on the servers supported version or blanketly include an empty string | ||
| if the user doesn't provide a reason. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| None. | ||
|
|
||
| ## Security considerations | ||
|
|
||
| Enumeration attacks are likely more severe for users than for rooms or events. | ||
| There are still not irrelevant, however. | ||
|
|
||
| ## Unstable prefix | ||
|
|
||
| None. | ||
|
|
||
| ## Dependencies | ||
|
|
||
| None. | ||
|
|
||
| [`/_matrix/client/v3/rooms/{roomId}/report/{eventId}`]: https://spec.matrix.org/v1.13/client-server-api/#post_matrixclientv3roomsroomidreporteventid | ||
| [added]: https://github.com/matrix-org/matrix-spec-proposals/pull/1264 | ||
| [`/_matrix/client/v3/rooms/{roomId}/report`]: https://spec.matrix.org/v1.13/client-server-api/#post_matrixclientv3roomsroomidreport | ||
| [1]: https://github.com/matrix-org/matrix-spec/pull/1938 | ||
| [MSC4151]: https://github.com/matrix-org/matrix-spec-proposals/pull/4151 | ||
| [2]: https://github.com/matrix-org/matrix-spec/pull/2093 | ||
| [MSC4260]: https://github.com/matrix-org/matrix-spec-proposals/pull/4260 | ||
| [MSC2414]: https://github.com/matrix-org/matrix-spec-proposals/pull/2414 | ||
| [allows]: https://github.com/matrix-org/matrix-spec-proposals/pull/4260/files#diff-cbb17920e2617e7a20ab0838879675f7aa70e828f0263a3cfa5f4c53913ce5f7R34-R35 | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
200 regardless of event / room existence: element-hq/synapse#18263
I'm unsure if the random delay / constant time part requires an implementation. The spec already mentions this for the event reporting endpoint but Synapse doesn't appear to implement it and it's not part of element-hq/synapse#18120 either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the removal of
scoreinto element-hq/synapse#18263. Synapse didn't do anything else than assert the type anyway.