From db7fc502167b9d706dd12397a1ec42297648bb70 Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 10 Nov 2021 19:27:04 -0800 Subject: [PATCH 1/6] metametrics - ensure segment submission failures do not bubble up (#12573) * metametrics - ensure segment submission failures do not bubble up * metametrics - differentiate between trackEvent and submitEvent * metametrics - validate event in trackEvent * metametrics - re-throw error on clean stack * lint fix * controllers/metametrics - take a captureException option * controllers/metametrics - capture and report any errors in trackPage * Update app/scripts/controllers/metametrics.js Co-authored-by: Mark Stacey Co-authored-by: Mark Stacey --- app/scripts/controllers/metametrics.js | 110 +++++++++++++------- app/scripts/controllers/metametrics.test.js | 22 ++-- app/scripts/metamask-controller.js | 2 + 3 files changed, 87 insertions(+), 47 deletions(-) diff --git a/app/scripts/controllers/metametrics.js b/app/scripts/controllers/metametrics.js index 1539cbcba..28e84d7b0 100644 --- a/app/scripts/controllers/metametrics.js +++ b/app/scripts/controllers/metametrics.js @@ -7,6 +7,14 @@ import { METAMETRICS_BACKGROUND_PAGE_OBJECT, } from '../../../shared/constants/metametrics'; +const defaultCaptureException = (err) => { + // throw error on clean stack so its captured by platform integrations (eg sentry) + // but does not interupt the call stack + setTimeout(() => { + throw err; + }); +}; + /** * @typedef {import('../../../shared/constants/metametrics').MetaMetricsContext} MetaMetricsContext * @typedef {import('../../../shared/constants/metametrics').MetaMetricsEventPayload} MetaMetricsEventPayload @@ -51,7 +59,9 @@ export default class MetaMetricsController { version, environment, initState, + captureException = defaultCaptureException, }) { + this._captureException = captureException; const prefState = preferencesStore.getState(); this.chainId = getCurrentChainId(); this.network = getNetworkIdentifier(); @@ -258,32 +268,52 @@ export default class MetaMetricsController { * view */ trackPage({ name, params, environmentType, page, referrer }, options) { - if (this.state.participateInMetaMetrics === false) { - return; - } + try { + if (this.state.participateInMetaMetrics === false) { + return; + } - if (this.state.participateInMetaMetrics === null && !options?.isOptInPath) { - return; + if ( + this.state.participateInMetaMetrics === null && + !options?.isOptInPath + ) { + return; + } + const { metaMetricsId } = this.state; + const idTrait = metaMetricsId ? 'userId' : 'anonymousId'; + const idValue = metaMetricsId ?? METAMETRICS_ANONYMOUS_ID; + this.segment.page({ + [idTrait]: idValue, + name, + properties: { + params, + locale: this.locale, + network: this.network, + chain_id: this.chainId, + environment_type: environmentType, + }, + context: this._buildContext(referrer, page), + }); + } catch (err) { + this._captureException(err); } - const { metaMetricsId } = this.state; - const idTrait = metaMetricsId ? 'userId' : 'anonymousId'; - const idValue = metaMetricsId ?? METAMETRICS_ANONYMOUS_ID; - this.segment.page({ - [idTrait]: idValue, - name, - properties: { - params, - locale: this.locale, - network: this.network, - chain_id: this.chainId, - environment_type: environmentType, - }, - context: this._buildContext(referrer, page), - }); } /** - * track a metametrics event, performing necessary payload manipulation and + * submits a metametrics event, not waiting for it to complete or allowing its error to bubble up + * @param {MetaMetricsEventPayload} payload - details of the event + * @param {MetaMetricsEventOptions} [options] - options for handling/routing the event + */ + trackEvent(payload, options) { + // validation is not caught and handled + this.validatePayload(payload); + this.submitEvent(payload, options).catch((err) => + this._captureException(err), + ); + } + + /** + * submits (or queues for submission) a metametrics event, performing necessary payload manipulation and * routing the event to the appropriate segment source. Will split events * with sensitiveProperties into two events, tracking the sensitiveProperties * with the anonymousId only. @@ -291,21 +321,8 @@ export default class MetaMetricsController { * @param {MetaMetricsEventOptions} [options] - options for handling/routing the event * @returns {Promise} */ - async trackEvent(payload, options) { - // event and category are required fields for all payloads - if (!payload.event || !payload.category) { - throw new Error( - `Must specify event and category. Event was: ${ - payload.event - }. Category was: ${payload.category}. Payload keys were: ${Object.keys( - payload, - )}. ${ - typeof payload.properties === 'object' - ? `Payload property keys were: ${Object.keys(payload.properties)}` - : '' - }`, - ); - } + async submitEvent(payload, options) { + this.validatePayload(payload); if (!this.state.participateInMetaMetrics && !options?.isOptIn) { return; @@ -345,4 +362,25 @@ export default class MetaMetricsController { await Promise.all(events); } + + /** + * validates a metametrics event + * @param {MetaMetricsEventPayload} payload - details of the event + */ + validatePayload(payload) { + // event and category are required fields for all payloads + if (!payload.event || !payload.category) { + throw new Error( + `Must specify event and category. Event was: ${ + payload.event + }. Category was: ${payload.category}. Payload keys were: ${Object.keys( + payload, + )}. ${ + typeof payload.properties === 'object' + ? `Payload property keys were: ${Object.keys(payload.properties)}` + : '' + }`, + ); + } + } } diff --git a/app/scripts/controllers/metametrics.test.js b/app/scripts/controllers/metametrics.test.js index bb0d15bed..9918d6319 100644 --- a/app/scripts/controllers/metametrics.test.js +++ b/app/scripts/controllers/metametrics.test.js @@ -196,14 +196,14 @@ describe('MetaMetricsController', function () { }); }); - describe('trackEvent', function () { + describe('submitEvent', function () { it('should not track an event if user is not participating in metametrics', function () { const mock = sinon.mock(segment); const metaMetricsController = getMetaMetricsController({ participateInMetaMetrics: false, }); mock.expects('track').never(); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', properties: { @@ -230,7 +230,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -260,7 +260,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -289,7 +289,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -317,7 +317,7 @@ describe('MetaMetricsController', function () { ...DEFAULT_EVENT_PROPERTIES, }, }); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', properties: { @@ -331,7 +331,7 @@ describe('MetaMetricsController', function () { const metaMetricsController = getMetaMetricsController(); const flushStub = sinon.stub(segment, 'flush'); const flushCalled = waitUntilCalled(flushStub, segment); - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -344,13 +344,13 @@ describe('MetaMetricsController', function () { it('should throw if event or category not provided', function () { const metaMetricsController = getMetaMetricsController(); assert.rejects( - () => metaMetricsController.trackEvent({ event: 'test' }), + () => metaMetricsController.submitEvent({ event: 'test' }), /Must specify event and category\./u, 'must specify category', ); assert.rejects( - () => metaMetricsController.trackEvent({ category: 'test' }), + () => metaMetricsController.submitEvent({ category: 'test' }), /Must specify event and category\./u, 'must specify event', ); @@ -360,7 +360,7 @@ describe('MetaMetricsController', function () { const metaMetricsController = getMetaMetricsController(); assert.rejects( () => - metaMetricsController.trackEvent( + metaMetricsController.submitEvent( { event: 'Fake Event', category: 'Unit Test', @@ -375,7 +375,7 @@ describe('MetaMetricsController', function () { it('should track sensitiveProperties in a separate, anonymous event', function () { const metaMetricsController = getMetaMetricsController(); const spy = sinon.spy(segment, 'track'); - metaMetricsController.trackEvent({ + metaMetricsController.submitEvent({ event: 'Fake Event', category: 'Unit Test', sensitiveProperties: { foo: 'bar' }, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index bdf26f80f..16b41ec54 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -17,6 +17,7 @@ import LedgerBridgeKeyring from '@metamask/eth-ledger-bridge-keyring'; import LatticeKeyring from 'eth-lattice-keyring'; import EthQuery from 'eth-query'; import nanoid from 'nanoid'; +import { captureException } from '@sentry/browser'; import { AddressBookController, ApprovalController, @@ -190,6 +191,7 @@ export default class MetamaskController extends EventEmitter { version: this.platform.getVersion(), environment: process.env.METAMASK_ENVIRONMENT, initState: initState.MetaMetricsController, + captureException, }); const gasFeeMessenger = this.controllerMessenger.getRestricted({ From 042c19e0cb5e50fabdf54735c5e311d138f37032 Mon Sep 17 00:00:00 2001 From: ryanml Date: Thu, 18 Nov 2021 14:14:03 -0700 Subject: [PATCH 2/6] Don't dispatch hideTestNetMessage (#12748) --- ui/components/app/dropdowns/network-dropdown.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/components/app/dropdowns/network-dropdown.js b/ui/components/app/dropdowns/network-dropdown.js index b446036e6..2e2528e47 100644 --- a/ui/components/app/dropdowns/network-dropdown.js +++ b/ui/components/app/dropdowns/network-dropdown.js @@ -68,7 +68,7 @@ function mapDispatchToProps(dispatch) { }), ); }, - hideTestNetMessage: () => dispatch(actions.hideTestNetMessage()), + hideTestNetMessage: () => actions.hideTestNetMessage(), }; } From baef54bd3110f892ad4863b7805d4ee14eb79738 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 19 Nov 2021 11:53:19 -0330 Subject: [PATCH 3/6] Update `improved-yarn-audit` and ignore 2 advisories (#12765) `improved-yarn-audit` has been updated so that it supports GitHub advisories. Two new GitHub advisories have been ignored, as they are both moderate RegExp DoS vulnerabilities that don't affect us, and they are embedded deep within our dependency graph and are difficult to update. --- .circleci/scripts/yarn-audit.sh | 2 +- package.json | 2 +- yarn.lock | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.circleci/scripts/yarn-audit.sh b/.circleci/scripts/yarn-audit.sh index 717b5f456..48bed3ce6 100755 --- a/.circleci/scripts/yarn-audit.sh +++ b/.circleci/scripts/yarn-audit.sh @@ -5,7 +5,7 @@ set -o pipefail # use `improved-yarn-audit` since that allows for exclude # exclude 1002401 until we remove use of 3Box, 1002581 until we can find a better solution -yarn run improved-yarn-audit --ignore-dev-deps --min-severity moderate --exclude 1002401,1002581 +yarn run improved-yarn-audit --ignore-dev-deps --min-severity moderate --exclude 1002401,1002581,GHSA-93q8-gq69-wqmw,GHSA-257v-vj4p-3w2h audit_status="$?" # Use a bitmask to ignore INFO and LOW severity audit results diff --git a/package.json b/package.json index 2e378ad55..0352508d4 100644 --- a/package.json +++ b/package.json @@ -292,7 +292,7 @@ "gulp-watch": "^5.0.1", "gulp-zip": "^4.0.0", "history": "^5.0.0", - "improved-yarn-audit": "^2.3.3", + "improved-yarn-audit": "^3.0.0", "jest": "^26.6.3", "jsdom": "^11.2.0", "koa": "^2.7.0", diff --git a/yarn.lock b/yarn.lock index db77530da..90a3eddd7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6510,9 +6510,9 @@ ansi-regex@^4.1.0: integrity sha512-1apePfXM1UOSqw0o9IiFAovVz9M5S1Dg+4TrDwfMewQ6p/rmMueb7tWZjQ1rx4Loy1ArBggoqGpfqqdI4rondg== ansi-regex@^5.0.0: - version "5.0.0" - resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-5.0.0.tgz#388539f55179bf39339c81af30a654d69f87cb75" - integrity sha512-bY6fj56OUQ0hU1KjFNDQuJFezqKdrAyFdIevADiqrWHwSlbmBNMHp5ak2f40Pm8JTFyM2mqxkG6ngkHO11f/lg== + version "5.0.1" + resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-5.0.1.tgz#082cb2c89c9fe8659a311a53bd6a4dc5301db304" + integrity sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ== ansi-styles@^2.2.1: version "2.2.1" @@ -16243,10 +16243,10 @@ import-local@^3.0.2: pkg-dir "^4.2.0" resolve-cwd "^3.0.0" -improved-yarn-audit@^2.3.3: - version "2.3.3" - resolved "https://registry.yarnpkg.com/improved-yarn-audit/-/improved-yarn-audit-2.3.3.tgz#da0be78be4b678c73733066c9ccd21e1958fae8c" - integrity sha512-chZ7zPKGsA+CZeMExNPf9WZhETJLkC+u8cQlkQC9XyPZqQPctn3FavefTjXBXmX3Azin8WcoAbaok1FvjkLf6A== +improved-yarn-audit@^3.0.0: + version "3.0.0" + resolved "https://registry.yarnpkg.com/improved-yarn-audit/-/improved-yarn-audit-3.0.0.tgz#dfb09cea1a3a92c790ea2b4056431f6fb1b99bfa" + integrity sha512-b7CrBYYwMidtPciCBkW62C7vqGjAV10bxcAWHeJvGrltrcMSEnG5I9CQgi14nmAlUKUQiSvpz47Lo3d7Z3Vjcg== imurmurhash@^0.1.4: version "0.1.4" From a533dcc403212d429b4ea5ac859361358f8fd55b Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 19 Nov 2021 14:07:50 -0330 Subject: [PATCH 4/6] Ensure that metametrics error related to anonymousId is not sent to sentry (#12763) --- app/scripts/controllers/metametrics.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/scripts/controllers/metametrics.js b/app/scripts/controllers/metametrics.js index 28e84d7b0..9e6ecfff7 100644 --- a/app/scripts/controllers/metametrics.js +++ b/app/scripts/controllers/metametrics.js @@ -15,6 +15,10 @@ const defaultCaptureException = (err) => { }); }; +const exceptionsToFilter = { + [`You must pass either an "anonymousId" or a "userId".`]: true, +}; + /** * @typedef {import('../../../shared/constants/metametrics').MetaMetricsContext} MetaMetricsContext * @typedef {import('../../../shared/constants/metametrics').MetaMetricsEventPayload} MetaMetricsEventPayload @@ -61,7 +65,13 @@ export default class MetaMetricsController { initState, captureException = defaultCaptureException, }) { - this._captureException = captureException; + this._captureException = (err) => { + // This is a temporary measure. Currently there are errors flooding sentry due to a problem in how we are tracking anonymousId + // We intend on removing this as soon as we understand how to correctly solve that problem. + if (!exceptionsToFilter[err.message]) { + captureException(err); + } + }; const prefState = preferencesStore.getState(); this.chainId = getCurrentChainId(); this.network = getNetworkIdentifier(); From 32c3d8b6fdc26d8f90a83cc49447dad325b7de80 Mon Sep 17 00:00:00 2001 From: MetaMask Bot Date: Fri, 19 Nov 2021 17:58:37 +0000 Subject: [PATCH 5/6] Version v10.6.1 --- CHANGELOG.md | 10 +++++++++- package.json | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 288626467..a0d787ee1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [10.6.1] +### Uncategorized +- Ensure that metametrics error related to anonymousId is not sent to sentry ([#12763](https://github.com/MetaMask/metamask-extension/pull/12763)) +- Update `improved-yarn-audit` and ignore 2 advisories ([#12765](https://github.com/MetaMask/metamask-extension/pull/12765)) +- Don't dispatch hideTestNetMessage ([#12748](https://github.com/MetaMask/metamask-extension/pull/12748)) +- metametrics - ensure segment submission failures do not bubble up ([#12573](https://github.com/MetaMask/metamask-extension/pull/12573)) + ## [10.6.0] ### Added - [#12053](https://github.com/MetaMask/metamask-extension/pull/12053): Add support for GridPlus Lattice1 hardware wallet @@ -2594,7 +2601,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Uncategorized - Added the ability to restore accounts from seed words. -[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v10.6.0...HEAD +[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v10.6.1...HEAD +[10.6.1]: https://github.com/MetaMask/metamask-extension/compare/v10.6.0...v10.6.1 [10.6.0]: https://github.com/MetaMask/metamask-extension/compare/v10.5.2...v10.6.0 [10.5.2]: https://github.com/MetaMask/metamask-extension/compare/v10.5.1...v10.5.2 [10.5.1]: https://github.com/MetaMask/metamask-extension/compare/v10.5.0...v10.5.1 diff --git a/package.json b/package.json index 0352508d4..392b1969f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "metamask-crx", - "version": "10.6.0", + "version": "10.6.1", "private": true, "repository": { "type": "git", From eb7d96e9889f2697c95282a3b5e963bd05444d58 Mon Sep 17 00:00:00 2001 From: ryanml Date: Fri, 19 Nov 2021 11:05:50 -0700 Subject: [PATCH 6/6] Updating changelog --- CHANGELOG.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0d787ee1..743920163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ## [10.6.1] -### Uncategorized -- Ensure that metametrics error related to anonymousId is not sent to sentry ([#12763](https://github.com/MetaMask/metamask-extension/pull/12763)) -- Update `improved-yarn-audit` and ignore 2 advisories ([#12765](https://github.com/MetaMask/metamask-extension/pull/12765)) -- Don't dispatch hideTestNetMessage ([#12748](https://github.com/MetaMask/metamask-extension/pull/12748)) -- metametrics - ensure segment submission failures do not bubble up ([#12573](https://github.com/MetaMask/metamask-extension/pull/12573)) +### Fixed +- [#12573](https://github.com/MetaMask/metamask-extension/pull/12573): Ensure metrics api errors do not impact user experience ## [10.6.0] ### Added