Code-review follow-ups: httputil, validation caps, a11y

Backend: - Extract shared internal/httputil (WriteJSON/ErrorJSON/BadRequest/ ServerError); drop the triple-duplicated helpers in docs, suggestions, vocab. ServerError now logs the real error and returns a generic 500 so raw DB/internal errors never reach the client. - vocab capture: validate doc_id ownership (blank -> none, unknown -> 400 instead of a leaked FK 500); rune-safe clamp word/gloss/definition/ phonetic/example. - vocab review(): wrap the read-modify-write in a transaction (TOCTOU). - /api request-size cap via MaxBytesReader middleware (2 MiB), exempting /api/images (own 10 MiB limit). Frontend: - StatusBar: drive the checking/voicing/collocating indicators from one array; llmDown uses !anyBusy. - Slide-overs: new useFocusTrap hook (focus-in, Tab trap, focus-restore) on GardenPanel + HistoryPanel, both role=dialog/aria-modal/aria-label. - speech.ts: export stopSpeech(); GardenPanel cancels audio on unmount. Tests: add doc_id-validation and field-clamp coverage; full suite green. Claude-Session: https://claude.ai/code/session_016Yr6jELuRc7hyzYLccQKZd
2026-06-26 16:59:23 -07:00
parent 4161830da6
commit 8c6bc1604b
18 changed files with 436 additions and 204 deletions
--- a/internal/vocab/handlers.go
+++ b/internal/vocab/handlers.go
@@ -12,6 +12,7 @@ import (
 	"github.com/go-chi/chi/v5"

 	"gitea.parodia.dev/drwily/petal/internal/db"
+	"gitea.parodia.dev/drwily/petal/internal/httputil"
 )

 // Word is one entry in the vocabulary garden: the looked-up word with its gloss,
@@ -81,7 +82,7 @@ func (h *Handler) due(w http.ResponseWriter, _ *http.Request) {
 func (h *Handler) queryList(w http.ResponseWriter, query string, args ...any) {
 	rows, err := h.DB.Query(query, args...)
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}
 	defer rows.Close()
@@ -89,16 +90,16 @@ func (h *Handler) queryList(w http.ResponseWriter, query string, args ...any) {
 	for rows.Next() {
 		word, err := scanWord(rows)
 		if err != nil {
-			serverError(w, err)
+			httputil.ServerError(w, err)
 			return
 		}
 		out = append(out, word)
 	}
 	if err := rows.Err(); err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}
-	writeJSON(w, http.StatusOK, out)
+	httputil.WriteJSON(w, http.StatusOK, out)
 }

 type captureRequest struct {
@@ -110,6 +111,28 @@ type captureRequest struct {
 	DocID      *string `json:"doc_id"`
 }

+// Field-length caps. The captured fields come from the offline lexicon and the
+// editor selection, not free-typed prose, so we clamp rather than reject — a
+// lookup should never fail because a definition ran long. Counts are runes so a
+// Chinese gloss isn't cut mid-character. The word is the upsert key, so an absurd
+// "word" is bounded too.
+const (
+	maxWordLen       = 128
+	maxGlossLen      = 512
+	maxDefinitionLen = 4096
+	maxPhoneticLen   = 256
+	maxExampleLen    = 4096
+)
+
+// clamp trims s to at most max runes (rune-safe so multibyte glosses survive).
+func clamp(s string, max int) string {
+	r := []rune(s)
+	if len(r) <= max {
+		return s
+	}
+	return string(r[:max])
+}
+
 // capture records a looked-up word. It's an idempotent upsert keyed on the word:
 // a new word lands due tomorrow (interval 1 day); an existing word keeps its
 // schedule untouched but refreshes its gloss/phonetic/example/doc_id so the most
@@ -117,18 +140,47 @@ type captureRequest struct {
 func (h *Handler) capture(w http.ResponseWriter, r *http.Request) {
 	var req captureRequest
 	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
-		errorJSON(w, http.StatusBadRequest, "invalid body")
+		httputil.ErrorJSON(w, http.StatusBadRequest, "invalid body")
 		return
 	}
 	// Normalize the same way the lexicon does (internal/lexicon: lower+trim) so
 	// the UNIQUE(user_id, word) upsert is genuinely idempotent — otherwise
 	// "Apple" at a sentence start and "apple" mid-line would create two separate
 	// cards on independent schedules.
-	word := strings.ToLower(strings.TrimSpace(req.Word))
+	word := clamp(strings.ToLower(strings.TrimSpace(req.Word)), maxWordLen)
 	if word == "" {
-		errorJSON(w, http.StatusBadRequest, "word is required")
+		httputil.ErrorJSON(w, http.StatusBadRequest, "word is required")
 		return
 	}
+	req.Gloss = clamp(req.Gloss, maxGlossLen)
+	req.Definition = clamp(req.Definition, maxDefinitionLen)
+	req.Phonetic = clamp(req.Phonetic, maxPhoneticLen)
+	req.Example = clamp(req.Example, maxExampleLen)
+
+	// A blank doc_id is the same as none; otherwise the word must belong to a
+	// document the user actually owns. Without this check a stale or forged id
+	// would hit the foreign key and leak a raw "FOREIGN KEY constraint" 500
+	// instead of a clean 400 (and, once auth lands, would let a word be attached
+	// to another user's document).
+	if req.DocID != nil {
+		if strings.TrimSpace(*req.DocID) == "" {
+			req.DocID = nil
+		} else {
+			var ok int
+			err := h.DB.QueryRow(
+				`SELECT 1 FROM documents WHERE id = ? AND user_id = ?`,
+				*req.DocID, db.LocalUserID,
+			).Scan(&ok)
+			if errors.Is(err, sql.ErrNoRows) {
+				httputil.ErrorJSON(w, http.StatusBadRequest, "unknown doc_id")
+				return
+			}
+			if err != nil {
+				httputil.ServerError(w, err)
+				return
+			}
+		}
+	}

 	// New rows start due tomorrow; ON CONFLICT refreshes context but leaves the
 	// schedule (due_at/reps/interval/ease) alone so re-looking-up a word never
@@ -145,16 +197,16 @@ func (h *Handler) capture(w http.ResponseWriter, r *http.Request) {
 		db.LocalUserID, word, req.Gloss, req.Definition, req.Phonetic, req.Example, req.DocID,
 	)
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}

 	out, err := h.fetch(word)
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}
-	writeJSON(w, http.StatusCreated, out)
+	httputil.WriteJSON(w, http.StatusCreated, out)
 }

 // fetch loads one word row by its (user, word) key.
