Limit UserIds to a length that fits in a state key #5198
Conversation
Signed-off-by: Reid Anderson <[email protected]>
There was a problem hiding this comment.
Thanks very much for this - it looks great!
The only thing is that https://matrix.org/docs/spec/appendices.html#user-identifiers says that the maximum length for a userid is 255 chars. I don't remember the exact reason, but we should probably follow the spec.
The other thing is that it would probably be best to factor out a constant. We already have a MAX_ALIAS_LENGTH in synapse.api.constants - I suggest you add a MAX_USERID_LENGTH next to it.
synapse/handlers/register.py
Outdated
| @@ -123,10 +126,12 @@ def check_username(self, localpart, guest_access_token=None, | |||
|
|
|||
| self.check_user_id_not_appservice_exclusive(user_id) | |||
|
|
|||
| if len(user_id) > 256: | |||
| if len(user_id) > MAX_ALIAS_LENGTH: | |||
There was a problem hiding this comment.
this should be MAX_USERID_LENGTH
There was a problem hiding this comment.
For reference, this looks fixed on master now synapse/handlers/register.py#L104
|
@richvdh Thanks for the crazy quick reviews. Should be updated and I've tested it out against riot/local synapse. |
Codecov Report
@@ Coverage Diff @@
## develop #5198 +/- ##
===========================================
- Coverage 62.17% 62.15% -0.02%
===========================================
Files 336 336
Lines 34716 34719 +3
Branches 5690 5691 +1
===========================================
- Hits 21583 21581 -2
- Misses 11598 11600 +2
- Partials 1535 1538 +3 |
Pull Request Checklist
Hello, this is my first PR that I've made against anything Matrix related. So definitely let me know if I've gone wrong anywhere, more than happy to make some changes (or if this is the wrong place entirely for the validation).
This should fix #5057, and require that user ids be no more than 256 characters.
And based off the contributing doc, it sounds like this goes here:
Signed-off-by: Reid Anderson