Skip to content

Sign-in with Ethereum Domain Binding#16616

Merged
digiwand merged 27 commits intoMetaMask:developfrom
spruceid:feat/siwe-domain-binding
Feb 8, 2023
Merged

Sign-in with Ethereum Domain Binding#16616
digiwand merged 27 commits intoMetaMask:developfrom
spruceid:feat/siwe-domain-binding

Conversation

@skgbafa
Copy link
Copy Markdown
Contributor

@skgbafa skgbafa commented Nov 21, 2022

Explanation

This PR prevents Sign-in with Ethereum messages being served from a domain that doesn't match the domain in message from being displayed to the user.

Manual Testing Steps

  1. Turn on the feature flag by adding SIWE_V1=1 to metamask-extension/.metamaskrc
  2. Build and run this version of the app.
  3. Go to Metamask's Test Dapp and scroll down to the Sign-In with Ethereum section. (If the dapp doesn't have this Add Sign-in with Ethereum Test Cases test-dapp#164, use this version).
  4. Test the 5 tests cases.
    i. The Sign In With Ethereum (Bad Domain) should show an error in the console. The rest of the tests should show the updated Sign-in with Ethereum display.

@skgbafa skgbafa requested a review from a team as a code owner November 21, 2022 21:23
@skgbafa skgbafa requested a review from darkwing November 21, 2022 21:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 21, 2022

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.

@skgbafa skgbafa changed the title Feat/siwe domain binding Sign-in with Ethereum Domain Binding Nov 21, 2022
brad-decker
brad-decker previously approved these changes Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

Wow. Nice.

@BelfordZ
Copy link
Copy Markdown
Contributor

BelfordZ commented Dec 7, 2022

Fantastic PR, thank you @skgbafa

Looks good, If you could please add a test or two here then we can get this puppy merged asap.

@BelfordZ BelfordZ mentioned this pull request Dec 7, 2022
Merged
jpuri
jpuri previously approved these changes Jan 13, 2023
View reviewed changes
digiwand
digiwand previously approved these changes Jan 13, 2023
View reviewed changes
This was referenced Jan 27, 2023
Closed
Closed
@skgbafa skgbafa dismissed stale reviews from digiwand and jpuri via b0fa6d7 February 1, 2023 15:16
@bschorchit bschorchit added the team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead label Feb 6, 2023
@bschorchit bschorchit requested review from digiwand and jpuri February 7, 2023 11:07
@digiwand digiwand merged commit 3233f76 into MetaMask:develop Feb 8, 2023
@digiwand digiwand deleted the feat/siwe-domain-binding branch February 8, 2023 15:06
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

@darkwing darkwing Awaiting requested review from darkwing darkwing is a code owner automatically assigned from MetaMask/extension-devs

+3 more reviewers

@segun segun segun approved these changes

@digiwand digiwand digiwand approved these changes

@brad-decker brad-decker brad-decker left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@skgbafa @BelfordZ @segun @jpuri @brad-decker @digiwand @bschorchit