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.

Sign up for GitHub

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

getSiteMetadata if possibly FQDN and not extension id, for eth_requestAccounts #7169

Closed
wants to merge 1 commit into from
Closed

getSiteMetadata if possibly FQDN and not extension id, for eth_requestAccounts #7169

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/scripts/controllers/provider-approval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Author

@deluca-mike deluca-mike Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Member

@Gudahtt Gudahtt Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@deluca-mike deluca-mike Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Gudahtt Gudahtt Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@deluca-mike deluca-mike Sep 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I like that actually. I was actually worried you and others wouldn't. I'd do "An unknown browser extension".

Copy link
Member

@Gudahtt Gudahtt Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@deluca-mike deluca-mike Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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).

this._handleProviderRequest(origin, metadata.name, metadata.icon)
// wait for resolution of request
const approved = await new Promise(resolve => this.once(`resolvedRequest:${origin}`, ({ approved }) => resolve(approved)))
Expand Down