ref(node): Remove query strings from transaction and span names (#2857)

* add stripQueryString function and use it in extractUrl

* add tests for extractUrl

* strip query string from handlers urls in span names

* add method to handle url edge cases

* clarify description and variable names

* strip fragments also

* move internal route cleaning to extractUrl

* simplify url trimming function

* add tests for tracing handler

* move stripUrlPath to utils

* change name of url-stripping method

* fix missing method when cleaning description

* escape ? in regex

Co-authored-by: Kamil Ogórek <[email protected]>

Author: lobsterkatie

packages/node/src/handlers.ts

@@ -3,7 +3,7 @@
 import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core';
 import { Span } from '@sentry/tracing';
 import { Event } from '@sentry/types';
-import { forget, isPlainObject, isString, logger, normalize } from '@sentry/utils';
+import { forget, isPlainObject, isString, logger, normalize, stripUrlQueryAndFragment } from '@sentry/utils';
 import * as cookie from 'cookie';
 import * as domain from 'domain';
 import * as http from 'http';
@@ -32,7 +32,7 @@ export function tracingHandler(): (
     // TODO: At this point `req.route.path` (which we use in `extractTransaction`) is not available
     // but `req.path` or `req.url` should do the job as well. We could unify this here.
     const reqMethod = (req.method || '').toUpperCase();
-    const reqUrl = req.url;
+    const reqUrl = req.url && stripUrlQueryAndFragment(req.url);
 
     let traceId;
     let parentSpanId;

packages/node/src/integrations/http.ts

@@ -1,6 +1,6 @@
 import { getCurrentHub } from '@sentry/core';
 import { Integration, Span, Transaction } from '@sentry/types';
-import { fill, parseSemver } from '@sentry/utils';
+import { fill, parseSemver, stripUrlQueryAndFragment } from '@sentry/utils';
 import * as http from 'http';
 import * as https from 'https';
 
@@ -102,6 +102,7 @@ function createHandlerWrapper(
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(res.statusCode);
+            cleanDescription(options, this, span);
             span.finish();
           }
         })
@@ -111,6 +112,7 @@ function createHandlerWrapper(
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(500);
+            cleanDescription(options, this, span);
             span.finish();
           }
         });
@@ -145,21 +147,54 @@ function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMess
 }
 
 /**
- * Function that can combine together a url that'll be used for our breadcrumbs.
+ * Assemble a URL to be used for breadcrumbs and spans.
  *
- * @param options url that should be returned or an object containing it's parts.
- * @returns constructed url
+ * @param requestArgs URL string or object containing the component parts
+ * @returns Fully-formed URL
  */
