Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add http requests to custom instrumentation #12258

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/store/sagas/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -125,8 +126,10 @@ export function* basicFunctionalityToggle() {

if (basicFunctionalityEnabled) {
restoreXMLHttpRequest();
// apply metrics if enabled
createLoggingXHROverride();
} else {
// apply global blocklist
// apply global blocklist and metrics if enabled
overrideXMLHttpRequest();
}
}
Expand Down
44 changes: 44 additions & 0 deletions app/store/sagas/xmlHttpRequestOverride.test.ts
Original file line number Diff line number Diff line change
@@ -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',
Expand Down Expand Up @@ -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: 'https://api.example.com/data',
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: 'https://api.example.com/data',
op: trace.TraceOperation.Http,
});
done();
};
xhr.open('GET', 'https://api.example.com/data');
xhr.send();
});
});

describe('restoreXMLHttpRequest', () => {
Expand Down
76 changes: 52 additions & 24 deletions app/store/sagas/xmlHttpRequestOverride.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import AppConstants from '../../../app/core/AppConstants';
import { TraceName, TraceOperation, endTrace, trace } from '../../util/trace';

if (process.env.JEST_WORKER_ID !== undefined) {
// monkeypatch for Jest
Expand Down Expand Up @@ -29,26 +30,14 @@ if (process.env.JEST_WORKER_ID !== undefined) {
const originalSend = global.XMLHttpRequest.prototype.send;
const originalOpen = global.XMLHttpRequest.prototype.open;

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),
);
interface XHROverrideOptions {
operation: TraceOperation;
beforeSend?: (url: string) => boolean | void;
}

const handleError = () =>
Promise.reject(new Error(`Disallowed URL: ${currentUrl}`)).catch(
(error) => {
console.error(error);
},
);
function createXHROverride(options: XHROverrideOptions) {
let currentUrl = '';

// Override the 'open' method to capture the request URL
global.XMLHttpRequest.prototype.open = function (
method: string,
url: string | URL,
Expand All @@ -57,6 +46,10 @@ export function overrideXMLHttpRequest() {
password?: string | null,
) {
currentUrl = url?.toString();
trace({
name: currentUrl as TraceName,
op: options.operation,
});
return originalOpen.apply(this, [
method,
currentUrl,
Expand All @@ -66,17 +59,52 @@ export function overrideXMLHttpRequest() {
]);
};

// Override the 'send' method to implement the blocking logic
global.XMLHttpRequest.prototype.send = function (...args: unknown[]) {
// 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
this.addEventListener('load', () => {
endTrace({ name: currentUrl as TraceName });
});

this.addEventListener('error', () => {
endTrace({ name: currentUrl as TraceName });
});

if (options.beforeSend && options.beforeSend(currentUrl)) {
return;
}
// For non-blocked requests, proceed as normal

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() {
const shouldBlockRequest = (url: string) =>
AppConstants.BASIC_FUNCTIONALITY_BLOCK_LIST.some((blockedUrl) =>
url.includes(blockedUrl),
);

const handleError = (url: string) =>
Promise.reject(new Error(`Disallowed URL: ${url}`)).catch((error) => {
console.error(error);
});

createXHROverride({
operation: TraceOperation.NoBasicFunctionalityHttp,
beforeSend: (url) => {
if (shouldBlockRequest(url)) {
handleError(url);
return true; // indicates request should be blocked
}
return false;
},
});
}

export function restoreXMLHttpRequest() {
global.XMLHttpRequest.prototype.open = originalOpen;
Expand Down
2 changes: 2 additions & 0 deletions app/util/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export enum TraceOperation {
AccountList = 'account.list',
StoreInit = 'store.initialization',
Login = 'login',
Http = 'http.client',
NoBasicFunctionalityHttp = 'http.client.no_basic_functionality',
}

const ID_DEFAULT = 'default';
Expand Down
Loading