Ensure that external domains can only open one tab at once #8147

rekmarks · 2020-03-01T23:11:25Z

Summary

Ensure that external domains can only cause the creation of a single tab at any one time
- The only case I could find of this was in the permission controller's requestUserApproval
Rename MetamaskTabsIDs to MetamaskTabIds, for great benefit to consistency and style
Note: The single instance I've identified where the motivating problem occurs should be eliminated by Prevent external domains from submitting more than one perm request at once #8148, but this may nevertheless be worthwhile.

Details

The requestUserApproval function for permissions requests opens the extension in a new browser tab. Currently, there is no limit to the number of tabs that an external origin can open by submitting permissions requests.

This PR establishes a "tab creation history" to track which origins have (effectively) created an open tab. If an origin already has an open tab and makes a new tab-creating request, no new tab will be created. When the tab created by an origin is closed, the origin is removed from the tab creation history, and can once again create a new tab.

The need for this was exposed when I left a faulty test dapp open in Chrome, and it spawned 10 tabs, like so:

Misc.

The behavior of that test dapps still creates, over time, an unbounded number of pending permission requests for the same origin, and we should consider implementing a similar logic for permissions requests (e.g., only one pending request at a time), but that would be a separate PR. For that, see: #8148

rekmarks · 2020-03-01T23:36:01Z

