properly sanitize url, add url data

Author: Lms24

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, RequestSpanData, Span, TracePropagationTargets } from '@sentry/types';
 import {
   dynamicSamplingContextToSentryBaggageHeader,
   fill,
@@ -129,16 +129,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!)

packages/sveltekit/src/client/load.ts

@@ -2,15 +2,15 @@ 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 type { ClientOptions } from '@sentry/types';
+import type { ClientOptions, RequestSpanData } from '@sentry/types';
 import {
   addExceptionMechanism,
   addTracingHeadersToFetchRequest,
   getFetchMethod,
   getFetchUrl,
+  getSanitizedUrlString,
   objectify,
   stringMatchesSomePattern,
-  stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import type { LoadEvent } from '@sveltejs/kit';
 
@@ -133,23 +133,25 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
     apply: (wrappingTarget, thisArg, args: Parameters<LoadEvent['fetch']>) => {
       const [input, init] = args;
       const rawUrl = getFetchUrl(args);
-      const sanitizedUrl = stripUrlQueryAndFragment(rawUrl);
+      const urlObject = new URL(rawUrl);
+      const sanitizedUrl = getSanitizedUrlString(urlObject);
       const method = getFetchMethod(args);
 
       // TODO: extract this to a util function (and use it in breadcrumbs integration as well)
       if (rawUrl.match(/sentry_key/) && method === 'POST') {
-        // We will not create breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests)
+        // We don't create spans or breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests)
         return wrappingTarget.apply(thisArg, args);
       }
 
       const patchedInit: RequestInit = { ...init } || {};
       const activeSpan = getCurrentHub().getScope().getSpan();
       const activeTransaction = activeSpan && activeSpan.transaction;
 
-      const attachHeaders = shouldAttachHeaders(rawUrl);
-      const attachSpan = shouldCreateSpan(rawUrl);
+      const attachHeaders = activeTransaction && shouldAttachHeaders(rawUrl);
+      const createSpan = activeTransaction && shouldCreateSpan(rawUrl);
 
-      if (attachHeaders && attachSpan && activeTransaction) {
+      // only attach headers if we should create a span
+      if (attachHeaders && createSpan) {
         const dsc = activeTransaction.getDynamicSamplingContext();
         const headers = addTracingHeadersToFetchRequest(
           input as string | Request,
@@ -168,13 +170,20 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
 
       let fetchPromise: Promise<Response>;
 
-      if (attachSpan) {
+      if (createSpan) {
+        const spanData: RequestSpanData = {
+          url: sanitizedUrl,
+          method,
+          'http.query': urlObject.search,
+          'http.fragment': urlObject.hash,
+        };
+
         fetchPromise = trace(
           {
             name: `${method} ${sanitizedUrl}`, // this will become the description of the span
             op: 'http.client',
             data: {
-              /* TODO: extract query data (we might actually only do this once we tackle sanitization on the browser-side) */
+              ...spanData,
             },
             parentSpanId: activeSpan && activeSpan.spanId,
           },

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, RequestSpanData } from './request';
 export type { Runtime } from './runtime';
 export type { CaptureContext, Scope, ScopeContext } from './scope';
 export type { SdkInfo } from './sdkinfo';

packages/types/src/request.ts

@@ -10,3 +10,14 @@ export interface Request {
 }
 
 export type QueryParams = string | { [key: string]: string } | Array<[string, string]>;
+
+/**
+ * span.data type for http.client spans
+ * See https://develop.sentry.dev/sdk/data-handling/#structuring-data
+ */
+export type RequestSpanData = {
+  url: string;
+  method: string;
+  'http.fragment'?: string;
+  'http.query'?: string;
+};

packages/utils/src/url.ts

@@ -50,3 +50,18 @@ 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: URL): string {
+  const { protocol, hostname, port: originalPort, pathname, username, password } = url;
+
+  // Don't show standard :80 (http) and :443 (https) ports to reduce the noise
+  const port = !originalPort || originalPort === '80' || originalPort === '443' ? '' : `:${originalPort}`;
+  // Always filter out authority
+  const authority = !username && !password ? '' : '[filtered]:[filtered]@';
+
+  return `${protocol}//${authority}${hostname}${port}${pathname}`;
+}

packages/utils/test/url.test.ts

@@ -1,4 +1,4 @@
-import { getNumberOfUrlSegments, stripUrlQueryAndFragment } from '../src/url';
+import { getNumberOfUrlSegments, getSanitizedUrlString, stripUrlQueryAndFragment } from '../src/url';
 
 describe('stripQueryStringAndFragment', () => {
   const urlString = 'http://dogs.are.great:1231/yay/';
@@ -31,3 +31,48 @@ 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&param2=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&param2=bar',
+      'https://somedomain.com/path/to/happiness',
+    ],
+    [
+      'url with authorization',
+      'https://username:[email protected]',
+      'https://[filtered]:[filtered]@somedomain.com',
+    ],
+  ])('returns a sanitized URL for a %s', (_, rawUrl: string, sanitizedURL: string) => {
+    const urlObject = new URL(rawUrl);
+    expect(getSanitizedUrlString(urlObject)).toEqual(sanitizedURL);
+  });
+});