From 0ecc166dcdb409e0f6ae5684f3994ca6c1838c2d Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 15 Dec 2020 21:08:00 -0800 Subject: [PATCH] Update error reporting middleware --- router/error.go | 116 ++++++++++++++++----------------- router/middleware.go | 18 ++--- router/router_download.go | 12 ++-- router/router_server.go | 8 +-- router/router_server_backup.go | 6 +- router/router_server_files.go | 38 +++++------ router/router_server_ws.go | 2 +- router/router_system.go | 6 +- router/router_transfer.go | 8 +-- 9 files changed, 100 insertions(+), 114 deletions(-) diff --git a/router/error.go b/router/error.go index 126094f..551d6ab 100644 --- a/router/error.go +++ b/router/error.go @@ -14,45 +14,42 @@ import ( ) type RequestError struct { - Err error - Uuid string - Message string + err error + uuid string + message string server *server.Server } // Generates a new tracked error, which simply tracks the specific error that // is being passed in, and also assigned a UUID to the error so that it can be // cross referenced in the logs. -func TrackedError(err error) *RequestError { +func NewTrackedError(err error) *RequestError { return &RequestError{ - Err: err, - Uuid: uuid.Must(uuid.NewRandom()).String(), - Message: "", + err: err, + uuid: uuid.Must(uuid.NewRandom()).String(), } } -// Same as TrackedError, except this will also attach the server instance that +// Same as NewTrackedError, except this will also attach the server instance that // generated this server for the purposes of logging. -func TrackedServerError(err error, s *server.Server) *RequestError { +func NewServerError(err error, s *server.Server) *RequestError { return &RequestError{ - Err: err, - Uuid: uuid.Must(uuid.NewRandom()).String(), - Message: "", - server: s, + err: err, + uuid: uuid.Must(uuid.NewRandom()).String(), + server: s, } } func (e *RequestError) logger() *log.Entry { if e.server != nil { - return e.server.Log().WithField("error_id", e.Uuid) + return e.server.Log().WithField("error_id", e.uuid) } - - return log.WithField("error_id", e.Uuid) + return log.WithField("error_id", e.uuid) } // Sets the output message to display to the user in the error. func (e *RequestError) SetMessage(msg string) *RequestError { - e.Message = msg + e.message = msg return e } @@ -60,35 +57,43 @@ func (e *RequestError) SetMessage(msg string) *RequestError { // will also include the error UUID in the output so that the user can report that // and link the response to a specific error in the logs. func (e *RequestError) AbortWithStatus(status int, c *gin.Context) { + // In instances where the status has already been set just use that existing status + // since we cannot change it at this point, and trying to do so will emit a gin warning + // into the program output. + if c.Writer.Status() != 200 { + status = c.Writer.Status() + } + // If this error is because the resource does not exist, we likely do not need to log // the error anywhere, just return a 404 and move on with our lives. - if errors.Is(e.Err, os.ErrNotExist) { - e.logger().WithField("error", e.Err).Debug("encountered os.IsNotExist error while handling request") - + if errors.Is(e.err, os.ErrNotExist) { + e.logger().WithField("error", e.err).Debug("encountered os.IsNotExist error while handling request") c.AbortWithStatusJSON(http.StatusNotFound, gin.H{ "error": "The requested resource was not found on the system.", }) return } + // If this is a Filesystem error just return it without all of the tracking code nonsense + // since we don't need to be logging it into the logs or anything, its just a normal error + // that the user can solve on their end. + if st, msg := e.getAsFilesystemError(); st != 0 { + c.AbortWithStatusJSON(st, gin.H{"error": msg}) + return + } + // Otherwise, log the error to zap, and then report the error back to the user. if status >= 500 { - e.logger().WithField("error", e.Err).Error("encountered HTTP/500 error while handling request") - - c.Error(e) + e.logger().WithField("error", e.err).Error("unexpected error while handling HTTP request") } else { - e.logger().WithField("error", e.Err).Debug("encountered non-HTTP/500 error while handling request") + e.logger().WithField("error", e.err).Debug("non-server error encountered while handling HTTP request") } - msg := "An unexpected error was encountered while processing this request." - if e.Message != "" { - msg = e.Message + if e.message == "" { + e.message = "An unexpected error was encountered while processing this request." } - c.AbortWithStatusJSON(status, gin.H{ - "error": msg, - "error_id": e.Uuid, - }) + c.AbortWithStatusJSON(status, gin.H{"error": e.message, "error_id": e.uuid}) } // Helper function to just abort with an internal server error. This is generally the response @@ -97,41 +102,30 @@ func (e *RequestError) Abort(c *gin.Context) { e.AbortWithStatus(http.StatusInternalServerError, c) } +// Looks at the given RequestError and determines if it is a specific filesystem error that +// we can process and return differently for the user. +func (e *RequestError) getAsFilesystemError() (int, string) { + if errors.Is(e.err, os.ErrNotExist) || filesystem.IsErrorCode(e.err, filesystem.ErrCodePathResolution) { + return http.StatusNotFound, "The requested resource was not found on the system." + } + if filesystem.IsErrorCode(e.err, filesystem.ErrCodeDiskSpace) { + return http.StatusConflict, "There is not enough disk space available to perform that action." + } + if strings.HasSuffix(e.err.Error(), "file name too long") { + return http.StatusBadRequest, "Cannot perform that action: file name is too long." + } + if e, ok := e.err.(*os.SyscallError); ok && e.Syscall == "readdirent" { + return http.StatusNotFound, "The requested directory does not exist." + } + return 0, "" +} + // Handle specific filesystem errors for a server. func (e *RequestError) AbortFilesystemError(c *gin.Context) { - if errors.Is(e.Err, os.ErrNotExist) || filesystem.IsErrorCode(e.Err, filesystem.ErrCodePathResolution) { - if filesystem.IsErrorCode(e.Err, filesystem.ErrCodePathResolution) { - e.logger().Warn(e.Err.Error()) - } - - c.AbortWithStatusJSON(http.StatusNotFound, gin.H{"error": "The requested resource was not found."}) - return - } - - if filesystem.IsErrorCode(e.Err, filesystem.ErrCodeDiskSpace) { - c.AbortWithStatusJSON(http.StatusConflict, gin.H{"error": "There is not enough disk space available to perform that action."}) - return - } - - if strings.HasSuffix(e.Err.Error(), "file name too long") { - c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": "File name is too long."}) - return - } - - if e, ok := e.Err.(*os.SyscallError); ok && e.Syscall == "readdirent" { - c.AbortWithStatusJSON(http.StatusNotFound, gin.H{"error": "The requested directory does not exist."}) - return - } - - if strings.HasSuffix(e.Err.Error(), "file name too long") { - c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": "Cannot perform that action: file name is too long."}) - return - } - e.Abort(c) } // Format the error to a string and include the UUID. func (e *RequestError) Error() string { - return fmt.Sprintf("%v (uuid: %s)", e.Err, e.Uuid) + return fmt.Sprintf("%v (uuid: %s)", e.err, e.uuid) } diff --git a/router/middleware.go b/router/middleware.go index 96f8d09..c8fd094 100644 --- a/router/middleware.go +++ b/router/middleware.go @@ -22,23 +22,15 @@ func (m *Middleware) ErrorHandler() gin.HandlerFunc { if err == nil { return } - - tracked := TrackedError(err) + tracked := NewTrackedError(err) // If there is a server in the context for this request pull it out so that we can // track the error specifically for that server. if s, ok := c.Get("server"); ok { - tracked = TrackedServerError(err, s.(*server.Server)) + tracked = NewServerError(err, s.(*server.Server)) } - - // Sometimes requests have already modifed the status by the time this handler is - // called. In those cases, try to attach the error message but don't try to change - // the response status since it has already been set. - if c.Writer.Status() != 200 { - if err.Error() == io.EOF.Error() { - c.JSON(c.Writer.Status(), gin.H{"error": "A JSON formatted body is required for this endpoint."}) - } else { - tracked.AbortWithStatus(c.Writer.Status(), c) - } + // This error occurs if you submit invalid JSON data to an endpoint. + if err.Error() == io.EOF.Error() { + c.JSON(c.Writer.Status(), gin.H{"error": "A JSON formatted body is required for this endpoint."}) return } tracked.Abort(c) diff --git a/router/router_download.go b/router/router_download.go index 274df94..9bafa65 100644 --- a/router/router_download.go +++ b/router/router_download.go @@ -15,7 +15,7 @@ import ( func getDownloadBackup(c *gin.Context) { token := tokens.BackupPayload{} if err := tokens.ParseToken([]byte(c.Query("token")), &token); err != nil { - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -36,13 +36,13 @@ func getDownloadBackup(c *gin.Context) { return } - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } f, err := os.Open(b.Path()) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } defer f.Close() @@ -58,7 +58,7 @@ func getDownloadBackup(c *gin.Context) { func getDownloadFile(c *gin.Context) { token := tokens.FilePayload{} if err := tokens.ParseToken([]byte(c.Query("token")), &token); err != nil { - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -75,7 +75,7 @@ func getDownloadFile(c *gin.Context) { // If there is an error or we're somehow trying to download a directory, just // respond with the appropriate error. if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } else if st.IsDir() { c.AbortWithStatusJSON(http.StatusNotFound, gin.H{ @@ -86,7 +86,7 @@ func getDownloadFile(c *gin.Context) { f, err := os.Open(p) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } diff --git a/router/router_server.go b/router/router_server.go index 12f341b..71ed588 100644 --- a/router/router_server.go +++ b/router/router_server.go @@ -41,7 +41,7 @@ func getServerLogs(c *gin.Context) { out, err := s.ReadLogfile(l) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -110,7 +110,7 @@ func postServerCommands(c *gin.Context) { s := GetServer(c.Param("server")) if running, err := s.Environment.IsRunning(); err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } else if !running { c.AbortWithStatusJSON(http.StatusBadGateway, gin.H{ @@ -144,7 +144,7 @@ func patchServer(c *gin.Context) { buf.ReadFrom(c.Request.Body) if err := s.UpdateDataStructure(buf.Bytes()); err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -214,7 +214,7 @@ func deleteServer(c *gin.Context) { // forcibly terminate it before removing the container, so we do not need to handle // that here. if err := s.Environment.Destroy(); err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) } // Once the environment is terminated, remove the server files from the system. This is diff --git a/router/router_server_backup.go b/router/router_server_backup.go index 5cb4794..aadb26a 100644 --- a/router/router_server_backup.go +++ b/router/router_server_backup.go @@ -34,7 +34,7 @@ func postServerBackup(c *gin.Context) { } if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -63,7 +63,7 @@ func deleteServerBackup(c *gin.Context) { return } - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -72,7 +72,7 @@ func deleteServerBackup(c *gin.Context) { // the backup previously and it is now missing when we go to delete, just treat it as having // been successful, rather than returning a 404. if !errors.Is(err, os.ErrNotExist) { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } } diff --git a/router/router_server_files.go b/router/router_server_files.go index c41d6f0..44fcd15 100644 --- a/router/router_server_files.go +++ b/router/router_server_files.go @@ -25,7 +25,7 @@ func getServerFileContents(c *gin.Context) { p := "/" + strings.TrimLeft(c.Query("file"), "/") st, err := s.Filesystem().Stat(p) if err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -49,7 +49,7 @@ func getServerFileContents(c *gin.Context) { // happen since we're doing so much before this point that would normally throw an error if there // was a problem with the file. if err := s.Filesystem().Readfile(p, c.Writer); err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } } @@ -60,7 +60,7 @@ func getServerListDirectory(c *gin.Context) { stats, err := s.Filesystem().ListDirectory(c.Query("directory")) if err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -127,7 +127,7 @@ func putServerRenameFiles(c *gin.Context) { return } - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -147,7 +147,7 @@ func postServerCopyFile(c *gin.Context) { } if err := s.Filesystem().Copy(data.Location); err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -192,7 +192,7 @@ func postServerDeleteFiles(c *gin.Context) { } if err := g.Wait(); err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -205,7 +205,7 @@ func postServerWriteFile(c *gin.Context) { f, err := url.QueryUnescape(c.Query("file")) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } f = "/" + strings.TrimLeft(f, "/") @@ -218,7 +218,7 @@ func postServerWriteFile(c *gin.Context) { return } - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -244,13 +244,13 @@ func postServerDownloadRemoteFile(c *gin.Context) { }) return } - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } resp, err := http.Get(u.String()) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } defer resp.Body.Close() @@ -263,7 +263,7 @@ func postServerDownloadRemoteFile(c *gin.Context) { }) return } - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -291,7 +291,7 @@ func postServerCreateDirectory(c *gin.Context) { return } - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -326,7 +326,7 @@ func postServerCompressFiles(c *gin.Context) { f, err := s.Filesystem().CompressFiles(data.RootPath, data.Files) if err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -359,7 +359,7 @@ func postServerDecompressFiles(c *gin.Context) { return } - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } @@ -390,7 +390,7 @@ func postServerDecompressFiles(c *gin.Context) { return } - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -452,7 +452,7 @@ func postServerChmodFile(c *gin.Context) { } if err := g.Wait(); err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } @@ -462,7 +462,7 @@ func postServerChmodFile(c *gin.Context) { func postServerUploadFiles(c *gin.Context) { token := tokens.UploadPayload{} if err := tokens.ParseToken([]byte(c.Query("token")), &token); err != nil { - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -500,14 +500,14 @@ func postServerUploadFiles(c *gin.Context) { for _, header := range headers { p, err := s.Filesystem().SafePath(filepath.Join(directory, header.Filename)) if err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } // We run this in a different method so I can use defer without any of // the consequences caused by calling it in a loop. if err := handleFileUpload(p, s, header); err != nil { - TrackedServerError(err, s).AbortFilesystemError(c) + NewServerError(err, s).AbortFilesystemError(c) return } } diff --git a/router/router_server_ws.go b/router/router_server_ws.go index 69688e9..2633b03 100644 --- a/router/router_server_ws.go +++ b/router/router_server_ws.go @@ -14,7 +14,7 @@ func getServerWebsocket(c *gin.Context) { s := GetServer(c.Param("server")) handler, err := websocket.GetHandler(s, c.Writer, c.Request) if err != nil { - TrackedServerError(err, s).Abort(c) + NewServerError(err, s).Abort(c) return } defer handler.Connection.Close() diff --git a/router/router_system.go b/router/router_system.go index ec04095..b3707e8 100644 --- a/router/router_system.go +++ b/router/router_system.go @@ -16,7 +16,7 @@ import ( func getSystemInformation(c *gin.Context) { i, err := system.GetSystemInformation() if err != nil { - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -45,7 +45,7 @@ func postCreateServer(c *gin.Context) { return } - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -99,7 +99,7 @@ func postUpdateConfiguration(c *gin.Context) { // before this code was run. config.Set(&ccopy) - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } diff --git a/router/router_transfer.go b/router/router_transfer.go index 5afe1ac..053cd8a 100644 --- a/router/router_transfer.go +++ b/router/router_transfer.go @@ -37,7 +37,7 @@ func getServerArchive(c *gin.Context) { token := tokens.TransferPayload{} if err := tokens.ParseToken([]byte(auth[1]), &token); err != nil { - TrackedError(err).Abort(c) + NewTrackedError(err).Abort(c) return } @@ -53,7 +53,7 @@ func getServerArchive(c *gin.Context) { st, err := s.Archiver.Stat() if err != nil { if !errors.Is(err, os.ErrNotExist) { - TrackedServerError(err, s).SetMessage("failed to stat archive").Abort(c) + NewServerError(err, s).SetMessage("failed to stat archive").Abort(c) return } @@ -63,13 +63,13 @@ func getServerArchive(c *gin.Context) { checksum, err := s.Archiver.Checksum() if err != nil { - TrackedServerError(err, s).SetMessage("failed to calculate checksum").Abort(c) + NewServerError(err, s).SetMessage("failed to calculate checksum").Abort(c) return } file, err := os.Open(s.Archiver.Path()) if err != nil { - tserr := TrackedServerError(err, s) + tserr := NewServerError(err, s) if !os.IsNotExist(err) { tserr.SetMessage("failed to open archive for reading") } else {