MM-11477 Use custom thunk middleware to intercept leaked network errors (#2222)

* MM-11477 Use custom thunk middleware to intercept leaked network errors

* MM-11477 Always include url in Client4 errors

* Update redux

app/fetch_preconfig.js

@@ -61,6 +61,7 @@ Client4.doFetchWithResponse = async (url, options) => {
                 id: t('mobile.request.invalid_response'),
                 defaultMessage: 'Received invalid response from the server.',
             },
+            url,
         };
     }
 

app/store/index.js

@@ -21,6 +21,7 @@ import mattermostBucket from 'app/mattermost_bucket';
 import Config from 'assets/config';
 
 import {messageRetention} from './middleware';
+import {createThunkMiddleware} from './thunk';
 import {transformSet} from './utils';
 
 function getAppReducer() {
@@ -276,8 +277,14 @@ export default function configureAppStore(initialState) {
         },
     };
 
-    const additionalMiddleware = [createSentryMiddleware(), messageRetention];
-    return configureStore(initialState, appReducer, offlineOptions, getAppReducer, {
-        additionalMiddleware,
-    });
+    const clientOptions = {
+        additionalMiddleware: [
+            createThunkMiddleware(),
+            createSentryMiddleware(),
+            messageRetention,
+        ],
+        enableThunk: false, // We override the default thunk middleware
+    };
+
+    return configureStore(initialState, appReducer, offlineOptions, getAppReducer, clientOptions);
 }

app/store/thunk.js

@@ -0,0 +1,41 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {
+    captureMessage,
+    cleanUrlForLogging,
+    LOGGER_JAVASCRIPT_WARNING,
+} from 'app/utils/sentry';
+
+// Creates middleware that mimics thunk while catching network errors thrown by Client4 that haven't
+// been otherwise handled.
+export function createThunkMiddleware() {
+    return (store) => (next) => (action) => {
+        if (typeof action === 'function') {
+            const result = action(store.dispatch, store.getState);
+
+            if (result instanceof Promise) {
+                return result.catch((error) => {
+                    if (error.url) {
+                        // This is a connection error from mattermost-redux. This should've been handled
+                        // within the action itself, so we'll log to Sentry enough to identify where
+                        // that handling is missing.
+                        captureMessage(
+                            `Caught Client4 error "${error.message}" from "${cleanUrlForLogging(error.url)}"`,
+                            LOGGER_JAVASCRIPT_WARNING,
+                            store
+                        );
+
+                        return {error};
+                    }
+
+                    throw error;
+                });
+            }
+
+            return result;
+        }
+
+        return next(action);
+    };
+}

app/utils/sentry/index.js

@@ -6,6 +6,7 @@ import {Sentry} from 'react-native-sentry';
 
 import Config from 'assets/config';
 
+import {Client4} from 'mattermost-redux/client';
 import {getConfig} from 'mattermost-redux/selectors/entities/general';
 import {getCurrentUser} from 'mattermost-redux/selectors/entities/users';
 import {getCurrentTeam, getCurrentTeamMembership} from 'mattermost-redux/selectors/entities/teams';
@@ -247,3 +248,53 @@ function getBuildTags(state) {
 
     return tags;
 }
+
+// Given a URL from an API request, return a URL that has any parts removed that are either sensitive or that would
+// prevent properly grouping the messages in Sentry.
+export function cleanUrlForLogging(original) {
+    let url = original;
+
+    // Trim the host name
+    url = url.substring(Client4.getUrl().length);
+
+    // Filter the query string
+    const index = url.indexOf('?');
+    if (index !== -1) {
+        url = url.substring(0, index);
+    }
+
+    // A non-exhaustive whitelist to exclude parts of the URL that are unimportant (eg IDs) or may be sentsitive
+    // (eg email addresses). We prefer filtering out fields that aren't recognized because there should generally
+    // be enough left over for debugging.
+    //
+    // Note that new API routes don't need to be added here since this shouldn't be happening for newly added routes.
+    const whitelist = [
+        'api', 'v4', 'users', 'teams', 'scheme', 'name', 'members', 'channels', 'posts', 'reactions', 'commands',
+        'files', 'preferences', 'hooks', 'incoming', 'outgoing', 'oauth', 'apps', 'emoji', 'brand', 'image',
+        'data_retention', 'jobs', 'plugins', 'roles', 'system', 'timezones', 'schemes', 'redirect_location', 'patch',
+        'mfa', 'password', 'reset', 'send', 'active', 'verify', 'terms_of_service', 'login', 'logout', 'ids',
+        'usernames', 'me', 'username', 'email', 'default', 'sessions', 'revoke', 'all', 'audits', 'device', 'status',
+        'search', 'switch', 'authorized', 'authorize', 'deauthorize', 'tokens', 'disable', 'enable', 'exists', 'unread',
+        'invite', 'batch', 'stats', 'import', 'schemeRoles', 'direct', 'group', 'convert', 'view', 'search_autocomplete',
+        'thread', 'info', 'flagged', 'pinned', 'pin', 'unpin', 'opengraph', 'actions', 'thumbnail', 'preview', 'link',
+        'delete', 'logs', 'ping', 'config', 'client', 'license', 'websocket', 'webrtc', 'token', 'regen_token',
+        'autocomplete', 'execute', 'regen_secret', 'policy', 'type', 'cancel', 'reload', 'environment', 's3_test', 'file',
+        'caches', 'invalidate', 'database', 'recycle', 'compliance', 'reports', 'cluster', 'ldap', 'test', 'sync', 'saml',
+        'certificate', 'public', 'private', 'idp', 'elasticsearch', 'purge_indexes', 'analytics', 'old', 'webapp', 'fake',
+    ];
+
+    url = url.split('/').map((part) => {
+        if (part !== '' && whitelist.indexOf(part) === -1) {
+            return '<filtered>';
+        }
+
+        return part;
+    }).join('/');
+
+    if (index !== -1) {
+        // Add this on afterwards since it wouldn't pass the whitelist
+        url += '?<filtered>';
+    }
+
+    return url;
+}

