From 5398f3e5ce38732d2c6add27e717ac34fd3567c2 Mon Sep 17 00:00:00 2001 From: tommasini Date: Tue, 12 Nov 2024 13:01:10 +0000 Subject: [PATCH 1/4] Add custom instrumentation to hostnames --- app/store/sagas/index.ts | 4 ++- app/store/sagas/xmlHttpRequestOverride.ts | 42 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/app/store/sagas/index.ts b/app/store/sagas/index.ts index 0985f917d33..3d284b09370 100644 --- a/app/store/sagas/index.ts +++ b/app/store/sagas/index.ts @@ -15,6 +15,7 @@ import Engine from '../../core/Engine'; import Logger from '../../util/Logger'; import LockManagerService from '../../core/LockManagerService'; import { + createLoggingXHROverride, overrideXMLHttpRequest, restoreXMLHttpRequest, } from './xmlHttpRequestOverride'; @@ -125,8 +126,9 @@ export function* basicFunctionalityToggle() { if (basicFunctionalityEnabled) { restoreXMLHttpRequest(); + createLoggingXHROverride(); } else { - // apply global blocklist + restoreXMLHttpRequest(); overrideXMLHttpRequest(); } } diff --git a/app/store/sagas/xmlHttpRequestOverride.ts b/app/store/sagas/xmlHttpRequestOverride.ts index 84939b0773f..bcd5921d84c 100644 --- a/app/store/sagas/xmlHttpRequestOverride.ts +++ b/app/store/sagas/xmlHttpRequestOverride.ts @@ -1,4 +1,5 @@ import AppConstants from '../../../app/core/AppConstants'; +import { TraceName, endTrace, trace } from '../../util/trace'; if (process.env.JEST_WORKER_ID !== undefined) { // monkeypatch for Jest @@ -28,6 +29,47 @@ if (process.env.JEST_WORKER_ID !== undefined) { const originalSend = global.XMLHttpRequest.prototype.send; const originalOpen = global.XMLHttpRequest.prototype.open; +/** + * This creates a trace of each URL request to sentry, sending just the hostname. + * Currently this is only used when basic functionality is enabled. + */ +export function createLoggingXHROverride() { + let currentUrl = ''; + + global.XMLHttpRequest.prototype.open = function ( + method: string, + url: string | URL, + async?: boolean, + user?: string | null, + password?: string | null, + ) { + currentUrl = url?.toString(); + const hostname = new URL(currentUrl).hostname; + trace({ + name: hostname as TraceName, + }); + return originalOpen.apply(this, [ + method, + currentUrl, + async, + user, + password, + ]); + }; + + global.XMLHttpRequest.prototype.send = function (...args: unknown[]) { + const hostname = new URL(currentUrl).hostname; + this.addEventListener('load', () => { + endTrace({ name: hostname as TraceName }); + }); + + this.addEventListener('error', () => { + endTrace({ name: hostname as TraceName }); + }); + + return originalSend.call(this, ...args); + }; +} export function overrideXMLHttpRequest() { // Store the URL of the current request - only valid under assumption of no new requests being initiated between `open` and `send` of one From 9d5473d1cf9997565e867afecd6bc3c3a7213065 Mon Sep 17 00:00:00 2001 From: tommasini Date: Tue, 12 Nov 2024 17:53:30 +0000 Subject: [PATCH 2/4] add operation names and unit tests --- app/store/sagas/index.ts | 3 +- .../sagas/xmlHttpRequestOverride.test.ts | 44 +++++++++++++++++++ app/store/sagas/xmlHttpRequestOverride.ts | 19 +++++++- app/util/trace.ts | 2 + 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/app/store/sagas/index.ts b/app/store/sagas/index.ts index 3d284b09370..51b0be17fb3 100644 --- a/app/store/sagas/index.ts +++ b/app/store/sagas/index.ts @@ -126,9 +126,10 @@ export function* basicFunctionalityToggle() { if (basicFunctionalityEnabled) { restoreXMLHttpRequest(); + // apply metrics if enabled createLoggingXHROverride(); } else { - restoreXMLHttpRequest(); + // apply global blocklist and metrics if enabled overrideXMLHttpRequest(); } } diff --git a/app/store/sagas/xmlHttpRequestOverride.test.ts b/app/store/sagas/xmlHttpRequestOverride.test.ts index fa2b4ed5ccb..97e2cd0f60a 100644 --- a/app/store/sagas/xmlHttpRequestOverride.test.ts +++ b/app/store/sagas/xmlHttpRequestOverride.test.ts @@ -1,7 +1,10 @@ import { + createLoggingXHROverride, overrideXMLHttpRequest, restoreXMLHttpRequest, } from './xmlHttpRequestOverride'; +// eslint-disable-next-line import/no-namespace +import * as trace from '../../util/trace'; const blockedURLs = [ 'https://example.com/price-api?foo=bar', @@ -61,6 +64,47 @@ describe('overrideXMLHttpRequest', () => { req.send('{"foo": "bar"}'); }), ); + + it('should trace XMLHttpRequest calls', (done) => { + const mockTrace = jest.spyOn(trace, 'trace').mockImplementation(); + overrideXMLHttpRequest(); + const xhr = new XMLHttpRequest(); + xhr.onload = () => { + expect(mockTrace).toHaveBeenCalledWith({ + name: 'api.example.com', + op: trace.TraceOperation.NoBasicFunctionalityHttp, + }); + done(); + }; + xhr.open('GET', 'https://api.example.com/data'); + xhr.send(); + }); +}); + +describe('createLoggingXHROverride', () => { + let mockTrace: jest.SpyInstance; + beforeEach(() => { + mockTrace = jest.spyOn(trace, 'trace').mockImplementation(); + createLoggingXHROverride(); + }); + + afterEach(() => { + restoreXMLHttpRequest(); + jest.clearAllMocks(); + }); + + it('should trace XMLHttpRequest calls', (done) => { + const xhr = new XMLHttpRequest(); + xhr.onload = () => { + expect(mockTrace).toHaveBeenCalledWith({ + name: 'api.example.com', + op: trace.TraceOperation.Http, + }); + done(); + }; + xhr.open('GET', 'https://api.example.com/data'); + xhr.send(); + }); }); describe('restoreXMLHttpRequest', () => { diff --git a/app/store/sagas/xmlHttpRequestOverride.ts b/app/store/sagas/xmlHttpRequestOverride.ts index bcd5921d84c..76b26158673 100644 --- a/app/store/sagas/xmlHttpRequestOverride.ts +++ b/app/store/sagas/xmlHttpRequestOverride.ts @@ -1,5 +1,5 @@ import AppConstants from '../../../app/core/AppConstants'; -import { TraceName, endTrace, trace } from '../../util/trace'; +import { TraceName, TraceOperation, endTrace, trace } from '../../util/trace'; if (process.env.JEST_WORKER_ID !== undefined) { // monkeypatch for Jest @@ -47,6 +47,7 @@ export function createLoggingXHROverride() { const hostname = new URL(currentUrl).hostname; trace({ name: hostname as TraceName, + op: TraceOperation.Http, }); return originalOpen.apply(this, [ method, @@ -99,6 +100,13 @@ export function overrideXMLHttpRequest() { password?: string | null, ) { currentUrl = url?.toString(); + + const hostname = new URL(currentUrl).hostname; + trace({ + name: hostname as TraceName, + op: TraceOperation.NoBasicFunctionalityHttp, + }); + return originalOpen.apply(this, [ method, currentUrl, @@ -110,6 +118,15 @@ export function overrideXMLHttpRequest() { // Override the 'send' method to implement the blocking logic global.XMLHttpRequest.prototype.send = function (...args: unknown[]) { + const hostname = new URL(currentUrl).hostname; + this.addEventListener('load', () => { + endTrace({ name: hostname as TraceName }); + }); + + this.addEventListener('error', () => { + endTrace({ name: hostname as TraceName }); + }); + // Check if the current request should be blocked if (shouldBlockRequest(currentUrl)) { handleError(); // Trigger an error callback or handle the blocked request as needed diff --git a/app/util/trace.ts b/app/util/trace.ts index 4579b07918c..b2e76147c43 100644 --- a/app/util/trace.ts +++ b/app/util/trace.ts @@ -55,6 +55,8 @@ export enum TraceOperation { AccountList = 'account.list', StoreInit = 'store.initialization', Login = 'login', + Http = 'custom.http', + NoBasicFunctionalityHttp = 'custom.no.basic.functionality.http', } const ID_DEFAULT = 'default'; From b08b0aec40ae6f0ef1a9b81636655c306983651e Mon Sep 17 00:00:00 2001 From: tommasini Date: Tue, 12 Nov 2024 20:32:59 +0000 Subject: [PATCH 3/4] remove duplicated code --- app/store/sagas/xmlHttpRequestOverride.ts | 93 ++++++++--------------- app/util/trace.ts | 4 +- 2 files changed, 34 insertions(+), 63 deletions(-) diff --git a/app/store/sagas/xmlHttpRequestOverride.ts b/app/store/sagas/xmlHttpRequestOverride.ts index 76b26158673..e864f30470c 100644 --- a/app/store/sagas/xmlHttpRequestOverride.ts +++ b/app/store/sagas/xmlHttpRequestOverride.ts @@ -29,11 +29,13 @@ if (process.env.JEST_WORKER_ID !== undefined) { const originalSend = global.XMLHttpRequest.prototype.send; const originalOpen = global.XMLHttpRequest.prototype.open; -/** - * This creates a trace of each URL request to sentry, sending just the hostname. - * Currently this is only used when basic functionality is enabled. - */ -export function createLoggingXHROverride() { + +interface XHROverrideOptions { + operation: TraceOperation; + beforeSend?: (url: string) => boolean | void; +} + +function createXHROverride(options: XHROverrideOptions) { let currentUrl = ''; global.XMLHttpRequest.prototype.open = function ( @@ -47,7 +49,7 @@ export function createLoggingXHROverride() { const hostname = new URL(currentUrl).hostname; trace({ name: hostname as TraceName, - op: TraceOperation.Http, + op: options.operation, }); return originalOpen.apply(this, [ method, @@ -68,73 +70,42 @@ export function createLoggingXHROverride() { endTrace({ name: hostname as TraceName }); }); + if (options.beforeSend && options.beforeSend(currentUrl)) { + return; + } + return originalSend.call(this, ...args); }; } +/** + * This creates a trace of each URL request to sentry, sending just the hostname. + * Currently this is only used when basic functionality is enabled. + */ +export function createLoggingXHROverride() { + createXHROverride({ operation: TraceOperation.Http }); +} export function overrideXMLHttpRequest() { - // Store the URL of the current request - only valid under assumption of no new requests being initiated between `open` and `send` of one - let currentUrl = ''; - - // TODO: This should operate on an allowlist rather than fuzzy-blocklist approach - // Only known "good" URLs should be explicitly allowed, as opposed to only "bad" ones disallowed - // see https://github.com/MetaMask/metamask-mobile/issues/10391 const shouldBlockRequest = (url: string) => AppConstants.BASIC_FUNCTIONALITY_BLOCK_LIST.some((blockedUrl) => url.includes(blockedUrl), ); - const handleError = () => - Promise.reject(new Error(`Disallowed URL: ${currentUrl}`)).catch( - (error) => { - console.error(error); - }, - ); - - // Override the 'open' method to capture the request URL - global.XMLHttpRequest.prototype.open = function ( - method: string, - url: string | URL, - async?: boolean, - user?: string | null, - password?: string | null, - ) { - currentUrl = url?.toString(); - - const hostname = new URL(currentUrl).hostname; - trace({ - name: hostname as TraceName, - op: TraceOperation.NoBasicFunctionalityHttp, + const handleError = (url: string) => + Promise.reject(new Error(`Disallowed URL: ${url}`)).catch((error) => { + console.error(error); }); - return originalOpen.apply(this, [ - method, - currentUrl, - async, - user, - password, - ]); - }; - - // Override the 'send' method to implement the blocking logic - global.XMLHttpRequest.prototype.send = function (...args: unknown[]) { - const hostname = new URL(currentUrl).hostname; - this.addEventListener('load', () => { - endTrace({ name: hostname as TraceName }); - }); - - this.addEventListener('error', () => { - endTrace({ name: hostname as TraceName }); - }); - - // Check if the current request should be blocked - if (shouldBlockRequest(currentUrl)) { - handleError(); // Trigger an error callback or handle the blocked request as needed - return; // Do not proceed with the request - } - // For non-blocked requests, proceed as normal - return originalSend.call(this, ...args); - }; + createXHROverride({ + operation: TraceOperation.NoBasicFunctionalityHttp, + beforeSend: (url) => { + if (shouldBlockRequest(url)) { + handleError(url); + return true; // indicates request should be blocked + } + return false; + }, + }); } export function restoreXMLHttpRequest() { diff --git a/app/util/trace.ts b/app/util/trace.ts index b2e76147c43..7c89a865eba 100644 --- a/app/util/trace.ts +++ b/app/util/trace.ts @@ -55,8 +55,8 @@ export enum TraceOperation { AccountList = 'account.list', StoreInit = 'store.initialization', Login = 'login', - Http = 'custom.http', - NoBasicFunctionalityHttp = 'custom.no.basic.functionality.http', + Http = 'http.client', + NoBasicFunctionalityHttp = 'http.client.no_basic_functionality', } const ID_DEFAULT = 'default'; From aaf496385df364291f7e0c413f7d5268c35849a2 Mon Sep 17 00:00:00 2001 From: tommasini Date: Wed, 13 Nov 2024 10:32:33 +0000 Subject: [PATCH 4/4] full url instead of hostname --- app/store/sagas/xmlHttpRequestOverride.test.ts | 4 ++-- app/store/sagas/xmlHttpRequestOverride.ts | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/store/sagas/xmlHttpRequestOverride.test.ts b/app/store/sagas/xmlHttpRequestOverride.test.ts index 97e2cd0f60a..471dc9cbd62 100644 --- a/app/store/sagas/xmlHttpRequestOverride.test.ts +++ b/app/store/sagas/xmlHttpRequestOverride.test.ts @@ -71,7 +71,7 @@ describe('overrideXMLHttpRequest', () => { const xhr = new XMLHttpRequest(); xhr.onload = () => { expect(mockTrace).toHaveBeenCalledWith({ - name: 'api.example.com', + name: 'https://api.example.com/data', op: trace.TraceOperation.NoBasicFunctionalityHttp, }); done(); @@ -97,7 +97,7 @@ describe('createLoggingXHROverride', () => { const xhr = new XMLHttpRequest(); xhr.onload = () => { expect(mockTrace).toHaveBeenCalledWith({ - name: 'api.example.com', + name: 'https://api.example.com/data', op: trace.TraceOperation.Http, }); done(); diff --git a/app/store/sagas/xmlHttpRequestOverride.ts b/app/store/sagas/xmlHttpRequestOverride.ts index e864f30470c..a2b35b65a46 100644 --- a/app/store/sagas/xmlHttpRequestOverride.ts +++ b/app/store/sagas/xmlHttpRequestOverride.ts @@ -46,9 +46,8 @@ function createXHROverride(options: XHROverrideOptions) { password?: string | null, ) { currentUrl = url?.toString(); - const hostname = new URL(currentUrl).hostname; trace({ - name: hostname as TraceName, + name: currentUrl as TraceName, op: options.operation, }); return originalOpen.apply(this, [ @@ -61,13 +60,12 @@ function createXHROverride(options: XHROverrideOptions) { }; global.XMLHttpRequest.prototype.send = function (...args: unknown[]) { - const hostname = new URL(currentUrl).hostname; this.addEventListener('load', () => { - endTrace({ name: hostname as TraceName }); + endTrace({ name: currentUrl as TraceName }); }); this.addEventListener('error', () => { - endTrace({ name: hostname as TraceName }); + endTrace({ name: currentUrl as TraceName }); }); if (options.beforeSend && options.beforeSend(currentUrl)) {