getSiteMetadata if possibly FQDN and not extension id, for eth_requestAccounts #7169
Conversation
Using regex to check if origin contains "localhost" or a ".", ":", or "/"
deluca-mike
force-pushed
the
fix/provider-approval-for-extenstion
branch
from
2855b7c to
a3f2d7c
Compare
| @@ -41,7 +41,7 @@ class ProviderApprovalController extends SafeEventEmitter { | |||
| return | |||
| } | |||
| // register the provider request | |||
| const metadata = await getSiteMetadata(origin) | |||
| const metadata = /localhost|[.:/]/.test(origin) ? await getSiteMetadata(origin) : { name: 'An extenstion' } | |||
There was a problem hiding this comment.
Interesting method of detecting extension URLs. I think it should work for now? 🤔
It doesn't seem ideal though. I'd prefer that we check the actual scheme of the origin, but unfortunately that's stripped out earlier on.
I'm tempted to try updating background.js to include the scheme in the origin it passes through, then you could just extract the scheme here and use that instead. It'll be a bit delicate though, as everything else is expecting the origin to not have a scheme...
There was a problem hiding this comment.
Ya, I noticed this was a bit delicate, so this is a simpler way that doesn't really break anything. Although, if the alphabetic extension id happens to contain 'localhost' (which in incredibly unlikely), well, edge case...
There was a problem hiding this comment.
I'm tempted to accept the regex for now and improve this using the scheme later on, but... I'm concerned there will be more edge cases than just localhost. It's not uncommon to use custom domains locally for testing for example (by modifying the hosts file).
Instead I think we can sidestep even attempting to detect extensions by using the origin as the fallback if a title isn't present (as mentioned in my other comment). This lets us avoid this until we can do it well.
There was a problem hiding this comment.
It's not uncommon to use custom domains locally for testing
So long as the the custom domain in the hosts file has a tld, or if a port is used, it will be picked up as a domain
Instead I think we can sidestep even attempting to detect extensions by using the origin as the fallback if a title isn't present
Keep in mind, this isn't detecting an extension, its detecting domains with a very large net. Also, I am not sure what you mean by "title". Do you mean the name in the object returned by getSiteMetadata? If so, the issue is that getSiteMetadata throws silently for extensions, and never returns null for name.
To be honest, I didn't really understand how getSiteMetadata gets what it gets, as the code got pretty consulted, and wasn't that functions. At least IMO.
There was a problem hiding this comment.
So long as the the custom domain in the hosts file has a tld, or if a port is used, it will be picked up as a domain
You're right about the tld. I don't think the port is included though - the origin parameter here is really just the hostname, which afaik doesn't include the port.
I am not sure what you mean by "title". Do you mean the name in the object returned by getSiteMetadata?
Sorry yes, that's what I meant.
the issue is that getSiteMetadata throws silently for extensions, and never returns null for name.
Ah I see 🤔. I had not realized - that does change things. I'll take a quick look to see if I can find out why this happens.
There was a problem hiding this comment.
Maybe we/I modify getSiteMetadata to do the regex test before calling its downstream functions, and immediately return a null name and icon, or a null object altogether? This way, we can ignore why downstream calls might crash and can implement the UI logic as you suggested.
There was a problem hiding this comment.
I've made an attempt here: 1b32801
I'm not entirely sure that the extensionId is a reliable indicator of whether the sender is an extension or not; the docs imply it'll bet set for extensions but I haven't tested it yet. We could check the scheme instead (for chrome-extension or moz-extension, and that should work reliably as well. It does avoid calling getSiteMetadata just as your solution did, so that;s good.
The tests still aren't working; I need to investigate a bit to find out why. I haven't updated the UI yet either, so it'll still likely break because it expects a name in the metadata.
| @@ -41,7 +41,7 @@ class ProviderApprovalController extends SafeEventEmitter { | |||
| return | |||
| } | |||
| // register the provider request | |||
| const metadata = await getSiteMetadata(origin) | |||
| const metadata = /localhost|[.:/]/.test(origin) ? await getSiteMetadata(origin) : { name: 'An extenstion' } | |||
There was a problem hiding this comment.
I have some reservations about this choice in default extension name. Maybe something like Unknown browser extension would be more descriptive, and would convey that we don't know what it is? Open to other ideas too.
There was a problem hiding this comment.
I like that actually. I was actually worried you and others wouldn't. I'd do "An unknown browser extension".
There was a problem hiding this comment.
Unknown might sound a bit scary to users, but I don't mind encouraging users to be cautious. Plus, it'll act as an incentive for extension authors to add proper metadata (once that's supported at least).
I chose to omit the An because of the context in which it's shown - it's styled as a title, not a description.
Though on that point - maybe we're better off choosing a fallback title in the UI rather than here. This feels like the wrong place to make UI decisions. Though the front-end does need to know that this is from an extension, not a normal page.
I've just had an idea - we could bypass the problem of detecting whether it's an extension or not completely and avoid making UI decisions in the background by adding origin to the metadata. Then on the front-end, you could fallback to showing the origin directly in place of a title if the title is missing.
I don't think it'd be super difficult to implement. You'd need to add it to this metadata object, and pass it through to the state update in approveProviderRequestByOrigin, and that's about it for the background. In the UI you'd need to update ui/app/components/app/provider-page-container/provider-page-container-content/ to make siteTitle optional, add origin, and set origin as the fallback. Then there are a few more components above that you'd need to pass the origin through - just search for siteTitle to find them.
There was a problem hiding this comment.
I chose to omit the An because of the context in which it's shown
Sure, I can drop the 'an'.
This feels like the wrong place to make UI decisions.
I completely agree. I can modify _handleProviderRequest and other downstream modules/functions, to take an additional, but optional, argument isPossiblyExtension, which will be handled by the UI to display the message accordingly. This will result in more lines of code for this PR, but it will result in the UI decision being made in a more appropriate place.
detecting whether it's an extension
I just want to make it clear that it's not possible to detect that something is an extension (without the Management API) and thus it is only possible to detect if something definitely isn't an extension.
by adding origin to the metadata
Yes, either the UI can detect with regex if the origin isn't an extension by regex, or by a null name, or as I mentioned in my previous comment, with a isPossiblyExtension bool passed to it as an argument or in the metadata object. However, this does not solve the issue that if you try to call getSiteMetadata when origin is an extension, the function will crash and you will not be able to proceed, so you still need to regex/filter before calling getSiteMetadata.
So, at the end of the day, while I can do what you suggested, I cannot call getSiteMetadata when origin is an extension ID, or else it will crash (as is current state).
|
I'm going to close this in favour of #7218 as I'd prefer the approach that exposes the trusted extension id from the browser API to the user. Thanks again for all the work you did toward this; I really appreciate the help and feedback. |
tl:dr This PR allows the provider approval logic to display "an extension is trying to connect..." when an extension calls eth_requestAccounts (via the web3 provider over the port stream to MetaMask).
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 tested this fix locally and it seems to work so far.
This is also a follow up to PR #7039. I think this is the safest and simplest fix for now, since there is no way for MetaMask to know for sure which extension is requesting to connect (without management permissions). As per comments in PR #7039, a second PR can come later which improves UX by allowing extensions to register their metadata.