From 690ea2ddafe6718a37ff4b56e1cac98893644a8a Mon Sep 17 00:00:00 2001
From: Arthur Belleville <arthur.belleville@datadoghq.com>
Date: Fri, 15 May 2026 12:50:07 +0200
Subject: [PATCH] fix(05): CR-01/WR-02/WR-03/WR-04 handlers_files.go fixes

- CR-01: add S3 cleanup before 500 when InsertTabloFile fails
- WR-02: validate empty filename, return 400 before S3 upload
- WR-03: remove dead errMsg variable (was silenced with _ = errMsg)
- WR-04: delete itoa/formatMBError helpers, inline strconv.Itoa

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
---
 backend/internal/web/handlers_files.go | 51 ++++++++++----------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/backend/internal/web/handlers_files.go b/backend/internal/web/handlers_files.go
index 5834d22..4e3f9fe 100644
--- a/backend/internal/web/handlers_files.go
+++ b/backend/internal/web/handlers_files.go
@@ -1,9 +1,13 @@
 package web
 
 import (
+	"context"
 	"errors"
 	"log/slog"
 	"net/http"
+	"strconv"
+	"strings"
+	"time"
 
 	"backend/internal/auth"
 	"backend/internal/db/sqlc"
@@ -156,9 +160,8 @@ func FileUploadHandler(deps FilesDeps) http.HandlerFunc {
 				}
 				w.Header().Set("Content-Type", "text/html; charset=utf-8")
 				w.WriteHeader(http.StatusUnprocessableEntity)
-				errMsg := "File too large (max %d MB)."
-				_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), formatMBError(deps.MaxUploadMB)).Render(r.Context(), w)
-				_ = errMsg
+				errMsg := "File too large (max " + strconv.Itoa(deps.MaxUploadMB) + " MB)."
+				_ = templates.UploadErrorFragment(tablo, fileList, csrf.Token(r), errMsg).Render(r.Context(), w)
 				return
 			}
 			http.Error(w, "bad request", http.StatusBadRequest)
@@ -172,6 +175,12 @@ func FileUploadHandler(deps FilesDeps) http.HandlerFunc {
 		}
 		defer file.Close()
 
+		filename := strings.TrimSpace(header.Filename)
+		if filename == "" {
+			http.Error(w, "bad request: file must have a filename", http.StatusBadRequest)
+			return
+		}
+
 		fileUUID := uuid.New()
 		s3Key := "files/" + tablo.ID.String() + "/" + fileUUID.String() // D-04
 
@@ -185,12 +194,18 @@ func FileUploadHandler(deps FilesDeps) http.HandlerFunc {
 		_, err = deps.Queries.InsertTabloFile(r.Context(), sqlc.InsertTabloFileParams{
 			TabloID:     tablo.ID,
 			S3Key:       s3Key,
-			Filename:    header.Filename,
+			Filename:    filename,
 			ContentType: contentType,
 			SizeBytes:   bytesWritten,
 		})
 		if err != nil {
-			slog.Default().Error("files upload: InsertTabloFile failed", "tablo_id", tablo.ID, "err", err)
+			slog.Default().Error("files upload: InsertTabloFile failed", "tablo_id", tablo.ID, "s3_key", s3Key, "err", err)
+			// Best-effort S3 cleanup — orphan prevention until Phase 6 reconciler exists.
+			cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+			defer cancel()
+			if delErr := deps.Files.Delete(cleanupCtx, s3Key); delErr != nil {
+				slog.Default().Error("files upload: S3 cleanup after DB failure", "s3_key", s3Key, "err", delErr)
+			}
 			http.Error(w, "internal server error", http.StatusInternalServerError)
 			return
 		}
@@ -215,32 +230,6 @@ func FileUploadHandler(deps FilesDeps) http.HandlerFunc {
 	}
 }
 
-// formatMBError formats the too-large error message with the max MB limit.
-func formatMBError(maxMB int) string {
-	return "File too large (max " + itoa(maxMB) + " MB)."
-}
-
-// itoa converts an integer to a string without importing strconv in this file.
-func itoa(n int) string {
-	if n == 0 {
-		return "0"
-	}
-	neg := false
-	if n < 0 {
-		neg = true
-		n = -n
-	}
-	buf := make([]byte, 0, 10)
-	for n > 0 {
-		buf = append([]byte{byte('0' + n%10)}, buf...)
-		n /= 10
-	}
-	if neg {
-		buf = append([]byte{'-'}, buf...)
-	}
-	return string(buf)
-}
-
 // FileDownloadHandler handles GET /tablos/{id}/files/{file_id}/download.
 // Generates a 5-minute presigned URL and returns a 302 redirect to it (FILE-04).
 //