app/utils/sentry/index.test.js

@@ -0,0 +1,38 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {Client4} from 'mattermost-redux/client';
+
+import {cleanUrlForLogging} from 'app/utils/sentry';
+
+/* eslint-disable max-nested-callbacks */
+
+describe('utils/sentry', () => {
+    describe('cleanUrlForLogging', () => {
+        Client4.setUrl('https://mattermost.example.com/subpath');
+
+        const tests = [{
+            name: 'should remove server URL',
+            input: Client4.getUserRoute('me'),
+            expected: `${Client4.urlVersion}/users/me`,
+        }, {
+            name: 'should filter user IDs',
+            input: Client4.getUserRoute('1234'),
+            expected: `${Client4.urlVersion}/users/<filtered>`,
+        }, {
+            name: 'should filter email addresses',
+            input: `${Client4.getUsersRoute()}/email/test@example.com`,
+            expected: `${Client4.urlVersion}/users/email/<filtered>`,
+        }, {
+            name: 'should filter query parameters',
+            input: `${Client4.getUserRoute('me')}?foo=bar`,
+            expected: `${Client4.urlVersion}/users/me?<filtered>`,
+        }];
+
+        for (const test of tests) {
+            it(test.name, () => {
+                expect(cleanUrlForLogging(test.input)).toEqual(test.expected);
+            });
+        }
+    });
+});

package-lock.json

@@ -2744,7 +2744,7 @@
     },
     "ansi-colors": {
       "version": "1.1.0",
-      "resolved": "http://registry.npmjs.org/ansi-colors/-/ansi-colors-1.1.0.tgz",
+      "resolved": "https://registry.npmjs.org/ansi-colors/-/ansi-colors-1.1.0.tgz",
       "integrity": "sha512-SFKX67auSNoVR38N3L+nvsPjOE0bybKTYbkf5tRvushrAPQ9V75huw0ZxBkKVeRU9kqH3d6HA4xTckbwZ4ixmA==",
       "requires": {
         "ansi-wrap": "^0.1.0"
@@ -9797,8 +9797,8 @@
       "integrity": "sha1-izqsWIuKZuSXXjzepn97sylgH6w="
     },
     "mattermost-redux": {
-      "version": "github:mattermost/mattermost-redux#fe7340ccd0e1909ab2cc98ab44a679d90672bdb9",
-      "from": "github:mattermost/mattermost-redux#fe7340ccd0e1909ab2cc98ab44a679d90672bdb9",
+      "version": "github:mattermost/mattermost-redux#aad431c2491e4dbe3a387c1536ca09f75ae78a27",
+      "from": "github:mattermost/mattermost-redux#aad431c2491e4dbe3a387c1536ca09f75ae78a27",
       "requires": {
         "deep-equal": "1.0.1",
         "eslint-plugin-header": "1.2.0",
@@ -11973,7 +11973,7 @@
     },
     "opn": {
       "version": "3.0.3",
-      "resolved": "http://registry.npmjs.org/opn/-/opn-3.0.3.tgz",
+      "resolved": "https://registry.npmjs.org/opn/-/opn-3.0.3.tgz",
       "integrity": "sha1-ttmec5n3jWXDuq/+8fsojpuFJDo=",
       "requires": {
         "object-assign": "^4.0.1"

package.json

@@ -17,7 +17,7 @@
     "intl": "1.2.5",
     "jail-monkey": "1.0.0",
     "jsc-android": "224109.1.0",
-    "mattermost-redux": "github:mattermost/mattermost-redux#fe7340ccd0e1909ab2cc98ab44a679d90672bdb9",
+    "mattermost-redux": "github:mattermost/mattermost-redux#aad431c2491e4dbe3a387c1536ca09f75ae78a27",
     "mime-db": "1.36.0",
     "moment-timezone": "0.5.21",
     "prop-types": "15.6.2",