-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix s3 integration error handling #15180
base: master
Are you sure you want to change the base?
Conversation
…ew instances of Error. Rethrowing errors with a statusCode set causes issues with koa
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
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.
LGTM but will let someone from firestorm review too.
|
||
// Do not rethrow AWS errors as the statusCode prop will be | ||
// interpreted by koa | ||
throw new Error("AWS S3: " + message) |
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.
Could you add { cause: e }
to this to make sure we don't lose the stack trace? I think you'd need to do something like this:
try {
// ...
} catch (e: unknown) {
if (e instanceof Error) {
throw new Error(e.message, { cause: e })
}
throw new Error("AWS S3: Unable to process request")
}
Alternately, is it enough to target the problem Koa behaviour directly like this:
try {
// ...
} catch (e: unknown) {
if ("statusCode" in e && e.statusCode === 403) {
e.statusCode = 400
}
throw e
}
Alternately alternately should we even trust 403s from integrations at all? Is there ever a case where we want to log the user out if an integration returns a 403? Should we go right to the source and map an integration 403 to 400 in all cases in whatever relevant bit of Koa is handling that?
Description
The default behaviour for handling integration errors is to throw the error and have koa relay it as a
400
.If an error object thrown by an integration contains a
statusCode
, koa will ignore the400
and use that instead.In development, if you encountered a permission restriction with your S3 integration, the AWS client would return a
403
and log you out. In prod, the page you were on would reload and you would lose the error message.All errors in the S3 integration throw a new instance of
Error
with any relevant messaging. This will then be relayed with the default400
status.Addresses
Launchcontrol
S3 integration error handling updated