Skip to content

Commit 52b764e

Browse files
authored
fix(sveltekit): Don't capture thrown Redirects as exceptions (#7731)
As outlined in the [SvelteKit docs](https://kit.svelte.dev/docs/load#redirects), users can `throw redirect(300, 'route/to/redirect')` in `load` functions. We don't want to capture these as errors and send them to Sentry.
1 parent 95e51a2 commit 52b764e

File tree

6 files changed

+81
-2
lines changed

6 files changed

+81
-2
lines changed

packages/sveltekit/src/client/load.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,18 @@ import { captureException } from '@sentry/svelte';
33
import { addExceptionMechanism, objectify } from '@sentry/utils';
44
import type { LoadEvent } from '@sveltejs/kit';
55

6+
import { isRedirect } from '../common/utils';
7+
68
function sendErrorToSentry(e: unknown): unknown {
79
// In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can
810
// store a seen flag on it.
911
const objectifiedErr = objectify(e);
1012

13+
// We don't want to capture thrown `Redirect`s as these are not errors but expected behaviour
14+
if (isRedirect(objectifiedErr)) {
15+
return objectifiedErr;
16+
}
17+
1118
captureException(objectifiedErr, scope => {
1219
scope.addEventProcessor(event => {
1320
addExceptionMechanism(event, {
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import type { Redirect } from '@sveltejs/kit';
2+
3+
/**
4+
* Determines if a thrown "error" is a Redirect object which SvelteKit users can throw to redirect to another route
5+
* see: https://kit.svelte.dev/docs/modules#sveltejs-kit-redirect
6+
* @param error the potential redirect error
7+
*/
8+
export function isRedirect(error: unknown): error is Redirect {
9+
if (error == null || typeof error !== 'object') {
10+
return false;
11+
}
12+
const hasValidLocation = 'location' in error && typeof error.location === 'string';
13+
const hasValidStatus =
14+
'status' in error && typeof error.status === 'number' && error.status >= 300 && error.status <= 308;
15+
return hasValidLocation && hasValidStatus;
16+
}

packages/sveltekit/src/server/load.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { TransactionContext } from '@sentry/types';
55
import { addExceptionMechanism, objectify } from '@sentry/utils';
66
import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit';
77

8+
import { isRedirect } from '../common/utils';
89
import { getTracePropagationData } from './utils';
910

1011
function isHttpError(err: unknown): err is HttpError {
@@ -19,7 +20,12 @@ function sendErrorToSentry(e: unknown): unknown {
1920
// The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error
2021
// If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they
2122
// could be noisy.
22-
if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) {
23+
// Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown
24+
// `Redirect`s as they're not errors but expected behaviour
25+
if (
26+
isRedirect(objectifiedErr) ||
27+
(isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400)
28+
) {
2329
return objectifiedErr;
2430
}
2531

packages/sveltekit/test/client/load.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { addTracingExtensions, Scope } from '@sentry/svelte';
22
import type { Load } from '@sveltejs/kit';
3+
import { redirect } from '@sveltejs/kit';
34
import { vi } from 'vitest';
45

56
import { wrapLoadWithSentry } from '../../src/client/load';
@@ -99,6 +100,18 @@ describe('wrapLoadWithSentry', () => {
99100
expect(mockCaptureException).toHaveBeenCalledTimes(1);
100101
});
101102

103+
it("doesn't call captureException for thrown `Redirect`s", async () => {
104+
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
105+
throw redirect(300, 'other/route');
106+
}
107+
108+
const wrappedLoad = wrapLoadWithSentry(load);
109+
const res = wrappedLoad(MOCK_LOAD_ARGS);
110+
await expect(res).rejects.toThrow();
111+
112+
expect(mockCaptureException).not.toHaveBeenCalled();
113+
});
114+
102115
it('calls trace function', async () => {
103116
async function load({ params }: Parameters<Load>[0]): Promise<ReturnType<Load>> {
104117
return {
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { isRedirect } from '../../src/common/utils';
2+
3+
describe('isRedirect', () => {
4+
it.each([
5+
{ location: '/users/id', status: 300 },
6+
{ location: '/users/id', status: 304 },
7+
{ location: '/users/id', status: 308 },
8+
{ location: '', status: 308 },
9+
])('returns `true` for valid Redirect objects', redirectObject => {
10+
expect(isRedirect(redirectObject)).toBe(true);
11+
});
12+
13+
it.each([
14+
300,
15+
'redirect',
16+
{ location: { route: { id: 'users/id' } }, status: 300 },
17+
{ status: 308 },
18+
{ location: '/users/id' },
19+
{ location: '/users/id', status: 201 },
20+
{ location: '/users/id', status: 400 },
21+
{ location: '/users/id', status: 500 },
22+
])('returns `false` for invalid Redirect objects', redirectObject => {
23+
expect(isRedirect(redirectObject)).toBe(false);
24+
});
25+
});

packages/sveltekit/test/server/load.test.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { addTracingExtensions } from '@sentry/core';
22
import { Scope } from '@sentry/node';
33
import type { Load, ServerLoad } from '@sveltejs/kit';
4-
import { error } from '@sveltejs/kit';
4+
import { error, redirect } from '@sveltejs/kit';
55
import { vi } from 'vitest';
66

77
import { wrapLoadWithSentry, wrapServerLoadWithSentry } from '../../src/server/load';
@@ -166,6 +166,18 @@ describe.each([
166166
});
167167
});
168168

169+
it("doesn't call captureException for thrown `Redirect`s", async () => {
170+
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
171+
throw redirect(300, 'other/route');
172+
}
173+
174+
const wrappedLoad = wrapLoadWithSentry(load);
175+
const res = wrappedLoad(MOCK_LOAD_ARGS);
176+
await expect(res).rejects.toThrow();
177+
178+
expect(mockCaptureException).not.toHaveBeenCalled();
179+
});
180+
169181
it('adds an exception mechanism', async () => {
170182
const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => {
171183
void callback({}, { event_id: 'fake-event-id' });

0 commit comments

Comments
 (0)