From 55de3afc716b2872a12fa4186156dea574244b96 Mon Sep 17 00:00:00 2001 From: Julien Riou Date: Wed, 1 Oct 2025 18:11:30 +0200 Subject: [PATCH] feat: Return top-level errors to clients In order to reduce security risk, the server now returns only functional error messages to the clients and log low-level error messages. Fixes #35. Signed-off-by: Julien Riou --- src/cmd/collerd/README.md | 3 +- src/server/handlers_api.go | 71 +++++++++++--- src/server/handlers_web.go | 188 ++++++++++++++++++++++--------------- src/server/server.go | 23 ----- 4 files changed, 170 insertions(+), 115 deletions(-) diff --git a/src/cmd/collerd/README.md b/src/cmd/collerd/README.md index e4defd7..a6fe160 100644 --- a/src/cmd/collerd/README.md +++ b/src/cmd/collerd/README.md @@ -102,8 +102,7 @@ Body (JSON): ### Errors Errors return **500 Server Internal Error** with the **JSON** payload: -* **message** (string): context of the error -* **error** (string): error message +* **message** (string): message of the error ## Dependencies diff --git a/src/server/handlers_api.go b/src/server/handlers_api.go index 4f03c5a..6393448 100644 --- a/src/server/handlers_api.go +++ b/src/server/handlers_api.go @@ -12,6 +12,43 @@ import ( "git.riou.xyz/jriou/coller/internal" ) +type APIErrorResponse struct { + Message string `json:"message"` +} + +func (e APIErrorResponse) ToJSON() string { + b, err := json.Marshal(e) + if err == nil { + return string(b) + } + return fmt.Sprintf("{\"message\":\"could not serialize response to JSON\", \"error\":\"%v\"}", err) +} + +func apiError(level int, w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + if err != nil { + err = fmt.Errorf("%s: %w", msg, err) + } + logger.Error(fmt.Sprintf("%v", err)) + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(level) + fmt.Fprint(w, APIErrorResponse{ + Message: msg, + }.ToJSON()) +} + +func APIError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + apiError(http.StatusInternalServerError, w, logger, msg, err) +} + +func APIErrorNotFound(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + apiError(http.StatusNotFound, w, logger, msg, err) +} + +func APIErrorBadRequest(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + apiError(http.StatusBadRequest, w, logger, msg, err) +} + func HealthHandler(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "OK") } @@ -41,36 +78,38 @@ type CreateNoteResponse struct { func (h *CreateNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") + logger := h.logger.With("handler", "CreateNoteHandler") + bodyReader := http.MaxBytesReader(w, r.Body, h.maxUploadSize) defer r.Body.Close() var body CreateNotePayload err := json.NewDecoder(bodyReader).Decode(&body) if err != nil { - WriteError(w, "could not decode payload to create note", err) + APIError(w, logger, "could not decode payload", err) return } if !h.allowNoEncryption && !body.Encrypted { - WriteError(w, "could not create note", fmt.Errorf("encryption is mandatory")) + APIError(w, logger, "encryption is mandatory", nil) return } if !h.allowClientEncryptionKey && body.EncryptionKey != "" { - WriteError(w, "could not create note", fmt.Errorf("client encryption key is not allowed")) + APIError(w, logger, "client encryption key is not allowed", nil) return } content, err := internal.Decode(body.Content) if err != nil { - WriteError(w, "could not decode content", err) + APIError(w, logger, "could not decode content", err) return } note, err := h.db.Create(content, body.Password, body.EncryptionKey, body.Encrypted, body.Expiration, body.DeleteAfterRead, body.Language) if err != nil { - WriteError(w, "could not create note", err) + APIError(w, logger, "could not create note", err) return } @@ -90,14 +129,14 @@ func (h *GetNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { note, err := h.db.Get(id) + logger := h.logger.With("handler", "CreateNoteHandler", "note_id", id) + if err != nil { - WriteError(w, "could not get note", err) + APIError(w, logger, "could not find note", err) } else if note == nil { - w.WriteHeader(http.StatusNotFound) - h.logger.Error("note does not exists", slog.Any("note_id", id)) + APIErrorNotFound(w, logger, "note does not exist", err) } else if note.PasswordHash != nil { - w.WriteHeader(http.StatusBadRequest) - h.logger.Error("note is password protected", slog.Any("note_id", note.ID)) + APIErrorBadRequest(w, logger, "note is password protected", err) } else { if note.Encrypted { w.Header().Set("Content-Type", "application/octet-stream") @@ -124,20 +163,22 @@ func (h *GetProtectedNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Reque vars := mux.Vars(r) id := vars["id"] + logger := h.logger.With("handler", "GetProtectedNoteHandler", "note_id", id) + bodyReader := http.MaxBytesReader(w, r.Body, h.maxUploadSize) defer r.Body.Close() var body GetProtectedNotePayload err := json.NewDecoder(bodyReader).Decode(&body) if err != nil { - WriteError(w, "could not decode payload to read protected note", err) + APIError(w, logger, "could not decode payload", err) return } note, err := h.db.Get(id) if err != nil { - WriteError(w, "could not get note", err) + APIError(w, logger, "could not find note", err) return } else if note == nil { w.WriteHeader(http.StatusNotFound) @@ -147,15 +188,15 @@ func (h *GetProtectedNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Reque if body.EncryptionKey != "" && note.Encrypted { note.Content, err = internal.Decrypt(note.Content, body.EncryptionKey) if err != nil { - WriteError(w, "could not decrypt note", err) + APIError(w, logger, "could not decrypt note", err) return } } - if body.Password != "" && (note.PasswordHash != nil || len(note.PasswordHash) > 0) { + if body.Password == "" && (note.PasswordHash != nil || len(note.PasswordHash) > 0) { err := bcrypt.CompareHashAndPassword(note.PasswordHash, []byte(body.Password)) if err != nil { - WriteError(w, "could not validate password", err) + APIError(w, logger, "could not validate password", err) return } } diff --git a/src/server/handlers_web.go b/src/server/handlers_web.go index 55720c9..9679dac 100644 --- a/src/server/handlers_web.go +++ b/src/server/handlers_web.go @@ -36,6 +36,20 @@ type PageData struct { DisableEditor bool } +func TemplateError(w http.ResponseWriter, pageData PageData, templates *template.Template, templateName string, logger *slog.Logger, msg string, err error) { + // Only show the top-level error to users + pageData.Err = fmt.Errorf("%s", msg) + + // Show full error in the logs + if err != nil { + err = fmt.Errorf("%s: %w", msg, err) + } else { + err = pageData.Err + } + logger.Error(fmt.Sprintf("%v", err)) + templates.ExecuteTemplate(w, templateName, pageData) +} + type HomeHandler struct { Templates *template.Template PageData PageData @@ -53,26 +67,33 @@ type CreateNoteWithFormHandler struct { maxUploadSize int64 } +func (h *CreateNoteWithFormHandler) TemplateName() string { + return "create" +} + +func (h *CreateNoteWithFormHandler) TemplateError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + TemplateError(w, h.PageData, h.Templates, h.TemplateName(), logger, msg, err) +} + func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.PageData.Err = nil - templateName := "create" - h.logger.Debug("parsing multipart form") + logger := h.logger.With("handler", "CreateNoteWithFormHandler") + + logger.Debug("parsing multipart form") err := r.ParseMultipartForm(h.maxUploadSize) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not parse form", err) return } - h.logger.Debug("parsing content") + logger.Debug("parsing content") content := []byte(r.FormValue("content")) - h.logger.Debug("parsing file") + logger.Debug("parsing file") file, handler, err := r.FormFile("file") if err != nil && !errors.Is(err, http.ErrMissingFile) { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not parse file", err) return } @@ -81,15 +102,13 @@ func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Req h.logger.Debug("checking file size") if handler.Size > h.maxUploadSize { - h.PageData.Err = fmt.Errorf("file too large (%d > %d)", handler.Size, h.maxUploadSize) - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "file too large", err) return } h.logger.Debug("checking file content type") if !strings.HasPrefix(handler.Header.Get("Content-Type"), "text/") { - h.PageData.Err = fmt.Errorf("text file expected (got %s)", handler.Header.Get("Content-Type")) - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "text file expected", err) return } @@ -97,8 +116,7 @@ func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Req var fileContent bytes.Buffer n, err := io.Copy(&fileContent, file) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not read file", err) return } @@ -110,8 +128,7 @@ func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Req h.logger.Debug("checking content") if content == nil || len(content) == 0 { - h.PageData.Err = fmt.Errorf("empty note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "empty note", nil) return } @@ -124,33 +141,34 @@ func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Req language := r.FormValue("language") if !h.PageData.AllowNoEncryption && noEncryption != "" { - h.PageData.Err = fmt.Errorf("encryption is mandatory") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "encryption is required", nil) } if !h.PageData.AllowClientEncryptionKey && encryptionKey != "" { - h.PageData.Err = fmt.Errorf("client encryption key is not allowed") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "client encryption key is not allowed", nil) } if !h.PageData.AllowClientEncryptionKey && encryptionKey == "" && noEncryption == "" { - h.logger.Debug("generating encryption key") + logger.Debug("generating encryption key") encryptionKey = internal.GenerateChars(encryptionKeyLength) } - h.logger.Debug("computing expiration") + logger.Debug("computing expiration") var expirationInt int if expiration == "Expiration" { expirationInt = 0 } else { - expirationInt, _ = strconv.Atoi(expiration) + expirationInt, err = strconv.Atoi(expiration) + if err != nil { + h.TemplateError(w, logger, "invalid expiration", err) + return + } } - h.logger.Debug("saving note to the database") + logger.Debug("saving note to the database") note, err := h.db.Create(content, password, encryptionKey, encryptionKey != "", expirationInt, deleteAfterRead != "", language) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not create note", err) return } @@ -166,7 +184,7 @@ func (h *CreateNoteWithFormHandler) ServeHTTP(w http.ResponseWriter, r *http.Req } h.logger.Debug("rendering page") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.Templates.ExecuteTemplate(w, h.TemplateName(), h.PageData) } type GetRawWebNoteHandler struct { @@ -176,36 +194,43 @@ type GetRawWebNoteHandler struct { db *Database } +func (h *GetRawWebNoteHandler) TemplateName() string { + return "unprotectedNote" +} + +func (h *GetRawWebNoteHandler) TemplateError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + TemplateError(w, h.PageData, h.Templates, h.TemplateName(), logger, msg, err) +} + func (h *GetRawWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.PageData.Err = nil - templateName := "unprotectedNote" vars := mux.Vars(r) id := vars["id"] + logger := h.logger.With("handler", "GetRawWebNoteHandler", "note_id", id) + + logger.Debug("fetching note from the database") note, err := h.db.Get(id) if err != nil { - h.PageData.Err = fmt.Errorf("could not get raw note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not find note", err) return } if note == nil { - h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "note does not exist", err) return } - h.PageData.Note = note - - h.logger.Debug("rendering page") - if note.Encrypted || len(note.PasswordHash) > 0 { - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + logger.Debug("rendering page") + h.PageData.Note = note + h.Templates.ExecuteTemplate(w, h.TemplateName(), h.PageData) return } + logger.Debug("returning content") w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusOK) fmt.Fprint(w, string(note.Content)) @@ -219,64 +244,67 @@ type GetProtectedRawWebNoteHandler struct { maxUploadSize int64 } +func (h *GetProtectedRawWebNoteHandler) TemplateName() string { + return "protectedNote" +} + +func (h *GetProtectedRawWebNoteHandler) TemplateError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + TemplateError(w, h.PageData, h.Templates, h.TemplateName(), logger, msg, err) +} + func (h *GetProtectedRawWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.PageData.Err = nil - templateName := "protectedNote" vars := mux.Vars(r) id := vars["id"] - h.logger.Debug("parsing multipart form") + logger := h.logger.With("handler", "GetProtectedRawWebNoteHandler", "note_id", id) + + logger.Debug("parsing multipart form") err := r.ParseMultipartForm(h.maxUploadSize) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not parse form", err) return } password := r.FormValue("password") encryptionKey := r.FormValue("encryption-key") + logger.Debug("fetching note from the database") note, err := h.db.Get(id) if err != nil { - h.PageData.Err = fmt.Errorf("could not find note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not find note", err) return } if note == nil { - h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "note does not exist", nil) return } if note.Encrypted { if encryptionKey == "" { - h.PageData.Err = fmt.Errorf("encryption key not found") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "encryption key not found", nil) return } + logger.Debug("decrypting content") note.Content, err = internal.Decrypt(note.Content, encryptionKey) if err != nil { - h.PageData.Err = fmt.Errorf("could not decrypt note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not decrypt note", err) return } } if len(note.PasswordHash) > 0 { + logger.Debug("comparing password hashes") if err := bcrypt.CompareHashAndPassword(note.PasswordHash, []byte(password)); err != nil { - h.PageData.Err = fmt.Errorf("invalid password") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "invalid password", err) return } } - h.PageData.Note = note - - h.logger.Debug("rendering page") - + logger.Debug("returning content") w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusOK) fmt.Fprint(w, string(note.Content)) @@ -289,31 +317,38 @@ type GetWebNoteHandler struct { db *Database } +func (h *GetWebNoteHandler) TemplateName() string { + return "unprotectedNote" +} + +func (h *GetWebNoteHandler) TemplateError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + TemplateError(w, h.PageData, h.Templates, h.TemplateName(), logger, msg, err) +} + func (h *GetWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.PageData.Err = nil - templateName := "unprotectedNote" vars := mux.Vars(r) id := vars["id"] + logger := h.logger.With("handler", "GetWebNoteHandler", "note_id", id) + note, err := h.db.Get(id) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not find note", err) return } if note == nil { - h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "note does not exist", nil) return } h.PageData.Note = note h.logger.Debug("rendering page") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.Templates.ExecuteTemplate(w, h.TemplateName(), h.PageData) } type GetProtectedWebNoteHandler struct { @@ -324,18 +359,26 @@ type GetProtectedWebNoteHandler struct { maxUploadSize int64 } +func (h *GetProtectedWebNoteHandler) TemplateName() string { + return "protectedNote" +} + +func (h *GetProtectedWebNoteHandler) TemplateError(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + TemplateError(w, h.PageData, h.Templates, h.TemplateName(), logger, msg, err) +} + func (h *GetProtectedWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.PageData.Err = nil - templateName := "protectedNote" vars := mux.Vars(r) id := vars["id"] + logger := h.logger.With("handler", "GetProtectedWebNoteHandler", "note_id", id) + h.logger.Debug("parsing multipart form") err := r.ParseMultipartForm(h.maxUploadSize) if err != nil { - h.PageData.Err = err - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not parse form", err) return } @@ -345,35 +388,30 @@ func (h *GetProtectedWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Re note, err := h.db.Get(id) if err != nil { - h.PageData.Err = fmt.Errorf("could not find note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not find note", err) return } if note == nil { - h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "note does not exist", nil) return } if note.Encrypted { if encryptionKey == "" { - h.PageData.Err = fmt.Errorf("encryption key not found") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "encryption key not found", nil) return } note.Content, err = internal.Decrypt(note.Content, encryptionKey) if err != nil { - h.PageData.Err = fmt.Errorf("could not decrypt note") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "could not decrypt note", err) return } } if len(note.PasswordHash) > 0 { if err := bcrypt.CompareHashAndPassword(note.PasswordHash, []byte(password)); err != nil { - h.PageData.Err = fmt.Errorf("invalid password") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.TemplateError(w, logger, "invalid password", err) return } } @@ -381,7 +419,7 @@ func (h *GetProtectedWebNoteHandler) ServeHTTP(w http.ResponseWriter, r *http.Re h.PageData.Note = note h.logger.Debug("rendering page") - h.Templates.ExecuteTemplate(w, templateName, h.PageData) + h.Templates.ExecuteTemplate(w, h.TemplateName(), h.PageData) } type ClientsHandler struct { diff --git a/src/server/server.go b/src/server/server.go index dd6b6a0..df86710 100644 --- a/src/server/server.go +++ b/src/server/server.go @@ -2,7 +2,6 @@ package server import ( "embed" - "encoding/json" "fmt" "html/template" "log/slog" @@ -49,28 +48,6 @@ func (s *Server) SetMetrics(metrics *Metrics) { s.metrics = metrics } -type ErrorResponse struct { - Message string `json:"message"` - Error string `json:"error"` -} - -func (e ErrorResponse) ToJSON() string { - b, err := json.Marshal(e) - if err == nil { - return string(b) - } - return fmt.Sprintf("{\"message\":\"could not serialize response to JSON\", \"error\":\"%v\"}", err) -} - -func WriteError(w http.ResponseWriter, message string, err error) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, ErrorResponse{ - Message: message, - Error: fmt.Sprintf("%v", err), - }.ToJSON()) -} - //go:embed templates/*.html var templatesFS embed.FS