element-hq / element-hq/synapse

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

Open
#11,031 0 comments 0 reactions 0 assignees View on GitHub
T-Enhancement
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#11031](https://github.com/matrix-org/synapse/issues/11031).

---

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](https://github.com/matrix-org/synapse/pull/11030); [discussion in #synapse-dev](https://matrix.to/#/!XaqDhxuTIlvldquJaV:matrix.org/$necZJEIYJFesBolvcdvN2MfxQfmdUH4P0_fjLahOb6E?via=matrix.org&via=vector.modular.im&via=envs.net)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.