From 9e5fec1d819a66d2aeeea4113cf7f84230ad2659 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Mon, 5 Mar 2018 12:44:27 -0500 Subject: [PATCH 01/21] Add `electron-unhandled` dependency --- package.json | 1 + yarn.lock | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/package.json b/package.json index d2101e3c4..dcc087ed4 100644 --- a/package.json +++ b/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": "^1.0.0", "electron-updater": "^2.21.0", "emoji-datasource": "4.0.0", "emoji-datasource-apple": "4.0.0", diff --git a/yarn.lock b/yarn.lock index 39d5b3ead..336eed740 100644 --- a/yarn.lock +++ b/yarn.lock @@ -785,6 +785,10 @@ circular-json@^0.3.1: version "0.3.3" resolved "https://registry.yarnpkg.com/circular-json/-/circular-json-0.3.3.tgz#815c99ea84f6809529d2f45791bdf82711352d66" +clean-stack@^1.3.0: + version "1.3.0" + resolved "https://registry.yarnpkg.com/clean-stack/-/clean-stack-1.3.0.tgz#9e821501ae979986c46b1d66d2d432db2fd4ae31" + cli-boxes@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/cli-boxes/-/cli-boxes-1.0.0.tgz#4fa917c3e59c94a004cd61f8ee509da651687143" @@ -1538,6 +1542,13 @@ electron-publisher-s3@^20.2.0: fs-extra-p "^4.5.2" mime "^2.2.0" +electron-unhandled@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/electron-unhandled/-/electron-unhandled-1.0.0.tgz#ecbc881c433cce053545072efab3647fcd40bfb0" + dependencies: + clean-stack "^1.3.0" + ensure-error "^1.0.0" + electron-updater@^2.21.0: version "2.21.0" resolved "https://registry.yarnpkg.com/electron-updater/-/electron-updater-2.21.0.tgz#3c8765af946090100f7df982127e4c3412cbc1af" @@ -1592,6 +1603,10 @@ end-of-stream@^1.0.0: dependencies: once "^1.4.0" +ensure-error@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/ensure-error/-/ensure-error-1.0.0.tgz#c54c13329132cad06f847423b52c22fd88099d0d" + entities@1.0: version "1.0.0" resolved "https://registry.yarnpkg.com/entities/-/entities-1.0.0.tgz#b2987aa3821347fcde642b24fdfc9e4fb712bf26" From 18785be639587142f133a367983ba6bc9938f4f7 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Mon, 5 Mar 2018 14:55:54 -0500 Subject: [PATCH 02/21] Log uncaught errors / promise rejections in main process See #2019. --- main.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/main.js b/main.js index a90ed0009..da28aa9e2 100644 --- a/main.js +++ b/main.js @@ -1,3 +1,4 @@ +const addUnhandledErrorHandler = require('electron-unhandled'); const path = require('path'); const url = require('url'); const os = require('os'); @@ -22,6 +23,16 @@ const logging = require('./app/logging'); const windowState = require('./app/window_state'); const { createTemplate } = require('./app/menu'); +addUnhandledErrorHandler({ + logger: (error) => { + console.log( + 'main.js: Uncaught error or unhandled promise rejection:', + error && error.stack ? error.stack : error + ); + }, + showDialog: false, +}); + const appUserModelId = `org.whispersystems.${packageJson.name}`; console.log('Set Windows Application User Model ID (AUMID)', { appUserModelId }); app.setAppUserModelId(appUserModelId); From 522b88d0d937271bdbcae0d8cc4b13f40a21a0b1 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Mon, 5 Mar 2018 15:00:41 -0500 Subject: [PATCH 03/21] Reduce scope of disabled `more/no-then` rule --- main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.js b/main.js index da28aa9e2..851a771c0 100644 --- a/main.js +++ b/main.js @@ -426,6 +426,7 @@ app.on('ready', () => { logging.initialize().catch((error) => { loggingSetupError = error; }).then(() => { + /* eslint-enable more/no-then */ logger = logging.getLogger(); logger.info('app ready'); @@ -450,7 +451,6 @@ app.on('ready', () => { setupMenu(); }); - /* eslint-enable more/no-then */ }); function setupMenu(options) { From 77e7e9ad4dd531c190c9f9dccd8a3f58e5d68887 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 11:26:22 -0500 Subject: [PATCH 04/21] Add `ensure-error` dependency --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index dcc087ed4..568f9f67c 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "emoji-datasource-apple": "4.0.0", "emoji-js": "^3.4.0", "emoji-panel": "https://github.com/scottnonnenberg/emoji-panel.git#v0.5.5", + "ensure-error": "^1.0.0", "firstline": "^1.2.1", "form-data": "^2.3.2", "google-libphonenumber": "^3.0.7", From be3e4d86c2be10a51559dc53305df6cfe5920b70 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:04:37 -0500 Subject: [PATCH 05/21] Add `Errors.toLogFormat` Allows errors to be formatted and sanitized for logging. Removes sensitive paths such as the app root directory. Ideally, this module would be called singular `Error` but that is already a global name. Using `Errors` plural is similar to Java convention for utilities such as `Arrays`, `Collections`, `Files`, etc. See: https://stackoverflow.com/a/11673838 --- js/modules/types/errors.js | 17 ++++++++++++++ test/modules/types/errors_test.js | 39 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 js/modules/types/errors.js create mode 100644 test/modules/types/errors_test.js diff --git a/js/modules/types/errors.js b/js/modules/types/errors.js new file mode 100644 index 000000000..7754bb15c --- /dev/null +++ b/js/modules/types/errors.js @@ -0,0 +1,17 @@ +/* eslint-env node */ + +const Path = require('path'); + +const toError = require('ensure-error'); + + +const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..'); +const APP_ROOT_PATH_PATTERN = new RegExp(APP_ROOT_PATH, 'g'); + +// toLogFormat :: Error -> String +exports.toLogFormat = (error) => { + const normalizedError = toError(error); + const stackWithoutPrivatePaths = + normalizedError.stack.replace(APP_ROOT_PATH_PATTERN, ''); + return stackWithoutPrivatePaths; +}; diff --git a/test/modules/types/errors_test.js b/test/modules/types/errors_test.js new file mode 100644 index 000000000..74a9f4e51 --- /dev/null +++ b/test/modules/types/errors_test.js @@ -0,0 +1,39 @@ +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 redact sensitive paths in stack trace', () => { + try { + throw new Error('boom'); + } catch (error) { + assert.include( + error.stack, + APP_ROOT_PATH, + 'Unformatted stack has sensitive paths' + ); + + const formattedStack = Errors.toLogFormat(error); + assert.notInclude( + formattedStack, + APP_ROOT_PATH, + 'Formatted stack does not have sensitive paths' + ); + assert.include( + formattedStack, + '', + 'Formatted stack has redactions' + ); + return; + } + // eslint-disable-next-line no-unreachable + assert.fail('Expected error to be thrown.'); + }); + }); +}); From 2575196617b32584d408480247e5753d9e739932 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:05:53 -0500 Subject: [PATCH 06/21] Extract `Errors.addGlobalHandler` procedure --- js/modules/errors.js | 17 +++++++++++++++++ main.js | 12 ++---------- 2 files changed, 19 insertions(+), 10 deletions(-) create mode 100644 js/modules/errors.js diff --git a/js/modules/errors.js b/js/modules/errors.js new file mode 100644 index 000000000..4a6bb7c3b --- /dev/null +++ b/js/modules/errors.js @@ -0,0 +1,17 @@ +const addUnhandledErrorHandler = require('electron-unhandled'); + +const Errors = require('./types/errors'); + + +// addGlobalHandler :: Unit -> Unit +exports.addGlobalHandler = () => { + addUnhandledErrorHandler({ + logger: (error) => { + console.error( + 'Uncaught error or unhandled promise rejection:', + Errors.toLogFormat(error) + ); + }, + showDialog: false, + }); +}; diff --git a/main.js b/main.js index 851a771c0..4108ebccd 100644 --- a/main.js +++ b/main.js @@ -1,4 +1,3 @@ -const addUnhandledErrorHandler = require('electron-unhandled'); const path = require('path'); const url = require('url'); const os = require('os'); @@ -19,19 +18,12 @@ const packageJson = require('./package.json'); const autoUpdate = require('./app/auto_update'); const createTrayIcon = require('./app/tray_icon'); +const Errors = require('./js/modules/errors'); const logging = require('./app/logging'); const windowState = require('./app/window_state'); const { createTemplate } = require('./app/menu'); -addUnhandledErrorHandler({ - logger: (error) => { - console.log( - 'main.js: Uncaught error or unhandled promise rejection:', - error && error.stack ? error.stack : error - ); - }, - showDialog: false, -}); +Errors.addGlobalHandler(); const appUserModelId = `org.whispersystems.${packageJson.name}`; console.log('Set Windows Application User Model ID (AUMID)', { appUserModelId }); From 3dffdc3f0babd084318442a3ba7c6834cd12c4d6 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:10:35 -0500 Subject: [PATCH 07/21] Rename `Errors` to `GlobalErrors` for clarity --- js/modules/{errors.js => global_errors.js} | 4 ++-- main.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) rename js/modules/{errors.js => global_errors.js} (81%) diff --git a/js/modules/errors.js b/js/modules/global_errors.js similarity index 81% rename from js/modules/errors.js rename to js/modules/global_errors.js index 4a6bb7c3b..b868bd0a5 100644 --- a/js/modules/errors.js +++ b/js/modules/global_errors.js @@ -3,8 +3,8 @@ const addUnhandledErrorHandler = require('electron-unhandled'); const Errors = require('./types/errors'); -// addGlobalHandler :: Unit -> Unit -exports.addGlobalHandler = () => { +// addHandler :: Unit -> Unit +exports.addHandler = () => { addUnhandledErrorHandler({ logger: (error) => { console.error( diff --git a/main.js b/main.js index 4108ebccd..0ea657e45 100644 --- a/main.js +++ b/main.js @@ -18,12 +18,12 @@ const packageJson = require('./package.json'); const autoUpdate = require('./app/auto_update'); const createTrayIcon = require('./app/tray_icon'); -const Errors = require('./js/modules/errors'); +const GlobalErrors = require('./js/modules/global_errors'); const logging = require('./app/logging'); const windowState = require('./app/window_state'); const { createTemplate } = require('./app/menu'); -Errors.addGlobalHandler(); +GlobalErrors.addHandler(); const appUserModelId = `org.whispersystems.${packageJson.name}`; console.log('Set Windows Application User Model ID (AUMID)', { appUserModelId }); From 289063b24bd70ab66e2e9c63c5ad401c5ef300be Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:17:06 -0500 Subject: [PATCH 08/21] Expose `Signal.Types.Errors` --- preload.js | 1 + 1 file changed, 1 insertion(+) diff --git a/preload.js b/preload.js index 9c99e38c0..ba280ab20 100644 --- a/preload.js +++ b/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'); From 33bbb12626854e87a230adb34972d09ed55f7601 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:17:48 -0500 Subject: [PATCH 09/21] Use `Errors.toLogFormat` in `backgrounds.js` --- js/background.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/js/background.js b/js/background.js index 506874823..f78836b36 100644 --- a/js/background.js +++ b/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', - error && error.stack ? error.stack : error + 'Invalid verified sync received:', + Errors.toLogFormat(error) ); return; } From 0e2f8a8a066949ed52e8e3a140e9c916872c9c5b Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:46:08 -0500 Subject: [PATCH 10/21] Extract `Errors.redactSensitivePaths` --- js/modules/types/errors.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/js/modules/types/errors.js b/js/modules/types/errors.js index 7754bb15c..43addcb2c 100644 --- a/js/modules/types/errors.js +++ b/js/modules/types/errors.js @@ -2,7 +2,8 @@ const Path = require('path'); -const toError = require('ensure-error'); +const ensureError = require('ensure-error'); +const isString = require('lodash/isString'); const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..'); @@ -10,8 +11,16 @@ const APP_ROOT_PATH_PATTERN = new RegExp(APP_ROOT_PATH, 'g'); // toLogFormat :: Error -> String exports.toLogFormat = (error) => { - const normalizedError = toError(error); - const stackWithoutPrivatePaths = - normalizedError.stack.replace(APP_ROOT_PATH_PATTERN, ''); - return stackWithoutPrivatePaths; + const normalizedError = ensureError(error); + const stackWithRedactedPaths = exports.redactSensitivePaths(normalizedError.stack); + return stackWithRedactedPaths; +}; + +// redactSensitivePaths :: String -> String +exports.redactSensitivePaths = (logLine) => { + if (!isString(logLine)) { + return logLine; + } + + return logLine.replace(APP_ROOT_PATH_PATTERN, ''); }; From 0c317c5498c4d583b0759f1e6467f0e3e3d9913d Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 12:46:38 -0500 Subject: [PATCH 11/21] Redact all private information from logs --- js/logging.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/js/logging.js b/js/logging.js index 45cd0a7e5..babf211a7 100644 --- a/js/logging.js +++ b/js/logging.js @@ -7,7 +7,7 @@ const bunyan = require('bunyan'); const _ = require('lodash'); const debuglogs = require('./modules/debuglogs'); - +const Errors = require('./modules/types/errors'); const ipc = electron.ipcRenderer; const PHONE_REGEX = /\+\d{7,12}(\d{3})/g; @@ -60,7 +60,8 @@ function log(...args) { return item; }); - const toSend = redactGroup(redactPhone(str.join(' '))); + + const toSend = redactAll(str.join(' ')); ipc.send('log-info', toSend); } @@ -95,7 +96,11 @@ function formatLine(entry) { } function format(entries) { - return redactGroup(redactPhone(entries.map(formatLine).join('\n'))); + return redactAll(entries.map(formatLine).join('\n'))); +} + +function redactAll(string) { + return Errors.redactSensitivePaths(redactGroup(redactPhone(string))); } function fetch() { From 49e0850fb24bab98736439db7a20a9c14433aca7 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 16:20:04 -0500 Subject: [PATCH 12/21] Extract `Privacy` module Centralizes how we redact sensitive information. --- js/logging.js | 26 +++------------ js/modules/privacy.js | 53 +++++++++++++++++++++++++++++++ js/modules/types/errors.js | 21 ++---------- test/modules/types/errors_test.js | 2 +- 4 files changed, 60 insertions(+), 42 deletions(-) create mode 100644 js/modules/privacy.js diff --git a/js/logging.js b/js/logging.js index babf211a7..15a38b30d 100644 --- a/js/logging.js +++ b/js/logging.js @@ -8,10 +8,9 @@ const _ = require('lodash'); const debuglogs = require('./modules/debuglogs'); const Errors = require('./modules/types/errors'); +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 +24,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(); @@ -61,8 +47,8 @@ function log(...args) { return item; }); - const toSend = redactAll(str.join(' ')); - ipc.send('log-info', toSend); + const logText = Privacy.redactAll(str.join(' ')); + ipc.send('log-info', logText); } if (window.console) { @@ -96,11 +82,7 @@ function formatLine(entry) { } function format(entries) { - return redactAll(entries.map(formatLine).join('\n'))); -} - -function redactAll(string) { - return Errors.redactSensitivePaths(redactGroup(redactPhone(string))); + return Privacy.redactAll(entries.map(formatLine).join('\n')); } function fetch() { diff --git a/js/modules/privacy.js b/js/modules/privacy.js new file mode 100644 index 000000000..55096daee --- /dev/null +++ b/js/modules/privacy.js @@ -0,0 +1,53 @@ +/* eslint-env node */ + +const Path = require('path'); + +const isString = require('lodash/isString'); +const compose = require('lodash/fp/compose'); + + +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 = new RegExp(APP_ROOT_PATH, 'g'); + +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'); + } + + return text.replace(APP_ROOT_PATH_PATTERN, REDACTION_PLACEHOLDER); +}; + +// redactAll :: String -> String +exports.redactAll = compose( + exports.redactSensitivePaths, + exports.redactGroupIds, + exports.redactPhoneNumbers +); diff --git a/js/modules/types/errors.js b/js/modules/types/errors.js index 43addcb2c..52252fb1f 100644 --- a/js/modules/types/errors.js +++ b/js/modules/types/errors.js @@ -1,26 +1,9 @@ -/* eslint-env node */ - -const Path = require('path'); - const ensureError = require('ensure-error'); -const isString = require('lodash/isString'); - -const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..'); -const APP_ROOT_PATH_PATTERN = new RegExp(APP_ROOT_PATH, 'g'); +const Privacy = require('../privacy'); // toLogFormat :: Error -> String exports.toLogFormat = (error) => { const normalizedError = ensureError(error); - const stackWithRedactedPaths = exports.redactSensitivePaths(normalizedError.stack); - return stackWithRedactedPaths; -}; - -// redactSensitivePaths :: String -> String -exports.redactSensitivePaths = (logLine) => { - if (!isString(logLine)) { - return logLine; - } - - return logLine.replace(APP_ROOT_PATH_PATTERN, ''); + return Privacy.redactAll(normalizedError.stack); }; diff --git a/test/modules/types/errors_test.js b/test/modules/types/errors_test.js index 74a9f4e51..77dfc82f8 100644 --- a/test/modules/types/errors_test.js +++ b/test/modules/types/errors_test.js @@ -27,7 +27,7 @@ describe('Errors', () => { ); assert.include( formattedStack, - '', + '[REDACTED]', 'Formatted stack has redactions' ); return; From e71246a9e31946ffff421803804405d49a738ec0 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Tue, 6 Mar 2018 16:25:43 -0500 Subject: [PATCH 13/21] Add tests for `Privacy` module --- test/modules/privacy_test.js | 56 ++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 test/modules/privacy_test.js diff --git a/test/modules/privacy_test.js b/test/modules/privacy_test.js new file mode 100644 index 000000000..156d3e826 --- /dev/null +++ b/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); + }); + }); +}); From 44b81f68dd1d37d780a26e8c5e20cad24e76fcdb Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Wed, 7 Mar 2018 10:57:04 -0500 Subject: [PATCH 14/21] Remove privacy redaction from `Errors.toLogFormat` --- js/modules/types/errors.js | 4 +--- test/modules/types/errors_test.js | 37 ++++++++++++++++++------------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/js/modules/types/errors.js b/js/modules/types/errors.js index 52252fb1f..eba76af47 100644 --- a/js/modules/types/errors.js +++ b/js/modules/types/errors.js @@ -1,9 +1,7 @@ const ensureError = require('ensure-error'); -const Privacy = require('../privacy'); - // toLogFormat :: Error -> String exports.toLogFormat = (error) => { const normalizedError = ensureError(error); - return Privacy.redactAll(normalizedError.stack); + return normalizedError.stack; }; diff --git a/test/modules/types/errors_test.js b/test/modules/types/errors_test.js index 77dfc82f8..945ce68ee 100644 --- a/test/modules/types/errors_test.js +++ b/test/modules/types/errors_test.js @@ -9,31 +9,36 @@ const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..'); describe('Errors', () => { describe('toLogFormat', () => { - it('should redact sensitive paths in stack trace', () => { + it('should convert non-errors to errors', () => { try { - throw new Error('boom'); - } catch (error) { - assert.include( - error.stack, - APP_ROOT_PATH, - 'Unformatted stack has sensitive paths' - ); + // eslint-disable-next-line no-throw-literal + throw 'boom'; + } catch (nonError) { + assert.typeOf(nonError, 'string'); + assert.isUndefined(nonError.stack); - const formattedStack = Errors.toLogFormat(error); - assert.notInclude( - formattedStack, - APP_ROOT_PATH, - 'Formatted stack does not have sensitive paths' - ); + const formattedStack = Errors.toLogFormat(nonError); assert.include( formattedStack, - '[REDACTED]', - 'Formatted stack has redactions' + APP_ROOT_PATH, + 'Formatted stack has app path' ); return; } + // eslint-disable-next-line no-unreachable assert.fail('Expected error to be thrown.'); }); + + it('should add stack to errors without one', () => { + const error = new Error('boom'); + error.stack = null; + assert.typeOf(error, 'Error'); + assert.isNull(error.stack); + + const formattedStack = Errors.toLogFormat(error); + assert.include(formattedStack, ''); + assert.include(formattedStack, APP_ROOT_PATH, 'Formatted stack has app path'); + }); }); }); From ea07915e6bbd68aceb0d40c7e2fd91cd2542b244 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Wed, 7 Mar 2018 10:57:39 -0500 Subject: [PATCH 15/21] Escape special characters in file path --- js/modules/privacy.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/js/modules/privacy.js b/js/modules/privacy.js index 55096daee..db0529376 100644 --- a/js/modules/privacy.js +++ b/js/modules/privacy.js @@ -2,15 +2,25 @@ const Path = require('path'); -const isString = require('lodash/isString'); 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 = new RegExp(APP_ROOT_PATH, 'g'); +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]'; @@ -42,6 +52,10 @@ exports.redactSensitivePaths = (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); }; From 43b47fd3493d3513dd46545f1616e7a6ccfaf55a Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 14:06:22 -0500 Subject: [PATCH 16/21] Avoid `ensure-error` for privacy reasons Example: ``` > node > const nonError = {foo: 'i-am-private'}; undefined // before > util.inspect(nonError); '{ foo: \'i-am-private\' }' // after > nonError.toString() '[object Object]' > ``` --- js/modules/types/errors.js | 13 +++++++--- test/modules/types/errors_test.js | 43 +++++++++++++++---------------- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/js/modules/types/errors.js b/js/modules/types/errors.js index eba76af47..0914eacc2 100644 --- a/js/modules/types/errors.js +++ b/js/modules/types/errors.js @@ -1,7 +1,12 @@ -const ensureError = require('ensure-error'); - // toLogFormat :: Error -> String exports.toLogFormat = (error) => { - const normalizedError = ensureError(error); - return normalizedError.stack; + if (!error) { + return error; + } + + if (error && error.stack) { + return error.stack; + } + + return error.toString(); }; diff --git a/test/modules/types/errors_test.js b/test/modules/types/errors_test.js index 945ce68ee..9cca14062 100644 --- a/test/modules/types/errors_test.js +++ b/test/modules/types/errors_test.js @@ -9,36 +9,35 @@ const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..'); describe('Errors', () => { describe('toLogFormat', () => { - it('should convert non-errors to errors', () => { - try { - // eslint-disable-next-line no-throw-literal - throw 'boom'; - } catch (nonError) { - assert.typeOf(nonError, 'string'); - assert.isUndefined(nonError.stack); + it('should return error stack trace if present', () => { + const error = new Error('boom'); + assert.typeOf(error, 'Error'); - const formattedStack = Errors.toLogFormat(nonError); - assert.include( - formattedStack, - APP_ROOT_PATH, - 'Formatted stack has app path' - ); - return; - } - - // eslint-disable-next-line no-unreachable - assert.fail('Expected error to be thrown.'); + const formattedError = Errors.toLogFormat(error); + assert.include(formattedError, 'errors_test.js'); + assert.include(formattedError, APP_ROOT_PATH, 'Formatted stack has app path'); }); - it('should add stack to errors without one', () => { + 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 formattedStack = Errors.toLogFormat(error); - assert.include(formattedStack, ''); - assert.include(formattedStack, APP_ROOT_PATH, 'Formatted stack has app path'); + 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); + }); }); }); }); From ef40dfa8412c25b2ea906fc69eb73c700fdfb701 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 15:04:24 -0500 Subject: [PATCH 17/21] Use forked `electron-unhandled` Omits use of `ensure-error` for privacy. Before: `Uncaught error or unhandled promise rejection: NonError: { private: true }` After: `Uncaught error or unhandled promise rejection: [object Object]` Tested using: ``` setTimeout(() => { throw new Error('sync: booooom!'); }, 5000); setTimeout(() => { Promise.reject(new Error('async: promise')) }, 10000); setTimeout(() => { Promise.reject({private: true}); }, 15000); ``` --- package.json | 2 +- yarn.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 568f9f67c..7e4c866f0 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "electron-config": "^1.0.0", "electron-editor-context-menu": "^1.1.1", "electron-is-dev": "^0.3.0", - "electron-unhandled": "^1.0.0", + "electron-unhandled": "https://github.com/gasi/electron-unhandled.git#dfb5d7688c3e1659ace7beaabacc0f547488dea1", "electron-updater": "^2.21.0", "emoji-datasource": "4.0.0", "emoji-datasource-apple": "4.0.0", diff --git a/yarn.lock b/yarn.lock index 336eed740..a73149b7e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1542,9 +1542,9 @@ electron-publisher-s3@^20.2.0: fs-extra-p "^4.5.2" mime "^2.2.0" -electron-unhandled@^1.0.0: +"electron-unhandled@https://github.com/gasi/electron-unhandled.git#dfb5d7688c3e1659ace7beaabacc0f547488dea1": version "1.0.0" - resolved "https://registry.yarnpkg.com/electron-unhandled/-/electron-unhandled-1.0.0.tgz#ecbc881c433cce053545072efab3647fcd40bfb0" + resolved "https://github.com/gasi/electron-unhandled.git#dfb5d7688c3e1659ace7beaabacc0f547488dea1" dependencies: clean-stack "^1.3.0" ensure-error "^1.0.0" From 96442967cc4b0c6c8f38c820e703fef915e8bb4f Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 15:49:19 -0500 Subject: [PATCH 18/21] Add `eslint-plugin-mocha` Allows us to set various lint rules for our tests. --- package.json | 1 + yarn.lock | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/package.json b/package.json index 7e4c866f0..ac3d510f1 100644 --- a/package.json +++ b/package.json @@ -87,6 +87,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", diff --git a/yarn.lock b/yarn.lock index a73149b7e..cf16b1c0d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1693,6 +1693,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" @@ -4329,6 +4335,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" From c7305db8c89392290da27dff2dba8b1f9a8020ad Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 15:49:34 -0500 Subject: [PATCH 19/21] Disallow exclusive tests, i.e. `.only` --- .eslintrc.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.eslintrc.js b/.eslintrc.js index 124289e01..5f0bcfb1e 100644 --- a/.eslintrc.js +++ b/.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', From c317f34f67d10211690ad55271e9043f2ec7e826 Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 17:38:05 -0500 Subject: [PATCH 20/21] Remove `ensure-error` and `clean-error` dependency It potentially could leak information from serialized non-errors that are thrown due to `util.inspect`. --- package.json | 3 +-- yarn.lock | 15 ++------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/package.json b/package.json index ac3d510f1..4e40bd8c1 100644 --- a/package.json +++ b/package.json @@ -52,13 +52,12 @@ "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#dfb5d7688c3e1659ace7beaabacc0f547488dea1", + "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", "emoji-js": "^3.4.0", "emoji-panel": "https://github.com/scottnonnenberg/emoji-panel.git#v0.5.5", - "ensure-error": "^1.0.0", "firstline": "^1.2.1", "form-data": "^2.3.2", "google-libphonenumber": "^3.0.7", diff --git a/yarn.lock b/yarn.lock index cf16b1c0d..f08d6c6b2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -785,10 +785,6 @@ circular-json@^0.3.1: version "0.3.3" resolved "https://registry.yarnpkg.com/circular-json/-/circular-json-0.3.3.tgz#815c99ea84f6809529d2f45791bdf82711352d66" -clean-stack@^1.3.0: - version "1.3.0" - resolved "https://registry.yarnpkg.com/clean-stack/-/clean-stack-1.3.0.tgz#9e821501ae979986c46b1d66d2d432db2fd4ae31" - cli-boxes@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/cli-boxes/-/cli-boxes-1.0.0.tgz#4fa917c3e59c94a004cd61f8ee509da651687143" @@ -1542,12 +1538,9 @@ 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#dfb5d7688c3e1659ace7beaabacc0f547488dea1": +"electron-unhandled@https://github.com/gasi/electron-unhandled.git#1edf81fe542e505368fafaeef27609dc21678f8c": version "1.0.0" - resolved "https://github.com/gasi/electron-unhandled.git#dfb5d7688c3e1659ace7beaabacc0f547488dea1" - dependencies: - clean-stack "^1.3.0" - ensure-error "^1.0.0" + resolved "https://github.com/gasi/electron-unhandled.git#1edf81fe542e505368fafaeef27609dc21678f8c" electron-updater@^2.21.0: version "2.21.0" @@ -1603,10 +1596,6 @@ end-of-stream@^1.0.0: dependencies: once "^1.4.0" -ensure-error@^1.0.0: - version "1.0.0" - resolved "https://registry.yarnpkg.com/ensure-error/-/ensure-error-1.0.0.tgz#c54c13329132cad06f847423b52c22fd88099d0d" - entities@1.0: version "1.0.0" resolved "https://registry.yarnpkg.com/entities/-/entities-1.0.0.tgz#b2987aa3821347fcde642b24fdfc9e4fb712bf26" From 8a1bff0fadfef4ceffb1a6ae898c6e234c8e3e6d Mon Sep 17 00:00:00 2001 From: Daniel Gasienica Date: Thu, 8 Mar 2018 17:50:07 -0500 Subject: [PATCH 21/21] Remove unused `require` --- js/logging.js | 1 - 1 file changed, 1 deletion(-) diff --git a/js/logging.js b/js/logging.js index 15a38b30d..2538503b0 100644 --- a/js/logging.js +++ b/js/logging.js @@ -7,7 +7,6 @@ const bunyan = require('bunyan'); const _ = require('lodash'); const debuglogs = require('./modules/debuglogs'); -const Errors = require('./modules/types/errors'); const Privacy = require('./modules/privacy'); const ipc = electron.ipcRenderer;