Merge remote-tracking branch 'origin/develop' into build-sys-2

* origin/develop: Fix connection removal bug (#9137) Add source map validator to CI (#9135) Update source map validator target files (#9133) Improve sourcemap validator console report (#9131) Add `validate-source-maps` npm script (#9134) Non-zero exit code upon failure to validate source maps (#9132) remove unused tx-list styles (#9121) delete unused confirm styles (#9118)
MetaMask · Aug 5, 2020 · cddbc25 · cddbc25
2 parents 8b8bce8 + 658e478
commit cddbc25
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 651 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -41,6 +41,9 @@ workflows:
       - test-unit-global:
           requires:
             - prep-deps
+      - validate-source-maps:
+          requires:
+            - prep-build
       - test-mozilla-lint:
           requires:
             - prep-deps
@@ -52,6 +55,7 @@ workflows:
             - test-lint-lockfile
             - test-unit
             - test-unit-global
+            - validate-source-maps
             - test-mozilla-lint
             - test-e2e-chrome
             - test-e2e-firefox
@@ -374,6 +378,18 @@ jobs:
       - run:
           name: test:unit:global
           command: yarn test:unit:global
+
+  validate-source-maps:
+    docker:
+      - image: circleci/node@sha256:e16740707de2ebed45c05d507f33ef204902349c7356d720610b5ec6a35d3d88
+    steps:
+      - checkout
+      - attach_workspace:
+          at: .
+      - run:
+          name: Validate source maps
+          command: yarn validate-source-maps
+
   test-mozilla-lint:
     docker:
       - image: circleci/node@sha256:e16740707de2ebed45c05d507f33ef204902349c7356d720610b5ec6a35d3d88

diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -1722,7 +1722,7 @@ export default class MetamaskController extends EventEmitter {
 
     delete connections[id]
 
-    if (Object.keys(connections.length === 0)) {
+    if (Object.keys(connections).length === 0) {
       delete this.connections[origin]
     }
   }

diff --git a/development/sourcemap-validator.js b/development/sourcemap-validator.js
@@ -13,13 +13,30 @@ const fsAsync = pify(fs)
 // if not working it may error or print minified garbage
 //
 
-start().catch(console.error)
+start().catch((error) => {
+  console.error(error)
+  process.exit(1)
+})
 
 
 async function start () {
-  const targetFiles = [`inpage.js`, `contentscript.js`, `ui.js`, `background.js`]
+  const targetFiles = [
+    `background.js`,
+    'common.js',
+    // `contentscript.js`, skipped because the validator is erroneously sampling the inlined `inpage.js` script
+    `inpage.js`,
+    'phishing-detect.js',
+    `ui.js`,
+  ]
+  let valid = true
+
   for (const buildName of targetFiles) {
-    await validateSourcemapForFile({ buildName })
+    const fileIsValid = await validateSourcemapForFile({ buildName })
+    valid = valid && fileIsValid
+  }
+
+  if (!valid) {
+    process.exit(1)
   }
 }
 
@@ -59,6 +76,7 @@ async function validateSourcemapForFile ({ buildName }) {
 
   console.log(`  sampling from ${consumer.sources.length} files`)
   let sampleCount = 0
+  let valid = true
 
   const buildLines = rawBuild.split('\n')
   const targetString = 'new Error'
@@ -71,6 +89,7 @@ async function validateSourcemapForFile ({ buildName }) {
       const result = consumer.originalPositionFor(position)
       // warn if source content is missing
       if (!result.source) {
+        valid = false
         console.warn(`!! missing source for position: ${JSON.stringify(position)}`)
         // const buildLine = buildLines[position.line - 1]
         console.warn(`   origin in build:`)
@@ -86,14 +105,27 @@ async function validateSourcemapForFile ({ buildName }) {
       const portion = line.slice(result.column)
       const isMaybeValid = portion.includes(targetString)
       if (!isMaybeValid) {
-        console.error('Sourcemap seems invalid:')
-        console.log(`\n========================== ${result.source} ====================================\n`)
-        console.log(line)
-        console.log(`\n==============================================================================\n`)
+        valid = false
+        console.error(`Sourcemap seems invalid:\n${getFencedCode(result.source, line)}`)
       }
     })
   })
   console.log(`  checked ${sampleCount} samples`)
+  return valid
+}
+
+const CODE_FENCE_LENGTH = 80
+const TITLE_PADDING_LENGTH = 1
+
+function getFencedCode (filename, code) {
+  const title = `${' '.repeat(TITLE_PADDING_LENGTH)}${filename}${' '.repeat(TITLE_PADDING_LENGTH)}`
+  const openingFenceLength = Math.max(CODE_FENCE_LENGTH - (filename.length + (TITLE_PADDING_LENGTH * 2)), 0)
+  const startOpeningFenceLength = Math.floor(openingFenceLength / 2)
+  const endOpeningFenceLength = Math.ceil(openingFenceLength / 2)
+  const openingFence = `${'='.repeat(startOpeningFenceLength)}${title}${'='.repeat(endOpeningFenceLength)}`
+  const closingFence = '='.repeat(CODE_FENCE_LENGTH)
+
+  return `${openingFence}\n${code}\n${closingFence}\n`
 }
 
 function indicesOf (substring, string) {

diff --git a/package.json b/package.json
@@ -37,6 +37,7 @@
     "lint:shellcheck": "./development/shellcheck.sh",
     "lint:styles": "stylelint '*/**/*.scss'",
     "lint:lockfile": "lockfile-lint --path yarn.lock --allowed-hosts npm yarn github.com codeload.github.com --empty-hostname false --allowed-schemes \"https:\" \"git+https:\"",
+    "validate-source-maps": "node ./development/sourcemap-validator.js",
     "verify-locales": "node ./development/verify-locale-strings.js",
     "verify-locales:fix": "node ./development/verify-locale-strings.js --fix",
     "mozilla-lint": "addons-linter dist/firefox",