mirror of
https://github.com/kremalicious/metamask-extension.git
synced 2024-11-22 18:00:18 +01:00
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>
This commit is contained in:
Dan J Miller
seaona
parent
6439a0e934
commit
557b5de08d
@ -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;
|
||||
}
|
||||
|
||||
241
app/scripts/lib/setupSentry.test.js
Normal file
241
app/scripts/lib/setupSentry.test.js
Normal file
@ -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',
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user