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 event size checks similar to Synapse #400

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Add event size checks similar to Synapse #400

merged 5 commits into from
Jul 7, 2023

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Jul 6, 2023

This tries to mimic the logic from Synapse to allow specific events.

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 64.55% and project coverage change: +1.52 🎉

Comparison is base (90ad5fa) 56.95% compared to head (da261f0) 58.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
+ Coverage   56.95%   58.47%   +1.52%     
==========================================
  Files          50       51       +1     
  Lines        6632     6993     +361     
==========================================
+ Hits         3777     4089     +312     
- Misses       2498     2517      +19     
- Partials      357      387      +30     
Impacted Files Coverage Δ
eventcontent.go 76.43% <ø> (ø)
json.go 86.11% <33.33%> (-0.94%) ⬇️
spec/senderid.go 62.50% <50.00%> (-37.50%) ⬇️
performjoin.go 59.79% <52.05%> (-1.49%) ⬇️
handlejoin.go 73.06% <52.63%> (-2.64%) ⬇️
keyring.go 40.37% <68.75%> (-0.21%) ⬇️
event.go 76.19% <100.00%> (+52.66%) ⬆️
eventV2.go 54.54% <100.00%> (+17.87%) ⬆️
eventcrypto.go 59.32% <100.00%> (+5.09%) ⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 247 to 249
if err := checkID(input.RoomID(), "room", '!'); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also check eventID like synapse does here?

eventV2.go Outdated
Comment on lines 218 to 235
if input.StateKey() != nil {
if l := len(*input.StateKey()); l > maxIDLength {
if l := utf8.RuneCountInString(*input.StateKey()); l > maxIDLength {
return EventValidationError{
Code: EventValidationTooLarge,
Message: fmt.Sprintf("gomatrixserverlib: state key is too long, length %d bytes > maximum %d bytes", l, maxIDLength),
}
}
}

_, persistable := lenientByteLimitRoomVersions[input.Version()]

if l := len(input.Type()); l > maxIDLength {
return EventValidationError{
Code: EventValidationTooLarge,
Message: fmt.Sprintf("gomatrixserverlib: event type is too long, length %d bytes > maximum %d bytes", l, maxIDLength),
Persistable: persistable,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should comment the reasoning behind the 2 checks like synapse does.

@S7evinK S7evinK merged commit 35f734f into main Jul 7, 2023
5 checks passed
@S7evinK S7evinK deleted the s7evink/eventsize branch July 7, 2023 18:00
S7evinK added a commit to matrix-org/dendrite that referenced this pull request Jul 7, 2023
Companion to matrix-org/gomatrixserverlib#400
This tries to mimic the logic found in Synapse, as dropping events can
break rooms (and we may end up in endless loops..)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants