MetaMask / metamask-mobile

Mobile web browser providing access to websites that use the Ethereum blockchain
https://metamask.io
Other
2.16k stars 1.11k forks source link

Use the URL `origin` instead of `hostname` to identify subject websites #11029

Open rekmarks opened 2 months ago

rekmarks commented 2 months ago

Due to some error made in the days of yore, the extension originally used the URL hostname component to identify dapps / websites/ (In the language of the permission controller, "subjects" that we identify by a part of their URLs.) This was changed to the more appropriate origin component in https://github.com/MetaMask/metamask-extension/pull/8717. This change never made it to mobile, creating a confusing and potentially dangerous discrepancy in permission enforcement between the two applications. The offending line in mobile is here, although this may have implications for the SDK as well. Note that this may require a state migration.

owencraston commented 2 months ago

I suspect we are seeing the below error because of this line in the SSK Snap. The pre defined permissions map is here. Since I am testing with the published dapp (https://metamask.github.io/snap-simple-keyring/latest/) I figured that it would work but somewhere in the chain of commands the prefix (https://) is being stripped so the permissions are not accepted. Is there a reason we are stripping the prefix on mobile and not on extension?

owencraston commented 2 months ago

I tried changing this line to ensure that we are using the raw url in the snapMethodMiddlewareBuilder but it did not completely trickle down to the snap.

from

app/core/BackgroundBridge/BackgroundBridge.js

snapMethodMiddlewareBuilder(
        Engine.context,
        Engine.controllerMessenger,
        origin,
        // We assume that origins connecting through the BackgroundBridge are websites
        SubjectType.Website,
      ),

to

snapMethodMiddlewareBuilder(
        Engine.context,
        Engine.controllerMessenger,
        this.url,
        // We assume that origins connecting through the BackgroundBridge are websites
        SubjectType.Website,
      ),
owencraston commented 2 months ago

I also tried this change but got the following error.

Screenshot 2024-09-04 at 10 46 57 AM
WARN  [MetaMask DEBUG]: {"code": -32603, "message": "Must specify non-empty string origin."}
 LOG  [MetaMask DEBUG]: RPC (https://metamask.github.io): {"id": 771516241, "jsonrpc": "2.0", "method": "wallet_requestSnaps", "origin": "https://metamask.github.io", "params": {"npm:@metamask/snap-simple-keyring-snap": {"version": "1.1.2"}}, "toNative": true} -> {"error": {"code": -32603, "message": "Must specify non-empty string origin."}, "id": 771516241, "jsonrpc": "2.0"}
rekmarks commented 2 months ago

@owencraston assuming that this.url is a URL, that last one has to be const { origin } = this.url.

Daniel-Cross commented 2 weeks ago

@owencraston I'm looking into this but I'm getting the following message.

Does anyone know what version I need to be on to connect first before I can debug?

{"version": "1.1.6"}}, "toNative": true} -> {"error": {"code": -32603, "data": {"originalError": [Object]}, "message": "Installing Snaps is currently disabled in this version of MetaMask."}, "id": 1160102261, "jsonrpc": "2.0"}

Image