Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events with rejected_auth_events must be rejected #1137

Merged
merged 2 commits into from Jul 5, 2022

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jun 17, 2022

This might be kinda obvious, but didn't seem to be spelt out anywhere.

Preview: https://pr1137--matrix-spec-previews.netlify.app

This might be kinda obvious, but didn't seem to be spelt out anywhere.
Comment on lines 40 to 42
5. If the `m.room.create` event content has the field `m.federate` set to `false`
and the `sender` domain of the event does not match the `sender` domain of
the create event, reject.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really a check on the auth_events or is it a check on the event being inspected?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiplied by however many times it shows up in this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the question. It's a check on the m.room.create event which is referred to in the auth_events of the event being inspected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the server be using the m.room.create event from the room rather than auth_events though? It feels more understandable to move this point back up a level rather than nest it unmodified into the auth_events checks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly, but that seems to be a change to its meaning. Currently we have:

  1. If event does not have a m.room.create in its auth_events, reject.
  2. If the create event...

It's not entirely clear which create event it is talking about in 4, but given the context from 3, surely it's the one from the auth_events.

This is all related to #1136.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrrrm, actually, looking at the source to Synapse, it does appear to apply the test to the create event in the room state, so I guess that's an argument that we should make that explicit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've updated this to make it apply to the room state, rather than the auth events.

@richvdh richvdh requested a review from turt2live July 1, 2022 19:05
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm either way, but would feel better about having which state explicit.

performed on receipt of a
PDU](server-server-api/#checks-performed-on-receipt-of-a-pdu), reject.
4. If there is no `m.room.create` event among the entries, reject.
3. If the `content` of the `m.room.create` event in the room state has the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to clarify which room state (at or before the event in question?), though hopefully for the create event it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applies to other snippets too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it's the same state that all the other auth rules refer to. Per https://spec.matrix.org/v1.3/server-server-api/#checks-performed-on-receipt-of-a-pdu, we actually check them at three different states.

I agree all this could be clarified, but I don't think it's the job of this PR.

@richvdh richvdh merged commit 848294e into main Jul 5, 2022
@richvdh richvdh deleted the rav/auth_rules_rejected_auth_events branch July 5, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants