8000 feat(node): Sanitize URLs in Span descriptions and breadcrumbs (#7667) · TrySound/sentry-javascript@9ecd152 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9ecd152

Browse files
Lms24aldenquimbycleptric
authored
feat(node): Sanitize URLs in Span descriptions and breadcrumbs (getsentry#7667)
Co-authored-by 8000 : Alden Quimby <aldenquimby@gmail.com> Co-authored-by: Alden Quimby <alden@freewill.com> Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>
1 parent 65c44ec commit 9ecd152

File tree

3 files changed

+98
-11
lines changed

3 files changed

+98
-11
lines changed

‎packages/node/src/integrations/http.ts

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { LRUMap } from 'lru_map';
1414

1515
import type { NodeClient } from '../client';
1616
import type { RequestMethod, RequestMethodArgs } from './utils/http';
17-
import { cleanSpanDescription, extractUrl, isSentryRequest, normalizeRequestArgs } from './utils/http';
17+
import { cleanSpanDescription, extractRawUrl, extractUrl, isSentryRequest, normalizeRequestArgs } from './utils/http';
1818

1919
const NODE_VERSION = parseSemver(process.versions.node);
2020

@@ -129,6 +129,16 @@ type OriginalRequestMethod = RequestMethod;
129129
type WrappedRequestMethod = RequestMethod;
130130
type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedRequestMethod;
131131

132+
/**
133+
* See https://develop.sentry.dev/sdk/data-handling/#structuring-data
134+
*/
135+
type RequestSpanData = {
136+
url: string;
137+
method: string;
138+
'http.fragment'?: string;
139+
'http.query'?: string;
140+
};
141+
132142
/**
133143
* Function which creates a function which creates wrapped versions of internal `request` and `get` calls within `http`
134144
* and `https` modules. (NB: Not a typo - this is a creator^2!)
@@ -180,6 +190,8 @@ function _createWrappedRequestMethodFactory(
180190
return function wrappedMethod(this: unknown, ...args: RequestMethodArgs): http.ClientRequest {
181191
const requestArgs = normalizeRequestArgs(httpModule, args);
182192
const requestOptions = requestArgs[0];
193+
// eslint-disable-next-line deprecation/deprecation
194+
const rawRequestUrl = extractRawUrl(requestOptions);
183195
const requestUrl = extractUrl(requestOptions);
184196

185197
// we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original method
@@ -192,16 +204,30 @@ function _createWrappedRequestMethodFactory(
192204

193205
const scope = getCurrentHub().getScope();
194206

195-
if (scope && tracingOptions && shouldCreateSpan(requestUrl)) {
207+
const requestSpanData: RequestSpanData = {
208+
url: requestUrl,
209+
method: requestOptions.method || 'GET',
210+
};
211+
if (requestOptions.hash) {
212+
// strip leading "#"
213+
requestSpanData['http.fragment'] = requestOptions.hash.substring(1);
214+
}
215+
if (requestOptions.search) {
216+
// strip leading "?"
217+
requestSpanData['http.query'] = requestOptions.search.substring(1);
218+
}
219+
220+
if (scope && tracingOptions && shouldCreateSpan(rawRequestUrl)) {
196221
parentSpan = scope.getSpan();
197222

198223
if (parentSpan) {
199224
requestSpan = parentSpan.startChild({
200-
description: `${requestOptions.method || 'GET'} ${requestUrl}`,
225+
description: `${requestSpanData.method} ${requestSpanData.url}`,
201226
op: 'http.client',
227+
data: requestSpanData,
202228
});
203229

204-
if (shouldAttachTraceData(requestUrl)) {
230+
if (shouldAttachTraceData(rawRequestUrl)) {
205231
const sentryTraceHeader = requestSpan.toTraceparent();
206232
__DEBUG_BUILD__ &&
207233
logger.log(
@@ -253,7 +279,7 @@ function _createWrappedRequestMethodFactory(
253279
// eslint-disable-next-line @typescript-eslint/no-this-alias
254280
const req = this;
255281
if (breadcrumbsEnabled) {
256-
addRequestBreadcrumb('response', requestUrl, req, res);
282+
addRequestBreadcrumb('response', requestSpanData, req, res);
257283
}
258284
if (requestSpan) {
259285
if (res.statusCode) {
@@ -268,7 +294,7 @@ function _createWrappedRequestMethodFactory(
268294
const req = this;
269295

270296
if (breadcrumbsEnabled) {
271-
addRequestBreadcrumb('error', requestUrl, req);
297+
addRequestBreadcrumb('error', requestSpanData, req);
272298
}
273299
if (requestSpan) {
274300
requestSpan.setHttpStatus(500);
@@ -283,7 +309,12 @@ function _createWrappedRequestMethodFactory(
283309
/**
284310
* Captures Breadcrumb based on provided request/response pair
285311
*/
286-
function addRequestBreadcrumb(event: string, url: string, req: http.ClientRequest, res?: http.IncomingMessage): void {
312+
function addRequestBreadcrumb(
313+
event: string,
314+
requestSpanData: RequestSpanData,
315+
req: http.ClientRequest,
316+
res?: http.IncomingMessage,
317+
): void {
287318
if (!getCurrentHub().getIntegration(Http)) {
288319
return;
289320
}
@@ -294,7 +325,7 @@ function addRequestBreadcrumb(event: string, url: string, req: http.ClientReques
294325
data: {
295326
method: req.method,
296327
status_code: res && res.statusCode,
297-
url,
328+
...requestSpanData,
298329
},
299330
type: 'http',
300331
},

‎packages/node/src/integrations/utils/http.ts

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,25 @@ export function isSentryRequest(url: string): boolean {
1515
return dsn ? url.includes(dsn.host) : false;
1616
}
1717

18+
/**
19+
* Assembles a URL that's passed to the users to filter on.
20+
* It can include raw (potentially PII containing) data, which we'll allow users to access to filter
21+
* but won't include in spans or breadcrumbs.
22+
*
23+
* @param requestOptions RequestOptions object containing the component parts for a URL
24+
* @returns Fully-formed URL
25+
*/
26+
// TODO (v8): This function should include auth, query and fragment (it's breaking, so we need to wait for v8)
27+
export function extractRawUrl(requestOptions: RequestOptions): string {
28+
const protocol = requestOptions.protocol || '';
29+
const hostname = requestOptions.hostname || requestOptions.host || '';
30+
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
31+
const port =
32+
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
33+
const path = requestOptions.path ? requestOptions.path : '/';
34+
return `${protocol}//${hostname}${port}${path}`;
35+
}
36+
1837
/**
1938
* Assemble a URL to be used for breadcrumbs and spans.
2039
*
@@ -27,9 +46,11 @@ export function extractUrl(requestOptions: RequestOptions): string {
2746
// Don't log standard :80 (http) and :443 (https) ports to reduce the noise
2847
const port =
2948
!requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
30-
const path = requestOptions.path ? requestOptions.path : '/';
49+
// do not include search or hash in span descriptions, per https://develop.sentry.dev/sdk/data-handling/#structuring-data
50+
const path = requestOptions.pathname || '/';
51+
const authority = requestOptions.auth ? `${requestOptions.auth}@` : '';
3152

32-
return `${protocol}//${hostname}${port}${path}`;
53+
return `${protocol}//${authority}${hostname}${port}${path}`;
3354
}
3455

3556
/**
@@ -59,6 +80,7 @@ export function cleanSpanDescription(
5980
if (requestOptions.host && !requestOptions.protocol) {
6081
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
6182
requestOptions.protocol = (request as any)?.agent?.protocol; // worst comes to worst, this is undefined and nothing changes
83+
// This URL contains the filtered authority ([filtered]:[filtered]@example.com) but no fragment or query params
6284
requestUrl = extractUrl(requestOptions);
6385
}
6486

@@ -101,7 +123,8 @@ export function urlToOptions(url: URL): RequestOptions {
101123
options.port = Number(url.port);
102124
}
103125
if (url.username || url.password) {
104-
options.auth = `${url.username}:${url.password}`;
126+
// always filter authority, see https://develop.sentry.dev/sdk/data-handling/#structuring-data
127+
options.auth = '[Filtered]:[Filtered]';
105128
}
106129
return options;
107130
}

‎packages/node/test/integrations/http.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,39 @@ describe('tracing', () => {
196196
expect(loggerLogSpy).toBeCalledWith('HTTP Integration is skipped because of instrumenter configuration.');
197197
});
198198

199+
it('omits query and fragment from description and adds to span data instead', () => {
200+
nock('http://dogs.are.great').get('/spaniel?tail=wag&cute=true#learn-more').reply(200);
201+
202+
const transaction = createTransactionOnScope();
203+
const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[];
204+
205+
http.get('http://dogs.are.great/spaniel?tail=wag&cute=true#learn-more');
206+
207+
expect(spans.length).toEqual(2);
208+
209+
// our span is at index 1 because the transaction itself is at index 0
210+
expect(spans[1].description).toEqual('GET http://dogs.are.great/spaniel');
211+
expect(spans[1].op).toEqual('http.client');
212+
expect(spans[1].data.method).toEqual('GET');
213+
expect(spans[1].data.url).toEqual('http://dogs.are.great/spaniel');
214+
expect(spans[1].data['http.query']).toEqual('tail=wag&cute=true');
215+
expect(spans[1].data['http.fragment']).toEqual('learn-more');
216+
});
217+
218+
it('filters the authority (username and password) in span description', () => {
219+
nock('http://username:password@dogs.are.great').get('/').reply(200);
220+
221+
const transaction = createTransactionOnScope();
222+
const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[];
223+
224+
http.get('http://username:password@dogs.are.great/');
225+
226+
expect(spans.length).toEqual(2);
227+
228+
// our span is at index 1 because the transaction itself is at index 0
229+
expect(spans[1].description).toEqual('GET http://[Filtered]:[Filtered]@dogs.are.great/');
230+
});
231+
199232
describe('Tracing options', () => {
200233
beforeEach(() => {
201234
// hacky way of restoring monkey patched functions

0 commit comments

Comments
 (0)
0