Log Uncaught Errors & Unhandled Promise Rejections In Main Process #2110

gasi · 2018-03-05T20:07:38Z

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
Log uncaught errors and unhandled promise rejections in main process
Tested using unguarded throw new TyperError(…) and Promise.reject(…) in setTimeout after app ready event.
Extract Privacy module that centralizes how we redact sensitive information such as phone numbers, group IDs, and user file paths.
Add eslint-plugin-mocha to disallow exclusive tests using *.only.

Fixes #2019.

scottnonnenberg

Short and sweet. Think we could do just a little bit better, though. :0)

scottnonnenberg · 2018-03-05T20:09:18Z

main.js

@@ -1,3 +1,4 @@
+const addUnhandledErrorHandler = require('electron-unhandled');


I would like us to separate built-ins (fs, path) from third-party node modules, from our own stuff. Can we move this to the node modules location?

Sure, no problem.

scottnonnenberg · 2018-03-05T20:12:14Z

main.js

+  logger: (error) => {
+    console.log(
+      'main.js: Uncaught error or unhandled promise rejection:',
+      error && error.stack ? error.stack : error


This might be a good opportunity to spend the time to remove the current directory from any paths on the call stack, so sensitive information doesn't get into the logs.

Good idea. Let me look into that.

scottnonnenberg · 2018-03-05T20:13:06Z

main.js

@@ -415,6 +426,7 @@ app.on('ready', () => {
  logging.initialize().catch((error) => {
    loggingSetupError = error;
  }).then(() => {
+  /* eslint-enable more/no-then */


What's happening here? Despite the title of the commit ('reducing the scope'), it seems like we're increasing its scope since we're enabling it higher in the file.

Yes, we reduce the places where we can use then, encouraging new code to use async / await.

gasi · 2018-03-06T21:31:03Z

@scottnonnenberg @scottnonnenberg-signal: Thanks for the code review. Addressed your feedback, incl. redacting sensitive file paths. Didn’t rebase but here’s a link to latest changes just in case: https://github.com/signalapp/Signal-Desktop/pull/2110/files/e3c9a81ba006f7fbebe9fe2e14299423effac665..75208f89d075de69dbfa3f08cf2ccd81e33b926d

scottnonnenberg

So great to get centralized Error handling and callstack redaction in place! A few things to consider:

scottnonnenberg · 2018-03-06T21:40:20Z

js/background.js

@@ -416,7 +416,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));


I just had a look at the ensure-error code, and it doesn't have the same behavior as our previous approach. Before, we looked for .stack and if we didn't find it, we would rely on the toString() of whatever we got. ensure-error does a util.inspect() on the incoming thing and creates a new callstack.

Seems like in most situations we'd be alright, perhaps even better than we were before. At least in terms of debuggability. But in some cases we'd show a whole lot more data than we did before since util.inspect() will enumerate keys while toString() didn't. That might have privacy implications.

Thanks for digging in. I saw the behavior slightly changed but didn’t notice any exposure so far. Let’s keep an eye on it during the beta.

Let me know what specific examples are that we have to be aware of. I am also happy to revert to error && error.stack ? error.stack : error.

scottnonnenberg · 2018-03-06T21:41:03Z

js/modules/types/errors.js

+//      toLogFormat :: Error -> String
+exports.toLogFormat = (error) => {
+  const normalizedError = ensureError(error);
+  return Privacy.redactAll(normalizedError.stack);


Do we need to redact here, if we're already redacting in logging.js?

Sure; I must have added this before the general redaction in the logging module.

scottnonnenberg · 2018-03-06T21:43:39Z

js/modules/privacy.js

+const GROUP_ID_PATTERN = /(group\()([^)]+)(\))/g;
+
+const APP_ROOT_PATH = Path.join(__dirname, '..', '..', '..');
+const APP_ROOT_PATH_PATTERN = new RegExp(APP_ROOT_PATH, 'g');


We're creating a regular expression from an arbitrary incoming path. Seems like we may want to put some protections in here to prevent crashes. Or perhaps rely on a recursive string replace instead of using regular expressions.

Also, doesn't the path.join() code above create a path ending with ../../..? path.resolve() should help with that.

Re: RegExp. Good call on being more defensive around creating the RegExp from a user supplied file path. Followed JDD’s advice and added _.escapeRegExp: lodash/lodash#1084 (comment). Wish there was simply a String::replaceAll function.

Re: path.join. Confirmed it does the right thing and normalizes the path to not have ../../... Otherwise, the tests wouldn’t have passed anyways. I prefer path.join to path.resolve if it meets our needs as it is a pure function. path.resolve falls back to process.cwd().

FWIW, PureScript offers a straightforward String::replaceAll function (which even minimizes chances of accidentally flipping arguments using Pattern and Replacement types): https://pursuit.purescript.org/packages/purescript-strings/3.5.0/docs/Data.String#v:replaceAll

The implementation shows that it’s non-trivial: https://github.com/purescript/purescript-strings/blob/06e048c1a6026e3fd6dd9aff0a2d26da23cec783/src/Data/String.js#L114-L120

😜

gasi · 2018-03-07T16:18:22Z

Thanks for pointing some edge cases I didn’t handle. Submitted changes to address these. I’d appreciate another look — thanks! https://github.com/signalapp/Signal-Desktop/pull/2110/files/75208f89d075de69dbfa3f08cf2ccd81e33b926d..2d02d620f0b086fce99d117e1eab24516b1a407a

gasi · 2018-03-08T20:08:55Z

@scottnonnenberg: Ready for final review with ensure-error removed: https://github.com/signalapp/Signal-Desktop/pull/2110/files/2d02d620f0b086fce99d117e1eab24516b1a407a..d97fcecea2995e46c61aa8e96626c01d85f72961

gasi · 2018-03-08T21:02:51Z

Rebased and pushed. Changes: Rebasing to fix .only leftover (thanks!) and adding mocha ESLint plugin 😄

See #2019.

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

Centralizes how we redact sensitive information.

Example: ``` > node > const nonError = {foo: 'i-am-private'}; undefined // before > util.inspect(nonError); '{ foo: \'i-am-private\' }' // after > nonError.toString() '[object Object]' > ```

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); ```

Allows us to set various lint rules for our tests.

gasi · 2018-03-08T22:34:15Z

Rebased against after merging #2111 and #2112: no changes.

It potentially could leak information from serialized non-errors that are thrown due to `util.inspect`.

gasi · 2018-03-08T22:47:30Z

Added one more commit to update our electron-unhandled fork and not even use ensure-error and clean-error. Will merge now.

…ions 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.

New option in settings: delete all application data (383e02e) Remove all configuration in database when we discover we are unlinked (9acb189) Delete everything in database when we link with a different phone number from previous link (9acb189) Windows: Delete all data on uninstall (c855597) Dev: - Switch to a new service for debug logs, since GitHub is retiring anonymous gists (#2111) - Redact file paths in anything that goes to the log on disk (#2110) - When top-level process errors happen, don't show dialog with stack trace (#2110) - Add `nsp` to CI runs (fd056e1) - Add eslint-plugin-mocha to disallow exclusive tests using *.only (#2110)

New option in settings: delete all application data (signalapp@383e02e) Remove all configuration in database when we discover we are unlinked (signalapp@9acb189) Delete everything in database when we link with a different phone number from previous link (signalapp@9acb189) Windows: Delete all data on uninstall (signalapp@c855597) Dev: - Switch to a new service for debug logs, since GitHub is retiring anonymous gists (signalapp#2111) - Redact file paths in anything that goes to the log on disk (signalapp#2110) - When top-level process errors happen, don't show dialog with stack trace (signalapp#2110) - Add `nsp` to CI runs (signalapp@fd056e1) - Add eslint-plugin-mocha to disallow exclusive tests using *.only (signalapp#2110)

@colefranz

Note: this release is equivalent to v1.7.0-beta.3. The changes listed below are compared to the previous production release, v1.6.1. Update to electron 1.8.4 (#2186) Migrate all attachments from IndexedDB to file system in the background (#2208, #2193, #2165, #2162, #2129) Save attachments to disk when importing Chrome app export (#2212) New option in settings: delete all application data (383e02e, #2144 and #2153) Remove all configuration in database when we discover we are unlinked (9acb189 and 1c6d91b) Delete everything in database when we link with a different phone number from previous link (9acb189) Windows: Delete all data on uninstall (c855597) Fixed: Read receipts setting would not be synchronized along with re-link (#2218) Fixed: Clicking conversation in left pane when already selected would remove focus on message composition field - thanks @colefranz! (#2032) Fixed: Searching for the phone number of an existing contact, then selecting 'start conversation' would erase contact details (#2191) Fixed: Selecting Settings menu option multiple times would open multiple instances of settings view - thanks @navdeepsinghkhalsa (#2167) Dev: - Redact file paths in anything that goes to the log on disk (#2110) - When top-level process errors happen, don't show dialog with stack trace (#2110) - Add `nsp` to CI runs (fd056e1) - Add eslint-plugin-mocha to disallow exclusive tests using *.only (#2110) - Preparation for encrypted backups (cea42bd) - Updates to structure of exported data - messages.zip, flat list of attachments (6d8f4b7) - Relax Node.js version requirements (#2203) - Fix a few typos in documentation - thanks @Vinnl (#2171) - Update issue template to mention that translation should be via Transifex (#2157)

gasi requested a review from scottnonnenberg March 5, 2018 20:07

scottnonnenberg approved these changes Mar 5, 2018

View reviewed changes

scottnonnenberg reviewed Mar 6, 2018

View reviewed changes

gasi force-pushed the log-uncaught-errors-promise-rejections branch from 6b138fc to 81b927c Compare March 8, 2018 20:59

gasi added 19 commits March 8, 2018 16:22

Add electron-unhandled dependency

9e5fec1

Log uncaught errors / promise rejections in main process

18785be

See #2019.

Reduce scope of disabled more/no-then rule

522b88d

Add ensure-error dependency

77e7e9a

Extract Errors.addGlobalHandler procedure

2575196

Rename Errors to GlobalErrors for clarity

3dffdc3

Expose Signal.Types.Errors

289063b

Use Errors.toLogFormat in backgrounds.js

33bbb12

Extract Errors.redactSensitivePaths

0e2f8a8

Redact all private information from logs

0c317c5

Extract Privacy module

49e0850

Centralizes how we redact sensitive information.

Add tests for Privacy module

e71246a

Remove privacy redaction from Errors.toLogFormat

44b81f6

Escape special characters in file path

ea07915

Avoid ensure-error for privacy reasons

43b47fd

Example: ``` > node > const nonError = {foo: 'i-am-private'}; undefined // before > util.inspect(nonError); '{ foo: \'i-am-private\' }' // after > nonError.toString() '[object Object]' > ```

Add eslint-plugin-mocha

9644296

Allows us to set various lint rules for our tests.

Disallow exclusive tests, i.e. .only

c7305db

gasi force-pushed the log-uncaught-errors-promise-rejections branch from 81b927c to c7305db Compare March 8, 2018 22:33

Remove ensure-error and clean-error dependency

c317f34

It potentially could leak information from serialized non-errors that are thrown due to `util.inspect`.

Remove unused require

8a1bff0

gasi merged commit 8a1bff0 into development Mar 8, 2018

gasi deleted the log-uncaught-errors-promise-rejections branch March 8, 2018 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log Uncaught Errors & Unhandled Promise Rejections In Main Process #2110

Log Uncaught Errors & Unhandled Promise Rejections In Main Process #2110

gasi commented Mar 5, 2018 •

edited

scottnonnenberg left a comment

scottnonnenberg Mar 5, 2018

gasi Mar 6, 2018

scottnonnenberg Mar 5, 2018

gasi Mar 6, 2018

scottnonnenberg Mar 5, 2018

gasi Mar 6, 2018

gasi commented Mar 6, 2018

scottnonnenberg left a comment

scottnonnenberg Mar 6, 2018

gasi Mar 7, 2018

gasi Mar 8, 2018

scottnonnenberg Mar 6, 2018

gasi Mar 7, 2018

scottnonnenberg Mar 6, 2018

gasi Mar 7, 2018 •

edited

gasi Mar 7, 2018 •

edited

gasi commented Mar 7, 2018

gasi commented Mar 8, 2018

gasi commented Mar 8, 2018

gasi commented Mar 8, 2018 •

edited

gasi commented Mar 8, 2018

		@@ -1,3 +1,4 @@
		const addUnhandledErrorHandler = require('electron-unhandled');

Log Uncaught Errors & Unhandled Promise Rejections In Main Process #2110

Log Uncaught Errors & Unhandled Promise Rejections In Main Process #2110

Conversation

gasi commented Mar 5, 2018 • edited

scottnonnenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi commented Mar 6, 2018

scottnonnenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi Mar 7, 2018 • edited

Choose a reason for hiding this comment

gasi Mar 7, 2018 • edited

Choose a reason for hiding this comment

gasi commented Mar 7, 2018

gasi commented Mar 8, 2018

gasi commented Mar 8, 2018

gasi commented Mar 8, 2018 • edited

gasi commented Mar 8, 2018

gasi commented Mar 5, 2018 •

edited

gasi Mar 7, 2018 •

edited

gasi Mar 7, 2018 •

edited

gasi commented Mar 8, 2018 •

edited