From 58fbbe0f1db78d9dc91a319874dc8409e77cbf4c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 20 Oct 2017 23:37:22 +0100 Subject: [PATCH 1/3] Disallow capital letters in userids Factor out a common function for checking user ids and group ids, which forbids capitals. --- synapse/groups/groups_server.py | 17 ++++++----------- synapse/handlers/register.py | 10 ++++------ synapse/types.py | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py index fc4edb7f0..c359bfa72 100644 --- a/synapse/groups/groups_server.py +++ b/synapse/groups/groups_server.py @@ -13,14 +13,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -from twisted.internet import defer - -from synapse.api.errors import SynapseError -from synapse.types import UserID, get_domain_from_id, RoomID, GroupID - - import logging -import urllib + +from synapse import types +from synapse.api.errors import SynapseError +from synapse.types import GroupID, RoomID, UserID, get_domain_from_id +from twisted.internet import defer logger = logging.getLogger(__name__) @@ -793,10 +791,7 @@ def _validate_group_id(group_id): """ localpart = GroupID.from_string(group_id).localpart - if localpart.lower() != localpart: - raise SynapseError(400, "Group ID must be lower case") - - if urllib.quote(localpart.encode('utf-8')) != localpart: + if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, "Group ID can only contain characters a-z, 0-9, or '_-./'", diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 560fb3625..c7c091f43 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -15,7 +15,6 @@ """Contains functions for registering clients.""" import logging -import urllib from twisted.internet import defer @@ -23,6 +22,7 @@ from synapse.api.errors import ( AuthError, Codes, SynapseError, RegistrationError, InvalidCaptchaError ) from synapse.http.client import CaptchaServerHttpClient +from synapse import types from synapse.types import UserID from synapse.util.async import run_on_reactor from ._base import BaseHandler @@ -46,9 +46,7 @@ class RegistrationHandler(BaseHandler): @defer.inlineCallbacks def check_username(self, localpart, guest_access_token=None, assigned_user_id=None): - yield run_on_reactor() - - if urllib.quote(localpart.encode('utf-8')) != localpart: + if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, "User ID can only contain characters a-z, 0-9, or '_-./'", @@ -81,7 +79,7 @@ class RegistrationHandler(BaseHandler): "A different user ID has already been registered for this session", ) - yield self.check_user_id_not_appservice_exclusive(user_id) + self.check_user_id_not_appservice_exclusive(user_id) users = yield self.store.get_users_by_id_case_insensitive(user_id) if users: @@ -254,7 +252,7 @@ class RegistrationHandler(BaseHandler): """ Registers email_id as SAML2 Based Auth. """ - if urllib.quote(localpart) != localpart: + if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, "User ID must only contain characters which do not" diff --git a/synapse/types.py b/synapse/types.py index 37d5fa7f9..efa721273 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -12,6 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import string from synapse.api.errors import SynapseError @@ -161,6 +162,21 @@ class GroupID(DomainSpecificString): SIGIL = "+" +mxid_localpart_allowed_characters = set("_-./" + string.ascii_lowercase + string.digits) + + +def contains_invalid_mxid_characters(localpart): + """Check for characters not allowed in an mxid or groupid localpart + + Args: + localpart (basestring): the localpart to be checked + + Returns: + bool: True if there are any naughty characters + """ + return any(c not in mxid_localpart_allowed_characters for c in localpart) + + class StreamToken( namedtuple("Token", ( "room_key", From 29812c628ba924448719f5d2cfe7e05a5b1d0f45 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 20 Oct 2017 23:42:53 +0100 Subject: [PATCH 2/3] Allow = in mxids and groupids ... because the spec says we should. --- synapse/groups/groups_server.py | 2 +- synapse/handlers/register.py | 5 ++--- synapse/types.py | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py index c359bfa72..3599bfe9c 100644 --- a/synapse/groups/groups_server.py +++ b/synapse/groups/groups_server.py @@ -794,5 +794,5 @@ def _validate_group_id(group_id): if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, - "Group ID can only contain characters a-z, 0-9, or '_-./'", + "Group ID can only contain characters a-z, 0-9, or '=_-./'", ) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index c7c091f43..52aa9964d 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -49,7 +49,7 @@ class RegistrationHandler(BaseHandler): if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, - "User ID can only contain characters a-z, 0-9, or '_-./'", + "User ID can only contain characters a-z, 0-9, or '=_-./'", Codes.INVALID_USERNAME ) @@ -255,8 +255,7 @@ class RegistrationHandler(BaseHandler): if types.contains_invalid_mxid_characters(localpart): raise SynapseError( 400, - "User ID must only contain characters which do not" - " require URL encoding." + "User ID can only contain characters a-z, 0-9, or '=_-./'", ) user = UserID(localpart, self.hs.hostname) user_id = user.to_string() diff --git a/synapse/types.py b/synapse/types.py index efa721273..88eb818de 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -162,7 +162,7 @@ class GroupID(DomainSpecificString): SIGIL = "+" -mxid_localpart_allowed_characters = set("_-./" + string.ascii_lowercase + string.digits) +mxid_localpart_allowed_characters = set("_-./=" + string.ascii_lowercase + string.digits) def contains_invalid_mxid_characters(localpart): From 1135193dfde2a844d38dab4bd50a69658891abcb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 20 Oct 2017 23:51:07 +0100 Subject: [PATCH 3/3] Validate group ids when parsing May as well do it whenever we parse a Group ID. We check the sigil and basic structure here so it makes sense to check the grammar in the same place. --- synapse/groups/groups_server.py | 21 +++++---------------- synapse/types.py | 17 +++++++++++++++++ tests/test_types.py | 24 +++++++++++++++++++++++- 3 files changed, 45 insertions(+), 17 deletions(-) diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py index 3599bfe9c..23beb3187 100644 --- a/synapse/groups/groups_server.py +++ b/synapse/groups/groups_server.py @@ -15,7 +15,6 @@ import logging -from synapse import types from synapse.api.errors import SynapseError from synapse.types import GroupID, RoomID, UserID, get_domain_from_id from twisted.internet import defer @@ -696,9 +695,11 @@ class GroupsServerHandler(object): def create_group(self, group_id, user_id, content): group = yield self.check_group_is_ours(group_id) - _validate_group_id(group_id) - logger.info("Attempting to create group with ID: %r", group_id) + + # parsing the id into a GroupID validates it. + group_id_obj = GroupID.from_string(group_id) + if group: raise SynapseError(400, "Group already exists") @@ -708,7 +709,7 @@ class GroupsServerHandler(object): raise SynapseError( 403, "Only server admin can create group on this server", ) - localpart = GroupID.from_string(group_id).localpart + localpart = group_id_obj.localpart if not localpart.startswith(self.hs.config.group_creation_prefix): raise SynapseError( 400, @@ -784,15 +785,3 @@ def _parse_visibility_from_contents(content): is_public = True return is_public - - -def _validate_group_id(group_id): - """Validates the group ID is valid for creation on this home server - """ - localpart = GroupID.from_string(group_id).localpart - - if types.contains_invalid_mxid_characters(localpart): - raise SynapseError( - 400, - "Group ID can only contain characters a-z, 0-9, or '=_-./'", - ) diff --git a/synapse/types.py b/synapse/types.py index 88eb818de..5e3d1fc0b 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -161,6 +161,23 @@ class GroupID(DomainSpecificString): """Structure representing a group ID.""" SIGIL = "+" + @classmethod + def from_string(cls, s): + group_id = super(GroupID, cls).from_string(s) + if not group_id.localpart: + raise SynapseError( + 400, + "Group ID cannot be empty", + ) + + if contains_invalid_mxid_characters(group_id.localpart): + raise SynapseError( + 400, + "Group ID can only contain characters a-z, 0-9, or '=_-./'", + ) + + return group_id + mxid_localpart_allowed_characters = set("_-./=" + string.ascii_lowercase + string.digits) diff --git a/tests/test_types.py b/tests/test_types.py index 24d61dbe5..115def228 100644 --- a/tests/test_types.py +++ b/tests/test_types.py @@ -17,7 +17,7 @@ from tests import unittest from synapse.api.errors import SynapseError from synapse.server import HomeServer -from synapse.types import UserID, RoomAlias +from synapse.types import UserID, RoomAlias, GroupID mock_homeserver = HomeServer(hostname="my.domain") @@ -60,3 +60,25 @@ class RoomAliasTestCase(unittest.TestCase): room = RoomAlias("channel", "my.domain") self.assertEquals(room.to_string(), "#channel:my.domain") + + +class GroupIDTestCase(unittest.TestCase): + def test_parse(self): + group_id = GroupID.from_string("+group/=_-.123:my.domain") + self.assertEqual("group/=_-.123", group_id.localpart) + self.assertEqual("my.domain", group_id.domain) + + def test_validate(self): + bad_ids = [ + "$badsigil:domain", + "+:empty", + ] + [ + "+group" + c + ":domain" for c in "A%?æ£" + ] + for id_string in bad_ids: + try: + GroupID.from_string(id_string) + self.fail("Parsing '%s' should raise exception" % id_string) + except SynapseError as exc: + self.assertEqual(400, exc.code) + self.assertEqual("M_UNKNOWN", exc.errcode)