Prevent accidental use of globals (#8340)

Previously all browser globals were allowed to be used anywhere by ESLint because we had set the `env` property to `browser` in the ESLint config. This has made it easy to accidentally use browser globals (e.g. #8338), so it has been removed. Instead we now have a short list of allowed globals. All browser globals are now accessed as properties on `window`. Unfortunately this change resulted in a few different confusing unit test errors, as some of our unit tests setup assumed that a particular global would be used via `window` or `global`. In particular, `window.fetch` didn't work correctly because it wasn't patched by the AbortController polyfill (only `global.fetch` was being patched). The `jsdom-global` package we were using complicated matters by setting all of the JSDOM `window` properties directly on `global`, overwriting the `AbortController` for example. The `helpers.js` test setup module has been simplified somewhat by removing `jsdom-global` and constructing the JSDOM instance manually. The JSDOM window is set on `window`, and a few properties are set on `global` as well as needed by various dependencies. `node-fetch` and the AbortController polyfill/patch now work as expected as well, though `fetch` is only available on `window` now.
MetaMask · Apr 15, 2020 · 5ee1291 · 5ee1291
1 parent 3735f0b
commit 5ee1291
Show file tree

Hide file tree

Showing 30 changed files with 78 additions and 76 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -25,10 +25,6 @@ module.exports = {
     'plugin:react/recommended',
   ],
 
-  env: {
-    'browser': true,
-  },
-
   plugins: [
     'babel',
     'react',
@@ -37,9 +33,10 @@ module.exports = {
   ],
 
   globals: {
-    'web3': true,
-    '$': false,
-    'QUnit': false,
+    '$': 'readonly',
+    document: 'readonly',
+    QUnit: 'readonly',
+    window: 'readonly',
   },
 
   rules: {

diff --git a/app/scripts/controllers/infura.js b/app/scripts/controllers/infura.js
@@ -20,7 +20,7 @@ class InfuraController {
   // Responsible for retrieving the status of Infura's nodes. Can return either
   // ok, degraded, or down.
   async checkInfuraNetworkStatus () {
-    const response = await fetch('https://api.infura.io/v1/status/metamask')
+    const response = await window.fetch('https://api.infura.io/v1/status/metamask')
     const parsedResponse = await response.json()
     this.store.updateState({
       infuraNetworkStatus: parsedResponse,

diff --git a/app/scripts/controllers/token-rates.js b/app/scripts/controllers/token-rates.js
@@ -37,7 +37,7 @@ class TokenRatesController {
     const query = `contract_addresses=${pairs}&vs_currencies=${nativeCurrency}`
     if (this._tokens.length > 0) {
       try {
-        const response = await fetch(`https://api.coingecko.com/api/v3/simple/token_price/ethereum?${query}`)
+        const response = await window.fetch(`https://api.coingecko.com/api/v3/simple/token_price/ethereum?${query}`)
         const prices = await response.json()
         this._tokens.forEach((token) => {
           const price = prices[token.address.toLowerCase()] || prices[ethUtil.toChecksumAddress(token.address)]

diff --git a/app/scripts/lib/ens-ipfs/setup.js b/app/scripts/lib/ens-ipfs/setup.js
@@ -50,7 +50,7 @@ function setupEnsIpfsResolver ({ provider, getCurrentNetwork, getIpfsGateway })
         const resolvedUrl = `https://${hash}.${ipfsGateway}${path}${search || ''}${fragment || ''}`
         try {
           // check if ipfs gateway has result
-          const response = await fetch(resolvedUrl, { method: 'HEAD' })
+          const response = await window.fetch(resolvedUrl, { method: 'HEAD' })
           if (response.status === 200) {
             url = resolvedUrl
           }

diff --git a/app/scripts/lib/fetch-with-timeout.js b/app/scripts/lib/fetch-with-timeout.js
@@ -1,8 +1,8 @@
 const fetchWithTimeout = ({ timeout = 120000 } = {}) => {
   return async function _fetch (url, opts) {
-    const abortController = new AbortController()
+    const abortController = new window.AbortController()
     const abortSignal = abortController.signal
-    const f = fetch(url, {
+    const f = window.fetch(url, {
       ...opts,
       signal: abortSignal,
     })

diff --git a/app/scripts/lib/network-store.js b/app/scripts/lib/network-store.js
@@ -24,7 +24,7 @@ class ReadOnlyNetworkStore {
    */
   async _init () {
     try {
-      const response = await fetch(FIXTURE_SERVER_URL)
+      const response = await window.fetch(FIXTURE_SERVER_URL)
       if (response.ok) {
         this._state = await response.json()
       }

diff --git a/app/scripts/lib/setupFetchDebugging.js b/app/scripts/lib/setupFetchDebugging.js
@@ -7,12 +7,12 @@ export default setupFetchDebugging
 //
 
 function setupFetchDebugging () {
-  if (!global.fetch) {
+  if (!window.fetch) {
     return
   }
-  const originalFetch = global.fetch
+  const originalFetch = window.fetch
 
-  global.fetch = wrappedFetch
+  window.fetch = wrappedFetch
 
   async function wrappedFetch (...args) {
     const initialStack = getCurrentStack()

diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js
@@ -105,7 +105,7 @@ function rewriteReportUrls (report) {
 }
 
 function toMetamaskUrl (origUrl) {
-  const filePath = origUrl.split(location.origin)[1]
+  const filePath = origUrl.split(window.location.origin)[1]
   if (!filePath) {
     return origUrl
   }

diff --git a/app/scripts/lib/util.js b/app/scripts/lib/util.js
@@ -45,7 +45,7 @@ const getEnvironmentType = (url = window.location.href) => {
  *
  */
 const getPlatform = (_) => {
-  const ua = navigator.userAgent
+  const ua = window.navigator.userAgent
   if (ua.search('Firefox') !== -1) {
     return PLATFORM_FIREFOX
   } else {

diff --git a/development/mock-3box.js b/development/mock-3box.js
@@ -3,13 +3,13 @@ function delay (time) {
 }
 
 async function loadFromMock3Box (key) {
-  const res = await fetch('http://localhost:8889?key=' + key)
+  const res = await window.fetch('http://localhost:8889?key=' + key)
   const text = await res.text()
   return text.length ? JSON.parse(text) : null
 }
 
 async function saveToMock3Box (key, newDataAtKey) {
-  const res = await fetch('http://localhost:8889', {
+  const res = await window.fetch('http://localhost:8889', {
     method: 'POST',
     body: JSON.stringify({
       key,

diff --git a/package.json b/package.json
@@ -245,7 +245,6 @@
     "gulp-watch": "^5.0.1",
     "gulp-zip": "^4.0.0",
     "jsdom": "^11.2.0",
-    "jsdom-global": "^3.0.2",
     "karma": "^4.1.0",
     "karma-chrome-launcher": "^2.2.0",
     "karma-cli": "^1.0.1",

diff --git a/test/e2e/contract-test/contract.js b/test/e2e/contract-test/contract.js
@@ -1,4 +1,4 @@
-/*global ethereum, MetamaskOnboarding */
+/*global ethereum, MetamaskOnboarding, web3 */
 
 /*
 The `piggybankContract` is compiled from:

diff --git a/test/e2e/webdriver/driver.js b/test/e2e/webdriver/driver.js
@@ -192,11 +192,11 @@ function collectMetrics () {
     navigation: [],
   }
 
-  performance.getEntriesByType('paint').forEach((paintEntry) => {
+  window.performance.getEntriesByType('paint').forEach((paintEntry) => {
     results.paint[paintEntry.name] = paintEntry.startTime
   })
 
-  performance.getEntriesByType('navigation').forEach((navigationEntry) => {
+  window.performance.getEntriesByType('navigation').forEach((navigationEntry) => {
     results.navigation.push({
       domContentLoaded: navigationEntry.domContentLoadedEventEnd,
       load: navigationEntry.loadEventEnd,

diff --git a/test/helper.js b/test/helper.js
@@ -32,7 +32,10 @@ Enzyme.configure({ adapter: new Adapter() })
 
 // ganache server
 const server = Ganache.server()
-server.listen(8545, () => {})
+server.listen(8545)
+
+server.on('error', console.error)
+server.on('clientError', console.error)
 
 log.setDefaultLevel(5)
 global.log = log
@@ -41,17 +44,30 @@ global.log = log
 // polyfills
 //
 
+// dom
+const { JSDOM } = require('jsdom')
+
+const jsdom = new JSDOM()
+global.window = jsdom.window
+
+// required by `trezor-connect/node_modules/whatwg-fetch`
+global.self = window
+// required by `dom-helpers` and various other libraries
+global.document = window.document
+// required by `react-tippy`
+global.navigator = window.navigator
+global.Element = window.Element
+
+// delete AbortController added by jsdom so it can be polyfilled correctly below
+delete window.AbortController
+
 // fetch
 const fetch = require('node-fetch')
 
-global.fetch = fetch
-global.Response = fetch.Response
-global.Headers = fetch.Headers
-global.Request = fetch.Request
-require('abortcontroller-polyfill/dist/polyfill-patch-fetch')
+const { Headers, Request, Response } = fetch
+Object.assign(window, { fetch, Headers, Request, Response })
 
-// dom
-require('jsdom-global')()
+require('abortcontroller-polyfill/dist/polyfill-patch-fetch')
 
 // localStorage
 window.localStorage = {}

diff --git a/test/integration/lib/confirm-sig-requests.js b/test/integration/lib/confirm-sig-requests.js
@@ -23,7 +23,7 @@ async function runConfirmSigRequestsTest (assert) {
   reactTriggerChange(selectState[0])
 
   const realFetch = window.fetch.bind(window)
-  global.fetch = (...args) => {
+  window.fetch = (...args) => {
     if (args[0] === 'https://ethgasstation.info/json/ethgasAPI.json') {
       return Promise.resolve({ json: () => Promise.resolve(JSON.parse(fetchMockResponses.ethGasBasic)) })
     } else if (args[0] === 'https://ethgasstation.info/json/predictTable.json') {

diff --git a/test/integration/lib/currency-localization.js b/test/integration/lib/currency-localization.js
@@ -22,7 +22,7 @@ async function runCurrencyLocalizationTest (assert) {
   selectState.val('currency localization')
 
   const realFetch = window.fetch.bind(window)
-  global.fetch = (...args) => {
+  window.fetch = (...args) => {
     if (args[0] === 'https://ethgasstation.info/json/ethgasAPI.json') {
       return Promise.resolve({ json: () => Promise.resolve(JSON.parse(fetchMockResponses.ethGasBasic)) })
     } else if (args[0] === 'https://ethgasstation.info/json/predictTable.json') {

diff --git a/test/integration/lib/tx-list-items.js b/test/integration/lib/tx-list-items.js
@@ -27,7 +27,7 @@ async function runTxListItemsTest (assert) {
   reactTriggerChange(selectState[0])
 
   const realFetch = window.fetch.bind(window)
-  global.fetch = (...args) => {
+  window.fetch = (...args) => {
     if (args[0] === 'https://ethgasstation.info/json/ethgasAPI.json') {
       return Promise.resolve({ json: () => Promise.resolve(JSON.parse(fetchMockResponses.ethGasBasic)) })
     } else if (args[0] === 'https://ethgasstation.info/json/predictTable.json') {

diff --git a/test/setup.js b/test/setup.js
@@ -5,10 +5,4 @@ require('@babel/register')({
 require('./helper')
 
 window.SVGPathElement = window.SVGPathElement || { prototype: {} }
-window.fetch = window.fetch || function fetch () {
-  return Promise.resolve()
-}
 global.indexedDB = {}
-global.fetch = global.fetch || function fetch () {
-  return Promise.resolve()
-}
diff --git a/test/unit/app/controllers/incoming-transactions-test.js b/test/unit/app/controllers/incoming-transactions-test.js
@@ -314,12 +314,12 @@ describe('IncomingTransactionsController', function () {
     }))
     let tempFetch
     beforeEach(function () {
-      tempFetch = global.fetch
-      global.fetch = mockFetch
+      tempFetch = window.fetch
+      window.fetch = mockFetch
     })
 
     afterEach(function () {
-      global.fetch = tempFetch
+      window.fetch = tempFetch
       mockFetch.resetHistory()
     })
 

diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js
@@ -1432,15 +1432,15 @@ describe('Actions', function () {
     let setCurrentLocaleSpy
 
     beforeEach(function () {
-      sinon.stub(global, 'fetch')
+      sinon.stub(window, 'fetch')
         .resolves({
           json: async () => enLocale,
         })
     })
 
     afterEach(function () {
       setCurrentLocaleSpy.restore()
-      global.fetch.restore()
+      window.fetch.restore()
     })
 
     it('calls expected actions', async function () {

diff --git a/ui/app/ducks/gas/gas-duck.test.js b/ui/app/ducks/gas/gas-duck.test.js
@@ -76,19 +76,19 @@ describe('Gas Duck', function () {
   })
 
   beforeEach(function () {
-    tempFetch = global.fetch
+    tempFetch = window.fetch
     tempDateNow = global.Date.now
 
     fakeLocalStorage.loadLocalStorageData = sinon.stub()
     fakeLocalStorage.saveLocalStorageData = sinon.spy()
-    global.fetch = sinon.stub().callsFake(fakeFetch)
+    window.fetch = sinon.stub().callsFake(fakeFetch)
     global.Date.now = () => 2000000
   })
 
   afterEach(function () {
     sinon.restore()
 
-    global.fetch = tempFetch
+    window.fetch = tempFetch
     global.Date.now = tempDateNow
   })
 
@@ -291,7 +291,7 @@ describe('Gas Duck', function () {
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
       assert.deepEqual(
-        global.fetch.getCall(0).args,
+        window.fetch.getCall(0).args,
         [
           'https://ethgasstation.info/json/ethgasAPI.json',
           {
@@ -353,7 +353,7 @@ describe('Gas Duck', function () {
         mockDistpatch.getCall(0).args,
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
-      assert.ok(global.fetch.notCalled)
+      assert.ok(window.fetch.notCalled)
       assert.deepEqual(
         mockDistpatch.getCall(1).args,
         [{
@@ -390,7 +390,7 @@ describe('Gas Duck', function () {
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
       assert.deepEqual(
-        global.fetch.getCall(0).args,
+        window.fetch.getCall(0).args,
         [
           'https://ethgasstation.info/json/ethgasAPI.json',
           {
@@ -444,7 +444,7 @@ describe('Gas Duck', function () {
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
       assert.deepEqual(
-        global.fetch.getCall(0).args,
+        window.fetch.getCall(0).args,
         [
           'https://ethgasstation.info/json/ethgasAPI.json',
           {
@@ -520,7 +520,7 @@ describe('Gas Duck', function () {
         mockDistpatch.getCall(0).args,
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
-      assert.ok(global.fetch.notCalled)
+      assert.ok(window.fetch.notCalled)
 
       assert.deepEqual(
         mockDistpatch.getCall(1).args,
@@ -565,7 +565,7 @@ describe('Gas Duck', function () {
         [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED } ]
       )
       assert.deepEqual(
-        global.fetch.getCall(0).args,
+        window.fetch.getCall(0).args,
         [
           'https://ethgasstation.info/json/ethgasAPI.json',
           {
@@ -626,7 +626,7 @@ describe('Gas Duck', function () {
         [{ type: GAS_ESTIMATE_LOADING_STARTED } ]
       )
       assert.deepEqual(
-        global.fetch.getCall(0).args,
+        window.fetch.getCall(0).args,
         [
           'https://ethgasstation.info/json/predictTable.json',
           {
@@ -679,7 +679,7 @@ describe('Gas Duck', function () {
         mockDistpatch.getCall(0).args,
         [{ type: GAS_ESTIMATE_LOADING_STARTED } ]
       )
-      assert.equal(global.fetch.callCount, 0)
+      assert.equal(window.fetch.callCount, 0)
 
       assert.deepEqual(
         mockDistpatch.getCall(1).args,

diff --git a/ui/app/ducks/gas/gas.duck.js b/ui/app/ducks/gas/gas.duck.js
@@ -195,7 +195,7 @@ export function fetchBasicGasEstimates () {
 }
 
 async function fetchExternalBasicGasEstimates (dispatch) {
-  const response = await fetch('https://ethgasstation.info/json/ethgasAPI.json', {
+  const response = await window.fetch('https://ethgasstation.info/json/ethgasAPI.json', {
     'headers': {},
     'referrer': 'http://ethgasstation.info/json/',
     'referrerPolicy': 'no-referrer-when-downgrade',
@@ -259,7 +259,7 @@ export function fetchBasicGasAndTimeEstimates () {
 }
 
 async function fetchExternalBasicGasAndTimeEstimates (dispatch) {
-  const response = await fetch('https://ethgasstation.info/json/ethgasAPI.json', {
+  const response = await window.fetch('https://ethgasstation.info/json/ethgasAPI.json', {
     'headers': {},
     'referrer': 'http://ethgasstation.info/json/',
     'referrerPolicy': 'no-referrer-when-downgrade',
@@ -377,7 +377,7 @@ export function fetchGasEstimates (blockTime) {
     dispatch(gasEstimatesLoadingStarted())
 
     const promiseToFetch = Date.now() - timeLastRetrieved > 75000
-      ? fetch('https://ethgasstation.info/json/predictTable.json', {
+      ? window.fetch('https://ethgasstation.info/json/predictTable.json', {
         'headers': {},
         'referrer': 'http://ethgasstation.info/json/',
         'referrerPolicy': 'no-referrer-when-downgrade',