@@ -175,25 +227,36 @@ func (h *Handler) review(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 	var req reviewRequest
 	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
-		errorJSON(w, http.StatusBadRequest, "invalid body")
+		httputil.ErrorJSON(w, http.StatusBadRequest, "invalid body")
 		return
 	}
 	if req.Grade != GradeAgain && req.Grade != GradeGood && req.Grade != GradeEasy {
-		errorJSON(w, http.StatusBadRequest, "grade must be again, good, or easy")
+		httputil.ErrorJSON(w, http.StatusBadRequest, "grade must be again, good, or easy")
 		return
 	}

+	// The grade is computed in Go (the SM-2-lite scheduler), so the read and the
+	// write must be one atomic unit: a bare SELECT-then-UPDATE could interleave
+	// with a concurrent review of the same card and lose an update. Wrap both in a
+	// transaction.
+	tx, err := h.DB.Begin()
+	if err != nil {
+		httputil.ServerError(w, err)
+		return
+	}
+	defer tx.Rollback() // no-op once committed
+
 	var cur State
-	err := h.DB.QueryRow(
+	err = tx.QueryRow(
 		`SELECT reps, interval_days, ease, lapses FROM vocab_words WHERE id = ? AND user_id = ?`,
 		id, db.LocalUserID,
 	).Scan(&cur.Reps, &cur.Interval, &cur.Ease, &cur.Lapses)
 	if errors.Is(err, sql.ErrNoRows) {
-		errorJSON(w, http.StatusNotFound, "word not found")
+		httputil.ErrorJSON(w, http.StatusNotFound, "word not found")
 		return
 	}
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}

