From 1cab46b1f52a7f1bf4a935ac6aa12667ede04d47 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 27 Mar 2023 10:13:26 +0200 Subject: [PATCH 1/4] reproduction: httpclient integration & axios --- .../browser-integration-tests/package.json | 1 + .../integrations/httpclient/axios/subject.js | 5 ++ .../integrations/httpclient/axios/test.ts | 67 +++++++++++++++++++ yarn.lock | 18 ++--- 4 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js create mode 100644 packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts diff --git a/packages/browser-integration-tests/package.json b/packages/browser-integration-tests/package.json index da75e00b87b4..65b285467904 100644 --- a/packages/browser-integration-tests/package.json +++ b/packages/browser-integration-tests/package.json @@ -46,6 +46,7 @@ "dependencies": { "@babel/preset-typescript": "^7.16.7", "@playwright/test": "^1.31.1", + "axios": "1.3.4", "babel-loader": "^8.2.2", "html-webpack-plugin": "^5.5.0", "pako": "^2.1.0", diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js b/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js new file mode 100644 index 000000000000..1c030aff4ff0 --- /dev/null +++ b/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js @@ -0,0 +1,5 @@ +import axios from 'axios'; + +axios.get('http://localhost:7654/foo').then(response => { + console.log(response.data); +}); diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts new file mode 100644 index 000000000000..a6dfcc755ae0 --- /dev/null +++ b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts @@ -0,0 +1,67 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest( + 'should assign request and response context from a failed 500 XHR request', + async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', route => { + return route.fulfill({ + status: 500, + body: JSON.stringify({ + error: { + message: 'Internal Server Error', + }, + }), + headers: { + 'Content-Type': 'text/html', + }, + }); + }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + + // Not able to get the cookies from the request/response because of Playwright bug + // https://github.com/microsoft/playwright/issues/11035 + expect(eventData).toMatchObject({ + message: 'HTTP Client Error with status code: 500', + exception: { + values: [ + { + type: 'Error', + value: 'HTTP Client Error with status code: 500', + mechanism: { + type: 'http.client', + handled: true, + }, + }, + ], + }, + request: { + url: 'http://localhost:7654/foo', + method: 'GET', + headers: { + Accept: 'application/json', + Cache: 'no-cache', + 'Content-Type': 'application/json', + }, + }, + contexts: { + response: { + status_code: 500, + body_size: 45, + headers: { + 'content-type': 'text/html', + 'content-length': '45', + }, + }, + }, + }); + }, +); diff --git a/yarn.lock b/yarn.lock index 3d5eb7344ea5..280d60a44626 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6686,6 +6686,15 @@ aws4@^1.8.0: resolved "https://registry.yarnpkg.com/aws4/-/aws4-1.11.0.tgz#d61f46d83b2519250e2784daf5b09479a8b41c59" integrity sha512-xh1Rl34h6Fi1DC2WWKfxUTVqRsNnr6LsKz2+hfwDxQJWmrx8+c7ylaqBMcHfl1U1r2dsifOvKX3LQuLNZ+XSvA== +axios@1.3.4, axios@^1.2.2: + version "1.3.4" + resolved "https://registry.yarnpkg.com/axios/-/axios-1.3.4.tgz#f5760cefd9cfb51fd2481acf88c05f67c4523024" + integrity sha512-toYm+Bsyl6VC5wSkfkbbNB6ROv7KY93PEBBL6xyDczaIHasAiv4wPqQ/c4RjoQzipxRD2W5g21cOqQulZ7rHwQ== + dependencies: + follow-redirects "^1.15.0" + form-data "^4.0.0" + proxy-from-env "^1.1.0" + axios@^0.27.2: version "0.27.2" resolved "https://registry.yarnpkg.com/axios/-/axios-0.27.2.tgz#207658cc8621606e586c85db4b41a750e756d972" @@ -6703,15 +6712,6 @@ axios@^1.0.0: form-data "^4.0.0" proxy-from-env "^1.1.0" -axios@^1.2.2: - version "1.3.4" - resolved "https://registry.yarnpkg.com/axios/-/axios-1.3.4.tgz#f5760cefd9cfb51fd2481acf88c05f67c4523024" - integrity sha512-toYm+Bsyl6VC5wSkfkbbNB6ROv7KY93PEBBL6xyDczaIHasAiv4wPqQ/c4RjoQzipxRD2W5g21cOqQulZ7rHwQ== - dependencies: - follow-redirects "^1.15.0" - form-data "^4.0.0" - proxy-from-env "^1.1.0" - babel-code-frame@^6.26.0: version "6.26.0" resolved "https://registry.yarnpkg.com/babel-code-frame/-/babel-code-frame-6.26.0.tgz#63fd43f7dc1e3bb7ce35947db8fe369a3f58c74b" From 39c2e4ac5fbd45bef9e6f243059d7c0520e2f4df Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Apr 2023 15:04:29 +0200 Subject: [PATCH 2/4] fix(integration): Ensure HttpClient integration works with axios This streamlines the integration to use the general fetch/xhr instrumentation, instead of adding it's own. This also adds `request_headers` to the xhr info (which we'll also need for replay). It also ensures that we always correctly get the method/url from fetch options, even when they are other valid objects. --- .../integrations/httpclient/axios/subject.js | 10 +- .../integrations/httpclient/axios/test.ts | 1 - packages/integrations/src/httpclient.ts | 106 +++++++----------- packages/types/src/instrument.ts | 1 + packages/utils/src/instrument.ts | 84 ++++++++++---- packages/utils/test/instrument.test.ts | 30 +++++ 6 files changed, 146 insertions(+), 86 deletions(-) create mode 100644 packages/utils/test/instrument.test.ts diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js b/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js index 1c030aff4ff0..5cac9d385e15 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js +++ b/packages/browser-integration-tests/suites/integrations/httpclient/axios/subject.js @@ -1,5 +1,9 @@ import axios from 'axios'; -axios.get('http://localhost:7654/foo').then(response => { - console.log(response.data); -}); +axios + .get('http://localhost:7654/foo', { + headers: { Accept: 'application/json', 'Content-Type': 'application/json', Cache: 'no-cache' }, + }) + .then(response => { + console.log(response.data); + }); diff --git a/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts index a6dfcc755ae0..89c2b768a440 100644 --- a/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts +++ b/packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts @@ -49,7 +49,6 @@ sentryTest( headers: { Accept: 'application/json', Cache: 'no-cache', - 'Content-Type': 'application/json', }, }, contexts: { diff --git a/packages/integrations/src/httpclient.ts b/packages/integrations/src/httpclient.ts index 8fab1825fa03..9608068edf94 100644 --- a/packages/integrations/src/httpclient.ts +++ b/packages/integrations/src/httpclient.ts @@ -1,5 +1,19 @@ -import type { Event as SentryEvent, EventProcessor, Hub, Integration } from '@sentry/types'; -import { addExceptionMechanism, fill, GLOBAL_OBJ, logger, supportsNativeFetch } from '@sentry/utils'; +import type { + Event as SentryEvent, + EventProcessor, + HandlerDataFetch, + HandlerDataXhr, + Hub, + Integration, + SentryWrappedXMLHttpRequest, +} from '@sentry/types'; +import { + addExceptionMechanism, + addInstrumentationHandler, + GLOBAL_OBJ, + logger, + supportsNativeFetch, +} from '@sentry/utils'; export type HttpStatusCodeRange = [number, number] | number; export type HttpRequestTarget = string | RegExp; @@ -283,25 +297,15 @@ export class HttpClient implements Integration { return; } - // eslint-disable-next-line @typescript-eslint/no-this-alias - const self = this; - - fill(GLOBAL_OBJ, 'fetch', function (originalFetch: (...args: unknown[]) => Promise) { - return function (this: Window, ...args: unknown[]): Promise { - const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; - const responsePromise: Promise = originalFetch.apply(this, args); - - responsePromise - .then((response: Response) => { - self._fetchResponseHandler(requestInfo, response, requestInit); - return response; - }) - .catch((error: Error) => { - throw error; - }); + addInstrumentationHandler('fetch', (handlerData: HandlerDataFetch & { response?: Response }) => { + const { response, args } = handlerData; + const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; + + if (!response) { + return; + } - return responsePromise; - }; + this._fetchResponseHandler(requestInfo, response, requestInit); }); } @@ -313,52 +317,28 @@ export class HttpClient implements Integration { return; } - // eslint-disable-next-line @typescript-eslint/no-this-alias - const self = this; - - fill(XMLHttpRequest.prototype, 'open', function (originalOpen: (...openArgs: unknown[]) => void): () => void { - return function (this: XMLHttpRequest, ...openArgs: unknown[]): void { - // eslint-disable-next-line @typescript-eslint/no-this-alias - const xhr = this; - const method = openArgs[0] as string; - const headers: Record = {}; - - // Intercepting `setRequestHeader` to access the request headers of XHR instance. - // This will only work for user/library defined headers, not for the default/browser-assigned headers. - // Request cookies are also unavailable for XHR, as `Cookie` header can't be defined by `setRequestHeader`. - fill( - xhr, - 'setRequestHeader', - // eslint-disable-next-line @typescript-eslint/ban-types - function (originalSetRequestHeader: (...setRequestHeaderArgs: unknown[]) => void): Function { - return function (...setRequestHeaderArgs: unknown[]): void { - const [header, value] = setRequestHeaderArgs as [string, string]; - - headers[header] = value; - - return originalSetRequestHeader.apply(xhr, setRequestHeaderArgs); - }; - }, - ); + addInstrumentationHandler( + 'xhr', + (handlerData: HandlerDataXhr & { xhr: SentryWrappedXMLHttpRequest & XMLHttpRequest }) => { + const { xhr } = handlerData; - // eslint-disable-next-line @typescript-eslint/ban-types - fill(xhr, 'onloadend', function (original?: (...onloadendArgs: unknown[]) => void): Function { - return function (...onloadendArgs: unknown[]): void { - try { - self._xhrResponseHandler(xhr, method, headers); - } catch (e) { - __DEBUG_BUILD__ && logger.warn('Error while extracting response event form XHR response', e); - } + if (!xhr.__sentry_xhr__) { + return; + } - if (original) { - return original.apply(xhr, onloadendArgs); - } - }; - }); + const { method, request_headers: headers } = xhr.__sentry_xhr__; - return originalOpen.apply(this, openArgs); - }; - }); + if (!method) { + return; + } + + try { + this._xhrResponseHandler(xhr, method, headers); + } catch (e) { + __DEBUG_BUILD__ && logger.warn('Error while extracting response event form XHR response', e); + } + }, + ); } /** diff --git a/packages/types/src/instrument.ts b/packages/types/src/instrument.ts index 94b4767fd875..5f00c0df926e 100644 --- a/packages/types/src/instrument.ts +++ b/packages/types/src/instrument.ts @@ -15,6 +15,7 @@ export interface SentryXhrData { body?: XHRSendInput; request_body_size?: number; response_body_size?: number; + request_headers: Record; } export interface HandlerDataXhr { diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 60401eafec97..d422caab44b2 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -9,7 +9,7 @@ import type { WrappedFunction, } from '@sentry/types'; -import { isInstanceOf, isString } from './is'; +import { isString } from './is'; import { CONSOLE_LEVELS, logger } from './logger'; import { fill } from './object'; import { getFunctionName } from './stacktrace'; @@ -142,11 +142,13 @@ function instrumentFetch(): void { fill(WINDOW, 'fetch', function (originalFetch: () => void): () => void { return function (...args: any[]): void { + const { method, url } = parseFetchArgs(args); + const handlerData: HandlerDataFetch = { args, fetchData: { - method: getFetchMethod(args), - url: getFetchUrl(args), + method, + url, }, startTimestamp: Date.now(), }; @@ -181,29 +183,55 @@ function instrumentFetch(): void { }); } -/* eslint-disable @typescript-eslint/no-unsafe-member-access */ -/** Extract `method` from fetch call arguments */ -function getFetchMethod(fetchArgs: any[] = []): string { - if ('Request' in WINDOW && isInstanceOf(fetchArgs[0], Request) && fetchArgs[0].method) { - return String(fetchArgs[0].method).toUpperCase(); +function hasProp(obj: unknown, prop: T): obj is Record { + return !!obj && typeof obj === 'object' && !!(obj as Record)[prop]; +} + +type FetchResource = string | { toString(): string } | { url: string }; + +function getUrlFromResource(resource: FetchResource): string { + if (typeof resource === 'string') { + return resource; + } + + if (!resource) { + return ''; } - if (fetchArgs[1] && fetchArgs[1].method) { - return String(fetchArgs[1].method).toUpperCase(); + + if (hasProp(resource, 'url')) { + return resource.url; + } + + if (resource.toString) { + return resource.toString(); } - return 'GET'; + + return ''; } -/** Extract `url` from fetch call arguments */ -function getFetchUrl(fetchArgs: any[] = []): string { - if (typeof fetchArgs[0] === 'string') { - return fetchArgs[0]; +/** + * Exported only for tests. + * @hidden + * */ +export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string } { + if (fetchArgs.length === 0) { + return { method: 'GET', url: '' }; } - if ('Request' in WINDOW && isInstanceOf(fetchArgs[0], Request)) { - return fetchArgs[0].url; + + if (fetchArgs.length === 2) { + const [url, options] = fetchArgs as [FetchResource, object]; + + return { + url: getUrlFromResource(url), + method: hasProp(options, 'method') ? String(options.method).toUpperCase() : 'GET', + }; } - return String(fetchArgs[0]); + + return { + url: getUrlFromResource(fetchArgs[0] as FetchResource), + method: 'GET', + }; } -/* eslint-enable @typescript-eslint/no-unsafe-member-access */ /** JSDoc */ function instrumentXHR(): void { @@ -220,6 +248,7 @@ function instrumentXHR(): void { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access method: isString(args[0]) ? args[0].toUpperCase() : args[0], url: args[1], + request_headers: {}, }); // if Sentry key appears in URL, don't capture it as a request @@ -265,6 +294,23 @@ function instrumentXHR(): void { this.addEventListener('readystatechange', onreadystatechangeHandler); } + // Intercepting `setRequestHeader` to access the request headers of XHR instance. + // This will only work for user/library defined headers, not for the default/browser-assigned headers. + // Request cookies are also unavailable for XHR, as `Cookie` header can't be defined by `setRequestHeader`. + fill(this, 'setRequestHeader', function (original: WrappedFunction): Function { + return function (this: SentryWrappedXMLHttpRequest, ...setRequestHeaderArgs: unknown[]): void { + const [header, value] = setRequestHeaderArgs as [string, string]; + + const xhrInfo = this.__sentry_xhr__; + + if (xhrInfo) { + xhrInfo.request_headers[header] = value; + } + + return original.apply(this, setRequestHeaderArgs); + }; + }); + return originalOpen.apply(this, args); }; }); diff --git a/packages/utils/test/instrument.test.ts b/packages/utils/test/instrument.test.ts new file mode 100644 index 000000000000..98b2ae5b3ad4 --- /dev/null +++ b/packages/utils/test/instrument.test.ts @@ -0,0 +1,30 @@ +import { parseFetchArgs } from '../src/instrument'; + +describe('instrument', () => { + describe('parseFetchArgs', () => { + it.each([ + ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com' }], + ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/' }], + ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com' }], + [ + 'string URL & options', + ['http://example.com', { method: 'post' }], + { method: 'POST', url: 'http://example.com' }, + ], + [ + 'URL object & options', + [new URL('http://example.com'), { method: 'post' }], + { method: 'POST', url: 'http://example.com/' }, + ], + [ + 'Request URL & options', + [{ url: 'http://example.com' }, { method: 'post' }], + { method: 'POST', url: 'http://example.com' }, + ], + ])('%s', (_name, args, expected) => { + const actual = parseFetchArgs(args as unknown[]); + + expect(actual).toEqual(expected); + }); + }); +}); From 2227d1915bb68e1269203b97f11d92ea54774423 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Apr 2023 15:34:31 +0200 Subject: [PATCH 3/4] PR feedback --- packages/utils/src/instrument.ts | 5 +++-- packages/utils/test/instrument.test.ts | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index d422caab44b2..f2ced4540d5f 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -227,9 +227,10 @@ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: str }; } + const arg = fetchArgs[0]; return { - url: getUrlFromResource(fetchArgs[0] as FetchResource), - method: 'GET', + url: getUrlFromResource(arg as FetchResource), + method: hasProp(arg, 'method') ? String(arg.method).toUpperCase() : 'GET', }; } diff --git a/packages/utils/test/instrument.test.ts b/packages/utils/test/instrument.test.ts index 98b2ae5b3ad4..f9088ca1257a 100644 --- a/packages/utils/test/instrument.test.ts +++ b/packages/utils/test/instrument.test.ts @@ -6,6 +6,11 @@ describe('instrument', () => { ['string URL only', ['http://example.com'], { method: 'GET', url: 'http://example.com' }], ['URL object only', [new URL('http://example.com')], { method: 'GET', url: 'http://example.com/' }], ['Request URL only', [{ url: 'http://example.com' }], { method: 'GET', url: 'http://example.com' }], + [ + 'Request URL & method only', + [{ url: 'http://example.com', method: 'post' }], + { method: 'POST', url: 'http://example.com' }, + ], [ 'string URL & options', ['http://example.com', { method: 'post' }], From b1825e97907c0fe6794fa1722256da6980ec7082 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 4 Apr 2023 09:06:15 +0200 Subject: [PATCH 4/4] fix test --- packages/replay/test/unit/coreHandlers/handleXhr.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/replay/test/unit/coreHandlers/handleXhr.test.ts b/packages/replay/test/unit/coreHandlers/handleXhr.test.ts index 0d19ba10c59d..48e4d0ebece6 100644 --- a/packages/replay/test/unit/coreHandlers/handleXhr.test.ts +++ b/packages/replay/test/unit/coreHandlers/handleXhr.test.ts @@ -1,4 +1,4 @@ -import type { HandlerDataXhr, SentryWrappedXMLHttpRequest } from '@sentry/types'; +import type { HandlerDataXhr, SentryWrappedXMLHttpRequest, SentryXhrData } from '@sentry/types'; import { handleXhr } from '../../../src/coreHandlers/handleXhr'; @@ -9,6 +9,7 @@ const DEFAULT_DATA: HandlerDataXhr = { method: 'GET', url: '/api/0/organizations/sentry/', status_code: 200, + request_headers: {}, }, } as SentryWrappedXMLHttpRequest, startTimestamp: 10000, @@ -45,7 +46,7 @@ describe('Unit | coreHandlers | handleXhr', () => { xhr: { ...DEFAULT_DATA.xhr, __sentry_xhr__: { - ...DEFAULT_DATA.xhr.__sentry_xhr__, + ...(DEFAULT_DATA.xhr.__sentry_xhr__ as SentryXhrData), request_body_size: 123, response_body_size: 456, },