From 2358cf4e433401e9a42336f056df1ab223f89057 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Fri, 2 Jun 2023 15:19:43 +0200 Subject: [PATCH] [bugfix] Overwrite API client closed errors with `499 - Client Closed Request` (#1857) * [bugfix] Overwrite client closed errors with 499 * bleep bloop * review changes --- internal/api/util/errorhandling.go | 37 ++++++++++++++++++++++-------- internal/gtserror/withcode.go | 27 +++++++++++++++++++++- internal/middleware/logger.go | 17 ++++++++++++-- 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/internal/api/util/errorhandling.go b/internal/api/util/errorhandling.go index ed7a18fb0..8c0251e57 100644 --- a/internal/api/util/errorhandling.go +++ b/internal/api/util/errorhandling.go @@ -83,25 +83,44 @@ func genericErrorHandler(c *gin.Context, instanceGet func(ctx context.Context) ( } } -// ErrorHandler takes the provided gin context and errWithCode and tries to serve -// a helpful error to the caller. It will do content negotiation to figure out if -// the caller prefers to see an html page with the error rendered there. If not, or -// if something goes wrong during the function, it will recover and just try to serve -// an appropriate application/json content-type error. +// ErrorHandler takes the provided gin context and errWithCode +// and tries to serve a helpful error to the caller. +// +// It will do content negotiation to figure out if the caller prefers +// to see an html page with the error rendered there. If not, or if +// something goes wrong during the function, it will recover and just +// try to serve an appropriate application/json content-type error. // To override the default response type, specify `offers`. +// +// If the requester already hung up on the request, ErrorHandler +// will overwrite the given errWithCode with a 499 error to indicate +// that the failure wasn't due to something we did, and will avoid +// trying to write extensive bytes to the caller by just aborting. +// +// See: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#nginx. func ErrorHandler(c *gin.Context, errWithCode gtserror.WithCode, instanceGet func(ctx context.Context) (*apimodel.InstanceV1, gtserror.WithCode), offers ...MIME) { - // set the error on the gin context so that it can be logged - // in the gin logger middleware (internal/router/logger.go) + if c.Request.Context().Err() != nil { + // Context error means requester probably left already. + // Wrap original error with a less alarming one. Then + // we can return early, because it doesn't matter what + // we send to the client at this point; they're gone. + errWithCode = gtserror.NewErrorClientClosedRequest(errWithCode.Unwrap()) + c.AbortWithStatus(errWithCode.Code()) + return + } + + // Set the error on the gin context so that it can be logged + // in the gin logger middleware (internal/middleware/logger.go). c.Error(errWithCode) //nolint:errcheck - // discover if we're allowed to serve a nice html error page, + // Discover if we're allowed to serve a nice html error page, // or if we should just use a json. Normally we would want to // check for a returned error, but if an error occurs here we // can just fall back to default behavior (serve json error). accept, _ := NegotiateAccept(c, JSONOrHTMLAcceptHeaders...) if errWithCode.Code() == http.StatusNotFound { - // use our special not found handler with useful status text + // Use our special not found handler with useful status text. NotFoundHandler(c, instanceGet, accept) } else { genericErrorHandler(c, instanceGet, accept, errWithCode) diff --git a/internal/gtserror/withcode.go b/internal/gtserror/withcode.go index b449926a8..55fe7502a 100644 --- a/internal/gtserror/withcode.go +++ b/internal/gtserror/withcode.go @@ -23,13 +23,23 @@ import ( "strings" ) +// Custom http response codes + text that +// aren't included in the net/http package. +const ( + StatusClientClosedRequest = 499 + StatusTextClientClosedRequest = "Client Closed Request" +) + // WithCode wraps an internal error with an http code, and a 'safe' version of // the error that can be served to clients without revealing internal business logic. // // A typical use of this error would be to first log the Original error, then return // the Safe error and the StatusCode to an API caller. type WithCode interface { - // Error returns the original internal error for debugging within the GoToSocial logs. + // Unwrap returns the original error. + // This should *NEVER* be returned to a client as it may contain sensitive information. + Unwrap() error + // Error serializes the original internal error for debugging within the GoToSocial logs. // This should *NEVER* be returned to a client as it may contain sensitive information. Error() string // Safe returns the API-safe version of the error for serialization towards a client. @@ -45,6 +55,10 @@ type withCode struct { code int } +func (e withCode) Unwrap() error { + return e.original +} + func (e withCode) Error() string { return e.original.Error() } @@ -173,3 +187,14 @@ func NewErrorGone(original error, helpText ...string) WithCode { code: http.StatusGone, } } + +// NewErrorClientClosedRequest returns an ErrorWithCode 499 with the given original error. +// This error type should only be used when an http caller has already hung up their request. +// See: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#nginx +func NewErrorClientClosedRequest(original error) WithCode { + return withCode{ + original: original, + safe: errors.New(StatusTextClientClosedRequest), + code: StatusClientClosedRequest, + } +} diff --git a/internal/middleware/logger.go b/internal/middleware/logger.go index 8acb742fb..58f90b088 100644 --- a/internal/middleware/logger.go +++ b/internal/middleware/logger.go @@ -27,6 +27,7 @@ import ( "codeberg.org/gruf/go-kv" "codeberg.org/gruf/go-logger/v2/level" "github.com/gin-gonic/gin" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/log" ) @@ -91,11 +92,23 @@ func Logger(logClientIP bool) gin.HandlerFunc { l = l.WithField("error", c.Errors) } + // Get appropriate text for this code. + statusText := http.StatusText(code) + if statusText == "" { + // Look for custom codes. + switch code { + case gtserror.StatusClientClosedRequest: + statusText = gtserror.StatusTextClientClosedRequest + default: + statusText = "Unknown Status" + } + } + // Generate a nicer looking bytecount size := bytesize.Size(c.Writer.Size()) - // Finally, write log entry with status text body size - l.Logf(lvl, "%s: wrote %s", http.StatusText(code), size) + // Finally, write log entry with status text + body size. + l.Logf(lvl, "%s: wrote %s", statusText, size) }() // Process request