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.
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
MSC3757: Restricting who can overwrite a state event #3757
base: main
Are you sure you want to change the base?
MSC3757: Restricting who can overwrite a state event #3757
Changes from 8 commits
ff5fd48
610f244
bfde329
344e876
ccb7e52
1ce7e0e
6df6109
6e108b3
bd4176f
e352a1d
5e95ff3
68dc97f
17890fd
f962bf3
dd9b33e
eb0eed6
ac24510
9490cbd
486b0cd
590ff96
d9b149d
ae17437
8222738
63955d7
07d784a
99698ef
9f4f31a
75f03da
e833e8a
a4b40b5
a0da59b
5855a7f
deba3b8
8090f69
3a0d095
e16482a
1ddddb6
fd87b8a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this is actually derestricting who can overwrite a state event.
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.
That's true for MSC3779, but not as much for this one; the former bypasses PL restrictions for setting state that belongs to the sender (where "belongs to" = the state key starts with the sender's MXID), but this MSC does not.
The restriction proposed by this MSC is to prevent state that belongs to a particular user from being overwritten by other, equally-powerful users.
The only PL restriction that's relaxed by this MSC is for allowing more powerful users to overwrite state that doesn't belong to them, for the sake of having a tool against state graffiti.
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.
You're right. Still, I find the title a bit confusing. How about:
or something
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.
TBH I informally refer to this MSC as "the owned state MSC" despite that being the title of MSC3779 😉
Since one of the distinguishing differences of this MSC over 3779 is the ability for admins to manage others' state, maybe we could call it
?
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 find the
@
prefix restriction a bit questionable, for the record. The only specified state event which uses it ism.room.member
, which the auth rules validatestate_key
well before the restriction is checked. Non-spec usage of the state key already applies tricks like prefixing user IDs with_
to ensure they aren't hit by the restriction, and I'm not really convinced that location sharing/beacons need to pack a user ID into the state key anyways (see https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r1103877363 ).This has me leaning towards removing the
@
prefix restriction entirely, but I recognize that's not exactly a helpful opinion for this MSC to tackle. Creating stronger arguments for why the dependent features require the user ID in the state key I think would help, though.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.
Last time we changed the auth rules, there was a lot of discussion around "let's never do this again without test vectors for the auth rules" (cf matrix-org/matrix-spec#1710, matrix-org/matrix-spec#1642).
The apparent confusion over what the current auth rules mean, and what they should say after the change, makes me think that we should start some progress in that direction.
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.
The Complement tests should be a step in the right direction.
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.
While the spec specifies a grammar for server names, are we SURE that servers verify that grammar? While officially underscores are invalid in domain names and you can't order a TLS certificate for it nowadays, such domains still exist in the wild and wildcard certificates even allow using TLS with them. For example: https://my_sarisari_store.typepad.com/
I think relying on underscores as a separator is a rather scary trap and I wouldn't bet on all currently developed Matrix servers rejecting such server names.
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.
ac24510 adds this to the "Potential issues" section.
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.
How about using
@
as the separator? Since the separator will never be the first character in the state key, it shouldn't be possible to confuse it with the user ID sigil.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.
|
could also be a good separator.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 remain concerned that this MSC opens the door to a proliferation of state events that we cannot delete, and that faster-joins can't help with. I would like to see some discussion of this in the MSC.
Admittedly, we have this problem to some extent today, in that there is nothing stopping users creating lots of state events with different types, but:
state_default
or higher. This MSC would change that, by allowing any user to (say) create millions ofm.beacon_info
events.This is not a veto from me, but I do think we need to consider this properly and at least set out guidelines for future MSCs that build on this.
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.
This is what MSC3779 proposes, not this one.
To clarify, the effective auth rule changes proposed by this MSC are to:
<sender>_<anySuffix>
to all users<sender>
is the MXID of the user sending the state event<mxid>
&<mxid>_<anySuffix>
to users whose PL is greater than that of<mxid>
's PL<mxid>
is any MXIDThe event-sending restrictions imposed by
m.room.power_levels
remain in effect.What this MSC does allow is creating lots of state events with different state key suffixes -- but that would be limited to room moderators, just like how creating lots of state events with different types currently is.
As a start, MSC4143 makes an explicit mention to this one (namely, in the section regarding MatrixRTC room state), and is what has driven interest in this MSC.