MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.88k stars 4.85k forks source link

[Bug]: Sign In with Ethereum throws an unhandled exception for bad domains #17707

Closed PeterYinusa closed 1 year ago

PeterYinusa commented 1 year ago

Describe the bug

Sign In with Ethereum throws an unhandled exception for bad domains

https://user-images.githubusercontent.com/53189696/218042372-ef68235e-51a1-402d-a696-ec8716c5d7f5.mov

Steps to reproduce

  1. Login to the extension
  2. Navigate to the test dapp
  3. Click Sign In With Ethereum (Bad Account)
  4. Check the background console

Error messages or log output

SES_UNHANDLED_REJECTION: Error: SIWE domain is not valid: "metamask.github.io" !== "metamask.badactor.io"
  at y.addUnapprovedMessage (personal-message-manager.js:159:15)
  at personal-message-manager.js:101:12
  at new Promise (<anonymous>)
  at y.addUnapprovedMessageAsync (personal-message-manager.js:94:12)
  at De.newUnsignedPersonalMessage (metamask-controller.js:2790:48)
  at wallet.js:161:1
  at async createAsyncMiddleware.js:45:1

Version

10.26.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

bschorchit commented 1 year ago

This is intended behavior as the domain that is requesting and domain that is in the message don't match (which would be attack vector to gain credentials to impersonate the user). But maybe we could handle it more gracefully to the user? cc: @holantonela

holantonela commented 1 year ago

This is not a bug, this is a feature.

It is indeed the behavior for dapp requests that do not comply with our enforced domain binding. The dapp can return an error in the dapp, we are returning a comprehensive error to developers Error: SIWE domain is not valid: "metamask.github.io" !== "metamask.badactor.io", but users should never face this situation.

We enforced domain binding to prevent a warning to users in the confirmation page since there is not recovery path. This is an enforcement that looks into reducing phishing attacks.

For future recovery paths of this flow, we discussed local allow lists and offering users the chance to include trusted unmatched domains in that list, but we are not there yet.

Ref https://github.com/MetaMask/metamask-extension/issues/16295#issuecomment-1307878082

PeterYinusa commented 1 year ago

Thanks for sharing this. I've updated the title to adjust the expectations. Although it's by design, we would still need to resolve the unhandled exception. Here's the difference between Eth Sign which fails gracefully and SIWE which throws an unhandled exception. The issue with the exception being unhandled is that it will fill up our Sentry account with this error, which is not the case for Eth Sign

ETH Sign: RPC Error SIWE: Unhandled Error

https://user-images.githubusercontent.com/53189696/218110483-3dbf0346-99e5-4534-94d5-7c89d599306e.mov

PeterYinusa commented 1 year ago

Sentry issue: METAMASK-WQS5

HeyFloM commented 1 year ago

Hi all,

I just discovered this is the reason players cannot connect Metamask version 10.26.1 to our Unity game anymore. I understand the point but this is a major change that will probably break a lot of exsiting Dapps :/

bschorchit commented 1 year ago

Hey @HeyFloM, thank you for reporting this. Could you share more info on existing dapps that are breaking with this so we can test on our side? Thank you!

yonigoldberg commented 1 year ago

@bschorchit We see the SWIE issue on localhost starting with version 10.26.1. Even though we follow the specs and pass a valid URI. Though I am not seeing any exception; MM just shuts down.

image

https://www.loom.com/share/fed210079d214a05be6276662d2220f1

danjm commented 1 year ago

@yonigoldberg are you directly sending an rpc request to metamask? if so, can you copy and paste the request here?

if not, are you using a library or SDK to handle the signing request? if so, which one?

danjm commented 1 year ago

Also, did you see this error message? Error: SIWE domain is not valid: "metamask.github.io" !== "metamask.badactor.io"

yonigoldberg commented 1 year ago

@danjm - Ethers JsonRPCSigner - signMessage And there are no error messages in the console, just the screen disappears (see in the video). I can confirm that a non SIWE message works, and SIWE works fine on non-localhost domains.

