MSC3757: Restricting who can overwrite a state event #3757
Conversation
andybalaam
changed the title
turt2live
added
requires-room-version
This comment was marked as duplicate.
This comment was marked as duplicate.
Co-authored-by: Andrew Morgan <[email protected]>
Co-authored-by: Travis Ralston <[email protected]>
Also add sub-headers for each issue
There was a problem hiding this comment.
Use cases need more fleshing out, particularly to justify why arbitrary bytes as a suffix are a good idea (they likely aren't). Assuming the reasons are "flexibility", then at the very least we should be aggressively restricting which arbitrary bytes we allow, e.g [a-z0-9].
| ## Proposal | ||
|
|
||
| Therefore, we need a different way to state that a given state event may only | ||
| be written by its owner. **We propose that if a state event's `state_key` *starts with* a matrix ID (followed by an underscore), only the sender with that matrix ID (or higher PL users) can set the state event.** This is an extension of the current behaviour where state events may be overwritten only if the sender's mxid *exactly equals* the `state_key`. |
There was a problem hiding this comment.
Is it valid to have an empty string as the suffix? E.g @kegan:matrix.org_? What about having arbitrary unicode characters? Null bytes? Please let's be sensible and force sensible restrictions on this new user-defined variable, lest we end up in another hell of poor validation causing problems for clients/servers.
I would suggest a very strong restriction of [a-z0-9] unless there are good reasons to allow other characters (there almost certainly are not).
There was a problem hiding this comment.
Allowing an empty suffix probably won't hurt. I can imagine cases of the suffix being a property that may take on an empty/nullish value.
For a device ID to be usable as a suffix, the suffix charset must include all characters that can be used as a device ID. Unfortunately, there's no specced device ID charset that I can find, and in practice it is quite broad:
- Device IDs generated by QR code logins (at least on Element X Android) can contain
/and+. /_matrix/client/v3/loginallows setting a custom device ID of any string, and the Synapse implementation allows all sorts of characters (and lets me set a device ID ofa b ?!@#$%^&*()-=_+/).
So if we want device ID suffixes now, maybe defining a suffix charset is premature, unless this MSC also defines a stricter device ID charset. But that raises the issue of what to with existing device IDs that don't follow the charset...
There was a problem hiding this comment.
This is surely too literal though. You can map device IDs deterministically to a valid character set, at its most basic SHA256(device_id).
There was a problem hiding this comment.
That's a good idea. I was worried that the input size of device IDs would be too large to safely hash, but the point is to avoid hash collisions, the chance of which should still be small.
Then, this MSC can do away with trying to define a suffix length / charset that can fit a raw device ID.
I will still propose a few non-word characters to be in the suffix charset, because it may be handy to have a suffix containing multiple properties that are easy to separate with a non-word character (eg. @user:hs_prop1_prop2_prop3). The spec already uses a charset of [0-9a-zA-Z.=_-] for email login secrets/tokens, and would be a decent charset to use here as well.
There was a problem hiding this comment.
On second thought, a handy consequence of not restricting the suffix charset is the ability to prefix any existing state key with a user's MXID to scope its ownership to that user. With a restricted suffix charset, there may be some state keys that would be invalid as a suffix.
IMO as long as the charset of state keys in general is left unrestricted, there's little benefit in restricting the suffix charset, unless doing so is a step towards restricting the general state key charset.
There was a problem hiding this comment.
I agree that there's no point in restricting these specific state keys when all other state keys are still arbitrary strings. To take advantage of this MSC, you must give users permission to send state events of certain types. That means users will be able to send unprefixed state keys too, which will not have any character set restrictions.
Restricting state keys in general might be a good idea to do in combination with MSC2828
There was a problem hiding this comment.
I don't agree with the attitude of "we don't have good validation, so let's not add validation". There's zero reason to punt this down the line to another MSC. It's absolutely trivial to expand the character set or length limits in another MSC. It's very difficult to restrict it once there are client implementations in the wild relying on there being no restrictions. Validation is important to reduce the attack surface of any newly added features. The suffix string will be stored in new places where the state key is not (e.g DBs will likely either have this as a column, or indexed in such a way to allow efficient ordered lookups), which means there will be new code written to read this input. Validate the input, please.
There was a problem hiding this comment.
If the intent of a restricted suffix charset is to ease parsing out the user ID prefix, then the usefulness of the restriction might be limited by how the prefix must be able to contain any printable ASCII character, due to having to support historical user IDs. Also, since the parsing is concerned more with the user ID prefix than with the suffix, locking down the suffix charset might not impact much.
Besides, the "suffix string" isn't really meant to be a new kind of syntax for state keys, but is just a result of the semantic of user-scoped state keys via string packing. It's perhaps better to think of a scoped state key not as a user ID + a suffix, but as an ordinary state key prefixed by a user ID; or rather, that this MSC proposes allowing state keys to optionally be prefixed by a user ID. Being prefixed doesn't change the nature of the content of the rest of the state key, but applying different restrictions to prefixed & unprefixed state keys would imply that it does.
Though if restricting the charset is non-negotiable, then maybe a compromise is to apply a broad charset, like all printable ASCII characters.
Link to the MSC: matrix-org/matrix-spec-proposals#3757 --------- Co-authored-by: Quentin Gliech <[email protected]>
- Update auth rules to enforce the suffix limit - Remove potential issue of long user IDs, as that is now solved
Highlight that the current restrictions prevent being able to set multiple state events of the same type with exclusive write access
- Use assertive language - Limit lines to ~100 characters - Improve consistency of terms
as it is already discussed in two other sections
|
That concern has already been raised. |
|
That concern has already been raised. |
|
Rendered
Implementations:
Written by @ara4n , with contributions from @Johennes and @andybalaam .
Shepherd: @AndrewFerr
FCP tickyboxes