Address Gadfly review on #7: garden cap, dim validation, shared errors

Fixes from the PR #26 adversarial review (graded 18 real / 0 false positive).

Correctness / security
- maxGardenCM fixed to 10_000 (100 m), matching its comment — it was
  100_000 cm (1 km), 10x too lax (5 models flagged this).
- Dimension validation now rejects NaN/Inf (which slip past naive
  comparisons) and subnormal-tiny positives, via a finite [1cm, 100m]
  check. Name (200) and notes (10_000) are length-capped so untrusted
  input can't balloon storage.
- Update version binding is `required,min=1`, so a negative/zero version
  is a 400, not a 409.

Maintainability / performance
- One unified writeServiceError (new errors.go) maps every auth + resource
  sentinel; writeResourceError removed. writeVersionConflict and
  parseIDParam moved to errors.go (shared, not in the gardens feature file).
- Request structs share an embedded gardenFields (one toInput).
- CreateGarden uses INSERT ... RETURNING (one round-trip).
- ListGardensForOwner has a defensive LIMIT (pagination is post-v1).

Tests: name/notes length, NaN/Inf/subnormal dims, dimension-at-cap valid,
negative/zero version -> 400.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

internal/api/auth.go

@@ -1,7 +1,6 @@
 package api
 
 import (
-	"errors"
 	"log/slog"
 	"net/http"
 	"net/url"
@@ -210,29 +209,3 @@ func (h *handlers) clearSessionCookie(c *gin.Context) {
 func mustActor(c *gin.Context) *domain.User {
 	return c.MustGet(actorKey).(*domain.User)
 }
-
-// writeServiceError maps a service-layer sentinel error to pansy's JSON error
-// envelope. Login failures never leak which of email/password was wrong.
-func writeServiceError(c *gin.Context, err error) {
-	switch {
-	case errors.Is(err, domain.ErrInvalidCredentials):
-		writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
-	case errors.Is(err, domain.ErrEmailTaken):
-		writeAPIError(c, http.StatusConflict, "EMAIL_TAKEN", "an account with that email already exists")
-	case errors.Is(err, domain.ErrRegistrationClosed):
-		writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed")
-	case errors.Is(err, domain.ErrLocalAuthDisabled):
-		writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled")
-	case errors.Is(err, domain.ErrOIDCNoEmail):
-		writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email")
-	case errors.Is(err, domain.ErrOIDCEmailUnverified):
-		writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified")
-	case errors.Is(err, domain.ErrOIDCIdentityConflict):
-		writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account")
-	case errors.Is(err, domain.ErrInvalidInput):
-		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
-	default:
-		slog.Error("api: unhandled service error", "error", err)
-		writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
-	}
-}

internal/api/errors.go

@@ -0,0 +1,73 @@
+package api
+
+import (
+	"errors"
+	"log/slog"
+	"net/http"
+	"strconv"
+
+	"github.com/gin-gonic/gin"
+
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
+)
+
+// writeServiceError maps a service-layer sentinel error to pansy's JSON error
+// envelope ({"error":{"code","message"}}). One mapper serves every handler
+// (auth and resources) so the status/code for a given sentinel is defined once.
+//
+// ErrNotFound covers both a genuinely missing row and one the actor may not see
+// (existence is masked). ErrVersionConflict here is a fallback that omits the
+// current row — handlers that can produce one special-case it with
+// writeVersionConflict before falling through here. Login failures never leak
+// which of email/password was wrong.
+func writeServiceError(c *gin.Context, err error) {
+	switch {
+	case errors.Is(err, domain.ErrNotFound):
+		writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found")
+	case errors.Is(err, domain.ErrForbidden):
+		writeAPIError(c, http.StatusForbidden, "FORBIDDEN", "you don't have access")
+	case errors.Is(err, domain.ErrVersionConflict):
+		writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")
+	case errors.Is(err, domain.ErrInvalidCredentials):
+		writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
+	case errors.Is(err, domain.ErrEmailTaken):
+		writeAPIError(c, http.StatusConflict, "EMAIL_TAKEN", "an account with that email already exists")
+	case errors.Is(err, domain.ErrRegistrationClosed):
+		writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed")
+	case errors.Is(err, domain.ErrLocalAuthDisabled):
+		writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled")
+	case errors.Is(err, domain.ErrOIDCNoEmail):
+		writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email")
+	case errors.Is(err, domain.ErrOIDCEmailUnverified):
+		writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified")
+	case errors.Is(err, domain.ErrOIDCIdentityConflict):
+		writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account")
+	case errors.Is(err, domain.ErrInvalidInput):
+		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
+	default:
+		slog.Error("api: unhandled service error", "error", err)
+		writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
+	}
+}
+
+// writeVersionConflict writes the 409 envelope for an optimistic-concurrency
+// failure: the standard error object plus the current server row under
+// "current", so the client can rebase its edit onto the fresh version and retry.
+// This shape is the contract for every version-guarded (mutable) resource.
+func writeVersionConflict(c *gin.Context, current any) {
+	c.JSON(http.StatusConflict, gin.H{
+		"error":   gin.H{"code": "VERSION_CONFLICT", "message": "the resource was modified; refetch and retry"},
+		"current": current,
+	})
+}
+
+// parseIDParam reads a positive int64 path parameter, writing a 400 and
+// returning ok=false on a malformed value.
+func parseIDParam(c *gin.Context, name string) (int64, bool) {
+	id, err := strconv.ParseInt(c.Param(name), 10, 64)
+	if err != nil || id < 1 {
+		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid id")
+		return 0, false
+	}
+	return id, true
+}

internal/api/gardens.go

@@ -2,9 +2,7 @@ package api
 
 import (
 	"errors"
-	"log/slog"
 	"net/http"
-	"strconv"
 
 	"github.com/gin-gonic/gin"
 
@@ -12,11 +10,10 @@ import (
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
 )
 
-// gardenCreateRequest / gardenUpdateRequest are the JSON bodies for the gardens
-// endpoints. Dimensions are centimeters (the API is metric-only; imperial is a
-// display concern). On create, omitted dimensions default server-side; on
-// update, version is required and every field is replaced.
-type gardenCreateRequest struct {
+// gardenFields is the shared mutable field set for the gardens endpoints.
+// Dimensions are centimeters (the API is metric-only; imperial is a display
+// concern). On create, omitted dimensions default server-side.
+type gardenFields struct {
 	Name     string  `json:"name" binding:"required"`
 	WidthCM  float64 `json:"widthCm"`
 	HeightCM float64 `json:"heightCm"`
@@ -24,20 +21,19 @@ type gardenCreateRequest struct {
 	Notes    string  `json:"notes"`
 }
 
+func (f gardenFields) toInput() service.GardenInput {
+	return service.GardenInput{Name: f.Name, WidthCM: f.WidthCM, HeightCM: f.HeightCM, UnitPref: f.UnitPref, Notes: f.Notes}
+}
+
+// gardenCreateRequest / gardenUpdateRequest are the JSON bodies. Update adds a
+// required current version (>= 1) for the optimistic-concurrency guard.
+type gardenCreateRequest struct {
+	gardenFields
+}
+
 type gardenUpdateRequest struct {
-	Name     string  `json:"name" binding:"required"`
-	WidthCM  float64 `json:"widthCm"`
-	HeightCM float64 `json:"heightCm"`
-	UnitPref string  `json:"unitPref"`
-	Notes    string  `json:"notes"`
-	Version  int64   `json:"version" binding:"required"`
-}
-
-func (r gardenCreateRequest) toInput() service.GardenInput {
-	return service.GardenInput{Name: r.Name, WidthCM: r.WidthCM, HeightCM: r.HeightCM, UnitPref: r.UnitPref, Notes: r.Notes}
-}
-func (r gardenUpdateRequest) toInput() service.GardenInput {
-	return service.GardenInput{Name: r.Name, WidthCM: r.WidthCM, HeightCM: r.HeightCM, UnitPref: r.UnitPref, Notes: r.Notes}
+	gardenFields
+	Version int64 `json:"version" binding:"required,min=1"`
 }
 
 // listGardens returns the actor's gardens as a JSON array (always an array,
@@ -45,7 +41,7 @@ func (r gardenUpdateRequest) toInput() service.GardenInput {
 func (h *handlers) listGardens(c *gin.Context) {
 	gardens, err := h.svc.ListGardens(c.Request.Context(), mustActor(c).ID)
 	if err != nil {
-		writeResourceError(c, err)
+		writeServiceError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, gardens)
@@ -59,7 +55,7 @@ func (h *handlers) createGarden(c *gin.Context) {
 	}
 	g, err := h.svc.CreateGarden(c.Request.Context(), mustActor(c).ID, req.toInput())
 	if err != nil {
-		writeResourceError(c, err)
+		writeServiceError(c, err)
 		return
 	}
 	c.JSON(http.StatusCreated, g)
@@ -72,7 +68,7 @@ func (h *handlers) getGarden(c *gin.Context) {
 	}
 	g, err := h.svc.GetGarden(c.Request.Context(), mustActor(c).ID, id)
 	if err != nil {
-		writeResourceError(c, err)
+		writeServiceError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, g)
@@ -96,7 +92,7 @@ func (h *handlers) updateGarden(c *gin.Context) {
 			writeVersionConflict(c, g)
 			return
 		}
-		writeResourceError(c, err)
+		writeServiceError(c, err)
 		return
 	}
 	c.JSON(http.StatusOK, g)
@@ -108,51 +104,8 @@ func (h *handlers) deleteGarden(c *gin.Context) {
 		return
 	}
 	if err := h.svc.DeleteGarden(c.Request.Context(), mustActor(c).ID, id); err != nil {
-		writeResourceError(c, err)
+		writeServiceError(c, err)
 		return
 	}
 	c.Status(http.StatusNoContent)
 }
-
-// parseIDParam reads a positive int64 path parameter, writing a 400 and
-// returning ok=false on a malformed value.
-func parseIDParam(c *gin.Context, name string) (int64, bool) {
-	id, err := strconv.ParseInt(c.Param(name), 10, 64)
-	if err != nil || id < 1 {
-		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid id")
-		return 0, false
-	}
-	return id, true
-}
-
-// writeVersionConflict writes the 409 envelope for an optimistic-concurrency
-// failure: the standard error object plus the current server row under
-// "current", so the client can rebase its edit onto the fresh version and retry.
-// This shape is the contract for every version-guarded (mutable) resource.
-func writeVersionConflict(c *gin.Context, current any) {
-	c.JSON(http.StatusConflict, gin.H{
-		"error":   gin.H{"code": "VERSION_CONFLICT", "message": "the resource was modified; refetch and retry"},
-		"current": current,
-	})
-}
-
-// writeResourceError maps the service-layer sentinel errors shared by every
-// resource to pansy's JSON error envelope. ErrNotFound is used both for a
-// genuinely missing row and for one the actor may not see (existence is masked).
-func writeResourceError(c *gin.Context, err error) {
-	switch {
-	case errors.Is(err, domain.ErrNotFound):
-		writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found")
-	case errors.Is(err, domain.ErrForbidden):
-		writeAPIError(c, http.StatusForbidden, "FORBIDDEN", "you don't have access")
-	case errors.Is(err, domain.ErrInvalidInput):
-		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
-	case errors.Is(err, domain.ErrVersionConflict):
-		// Reached only if a caller forgot to special-case the conflict (which
-		// needs the current row); still return a coherent 409.
-		writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")
-	default:
-		slog.Error("api: unhandled service error", "error", err)
-		writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
-	}
-}

internal/api/gardens_test.go

@@ -162,6 +162,21 @@ func TestGardenCreateValidation(t *testing.T) {
 	}
 }
 
+func TestGardenUpdateRejectsBadVersion(t *testing.T) {
+	r := authEngine(t, localCfg())
+	cookie := registerAndCookie(t, r, "[email protected]")
+	w := doJSON(t, r, http.MethodPost, "/api/v1/gardens", map[string]any{"name": "Yard"}, cookie)
+	id := int64(decodeGarden(t, w.Body.Bytes())["id"].(float64))
+
+	// A negative (or zero) version is invalid input (400), not a version conflict.
+	for _, v := range []int{0, -1} {
+		body := map[string]any{"name": "x", "widthCm": 100, "heightCm": 100, "unitPref": "metric", "version": v}
+		if w := doJSON(t, r, http.MethodPatch, gardenPath(id), body, cookie); w.Code != http.StatusBadRequest {
+			t.Errorf("version %d patch = %d, want 400", v, w.Code)
+		}
+	}
+}
+
 func gardenPath(id int64) string {
 	return "/api/v1/gardens/" + strconv.FormatInt(id, 10)
 }

internal/service/gardens.go

@@ -2,17 +2,22 @@ package service
 
 import (
 	"context"
+	"math"
 	"strings"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
 )
 
-// Garden sizing bounds. A new garden with no dimensions defaults to a 10 m
-// square; the max is a generous sanity cap (100 m) that also guards against
-// absurd or overflow-y values.
+// Garden sizing and field bounds. A new garden with no dimensions defaults to a
+// 10 m square; dimensions must be finite and within [1 cm, 100 m] — a generous
+// sanity range that also rejects NaN/Inf and absurd values. Name/notes are
+// length-capped so untrusted input can't balloon storage.
 const (
-	defaultGardenCM = 1000
-	maxGardenCM     = 100_000
+	defaultGardenCM   = 1000   // 10 m
+	minGardenCM       = 1      // 1 cm
+	maxGardenCM       = 10_000 // 100 m
+	maxGardenNameLen  = 200
+	maxGardenNotesLen = 10_000
 )
 
 // gardenRole ranks a user's access to a garden. Higher includes lower
@@ -118,12 +123,17 @@ func (s *Service) DeleteGarden(ctx context.Context, actorID, gardenID int64) err
 // filled in (create); without it, every field must be supplied (update).
 func gardenFromInput(in GardenInput, applyDefaults bool) (*domain.Garden, error) {
 	name := strings.TrimSpace(in.Name)
-	if name == "" {
+	if name == "" || len(name) > maxGardenNameLen {
+		return nil, domain.ErrInvalidInput
+	}
+	notes := strings.TrimSpace(in.Notes)
+	if len(notes) > maxGardenNotesLen {
 		return nil, domain.ErrInvalidInput
 	}
 
-	// 0 means "unset": defaulted on create, rejected on update. A negative value
-	// is always an explicit error (never silently corrected to the default).
+	// 0 means "unset": defaulted on create, rejected on update. Anything else
+	// must be a finite length in range — this also rejects negatives, NaN, Inf,
+	// and subnormal-tiny values (which are > 0 but below 1 cm).
 	width, height := in.WidthCM, in.HeightCM
 	if applyDefaults {
 		if width == 0 {
@@ -133,7 +143,7 @@ func gardenFromInput(in GardenInput, applyDefaults bool) (*domain.Garden, error)
 			height = defaultGardenCM
 		}
 	}
-	if width <= 0 || height <= 0 || width > maxGardenCM || height > maxGardenCM {
+	if !validDimensionCM(width) || !validDimensionCM(height) {
 		return nil, domain.ErrInvalidInput
 	}
 
@@ -150,6 +160,13 @@ func gardenFromInput(in GardenInput, applyDefaults bool) (*domain.Garden, error)
 		WidthCM:  width,
 		HeightCM: height,
 		UnitPref: unit,
-		Notes:    strings.TrimSpace(in.Notes),
+		Notes:    notes,
 	}, nil
 }
+
+// validDimensionCM reports whether v is a finite length within the allowed
+// garden range. Guards against NaN/Inf (which slip past naive < / > comparisons)
+// and subnormal-tiny positives.
+func validDimensionCM(v float64) bool {
+	return !math.IsNaN(v) && !math.IsInf(v, 0) && v >= minGardenCM && v <= maxGardenCM
+}

internal/service/gardens_test.go

@@ -3,6 +3,8 @@ package service
 import (
 	"context"
 	"errors"
+	"math"
+	"strings"
 	"testing"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
@@ -45,17 +47,27 @@ func TestCreateGardenValidation(t *testing.T) {
 	owner := seedUser(t, s, "[email protected]")
 
 	cases := []GardenInput{
-		{Name: "   "},                           // blank name
-		{Name: "X", UnitPref: "furlongs"},       // bad unit
-		{Name: "X", WidthCM: -5},                // non-positive dim (defaults only fill 0)
-		{Name: "X", WidthCM: maxGardenCM + 1},   // over the cap
-		{Name: "X", HeightCM: maxGardenCM * 10}, // over the cap
+		{Name: "   "}, // blank name
+		{Name: strings.Repeat("a", maxGardenNameLen+1)},              // name too long
+		{Name: "X", Notes: strings.Repeat("b", maxGardenNotesLen+1)}, // notes too long
+		{Name: "X", UnitPref: "furlongs"},                            // bad unit
+		{Name: "X", WidthCM: -5},                                     // negative (defaults only fill exact 0)
+		{Name: "X", WidthCM: maxGardenCM + 1},                        // over the cap
+		{Name: "X", HeightCM: maxGardenCM * 10},                      // over the cap
+		{Name: "X", WidthCM: math.NaN()},                             // NaN slips past naive < / >
+		{Name: "X", HeightCM: math.Inf(1)},                           // +Inf
+		{Name: "X", WidthCM: math.SmallestNonzeroFloat64},            // subnormal, > 0 but < 1 cm
 	}
 	for i, in := range cases {
 		if _, err := s.CreateGarden(context.Background(), owner, in); !errors.Is(err, domain.ErrInvalidInput) {
 			t.Errorf("case %d: err = %v, want ErrInvalidInput", i, err)
 		}
 	}
+
+	// A dimension exactly at the cap is valid.
+	if _, err := s.CreateGarden(context.Background(), owner, GardenInput{Name: "Big", WidthCM: maxGardenCM, HeightCM: maxGardenCM}); err != nil {
+		t.Errorf("dimension at cap should be valid: %v", err)
+	}
 }
 
 func TestListGardensOwnedOnly(t *testing.T) {

internal/store/gardens.go

@@ -23,22 +23,24 @@ func scanGarden(s scanner) (*domain.Garden, error) {
 	return &g, nil
 }
 
+// maxGardensListed caps ListGardensForOwner as a defensive backstop against a
+// pathologically large result. At household scale a user has a handful of
+// gardens, so this is never hit; genuine pagination is post-v1 if ever needed.
+const maxGardensListed = 1000
+
 // CreateGarden inserts a garden (owner_id, name, dimensions, unit, notes already
 // set and validated by the service) and returns the stored row.
 func (d *DB) CreateGarden(ctx context.Context, g *domain.Garden) (*domain.Garden, error) {
-	res, err := d.sql.ExecContext(ctx,
+	created, err := scanGarden(d.sql.QueryRowContext(ctx,
 		`INSERT INTO gardens (owner_id, name, width_cm, height_cm, unit_pref, notes)
-		 VALUES (?, ?, ?, ?, ?, ?)`,
+		 VALUES (?, ?, ?, ?, ?, ?)
+		 RETURNING `+gardenColumns,
 		g.OwnerID, g.Name, g.WidthCM, g.HeightCM, g.UnitPref, g.Notes,
-	)
+	))
 	if err != nil {
 		return nil, fmt.Errorf("store: insert garden: %w", err)
 	}
-	id, err := res.LastInsertId()
-	if err != nil {
-		return nil, fmt.Errorf("store: garden insert id: %w", err)
-	}
-	return d.GetGarden(ctx, id)
+	return created, nil
 }
 
 // GetGarden returns the garden with the given id, or domain.ErrNotFound.
@@ -59,8 +61,8 @@ func (d *DB) GetGarden(ctx context.Context, id int64) (*domain.Garden, error) {
 // added in #16.
 func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.Garden, error) {
 	rows, err := d.sql.QueryContext(ctx,
-		`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC`,
-		ownerID,
+		`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC LIMIT ?`,
+		ownerID, maxGardensListed,
 	)
 	if err != nil {
 		return nil, fmt.Errorf("store: list gardens: %w", err)