Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix event size checks #13710

Merged
merged 3 commits into from
Oct 21, 2022
Merged

Fix event size checks #13710

merged 3 commits into from
Oct 21, 2022

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Sep 4, 2022

This fixes Synapse possibly defederating some rooms with conduit and dendrite<=0.9.5 as well as breaking some clients relying on the guarantees of the client-server API.

Spec link: https://spec.matrix.org/v1.3/client-server-api/#size-limits

Spec issue: matrix-org/matrix-spec#1001

This does break some rooms, if they have any events violating these constraints in their auth chain. This however should not affect that many rooms, considering how long it took to discover this. Most notably those rooms didn't federate with conduit or dendrite servers (although the latter has changed their auth code to match the current synapse implementation recently). More specifically:

  • the event id and room id is not user controlled and all implementations so far use plain ASCII. There are some servers that patched in custom overrides for these IDs, but imo we can safely ignore those as any rooms I encountered so far that violate the tighter constraints were created to test this bug.
  • similarly no event type in the specification currently violates this constraint and events outside of the spec are rarely an important part of the auth chain and can usually be safely dropped. Clients also generally use ascii and short event types when possible, but it might serve well to check the matrix.org database for any events falling into the now invalid area. Please ignore any rooms with room ids from maunium.net of course.
  • The state key and user ids are trickier. Servers in general don't allow you to register non-ascii userids, many even properly restricting to lowercase characters only. So on unmodified servers such userids don't exist. However, you can somewhat trivially specify arbitrary state_keys using the devtools in Element. Though again, those events are usually not in the auth chain: https://spec.matrix.org/v1.3/server-server-api/#auth-events-selection. The only event with a non-empty state_key in the auth-chain is the m.room.member event, which uses the user_id as the state_key, which as previously discussed usually can't use non-ascii characters.

Considering all of the above, imo this should only break rooms intentionally abusing this issue to break rooms. It also makes Matrix a lot more predictable to handle and means you don't need to implement codepoint parsing in SDKs and other Matrix middleware.

Signed-off-by: Nicolas Werner [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Signed-off-by: Nicolas Werner <[email protected]>
@deepbluev7 deepbluev7 requested a review from a team as a code owner September 4, 2022 22:40
Signed-off-by: Nicolas Werner <[email protected]>
changelog.d/13710.bugfix Outdated Show resolved Hide resolved
@reivilibre reivilibre enabled auto-merge (squash) September 9, 2022 10:22
@reivilibre
Copy link
Contributor

reivilibre commented Sep 9, 2022

As far as I can tell, this was 'introduced' when we started to use Python 3. There isn't a clear introduction date since Synapse was, at one point in history, usable on either Python 2 or 3.

Unfortunately this is now blocked until the spec makes its mind up about what to do here :/

@turt2live turt2live added the Z-Spec-Blocked This change is blocked on specification (e.g. an MSC). label Sep 11, 2022
@erikjohnston erikjohnston removed the Z-Spec-Blocked This change is blocked on specification (e.g. an MSC). label Oct 21, 2022
@erikjohnston
Copy link
Member

Consensus on spec issue is that this is fine.

@erikjohnston erikjohnston merged commit fab495a into matrix-org:develop Oct 21, 2022
neilalexander added a commit to matrix-org/gomatrixserverlib that referenced this pull request Oct 21, 2022
…es rather than codepoints (#338)

This effectively reverts the change made in
[5f66df0](5f66df0)
to bytes instead of codepoints, since Synapse will now enforce the same
after matrix-org/synapse#13710.

History here is that Synapse originally calculated bytes in Python 2.x,
started counting codepoints in Python 3.x pretty much by accident and
then the spec was ambiguous after the fact (hence
matrix-org/matrix-spec#1001).

Rationale is that bytes are probably easier for implementations to
manage and less likely to generate huge indexes for client-side
databases (especially where limits might exist like LMDB).

cc @reivilibre
@DMRobertson
Copy link
Contributor

I'm going to revert this on develop. We want to see this change land for the protocol's sake (and plan to un-revert it) but want to give this a little more time before releasing this.

DMRobertson pushed a commit that referenced this pull request Nov 1, 2022
This reverts commit fab495a.

As noted in
#13710 (comment):

> We want to see this change land for the protocol's sake (and plan to
  un-revert it) but want to give this a little more time before releasing
  this.
@DMRobertson
Copy link
Contributor

I'm going to revert this on develop. We want to see this change land for the protocol's sake (and plan to un-revert it) but want to give this a little more time before releasing this.

This ended up happening in #14664, due for release in Synapse 1.74.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Spec-Compliance places where synapse does not conform to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants