Merge #2110 from gasi: Log Uncaught Errors & Unhandled Promise Rejections In Main Process

- [x] Add `electron-unhandled` dependency:
  - ~~Ensures errors are normalized~~ (disabled to prevent serializing non-errors that are thrown and leaking information)
  - Distinguishes between main and renderer processes
  - Allows suppression of error dialog
- [x] Log uncaught errors and unhandled promise rejections in main process
- [x] Tested using unguarded `throw new TyperError(…)` and `Promise.reject(…)` in `setTimeout` after `app` `ready` event.
- [x] Extract `Privacy` module that centralizes how we redact sensitive information such as phone numbers, group IDs, and user file paths.
- [x] Add `eslint-plugin-mocha` to disallow exclusive tests using `*.only`.

Fixes #2019.

.eslintrc.js

@@ -12,6 +12,7 @@ module.exports = {
   ],
 
   plugins: [
+    'mocha',
     'more',
   ],
 
@@ -33,6 +34,9 @@ module.exports = {
       ignoreUrls: true,
     }],
 
+    // prevents us from accidentally checking in exclusive tests (`.only`):
+    'mocha/no-exclusive-tests': 'error',
+
     // encourage consistent use of `async` / `await` instead of `then`
     'more/no-then': 'error',
 

js/background.js

@@ -14,7 +14,7 @@
 ;(function() {
     'use strict';
 
-    const { Message } = window.Signal.Types;
+    const { Errors, Message } = window.Signal.Types;
 
     // Implicitly used in `indexeddb-backbonejs-adapter`:
     // https://github.com/signalapp/Signal-Desktop/blob/4033a9f8137e62ed286170ed5d4941982b1d3a64/components/indexeddb-backbonejs-adapter/backbone-indexeddb.js#L569
@@ -414,7 +414,7 @@
         });
         var error = c.validateNumber();
         if (error) {
-            console.log('Invalid contact received', error && error.stack ? error.stack : error);
+            console.log('Invalid contact received:', Errors.toLogFormat(error));
             return;
         }
 
@@ -483,7 +483,7 @@
             .catch(function(error) {
                 console.log(
                     'onContactReceived error:',
-                    error && error.stack ? error.stack : error
+                    Errors.toLogFormat(error)
                 );
             });
     }
@@ -670,7 +670,7 @@
                 return resolve(false);
             });
         }).catch(function(error) {
-            console.log('isMessageDuplicate error:', error && error.stack ? error.stack : error);
+            console.log('isMessageDuplicate error:', Errors.toLogFormat(error));
             return false;
         });
     }
@@ -691,7 +691,7 @@
 
     function onError(ev) {
         var error = ev.error;
-        console.log('background onError:', error && error.stack ? error.stack : error);
+        console.log('background onError:', Errors.toLogFormat(error));
 
         if (error.name === 'HTTPError' && (error.code == 401 || error.code == 403)) {
             Whisper.Registration.remove();
@@ -804,8 +804,8 @@
         var error = c.validateNumber();
         if (error) {
             console.log(
-                'Invalid verified sync received',
+                'Invalid verified sync received:',
-                error && error.stack ? error.stack : error
+                Errors.toLogFormat(error)
             );
             return;
         }

js/logging.js

@@ -7,11 +7,9 @@ const bunyan = require('bunyan');
 const _ = require('lodash');
 
 const debuglogs = require('./modules/debuglogs');
-
+const Privacy = require('./modules/privacy');
 
 const ipc = electron.ipcRenderer;
-const PHONE_REGEX = /\+\d{7,12}(\d{3})/g;
-const GROUP_REGEX = /(group\()([^)]+)(\))/g;
 
 // Default Bunyan levels: https://github.com/trentm/node-bunyan#levels
 // To make it easier to visually scan logs, we make all levels the same length
@@ -25,20 +23,7 @@ const LEVELS = {
   10: 'trace',
 };
 
-
 // Backwards-compatible logging, simple strings and no level (defaulted to INFO)
-
-function redactPhone(text) {
-  return text.replace(PHONE_REGEX, '+[REDACTED]$1');
-}
-
-function redactGroup(text) {
-  return text.replace(
-    GROUP_REGEX,
-    (match, before, id, after) => `${before}[REDACTED]${id.slice(-3)}${after}`
-  );
-}
-
 function now() {
   const date = new Date();
   return date.toJSON();
@@ -60,8 +45,9 @@ function log(...args) {
 
     return item;
   });
-  const toSend = redactGroup(redactPhone(str.join(' ')));
+
-  ipc.send('log-info', toSend);
+  const logText = Privacy.redactAll(str.join(' '));
+  ipc.send('log-info', logText);
 }
 
 if (window.console) {
@@ -95,7 +81,7 @@ function formatLine(entry) {
 }
 
 function format(entries) {
-  return redactGroup(redactPhone(entries.map(formatLine).join('\n')));
+  return Privacy.redactAll(entries.map(formatLine).join('\n'));
 }
 
 function fetch() {

js/modules/global_errors.js

@@ -0,0 +1,17 @@
+const addUnhandledErrorHandler = require('electron-unhandled');
+
+const Errors = require('./types/errors');
+
+
+//      addHandler :: Unit -> Unit
+exports.addHandler = () => {
+  addUnhandledErrorHandler({
+    logger: (error) => {
+      console.error(
+        'Uncaught error or unhandled promise rejection:',
+        Errors.toLogFormat(error)
+      );
+    },
+    showDialog: false,
+  });
+};

js/modules/privacy.js

@@ -0,0 +1,67 @@
+/* eslint-env node */
+
+const Path = require('path');
+
+const compose = require('lodash/fp/compose');
+const escapeRegExp = require('lodash/escapeRegExp');
+const isRegExp = require('lodash/isRegExp');
+const isString = require('lodash/isString');
+
+
+const PHONE_NUMBER_PATTERN = /\+\d{7,12}(\d{3})/g;
+const GROUP_ID_PATTERN = /(group\()([^)]+)(\))/g;
+
+const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..');
+const APP_ROOT_PATH_PATTERN = (() => {
+  try {
+    // Safe `String::replaceAll`:
+    // https://github.com/lodash/lodash/issues/1084#issuecomment-86698786
+    return new RegExp(escapeRegExp(APP_ROOT_PATH), 'g');
+  } catch (error) {
+    return null;
+  }
+})();
+
+const REDACTION_PLACEHOLDER = '[REDACTED]';
+
+//      redactPhoneNumbers :: String -> String
+exports.redactPhoneNumbers = (text) => {
+  if (!isString(text)) {
+    throw new TypeError('`text` must be a string');
+  }
+
+  return text.replace(PHONE_NUMBER_PATTERN, `+${REDACTION_PLACEHOLDER}$1`);
+};
+
+//      redactGroupIds :: String -> String
+exports.redactGroupIds = (text) => {
+  if (!isString(text)) {
+    throw new TypeError('`text` must be a string');
+  }
+
+  return text.replace(
+    GROUP_ID_PATTERN,
+    (match, before, id, after) =>
+      `${before}${REDACTION_PLACEHOLDER}${id.slice(-3)}${after}`
+  );
+};
+
+//      redactSensitivePaths :: String -> String
+exports.redactSensitivePaths = (text) => {
+  if (!isString(text)) {
+    throw new TypeError('`text` must be a string');
+  }
+
+  if (!isRegExp(APP_ROOT_PATH_PATTERN)) {
+    return text;
+  }
+
+  return text.replace(APP_ROOT_PATH_PATTERN, REDACTION_PLACEHOLDER);
+};
+
+//      redactAll :: String -> String
+exports.redactAll = compose(
+  exports.redactSensitivePaths,
+  exports.redactGroupIds,
+  exports.redactPhoneNumbers
+);

js/modules/types/errors.js

@@ -0,0 +1,12 @@
+//      toLogFormat :: Error -> String
+exports.toLogFormat = (error) => {
+  if (!error) {
+    return error;
+  }
+
+  if (error && error.stack) {
+    return error.stack;
+  }
+
+  return error.toString();
+};

main.js

@@ -18,10 +18,13 @@ const packageJson = require('./package.json');
 
 const autoUpdate = require('./app/auto_update');
 const createTrayIcon = require('./app/tray_icon');
+const GlobalErrors = require('./js/modules/global_errors');
 const logging = require('./app/logging');
 const windowState = require('./app/window_state');
 const { createTemplate } = require('./app/menu');
 
+GlobalErrors.addHandler();
+
 const appUserModelId = `org.whispersystems.${packageJson.name}`;
 console.log('Set Windows Application User Model ID (AUMID)', { appUserModelId });
 app.setAppUserModelId(appUserModelId);
@@ -415,6 +418,7 @@ app.on('ready', () => {
   logging.initialize().catch((error) => {
     loggingSetupError = error;
   }).then(() => {
+  /* eslint-enable more/no-then */
     logger = logging.getLogger();
     logger.info('app ready');
 
@@ -439,7 +443,6 @@ app.on('ready', () => {
 
     setupMenu();
   });
-  /* eslint-enable more/no-then */
 });
 
 function setupMenu(options) {

package.json

@@ -52,6 +52,7 @@
     "electron-config": "^1.0.0",
     "electron-editor-context-menu": "^1.1.1",
     "electron-is-dev": "^0.3.0",
+    "electron-unhandled": "https://github.com/gasi/electron-unhandled.git#1edf81fe542e505368fafaeef27609dc21678f8c",
     "electron-updater": "^2.21.0",
     "emoji-datasource": "4.0.0",
     "emoji-datasource-apple": "4.0.0",
@@ -85,6 +86,7 @@
     "eslint": "^4.14.0",
     "eslint-config-airbnb-base": "^12.1.0",
     "eslint-plugin-import": "^2.8.0",
+    "eslint-plugin-mocha": "^4.12.1",
     "eslint-plugin-more": "^0.3.1",
     "extract-zip": "^1.6.6",
     "grunt": "^1.0.1",

preload.js

@@ -104,6 +104,7 @@
   window.Signal.OS = require('./js/modules/os');
   window.Signal.Types = window.Signal.Types || {};
   window.Signal.Types.Attachment = require('./js/modules/types/attachment');
+  window.Signal.Types.Errors = require('./js/modules/types/errors');
   window.Signal.Types.Message = require('./js/modules/types/message');
   window.Signal.Types.MIME = require('./js/modules/types/mime');
   window.Signal.Types.Settings = require('./js/modules/types/settings');

test/modules/privacy_test.js

@@ -0,0 +1,56 @@
+const Path = require('path');
+
+const { assert } = require('chai');
+
+const Privacy = require('../../js/modules/privacy');
+
+
+const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..');
+
+describe('Privacy', () => {
+  describe('redactPhoneNumbers', () => {
+    it('should redact all phone numbers', () => {
+      const text = 'This is a log line with a phone number +12223334455\n' +
+        'and another one +13334445566';
+
+      const actual = Privacy.redactPhoneNumbers(text);
+      const expected = 'This is a log line with a phone number +[REDACTED]455\n' +
+        'and another one +[REDACTED]566';
+      assert.equal(actual, expected);
+    });
+  });
+
+  describe('redactGroupIds', () => {
+    it('should redact all group IDs', () => {
+      const text = 'This is a log line with two group IDs: group(123456789)\n' +
+        'and group(abcdefghij)';
+
+      const actual = Privacy.redactGroupIds(text);
+      const expected = 'This is a log line with two group IDs: group([REDACTED]789)\n' +
+        'and group([REDACTED]hij)';
+      assert.equal(actual, expected);
+    });
+  });
+
+  describe('redactAll', () => {
+    it('should redact all sensitive information', () => {
+      const text = 'This is a log line with sensitive information:\n' +
+        `path1 ${APP_ROOT_PATH}/main.js\n` +
+        'phone1 +12223334455 ipsum\n' +
+        'group1 group(123456789) doloret\n' +
+        `path2 file:///${APP_ROOT_PATH}/js/background.js.` +
+        'phone2 +13334445566 lorem\n' +
+        'group2 group(abcdefghij) doloret\n';
+
+      const actual = Privacy.redactAll(text);
+      const expected = 'This is a log line with sensitive information:\n' +
+        'path1 [REDACTED]/main.js\n' +
+        'phone1 +[REDACTED]455 ipsum\n' +
+        'group1 group([REDACTED]789) doloret\n' +
+        'path2 file:///[REDACTED]/js/background.js.' +
+        'phone2 +[REDACTED]566 lorem\n' +
+        'group2 group([REDACTED]hij) doloret\n';
+      assert.equal(actual, expected);
+    });
+  });
+});

test/modules/types/errors_test.js

@@ -0,0 +1,43 @@
+const Path = require('path');
+
+const { assert } = require('chai');
+
+const Errors = require('../../../js/modules/types/errors');
+
+
+const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..');
+
+describe('Errors', () => {
+  describe('toLogFormat', () => {
+    it('should return error stack trace if present', () => {
+      const error = new Error('boom');
+      assert.typeOf(error, 'Error');
+
+      const formattedError = Errors.toLogFormat(error);
+      assert.include(formattedError, 'errors_test.js');
+      assert.include(formattedError, APP_ROOT_PATH, 'Formatted stack has app path');
+    });
+
+    it('should return error string representation if stack is missing', () => {
+      const error = new Error('boom');
+      error.stack = null;
+      assert.typeOf(error, 'Error');
+      assert.isNull(error.stack);
+
+      const formattedError = Errors.toLogFormat(error);
+      assert.strictEqual(formattedError, 'Error: boom');
+    });
+
+    [
+      0,
+      false,
+      null,
+      undefined,
+    ].forEach((value) => {
+      it(`should return \`${value}\` argument`, () => {
+        const formattedNonError = Errors.toLogFormat(value);
+        assert.strictEqual(formattedNonError, value);
+      });
+    });
+  });
+});

yarn.lock

@@ -1538,6 +1538,10 @@ electron-publisher-s3@^20.2.0:
     fs-extra-p "^4.5.2"
     mime "^2.2.0"
 
+"electron-unhandled@https://github.com/gasi/electron-unhandled.git#1edf81fe542e505368fafaeef27609dc21678f8c":
+  version "1.0.0"
+  resolved "https://github.com/gasi/electron-unhandled.git#1edf81fe542e505368fafaeef27609dc21678f8c"
+
 electron-updater@^2.21.0:
   version "2.21.0"
   resolved "https://registry.yarnpkg.com/electron-updater/-/electron-updater-2.21.0.tgz#3c8765af946090100f7df982127e4c3412cbc1af"
@@ -1678,6 +1682,12 @@ eslint-plugin-import@^2.8.0:
     minimatch "^3.0.3"
     read-pkg-up "^2.0.0"
 
+eslint-plugin-mocha@^4.12.1:
+  version "4.12.1"
+  resolved "https://registry.yarnpkg.com/eslint-plugin-mocha/-/eslint-plugin-mocha-4.12.1.tgz#dbacc543b178b4536ec5b19d7f8e8864d85404bf"
+  dependencies:
+    ramda "^0.25.0"
+
 eslint-plugin-more@^0.3.1:
   version "0.3.1"
   resolved "https://registry.yarnpkg.com/eslint-plugin-more/-/eslint-plugin-more-0.3.1.tgz#ff688fb3fa8f153c8bfd5d70c15a68dc222a1b31"
@@ -4314,6 +4324,10 @@ querystring@0.2.0:
   version "0.2.0"
   resolved "https://registry.yarnpkg.com/querystring/-/querystring-0.2.0.tgz#b209849203bb25df820da756e747005878521620"
 
+ramda@^0.25.0:
+  version "0.25.0"
+  resolved "https://registry.yarnpkg.com/ramda/-/ramda-0.25.0.tgz#8fdf68231cffa90bc2f9460390a0cb74a29b29a9"
+
 randomatic@^1.1.3:
   version "1.1.7"
   resolved "https://registry.yarnpkg.com/randomatic/-/randomatic-1.1.7.tgz#c7abe9cc8b87c0baa876b19fde83fd464797e38c"