bschorchit commented 1 year ago

Feedback from @skgbafa: The port also needs to be specified for domain matching to work. So localhost:3000 instead of localhost

Could you try this and let us know if it works for you @yonigoldberg ?

bschorchit commented 1 year ago

Oh wait, I believe you're doing it already so this might not be the case

yonigoldberg commented 1 year ago

Here is an example of a SIWE that we use at Dynamic.xyz:

localhost wants you to sign in with your Ethereum account:
0x1B095A01c1F2893aD19885caab642f647cddf82d

Welcome to Dynamic Example. Signing is the only way we can truly know that you are the owner of the wallet you are connecting. Signing is a safe, gas-less transaction that does not in any way give Dynamic Example permission to perform any transactions with your wallet.

URI: http://localhost:4201/
Version: 1
Chain ID: 1
Nonce: 99093988031a4e0f922c536ace06304a
Issued At: 2023-03-15T20:19:45.324Z
Request ID: 67ddb74b-e30f-4039-9a25-f033c79f1207
HeyFloM commented 1 year ago

On our side, this is problematic for all integrations of Metamask with Unity WebGL games/metaverse. Why? Because all WebGL builds needs to be hosted on specific hosting services like itch.io so we can easily test them without having to upload each build on our Amazon S3 Buckets. So each development build have this error right now image

Question: is the rejection happening if we use two different subdomains? eg: metamask.github.io and mysubdomain.github.io?

vandan commented 1 year ago

@HeyFloM Can you confirm whether you specifically expect domains not to match in your development environment and if you have any alternative ways to get the domains to match. According to SIWE Specs matching of domains is required.

cc skgbafa

HeyFloM commented 1 year ago

@vandan I confirm that for many Web3 gaming / Metaverse projects based on Unity WebGL, it is much easier to use specialized hosting platforms to host the Dapp and then interact with the SDK that manages Web3 authentication. By default, this means two different domains.

On our side, we have aligned the domain names with the game in production and everything is working again, but this update will constantly force us to modify the URLs during testing phases (for your information, we release multiple Unity WebGL builds per day).

The solution we have in mind for now is to use version 10.25 for daily tests to save time and host the tested version on our servers.

digiwand commented 1 year ago

Hey @HeyFloM,

Question: is the rejection happening if we use two different subdomains? eg: metamask.github.io and mysubdomain.github.io?

A: Yes, mismatched subdomains are blocked for more robust security.

Thank you for providing the details related to your use-case! It's very helpful. glad to hear that the team has found a solution for production. We're currently discussing a plan to support mismatched domains in development mode.

HeyFloM commented 1 year ago

@digiwand my pleasure! Initially, it impacted all development and production versions. After identifying the issue, we aligned the URL of the domain hosting the production Unity build with the server URL that manages the Unity SDK and Web3 authentication (Moralis Unity SDK self-hosted server). As a result, there are no longer any problems with the production part.

However, the impact is still present on the development side because we use different hosting services for speed and agility to export and test quickly. These services make it very easy to see what are the results of your latest dev changes in the build, but they are on another domain. We could create a test environment on our servers, but this would require our developers to:

And this at least 4 times per day, which would be a significant timeloss

bschorchit commented 1 year ago

It seems that are 3 different issues here, so I've created a new issue https://github.com/MetaMask/metamask-extension/issues/18188 for what @yonigoldberg reported above so we investigate it separately.

digiwand commented 1 year ago

Hey @HeyFloM, thank you for the additional details! @legobeat has provided a workaround for this second hurdle you've run into: https://github.com/MetaMask/metamask-extension/issues/18188#issuecomment-1499919118:

My answer to the 1A scenario where the developer wants an application served from an origin of cdn.example.com to be able to claim a domain of dapp.example.com is to either distribute appropriate TLS certificates for dapp.example.com to the utilized CDN or introduce terminating TLS proxies, in either case to the effect of aligning domains. This not being circumventable is intentional. Same applies for 1E.

Think this is something that could help?