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

Add state-event size validation to prevent non-compliant state events when adding custom emoji packs #866

Open
williamkray opened this issue Sep 21, 2022 · 9 comments

Comments

@williamkray
Copy link

Describe the problem

Currently, users can add custom emojis to rooms and spaces with no limit. This can lead to violating specification on state-event size limits, and while users in the room will not notice an issue, new users who attempt to join will be unable to.

Describe the solution you'd like

I would like the Cinny user experience for uploading or managing custom emojis to sanity-check the size of the state event before the emoji is added, and prevent users from creating state events that do not comply with the specification.

Alternatives considered

Users can guess and estimate how many emojis to put in a pack and manually verify the state events themselves. This is exceptionally cumbersome.

Cinny could implement an arbitrary limit on emojis in a pack. This may result in fringe cases of extremely verbosely-named emojis in a single pack still violating event size.

Additional context

No response

@kfiven
Copy link
Collaborator

kfiven commented Oct 12, 2022

Yeah, we should do this. But more importantly, this should be suggested on MSC side so that every client follow this.

@williamkray
Copy link
Author

Not sure I understand what spec change should be proposed... The spec states the max size of state events, it's up to client not to violate that size. In that sense it is purely an implementation detail and has no impact on the spec itself, therefore does not require an MSC.

If I'm wrong, please clarify what you would need to see in the spec as a client dev, I'd be happy to submit an appropriate spec change suggestion.

@kfiven
Copy link
Collaborator

kfiven commented Oct 22, 2022

The custom emoji and sticker spec I meant. I have seen case when room was failing well before hitting the 65k state event size. So IMO there should be limits on number of emojis and packs.

@tulir
Copy link

tulir commented Oct 27, 2022

This can lead to violating specification on state-event size limits, and while users in the room will not notice an issue, new users who attempt to join will be unable to.

That doesn't make any sense. Unless the homeserver is very badly broken, it will reject oversized state events and they will never be sent to the room. The server should return a M_TOO_LARGE errcode with that rejection.

@williamkray
Copy link
Author

That doesn't make any sense. Unless the homeserver is very badly broken, it will reject oversized state events and they will never be sent to the room. The server should return a M_TOO_LARGE errcode with that rejection.

interesting, I am using normal Synapse official docker container images, and i always stay up to date within a day or two of new releases... so according to your comment, either Synapse is very badly broken or somehow the client implementation in Cinny is doing something to work around that state event rejection. or a third option i'm unable to come up with right now.

@williamkray
Copy link
Author

well, i tried @kfiven

@ShadowJonathan
Copy link
Contributor

@williamkray This looks like a server-side issue, and it feels like a bad server implementation. Even if such events are created, federation servers should reject them as they come in over federation.

Could you create reproducibility steps? Then I can submit this to synapse/dendrite/conduit, and add a test in complement to make sure it doesn't happen again.

@williamkray
Copy link
Author

williamkray commented Oct 28, 2022

@ShadowJonathan i uh... i'm confused. i just tried again to use a secondary account to join my broken room in order to screenshot it for this description for you, and i was able to successfully join the room. so, maybe this is a non-issue? i swear i definitely ran into this behavior. the only thing i can think of is that it's not that the state event size is in violation, but perhaps something about the complexity of the room was too high for the secondary account's server (matrix.org)?

either way, here's the steps. i apologize if this is turning into a wild goose chase.

to reproduce (using Cinny):

  1. create a new room for emoji-packs
  2. use Cinny interface to create a new emoji-pack in the room
  3. add lots of emoji (Cinny currently requires you do this one-by-one) (i have been adding emojis from the microsoft fluent-ui repo)
  4. when there are sufficiently a lot of emojis added to the pack (i ran into this issue with approximately 300 emojis in a pack), try to join the room with a different account on a different server.

expected result: the account can join the room
actual result: the join never succeeds for the second account. If Cinny is the client used, it seems to time out. If element-web is used, it throws an error stating that an event is too large.

@ShadowJonathan
Copy link
Contributor

For reproducibility sake, this is a Synapse server, 1.70.1 at the time of testing.

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

No branches or pull requests

4 participants