Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch Extension Metadata on eth_requestAccounts #7039

Conversation

deluca-mike
Copy link

@deluca-mike deluca-mike commented Aug 20, 2019

tl;dr This PR allows the provider approval logic to fetch extension metadata in order to allow eth_requestAccounts calls to succeed when triggered from other extensions.

This should resolve #5950 and MetaMask/extension-provider#3

When an extension tries to call the eth_requestAccounts method via the web3 provider over the port stream to MetaMask, the provider approval logic was trying to fetch site metadata using an origin string that was not a FQDN, but rather an extension id.

This was silently throwing, resulting in no approval dialog/window.

I have added the "management" permission to this extension to allow it to determine if the origin string is an extension id, and to fetch the relevant extension name and its largest icon. This will only work in Chrome, or any other browser that supports chrome.management.getAll. However, it will simply behave as it did before if it cannot fetch the extension.

I have tested this locally and it seems to work so far. Not familiar with the linting rules around here, so this will likely fail some long-line rule.

@deluca-mike
Copy link
Author

deluca-mike commented Aug 20, 2019

If someone can help me out with failing tests, I'd appreciate it.

My bad, I forgot to resolve a Promise on exit cases...

@deluca-mike deluca-mike force-pushed the bugfix/extension_eth_requestAccounts branch from 24b2324 to c61aaba Compare August 20, 2019 15:00
@bdresser
Copy link
Contributor

hey @coinsquareMike, thank you for the contribution. This is a great feature and we're keen on adding inter-extension support.

However, we are very cautious about adding new permissions – our past experience has taught us they cause widespread confusion across our userbase and can result in the extension being disabled for thousands. Unless there is an overwhelmingly compelling reason, we keep the permissions list as-is.

Is there any way to implement this without the management permission? Otherwise this may have to be an optional / opt-in feature or something similar. (cc @whymarrh @danfinlay )

@deluca-mike
Copy link
Author

deluca-mike commented Aug 20, 2019

@bdresser I understand and was worried this wouldn't be accepted due to the added permissions.

There are 2 ways we can fix the issue for extensions calling eth_requestAccounts without the need for the management permission:

  1. Have MetaMask listen via runtime.onMessageExternal for some custom registerExtension request that allows an extension to register its id, name, and icon with MetaMask, so that the extension metadata can already be known and then displayed to the user in the Approval Dialog. Keep in mind, though, that there would be no way, without the management permission, to verify the accuracy of the metadata the extension is providing, which means an extension can pretend to be another.

  2. Throw all of this out and simply check if the origin string (in the provider approval logic) is not formatted as an FQDN, or is formatted as an extension id, and simply tell the user "Some extension is trying to connect" with no name or icon, possibly still showing the extension id, which may be odd UX for most users.

The second way is much easier, albeit worse UX. The only trusted source on extension metadata is the chrome API. Let me know which you prefer and I can implement.

@deluca-mike
Copy link
Author

deluca-mike commented Aug 20, 2019

Just thought about it some more. While I am not sure if Chrome Web Store has a query API (let's just assume it does, or can be scrapped), then in either case, the extension id can be queried (as such) in MetaMask at approval-time, and the metadata can be fetched directly from the store.

This would make the above method 2 easier.

Looks like this API needs a Google API key (for Chrome) and likely for every other browser's store. Not difficult, just a bit of a pain to maintain 4-5 different APIs.

It's unlikely another extension can scrape the internal extension page (chrome://extensions/?id=nkbihfbeogaeaoehlefnkodbefgpgknn), though.

@deluca-mike
Copy link
Author

deluca-mike commented Aug 20, 2019

Upon further poking around, it would be possible to take advantage of remotePort.postMessage and remotePort.onMessage in function connectExternal (remotePort) (backgorund.js) to pass around some one-time PIN (similar to Bluetooth connections) that the user sees and needs to approve/confirm on the calling extension's side, before calling controller.setupUntrustedCommunication(portStream, originDomain) with additional gathered metadata, if the originDomain is formatted as an extension id.

@Gudahtt
Copy link
Member

Gudahtt commented Aug 21, 2019

Thanks for the thorough investigation, this was very helpful!

We could scrape the Chrome or Firefox web stores, or use whatever APIs might exist for discovering extension metadata, but I'm a bit wary of that approach. I'm concerned that it might be difficult to maintain. Plus we'd be scraping data for the latest version of the extension rather than the current one, which is a bit odd (the version of the extension installed isn't knowable either).

The one-time PIN idea is pretty interesting, but I don't think it's bullet-proof - an extension could still pretend to be something it's not using their own UI (e.g. using a notification window without an address bar shown, or a browser-action button with a false logo). It also seems complex, and it'd be a lot to ask of an extension that wants to connect with us.

Have MetaMask listen via runtime.onMessageExternal for some custom registerExtension request that allows an extension to register its id, name, and icon with MetaMask, so that the extension metadata can already be known and then displayed to the user in the Approval Dialog. Keep in mind, though, that there would be no way, without the management permission, to verify the accuracy of the metadata the extension is providing, which means an extension can pretend to be another.

This sounds like the best approach. We don't need to have a way of verifying the name and logo are correct - we don't currently verify that with sites either. We could ban specific extension ids if we notice any abuse in the wild, and probably get them removed from each of the browser stores as well. And certainly allowing extensions to register a name and logo is better than lacking both.

