Room version 11 & MSC2175: creator improperly handled on events

[turt2live]

Description

At

synapse/synapse/events/utils.py

Line 143 in 67f9e52

add_fields("creator")

it appears the creator field is always protected from redaction, however the very last line of MSC2175 states:

creator is also mentioned as a key to be preserved during Event redaction. It should be removed from that list.

I believe there should be an if statement around here somewhere. It does not appear as though Synapse is populating creator, but it is improperly allowing the field to survive redaction if present.

[turt2live]

565b9f9 fixes this without tests

[H-Shay]

While looking into this (essentially trying to write a test for this by sending a redact event), I discovered that it appears that Synapse categorically does not allow redaction of create events - is this expected? I would imagine that it conflicts with the desired behavior - that create events can be redacted and when they are, the creator field is stripped from them.

synapse/synapse/handlers/message.py

Lines 1782 to 1799 in 67f9e52

	if event.type == EventTypes.Redaction:
	assert event.redacts is not None
	original_event = await self.store.get_event(
	event.redacts,
	redact_behaviour=EventRedactBehaviour.as_is,
	get_prev_content=False,
	allow_rejected=False,
	allow_none=True,
	)
	room_version = await self.store.get_room_version_id(event.room_id)
	room_version_obj = KNOWN_ROOM_VERSIONS[room_version]
	# we can make some additional checks now if we have the original event.
	if original_event:
	if original_event.type == EventTypes.Create:
	raise AuthError(403, "Redacting create events is not permitted")

[turt2live]

Synapse does block redacting the create event over the client-server API, but over federation it doesn't have that much control. It's largely to prevent people from casually breaking rooms (because some fairly important fields aren't covered by redaction in some room versions, oops).

[anoadragon453]

Both the proposed fix for this issue and #15963 look good to me. I agree that including tests would help build confidence further.

@H-Shay did you want to take on writing tests and PRing both of these? In this case, I think it would be fine to simply unit test prune_event or prune_event_dict directly, as all of the redaction logic is contained in there.

[H-Shay]

I can PR and write the tests, FWIW the timeline link in #15963 is failing for me, is there anything pertinent there or can I PR the fix in #15963 (with a test) as-is?

[turt2live]

Should be fine to commit as-is. The permalinks were just confirming the intention, and it was confirmed :D

[anoadragon453]

Confirming that the release blocker for this issue and #15963 are correct, and that we'd like to get these in for v1.89.0rc1.

[H-Shay]

Sorry, another dumb question: in #15963 it is stated that the entirety of the content field should be kept intact, but this issue here seems to imply that creator should be stripped - however AFAICT creator is always a sub field of content - thus these instructions seem to be in conflict with one another?

[H-Shay]

To be more specific in my question (and apologies for my ignorance), but this is the two proposed fixes together:

    elif event_type == EventTypes.Create:
        # MSC2176 rules state that create events cannot have their `content` redacted.
        if room_version.updated_redaction_rules:
            new_content = event_dict["content"]

        if not room_version.implicit_room_creator:
            add_fields("creator")

If the room_version used is v11, then we use the updated redaction rules and add content back in (and I am assuming creator as a sub-field of content). If not v11, this is skipped and we move to the next line of code, which checks if the room version supports implicit room creation - which everything but room version 11 does not, so the creator field is added back in unless it is v11, which still has the creator field as it is a sub-field of the preserved content field. I feel like I am missing something here?

[turt2live]

hmm, that's fun. There's a few MSCs being stacked up here - we both remove meaning from creator and preserve the entirety of content. We can probably fix the meaning like so:

    elif event_type == EventTypes.Create:
        if room_version.updated_redaction_rules:
            # MSC2176 rules state that create events cannot have their `content` redacted.
            new_content = event_dict["content"]
        elif not room_version.implicit_room_creator:
            # Some room versions give meaning to `creator`
            add_fields("creator")

[H-Shay]

So just to be clear - the practical result of this is that creator is always preserved because for room versions < v11 (ie implicit_room_creator is false) we will add it back in, and for room versions == v11 (updated_redaction_rules is True) we keep it because it is a sub-field of content - but that's essentially a function of the stacking of the MSC's, not necessarily that creator needs to be inherently preserved, and writing it this way makes that clearer.

[turt2live]

I think so, yea. It's also not impossible that v12 (for example) dictates that it wants implicit_room_creator=False but not updated_redaction_rules because room versions aren't linear.

[H-Shay]

Right on thanks for clarifying!

[H-Shay]

I've added the fix for this and #15963 are at #15973 + change for the tests that were already there - if there's more testing you think I should add let me know. @anoadragon453 I've requested a review from you as reviews have been slow and I think you have the most context for this.