-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update error handling for Apollo 4 #2483
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,21 +70,25 @@ const server = new ApolloServer({ | |
resolvers, | ||
introspection: true, | ||
plugins: [ApolloServerPluginDrainHttpServer({ httpServer })], | ||
formatError: error => { | ||
formatError: (formattedError, error) => { | ||
log.error({ | ||
// TODO: request is no longer available in formatError, figure out | ||
// another way to do this. | ||
// userId: request.user && request.user.id, | ||
code: error?.extensions?.code ?? 'INTERNAL_SERVER_ERROR', | ||
error: formattedError, | ||
msg: "GraphQL error" | ||
}); | ||
|
||
if (process.env.SHOW_SERVER_ERROR || process.env.DEBUG) { | ||
if (error instanceof GraphQLError) { | ||
return error; | ||
} | ||
return new GraphQLError(error.message); | ||
return formattedError; | ||
} | ||
|
||
return new GraphQLError( | ||
error && | ||
error.originalError && | ||
error.originalError.code === "UNAUTHORIZED" | ||
? "UNAUTHORIZED" | ||
: "Internal server error" | ||
); | ||
// Strip out stacktrace and other potentially sensative details. | ||
return { | ||
message: formattedError.message, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my main concern is what formattedError.message is/can be if an unexpected (e.g. internal library) error happens. E.g. Can this leak a database connection error/failure? I think some courses would make us more confident of this:
I feel like the code line might still be possible to keep as long as this isn't a new (I'm not up on new JS stuff) thing that goes beyond Apollo frameworks, though I'm a bit nervous of that -- best to whitelist a specific set of codes or only allow it if error typeof 'GraphQLError' -- i.e. something we've consciously wrapped (and you have explicit sets of codes above). The thing we must block is the ability for a hacker to probe the app for ways to trigger a specific kind of failure or learn more info in the contexts of auth of which stage it fails in (e.g. authentication vs authorization). [deleted and re-created comment from before] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely want to find the right balance. There's just a lot of places where giving users errors like "That invitation is no longer valid" or "Not allowed to add contacts after the campaign starts" is a lot more helpful than "Internal server error". Another possible option, which I thought about but didn't test, would be to create our own error class that extends Not 100% sure if that's possible, but I think it might be. |
||
code: formattedError?.extensions?.code ?? 'INTERNAL_SERVER_ERROR', | ||
}; | ||
} | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just add a code here like in the others.