Warn users when an ENS name contains 'confusable' characters#9187
Warn users when an ENS name contains 'confusable' characters#9187Gudahtt merged 2 commits intoMetaMask:developfrom
Conversation
|
Hi! As we near the eth2 phase 0 launch, we're interested in using an ENS name, but terrified of these types of issues being a boon to phishers. Is this under serious consideration for merge/release or is it just in the idea phase? |
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
Hi! I wrote this on a whim and am still waiting for a concept ACK from the MetaMask core team. So, at this point it's still in the idea phase, although with a working POC. I am happy to rebase if MetaMask considers this a good solution for these types of ENS issues. |
|
Hello @lastcanal ! Could you rebase and I'll talk to the team about this early next week? Thank you! |
e0685a9 to
5999161
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @darkwing , thanks for picking this up! I've rebased off develop. |
Gudahtt
left a comment
There was a problem hiding this comment.
Hey @lastcanal, sorry for the delay! We're ready again now to review this, once it as been rebased. If you're too busy we'd be happy to take over the PR and finish this up, just let us know. Thanks!
Regarding the design, we've decided to take this basically as-is for now, aside from a few copy updates. The rest of the design changes shown in the issue thread can be implemented later on.
Uses unicode.org's TR39 confusables.txt to display a warning when 'confusable' unicode points are detected. Currently only the `AddRecipient` component has been updated, but the new `Confusable` component could be used elsewhere The new `unicode-confusables` dependency adds close to 100KB to the bundle size, and around 30KB when gzipped. Adds 'tag' prop to the tooltop-v2 component Use $Red-500 for confusable ens warning Lint Tooltip component Update copy for confusing ENS domain warning.
5999161 to
413d76e
Compare
|
Hi @Gudahtt I've rebased and updated the copy. Should be good for review now. :) |
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM, great work!
Two improvements I'd really like to see are:
- Making the warning message visible on the select recipient page, and
- Highlighting the confusable characters in the ENS input component.
But they can both be done in separate PRs. I think the first is relatively simple, but the second looks to be challenging because I don't see any obvious way to distinguish between contact names and ENS names. It might require an adjustment to state. That one might wait until the upcoming confirmation redesign.
|
I've just put up this PR to make the warning visible on the Add Recipient page: #10530 |
Description
Hello! This PR adds a new component called
Confusablethat extracts all code points from a value and checks if they are considered 'confusing' by unicode.org's TR39 working group.It hopefully addresses the issues brought up in #9129
I have added
Confusableto the send page'sAddRecipientcomponent and included an extra validation check so the warning text appears. Hopefully it can be useful elsewhere.Future Work
This does increase the UI bundle size by around 100KB and might affect time-to-first-draw; however, I do think there are a number of things we can do to bring the size down.
Could also be added to the confirmation screen.