From 9c9c3aba82a07e7993f2c8c27fa02d5567a8bb4b Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 13 Apr 2022 16:13:21 +0100 Subject: [PATCH 1/3] Don't swallow `CancelledError`s when running module callbacks Update module callbacks with read semantics to not suppress `CancelledError`s. Module callbacks with write semantics have been left alone, since we don't expect cancellation to work correctly for those code paths. Signed-off-by: Sean Quah --- synapse/events/presence_router.py | 6 ++++++ synapse/events/third_party_rules.py | 12 ++++++++++++ synapse/handlers/auth.py | 11 +++++++++++ 3 files changed, 29 insertions(+) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index a58f313e8b1c..c39646e91d39 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -25,6 +25,8 @@ Union, ) +from twisted.internet.defer import CancelledError + from synapse.api.presence import UserPresenceState from synapse.util.async_helpers import maybe_awaitable @@ -148,6 +150,8 @@ async def get_users_for_states( for callback in self._get_users_for_states_callbacks: try: result = await callback(state_updates) + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) continue @@ -200,6 +204,8 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], str]: for callback in self._get_interested_users_callbacks: try: result = await callback(user_id) + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) continue diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index ef68e2028220..3863bebd1bca 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -14,6 +14,8 @@ import logging from typing import TYPE_CHECKING, Any, Awaitable, Callable, List, Optional, Tuple +from twisted.internet.defer import CancelledError + from synapse.api.errors import ModuleFailedException, SynapseError from synapse.events import EventBase from synapse.events.snapshot import EventContext @@ -264,6 +266,8 @@ async def check_event_allowed( for callback in self._check_event_allowed_callbacks: try: res, replacement_data = await callback(event, state_events) + except CancelledError: + raise except SynapseError as e: # FIXME: Being able to throw SynapseErrors is relied upon by # some modules. PR #10386 accidentally broke this ability. @@ -335,6 +339,8 @@ async def check_threepid_can_be_invited( try: if await callback(medium, address, state_events) is False: return False + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) @@ -363,6 +369,8 @@ async def check_visibility_can_be_modified( try: if await callback(room_id, state_events, new_visibility) is False: return False + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) @@ -402,6 +410,8 @@ async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: try: if await callback(user_id, room_id) is False: return False + except CancelledError: + raise except Exception as e: logger.exception( "Failed to run module API callback %s: %s", callback, e @@ -424,6 +434,8 @@ async def check_can_deactivate_user( try: if await callback(user_id, by_admin) is False: return False + except CancelledError: + raise except Exception as e: logger.exception( "Failed to run module API callback %s: %s", callback, e diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 22678d486d73..bec07f065f6a 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -41,6 +41,7 @@ import unpaddedbase64 from pymacaroons.exceptions import MacaroonVerificationFailedException +from twisted.internet.defer import CancelledError from twisted.web.server import Request from synapse.api.constants import LoginType @@ -2203,6 +2204,8 @@ async def check_auth( for callback in self.auth_checker_callbacks[login_type]: try: result = await callback(username, login_type, login_dict) + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) continue @@ -2264,6 +2267,8 @@ async def check_3pid_auth( for callback in self.check_3pid_auth_callbacks: try: result = await callback(medium, address, password) + except CancelledError: + raise except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) continue @@ -2359,6 +2364,8 @@ async def get_username_for_registration( callback, res, ) + except CancelledError: + raise except Exception as e: logger.error( "Module raised an exception in get_username_for_registration: %s", @@ -2402,6 +2409,8 @@ async def get_displayname_for_registration( callback, res, ) + except CancelledError: + raise except Exception as e: logger.error( "Module raised an exception in get_displayname_for_registration: %s", @@ -2443,6 +2452,8 @@ async def is_3pid_allowed( callback, res, ) + except CancelledError: + raise except Exception as e: logger.error("Module raised an exception in is_3pid_allowed: %s", e) raise SynapseError(code=500, msg="Internal Server Error") From 275d8597e7fcfa7e597cfd5e22d7c8f1a43b21de Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 13 Apr 2022 19:47:59 +0100 Subject: [PATCH 2/3] Protect module callbacks with read semantics against cancellation The `on_*` callbacks have been left alone, since they are presumed to be run on code paths that aren't cancellation-friendly. Signed-off-by: Sean Quah --- synapse/events/presence_router.py | 6 ++--- synapse/events/spamcheck.py | 36 +++++++++++++++++++--------- synapse/events/third_party_rules.py | 24 ++++++++++++++----- synapse/handlers/account_validity.py | 3 ++- synapse/handlers/auth.py | 14 ++++++----- 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index c39646e91d39..b636212da9ff 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -28,7 +28,7 @@ from twisted.internet.defer import CancelledError from synapse.api.presence import UserPresenceState -from synapse.util.async_helpers import maybe_awaitable +from synapse.util.async_helpers import delay_cancellation, maybe_awaitable if TYPE_CHECKING: from synapse.server import HomeServer @@ -149,7 +149,7 @@ async def get_users_for_states( # run all the callbacks for get_users_for_states and combine the results for callback in self._get_users_for_states_callbacks: try: - result = await callback(state_updates) + result = await delay_cancellation(callback(state_updates)) except CancelledError: raise except Exception as e: @@ -203,7 +203,7 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], str]: # run all the callbacks for get_interested_users and combine the results for callback in self._get_interested_users_callbacks: try: - result = await callback(user_id) + result = await delay_cancellation(callback(user_id)) except CancelledError: raise except Exception as e: diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index cd80fcf9d13a..3b6795d40f6b 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -31,7 +31,7 @@ from synapse.rest.media.v1.media_storage import ReadableFileWrapper from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import RoomAlias, UserProfile -from synapse.util.async_helpers import maybe_awaitable +from synapse.util.async_helpers import delay_cancellation, maybe_awaitable if TYPE_CHECKING: import synapse.events @@ -255,7 +255,7 @@ async def check_event_for_spam( will be used as the error message returned to the user. """ for callback in self._check_event_for_spam_callbacks: - res: Union[bool, str] = await callback(event) + res: Union[bool, str] = await delay_cancellation(callback(event)) if res: return res @@ -276,7 +276,10 @@ async def user_may_join_room( Whether the user may join the room """ for callback in self._user_may_join_room_callbacks: - if await callback(user_id, room_id, is_invited) is False: + may_join_room = await delay_cancellation( + callback(user_id, room_id, is_invited) + ) + if may_join_room is False: return False return True @@ -297,7 +300,10 @@ async def user_may_invite( True if the user may send an invite, otherwise False """ for callback in self._user_may_invite_callbacks: - if await callback(inviter_userid, invitee_userid, room_id) is False: + may_invite = await delay_cancellation( + callback(inviter_userid, invitee_userid, room_id) + ) + if may_invite is False: return False return True @@ -322,7 +328,10 @@ async def user_may_send_3pid_invite( True if the user may send the invite, otherwise False """ for callback in self._user_may_send_3pid_invite_callbacks: - if await callback(inviter_userid, medium, address, room_id) is False: + may_send_3pid_invite = await delay_cancellation( + callback(inviter_userid, medium, address, room_id) + ) + if may_send_3pid_invite is False: return False return True @@ -339,7 +348,8 @@ async def user_may_create_room(self, userid: str) -> bool: True if the user may create a room, otherwise False """ for callback in self._user_may_create_room_callbacks: - if await callback(userid) is False: + may_create_room = await delay_cancellation(callback(userid)) + if may_create_room is False: return False return True @@ -359,7 +369,10 @@ async def user_may_create_room_alias( True if the user may create a room alias, otherwise False """ for callback in self._user_may_create_room_alias_callbacks: - if await callback(userid, room_alias) is False: + may_create_room_alias = await delay_cancellation( + callback(userid, room_alias) + ) + if may_create_room_alias is False: return False return True @@ -377,7 +390,8 @@ async def user_may_publish_room(self, userid: str, room_id: str) -> bool: True if the user may publish the room, otherwise False """ for callback in self._user_may_publish_room_callbacks: - if await callback(userid, room_id) is False: + may_publish_room = await delay_cancellation(callback(userid, room_id)) + if may_publish_room is False: return False return True @@ -400,7 +414,7 @@ async def check_username_for_spam(self, user_profile: UserProfile) -> bool: for callback in self._check_username_for_spam_callbacks: # Make a copy of the user profile object to ensure the spam checker cannot # modify it. - if await callback(user_profile.copy()): + if await delay_cancellation(callback(user_profile.copy())): return True return False @@ -428,7 +442,7 @@ async def check_registration_for_spam( """ for callback in self._check_registration_for_spam_callbacks: - behaviour = await ( + behaviour = await delay_cancellation( callback(email_threepid, username, request_info, auth_provider_id) ) assert isinstance(behaviour, RegistrationBehaviour) @@ -472,7 +486,7 @@ async def check_media_file_for_spam( """ for callback in self._check_media_file_for_spam_callbacks: - spam = await callback(file_wrapper, file_info) + spam = await delay_cancellation(callback(file_wrapper, file_info)) if spam: return True diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 3863bebd1bca..9f4ff9799c00 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -21,7 +21,7 @@ from synapse.events.snapshot import EventContext from synapse.storage.roommember import ProfileInfo from synapse.types import Requester, StateMap -from synapse.util.async_helpers import maybe_awaitable +from synapse.util.async_helpers import delay_cancellation, maybe_awaitable if TYPE_CHECKING: from synapse.server import HomeServer @@ -265,7 +265,9 @@ async def check_event_allowed( for callback in self._check_event_allowed_callbacks: try: - res, replacement_data = await callback(event, state_events) + res, replacement_data = await delay_cancellation( + callback(event, state_events) + ) except CancelledError: raise except SynapseError as e: @@ -337,7 +339,10 @@ async def check_threepid_can_be_invited( for callback in self._check_threepid_can_be_invited_callbacks: try: - if await callback(medium, address, state_events) is False: + threepid_can_be_invited = await delay_cancellation( + callback(medium, address, state_events) + ) + if threepid_can_be_invited is False: return False except CancelledError: raise @@ -367,7 +372,10 @@ async def check_visibility_can_be_modified( for callback in self._check_visibility_can_be_modified_callbacks: try: - if await callback(room_id, state_events, new_visibility) is False: + visibility_can_be_modified = await delay_cancellation( + callback(room_id, state_events, new_visibility) + ) + if visibility_can_be_modified is False: return False except CancelledError: raise @@ -408,7 +416,8 @@ async def check_can_shutdown_room(self, user_id: str, room_id: str) -> bool: """ for callback in self._check_can_shutdown_room_callbacks: try: - if await callback(user_id, room_id) is False: + can_shutdown_room = await delay_cancellation(callback(user_id, room_id)) + if can_shutdown_room is False: return False except CancelledError: raise @@ -432,7 +441,10 @@ async def check_can_deactivate_user( """ for callback in self._check_can_deactivate_user_callbacks: try: - if await callback(user_id, by_admin) is False: + can_deactivate_user = await delay_cancellation( + callback(user_id, by_admin) + ) + if can_deactivate_user is False: return False except CancelledError: raise diff --git a/synapse/handlers/account_validity.py b/synapse/handlers/account_validity.py index 05a138410e25..33e45e3a1136 100644 --- a/synapse/handlers/account_validity.py +++ b/synapse/handlers/account_validity.py @@ -23,6 +23,7 @@ from synapse.metrics.background_process_metrics import wrap_as_background_process from synapse.types import UserID from synapse.util import stringutils +from synapse.util.async_helpers import delay_cancellation if TYPE_CHECKING: from synapse.server import HomeServer @@ -150,7 +151,7 @@ async def is_user_expired(self, user_id: str) -> bool: Whether the user has expired. """ for callback in self._is_user_expired_callbacks: - expired = await callback(user_id) + expired = await delay_cancellation(callback(user_id)) if expired is not None: return expired diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index bec07f065f6a..e2c87f7503ea 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -68,7 +68,7 @@ from synapse.storage.roommember import ProfileInfo from synapse.types import JsonDict, Requester, UserID from synapse.util import stringutils as stringutils -from synapse.util.async_helpers import maybe_awaitable +from synapse.util.async_helpers import delay_cancellation, maybe_awaitable from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.stringutils import base62_encode @@ -2203,7 +2203,9 @@ async def check_auth( # other than None (i.e. until a callback returns a success) for callback in self.auth_checker_callbacks[login_type]: try: - result = await callback(username, login_type, login_dict) + result = await delay_cancellation( + callback(username, login_type, login_dict) + ) except CancelledError: raise except Exception as e: @@ -2266,7 +2268,7 @@ async def check_3pid_auth( for callback in self.check_3pid_auth_callbacks: try: - result = await callback(medium, address, password) + result = await delay_cancellation(callback(medium, address, password)) except CancelledError: raise except Exception as e: @@ -2350,7 +2352,7 @@ async def get_username_for_registration( """ for callback in self.get_username_for_registration_callbacks: try: - res = await callback(uia_results, params) + res = await delay_cancellation(callback(uia_results, params)) if isinstance(res, str): return res @@ -2395,7 +2397,7 @@ async def get_displayname_for_registration( """ for callback in self.get_displayname_for_registration_callbacks: try: - res = await callback(uia_results, params) + res = await delay_cancellation(callback(uia_results, params)) if isinstance(res, str): return res @@ -2438,7 +2440,7 @@ async def is_3pid_allowed( """ for callback in self.is_3pid_allowed_callbacks: try: - res = await callback(medium, address, registration) + res = await delay_cancellation(callback(medium, address, registration)) if res is False: return res From 1294ada207444e957370b3fff38438f7f578d620 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 13 Apr 2022 19:51:45 +0100 Subject: [PATCH 3/3] Add newsfile --- changelog.d/12568.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12568.misc diff --git a/changelog.d/12568.misc b/changelog.d/12568.misc new file mode 100644 index 000000000000..f64dc67c4f9a --- /dev/null +++ b/changelog.d/12568.misc @@ -0,0 +1 @@ +Protect module callbacks with read semantics against cancellation.