event.freeze() causes subtle downstream bugs that makes type comparisons brittle

[MadLittleMods]

Spawning from reviewing #18066 by @blackmad which has the original investigation on this,

Normally, when usingEventBase, you would expect to be able to do naive comparisons like this isinstance(event.content.get("foo"), list) or isinstance(event.content.get("foo"), dict). But if event.freeze() was run beforehand, we actually need to take into account (list, tuple) and (dict, immutabledict).

What makes this even more subtle is that event.freeze() is only run when Synapse modules are configured.

It would be nice to for the type system, linting or anything to be able to catch this mistake. Or refactor things so that event.freeze() doesn't have such big downstream effects (perhaps we just make a copy that the Synapse module can do whatever it wants with).

Related issues:

• Fix overly restrictive typeguards in get_rooms_that_allow_join #18066
• check_event_allowed callback from the module API cause a TypeError #18101

Here is a list of places in the codebase where this matters (compiled from searching isinstance\(.+list and isinstance\(.+dict and manually inspecting if they're interacting with EventBase):

Places we already take this into account ✅

It looks like we already take this into account in a few places:

isinstance\(.+list:

• synapse/synapse/events/auto_accept_invites.py

  Line 160 in 95a85b1

  assert isinstance(dm_rooms_for_user, (tuple, list))

• synapse/synapse/handlers/message.py

  Line 1780 in 95a85b1

  if not isinstance(alt_aliases, (list, tuple)):

• synapse/synapse/handlers/message.py

  Line 1789 in 95a85b1

  if not isinstance(original_alt_aliases, (list, tuple)):

• synapse/synapse/push/bulk_push_rule_evaluator.py

  Line 555 in 95a85b1

  elif isinstance(value, (list, tuple)):

• synapse/synapse/storage/controllers/state.py

  Line 939 in 95a85b1

  if not isinstance(deny, (list, tuple)):

• synapse/synapse/storage/controllers/state.py

  Line 947 in 95a85b1

  if not isinstance(allow, (list, tuple)):

isinstance\(.+dict):

• None

Places we still need to update ❌

But there are also other spots we also need to update:

isinstance\(.+list:

• synapse/synapse/handlers/event_auth.py

  Line 329 in 95a85b1

  if not isinstance(allow_list, list):

• synapse/synapse/handlers/room_summary.py

  Line 958 in 95a85b1

  if not via or not isinstance(via, list):

• synapse/synapse/handlers/sliding_sync/__init__.py

  Line 821 in 95a85b1

  if not isinstance(invite_state, list):

• synapse/synapse/handlers/sliding_sync/__init__.py

  Line 827 in 95a85b1

  if not isinstance(knock_state, list):

• synapse/synapse/handlers/sliding_sync/room_lists.py

  Line 1440 in 95a85b1

  if isinstance(raw_stripped_state_events, list):

• synapse/synapse/push/bulk_push_rule_evaluator.py

  Line 571 in 95a85b1

  if markup is not None and isinstance(markup, list):

• synapse/synapse/push/push_tools.py

  Line 78 in 95a85b1

  if isinstance(invite_room_state, list):

• synapse/synapse/push/push_tools.py

  Line 82 in 95a85b1

  if isinstance(knock_room_state, list):

• synapse/synapse/storage/databases/main/events.py

  Line 2067 in 95a85b1

  if isinstance(unsigned_stripped_state_events, list):

isinstance\(.+dict:

• synapse/synapse/handlers/event_auth.py

  Line 335 in 95a85b1

  if not isinstance(allow, dict):

• synapse/synapse/events/utils.py

  Line 864 in 95a85b1

  if isinstance(raw_stripped_event, dict):

• synapse/synapse/events/utils.py

  Line 874 in 95a85b1

  and isinstance(content, dict)

• synapse/synapse/handlers/event_auth.py

  Line 335 in 95a85b1

  if not isinstance(allow, dict):

• synapse/synapse/handlers/room.py

  Line 557 in 95a85b1

  if not isinstance(event_power_levels, dict):

• synapse/synapse/handlers/room.py

  Line 576 in 95a85b1

  if not isinstance(user_power_levels, dict):

• synapse/synapse/push/bulk_push_rule_evaluator.py

  Line 574 in 95a85b1

  if not isinstance(rep, dict):

[MadLittleMods]

I've just had a thought on how we could solve all of the cases in #18117 pretty elegantly without having to muck about in the downstream code,

Instead of calling unfreeze at the spot we're trying to use it here, we could unfreeze after we're done with the frozen event in the third party module callbacks (where we called freeze in the first place).

(unfreeze after we run all of the callbacks)

synapse/synapse/module_api/callbacks/third_party_event_rules_callbacks.py

Lines 291 to 316 in a0b7047

	# Ensure that the event is frozen, to make sure that the module is not tempted
	# to try to modify it. Any attempt to modify it at this point will invalidate
	# the hashes and signatures.
	event.freeze()
	for callback in self._check_event_allowed_callbacks:
	try:
	res, replacement_data = await delay_cancellation(
	callback(event, state_events)
	)
	except CancelledError:
	raise
	except SynapseError as e:
	# FIXME: Being able to throw SynapseErrors is relied upon by
	# some modules. PR https://github.com/matrix-org/synapse/pull/10386
	# accidentally broke this ability.
	# That said, we aren't keen on exposing this implementation detail
	# to modules and we should one day have a proper way to do what
	# is wanted.
	# This module callback needs a rework so that hacks such as
	# this one are not necessary.
	raise e
	except Exception:
	raise ModuleFailedException(
	"Failed to run `check_event_allowed` module API callback"
	)

-- @MadLittleMods, #18103 (comment)