From ee4e37ef4dc2549daa8accfcd4a4a00221f8f4ea Mon Sep 17 00:00:00 2001
From: Lukas Stracke <lukas.stracke@sentry.io>
Date: Thu, 22 Jun 2023 13:30:48 +0200
Subject: [PATCH] fix(sveltekit): Only instrument SvelteKit `fetch` if the SDK
 client is valid (#8381)

In the client-side SvelteKit fetch instrumentation, our previous type
cast when retrieving the SDK client was wrong, causing us to not guard
the fetch instrumentation correctly if the client was undefined. This fix
adds an undefined check.

fixes #8290
---
 packages/sveltekit/src/client/load.ts       | 25 +++++++++++----
 packages/sveltekit/test/client/load.test.ts | 34 ++++++++++++++++++---
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts
index 1996523346e2..bbc184b6d3a0 100644
--- a/packages/sveltekit/src/client/load.ts
+++ b/packages/sveltekit/src/client/load.ts
@@ -3,7 +3,7 @@ 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, SanitizedRequestData } from '@sentry/types';
+import type { Client, ClientOptions, SanitizedRequestData } from '@sentry/types';
 import {
   addExceptionMechanism,
   addNonEnumerableProperty,
@@ -122,12 +122,14 @@ type SvelteKitFetch = LoadEvent['fetch'];
  * @returns a proxy of SvelteKit's fetch implementation
  */
 function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch {
-  const client = getCurrentHub().getClient() as BaseClient<ClientOptions>;
+  const client = getCurrentHub().getClient();
 
-  const browserTracingIntegration =
-    client.getIntegrationById && (client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined);
-  const breadcrumbsIntegration =
-    client.getIntegrationById && (client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined);
+  if (!isValidClient(client)) {
+    return originalFetch;
+  }
+
+  const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined;
+  const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined;
 
   const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options;
 
@@ -270,3 +272,14 @@ function addFetchBreadcrumb(
     },
   );
 }
+
+type MaybeClientWithGetIntegrationsById =
+  | (Client & { getIntegrationById?: BaseClient<ClientOptions>['getIntegrationById'] })
+  | undefined;
+
+type ClientWithGetIntegrationById = Required<MaybeClientWithGetIntegrationsById> &
+  Exclude<MaybeClientWithGetIntegrationsById, undefined>;
+
+function isValidClient(client: MaybeClientWithGetIntegrationsById): client is ClientWithGetIntegrationById {
+  return !!client && typeof client.getIntegrationById === 'function';
+}
diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts
index b07e0c12108f..65b4cc1da3b1 100644
--- a/packages/sveltekit/test/client/load.test.ts
+++ b/packages/sveltekit/test/client/load.test.ts
@@ -52,6 +52,12 @@ const mockedGetIntegrationById = vi.fn(id => {
   return undefined;
 });
 
+const mockedGetClient = vi.fn(() => {
+  return {
+    getIntegrationById: mockedGetIntegrationById,
+  };
+});
+
 vi.mock('@sentry/core', async () => {
   const original = (await vi.importActual('@sentry/core')) as any;
   return {
@@ -62,11 +68,7 @@ vi.mock('@sentry/core', async () => {
     },
     getCurrentHub: () => {
       return {
-        getClient: () => {
-          return {
-            getIntegrationById: mockedGetIntegrationById,
-          };
-        },
+        getClient: mockedGetClient,
         getScope: () => {
           return {
             getSpan: () => {
@@ -427,6 +429,28 @@ describe('wrapLoadWithSentry', () => {
     });
   });
 
+  it.each([
+    ['is undefined', undefined],
+    ["doesn't have a `getClientById` method", {}],
+  ])("doesn't instrument fetch if the client %s", async (_, client) => {
+    // @ts-expect-error: we're mocking the client
+    mockedGetClient.mockImplementationOnce(() => client);
+
+    async function load(_event: Parameters<Load>[0]): Promise<ReturnType<Load>> {
+      return {
+        msg: 'hi',
+      };
+    }
+    const wrappedLoad = wrapLoadWithSentry(load);
+
+    const originalFetch = MOCK_LOAD_ARGS.fetch;
+    await wrappedLoad(MOCK_LOAD_ARGS);
+
+    expect(MOCK_LOAD_ARGS.fetch).toStrictEqual(originalFetch);
+
+    expect(mockTrace).toHaveBeenCalledTimes(1);
+  });
+
   it('adds an exception mechanism', async () => {
     const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => {
       void callback({}, { event_id: 'fake-event-id' });