Browse Source

feat(utils): add safeURL (#62839)

Simplify control flow for safe URL initialization by providing a safe
URL initializer utility function.
Jonas 1 year ago
parent
commit
45e8866169

+ 5 - 4
static/app/components/events/interfaces/crashContent/exception/utils.tsx

@@ -7,14 +7,15 @@ import {IconOpen} from 'sentry/icons';
 import type {Frame} from 'sentry/types';
 import {isUrl} from 'sentry/utils';
 import {getFileExtension} from 'sentry/utils/fileExtension';
+import {safeURL} from 'sentry/utils/url/safeURL';
 
 const fileNameBlocklist = ['@webkit-masked-url'];
 export function isFrameFilenamePathlike(frame: Frame): boolean {
   let filename = frame.absPath ?? '';
-  try {
-    filename = new URL(filename).pathname.split('/').reverse()[0];
-  } catch {
-    // do nothing
+
+  const parsedURL = safeURL(filename);
+  if (parsedURL) {
+    filename = parsedURL.pathname.split('/').reverse()[0];
   }
 
   return (

+ 10 - 16
static/app/components/events/interfaces/performance/spanEvidenceKeyValueList.tsx

@@ -28,6 +28,7 @@ import {formatBytesBase2} from 'sentry/utils';
 import {generateEventSlug} from 'sentry/utils/discover/urls';
 import toRoundedPercent from 'sentry/utils/number/toRoundedPercent';
 import {getTransactionDetailsUrl} from 'sentry/utils/performance/urls';
+import {safeURL} from 'sentry/utils/url/safeURL';
 import useOrganization from 'sentry/utils/useOrganization';
 import {transactionSummaryRouteWithQuery} from 'sentry/views/performance/transactionSummary/utils';
 import {getPerformanceDuration} from 'sentry/views/performance/utils';
@@ -655,29 +656,22 @@ export const extractSpanURLString = (span: Span, baseURL?: string): URL | null =
 
   URLString = span?.data?.url;
   if (URLString) {
-    try {
-      let url = span?.data?.url ?? '';
-      const query = span?.data?.['http.query'];
-      if (query) {
-        url += `?${query}`;
-      }
+    let url = span?.data?.url ?? '';
+    const query = span?.data?.['http.query'];
+    if (query) {
+      url += `?${query}`;
+    }
 
-      return new URL(url, baseURL);
-    } catch (e) {
-      // Ignore error
+    const parsedURL = safeURL(url, baseURL);
+    if (parsedURL) {
+      return parsedURL;
     }
   }
 
   const [_method, _url] = (span?.description ?? '').split(' ', 2);
   URLString = _url;
 
-  try {
-    return new URL(_url, baseURL);
-  } catch (e) {
-    // Ignore error
-  }
-
-  return null;
+  return safeURL(_url, baseURL) ?? null;
 };
 
 export function extractQueryParameters(URLs: URL[]): ParameterLookup {

+ 3 - 4
static/app/utils/queryString.tsx

@@ -1,6 +1,7 @@
 import * as qs from 'query-string';
 
 import {escapeDoubleQuotes} from 'sentry/utils';
+import {safeURL} from 'sentry/utils/url/safeURL';
 
 // remove leading and trailing whitespace and remove double spaces
 export function formatQueryString(query: string): string {
@@ -11,11 +12,9 @@ export function addQueryParamsToExistingUrl(
   origUrl: string,
   queryParams: object
 ): string {
-  let url;
+  const url = safeURL(origUrl);
 
-  try {
-    url = new URL(origUrl);
-  } catch {
+  if (!url) {
     return '';
   }
 

+ 5 - 5
static/app/utils/replays/getCurrentUrl.tsx

@@ -4,8 +4,8 @@ import type {
   SpanFrame,
 } from 'sentry/utils/replays/types';
 import {isSpanFrame} from 'sentry/utils/replays/types';
-import parseUrl from 'sentry/utils/url/parseUrl';
-import stripOrigin from 'sentry/utils/url/stripOrigin';
+import {safeURL} from 'sentry/utils/url/safeURL';
+import stripURLOrigin from 'sentry/utils/url/stripURLOrigin';
 import type {ReplayRecord} from 'sentry/views/replays/types';
 
 function getCurrentUrl(
@@ -23,10 +23,10 @@ function getCurrentUrl(
   }
 
   const initialUrl = replayRecord?.urls[0] ?? '';
-  const origin = initialUrl ? parseUrl(initialUrl)?.origin || initialUrl : '';
+  const origin = initialUrl ? safeURL(initialUrl)?.origin || initialUrl : '';
 
   if ('category' in mostRecentFrame && mostRecentFrame.category === 'replay.init') {
-    return origin + stripOrigin(mostRecentFrame.message ?? '');
+    return origin + stripURLOrigin(mostRecentFrame.message ?? '');
   }
 
   if (
@@ -40,7 +40,7 @@ function getCurrentUrl(
   ) {
     // navigation.push will have the pathname while the other `navigate.*`
     // operations will have a full url.
-    return origin + stripOrigin((mostRecentFrame as NavigationFrame).description);
+    return origin + stripURLOrigin((mostRecentFrame as NavigationFrame).description);
   }
 
   throw new Error('Unknown frame type in getCurrentUrl');

+ 7 - 7
static/app/utils/replays/getFrameDetails.tsx

@@ -35,7 +35,7 @@ import {
   isRageClick,
 } from 'sentry/utils/replays/types';
 import type {Color} from 'sentry/utils/theme';
-import stripOrigin from 'sentry/utils/url/stripOrigin';
+import stripURLOrigin from 'sentry/utils/url/stripURLOrigin';
 
 interface Details {
   color: Color;
@@ -48,14 +48,14 @@ interface Details {
 const MAPPER_FOR_FRAME: Record<string, (frame) => Details> = {
   'replay.init': (frame: BreadcrumbFrame) => ({
     color: 'gray300',
-    description: stripOrigin(frame.message ?? ''),
+    description: stripURLOrigin(frame.message ?? ''),
     tabKey: TabKey.CONSOLE,
     title: 'Replay Start',
     icon: <IconTerminal size="xs" />,
   }),
   navigation: (frame: NavFrame) => ({
     color: 'green300',
-    description: stripOrigin((frame as NavFrame).data.to),
+    description: stripURLOrigin((frame as NavFrame).data.to),
     tabKey: TabKey.NETWORK,
     title: 'Navigation',
     icon: <IconLocation size="xs" />,
@@ -209,28 +209,28 @@ const MAPPER_FOR_FRAME: Record<string, (frame) => Details> = {
   }),
   'navigation.navigate': frame => ({
     color: 'green300',
-    description: stripOrigin(frame.description),
+    description: stripURLOrigin(frame.description),
     tabKey: TabKey.NETWORK,
     title: 'Page Load',
     icon: <IconLocation size="xs" />,
   }),
   'navigation.reload': frame => ({
     color: 'green300',
-    description: stripOrigin(frame.description),
+    description: stripURLOrigin(frame.description),
     tabKey: TabKey.NETWORK,
     title: 'Reload',
     icon: <IconLocation size="xs" />,
   }),
   'navigation.back_forward': frame => ({
     color: 'green300',
-    description: stripOrigin(frame.description),
+    description: stripURLOrigin(frame.description),
     tabKey: TabKey.NETWORK,
     title: 'Navigate Back/Forward',
     icon: <IconLocation size="xs" />,
   }),
   'navigation.push': frame => ({
     color: 'green300',
-    description: stripOrigin(frame.description),
+    description: stripURLOrigin(frame.description),
     tabKey: TabKey.NETWORK,
     title: 'Navigation',
     icon: <IconLocation size="xs" />,

+ 0 - 11
static/app/utils/url/parseUrl.spec.tsx

@@ -1,11 +0,0 @@
-import parseUrl from 'sentry/utils/url/parseUrl';
-
-describe('parseUrl', () => {
-  it('should return a URL object when a valid string is passed', () => {
-    expect(parseUrl('https://example.com')).toStrictEqual(expect.any(URL));
-  });
-
-  it('should return undefined, not throw, when an invalid string is passed', () => {
-    expect(parseUrl('foo')).toBeUndefined();
-  });
-});

+ 0 - 7
static/app/utils/url/parseUrl.tsx

@@ -1,7 +0,0 @@
-export default function parseUrl(url: string) {
-  try {
-    return new URL(url);
-  } catch {
-    return undefined;
-  }
-}

+ 43 - 0
static/app/utils/url/safeURL.spec.tsx

@@ -0,0 +1,43 @@
+import {safeURL} from './safeURL';
+
+describe('safeURL', () => {
+  const nativeConstructor = globalThis.URL;
+  afterEach(() => {
+    jest.resetAllMocks();
+    globalThis.URL = nativeConstructor;
+  });
+
+  describe(`invalid argument types`, () => {
+    it('does not throw on unknown input', () => {
+      expect(() => safeURL('/')).not.toThrow();
+      expect(() => safeURL('/', 'bad_base')).not.toThrow();
+      // @ts-expect-error force invalid input
+      expect(() => safeURL('/', null)).not.toThrow();
+      // @ts-expect-error force invalid input
+      expect(() => safeURL(undefined)).not.toThrow();
+      // @ts-expect-error force invalid input
+      expect(() => safeURL(undefined, undefined)).not.toThrow();
+      // @ts-expect-error force invalid input
+      expect(() => safeURL(null)).not.toThrow();
+      // @ts-expect-error force invalid input
+      expect(() => safeURL([])).not.toThrow();
+    });
+  });
+
+  describe(`valid argument values`, () => {
+    it('returns a new URL object', () => {
+      expect(safeURL('https://example.com')).toBeInstanceOf(URL);
+      expect(safeURL('/path', 'http://example.com')).toBeInstanceOf(URL);
+    });
+
+    it('returns correct URL', () => {
+      expect(safeURL('https://example.com')?.href).toBe('https://example.com/');
+    });
+
+    it('respects base argv', () => {
+      expect(safeURL('/path', 'https://example.com')?.href).toBe(
+        'https://example.com/path'
+      );
+    });
+  });
+});

+ 11 - 0
static/app/utils/url/safeURL.tsx

@@ -0,0 +1,11 @@
+/**
+ * Does not throw error on invalid input and returns the parsed URL object
+ * if the input is a valid URL, otherwise returns undefined.
+ */
+export function safeURL(input: string | URL, base?: string | undefined): URL | undefined {
+  try {
+    return new globalThis.URL(input, base);
+  } catch {
+    return undefined;
+  }
+}

+ 0 - 10
static/app/utils/url/stripOrigin.tsx

@@ -1,10 +0,0 @@
-import parseUrl from 'sentry/utils/url/parseUrl';
-
-/**
- * Accept a url like:
- * `https://example.com/path/name?query=params#hash` and return
- * `/path/name?query=params#hash`
- */
-export default function stripOrigin(url: string) {
-  return url.replace(parseUrl(url)?.origin ?? '', '');
-}

Some files were not shown because too many files changed in this diff