Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2938: Report content to moderators #2938

Open
wants to merge 9 commits into
base: old_master
Choose a base branch
from

Conversation

Yoric
Copy link

@Yoric Yoric commented Jan 5, 2021

As a followup to element-hq/element-meta#1848, this proposal introduces a mechanism to report content to room moderators instead of reporting it to homeserver administrators.

Rendered: https://github.com/Yoric/matrix-doc/blob/master/proposals/2938-report-content-to-moderators.md

Copy link
Contributor

@erdnaxeli erdnaxeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good MSC, I would like to see it merged. I am just worried about it being a new way to do spam.

proposals/2938-report-content-to-moderators.md Outdated Show resolved Hide resolved
@Half-Shot Half-Shot changed the title [MSC] Report content to moderators MSC2938: Report content to moderators Jan 5, 2021
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Jan 5, 2021
Comment on lines 48 to 49
If `target` is `"room-moderators"`, the message is intended to be propagated as a server notice to
moderators and administrators of this room.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"moderators and administrators of this room" should probably be defined precisely, e.g. with a specific power level (some key in the power level event, not a hardcoded value)

Copy link
Author

@Yoric Yoric Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
I've added the definition "moderator == someone who can both kick and ban" and removed the word "administrator" entirely. Do you feel it's now clear enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should they also be able to redact the reported messages?

@Yoric
Copy link
Author

Yoric commented Jan 13, 2021

I have added a first draft of how to make this work with federation.

@Yoric Yoric requested review from Half-Shot and uhoreg January 13, 2021 12:24
----------------------------------
| target | string | One of "room-moderators", "homeserver-admins". |

If `target` is `"homeserver-admins"` or unspecified, the behavior is unchanged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially tricky because the spec doesn't actually a concept of a homeserver admin. Technically reports can be read by admins because they end up in the report table, but I wonder if we need to further define the role of a homeserver ad min. This is probably something for the spec team to think about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the client-server spec already mentions "server administrator" 10 times. It just doesn't say how that must be implemented.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could easily rename this server-administrator, it it changes anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've informally referred to it as "a implementation may have a server admin that may do blah if they like", but I've not seen it referred to explicitly in the keys and strings of the spec, which is my concern here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we could rename it to homeserver, without mentioning a specific role.


| Parameter | Type | Description |
----------------------------------
| target | string | One of "room-moderators", "homeserver-admins". |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me wonders if this would be suited to an integer field, where users with a PL above this number will get the report. It might make things more flexible

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably would but it would also open this API to sending notices to everyone in the room. Is this something we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced it should be tied to PLs.

On the other hand hyphen-separated identifiers are not something the matrix spec uses anywhere else, afaik: we use underscores to separate words.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could make this "admin.room" vs. "admin.server".

proposals/2938-report-content-to-moderators.md Outdated Show resolved Hide resolved
homeserver as the user. This is a limitation that needs to be fixed before merging this proposal.
However, it may be possible to run an experiment before this limitation is fixed.

This proposal may end up being used for spamming the moderators of rooms. We believe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have concerns about this, as previously the homeserver would just stick events into a table and essentially making it a poor spam vector (due to lack of vis for most people) especially when rate limited. This PR changes the reports from being invisible-by-default to visible-by-default which IS making the problem worse as it stands.

The other problem is that one report presumably causes a message to be sent to N moderators, which can be used as an amplification attack generally mass spamy thing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have concerns about this, as previously the homeserver would just stick events into a table and essentially making it a poor spam vector (due to lack of vis for most people) especially when rate limited. This PR changes the reports from being invisible-by-default to visible-by-default which IS making the problem worse as it stands.

That is true.

A few ideas from the top of my head:

  • we could rate-limit this;
  • we could make spam-checkers able to inspect these reports.

