From a44b2e12a3469849ad6e07e6aad0974874ad219d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 25 May 2023 15:21:50 +0200 Subject: [PATCH] fix(sveltekit): Avoid capturing redirects and 4xx Http errors in request Handler --- packages/sveltekit/src/common/utils.ts | 9 +++++- packages/sveltekit/src/server/handle.ts | 9 ++++++ packages/sveltekit/src/server/load.ts | 8 ++--- packages/sveltekit/test/common/utils.test.ts | 20 ++++++++++++- packages/sveltekit/test/server/handle.test.ts | 30 ++++++++++++++++++- 5 files changed, 67 insertions(+), 9 deletions(-) diff --git a/packages/sveltekit/src/common/utils.ts b/packages/sveltekit/src/common/utils.ts index ecf762cee8c4..84b384861dff 100644 --- a/packages/sveltekit/src/common/utils.ts +++ b/packages/sveltekit/src/common/utils.ts @@ -1,4 +1,4 @@ -import type { Redirect } from '@sveltejs/kit'; +import type { HttpError, Redirect } from '@sveltejs/kit'; export type SentryWrappedFlag = { /** @@ -22,3 +22,10 @@ export function isRedirect(error: unknown): error is Redirect { 'status' in error && typeof error.status === 'number' && error.status >= 300 && error.status <= 308; return hasValidLocation && hasValidStatus; } + +/** + * Determines if a thrown "error" is a HttpError + */ +export function isHttpError(err: unknown): err is HttpError { + return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; +} diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 88e0615d13bb..4fa725a15a2e 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -5,6 +5,7 @@ import { captureException } from '@sentry/node'; import { addExceptionMechanism, dynamicSamplingContextToSentryBaggageHeader, objectify } from '@sentry/utils'; import type { Handle, ResolveOptions } from '@sveltejs/kit'; +import { isHttpError, isRedirect } from '../common/utils'; import { getTracePropagationData } from './utils'; function sendErrorToSentry(e: unknown): unknown { @@ -12,6 +13,14 @@ function sendErrorToSentry(e: unknown): unknown { // store a seen flag on it. const objectifiedErr = objectify(e); + // similarly to the `load` function, we don't want to capture 4xx errors or redirects + if ( + isRedirect(objectifiedErr) || + (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) + ) { + return objectifiedErr; + } + captureException(objectifiedErr, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index ab9f9ebdafb3..c776dc639d3a 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -3,19 +3,15 @@ import { trace } from '@sentry/core'; import { captureException } from '@sentry/node'; import type { TransactionContext } from '@sentry/types'; import { addExceptionMechanism, addNonEnumerableProperty, objectify } from '@sentry/utils'; -import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; +import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; -import { isRedirect } from '../common/utils'; +import { isHttpError, isRedirect } from '../common/utils'; import { getTracePropagationData } from './utils'; type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; -function isHttpError(err: unknown): err is HttpError { - return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; -} - function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. diff --git a/packages/sveltekit/test/common/utils.test.ts b/packages/sveltekit/test/common/utils.test.ts index 51af645cc961..5581fe60c5e4 100644 --- a/packages/sveltekit/test/common/utils.test.ts +++ b/packages/sveltekit/test/common/utils.test.ts @@ -1,4 +1,6 @@ -import { isRedirect } from '../../src/common/utils'; +import { redirect } from '@sveltejs/kit'; + +import { isHttpError, isRedirect } from '../../src/common/utils'; describe('isRedirect', () => { it.each([ @@ -23,3 +25,19 @@ describe('isRedirect', () => { expect(isRedirect(redirectObject)).toBe(false); }); }); + +describe('isHttpError', () => { + it.each([ + { status: 404, body: 'Not found' }, + { status: 500, body: 'Internal server error' }, + ])('returns `true` for valid HttpError objects (%s)', httpErrorObject => { + expect(isHttpError(httpErrorObject)).toBe(true); + }); + + it.each([new Error(), redirect(301, '/users/id'), 'string error', { status: 404 }, { body: 'Not found' }])( + 'returns `false` for other thrown objects (%s)', + httpErrorObject => { + expect(isHttpError(httpErrorObject)).toBe(false); + }, + ); +}); diff --git a/packages/sveltekit/test/server/handle.test.ts b/packages/sveltekit/test/server/handle.test.ts index 94d471f20689..a1c6a5a3d0d8 100644 --- a/packages/sveltekit/test/server/handle.test.ts +++ b/packages/sveltekit/test/server/handle.test.ts @@ -2,6 +2,7 @@ import { addTracingExtensions, Hub, makeMain, Scope } from '@sentry/core'; import { NodeClient } from '@sentry/node'; import type { Transaction } from '@sentry/types'; import type { Handle } from '@sveltejs/kit'; +import { redirect } from '@sveltejs/kit'; import { vi } from 'vitest'; import { sentryHandle, transformPageChunk } from '../../src/server/handle'; @@ -69,7 +70,18 @@ const enum Type { Async = 'async', } -function resolve(type: Type, isError: boolean): Parameters[0]['resolve'] { +function resolve( + type: Type, + isError: boolean, + throwSpecialError?: 'redirect' | 'http', +): Parameters[0]['resolve'] { + if (throwSpecialError === 'redirect') { + throw redirect(302, '/redirect'); + } + if (throwSpecialError === 'http') { + throw { status: 404, body: 'Not found' }; + } + if (type === Type.Sync) { return (..._args: unknown[]) => { if (isError) { @@ -292,6 +304,22 @@ describe('handleSentry', () => { } }); + it("doesn't send redirects in a request handler to Sentry", async () => { + try { + await sentryHandle()({ event: mockEvent(), resolve: resolve(type, false, 'redirect') }); + } catch (e) { + expect(mockCaptureException).toBeCalledTimes(0); + } + }); + + it("doesn't send Http 4xx errors in a request handler to Sentry", async () => { + try { + await sentryHandle()({ event: mockEvent(), resolve: resolve(type, false, 'http') }); + } catch (e) { + expect(mockCaptureException).toBeCalledTimes(0); + } + }); + it('calls `transformPageChunk`', async () => { const mockResolve = vi.fn().mockImplementation(resolve(type, isError)); const event = mockEvent();