Module callbacks should not fail open: exceptions should not equal acceptance

[reivilibre]

Currently, if a check_event_allowed callback raises an exception, the check is skipped and we fail open (we move on to the next callback if there is one, or we accept the event otherwise).

This behaviour is undesirable for a security feature; intuitively, a failure in a security system should not be ignored.

If we failed closed by default, you could still wrap your module callback in try-catch and return True on failure if you desired the fail open behaviour.

On the other hand, failing open means that there is no way to fail closed without causing issues with federation: when we receive an event over federation and the callback fails, neither 'accept the event' or 'permanently reject the event' seems like a good answer: a failure instead should probably mean 'I can't accept this right now, but in the future I might be able to'.

To summarise:

• true should accept
• false should successfully reject
• an exception should be a failure, but neither a rejection nor an acceptance.
  • it has been suggested that we wrap these in a new PluginFailedException

(as an aside, we also mentioned that module authors should not be using SynapseError in their modules except as a response to a module's own HTTP resource: modules should prefer to use a generic exception type and allow Synapse to convert it to a 500 if that's the correct thing to do)

context: the PR sparking this discussion; discussion in #synapse-dev

[babolivier]

Note that this isn't the case only for check_event_allowed but for all module callbacks.