Metamask-extension: Add whitelist of domains to not inject Web3 on

Created on 1 Mar 2018 · 24Comments · Source: MetaMask/metamask-extension

I maintain an extension that parses U.S. government court websites for metadata. Apparently, today, somebody started using both of our extensions, with these results:

We do server-side parsing of webpages that our extension uploads to our server. As of today, our parser (lxml's HTML parser) is failing because the HTML we're receiving is too...broken? Funky? Not sure. But instead of getting a nice DOM out of the parser, we are getting:
```
<html><body><html xmlns:html="http://www.w3.org/1999/xhtml"><head></head></html></body></html>
```
From what I can tell, it looks like your extension adds a <script> tag to the <html> node, not the <head> or <body>, which seems to break things in my parser. It could have something to do with your specific JS though, because if we take the JS out and just leave an empty <script> tag, the parser works fine.

Something weird there.
The parser for an HTML page that's normally about 50 lines long is now taking about 10 seconds to parse and create a tree data structure. This is very slow for server-side parsing — or any parsing.
There is now 10,000 extra lines of JavaScript on the page, ballooning it from 5kb to nearly half a megabyte! That's a 100× explosion. Does your extension do that to every page it encounters?!

I'm wondering if you have any solutions for this. I don't know much about your project (cryptocurrency isn't something I'm keeping up with), but a few ideas:

Our extension only operates on one domain, uscourts.gov. Could you whitelist that domain so that your extension doesn't work there?
There are ways for extensions to communicate. Could we use that to disable your extension when ours is present? This is a bigger solution, perhaps, but it avoids you maintaining an ever-growing whitelist of URLs.

I don't know. I'm more of a back end person, so extensions aren't my strong suit, but it'd be great if we could do something to make this better. Thanks.

P2-sooner T00-bug bounty worthy

Source

mlissner

Most helpful comment

I'm not a big JS dev, but this feels urgent if you're really injecting 500kb of JS into every page a user loads.

I think it's less sluggish than you'd think, because it's not over network, it's just local DOM manipulation.

If it's possible to do whitelisting, that really would be wonderful.

It is, and hopefully we'll get it done soon.

danfinlay on 15 Mar 2018

👍2 ❤1

All 24 comments

I did some further research on this and found a few other examples of this problem with other pairs of extensions:

It seems like the best solution is to work together. The easiest from my perspective is to just have uscourts.gov whitelisted by you guys so that MetaMask is disabled on the domain.

Finally, should we be concerned that this JavaScript was sent to our servers? Given that this is some kind of cryptocurrency, I worry that there might be private info in here.

mlissner on 2 Mar 2018

You'll see that I just posted a fix for this in our parsing code, so the issue is "resolved", sort of, on our end, but we don't have a way of detecting your extension client side, so we can't tell our users what the problem is and that they need to either not use our extension or not use yours.

I remain concerned about what's in that JS that we are now receiving, if there are any security issues there.

If it's possible, the whitelist approach would be much better for our users since it wouldn't cause breakage on our end (and I think it'd leave your extension working too?)

mlissner on 2 Mar 2018

Is anybody available to respond to this issue between our extensions? We ran into additional problems today.

mlissner on 14 Mar 2018

Hi @mlissner, sorry this slipped through the cracks. Thanks for the ping @lazaridiscom.

We are currently working on a feature where by default we will never mutate the DOM of any site, until a user explicitly opts in. That's our long term solution.

Shortest term, users can disable our extension using the usual method (right-click our fox, select Manage Extensions, uncheck the active checkbox next to it.

There could be a medium term fix too, we could add a list of sites to definitely not inject on here. I'll consider creating that list and adding your site to it the criteria for closing this issue. We're all pretty busy at the moment, but we've had great lucky with bounties for discrete tasks like this.

There is now 10,000 extra lines of JavaScript on the page, ballooning it from 5kb to nearly half a megabyte! That's a 100× explosion. Does your extension do that to every page it encounters?!

Most of this is the web3.js library, which we plan to deprecate eventually, but currently many of our users rely on.

Finally, should we be concerned that this JavaScript was sent to our servers? Given that this is some kind of cryptocurrency, I worry that there might be private info in here.

No, should include no private info.

danfinlay on 15 Mar 2018

We are currently working on a feature where by default we will never mutate the DOM of any site, until a user explicitly opts in. That's our long term solution.

I'm not a big JS dev, but this feels urgent if you're really injecting 500kb of JS into every page a user loads. I'm surprised that's not more sluggish. But I don't really have a horse in that race, so...moving on.

If it's possible to do whitelisting, that really would be wonderful.

mlissner on 15 Mar 2018

I'm not a big JS dev, but this feels urgent if you're really injecting 500kb of JS into every page a user loads.

I think it's less sluggish than you'd think, because it's not over network, it's just local DOM manipulation.

If it's possible to do whitelisting, that really would be wonderful.

It is, and hopefully we'll get it done soon.

danfinlay on 15 Mar 2018

👍2 ❤1

__This issue now has a funding of 0.15 ETH (91.61 USD @ $610.73/ETH) attached to it.__

If you would like to work on this issue you can claim it here.
If you've completed this issue and want to claim the bounty you can do so here
Questions? Get help on the Gitcoin Slack
$3331.86 more Funded OSS Work Available at: https://gitcoin.co/explorer

gitcoinbot on 15 Mar 2018

Any bounty sumbmission must be configurable to point at an API update so that the list can be revised without pushing an app update.

danfinlay on 15 Mar 2018

👍1

__Work has been started on the 0.15 ETH (91.67 USD @ $611.15/ETH) funding by__:

@SaptakS

__Please work together__ and coordinate delivery of the issue scope. Gitcoin doesn't know enough about everyones skillsets / free time to say who should work on what, but we trust that the community is smart and well-intentioned enough to work together. As a general rule; if you start work first, youll be at the top of the above list ^^, and should have 'dibs' as long as you follow through.

On the above list? Please leave a comment to let the funder (@vs77bb) and the other parties involved what you're working, with respect to this issue and your plans to resolve it. If you don't leave a comment, the funder may expire your submission at their discretion.

Learn more on the gitcoin issue page
Questions? Get help on the Gitcoin Slack
$3318.86 more Funded OSS Work Available at: https://gitcoin.co/explorer

gitcoinbot on 15 Mar 2018

@danfinlay how can I decide on the API the extension wants to point to? Or should I make a global configuration variable to add the API url to? I have a confusion in that part. Apart from that, I believe I have fixed this issue.

SaptakS on 15 Mar 2018

@SaptakS you can make the lists local for now, as long as they are passed in as an option, so we can fetch them eventually.

I think the "Blacklist" is the most important part of this, because it's about not injecting web3 on domains we conflict with. Other domains that should be included are:

dropbox.com

(That's the one I know of right now)

danfinlay on 16 Mar 2018

Just curious: What's the issue with dropbox?

mlissner on 16 Mar 2018

@danfinlay Okay. will do that. Please do review my PR once to see the approach.

SaptakS on 16 Mar 2018

Just curious: What's the issue with dropbox?

I'm surprised there's no issue for it, but at one point at least we interfered with dropbox.

danfinlay on 16 Mar 2018

Okay. will do that. Please do review my PR once to see the approach.

Reviewed PR, I think this is good enough to merge as an MVP, but I would like you to switch the naming from "whitelist" to "blacklist", because that's what it is.

danfinlay on 16 Mar 2018

👍1

Merged this fix into master, the blacklist will be enabled for this site on the next release.

danfinlay on 16 Mar 2018

@SaptakS You can go on GitCoin now and claim the bounty and we'll pay it out.

danfinlay on 16 Mar 2018

This is fantastic news. Thank you so much!

mlissner on 16 Mar 2018

Hi @mlissner go ahead and click submit work on Gitcoin and we'll be able to pay out!

vs77bb on 20 Mar 2018

Hi @danfinlay @vs77bb I am unable to submit work since it says I will be needing some ether to do so. Not sure.

SaptakS on 20 Mar 2018

Hi @mlissner go ahead and click submit work on Gitcoin and we'll be able to pay out!

Um, I don't know what Gitcoin is.....kinda feel like I'm not the person for this task and that I'd screw it up?

mlissner on 20 Mar 2018

@mlissner My apologies - yes, a typo. Thank you @lazaridiscom for noting - much appreciated.

@SaptakS Check out gitcoin.co/faucet and make a submission. We'll send you some ETH for your initial transaction and then will accept your submission for this bounty!

vs77bb on 30 Mar 2018

__Work for 0.15 ETH (59.91 USD @ $399.4/ETH) has been submitted by__:

@SaptakS

Submitters, please leave a comment to let the funder (@vs77bb) (and the other parties involved) that you've submitted you work. If you don't leave a comment, the funder may expire your submission at their discretion.