Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2676: Message editing #2676
MSC2676: Message editing #2676
Changes from 22 commits
8b95ffc
7f42c8b
5473009
e44f566
634dc2e
f9768e7
1de6c11
adcdddc
fc2a690
80c467b
6cea76a
dac2399
b8a7745
79a362e
bb96694
dd1ca71
eba4753
78550a2
e58715c
4c77c01
1850fb5
c9e6652
1e63094
2373873
8ec4cf3
08489e8
f3d328d
b245761
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth noting that what we should encrypt with? I.e. think we encrypt with the "next" key as if it was a normal message, rather than reusing the same key as for the edit event? Not 100% sure though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You always need to use a new key. Not only for security, but if you didn't the fallback would be useless since clients relying on the fallback would mark the event as a key reuse attack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully clarified in uhoreg@1e63094.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the original
content
is replaced, what happens, when the edit is deleted? Where does the originalcontent
come from, when it already has been replaced? Wouldn't it be more consistent to keep the originalcontent
and the client just uses the aggregation for the latestcontent
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Hopefully, processes have improved over the last three years"
. I don't think that merging an immature MSC just because Element does implement it is an "improvement". Element should implement the matrix spec and not the matrix spec should specify Elements implementation ;)The section "Applying
m.new_content
" is from my viewpoint of a SDK developer far more complicated then it needs to be.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit strange to call out Element here specifically. Is this behaviour applicable only to Element and not to other clients? Or is this how most clients are expected to behave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least a few other clients don't behave that way, since they consider it a privacy violation. :3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means "I haven't done a comprehensive survey, and I wouldn't necessarily expect other clients to follow suit".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the MSC could at least suggest that clients redacting events (as moderators or as senders) redact the edits too, if it knows about them...
From a moderation perspective it's certainly a good idea to redact the edits too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what we actually do though? I'm reluctant to spec it if it's not a thing that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not what we do, but the rationale for why we should be redacting old versions is much easier to reason about than why we didn't implement the feature in the first place.
it's long-since been considered a security-ish issue aiui that clients (including bots) don't redact old versions properly. What would be future MSC territory though is making it happen magically from the server rather than requiring the client to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, it just feels like a can of worms that I don't particularly want to open right now. What happens if the client doesn't have all the edits? (ok, it's better, but it's still a crappy solution). What happens if there are a million edits and we unexpectedly hit a rate limit?
I'm not really sure what we gain by adding a "hey, we could do this totally untested temporary hack" to an MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nheko does, but I don't think other clients do.
Link: https://github.com/Nheko-Reborn/nheko/blob/b6bbbdeae7966761ad2de7cdad92363c6926cfb5/src/timeline/TimelineModel.cpp#L1275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we're talking about abuse situations, not the "average", aren't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's several degrees of abuse which we can consider. Spamming edits is one form of abuse, but so is redacting a message and not the edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well fine, this isn't a hill I'm going to die on. Can you suggest some words?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Telegram bridge also redacts all edits when a message is deleted on Telegram. It causes a fun redacted event placeholder spam on Matrix because clients don't know they're edits anymore (#3389), especially if you delete a message sent by some fun telegram bot that edits a message every second for a few hours. But appservices aren't ratelimited so it's fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(A follow up MSC could also remove ratelimit for edit and reaction redactions)