app/scripts/platforms/extension.js


 class ExtensionPlatform {

+  constructor () {
+
+    if (extension.tabs) {


I had to add this conditional to get the integration tests to pass.

If anyone can tell me how to mock this value, please let me know, and I'll do so.

app/scripts/platforms/extension.js

metamaskbot · 2020-03-01T23:51:09Z

Builds ready [ddca6b1]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (587 ± 55 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	33	99	48	19	9
		domContentLoaded	330	681	585	116	56
		load	332	683	587	116	55
		domInteractive	330	681	585	116	56

metamaskbot · 2020-03-03T02:22:24Z

Builds ready [374f97f]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (584 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	33	82	40	10	5
		domContentLoaded	330	695	583	88	42
		load	332	697	584	88	42
		domInteractive	330	695	583	88	42

Gudahtt · 2020-03-06T03:05:55Z

Hmm.... how would this handle the case where the user used a tab opened by us to navigate away? 🤔 e.g. if I used the connect flow tab to google something. I think that'd just break any subsequent interactions with that dapp.

Gudahtt · 2020-03-06T04:41:57Z

Maybe we could use openMetamaskTabIds to keep track of that case 🤔

There is a current bug with openMetamaskTabIds that I've been meaning to investigate though; there are cases where that gets "stuck" too, leading to the popup never opening (you need to click the browser-action button instead to see confirmations). This is probably what's behind #7292 and #7051. Happens all the time in development. So we'd probably want to fix that bug before relying on openMetamaskTabIds any further.

rekmarks · 2020-03-06T07:05:40Z

@Gudahtt

We should definitely resolve the openMetaMaskTabIds problem regardless, but getting this down for future reference:

If the user navigates to a different site in a MetaMask UI tab, that ought to destroy that UI instance, and cause stream teardowns to fire. Could we get the origin on the UI stream teardowns and handle it at that time?

Not sure how it would work if the user goes back to the MetaMask UI in such a tab. Would that be problematic?

Gudahtt · 2020-03-06T07:49:03Z

Could we get the origin on the UI stream teardowns and handle it at that time?

Yep! I'm fairly certain we can anyway. The endOfStream callback we use to delete the tab from openMetaMaskTabIds is in-scope of the initial connectRemote function, which has the remotePort parameter. remotePort.sender contains the origin.

Not sure how it would work if the user goes back to the MetaMask UI in such a tab. Would that be problematic?

I think that would be fine - if the user navigates back, we can treat it as a normal MetaMask fullscreen tab. By that time the connection to the dapp will have been severed, and the dapp can still initiate new screens.

Gudahtt · 2020-03-13T16:20:21Z

^ I'm just gonna mark this as DONOTMERGE for now because I keep opening this PR to review, forgetting that it has changes pending 🤦‍♂️

Gudahtt · 2020-04-09T23:55:13Z

This should be unblocked now that #8314 has been merged

metamaskbot · 2020-04-10T05:53:22Z

Builds ready [3c01867]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (646 ± 15 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	34	47	39	4	2
		domContentLoaded	602	714	644	31	15
		load	604	716	646	31	15
		domInteractive	602	714	644	31	15

rekmarks · 2020-04-10T15:26:50Z

app/scripts/background.js

+
+/**
+ * Set up listeners for managing tab creation.
+ */
+function setupBrowserListeners () {
+  extension.tabs.onRemoved.addListener(handleTabRemoved)
+}


Given the handling in endOfStream above (L390), do we even need this?

rekmarks · 2020-04-10T15:27:58Z

@Gudahtt rebased and updated per previous discussions on this PR. One open question re: a possible simplification.

metamaskbot · 2020-04-10T15:49:02Z

Builds ready [76be4db]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (587 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	34	95	41	13	6
		domContentLoaded	342	700	586	117	56
		load	346	702	587	116	56
		domInteractive	342	700	585	117	56

metamaskbot · 2020-04-20T20:18:53Z

Builds ready [50ad55f]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

metamaskbot · 2020-04-20T20:46:55Z

Builds ready [154d500]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

MetamaskTabsIds -> MetamaskTabIds

metamaskbot · 2020-04-26T04:37:27Z

Builds ready [d63acc7]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

whymarrh · 2020-07-07T14:35:00Z

We can come back to this at some point

rekmarks requested review from danjm, Gudahtt and whymarrh as code owners March 1, 2020 23:11

rekmarks changed the title ~~Ensure that websites can only open one tab at once~~ Ensure that external domains can only open one tab at once Mar 1, 2020

rekmarks commented Mar 1, 2020

View reviewed changes

app/scripts/platforms/extension.js Outdated Show resolved Hide resolved

Gudahtt added the DO-NOT-MERGE Pull requests that should not be merged label Mar 13, 2020

rekmarks force-pushed the only-one-tab branch from 374f97f to 5675154 Compare April 10, 2020 04:48

rekmarks requested a review from jennypollack as a code owner April 10, 2020 04:48

Gudahtt removed the DO-NOT-MERGE Pull requests that should not be merged label Apr 10, 2020

rekmarks commented Apr 10, 2020

View reviewed changes

rekmarks force-pushed the only-one-tab branch from 76be4db to 50ad55f Compare April 20, 2020 19:58

rekmarks added 5 commits April 25, 2020 21:13

ensure that external domains can only open one tab at once

f672dd7

MetamaskTabsIds -> MetamaskTabIds

MetamaskTabsIds -> MetamaskTabIds

fbbf9a4

remove tab history on UI teardown

a81e2aa

rebase fixup

f40d847

fixup! rebase fixup

d63acc7

rekmarks force-pushed the only-one-tab branch from 154d500 to d63acc7 Compare April 26, 2020 04:20

rekmarks requested a review from a team as a code owner April 26, 2020 04:20

whymarrh closed this Jul 7, 2020

whymarrh deleted the only-one-tab branch August 20, 2020 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that external domains can only open one tab at once #8147

Ensure that external domains can only open one tab at once #8147

rekmarks commented Mar 1, 2020 •

edited

Loading

rekmarks Mar 1, 2020

metamaskbot commented Mar 1, 2020

metamaskbot commented Mar 3, 2020

Gudahtt commented Mar 6, 2020

Gudahtt commented Mar 6, 2020 •

edited

Loading

rekmarks commented Mar 6, 2020

Gudahtt commented Mar 6, 2020

Gudahtt commented Mar 13, 2020

Gudahtt commented Apr 9, 2020

metamaskbot commented Apr 10, 2020

rekmarks Apr 10, 2020

rekmarks commented Apr 10, 2020

metamaskbot commented Apr 10, 2020

metamaskbot commented Apr 20, 2020

metamaskbot commented Apr 20, 2020

metamaskbot commented Apr 26, 2020

whymarrh commented Jul 7, 2020

Ensure that external domains can only open one tab at once #8147

Ensure that external domains can only open one tab at once #8147

Conversation

rekmarks commented Mar 1, 2020 • edited Loading

Summary

Details

Misc.

rekmarks Mar 1, 2020

Choose a reason for hiding this comment

metamaskbot commented Mar 1, 2020

metamaskbot commented Mar 3, 2020

Gudahtt commented Mar 6, 2020

Gudahtt commented Mar 6, 2020 • edited Loading

rekmarks commented Mar 6, 2020

Gudahtt commented Mar 6, 2020

Gudahtt commented Mar 13, 2020

Gudahtt commented Apr 9, 2020

metamaskbot commented Apr 10, 2020

rekmarks Apr 10, 2020

Choose a reason for hiding this comment

rekmarks commented Apr 10, 2020

metamaskbot commented Apr 10, 2020

metamaskbot commented Apr 20, 2020

metamaskbot commented Apr 20, 2020

metamaskbot commented Apr 26, 2020

whymarrh commented Jul 7, 2020

rekmarks commented Mar 1, 2020 •

edited

Loading

Gudahtt commented Mar 6, 2020 •

edited

Loading