Expose error message from shadow endpoint errors in __error.svelte #4032
Conversation
|
|
Returning an export function get({ params }) {
const thing = await get_thing(params.thing);
if (!thing) {
return {
status: 404,
error: new Error('not a thing')
};
}
return {
body: { thing }
};
}This would solve a useful additional purpose: at the moment it's awkward to differentiate between errors in a non-GET handler that should result in an error page (let's call them 'fatal errors'), and those that should be treated as props ('validation errors'). We could fix that: // post handler can return validation errors
export function post({ request, locals }) {
if (!locals.user) {
// fatal error — results in error page
return {
status: 401,
error: new Error('show yourself, coward')
};
}
if (!locals.user.mojo < 50) {
// fatal error — results in error page
return {
status: 403,
error: new Error('insufficient mojo')
};
}
const [errors, item] = await create_item(await request.formData());
if (errors) {
// validation errors — mixed with GET props and rendered
return {
status: 400,
body: { errors }
};
}
// great success
return {
body: { item }
};
}Not sure if this should apply to standalone endpoints, or what it would mean if it did. Also, I'm glossing over a bunch of implementation details here. |
|
In my brief look into the code, I initially assumed that a top-level |
|
Been discussing this with the other maintainers. We concluded that rather than adding a new top-level // post handler can return validation errors
export function post({ request, locals }) {
if (!locals.user) {
// fatal error — results in error page
return {
status: 401,
- error: new Error('show yourself, coward')
+ body: new Error('show yourself, coward')
};
}
if (!locals.user.mojo < 50) {
// fatal error — results in error page
return {
status: 403,
- error: new Error('insufficient mojo')
+ body: new Error('insufficient mojo')
};
}
const [errors, item] = await create_item(await request.formData());
if (errors) {
// validation errors — mixed with GET props and rendered
return {
status: 400,
body: { errors }
};
}
// great success
return {
body: { item }
};
}So this code... kit/packages/kit/src/runtime/server/page/load_node.js Lines 416 to 427 in 64234fa // Redirects are respected...
if (status >= 300 && status < 400) {
data.redirect = /** @type {string} */ (
headers instanceof Headers ? headers.get('location') : headers.location
);
return data;
}
+ // ...errors are propagated to the page...
+ if (status >= 400 && body instanceof Error) {
+ data.error = body;
+ return data;
+ }
// ...but 4xx and 5xx status codes _don't_ result in the error page
+ // if they are accompanied by a non-Error body
// rendering for non-GET requests — instead, we allow the page
// to render with any validation errors etc that were returned
data.body = body;When hitting the endpoint directly, errors could be serialized as POJOs: export function get() {
return {
status: 400,
body: new Error('nope', { cause: {...} })
};
}
// equivalent to this in dev
export function get() {
return {
status: 400,
body: {
message: 'nope',
stack: new Error().stack,
cause: {...}
}
};
}
// or this in prod
export function get() {
return {
status: 400,
body: {
message: 'nope',
cause: {...}
}
};
}(If The nice thing about this is it would apply to standalone endpoints just as easily, and error responses would have a standard 'shape'. Thoughts? |
|
Yep, I like this a lot. Questions:
// Only leak stack-trace in local dev
if (NODE_ENV === 'development') {
const serialized = serializeError(error);
return new Response(JSON.stringify({error: serialized}), {
status: 500,
headers,
});
}
// Obfuscate API error messages in non-dev, so that they don't leak
// sensitive information to the client
return new Response(
JSON.stringify({
error: {
message: 'Internal Server Error',
sentryId,
requestId,
},
}),
{
status: 500,
headers,
}
);
|
|
man, the devil is in the details!
Just to fill you in on some separate-but-related maintainers' chat, we've been wondering about a way to prevent unexpected (i.e. thrown) errors from reaching the client, since they might expose privileged information. That might look something like allowing error = options.handle_error(error, event) || new Error('Internal server error');export function handleError({ error, event }) {
console.error(`Error: ${event.request.method} ${event.request.url}`);
console.error(error.stack);
if (error.message === 'Please obfuscate me') {
return new Error('A terrible thing happened.');
}
// otherwise make the message visible to the client
return error;
}I don't think it would affect this PR, but worth flagging anyway. |
|
Thanks Rich. That all sounds good - especially the new powers designated to I'm not sure how much time I'll be able to give this, but I'll do my best. Is it something you'd want to work on yourself, or do you have higher priorities atm? |
|
It's not a burning priority, so I probably won't pick it up in the next few days. But no worries if you don't have the bandwidth — it'll happen eventually either way |
georgecrawford
force-pushed
the
shadow-endpoint-error-message
branch
from
a576485 to
1504e3f
Compare
|
@Rich-Harris What are your thoughts on this start I've made? A few notes:
Thanks for any tips! |
|
Hah, and now I find |
|
OK, I managed to get the hydration working by continuing to use I can now I experimented with using @Rich-Harris very interested to hear your thoughts on all this. |
|
I'm a bit stuck on what to do next with this PR. Happy to move it forward with tests, etc., but I'd like some confirmation that it's the right approach. |
|
Appreciate your work on this. Inconsistent SvelteKit error handling is a pretty big blocker for me at this point, especially if (as discussed above) there are forthcoming changes to how endpoints are expected to throw or return errors. I've taken a stab at fixing the problem as it now stands. I'm pasting my code here because this PR seems to be where you are tracking the issue, and I'm not quite sure whether my (somewhat opinionated) solution aligns with what the maintainers are planning. I'm working off this commit. I've attached my full, changed files to this comment. Let me know if the following makes sense. Happy to flesh this out with tests, etc. The IssueClient side code makes a fetch call to an endpoint. There are two cases:
In both cases, if the endpoint
The first case (SvelteKit client navigation) is the most noticeable failing case (see #3715 (comment) and #4303 (comment).) But text/html is returned in all cases when an endpoint throws. If userland client code makes a fetch request, one would expect an error to be returned as JSON. The only current workaround is to explicitly Opinions/NotesBeing able to The baseline error handling paradigm (in userland) should be to The one exception to this paradigm (that I can currently think of) is dealing with form validation errors in shadow endpoints. In this case it make sense to Error serialization. Developers can already see the server error stack in the console. So I've ignored that and other gnarly serialization concerns. In my change I just do The FixOnly two minor changes were necessary. src.zip First, fix the server to check whether the request is a client side fetch, and if so, respond with JSON... // kit/packages/kit/src/runtime/server/index.js
/** @type {import('types').Respond} */
export async function respond(request, options, state) {
// omitted for clarity
try {
// omitted for clarity
} catch (/** @type {unknown} */ e) {
const error = coalesce_to_error(e);
options.handle_error(error, event);
// start change
/**
* Note: error is guaranteed to be an Error at this point, because coalesce_to_error
*
* We need to check whether the request is a client side fetch. There are two cases:
* - It's a navigation, in which case the const is_data_request,
* defined further up in this function, is true
* - It's some other client side fetch initiated from userland (as opposed to SvelteKit) code.
* In this case we check whether the Accept header starts with application/json.
*/
if (
is_data_request ||
(typeof event.request.headers.get('accept') === 'string' &&
event.request.headers.get('accept')?.startsWith('application/json'))
) {
/**
* Return the error as JSON. For simplicity's sake, boil it down to the
* message.
*/
return new Response(JSON.stringify({ message: error.message }), {
status: 500,
headers: { 'content-type': 'application/json; charset=utf-8' }
});
}
// end change
try {
const $session = await options.hooks.getSession(event);
return await respond_with_error({
event,
options,
state,
$session,
status: 500,
error,
resolve_opts
});
} catch (/** @type {unknown} */ e) {
const error = coalesce_to_error(e);
return new Response(options.dev ? error.stack : error.message, {
status: 500
});
}
}
}Second, fix the client to handle errors resulting from navigation to a shadowed route. This is the same change to // kit/packages/kit/src/runtime/client/client.js
export function create_client({ target, session, base, trailing_slash }) {
// omitted for clarity...
async function load_route({ id, url, params, route }, no_cache) {
// omitted for clarity...
load: for (let i = 0; i < a.length; i += 1) {
/** @type {import('./types').BranchNode | undefined} */
let node;
try {
// omitted for clarity...
if (is_shadow_page) {
const res = await fetch(
`${url.pathname}${
url.pathname.endsWith('/') ? '' : '/'
}__data.json${url.search}`,
{
headers: {
'x-sveltekit-load': 'true'
}
}
);
if (res.ok) {
// omitted for clarity...
} else {
status = res.status;
// start change
try {
error = await res.json();
} catch (e) {
error = new Error('Failed to load data');
}
// end change
}
}
// omitted for clarity...
} catch (e) {
status = 500;
error = coalesce_to_error(e);
}
// omitted for clarity...
}
// omitted for clarity...
} |
|
To connect some dots here, I couldn't agree more with @cdcarson sentiment:
We are tracking something similar in
I brought up the "throw an http error from endpoints" before #4712 At a high level, I'm seeing that SvelteKit prefers, (assumes?) a return-everything pattern, similar to Golang. Exceptions and exception handling also need to be handled but it's not apparent that exceptions are a supported way to control flow, even when the flow is "return an error to the client because something bad happened but I don't know what." |
|
Closing in favour of #5314 — thank you |
Fixes #3715
When a shadow endpoint throws an error (or returns a status code > 400, with an
errorproperty in the body), it is useful for the__error.sveltehandler to have access to that error message. This could be obfuscated in non-dev environments, if that's what the maintainers would advise.Ideally, we'd find a way to transfer the full error object. This is super useful in local development - in the past I've used https://www.npmjs.com/package/serialize-error to serialize/deserialize the whole error object over JSON so that the
__error.sveltehandler can print the stack trace.NB: I haven't written a new test yet, but will do so if this approach is acceptable.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0