breaking: tighten up error handling (#11289)

* breaking: tighten up error handling Introduces a NonFatalError object that is used internally and that is user-detectable in handleError * how did this happen * fix tests * lint * types * adjust wording (is this even a breaking change now?) * adjust * pass status and message to handleError * Apply suggestions from code review Co-authored-by: Rich Harris <[email protected]> * lint * Update documentation/docs/30-advanced/20-hooks.md Co-authored-by: Ben McCann <[email protected]> * lint * simplify example * tweak docs * Update documentation/docs/30-advanced/20-hooks.md * various tweaks. we can be less duplicative i think * tweak * tweak * handle too large body after streaming has started * cancel stream from the inside if content-length exceeds limit * remove unnecessary try-catch, bump adapter-node/adapter-vercel majors * migration docs * tweak/fix tests * fix more * more --------- Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Ben McCann <[email protected]>
sveltejs · Dec 13, 2023 · 7718d0f · 7718d0f
1 parent 57be35a
commit 7718d0f
Show file tree

Hide file tree

Showing 31 changed files with 246 additions and 200 deletions.
diff --git a/.changeset/fast-dolls-clean.md b/.changeset/fast-dolls-clean.md
@@ -0,0 +1,6 @@
+---
+'@sveltejs/adapter-vercel': major
+'@sveltejs/adapter-node': major
+---
+
+breaking: require SvelteKit 2 peer dependency
diff --git a/.changeset/real-pets-fix.md b/.changeset/real-pets-fix.md
@@ -0,0 +1,5 @@
+---
+"@sveltejs/kit": major
+---
+
+breaking: tighten up error handling
diff --git a/documentation/docs/30-advanced/20-hooks.md b/documentation/docs/30-advanced/20-hooks.md
@@ -139,12 +139,14 @@ The following can be added to `src/hooks.server.js` _and_ `src/hooks.client.js`:
 
 ### handleError
 
-If an unexpected error is thrown during loading or rendering, this function will be called with the `error` and the `event`. This allows for two things:
+If an [unexpected error](/docs/errors#unexpected-errors) is thrown during loading or rendering, this function will be called with the `error`, `event`, `status` code and `message`. This allows for two things:
 
 - you can log the error
-- you can generate a custom representation of the error that is safe to show to users, omitting sensitive details like messages and stack traces. The returned value becomes the value of `$page.error`. It defaults to `{ message: 'Not Found' }` in case of a 404 (you can detect them through `event.route.id` being `null`) and to `{ message: 'Internal Error' }` for everything else. To make this type-safe, you can customize the expected shape by declaring an `App.Error` interface (which must include `message: string`, to guarantee sensible fallback behavior).
+- you can generate a custom representation of the error that is safe to show to users, omitting sensitive details like messages and stack traces. The returned value, which defaults to `{ message }`, becomes the value of `$page.error`.
 
-The following code shows an example of typing the error shape as `{ message: string; errorId: string }` and returning it accordingly from the `handleError` functions:
+For errors thrown from your code (or library code called by your code) the status will be 500 and the message will be "Internal Error". While `error.message` may contain sensitive information that should not be exposed to users, `message` is safe (albeit meaningless to the average user).
+
+To add more information to the `$page.error` object in a type-safe way, you can customize the expected shape by declaring an `App.Error` interface (which must include `message: string`, to guarantee sensible fallback behavior). This allows you to — for example — append a tracking ID for users to quote in correspondence with your technical support staff:
 
 ```ts
 /// file: src/app.d.ts
@@ -172,15 +174,17 @@ declare module '@sentry/sveltekit' {
 // @filename: index.js
 // ---cut---
 import * as Sentry from '@sentry/sveltekit';
-import crypto from 'crypto';
 
 Sentry.init({/*...*/})
 
 /** @type {import('@sveltejs/kit').HandleServerError} */
-export async function handleError({ error, event }) {
+export async function handleError({ error, event, status, message }) {
 	const errorId = crypto.randomUUID();
+
 	// example integration with https://sentry.io/
-	Sentry.captureException(error, { extra: { event, errorId } });
+	Sentry.captureException(error, {
+		extra: { event, errorId, status }
+	});
 
 	return {
 		message: 'Whoops!',
@@ -205,10 +209,13 @@ import * as Sentry from '@sentry/sveltekit';
 Sentry.init({/*...*/})
 
 /** @type {import('@sveltejs/kit').HandleClientError} */
-export async function handleError({ error, event }) {
+export async function handleError({ error, event, status, message }) {
 	const errorId = crypto.randomUUID();
+
 	// example integration with https://sentry.io/
-	Sentry.captureException(error, { extra: { event, errorId } });
+	Sentry.captureException(error, {
+		extra: { event, errorId, status }
+	});
 
 	return {
 		message: 'Whoops!',

diff --git a/documentation/docs/30-advanced/25-errors.md b/documentation/docs/30-advanced/25-errors.md
@@ -77,36 +77,7 @@ By default, unexpected errors are printed to the console (or, in production, you
 { "message": "Internal Error" }
 ```
 
-Unexpected errors will go through the [`handleError`](hooks#shared-hooks-handleerror) hook, where you can add your own error handling — for example, sending errors to a reporting service, or returning a custom error object.
-
-```js
-/// file: src/hooks.server.js
-// @errors: 2322 1360 2571 2339 2353
-// @filename: ambient.d.ts
-declare module '@sentry/sveltekit' {
-	export const init: (opts: any) => void;
-	export const captureException: (error: any, opts: any) => void;
-}
-
-// @filename: index.js
-// ---cut---
-import * as Sentry from '@sentry/sveltekit';
-
-Sentry.init({/*...*/})
-
-/** @type {import('@sveltejs/kit').HandleServerError} */
-export function handleError({ error, event }) {
-	// example integration with https://sentry.io/
-	Sentry.captureException(error, { extra: { event } });
-
-	return {
-		message: 'Whoops!',
-		code: error?.code ?? 'UNKNOWN'
-	};
-}
-```
-
-> Make sure that `handleError` _never_ throws an error
+Unexpected errors will go through the [`handleError`](hooks#shared-hooks-handleerror) hook, where you can add your own error handling — for example, sending errors to a reporting service, or returning a custom error object which becomes `$page.error`.
 
 ## Responses
 

diff --git a/documentation/docs/60-appendix/30-migrating-to-sveltekit-2.md b/documentation/docs/60-appendix/30-migrating-to-sveltekit-2.md
@@ -105,6 +105,12 @@ As such, SvelteKit 2 replaces `resolvePath` with a (slightly better named) funct
 
 `svelte-migrate` will do the method replacement for you, though if you later prepend the result with `base`, you need to remove that yourself.
 
+## Improved error handling
+
+Errors are handled inconsistently in SvelteKit 1. Some errors trigger the `handleError` hook but there is no good way to discern their status (for example, the only way to tell a 404 from a 500 is by seeing if `event.route.id` is `null`), while others (such as 405 errors for `POST` requests to pages without actions) don't trigger `handleError` at all, but should. In the latter case, the resulting `$page.error` will deviate from the [`App.Error`](/docs/types#app-error) type, if it is specified.
+
+SvelteKit 2 cleans this up by calling `handleError` hooks with two new properties: `status` and `message`. For errors thrown from your code (or library code called by your code) the status will be `500` and the message will be `Internal Error`. While `error.message` may contain sensitive information that should not be exposed to users, `message` is safe.
+
 ## Dynamic environment variables cannot be used during prerendering
 
 The `$env/dynamic/public` and `$env/dynamic/private` modules provide access to _run time_ environment variables, as opposed to the _build time_ environment variables exposed by `$env/static/public` and `$env/static/private`.
@@ -127,6 +133,12 @@ If a form contains an `<input type="file">` but does not have an `enctype="multi
 
 Previously, the generated `tsconfig.json` was trying its best to still produce a somewhat valid config when your `tsconfig.json` included `paths` or `baseUrl`. In SvelteKit 2, the validation is more strict and will warn when you use either `paths` or `baseUrl` in your `tsconfig.json`. These settings are used to generate path aliases and you should use [the `alias` config](configuration#alias) option in your `svelte.config.js` instead, to also create a corresponding alias for the bundler.
 
+## `getRequest` no longer throws errors
+
+The `@sveltejs/kit/node` module exports helper functions for use in Node environments, including `getRequest` which turns a Node [`ClientRequest`](https://nodejs.org/api/http.html#class-httpclientrequest) into a standard [`Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) object.
+
+In SvelteKit 1, `getRequest` could throw if the `Content-Length` header exceeded the specified size limit. In SvelteKit 2, the error will not be thrown until later, when the request body (if any) is being read. This enables better diagnostics and simpler code.
+
 ## `vitePreprocess` is no longer exported from `@sveltejs/kit/vite`
 
 Since `@sveltejs/vite-plugin-svelte` is now a peer dependency, SvelteKit 2 no longer re-exports `vitePreprocess`. You should import it directly from `@svelte/vite-plugin-svelte`.

diff --git a/packages/adapter-node/package.json b/packages/adapter-node/package.json
@@ -50,6 +50,6 @@
 		"rollup": "^4.8.0"
 	},
 	"peerDependencies": {
-		"@sveltejs/kit": "^1.0.0 || ^2.0.0"
+		"@sveltejs/kit": "^2.0.0"
 	}
 }
diff --git a/packages/adapter-node/src/handler.js b/packages/adapter-node/src/handler.js
@@ -76,20 +76,11 @@ function serve_prerendered() {
 
 /** @type {import('polka').Middleware} */
 const ssr = async (req, res) => {
-	/** @type {Request | undefined} */
-	let request;
-
-	try {
-		request = await getRequest({
-			base: origin || get_origin(req.headers),
-			request: req,
-			bodySizeLimit: body_size_limit
-		});
-	} catch (err) {
-		res.statusCode = err.status || 400;
-		res.end('Invalid request body');
-		return;
-	}
+	const request = await getRequest({
+		base: origin || get_origin(req.headers),
+		request: req,
+		bodySizeLimit: body_size_limit
+	});
 
 	setResponse(
 		res,

diff --git a/packages/adapter-vercel/files/serverless.js b/packages/adapter-vercel/files/serverless.js
@@ -32,15 +32,7 @@ export default async (req, res) => {
 		}
 	}
 
-	/** @type {Request} */
-	let request;
-
-	try {
-		request = await getRequest({ base: `https://${req.headers.host}`, request: req });
-	} catch (err) {
-		res.statusCode = /** @type {any} */ (err).status || 400;
-		return res.end('Invalid request body');
-	}
+	const request = await getRequest({ base: `https://${req.headers.host}`, request: req });
 
 	setResponse(
 		res,

diff --git a/packages/adapter-vercel/package.json b/packages/adapter-vercel/package.json
@@ -42,6 +42,6 @@
 		"vitest": "^1.0.4"
 	},
 	"peerDependencies": {
-		"@sveltejs/kit": "^1.5.0 || ^2.0.0"
+		"@sveltejs/kit": "^2.0.0"
 	}
 }
diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js
@@ -1,5 +1,5 @@
 import * as set_cookie_parser from 'set-cookie-parser';
-import { error } from '../index.js';
+import { SvelteKitError } from '../../runtime/control.js';
 
 /**
  * @param {import('http').IncomingMessage} req
@@ -22,19 +22,6 @@ function get_raw_body(req, body_size_limit) {
 		return null;
 	}
 
-	let length = content_length;
-
-	if (body_size_limit) {
-		if (!length) {
-			length = body_size_limit;
-		} else if (length > body_size_limit) {
-			error(
-				413,
-				`Received content-length of ${length}, but only accept up to ${body_size_limit} bytes.`
-			);
-		}
-	}
-
 	if (req.destroyed) {
 		const readable = new ReadableStream();
 		readable.cancel();
@@ -46,6 +33,17 @@ function get_raw_body(req, body_size_limit) {
 
 	return new ReadableStream({
 		start(controller) {
+			if (body_size_limit !== undefined && content_length > body_size_limit) {
+				const error = new SvelteKitError(
+					413,
+					'Payload Too Large',
+					`Content-length of ${content_length} exceeds limit of ${body_size_limit} bytes.`
+				);
+
+				controller.error(error);
+				return;
+			}
+
 			req.on('error', (error) => {
 				cancelled = true;
 				controller.error(error);
@@ -60,16 +58,15 @@ function get_raw_body(req, body_size_limit) {
 				if (cancelled) return;
 
 				size += chunk.length;
-				if (size > length) {
+				if (size > content_length) {
 					cancelled = true;
-					controller.error(
-						error(
-							413,
-							`request body size exceeded ${
-								content_length ? "'content-length'" : 'BODY_SIZE_LIMIT'
-							} of ${length}`
-						)
-					);
+
+					const constraint = content_length ? 'content-length' : 'BODY_SIZE_LIMIT';
+					const message = `request body size exceeded ${constraint} of ${content_length}`;
+
+					const error = new SvelteKitError(413, 'Payload Too Large', message);
+					controller.error(error);
+
 					return;
 				}
 

diff --git a/packages/kit/src/exports/public.d.ts b/packages/kit/src/exports/public.d.ts
@@ -657,6 +657,8 @@ export type Handle = (input: {
 export type HandleServerError = (input: {
 	error: unknown;
 	event: RequestEvent;
+	status: number;
+	message: string;
 }) => MaybePromise<void | App.Error>;
 
 /**
@@ -668,6 +670,8 @@ export type HandleServerError = (input: {
 export type HandleClientError = (input: {
 	error: unknown;
 	event: NavigationEvent;
+	status: number;
+	message: string;
 }) => MaybePromise<void | App.Error>;
 
 /**

diff --git a/packages/kit/src/exports/vite/dev/index.js b/packages/kit/src/exports/vite/dev/index.js
@@ -355,7 +355,8 @@ export async function dev(vite, vite_config, svelte_config) {
 		control_module_node.replace_implementations({
 			ActionFailure: control_module_vite.ActionFailure,
 			HttpError: control_module_vite.HttpError,
-			Redirect: control_module_vite.Redirect
+			Redirect: control_module_vite.Redirect,
+			SvelteKitError: control_module_vite.SvelteKitError
 		});
 	}
 	align_exports();
@@ -471,17 +472,10 @@ export async function dev(vite, vite_config, svelte_config) {
 
 				await server.init({ env });
 
-				let request;
-
-				try {
-					request = await getRequest({
-						base,
-						request: req
-					});
-				} catch (/** @type {any} */ err) {
-					res.statusCode = err.status || 400;
-					return res.end('Invalid request body');
-				}
+				const request = await getRequest({
+					base,
+					request: req
+				});
 
 				if (manifest_error) {
 					console.error(colors.bold().red(manifest_error.message));

diff --git a/packages/kit/src/exports/vite/preview/index.js b/packages/kit/src/exports/vite/preview/index.js
@@ -122,16 +122,11 @@ export async function preview(vite, vite_config, svelte_config) {
 		vite.middlewares.use(async (req, res) => {
 			const host = req.headers['host'];
 			req.url = req.originalUrl;
-			let request;
-			try {
-				request = await getRequest({
-					base: `${protocol}://${host}`,
-					request: req
-				});
-			} catch (/** @type {any} */ err) {
-				res.statusCode = err.status || 400;
-				return res.end('Invalid request body');
-			}
+
+			const request = await getRequest({
+				base: `${protocol}://${host}`,
+				request: req
+			});
 
 			setResponse(
 				res,