diff --git a/internal/api/auth.go b/internal/api/auth.go index 6372928..1d29303 100644 --- a/internal/api/auth.go +++ b/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") - } -} diff --git a/internal/api/errors.go b/internal/api/errors.go new file mode 100644 index 0000000..eac1f11 --- /dev/null +++ b/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 +} diff --git a/internal/api/gardens.go b/internal/api/gardens.go index 7ef972c..fe9190b 100644 --- a/internal/api/gardens.go +++ b/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") - } -} diff --git a/internal/api/gardens_test.go b/internal/api/gardens_test.go index aedf696..e78e083 100644 --- a/internal/api/gardens_test.go +++ b/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, "a@example.com") + 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) } diff --git a/internal/service/gardens.go b/internal/service/gardens.go index 594dc28..d02e6db 100644 --- a/internal/service/gardens.go +++ b/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 +} diff --git a/internal/service/gardens_test.go b/internal/service/gardens_test.go index 62fabb8..45c0e1a 100644 --- a/internal/service/gardens_test.go +++ b/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, "a@example.com") 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) { diff --git a/internal/store/gardens.go b/internal/store/gardens.go index 6060fac..ac416ab 100644 --- a/internal/store/gardens.go +++ b/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)