From cb98efc6da8cb09fb849671edea9fac53e3fa2ec Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 9 Oct 2018 10:13:28 -0400 Subject: [PATCH] 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 | 1 + app/store/index.js | 15 +++++++--- app/store/thunk.js | 41 +++++++++++++++++++++++++++ app/utils/sentry/index.js | 51 ++++++++++++++++++++++++++++++++++ app/utils/sentry/index.test.js | 38 +++++++++++++++++++++++++ package-lock.json | 8 +++--- package.json | 2 +- 7 files changed, 147 insertions(+), 9 deletions(-) create mode 100644 app/store/thunk.js create mode 100644 app/utils/sentry/index.test.js diff --git a/app/fetch_preconfig.js b/app/fetch_preconfig.js index 6aa6dbe399..276c94d0f2 100644 --- a/app/fetch_preconfig.js +++ b/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, }; } diff --git a/app/store/index.js b/app/store/index.js index ea67921b3e..91ad882902 100644 --- a/app/store/index.js +++ b/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); } diff --git a/app/store/thunk.js b/app/store/thunk.js new file mode 100644 index 0000000000..370a5498c1 --- /dev/null +++ b/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); + }; +} diff --git a/app/utils/sentry/index.js b/app/utils/sentry/index.js index be665e8d89..a321f536a7 100644 --- a/app/utils/sentry/index.js +++ b/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 ''; + } + + return part; + }).join('/'); + + if (index !== -1) { + // Add this on afterwards since it wouldn't pass the whitelist + url += '?'; + } + + return url; +} diff --git a/app/utils/sentry/index.test.js b/app/utils/sentry/index.test.js new file mode 100644 index 0000000000..f0f64ba381 --- /dev/null +++ b/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/`, + }, { + name: 'should filter email addresses', + input: `${Client4.getUsersRoute()}/email/test@example.com`, + expected: `${Client4.urlVersion}/users/email/`, + }, { + name: 'should filter query parameters', + input: `${Client4.getUserRoute('me')}?foo=bar`, + expected: `${Client4.urlVersion}/users/me?`, + }]; + + for (const test of tests) { + it(test.name, () => { + expect(cleanUrlForLogging(test.input)).toEqual(test.expected); + }); + } + }); +}); diff --git a/package-lock.json b/package-lock.json index f0724e1414..263f40fa2c 100644 --- a/package-lock.json +++ b/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" diff --git a/package.json b/package.json index 8f16094909..9d152a2412 100644 --- a/package.json +++ b/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",