digitalentity / matrix_encryption_disabler

Mozilla Public License 2.0
14 stars 2 forks source link

Patch power levels on room creation #4

Closed digitalentity closed 2 years ago

digitalentity commented 2 years ago

Fixes https://github.com/digitalentity/matrix_encryption_disabler/issues/3

This now works similar to https://github.com/fhirfactory/pegacorn-communicate-roomserver/pull/7, but still keeps encryption from being enabled from the start during room creation and filters the encryption events just to be safe.

If federated servers and clients respect permissions room encryption should never be enabled even if the room is federated.

@spantaleev you might want to test this change and make sure it works with your ansible playbooks (no reason why it shouldn't though).

digitalentity commented 2 years ago

Tested on a private server, works as intended. Not tested with federated servers, but I expect it work correctly if servers comply to the protocol and enforce power levels.

spantaleev commented 2 years ago

Does Synapse not validate these power level values? Is 150 a valid value? The m.room.power_levels description in the Matrix 1.2 spec does not seem to explicitly state that you can't use a value higher than 100.

In practice, it seems like you've tested it and Synapse and the clients you've tested don't break, at least not yet.

Still, it seems a little dangerous to use such values. Any homeserver that gets involved with this room over federation may encounter trouble. Even Synapse itself may reject this unusual value if it encounters it over federation (not tested, but it's a reasonable suspicion).


So I think that this feature is a little dangerous to have, especially when enabled by default.

It may make sense to:

spantaleev commented 2 years ago

Also, rewriting the m.room.power_levels state event seems risky, because the client may have sent its own other m.room.power_levels configuration. I suppose rooms created using some automation (a bot, a bridge, etc.), may be pre-configured with special power levels. Not sure if bridge-created events go through this "spam checker" event filter though, but bot-created events via the API do. Having matrix_encryption_disabler enabled on the server will throw away their custom values and replace them with these hardcoded defaults, wouldn't it?

m.room.power_levels logic with a higher fidelity would be preferable. Say, if no m.room.power_levels is found, use these defaults (just like now). But if such an event exists, reach into and set or modify its m.room.encryption key only, leaving all other levels intact. For additional safety, you may also wish to verify that the power level for the user (content.user.requestor_id) is less than what you're specifying for m.encryption, etc.

digitalentity commented 2 years ago

Makes sense. I've tested this with Element (Linux, Android, Web), FluffyChat (Android and Web), Syphon (Android). No issues.

I'll propose changes to the spec to allow higher power levels (non-interactively set by automation).

I also thought about something that would rewrite the power_levels event if present, but I wanted something fast to verify the idea and make sure it works on federated rooms. Proper patching is on my to-do list.

Hiding this hack behind a setting makes perfect sense, thanks, that didn't come to my mind when I was writing this.