feat(pii) Sanitize URLs in Span descriptions and breadcrumbs

Author: aldenquimby

packages/node/src/integrations/http.ts

@@ -192,13 +192,28 @@ function _createWrappedRequestMethodFactory(
 
       const scope = getCurrentHub().getScope();
 
+      const spanData: Record<string, string> = {
+        url: requestUrl,
+        method: requestOptions.method || 'GET',
+      };
+      if (requestOptions.hash) {
+        // strip leading "#"
+        spanData['http.fragment'] = requestOptions.hash.substring(1);
+      }
+      if (requestOptions.search) {
+        // strip leading "?"
+        spanData['http.query'] = requestOptions.search.substring(1);
+      }
+
+      // TODO potential breaking change... shouldCreateSpanForRequest no longer has access to query + fragment, is that a problem?
       if (scope && tracingOptions && shouldCreateSpan(requestUrl)) {
         parentSpan = scope.getSpan();
 
         if (parentSpan) {
           requestSpan = parentSpan.startChild({
-            description: `${requestOptions.method || 'GET'} ${requestUrl}`,
+            description: `${spanData.method} ${spanData.url}`,
             op: 'http.client',
+            data: spanData,
           });
 
           if (shouldAttachTraceData(requestUrl)) {
@@ -253,7 +268,7 @@ function _createWrappedRequestMethodFactory(
           // eslint-disable-next-line @typescript-eslint/no-this-alias
           const req = this;
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('response', requestUrl, req, res);
+            addRequestBreadcrumb('response', spanData, req, res);
           }
           if (requestSpan) {
             if (res.statusCode) {
@@ -268,7 +283,7 @@ function _createWrappedRequestMethodFactory(
           const req = this;
 
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('error', requestUrl, req);
+            addRequestBreadcrumb('error', spanData, req);
           }
           if (requestSpan) {
             requestSpan.setHttpStatus(500);
@@ -283,7 +298,12 @@ function _createWrappedRequestMethodFactory(
 /**
  * Captures Breadcrumb based on provided request/response pair
  */
-function addRequestBreadcrumb(event: string, url: string, req: http.ClientRequest, res?: http.IncomingMessage): void {
+function addRequestBreadcrumb(
+  event: string,
+  spanData: Record<string, string>,
+  req: http.ClientRequest,
+  res?: http.IncomingMessage,
+): void {
   if (!getCurrentHub().getIntegration(Http)) {
     return;
   }
@@ -294,7 +314,7 @@ function addRequestBreadcrumb(event: string, url: string, req: http.ClientReques
       data: {
         method: req.method,
         status_code: res && res.statusCode,
-        url,
+        ...spanData,
       },
       type: 'http',
     },

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

@@ -27,7 +27,8 @@ export function extractUrl(requestOptions: RequestOptions): string {
   // Don't log standard :80 (http) and :443 (https) ports to reduce the noise
   const port =
     !requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
-  const path = requestOptions.path ? requestOptions.path : '/';
+  // do not include search or hash in span descriptions, per https://github.com/getsentry/develop/pull/773
+  const path = requestOptions.pathname || '/';
 
   return `${protocol}//${hostname}${port}${path}`;
 }

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

@@ -197,6 +197,25 @@ describe('tracing', () => {
     expect(loggerLogSpy).toBeCalledWith('HTTP Integration is skipped because of instrumenter configuration.');
   });
 
+  it('omits query and fragment from description and adds to span data instead', () => {
+    nock('http://dogs.are.great').get('/spaniel?tail=wag&cute=true#learn-more').reply(200);
+
+    const transaction = createTransactionOnScope();
+    const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[];
+
+    http.get('http://dogs.are.great/spaniel?tail=wag&cute=true#learn-more');
+
+    expect(spans.length).toEqual(2);
+
+    // our span is at index 1 because the transaction itself is at index 0
+    expect(spans[1].description).toEqual('GET http://dogs.are.great/spaniel');
+    expect(spans[1].op).toEqual('http.client');
+    expect(spans[1].data.method).toEqual('GET');
+    expect(spans[1].data.url).toEqual('http://dogs.are.great/spaniel');
+    expect(spans[1].data['http.query']).toEqual('tail=wag&cute=true');
+    expect(spans[1].data['http.fragment']).toEqual('learn-more');
+  });
+
   describe('Tracing options', () => {
     beforeEach(() => {
       // hacky way of restoring monkey patched functions