"reason": "{reason}" // From JSON body parameter
}
```
3. for all homeservers other than local with at least one moderator in room `roomId`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would send the report into the room as it stands, which would out the reporter to the whole room? :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the "When a server receives a PDU with the following content" section below, but any server admins (= users with their own server in the room) could trivially bypass that and read all reports even if they're not a room admin.

Copy link
Author

@Yoric Yoric Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would send the report into the room as it stands, which would out the reporter to the whole room? :(

I don't understand. What should I change to make it visible only to moderators?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the "When a server receives a PDU with the following content" section below, but any server admins (= users with their own server in the room) could trivially bypass that and read all reports even if they're not a room admin.

Yes, is that something we should defend against?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use encryption to solve this. Just e2e encrypt it to the mods. Although this still leaks that you are messaging the mods and would require some sort of UX to hide this from clients who are not mods. Also need to deal with the UX of what sessions for which mods you trust.

Otherwise this is hard because it requires bypassing the regular room sync protocol IIUC. Basically we would need to replicate this report only to servers where at least one user is an admin.

One option would be to create a new room with the mods, but then you need to define a lot of metadata to link that chat back to the originating room and the client UX would probably want to display it as events in the original room rather than a new room. This approach does have some advantages though such as allowing e2ee even in non-encrypted rooms and a natural place for follow up discussion about the report.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new room, world-writable but only readable by mods?

Sounds like e-mail :)

I'm open to exploring this (hopefully in a followup) but this raises quite a few questions:

  • which user opened the room?
  • assuming that we wish to reuse rooms across reports, how do we find out (across federation) about the already opened room?
  • should we kick moderators from the room when they lose moderator status?
  • etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be only readable to mods because then they can't ask follow up questions. Right now it seems like opening a new room per report would be the best option. (Maybe reusing if the same user reports again to the same mods? Or the user can reuse the room but update the mod list?)

@ShadowJonathan
Copy link
Contributor

Leaving my thoughts here like i did in the synapse-dev room: I think the best course of action would be to make this an _unstable endpoint (if anything, for now), never stabilising it to a normal endpoint, and fixing the eventual problem with a homeserver-to-homeserver room, which'd require a seperate MSC.

(This room would establish a DM with another homeserver "server" user (e.g. @server:<serverpart> or @:<serverpart>), and then send any server-to-server informational data over that room, over federation)

@Yoric
Copy link
Author

Yoric commented Feb 1, 2021

Leaving my thoughts here like i did in the synapse-dev room: I think the best course of action would be to make this an _unstable endpoint (if anything, for now), never stabilising it to a normal endpoint, and fixing the eventual problem with a homeserver-to-homeserver room, which'd require a seperate MSC.

I have no problem with using the _unstable, especially for a prototype.

Comment on lines 47 to 49
| Parameter | Type | Description |
----------------------------------
| target | string | One of "room-moderators", "homeserver-admins". |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 66 to 75
| Parameter | Type | Description |
----------------------------------
| body | string | **Required** A human-readable description of the problem, generated by the server (e.g. "User %s has reported that '%s'"). |
| msgtype | enum | **Required** In this case, `m.server_notice.content_report`. |
| roomId | string | **Required** The id of the room being reported. |
| eventId | string | **Required** The id of the event being reported. |
| userId | string | **Required** The id of the user reporting the event. |
| score | integer | The score to rate this content as where -100 is most offensive and 0 is inoffensive, as given by the reporting user. |
| reason | string | The reason given by the reporting user. |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please can you fix the formatting on this one too?

1. ignore the message if `eventId` is not an event in `roomId` or if the user is not a member of `roomId`; otherwise
2. for all moderators in room `roomId` *on the local homeserver*:
1. send a server notice
```json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use jsonc for these code blocks to stop github making them all bright red?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```json
```jsonc


| Parameter | Type | Description |
----------------------------------
| target | string | One of "room-moderators", "homeserver-admins". |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced it should be tied to PLs.

On the other hand hyphen-separated identifiers are not something the matrix spec uses anywhere else, afaik: we use underscores to separate words.

Comment on lines 102 to 104
"roomId": "{roomId}", // From path parameter
"eventId": "{eventId}", // From path parameter
"userId": "{userId}", // From authentication token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do NOT use camel-case in json. Per https://matrix.org/docs/spec/client_server/r0.6.1#api-standards

The names of the API endpoints for the HTTP transport follow a convention of using underscores to separate words (for example /delete_devices). The key names in JSON objects passed over the API also follow this convention.

@@ -0,0 +1,222 @@
# MSC2938: Report Content to Moderators
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proposals/2938-report-content-to-moderators.md Outdated Show resolved Hide resolved
1. ignore the message if `eventId` is not an event in `roomId` or if the user is not a member of `roomId`; otherwise
2. for all moderators in room `roomId` *on the local homeserver*:
1. send a server notice
```json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```json
```jsonc

"reason": "{reason}" // From JSON body parameter
}
```
3. for all homeservers other than local with at least one moderator in room `roomId`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use encryption to solve this. Just e2e encrypt it to the mods. Although this still leaks that you are messaging the mods and would require some sort of UX to hide this from clients who are not mods. Also need to deal with the UX of what sessions for which mods you trust.

Otherwise this is hard because it requires bypassing the regular room sync protocol IIUC. Basically we would need to replicate this report only to servers where at least one user is an admin.

One option would be to create a new room with the mods, but then you need to define a lot of metadata to link that chat back to the originating room and the client UX would probably want to display it as events in the original room rather than a new room. This approach does have some advantages though such as allowing e2ee even in non-encrypted rooms and a natural place for follow up discussion about the report.


| Parameter | Type | Description |
| --------- |------- | ----------- |
| target | string | One of "server-notice", "homeserver_admins". |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As below

Suggested change
| target | string | One of "server-notice", "homeserver_admins". |
| target | string | One of "room_moderators", "homeserver_admins". |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.