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

Redact message when message is edited and all text is deleted #4623

Closed
wants to merge 2 commits into from

Conversation

aaronraimist
Copy link
Contributor

@t3chguy
Copy link
Member

t3chguy commented May 22, 2020

might still want to this._cancelPreviousPendingEdit();

@t3chguy t3chguy requested a review from a team May 22, 2020 22:20
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

other than the missing this._cancelPreviousPendingEdit();

Signed-off-by: Aaron Raimist <[email protected]>
@jryans
Copy link
Collaborator

jryans commented Jun 5, 2020

To summarise for @matrix-org/product, this work allows you redact / remove a message by editing it to an empty string. Without this change, if you edit a message to nothing, you just see " (edited)" as the message:

image

@dbkr
Copy link
Member

dbkr commented Jun 15, 2020

Have commented on the issue (element-hq/element-web#11024 (comment)). tl;dr: maybe we can change how these messages are presented in the UI instead?

@nadonomy
Copy link
Contributor

Having read through this PR & the related issue I think the missing piece in the critical thinking so far is that redactions are destructive, so therefore would block further edits. Conflating the 2 automatically without explicit action from users would be unexpected.

The solution I'd be interested in to merge this into master would be to decorate blank messages better with an action, like this quick comp:

Comp

@t3chguy
Copy link
Member

t3chguy commented Jun 22, 2020

@nadonomy that looks quite exploitable, I can craft a message which will look identical but on click takes you to some phishing site.

The styling might not be perfect but can probably get it a little closer
image

(Blank) [![](mxc://matrix.org/jatEhazTABVphsTufKcCWnRz) Delete](https://rick.roll)

Also inconsistent terminology Delete vs Remove in the contextual menu

@nadonomy
Copy link
Contributor

Also inconsistent terminology Delete vs Remove in the contextual menu

Yup, was just looking for quick wins on this open PR and favoured consistency in context in the timeline at least.

@nadonomy that looks quite exploitable, I can craft a message which will look identical but on click takes you to some phishing site.

The styling might not be perfect but can probably get it a little closer
image

If this is the case, then I'd block this on design, noting that it's probably around a p2-3 and we have lots of p1 work in flight at the moment so we'd realistically get to this in weeks-months unfortunately.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider treating an edit of deleting all text a redaction
5 participants