feat: controller-utils: Add Sign-in-with-Ethereum origin validation#1163
feat: controller-utils: Add Sign-in-with-Ethereum origin validation#1163legobeat merged 12 commits intoMetaMask:mainfrom
Conversation
adcb80a to
65ddfa3
Compare
9630a78 to
00331b2
Compare
|
Updated with proposal of new implementation of validation logic. For context see comment thread in MetaMask/metamask-extension#18188. Test cases in |
00331b2 to
4a05ea3
Compare
bd00648 to
b9b9ea7
Compare
There was a problem hiding this comment.
Why is URL() not used here? It would parse the origin to an object representation and let us remap the fields onto what we need.
There was a problem hiding this comment.
Main motivation being transparency (in terms of implementation) and flexibility (in case expected behavior would change).
I just pushed changes which replace both of the custom parsing implementations with URL(). As you note, the origin is the more obvious one.
https://github.com/MetaMask/core/pull/1163/files#diff-c7f76a57f21551632599e2cfb0cda2a7bd6e893fdf38e15b3ab49fd4420402e5R73 feels so-so, so I'm open to reverting the parseDomainParts part but mostly agnostic on which approach we use here.
- handle userinfo and port - add tests
98cc110 to
3b7cb0a
Compare
| return true; | ||
| } catch (e) { | ||
| log(e); | ||
| return false; |
212cac2 to
2a06bc1
Compare
digiwand
left a comment
There was a problem hiding this comment.
lgtm! nice implementation with tests! helpful change to provide better accuracy for domain binding warnings.
2 related notes for future iteration:
- I think when we introduce developer-mode and further restrict domain binding, we will want to update this logic. maybe we can consider providing the mismatched parts to the UI in this future iteration
- maybe we can consider running the check where we call
detectSIWEand providing the value in the msgParams if the value will be handy in other places. In other words, we could replace calls todetectSIWEwith something like detectSIWEAndCheckDomainBinding
c766fee to
4ab1f02
Compare
Sign-in-with-Ethereum origin validation
isValidSIWEOriginfor origin validation of SIWE requestsDescription
This is intended as a starting point and the current validation logic needs to be replaced or extended before merging.
ADDED:
controller-utils: Add functionisValidSIWEOriginfor origin validation of SIWE requestsChecklist
Issue
metamask-extension: Use SIWE origin validation logic from @metamask/controller-utils metamask-extension#18518