-function extractUrl(options: string | http.ClientRequestArgs): string {
-  if (typeof options === 'string') {
-    return options;
+export function extractUrl(requestArgs: string | http.ClientRequestArgs): string {
+  if (typeof requestArgs === 'string') {
+    return stripUrlQueryAndFragment(requestArgs);
   }
-  const protocol = options.protocol || '';
-  const hostname = options.hostname || options.host || '';
+  const protocol = requestArgs.protocol || '';
+  const hostname = requestArgs.hostname || requestArgs.host || '';
   // Don't log standard :80 (http) and :443 (https) ports to reduce the noise
-  const port = !options.port || options.port === 80 || options.port === 443 ? '' : `:${options.port}`;
-  const path = options.path || '/';
-  return `${protocol}//${hostname}${port}${path}`;
+  const port = !requestArgs.port || requestArgs.port === 80 || requestArgs.port === 443 ? '' : `:${requestArgs.port}`;
+  const path = requestArgs.path ? stripUrlQueryAndFragment(requestArgs.path) : '/';
+
+  // internal routes end up with too many slashes
+  return `${protocol}//${hostname}${port}${path}`.replace('///', '/');
+}
+
+/**
+ * Handle an edge case with urls in the span description. Runs just before the span closes because it relies on
+ * data from the response object.
+ *
+ * @param requestOptions Configuration data for the request
+ * @param response Response object
+ * @param span Span representing the request
+ */
+function cleanDescription(
+  requestOptions: string | http.ClientRequestArgs,
+  response: http.IncomingMessage,
+  span: Span,
+): void {
+  // There are some libraries which don't pass the request protocol in the options object, so attempt to retrieve it
+  // from the response and run the URL processing again. We only do this in the presence of a (non-empty) host value,
+  // because if we're missing both, it's likely we're dealing with an internal route, in which case we don't want to be
+  // jamming a random `http:` on the front of it.
+  if (typeof requestOptions !== 'string' && !Object.keys(requestOptions).includes('protocol') && requestOptions.host) {
+    // Neither http.IncomingMessage nor any of its ancestors have an `agent` property in their type definitions, and
+    // http.Agent doesn't have a `protocol` property in its type definition. Nonetheless, at least one request library
+    // (superagent) arranges things that way, so might as well give it a shot.
+    try {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
+      requestOptions.protocol = (response as any).agent.protocol;
+      span.description = `${requestOptions.method || 'GET'} ${extractUrl(requestOptions)}`;
+    } catch (error) {
+      // well, we tried
+    }
+  }
 }
 
 /**

packages/node/test/handlers.test.ts

@@ -1,7 +1,14 @@
-import { Runtime } from '@sentry/types';
+import * as sentryCore from '@sentry/core';
+import { Hub } from '@sentry/hub';
+import * as sentryHub from '@sentry/hub';
+import { SpanStatus, Transaction } from '@sentry/tracing';
+import { Runtime, Transaction as TransactionType } from '@sentry/types';
+import * as http from 'http';
+import * as net from 'net';
 
 import { Event, Request, User } from '../src';
-import { parseRequest } from '../src/handlers';
+import { NodeClient } from '../src/client';
+import { parseRequest, tracingHandler } from '../src/handlers';
 
 describe('parseRequest', () => {
   let mockReq: { [key: string]: any };
@@ -164,4 +171,133 @@ describe('parseRequest', () => {
       expect(parsedRequest.transaction).toEqual('routeHandler');
     });
   });
-});
+}); // end describe('parseRequest()')
+
+describe('tracingHandler', () => {
+  const urlString = 'http://dogs.are.great:1231/yay/';
+  const queryString = '?furry=yes&funny=very';
+  const fragment = '#adoptnotbuy';
+
+  const sentryTracingMiddleware = tracingHandler();
+
+  let req: http.IncomingMessage, res: http.ServerResponse, next: () => undefined;
+
+  function createNoOpSpy() {
+    const noop = { noop: () => undefined }; // this is wrapped in an object so jest can spy on it
+    return jest.spyOn(noop, 'noop') as any;
+  }
+
+  beforeEach(() => {
+    req = new http.IncomingMessage(new net.Socket());
+    req.url = `${urlString}`;
+    req.method = 'GET';
+    res = new http.ServerResponse(req);
+    next = createNoOpSpy();
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  it('creates a transaction when handling a request', () => {
+    const startTransaction = jest.spyOn(sentryCore, 'startTransaction');
+
+    sentryTracingMiddleware(req, res, next);
+
+    expect(startTransaction).toHaveBeenCalled();
+  });
+
+  it("pulls parent's data from tracing header on the request", () => {
+    req.headers = { 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0' };
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = (res as any).__sentry_transaction;
+
+    expect(transaction.traceId).toEqual('12312012123120121231201212312012');
+    expect(transaction.parentSpanId).toEqual('1121201211212012');
+    expect(transaction.sampled).toEqual(false);
+  });
+
+  it('puts its transaction on the scope', () => {
+    const hub = new Hub(new NodeClient({ tracesSampleRate: 1.0 }));
+    // we need to mock both of these because the tracing handler relies on `@sentry/core` while the sampler relies on
+    // `@sentry/hub`, and mocking breaks the link between the two
+    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+    jest.spyOn(sentryHub, 'getCurrentHub').mockReturnValue(hub);
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = sentryCore
+      .getCurrentHub()
+      .getScope()
+      ?.getTransaction();
+
+    expect(transaction).toBeDefined();
+    expect(transaction).toEqual(expect.objectContaining({ name: `GET ${urlString}`, op: 'http.server' }));
+  });
+
+  it('puts its transaction on the response object', () => {
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = (res as any).__sentry_transaction;
+
+    expect(transaction).toBeDefined();
+    expect(transaction).toEqual(expect.objectContaining({ name: `GET ${urlString}`, op: 'http.server' }));
+  });
+
+  it('pulls status code from the response', async () => {
+    const transaction = new Transaction({ name: 'mockTransaction' });
+    jest.spyOn(sentryCore, 'startTransaction').mockReturnValue(transaction as TransactionType);
+    const finishTransaction = jest.spyOn(transaction, 'finish');
+
+    sentryTracingMiddleware(req, res, next);
+    res.statusCode = 200;
+    res.emit('finish');
+
+    expect(finishTransaction).toHaveBeenCalled();
+    expect(transaction.status).toBe(SpanStatus.Ok);
+    expect(transaction.tags).toEqual(expect.objectContaining({ 'http.status_code': '200' }));
+  });
+
+  it('strips query string from request path', () => {
+    req.url = `${urlString}${queryString}`;
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = (res as any).__sentry_transaction;
+
+    expect(transaction?.name).toBe(`GET ${urlString}`);
+  });
+
+  it('strips fragment from request path', () => {
+    req.url = `${urlString}${fragment}`;
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = (res as any).__sentry_transaction;
+
+    expect(transaction?.name).toBe(`GET ${urlString}`);
+  });
+
+  it('strips query string and fragment from request path', () => {
+    req.url = `${urlString}${queryString}${fragment}`;
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = (res as any).__sentry_transaction;
+
+    expect(transaction?.name).toBe(`GET ${urlString}`);
+  });
+
+  it('closes the transaction when request processing is done', () => {
+    const transaction = new Transaction({ name: 'mockTransaction' });
+    jest.spyOn(sentryCore, 'startTransaction').mockReturnValue(transaction as TransactionType);
+    const finishTransaction = jest.spyOn(transaction, 'finish');
+
+    sentryTracingMiddleware(req, res, next);
+    res.emit('finish');
+
+    expect(finishTransaction).toHaveBeenCalled();
+  });
+}); // end describe('tracingHandler')

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

@@ -0,0 +1,54 @@
+import { RequestOptions } from 'http';
+
+import { extractUrl } from './../../src/integrations/http';
+
+describe('extractUrl()', () => {
+  const urlString = 'http://dogs.are.great:1231/yay/';
+  const urlParts: RequestOptions = {
+    protocol: 'http:',
+    host: 'dogs.are.great',
+    method: 'GET',
+    path: '/yay/',
+    port: 1231,
+  };
+  const queryString = '?furry=yes&funny=very';
+  const fragment = '#adoptnotbuy';
+
+  it('accepts a url string', () => {
+    expect(extractUrl(urlString)).toBe(urlString);
+  });
+
+  it('accepts a http.RequestOptions object and returns a string with everything in the right place', () => {
+    expect(extractUrl(urlParts)).toBe('http://dogs.are.great:1231/yay/');
+  });
+
+  it('strips query string from url string', () => {
+    const urlWithQueryString = `${urlString}${queryString}`;
+    expect(extractUrl(urlWithQueryString)).toBe(urlString);
+  });
+
+  it('strips query string from path in http.RequestOptions object', () => {
+    const urlPartsWithQueryString = { ...urlParts, path: `${urlParts.path}${queryString}` };
+    expect(extractUrl(urlPartsWithQueryString)).toBe(urlString);
+  });
+
+  it('strips fragment from url string', () => {
+    const urlWithFragment = `${urlString}${fragment}`;
+    expect(extractUrl(urlWithFragment)).toBe(urlString);
+  });
+
+  it('strips fragment from path in http.RequestOptions object', () => {
+    const urlPartsWithFragment = { ...urlParts, path: `${urlParts.path}${fragment}` };
+    expect(extractUrl(urlPartsWithFragment)).toBe(urlString);
+  });
+
+  it('strips query string and fragment from url string', () => {
+    const urlWithQueryStringAndFragment = `${urlString}${queryString}${fragment}`;
+    expect(extractUrl(urlWithQueryStringAndFragment)).toBe(urlString);
+  });
+
+  it('strips query string and fragment from path in http.RequestOptions object', () => {
+    const urlPartsWithQueryStringAndFragment = { ...urlParts, path: `${urlParts.path}${queryString}${fragment}` };
+    expect(extractUrl(urlPartsWithQueryStringAndFragment)).toBe(urlString);
+  });
+}); // end describe('extractUrl()')

packages/utils/src/misc.ts

@@ -510,3 +510,14 @@ export function addContextToFrame(lines: string[], frame: StackFrame, linesOfCon
     .slice(Math.min(sourceLine + 1, maxLines), sourceLine + 1 + linesOfContext)
     .map((line: string) => snipLine(line, 0));
 }
+
+/**
+ * Strip the query string and fragment off of a given URL or path (if present)
+ *
+ * @param urlPath Full URL or path, including possible query string and/or fragment
+ * @returns URL or path without query string or fragment
+ */
+export function stripUrlQueryAndFragment(urlPath: string): string {
+  // eslint-disable-next-line no-useless-escape
+  return urlPath.split(/[\?#]/, 1)[0];
+}