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.

Sign up for GitHub

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

MSC3667: Enforce integer power levels #3667

Merged
merged 5 commits into from
Mar 27, 2022
Merged

MSC3667: Enforce integer power levels #3667

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd85016
Create mscxxxx-enforce-integer-power-levels.md
neilalexander Jan 21, 2022
fb65ed0
Update MSC number
neilalexander Jan 21, 2022
feb6b92
Update MSC to mention implementation
neilalexander Jan 24, 2022
9c5ae82
Update proposals/msc3667-enforce-integer-power-levels.md
neilalexander Feb 20, 2022
2087c4d
Update proposals/msc3667-enforce-integer-power-levels.md
neilalexander Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions proposals/msc3667-enforce-integer-power-levels.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# MSC3667: Enforce integer power levels
Copy link
Member

Choose a reason for hiding this comment

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

just to pre-empt some potential discussion: other event types, like join rules, aren't really subject to the same nuances that integer-based power levels are. As a power event itself in state res v2, the power levels are critical for determining whether or not a user can do something - the same issue can theoretically arise from incorrect membership or join rule, however the handling of those two events is targeted at the specific enum values whereas power levels are a range.


The spec defines power levels in `m.room.power_levels` events as integers, but due to legacy
behaviour in Synapse, string power levels are also accepted and parsed. The string parsing itself
Comment on lines +3 to +4
Copy link
Member

Choose a reason for hiding this comment

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

This first sentence is a little confusing. String power levels are not accepted by the spec - but in practice are often accepted by homeserver implementations as not doing so would break existing rooms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec PR #3688 will formally allow today's behaviour into the spec for all room versions up to and including version 9, since this issue was still not yet fixed in Synapse. That will allow us and future implementations to preserve compatibility with existing rooms.

This MSC, on the other hand, will fix the problem in future room versions (hopefully from room version 10).

is problematic because it is done using Python's [int()](https://docs.python.org/3/library/functions.html#int)
function, which has all sorts of associated behaviours:

> If x is not a number or if base is given, then x must be a string, bytes, or bytearray instance
> representing an integer literal in radix base. Optionally, the literal can be preceded by + or -
> (with no space in between) and surrounded by whitespace. A base-n literal consists of the digits 0
> to n-1, with a to z (or A to Z) having values 10 to 35. The default base is 10. The allowed values
> are 0 and 2–36. Base-2, -8, and -16 literals can be optionally prefixed with 0b/0B, 0o/0O, or 0x/0X,
> as with integer literals in code. Base 0 means to interpret exactly as a code literal, so that the
> actual base is 2, 8, 10, or 16, and so that int('010', 0) is not legal, while int('010') is, as
> well as int('010', 8).
Comment on lines +8 to +15
Copy link
Member

Choose a reason for hiding this comment

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

most of this isn't relevant as it requires an explicit base.

Suggested change
> If x is not a number or if base is given, then x must be a string, bytes, or bytearray instance
> representing an integer literal in radix base. Optionally, the literal can be preceded by + or -
> (with no space in between) and surrounded by whitespace. A base-n literal consists of the digits 0
> to n-1, with a to z (or A to Z) having values 10 to 35. The default base is 10. The allowed values
> are 0 and 2–36. Base-2, -8, and -16 literals can be optionally prefixed with 0b/0B, 0o/0O, or 0x/0X,
> as with integer literals in code. Base 0 means to interpret exactly as a code literal, so that the
> actual base is 2, 8, 10, or 16, and so that int('010', 0) is not legal, while int('010') is, as
> well as int('010', 8).
> If x is not a number ... then x must be a string, bytes, or bytearray instance
> representing an integer literal. Optionally, the literal can be preceded by + or -
> (with no space in between) and surrounded by whitespace. A base-10 literal consists of the digits 0
> to 9.

(notably, all the prefixes here are not valid)


All of this behaviour is exceptionally difficult for non-Python implementations to reproduce
accurately, so we should not try to do so.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not specifically, i've drawn an algorithm to come close; (original comment)

  1. trim all whitespace
  2. remove leading +
  3. if leading character is -, remove and remember
  4. remove leading zeroes (except the last one, if remaining string is all-zeroes)
  5. interpret string as an integer
  6. if - was encountered, negate the integer

I'm plenty sure this is how python does it, and i think at least a mention/help for the spec on how to start to parse the old values would help a ton.


## Proposal

In a future room version, we should enforce the letter of the spec and only allow power levels
as integers and reject events which try to define them as any other type. This removes all of the
associated headaches with string parsing.
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, which system are we hoping to enforce this within? If it's the auth rules then they might be complicated to represent, but if it's just evaluation-time parsing (ie: non-int is default power level) then that might be a bit easier to write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to modify the auth rules themselves here. I think an acceptable outcome is that parsing the power level content at the beginning of event auth altogether fails, therefore auth fails with it. This is what the Dendrite implementation does currently. We don't attempt to fall back to a default level, although if anyone has feelings on whether that approach would be better, I'd welcome opinions.

Copy link
Member

Choose a reason for hiding this comment

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

Failing event auth sounds ideal - can we get words to that effect in the MSC? 😇


## Potential issues

We can't avoid the string parsing behaviour altogether because we need to continue to do so for
existing room versions so that we do not break existing rooms. However, there are already documented
cases of this causing problems across implementations. For example, Synapse will accept `" +2 "` as
a power level but Dendrite will outright fail to parse it.
Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have a line here explicitly stating that: as a result, homeserver implementations may need to update their existing room version implementations to remain compatible with the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is my hope that the wording in #3688 will be sufficient to cover what implementations should do today and then this MSC can focus on what we do to plug the hole going forward.

Copy link
Member

Choose a reason for hiding this comment

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

(directionally, I think it's best to work on this MSC and that spec PR almost concurrently: the MSC can make reference to what the spec intended to say, as we know we'd accept a clarification about how stringy power levels work, and the spec PR can either continue working on inclusion or wait for this MSC to land and be assigned a room version)


## Alternatives

Event schema enforcement for all event types used in event auth could solve this problem and
more but is a significantly bigger task.

## Unstable prefix

An implementation exists in Dendrite using the `org.matrix.msc3667` room version identifier. The
experimental room version is based on room version 7, with the additional requirement that power
levels must be integers or the power level content will fail to unmarshal altogether.