Local auth: users, sessions, register/login/logout/me (#4) #23

Merged
steve merged 2 commits from phase-1-local-auth into main 2026-07-18 21:05:01 +00:00
Owner

Closes #4. Phase 1, step 1 of the tracking epic #20.

Implements local (email + password) auth and the session layer that OIDC (#5) will reuse. Both auth paths will end in the same pansy session cookie.

What's here

Store (internal/store/)

  • users.goCreateUser, GetUserByID, GetUserByEmail (NOCASE), CountUsers. is_admin scanned via an int intermediate (driver returns int64, not bool).
  • sessions.goCreateSession, GetSession, TouchSession, DeleteSession, DeleteExpiredSessions.

Service (internal/service/ — the seam; all logic + future agent tools go through here)

  • password.go — argon2id (64 MiB / t=1 / p=4), PHC-string encoded, constant-time verify, malformed-hash rejection.
  • auth.goRegister/Login/session lifecycle/Providers. First user is admin; PANSY_REGISTRATION=closed still allows the first user (bootstrap); unknown-email and wrong-password return the same error and spend the same argon2 work (dummy-hash timing equalizer) so accounts can't be enumerated.
  • service.goService struct, injectable clock (now) for expiry tests, session token hashing (raw 32-byte token in cookie, sha256 hex persisted).

API (internal/api/)

  • POST /auth/register (auto-login), POST /auth/login, POST /auth/logout (idempotent), GET /auth/me, GET /auth/providers.
  • requireAuth middleware resolves the cookie → actor in context; any resolution failure is 401 (never 404). Session cookie: HttpOnly, SameSite=Lax, Secure only when PANSY_BASE_URL is https, path /.
  • oidc in /auth/providers is hard-false until #5 wires the endpoints.

main — wires the service and a background expired-session sweep (also dropped lazily on access). Sliding 30-day expiry, renewed at most ~hourly.

Verification

  • go build ./..., go vet ./..., go test ./... all green (CGO disabled, GOWORK=off).
  • Service tests: register (admin/bootstrap/duplicate/blank), login (success + indistinguishable failures + OIDC-only rejection), session expiry/sliding-renewal/cleanup, password hash/verify/salt/malformed.
  • API tests: full cookie flow, /me auth requirement, validation, providers, Secure-cookie-under-https.
  • curl end-to-end: register → /meserver restart → session persists (DB-backed) → logout → 401; wrong-password and unknown-email both 401 with identical INVALID_CREDENTIALS body.

Notes / out of scope

  • OIDC (#5), login UI (#6), password reset/change, email verification, rate limiting (all post-v1 or later issues).
  • PANSY_LOCAL_AUTH=false already rejects local register/login here (config existed); #5 will flip providers.oidc on and add the OIDC endpoints.

🤖 Generated with Claude Code

Closes #4. Phase 1, step 1 of the tracking epic #20. Implements local (email + password) auth and the session layer that OIDC (#5) will reuse. Both auth paths will end in the same pansy session cookie. ## What's here **Store** (`internal/store/`) - `users.go` — `CreateUser`, `GetUserByID`, `GetUserByEmail` (NOCASE), `CountUsers`. `is_admin` scanned via an int intermediate (driver returns int64, not bool). - `sessions.go` — `CreateSession`, `GetSession`, `TouchSession`, `DeleteSession`, `DeleteExpiredSessions`. **Service** (`internal/service/` — the seam; all logic + future agent tools go through here) - `password.go` — argon2id (64 MiB / t=1 / p=4), PHC-string encoded, constant-time verify, malformed-hash rejection. - `auth.go` — `Register`/`Login`/session lifecycle/`Providers`. First user is admin; `PANSY_REGISTRATION=closed` still allows the *first* user (bootstrap); unknown-email and wrong-password return the **same** error and spend the **same** argon2 work (dummy-hash timing equalizer) so accounts can't be enumerated. - `service.go` — `Service` struct, injectable clock (`now`) for expiry tests, session token hashing (raw 32-byte token in cookie, sha256 hex persisted). **API** (`internal/api/`) - `POST /auth/register` (auto-login), `POST /auth/login`, `POST /auth/logout` (idempotent), `GET /auth/me`, `GET /auth/providers`. - `requireAuth` middleware resolves the cookie → actor in context; any resolution failure is 401 (never 404). Session cookie: HttpOnly, SameSite=Lax, Secure only when `PANSY_BASE_URL` is https, path `/`. - `oidc` in `/auth/providers` is hard-false until #5 wires the endpoints. **main** — wires the service and a background expired-session sweep (also dropped lazily on access). Sliding 30-day expiry, renewed at most ~hourly. ## Verification - `go build ./...`, `go vet ./...`, `go test ./...` all green (CGO disabled, GOWORK=off). - Service tests: register (admin/bootstrap/duplicate/blank), login (success + indistinguishable failures + OIDC-only rejection), session expiry/sliding-renewal/cleanup, password hash/verify/salt/malformed. - API tests: full cookie flow, `/me` auth requirement, validation, providers, Secure-cookie-under-https. - curl end-to-end: register → `/me` → **server restart** → session persists (DB-backed) → logout → 401; wrong-password and unknown-email both 401 with identical `INVALID_CREDENTIALS` body. ## Notes / out of scope - OIDC (#5), login UI (#6), password reset/change, email verification, rate limiting (all post-v1 or later issues). - `PANSY_LOCAL_AUTH=false` already rejects local register/login here (config existed); #5 will flip `providers.oidc` on and add the OIDC endpoints. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-18 20:38:24 +00:00
Add local auth: users, sessions, register/login/logout/me (#4)
Build image / build-and-push (push) Successful in 8s
Gadfly review (reusable) / review (pull_request) Successful in 7m7s
Adversarial Review (Gadfly) / review (pull_request) Successful in 7m7s
0e41ccd95a
Implements pansy's local (email + password) authentication and the
session layer that OIDC (#5) will also reuse.

- store: users.go (create/get-by-id/get-by-email/count) and sessions.go
  (create/get/touch/delete/delete-expired), scanning the existing 0001
  schema.
- service: the business-logic seam. auth.go (Register/Login/session
  lifecycle/Providers) + password.go (argon2id, 64 MiB/1/4, PHC-encoded,
  constant-time verify) + service.go (Service, clock injection, token
  hashing). First user is admin; closed registration still allows the
  bootstrap user; unknown-email and wrong-password are indistinguishable
  (same error, same argon2 work via a dummy hash).
- api: POST /auth/register|login|logout, GET /auth/me|providers, plus a
  requireAuth middleware that resolves the HttpOnly session cookie
  (SameSite=Lax, Secure under https) to the actor. Handlers stay thin.
- main: wires the service and a periodic expired-session sweep; sessions
  are also dropped lazily on access. Sliding 30-day expiry.
- tests: service (register/login/expiry/renewal/cleanup, password) and
  api (cookie flow, middleware, validation, providers).

Verified end-to-end via curl: register -> me -> restart -> session
persists -> logout -> 401.

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

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-18 20:45:30Z

claude-code/sonnet · claude-code — done

  • security — Minor issues
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — Minor issues
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — No material issues found

kimi-k2.6:cloud · ollama-cloud — done

  • security — Blocking issues found
  • correctness — Blocking issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

opencode/glm-5.2:cloud · opencode — done

  • security — Minor issues
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-18 20:45:30Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-18 20:45:31 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 25 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +22,4 @@
// registerRequest / loginRequest are the JSON bodies for the local-auth
// endpoints. gin's binding validates them before the service is called.
type registerRequest struct {

🟠 Password length validation only in HTTP binding tags, not service layer

maintainability · flagged by 1 model

  • internal/api/auth.go:25-28 Password length validation only in HTTP binding tags, not service layer — registerRequest enforces min=8,max=1024 via gin binding tags, but service.Register (internal/service/auth.go:48-50) only checks in.Password == "". The service layer is documented as the place where "all permission checks and invariants live" (internal/service/service.go:2-4), so a caller bypassing REST (e.g. an agent tool or internal caller) will skip the length check. **Fix:*…

🪰 Gadfly · advisory

🟠 **Password length validation only in HTTP binding tags, not service layer** _maintainability · flagged by 1 model_ - **`internal/api/auth.go:25-28`** Password length validation only in HTTP binding tags, not service layer — `registerRequest` enforces `min=8,max=1024` via gin binding tags, but `service.Register` (`internal/service/auth.go:48-50`) only checks `in.Password == ""`. The service layer is documented as the place where "all permission checks and invariants live" (`internal/service/service.go:2-4`), so a caller bypassing REST (e.g. an agent tool or internal caller) will skip the length check. **Fix:*… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +30,4 @@
type loginRequest struct {
Email string `json:"email" binding:"required,email"`
Password string `json:"password" binding:"required"`

🟡 loginRequest missing max password boundary

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **loginRequest missing max password boundary** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +51,4 @@
return
}
if err := h.startSession(c, user.ID); err != nil {

🟠 startSession error in register/login is unlogged; operator-invisible, leaves half-created user on register

correctness, error-handling, maintainability · flagged by 3 models

  • internal/api/auth.go:54-57 & :75-78startSession failure is swallowed (unlogged). When CreateSession returns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. The logout handler (auth.go:87) and writeServiceError's default branch (auth.go:177) both slog.Error the cause; these two paths bypass that. For register this is especially bad: the user row is already committed, so the client…

🪰 Gadfly · advisory

🟠 **startSession error in register/login is unlogged; operator-invisible, leaves half-created user on register** _correctness, error-handling, maintainability · flagged by 3 models_ - **`internal/api/auth.go:54-57` & `:75-78` — `startSession` failure is swallowed (unlogged).** When `CreateSession` returns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. The `logout` handler (auth.go:87) and `writeServiceError`'s default branch (auth.go:177) both `slog.Error` the cause; these two paths bypass that. For `register` this is especially bad: the user row is already committed, so the client… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +59,4 @@
}
// login verifies credentials and sets the session cookie.
func (h *handlers) login(c *gin.Context) {

🟡 Login CSRF: no Origin/Referer validation on /auth/login (and other auth POSTs) with cookie-based sessions; SameSite=Lax does not block cross-site Set-Cookie on login

security · flagged by 1 model

  • Login CSRF via unvalidated Origin on auth POSTs (internal/api/auth.go:62-92, internal/api/auth.go:37-59). The session is a cookie (HttpOnly, SameSite=Lax), and SameSite=Lax blocks sending existing cookies on cross-site POSTs but does not block a cross-site POST to /auth/login from being processed and from Set-Cookie taking effect on the victim's browser. An attacker page can fetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …}) with the attacker's own cred…

🪰 Gadfly · advisory

🟡 **Login CSRF: no Origin/Referer validation on /auth/login (and other auth POSTs) with cookie-based sessions; SameSite=Lax does not block cross-site Set-Cookie on login** _security · flagged by 1 model_ - **Login CSRF via unvalidated Origin on auth POSTs** (`internal/api/auth.go:62-92`, `internal/api/auth.go:37-59`). The session is a cookie (HttpOnly, SameSite=Lax), and SameSite=Lax blocks sending *existing* cookies on cross-site POSTs but does **not** block a cross-site POST to `/auth/login` from being processed and from `Set-Cookie` taking effect on the victim's browser. An attacker page can `fetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …})` with the attacker's own cred… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +104,4 @@
// requireAuth is middleware that rejects requests without a valid session and,
// on success, stashes the resolved user in the context. Feature routers added by
// later issues (gardens, objects, …) attach this to their protected groups.
func (h *handlers) requireAuth() gin.HandlerFunc {

🔴 Sliding session expiry (server-side) is never propagated to the client cookie's Max-Age, so active users are logged out after 30 days from login regardless of activity

correctness · flagged by 2 models

  • internal/api/auth.go:107-126,145-148 — Sliding session expiry never reaches the browser cookie, so "sliding 30-day expiry" doesn't actually work. setSessionCookie (sets Max-Age) is only called from startSession (auth.go:129-136), which is only invoked by register/login (auth.go:54,75). requireAuth (auth.go:107-126) calls svc.ResolveSession, which slides sessions.expires_at forward in the DB via TouchSession (internal/service/auth.go:159-161) but never re-issues…

🪰 Gadfly · advisory

🔴 **Sliding session expiry (server-side) is never propagated to the client cookie's Max-Age, so active users are logged out after 30 days from login regardless of activity** _correctness · flagged by 2 models_ - **`internal/api/auth.go:107-126,145-148` — Sliding session expiry never reaches the browser cookie, so "sliding 30-day expiry" doesn't actually work.** `setSessionCookie` (sets `Max-Age`) is only called from `startSession` (`auth.go:129-136`), which is only invoked by `register`/`login` (`auth.go:54,75`). `requireAuth` (`auth.go:107-126`) calls `svc.ResolveSession`, which slides `sessions.expires_at` forward in the DB via `TouchSession` (`internal/service/auth.go:159-161`) but never re-issues… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +108,4 @@
return func(c *gin.Context) {
token, err := c.Cookie(sessionCookie)
if err != nil || token == "" {
writeAPIError(c, http.StatusUnauthorized, "UNAUTHENTICATED", "authentication required")

requireAuth repeats an identical unauthorized-response triplet in two branches

maintainability · flagged by 1 model

  • internal/api/auth.go:111 and internal/api/auth.go:119 — the two failure branches in requireAuth (missing cookie and ResolveSession error) write the exact same writeAPIError(...) / c.Abort() / return triplet. Verified by reading the middleware. Minor, but a one-line unauthorized(c) helper would remove the duplication and make it obvious both paths are intentionally treated the same (the comment already explains why, so the intent isn't lost — just the repetition).

🪰 Gadfly · advisory

⚪ **requireAuth repeats an identical unauthorized-response triplet in two branches** _maintainability · flagged by 1 model_ - `internal/api/auth.go:111` and `internal/api/auth.go:119` — the two failure branches in `requireAuth` (`missing cookie` and `ResolveSession error`) write the exact same `writeAPIError(...) / c.Abort() / return` triplet. Verified by reading the middleware. Minor, but a one-line `unauthorized(c)` helper would remove the duplication and make it obvious both paths are intentionally treated the same (the comment already explains why, so the intent isn't lost — just the repetition). <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +138,4 @@
// cookieSecure reports whether the session cookie should carry the Secure
// attribute: only when the instance is served over HTTPS (PANSY_BASE_URL).
// Marking it Secure under plain-http local dev would make the browser drop it.
func (h *handlers) cookieSecure() bool {

🟡 Session cookie Secure attribute defaults to false when PANSY_BASE_URL unset, risking token leak if deployed behind a proxy without setting the env var

security · flagged by 1 model

  • Session cookie defaults to non-Secure when PANSY_BASE_URL is unset (internal/api/auth.go:141-143, internal/config/config.go:70). cookieSecure() only returns true when BaseURL starts with https://, and the default BaseURL is "". A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to set PANSY_BASE_URL gets a session cookie without Secure; any same-host HTTP request (or downgrade/MITM) then leaks the bearer token. This is documented as inten…

🪰 Gadfly · advisory

🟡 **Session cookie Secure attribute defaults to false when PANSY_BASE_URL unset, risking token leak if deployed behind a proxy without setting the env var** _security · flagged by 1 model_ - **Session cookie defaults to non-Secure when `PANSY_BASE_URL` is unset** (`internal/api/auth.go:141-143`, `internal/config/config.go:70`). `cookieSecure()` only returns true when `BaseURL` starts with `https://`, and the default `BaseURL` is `""`. A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to set `PANSY_BASE_URL` gets a session cookie without `Secure`; any same-host HTTP request (or downgrade/MITM) then leaks the bearer token. This is documented as inten… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +143,4 @@
}
func (h *handlers) setSessionCookie(c *gin.Context, token string, expiresAt time.Time) {
maxAge := max(int(time.Until(expiresAt).Seconds()), 1)

setSessionCookie MaxAge floor of 1 masks clock-skew/negative-duration bugs instead of clearing the cookie

error-handling · flagged by 1 model

  • internal/api/auth.go:146maxAge floor of 1 masks clock-skew bugs. max(int(time.Until(expiresAt).Seconds()), 1) clamps a negative duration to a 1-second cookie instead of clearing it. Not currently reachable (expiresAt = now + 30d), but the floor silently swallows a clock-skew class of bug. Trivial; leave or change to clear the cookie when time.Until(expiresAt) <= 0.

🪰 Gadfly · advisory

⚪ **setSessionCookie MaxAge floor of 1 masks clock-skew/negative-duration bugs instead of clearing the cookie** _error-handling · flagged by 1 model_ - **`internal/api/auth.go:146` — `maxAge` floor of 1 masks clock-skew bugs.** `max(int(time.Until(expiresAt).Seconds()), 1)` clamps a negative duration to a 1-second cookie instead of clearing it. Not currently reachable (`expiresAt = now + 30d`), but the floor silently swallows a clock-skew class of bug. Trivial; leave or change to clear the cookie when `time.Until(expiresAt) <= 0`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +161,4 @@
// writeServiceError maps a service-layer sentinel error to pansy's JSON error
// envelope. Login failures never leak which of email/password was wrong.
func (h *handlers) writeServiceError(c *gin.Context, err error) {

🟡 writeServiceError method has unused receiver

maintainability · flagged by 1 model

  • internal/api/auth.go:164 writeServiceError method has unused receiver — Declared as func (h *handlers) writeServiceError(...), but h is never referenced inside the body, which is a small readability misdirection. Fix: Change it to a package-level function: func writeServiceError(c *gin.Context, err error).

🪰 Gadfly · advisory

🟡 **writeServiceError method has unused receiver** _maintainability · flagged by 1 model_ - **`internal/api/auth.go:164`** `writeServiceError` method has unused receiver — Declared as `func (h *handlers) writeServiceError(...)`, but `h` is never referenced inside the body, which is a small readability misdirection. **Fix:** Change it to a package-level function: `func writeServiceError(c *gin.Context, err error)`. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +43,4 @@
if !s.cfg.LocalAuth {
return nil, domain.ErrLocalAuthDisabled
}

🟠 TOCTOU race between CountUsers and CreateUser in Register lets concurrent first-time registrations both become admin and both bypass PANSY_REGISTRATION=closed bootstrap gating

correctness · flagged by 2 models

  • internal/service/auth.go:49 — Service layer does not enforce the 1024-character password ceiling Register validates in.Password == "" but never checks len(in.Password) > 1024. The API binding catches it today, but the PR positions internal/service as the seam for “future agent tools”. A direct caller (CLI, agent, test helper) can pass an arbitrarily large password, and the service will hash it rather than reject it. Fix: Add len(in.Password) > 1024 to the service-level vali…

🪰 Gadfly · advisory

🟠 **TOCTOU race between CountUsers and CreateUser in Register lets concurrent first-time registrations both become admin and both bypass PANSY_REGISTRATION=closed bootstrap gating** _correctness · flagged by 2 models_ - **`internal/service/auth.go:49` — Service layer does not enforce the 1024-character password ceiling** `Register` validates `in.Password == ""` but never checks `len(in.Password) > 1024`. The API binding catches it today, but the PR positions `internal/service` as the seam for “future agent tools”. A direct caller (CLI, agent, test helper) can pass an arbitrarily large password, and the service will hash it rather than reject it. **Fix:** Add `len(in.Password) > 1024` to the service-level vali… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +50,4 @@
return nil, domain.ErrInvalidInput
}
count, err := s.store.CountUsers(ctx)

🔴 TOCTOU race in Register allows multiple bootstrap admins

correctness, error-handling, security · flagged by 3 models

  • TOCTOU race allows multiple bootstrap admins on a fresh instance internal/service/auth.go:53-82Register reads CountUsers, checks count > 0 && !RegistrationOpen(), then calls CreateUser. Two concurrent requests on a fresh DB can both observe count == 0, pass the closed-registration gate, and both create accounts with IsAdmin: true (as long as they use different emails). This breaks the intended “exactly one bootstrap admin” security model and lets an attacker race the legit…

🪰 Gadfly · advisory

🔴 **TOCTOU race in Register allows multiple bootstrap admins** _correctness, error-handling, security · flagged by 3 models_ - **TOCTOU race allows multiple bootstrap admins on a fresh instance** `internal/service/auth.go:53-82` — `Register` reads `CountUsers`, checks `count > 0 && !RegistrationOpen()`, then calls `CreateUser`. Two concurrent requests on a fresh DB can both observe `count == 0`, pass the closed-registration gate, and both create accounts with `IsAdmin: true` (as long as they use different emails). This breaks the intended “exactly one bootstrap admin” security model and lets an attacker race the legit… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +74,4 @@
return nil, err
}
return s.store.CreateUser(ctx, &domain.User{

🟡 First-user-is-admin decided by CountUsers+CreateUser without a transaction; concurrent first registrations can both become admin

correctness · flagged by 1 model

  • internal/service/auth.go:77 (Register) — first-user-is-admin is decided by CountUsers then CreateUser with IsAdmin: count == 0, with no transaction or post-insert guard. Two concurrent first registrations with different emails both observe count == 0 and both insert as admin (the UNIQUE index only guards email collisions, which the comment acknowledges; it does not guard the admin grant). Result: two admins on a fresh instance. Low likelihood at household scale, but it is a real…

🪰 Gadfly · advisory

🟡 **First-user-is-admin decided by CountUsers+CreateUser without a transaction; concurrent first registrations can both become admin** _correctness · flagged by 1 model_ - `internal/service/auth.go:77` (`Register`) — first-user-is-admin is decided by `CountUsers` then `CreateUser` with `IsAdmin: count == 0`, with no transaction or post-insert guard. Two concurrent first registrations with *different* emails both observe `count == 0` and both insert as admin (the UNIQUE index only guards email collisions, which the comment acknowledges; it does not guard the admin grant). Result: two admins on a fresh instance. Low likelihood at household scale, but it is a real… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +78,4 @@
Email: email,
DisplayName: displayName,
PasswordHash: &hash,
IsAdmin: count == 0,

🟠 First-user-is-admin is read-then-write TOCTOU: two concurrent first registrations with different emails both become admin

correctness · flagged by 1 model

🪰 Gadfly · advisory

🟠 **First-user-is-admin is read-then-write TOCTOU: two concurrent first registrations with different emails both become admin** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +94,4 @@
switch {
case errors.Is(err, domain.ErrNotFound):
// Spend comparable time so timing can't distinguish a missing account.
_, _ = verifyPassword(s.dummyHash, password)

🟠 Empty dummyHash (rand failure in New) short-circuits verifyPassword with zero argon2 work, defeating the timing equalizer it claims to provide

error-handling · flagged by 2 models

  • internal/service/auth.go:97, 103 + internal/service/service.go:47-51 — empty dummyHash defeats the timing equalizer with no runtime signal. If hashPassword fails in New (only path: crypto/rand.Read failure), s.dummyHash stays "". verifyPassword("", password) short-circuits in decodeHash (verified: strings.Split("", "$")[""], length 1 ≠ 6 → errBadHash) doing zero argon2 work. Then unknown-email returns ~instantly while wrong-password pays full argon2 cost —…

🪰 Gadfly · advisory

🟠 **Empty dummyHash (rand failure in New) short-circuits verifyPassword with zero argon2 work, defeating the timing equalizer it claims to provide** _error-handling · flagged by 2 models_ - **`internal/service/auth.go:97, 103` + `internal/service/service.go:47-51` — empty `dummyHash` defeats the timing equalizer with no runtime signal.** If `hashPassword` fails in `New` (only path: `crypto/rand.Read` failure), `s.dummyHash` stays `""`. `verifyPassword("", password)` short-circuits in `decodeHash` (verified: `strings.Split("", "$")` → `[""]`, length 1 ≠ 6 → `errBadHash`) doing **zero** argon2 work. Then unknown-email returns ~instantly while wrong-password pays full argon2 cost —… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +104,4 @@
return nil, domain.ErrInvalidCredentials
}
ok, err := verifyPassword(*u.PasswordHash, password)

🟠 Malformed password hash silently returned as wrong-password with no log

correctness, error-handling · flagged by 1 model

  • internal/service/auth.go:107-110 — Malformed password hash silently returned as wrong-password with no log verifyPassword returns errBadHash when a stored hash is malformed (corruption, partial write, etc.). The password.go comment explicitly says “Callers treat it as an auth failure but should log it”, yet Login maps it straight to domain.ErrInvalidCredentials with no slog.Error. An operator has no server-side signal that data corruption is locking a user out. Fix: Log…

🪰 Gadfly · advisory

🟠 **Malformed password hash silently returned as wrong-password with no log** _correctness, error-handling · flagged by 1 model_ - **`internal/service/auth.go:107-110` — Malformed password hash silently returned as wrong-password with no log** `verifyPassword` returns `errBadHash` when a stored hash is malformed (corruption, partial write, etc.). The `password.go` comment explicitly says *“Callers treat it as an auth failure but should log it”*, yet `Login` maps it straight to `domain.ErrInvalidCredentials` with no `slog.Error`. An operator has no server-side signal that data corruption is locking a user out. **Fix:** Log… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +138,4 @@
return nil, domain.ErrNotFound
}
hash := hashToken(rawToken)
sess, err := s.store.GetSession(ctx, hash)

🟡 Two DB round-trips on every authenticated request

performance · flagged by 1 model

  • internal/service/auth.go:141Two DB round-trips on every authenticated request (observation, not blocking). ResolveSession calls GetSession (by PK) and then GetUserByID (by PK). Both are indexed and fast, but every protected endpoint pays two query overheads. A single JOIN query would halve the DB work on the hot path. Verified by reading store/sessions.go and store/users.go — neither currently exposes a joined fetch. Fix: Consider adding GetSessionWithUser in a follow-…

🪰 Gadfly · advisory

🟡 **Two DB round-trips on every authenticated request** _performance · flagged by 1 model_ - `internal/service/auth.go:141` — **Two DB round-trips on every authenticated request** (observation, not blocking). `ResolveSession` calls `GetSession` (by PK) and then `GetUserByID` (by PK). Both are indexed and fast, but every protected endpoint pays two query overheads. A single JOIN query would halve the DB work on the hot path. Verified by reading `store/sessions.go` and `store/users.go` — neither currently exposes a joined fetch. **Fix:** Consider adding `GetSessionWithUser` in a follow-… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +146,4 @@
exp, err := parseTime(sess.ExpiresAt)
if err != nil {
// A corrupt expiry means we can't trust the session; drop it.
_ = s.store.DeleteSession(ctx, hash)

🟡 DeleteSession error silently discarded on corrupt expiry

error-handling · flagged by 3 models

  • internal/service/auth.go:149ResolveSession also silently discards errors from s.store.DeleteSession(...) on the corrupt-expiry path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away.

🪰 Gadfly · advisory

🟡 **DeleteSession error silently discarded on corrupt expiry** _error-handling · flagged by 3 models_ - **`internal/service/auth.go:149`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the corrupt-expiry path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +152,4 @@
now := s.now()
if !now.Before(exp) {
_ = s.store.DeleteSession(ctx, hash)

🟡 DeleteSession error silently discarded on expired session

error-handling · flagged by 2 models

  • internal/service/auth.go:155ResolveSession also silently discards errors from s.store.DeleteSession(...) on the already-expired path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away.

🪰 Gadfly · advisory

🟡 **DeleteSession error silently discarded on expired session** _error-handling · flagged by 2 models_ - **`internal/service/auth.go:155`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the already-expired path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +156,4 @@
return nil, domain.ErrNotFound
}
if newExp := now.Add(sessionTTL); newExp.Sub(exp) > time.Hour {

🟠 Sliding session renewal bumps DB expiry but never refreshes the cookie Max-Age — browser logs the user out ~30 days after the original login regardless of activity

correctness, error-handling · flagged by 3 models

  • internal/service/auth.go:159 / internal/api/auth.go:145-149 — sliding renewal extends the DB session but never refreshes the cookie's Max-Age. ResolveSession calls s.store.TouchSession(...) to push expires_at forward, but the cookie set at login (setSessionCookie) computes maxAge once from expiresAt and is never re-issued on /me or any subsequent request. So a user who keeps using the app will have their server-side session slid to "now + 30 days" indefinitely, but the br…

🪰 Gadfly · advisory

🟠 **Sliding session renewal bumps DB expiry but never refreshes the cookie Max-Age — browser logs the user out ~30 days after the original login regardless of activity** _correctness, error-handling · flagged by 3 models_ - **`internal/service/auth.go:159` / `internal/api/auth.go:145-149` — sliding renewal extends the DB session but never refreshes the cookie's Max-Age.** `ResolveSession` calls `s.store.TouchSession(...)` to push `expires_at` forward, but the cookie set at login (`setSessionCookie`) computes `maxAge` once from `expiresAt` and is never re-issued on `/me` or any subsequent request. So a user who keeps using the app will have their server-side session slid to "now + 30 days" indefinitely, but the br… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +17,4 @@
// later leaves old hashes verifiable.
const (
argonMemKiB = 64 * 1024 // 64 MiB
argonTime = 1

🟡 Argon2id t=1 with m=64MiB/p=4 doesn't match either RFC 9106 recommended parameter profile, weakening password hash resistance

security · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Argon2id t=1 with m=64MiB/p=4 doesn't match either RFC 9106 recommended parameter profile, weakening password hash resistance** _security · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +24,4 @@
)
// errBadHash marks a stored hash string that could not be parsed — a data or
// programming error, not a wrong password. Callers treat it as an auth failure

🟡 errBadHash doc says callers should log it but no caller does

maintainability · flagged by 1 model

  • internal/service/password.go:27-28 — The errBadHash doc says "Callers treat it as an auth failure but should log it." No caller logs it: service.Login (auth.go:107-110) folds errBadHash into ErrInvalidCredentials via if err != nil || !ok with no log, and the only other caller in tests asserts the error. So the "should log it" guidance is unfulfilled — either drop the prescription from the comment or actually log it in Login (a stored hash that won't parse is a real data/ops s…

🪰 Gadfly · advisory

🟡 **errBadHash doc says callers should log it but no caller does** _maintainability · flagged by 1 model_ - **`internal/service/password.go:27-28`** — The `errBadHash` doc says "Callers treat it as an auth failure but should log it." No caller logs it: `service.Login` (auth.go:107-110) folds `errBadHash` into `ErrInvalidCredentials` via `if err != nil || !ok` with no log, and the only other caller in tests asserts the error. So the "should log it" guidance is unfulfilled — either drop the prescription from the comment or actually log it in `Login` (a stored hash that won't parse is a real data/ops s… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +66,4 @@
}
var version int
if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil || version != argon2.Version {

decodeHash inconsistently shadows vs. assigns the named err return

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **decodeHash inconsistently shadows vs. assigns the named err return** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +1,76 @@
// Package service is pansy's business-logic seam: every operation is a method on
// *Service taking (ctx, actor, args), and all permission checks and invariants

🟡 Package doc claims methods take (ctx, actor, args) but no method in this PR takes an actor

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Package doc claims methods take (ctx, actor, args) but no method in this PR takes an actor** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +42,4 @@
// New constructs a Service. It precomputes a dummy password hash used to
// equalize login timing; if that fails (it shouldn't), login still works but
// loses the timing defense.
func New(st *store.DB, cfg *config.Config) *Service {

🔴 Empty dummyHash breaks login timing equalization and enables account enumeration

correctness, error-handling, security · flagged by 2 models

  • Empty dummyHash nullifies the login timing defense. In Service.New (internal/service/service.go:47-51), if hashPassword fails, s.dummyHash remains "". In Login (internal/service/auth.go:95-104), the unknown-email and OIDC-only paths call verifyPassword(s.dummyHash, password), which returns errBadHash instantly without touching argon2. The wrong-password path on a real account, however, performs the full 64 MiB argon2 computation. The resulting timing gap is large and me…

🪰 Gadfly · advisory

🔴 **Empty dummyHash breaks login timing equalization and enables account enumeration** _correctness, error-handling, security · flagged by 2 models_ - **Empty `dummyHash` nullifies the login timing defense.** In `Service.New` (`internal/service/service.go:47-51`), if `hashPassword` fails, `s.dummyHash` remains `""`. In `Login` (`internal/service/auth.go:95-104`), the unknown-email and OIDC-only paths call `verifyPassword(s.dummyHash, password)`, which returns `errBadHash` instantly without touching argon2. The wrong-password path on a real account, however, performs the full 64 MiB argon2 computation. The resulting timing gap is large and me… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +19,4 @@
}
// scanUser reads one users row. is_admin is stored as INTEGER 0/1 (the driver
// returns it as int64, which does not convert straight to bool), so it is read

🟡 scanUser comment says is_admin read into an int but code declares int64

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **scanUser comment says is_admin read into an int but code declares int64** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Blocking issues found · 26 findings (9 with multi-model agreement)

Finding Where Models Lens
🔴 TOCTOU race in Register allows multiple bootstrap admins internal/service/auth.go:53 3/5 correctness, error-handling, security
🟠 startSession error in register/login is unlogged; operator-invisible, leaves half-created user on register internal/api/auth.go:54 3/5 correctness, error-handling, maintainability
🟠 Sliding session renewal bumps DB expiry but never refreshes the cookie Max-Age — browser logs the user out ~30 days after the original login regardless of activity internal/service/auth.go:159 3/5 correctness, error-handling
🟡 DeleteSession error silently discarded on corrupt expiry internal/service/auth.go:149 3/5 error-handling
🔴 Sliding session expiry (server-side) is never propagated to the client cookie's Max-Age, so active users are logged out after 30 days from login regardless of activity internal/api/auth.go:107 2/5 correctness
🔴 Empty dummyHash breaks login timing equalization and enables account enumeration internal/service/service.go:45 2/5 correctness, error-handling, security
🟠 TOCTOU race between CountUsers and CreateUser in Register lets concurrent first-time registrations both become admin and both bypass PANSY_REGISTRATION=closed bootstrap gating internal/service/auth.go:46 2/5 correctness
🟠 Empty dummyHash (rand failure in New) short-circuits verifyPassword with zero argon2 work, defeating the timing equalizer it claims to provide internal/service/auth.go:97 2/5 error-handling
🟡 DeleteSession error silently discarded on expired session internal/service/auth.go:155 2/5 error-handling
17 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Password length validation only in HTTP binding tags, not service layer internal/api/auth.go:25 opencode/kimi-k2.6:cloud maintainability
🟠 First-user-is-admin is read-then-write TOCTOU: two concurrent first registrations with different emails both become admin internal/service/auth.go:81 opencode/glm-5.2:cloud correctness
🟠 Malformed password hash silently returned as wrong-password with no log internal/service/auth.go:107 opencode/kimi-k2.6:cloud correctness, error-handling
🟠 Missing index on sessions.expires_at causes full table scan during expired-session cleanup internal/store/migrations/0001_init.sql:32 opencode/kimi-k2.6:cloud performance
🟡 loginRequest missing max password boundary internal/api/auth.go:33 opencode/kimi-k2.6:cloud error-handling
🟡 Login CSRF: no Origin/Referer validation on /auth/login (and other auth POSTs) with cookie-based sessions; SameSite=Lax does not block cross-site Set-Cookie on login internal/api/auth.go:62 opencode/glm-5.2:cloud security
🟡 Session cookie Secure attribute defaults to false when PANSY_BASE_URL unset, risking token leak if deployed behind a proxy without setting the env var internal/api/auth.go:141 opencode/glm-5.2:cloud security
🟡 writeServiceError method has unused receiver internal/api/auth.go:164 opencode/kimi-k2.6:cloud maintainability
🟡 First-user-is-admin decided by CountUsers+CreateUser without a transaction; concurrent first registrations can both become admin internal/service/auth.go:77 glm-5.2:cloud correctness
🟡 Two DB round-trips on every authenticated request internal/service/auth.go:141 opencode/kimi-k2.6:cloud performance
🟡 Argon2id t=1 with m=64MiB/p=4 doesn't match either RFC 9106 recommended parameter profile, weakening password hash resistance internal/service/password.go:20 claude-code/sonnet security
🟡 errBadHash doc says callers should log it but no caller does internal/service/password.go:27 opencode/glm-5.2:cloud maintainability
🟡 Package doc claims methods take (ctx, actor, args) but no method in this PR takes an actor internal/service/service.go:2 opencode/glm-5.2:cloud maintainability
🟡 scanUser comment says is_admin read into an int but code declares int64 internal/store/users.go:22 opencode/glm-5.2:cloud maintainability
requireAuth repeats an identical unauthorized-response triplet in two branches internal/api/auth.go:111 claude-code/sonnet maintainability
setSessionCookie MaxAge floor of 1 masks clock-skew/negative-duration bugs instead of clearing the cookie internal/api/auth.go:146 opencode/glm-5.2:cloud error-handling
decodeHash inconsistently shadows vs. assigns the named err return internal/service/password.go:69 claude-code/sonnet maintainability
Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

Confirmed accurate. Both findings check out against the actual code — the TOCTOU race is real (no transaction/mutex around CountUsersCreateUser, no DB constraint limiting admin count), and the Argon2id parameters (m=64MiB, t=1, p=4) indeed match neither RFC 9106 profile. The "everything else checked out" claims (JSON tag suppression, etc.) also verify against the code.

Review

VERDICT: Minor issues

  • internal/service/auth.go:53-82 (Register) — TOCTOU race in first-user/admin bootstrap. CountUsers (line 53) and CreateUser (line 77) are separate round-trips with no transaction or lock between them, and the users table has no constraint limiting the number of admins (internal/store/migrations/0001_init.sql only has a UNIQUE on email and (oidc_issuer, oidc_subject)). Two concurrent POST /auth/register calls hitting an empty table can both observe count == 0 (internal/store/users.go:87-93) and both get IsAdmin: true; the same gap lets a registration slip through during PANSY_REGISTRATION=closed bootstrap (auth.go:59-61). Note: internal/store/sqlite.go:50's SetMaxOpenConns(1) only pins the pool to one connection for :memory: DSNs (used in tests); a production file-backed DB is not restricted to one connection, so the race is not even mitigated by connection serialization in the deployed case. Impact: an attacker who can fire a concurrent registration during the deployment bootstrap window can win admin alongside the intended first user. Fix: wrap count-check-then-insert in a single transaction/BEGIN IMMEDIATE, use an atomic INSERT ... WHERE (SELECT COUNT(*) FROM users) = 0-style guard, or serialize Register with an in-process mutex.

  • internal/service/password.go:14-23Argon2id time-cost parameter is below either RFC 9106-recommended profile. Constants confirmed: m=65536 KiB (64 MiB), t=1, p=4. RFC 9106 §4 gives two vetted profiles: (1) t=1, p=4, m=2 GiB when memory is abundant, or (2) t=3, p=4, m=64 MiB when memory is constrained. This pairs the low time-cost of profile 1 (t=1) with the low memory of profile 2 (64 MiB), a combination neither profile recommends. Suggest t=3 (matching profile 2) or increasing memory if t=1 is kept, and correct the comment's claim about "the interactive profile recommended by the argon2 authors" since it doesn't match RFC 9106's own examples.

Everything else checked out:

  • All SQL is parameterized (internal/store/users.go) — no injection.
  • PasswordHash/OIDCIssuer/OIDCSubject are json:"-" (internal/domain/domain.go:73-75) — confirmed no credential leakage via /auth/me, /auth/register, /auth/login responses.
  • Session tokens: 32 bytes from crypto/rand, only sha256 persisted, raw value only in the HttpOnly cookie.
  • Login/unknown-email/OIDC-only paths all route through verifyPassword(dummyHash, …) for timing equalization, and return the identical ErrInvalidCredentials.
  • Cookie flags: HttpOnly, SameSite=Lax, Secure gated on https:// prefix, empty Domain (host-only).
  • requireAuth fails closed (401) on any resolution error, never leaks existence via 404.
🎯 Correctness — Blocking issues found

Both findings confirmed by reading the actual source. Line numbers check out (auth.go:107-126 is requireAuth, service/auth.go Register matches, migrations 0001_init.sql:14-25 is the users table with no constraint preventing multiple is_admin=1 rows).

VERDICT: Blocking issues found

  • internal/api/auth.go:107-126,145-148 — Sliding session expiry never reaches the browser cookie, so "sliding 30-day expiry" doesn't actually work. setSessionCookie (sets Max-Age) is only called from startSession (auth.go:129-136), which is only invoked by register/login (auth.go:54,75). requireAuth (auth.go:107-126) calls svc.ResolveSession, which slides sessions.expires_at forward in the DB via TouchSession (internal/service/auth.go:159-161) but never re-issues a Set-Cookie. Concretely: a user logs in on day 0 with a fixed ~30-day cookie Max-Age. If they stay active for 40 days, the DB row keeps getting renewed, but the browser drops the cookie at day 30 regardless of activity, logging the user out — contradicting the stated "sliding 30-day expiry."

  • internal/service/auth.go:42-83 (Register) and internal/store/users.go:42-93 (CreateUser/CountUsers) — TOCTOU race on "first user is admin" / bootstrap-registration. Register calls CountUsers then later CreateUser with IsAdmin: count == 0 as two unsynchronized statements — no transaction, no locking. Verified store.Open (internal/store/sqlite.go:46-51) only pins the connection pool to 1 for in-memory DBs; a real file-backed deployment gets a normal pool (WAL mode, multiple connections), so two concurrent POST /auth/register requests can race while the table is empty and both observe count == 0, both inserting with is_admin=1. The schema (0001_init.sql:14-25) has no constraint capping is_admin=1 to one row, confirming nothing else blocks this. The same race also lets more than one account bypass the PANSY_REGISTRATION=closed bootstrap check (internal/service/auth.go:59-61), since both requests observe count == 0.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • internal/api/auth.go:54-58 and internal/api/auth.go:75-79register and login end with an identical 5-line block (startSession → error → c.JSON(user)). Verified by reading both handlers in full; the block is byte-for-byte the same. Low-risk duplication, but if a third caller of this pattern ever appears (or the response shape changes) both copies must be edited in lockstep. Suggest extracting func (h *handlers) respondWithSession(c *gin.Context, user *domain.User) and calling it from both.

  • internal/api/auth.go:111 and internal/api/auth.go:119 — the two failure branches in requireAuth (missing cookie and ResolveSession error) write the exact same writeAPIError(...) / c.Abort() / return triplet. Verified by reading the middleware. Minor, but a one-line unauthorized(c) helper would remove the duplication and make it obvious both paths are intentionally treated the same (the comment already explains why, so the intent isn't lost — just the repetition).

  • internal/service/password.go:68-79decodeHash mixes error-handling styles inconsistently: the two fmt.Sscanf checks (lines 69, 72) use if _, err := ... (a locally shadowed err, scoped only to the if-statement, shadowing the named return), while the two b64.DecodeString checks (lines 75, 78) use if salt, err = ... / if key, err = ... (assigning the named return err, salt, key directly). Verified by reading the function — every branch does an explicit return with literal zero values, so this doesn't cause a bug, but the inconsistency makes the named return values (mem, t, threads, salt, key, err) look like they matter for an implicit return path that doesn't actually exist here, which is confusing on a second read. Suggest picking one style (plain local err throughout, since nothing relies on the named err being set) for consistency.

Everything else in the lens is clean: the store/service/api layering is consistent with the package doc in service.go, naming is clear throughout, handlers/Service structs are appropriately small, and the new users.go/sessions.go establish (rather than diverge from) the store package's only real pattern so far.

Performance — No material issues found

VERDICT: No material issues found

Reviewed through the Performance lens (N+1s, unnecessary allocations, hot-loop work, unbounded growth, missing pagination, blocking calls, quadratic behavior).

What I checked:

  • internal/service/auth.go ResolveSession (internal/service/auth.go:104) — runs GetSession (PK lookup on sessions.token_hash, which is the table's PRIMARY KEY) then conditionally TouchSession, then GetUserByID (PK lookup). Verified against internal/store/migrations/0001_init.sql:32-38: both lookups hit primary-key indexes, no scans. This is the auth hot path (every future requireAuth-gated request), but it's 2 point-lookup queries against an embedded SQLite file — not a justified regression.
  • internal/service/auth.go Register — does an extra GetUserByEmail existence check before insert (internal/service/auth.go:66-71), but registration is a low-frequency, user-initiated action, not a hot loop.
  • internal/store/sessions.go DeleteExpiredSessions filters on expires_at, which has no index (only idx_sessions_user exists per the migration). This causes a full table scan on every sweep, but the sweep runs once per 6h (cmd/pansy/main.go:20 sessionSweepInterval) against a table that's bounded by active-user count — not material at the stated household/self-hosted scale.
  • internal/service/password.go argon2id hashing (64 MiB/t=1/p=4) runs synchronously per register/login call including the timing-equalizer dummy-hash path on every failed login. This is deliberate, security-motivated work rather than an accidental hot-path inefficiency, and the PR description explicitly defers rate limiting to a later issue — flagging the resulting resource-amplification risk belongs to the security lens, not mine.
  • Confirmed internal/store/sqlite.go:50 caps the DB to a single connection (SetMaxOpenConns(1)), pre-existing and untouched by this diff; the new session/user queries funnel through it but each is a cheap PK lookup, so this doesn't rise to a material regression introduced by this PR.

No N+1 patterns, unbounded loops, missing pagination, or avoidable quadratic behavior found in the diff.

🧯 Error handling & edge cases — Minor issues

Both findings check out against the actual source. Confirmed line numbers match: register handler at internal/api/auth.go:44-58 does call Register then startSession with no compensation on failure, and Login's dummy-hash fallback at internal/service/auth.go:97/103 depends on s.dummyHash from service.go:47-50, which stays "" if hashPassword errors — decodeHash("") in password.go:64 short-circuits before any argon2 work, silently defeating the timing equalizer.

VERDICT: Minor issues

  • internal/api/auth.go:54-56 (register) — When h.svc.Register succeeds but the subsequent h.startSession call fails, the handler returns 500 INTERNAL, but the user row from Register was already committed with no rollback. A retry of POST /auth/register with the same email then fails with 409 EMAIL_TAKEN, leaving the caller stuck unless they think to try /auth/login instead. Confirmed: Register (internal/service/auth.go:42-83) has no awareness of session creation, so nothing rolls the user back on a later session-write failure.

  • internal/service/auth.go:97,103 / internal/service/service.go:45-53 — If hashPassword fails while precomputing dummyHash in New (only possible if crypto/rand.Read fails), s.dummyHash stays "". Every subsequent unknown-email or OIDC-only login then calls verifyPassword(s.dummyHash, password), and decodeHash("") (internal/service/password.go:62-66) immediately fails the len(parts) != 6 check, returning before any argon2 work. ErrInvalidCredentials is still returned correctly, but the timing-equalizer this code exists for (per the comment at internal/service/auth.go:85-87) is silently defeated, with no ongoing signal beyond the one-time slog.Warn at startup (internal/service/service.go:48). Low likelihood (requires OS entropy failure at boot), but the error is genuinely swallowed (_, _ = verifyPassword(...)).

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

The finding is confirmed against the actual code:

  • service.go:47-51: if hashPassword fails, s.dummyHash stays "" (only slog.Warn).
  • auth.go:97,103: unknown-email and OIDC-only branches call verifyPassword(s.dummyHash, password).
  • password.go:62-66: decodeHash("") splits "" into [""] (len 1 ≠ 6) → returns errBadHash immediately, before any argon2.IDKey.
  • password.go:55: the real-password branch runs full argon2.IDKey.

So in the degraded state, a missing account responds measurably faster than a present one, reopening the enumeration side channel. The finding survives.

Verdict: Minor issues found

  • internal/service/service.go:46-49 / internal/service/auth.go:97,103Degraded timing-equalizer fails open silently. If the one-time hashPassword(...) in New fails (e.g. crypto/rand unavailable), s.dummyHash is left as the empty string "". In Login, the unknown-email and OIDC-only branches then call verifyPassword(s.dummyHash, password), which hits decodeHash("")errBadHash and returns without performing any argon2 work. The real-password branch still runs full argon2. So in that degraded state, a missing account responds measurably faster than a present one, re-opening the account-enumeration side channel the equalizer was built to close. The code comment acknowledges "loses the timing defense," but the failure is only slog.Warn'd and not otherwise enforced. Verified by reading service.go (New leaves dummyHash unset on error) and password.go (decodeHash rejects "" immediately before any argon2.IDKey). Suggested fix: if dummyHash can't be precomputed, either refuse to start (log.Fatal / return an error from New), or fall back to a hardcoded valid PHC hash constant so the equalizer always spends real argon2 work.

Everything else in my lane checks out: SQL is fully parameterized (store/users.go, store/sessions.go); the raw session token is 32 bytes of crypto/rand and only its sha256 is persisted; the cookie is HttpOnly + SameSite=Lax (which blocks cross-site POSTs, giving CSRF mitigation for these cookie-auth endpoints); PasswordHash/OIDC* carry json:"-" so /me and register/login responses never leak secrets; requireAuth returns 401 (never 404) on every resolution failure; argon2id verify is constant-time and rejects malformed hashes; login returns an identical sentinel for unknown-email and wrong-password. ErrEmailTaken on register reveals account existence, but that's an inherent property of a registration endpoint and not an enumeration regression.

🎯 Correctness — Minor issues

Both findings are confirmed against the actual source code.

  • Finding 1 (internal/service/auth.go:42-83): Register calls CountUsers (line 53) then CreateUser with IsAdmin: count == 0 (line 81) with no transaction. The comment at lines 63-65 acknowledges the email-collision race but not the admin-grant race. Two concurrent first registrations with different emails both observe count == 0 and both insert as admin. Confirmed.
  • Finding 2 (internal/api/auth.go:44-58): Register commits the user (line 44-52), then startSession is called separately (line 54). If startSession fails, the handler returns 500 (line 55) while the user row already exists; a retry hits ErrEmailTaken → 409. Confirmed.

Verdict: Minor issues found

I verified the new files end-to-end against the schema, domain types, config, and callers. The core auth/session logic is correct: timing equalization returns ErrInvalidCredentials regardless of dummyHash parse failure (return values discarded), expiry check !now.Before(exp) is right, sliding renewal (newExp.Sub(exp) > time.Hour) only writes when meaningfully moved and always renews near expiry, lexicographic ISO-8601 UTC comparison for DeleteExpiredSessions is valid for the fixed-width Z format, and PasswordHash carries json:"-" so /me can't leak it. Two minor correctness gaps:

  • internal/service/auth.go:77 (Register) — first-user-is-admin is decided by CountUsers then CreateUser with IsAdmin: count == 0, with no transaction or post-insert guard. Two concurrent first registrations with different emails both observe count == 0 and both insert as admin (the UNIQUE index only guards email collisions, which the comment acknowledges; it does not guard the admin grant). Result: two admins on a fresh instance. Low likelihood at household scale, but it is a real race in the admin-grant logic. Suggested fix: grant admin inside the insert only when a count == 0 check is held under a transaction, or recompute/UPDATE users SET is_admin=1 WHERE id=(SELECT MIN(id) FROM users) post-hoc.
  • internal/api/auth.go:54 (register) — Register commits the user, then startSession is called separately. If startSession fails (DB/token error), the handler returns 500 but the user row already exists. A client retry of /register then gets EMAIL_TAKEN (409) and is forced through /login instead of being told to log in. The account is created in an "orphaned, not logged in" state. Suggested fix: either fold session creation into the service Register so it's atomic-ish, or return a clearer error/body that tells the client the account was created and to log in.

Neither is blocking; both are edge cases. The rest of the change is correct under the correctness lens.

🧹 Code cleanliness & maintainability — No material issues found

Verdict: No material issues found

Through the code-cleanliness & maintainability lens, this change is well-structured and follows the patterns the surrounding code establishes. Verified by reading the actual checked-out files:

  • Layering is clean and consistent. Handlers (internal/api/auth.go) stay thin — bind, call a service method, encode. The service (internal/service/auth.go) owns all logic/invariants. The store (internal/store/users.go, sessions.go) is mechanical SQL. This matches the stated seam design and the existing domain/store/api split.
  • No dead/duplicated code of note. scanUser + the scanner interface + the shared userColumns constant correctly dedup the three user queries (users.go:13,24). startSession is correctly shared by both register and login (auth.go:128). The near-identical register/login handler bodies differ only in input type and error message, which is acceptable.
  • Names are clear and consistent with the rest of the package (ErrNotFound/ErrForbidden style extended cleanly by the new sentinels in domain.go:21-37; formatTime/parseTime/hashToken/newSessionToken are self-describing).
  • No leaky abstractions. The raw token never leaves the service→cookie boundary; only hashToken is persisted. domain.User.PasswordHash is json:"-" so /me can't leak it (verified at domain.go:73).
  • boolToInt (users.go:96) is a generic helper living in users.go rather than sqlite.go, but it's the only bool-column table right now, so the placement is fine.

One truly trivial nit (not material): setSessionCookie and clearSessionCookie (auth.go:145,151) each repeat c.SetSameSite(http.SameSiteLaxMode) before c.SetCookie; a tiny writeSessionCookie(c, token, maxAge) helper would collapse the two to one call site each. Not worth churning on its own.

No blocking or minor cleanliness issues to report.

Performance — No material issues found

VERDICT: No material issues found.

Performance review of the auth/session layer:

  • Hot path (ResolveSession, on every authenticated request): GetSession is a primary-key lookup on token_hash (sessions.token_hash is PK per migrations/0001_init.sql:33), and GetUserByID is a PK lookup. The sliding-renewal TouchSession write fires at most once per hour per session (newExp.Sub(exp) > time.Hour), so it doesn't write on every request. Two indexed reads + an occasional indexed update is fine; no N+1, no unbounded work.
  • Login/Register: GetUserByEmail hits the UNIQUE ... COLLATE NOCASE index (migrations:16); CountUsers is a single count(*). CreateUser inserts then re-reads by PK. No quadratic behavior.
  • Argon2id (64 MiB / t=1 / p=4): deliberately heavy on Login/Register, including the dummy-hash equalizer for unknown emails. That's a security tradeoff, not a performance regression, and these are low-frequency endpoints.
  • DeleteExpiredSessions: DELETE FROM sessions WHERE expires_at <= ? has no index on expires_at (only idx_sessions_user on user_id), so it's a full scan. But it runs on a background ticker every 6h (sessionSweepInterval), and the sessions table is bounded by the user base (household scale) plus lazy deletion on access. Not a hot path; not material enough to flag.
  • sweepSessions: one sweep at startup then every 6h; failures logged, never fatal. No goroutine leak (ticker stopped via defer, returns on ctx.Done()).

No efficiency regressions, unbounded growth, or hot-path blockers in this change.

🧯 Error handling & edge cases — No material issues found

All findings verified against the actual source. Every bullet accurately describes the code, and all are explicitly self-documented/accepted tradeoffs rather than material defects. The draft's own gadfly-findings array is empty, consistent with "no actionable issues." Recomputing the verdict accordingly.

VERDICT: No material issues found

  • internal/service/auth.go:97,103dummyHash can be empty, causing verifyPassword to run with a malformed hash (confirmed). In New (service.go:47-51), if hashPassword fails, s.dummyHash stays "". Login then calls verifyPassword(""), which goes through decodeHash("")strings.Split("", "$") returns [""] (length 1) → errBadHash. The return value is discarded (_, _ =), so no panic; ErrInvalidCredentials is still returned correctly. The only loss is the timing-equalization defense, which the code explicitly documents as acceptable. Self-documented edge case, not a bug.

  • internal/service/auth.go:159-160TouchSession error is silently swallowed (confirmed). _ = s.store.TouchSession(...) discards the error. If the DB update fails, the stored expiry is stale; the next request retries within the >1h window, or the session is lazily deleted once it expires. No security or correctness break — just a missed renewal. Benign at the stated scale.

  • internal/service/auth.go:149,155DeleteSession errors during lazy cleanup are swallowed (confirmed). Corrupt-expiry and expired-session cleanup both ignore the delete error. A failed delete leaves the row to be re-attempted on next access or reaped by the background sweep. The session is already treated as absent, so no correctness issue.

  • internal/service/auth.go:146-151 — corrupt expiry parsing is fully covered (confirmed). parseTime uses time.Parse(timeLayout, sess.ExpiresAt); any non-conforming value triggers the DeleteSession+ErrNotFound path. Correct.

  • internal/api/auth.go:146 — cookie maxAge truncation to seconds is benign (confirmed). int(time.Until(expiresAt).Seconds()) floors toward zero; the max(..., 1) guard prevents a zero/negative maxAge (which Gin/http.SetCookie treats as "delete now"). For a 30-day TTL a sub-second remainder is irrelevant.

  • internal/store/users.go:42-55CreateUser race on duplicate email surfaces as a generic 500 (confirmed, documented). The friendly GetUserByEmail pre-check + UNIQUE index race is acknowledged in the comment at auth.go:63-65. Under concurrency the loser hits the UNIQUE constraint and gets a wrapped store: insert user error → 500. A real but low-probability edge case, explicitly scoped as acceptable. Mapping the constraint violation to ErrEmailTaken (409) would be an improvement but is a known tradeoff, not an oversight.

No panics, no nil dereferences, no ignored critical errors, and no missing defer/cleanup found. The unhappy paths (empty token, garbage token, expired session, malformed hash, OIDC-only login, blank input, disabled local auth, closed registration) are all handled and mapped to appropriate sentinels/status codes. sweepSessions correctly checks ctx.Err() before logging and exits on ctx.Done().

kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Blocking issues found

Blocking issues found

  • TOCTOU race allows multiple bootstrap admins on a fresh instance
    internal/service/auth.go:53-82Register reads CountUsers, checks count > 0 && !RegistrationOpen(), then calls CreateUser. Two concurrent requests on a fresh DB can both observe count == 0, pass the closed-registration gate, and both create accounts with IsAdmin: true (as long as they use different emails). This breaks the intended “exactly one bootstrap admin” security model and lets an attacker race the legitimate owner during initial setup.
    Fix: Wrap the count-check-and-insert in a SQLite transaction (or use a partial unique index ON users(is_admin) WHERE is_admin = 1 so the second admin insert fails with a unique violation that you map to ErrRegistrationClosed).

  • Account-enumeration timing defense degrades silently on startup failure
    internal/service/service.go:45-52 — If hashPassword fails during New (e.g. crypto/rand exhaustion), s.dummyHash stays empty. Unknown-email logins then call verifyPassword("", password), which returns errBadHash instantly without doing any argon2 work. Wrong-password logins for real users still burn the full argon2 cost, producing a measurable timing gap that defeats the equalization defense.
    Fix: If dummy-hash generation fails, make Login fall back to an explicit constant-time delay (e.g. time.Sleep(estimatedArgon2Duration)) rather than a no-op verify, or treat the failure as fatal on startup.

🎯 Correctness — Blocking issues found

Blocking issues found

  • Cookie maxAge is never refreshed, breaking the sliding window.
    ResolveSession extends the server-side expiry via TouchSession (internal/service/auth.go:159-161), but requireAuth (internal/api/auth.go:107-126) never re-issues the Set-Cookie header. The browser’s cookie still carries the original maxAge (≈30 days from login). Once that expires, the browser drops the cookie and the active user is 401’d, even though the DB session was renewed. The PR claims a “sliding 30-day expiry,” but from the client perspective it is a fixed 30-day window.
    Fix: Have requireAuth call setSessionCookie with the updated expiry whenever ResolveSession succeeds and the session was touched (or on every authenticated request).

  • Empty dummyHash nullifies the login timing defense.
    In Service.New (internal/service/service.go:47-51), if hashPassword fails, s.dummyHash remains "". In Login (internal/service/auth.go:95-104), the unknown-email and OIDC-only paths call verifyPassword(s.dummyHash, password), which returns errBadHash instantly without touching argon2. The wrong-password path on a real account, however, performs the full 64 MiB argon2 computation. The resulting timing gap is large and measurable, so an attacker can enumerate registered emails by comparing response times. The PR says “login still works but loses the timing defense,” but the actual behavior is an active account-enumeration side-channel.
    Fix: If dummy-hash generation fails, generate a deterministic placeholder at start-up (e.g., a hard-coded valid PHC string) so verifyPassword always executes comparable work.

  • First-user-is-admin race condition.
    Register (internal/service/auth.go:53-81) reads CountUsers, then later calls CreateUser. Two concurrent requests on a fresh database both see count == 0, both pass the closed-registration bootstrap, and both create admin accounts. There is no atomic guard (e.g., INSERT … WHERE (SELECT count(*)=0) or a conditional unique constraint on is_admin).
    Fix: Make the first-user check atomic—either with a single SQL statement that inserts only when the users table is empty, or by catching a unique-constraint-like race at the DB level.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

The PR is clean and well-structured through the maintainability lens. Functions are appropriately sized and single-purpose, naming is consistent, the service/API/store layering is clean, and there's no dead or duplicated production code. Test helpers have minor cross-package duplication (authEngine vs newTestService), but that's acceptable isolation between layers.

Performance — No material issues found

No material issues found — almost. One genuine inefficiency.

  • internal/store/migrations/0001_init.sql:38 — The sessions table has only idx_sessions_user on user_id, but DeleteExpiredSessions (internal/store/sessions.go:70) queries WHERE expires_at <= ? and will table-scan the entire sessions table every 6 hours during the background sweep. For a long-running instance with accumulated stale sessions this is an avoidable full scan.
    Fix: Add CREATE INDEX idx_sessions_expires ON sessions (expires_at); to the schema so the sweep and any future expiry-based queries use the index.
🧯 Error handling & edge cases — Minor issues

Minor issues

  • internal/service/auth.go:160ResolveSession silently discards the error returned by s.store.TouchSession(...) when renewing a sliding session. If the update fails (e.g., database lock, disk full), the user’s session is not extended, yet the current request succeeds with a 200. The next request may then find an unexpectedly expired session and force a re-login. The error should be logged so operators can detect the write failure.

  • internal/service/auth.go:149ResolveSession also silently discards errors from s.store.DeleteSession(...) on the corrupt-expiry path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away.

  • internal/service/auth.go:155ResolveSession also silently discards errors from s.store.DeleteSession(...) on the already-expired path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away.

  • internal/service/service.go:47 — If hashPassword fails during service.New (e.g., crypto/rand.Read fails at startup), s.dummyHash remains the empty string. Later, in auth.go:97 and auth.go:103, verifyPassword("", password) returns errBadHash instantly without performing any argon2 work. This destroys the timing-equalization defense: an attacker can now distinguish “unknown email” (fast path) from “wrong password” (slow argon2 path) by measuring response time. The initialization path should either retry, fail hard, or at minimum log and refuse to process login requests when the dummy hash is missing.

opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

Both findings are confirmed. The login CSRF concern: no Origin/Referer validation anywhere; SameSite=Lax does not prevent cross-site POST setting a new cookie. The cookie Secure default concern: cookieSecure() returns false when BaseURL is empty/default, confirmed at lines 141-143 and config.go line 70.

VERDICT: Minor issues

  • Login CSRF via unvalidated Origin on auth POSTs (internal/api/auth.go:62-92, internal/api/auth.go:37-59). The session is a cookie (HttpOnly, SameSite=Lax), and SameSite=Lax blocks sending existing cookies on cross-site POSTs but does not block a cross-site POST to /auth/login from being processed and from Set-Cookie taking effect on the victim's browser. An attacker page can fetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …}) with the attacker's own credentials; the victim's browser accepts the resulting pansy_session cookie and is now silently logged in as the attacker (classic login CSRF), letting the attacker observe any data the victim then enters. Register and logout are similarly POSTable cross-site (logout is a state change, lower impact but still CSRF-relevant). Verified there is no Origin/Referer check anywhere (grep for CSRF|Origin|Referer returned nothing). Suggested fix: validate the Origin header against the configured base URL (or a same-origin check) on every state-changing POST under /auth, since this is cookie-auth with no CSRF token. Note the PR scopes rate limiting to "later" but doesn't explicitly defer CSRF, so flagging.

  • Session cookie defaults to non-Secure when PANSY_BASE_URL is unset (internal/api/auth.go:141-143, internal/config/config.go:70). cookieSecure() only returns true when BaseURL starts with https://, and the default BaseURL is "". A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to set PANSY_BASE_URL gets a session cookie without Secure; any same-host HTTP request (or downgrade/MITM) then leaks the bearer token. This is documented as intentional, but "secure by default" would be to set Secure whenever the request itself arrived over TLS (r.TLS != nil) rather than relying solely on an env var, or at minimum to log a loud warning at startup when serving real traffic without Secure. Severity small — it's a footgun in default configuration, not a code bug.

What I checked and found clean (no finding): domain.User.PasswordHash/OIDCIssuer/OIDCSubject carry json:"-" (internal/domain/domain.go:73-75), so c.JSON(http.StatusOK, user) in register/login/me cannot leak the hash; all SQL uses ? placeholders (no injection); argon2id uses a random 16-byte salt, constant-time compare, and the dummy-hash timing equalizer in Login runs argon2 on the unknown-email/OIDC-only paths (internal/service/auth.go:95-104); session tokens are 32 random bytes with only the sha256 stored (internal/service/service.go); startSession always mints a fresh token on login/register (no session fixation); cookie is scoped to the request host (empty domain) and HttpOnly.

🎯 Correctness — Minor issues

Both findings verified against actual source.

Finding 1 verified: ResolveSession (auth.go:159-161) calls s.store.TouchSession to slide DB expiry. The cookie is set only in startSessionsetSessionCookie (api/auth.go:145-149), called exclusively from register and login. requireAuth (api/auth.go:107-126) resolves the session and stashes the user but never re-sets the cookie. /me (line 95-97) just returns the actor. No path refreshes the cookie Max-Age after login. Confirmed.

Finding 2 verified: Register (auth.go:42-83) calls CountUsers (53), gates on count > 0 && !RegistrationOpen() (59), and sets IsAdmin: count == 0 (81). Two concurrent first-registrations with distinct emails both observe count == 0, both bypass the closed-registration gate, both insert with IsAdmin=true. The UNIQUE email index prevents same-email duplicates but not two different admins. Confirmed. (The bootstrap-when-closed behavior itself is documented in the README, so I soften that sub-claim, but the TOCTOU race producing multiple admins is real.)

VERDICT: Minor issues

  • internal/service/auth.go:159 / internal/api/auth.go:145-149 — sliding renewal extends the DB session but never refreshes the cookie's Max-Age. ResolveSession calls s.store.TouchSession(...) to push expires_at forward, but the cookie set at login (setSessionCookie) computes maxAge once from expiresAt and is never re-issued on /me or any subsequent request. So a user who keeps using the app will have their server-side session slid to "now + 30 days" indefinitely, but the browser will drop the pansy_session cookie ~30 days after the original login. After that the user is silently logged out despite a still-valid server-side row. The PR description advertises "Sliding 30-day expiry, renewed at most ~hourly," but the sliding only holds for the server half — the client half is fixed. Suggested fix: when ResolveSession renews (i.e. TouchSession runs), surface the new expiry so requireAuth/handlers can re-Set-Cookie with the bumped Max-Age; or set the cookie without a Max-Age (session-style) and rely solely on server-side expiry.

  • internal/service/auth.go:42-83 — first-user-is-admin is TOCTOU-prone: two concurrent first registrations both become admin. Register calls CountUsers (line 53), gates on count > 0 (line 59), and sets IsAdmin: count == 0 (line 81). Two concurrent first-registrations with different emails both observe count == 0, both pass the closed-registration gate, and both insert with IsAdmin=true. The UNIQUE email index prevents same-email duplicates but NOT two different emails both becoming admin. The PR description claims "The first account registered becomes admin" (singular) — under contention you can end up with two admins. The author already accepts races for the duplicate-email path ("household scale"), but the admin-grant race is higher-impact than a 500. Suggested fix: after insert, re-check CountUsers and demote if >1, or use a single SQL statement that conditionally sets is_admin = (NOT EXISTS (SELECT 1 FROM users)) so the admin decision is atomic with the insert.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues found

Reviewed through the code-cleanliness/maintainability lens. The code is generally well-structured, but a few documentation/comment mismatches stand out:

  • internal/service/service.go:1-6 — The package doc claims "every operation is a method on *Service taking (ctx, actor, args)", but none of the methods added in this PR take an actor parameter (Register(ctx, in), Login(ctx, email, password), CreateSession(ctx, userID), ResolveSession(ctx, rawToken), Logout(ctx, rawToken), CleanupExpiredSessions(ctx)). The comment is aspirational for a future issue and is currently inaccurate, which will confuse a reader landing here for the first time. Suggest softening to "operations are methods on *Service taking a context and args; protected operations will also take an actor once feature routers land."

  • internal/store/users.go:21-23 — The scanUser comment says "it is read into an int and mapped", but the declared intermediate is isAdmin int64. The comment and the code disagree on the type. Minor, but a reader will trust the comment and be puzzled. Fix the comment to "read into an int64 and mapped" (or change the variable to int).

  • internal/service/password.go:27-28 — The errBadHash doc says "Callers treat it as an auth failure but should log it." No caller logs it: service.Login (auth.go:107-110) folds errBadHash into ErrInvalidCredentials via if err != nil || !ok with no log, and the only other caller in tests asserts the error. So the "should log it" guidance is unfulfilled — either drop the prescription from the comment or actually log it in Login (a stored hash that won't parse is a real data/ops signal worth surfacing).

Verified by reading internal/service/auth.go, internal/service/password.go, internal/store/users.go, and internal/service/service.go in full and grepping for errBadHash and actor usages. No dead code, no duplicated logic, no naming collisions worth flagging; the rest of the change follows the existing patterns in api/ and store/.

Performance — No material issues found

I reviewed this PR through the performance lens: hot-path queries, indexing, allocations, unbounded growth, blocking work, and the session sweep. I verified the schema (internal/store/migrations/0001_init.sql), the store query methods, and the service hot paths (ResolveSession, Login, Register).

Verdict: No material issues found

What I checked and why it's fine at this PR's stated (household) scale:

  • ResolveSession (the per-request hot path behind requireAuth) does two indexed lookups — GetSession on token_hash (PRIMARY KEY) and GetUserByID on id (PRIMARY KEY). Sliding renewal writes at most once per hour per session (newExp.Sub(exp) > time.Hour), so a busy client does not write on every request. Good.
  • Login spends 64 MiB / argon2 t=1 p=4 per attempt, including the dummy-hash path for unknown emails. That is a deliberate security cost (timing equalization, called out in the diff), not a regression, and login is human-driven, not a tight loop.
  • Register runs CountUsers + GetUserByEmail + insert + GetUserByID. CountUsers is a full count(*) but the users table is tiny at this scale; only invoked on signup, not on any hot path.
  • DeleteExpiredSessions issues DELETE FROM sessions WHERE expires_at <= ? with no index on expires_at (only idx_sessions_user exists per 0001_init.sql:38), so each 6-hour sweep is a full table scan. The table is bounded by ~30 days of logins and expired rows are also dropped lazily on access, so the scan stays trivial. Not worth an index at this scale; would only matter if sessions grew into the tens of thousands, which the threat model (no rate limiting yet) doesn't push toward.
  • sweepSessions correctly stops its ticker on ctx.Done() — no goroutine leak.

Nothing here is a material efficiency regression.

🧯 Error handling & edge cases — Minor issues

I'll verify each finding against the actual checked-out code.
All four findings verified against actual code.

Finding 1 ✓ — auth.go:54-57 & 75-78: startSession error → generic 500, no slog.Error. Confirmed logout (line 87) and writeServiceError default (line 177) do log. Confirmed.

Finding 2 ✓ — service/auth.go:149, 155, 160: _ = s.store.DeleteSession/TouchSession(...) discard errors silently. Confirmed.

Finding 3 ✓ — service.go:47-51: on hashPassword error, dummyHash stays "". password.go:62-65: strings.Split("", "$")[""] (len 1 ≠ 6) → errBadHash returned before any argon2 work. Confirmed short-circuit, zero argon2 work, defeats timing equalizer.

Finding 4 ✓ — auth.go:146: max(int(time.Until(expiresAt).Seconds()), 1) clamps negative to 1, not clearing. Confirmed; trivial since expiresAt = now + 30d.


VERDICT: Minor issues

  • internal/api/auth.go:54-57 & :75-78startSession failure is swallowed (unlogged). When CreateSession returns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. The logout handler (auth.go:87) and writeServiceError's default branch (auth.go:177) both slog.Error the cause; these two paths bypass that. For register this is especially bad: the user row is already committed, so the client sees a 500 with no cookie, retries, and gets EMAIL_TAKEN — an operator-invisible confusing sequence. Fix: log the error before writing the 500 (slog.Error("api: start session", "error", err)), and ideally route the underlying error through writeServiceError's default branch.

  • internal/service/auth.go:149, 155, 160 — store errors from DeleteSession/TouchSession silently dropped. ResolveSession discards all three return errors with _ =. The lazy-delete paths (149 corrupt expiry, 155 expired) are mostly self-healing, but a persistent DB failure here leaves the user stuck at 401 with no diagnostic in the logs — the row never gets removed. The TouchSession swallow (160) silently disables sliding renewal on write failure. Fix: slog.Warn these so a degrading DB is observable.

  • internal/service/auth.go:97, 103 + internal/service/service.go:47-51 — empty dummyHash defeats the timing equalizer with no runtime signal. If hashPassword fails in New (only path: crypto/rand.Read failure), s.dummyHash stays "". verifyPassword("", password) short-circuits in decodeHash (verified: strings.Split("", "$")[""], length 1 ≠ 6 → errBadHash) doing zero argon2 work. Then unknown-email returns ~instantly while wrong-password pays full argon2 cost — the exact account-enumeration the code claims to prevent. The startup slog.Warn is easy to miss; the failure is silent for the process lifetime. Fix: in Login, if s.dummyHash == "", either recompute it lazily under a mutex or fall back to a hardcoded known-good hash string constant.

  • internal/api/auth.go:146maxAge floor of 1 masks clock-skew bugs. max(int(time.Until(expiresAt).Seconds()), 1) clamps a negative duration to a 1-second cookie instead of clearing it. Not currently reachable (expiresAt = now + 30d), but the floor silently swallows a clock-skew class of bug. Trivial; leave or change to clear the cookie when time.Until(expiresAt) <= 0.

opencode/kimi-k2.6:cloud (opencode) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

  • Auth flow: requireAuth correctly gates /auth/me and resolves the session cookie → user via ResolveSession; any failure is 401. The cookie is HttpOnly, SameSite=Lax, and Secure only when the base URL is HTTPS. Session tokens are 32 random bytes (CSPRNG) and only their SHA-256 is persisted.
  • Password handling: Uses argon2id with standard PHC encoding; salts are 16 bytes from crypto/rand; verification uses subtle.ConstantTimeCompare. The dummyHash timing equalizer in Login makes unknown-email and wrong-password paths indistinguishable in timing.
  • Injection resistance: All store methods use parameterized queries (? placeholders); userColumns is a hardcoded constant. No SQL injection surface.
  • Input validation: registerRequest validates email format and password length (8–1024) via gin binding; loginRequest validates email format. The service layer re-checks for emptiness as defense-in-depth.
  • Secret leakage: PasswordHash, OIDCIssuer, and OIDCSubject carry json:"-" tags, so they are excluded from JSON serialization. No credentials or raw tokens are logged.
  • Session lifecycle: CreateSession issues a new token on every login/register (no fixation risk). Logout is idempotent and clears the client cookie even if the server-side session is already gone. Sliding expiry and lazy deletion are correctly implemented.

Verified by reading internal/api/auth.go, internal/service/auth.go, internal/service/password.go, internal/service/service.go, internal/store/sessions.go, internal/store/users.go, internal/domain/domain.go, internal/config/config.go, and the migration schema.

🎯 Correctness — Blocking issues found

Verdict: Blocking issues found

  • internal/service/auth.go:53-82 — Race condition: concurrent first registrations can create multiple admins
    CountUsers and CreateUser are separate, non-transactional queries. Under WAL mode two goroutines can both read count == 0 before either writes, then both insert with IsAdmin: true. The UNIQUE guard only protects email, not is_admin, so two simultaneous sign-ups on a fresh instance both become admin.
    Fix: Perform the count-and-insert in a single atomic step (e.g., INSERT ... SELECT CASE WHEN EXISTS(SELECT 1 FROM users) THEN 0 ELSE 1 END) or wrap the logic in a BEGIN IMMEDIATE transaction so the read locks the table for writing.

  • internal/service/auth.go:49 — Service layer does not enforce the 1024-character password ceiling
    Register validates in.Password == "" but never checks len(in.Password) > 1024. The API binding catches it today, but the PR positions internal/service as the seam for “future agent tools”. A direct caller (CLI, agent, test helper) can pass an arbitrarily large password, and the service will hash it rather than reject it.
    Fix: Add len(in.Password) > 1024 to the service-level validation, returning domain.ErrInvalidInput.

  • internal/service/auth.go:107-110 — Malformed password hash silently returned as wrong-password with no log
    verifyPassword returns errBadHash when a stored hash is malformed (corruption, partial write, etc.). The password.go comment explicitly says “Callers treat it as an auth failure but should log it”, yet Login maps it straight to domain.ErrInvalidCredentials with no slog.Error. An operator has no server-side signal that data corruption is locking a user out.
    Fix: Log the malformed-hash error in Login before returning ErrInvalidCredentials, e.g. slog.Error("service: user has malformed password hash", "user_id", u.ID, "error", err).

🧹 Code cleanliness & maintainability — Minor issues

Minor issues

  • internal/api/auth.go:25-28 Password length validation only in HTTP binding tags, not service layer — registerRequest enforces min=8,max=1024 via gin binding tags, but service.Register (internal/service/auth.go:48-50) only checks in.Password == "". The service layer is documented as the place where "all permission checks and invariants live" (internal/service/service.go:2-4), so a caller bypassing REST (e.g. an agent tool or internal caller) will skip the length check.
    Fix: Move the min=8 / max=1024 check (and the display-name non-empty check) into service.Register and return domain.ErrInvalidInput. The handler can keep the binding tags as a fast-fail convenience, but the service should be the single source of truth. Also update the hard-coded error message at internal/api/auth.go:40 so it doesn't duplicate the rule.

  • internal/api/auth.go:164 writeServiceError method has unused receiver — Declared as func (h *handlers) writeServiceError(...), but h is never referenced inside the body, which is a small readability misdirection.
    Fix: Change it to a package-level function: func writeServiceError(c *gin.Context, err error).

Performance — Minor issues

Minor issues

  • internal/store/migrations/0001_init.sql:32Missing index on sessions.expires_at.
    DeleteExpiredSessions runs DELETE FROM sessions WHERE expires_at <= ? every 6 hours. Without an index on expires_at, SQLite performs a full table scan. At household scale this is tolerable today, but sessions accumulate over time (multiple devices, long-lived tokens) and the sweep will degrade linearly.
    Fix: Add CREATE INDEX idx_sessions_expires ON sessions (expires_at); alongside the existing idx_sessions_user.

  • internal/service/auth.go:141Two DB round-trips on every authenticated request (observation, not blocking).
    ResolveSession calls GetSession (by PK) and then GetUserByID (by PK). Both are indexed and fast, but every protected endpoint pays two query overheads. A single JOIN query would halve the DB work on the hot path. Verified by reading store/sessions.go and store/users.go — neither currently exposes a joined fetch.
    Fix: Consider adding GetSessionWithUser in a follow-up if profiling shows it matters.

🧯 Error handling & edge cases — Minor issues

Minor issues

  • internal/service/auth.go:160ResolveSession silently discards the error from s.store.TouchSession. If the DB write fails (busy, context cancelled, disk issue), the session expiry does not slide forward, so an active user can be logged out unexpectedly when the original TTL expires. The error should at least be logged so operators can detect when sliding renewal is failing.

  • internal/service/auth.go:53-82Register reads CountUsers and then CreateUser in separate transactions. On a fresh instance (or with PANSY_REGISTRATION=closed), concurrent bootstrap requests can both observe count == 0, so more than one user becomes admin. The PR acknowledges the duplicate-email race (handled by the UNIQUE index), but not this first-user admin race.

  • internal/service/auth.go:149,155ResolveSession ignores DeleteSession errors when dropping corrupt or expired sessions. A persistent DB fault can leave garbage rows that ResolveSession will keep re-discovering and re-failing on every request until the background sweep happens to succeed.

  • internal/service/auth.go:107-110Login swallows verifyPassword's errBadHash, returning only ErrInvalidCredentials with no log entry. If a stored hash is ever corrupted, the user gets a permanent “wrong password” response and there is no server-side trace to diagnose it.

  • internal/api/auth.go:31-34loginRequest has no max tag on Password, while registerRequest caps it at 1024. This is an unhandled boundary condition: login accepts arbitrarily long passwords, which is inconsistent and opens a trivial memory-pressure DoS vector against the argon2 verifier.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 5 models **Verdict: Blocking issues found** · 26 findings (9 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | TOCTOU race in Register allows multiple bootstrap admins | `internal/service/auth.go:53` | 3/5 | correctness, error-handling, security | | 🟠 | startSession error in register/login is unlogged; operator-invisible, leaves half-created user on register | `internal/api/auth.go:54` | 3/5 | correctness, error-handling, maintainability | | 🟠 | Sliding session renewal bumps DB expiry but never refreshes the cookie Max-Age — browser logs the user out ~30 days after the original login regardless of activity | `internal/service/auth.go:159` | 3/5 | correctness, error-handling | | 🟡 | DeleteSession error silently discarded on corrupt expiry | `internal/service/auth.go:149` | 3/5 | error-handling | | 🔴 | Sliding session expiry (server-side) is never propagated to the client cookie's Max-Age, so active users are logged out after 30 days from login regardless of activity | `internal/api/auth.go:107` | 2/5 | correctness | | 🔴 | Empty dummyHash breaks login timing equalization and enables account enumeration | `internal/service/service.go:45` | 2/5 | correctness, error-handling, security | | 🟠 | TOCTOU race between CountUsers and CreateUser in Register lets concurrent first-time registrations both become admin and both bypass PANSY_REGISTRATION=closed bootstrap gating | `internal/service/auth.go:46` | 2/5 | correctness | | 🟠 | Empty dummyHash (rand failure in New) short-circuits verifyPassword with zero argon2 work, defeating the timing equalizer it claims to provide | `internal/service/auth.go:97` | 2/5 | error-handling | | 🟡 | DeleteSession error silently discarded on expired session | `internal/service/auth.go:155` | 2/5 | error-handling | <details><summary>17 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Password length validation only in HTTP binding tags, not service layer | `internal/api/auth.go:25` | opencode/kimi-k2.6:cloud | maintainability | | 🟠 | First-user-is-admin is read-then-write TOCTOU: two concurrent first registrations with different emails both become admin | `internal/service/auth.go:81` | opencode/glm-5.2:cloud | correctness | | 🟠 | Malformed password hash silently returned as wrong-password with no log | `internal/service/auth.go:107` | opencode/kimi-k2.6:cloud | correctness, error-handling | | 🟠 | Missing index on sessions.expires_at causes full table scan during expired-session cleanup | `internal/store/migrations/0001_init.sql:32` | opencode/kimi-k2.6:cloud | performance | | 🟡 | loginRequest missing max password boundary | `internal/api/auth.go:33` | opencode/kimi-k2.6:cloud | error-handling | | 🟡 | Login CSRF: no Origin/Referer validation on /auth/login (and other auth POSTs) with cookie-based sessions; SameSite=Lax does not block cross-site Set-Cookie on login | `internal/api/auth.go:62` | opencode/glm-5.2:cloud | security | | 🟡 | Session cookie Secure attribute defaults to false when PANSY_BASE_URL unset, risking token leak if deployed behind a proxy without setting the env var | `internal/api/auth.go:141` | opencode/glm-5.2:cloud | security | | 🟡 | writeServiceError method has unused receiver | `internal/api/auth.go:164` | opencode/kimi-k2.6:cloud | maintainability | | 🟡 | First-user-is-admin decided by CountUsers+CreateUser without a transaction; concurrent first registrations can both become admin | `internal/service/auth.go:77` | glm-5.2:cloud | correctness | | 🟡 | Two DB round-trips on every authenticated request | `internal/service/auth.go:141` | opencode/kimi-k2.6:cloud | performance | | 🟡 | Argon2id t=1 with m=64MiB/p=4 doesn't match either RFC 9106 recommended parameter profile, weakening password hash resistance | `internal/service/password.go:20` | claude-code/sonnet | security | | 🟡 | errBadHash doc says callers should log it but no caller does | `internal/service/password.go:27` | opencode/glm-5.2:cloud | maintainability | | 🟡 | Package doc claims methods take (ctx, actor, args) but no method in this PR takes an actor | `internal/service/service.go:2` | opencode/glm-5.2:cloud | maintainability | | 🟡 | scanUser comment says is_admin read into an int but code declares int64 | `internal/store/users.go:22` | opencode/glm-5.2:cloud | maintainability | | ⚪ | requireAuth repeats an identical unauthorized-response triplet in two branches | `internal/api/auth.go:111` | claude-code/sonnet | maintainability | | ⚪ | setSessionCookie MaxAge floor of 1 masks clock-skew/negative-duration bugs instead of clearing the cookie | `internal/api/auth.go:146` | opencode/glm-5.2:cloud | error-handling | | ⚪ | decodeHash inconsistently shadows vs. assigns the named err return | `internal/service/password.go:69` | claude-code/sonnet | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> Confirmed accurate. Both findings check out against the actual code — the TOCTOU race is real (no transaction/mutex around `CountUsers`→`CreateUser`, no DB constraint limiting admin count), and the Argon2id parameters (`m=64MiB, t=1, p=4`) indeed match neither RFC 9106 profile. The "everything else checked out" claims (JSON tag suppression, etc.) also verify against the code. ## Review **VERDICT: Minor issues** - `internal/service/auth.go:53-82` (`Register`) — **TOCTOU race in first-user/admin bootstrap.** `CountUsers` (line 53) and `CreateUser` (line 77) are separate round-trips with no transaction or lock between them, and the `users` table has no constraint limiting the number of admins (`internal/store/migrations/0001_init.sql` only has a `UNIQUE` on `email` and `(oidc_issuer, oidc_subject)`). Two concurrent `POST /auth/register` calls hitting an empty table can both observe `count == 0` (`internal/store/users.go:87-93`) and both get `IsAdmin: true`; the same gap lets a registration slip through during `PANSY_REGISTRATION=closed` bootstrap (`auth.go:59-61`). Note: `internal/store/sqlite.go:50`'s `SetMaxOpenConns(1)` only pins the pool to one connection for `:memory:` DSNs (used in tests); a production file-backed DB is not restricted to one connection, so the race is not even mitigated by connection serialization in the deployed case. Impact: an attacker who can fire a concurrent registration during the deployment bootstrap window can win admin alongside the intended first user. Fix: wrap count-check-then-insert in a single transaction/`BEGIN IMMEDIATE`, use an atomic `INSERT ... WHERE (SELECT COUNT(*) FROM users) = 0`-style guard, or serialize `Register` with an in-process mutex. - `internal/service/password.go:14-23` — **Argon2id time-cost parameter is below either RFC 9106-recommended profile.** Constants confirmed: `m=65536 KiB (64 MiB), t=1, p=4`. RFC 9106 §4 gives two vetted profiles: (1) `t=1, p=4, m=2 GiB` when memory is abundant, or (2) `t=3, p=4, m=64 MiB` when memory is constrained. This pairs the low time-cost of profile 1 (`t=1`) with the low memory of profile 2 (`64 MiB`), a combination neither profile recommends. Suggest `t=3` (matching profile 2) or increasing memory if `t=1` is kept, and correct the comment's claim about "the interactive profile recommended by the argon2 authors" since it doesn't match RFC 9106's own examples. Everything else checked out: - All SQL is parameterized (`internal/store/users.go`) — no injection. - `PasswordHash`/`OIDCIssuer`/`OIDCSubject` are `json:"-"` (`internal/domain/domain.go:73-75`) — confirmed no credential leakage via `/auth/me`, `/auth/register`, `/auth/login` responses. - Session tokens: 32 bytes from `crypto/rand`, only sha256 persisted, raw value only in the HttpOnly cookie. - Login/unknown-email/OIDC-only paths all route through `verifyPassword(dummyHash, …)` for timing equalization, and return the identical `ErrInvalidCredentials`. - Cookie flags: `HttpOnly`, `SameSite=Lax`, `Secure` gated on `https://` prefix, empty `Domain` (host-only). - `requireAuth` fails closed (401) on any resolution error, never leaks existence via 404. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> Both findings confirmed by reading the actual source. Line numbers check out (auth.go:107-126 is `requireAuth`, service/auth.go `Register` matches, migrations 0001_init.sql:14-25 is the `users` table with no constraint preventing multiple `is_admin=1` rows). ## VERDICT: Blocking issues found - **`internal/api/auth.go:107-126,145-148` — Sliding session expiry never reaches the browser cookie, so "sliding 30-day expiry" doesn't actually work.** `setSessionCookie` (sets `Max-Age`) is only called from `startSession` (`auth.go:129-136`), which is only invoked by `register`/`login` (`auth.go:54,75`). `requireAuth` (`auth.go:107-126`) calls `svc.ResolveSession`, which slides `sessions.expires_at` forward in the DB via `TouchSession` (`internal/service/auth.go:159-161`) but never re-issues a `Set-Cookie`. Concretely: a user logs in on day 0 with a fixed ~30-day cookie `Max-Age`. If they stay active for 40 days, the DB row keeps getting renewed, but the browser drops the cookie at day 30 regardless of activity, logging the user out — contradicting the stated "sliding 30-day expiry." - **`internal/service/auth.go:42-83` (`Register`) and `internal/store/users.go:42-93` (`CreateUser`/`CountUsers`) — TOCTOU race on "first user is admin" / bootstrap-registration.** `Register` calls `CountUsers` then later `CreateUser` with `IsAdmin: count == 0` as two unsynchronized statements — no transaction, no locking. Verified `store.Open` (`internal/store/sqlite.go:46-51`) only pins the connection pool to 1 for in-memory DBs; a real file-backed deployment gets a normal pool (WAL mode, multiple connections), so two concurrent `POST /auth/register` requests can race while the table is empty and both observe `count == 0`, both inserting with `is_admin=1`. The schema (`0001_init.sql:14-25`) has no constraint capping `is_admin=1` to one row, confirming nothing else blocks this. The same race also lets more than one account bypass the `PANSY_REGISTRATION=closed` bootstrap check (`internal/service/auth.go:59-61`), since both requests observe `count == 0`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **VERDICT: Minor issues** - `internal/api/auth.go:54-58` and `internal/api/auth.go:75-79` — `register` and `login` end with an identical 5-line block (`startSession` → error → `c.JSON(user)`). Verified by reading both handlers in full; the block is byte-for-byte the same. Low-risk duplication, but if a third caller of this pattern ever appears (or the response shape changes) both copies must be edited in lockstep. Suggest extracting `func (h *handlers) respondWithSession(c *gin.Context, user *domain.User)` and calling it from both. - `internal/api/auth.go:111` and `internal/api/auth.go:119` — the two failure branches in `requireAuth` (`missing cookie` and `ResolveSession error`) write the exact same `writeAPIError(...) / c.Abort() / return` triplet. Verified by reading the middleware. Minor, but a one-line `unauthorized(c)` helper would remove the duplication and make it obvious both paths are intentionally treated the same (the comment already explains why, so the intent isn't lost — just the repetition). - `internal/service/password.go:68-79` — `decodeHash` mixes error-handling styles inconsistently: the two `fmt.Sscanf` checks (lines 69, 72) use `if _, err := ...` (a *locally shadowed* `err`, scoped only to the if-statement, shadowing the named return), while the two `b64.DecodeString` checks (lines 75, 78) use `if salt, err = ...` / `if key, err = ...` (assigning the *named return* `err`, `salt`, `key` directly). Verified by reading the function — every branch does an explicit `return` with literal zero values, so this doesn't cause a bug, but the inconsistency makes the named return values (`mem, t, threads, salt, key, err`) look like they matter for an implicit return path that doesn't actually exist here, which is confusing on a second read. Suggest picking one style (plain local `err` throughout, since nothing relies on the named `err` being set) for consistency. Everything else in the lens is clean: the store/service/api layering is consistent with the package doc in `service.go`, naming is clear throughout, `handlers`/`Service` structs are appropriately small, and the new `users.go`/`sessions.go` establish (rather than diverge from) the store package's only real pattern so far. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the Performance lens (N+1s, unnecessary allocations, hot-loop work, unbounded growth, missing pagination, blocking calls, quadratic behavior). What I checked: - `internal/service/auth.go` `ResolveSession` (`internal/service/auth.go:104`) — runs `GetSession` (PK lookup on `sessions.token_hash`, which is the table's `PRIMARY KEY`) then conditionally `TouchSession`, then `GetUserByID` (PK lookup). Verified against `internal/store/migrations/0001_init.sql:32-38`: both lookups hit primary-key indexes, no scans. This is the auth hot path (every future `requireAuth`-gated request), but it's 2 point-lookup queries against an embedded SQLite file — not a justified regression. - `internal/service/auth.go` `Register` — does an extra `GetUserByEmail` existence check before insert (`internal/service/auth.go:66-71`), but registration is a low-frequency, user-initiated action, not a hot loop. - `internal/store/sessions.go` `DeleteExpiredSessions` filters on `expires_at`, which has no index (only `idx_sessions_user` exists per the migration). This causes a full table scan on every sweep, but the sweep runs once per 6h (`cmd/pansy/main.go:20` `sessionSweepInterval`) against a table that's bounded by active-user count — not material at the stated household/self-hosted scale. - `internal/service/password.go` argon2id hashing (64 MiB/t=1/p=4) runs synchronously per register/login call including the timing-equalizer dummy-hash path on every failed login. This is deliberate, security-motivated work rather than an accidental hot-path inefficiency, and the PR description explicitly defers rate limiting to a later issue — flagging the resulting resource-amplification risk belongs to the security lens, not mine. - Confirmed `internal/store/sqlite.go:50` caps the DB to a single connection (`SetMaxOpenConns(1)`), pre-existing and untouched by this diff; the new session/user queries funnel through it but each is a cheap PK lookup, so this doesn't rise to a material regression introduced by this PR. No N+1 patterns, unbounded loops, missing pagination, or avoidable quadratic behavior found in the diff. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings check out against the actual source. Confirmed line numbers match: `register` handler at internal/api/auth.go:44-58 does call `Register` then `startSession` with no compensation on failure, and `Login`'s dummy-hash fallback at internal/service/auth.go:97/103 depends on `s.dummyHash` from service.go:47-50, which stays `""` if `hashPassword` errors — `decodeHash("")` in password.go:64 short-circuits before any argon2 work, silently defeating the timing equalizer. VERDICT: Minor issues - `internal/api/auth.go:54-56` (register) — When `h.svc.Register` succeeds but the subsequent `h.startSession` call fails, the handler returns 500 `INTERNAL`, but the user row from `Register` was already committed with no rollback. A retry of `POST /auth/register` with the same email then fails with 409 `EMAIL_TAKEN`, leaving the caller stuck unless they think to try `/auth/login` instead. Confirmed: `Register` (`internal/service/auth.go:42-83`) has no awareness of session creation, so nothing rolls the user back on a later session-write failure. - `internal/service/auth.go:97,103` / `internal/service/service.go:45-53` — If `hashPassword` fails while precomputing `dummyHash` in `New` (only possible if `crypto/rand.Read` fails), `s.dummyHash` stays `""`. Every subsequent unknown-email or OIDC-only login then calls `verifyPassword(s.dummyHash, password)`, and `decodeHash("")` (`internal/service/password.go:62-66`) immediately fails the `len(parts) != 6` check, returning before any argon2 work. `ErrInvalidCredentials` is still returned correctly, but the timing-equalizer this code exists for (per the comment at `internal/service/auth.go:85-87`) is silently defeated, with no ongoing signal beyond the one-time `slog.Warn` at startup (`internal/service/service.go:48`). Low likelihood (requires OS entropy failure at boot), but the error is genuinely swallowed (`_, _ = verifyPassword(...)`). </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> The finding is confirmed against the actual code: - `service.go:47-51`: if `hashPassword` fails, `s.dummyHash` stays `""` (only `slog.Warn`). - `auth.go:97,103`: unknown-email and OIDC-only branches call `verifyPassword(s.dummyHash, password)`. - `password.go:62-66`: `decodeHash("")` splits `""` into `[""]` (len 1 ≠ 6) → returns `errBadHash` immediately, before any `argon2.IDKey`. - `password.go:55`: the real-password branch runs full `argon2.IDKey`. So in the degraded state, a missing account responds measurably faster than a present one, reopening the enumeration side channel. The finding survives. **Verdict: Minor issues found** - `internal/service/service.go:46-49` / `internal/service/auth.go:97,103` — **Degraded timing-equalizer fails open silently.** If the one-time `hashPassword(...)` in `New` fails (e.g. `crypto/rand` unavailable), `s.dummyHash` is left as the empty string `""`. In `Login`, the unknown-email and OIDC-only branches then call `verifyPassword(s.dummyHash, password)`, which hits `decodeHash("")` → `errBadHash` and returns *without performing any argon2 work*. The real-password branch still runs full argon2. So in that degraded state, a missing account responds measurably faster than a present one, re-opening the account-enumeration side channel the equalizer was built to close. The code comment acknowledges "loses the timing defense," but the failure is only `slog.Warn`'d and not otherwise enforced. Verified by reading `service.go` (New leaves `dummyHash` unset on error) and `password.go` (`decodeHash` rejects `""` immediately before any `argon2.IDKey`). Suggested fix: if `dummyHash` can't be precomputed, either refuse to start (`log.Fatal` / return an error from `New`), or fall back to a hardcoded valid PHC hash constant so the equalizer always spends real argon2 work. Everything else in my lane checks out: SQL is fully parameterized (`store/users.go`, `store/sessions.go`); the raw session token is 32 bytes of `crypto/rand` and only its sha256 is persisted; the cookie is `HttpOnly` + `SameSite=Lax` (which blocks cross-site POSTs, giving CSRF mitigation for these cookie-auth endpoints); `PasswordHash`/`OIDC*` carry `json:"-"` so `/me` and register/login responses never leak secrets; `requireAuth` returns 401 (never 404) on every resolution failure; argon2id verify is constant-time and rejects malformed hashes; login returns an identical sentinel for unknown-email and wrong-password. `ErrEmailTaken` on register reveals account existence, but that's an inherent property of a registration endpoint and not an enumeration regression. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings are confirmed against the actual source code. - **Finding 1** (`internal/service/auth.go:42-83`): `Register` calls `CountUsers` (line 53) then `CreateUser` with `IsAdmin: count == 0` (line 81) with no transaction. The comment at lines 63-65 acknowledges the email-collision race but not the admin-grant race. Two concurrent first registrations with different emails both observe `count == 0` and both insert as admin. Confirmed. - **Finding 2** (`internal/api/auth.go:44-58`): `Register` commits the user (line 44-52), then `startSession` is called separately (line 54). If `startSession` fails, the handler returns 500 (line 55) while the user row already exists; a retry hits `ErrEmailTaken` → 409. Confirmed. **Verdict: Minor issues found** I verified the new files end-to-end against the schema, domain types, config, and callers. The core auth/session logic is correct: timing equalization returns `ErrInvalidCredentials` regardless of `dummyHash` parse failure (return values discarded), expiry check `!now.Before(exp)` is right, sliding renewal (`newExp.Sub(exp) > time.Hour`) only writes when meaningfully moved and always renews near expiry, lexicographic ISO-8601 UTC comparison for `DeleteExpiredSessions` is valid for the fixed-width `Z` format, and `PasswordHash` carries `json:"-"` so `/me` can't leak it. Two minor correctness gaps: - `internal/service/auth.go:77` (`Register`) — first-user-is-admin is decided by `CountUsers` then `CreateUser` with `IsAdmin: count == 0`, with no transaction or post-insert guard. Two concurrent first registrations with *different* emails both observe `count == 0` and both insert as admin (the UNIQUE index only guards email collisions, which the comment acknowledges; it does not guard the admin grant). Result: two admins on a fresh instance. Low likelihood at household scale, but it is a real race in the admin-grant logic. Suggested fix: grant admin inside the insert only when a `count == 0` check is held under a transaction, or recompute/`UPDATE users SET is_admin=1 WHERE id=(SELECT MIN(id) FROM users)` post-hoc. - `internal/api/auth.go:54` (`register`) — `Register` commits the user, then `startSession` is called separately. If `startSession` fails (DB/token error), the handler returns 500 but the user row already exists. A client retry of `/register` then gets `EMAIL_TAKEN` (409) and is forced through `/login` instead of being told to log in. The account is created in an "orphaned, not logged in" state. Suggested fix: either fold session creation into the service `Register` so it's atomic-ish, or return a clearer error/body that tells the client the account was created and to log in. Neither is blocking; both are edge cases. The rest of the change is correct under the correctness lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> ## Verdict: No material issues found Through the code-cleanliness & maintainability lens, this change is well-structured and follows the patterns the surrounding code establishes. Verified by reading the actual checked-out files: - **Layering is clean and consistent.** Handlers (`internal/api/auth.go`) stay thin — bind, call a service method, encode. The service (`internal/service/auth.go`) owns all logic/invariants. The store (`internal/store/users.go`, `sessions.go`) is mechanical SQL. This matches the stated seam design and the existing `domain`/`store`/`api` split. - **No dead/duplicated code of note.** `scanUser` + the `scanner` interface + the shared `userColumns` constant correctly dedup the three user queries (`users.go:13,24`). `startSession` is correctly shared by both `register` and `login` (`auth.go:128`). The near-identical register/login handler bodies differ only in input type and error message, which is acceptable. - **Names are clear and consistent** with the rest of the package (`ErrNotFound`/`ErrForbidden` style extended cleanly by the new sentinels in `domain.go:21-37`; `formatTime`/`parseTime`/`hashToken`/`newSessionToken` are self-describing). - **No leaky abstractions.** The raw token never leaves the service→cookie boundary; only `hashToken` is persisted. `domain.User.PasswordHash` is `json:"-"` so `/me` can't leak it (verified at `domain.go:73`). - `boolToInt` (`users.go:96`) is a generic helper living in `users.go` rather than `sqlite.go`, but it's the only bool-column table right now, so the placement is fine. One truly trivial nit (not material): `setSessionCookie` and `clearSessionCookie` (`auth.go:145,151`) each repeat `c.SetSameSite(http.SameSiteLaxMode)` before `c.SetCookie`; a tiny `writeSessionCookie(c, token, maxAge)` helper would collapse the two to one call site each. Not worth churning on its own. No blocking or minor cleanliness issues to report. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found. Performance review of the auth/session layer: - **Hot path (`ResolveSession`, on every authenticated request):** `GetSession` is a primary-key lookup on `token_hash` (`sessions.token_hash` is PK per `migrations/0001_init.sql:33`), and `GetUserByID` is a PK lookup. The sliding-renewal `TouchSession` write fires at most once per hour per session (`newExp.Sub(exp) > time.Hour`), so it doesn't write on every request. Two indexed reads + an occasional indexed update is fine; no N+1, no unbounded work. - **`Login`/`Register`:** `GetUserByEmail` hits the `UNIQUE ... COLLATE NOCASE` index (`migrations:16`); `CountUsers` is a single `count(*)`. `CreateUser` inserts then re-reads by PK. No quadratic behavior. - **Argon2id (64 MiB / t=1 / p=4):** deliberately heavy on `Login`/`Register`, including the dummy-hash equalizer for unknown emails. That's a security tradeoff, not a performance regression, and these are low-frequency endpoints. - **`DeleteExpiredSessions`:** `DELETE FROM sessions WHERE expires_at <= ?` has no index on `expires_at` (only `idx_sessions_user` on `user_id`), so it's a full scan. But it runs on a background ticker every 6h (`sessionSweepInterval`), and the sessions table is bounded by the user base (household scale) plus lazy deletion on access. Not a hot path; not material enough to flag. - **`sweepSessions`:** one sweep at startup then every 6h; failures logged, never fatal. No goroutine leak (ticker stopped via `defer`, returns on `ctx.Done()`). No efficiency regressions, unbounded growth, or hot-path blockers in this change. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> All findings verified against the actual source. Every bullet accurately describes the code, and all are explicitly self-documented/accepted tradeoffs rather than material defects. The draft's own `gadfly-findings` array is empty, consistent with "no actionable issues." Recomputing the verdict accordingly. VERDICT: No material issues found - **`internal/service/auth.go:97,103` — `dummyHash` can be empty, causing `verifyPassword` to run with a malformed hash (confirmed).** In `New` (`service.go:47-51`), if `hashPassword` fails, `s.dummyHash` stays `""`. `Login` then calls `verifyPassword("")`, which goes through `decodeHash("")` → `strings.Split("", "$")` returns `[""]` (length 1) → `errBadHash`. The return value is discarded (`_, _ =`), so no panic; `ErrInvalidCredentials` is still returned correctly. The only loss is the timing-equalization defense, which the code explicitly documents as acceptable. Self-documented edge case, not a bug. - **`internal/service/auth.go:159-160` — `TouchSession` error is silently swallowed (confirmed).** `_ = s.store.TouchSession(...)` discards the error. If the DB update fails, the stored expiry is stale; the next request retries within the >1h window, or the session is lazily deleted once it expires. No security or correctness break — just a missed renewal. Benign at the stated scale. - **`internal/service/auth.go:149,155` — `DeleteSession` errors during lazy cleanup are swallowed (confirmed).** Corrupt-expiry and expired-session cleanup both ignore the delete error. A failed delete leaves the row to be re-attempted on next access or reaped by the background sweep. The session is already treated as absent, so no correctness issue. - **`internal/service/auth.go:146-151` — corrupt expiry parsing is fully covered (confirmed).** `parseTime` uses `time.Parse(timeLayout, sess.ExpiresAt)`; any non-conforming value triggers the `DeleteSession`+`ErrNotFound` path. Correct. - **`internal/api/auth.go:146` — cookie `maxAge` truncation to seconds is benign (confirmed).** `int(time.Until(expiresAt).Seconds())` floors toward zero; the `max(..., 1)` guard prevents a zero/negative `maxAge` (which Gin/`http.SetCookie` treats as "delete now"). For a 30-day TTL a sub-second remainder is irrelevant. - **`internal/store/users.go:42-55` — `CreateUser` race on duplicate email surfaces as a generic 500 (confirmed, documented).** The friendly `GetUserByEmail` pre-check + UNIQUE index race is acknowledged in the comment at `auth.go:63-65`. Under concurrency the loser hits the UNIQUE constraint and gets a wrapped `store: insert user` error → 500. A real but low-probability edge case, explicitly scoped as acceptable. Mapping the constraint violation to `ErrEmailTaken` (409) would be an improvement but is a known tradeoff, not an oversight. No panics, no nil dereferences, no ignored critical errors, and no missing `defer`/cleanup found. The unhappy paths (empty token, garbage token, expired session, malformed hash, OIDC-only login, blank input, disabled local auth, closed registration) are all handled and mapped to appropriate sentinels/status codes. `sweepSessions` correctly checks `ctx.Err()` before logging and exits on `ctx.Done()`. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> **Blocking issues found** - **TOCTOU race allows multiple bootstrap admins on a fresh instance** `internal/service/auth.go:53-82` — `Register` reads `CountUsers`, checks `count > 0 && !RegistrationOpen()`, then calls `CreateUser`. Two concurrent requests on a fresh DB can both observe `count == 0`, pass the closed-registration gate, and both create accounts with `IsAdmin: true` (as long as they use different emails). This breaks the intended “exactly one bootstrap admin” security model and lets an attacker race the legitimate owner during initial setup. **Fix:** Wrap the count-check-and-insert in a SQLite transaction (or use a partial unique index `ON users(is_admin) WHERE is_admin = 1` so the second admin insert fails with a unique violation that you map to `ErrRegistrationClosed`). - **Account-enumeration timing defense degrades silently on startup failure** `internal/service/service.go:45-52` — If `hashPassword` fails during `New` (e.g. `crypto/rand` exhaustion), `s.dummyHash` stays empty. Unknown-email logins then call `verifyPassword("", password)`, which returns `errBadHash` instantly without doing any argon2 work. Wrong-password logins for real users still burn the full argon2 cost, producing a measurable timing gap that defeats the equalization defense. **Fix:** If dummy-hash generation fails, make `Login` fall back to an explicit constant-time delay (e.g. `time.Sleep(estimatedArgon2Duration)`) rather than a no-op verify, or treat the failure as fatal on startup. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** - **Cookie `maxAge` is never refreshed, breaking the sliding window.** `ResolveSession` extends the server-side expiry via `TouchSession` (`internal/service/auth.go:159-161`), but `requireAuth` (`internal/api/auth.go:107-126`) never re-issues the `Set-Cookie` header. The browser’s cookie still carries the original `maxAge` (≈30 days from login). Once that expires, the browser drops the cookie and the active user is 401’d, even though the DB session was renewed. The PR claims a “sliding 30-day expiry,” but from the client perspective it is a fixed 30-day window. **Fix:** Have `requireAuth` call `setSessionCookie` with the updated expiry whenever `ResolveSession` succeeds and the session was touched (or on every authenticated request). - **Empty `dummyHash` nullifies the login timing defense.** In `Service.New` (`internal/service/service.go:47-51`), if `hashPassword` fails, `s.dummyHash` remains `""`. In `Login` (`internal/service/auth.go:95-104`), the unknown-email and OIDC-only paths call `verifyPassword(s.dummyHash, password)`, which returns `errBadHash` instantly without touching argon2. The wrong-password path on a real account, however, performs the full 64 MiB argon2 computation. The resulting timing gap is large and measurable, so an attacker can enumerate registered emails by comparing response times. The PR says “login still works but loses the timing defense,” but the actual behavior is an active account-enumeration side-channel. **Fix:** If dummy-hash generation fails, generate a deterministic placeholder at start-up (e.g., a hard-coded valid PHC string) so `verifyPassword` always executes comparable work. - **First-user-is-admin race condition.** `Register` (`internal/service/auth.go:53-81`) reads `CountUsers`, then later calls `CreateUser`. Two concurrent requests on a fresh database both see `count == 0`, both pass the closed-registration bootstrap, and both create admin accounts. There is no atomic guard (e.g., `INSERT … WHERE (SELECT count(*)=0)` or a conditional unique constraint on `is_admin`). **Fix:** Make the first-user check atomic—either with a single SQL statement that inserts only when the users table is empty, or by catching a unique-constraint-like race at the DB level. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **No material issues found** The PR is clean and well-structured through the maintainability lens. Functions are appropriately sized and single-purpose, naming is consistent, the service/API/store layering is clean, and there's no dead or duplicated production code. Test helpers have minor cross-package duplication (`authEngine` vs `newTestService`), but that's acceptable isolation between layers. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** — almost. One genuine inefficiency. * `internal/store/migrations/0001_init.sql:38` — The `sessions` table has only `idx_sessions_user` on `user_id`, but `DeleteExpiredSessions` (`internal/store/sessions.go:70`) queries `WHERE expires_at <= ?` and will table-scan the entire sessions table every 6 hours during the background sweep. For a long-running instance with accumulated stale sessions this is an avoidable full scan. **Fix:** Add `CREATE INDEX idx_sessions_expires ON sessions (expires_at);` to the schema so the sweep and any future expiry-based queries use the index. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`internal/service/auth.go:160`** — `ResolveSession` silently discards the error returned by `s.store.TouchSession(...)` when renewing a sliding session. If the update fails (e.g., database lock, disk full), the user’s session is *not* extended, yet the current request succeeds with a 200. The next request may then find an unexpectedly expired session and force a re-login. The error should be logged so operators can detect the write failure. - **`internal/service/auth.go:149`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the corrupt-expiry path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. - **`internal/service/auth.go:155`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the already-expired path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. - **`internal/service/service.go:47`** — If `hashPassword` fails during `service.New` (e.g., `crypto/rand.Read` fails at startup), `s.dummyHash` remains the empty string. Later, in `auth.go:97` and `auth.go:103`, `verifyPassword("", password)` returns `errBadHash` *instantly* without performing any argon2 work. This destroys the timing-equalization defense: an attacker can now distinguish “unknown email” (fast path) from “wrong password” (slow argon2 path) by measuring response time. The initialization path should either retry, fail hard, or at minimum log and refuse to process login requests when the dummy hash is missing. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> Both findings are confirmed. The login CSRF concern: no `Origin`/`Referer` validation anywhere; SameSite=Lax does not prevent cross-site POST setting a new cookie. The cookie Secure default concern: `cookieSecure()` returns false when BaseURL is empty/default, confirmed at lines 141-143 and config.go line 70. **VERDICT: Minor issues** - **Login CSRF via unvalidated Origin on auth POSTs** (`internal/api/auth.go:62-92`, `internal/api/auth.go:37-59`). The session is a cookie (HttpOnly, SameSite=Lax), and SameSite=Lax blocks sending *existing* cookies on cross-site POSTs but does **not** block a cross-site POST to `/auth/login` from being processed and from `Set-Cookie` taking effect on the victim's browser. An attacker page can `fetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …})` with the attacker's own credentials; the victim's browser accepts the resulting `pansy_session` cookie and is now silently logged in as the attacker (classic login CSRF), letting the attacker observe any data the victim then enters. `Register` and `logout` are similarly POSTable cross-site (logout is a state change, lower impact but still CSRF-relevant). Verified there is no `Origin`/`Referer` check anywhere (`grep` for `CSRF|Origin|Referer` returned nothing). Suggested fix: validate the `Origin` header against the configured base URL (or a same-origin check) on every state-changing POST under `/auth`, since this is cookie-auth with no CSRF token. Note the PR scopes rate limiting to "later" but doesn't explicitly defer CSRF, so flagging. - **Session cookie defaults to non-Secure when `PANSY_BASE_URL` is unset** (`internal/api/auth.go:141-143`, `internal/config/config.go:70`). `cookieSecure()` only returns true when `BaseURL` starts with `https://`, and the default `BaseURL` is `""`. A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to set `PANSY_BASE_URL` gets a session cookie without `Secure`; any same-host HTTP request (or downgrade/MITM) then leaks the bearer token. This is documented as intentional, but "secure by default" would be to set `Secure` whenever the request itself arrived over TLS (`r.TLS != nil`) rather than relying solely on an env var, or at minimum to log a loud warning at startup when serving real traffic without `Secure`. Severity small — it's a footgun in default configuration, not a code bug. What I checked and found *clean* (no finding): `domain.User.PasswordHash`/`OIDCIssuer`/`OIDCSubject` carry `json:"-"` (`internal/domain/domain.go:73-75`), so `c.JSON(http.StatusOK, user)` in register/login/me cannot leak the hash; all SQL uses `?` placeholders (no injection); argon2id uses a random 16-byte salt, constant-time compare, and the dummy-hash timing equalizer in `Login` runs argon2 on the unknown-email/OIDC-only paths (`internal/service/auth.go:95-104`); session tokens are 32 random bytes with only the sha256 stored (`internal/service/service.go`); `startSession` always mints a fresh token on login/register (no session fixation); cookie is scoped to the request host (empty domain) and `HttpOnly`. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings verified against actual source. **Finding 1 verified:** `ResolveSession` (auth.go:159-161) calls `s.store.TouchSession` to slide DB expiry. The cookie is set only in `startSession` → `setSessionCookie` (api/auth.go:145-149), called exclusively from `register` and `login`. `requireAuth` (api/auth.go:107-126) resolves the session and stashes the user but never re-sets the cookie. `/me` (line 95-97) just returns the actor. No path refreshes the cookie Max-Age after login. Confirmed. **Finding 2 verified:** `Register` (auth.go:42-83) calls `CountUsers` (53), gates on `count > 0 && !RegistrationOpen()` (59), and sets `IsAdmin: count == 0` (81). Two concurrent first-registrations with distinct emails both observe `count == 0`, both bypass the closed-registration gate, both insert with `IsAdmin=true`. The UNIQUE email index prevents same-email duplicates but not two different admins. Confirmed. (The bootstrap-when-closed behavior itself is documented in the README, so I soften that sub-claim, but the TOCTOU race producing multiple admins is real.) ## VERDICT: Minor issues - **`internal/service/auth.go:159` / `internal/api/auth.go:145-149` — sliding renewal extends the DB session but never refreshes the cookie's Max-Age.** `ResolveSession` calls `s.store.TouchSession(...)` to push `expires_at` forward, but the cookie set at login (`setSessionCookie`) computes `maxAge` once from `expiresAt` and is never re-issued on `/me` or any subsequent request. So a user who keeps using the app will have their server-side session slid to "now + 30 days" indefinitely, but the browser will drop the `pansy_session` cookie ~30 days after the original login. After that the user is silently logged out despite a still-valid server-side row. The PR description advertises "Sliding 30-day expiry, renewed at most ~hourly," but the sliding only holds for the server half — the client half is fixed. Suggested fix: when `ResolveSession` renews (i.e. `TouchSession` runs), surface the new expiry so `requireAuth`/handlers can re-`Set-Cookie` with the bumped Max-Age; or set the cookie without a Max-Age (session-style) and rely solely on server-side expiry. - **`internal/service/auth.go:42-83` — first-user-is-admin is TOCTOU-prone: two concurrent first registrations both become admin.** `Register` calls `CountUsers` (line 53), gates on `count > 0` (line 59), and sets `IsAdmin: count == 0` (line 81). Two concurrent first-registrations with different emails both observe `count == 0`, both pass the closed-registration gate, and both insert with `IsAdmin=true`. The UNIQUE email index prevents same-email duplicates but NOT two different emails both becoming admin. The PR description claims "The first account registered becomes admin" (singular) — under contention you can end up with two admins. The author already accepts races for the duplicate-email path ("household scale"), but the admin-grant race is higher-impact than a 500. Suggested fix: after insert, re-check `CountUsers` and demote if `>1`, or use a single SQL statement that conditionally sets `is_admin = (NOT EXISTS (SELECT 1 FROM users))` so the admin decision is atomic with the insert. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## VERDICT: Minor issues found Reviewed through the code-cleanliness/maintainability lens. The code is generally well-structured, but a few documentation/comment mismatches stand out: - **`internal/service/service.go:1-6`** — The package doc claims "every operation is a method on `*Service` taking `(ctx, actor, args)`", but none of the methods added in this PR take an `actor` parameter (`Register(ctx, in)`, `Login(ctx, email, password)`, `CreateSession(ctx, userID)`, `ResolveSession(ctx, rawToken)`, `Logout(ctx, rawToken)`, `CleanupExpiredSessions(ctx)`). The comment is aspirational for a future issue and is currently inaccurate, which will confuse a reader landing here for the first time. Suggest softening to "operations are methods on `*Service` taking a context and args; protected operations will also take an actor once feature routers land." - **`internal/store/users.go:21-23`** — The `scanUser` comment says "it is read into an int and mapped", but the declared intermediate is `isAdmin int64`. The comment and the code disagree on the type. Minor, but a reader will trust the comment and be puzzled. Fix the comment to "read into an int64 and mapped" (or change the variable to `int`). - **`internal/service/password.go:27-28`** — The `errBadHash` doc says "Callers treat it as an auth failure but should log it." No caller logs it: `service.Login` (auth.go:107-110) folds `errBadHash` into `ErrInvalidCredentials` via `if err != nil || !ok` with no log, and the only other caller in tests asserts the error. So the "should log it" guidance is unfulfilled — either drop the prescription from the comment or actually log it in `Login` (a stored hash that won't parse is a real data/ops signal worth surfacing). Verified by reading `internal/service/auth.go`, `internal/service/password.go`, `internal/store/users.go`, and `internal/service/service.go` in full and grepping for `errBadHash` and `actor` usages. No dead code, no duplicated logic, no naming collisions worth flagging; the rest of the change follows the existing patterns in `api/` and `store/`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I reviewed this PR through the performance lens: hot-path queries, indexing, allocations, unbounded growth, blocking work, and the session sweep. I verified the schema (`internal/store/migrations/0001_init.sql`), the store query methods, and the service hot paths (`ResolveSession`, `Login`, `Register`). **Verdict: No material issues found** What I checked and why it's fine at this PR's stated (household) scale: - `ResolveSession` (the per-request hot path behind `requireAuth`) does two indexed lookups — `GetSession` on `token_hash` (PRIMARY KEY) and `GetUserByID` on `id` (PRIMARY KEY). Sliding renewal writes at most once per hour per session (`newExp.Sub(exp) > time.Hour`), so a busy client does not write on every request. Good. - `Login` spends 64 MiB / argon2 t=1 p=4 per attempt, including the dummy-hash path for unknown emails. That is a deliberate security cost (timing equalization, called out in the diff), not a regression, and login is human-driven, not a tight loop. - `Register` runs `CountUsers` + `GetUserByEmail` + insert + `GetUserByID`. `CountUsers` is a full `count(*)` but the users table is tiny at this scale; only invoked on signup, not on any hot path. - `DeleteExpiredSessions` issues `DELETE FROM sessions WHERE expires_at <= ?` with no index on `expires_at` (only `idx_sessions_user` exists per `0001_init.sql:38`), so each 6-hour sweep is a full table scan. The table is bounded by ~30 days of logins and expired rows are also dropped lazily on access, so the scan stays trivial. Not worth an index at this scale; would only matter if sessions grew into the tens of thousands, which the threat model (no rate limiting yet) doesn't push toward. - `sweepSessions` correctly stops its ticker on `ctx.Done()` — no goroutine leak. Nothing here is a material efficiency regression. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> I'll verify each finding against the actual checked-out code. All four findings verified against actual code. **Finding 1** ✓ — auth.go:54-57 & 75-78: `startSession` error → generic 500, no `slog.Error`. Confirmed `logout` (line 87) and `writeServiceError` default (line 177) do log. Confirmed. **Finding 2** ✓ — service/auth.go:149, 155, 160: `_ = s.store.DeleteSession/TouchSession(...)` discard errors silently. Confirmed. **Finding 3** ✓ — service.go:47-51: on `hashPassword` error, `dummyHash` stays `""`. password.go:62-65: `strings.Split("", "$")` → `[""]` (len 1 ≠ 6) → `errBadHash` returned before any argon2 work. Confirmed short-circuit, zero argon2 work, defeats timing equalizer. **Finding 4** ✓ — auth.go:146: `max(int(time.Until(expiresAt).Seconds()), 1)` clamps negative to 1, not clearing. Confirmed; trivial since `expiresAt = now + 30d`. --- **VERDICT: Minor issues** - **`internal/api/auth.go:54-57` & `:75-78` — `startSession` failure is swallowed (unlogged).** When `CreateSession` returns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. The `logout` handler (auth.go:87) and `writeServiceError`'s default branch (auth.go:177) both `slog.Error` the cause; these two paths bypass that. For `register` this is especially bad: the user row is already committed, so the client sees a 500 with no cookie, retries, and gets `EMAIL_TAKEN` — an operator-invisible confusing sequence. Fix: log the error before writing the 500 (`slog.Error("api: start session", "error", err)`), and ideally route the underlying error through `writeServiceError`'s default branch. - **`internal/service/auth.go:149, 155, 160` — store errors from `DeleteSession`/`TouchSession` silently dropped.** `ResolveSession` discards all three return errors with `_ =`. The lazy-delete paths (149 corrupt expiry, 155 expired) are mostly self-healing, but a *persistent* DB failure here leaves the user stuck at 401 with no diagnostic in the logs — the row never gets removed. The `TouchSession` swallow (160) silently disables sliding renewal on write failure. Fix: `slog.Warn` these so a degrading DB is observable. - **`internal/service/auth.go:97, 103` + `internal/service/service.go:47-51` — empty `dummyHash` defeats the timing equalizer with no runtime signal.** If `hashPassword` fails in `New` (only path: `crypto/rand.Read` failure), `s.dummyHash` stays `""`. `verifyPassword("", password)` short-circuits in `decodeHash` (verified: `strings.Split("", "$")` → `[""]`, length 1 ≠ 6 → `errBadHash`) doing **zero** argon2 work. Then unknown-email returns ~instantly while wrong-password pays full argon2 cost — the exact account-enumeration the code claims to prevent. The startup `slog.Warn` is easy to miss; the failure is silent for the process lifetime. Fix: in `Login`, if `s.dummyHash == ""`, either recompute it lazily under a mutex or fall back to a hardcoded known-good hash string constant. - **`internal/api/auth.go:146` — `maxAge` floor of 1 masks clock-skew bugs.** `max(int(time.Until(expiresAt).Seconds()), 1)` clamps a negative duration to a 1-second cookie instead of clearing it. Not currently reachable (`expiresAt = now + 30d`), but the floor silently swallows a clock-skew class of bug. Trivial; leave or change to clear the cookie when `time.Until(expiresAt) <= 0`. </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** - **Auth flow**: `requireAuth` correctly gates `/auth/me` and resolves the session cookie → user via `ResolveSession`; any failure is 401. The cookie is `HttpOnly`, `SameSite=Lax`, and `Secure` only when the base URL is HTTPS. Session tokens are 32 random bytes (CSPRNG) and only their SHA-256 is persisted. - **Password handling**: Uses argon2id with standard PHC encoding; salts are 16 bytes from `crypto/rand`; verification uses `subtle.ConstantTimeCompare`. The `dummyHash` timing equalizer in `Login` makes unknown-email and wrong-password paths indistinguishable in timing. - **Injection resistance**: All store methods use parameterized queries (`?` placeholders); `userColumns` is a hardcoded constant. No SQL injection surface. - **Input validation**: `registerRequest` validates email format and password length (8–1024) via gin binding; `loginRequest` validates email format. The service layer re-checks for emptiness as defense-in-depth. - **Secret leakage**: `PasswordHash`, `OIDCIssuer`, and `OIDCSubject` carry `json:"-"` tags, so they are excluded from JSON serialization. No credentials or raw tokens are logged. - **Session lifecycle**: `CreateSession` issues a new token on every login/register (no fixation risk). `Logout` is idempotent and clears the client cookie even if the server-side session is already gone. Sliding expiry and lazy deletion are correctly implemented. Verified by reading `internal/api/auth.go`, `internal/service/auth.go`, `internal/service/password.go`, `internal/service/service.go`, `internal/store/sessions.go`, `internal/store/users.go`, `internal/domain/domain.go`, `internal/config/config.go`, and the migration schema. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Verdict:** Blocking issues found - **`internal/service/auth.go:53-82` — Race condition: concurrent first registrations can create multiple admins** `CountUsers` and `CreateUser` are separate, non-transactional queries. Under WAL mode two goroutines can both read `count == 0` before either writes, then both insert with `IsAdmin: true`. The UNIQUE guard only protects `email`, not `is_admin`, so two simultaneous sign-ups on a fresh instance both become admin. **Fix:** Perform the count-and-insert in a single atomic step (e.g., `INSERT ... SELECT CASE WHEN EXISTS(SELECT 1 FROM users) THEN 0 ELSE 1 END`) or wrap the logic in a `BEGIN IMMEDIATE` transaction so the read locks the table for writing. - **`internal/service/auth.go:49` — Service layer does not enforce the 1024-character password ceiling** `Register` validates `in.Password == ""` but never checks `len(in.Password) > 1024`. The API binding catches it today, but the PR positions `internal/service` as the seam for “future agent tools”. A direct caller (CLI, agent, test helper) can pass an arbitrarily large password, and the service will hash it rather than reject it. **Fix:** Add `len(in.Password) > 1024` to the service-level validation, returning `domain.ErrInvalidInput`. - **`internal/service/auth.go:107-110` — Malformed password hash silently returned as wrong-password with no log** `verifyPassword` returns `errBadHash` when a stored hash is malformed (corruption, partial write, etc.). The `password.go` comment explicitly says *“Callers treat it as an auth failure but should log it”*, yet `Login` maps it straight to `domain.ErrInvalidCredentials` with no `slog.Error`. An operator has no server-side signal that data corruption is locking a user out. **Fix:** Log the malformed-hash error in `Login` before returning `ErrInvalidCredentials`, e.g. `slog.Error("service: user has malformed password hash", "user_id", u.ID, "error", err)`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Minor issues - **`internal/api/auth.go:25-28`** Password length validation only in HTTP binding tags, not service layer — `registerRequest` enforces `min=8,max=1024` via gin binding tags, but `service.Register` (`internal/service/auth.go:48-50`) only checks `in.Password == ""`. The service layer is documented as the place where "all permission checks and invariants live" (`internal/service/service.go:2-4`), so a caller bypassing REST (e.g. an agent tool or internal caller) will skip the length check. **Fix:** Move the `min=8` / `max=1024` check (and the display-name non-empty check) into `service.Register` and return `domain.ErrInvalidInput`. The handler can keep the binding tags as a fast-fail convenience, but the service should be the single source of truth. Also update the hard-coded error message at `internal/api/auth.go:40` so it doesn't duplicate the rule. - **`internal/api/auth.go:164`** `writeServiceError` method has unused receiver — Declared as `func (h *handlers) writeServiceError(...)`, but `h` is never referenced inside the body, which is a small readability misdirection. **Fix:** Change it to a package-level function: `func writeServiceError(c *gin.Context, err error)`. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Minor issues** - `internal/store/migrations/0001_init.sql:32` — **Missing index on `sessions.expires_at`**. `DeleteExpiredSessions` runs `DELETE FROM sessions WHERE expires_at <= ?` every 6 hours. Without an index on `expires_at`, SQLite performs a full table scan. At household scale this is tolerable today, but sessions accumulate over time (multiple devices, long-lived tokens) and the sweep will degrade linearly. **Fix:** Add `CREATE INDEX idx_sessions_expires ON sessions (expires_at);` alongside the existing `idx_sessions_user`. - `internal/service/auth.go:141` — **Two DB round-trips on every authenticated request** (observation, not blocking). `ResolveSession` calls `GetSession` (by PK) and then `GetUserByID` (by PK). Both are indexed and fast, but every protected endpoint pays two query overheads. A single JOIN query would halve the DB work on the hot path. Verified by reading `store/sessions.go` and `store/users.go` — neither currently exposes a joined fetch. **Fix:** Consider adding `GetSessionWithUser` in a follow-up if profiling shows it matters. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues** - **`internal/service/auth.go:160`** — `ResolveSession` silently discards the error from `s.store.TouchSession`. If the DB write fails (busy, context cancelled, disk issue), the session expiry does **not** slide forward, so an active user can be logged out unexpectedly when the original TTL expires. The error should at least be logged so operators can detect when sliding renewal is failing. - **`internal/service/auth.go:53-82`** — `Register` reads `CountUsers` and then `CreateUser` in separate transactions. On a fresh instance (or with `PANSY_REGISTRATION=closed`), concurrent bootstrap requests can both observe `count == 0`, so **more than one user becomes admin**. The PR acknowledges the duplicate-email race (handled by the UNIQUE index), but not this first-user admin race. - **`internal/service/auth.go:149,155`** — `ResolveSession` ignores `DeleteSession` errors when dropping corrupt or expired sessions. A persistent DB fault can leave garbage rows that `ResolveSession` will keep re-discovering and re-failing on every request until the background sweep happens to succeed. - **`internal/service/auth.go:107-110`** — `Login` swallows `verifyPassword`'s `errBadHash`, returning only `ErrInvalidCredentials` with no log entry. If a stored hash is ever corrupted, the user gets a permanent “wrong password” response and there is no server-side trace to diagnose it. - **`internal/api/auth.go:31-34`** — `loginRequest` has no `max` tag on `Password`, while `registerRequest` caps it at 1024. This is an unhandled boundary condition: login accepts arbitrarily long passwords, which is inconsistent and opens a trivial memory-pressure DoS vector against the argon2 verifier. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-18 21:04:37 +00:00
Address Gadfly review on #4: TOCTOU, sliding cookie, CSRF, argon2
Build image / build-and-push (push) Successful in 5s
02c928ac6d
Fixes from the PR #23 adversarial review (graded 35 real / 1 false positive):

Security / correctness
- Race-free registration: is_admin and the registration gate are now
  computed atomically inside a single INSERT...SELECT, so concurrent
  first registrations can't both become admin or bypass closed
  registration (fixed the whole TOCTOU cluster).
- Sliding session now reaches the browser: ResolveSession returns the
  current expiry and requireAuth re-sets the cookie, so active users
  aren't logged out 30 days after login regardless of activity.
- Login CSRF: csrfGuard rejects state-changing requests whose Origin
  doesn't match PANSY_BASE_URL (no-op when unset, so the dev proxy is
  unaffected). SameSite=Lax alone didn't cover this.
- argon2id tuned to RFC 9106's second recommended profile (t=3).
- Timing equalizer can't fail open: the dummy hash is derived
  deterministically (fixed salt, no RNG) so it's always present.
- Password length (<=1024) enforced in the service for both register
  and login, not just HTTP binding tags; login rejects over-long input
  before spending argon2 work.

Error handling / robustness
- Login logs a malformed stored hash instead of silently treating it as
  a wrong password.
- Best-effort session writes (Touch/Delete during renewal, expiry, and
  corrupt-expiry cleanup) now log on failure.
- index sessions.expires_at via new migration 0002 (0001 is immutable).

Maintainability
- Extract startSessionAndRespond and abortUnauthenticated; make
  writeServiceError a free function; consistent error handling in
  decodeHash; doc/comment fixes.

Tests: over-long password, CSRF guard (cross-origin/same-origin/dev
no-op), and cookie refresh on authenticated requests; migration-version
assertions bumped to 2.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve merged commit eb03d09d19 into main 2026-07-18 21:05:01 +00:00
steve deleted branch phase-1-local-auth 2026-07-18 21:05:01 +00:00
Sign in to join this conversation.