From b872c7b1b43431b8933e2afd2f226aa34ad81a0f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 28 Jan 2019 17:00:14 +0000 Subject: [PATCH 1/5] Split up event validation between event and builder The validator was being run on the EventBuilder objects, and so the validator only checked a subset of fields. With the upcoming EventBuilder refactor even fewer fields will be there to validate. To get around this we split the validation into those that can be run against an EventBuilder and those run against a fully fledged event. --- synapse/events/validator.py | 77 ++++++++++++++++++++++++---------- synapse/handlers/federation.py | 7 +++- synapse/handlers/message.py | 4 +- 3 files changed, 63 insertions(+), 25 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index cf184748a1..55d44d093d 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -24,14 +24,13 @@ class EventValidator(object): def validate(self, event): EventID.from_string(event.event_id) - RoomID.from_string(event.room_id) required = [ - # "auth_events", + "auth_events", "content", - # "hashes", + "hashes", "origin", - # "prev_events", + "prev_events", "sender", "type", ] @@ -43,31 +42,19 @@ class EventValidator(object): # Check that the following keys have string values strings = [ "origin", - "sender", - "type", ] - if hasattr(event, "state_key"): - strings.append("state_key") - for s in strings: if not isinstance(getattr(event, s), string_types): raise SynapseError(400, "Not '%s' a string type" % (s,)) - if event.type == EventTypes.Member: - if "membership" not in event.content: - raise SynapseError(400, "Content has not membership key") - - if event.content["membership"] not in Membership.LIST: - raise SynapseError(400, "Invalid membership key") - - # Check that the following keys have dictionary values - # TODO - - # Check that the following keys have the correct format for DAGs - # TODO - def validate_new(self, event): + """Validates the event has roughly the right format + + Args: + event (FrozenEvent) + """ + self.validate_builder(event) self.validate(event) UserID.from_string(event.sender) @@ -86,6 +73,52 @@ class EventValidator(object): elif event.type == EventTypes.Name: self._ensure_strings(event.content, ["name"]) + def validate_builder(self, event): + """Validates that the builder/event has roughly the right format. Only + checks values that we expect a proto event to have, rather than all the + fields an event would have + + Args: + event (EventBuilder|FrozenEvent) + """ + + strings = [ + "room_id", + "sender", + "type", + ] + + if hasattr(event, "state_key"): + strings.append("state_key") + + for s in strings: + if not isinstance(getattr(event, s), string_types): + raise SynapseError(400, "Not '%s' a string type" % (s,)) + + RoomID.from_string(event.room_id) + UserID.from_string(event.sender) + + if event.type == EventTypes.Message: + strings = [ + "body", + "msgtype", + ] + + self._ensure_strings(event.content, strings) + + elif event.type == EventTypes.Topic: + self._ensure_strings(event.content, ["topic"]) + + elif event.type == EventTypes.Name: + self._ensure_strings(event.content, ["name"]) + + elif event.type == EventTypes.Member: + if "membership" not in event.content: + raise SynapseError(400, "Content has not membership key") + + if event.content["membership"] not in Membership.LIST: + raise SynapseError(400, "Invalid membership key") + def _ensure_strings(self, d, keys): for s in keys: if s not in d: diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index a4b771049c..13333818ae 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2278,7 +2278,7 @@ class FederationHandler(BaseHandler): room_version = yield self.store.get_room_version(room_id) builder = self.event_builder_factory.new(room_version, event_dict) - EventValidator().validate_new(builder) + EventValidator().validate_builder(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder ) @@ -2287,6 +2287,8 @@ class FederationHandler(BaseHandler): room_version, event_dict, event, context ) + EventValidator().validate_new(event) + try: yield self.auth.check_from_context(event, context) except AuthError as e: @@ -2372,10 +2374,11 @@ class FederationHandler(BaseHandler): # auth check code will explode appropriately. builder = self.event_builder_factory.new(room_version, event_dict) - EventValidator().validate_new(builder) + EventValidator().validate_builder(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, ) + EventValidator().validate_new(event) defer.returnValue((event, context)) @defer.inlineCallbacks diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 7aaa4fba33..d2aab25111 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -288,7 +288,7 @@ class EventCreationHandler(object): builder = self.event_builder_factory.new(room_version, event_dict) - self.validator.validate_new(builder) + self.validator.validate_builder(builder) if builder.type == EventTypes.Member: membership = builder.content.get("membership", None) @@ -326,6 +326,8 @@ class EventCreationHandler(object): prev_events_and_hashes=prev_events_and_hashes, ) + self.validator.validate_new(event) + defer.returnValue((event, context)) def _is_exempt_from_privacy_policy(self, builder, requester): From 1977a9b00699fba0757d723d5ca7da38cc8f9af9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 28 Jan 2019 17:05:04 +0000 Subject: [PATCH 2/5] Newsfile --- changelog.d/4494.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4494.misc diff --git a/changelog.d/4494.misc b/changelog.d/4494.misc new file mode 100644 index 0000000000..43f8963614 --- /dev/null +++ b/changelog.d/4494.misc @@ -0,0 +1 @@ +Add infrastructure to support different event formats From 28efc8072395e84cc747425bf8ee6c5b4401b55d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 10:34:49 +0000 Subject: [PATCH 3/5] Fold validate into validate_new --- synapse/events/validator.py | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 55d44d093d..5c799493fb 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -21,8 +21,14 @@ from synapse.types import EventID, RoomID, UserID class EventValidator(object): + def validate_new(self, event): + """Validates the event has roughly the right format + + Args: + event (FrozenEvent) + """ + self.validate_builder(event) - def validate(self, event): EventID.from_string(event.event_id) required = [ @@ -40,32 +46,21 @@ class EventValidator(object): raise SynapseError(400, "Event does not have key %s" % (k,)) # Check that the following keys have string values - strings = [ + event_strings = [ "origin", ] - for s in strings: + for s in event_strings: if not isinstance(getattr(event, s), string_types): raise SynapseError(400, "Not '%s' a string type" % (s,)) - def validate_new(self, event): - """Validates the event has roughly the right format - - Args: - event (FrozenEvent) - """ - self.validate_builder(event) - self.validate(event) - - UserID.from_string(event.sender) - if event.type == EventTypes.Message: - strings = [ + content_strings = [ "body", "msgtype", ] - self._ensure_strings(event.content, strings) + self._ensure_strings(event.content, content_strings) elif event.type == EventTypes.Topic: self._ensure_strings(event.content, ["topic"]) From 9fa3c6ffa313bba36f24719ab78af735ff0c14cd Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 10:36:46 +0000 Subject: [PATCH 4/5] Fix up error messages --- synapse/events/validator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 5c799493fb..4fa03501f8 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -52,7 +52,7 @@ class EventValidator(object): for s in event_strings: if not isinstance(getattr(event, s), string_types): - raise SynapseError(400, "Not '%s' a string type" % (s,)) + raise SynapseError(400, "'%s' not a string type" % (s,)) if event.type == EventTypes.Message: content_strings = [ @@ -119,4 +119,4 @@ class EventValidator(object): if s not in d: raise SynapseError(400, "'%s' not in content" % (s,)) if not isinstance(d[s], string_types): - raise SynapseError(400, "Not '%s' a string type" % (s,)) + raise SynapseError(400, "'%s' not a string type" % (s,)) From 40638ae7f50428454edd9961450f5dfb3cbd405a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 10:37:40 +0000 Subject: [PATCH 5/5] Remove duplicate checks --- synapse/events/validator.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 4fa03501f8..c53bf44e51 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -54,20 +54,6 @@ class EventValidator(object): if not isinstance(getattr(event, s), string_types): raise SynapseError(400, "'%s' not a string type" % (s,)) - if event.type == EventTypes.Message: - content_strings = [ - "body", - "msgtype", - ] - - self._ensure_strings(event.content, content_strings) - - elif event.type == EventTypes.Topic: - self._ensure_strings(event.content, ["topic"]) - - elif event.type == EventTypes.Name: - self._ensure_strings(event.content, ["name"]) - def validate_builder(self, event): """Validates that the builder/event has roughly the right format. Only checks values that we expect a proto event to have, rather than all the