Make check_event_allowed module API callback not fail open (accept events) when an exception is raised (#11033)

This commit is contained in:
reivilibre 2021-11-01 15:45:56 +00:00 committed by GitHub
parent 66bdca3e31
commit 69ab3dddbc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 24 additions and 17 deletions

1
changelog.d/11033.bugfix Normal file
View File

@ -0,0 +1 @@
Do not accept events if a third-party rule module API callback raises an exception.

View File

@ -43,6 +43,14 @@ event with new data by returning the new event's data as a dictionary. In order
that, it is recommended the module calls `event.get_dict()` to get the current event as a
dictionary, and modify the returned dictionary accordingly.
If `check_event_allowed` raises an exception, the module is assumed to have failed.
The event will not be accepted but is not treated as explicitly rejected, either.
An HTTP request causing the module check will likely result in a 500 Internal
Server Error.
When the boolean returned by the module is `False`, the event is rejected.
(Module developers should not use exceptions for rejection.)
Note that replacing the event only works for events sent by local users, not for events
received over federation.

View File

@ -596,3 +596,10 @@ class ShadowBanError(Exception):
This should be caught and a proper "fake" success response sent to the user.
"""
class ModuleFailedException(Exception):
"""
Raised when a module API callback fails, for example because it raised an
exception.
"""

View File

@ -14,7 +14,7 @@
import logging
from typing import TYPE_CHECKING, Any, Awaitable, Callable, List, Optional, Tuple
from synapse.api.errors import SynapseError
from synapse.api.errors import ModuleFailedException, SynapseError
from synapse.events import EventBase
from synapse.events.snapshot import EventContext
from synapse.types import Requester, StateMap
@ -233,9 +233,10 @@ class ThirdPartyEventRules:
# This module callback needs a rework so that hacks such as
# this one are not necessary.
raise e
except Exception as e:
logger.warning("Failed to run module API callback %s: %s", callback, e)
continue
except Exception:
raise ModuleFailedException(
"Failed to run `check_event_allowed` module API callback"
)
# Return if the event shouldn't be allowed or if the module came up with a
# replacement dict for the event.

View File

@ -216,19 +216,9 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase):
{"x": "x"},
access_token=self.tok,
)
# check_event_allowed has some error handling, so it shouldn't 500 just because a
# module did something bad.
self.assertEqual(channel.code, 200, channel.result)
event_id = channel.json_body["event_id"]
channel = self.make_request(
"GET",
"/_matrix/client/r0/rooms/%s/event/%s" % (self.room_id, event_id),
access_token=self.tok,
)
self.assertEqual(channel.code, 200, channel.result)
ev = channel.json_body
self.assertEqual(ev["content"]["x"], "x")
# Because check_event_allowed raises an exception, it leads to a
# 500 Internal Server Error
self.assertEqual(channel.code, 500, channel.result)
def test_modify_event(self):
"""The module can return a modified version of the event"""