Address code review findings across API, web and deployment

High severity: - wire TRUSTED_PROXY through Compose/deploy/docs and warn at web startup - bucket public rate limits on effective client IP, add per-bookmark budget, and forward the signed browser identity from the share page - disable first-run bootstrap in server deployments and document seeding - serialize last-admin demotion in one advisory-locked transaction Medium/low: - cap and batch share recipient inserts - add a shared outbound budget for preview/CardDAV/IMAP/signal work - pin CardDAV dials to 443 and bound IMAP logout - fix rightward kanban column drags and normalize cards on column delete - classify backend outages in auth guards, add request timeouts and an error boundary, remove the dead login route, surface throttling - enforce pinned/unpinned page reorder grouping server-side - avoid preview work for stale bookmark versions; drop duplicate dialog fetch - sequence-guard collection/tag/calendar/quick-find loads - validate property config and fix MULTI_SELECT/DATE filter inputs - document index maintenance; apply same-origin check to sign-out - trim signup names, unique shared dates and kanban keys (migration 010), reset reference cache on identity changes, hide OIDC issuer, return 413 for oversized direct bodies, escape Markdown link destinations - remove dead template files and duplicate test helper Verified with go build/vet, full PostgreSQL race suite, db convergence, tsc, both ESLint configs and a Next production build.
2026-10-20 00:13:30 -05:00
parent 5eab58f99c
commit 4e421f6ea2
58 changed files with 1419 additions and 333 deletions
@@ -0,0 +1,332 @@
+package http
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/cookiejar"
+	"strings"
+	"sync"
+	"testing"
+)
+
+// rawCall issues a request with the client's cookies and explicit headers,
+// without the apiClient helper's automatic status assertion.
+func rawCall(t *testing.T, c apiClient, method, path string, body []byte, headers map[string]string) (int, []byte) {
+	t.Helper()
+	var r io.Reader
+	if body != nil {
+		r = bytes.NewReader(body)
+	}
+	req, err := http.NewRequest(method, c.base+path, r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.Header.Set("Content-Type", "application/json")
+	for k, v := range headers {
+		req.Header.Set(k, v)
+	}
+	res, err := c.c.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	out, _ := io.ReadAll(res.Body)
+	return res.StatusCode, out
+}
+
+// Review H-4: two admins demoting each other must not both pass the
+// "at least one administrator" check. The advisory-locked transaction makes
+// one of them win and the other observe the committed count.
+func TestConcurrentAdminDemotionsKeepOneAdmin(t *testing.T) {
+	srv, d := testServerDB(t)
+	a, _ := accountClient(t, srv.URL, "admin-a@computer.test")
+	b, bID := accountClient(t, srv.URL, "admin-b@computer.test")
+	a.request("PATCH", "/api/admin/users", map[string]any{"userId": bID, "role": "ADMIN"}, 200)
+
+	aID := meID(a)
+	demote := func(c apiClient, target string) int {
+		body, _ := json.Marshal(map[string]any{"userId": target, "role": "USER"})
+		status, _ := rawCall(t, c, "PATCH", "/api/admin/users", body, nil)
+		return status
+	}
+	start := make(chan struct{})
+	var wg sync.WaitGroup
+	statuses := make([]int, 2)
+	wg.Add(2)
+	go func() { defer wg.Done(); <-start; statuses[0] = demote(a, bID) }()
+	go func() { defer wg.Done(); <-start; statuses[1] = demote(b, aID) }()
+	close(start)
+	wg.Wait()
+
+	ok, refused := 0, 0
+	for _, s := range statuses {
+		switch s {
+		case 200:
+			ok++
+		case 400, 403:
+			// 400: still an admin, but the locked count now shows one.
+			// 403: the other demotion committed first and this session is no
+			// longer an admin by the time its request is authorized.
+			refused++
+		default:
+			t.Fatalf("unexpected demotion status: %v", statuses)
+		}
+	}
+	if ok != 1 || refused != 1 {
+		t.Fatalf("wanted one successful and one refused demotion, got %v", statuses)
+	}
+	var admins int
+	if err := d.QueryRow(`SELECT count(*) FROM "User" WHERE role='ADMIN'`).Scan(&admins); err != nil {
+		t.Fatal(err)
+	}
+	if admins != 1 {
+		t.Fatalf("concurrent demotions left %d administrators", admins)
+	}
+}
+
+// Review L-2: a whitespace-only display name is trimmed and falls back to the
+// email local-part instead of being stored verbatim.
+func TestSignupTrimsWhitespaceName(t *testing.T) {
+	srv, _ := testServerDB(t)
+	jar, _ := cookiejar.New(nil)
+	c := apiClient{t, &http.Client{Jar: jar}, srv.URL}
+	body := map[string]any{"email": "trimmed@computer.test", "password": "test-long-password", "name": "   "}
+	c.request("POST", "/api/signup", body, 201)
+	c.request("POST", "/api/auth/login", body, 200)
+	me := c.request("GET", "/api/me", nil, 200)
+	user := me["user"].(map[string]any)
+	if user["name"] != "trimmed" {
+		t.Fatalf("whitespace name was not normalized: %v", user["name"])
+	}
+}
+
+// Review M-1: share recipient arrays are capped before they become one
+// INSERT per id inside a user-locked transaction.
+func TestShareRecipientListsAreCapped(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "cap-owner@computer.test")
+	id := owner.request("POST", "/api/bookmarks", map[string]any{"url": "https://example.com/cap"}, 201)["id"].(string)
+
+	tooMany := make([]string, maxTestShareRecipients+1)
+	for i := range tooMany {
+		tooMany[i] = fmt.Sprintf("user-%d", i)
+	}
+	owner.request("POST", "/api/bookmarks/"+id+"/share", map[string]any{"userIds": tooMany}, 400)
+
+	dates := make([]string, maxTestSharedDateRecipients+1)
+	for i := range dates {
+		dates[i] = fmt.Sprintf("user-%d", i)
+	}
+	owner.request("POST", "/api/shared-dates", map[string]any{"date": "2026-12-01", "recipientIds": dates}, 400)
+}
+
+// Mirrors handlers.maxShareRecipients / maxSharedDateRecipients (unexported).
+const (
+	maxTestShareRecipients      = 100
+	maxTestSharedDateRecipients = 200
+)
+
+// Review L-8: an oversized direct API body is 413, not a misleading 400.
+func TestOversizedDirectBodyReturns413(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "oversized@computer.test")
+	body := []byte(`{"url":"https://example.com","notes":"` + strings.Repeat("a", 1<<20) + `"}`)
+	status, _ := rawCall(t, owner, "POST", "/api/bookmarks", body, nil)
+	if status != http.StatusRequestEntityTooLarge {
+		t.Fatalf("oversized body: wanted 413, got %d", status)
+	}
+}
+
+// Review M-16: sign-out rejects a cross-site Origin like the other auth
+// endpoints, while an idempotent same-origin call still succeeds.
+func TestSignoutRejectsCrossOrigin(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "signout@computer.test")
+	body := []byte(`{}`)
+	status, _ := rawCall(t, owner, "POST", "/api/auth/signout", body, map[string]string{"Origin": "https://attacker.invalid"})
+	if status != http.StatusForbidden {
+		t.Fatalf("cross-origin signout: wanted 403, got %d", status)
+	}
+	status, _ = rawCall(t, owner, "POST", "/api/auth/signout", body, map[string]string{"Origin": srv.URL})
+	if status != http.StatusOK {
+		t.Fatalf("same-origin signout: wanted 200, got %d", status)
+	}
+}
+
+// Review M-9: the server normalizes positions inside the pinned/unpinned
+// groups instead of trusting a client to keep them apart.
+func TestPageReorderKeepsPinnedGrouping(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "reorder@computer.test")
+	pinned := owner.request("POST", "/api/pages", map[string]any{"title": "Pinned", "pinned": true}, 201)["id"].(string)
+	second := owner.request("POST", "/api/pages", map[string]any{"title": "Second"}, 201)["id"].(string)
+	third := owner.request("POST", "/api/pages", map[string]any{"title": "Third"}, 201)["id"].(string)
+
+	// Interleaved request: unpinned pages first, the pinned page last.
+	owner.request("PUT", "/api/pages", map[string]any{"order": []string{third, second, pinned}}, 200)
+
+	res, err := owner.c.Get(srv.URL + "/api/pages")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	var list []map[string]any
+	if err := json.NewDecoder(res.Body).Decode(&list); err != nil {
+		t.Fatal(err)
+	}
+	got := make([]string, 0, len(list))
+	for _, p := range list {
+		got = append(got, p["id"].(string))
+	}
+	want := []string{pinned, third, second}
+	if strings.Join(got, ",") != strings.Join(want, ",") {
+		t.Fatalf("page order after reorder = %v, want %v", got, want)
+	}
+
+	owner.request("PUT", "/api/pages", map[string]any{"order": []string{pinned, second}}, 400)
+}
+
+// Review M-13: non-object / non-empty config on types that take no config is
+// rejected instead of silently discarded.
+func TestPropertyConfigRejectsMalformedValues(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "props@computer.test")
+	cid := owner.request("POST", "/api/collections", map[string]any{"name": "Config"}, 201)["id"].(string)
+	for _, cfg := range []any{"x", []any{}, 5, true} {
+		owner.request("POST", "/api/collections/"+cid+"/properties", map[string]any{"name": "Bad", "type": "TEXT", "config": cfg}, 400)
+	}
+	owner.request("POST", "/api/collections/"+cid+"/properties", map[string]any{"name": "Null", "type": "TEXT", "config": nil}, 201)
+	owner.request("POST", "/api/collections/"+cid+"/properties", map[string]any{"name": "Empty", "type": "TEXT", "config": map[string]any{}}, 201)
+}
+
+// Review L-3: re-sharing the same date updates the surviving row instead of
+// creating duplicate calendar entries.
+func TestSharedDateReshareIsIdempotent(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "dates-owner@computer.test")
+	_, recipientID := accountClient(t, srv.URL, "dates-recipient@computer.test")
+
+	post := func(note string) []map[string]any {
+		body, _ := json.Marshal(map[string]any{"date": "2026-11-05", "note": note, "recipientIds": []string{recipientID}})
+		status, raw := rawCall(t, owner, "POST", "/api/shared-dates", body, nil)
+		if status != 201 {
+			t.Fatalf("shared-date create: %d %s", status, raw)
+		}
+		var rows []map[string]any
+		if err := json.Unmarshal(raw, &rows); err != nil {
+			t.Fatal(err)
+		}
+		return rows
+	}
+	first := post("first")
+	second := post("second")
+	if len(first) != 1 || len(second) != 1 {
+		t.Fatalf("want one returned share, got %d and %d", len(first), len(second))
+	}
+	if second[0]["id"] != first[0]["id"] {
+		t.Fatalf("re-share created a new row: %v vs %v", second[0]["id"], first[0]["id"])
+	}
+	if second[0]["note"] != "second" {
+		t.Fatalf("re-share did not update the note: %v", second[0]["note"])
+	}
+
+	res, err := owner.c.Get(srv.URL + "/api/shared-dates")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	var list []map[string]any
+	if err := json.NewDecoder(res.Body).Decode(&list); err != nil {
+		t.Fatal(err)
+	}
+	count := 0
+	for _, row := range list {
+		date, _ := row["date"].(string)
+		if row["recipientId"] == recipientID && strings.HasPrefix(date, "2026-11-05") {
+			count++
+		}
+	}
+	if count != 1 {
+		t.Fatalf("want one shared-date row, got %d", count)
+	}
+}
+
+// Review M-3: production CardDAV dials are pinned to 443.
+func TestCardDAVRejectsNonStandardPort(t *testing.T) {
+	srv, _ := testServerDB(t)
+	t.Setenv("INTEGRATIONS_ALLOW_PRIVATE", "0")
+	owner, _ := accountClient(t, srv.URL, "carddav@computer.test")
+	v := owner.request("POST", "/api/contacts/sync", map[string]any{
+		"nextcloudUrl": "https://198.51.100.10:8443",
+		"username":     "user",
+		"password":     "secret",
+	}, 400)
+	msg, _ := v["error"].(string)
+	if !strings.Contains(msg, "443") {
+		t.Fatalf("expected a port-443 error, got %q", msg)
+	}
+}
+
+// Review M-5: deleting a column appends its cards to the destination and
+// renumbers positions contiguously, so reload order is deterministic.
+func TestColumnDeleteNormalizesMovedCardPositions(t *testing.T) {
+	srv, _ := testServerDB(t)
+	owner, _ := accountClient(t, srv.URL, "columns@computer.test")
+	var columns []map[string]any
+	res, err := owner.c.Get(srv.URL + "/api/kanban/columns")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := json.NewDecoder(res.Body).Decode(&columns); err != nil {
+		t.Fatal(err)
+	}
+	res.Body.Close()
+	if len(columns) < 2 {
+		t.Fatalf("expected the seeded board, got %d columns", len(columns))
+	}
+	first, second := columns[0]["id"].(string), columns[1]["id"].(string)
+
+	mk := func(column, status, title string) string {
+		return owner.request("POST", "/api/bookmarks", map[string]any{
+			"url": "https://example.com/" + title, "title": title,
+			"kanbanColumnId": column, "kanbanStatus": status,
+		}, 201)["id"].(string)
+	}
+	mk(second, "TODO", "dest-1")
+	mk(second, "TODO", "dest-2")
+	mk(first, "INBOX", "moved-1")
+	mk(first, "INBOX", "moved-2")
+
+	owner.request("DELETE", "/api/kanban/columns/"+first, nil, 200)
+
+	res, err = owner.c.Get(srv.URL + "/api/bookmarks?limit=200")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	var bookmarks []map[string]any
+	if err := json.NewDecoder(res.Body).Decode(&bookmarks); err != nil {
+		t.Fatal(err)
+	}
+	positions := map[float64]bool{}
+	for _, b := range bookmarks {
+		if b["kanbanColumnId"] != second {
+			t.Fatalf("bookmark %v was not moved to the destination", b["title"])
+		}
+		if b["kanbanStatus"] != "TODO" {
+			t.Fatalf("legacy status for %v still points at the deleted column: %v", b["title"], b["kanbanStatus"])
+		}
+		positions[b["kanbanPosition"].(float64)] = true
+	}
+	if len(bookmarks) != 4 || len(positions) != 4 {
+		t.Fatalf("want four contiguously positioned cards, got %d cards at %d positions", len(bookmarks), len(positions))
+	}
+	for i := 0; i < 4; i++ {
+		if !positions[float64(i)] {
+			t.Fatalf("positions are not contiguous 0..3: %v", positions)
+		}
+	}
+}
@@ -142,16 +142,25 @@ func NewServer(cfg *config.Config, database *db.DB) *http.Server {
 	mux.HandleFunc("DELETE /api/collections/{id}/views/{viewId}", authed(handlers.HandleCollectionViewsDelete))

 	// 4.10 Public page (no login)
-	// Review SEC-10: public endpoints get per-IP rate limiting (no auth
-	// context, so no per-user limit). 30 req/min for public bookmark lookup,
-	// 20 req/min for preview (heavier outbound work).
+	// Review SEC-10/H-2: public endpoints get two rate-limit layers keyed on
+	// the EFFECTIVE client identity (signed forwarded browser IP, else the
+	// direct peer), never the shared Next container address:
+	//   - per client: 30 public lookups/min, 20 previews/min;
+	//   - per public bookmark id: 120/min, so one hot shared link cannot
+	//     exhaust the instance-wide budget.
+	// Preview is rate limited AFTER auth: unauthenticated requests must not
+	// consume the bucket and deny previews to signed-in users.
 	publicBookmarkLimiter := auth.NewLimiterStore()
+	publicBookmarkIDLimiter := auth.NewLimiterStore()
 	previewLimiter := auth.NewLimiterStore()
 	publicLimit := auth.Limiter(publicBookmarkLimiter, 30, time.Minute)
+	publicIDLimit := auth.LimiterKeyed(publicBookmarkIDLimiter, 120, time.Minute, func(r *http.Request) string {
+		return "bookmark:" + r.PathValue("id")
+	})
 	previewLimit := auth.Limiter(previewLimiter, 20, time.Minute)
-	mux.HandleFunc("GET /share/bookmark/{id}", publicLimit(handlers.HandlePublicBookmark))
-	mux.HandleFunc("GET /api/public/bookmarks/{id}", publicLimit(handlers.HandlePublicBookmark))
-	mux.HandleFunc("GET /api/preview", previewLimit(authed(handlers.HandlePreview)))
+	mux.HandleFunc("GET /share/bookmark/{id}", publicLimit(publicIDLimit(handlers.HandlePublicBookmark)))
+	mux.HandleFunc("GET /api/public/bookmarks/{id}", publicLimit(publicIDLimit(handlers.HandlePublicBookmark)))
+	mux.HandleFunc("GET /api/preview", authed(previewLimit(handlers.HandlePreview)))

 	return &http.Server{
 		Handler:      withDB(database, mux),