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).
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)
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
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>
🪰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>
🟠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>
🟠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…
🪰 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>
🟡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>
🔴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>
⚪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>
🟡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>
⚪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.
🪰 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>
internal/api/auth.go:164writeServiceError 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>
🟠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 ceilingRegister 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>
🔴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 instanceinternal/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…
🪰 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>
🟡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>
🟠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>
🟠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>
🟠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 logverifyPassword 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>
🟡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-…
🪰 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>
🟡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.
🪰 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>
🟡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.
🪰 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>
🟠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>
🟡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>
🟡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>
🔴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>
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 detailclaude-code/sonnet (claude-code) — Blocking issues found
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.
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-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 shadowederr, 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 returnerr, 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.goResolveSession (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.goRegister — 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.goDeleteExpiredSessions 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:20sessionSweepInterval) 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(...)).
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.
🎯 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,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().
kimi-k2.6:cloud (ollama-cloud) — 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.
🎯 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: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 errBadHashinstantly 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.
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 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.
🧹 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 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.
opencode/kimi-k2.6:cloud (opencode) — Blocking 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:164writeServiceError 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: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.
🧯 Error handling & edge cases — Minor issues
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.
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>
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 main2026-07-18 21:05:01 +00:00
steve
deleted branch phase-1-local-auth2026-07-18 21:05:01 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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_adminscanned 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=closedstill 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—Servicestruct, 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.requireAuthmiddleware resolves the cookie → actor in context; any resolution failure is 401 (never 404). Session cookie: HttpOnly, SameSite=Lax, Secure only whenPANSY_BASE_URLis https, path/.oidcin/auth/providersis 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)./meauth requirement, validation, providers, Secure-cookie-under-https./me→ server restart → session persists (DB-backed) → logout → 401; wrong-password and unknown-email both 401 with identicalINVALID_CREDENTIALSbody.Notes / out of scope
PANSY_LOCAL_AUTH=falsealready rejects local register/login here (config existed); #5 will flipproviders.oidcon and add the OIDC endpoints.🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 20:45:30Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 25 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -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-28Password length validation only in HTTP binding tags, not service layer —registerRequestenforcesmin=8,max=1024via gin binding tags, butservice.Register(internal/service/auth.go:48-50) only checksin.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
@@ -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
@@ -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-78—startSessionfailure is swallowed (unlogged). WhenCreateSessionreturns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. Thelogouthandler (auth.go:87) andwriteServiceError's default branch (auth.go:177) bothslog.Errorthe cause; these two paths bypass that. Forregisterthis is especially bad: the user row is already committed, so the client…🪰 Gadfly · advisory
@@ -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
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/loginfrom being processed and fromSet-Cookietaking effect on the victim's browser. An attacker page canfetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …})with the attacker's own cred…🪰 Gadfly · advisory
@@ -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(setsMax-Age) is only called fromstartSession(auth.go:129-136), which is only invoked byregister/login(auth.go:54,75).requireAuth(auth.go:107-126) callssvc.ResolveSession, which slidessessions.expires_atforward in the DB viaTouchSession(internal/service/auth.go:159-161) but never re-issues…🪰 Gadfly · advisory
@@ -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:111andinternal/api/auth.go:119— the two failure branches inrequireAuth(missing cookieandResolveSession error) write the exact samewriteAPIError(...) / c.Abort() / returntriplet. Verified by reading the middleware. Minor, but a one-lineunauthorized(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
@@ -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
PANSY_BASE_URLis unset (internal/api/auth.go:141-143,internal/config/config.go:70).cookieSecure()only returns true whenBaseURLstarts withhttps://, and the defaultBaseURLis"". A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to setPANSY_BASE_URLgets a session cookie withoutSecure; any same-host HTTP request (or downgrade/MITM) then leaks the bearer token. This is documented as inten…🪰 Gadfly · advisory
@@ -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:146—maxAgefloor 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 whentime.Until(expiresAt) <= 0.🪰 Gadfly · advisory
@@ -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:164writeServiceErrormethod has unused receiver — Declared asfunc (h *handlers) writeServiceError(...), buthis 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
@@ -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 ceilingRegistervalidatesin.Password == ""but never checkslen(in.Password) > 1024. The API binding catches it today, but the PR positionsinternal/serviceas 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: Addlen(in.Password) > 1024to the service-level vali…🪰 Gadfly · advisory
@@ -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
internal/service/auth.go:53-82—RegisterreadsCountUsers, checkscount > 0 && !RegistrationOpen(), then callsCreateUser. Two concurrent requests on a fresh DB can both observecount == 0, pass the closed-registration gate, and both create accounts withIsAdmin: 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
@@ -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 byCountUsersthenCreateUserwithIsAdmin: count == 0, with no transaction or post-insert guard. Two concurrent first registrations with different emails both observecount == 0and 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
@@ -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
@@ -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— emptydummyHashdefeats the timing equalizer with no runtime signal. IfhashPasswordfails inNew(only path:crypto/rand.Readfailure),s.dummyHashstays"".verifyPassword("", password)short-circuits indecodeHash(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
@@ -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 logverifyPasswordreturnserrBadHashwhen a stored hash is malformed (corruption, partial write, etc.). Thepassword.gocomment explicitly says “Callers treat it as an auth failure but should log it”, yetLoginmaps it straight todomain.ErrInvalidCredentialswith noslog.Error. An operator has no server-side signal that data corruption is locking a user out. Fix: Log…🪰 Gadfly · advisory
@@ -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:141— Two DB round-trips on every authenticated request (observation, not blocking).ResolveSessioncallsGetSession(by PK) and thenGetUserByID(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 readingstore/sessions.goandstore/users.go— neither currently exposes a joined fetch. Fix: Consider addingGetSessionWithUserin a follow-…🪰 Gadfly · advisory
@@ -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:149—ResolveSessionalso silently discards errors froms.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
@@ -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:155—ResolveSessionalso silently discards errors froms.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
@@ -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.ResolveSessioncallss.store.TouchSession(...)to pushexpires_atforward, but the cookie set at login (setSessionCookie) computesmaxAgeonce fromexpiresAtand is never re-issued on/meor 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
@@ -0,0 +17,4 @@// later leaves old hashes verifiable.const (argonMemKiB = 64 * 1024 // 64 MiBargonTime = 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
@@ -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— TheerrBadHashdoc says "Callers treat it as an auth failure but should log it." No caller logs it:service.Login(auth.go:107-110) foldserrBadHashintoErrInvalidCredentialsviaif err != nil || !okwith 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 inLogin(a stored hash that won't parse is a real data/ops s…🪰 Gadfly · advisory
@@ -0,0 +66,4 @@}var version intif _, 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
@@ -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
@@ -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
dummyHashnullifies the login timing defense. InService.New(internal/service/service.go:47-51), ifhashPasswordfails,s.dummyHashremains"". InLogin(internal/service/auth.go:95-104), the unknown-email and OIDC-only paths callverifyPassword(s.dummyHash, password), which returnserrBadHashinstantly 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
@@ -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
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 26 findings (9 with multi-model agreement)
internal/service/auth.go:53internal/api/auth.go:54internal/service/auth.go:159internal/service/auth.go:149internal/api/auth.go:107internal/service/service.go:45internal/service/auth.go:46internal/service/auth.go:97internal/service/auth.go:15517 single-model findings (lower confidence)
internal/api/auth.go:25internal/service/auth.go:81internal/service/auth.go:107internal/store/migrations/0001_init.sql:32internal/api/auth.go:33internal/api/auth.go:62internal/api/auth.go:141internal/api/auth.go:164internal/service/auth.go:77internal/service/auth.go:141internal/service/password.go:20internal/service/password.go:27internal/service/service.go:2internal/store/users.go:22internal/api/auth.go:111internal/api/auth.go:146internal/service/password.go:69Per-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
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) andCreateUser(line 77) are separate round-trips with no transaction or lock between them, and theuserstable has no constraint limiting the number of admins (internal/store/migrations/0001_init.sqlonly has aUNIQUEonemailand(oidc_issuer, oidc_subject)). Two concurrentPOST /auth/registercalls hitting an empty table can both observecount == 0(internal/store/users.go:87-93) and both getIsAdmin: true; the same gap lets a registration slip through duringPANSY_REGISTRATION=closedbootstrap (auth.go:59-61). Note:internal/store/sqlite.go:50'sSetMaxOpenConns(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 atomicINSERT ... WHERE (SELECT COUNT(*) FROM users) = 0-style guard, or serializeRegisterwith 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 GiBwhen memory is abundant, or (2)t=3, p=4, m=64 MiBwhen 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. Suggestt=3(matching profile 2) or increasing memory ift=1is 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:
internal/store/users.go) — no injection.PasswordHash/OIDCIssuer/OIDCSubjectarejson:"-"(internal/domain/domain.go:73-75) — confirmed no credential leakage via/auth/me,/auth/register,/auth/loginresponses.crypto/rand, only sha256 persisted, raw value only in the HttpOnly cookie.verifyPassword(dummyHash, …)for timing equalization, and return the identicalErrInvalidCredentials.HttpOnly,SameSite=Lax,Securegated onhttps://prefix, emptyDomain(host-only).requireAuthfails 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.goRegistermatches, migrations 0001_init.sql:14-25 is theuserstable with no constraint preventing multipleis_admin=1rows).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(setsMax-Age) is only called fromstartSession(auth.go:129-136), which is only invoked byregister/login(auth.go:54,75).requireAuth(auth.go:107-126) callssvc.ResolveSession, which slidessessions.expires_atforward in the DB viaTouchSession(internal/service/auth.go:159-161) but never re-issues aSet-Cookie. Concretely: a user logs in on day 0 with a fixed ~30-day cookieMax-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) andinternal/store/users.go:42-93(CreateUser/CountUsers) — TOCTOU race on "first user is admin" / bootstrap-registration.RegistercallsCountUsersthen laterCreateUserwithIsAdmin: count == 0as two unsynchronized statements — no transaction, no locking. Verifiedstore.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 concurrentPOST /auth/registerrequests can race while the table is empty and both observecount == 0, both inserting withis_admin=1. The schema (0001_init.sql:14-25) has no constraint cappingis_admin=1to one row, confirming nothing else blocks this. The same race also lets more than one account bypass thePANSY_REGISTRATION=closedbootstrap check (internal/service/auth.go:59-61), since both requests observecount == 0.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/api/auth.go:54-58andinternal/api/auth.go:75-79—registerandloginend 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 extractingfunc (h *handlers) respondWithSession(c *gin.Context, user *domain.User)and calling it from both.internal/api/auth.go:111andinternal/api/auth.go:119— the two failure branches inrequireAuth(missing cookieandResolveSession error) write the exact samewriteAPIError(...) / c.Abort() / returntriplet. Verified by reading the middleware. Minor, but a one-lineunauthorized(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—decodeHashmixes error-handling styles inconsistently: the twofmt.Sscanfchecks (lines 69, 72) useif _, err := ...(a locally shadowederr, scoped only to the if-statement, shadowing the named return), while the twob64.DecodeStringchecks (lines 75, 78) useif salt, err = .../if key, err = ...(assigning the named returnerr,salt,keydirectly). Verified by reading the function — every branch does an explicitreturnwith 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 localerrthroughout, since nothing relies on the namederrbeing 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/Servicestructs are appropriately small, and the newusers.go/sessions.goestablish (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.goResolveSession(internal/service/auth.go:104) — runsGetSession(PK lookup onsessions.token_hash, which is the table'sPRIMARY KEY) then conditionallyTouchSession, thenGetUserByID(PK lookup). Verified againstinternal/store/migrations/0001_init.sql:32-38: both lookups hit primary-key indexes, no scans. This is the auth hot path (every futurerequireAuth-gated request), but it's 2 point-lookup queries against an embedded SQLite file — not a justified regression.internal/service/auth.goRegister— does an extraGetUserByEmailexistence 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.goDeleteExpiredSessionsfilters onexpires_at, which has no index (onlyidx_sessions_userexists per the migration). This causes a full table scan on every sweep, but the sweep runs once per 6h (cmd/pansy/main.go:20sessionSweepInterval) against a table that's bounded by active-user count — not material at the stated household/self-hosted scale.internal/service/password.goargon2id 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.internal/store/sqlite.go:50caps 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:
registerhandler at internal/api/auth.go:44-58 does callRegisterthenstartSessionwith no compensation on failure, andLogin's dummy-hash fallback at internal/service/auth.go:97/103 depends ons.dummyHashfrom service.go:47-50, which stays""ifhashPassworderrors —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) — Whenh.svc.Registersucceeds but the subsequenth.startSessioncall fails, the handler returns 500INTERNAL, but the user row fromRegisterwas already committed with no rollback. A retry ofPOST /auth/registerwith the same email then fails with 409EMAIL_TAKEN, leaving the caller stuck unless they think to try/auth/logininstead. 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— IfhashPasswordfails while precomputingdummyHashinNew(only possible ifcrypto/rand.Readfails),s.dummyHashstays"". Every subsequent unknown-email or OIDC-only login then callsverifyPassword(s.dummyHash, password), anddecodeHash("")(internal/service/password.go:62-66) immediately fails thelen(parts) != 6check, returning before any argon2 work.ErrInvalidCredentialsis still returned correctly, but the timing-equalizer this code exists for (per the comment atinternal/service/auth.go:85-87) is silently defeated, with no ongoing signal beyond the one-timeslog.Warnat 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: ifhashPasswordfails,s.dummyHashstays""(onlyslog.Warn).auth.go:97,103: unknown-email and OIDC-only branches callverifyPassword(s.dummyHash, password).password.go:62-66:decodeHash("")splits""into[""](len 1 ≠ 6) → returnserrBadHashimmediately, before anyargon2.IDKey.password.go:55: the real-password branch runs fullargon2.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-timehashPassword(...)inNewfails (e.g.crypto/randunavailable),s.dummyHashis left as the empty string"". InLogin, the unknown-email and OIDC-only branches then callverifyPassword(s.dummyHash, password), which hitsdecodeHash("")→errBadHashand 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 onlyslog.Warn'd and not otherwise enforced. Verified by readingservice.go(New leavesdummyHashunset on error) andpassword.go(decodeHashrejects""immediately before anyargon2.IDKey). Suggested fix: ifdummyHashcan't be precomputed, either refuse to start (log.Fatal/ return an error fromNew), 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 ofcrypto/randand only its sha256 is persisted; the cookie isHttpOnly+SameSite=Lax(which blocks cross-site POSTs, giving CSRF mitigation for these cookie-auth endpoints);PasswordHash/OIDC*carryjson:"-"so/meand register/login responses never leak secrets;requireAuthreturns 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.ErrEmailTakenon 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.
internal/service/auth.go:42-83):RegistercallsCountUsers(line 53) thenCreateUserwithIsAdmin: 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 observecount == 0and both insert as admin. Confirmed.internal/api/auth.go:44-58):Registercommits the user (line 44-52), thenstartSessionis called separately (line 54). IfstartSessionfails, the handler returns 500 (line 55) while the user row already exists; a retry hitsErrEmailTaken→ 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
ErrInvalidCredentialsregardless ofdummyHashparse 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 forDeleteExpiredSessionsis valid for the fixed-widthZformat, andPasswordHashcarriesjson:"-"so/mecan't leak it. Two minor correctness gaps:internal/service/auth.go:77(Register) — first-user-is-admin is decided byCountUsersthenCreateUserwithIsAdmin: count == 0, with no transaction or post-insert guard. Two concurrent first registrations with different emails both observecount == 0and 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 acount == 0check 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) —Registercommits the user, thenstartSessionis called separately. IfstartSessionfails (DB/token error), the handler returns 500 but the user row already exists. A client retry of/registerthen getsEMAIL_TAKEN(409) and is forced through/logininstead 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 serviceRegisterso 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:
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 existingdomain/store/apisplit.scanUser+ thescannerinterface + the shareduserColumnsconstant correctly dedup the three user queries (users.go:13,24).startSessionis correctly shared by bothregisterandlogin(auth.go:128). The near-identical register/login handler bodies differ only in input type and error message, which is acceptable.ErrNotFound/ErrForbiddenstyle extended cleanly by the new sentinels indomain.go:21-37;formatTime/parseTime/hashToken/newSessionTokenare self-describing).hashTokenis persisted.domain.User.PasswordHashisjson:"-"so/mecan't leak it (verified atdomain.go:73).boolToInt(users.go:96) is a generic helper living inusers.gorather thansqlite.go, but it's the only bool-column table right now, so the placement is fine.One truly trivial nit (not material):
setSessionCookieandclearSessionCookie(auth.go:145,151) each repeatc.SetSameSite(http.SameSiteLaxMode)beforec.SetCookie; a tinywriteSessionCookie(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:
ResolveSession, on every authenticated request):GetSessionis a primary-key lookup ontoken_hash(sessions.token_hashis PK permigrations/0001_init.sql:33), andGetUserByIDis a PK lookup. The sliding-renewalTouchSessionwrite 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:GetUserByEmailhits theUNIQUE ... COLLATE NOCASEindex (migrations:16);CountUsersis a singlecount(*).CreateUserinserts then re-reads by PK. No quadratic behavior.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 onexpires_at(onlyidx_sessions_useronuser_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 viadefer, returns onctx.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-findingsarray is empty, consistent with "no actionable issues." Recomputing the verdict accordingly.VERDICT: No material issues found
internal/service/auth.go:97,103—dummyHashcan be empty, causingverifyPasswordto run with a malformed hash (confirmed). InNew(service.go:47-51), ifhashPasswordfails,s.dummyHashstays"".Loginthen callsverifyPassword(""), which goes throughdecodeHash("")→strings.Split("", "$")returns[""](length 1) →errBadHash. The return value is discarded (_, _ =), so no panic;ErrInvalidCredentialsis 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—TouchSessionerror 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—DeleteSessionerrors 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).parseTimeusestime.Parse(timeLayout, sess.ExpiresAt); any non-conforming value triggers theDeleteSession+ErrNotFoundpath. Correct.internal/api/auth.go:146— cookiemaxAgetruncation to seconds is benign (confirmed).int(time.Until(expiresAt).Seconds())floors toward zero; themax(..., 1)guard prevents a zero/negativemaxAge(which Gin/http.SetCookietreats as "delete now"). For a 30-day TTL a sub-second remainder is irrelevant.internal/store/users.go:42-55—CreateUserrace on duplicate email surfaces as a generic 500 (confirmed, documented). The friendlyGetUserByEmailpre-check + UNIQUE index race is acknowledged in the comment atauth.go:63-65. Under concurrency the loser hits the UNIQUE constraint and gets a wrappedstore: insert usererror → 500. A real but low-probability edge case, explicitly scoped as acceptable. Mapping the constraint violation toErrEmailTaken(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.sweepSessionscorrectly checksctx.Err()before logging and exits onctx.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-82—RegisterreadsCountUsers, checkscount > 0 && !RegistrationOpen(), then callsCreateUser. Two concurrent requests on a fresh DB can both observecount == 0, pass the closed-registration gate, and both create accounts withIsAdmin: 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 = 1so the second admin insert fails with a unique violation that you map toErrRegistrationClosed).Account-enumeration timing defense degrades silently on startup failure
internal/service/service.go:45-52— IfhashPasswordfails duringNew(e.g.crypto/randexhaustion),s.dummyHashstays empty. Unknown-email logins then callverifyPassword("", password), which returnserrBadHashinstantly 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
Loginfall 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
maxAgeis never refreshed, breaking the sliding window.ResolveSessionextends the server-side expiry viaTouchSession(internal/service/auth.go:159-161), butrequireAuth(internal/api/auth.go:107-126) never re-issues theSet-Cookieheader. The browser’s cookie still carries the originalmaxAge(≈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
requireAuthcallsetSessionCookiewith the updated expiry wheneverResolveSessionsucceeds and the session was touched (or on every authenticated request).Empty
dummyHashnullifies the login timing defense.In
Service.New(internal/service/service.go:47-51), ifhashPasswordfails,s.dummyHashremains"". InLogin(internal/service/auth.go:95-104), the unknown-email and OIDC-only paths callverifyPassword(s.dummyHash, password), which returnserrBadHashinstantly 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
verifyPasswordalways executes comparable work.First-user-is-admin race condition.
Register(internal/service/auth.go:53-81) readsCountUsers, then later callsCreateUser. Two concurrent requests on a fresh database both seecount == 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 onis_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 (
authEnginevsnewTestService), 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— Thesessionstable has onlyidx_sessions_useronuser_id, butDeleteExpiredSessions(internal/store/sessions.go:70) queriesWHERE 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:160—ResolveSessionsilently discards the error returned bys.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—ResolveSessionalso silently discards errors froms.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—ResolveSessionalso silently discards errors froms.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— IfhashPasswordfails duringservice.New(e.g.,crypto/rand.Readfails at startup),s.dummyHashremains the empty string. Later, inauth.go:97andauth.go:103,verifyPassword("", password)returnserrBadHashinstantly 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/Referervalidation 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/loginfrom being processed and fromSet-Cookietaking effect on the victim's browser. An attacker page canfetch('…/api/v1/auth/login', {method:'POST', credentials:'include', …})with the attacker's own credentials; the victim's browser accepts the resultingpansy_sessioncookie and is now silently logged in as the attacker (classic login CSRF), letting the attacker observe any data the victim then enters.Registerandlogoutare similarly POSTable cross-site (logout is a state change, lower impact but still CSRF-relevant). Verified there is noOrigin/Referercheck anywhere (grepforCSRF|Origin|Refererreturned nothing). Suggested fix: validate theOriginheader 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_URLis unset (internal/api/auth.go:141-143,internal/config/config.go:70).cookieSecure()only returns true whenBaseURLstarts withhttps://, and the defaultBaseURLis"". A self-hoster who puts pansy behind an HTTPS-terminating reverse proxy but forgets to setPANSY_BASE_URLgets a session cookie withoutSecure; 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 setSecurewhenever 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 withoutSecure. 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/OIDCSubjectcarryjson:"-"(internal/domain/domain.go:73-75), soc.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 inLoginruns 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);startSessionalways mints a fresh token on login/register (no session fixation); cookie is scoped to the request host (empty domain) andHttpOnly.🎯 Correctness — Minor issues
Both findings verified against actual source.
Finding 1 verified:
ResolveSession(auth.go:159-161) callss.store.TouchSessionto slide DB expiry. The cookie is set only instartSession→setSessionCookie(api/auth.go:145-149), called exclusively fromregisterandlogin.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) callsCountUsers(53), gates oncount > 0 && !RegistrationOpen()(59), and setsIsAdmin: count == 0(81). Two concurrent first-registrations with distinct emails both observecount == 0, both bypass the closed-registration gate, both insert withIsAdmin=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.ResolveSessioncallss.store.TouchSession(...)to pushexpires_atforward, but the cookie set at login (setSessionCookie) computesmaxAgeonce fromexpiresAtand is never re-issued on/meor 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 thepansy_sessioncookie ~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: whenResolveSessionrenews (i.e.TouchSessionruns), surface the new expiry sorequireAuth/handlers can re-Set-Cookiewith 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.RegistercallsCountUsers(line 53), gates oncount > 0(line 59), and setsIsAdmin: count == 0(line 81). Two concurrent first-registrations with different emails both observecount == 0, both pass the closed-registration gate, and both insert withIsAdmin=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-checkCountUsersand demote if>1, or use a single SQL statement that conditionally setsis_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*Servicetaking(ctx, actor, args)", but none of the methods added in this PR take anactorparameter (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*Servicetaking a context and args; protected operations will also take an actor once feature routers land."internal/store/users.go:21-23— ThescanUsercomment says "it is read into an int and mapped", but the declared intermediate isisAdmin 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 toint).internal/service/password.go:27-28— TheerrBadHashdoc says "Callers treat it as an auth failure but should log it." No caller logs it:service.Login(auth.go:107-110) foldserrBadHashintoErrInvalidCredentialsviaif err != nil || !okwith 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 inLogin(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, andinternal/service/service.goin full and grepping forerrBadHashandactorusages. No dead code, no duplicated logic, no naming collisions worth flagging; the rest of the change follows the existing patterns inapi/andstore/.⚡ 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 behindrequireAuth) does two indexed lookups —GetSessionontoken_hash(PRIMARY KEY) andGetUserByIDonid(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.Loginspends 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.RegisterrunsCountUsers+GetUserByEmail+ insert +GetUserByID.CountUsersis a fullcount(*)but the users table is tiny at this scale; only invoked on signup, not on any hot path.DeleteExpiredSessionsissuesDELETE FROM sessions WHERE expires_at <= ?with no index onexpires_at(onlyidx_sessions_userexists per0001_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.sweepSessionscorrectly stops its ticker onctx.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:
startSessionerror → generic 500, noslog.Error. Confirmedlogout(line 87) andwriteServiceErrordefault (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
hashPassworderror,dummyHashstays"". password.go:62-65:strings.Split("", "$")→[""](len 1 ≠ 6) →errBadHashreturned 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 sinceexpiresAt = now + 30d.VERDICT: Minor issues
internal/api/auth.go:54-57&:75-78—startSessionfailure is swallowed (unlogged). WhenCreateSessionreturns an error (DB write failure, disk full, unique constraint), the handler returns a generic 500 envelope without logging the underlying cause. Thelogouthandler (auth.go:87) andwriteServiceError's default branch (auth.go:177) bothslog.Errorthe cause; these two paths bypass that. Forregisterthis is especially bad: the user row is already committed, so the client sees a 500 with no cookie, retries, and getsEMAIL_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 throughwriteServiceError's default branch.internal/service/auth.go:149, 155, 160— store errors fromDeleteSession/TouchSessionsilently dropped.ResolveSessiondiscards 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. TheTouchSessionswallow (160) silently disables sliding renewal on write failure. Fix:slog.Warnthese so a degrading DB is observable.internal/service/auth.go:97, 103+internal/service/service.go:47-51— emptydummyHashdefeats the timing equalizer with no runtime signal. IfhashPasswordfails inNew(only path:crypto/rand.Readfailure),s.dummyHashstays"".verifyPassword("", password)short-circuits indecodeHash(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 startupslog.Warnis easy to miss; the failure is silent for the process lifetime. Fix: inLogin, ifs.dummyHash == "", either recompute it lazily under a mutex or fall back to a hardcoded known-good hash string constant.internal/api/auth.go:146—maxAgefloor 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 whentime.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
requireAuthcorrectly gates/auth/meand resolves the session cookie → user viaResolveSession; any failure is 401. The cookie isHttpOnly,SameSite=Lax, andSecureonly when the base URL is HTTPS. Session tokens are 32 random bytes (CSPRNG) and only their SHA-256 is persisted.crypto/rand; verification usessubtle.ConstantTimeCompare. ThedummyHashtiming equalizer inLoginmakes unknown-email and wrong-password paths indistinguishable in timing.?placeholders);userColumnsis a hardcoded constant. No SQL injection surface.registerRequestvalidates email format and password length (8–1024) via gin binding;loginRequestvalidates email format. The service layer re-checks for emptiness as defense-in-depth.PasswordHash,OIDCIssuer, andOIDCSubjectcarryjson:"-"tags, so they are excluded from JSON serialization. No credentials or raw tokens are logged.CreateSessionissues a new token on every login/register (no fixation risk).Logoutis 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 adminsCountUsersandCreateUserare separate, non-transactional queries. Under WAL mode two goroutines can both readcount == 0before either writes, then both insert withIsAdmin: true. The UNIQUE guard only protectsemail, notis_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 aBEGIN IMMEDIATEtransaction so the read locks the table for writing.internal/service/auth.go:49— Service layer does not enforce the 1024-character password ceilingRegistervalidatesin.Password == ""but never checkslen(in.Password) > 1024. The API binding catches it today, but the PR positionsinternal/serviceas 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) > 1024to the service-level validation, returningdomain.ErrInvalidInput.internal/service/auth.go:107-110— Malformed password hash silently returned as wrong-password with no logverifyPasswordreturnserrBadHashwhen a stored hash is malformed (corruption, partial write, etc.). Thepassword.gocomment explicitly says “Callers treat it as an auth failure but should log it”, yetLoginmaps it straight todomain.ErrInvalidCredentialswith noslog.Error. An operator has no server-side signal that data corruption is locking a user out.Fix: Log the malformed-hash error in
Loginbefore returningErrInvalidCredentials, 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-28Password length validation only in HTTP binding tags, not service layer —registerRequestenforcesmin=8,max=1024via gin binding tags, butservice.Register(internal/service/auth.go:48-50) only checksin.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=1024check (and the display-name non-empty check) intoservice.Registerand returndomain.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 atinternal/api/auth.go:40so it doesn't duplicate the rule.internal/api/auth.go:164writeServiceErrormethod has unused receiver — Declared asfunc (h *handlers) writeServiceError(...), buthis 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:32— Missing index onsessions.expires_at.DeleteExpiredSessionsrunsDELETE FROM sessions WHERE expires_at <= ?every 6 hours. Without an index onexpires_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 existingidx_sessions_user.internal/service/auth.go:141— Two DB round-trips on every authenticated request (observation, not blocking).ResolveSessioncallsGetSession(by PK) and thenGetUserByID(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 readingstore/sessions.goandstore/users.go— neither currently exposes a joined fetch.Fix: Consider adding
GetSessionWithUserin a follow-up if profiling shows it matters.🧯 Error handling & edge cases — Minor issues
Minor issues
internal/service/auth.go:160—ResolveSessionsilently discards the error froms.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—RegisterreadsCountUsersand thenCreateUserin separate transactions. On a fresh instance (or withPANSY_REGISTRATION=closed), concurrent bootstrap requests can both observecount == 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—ResolveSessionignoresDeleteSessionerrors when dropping corrupt or expired sessions. A persistent DB fault can leave garbage rows thatResolveSessionwill keep re-discovering and re-failing on every request until the background sweep happens to succeed.internal/service/auth.go:107-110—LoginswallowsverifyPassword'serrBadHash, returning onlyErrInvalidCredentialswith 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—loginRequesthas nomaxtag onPassword, whileregisterRequestcaps 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.