From 03149498d3b53b066f5c6835f72e92a4b3bcb2b8 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Mar 2023 16:38:47 +0200 Subject: [PATCH] feat(sveltekit): Add instrumentation for client-side fetch properly sanitize url, add url data apply data to breadcrumbs cleanup set span status add tests sorry fix authority url "fix" authority regex again cleanup move addTracingHeadersToFetchRequest back to tracing-internal cleanup apply suggestions re-activate request object test adjust types after exporting --- packages/node/src/integrations/http.ts | 17 +- packages/sveltekit/src/client/load.ts | 204 ++++++++++- packages/sveltekit/test/client/load.test.ts | 344 ++++++++++++++++-- .../tracing-internal/src/browser/index.ts | 6 +- .../tracing-internal/src/browser/request.ts | 15 +- packages/tracing-internal/src/index.ts | 1 + packages/types/src/index.ts | 2 +- packages/types/src/request.ts | 12 + packages/utils/src/instrument.ts | 5 +- packages/utils/src/url.ts | 38 +- packages/utils/test/url.test.ts | 50 ++- 11 files changed, 631 insertions(+), 63 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 058393bc0767..c7239d924af3 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,6 +1,6 @@ import type { Hub } from '@sentry/core'; import { getCurrentHub } from '@sentry/core'; -import type { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types'; +import type { EventProcessor, Integration, SanitizedRequestData, Span, TracePropagationTargets } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, fill, logger, stringMatchesSomePattern } from '@sentry/utils'; import type * as http from 'http'; import type * as https from 'https'; @@ -122,16 +122,6 @@ type OriginalRequestMethod = RequestMethod; type WrappedRequestMethod = RequestMethod; type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedRequestMethod; -/** - * See https://develop.sentry.dev/sdk/data-handling/#structuring-data - */ -type RequestSpanData = { - url: string; - method: string; - 'http.fragment'?: string; - 'http.query'?: string; -}; - /** * Function which creates a function which creates wrapped versions of internal `request` and `get` calls within `http` * and `https` modules. (NB: Not a typo - this is a creator^2!) @@ -197,7 +187,7 @@ function _createWrappedRequestMethodFactory( const scope = getCurrentHub().getScope(); - const requestSpanData: RequestSpanData = { + const requestSpanData: SanitizedRequestData = { url: requestUrl, method: requestOptions.method || 'GET', }; @@ -304,7 +294,7 @@ function _createWrappedRequestMethodFactory( */ function addRequestBreadcrumb( event: string, - requestSpanData: RequestSpanData, + requestSpanData: SanitizedRequestData, req: http.ClientRequest, res?: http.IncomingMessage, ): void { @@ -316,7 +306,6 @@ function addRequestBreadcrumb( { category: 'http', data: { - method: req.method, status_code: res && res.statusCode, ...requestSpanData, }, diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index a154816b3dba..6db4dae09840 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,6 +1,17 @@ -import { trace } from '@sentry/core'; +import { addTracingHeadersToFetchRequest } from '@sentry-internal/tracing'; +import type { BaseClient } from '@sentry/core'; +import { getCurrentHub, trace } from '@sentry/core'; +import type { Breadcrumbs, BrowserTracing } from '@sentry/svelte'; import { captureException } from '@sentry/svelte'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import type { ClientOptions, SanitizedRequestData } from '@sentry/types'; +import { + addExceptionMechanism, + getSanitizedUrlString, + objectify, + parseFetchArgs, + parseUrl, + stringMatchesSomePattern, +} from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; import { isRedirect } from '../common/utils'; @@ -34,7 +45,17 @@ function sendErrorToSentry(e: unknown): unknown { } /** - * @inheritdoc + * Wrap load function with Sentry. This wrapper will + * + * - catch errors happening during the execution of `load` + * - create a load span if performance monitoring is enabled + * - attach tracing Http headers to `fech` requests if performance monitoring is enabled to get connected traces. + * - add a fetch breadcrumb for every `fetch` request + * + * Note that tracing Http headers are only attached if the url matches the specified `tracePropagationTargets` + * entries to avoid CORS errors. + * + * @param origLoad SvelteKit user defined load function */ // The liberal generic typing of `T` is necessary because we cannot let T extend `Load`. // This function needs to tell TS that it returns exactly the type that it was called with @@ -47,6 +68,11 @@ export function wrapLoadWithSentry any>(origLoad: T) // Type casting here because `T` cannot extend `Load` (see comment above function signature) const event = args[0] as LoadEvent; + const patchedEvent = { + ...event, + fetch: instrumentSvelteKitFetch(event.fetch), + }; + const routeId = event.route.id; return trace( { @@ -57,9 +83,179 @@ export function wrapLoadWithSentry any>(origLoad: T) source: routeId ? 'route' : 'url', }, }, - () => wrappingTarget.apply(thisArg, args), + () => wrappingTarget.apply(thisArg, [patchedEvent]), sendErrorToSentry, ); }, }); } + +type SvelteKitFetch = LoadEvent['fetch']; + +/** + * Instruments SvelteKit's client `fetch` implementation which is passed to the client-side universal `load` functions. + * + * We need to instrument this in addition to the native fetch we instrument in BrowserTracing because SvelteKit + * stores the native fetch implementation before our SDK is initialized. + * + * see: https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/client/fetcher.js + * + * This instrumentation takes the fetch-related options from `BrowserTracing` to determine if we should + * instrument fetch for perfomance monitoring, create a span for or attach our tracing headers to the given request. + * + * To dertermine if breadcrumbs should be recorded, this instrumentation relies on the availability of and the options + * set in the `BreadCrumbs` integration. + * + * @param originalFetch SvelteKit's original fetch implemenetation + * + * @returns a proxy of SvelteKit's fetch implementation + */ +function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch { + const client = getCurrentHub().getClient() as BaseClient; + + const browserTracingIntegration = + client.getIntegrationById && (client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined); + const breadcrumbsIntegration = + client.getIntegrationById && (client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined); + + const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options; + + const shouldTraceFetch = browserTracingOptions && browserTracingOptions.traceFetch; + const shouldAddFetchBreadcrumb = breadcrumbsIntegration && breadcrumbsIntegration.options.fetch; + + /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ + const shouldCreateSpan = + browserTracingOptions && typeof browserTracingOptions.shouldCreateSpanForRequest === 'function' + ? browserTracingOptions.shouldCreateSpanForRequest + : (_: string) => shouldTraceFetch; + + /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ + const shouldAttachHeaders: (url: string) => boolean = url => { + return ( + !!shouldTraceFetch && + stringMatchesSomePattern(url, browserTracingOptions.tracePropagationTargets || ['localhost', /^\//]) + ); + }; + + return new Proxy(originalFetch, { + apply: (wrappingTarget, thisArg, args: Parameters) => { + const [input, init] = args; + const { url: rawUrl, method } = parseFetchArgs(args); + + // TODO: extract this to a util function (and use it in breadcrumbs integration as well) + if (rawUrl.match(/sentry_key/)) { + // We don't create spans or breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests) + return wrappingTarget.apply(thisArg, args); + } + + const urlObject = parseUrl(rawUrl); + + const requestData: SanitizedRequestData = { + url: getSanitizedUrlString(urlObject), + method, + ...(urlObject.search && { 'http.query': urlObject.search.substring(1) }), + ...(urlObject.hash && { 'http.hash': urlObject.hash.substring(1) }), + }; + + const patchedInit: RequestInit = { ...init }; + const activeSpan = getCurrentHub().getScope().getSpan(); + const activeTransaction = activeSpan && activeSpan.transaction; + + const createSpan = activeTransaction && shouldCreateSpan(rawUrl); + const attachHeaders = createSpan && activeTransaction && shouldAttachHeaders(rawUrl); + + // only attach headers if we should create a span + if (attachHeaders) { + const dsc = activeTransaction.getDynamicSamplingContext(); + + const headers = addTracingHeadersToFetchRequest( + input as string | Request, + dsc, + activeSpan, + patchedInit as { + headers: + | { + [key: string]: string[] | string | undefined; + } + | Request['headers']; + }, + ) as HeadersInit; + + patchedInit.headers = headers; + } + let fetchPromise: Promise; + + const patchedFetchArgs = [input, patchedInit]; + + if (createSpan) { + fetchPromise = trace( + { + name: `${requestData.method} ${requestData.url}`, // this will become the description of the span + op: 'http.client', + data: requestData, + }, + span => { + const promise: Promise = wrappingTarget.apply(thisArg, patchedFetchArgs); + if (span) { + promise.then(res => span.setHttpStatus(res.status)).catch(_ => span.setStatus('internal_error')); + } + return promise; + }, + ); + } else { + fetchPromise = wrappingTarget.apply(thisArg, patchedFetchArgs); + } + + if (shouldAddFetchBreadcrumb) { + addFetchBreadcrumb(fetchPromise, requestData, args); + } + + return fetchPromise; + }, + }); +} + +/* Adds a breadcrumb for the given fetch result */ +function addFetchBreadcrumb( + fetchResult: Promise, + requestData: SanitizedRequestData, + args: Parameters, +): void { + const breadcrumbStartTimestamp = Date.now(); + fetchResult.then( + response => { + getCurrentHub().addBreadcrumb( + { + type: 'http', + category: 'fetch', + data: { + ...requestData, + status_code: response.status, + }, + }, + { + input: args, + response, + startTimestamp: breadcrumbStartTimestamp, + endTimestamp: Date.now(), + }, + ); + }, + error => { + getCurrentHub().addBreadcrumb( + { + type: 'http', + category: 'fetch', + level: 'error', + data: requestData, + }, + { + input: args, + data: error, + startTimestamp: breadcrumbStartTimestamp, + endTimestamp: Date.now(), + }, + ); + }, + ); +} diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index ed5193e81e47..22c13157e9db 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,10 +1,17 @@ import { addTracingExtensions, Scope } from '@sentry/svelte'; +import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; import type { Load } from '@sveltejs/kit'; import { redirect } from '@sveltejs/kit'; import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/client/load'; +const SENTRY_TRACE_HEADER = '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; +const BAGGAGE_HEADER = + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1'; + const mockCaptureException = vi.fn(); let mockScope = new Scope(); @@ -22,6 +29,29 @@ vi.mock('@sentry/svelte', async () => { const mockTrace = vi.fn(); +const mockedBrowserTracing = { + options: { + tracePropagationTargets: ['example.com', /^\\/], + traceFetch: true, + shouldCreateSpanForRequest: undefined as undefined | (() => boolean), + }, +}; + +const mockedBreadcrumbs = { + options: { + fetch: true, + }, +}; + +const mockedGetIntegrationById = vi.fn(id => { + if (id === 'BrowserTracing') { + return mockedBrowserTracing; + } else if (id === 'Breadcrumbs') { + return mockedBreadcrumbs; + } + return undefined; +}); + vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { @@ -30,10 +60,37 @@ vi.mock('@sentry/core', async () => { mockTrace(...args); return original.trace(...args); }, + getCurrentHub: () => { + return { + getClient: () => { + return { + getIntegrationById: mockedGetIntegrationById, + }; + }, + getScope: () => { + return { + getSpan: () => { + return { + transaction: { + getDynamicSamplingContext: () => { + return baggageHeaderToDynamicSamplingContext(BAGGAGE_HEADER); + }, + }, + toTraceparent: () => { + return SENTRY_TRACE_HEADER; + }, + }; + }, + }; + }, + addBreadcrumb: mockedAddBreadcrumb, + }; + }, }; }); const mockAddExceptionMechanism = vi.fn(); +const mockedAddBreadcrumb = vi.fn(); vi.mock('@sentry/utils', async () => { const original = (await vi.importActual('@sentry/utils')) as any; @@ -47,6 +104,8 @@ function getById(_id?: string) { throw new Error('error'); } +const mockedSveltekitFetch = vi.fn().mockReturnValue(Promise.resolve({ status: 200 })); + const MOCK_LOAD_ARGS: any = { params: { id: '123' }, route: { @@ -57,21 +116,18 @@ const MOCK_LOAD_ARGS: any = { headers: { get: (key: string) => { if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + return SENTRY_TRACE_HEADER; } if (key === 'baggage') { - return ( - 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' - ); + return BAGGAGE_HEADER; } return null; }, }, }, + fetch: mockedSveltekitFetch, }; beforeAll(() => { @@ -83,6 +139,9 @@ describe('wrapLoadWithSentry', () => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); mockTrace.mockClear(); + mockedGetIntegrationById.mockClear(); + mockedSveltekitFetch.mockClear(); + mockedAddBreadcrumb.mockClear(); mockScope = new Scope(); }); @@ -112,29 +171,260 @@ describe('wrapLoadWithSentry', () => { expect(mockCaptureException).not.toHaveBeenCalled(); }); - it('calls trace function', async () => { - async function load({ params }: Parameters[0]): Promise> { - return { - post: params.id, - }; - } + describe('calls trace function', async () => { + it('creates a load span', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, }, - }, - expect.any(Function), - expect.any(Function), - ); + expect.any(Function), + expect.any(Function), + ); + }); + + describe.each([ + [ + 'fetch call with fragment and params', + ['example.com/api/users/?id=123#testfragment'], + { + op: 'http.client', + name: 'GET example.com/api/users/', + data: { + method: 'GET', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with options object', + ['example.com/api/users/?id=123#testfragment', { method: 'POST' }], + { + op: 'http.client', + name: 'POST example.com/api/users/', + data: { + method: 'POST', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with custom headers in options ', + ['example.com/api/users/?id=123#testfragment', { method: 'POST', headers: { 'x-my-header': 'some value' } }], + { + op: 'http.client', + name: 'POST example.com/api/users/', + data: { + method: 'POST', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with a Request object ', + [{ url: '/api/users?id=123', headers: { 'x-my-header': 'value' } } as unknown as Request], + { + op: 'http.client', + name: 'GET /api/users', + data: { + method: 'GET', + url: '/api/users', + 'http.query': 'id=123', + }, + }, + ], + ])('instruments fetch (%s)', (_, originalFetchArgs, spanCtx) => { + beforeEach(() => { + mockedBrowserTracing.options = { + tracePropagationTargets: ['example.com', /^\//], + traceFetch: true, + shouldCreateSpanForRequest: undefined, + }; + }); + + const load = async ({ params, fetch }) => { + await fetch(...originalFetchArgs); + return { + post: params.id, + }; + }; + + it('creates a fetch span and attaches tracing headers by default when event.fetch was called', async () => { + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(2); + expect(mockTrace).toHaveBeenNthCalledWith( + 1, + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); + + const hasSecondArg = originalFetchArgs.length > 1; + const expectedFetchArgs = [ + originalFetchArgs[0], + { + ...(hasSecondArg && (originalFetchArgs[1] as RequestInit)), + headers: { + // @ts-ignore that's fine + ...(hasSecondArg && (originalFetchArgs[1].headers as RequestInit['headers'])), + baggage: expect.any(String), + 'sentry-trace': expect.any(String), + }, + }, + ]; + + expect(mockedSveltekitFetch).toHaveBeenCalledWith(...expectedFetchArgs); + }); + + it("only creates a span but doesn't propagate headers if traceProgagationTargets don't match", async () => { + const previousPropagationTargets = mockedBrowserTracing.options.tracePropagationTargets; + mockedBrowserTracing.options.tracePropagationTargets = []; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(2); + expect(mockTrace).toHaveBeenNthCalledWith( + 1, + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.tracePropagationTargets = previousPropagationTargets; + }); + + it("doesn't create a span nor propagate headers, if `Browsertracing.options.traceFetch` is false", async () => { + mockedBrowserTracing.options.traceFetch = false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.traceFetch = true; + }); + + it("doesn't create a span nor propagate headers, if `shouldCreateSpanForRequest` returns false", async () => { + mockedBrowserTracing.options.shouldCreateSpanForRequest = () => false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true; + }); + + it('adds a breadcrumb for the fetch call', async () => { + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockedAddBreadcrumb).toHaveBeenCalledWith( + { + category: 'fetch', + data: { + ...spanCtx.data, + status_code: 200, + }, + type: 'http', + }, + { + endTimestamp: expect.any(Number), + input: [...originalFetchArgs], + response: { + status: 200, + }, + startTimestamp: expect.any(Number), + }, + ); + }); + + it("doesn't add a breadcrumb if fetch breadcrumbs are deactivated in the integration", async () => { + mockedBreadcrumbs.options.fetch = false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockedAddBreadcrumb).not.toHaveBeenCalled(); + + mockedBreadcrumbs.options.fetch = true; + }); + }); }); it('adds an exception mechanism', async () => { diff --git a/packages/tracing-internal/src/browser/index.ts b/packages/tracing-internal/src/browser/index.ts index 3ed465eea6ca..4cca5a19d9cf 100644 --- a/packages/tracing-internal/src/browser/index.ts +++ b/packages/tracing-internal/src/browser/index.ts @@ -3,4 +3,8 @@ export * from '../exports'; export type { RequestInstrumentationOptions } from './request'; export { BrowserTracing, BROWSER_TRACING_INTEGRATION_ID } from './browsertracing'; -export { instrumentOutgoingRequests, defaultRequestInstrumentationOptions } from './request'; +export { + instrumentOutgoingRequests, + defaultRequestInstrumentationOptions, + addTracingHeadersToFetchRequest, +} from './request'; diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index e1e1c9aaf7fb..bce017af48f4 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -98,7 +98,7 @@ type PolymorphicRequestHeaders = // eslint-disable-next-line @typescript-eslint/no-explicit-any [key: string]: any; append: (key: string, value: string) => void; - get: (key: string) => string; + get: (key: string) => string | null | undefined; }; export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { @@ -221,8 +221,11 @@ export function fetchCallback( } } -function addTracingHeadersToFetchRequest( - request: string | Request, +/** + * Adds sentry-trace and baggage headers to the various forms of fetch headers + */ +export function addTracingHeadersToFetchRequest( + request: string | unknown, // unknown is actually type Request but we can't export DOM types from this package, dynamicSamplingContext: Partial, span: Span, options: { @@ -230,7 +233,7 @@ function addTracingHeadersToFetchRequest( | { [key: string]: string[] | string | undefined; } - | Request['headers']; + | PolymorphicRequestHeaders; }, ): PolymorphicRequestHeaders { const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); @@ -247,7 +250,7 @@ function addTracingHeadersToFetchRequest( newHeaders.append('sentry-trace', sentryTraceHeader); if (sentryBaggageHeader) { - // If the same header is appended miultiple times the browser will merge the values into a single request header. + // If the same header is appended multiple times the browser will merge the values into a single request header. // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. newHeaders.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); } @@ -262,7 +265,7 @@ function addTracingHeadersToFetchRequest( newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); } - return newHeaders; + return newHeaders as PolymorphicRequestHeaders; } else { const existingBaggageHeader = 'baggage' in headers ? headers.baggage : undefined; const newBaggageHeaders: string[] = []; diff --git a/packages/tracing-internal/src/index.ts b/packages/tracing-internal/src/index.ts index da9b94aac666..8742ec7c3ea5 100644 --- a/packages/tracing-internal/src/index.ts +++ b/packages/tracing-internal/src/index.ts @@ -17,6 +17,7 @@ export { BROWSER_TRACING_INTEGRATION_ID, instrumentOutgoingRequests, defaultRequestInstrumentationOptions, + addTracingHeadersToFetchRequest, } from './browser'; export type { RequestInstrumentationOptions } from './browser'; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index e62fe6390ac8..7768a73fb6da 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -48,7 +48,7 @@ export type { ClientOptions, Options } from './options'; export type { Package } from './package'; export type { PolymorphicEvent, PolymorphicRequest } from './polymorphics'; export type { ReplayEvent, ReplayRecordingData, ReplayRecordingMode } from './replay'; -export type { QueryParams, Request } from './request'; +export type { QueryParams, Request, SanitizedRequestData } from './request'; export type { Runtime } from './runtime'; export type { CaptureContext, Scope, ScopeContext } from './scope'; export type { SdkInfo } from './sdkinfo'; diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index c06b29525a84..0c5677a35fdd 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -10,3 +10,15 @@ export interface Request { } export type QueryParams = string | { [key: string]: string } | Array<[string, string]>; + +/** + * Request data that is considered safe for `span.data` on `http.client` spans + * and for `http` breadcrumbs + * See https://develop.sentry.dev/sdk/data-handling/#structuring-data + */ +export type SanitizedRequestData = { + url: string; + method: string; + 'http.fragment'?: string; + 'http.query'?: string; +}; diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index f2ced4540d5f..d8779afee1b5 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -210,9 +210,8 @@ function getUrlFromResource(resource: FetchResource): string { } /** - * Exported only for tests. - * @hidden - * */ + * Parses the fetch arguments to find the used Http method and the url of the request + */ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string } { if (fetchArgs.length === 0) { return { method: 'GET', url: '' }; diff --git a/packages/utils/src/url.ts b/packages/utils/src/url.ts index 8dce48e7428b..71ce23e63a57 100644 --- a/packages/utils/src/url.ts +++ b/packages/utils/src/url.ts @@ -1,3 +1,12 @@ +type PartialURL = { + host?: string; + path?: string; + protocol?: string; + relative?: string; + search?: string; + hash?: string; +}; + /** * Parses string form of URL into an object * // borrowed from https://tools.ietf.org/html/rfc3986#appendix-B @@ -5,12 +14,7 @@ * // environments where DOM might not be available * @returns parsed URL object */ -export function parseUrl(url: string): { - host?: string; - path?: string; - protocol?: string; - relative?: string; -} { +export function parseUrl(url: string): PartialURL { if (!url) { return {}; } @@ -28,6 +32,8 @@ export function parseUrl(url: string): { host: match[4], path: match[5], protocol: match[2], + search: query, + hash: fragment, relative: match[5] + query + fragment, // everything minus origin }; } @@ -50,3 +56,23 @@ export function getNumberOfUrlSegments(url: string): number { // split at '/' or at '\/' to split regex urls correctly return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; } + +/** + * Takes a URL object and returns a sanitized string which is safe to use as span description + * see: https://develop.sentry.dev/sdk/data-handling/#structuring-data + */ +export function getSanitizedUrlString(url: PartialURL): string { + const { protocol, host, path } = url; + + const filteredHost = + (host && + host + // Always filter out authority + .replace(/^.*@/, '[filtered]:[filtered]@') + // Don't show standard :80 (http) and :443 (https) ports to reduce the noise + .replace(':80', '') + .replace(':443', '')) || + ''; + + return `${protocol ? `${protocol}://` : ''}${filteredHost}${path}`; +} diff --git a/packages/utils/test/url.test.ts b/packages/utils/test/url.test.ts index e356662b9b29..9caad36f572b 100644 --- a/packages/utils/test/url.test.ts +++ b/packages/utils/test/url.test.ts @@ -1,4 +1,4 @@ -import { getNumberOfUrlSegments, stripUrlQueryAndFragment } from '../src/url'; +import { getNumberOfUrlSegments, getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '../src/url'; describe('stripQueryStringAndFragment', () => { const urlString = 'http://dogs.are.great:1231/yay/'; @@ -31,3 +31,51 @@ describe('getNumberOfUrlSegments', () => { expect(getNumberOfUrlSegments(input)).toEqual(output); }); }); + +describe('getSanitizedUrlString', () => { + it.each([ + ['regular url', 'https://somedomain.com', 'https://somedomain.com'], + ['regular url with a path', 'https://somedomain.com/path/to/happiness', 'https://somedomain.com/path/to/happiness'], + [ + 'url with standard http port 80', + 'http://somedomain.com:80/path/to/happiness', + 'http://somedomain.com/path/to/happiness', + ], + [ + 'url with standard https port 443', + 'https://somedomain.com:443/path/to/happiness', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with non-standard port', + 'https://somedomain.com:4200/path/to/happiness', + 'https://somedomain.com:4200/path/to/happiness', + ], + [ + 'url with query params', + 'https://somedomain.com:4200/path/to/happiness?auhtToken=abc123¶m2=bar', + 'https://somedomain.com:4200/path/to/happiness', + ], + [ + 'url with a fragment', + 'https://somedomain.com/path/to/happiness#somewildfragment123', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with a fragment and query params', + 'https://somedomain.com/path/to/happiness#somewildfragment123?auhtToken=abc123¶m2=bar', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with authorization', + 'https://username:password@somedomain.com', + 'https://[filtered]:[filtered]@somedomain.com', + ], + ['same-origin url', '/api/v4/users?id=123', '/api/v4/users'], + ['url without a protocol', 'example.com', 'example.com'], + ['url without a protocol with a path', 'example.com/sub/path?id=123', 'example.com/sub/path'], + ])('returns a sanitized URL for a %s', (_, rawUrl: string, sanitizedURL: string) => { + const urlObject = parseUrl(rawUrl); + expect(getSanitizedUrlString(urlObject)).toEqual(sanitizedURL); + }); +});