A m.room.power_levels state event was accepted with newlines in user IDs

[ShadowJonathan]

Description

Synapse is accepting m.room.power_levels state events with the following content;

"content": {

  "users": {
    "@user:example.org": 50,
    "@user:example.org\n": 50,
    "@user:example.org\\n": 0,

    // ...
  },

  // ...
}

Ruma trips on this content, because it expects fully well-formed User IDs, and so rejects parsing this kind of event.

The spec says not much of substance regarding validating these user IDs.

Note: Event has been partially redacted to keep community privacy, i can provide more info and the corresponding room via DMs to synapse devs.

Version information

Own homeserver version (atm): 1.40.0
Their homeserver version (atm): 1.40.0

[timokoesters]

The spec says in Checks performed on receipt of a PDU that the event has to be valid.

But "valid" is not properly defined in the spec, also see https://github.com/matrix-org/matrix-doc/issues/1646

[richvdh]

It's (arguably) a bug in Synapse that it accepted this event over the C-S api, but also Ruma needs to accept it over federation anyway, for compatibility with older/other homeservers.

[jplatte]

It's the servername validation that errors here. The PDU will be kept in its original form anyways, but would it make more sense for compatibility to "fix" the ID by removing the trailing newline (i.e. when checking power levels, have @user:example.org\n's power level apply to @user:example.org) or not? Alternatively the rooms where such an event exists could just be declared broken and replaced through a room upgrade (I guess that mirrors the power levels so if latest PL event is broken, it would have to be fixed up first).

[richvdh]

it make more sense for compatibility to "fix" the ID by removing the trailing newline (i.e. when checking power levels, have @user:example.org\n's power level apply to @user:example.org) or not?

not. You should only apply PLs when the user id matches exactly. For the same reason, you should also not apply @user:EXAMPLE.ORG's PL.

Alternatively the rooms where such an event exists could just be declared broken

The situation is clear here. We cannot strengthen the rules in what constitutes an "acceptable" m.room.power_levels event for federation without a new room version. If your server implementation wants to support existing room versions, it needs to accept m.room.power_levels events with malformed user-ids in the users map - and essentially ignore those malformed user-ids.

[jplatte]

Just remembered that we were recently talking about event validation in one of the ruma rooms and I re-realized that the strong types from ruma-events aren't really feasible for validating events over federation anyways.

Overall, I think it is very much warranted to not have this event supported "natively" in Ruma. Conduit will have to do its validation differently anyways, and clients built using Ruma might as well fail to recognize this event for all I care. I'm not willing to build in a workaround for this unless it affects lots of rooms and/or is very hard to fix.

[reivilibre]

To summarise my understanding of this issue: Synapse is defective in that it accepts these events from local clients, but accepting them from over federation is unfortunately correct.