Add support for MSC4293 - Redact on Kick/Ban #18540

H-Shay · 2025-06-12T00:57:55Z

Adds support for MSC4293

tulir

Does synapse already pass through the field from /ban requests to the member event?

synapse/config/experimental.py

H-Shay · 2025-06-12T23:13:41Z

Does synapse already pass through the field from /ban requests to the member event?

it does not, I have added support for that as well, missed it the first go around.

anoadragon453

Not a complete review, as I still need to look at tests and the DB updates. But a solid start!

Also be aware that I typically advise people to aim for writing Complement tests when implementing unstable MSCs, such that other homeserver implementations can test against it when it comes time for them to implement the feature.

You don't need to do it here as you've already written a lot of unit tests, but for future reference.

synapse/handlers/federation_event.py

synapse/storage/databases/main/events.py

tests/rest/client/test_rooms.py

anoadragon453 · 2025-06-13T18:02:38Z

synapse/handlers/federation_event.py

@@ -2065,6 +2068,42 @@ async def _check_for_soft_fail(
            soft_failed_event_counter.inc()
            event.internal_metadata.soft_failed = True

+        if self._config.experimental.msc4293_enabled:


Since we're not actually setting internal_metadata.soft_failed in the msc4293_enabled-related code, I'm not sure if it makes sense for the checks to be in this function.

We might even still hit _persist_events_and_state_updates code later on, event if an event is soft failed(?).

My rationale for putting this check here was that this check only applies to events coming in over federation - if a request to send a message into a room the user is banned from comes over the cs api it will just be denied - thus we don't need to worry about redacting it. In this case, however, since the request comes over federation the event is soft-failed but still created, so we need to redact it. I chose this particular location because I was looking for a place where we had already pulled and calculated the state for event, as I was trying to avoid adding database calls if at all possible. If that's not a strong enough rationale for adding it here I can find another place!

I see, that is very handy. My hangup is that the logic we're doing for redactions here doesn't result in any soft fails occurring.

One way we could refactor this is to take all of the auth DAG calculation out of _check_for_soft_fail and put that in a _get_current_auth_events function. Then pass the result of that to _check_for_soft_fail and, i.e. _check_for_redacted_sender.

H-Shay · 2025-06-16T23:22:54Z

Build errors are odd, I wonder if they are related to #18559 , as the errors mention base64:

Building editable for matrix-synapse (pyproject.toml) did not run successfully.
    │ exit code: 1
    ╰─> [48 lines of output]
        A setup.py file already exists. Using it.
        running build_ext
        running build_rust
        error: failed to parse lock file at: /home/runner/work/synapse/synapse/Cargo.lock
        
        Caused by:
          package `base64` is specified twice in the lockfile
        error: `cargo metadata --manifest-path rust/Cargo.toml --format-version 1` failed with code 101

Otherwise I am not sure what I did to cause these

erikjohnston · 2025-06-17T09:38:01Z

Sorry, I broke develop :(

This PR should fix it #18561

turt2live

drive-by SCT review to meet implementation requirements on the MSC. Overall, looks great - thank you!

The major detail would be removal of the event type filters, but that looks trivial enough to consider the MSC implemented regardless.

synapse/handlers/federation_event.py

synapse/storage/databases/main/events.py

H-Shay · 2025-06-23T21:23:17Z

@anoadragon453 just checking that this is still on your radar

erikjohnston · 2025-06-24T14:08:18Z

changelog.d/18540.feature

I'm wondering if this is the right architecture for dealing with such redactions. Pulling out all events sent by the user has a few potential issues: a) if there are a lot of events it might take a lot of time, and b) won't redact events that we haven't backfilled over federation yet. The MSC also seems to suggest that events should get unredacted if the ban is replaced with another state event without a redactions?

An alternate method may be to have a separate table that is room_redactions(room_id, sender), which we then check (as well as redactions table) when fetching the event? The new room_redactions would be updated when we add the ban/kick to the current_state_events table, and removed if it gets replaced?

The MSC also seems to suggest that events should get unredacted if the ban is replaced with another state event without a redactions?

They shouldn't get unredacted, but the auto-redaction should cease for newly received events. I'll try to clarify this in the MSC itself.

An example series of events:

Alice sends a message

Bob bans Alice, with redact_events: true

[Servers and clients redact Alice's message in step 1]

Due to propagation delays, Alice's second message arrives after the ban. The server (and client, if it received it) redacts that message too.

Later, Bob unbans Alice, setting redact_events: false

Again due to propagation delays, or because Alice rejoined, Alice's third message arrives. It is not redacted.

The messages in steps 1 and 4 remain redacted even after step 5.

anoadragon453

Agree with Erik's comment above.

Otherwise some minor notes on the current code - which is a lot more readable, thank you!

anoadragon453 · 2025-06-24T14:05:06Z

synapse/handlers/federation_event.py

+            # self-bans currently are not authorized so we don't check for that
+            # case


So self-bans would never appear in auth_events, is that correct?

anoadragon453 · 2025-06-24T14:09:09Z

synapse/storage/databases/main/events.py

+                # check for msc4293 redact_events flag and apply if found
+                if (
+                    not self.hs.config.experimental.msc4293_enabled
+                    or event.type != EventTypes.Member


Sanity check:

Suggested change

or event.type != EventTypes.Member

or event.type != EventTypes.Member

or event.state_key is None

anoadragon453 · 2025-06-24T14:12:16Z

synapse/storage/databases/main/events.py

+                        if sender_level < room_redaction_level:
+                            continue
+
+                    if sender_level > redact_level:


Suggested change

if sender_level > redact_level:

if sender_level >= redact_level:

anoadragon453 · 2025-06-24T14:13:44Z

synapse/storage/databases/main/events.py

+                        value_values = [
+                            (self._clock.time_msec(),) for x in ids_to_redact
+                        ]


Suggested change

value_values = [

(self._clock.time_msec(),) for x in ids_to_redact

]

time_now_ms = self._clock.time_msec()

value_values = [

(time_now_ms,) for x in ids_to_redact

]

rather than calling self._clock.time_msec() repeatedly.

anoadragon453 · 2025-06-24T14:15:22Z

synapse/storage/databases/main/events.py

@@ -2768,9 +2850,8 @@ def _store_redaction(self, txn: LoggingTransaction, event: EventBase) -> None:
        self.db_pool.simple_upsert_txn(
            txn,
            table="redactions",
-            keyvalues={"event_id": event.event_id},
+            keyvalues={"event_id": event.event_id, "redacts": event.redacts},


Is redacts unique? I feel you could have multiple redactions redacting a single event.

anoadragon453 · 2025-06-24T14:25:14Z

synapse/handlers/federation_event.py

@@ -2065,6 +2068,42 @@ async def _check_for_soft_fail(
            soft_failed_event_counter.inc()
            event.internal_metadata.soft_failed = True

+        if self._config.experimental.msc4293_enabled:


I see, that is very handy. My hangup is that the logic we're doing for redactions here doesn't result in any soft fails occurring.

One way we could refactor this is to take all of the auth DAG calculation out of _check_for_soft_fail and put that in a _get_current_auth_events function. Then pass the result of that to _check_for_soft_fail and, i.e. _check_for_redacted_sender.

H-Shay added 2 commits June 11, 2025 17:55

add support for MSC4293

0766119

tests

ce555e7

H-Shay requested a review from a team as a code owner June 12, 2025 00:57

H-Shay marked this pull request as draft June 12, 2025 00:59

tulir reviewed Jun 12, 2025

View reviewed changes

synapse/config/experimental.py Outdated Show resolved Hide resolved

H-Shay added 5 commits June 12, 2025 14:10

fix msc number typo

8c606cd

add indexes in background

e8d328f

newsfragment + run indexes in background

b6aa6c0

lint

e4a1014

ensure redact flag is respected when using /kick and /ban

5ad9e66

remove debugging artifact

656b648

H-Shay marked this pull request as ready for review June 12, 2025 23:16

turt2live mentioned this pull request Jun 13, 2025

MSC4293: Redact on ban matrix-org/matrix-spec-proposals#4293

Open

anoadragon453 requested changes Jun 13, 2025

View reviewed changes

requested changes

5a53b93

H-Shay requested a review from anoadragon453 June 16, 2025 23:23

Merge branch 'develop' into shay/redact_on_ban

80e0979

turt2live reviewed Jun 21, 2025

View reviewed changes

requested changes

4323879

erikjohnston reviewed Jun 24, 2025

View reviewed changes

anoadragon453 requested changes Jun 24, 2025

View reviewed changes

		# self-bans currently are not authorized so we don't check for that
		# case

	or event.type != EventTypes.Member
	or event.type != EventTypes.Member
	or event.state_key is None

	if sender_level > redact_level:
	if sender_level >= redact_level:

Add support for MSC4293 - Redact on Kick/Ban #18540

Are you sure you want to change the base?

Add support for MSC4293 - Redact on Kick/Ban #18540

Conversation

H-Shay commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tulir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

H-Shay commented Jun 12, 2025

Uh oh!

anoadragon453 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H-Shay commented Jun 16, 2025

Uh oh!

erikjohnston commented Jun 17, 2025

Uh oh!

turt2live left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

H-Shay commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

H-Shay commented Jun 12, 2025 •

edited

Loading