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 <julien@riou.xyz>

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
+* **message** (string): message of the error
-* **error** (string): error message
 
 ## Dependencies
 

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)
+		APIErrorNotFound(w, logger, "note does not exist", err)
-		h.logger.Error("note does not exists", slog.Any("note_id", id))
 	} else if note.PasswordHash != nil {
-		w.WriteHeader(http.StatusBadRequest)
+		APIErrorBadRequest(w, logger, "note is password protected", err)
-		h.logger.Error("note is password protected", slog.Any("note_id", note.ID))
 	} 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
 		}
 	}

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.TemplateError(w, logger, "could not parse form", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not parse file", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "file too large", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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.TemplateError(w, logger, "text file expected", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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.TemplateError(w, logger, "could not read file", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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.TemplateError(w, logger, "empty note", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "encryption is required", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 	}
 
 	if !h.PageData.AllowClientEncryptionKey && encryptionKey != "" {
-		h.PageData.Err = fmt.Errorf("client encryption key is not allowed")
+		h.TemplateError(w, logger, "client encryption key is not allowed", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 	}
 
 	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.TemplateError(w, logger, "could not create note", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not find note", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note == nil {
-		h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted")
+		h.TemplateError(w, logger, "note does not exist", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not parse form", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not find note", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note == nil {
-		h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted")
+		h.TemplateError(w, logger, "note does not exist", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note.Encrypted {
 		if encryptionKey == "" {
-			h.PageData.Err = fmt.Errorf("encryption key not found")
+			h.TemplateError(w, logger, "encryption key not found", nil)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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.TemplateError(w, logger, "could not decrypt note", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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.TemplateError(w, logger, "invalid password", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			return
 		}
 	}
 
-	h.PageData.Note = note
+	logger.Debug("returning content")
-
-	h.logger.Debug("rendering page")
-
 	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.TemplateError(w, logger, "could not find note", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note == nil {
-		h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted")
+		h.TemplateError(w, logger, "note does not exist", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not parse form", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		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.TemplateError(w, logger, "could not find note", err)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note == nil {
-		h.PageData.Err = fmt.Errorf("note doesn't exist or has been deleted")
+		h.TemplateError(w, logger, "note does not exist", nil)
-		h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 		return
 	}
 
 	if note.Encrypted {
 		if encryptionKey == "" {
-			h.PageData.Err = fmt.Errorf("encryption key not found")
+			h.TemplateError(w, logger, "encryption key not found", nil)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			return
 		}
 		note.Content, err = internal.Decrypt(note.Content, encryptionKey)
 		if err != nil {
-			h.PageData.Err = fmt.Errorf("could not decrypt note")
+			h.TemplateError(w, logger, "could not decrypt note", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			return
 		}
 	}
 
 	if len(note.PasswordHash) > 0 {
 		if err := bcrypt.CompareHashAndPassword(note.PasswordHash, []byte(password)); err != nil {
-			h.PageData.Err = fmt.Errorf("invalid password")
+			h.TemplateError(w, logger, "invalid password", err)
-			h.Templates.ExecuteTemplate(w, templateName, h.PageData)
 			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 {

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