@@ -201,25 +264,29 @@ func (h *Handler) review(w http.ResponseWriter, r *http.Request) {
 	// `datetime('now', '+N days')` keeps the stored value in SQLite's canonical
 	// text format, matching CURRENT_TIMESTAMP and the due query's comparison.
 	offset := "+" + strconv.Itoa(nxt.Interval) + " days"
-	if _, err := h.DB.Exec(
+	if _, err := tx.Exec(
 		`UPDATE vocab_words SET
 		   reps = ?, interval_days = ?, ease = ?, lapses = ?,
 		   last_reviewed = datetime('now'), due_at = datetime('now', ?)
 		 WHERE id = ? AND user_id = ?`,
 		nxt.Reps, nxt.Interval, nxt.Ease, nxt.Lapses, offset, id, db.LocalUserID,
 	); err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}

-	out, err := scanWord(h.DB.QueryRow(
+	out, err := scanWord(tx.QueryRow(
 		`SELECT `+vocabColumns+` FROM vocab_words WHERE id = ? AND user_id = ?`, id, db.LocalUserID,
 	))
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}
-	writeJSON(w, http.StatusOK, out)
+	if err := tx.Commit(); err != nil {
+		httputil.ServerError(w, err)
+		return
+	}
+	httputil.WriteJSON(w, http.StatusOK, out)
 }

 // remove deletes a word from the garden (e.g. the writer already knows it).
@@ -229,28 +296,12 @@ func (h *Handler) remove(w http.ResponseWriter, r *http.Request) {
 		chi.URLParam(r, "id"), db.LocalUserID,
 	)
 	if err != nil {
-		serverError(w, err)
+		httputil.ServerError(w, err)
 		return
 	}
 	if n, _ := res.RowsAffected(); n == 0 {
-		errorJSON(w, http.StatusNotFound, "word not found")
+		httputil.ErrorJSON(w, http.StatusNotFound, "word not found")
 		return
 	}
 	w.WriteHeader(http.StatusNoContent)
 }
-
-// --- response helpers -------------------------------------------------------
-
-func writeJSON(w http.ResponseWriter, status int, v any) {
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(status)
-	_ = json.NewEncoder(w).Encode(v)
-}
-
-func errorJSON(w http.ResponseWriter, status int, msg string) {
-	writeJSON(w, status, map[string]string{"error": msg})
-}
-
-func serverError(w http.ResponseWriter, err error) {
-	errorJSON(w, http.StatusInternalServerError, err.Error())
-}
--- a/internal/vocab/handlers_test.go
+++ b/internal/vocab/handlers_test.go
@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"path/filepath"
+	"strings"
 	"testing"

 	"github.com/go-chi/chi/v5"
@@ -183,6 +184,42 @@ func TestCaptureCaseInsensitive(t *testing.T) {
 	}
 }

+// TestCaptureUnknownDocID rejects an unowned/stale doc_id with a clean 400
+// rather than letting it hit the foreign key and leak a raw 500.
+func TestCaptureUnknownDocID(t *testing.T) {
+	srv, _ := newTestServer(t)
+	rec := do(t, srv, http.MethodPost, "/vocab",
+		`{"word":"quixotic","gloss":"不切实际的","doc_id":"does-not-exist"}`)
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("unknown doc_id: want 400, got %d body=%s", rec.Code, rec.Body)
+	}
+	// A blank doc_id is treated as none, not an error.
+	if rec := do(t, srv, http.MethodPost, "/vocab", `{"word":"limpid","gloss":"清澈的","doc_id":""}`); rec.Code != http.StatusCreated {
+		t.Fatalf("blank doc_id should be accepted as none: code=%d body=%s", rec.Code, rec.Body)
+	}
+}
+
+// TestCaptureClampsLongFields proves over-long fields are clamped (not rejected)
+// so a lookup never fails on length, and the word key stays bounded.
+func TestCaptureClampsLongFields(t *testing.T) {
+	srv, _ := newTestServer(t)
+	longWord := strings.Repeat("a", maxWordLen+50)
+	longDef := strings.Repeat("x", maxDefinitionLen+50)
+	rec := do(t, srv, http.MethodPost, "/vocab",
+		`{"word":"`+longWord+`","definition":"`+longDef+`"}`)
+	if rec.Code != http.StatusCreated {
+		t.Fatalf("capture: code=%d body=%s", rec.Code, rec.Body)
+	}
+	var w Word
+	_ = json.Unmarshal(rec.Body.Bytes(), &w)
+	if len([]rune(w.Word)) != maxWordLen {
+		t.Fatalf("word should clamp to %d runes, got %d", maxWordLen, len([]rune(w.Word)))
+	}
+	if len([]rune(w.Definition)) != maxDefinitionLen {
+		t.Fatalf("definition should clamp to %d runes, got %d", maxDefinitionLen, len([]rune(w.Definition)))
+	}
+}
+
 // TestDocLinkSurvivesDocDelete proves the ON DELETE SET NULL keeps a word in the
 // garden when its source document is removed.
 func TestDocLinkSurvivesDocDelete(t *testing.T) {