@deluca-mike
Copy link
Author

deluca-mike commented Aug 21, 2019

@Gudahtt

This sounds like the best approach. We don't need to have a way of verifying the name and logo are correct - we don't currently verify that with sites either. We could ban specific extension ids if we notice any abuse in the wild, and probably get them removed from each of the browser stores as well. And certainly allowing extensions to register a name and logo is better than lacking both.

The thing is, this isn't necessarily true. As an extension, you get "verified" information about origin requests (from sites) for "free" from the browser. A site cannot (easily, or at all) fake its domain and favicon/icon to the browser, and thus, getSiteMetadata will get accurate info from the browser.

If we just allow the calling extension to communicate with MetaMask to "pre-register" its metadata, it can do something like give "MetaMask", "Status.im" or "etherscan.io" as its name, and the corresponding icon. What the "pre-register" mechanism I proposed ends up doing is burdening all honest extensions with a bi-directional registration communication channel, thereby coupling what should be standard a one-way interface, and creating a false sense of security.

As you put it, it would also be

complex, and it'd be a lot to ask of an extension that wants to connect with us.

I'd much rather just have no name and icon and just have MetaMask say "some extension wants to connect", which is not only more accurate and honest (since there is no way MetaMask can accurately get the extension's name, without the management permission), but also does not allow for the user to be fooled. Also, this approach is by far the easiest to implement.

The one-time PIN idea is pretty interesting, but I don't think it's bullet-proof - an extension could still pretend to be something it's not using their own UI

As for the PIN mechanism I proposed, this would be the procedure:

  1. Extension says "I will connect to MetaMask now. Only approve the connection if you see 4271"
  2. Extension exchanges PIN with MetaMask, then connects to MetaMask's port-stream/provider. This can be in one step, or 2, depending on the implementation.
  3. MetaMask can't fetch the siteMetadata (given the origin is not a FQDN), so instead it shows "Some extension is trying to connect with PIN 4271. Only approve if you recognize this".
  4. User approves.

There is no additional steps in the MetaMask user flow, and the UX work is entirely on the calling extension. What's better is, it doesn't even need to be a PIN; it can be an image, color, etc. So long as it is something shown to the user first (in the calling extension) before the approval is requested, and cannot easily be guessed/generated by another extension in the background.

@Gudahtt
Copy link
Member

Gudahtt commented Aug 21, 2019

As an extension, you get "verified" information about origin requests (from sites) for "free" from the browser. A site cannot (easily, or at all) fake its domain and favicon/icon to the browser, and thus, getSiteMetadata will get accurate info from the browser.

getSiteMetadata doesn't get the icon or the name from the domain - it gets it from the DOM. That's not a trusted source. The only trusted piece of information is the origin - and the same is true of extension id in the case of an extension.

The level of trust extended is the same in both cases - the user can always verify that the domain is what they expect for a site, and likewise they can verify that the extension id is what they expect for an extension.

What the "pre-register" mechanism I proposed ends up doing is burdening all honest extensions with a bi-directional registration communication channel, thereby coupling what should be standard a one-way interface

They'd be required to send a single message to MetaMask to register their metadata before connecting. That's not particularly burdensome. It's also optional.

There is no additional steps in the MetaMask user flow, and the UX work is entirely on the calling extension.

Exactly - that's a bad thing. We'd be asking any extension that wants to connect to us to implement their own "Connect Request" UI. That's too much to ask. We'd also be thrusting an incongruous UX upon users, as the third-party extension Connect Request would look different for each extension. It could also malfunction or perform poorly, which reflects poorly upon us.

@deluca-mike
Copy link
Author

deluca-mike commented Aug 21, 2019

the user can always verify that the domain is what they expect for a site, and likewise they can verify that the extension id is what they expect for an extension.

I guess the difference is that 99% of users don't even know extensions have ids, let alone what their favourite extension's id is. But thanks for clearing up my confusion on getSiteMetadata. I didn't dig through how it got what it got, so I assumed.

Also, I see where you're coming from for the rest. Fair enough.

So, given this, shall I change the PR to implement the optional "pre-registering", where if there is no pre-registered metadata, MetaMask simply says "some extension is trying to connect", along with the extension id?

The extension I am working on needs to be able to connect to MetaMask, and while this PR's acceptance and a new cut release of MetaMask might not happen soon, if I can get consensus around an implementation, I can continue my extension knowing what functionality I can expect later.

@Gudahtt
Copy link
Member

Gudahtt commented Aug 21, 2019

I guess the difference is that 99% of users don't even know extensions have ids, let alone what their favourite extension's id is.

Fair point. This is probably something we can address with a warning and a link to a help article explaining how to check extension ids on each browser. I agree that it's worth cautioning users.

So, given this, shall I change the PR to implement the optional "pre-registering", where if there is no pre-registered metadata, MetaMask simply says "some extension is trying to connect", along with the extension id?

Sure, that would be fantastic!

I suspect we could use our existing Connection Request UI with minimal changes. The logo is already optional, so you'll just have to substitute something for the name.

@Gudahtt
Copy link
Member

Gudahtt commented Sep 17, 2019

Thanks again for your work on this. I'm going to close this PR - we can consider it as superseded by #7169

@Gudahtt Gudahtt closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the metamask-extension-provider work with privacy mode
3 participants