Sanitize privacy sensitive data before sending to sentry. (#16780)

* Sanitize privacy sensitive data before sending to sentry.

Temp

Near complete

Complete

* Temp

* Fix url error message rewrite

* Add unit tests and cleanup code

* Improvements

* Update app/scripts/lib/setupSentry.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Update app/scripts/lib/setupSentry.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Update app/scripts/lib/setupSentry.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Fix syntax of doc comments

* Catch errors caused by invalid urls in sanitizeUrlsFromErrorMessages

* Ensure our allowlist matches multiple subdomains

* Ensure sanitizeUrlsFromErrorMessages correctly matches hostnames

* Update app/scripts/lib/setupSentry.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Improve test descriptions

* fix

Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>

app/scripts/lib/setupSentry.js

@@ -16,6 +16,14 @@ const METAMASK_BUILD_TYPE = process.env.METAMASK_BUILD_TYPE;
 const IN_TEST = process.env.IN_TEST;
 /* eslint-enable prefer-destructuring */
 
+export const ERROR_URL_ALLOWLIST = {
+  CRYPTOCOMPARE: 'cryptocompare.com',
+  COINGECKO: 'coingecko.com',
+  ETHERSCAN: 'etherscan.io',
+  CODEFI: 'codefi.network',
+  SEGMENT: 'segment.io',
+};
+
 // This describes the subset of Redux state attached to errors sent to Sentry
 // These properties have some potential to be useful for debugging, and they do
 // not contain any identifiable information.
@@ -131,7 +139,7 @@ export default function setupSentry({ release, getState }) {
       new ExtraErrorData(),
     ],
     release,
-    beforeSend: (report) => rewriteReport(report),
+    beforeSend: (report) => rewriteReport(report, getState),
     beforeBreadcrumb(breadcrumb) {
       if (getState) {
         const appState = getState();
@@ -146,33 +154,134 @@ export default function setupSentry({ release, getState }) {
       } else {
         return null;
       }
-      return breadcrumb;
+      const newBreadcrumb = removeUrlsFromBreadCrumb(breadcrumb);
+      return newBreadcrumb;
     },
   });
 
-  function rewriteReport(report) {
-    try {
-      // simplify certain complex error messages (e.g. Ethjs)
-      simplifyErrorMessages(report);
-      // modify report urls
-      rewriteReportUrls(report);
-      // append app state
-      if (getState) {
-        const appState = getState();
-        if (!report.extra) {
-          report.extra = {};
-        }
-        report.extra.appState = appState;
-      }
-    } catch (err) {
-      console.warn(err);
-    }
-    return report;
-  }
-
   return Sentry;
 }
 
