From 38fe5b39090fa3b8c0600f1403ea26e1362b3556 Mon Sep 17 00:00:00 2001 From: Arthur Belleville Date: Fri, 15 May 2026 08:30:22 +0200 Subject: [PATCH] fix(03): CR-02 capture user from loadOwnedTablo on update error path - TabloUpdateHandler: capture user from loadOwnedTablo (was discarded with _) - Pass captured user to TabloDetailPage on non-HTMX validation error path instead of nil, preventing broken layout (no logout button/email shown) - TabloUpdateHandler: pass tablo.Color to UpdateTablo to preserve color on update (CR-01) - loadOwnedTablo: pass GetTabloByIDParams{ID, UserID} to DB query (WR-01 call site) - TabloDeleteHandler: pass DeleteTabloParams{ID, UserID} to DB query (WR-02 call site) - TabloDeleteHandler: on DB error with HX-Request, render TabloDeleteConfirmFragment instead of plain http.Error to avoid broken HTMX DOM state (CR-03) - renderTabloCreateError: log secondary ListTablosByUser fetch failure (WR-03) - TablosCreateHandler: validate color with isValidCSSColor (hex only) and surface TabloCreateErrors.Color field error to prevent CSS injection (WR-04) - Add isValidCSSColor helper using ^#[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$ regex - Update test call sites for GetTabloByID and DeleteTablo new param types Co-Authored-By: Claude Sonnet 4.6 (1M context) --- backend/internal/web/handlers_tablos.go | 53 ++++++++++++++------ backend/internal/web/handlers_tablos_test.go | 8 +-- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/backend/internal/web/handlers_tablos.go b/backend/internal/web/handlers_tablos.go index 1682a3f..9518dcf 100644 --- a/backend/internal/web/handlers_tablos.go +++ b/backend/internal/web/handlers_tablos.go @@ -4,6 +4,7 @@ import ( "errors" "log/slog" "net/http" + "regexp" "strings" "backend/internal/auth" @@ -17,6 +18,14 @@ import ( "github.com/jackc/pgx/v5/pgtype" ) +// hexColorRE matches CSS hex colors: #RGB or #RRGGBB (case-insensitive). +var hexColorRE = regexp.MustCompile(`^#[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$`) + +// isValidCSSColor returns true when s is a valid hex color string. +func isValidCSSColor(s string) bool { + return hexColorRE.MatchString(s) +} + // TablosDeps holds dependencies for all tablo handlers. // Introduced in Plan 01 as a stub to allow handlers_tablos_test.go to compile. // Plan 02 adds the actual handler implementations. @@ -90,7 +99,12 @@ func TablosCreateHandler(deps TablosDeps) http.HandlerFunc { errs.Title = "Title must be 255 characters or fewer." } - if errs.Title != "" { + // Validate color: accept only hex #RGB or #RRGGBB format (WR-04). + if color != "" && !isValidCSSColor(color) { + errs.Color = "Color must be a valid hex color (e.g. #6366f1)." + } + + if errs.Title != "" || errs.Color != "" { renderTabloCreateError(w, r, templates.TabloCreateForm{ Title: title, Description: description, @@ -152,8 +166,8 @@ func loadOwnedTablo(w http.ResponseWriter, r *http.Request, deps TablosDeps) (sq return sqlc.Tablo{}, nil, false } - // Step 2: fetch from DB. - tablo, err := deps.Queries.GetTabloByID(r.Context(), tabloID) + // Step 2: fetch from DB — user_id filter pushes ownership into the query (WR-01). + tablo, err := deps.Queries.GetTabloByID(r.Context(), sqlc.GetTabloByIDParams{ID: tabloID, UserID: user.ID}) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.NotFound(w, r) @@ -164,12 +178,6 @@ func loadOwnedTablo(w http.ResponseWriter, r *http.Request, deps TablosDeps) (sq return sqlc.Tablo{}, nil, false } - // Step 3: ownership check (D-04: 404 not 403 — no existence leakage). - if tablo.UserID != user.ID { - http.NotFound(w, r) - return sqlc.Tablo{}, nil, false - } - return tablo, user, true } @@ -250,7 +258,8 @@ func TabloShowDescHandler(deps TablosDeps) http.HandlerFunc { // - HTMX path: 200 + display fragment; non-HTMX path: 303 to /tablos/{id} func TabloUpdateHandler(deps TablosDeps) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - tablo, _, ok := loadOwnedTablo(w, r, deps) + // Capture user from loadOwnedTablo for non-HTMX error path (CR-02). + tablo, user, ok := loadOwnedTablo(w, r, deps) if !ok { return } @@ -277,16 +286,18 @@ func TabloUpdateHandler(deps TablosDeps) http.HandlerFunc { _ = templates.TabloTitleEditFragment(tablo, errs, csrf.Token(r)).Render(ctx, w) return } - // Non-HTMX: render full detail page with errors surfaced. - _ = templates.TabloDetailPage(nil, csrf.Token(r), tablo).Render(ctx, w) + // Non-HTMX: render full detail page with errors surfaced using the + // authenticated user (not nil) to avoid broken layout (CR-02). + _ = templates.TabloDetailPage(user, csrf.Token(r), tablo).Render(ctx, w) return } - // Update the DB row (UpdateTablo sets updated_at = now() per sqlc query — Pitfall 7). + // Update the DB row — pass tablo.Color to preserve it across title/desc edits (CR-01). updated, err := deps.Queries.UpdateTablo(ctx, sqlc.UpdateTabloParams{ ID: tablo.ID, Title: title, Description: pgtype.Text{String: description, Valid: description != ""}, + Color: tablo.Color, }) if err != nil { slog.Default().Error("tablos update: query failed", "id", tablo.ID, "err", err) @@ -354,8 +365,17 @@ func TabloDeleteHandler(deps TablosDeps) http.HandlerFunc { return } - if err := deps.Queries.DeleteTablo(r.Context(), tablo.ID); err != nil { + // DeleteTablo includes user_id in the WHERE clause for defense-in-depth (WR-02). + if err := deps.Queries.DeleteTablo(r.Context(), sqlc.DeleteTabloParams{ID: tablo.ID, UserID: tablo.UserID}); err != nil { slog.Default().Error("tablos delete: query failed", "id", tablo.ID, "err", err) + // On HTMX request, render a meaningful fragment instead of plain-text 500 + // to avoid leaving the delete zone in a broken DOM state (CR-03). + if r.Header.Get("HX-Request") == "true" { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusInternalServerError) + _ = templates.TabloDeleteConfirmFragment(tablo, csrf.Token(r)).Render(r.Context(), w) + return + } http.Error(w, "internal server error", http.StatusInternalServerError) return } @@ -382,8 +402,9 @@ func renderTabloCreateError(w http.ResponseWriter, r *http.Request, form templat // Non-HTMX: render full dashboard with errs embedded in the form. // Fetch the user's tablos so the list is still accurate on re-render. _, user, _ := auth.Authed(r.Context()) - tablos, err := deps.Queries.ListTablosByUser(r.Context(), user.ID) - if err != nil { + tablos, fetchErr := deps.Queries.ListTablosByUser(r.Context(), user.ID) + if fetchErr != nil { + slog.Default().Error("renderTabloCreateError: list fetch failed", "user_id", user.ID, "err", fetchErr) tablos = []sqlc.Tablo{} } // Render full page — form fragment is not embedded in the full page by default; diff --git a/backend/internal/web/handlers_tablos_test.go b/backend/internal/web/handlers_tablos_test.go index 79944db..9ada092 100644 --- a/backend/internal/web/handlers_tablos_test.go +++ b/backend/internal/web/handlers_tablos_test.go @@ -272,7 +272,7 @@ func TestTabloCreate(t *testing.T) { // Delete any existing tablos to keep state clean. tablos, _ := q.ListTablosByUser(ctx, user.ID) for _, tbl := range tablos { - _ = q.DeleteTablo(ctx, tbl.ID) + _ = q.DeleteTablo(ctx, sqlc.DeleteTabloParams{ID: tbl.ID, UserID: tbl.UserID}) } cookieVal, _, err := store.Create(ctx, user.ID) @@ -523,7 +523,7 @@ func TestTabloUpdate(t *testing.T) { } // Verify DB state. - updated, err := q.GetTabloByID(ctx, tablo.ID) + updated, err := q.GetTabloByID(ctx, sqlc.GetTabloByIDParams{ID: tablo.ID, UserID: user.ID}) if err != nil { t.Fatalf("GetTabloByID after update: %v", err) } @@ -649,7 +649,7 @@ func TestTabloDelete(t *testing.T) { } // DB row must be gone. - _, err = q.GetTabloByID(ctx, tablo.ID) + _, err = q.GetTabloByID(ctx, sqlc.GetTabloByIDParams{ID: tablo.ID, UserID: user.ID}) if err == nil { t.Error("tablo row still exists in DB after delete") } @@ -691,7 +691,7 @@ func TestTabloDelete(t *testing.T) { } // DB row must be gone. - _, err = q.GetTabloByID(ctx, tablo.ID) + _, err = q.GetTabloByID(ctx, sqlc.GetTabloByIDParams{ID: tablo.ID, UserID: user.ID}) if err == nil { t.Error("tablo row still exists in DB after non-HTMX delete") }