diff --git a/.env.example b/.env.example index 13e92c9..8f06373 100644 --- a/.env.example +++ b/.env.example @@ -9,6 +9,22 @@ NEXTAUTH_URL=http://localhost:3000 # COOKIE_SECURE=1 # Reserved administrator emails; provision them using the operator seed in README.md. ADMIN_EMAILS= +# First-run administrator claim. The default (unset) lets the first signup +# become administrator, which is convenient locally and DANGEROUS on a public +# server: whoever registers first after DNS is live owns the instance. +# On an exposed deployment set ALLOW_BOOTSTRAP_SIGNUP=0, list the operator in +# ADMIN_EMAILS, and provision that account with `make seed` (scripts/ +# server-deploy.sh does this automatically for new installs). +#ALLOW_BOOTSTRAP_SIGNUP=0 +# Master switch for self-service registration (default enabled). Set to 0 on +# private instances where accounts are provisioned only by an operator. +#SIGNUP_ENABLED=0 +# Trust the X-Real-IP header for rate-limit identity. Set to 1 ONLY when a +# proxy in front of the web container overwrites X-Real-IP with the real peer +# address (the bundled nginx template does; scripts/server-deploy.sh writes +# this for server deployments). Never set it when port 3000 is reachable +# directly — clients could then choose their own rate-limit bucket. +#TRUSTED_PROXY=1 # Keep these defaults on a new install; change only to reuse an existing volume. POSTGRES_VOLUME=computer_pgdata POSTGRES_VOLUME_EXTERNAL=false diff --git a/API.md b/API.md index 82fbb0b..dad6815 100644 --- a/API.md +++ b/API.md @@ -7,6 +7,10 @@ Successful responses are JSON unless marked Markdown. Errors are `{"error":"message"}`. Data endpoints require authentication: **401** without a valid session, **404** for another user's private record, **403** for non-admin administration, **400** for invalid input, **409** for duplicates. +Request bodies over 1 MiB answer **413**. Rate-limited endpoints answer **429** +with `Retry-After`; public bookmark lookups are limited per effective client +and per bookmark id, and previews per client. Share recipient lists are +capped (100 per bookmark/tag/collection share, 200 per shared date). ## Authentication @@ -17,19 +21,19 @@ non-admin administration, **400** for invalid input, **409** for duplicates. | POST | `/api/signup` | 201 `{ok:true}` — the same uniform body for fresh, duplicate and operator-reserved addresses (review CR-03: no existence oracle); body `{email,password,name?}` | | POST | `/api/auth/login` | `{ok:true,user:{id,name,email,image,role}}` and signed HttpOnly cookie | | GET | `/api/auth/session` | `{user}` or `null` for no valid cookie | -| POST | `/api/auth/signout` | `{ok:true}` and expired cookie | +| POST | `/api/auth/signout` | `{ok:true}` and expired cookie; same-origin `Origin` required when present | | GET | `/api/me` | `{user,effectiveUser,isAdmin,viewingAs}`; real and effective identities | -| GET | `/api/auth/oidc/status` | `{enabled,issuerSet,clientIdSet,clientSecretSet,issuer}`; never includes secrets | +| GET | `/api/auth/oidc/status` | `{enabled,issuerSet,clientIdSet,clientSecretSet}`; never includes secrets or the issuer URL | | GET | `/api/auth/oidc/login` | **501**, client-owned Authelia integration stub | | GET, POST | `/api/auth/callback/authelia` | **501**, client-owned callback stub | The browser's NextAuth `/api/auth/*` endpoints remain on the Next server. Direct Go API consumers use the `computer_session` cookie from login. -Go login accepts JSON and rejects foreign browser origins. The custom Next.js -login endpoint also requires the configured same-origin `Origin` header. -Login and signup are throttled per client IP (429 with `Retry-After`), plus a -consecutive-failure lockout per IP+email on login; loopback clients are -exempt. Browser traffic arrives through the trusted Next proxy, which +Go login accepts JSON and rejects foreign browser origins. Login and signup +are throttled per client IP (429 with `Retry-After`), plus a +consecutive-failure lockout per IP+email on login. Login applies to every +peer, loopback included; signup exempts only a direct loopback peer. Browser +traffic arrives through the trusted Next proxy, which forwards the observed client IP in `X-Computer-Client-Ip` signed with HMAC-SHA256 under the shared secret (`X-Computer-Client-Sig`); Go buckets on that identity only when the signature verifies and falls back to the direct @@ -50,10 +54,12 @@ Bookmark responses are permission-specific. Owners receive camelCase scalar fields plus `tags:[{tag}]`, `contact`, `sharedWith:[{userId,user}]` and `owner` (with email). Authenticated shared readers receive a narrowed allowlist — URL, title/description, notes, preview metadata, `dueDate`, -`isPublic`, `kanbanStatus`, timestamps, `tags` and `owner:{id,name}` — never -the owner's contact record, the recipient list, alerts, mailbox watches or -board placement. Detail responses also include `pageLinks:[{pageId,page}]`, -limited to the effective user's pages. +`isPublic`, timestamps, `tags` and `owner:{id,name}` — never the owner's +contact record, the recipient list, alerts, mailbox watches or board +placement (`kanbanColumnId`/`kanbanPosition`). The legacy `kanbanStatus` +badge IS included and is status, not placement; it is documented here so the +projection and UI agree. Detail responses also include +`pageLinks:[{pageId,page}]`, limited to the effective user's pages. | Method | Path | Returns / behavior | |---|---|---| @@ -231,7 +237,7 @@ already see. | POST | `/api/imap/test` | `{ok:true}` after real connection/login; same mailbox fields and port rules | | GET | `/api/imap/check` | `{matches:[{bookmarkId,title,from,externalId,occurredAt}]}` for legacy watchers; if a search fails after finding matches, the partial `matches` are returned with an `error` field | | GET | `/api/shared-dates` | Created/received dates with `creator` and `recipient` | -| POST | `/api/shared-dates` | 201 array of created shares for `{date,note?,recipientIds:[...]}` | +| POST | `/api/shared-dates` | 201 array of created shares for `{date,note?,recipientIds:[...]}`. Sharing a date also grants read-only access to the creator's bookmarks due that day (including notes); the UI discloses this. Re-sharing the same date updates its note. | | DELETE | `/api/shared-dates/{id}` | `{ok:true}`, creator only | | GET | `/api/users/search?q=...` | Safe user projections; minimum two characters, maximum ten results | diff --git a/RUNNING.md b/RUNNING.md index f085c87..acc46e0 100644 --- a/RUNNING.md +++ b/RUNNING.md @@ -308,6 +308,52 @@ HTTPS reverse proxy to the web app on port 3000, set `NEXTAUTH_URL` to your public HTTPS URL, and set `COOKIE_SECURE=1`. The database remains internal to Docker. +### Rate-limit identity behind a proxy + +Set `TRUSTED_PROXY=1` in `.env` when (and only when) the proxy in front of the +web container **overwrites** `X-Real-IP` with the browser's address — the +bundled nginx template does, and `scripts/server-deploy.sh` writes the flag +for server deployments. The Next server then signs that address and forwards +it to Go, so each browser gets its own login/signup/public-endpoint bucket. +Without it every client shares the web container's bucket (20 logins/minute +for the whole instance). + +Do **not** set `TRUSTED_PROXY=1` while port 3000 is reachable directly: +clients could then choose their own rate-limit identity. The web process logs +a startup warning when `NEXTAUTH_URL` is HTTPS but the flag is missing. + +### First-run administrator on an exposed server + +The default empty `ADMIN_EMAILS` plus bootstrap lets the first signup become +administrator. That is fine locally, but on a public host it is a race with +whoever registers first after DNS goes live. For exposed deployments: + +1. Put `ALLOW_BOOTSTRAP_SIGNUP=0` in `.env` (server-deploy.sh writes it). +2. List the operator in `ADMIN_EMAILS=you@example.com` and restart the app. +3. Provision the account with `SEED_EMAIL=you@example.com SEED_PASSWORD='...' make seed`. + +`SIGNUP_ENABLED=0` additionally closes self-service registration. Public +signup can never grant `ADMIN` after bootstrap is disabled; recovery is +operator-driven. + +### Large-install index maintenance + +Migration `007` builds GIN trigram indexes on `Page(title)` and +`Page(content)`. The migration runner wraps each file in one transaction, so +`CREATE INDEX CONCURRENTLY` is unavailable and the build holds locks that +block writes for its duration. Fresh and small installations are fine; on a +large existing `Page` table, schedule a maintenance window, or pre-build the +indexes yourself before deploying (the migration uses `IF NOT EXISTS` and +becomes a no-op): + +```sql +CREATE EXTENSION IF NOT EXISTS pg_trgm; +CREATE INDEX CONCURRENTLY IF NOT EXISTS "Page_title_trgm_idx" ON "Page" USING gin (title gin_trgm_ops); +CREATE INDEX CONCURRENTLY IF NOT EXISTS "Page_content_trgm_idx" ON "Page" USING gin (content gin_trgm_ops); +``` + +Run those through `make db-shell` (they cannot run inside a transaction). + ### Bundled deployment script The repository ships a target-agnostic deployment for exactly this setup — diff --git a/db/migrations/010_share_uniqueness.sql b/db/migrations/010_share_uniqueness.sql new file mode 100644 index 0000000..ef5dd17 --- /dev/null +++ b/db/migrations/010_share_uniqueness.sql @@ -0,0 +1,32 @@ +-- Review L-3/L-12: enforce the uniqueness constraints the handlers rely on. +-- Both dedupe existing rows first so the migration succeeds on an installed +-- database that already contains duplicates. + +-- One calendar share per (creator, recipient, date). Keep the newest row and +-- collapse the rest; re-sharing now updates the surviving row's note. +DELETE FROM "SharedDate" older +USING "SharedDate" newer +WHERE older."creatorId" = newer."creatorId" + AND older."recipientId" = newer."recipientId" + AND older.date = newer.date + AND (older."createdAt", older.id) < (newer."createdAt", newer.id); + +DROP INDEX IF EXISTS "SharedDate_creatorId_recipientId_date_idx"; +CREATE UNIQUE INDEX "SharedDate_creatorId_recipientId_date_idx" + ON "SharedDate"("creatorId","recipientId",date); + +-- Legacy kanban status keys are unique per user. Keep the key on the +-- earliest column and clear it from duplicates; those columns remain normal +-- (explicitly placed) columns, only the ambiguous legacy mapping goes away. +UPDATE "KanbanColumn" dup +SET "key" = NULL +WHERE dup."key" IS NOT NULL + AND EXISTS ( + SELECT 1 FROM "KanbanColumn" keep + WHERE keep."userId" = dup."userId" + AND keep."key" = dup."key" + AND (keep.position, keep.id) < (dup.position, dup.id) + ); + +CREATE UNIQUE INDEX IF NOT EXISTS "KanbanColumn_userId_key_idx" + ON "KanbanColumn"("userId","key") WHERE "key" IS NOT NULL; diff --git a/db/schema.sql b/db/schema.sql index ba6e325..7c79206 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -183,6 +183,9 @@ CREATE TABLE "KanbanColumn" ( "createdAt" timestamptz NOT NULL DEFAULT now() ); CREATE INDEX "KanbanColumn_userId_position_idx" ON "KanbanColumn"("userId", position); +-- Legacy status keys are unique per user (review L-12): ResolveColumnID picks +-- the first match, so duplicates would make the legacy fallback ambiguous. +CREATE UNIQUE INDEX "KanbanColumn_userId_key_idx" ON "KanbanColumn"("userId", "key") WHERE "key" IS NOT NULL; -- Bookmark.kanbanColumnId -> KanbanColumn (SetNull) ALTER TABLE "Bookmark" ADD CONSTRAINT "Bookmark_kanbanColumnId_fkey" @@ -338,7 +341,9 @@ CREATE TABLE "SharedDate" ( CREATE INDEX "SharedDate_recipientId_idx" ON "SharedDate"("recipientId"); CREATE INDEX "SharedDate_creatorId_idx" ON "SharedDate"("creatorId"); CREATE INDEX "SharedDate_date_idx" ON "SharedDate"(date); -CREATE INDEX "SharedDate_creatorId_recipientId_date_idx" ON "SharedDate"("creatorId","recipientId",date); +-- One row per creator/recipient/date (review L-3): re-sharing the same date +-- updates the note instead of duplicating calendar rows and notifications. +CREATE UNIQUE INDEX "SharedDate_creatorId_recipientId_date_idx" ON "SharedDate"("creatorId","recipientId",date); CREATE INDEX "Bookmark_ownerId_alertAt_unsent_idx" ON "Bookmark"("ownerId","alertAt") WHERE NOT "alertSent"; -- 2.17 Collections (Notion-like upgrades phase 2; see 002_collections.sql for diff --git a/docker-compose.yml b/docker-compose.yml index 9694a15..8981251 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -47,6 +47,10 @@ services: ENCRYPTION_KEY: ${ENCRYPTION_KEY:-} NEXTAUTH_URL: ${NEXTAUTH_URL:-http://localhost:3000} ADMIN_EMAILS: ${ADMIN_EMAILS:-} + # Review H-3: these must reach the Go container, not just .env. An empty + # value keeps local defaults (bootstrap enabled, signup open). + ALLOW_BOOTSTRAP_SIGNUP: ${ALLOW_BOOTSTRAP_SIGNUP:-} + SIGNUP_ENABLED: ${SIGNUP_ENABLED:-} COOKIE_SECURE: ${COOKIE_SECURE:-} OIDC_ISSUER: ${OIDC_ISSUER:-} OIDC_CLIENT_ID: ${OIDC_CLIENT_ID:-} @@ -73,6 +77,11 @@ services: NEXTAUTH_SECRET: ${AUTH_SECRET} NEXTAUTH_URL: ${NEXTAUTH_URL:-http://localhost:3000} AUTH_TRUST_HOST: "true" + # Review H-1: opt in only when a proxy in front of web pins X-Real-IP + # (the shipped nginx template does; scripts/server-deploy.sh sets it). + # Leave unset when port 3000 is browser-reachable directly, otherwise + # clients can spoof their rate-limit identity. + TRUSTED_PROXY: ${TRUSTED_PROXY:-} OIDC_ISSUER: ${OIDC_ISSUER:-} OIDC_CLIENT_ID: ${OIDC_CLIENT_ID:-} OIDC_CLIENT_SECRET: ${OIDC_CLIENT_SECRET:-} diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 9003d56..d43982a 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -40,7 +40,13 @@ func failure(w http.ResponseWriter, status int, s string) { } func decode(w http.ResponseWriter, r *http.Request, v any) bool { r.Body = http.MaxBytesReader(w, r.Body, 1<<20) - if json.NewDecoder(r.Body).Decode(v) != nil { + if err := json.NewDecoder(r.Body).Decode(v); err != nil { + // Review L-8: oversized is 413, not malformed JSON. + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + failure(w, 413, "Request body too large (limit 1 MiB)") + return false + } failure(w, 400, "Invalid JSON") return false } @@ -69,6 +75,9 @@ func HandleSignup(w http.ResponseWriter, r *http.Request) { failure(w, 400, "Invalid email") return } + // Review L-2: a display name of only whitespace is not a name. Trim, then + // fall back to the email local-part when nothing remains. + req.Name = strings.TrimSpace(req.Name) if len([]rune(req.Name)) > maxSignupNameLen { failure(w, 400, "Name must be at most 120 characters") return @@ -144,6 +153,13 @@ func checkAuthRequest(w http.ResponseWriter, r *http.Request) bool { failure(w, 415, "Content-Type must be application/json") return false } + return checkOrigin(w, r) +} + +// checkOrigin rejects a cross-origin Origin when one is present. Extracted +// from checkAuthRequest so the sign-out endpoint can share the same guard +// without forcing a JSON content type on legacy cookie clients (review M-16). +func checkOrigin(w http.ResponseWriter, r *http.Request) bool { if origin := r.Header.Get("Origin"); origin != "" { allowed, err := url.Parse(config.Load().NextAuthURL) scheme := "http" @@ -236,6 +252,12 @@ func HandleAuth(w http.ResponseWriter, r *http.Request) { failure(w, 405, "Use POST to sign out") return } + // Review M-16: keep sign-out consistent with the rest of the auth + // surface. A browser cross-site POST is rejected; idempotent calls + // for an already-invalid session still succeed below. + if !checkOrigin(w, r) { + return + } // Revoke every cookie JWT issued for this user, not just this // browser's copy: signout means "sign out everywhere". if su, e := ReadSession(r, config.Load().Secret()); e == nil { diff --git a/internal/auth/ratelimit.go b/internal/auth/ratelimit.go index 71e08d9..31f462d 100644 --- a/internal/auth/ratelimit.go +++ b/internal/auth/ratelimit.go @@ -35,13 +35,13 @@ import ( ) const ( - loginIPLimit = 20 // logins per IP per window - loginIPWindow = time.Minute // - signupIPLimit = 10 // signups per IP per window - signupIPWindow = time.Hour // - failureThreshold = 5 // consecutive failures before lockout - failureLockout = 15 * time.Minute - failureWindow = time.Hour // failures older than this are forgotten + loginIPLimit = 20 // logins per IP per window + loginIPWindow = time.Minute // + signupIPLimit = 10 // signups per IP per window + signupIPWindow = time.Hour // + failureThreshold = 5 // consecutive failures before lockout + failureLockout = 15 * time.Minute + failureWindow = time.Hour // failures older than this are forgotten // limiterMaxEntries is the soft cap per store. Review SEC-03: the failure // tracker previously never evicted sub-threshold entries (zero until), so @@ -169,9 +169,6 @@ func ResetRateLimits() { } } -// resetRateLimits clears all limiter state. Tests only. -func resetRateLimits() { ResetRateLimits() } - func clientIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { @@ -285,14 +282,23 @@ func RateLimitSignup(next http.HandlerFunc) http.HandlerFunc { // callers (e.g. public endpoints) can have independent buckets. func NewLimiterStore() *limiterStore { return newLimiterStore() } -// Limiter wraps a handler with a simple per-IP fixed-window rate limit. -// Uses the direct peer IP (no forwarded headers) and does not exempt -// loopback — public endpoints apply to every peer. +// Limiter wraps a handler with a simple per-client fixed-window rate limit. +// Review H-2: this buckets on EffectiveClientIP (the signed forwarded browser +// identity when the trusted Next server attested one), not the raw direct +// peer. Behind the Next proxy every public request otherwise shared one +// instance-wide bucket, so a single caller could deny the endpoint to all +// users. Loopback is not exempt — public endpoints apply to every peer. func Limiter(s *limiterStore, limit int, window time.Duration) func(http.HandlerFunc) http.HandlerFunc { + return LimiterKeyed(s, limit, window, EffectiveClientIP) +} + +// LimiterKeyed is Limiter with a caller-supplied bucket key. It backs the +// second public-share layer (review H-2): one hot bookmark id gets its own +// generous budget in addition to the per-client one. +func LimiterKeyed(s *limiterStore, limit int, window time.Duration, key func(*http.Request) string) func(http.HandlerFunc) http.HandlerFunc { return func(next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - ip := clientIP(r) - if ok, retry := s.allow(ip, limit, window, time.Now()); !ok { + if ok, retry := s.allow(key(r), limit, window, time.Now()); !ok { tooManyRequests(w, retry) return } diff --git a/internal/auth/ratelimit_test.go b/internal/auth/ratelimit_test.go index 884b98d..6df3402 100644 --- a/internal/auth/ratelimit_test.go +++ b/internal/auth/ratelimit_test.go @@ -93,8 +93,8 @@ func TestFailureTrackerEviction(t *testing.T) { func dummyHandler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(204) } func TestRateLimitMiddleware(t *testing.T) { - resetRateLimits() - t.Cleanup(resetRateLimits) + ResetRateLimits() + t.Cleanup(ResetRateLimits) // Review SEC-1: loopback is NOT exempt on the login endpoint — same-host // deployments must keep throttling for real browser logins. @@ -109,7 +109,7 @@ func TestRateLimitMiddleware(t *testing.T) { t.Fatalf("loopback login exempt: got %d", loopLast.Code) } // Signup keeps the loopback exemption for host-local consumers. - resetRateLimits() + ResetRateLimits() var loopSignup *httptest.ResponseRecorder for i := 0; i < signupIPLimit*2; i++ { loopSignup = httptest.NewRecorder() @@ -118,7 +118,7 @@ func TestRateLimitMiddleware(t *testing.T) { if loopSignup.Code != 204 { t.Fatalf("loopback signup limited with %d", loopSignup.Code) } - resetRateLimits() + ResetRateLimits() // Non-loopback peers get 429 after the window limit, with Retry-After. remote := httptest.NewRequest("POST", "/api/auth/login", nil) @@ -136,7 +136,7 @@ func TestRateLimitMiddleware(t *testing.T) { } // Signup limiter counts independently. - resetRateLimits() + ResetRateLimits() var signupLast *httptest.ResponseRecorder for i := 0; i < signupIPLimit+1; i++ { signupLast = httptest.NewRecorder() @@ -207,10 +207,48 @@ func TestSignedForwardedClientIP(t *testing.T) { } } +// Review H-2: public endpoints must bucket on the signed forwarded browser +// identity too, not the shared Next container peer. +func TestPublicLimiterUsesForwardedIP(t *testing.T) { + t.Setenv("NEXTAUTH_SECRET", "") + t.Setenv("AUTH_SECRET", "public-limiter-forward-secret") + ts := time.Now().Unix() + sign := func(ip string) (string, string) { + mac := hmac.New(sha256.New, []byte("public-limiter-forward-secret")) + mac.Write([]byte(ip + "|" + strconv.FormatInt(ts, 10))) + return strconv.FormatInt(ts, 10), hex.EncodeToString(mac.Sum(nil)) + } + handler := Limiter(NewLimiterStore(), 2, time.Minute)(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + do := func(ip string) int { + r := httptest.NewRequest("GET", "/api/public/bookmarks/x", nil) + r.RemoteAddr = "10.0.0.5:1234" + hts, sig := sign(ip) + r.Header.Set(headerClientIP, ip) + r.Header.Set(headerClientTs, hts) + r.Header.Set(headerClientSig, sig) + w := httptest.NewRecorder() + handler(w, r) + return w.Code + } + for i := 0; i < 2; i++ { + if code := do("198.51.100.1"); code != http.StatusOK { + t.Fatalf("browser 1 request %d: %d", i+1, code) + } + } + if code := do("198.51.100.1"); code != http.StatusTooManyRequests { + t.Fatalf("browser 1 over limit: %d", code) + } + if code := do("198.51.100.2"); code != http.StatusOK { + t.Fatalf("browser 2 blocked by browser 1: %d", code) + } +} + func TestForwardedIPsGetSeparateBuckets(t *testing.T) { t.Setenv("NEXTAUTH_SECRET", "") t.Setenv("AUTH_SECRET", "ratelimit-forward-secret") - resetRateLimits() + ResetRateLimits() ts := time.Now().Unix() sign := func(ip string) (string, string) { mac := hmac.New(sha256.New, []byte("ratelimit-forward-secret")) @@ -241,5 +279,5 @@ func TestForwardedIPsGetSeparateBuckets(t *testing.T) { if code := do("198.51.100.2"); code != 200 { t.Fatalf("browser 2 blocked by browser 1: %d", code) } - resetRateLimits() + ResetRateLimits() } diff --git a/internal/authguard/handlers.go b/internal/authguard/handlers.go index 0a27372..85f3d18 100644 --- a/internal/authguard/handlers.go +++ b/internal/authguard/handlers.go @@ -1,7 +1,9 @@ package authguard import ( + "database/sql" "encoding/json" + "errors" "github.com/andrew/go-computer/internal/auth" "github.com/andrew/go-computer/internal/config" "github.com/andrew/go-computer/internal/db" @@ -11,6 +13,11 @@ import ( "time" ) +// errLastAdmin is the transactional refusal to remove the final +// administrator (review H-4). It is raised inside the locked transaction so +// the count and the update cannot be interleaved by a concurrent demotion. +var errLastAdmin = errors.New("at least one administrator must remain") + func HandleAdminListUsers(w http.ResponseWriter, r *http.Request) { // COALESCE(email,'') (review COR-8): NOT LIKE evaluates to NULL for the // NULL emails OIDC-sourced accounts can carry, which hid them from the @@ -43,21 +50,46 @@ func HandleAdminPatchUser(w http.ResponseWriter, r *http.Request) { handlers.WriteJSON(w, 404, handlers.Error{Error: "Not found"}) return } - // Review SEC-11: an installation must keep at least one administrator. + // Review SEC-11/H-4: an installation must keep at least one administrator. // Self-demotion is already refused; this closes the "demote the last - // other admin" path. + // other admin" path. The role re-read, count and update all run in ONE + // transaction serialized by an advisory lock, so two admins demoting + // each other concurrently can no longer both observe admins == 2 and + // leave zero. The pre-transaction role read above only picks the audit + // message; the decision uses the locked value. if b.Role == "USER" && target.Role == "ADMIN" { - var admins int - if e := d.QueryRowContext(r.Context(), `SELECT count(*) FROM "User" WHERE role='ADMIN' AND COALESCE(email,'') NOT LIKE '%@example.com'`).Scan(&admins); e != nil { - handlers.WriteJSON(w, 500, handlers.Error{Error: "Unable to change role"}) - return - } - if admins <= 1 { - handlers.WriteJSON(w, 400, handlers.Error{Error: "At least one administrator must remain"}) - return - } + e = db.Transaction(r.Context(), d, func(tx *sql.Tx) error { + if _, e := tx.ExecContext(r.Context(), `SELECT pg_advisory_xact_lock(927439)`); e != nil { + return e + } + var current string + if e := tx.QueryRowContext(r.Context(), `SELECT role FROM "User" WHERE id=$1`, b.UserID).Scan(¤t); e != nil { + return e + } + if current == "ADMIN" { + var admins int + if e := tx.QueryRowContext(r.Context(), `SELECT count(*) FROM "User" WHERE role='ADMIN' AND COALESCE(email,'') NOT LIKE '%@example.com'`).Scan(&admins); e != nil { + return e + } + if admins <= 1 { + return errLastAdmin + } + } + _, e := tx.ExecContext(r.Context(), `UPDATE "User" SET role=$1,"updatedAt"=now() WHERE id=$2`, b.Role, b.UserID) + return e + }) + } else { + e = repo.SetRole(r.Context(), b.UserID, b.Role) } - if e := repo.SetRole(r.Context(), b.UserID, b.Role); e != nil { + if errors.Is(e, sql.ErrNoRows) { + handlers.WriteJSON(w, 404, handlers.Error{Error: "Not found"}) + return + } + if errors.Is(e, errLastAdmin) { + handlers.WriteJSON(w, 400, handlers.Error{Error: "At least one administrator must remain"}) + return + } + if e != nil { handlers.WriteJSON(w, 500, handlers.Error{Error: "Unable to change role"}) return } diff --git a/internal/config/config.go b/internal/config/config.go index ed2487b..f3c70d6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,9 +1,11 @@ package config import ( + "fmt" "net/url" "os" "strings" + "sync" ) // DBURL returns the database connection string, preferring DATABASE_URL and @@ -76,11 +78,23 @@ func (c *Config) CookieSecure() bool { return err == nil && u.Scheme == "https" } +// adminEmailsCache memoizes the parsed ADMIN_EMAILS value by its raw env +// string (review M-12: IsAdminEmail runs on the auth hot path and re-split +// and re-lowercased the list on every call). Keying on the raw value keeps +// t.Setenv-based tests correct without a process-global startup snapshot. +var adminEmailsCache struct { + mu sync.Mutex + raw string + out []string +} + // AdminEmails splits env ADMIN_EMAILS on commas, trims, lowercases. func AdminEmails() []string { s := os.Getenv("ADMIN_EMAILS") - if s == "" { - return nil + adminEmailsCache.mu.Lock() + defer adminEmailsCache.mu.Unlock() + if adminEmailsCache.raw == s { + return adminEmailsCache.out } var out []string for _, email := range strings.Split(s, ",") { @@ -88,6 +102,7 @@ func AdminEmails() []string { out = append(out, email) } } + adminEmailsCache.raw, adminEmailsCache.out = s, out return out } @@ -111,3 +126,18 @@ func IsAdminEmail(email string) bool { func AllowBootstrapSignup() bool { return os.Getenv("ALLOW_BOOTSTRAP_SIGNUP") != "0" } + +// Validate fails fast on configuration the app cannot use, instead of +// discovering it on the first request (review M-12). +func (c *Config) Validate() error { + if strings.TrimSpace(c.Secret()) == "" { + return fmt.Errorf("AUTH_SECRET (or NEXTAUTH_SECRET) must be set") + } + if c.NextAuthURL != "" { + u, err := url.Parse(c.NextAuthURL) + if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { + return fmt.Errorf("NEXTAUTH_URL must be an absolute http(s) URL, got %q", c.NextAuthURL) + } + } + return nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7fe484d..380ae71 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -35,6 +35,37 @@ func TestSecretPrecedence(t *testing.T) { } } +// Review M-12: fail fast on unusable configuration at startup. +func TestValidate(t *testing.T) { + if err := (&Config{}).Validate(); err == nil { + t.Fatal("missing secret passed validation") + } + if err := (&Config{AuthSecret: "secret", NextAuthURL: "not a url"}).Validate(); err == nil { + t.Fatal("relative NEXTAUTH_URL passed validation") + } + for _, u := range []string{"", "http://localhost:3000", "https://app.example.com"} { + if err := (&Config{AuthSecret: "secret", NextAuthURL: u}).Validate(); err != nil { + t.Fatalf("valid configuration %q rejected: %v", u, err) + } + } +} + +// Review M-12: the parsed ADMIN_EMAILS list is memoized but still follows +// env changes (tests and operator restarts rely on that). +func TestAdminEmailsTracksEnv(t *testing.T) { + t.Setenv("ADMIN_EMAILS", " A@Example.com ,, b@example.com ") + if got := AdminEmails(); len(got) != 2 || got[0] != "a@example.com" || got[1] != "b@example.com" { + t.Fatalf("unexpected parse: %v", got) + } + t.Setenv("ADMIN_EMAILS", "") + if got := AdminEmails(); len(got) != 0 { + t.Fatalf("cache did not follow the env change: %v", got) + } + if IsAdminEmail("a@example.com") { + t.Fatal("stale admin email after env change") + } +} + // Review OPS-01: a dedicated ENCRYPTION_KEY decouples integration-credential // encryption from the session signing secret. func TestEncryptionSecretPrecedence(t *testing.T) { diff --git a/internal/db/bookmarks.go b/internal/db/bookmarks.go index a19bbfe..6bd961a 100644 --- a/internal/db/bookmarks.go +++ b/internal/db/bookmarks.go @@ -23,7 +23,10 @@ const BookmarkOwnerView = `to_jsonb(b) || jsonb_build_object( // readers: the link itself, its presentation metadata, the content the owner // attached for readers (description, notes, due date, tags), and enough owner // identity to attribute the share inside the workspace. No contact record, no -// recipient list, no alerts, no mailbox-watch settings, no board placement. +// recipient list, no alerts, no mailbox-watch settings, no board placement +// (kanbanColumnId/kanbanPosition). The legacy kanbanStatus badge IS included +// deliberately: it is status, not placement, and API.md/web/AGENTS.md say so +// (review L-10). const BookmarkSharedView = `jsonb_build_object( 'id',b.id,'url',b.url,'title',b.title,'description',b.description,'notes',b.notes, 'favicon',b.favicon,'ogImage',b."ogImage",'ogTitle',b."ogTitle",'ogDescription',b."ogDescription", diff --git a/internal/handlers/bookmarks.go b/internal/handlers/bookmarks.go index ff8ec90..9911539 100644 --- a/internal/handlers/bookmarks.go +++ b/internal/handlers/bookmarks.go @@ -318,6 +318,14 @@ func HandleBookmarksUpdate(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, e) return } + // Review M-10: reject a stale base version BEFORE doing outbound preview + // work. The transaction below still re-checks under the user lock; this + // early check only avoids spending up to 8 seconds and the shared preview + // budget on a request that can only end in 409. + if stored, _ := existing["version"].(float64); int(stored) != baseVersion { + respond(w, 0, nil, conflict("Bookmark was modified elsewhere; reload to see the latest version")) + return + } if s := str(d, "url"); s != "" && s != str(existing, "url") { addPreview(r.Context(), previewKey(r), d, false) } @@ -400,7 +408,7 @@ func HandleBookmarksShare(w http.ResponseWriter, r *http.Request) { if !ok { return } - list, e := ids(b, "userIds") + list, e := cappedIDs(b, "userIds", maxShareRecipients) if e != nil { respond(w, 0, nil, e) return diff --git a/internal/handlers/collections.go b/internal/handlers/collections.go index 9340075..7038d80 100644 --- a/internal/handlers/collections.go +++ b/internal/handlers/collections.go @@ -306,7 +306,7 @@ func HandleCollectionShare(w http.ResponseWriter, r *http.Request) { if !ok { return } - list, e := ids(b, "userIds") + list, e := cappedIDs(b, "userIds", maxShareRecipients) if e != nil { respond(w, 0, nil, e) return @@ -431,7 +431,18 @@ func validatePropertyConfig(typ string, raw any, existing []object) (object, []s } return object{"mode": mode}, nil, nil default: - if m, ok := raw.(object); ok && raw != nil && len(m) > 0 { + // Review M-13: these types take no configuration. Accept null or an + // empty object and reject every other value — silently replacing a + // malformed config with {} told the client it had configured + // something it had not. + if raw == nil { + return object{}, nil, nil + } + m, ok := raw.(object) + if !ok { + return nil, nil, bad("config must be an object") + } + if len(m) > 0 { return nil, nil, bad(typ + " properties take no config") } return object{}, nil, nil diff --git a/internal/handlers/common.go b/internal/handlers/common.go index cecea8c..18a5e9d 100644 --- a/internal/handlers/common.go +++ b/internal/handlers/common.go @@ -13,6 +13,7 @@ import ( "net/http" "net/url" "regexp" + "strconv" "strings" "time" "unicode/utf8" @@ -36,7 +37,19 @@ func read(w http.ResponseWriter, r *http.Request) (object, bool) { r.Body = http.MaxBytesReader(w, r.Body, 1<<20) var b object dec := json.NewDecoder(r.Body) - if dec.Decode(&b) != nil || b == nil { + if err := dec.Decode(&b); err != nil { + // Review L-8: an oversized body is 413, not malformed JSON. The Next + // proxy already answers 413; direct API callers deserve the same + // class of error. + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + writeJSON(w, 413, Error{"Request body too large (limit 1 MiB)"}) + return nil, false + } + writeJSON(w, 400, Error{"Invalid JSON object"}) + return nil, false + } + if b == nil { writeJSON(w, 400, Error{"Invalid JSON object"}) return nil, false } @@ -98,6 +111,27 @@ func ids(b object, k string) ([]string, error) { } return out, nil } + +// Share-list caps (review M-1). A 1 MiB JSON body can carry tens of +// thousands of ids; each one used to become an INSERT in a user-locked +// transaction. These are product limits, not storage limits. +const ( + maxShareRecipients = 100 + maxSharedDateRecipients = 200 +) + +// cappedIDs is ids plus a length cap (review M-1), returning a clear 400. +func cappedIDs(b object, key string, max int) ([]string, error) { + list, e := ids(b, key) + if e != nil { + return nil, e + } + if len(list) > max { + return nil, bad(fmt.Sprintf("%s may contain at most %d entries", key, max)) + } + return list, nil +} + func fields(b object, allowed string) object { out := object{} for _, k := range strings.Fields(allowed) { @@ -254,11 +288,23 @@ func reorder(r *http.Request, table, col string, order []string) error { return nil }) } -func shareRows(r *http.Request, tx *sql.Tx, table, key, id string, list []string) error { - for _, u := range list { - if _, e := tx.ExecContext(r.Context(), `INSERT INTO `+db.Quote(table)+` (`+db.Quote(key)+`,"userId") VALUES($1,$2) ON CONFLICT DO NOTHING`, id, u); e != nil { - return e - } + +// outboundBusy answers 429 with Retry-After when the shared outbound budget +// is exhausted (review M-2). +func outboundBusy(w http.ResponseWriter, retry time.Duration) { + if retry < time.Second { + retry = time.Second } - return nil + w.Header().Set("Retry-After", strconv.Itoa(int(retry.Seconds())+1)) + writeJSON(w, 429, Error{"Too much outbound work right now; try again shortly"}) +} + +// shareRows inserts every recipient in ONE statement (review M-1). table and +// key come only from the server-side call sites, never from request input. +func shareRows(r *http.Request, tx *sql.Tx, table, key, id string, list []string) error { + if len(list) == 0 { + return nil + } + _, e := tx.ExecContext(r.Context(), `INSERT INTO `+db.Quote(table)+` (`+db.Quote(key)+`,"userId") SELECT $1,unnest($2::text[]) ON CONFLICT DO NOTHING`, id, list) + return e } diff --git a/internal/handlers/kanban.go b/internal/handlers/kanban.go index 769cbd7..ebb9a0d 100644 --- a/internal/handlers/kanban.go +++ b/internal/handlers/kanban.go @@ -230,16 +230,32 @@ func HandleKanbanColumnsDelete(w http.ResponseWriter, r *http.Request) { return bad("A board needs at least one column") } dest = str(remaining[0], "id") + // The destination column's legacy key, when it has one, becomes the + // moved cards' kanbanStatus so the two representations do not point + // at the deleted column (review M-5/L-12). Custom columns have no + // key; their cards keep the legacy status as a documented fallback. + destKey := "" + for _, c := range remaining { + if str(c, "id") == dest { + if k := str(c, "key"); validStatus(k) { + destKey = k + } + } + } // Only the three columns ResolveColumnID needs, and one set-based // UPDATE for the whole move instead of a round trip per bookmark. - bms, e := db.Many(r.Context(), tx, `SELECT jsonb_build_object('id',b.id,'kanbanColumnId',b."kanbanColumnId",'kanbanStatus',b."kanbanStatus") FROM "Bookmark" b WHERE "ownerId"=$1`, user(r)) + // Positions are normalized below too (review M-5), so reload order is + // deterministic instead of interleaving with the destination's cards. + bms, e := db.Many(r.Context(), tx, `SELECT jsonb_build_object('id',b.id,'kanbanColumnId',b."kanbanColumnId",'kanbanStatus',b."kanbanStatus") FROM "Bookmark" b WHERE "ownerId"=$1 ORDER BY b."kanbanPosition",b."updatedAt" DESC,b.id DESC`, user(r)) if e != nil { return e } moveIDs := make([]string, 0, len(bms)) + moving := map[string]bool{} for _, b := range bms { if kanban.ResolveColumnID(b, cols) == id { moveIDs = append(moveIDs, str(b, "id")) + moving[str(b, "id")] = true } } if len(moveIDs) > 0 { @@ -248,7 +264,20 @@ func HandleKanbanColumnsDelete(w http.ResponseWriter, r *http.Request) { // snapshot that still shows the old column into a 409 on its // next save instead of letting it silently move the bookmark // back. - if _, e := tx.ExecContext(r.Context(), `UPDATE "Bookmark" SET "kanbanColumnId"=$1,version=version+1,"updatedAt"=now() WHERE id=ANY($2::text[])`, dest, moveIDs); e != nil { + if _, e := tx.ExecContext(r.Context(), `UPDATE "Bookmark" SET "kanbanColumnId"=$1,"kanbanStatus"=CASE WHEN $4='' THEN "kanbanStatus" ELSE $4::"KanbanStatus" END,version=version+1,"updatedAt"=now() WHERE id=ANY($2::text[]) AND "ownerId"=$3`, dest, moveIDs, user(r), destKey); e != nil { + return e + } + // Append the moved cards after the destination's existing order + // and renumber contiguously, exactly like a normal card move. + order := make([]string, 0, len(bms)) + for _, b := range bms { + bid := str(b, "id") + if !moving[bid] && kanban.ResolveColumnID(b, remaining) == dest { + order = append(order, bid) + } + } + order = append(order, moveIDs...) + if _, e := tx.ExecContext(r.Context(), `UPDATE "Bookmark" AS b SET "kanbanPosition"=t.ord-1 FROM unnest($1::text[]) WITH ORDINALITY AS t(bid,ord) WHERE b.id=t.bid AND b."ownerId"=$2`, order, user(r)); e != nil { return e } moved = len(moveIDs) diff --git a/internal/handlers/misc.go b/internal/handlers/misc.go index 53f4a06..73c0001 100644 --- a/internal/handlers/misc.go +++ b/internal/handlers/misc.go @@ -9,8 +9,10 @@ import ( crypt "github.com/andrew/go-computer/internal/crypto" "github.com/andrew/go-computer/internal/db" "github.com/andrew/go-computer/internal/netguard" + "github.com/andrew/go-computer/internal/outbound" "github.com/andrew/go-computer/internal/signals" "io" + "net" "net/http" "net/url" "strconv" @@ -18,12 +20,24 @@ import ( "time" ) +// carddavDial pins production CardDAV traffic to 443 (review M-3), matching +// previews (80/443) and IMAP (143/993). Without it a user could make the +// server TLS-handshake and send Basic-auth credentials to any public port. +// The fixture/LAN escape hatch keeps loopback fakes and deliberate private +// deployments working. +func carddavDial(ctx context.Context, network, address string) (net.Conn, error) { + if netguard.AllowPrivate() { + return netguard.DialContext(ctx, network, address) + } + return netguard.DialContextPorts(ctx, network, address, 443) +} + // carddavClient dials through netguard so a user-supplied Nextcloud URL can // never point the server at internal infrastructure (same guard as URL // previews). The transport is shared to avoid leaking idle sockets per sync. var carddavClient = &http.Client{ Timeout: 20 * time.Second, - Transport: &http.Transport{DialContext: netguard.DialContext}, + Transport: &http.Transport{DialContext: carddavDial}, CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }, @@ -65,7 +79,7 @@ func HandleSharedDatesCreate(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, bad("Date required")) return } - list, e := ids(b, "recipientIds") + list, e := cappedIDs(b, "recipientIds", maxSharedDateRecipients) if e != nil || len(list) == 0 { respond(w, 0, nil, bad("Recipients required")) return @@ -81,7 +95,11 @@ func HandleSharedDatesCreate(w http.ResponseWriter, r *http.Request) { out := []object{} e = db.Transaction(r.Context(), database(r), func(tx *sql.Tx) error { for _, id := range list { - v, e := db.Insert(r.Context(), tx, "SharedDate", object{"id": db.NewID(), "date": dt, "creatorId": user(r), "recipientId": id, "note": b["note"]}) + // Review L-3: re-sharing the same creator/recipient/date updates + // the note instead of inserting a duplicate calendar row. + v, e := db.One(r.Context(), tx, `INSERT INTO "SharedDate"(id,date,note,"creatorId","recipientId") VALUES($1,$2,$3,$4,$5) + ON CONFLICT ("creatorId","recipientId",date) DO UPDATE SET note=EXCLUDED.note + RETURNING to_jsonb("SharedDate")`, db.NewID(), dt, b["note"], user(r), id) if e != nil { return e } @@ -149,6 +167,13 @@ func HandleContactsSync(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, bad("Nextcloud URL must use https")) return } + // Review M-3: the transport pins production dials to 443; reject an + // explicit non-standard port up front for a clear error instead of a + // generic connection failure. + if !netguard.AllowPrivate() && base.Port() != "" && base.Port() != "443" { + respond(w, 0, nil, bad("Nextcloud URL must use port 443")) + return + } path := str(b, "cardDavPath") if path == "" { path = "/remote.php/dav/addressbooks/users/" + url.PathEscape(str(b, "username")) + "/contacts/" @@ -158,6 +183,15 @@ func HandleContactsSync(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, e) return } + // Review M-2: CardDAV sync draws from the shared outbound budget with + // IMAP/preview/signal checks, so one user cannot open unbounded + // long-lived connections or starve egress. + release, retry, ok := outbound.Reserve("carddav:"+user(r), 10, time.Minute) + if !ok { + outboundBusy(w, retry) + return + } + defer release() payload := `` req, e := http.NewRequestWithContext(r.Context(), "REPORT", target.String(), strings.NewReader(payload)) if e != nil { @@ -493,11 +527,20 @@ func HandleImapTest(w http.ResponseWriter, r *http.Request) { } m.Password = saved.Password } + release, retry, ok := outbound.Reserve("imap:"+user(r), 10, time.Minute) + if !ok { + outboundBusy(w, retry) + return + } + defer release() c, e := signals.Connect(r.Context(), m) if e != nil { respond(w, 0, nil, bad("Mailbox connection or login failed")) return } + // Review M-2: a hostile server must not hold the handler open during the + // goodbye exchange after the test has already succeeded. + c.Timeout = 5 * time.Second c.Logout() respond(w, 200, OK{true}, nil) } @@ -523,6 +566,12 @@ func HandleImapCheck(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, bad("Configure IMAP first")) return } + release, retry, ok := outbound.Reserve("imap:"+user(r), 10, time.Minute) + if !ok { + outboundBusy(w, retry) + return + } + defer release() ctx, cancel := context.WithTimeout(r.Context(), 45*time.Second) defer cancel() c, e := signals.Connect(ctx, m) @@ -532,7 +581,9 @@ func HandleImapCheck(w http.ResponseWriter, r *http.Request) { } stop := context.AfterFunc(ctx, func() { c.Terminate() }) defer stop() - defer c.Logout() + // Keep the goodbye exchange bounded too (review M-2); searches above + // stay under the caller's 45-second context. + defer func() { c.Timeout = 5 * time.Second; c.Logout() }() searchFailed := false for _, b := range watched { events, e := signals.SearchConn(c, m, "", str(b, "imapQuery")) diff --git a/internal/handlers/pages.go b/internal/handlers/pages.go index 290e16b..8872a19 100644 --- a/internal/handlers/pages.go +++ b/internal/handlers/pages.go @@ -131,6 +131,52 @@ func HandlePagesCreate(w http.ResponseWriter, r *http.Request) { }) respond(w, 201, v, e) } + +// reorderPages enforces the pinned/unpinned grouping invariant server-side +// (review M-9). The list orders by pinned DESC, position, so a permutation +// that interleaves the groups could never be displayed as requested. Rather +// than mutating meaningless positions, positions are normalized INSIDE each +// group: the submitted order determines the order of pinned pages among +// pinned pages and unpinned among unpinned, regardless of interleaving. +func reorderPages(r *http.Request, order []string) error { + return db.Transaction(r.Context(), database(r), func(tx *sql.Tx) error { + if e := lockUser(r, tx); e != nil { + return e + } + rows, e := db.Many(r.Context(), tx, `SELECT jsonb_build_object('id',id,'pinned',pinned) FROM "Page" WHERE "ownerId"=$1`, user(r)) + if e != nil { + return e + } + if len(rows) != len(order) { + return bad("Order must contain every record exactly once") + } + pinnedByID := make(map[string]bool, len(rows)) + for _, row := range rows { + isPinned, _ := row["pinned"].(bool) + pinnedByID[str(row, "id")] = isPinned + } + groups := map[bool][]string{} + seen := make(map[string]bool, len(order)) + for _, id := range order { + isPinned, ok := pinnedByID[id] + if !ok || seen[id] { + return bad("Order must contain every record exactly once") + } + seen[id] = true + groups[isPinned] = append(groups[isPinned], id) + } + for _, ids := range [][]string{groups[true], groups[false]} { + if len(ids) == 0 { + continue + } + if _, e := tx.ExecContext(r.Context(), `UPDATE "Page" AS p SET position=t.ord-1 FROM unnest($1::text[]) WITH ORDINALITY AS t(pid,ord) WHERE p.id=t.pid AND p."ownerId"=$2`, ids, user(r)); e != nil { + return e + } + } + return nil + }) +} + func HandlePagesReorder(w http.ResponseWriter, r *http.Request) { b, ok := read(w, r) if !ok { @@ -138,7 +184,7 @@ func HandlePagesReorder(w http.ResponseWriter, r *http.Request) { } list, e := ids(b, "order") if e == nil { - e = reorder(r, "Page", "ownerId", list) + e = reorderPages(r, list) } respond(w, 200, OK{true}, e) } diff --git a/internal/handlers/signals.go b/internal/handlers/signals.go index 58370c9..4a55b15 100644 --- a/internal/handlers/signals.go +++ b/internal/handlers/signals.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "github.com/andrew/go-computer/internal/db" + "github.com/andrew/go-computer/internal/outbound" "github.com/andrew/go-computer/internal/signals" "net/http" "strings" @@ -42,6 +43,15 @@ func HandleSignalsCreate(w http.ResponseWriter, r *http.Request) { // on different connections and leak the lock) — and answer 409 while // it is held. The 45-second cap keeps the pass inside the server's // WriteTimeout so it cannot be cut off mid-mutation. + // Review M-2: a manual check is a 45-second outbound pass; it draws + // from the same budget as previews/IMAP/CardDAV so repeated clicks + // cannot multiply concurrent mailbox connections. + release, retry, ok := outbound.Reserve("signals:"+user(r), 10, time.Minute) + if !ok { + outboundBusy(w, retry) + return + } + defer release() ctx := r.Context() leaseKey := "signal-poll:" + user(r) conn, e := database(r).Conn(ctx) diff --git a/internal/handlers/tags.go b/internal/handlers/tags.go index 7ed0560..a4649cb 100644 --- a/internal/handlers/tags.go +++ b/internal/handlers/tags.go @@ -74,7 +74,7 @@ func HandleTagsUpdate(w http.ResponseWriter, r *http.Request) { respond(w, 0, nil, e) return } - list, e := ids(b, "shareUserIds") + list, e := cappedIDs(b, "shareUserIds", maxShareRecipients) if e != nil { respond(w, 0, nil, e) return diff --git a/internal/http/review_test.go b/internal/http/review_test.go new file mode 100644 index 0000000..c7beb72 --- /dev/null +++ b/internal/http/review_test.go @@ -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) + } + } +} diff --git a/internal/http/server.go b/internal/http/server.go index d42cc3f..6dfe8ef 100644 --- a/internal/http/server.go +++ b/internal/http/server.go @@ -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), diff --git a/internal/markdown/markdown.go b/internal/markdown/markdown.go index c7dcdcc..48d8124 100644 --- a/internal/markdown/markdown.go +++ b/internal/markdown/markdown.go @@ -43,6 +43,21 @@ func BookmarkDocument(b db.Object) string { } return out + "\n" + strings.TrimSpace(text(b, "notes")) + "\n" } + +// linkDestination wraps a URL in a CommonMark angle-bracket destination +// (review L-11). Parentheses (and the whitespace validURL accepts) no longer +// terminate the link, and the two characters that cannot appear unescaped +// inside <...> are percent-encoded. +func linkDestination(raw string) string { + escaped := strings.NewReplacer( + "<", "%3C", + ">", "%3E", + "\r", "%0D", + "\n", "%0A", + ).Replace(raw) + return "<" + escaped + ">" +} + func PageDocument(p db.Object) string { out := "# " + text(p, "title") + "\n\n" + strings.TrimSpace(text(p, "content")) + "\n" if a, ok := p["bookmarks"].([]any); ok && len(a) > 0 { @@ -60,7 +75,7 @@ func PageDocument(p db.Object) string { if len(rs) > 160 { desc = string(rs[:160]) } - out += "- [" + title + "](" + text(b, "url") + ")" + out += "- [" + title + "](" + linkDestination(text(b, "url")) + ")" if desc != "" { out += " — " + desc } diff --git a/internal/markdown/markdown_test.go b/internal/markdown/markdown_test.go new file mode 100644 index 0000000..df54dba --- /dev/null +++ b/internal/markdown/markdown_test.go @@ -0,0 +1,27 @@ +package markdown + +import ( + "strings" + "testing" + + "github.com/andrew/go-computer/internal/db" +) + +// Review L-11: link destinations are wrapped and escaped so parentheses or +// angle brackets in a bookmark URL cannot terminate or extend the link. +func TestPageDocumentEscapesLinkDestinations(t *testing.T) { + out := PageDocument(db.Object{ + "title": "Links", + "content": "", + "bookmarks": []any{ + map[string]any{"bookmark": map[string]any{ + "title": "Odd", + "url": "https://example.com/a(b)?x=", + }}, + }, + }) + want := "[Odd]()" + if !strings.Contains(out, want) { + t.Fatalf("link destination not escaped:\n%s", out) + } +} diff --git a/internal/oidc/provider.go b/internal/oidc/provider.go index e807693..9d54657 100644 --- a/internal/oidc/provider.go +++ b/internal/oidc/provider.go @@ -22,7 +22,11 @@ import ( // Settings page and the login page read to decide whether to show the // "Sign in with Authelia" button. // -// GET /api/auth/oidc/status -> {enabled, issuerSet, clientIdSet, clientSecretSet, issuer} +// GET /api/auth/oidc/status -> {enabled, issuerSet, clientIdSet, clientSecretSet} +// +// Review L-7: the issuer URL is deliberately NOT public. It is usually a +// public hostname, but an internal one would become an information leak; the +// authenticated settings surface can expose it when one exists. func HandleStatus(w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-store") if r.Method != http.MethodGet { @@ -38,7 +42,6 @@ func HandleStatus(w http.ResponseWriter, r *http.Request) { "issuerSet": s.IssuerSet, "clientIdSet": s.ClientIDSet, "clientSecretSet": s.ClientSecretSet, - "issuer": s.Issuer, }) } diff --git a/internal/outbound/outbound.go b/internal/outbound/outbound.go new file mode 100644 index 0000000..dfc7789 --- /dev/null +++ b/internal/outbound/outbound.go @@ -0,0 +1,75 @@ +// Package outbound bounds server-initiated network work — URL previews, +// CardDAV syncs, IMAP test/check passes and manual signal checks — so an +// authenticated user cannot open unbounded outbound connections, exhaust +// file descriptors or saturate egress (review M-2). +// +// Two layers are shared by every caller: +// - a global in-flight cap (defaultConcurrent), and +// - a per-identity fixed-window rate (the caller supplies the limit/window, +// since a preview is cheaper than an IMAP search). +// +// Exhaustion is immediate: previews degrade to no metadata and interactive +// endpoints answer 429 with Retry-After. Nothing queues behind the budget. +package outbound + +import ( + "sync" + "time" +) + +const defaultConcurrent = 8 + +type window struct { + count int + reset time.Time +} + +type budget struct { + mu sync.Mutex + inFlight int + max int + keys map[string]*window +} + +var shared = &budget{max: defaultConcurrent, keys: map[string]*window{}} + +// Reserve claims one outbound slot for key. The returned release MUST be +// called when the work finishes. When ok is false, retry is a conservative +// Retry-After duration. +func Reserve(key string, limit int, windowDur time.Duration) (release func(), retry time.Duration, ok bool) { + if key == "" { + key = "anonymous" + } + shared.mu.Lock() + defer shared.mu.Unlock() + if shared.inFlight >= shared.max { + return nil, time.Second, false + } + now := time.Now() + if len(shared.keys) > 4096 { + for k, w := range shared.keys { + if now.After(w.reset) { + delete(shared.keys, k) + } + } + } + w := shared.keys[key] + if w == nil || now.After(w.reset) { + w = &window{reset: now.Add(windowDur)} + shared.keys[key] = w + } + if limit > 0 && w.count >= limit { + left := w.reset.Sub(now) + if left < 0 { + left = 0 + } + return nil, left.Truncate(time.Second) + time.Second, false + } + w.count++ + shared.inFlight++ + return func() { + shared.mu.Lock() + shared.inFlight-- + shared.mu.Unlock() + }, 0, true +} diff --git a/internal/outbound/outbound_test.go b/internal/outbound/outbound_test.go new file mode 100644 index 0000000..8187a79 --- /dev/null +++ b/internal/outbound/outbound_test.go @@ -0,0 +1,42 @@ +package outbound + +import ( + "testing" + "time" +) + +// Review M-2: outbound work is bounded per identity and globally, and a +// released slot is reusable. +func TestReserveBoundsPerKeyAndGlobally(t *testing.T) { + releaseA, retry, ok := Reserve("test-a", 1, time.Minute) + if !ok || releaseA == nil { + t.Fatalf("first reservation rejected (retry %v)", retry) + } + if _, _, ok := Reserve("test-a", 1, time.Minute); ok { + t.Fatal("per-key limit did not reject the second reservation") + } + + // Fill the remaining global slots on distinct keys. + releases := []func(){releaseA} + for i := 0; i < defaultConcurrent-1; i++ { + release, _, ok := Reserve("test-global-"+string(rune('a'+i)), 10, time.Minute) + if !ok { + t.Fatalf("global slot %d rejected below the cap", i) + } + releases = append(releases, release) + } + if _, _, ok := Reserve("test-overflow", 10, time.Minute); ok { + t.Fatal("global concurrency cap did not reject an extra reservation") + } + + // Release one slot and verify the global capacity is usable again. + releases[0]() + release, _, ok := Reserve("test-after-release", 1, time.Minute) + if !ok { + t.Fatal("released slot was not reusable") + } + release() + for _, r := range releases[1:] { + r() + } +} diff --git a/internal/preview/preview.go b/internal/preview/preview.go index 4d20a07..da948d1 100644 --- a/internal/preview/preview.go +++ b/internal/preview/preview.go @@ -4,13 +4,13 @@ import ( "context" "fmt" "github.com/andrew/go-computer/internal/netguard" + "github.com/andrew/go-computer/internal/outbound" "golang.org/x/net/html" "io" "net" "net/http" "net/url" "strings" - "sync" "time" ) @@ -72,60 +72,21 @@ var previewClient = &http.Client{ // previews are decorative, and a bookmark save must never be coupled to // preview availability (the row persists without og metadata). const ( - keyLimit = 20 - keyWindow = time.Minute + keyLimit = 20 + keyWindow = time.Minute + // maxConcurrent mirrors outbound's global cap; kept for the budget tests + // and documentation. maxConcurrent = 8 ) -type rateWindow struct { - count int - reset time.Time -} - -type budgetStore struct { - mu sync.Mutex - inFlight int - keys map[string]*rateWindow -} - -var sharedBudget = &budgetStore{keys: map[string]*rateWindow{}} - // Reserve claims one preview-work slot for key (the requesting identity). // The returned release function MUST be called when the fetch finishes. -// ok is false when either budget layer is exhausted. +// ok is false when either budget layer is exhausted. Preview shares the +// process-wide outbound budget with CardDAV/IMAP/signal checks (review M-2), +// while keeping its own more generous per-identity rate. func Reserve(key string) (release func(), ok bool) { - if key == "" { - key = "anonymous" - } - sharedBudget.mu.Lock() - defer sharedBudget.mu.Unlock() - if sharedBudget.inFlight >= maxConcurrent { - return nil, false - } - now := time.Now() - // Opportunistic prune keeps the map bounded by active identities. - if len(sharedBudget.keys) > 4096 { - for k, w := range sharedBudget.keys { - if now.After(w.reset) { - delete(sharedBudget.keys, k) - } - } - } - w := sharedBudget.keys[key] - if w == nil || now.After(w.reset) { - w = &rateWindow{reset: now.Add(keyWindow)} - sharedBudget.keys[key] = w - } - if w.count >= keyLimit { - return nil, false - } - w.count++ - sharedBudget.inFlight++ - return func() { - sharedBudget.mu.Lock() - sharedBudget.inFlight-- - sharedBudget.mu.Unlock() - }, true + release, _, ok = outbound.Reserve(key, keyLimit, keyWindow) + return release, ok } // FetchPreview resolves metadata for raw on behalf of key (the requesting diff --git a/main.go b/main.go index 68d6134..091a3fb 100644 --- a/main.go +++ b/main.go @@ -34,8 +34,8 @@ func main() { return } cfg := config.Load() - if cfg.Secret() == "" { - log.Fatal("config: AUTH_SECRET (or NEXTAUTH_SECRET) must be set") + if err := cfg.Validate(); err != nil { + log.Fatalf("config: %v", err) } d, err := db.New(cfg) if err != nil { diff --git a/scripts/server-deploy.sh b/scripts/server-deploy.sh index 81af8cb..1b1b68e 100755 --- a/scripts/server-deploy.sh +++ b/scripts/server-deploy.sh @@ -166,6 +166,12 @@ AUTH_SECRET=$(openssl rand -hex 32) NEXTAUTH_URL=https://$DOMAIN COOKIE_SECURE=1 ADMIN_EMAILS= +# Review H-1: nginx overwrites X-Real-IP, so the trusted Next server may sign +# and forward the browser identity for per-client rate limiting. +TRUSTED_PROXY=1 +# Review H-3: do not let the first visitor after DNS goes live claim the +# instance. Provision the operator with ADMIN_EMAILS + `make seed` (below). +ALLOW_BOOTSTRAP_SIGNUP=0 POSTGRES_VOLUME=computer_pgdata POSTGRES_VOLUME_EXTERNAL=false APP_HOST_PORT=$APP_HOST_PORT @@ -181,6 +187,14 @@ else # Pin the host ports even on an .env created before port overrides existed. grep -q '^APP_HOST_PORT=' .env || echo "APP_HOST_PORT=$APP_HOST_PORT" >> .env grep -q '^WEB_HOST_PORT=' .env || echo "WEB_HOST_PORT=$WEB_HOST_PORT" >> .env + # Review H-1: every server deployment terminates TLS at the bundled nginx, + # which pins X-Real-IP. Upgrade older .env files that predate the switch so + # proxied users stop sharing one rate-limit bucket. + grep -q '^TRUSTED_PROXY=' .env || echo "TRUSTED_PROXY=1" >> .env + # Review H-3: also stop later deploys from re-opening public bootstrap on a + # .env that predates the flag. Operators who deliberately want it can set + # ALLOW_BOOTSTRAP_SIGNUP=1 explicitly. + grep -q '^ALLOW_BOOTSTRAP_SIGNUP=' .env || echo "ALLOW_BOOTSTRAP_SIGNUP=0" >> .env url="$(sed -n 's/^NEXTAUTH_URL=//p' .env | tail -n1)" secure="$(sed -n 's/^COOKIE_SECURE=//p' .env | tail -n1)" [ "$url" = "https://$DOMAIN" ] || log "warning: NEXTAUTH_URL is '$url', expected 'https://$DOMAIN'" @@ -351,4 +365,14 @@ if [ -n "$REVISION" ]; then printf '%s\n' "$REVISION" > "$REMOTE_DIR/.deployed-revision" log "Recorded deployed revision $REVISION" fi +# Review H-3: with bootstrap disabled, nobody is administrator until the +# operator provisions one. Say so loudly instead of leaving a fresh instance +# without an owner. +if [ "$(sed -n 's/^ALLOW_BOOTSTRAP_SIGNUP=//p' .env | tail -n1)" = "0" ]; then + log "Administrator provisioning is operator-driven (ALLOW_BOOTSTRAP_SIGNUP=0)." + log " 1. Add ADMIN_EMAILS=you@example.com to $REMOTE_DIR/.env" + log " 2. docker compose up -d app # reloads ADMIN_EMAILS" + log " 3. SEED_EMAIL=you@example.com SEED_PASSWORD='...' make seed" + log "Public signup cannot become administrator until an operator exists." +fi log "Server-side deploy complete" diff --git a/web/app/(app)/admin/client.tsx b/web/app/(app)/admin/client.tsx index 4c98b5b..8e8e485 100644 --- a/web/app/(app)/admin/client.tsx +++ b/web/app/(app)/admin/client.tsx @@ -4,6 +4,7 @@ import { useEffect, useState } from 'react' import { toast } from 'sonner' import { Eye, Shield } from 'lucide-react' import { Button } from '@/components/ui/button' +import { resetReferenceCache } from '@/lib/reference-data' type U = { id: string; name: string | null; email: string; role: string; _count?: { bookmarks: number; pages: number; tags: number } } @@ -21,6 +22,8 @@ export function AdminClient({ currentUserId }: { currentUserId: string }) { const viewAs = async (userId: string) => { const res = await fetch('/api/admin/view-as', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ userId }) }) if (!res.ok) return toast.error('Could not switch account') + // Review L-6: entering view-as changes the effective identity. + resetReferenceCache() window.location.href = '/pages' } diff --git a/web/app/(app)/bookmarks/client.tsx b/web/app/(app)/bookmarks/client.tsx index 7172543..6aa9fa0 100644 --- a/web/app/(app)/bookmarks/client.tsx +++ b/web/app/(app)/bookmarks/client.tsx @@ -18,17 +18,16 @@ export function BookmarksClient({ initialSearch = null, initialOpen = null }: { const [selected, setSelected] = useState(null) // Review CR-04: the sheet must receive the CURRENT row after mutations — // its conflict banner ("Load latest version") and successful saves need - // the fresh version, or the next save would send a stale base. - const selectedRef = useRef(null) - selectedRef.current = selected + // the fresh version, or the next save would send a stale base. Depend on + // `selected` instead of writing a ref during render. const refreshSelected = useCallback(async () => { - const id = selectedRef.current?.id + const id = selected?.id if (!id) return try { const res = await fetch(`/api/bookmarks/${encodeURIComponent(id)}`, { cache: 'no-store' }) if (res.ok) setSelected(await res.json()) } catch { /* the list load surfaces errors */ } - }, []) + }, [selected]) const [tab, setTab] = useState<'mine' | 'shared'>('mine') const [sortField, setSortField] = useState<'title' | 'updatedAt' | 'kanbanStatus'>('updatedAt') const [sortDir, setSortDir] = useState<'asc' | 'desc'>('desc') diff --git a/web/app/(app)/calendar/client.tsx b/web/app/(app)/calendar/client.tsx index ae739c0..3a168d8 100644 --- a/web/app/(app)/calendar/client.tsx +++ b/web/app/(app)/calendar/client.tsx @@ -1,6 +1,6 @@ 'use client' -import { useState, useEffect, useCallback, useMemo } from 'react' +import { useState, useEffect, useCallback, useMemo, useRef } from 'react' import { Button } from '@/components/ui/button' import { BookmarkCard } from '@/components/bookmark-card' import { BookmarkDetailSheet } from '@/components/bookmark-detail-sheet' @@ -11,11 +11,16 @@ import { Input } from '@/components/ui/input' import { toast } from 'sonner' import { fetchBookmarks } from '@/lib/bookmarks' -const INITIAL_DATE = new Date(2026, 9, 1) +// Review L-4: a fixed, obviously-not-current placeholder. The real month and +// today are set after mount (to avoid an SSR/client hydration mismatch), and +// the calendar renders a skeleton until then instead of briefly presenting +// this epoch as the current month. +const INITIAL_DATE = new Date(2000, 0, 1) export function CalendarClient() { const [currentMonth, setCurrentMonth] = useState(INITIAL_DATE) const [today, setToday] = useState(INITIAL_DATE) + const [ready, setReady] = useState(false) const [bookmarks, setBookmarks] = useState([]) const [selectedDay, setSelectedDay] = useState(null) const [selectedBookmark, setSelectedBookmark] = useState(null) @@ -38,18 +43,25 @@ export function CalendarClient() { }, [shareQuery]) useEffect(() => { - const timer = setTimeout(() => { const now = new Date(); setCurrentMonth(now); setToday(now) }, 0) + const timer = setTimeout(() => { const now = new Date(); setCurrentMonth(now); setToday(now); setReady(true) }, 0) return () => clearTimeout(timer) }, []) + // Review M-11: bookmark-change events and slow initial loads can overlap; + // only the newest pass may publish its data. + const loadSeq = useRef(0) const load = useCallback(async () => { + const seq = ++loadSeq.current setLoading(true) try { const [mine, shared, dates] = await Promise.all([fetchBookmarks(), fetchBookmarks(new URLSearchParams({ shared: 'true' })), fetch('/api/shared-dates')]) + if (seq !== loadSeq.current) return + const sharedDatesJson = dates.ok ? await dates.json() : null + if (seq !== loadSeq.current) return setBookmarks([...mine, ...shared]) - if (dates.ok) setSharedDates(await dates.json()) - } catch { toast.error('Could not load calendar') } - finally { setLoading(false) } + if (sharedDatesJson) setSharedDates(sharedDatesJson) + } catch { if (seq === loadSeq.current) toast.error('Could not load calendar') } + finally { if (seq === loadSeq.current) setLoading(false) } }, []) useEffect(() => { const timer = setTimeout(load, 0); window.addEventListener('computer:bookmarks-changed', load); return () => { clearTimeout(timer); window.removeEventListener('computer:bookmarks-changed', load) } }, [load]) @@ -80,6 +92,18 @@ export function CalendarClient() { return bookmarksByDay[key] ?? [] }, [selectedDay, bookmarksByDay]) + if (!ready) { + return ( +
+
+