+/**
+ * Receives a string and returns that string if it is a
+ * regex match for a url with a `chrome-extension` or `moz-extension`
+ * protocol, and an empty string otherwise.
+ *
+ * @param {string} url - The URL to check.
+ * @returns {string} An empty string if the URL was internal, or the unmodified URL otherwise.
+ */
+function hideUrlIfNotInternal(url) {
+  const re = /^(chrome-extension|moz-extension):\/\//u;
+  if (!url.match(re)) {
+    return '';
+  }
+  return url;
+}
+
+/**
+ * Receives a Sentry breadcrumb object and potentially removes urls
+ * from its `data` property, it particular those possibly found at
+ * data.from, data.to and data.url
+ *
+ * @param {object} breadcrumb - A Sentry breadcrumb object: https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/
+ * @returns {object} A modified Sentry breadcrumb object.
+ */
+export function removeUrlsFromBreadCrumb(breadcrumb) {
+  if (breadcrumb?.data?.url) {
+    breadcrumb.data.url = hideUrlIfNotInternal(breadcrumb.data.url);
+  }
+  if (breadcrumb?.data?.to) {
+    breadcrumb.data.to = hideUrlIfNotInternal(breadcrumb.data.to);
+  }
+  if (breadcrumb?.data?.from) {
+    breadcrumb.data.from = hideUrlIfNotInternal(breadcrumb.data.from);
+  }
+  return breadcrumb;
+}
+
+/**
+ * Receives a Sentry event object and modifies it before the
+ * error is sent to Sentry. Modifications include both sanitization
+ * of data via helper methods and addition of state data from the
+ * return value of the second parameter passed to the function.
+ *
+ * @param {object} report - A Sentry event object: https://develop.sentry.dev/sdk/event-payloads/
+ * @param {Function} getState - A function that should return an object representing some amount
+ * of app state that we wish to submit with our error reports
+ * @returns {object} A modified Sentry event object.
+ */
+export function rewriteReport(report, getState) {
+  try {
+    // simplify certain complex error messages (e.g. Ethjs)
+    simplifyErrorMessages(report);
+    // remove urls from error message
+    sanitizeUrlsFromErrorMessages(report);
+    // Remove evm addresses from error message.
+    // Note that this is redundent with data scrubbing we do within our sentry dashboard,
+    // but putting the code here as well gives public visibility to how we are handling
+    // privacy with respect to sentry.
+    sanitizeAddressesFromErrorMessages(report);
+    // modify report urls
+    rewriteReportUrls(report);
+    // append app state
+    if (getState) {
+      const appState = getState();
+      if (!report.extra) {
+        report.extra = {};
+      }
+      report.extra.appState = appState;
+    }
+  } catch (err) {
+    console.warn(err);
+  }
+  return report;
+}
+
+/**
+ * Receives a Sentry event object and modifies it so that urls are removed from any of its
+ * error messages.
+ *
+ * @param {object} report - the report to modify
+ */
+function sanitizeUrlsFromErrorMessages(report) {
+  rewriteErrorMessages(report, (errorMessage) => {
+    let newErrorMessage = errorMessage;
+    const re = /(([-.+a-zA-Z]+:\/\/)|(www\.))\S+[@:.]\S+/gu;
+    const urlsInMessage = newErrorMessage.match(re) || [];
+    urlsInMessage.forEach((url) => {
+      try {
+        const urlObj = new URL(url);
+        const { hostname } = urlObj;
+        if (
+          !Object.values(ERROR_URL_ALLOWLIST).some(
+            (allowedHostname) =>
+              hostname === allowedHostname ||
+              hostname.endsWith(`.${allowedHostname}`),
+          )
+        ) {
+          newErrorMessage = newErrorMessage.replace(url, '**');
+        }
+      } catch (e) {
+        newErrorMessage = newErrorMessage.replace(url, '**');
+      }
+    });
+    return newErrorMessage;
+  });
+}
+
+/**
+ * Receives a Sentry event object and modifies it so that ethereum addresses are removed from
+ * any of its error messages.
+ *
+ * @param {object} report - the report to modify
+ */
+function sanitizeAddressesFromErrorMessages(report) {
+  rewriteErrorMessages(report, (errorMessage) => {
+    const newErrorMessage = errorMessage.replace(/0x[A-Fa-f0-9]{40}/u, '0x**');
+    return newErrorMessage;
+  });
+}
+
 function simplifyErrorMessages(report) {
   rewriteErrorMessages(report, (errorMessage) => {
     // simplify ethjs error messages
@@ -221,7 +330,7 @@ function rewriteReportUrls(report) {
 }
 
 function toMetamaskUrl(origUrl) {
-  const filePath = origUrl.split(globalThis.location.origin)[1];
+  const filePath = origUrl?.split(globalThis.location.origin)[1];
   if (!filePath) {
     return origUrl;
   }

app/scripts/lib/setupSentry.test.js

@@ -0,0 +1,241 @@
+import { rewriteReport, removeUrlsFromBreadCrumb } from './setupSentry';
+
+describe('Setup Sentry', () => {
+  describe('rewriteReport', () => {
+    it('should remove urls from error messages', () => {
+      const testReport = {
+        message: 'This report has a test url: http://example.com',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report has a test url: **',
+      );
+    });
+
+    it('should remove urls from error reports that have an exception with an array of values', () => {
+      const testReport = {
+        exception: {
+          values: [
+            {
+              value: 'This report has a test url: http://example.com',
+            },
+            {
+              value: 'https://example.com is another url',
+            },
+          ],
+        },
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.exception.values).toStrictEqual([
+        {
+          value: 'This report has a test url: **',
+        },
+        {
+          value: '** is another url',
+        },
+      ]);
+    });
+
+    it('should remove ethereum addresses from error messages', () => {
+      const testReport = {
+        message:
+          'There is an ethereum address 0x790A8A9E9bc1C9dB991D8721a92e461Db4CfB235 in this message',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'There is an ethereum address 0x** in this message',
+      );
+    });
+
+    it('should not remove urls from our allow list', () => {
+      const testReport = {
+        message: 'This report has an allowed url: https://codefi.network/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report has an allowed url: https://codefi.network/',
+      );
+    });
+
+    it('should not remove urls at subdomains of the urls in the allow list', () => {
+      const testReport = {
+        message:
+          'This report has an allowed url: https://subdomain.codefi.network/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report has an allowed url: https://subdomain.codefi.network/',
+      );
+    });
+
+    it('should remove urls very similar to, but different from, those in our allow list', () => {
+      const testReport = {
+        message:
+          'This report does not have an allowed url: https://nodefi.network/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report does not have an allowed url: **',
+      );
+    });
+
+    it('should remove urls with allow list urls in their domain path', () => {
+      const testReport = {
+        message:
+          'This report does not have an allowed url: https://codefi.network.another.domain.com/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report does not have an allowed url: **',
+      );
+    });
+
+    it('should remove urls have allowed urls in their URL path', () => {
+      const testReport = {
+        message:
+          'This report does not have an allowed url: https://example.com/test?redirect=http://codefi.network',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report does not have an allowed url: **',
+      );
+    });
+
+    it('should remove urls with subdomains', () => {
+      const testReport = {
+        message:
+          'This report does not have an allowed url: https://subdomain.example.com/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report does not have an allowed url: **',
+      );
+    });
+
+    it('should remove invalid urls', () => {
+      const testReport = {
+        message:
+          'This report does not have an allowed url: https://example.%%%/',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This report does not have an allowed url: **',
+      );
+    });
+
+    it('should remove urls and ethereum addresses from error messages', () => {
+      const testReport = {
+        message:
+          'This 0x790A8A9E9bc1C9dB991D8721a92e461Db4CfB235 address used http://example.com on Saturday',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual(
+        'This 0x** address used ** on Saturday',
+      );
+    });
+
+    it('should not modify an error message with no urls or addresses', () => {
+      const testReport = {
+        message: 'This is a simple report',
+        request: {},
+      };
+      const rewrittenReport = rewriteReport(testReport);
+      expect(rewrittenReport.message).toStrictEqual('This is a simple report');
+    });
+  });
+
+  describe('removeUrlsFromBreadCrumb', () => {
+    it('should hide the breadcrumb data url', () => {
+      const testBreadcrumb = {
+        data: {
+          url: 'https://example.com',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.url).toStrictEqual('');
+    });
+
+    it('should hide the breadcrumb data "to" page', () => {
+      const testBreadcrumb = {
+        data: {
+          to: 'https://example.com',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.to).toStrictEqual('');
+    });
+
+    it('should hide the breadcrumb data "from" page', () => {
+      const testBreadcrumb = {
+        data: {
+          from: 'https://example.com',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.from).toStrictEqual('');
+    });
+
+    it('should NOT hide the breadcrumb data url if the url is on the extension protocol', () => {
+      const testBreadcrumb = {
+        data: {
+          url: 'chrome-extension://abcefg/home.html',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.url).toStrictEqual(
+        'chrome-extension://abcefg/home.html',
+      );
+    });
+
+    it('should NOT hide the breadcrumb data "to" page if the url is on the extension protocol', () => {
+      const testBreadcrumb = {
+        data: {
+          to: 'chrome-extension://abcefg/home.html',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.to).toStrictEqual(
+        'chrome-extension://abcefg/home.html',
+      );
+    });
+
+    it('should NOT hide the breadcrumb data "from" page if the url is on the extension protocol', () => {
+      const testBreadcrumb = {
+        data: {
+          from: 'chrome-extension://abcefg/home.html',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data.from).toStrictEqual(
+        'chrome-extension://abcefg/home.html',
+      );
+    });
+
+    it('should hide "to" but not "from" or url if "to" is the only one not matching an internal url', () => {
+      const testBreadcrumb = {
+        data: {
+          url: 'chrome-extension://abcefg/home.html',
+          to: 'https://example.com',
+          from: 'chrome-extension://abcefg/home.html',
+        },
+      };
+      const rewrittenBreadcrumb = removeUrlsFromBreadCrumb(testBreadcrumb);
+      expect(rewrittenBreadcrumb.data).toStrictEqual({
+        url: 'chrome-extension://abcefg/home.html',
+        to: '',
+        from: 'chrome-extension://abcefg/home.html',
+      });
+    });
+  });
+});