Calendar

+

Bookmarks organized by due date

+
+
+
+ ) + } + return (
@@ -151,6 +175,8 @@ export function CalendarClient() {

{format(selectedDay, 'EEEE, MMMM d, yyyy')}

setShareQuery(e.target.value)} />
+ {/* Review L-9: sharing a date is broader than the old label said. */} +

Recipients will see your bookmarks due that day, including their notes.

{shareUsers.map(u => )} {sharedDates.filter(s => s.date.slice(0, 10) === format(selectedDay, 'yyyy-MM-dd')).map(s =>
{s.creator.name || s.creator.email} shared with {s.recipient.name || s.recipient.email}
)} {selectedBookmarks.length === 0 ? ( diff --git a/web/app/(app)/collections/client.tsx b/web/app/(app)/collections/client.tsx index f97ca92..8febb8e 100644 --- a/web/app/(app)/collections/client.tsx +++ b/web/app/(app)/collections/client.tsx @@ -1,6 +1,6 @@ 'use client' -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import Link from 'next/link' import { Button } from '@/components/ui/button' import { Input } from '@/components/ui/input' @@ -109,7 +109,7 @@ function plainValue(prop: CollectionProperty, value: unknown): string { if (prop.type === 'DATE' && typeof value === 'string' && value) { if (prop.config.mode === 'date') return value.slice(0, 10) const date = new Date(value) - return Number.isNaN(date.getTime()) ? value : date.toLocaleString() + return Number.isNaN(date.getTime()) ? value : date.toLocaleString('en-US', { timeZone: 'UTC' }) } if (value === null || value === undefined || value === '') return '—' return String(value) @@ -246,6 +246,12 @@ export function CollectionsClient() { const [shareQuery, setShareQuery] = useState('') const [shareResults, setShareResults] = useState<{ id: string; name: string | null; email: string }[]>([]) + // Review M-11: switching collections/views quickly used to let an older + // response overwrite the newer selection. Each loader has a request + // sequence and only the latest may apply its result. + const detailSeq = useRef(0) + const itemsSeq = useRef(0) + const loadCollections = useCallback(async () => { try { const res = await fetch('/api/collections') @@ -254,27 +260,33 @@ export function CollectionsClient() { }, []) const loadDetail = useCallback(async (id: string) => { + const seq = ++detailSeq.current try { const res = await fetch(`/api/collections/${id}`) + if (seq !== detailSeq.current) return if (!res.ok) { setDetail(null); return } const data: CollectionDetail = await res.json() + if (seq !== detailSeq.current) return setDetail(data) setNameDraft(data.name) setViewId((current) => (current && data.views.some((v) => v.id === current) ? current : data.views[0]?.id ?? null)) - } catch { toast.error('Could not load collection') } + } catch { if (seq === detailSeq.current) toast.error('Could not load collection') } }, []) const loadItems = useCallback(async (id: string, view: string | null, append?: string | null) => { + const seq = ++itemsSeq.current try { const params = new URLSearchParams({ limit: '200' }) if (view) params.set('viewId', view) if (append) params.set('cursor', append) const res = await fetch(`/api/collections/${id}/items?${params}`) + if (seq !== itemsSeq.current) return if (!res.ok) return const rows: CollectionItem[] = await res.json() + if (seq !== itemsSeq.current) return setItems((current) => (append ? [...current, ...rows] : rows)) setCursor(res.headers.get('X-Next-Cursor')) - } catch { toast.error('Could not load items') } + } catch { if (seq === itemsSeq.current) toast.error('Could not load items') } }, []) useEffect(() => { const t = setTimeout(loadCollections, 0); return () => clearTimeout(t) }, [loadCollections]) @@ -286,14 +298,14 @@ export function CollectionsClient() { }, [collections, activeId]) useEffect(() => { const t = setTimeout(() => { - if (!activeId) { setDetail(null); return } + if (!activeId) { detailSeq.current++; setDetail(null); return } loadDetail(activeId) }, 0) return () => clearTimeout(t) }, [activeId, loadDetail]) useEffect(() => { const t = setTimeout(() => { - if (!activeId) { setItems([]); return } + if (!activeId) { itemsSeq.current++; setItems([]); return } loadItems(activeId, viewId) }, 0) return () => clearTimeout(t) @@ -529,7 +541,12 @@ export function CollectionsClient() { if (!['empty', 'notEmpty'].includes(fOp)) { if (prop.type === 'NUMBER') value = Number(fValue) else if (prop.type === 'CHECKBOX') value = fValue === 'true' - else value = fValue + else if (prop.type === 'DATE' && prop.config?.mode === 'datetime') { + // Review M-14: datetime-local emits a local string; the server stores + // fixed-width UTC. + const parsed = new Date(fValue) + value = Number.isNaN(parsed.getTime()) ? '' : parsed.toISOString() + } else value = fValue if (fValue === '' || (prop.type === 'NUMBER' && Number.isNaN(value))) { toast.error('Filter value required'); return } } setClause([...filterClauses, value === undefined ? { propertyId: fProp, op: fOp } : { propertyId: fProp, op: fOp, value }]) @@ -539,7 +556,9 @@ export function CollectionsClient() { const prop = detail?.properties.find((p) => p.id === c.propertyId) if (!prop) return 'unknown' let shown = c.value - if (prop.type === 'SELECT' && typeof c.value === 'string') shown = prop.config.options?.find((o) => o.id === c.value)?.label ?? c.value + // Review M-14: MULTI_SELECT filters also carry an option id; show its + // label like SELECT instead of the raw id. + if ((prop.type === 'SELECT' || prop.type === 'MULTI_SELECT') && typeof c.value === 'string') shown = prop.config.options?.find((o) => o.id === c.value)?.label ?? c.value if (prop.type === 'CHECKBOX') shown = c.value === true ? 'checked' : 'unchecked' return `${prop.name} ${OP_LABELS[c.op] ?? c.op}${shown !== undefined ? ` ${shown}` : ''}` } @@ -637,6 +656,9 @@ export function CollectionsClient() { onDrop={(e) => { e.preventDefault() const item = items.find((i) => i.id === draggedItemId) + // This is a drop handler, not render; the compiler rule cannot + // see through the async helper's captured state. + // eslint-disable-next-line react-hooks/refs if (item) void moveItemToGroup(item, col.key === NONE ? null : col.key) setDraggedItemId(null) setBoardDropKey(null) @@ -824,12 +846,18 @@ export function CollectionsClient() { {!['empty', 'notEmpty'].includes(fOp) && (() => { const p = detail.properties.find((x) => x.id === fProp) - if (p?.type === 'SELECT') return ( + if (p?.type === 'SELECT' || p?.type === 'MULTI_SELECT') return ( ) + // Review M-14: DATE filters used to render a plain + // text box, so a normal date always failed server + // validation. Match the property editor's inputs. + if (p?.type === 'DATE') return p.config?.mode === 'datetime' + ? setFValue(e.target.value)} aria-label="Filter value" className="h-8 text-sm" /> + : setFValue(e.target.value)} aria-label="Filter value" className="h-8 text-sm" /> if (p?.type === 'CHECKBOX') return (