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

Merged
steve merged 2 commits from phase-1-local-auth into main 2026-07-18 21:05:01 +00:00
16 changed files with 1532 additions and 44 deletions
+1 -1
View File
@@ -57,7 +57,7 @@ All configuration is via environment variables; every value has a default, so `.
| `PANSY_OIDC_BUTTON_LABEL` | `Sign in with SSO` | Label for the OIDC button on the login page. |
| `PANSY_TRUSTED_PROXIES` | *(none)* | Comma-separated proxy CIDRs/IPs to trust for client-IP resolution. |
Auth variables are read at startup now and consumed as the auth features land (issues #4/#5).
Local email/password auth is live (`POST /api/v1/auth/register`, `/auth/login`, `/auth/logout`, `GET /auth/me`, `GET /auth/providers`); the session is an HttpOnly cookie (`Secure` when `PANSY_BASE_URL` is https). The first account registered becomes admin, and it may register even when `PANSY_REGISTRATION=closed` to bootstrap the instance. OIDC (`PANSY_OIDC_*`) lands in #5.
## Docker & deployment
+32 -1
View File
@@ -16,10 +16,15 @@ import (
"gitea.stevedudenhoeffer.com/steve/pansy/internal/api"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/webdist"
)
// sessionSweepInterval is how often expired sessions are purged in the
// background (they are also dropped lazily on access).
const sessionSweepInterval = 6 * time.Hour
func main() {
slog.SetDefault(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelInfo})))
@@ -47,7 +52,9 @@ func run() error {
}
slog.Info("pansy: database ready", "path", cfg.DBPath)
r := api.New(cfg)
svc := service.New(db, cfg)
r := api.New(cfg, svc)
api.RegisterSPA(r, webdist.Dist())
srv := &http.Server{
@@ -71,6 +78,8 @@ func run() error {
ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
defer stop()
go sweepSessions(ctx, svc)
select {
case err := <-serveErr:
return fmt.Errorf("http server: %w", err)
@@ -85,3 +94,25 @@ func run() error {
}
return nil
}
// sweepSessions periodically purges expired sessions until ctx is cancelled. It
// runs one sweep immediately so a long-lived process doesn't wait a full
// interval after startup. Failures are logged, never fatal.
func sweepSessions(ctx context.Context, svc *service.Service) {
ticker := time.NewTicker(sessionSweepInterval)
defer ticker.Stop()
for {
if n, err := svc.CleanupExpiredSessions(ctx); err != nil {
if ctx.Err() == nil {
slog.Warn("pansy: session sweep failed", "error", err)
}
} else if n > 0 {
slog.Info("pansy: purged expired sessions", "count", n)
}
select {
case <-ctx.Done():
return
case <-ticker.C:
}
}
}
+1 -1
View File
@@ -5,6 +5,7 @@ go 1.26.2
require (
github.com/gin-gonic/gin v1.10.1
github.com/samber/slog-gin v1.15.0
golang.org/x/crypto v0.31.0
modernc.org/sqlite v1.34.4
)
@@ -36,7 +37,6 @@ require (
go.opentelemetry.io/otel v1.29.0 // indirect
go.opentelemetry.io/otel/trace v1.29.0 // indirect
golang.org/x/arch v0.8.0 // indirect
golang.org/x/crypto v0.31.0 // indirect
golang.org/x/net v0.33.0 // indirect
golang.org/x/sys v0.28.0 // indirect
golang.org/x/text v0.21.0 // indirect
+27 -4
View File
@@ -13,13 +13,21 @@ import (
sloggin "github.com/samber/slog-gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// handlers carries the dependencies shared by every HTTP handler. Handlers stay
// thin: decode the request, call a service method, encode the result.
type handlers struct {
cfg *config.Config
svc *service.Service
}
// New builds the gin engine with the standard middleware stack and registers the
// API routes. The embedded SPA fallback is registered separately by the caller
// via RegisterSPA (see spa.go) so the API can be built and tested without a web
// build present.
func New(cfg *config.Config) *gin.Engine {
// API routes against the given service. The embedded SPA fallback is registered
// separately by the caller via RegisterSPA (see spa.go) so the API can be built
// and tested without a web build present.
func New(cfg *config.Config, svc *service.Service) *gin.Engine {
gin.SetMode(gin.ReleaseMode)
r := gin.New()
@@ -33,9 +41,24 @@ func New(cfg *config.Config) *gin.Engine {
_ = r.SetTrustedProxies(nil)
}
h := &handlers{cfg: cfg, svc: svc}
v1 := r.Group("/api/v1")
// CSRF defense for every state-changing API call (no-op unless PANSY_BASE_URL
// is set; see csrfGuard).
v1.Use(h.csrfGuard())
v1.GET("/healthz", healthz)
// Auth endpoints are exempt from requireAuth (you can't be logged in yet);
// /me is the one that needs a session. Feature routers in later issues attach
// h.requireAuth() to their own protected groups.
auth := v1.Group("/auth")
auth.POST("/register", h.register)
auth.POST("/login", h.login)
auth.POST("/logout", h.logout)
auth.GET("/providers", h.providers)
auth.GET("/me", h.requireAuth(), h.me)
return r
}
+224
View File
@@ -0,0 +1,224 @@
package api
import (
"errors"
"log/slog"
"net/http"
"net/url"
"strings"
"time"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
)
// sessionCookie is the name of the HttpOnly cookie carrying the raw session token.
const sessionCookie = "pansy_session"
// actorKey is the gin-context key under which requireAuth stashes the
// authenticated *domain.User.
const actorKey = "actor"
// registerRequest / loginRequest are the JSON bodies for the local-auth
// endpoints. gin's binding validates them before the service is called.
Review

🟠 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>
type registerRequest struct {
Email string `json:"email" binding:"required,email"`
DisplayName string `json:"displayName" binding:"required"`
Password string `json:"password" binding:"required,min=8,max=1024"`
}
type loginRequest struct {
Email string `json:"email" binding:"required,email"`
Review

🟡 loginRequest missing max password boundary

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **loginRequest missing max password boundary** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
Password string `json:"password" binding:"required,max=1024"`
}
// register creates a local account and logs it in (sets the session cookie).
func (h *handlers) register(c *gin.Context) {
var req registerRequest
if err := c.ShouldBindJSON(&req); err != nil {
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "email, display name, and an 8+ character password are required")
return
}
user, err := h.svc.Register(c.Request.Context(), service.RegisterInput{
Email: req.Email,
DisplayName: req.DisplayName,
Password: req.Password,
})
if err != nil {
writeServiceError(c, err)
return
}
Review

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

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

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

🪰 Gadfly · advisory

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

🟡 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>
return
}
user, err := h.svc.Login(c.Request.Context(), req.Email, req.Password)
if err != nil {
writeServiceError(c, err)
return
}
h.startSessionAndRespond(c, user)
}
// logout deletes the current session (if any) and clears the cookie. It is
// idempotent and never requires a valid session.
func (h *handlers) logout(c *gin.Context) {
if token, err := c.Cookie(sessionCookie); err == nil && token != "" {
if err := h.svc.Logout(c.Request.Context(), token); err != nil {
slog.Error("api: logout", "error", err)
}
}
h.clearSessionCookie(c)
c.JSON(http.StatusOK, gin.H{"ok": true})
}
// me returns the authenticated user. It sits behind requireAuth.
func (h *handlers) me(c *gin.Context) {
c.JSON(http.StatusOK, mustActor(c))
}
// providers reports which login methods to render on the login page.
func (h *handlers) providers(c *gin.Context) {
c.JSON(http.StatusOK, h.svc.Providers())
}
// requireAuth is middleware that rejects requests without a valid session and,
// on success, stashes the resolved user in the context and slides the cookie's
// lifetime forward in step with the server-side session (otherwise an active
// user would be logged out 30 days after login regardless of activity). Feature
// routers added by later issues (gardens, objects, …) attach this to their
// protected groups.
func (h *handlers) requireAuth() gin.HandlerFunc {
return func(c *gin.Context) {
token, err := c.Cookie(sessionCookie)
if err != nil || token == "" {
abortUnauthenticated(c)
Outdated
Review

🔴 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>
return
}
user, expiresAt, err := h.svc.ResolveSession(c.Request.Context(), token)
if err != nil {
Review

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>
// Any resolution failure (missing/expired/tampered) is 401, never 404:
// the client should log in, not think a resource is gone.
abortUnauthenticated(c)
return
}
c.Set(actorKey, user)
// Refresh the client cookie to the session's current expiry so browser and
// server slide together.
h.setSessionCookie(c, token, expiresAt)
c.Next()
}
}
// abortUnauthenticated writes the standard 401 and stops the handler chain.
func abortUnauthenticated(c *gin.Context) {
writeAPIError(c, http.StatusUnauthorized, "UNAUTHENTICATED", "authentication required")
c.Abort()
}
// csrfGuard rejects state-changing requests whose Origin header doesn't match
// the instance's own origin — a defense against login/logout CSRF, which
// SameSite=Lax cookies alone do not prevent. It is a no-op unless PANSY_BASE_URL
// is set (so it never interferes with the local Vite dev proxy), and it allows
// requests with no Origin (non-browser clients like curl, which aren't a CSRF
// vector). Safe methods are always allowed.
func (h *handlers) csrfGuard() gin.HandlerFunc {
var wantHost string
if u, err := url.Parse(h.cfg.BaseURL); err == nil {
wantHost = u.Host
}
Outdated
Review

🟡 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>
return func(c *gin.Context) {
if wantHost == "" {
c.Next()
return
}
Outdated
Review

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

error-handling · flagged by 1 model

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

🪰 Gadfly · advisory

⚪ **setSessionCookie MaxAge floor of 1 masks clock-skew/negative-duration bugs instead of clearing the cookie** _error-handling · flagged by 1 model_ - **`internal/api/auth.go:146` — `maxAge` floor of 1 masks clock-skew bugs.** `max(int(time.Until(expiresAt).Seconds()), 1)` clamps a negative duration to a 1-second cookie instead of clearing it. Not currently reachable (`expiresAt = now + 30d`), but the floor silently swallows a clock-skew class of bug. Trivial; leave or change to clear the cookie when `time.Until(expiresAt) <= 0`. <sub>🪰 Gadfly · advisory</sub>
switch c.Request.Method {
case http.MethodGet, http.MethodHead, http.MethodOptions:
c.Next()
return
}
if origin := c.GetHeader("Origin"); origin != "" {
if u, err := url.Parse(origin); err != nil || !strings.EqualFold(u.Host, wantHost) {
writeAPIError(c, http.StatusForbidden, "CROSS_ORIGIN", "cross-origin request rejected")
c.Abort()
return
}
}
c.Next()
}
}
// startSessionAndRespond issues a session, sets the cookie, and returns the user
// as JSON — the shared tail of register and login.
Outdated
Review

🟡 writeServiceError method has unused receiver

maintainability · flagged by 1 model

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

🪰 Gadfly · advisory

🟡 **writeServiceError method has unused receiver** _maintainability · flagged by 1 model_ - **`internal/api/auth.go:164`** `writeServiceError` method has unused receiver — Declared as `func (h *handlers) writeServiceError(...)`, but `h` is never referenced inside the body, which is a small readability misdirection. **Fix:** Change it to a package-level function: `func writeServiceError(c *gin.Context, err error)`. <sub>🪰 Gadfly · advisory</sub>
func (h *handlers) startSessionAndRespond(c *gin.Context, user *domain.User) {
token, expiresAt, err := h.svc.CreateSession(c.Request.Context(), user.ID)
if err != nil {
// The account exists and is usable via login; only the auto-login cookie
// failed. Log it so the operator can see the underlying DB problem.
slog.Error("api: could not start session", "user_id", user.ID, "error", err)
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not start session")
return
}
h.setSessionCookie(c, token, expiresAt)
c.JSON(http.StatusOK, user)
}
// 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 {
return strings.HasPrefix(h.cfg.BaseURL, "https://")
}
func (h *handlers) setSessionCookie(c *gin.Context, token string, expiresAt time.Time) {
// Callers only ever pass a future expiry (login/register set now+TTL;
// requireAuth uses a session already checked to be unexpired), so this is
// always well above the floor; the floor merely avoids emitting a delete
// cookie (MaxAge<=0) from a pathological clock skew.
maxAge := max(int(time.Until(expiresAt).Seconds()), 1)
c.SetSameSite(http.SameSiteLaxMode)
c.SetCookie(sessionCookie, token, maxAge, "/", "", h.cookieSecure(), true)
}
func (h *handlers) clearSessionCookie(c *gin.Context) {
c.SetSameSite(http.SameSiteLaxMode)
c.SetCookie(sessionCookie, "", -1, "/", "", h.cookieSecure(), true)
}
// mustActor returns the user stashed by requireAuth. It panics if called on a
// route that isn't behind requireAuth — a programming error.
func mustActor(c *gin.Context) *domain.User {
return c.MustGet(actorKey).(*domain.User)
}
// writeServiceError maps a service-layer sentinel error to pansy's JSON error
// envelope. Login failures never leak which of email/password was wrong.
func writeServiceError(c *gin.Context, err error) {
switch {
case errors.Is(err, domain.ErrInvalidCredentials):
writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
case errors.Is(err, domain.ErrEmailTaken):
writeAPIError(c, http.StatusConflict, "EMAIL_TAKEN", "an account with that email already exists")
case errors.Is(err, domain.ErrRegistrationClosed):
writeAPIError(c, http.StatusForbidden, "REGISTRATION_CLOSED", "registration is closed")
case errors.Is(err, domain.ErrLocalAuthDisabled):
writeAPIError(c, http.StatusForbidden, "LOCAL_AUTH_DISABLED", "local authentication is disabled")
case errors.Is(err, domain.ErrInvalidInput):
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
default:
slog.Error("api: unhandled service error", "error", err)
writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "internal error")
}
}
+239
View File
@@ -0,0 +1,239 @@
package api
import (
"bytes"
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/gin-gonic/gin"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
)
// authEngine builds an API engine backed by a fresh in-memory database.
func authEngine(t *testing.T, cfg *config.Config) *gin.Engine {
t.Helper()
gin.SetMode(gin.TestMode)
db, err := store.Open(":memory:")
if err != nil {
t.Fatalf("store.Open: %v", err)
}
t.Cleanup(func() { db.Close() })
if err := db.Migrate(context.Background()); err != nil {
t.Fatalf("Migrate: %v", err)
}
return New(cfg, service.New(db, cfg))
}
func localCfg() *config.Config {
return &config.Config{Registration: config.RegistrationOpen, LocalAuth: true}
}
// encodeBody JSON-encodes v into a reader for a request body.
func encodeBody(t *testing.T, v any) *bytes.Buffer {
t.Helper()
var buf bytes.Buffer
if v != nil {
if err := json.NewEncoder(&buf).Encode(v); err != nil {
t.Fatalf("encode body: %v", err)
}
}
return &buf
}
// doJSON issues a JSON request, optionally carrying a session cookie.
func doJSON(t *testing.T, r *gin.Engine, method, path string, body any, cookie *http.Cookie) *httptest.ResponseRecorder {
t.Helper()
req := httptest.NewRequest(method, path, encodeBody(t, body))
req.Header.Set("Content-Type", "application/json")
if cookie != nil {
req.AddCookie(cookie)
}
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
return w
}
// sessionCookieFrom extracts the pansy_session cookie from a response.
func sessionCookieFrom(t *testing.T, w *httptest.ResponseRecorder) *http.Cookie {
t.Helper()
for _, ck := range w.Result().Cookies() {
if ck.Name == sessionCookie {
return ck
}
}
t.Fatalf("no %s cookie set (status %d, body %s)", sessionCookie, w.Code, w.Body.String())
return nil
}
func TestRegisterLoginMeLogoutFlow(t *testing.T) {
r := authEngine(t, localCfg())
// Register auto-logs-in and sets a cookie.
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}, nil)
if w.Code != http.StatusOK {
t.Fatalf("register status = %d, body %s", w.Code, w.Body.String())
}
cookie := sessionCookieFrom(t, w)
if !cookie.HttpOnly {
t.Error("session cookie must be HttpOnly")
}
if cookie.SameSite != http.SameSiteLaxMode {
t.Errorf("session cookie SameSite = %v, want Lax", cookie.SameSite)
}
if cookie.Secure {
t.Error("session cookie must not be Secure without an https base URL")
}
// /me with the cookie returns the user; password hash is never serialized.
w = doJSON(t, r, http.MethodGet, "/api/v1/auth/me", nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("me status = %d, body %s", w.Code, w.Body.String())
}
if strings.Contains(w.Body.String(), "password") {
t.Errorf("/me leaked a password field: %s", w.Body.String())
}
// Logout clears the cookie and invalidates the session.
w = doJSON(t, r, http.MethodPost, "/api/v1/auth/logout", nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("logout status = %d", w.Code)
}
w = doJSON(t, r, http.MethodGet, "/api/v1/auth/me", nil, cookie)
if w.Code != http.StatusUnauthorized {
t.Errorf("me after logout status = %d, want 401", w.Code)
}
}
func TestMeRequiresAuth(t *testing.T) {
r := authEngine(t, localCfg())
w := doJSON(t, r, http.MethodGet, "/api/v1/auth/me", nil, nil)
if w.Code != http.StatusUnauthorized {
t.Errorf("me without cookie status = %d, want 401", w.Code)
}
}
func TestLoginWrongPasswordIs401(t *testing.T) {
r := authEngine(t, localCfg())
doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}, nil)
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/login",
map[string]string{"email": "[email protected]", "password": "wrong"}, nil)
if w.Code != http.StatusUnauthorized {
t.Errorf("bad login status = %d, want 401", w.Code)
}
}
func TestRegisterValidationRejectsShortPassword(t *testing.T) {
r := authEngine(t, localCfg())
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "short"}, nil)
if w.Code != http.StatusBadRequest {
t.Errorf("short-password register status = %d, want 400", w.Code)
}
}
func TestProvidersEndpoint(t *testing.T) {
r := authEngine(t, localCfg())
w := doJSON(t, r, http.MethodGet, "/api/v1/auth/providers", nil, nil)
if w.Code != http.StatusOK {
t.Fatalf("providers status = %d", w.Code)
}
var got struct {
Local bool `json:"local"`
OIDC bool `json:"oidc"`
}
if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
t.Fatalf("decode providers: %v", err)
}
if !got.Local || got.OIDC {
t.Errorf("providers = %+v, want local=true oidc=false", got)
}
}
func TestAuthenticatedRequestRefreshesCookie(t *testing.T) {
// requireAuth must re-set the session cookie so the browser's Max-Age slides
// with the server session rather than expiring 30 days after login.
r := authEngine(t, localCfg())
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}, nil)
cookie := sessionCookieFrom(t, w)
w = doJSON(t, r, http.MethodGet, "/api/v1/auth/me", nil, cookie)
if w.Code != http.StatusOK {
t.Fatalf("me status = %d", w.Code)
}
// A fresh session cookie should be present on the /me response.
refreshed := sessionCookieFrom(t, w)
if refreshed.Value != cookie.Value {
t.Errorf("refreshed cookie value changed: got %q want same token", refreshed.Value)
}
if refreshed.MaxAge <= 0 {
t.Errorf("refreshed cookie Max-Age = %d, want > 0", refreshed.MaxAge)
}
}
func TestCSRFGuardRejectsCrossOrigin(t *testing.T) {
cfg := localCfg()
cfg.BaseURL = "https://pansy.example.com"
r := authEngine(t, cfg)
body := map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}
// Cross-origin POST is rejected before touching the service.
req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/register", encodeBody(t, body))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Origin", "https://evil.example.com")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusForbidden {
t.Fatalf("cross-origin register status = %d, want 403", w.Code)
}
// Same-origin POST is allowed.
req = httptest.NewRequest(http.MethodPost, "/api/v1/auth/register", encodeBody(t, body))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Origin", "https://pansy.example.com")
w = httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("same-origin register status = %d, want 200 (body %s)", w.Code, w.Body.String())
}
}
func TestCSRFGuardNoopWithoutBaseURL(t *testing.T) {
// In local dev (no PANSY_BASE_URL) the guard must not interfere, even with a
// foreign Origin (the Vite dev proxy forwards the browser's origin).
r := authEngine(t, localCfg())
req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/register",
encodeBody(t, map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}))
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Origin", "http://localhost:5173")
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
if w.Code != http.StatusOK {
t.Fatalf("dev cross-origin register status = %d, want 200", w.Code)
}
}
func TestSecureCookieWithHTTPSBaseURL(t *testing.T) {
cfg := localCfg()
cfg.BaseURL = "https://pansy.example.com"
r := authEngine(t, cfg)
w := doJSON(t, r, http.MethodPost, "/api/v1/auth/register",
map[string]string{"email": "[email protected]", "displayName": "Alice", "password": "password123"}, nil)
if w.Code != http.StatusOK {
t.Fatalf("register status = %d", w.Code)
}
if !sessionCookieFrom(t, w).Secure {
t.Error("session cookie should be Secure when base URL is https")
}
}
+50 -32
View File
@@ -17,6 +17,24 @@ var (
// ErrVersionConflict means the row's version did not match the one supplied
// with a PATCH/DELETE; the caller should refetch and retry.
ErrVersionConflict = errors.New("version conflict")
// ErrInvalidInput means the caller supplied structurally invalid data (empty
// required field, malformed value). Mapped to 400.
ErrInvalidInput = errors.New("invalid input")
// ErrInvalidCredentials means a login attempt failed. It is deliberately
// identical for an unknown email and a wrong password so neither can be
// enumerated. Mapped to 401.
ErrInvalidCredentials = errors.New("invalid credentials")
// ErrEmailTaken means registration collided with an existing account's email.
// Mapped to 409.
ErrEmailTaken = errors.New("email already registered")
// ErrRegistrationClosed means local self-service signup is disabled and at
// least one user already exists (the very first user may always register to
// bootstrap the instance). Mapped to 403.
ErrRegistrationClosed = errors.New("registration closed")
// ErrLocalAuthDisabled means PANSY_LOCAL_AUTH=false, so local register/login
// are rejected in favor of OIDC. Mapped to 403.
ErrLocalAuthDisabled = errors.New("local authentication disabled")
)
// Enumerated string values mirrored from the schema CHECK constraints.
@@ -99,25 +117,25 @@ type GardenShare struct {
// GardenObject is any placeable object in a garden (bed, container, tree, path…).
// Positioned by center point + rotation about center, in garden space.
type GardenObject struct {
ID int64 `json:"id"`
GardenID int64 `json:"gardenId"`
Kind string `json:"kind"`
Name string `json:"name"`
Shape string `json:"shape"`
Points *string `json:"points,omitempty"` // reserved: JSON for polygons
XCM float64 `json:"xCm"`
YCM float64 `json:"yCm"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
RotationDeg float64 `json:"rotationDeg"`
ZIndex int `json:"zIndex"`
Plantable bool `json:"plantable"`
Color *string `json:"color,omitempty"`
Props *string `json:"props,omitempty"` // kind-specific JSON
Notes string `json:"notes"`
Version int64 `json:"version"`
CreatedAt string `json:"createdAt"`
UpdatedAt string `json:"updatedAt"`
ID int64 `json:"id"`
GardenID int64 `json:"gardenId"`
Kind string `json:"kind"`
Name string `json:"name"`
Shape string `json:"shape"`
Points *string `json:"points,omitempty"` // reserved: JSON for polygons
XCM float64 `json:"xCm"`
YCM float64 `json:"yCm"`
WidthCM float64 `json:"widthCm"`
HeightCM float64 `json:"heightCm"`
RotationDeg float64 `json:"rotationDeg"`
ZIndex int `json:"zIndex"`
Plantable bool `json:"plantable"`
Color *string `json:"color,omitempty"`
Props *string `json:"props,omitempty"` // kind-specific JSON
Notes string `json:"notes"`
Version int64 `json:"version"`
CreatedAt string `json:"createdAt"`
UpdatedAt string `json:"updatedAt"`
}
// Plant is a catalog entry. OwnerID nil means a read-only seeded built-in.
@@ -139,17 +157,17 @@ type Plant struct {
// Planting ("plop") is a circular patch of one plant, positioned in its parent
// object's local frame. Count nil means it is derived from area / spacing².
type Planting struct {
ID int64 `json:"id"`
ObjectID int64 `json:"objectId"`
PlantID int64 `json:"plantId"`
XCM float64 `json:"xCm"`
YCM float64 `json:"yCm"`
RadiusCM float64 `json:"radiusCm"`
Count *int `json:"count,omitempty"` // nil = derived
Label *string `json:"label,omitempty"`
PlantedAt *string `json:"plantedAt,omitempty"`
RemovedAt *string `json:"removedAt,omitempty"`
Version int64 `json:"version"`
CreatedAt string `json:"createdAt"`
UpdatedAt string `json:"updatedAt"`
ID int64 `json:"id"`
ObjectID int64 `json:"objectId"`
PlantID int64 `json:"plantId"`
XCM float64 `json:"xCm"`
YCM float64 `json:"yCm"`
RadiusCM float64 `json:"radiusCm"`
Count *int `json:"count,omitempty"` // nil = derived
Label *string `json:"label,omitempty"`
PlantedAt *string `json:"plantedAt,omitempty"`
RemovedAt *string `json:"removedAt,omitempty"`
Version int64 `json:"version"`
CreatedAt string `json:"createdAt"`
UpdatedAt string `json:"updatedAt"`
}
+210
View File
@@ -0,0 +1,210 @@
package service
import (
"context"
"errors"
"log/slog"
"strings"
"time"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// maxPasswordLen caps accepted password length. It bounds argon2 input and
// request work, and sits far above any real password.
const maxPasswordLen = 1024
// RegisterInput is the payload for local self-service signup.
type RegisterInput struct {
Email string
DisplayName string
Password string
}
// Providers reports which login methods the server offers, so the login page
// (#6) can render the right controls. OIDCLabel is only meaningful when OIDC is
// true.
type Providers struct {
Local bool `json:"local"`
OIDC bool `json:"oidc"`
OIDCLabel string `json:"oidcLabel"`
}
// Providers returns the enabled auth methods. OIDC is always false until #5
// wires the endpoints; advertising it before then would point the UI at routes
// that don't exist.
func (s *Service) Providers() Providers {
return Providers{
Local: s.cfg.LocalAuth,
OIDC: false,
OIDCLabel: s.cfg.OIDC.ButtonLabel,
}
}
// Register creates a local (password) account and returns it. The first user on
// a fresh instance becomes admin and may always register (bootstrap), even when
// PANSY_REGISTRATION=closed; afterward, closed registration is enforced.
Review

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

correctness · flagged by 2 models

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

🪰 Gadfly · advisory

🟠 **TOCTOU race between CountUsers and CreateUser in Register lets concurrent first-time registrations both become admin and both bypass PANSY_REGISTRATION=closed bootstrap gating** _correctness · flagged by 2 models_ - **`internal/service/auth.go:49` — Service layer does not enforce the 1024-character password ceiling** `Register` validates `in.Password == ""` but never checks `len(in.Password) > 1024`. The API binding catches it today, but the PR positions `internal/service` as the seam for “future agent tools”. A direct caller (CLI, agent, test helper) can pass an arbitrarily large password, and the service will hash it rather than reject it. **Fix:** Add `len(in.Password) > 1024` to the service-level vali… <sub>🪰 Gadfly · advisory</sub>
func (s *Service) Register(ctx context.Context, in RegisterInput) (*domain.User, error) {
if !s.cfg.LocalAuth {
return nil, domain.ErrLocalAuthDisabled
}
email := normalizeEmail(in.Email)
displayName := strings.TrimSpace(in.DisplayName)
Review

🔴 TOCTOU race in Register allows multiple bootstrap admins

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

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

🪰 Gadfly · advisory

🔴 **TOCTOU race in Register allows multiple bootstrap admins** _correctness, error-handling, security · flagged by 3 models_ - **TOCTOU race allows multiple bootstrap admins on a fresh instance** `internal/service/auth.go:53-82` — `Register` reads `CountUsers`, checks `count > 0 && !RegistrationOpen()`, then calls `CreateUser`. Two concurrent requests on a fresh DB can both observe `count == 0`, pass the closed-registration gate, and both create accounts with `IsAdmin: true` (as long as they use different emails). This breaks the intended “exactly one bootstrap admin” security model and lets an attacker race the legit… <sub>🪰 Gadfly · advisory</sub>
if email == "" || displayName == "" || in.Password == "" || len(in.Password) > maxPasswordLen {
return nil, domain.ErrInvalidInput
}
// Cheap best-effort gate so a closed instance doesn't burn argon2 work on
// signups it will reject anyway. The authoritative, race-free gate — plus
// atomic first-user-is-admin assignment and duplicate-email detection — lives
// in store.CreateUser's single INSERT.
if !s.cfg.RegistrationOpen() {
n, err := s.store.CountUsers(ctx)
if err != nil {
return nil, err
}
if n > 0 {
return nil, domain.ErrRegistrationClosed
}
}
hash, err := hashPassword(in.Password)
if err != nil {
return nil, err
}
return s.store.CreateUser(ctx, &domain.User{
Review

🟡 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>
Email: email,
DisplayName: displayName,
PasswordHash: &hash,
}, s.cfg.RegistrationOpen())
Outdated
Review

🟠 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>
}
// Login verifies an email/password pair and returns the user. Unknown-email and
// wrong-password both return domain.ErrInvalidCredentials, and both spend the
// same argon2 work, so neither the error nor the timing reveals which failed.
func (s *Service) Login(ctx context.Context, email, password string) (*domain.User, error) {
if !s.cfg.LocalAuth {
return nil, domain.ErrLocalAuthDisabled
}
if len(password) > maxPasswordLen {
// No stored password is this long; reject without spending argon2 work.
return nil, domain.ErrInvalidCredentials
}
u, err := s.store.GetUserByEmail(ctx, normalizeEmail(email))
switch {
Review

🟠 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>
case errors.Is(err, domain.ErrNotFound):
// Spend comparable time so timing can't distinguish a missing account.
_, _ = verifyPassword(s.dummyHash, password)
return nil, domain.ErrInvalidCredentials
case err != nil:
return nil, err
case u.PasswordHash == nil:
// OIDC-only account: no local password to check, but equalize timing.
_, _ = verifyPassword(s.dummyHash, password)
return nil, domain.ErrInvalidCredentials
Review

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

correctness, error-handling · flagged by 1 model

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

🪰 Gadfly · advisory

🟠 **Malformed password hash silently returned as wrong-password with no log** _correctness, error-handling · flagged by 1 model_ - **`internal/service/auth.go:107-110` — Malformed password hash silently returned as wrong-password with no log** `verifyPassword` returns `errBadHash` when a stored hash is malformed (corruption, partial write, etc.). The `password.go` comment explicitly says *“Callers treat it as an auth failure but should log it”*, yet `Login` maps it straight to `domain.ErrInvalidCredentials` with no `slog.Error`. An operator has no server-side signal that data corruption is locking a user out. **Fix:** Log… <sub>🪰 Gadfly · advisory</sub>
}
ok, err := verifyPassword(*u.PasswordHash, password)
if err != nil {
// A stored hash we can't parse is a data problem, not a wrong password;
// surface it so a corrupt row doesn't silently lock a user out unnoticed.
slog.Error("service: malformed stored password hash", "user_id", u.ID, "error", err)
return nil, domain.ErrInvalidCredentials
}
if !ok {
return nil, domain.ErrInvalidCredentials
}
return u, nil
}
// CreateSession issues a new session for a user and returns the raw bearer token
// (to place in the cookie) and its expiry.
func (s *Service) CreateSession(ctx context.Context, userID int64) (token string, expiresAt time.Time, err error) {
raw, err := newSessionToken()
if err != nil {
return "", time.Time{}, err
}
exp := s.now().Add(sessionTTL)
if err := s.store.CreateSession(ctx, &domain.Session{
TokenHash: hashToken(raw),
UserID: userID,
ExpiresAt: formatTime(exp),
}); err != nil {
return "", time.Time{}, err
}
return raw, exp, nil
}
// ResolveSession validates a raw bearer token and returns its user and the
Outdated
Review

🟡 Two DB round-trips on every authenticated request

performance · flagged by 1 model

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

🪰 Gadfly · advisory

🟡 **Two DB round-trips on every authenticated request** _performance · flagged by 1 model_ - `internal/service/auth.go:141` — **Two DB round-trips on every authenticated request** (observation, not blocking). `ResolveSession` calls `GetSession` (by PK) and then `GetUserByID` (by PK). Both are indexed and fast, but every protected endpoint pays two query overheads. A single JOIN query would halve the DB work on the hot path. Verified by reading `store/sessions.go` and `store/users.go` — neither currently exposes a joined fetch. **Fix:** Consider adding `GetSessionWithUser` in a follow-… <sub>🪰 Gadfly · advisory</sub>
// session's current expiry (so the caller can slide the client cookie in
// lockstep with the server). An expired session is deleted and treated as absent
// (domain.ErrNotFound). A still-valid session has its expiry slid forward, but
// only when that moves it by more than an hour, so a busy client doesn't write
// on every request.
func (s *Service) ResolveSession(ctx context.Context, rawToken string) (*domain.User, time.Time, error) {
if rawToken == "" {
return nil, time.Time{}, domain.ErrNotFound
Outdated
Review

🟡 DeleteSession error silently discarded on corrupt expiry

error-handling · flagged by 3 models

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

🪰 Gadfly · advisory

🟡 **DeleteSession error silently discarded on corrupt expiry** _error-handling · flagged by 3 models_ - **`internal/service/auth.go:149`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the corrupt-expiry path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. <sub>🪰 Gadfly · advisory</sub>
}
hash := hashToken(rawToken)
sess, err := s.store.GetSession(ctx, hash)
if err != nil {
return nil, time.Time{}, err
}
Review

🟡 DeleteSession error silently discarded on expired session

error-handling · flagged by 2 models

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

🪰 Gadfly · advisory

🟡 **DeleteSession error silently discarded on expired session** _error-handling · flagged by 2 models_ - **`internal/service/auth.go:155`** — `ResolveSession` also silently discards errors from `s.store.DeleteSession(...)` on the already-expired path. If the lazy delete fails, the stale/corrupt row stays in the database until the background sweeper eventually removes it. These errors should be logged rather than thrown away. <sub>🪰 Gadfly · advisory</sub>
exp, err := parseTime(sess.ExpiresAt)
if err != nil {
// A corrupt expiry means we can't trust the session; drop it.
Review

🟠 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>
if delErr := s.store.DeleteSession(ctx, hash); delErr != nil {
slog.Warn("service: deleting session with corrupt expiry", "error", delErr)
}
return nil, time.Time{}, domain.ErrNotFound
}
now := s.now()
if !now.Before(exp) {
if delErr := s.store.DeleteSession(ctx, hash); delErr != nil {
slog.Warn("service: deleting expired session", "error", delErr)
}
return nil, time.Time{}, domain.ErrNotFound
}
if newExp := now.Add(sessionTTL); newExp.Sub(exp) > time.Hour {
if err := s.store.TouchSession(ctx, hash, formatTime(newExp)); err != nil {
// Non-fatal: the session is still valid at its current expiry.
slog.Warn("service: sliding session expiry", "error", err)
} else {
exp = newExp
}
}
user, err := s.store.GetUserByID(ctx, sess.UserID)
if err != nil {
return nil, time.Time{}, err
}
return user, exp, nil
}
// Logout deletes the session behind a raw bearer token. It is idempotent.
func (s *Service) Logout(ctx context.Context, rawToken string) error {
if rawToken == "" {
return nil
}
return s.store.DeleteSession(ctx, hashToken(rawToken))
}
// CleanupExpiredSessions deletes all sessions that have passed their expiry and
// returns the count. Called periodically; expired sessions are also dropped
// lazily on access by ResolveSession.
func (s *Service) CleanupExpiredSessions(ctx context.Context) (int64, error) {
return s.store.DeleteExpiredSessions(ctx, formatTime(s.now()))
}
// normalizeEmail trims and lowercases an email for consistent storage and
// lookup. (The users.email column is also NOCASE, so lookups are robust either
// way; normalizing keeps stored values tidy.)
func normalizeEmail(email string) string {
return strings.ToLower(strings.TrimSpace(email))
}
+294
View File
@@ -0,0 +1,294 @@
package service
import (
"context"
"errors"
"strings"
"testing"
"time"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
)
// newTestService builds a Service over a fresh in-memory database.
func newTestService(t *testing.T, cfg *config.Config) *Service {
t.Helper()
db, err := store.Open(":memory:")
if err != nil {
t.Fatalf("store.Open: %v", err)
}
t.Cleanup(func() { db.Close() })
if err := db.Migrate(context.Background()); err != nil {
t.Fatalf("Migrate: %v", err)
}
return New(db, cfg)
}
func openConfig() *config.Config {
return &config.Config{Registration: config.RegistrationOpen, LocalAuth: true}
}
func mustRegister(t *testing.T, s *Service, email, name, pw string) *domain.User {
t.Helper()
u, err := s.Register(context.Background(), RegisterInput{Email: email, DisplayName: name, Password: pw})
if err != nil {
t.Fatalf("Register(%s): %v", email, err)
}
return u
}
func TestRegisterFirstUserIsAdmin(t *testing.T) {
s := newTestService(t, openConfig())
first := mustRegister(t, s, "[email protected]", "Alice", "password123")
if !first.IsAdmin {
t.Error("first user should be admin")
}
second := mustRegister(t, s, "[email protected]", "Bob", "password123")
if second.IsAdmin {
t.Error("second user should not be admin")
}
}
func TestRegisterNormalizesAndRejectsDuplicateEmail(t *testing.T) {
s := newTestService(t, openConfig())
mustRegister(t, s, "[email protected]", "Alice", "password123")
// Stored normalized (lowercased).
u, err := s.store.GetUserByEmail(context.Background(), "[email protected]")
if err != nil {
t.Fatalf("lookup normalized email: %v", err)
}
if u.Email != "[email protected]" {
t.Errorf("stored email = %q, want lowercased", u.Email)
}
// A different-cased duplicate is rejected.
_, err = s.Register(context.Background(), RegisterInput{Email: "[email protected]", DisplayName: "A2", Password: "password123"})
if !errors.Is(err, domain.ErrEmailTaken) {
t.Errorf("duplicate register err = %v, want ErrEmailTaken", err)
}
}
func TestRegisterRejectsBlankFields(t *testing.T) {
s := newTestService(t, openConfig())
_, err := s.Register(context.Background(), RegisterInput{Email: " ", DisplayName: "", Password: ""})
if !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("blank register err = %v, want ErrInvalidInput", err)
}
}
func TestRegistrationClosedAllowsBootstrapThenBlocks(t *testing.T) {
cfg := openConfig()
cfg.Registration = config.RegistrationClosed
s := newTestService(t, cfg)
// The very first user may register even when closed (bootstrap).
first := mustRegister(t, s, "[email protected]", "Admin", "password123")
if !first.IsAdmin {
t.Error("bootstrap user should be admin")
}
// Subsequent signups are blocked.
_, err := s.Register(context.Background(), RegisterInput{Email: "[email protected]", DisplayName: "Bob", Password: "password123"})
if !errors.Is(err, domain.ErrRegistrationClosed) {
t.Errorf("closed register err = %v, want ErrRegistrationClosed", err)
}
}
func TestRejectsOverlongPassword(t *testing.T) {
s := newTestService(t, openConfig())
long := strings.Repeat("a", maxPasswordLen+1)
if _, err := s.Register(context.Background(), RegisterInput{Email: "[email protected]", DisplayName: "A", Password: long}); !errors.Is(err, domain.ErrInvalidInput) {
t.Errorf("register overlong err = %v, want ErrInvalidInput", err)
}
mustRegister(t, s, "[email protected]", "Bob", "password123")
if _, err := s.Login(context.Background(), "[email protected]", long); !errors.Is(err, domain.ErrInvalidCredentials) {
t.Errorf("login overlong err = %v, want ErrInvalidCredentials", err)
}
}
func TestLocalAuthDisabledRejectsRegisterAndLogin(t *testing.T) {
cfg := openConfig()
cfg.LocalAuth = false
s := newTestService(t, cfg)
if _, err := s.Register(context.Background(), RegisterInput{Email: "[email protected]", DisplayName: "A", Password: "password123"}); !errors.Is(err, domain.ErrLocalAuthDisabled) {
t.Errorf("register err = %v, want ErrLocalAuthDisabled", err)
}
if _, err := s.Login(context.Background(), "[email protected]", "password123"); !errors.Is(err, domain.ErrLocalAuthDisabled) {
t.Errorf("login err = %v, want ErrLocalAuthDisabled", err)
}
}
func TestLoginSucceedsAndFailsIndistinguishably(t *testing.T) {
s := newTestService(t, openConfig())
mustRegister(t, s, "[email protected]", "Alice", "correct-horse")
// Correct credentials (case-insensitive email).
u, err := s.Login(context.Background(), "[email protected]", "correct-horse")
if err != nil {
t.Fatalf("login correct: %v", err)
}
if u.Email != "[email protected]" {
t.Errorf("logged-in user = %q", u.Email)
}
// Wrong password and unknown email both yield the same sentinel.
if _, err := s.Login(context.Background(), "[email protected]", "wrong"); !errors.Is(err, domain.ErrInvalidCredentials) {
t.Errorf("wrong-password err = %v, want ErrInvalidCredentials", err)
}
if _, err := s.Login(context.Background(), "[email protected]", "whatever"); !errors.Is(err, domain.ErrInvalidCredentials) {
t.Errorf("unknown-email err = %v, want ErrInvalidCredentials", err)
}
}
func TestLoginRejectsOIDCOnlyUser(t *testing.T) {
s := newTestService(t, openConfig())
// Simulate an OIDC-only account (no password hash) directly in the store.
iss, sub := "https://idp.example", "subject-1"
if _, err := s.store.CreateUser(context.Background(), &domain.User{
Email: "[email protected]", DisplayName: "O", OIDCIssuer: &iss, OIDCSubject: &sub,
}, true); err != nil {
t.Fatalf("seed oidc user: %v", err)
}
if _, err := s.Login(context.Background(), "[email protected]", "anything"); !errors.Is(err, domain.ErrInvalidCredentials) {
t.Errorf("oidc-only login err = %v, want ErrInvalidCredentials", err)
}
}
func TestSessionLifecycle(t *testing.T) {
s := newTestService(t, openConfig())
u := mustRegister(t, s, "[email protected]", "Alice", "password123")
token, _, err := s.CreateSession(context.Background(), u.ID)
if err != nil {
t.Fatalf("CreateSession: %v", err)
}
got, exp, err := s.ResolveSession(context.Background(), token)
if err != nil {
t.Fatalf("ResolveSession: %v", err)
}
if got.ID != u.ID {
t.Errorf("resolved user %d, want %d", got.ID, u.ID)
}
if !exp.After(s.now()) {
t.Errorf("resolved expiry %v is not in the future", exp)
}
// Logout invalidates it.
if err := s.Logout(context.Background(), token); err != nil {
t.Fatalf("Logout: %v", err)
}
if _, _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("resolve after logout err = %v, want ErrNotFound", err)
}
}
func TestResolveSessionRejectsGarbageToken(t *testing.T) {
s := newTestService(t, openConfig())
if _, _, err := s.ResolveSession(context.Background(), "not-a-real-token"); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("garbage token err = %v, want ErrNotFound", err)
}
if _, _, err := s.ResolveSession(context.Background(), ""); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("empty token err = %v, want ErrNotFound", err)
}
}
func TestSessionExpiryAndLazyDeletion(t *testing.T) {
s := newTestService(t, openConfig())
u := mustRegister(t, s, "[email protected]", "Alice", "password123")
base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
s.now = func() time.Time { return base }
token, _, err := s.CreateSession(context.Background(), u.ID)
if err != nil {
t.Fatalf("CreateSession: %v", err)
}
// Jump past the 30-day TTL: the session must be treated as gone...
s.now = func() time.Time { return base.Add(sessionTTL + time.Hour) }
if _, _, err := s.ResolveSession(context.Background(), token); !errors.Is(err, domain.ErrNotFound) {
t.Fatalf("expired resolve err = %v, want ErrNotFound", err)
}
// ...and lazily deleted from the store.
if _, err := s.store.GetSession(context.Background(), hashToken(token)); !errors.Is(err, domain.ErrNotFound) {
t.Errorf("expired session row still present: %v", err)
}
}
func TestSessionSlidingRenewal(t *testing.T) {
s := newTestService(t, openConfig())
u := mustRegister(t, s, "[email protected]", "Alice", "password123")
base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
s.now = func() time.Time { return base }
token, firstExp, err := s.CreateSession(context.Background(), u.ID)
if err != nil {
t.Fatalf("CreateSession: %v", err)
}
// Use it 10 days later; expiry should slide forward.
s.now = func() time.Time { return base.Add(10 * 24 * time.Hour) }
_, resolvedExp, err := s.ResolveSession(context.Background(), token)
if err != nil {
t.Fatalf("ResolveSession: %v", err)
}
if !resolvedExp.After(firstExp) {
t.Errorf("returned expiry did not slide: %v not after %v", resolvedExp, firstExp)
}
sess, err := s.store.GetSession(context.Background(), hashToken(token))
if err != nil {
t.Fatalf("GetSession: %v", err)
}
newExp, err := parseTime(sess.ExpiresAt)
if err != nil {
t.Fatalf("parse expiry: %v", err)
}
if !newExp.After(firstExp) {
t.Errorf("expiry did not slide: new %v not after first %v", newExp, firstExp)
}
}
func TestCleanupExpiredSessions(t *testing.T) {
s := newTestService(t, openConfig())
u := mustRegister(t, s, "[email protected]", "Alice", "password123")
base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
s.now = func() time.Time { return base }
if _, _, err := s.CreateSession(context.Background(), u.ID); err != nil {
t.Fatalf("CreateSession: %v", err)
}
// Before expiry: nothing to clean.
if n, err := s.CleanupExpiredSessions(context.Background()); err != nil || n != 0 {
t.Fatalf("early cleanup = (%d, %v), want (0, nil)", n, err)
}
// After expiry: the one session is purged.
s.now = func() time.Time { return base.Add(sessionTTL + time.Hour) }
if n, err := s.CleanupExpiredSessions(context.Background()); err != nil || n != 1 {
t.Fatalf("late cleanup = (%d, %v), want (1, nil)", n, err)
}
}
func TestProvidersReflectsConfig(t *testing.T) {
cfg := openConfig()
cfg.OIDC.ButtonLabel = "Sign in with Authentik"
s := newTestService(t, cfg)
p := s.Providers()
if !p.Local {
t.Error("expected local=true")
}
if p.OIDC {
t.Error("OIDC must be false until #5 wires it")
}
}
+102
View File
@@ -0,0 +1,102 @@
package service
import (
"crypto/rand"
"crypto/subtle"
"encoding/base64"
"errors"
"fmt"
"strings"
"golang.org/x/crypto/argon2"
)
// argon2id parameters: RFC 9106's second recommended profile (m=64 MiB, t=3,
// p=4) — the memory-constrained option, appropriate for a self-hosted box where
// the 2 GiB first profile is too heavy. They are encoded into every stored hash,
// so raising them later leaves old hashes verifiable.
const (
argonMemKiB = 64 * 1024 // 64 MiB
argonTime = 3
Outdated
Review

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

security · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Argon2id t=1 with m=64MiB/p=4 doesn't match either RFC 9106 recommended parameter profile, weakening password hash resistance** _security · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
argonThreads = 4
argonKeyLen = 32
argonSaltLen = 16
)
// 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
Review

🟡 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>
// and log it (Login does).
var errBadHash = errors.New("service: malformed password hash")
// b64 is the padding-free base64 used inside the PHC-style hash string.
var b64 = base64.RawStdEncoding
// timingHash is a valid argon2id hash used to equalize login response time when
// an email is unknown (see Service.dummyHash). It uses a fixed salt rather than
// crypto/rand so it can never fail to be produced at startup — an all-important
// property, since a missing timing hash would silently re-open account
// enumeration. It guards nothing, so a static salt is safe; deriving it from the
// live argon parameters keeps its cost matched to a real verify.
func timingHash() string {
salt := []byte("pansy-timing-eq!") // exactly argonSaltLen (16) bytes
key := argon2.IDKey([]byte("x"), salt, argonTime, argonMemKiB, argonThreads, argonKeyLen)
return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s",
argon2.Version, argonMemKiB, argonTime, argonThreads,
b64.EncodeToString(salt), b64.EncodeToString(key),
)
}
// hashPassword returns a self-describing argon2id hash in the PHC string format
// "$argon2id$v=19$m=...,t=...,p=...$salt$hash" (all base64, no padding).
func hashPassword(password string) (string, error) {
salt := make([]byte, argonSaltLen)
if _, err := rand.Read(salt); err != nil {
return "", fmt.Errorf("service: generate salt: %w", err)
}
key := argon2.IDKey([]byte(password), salt, argonTime, argonMemKiB, argonThreads, argonKeyLen)
return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s",
argon2.Version, argonMemKiB, argonTime, argonThreads,
b64.EncodeToString(salt), b64.EncodeToString(key),
), nil
}
// verifyPassword reports whether password matches the encoded argon2id hash. The
// comparison is constant-time. A malformed encoded value returns errBadHash.
func verifyPassword(encoded, password string) (bool, error) {
mem, t, threads, salt, want, err := decodeHash(encoded)
if err != nil {
return false, err
}
Review

decodeHash inconsistently shadows vs. assigns the named err return

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **decodeHash inconsistently shadows vs. assigns the named err return** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
got := argon2.IDKey([]byte(password), salt, t, mem, threads, uint32(len(want)))
return subtle.ConstantTimeCompare(got, want) == 1, nil
}
// decodeHash parses a PHC-format argon2id string into its parameters, salt, and
// derived key. Any structural problem returns errBadHash (with zero values) so
// the caller never has to distinguish parse failures.
func decodeHash(encoded string) (mem, t uint32, threads uint8, salt, key []byte, err error) {
fail := func() (uint32, uint32, uint8, []byte, []byte, error) {
return 0, 0, 0, nil, nil, errBadHash
}
parts := strings.Split(encoded, "$")
// ["", "argon2id", "v=19", "m=..,t=..,p=..", "<salt>", "<hash>"]
if len(parts) != 6 || parts[1] != "argon2id" {
return fail()
}
var version int
if _, e := fmt.Sscanf(parts[2], "v=%d", &version); e != nil || version != argon2.Version {
return fail()
}
if _, e := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &mem, &t, &threads); e != nil {
return fail()
}
if salt, err = b64.DecodeString(parts[4]); err != nil {
return fail()
}
if key, err = b64.DecodeString(parts[5]); err != nil || len(key) == 0 {
return fail()
}
return mem, t, threads, salt, key, nil
}
+54
View File
@@ -0,0 +1,54 @@
package service
import (
"strings"
"testing"
)
func TestHashAndVerifyPassword(t *testing.T) {
hash, err := hashPassword("correct-horse-battery-staple")
if err != nil {
t.Fatalf("hashPassword: %v", err)
}
if !strings.HasPrefix(hash, "$argon2id$v=19$") {
t.Errorf("hash %q lacks argon2id PHC prefix", hash)
}
ok, err := verifyPassword(hash, "correct-horse-battery-staple")
if err != nil || !ok {
t.Errorf("verify correct = (%v, %v), want (true, nil)", ok, err)
}
ok, err = verifyPassword(hash, "wrong-password")
if err != nil || ok {
t.Errorf("verify wrong = (%v, %v), want (false, nil)", ok, err)
}
}
func TestHashPasswordIsSalted(t *testing.T) {
// Two hashes of the same password must differ (random salt), yet both verify.
h1, _ := hashPassword("same-password")
h2, _ := hashPassword("same-password")
if h1 == h2 {
t.Error("two hashes of the same password are identical; salt not random")
}
for _, h := range []string{h1, h2} {
if ok, err := verifyPassword(h, "same-password"); err != nil || !ok {
t.Errorf("verify(%q) = (%v, %v), want (true, nil)", h, ok, err)
}
}
}
func TestVerifyPasswordRejectsMalformedHash(t *testing.T) {
for _, bad := range []string{
"",
"not-a-hash",
"$argon2id$v=19$m=65536,t=1,p=4$onlyfourparts",
"$argon2i$v=19$m=65536,t=1,p=4$c2FsdA$aGFzaA", // wrong variant
"$argon2id$v=1$m=65536,t=1,p=4$c2FsdA$aGFzaA", // wrong version
} {
if _, err := verifyPassword(bad, "whatever"); err == nil {
t.Errorf("verifyPassword(%q) err = nil, want errBadHash", bad)
}
}
}
+75
View File
@@ -0,0 +1,75 @@
// Package service is pansy's business-logic seam: all permission checks and
// invariants live here rather than in the HTTP handlers. Resource operations
Outdated
Review

🟡 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>
// take (ctx, actor, args) so every rule is enforced regardless of caller; the
// auth operations here are the exception — they establish the actor, so they
// take credentials rather than one. REST handlers (internal/api) and, later,
// agent tools (internal/agent) are thin adapters over these methods, so both
// inherit the same rules. This file holds the shared plumbing; feature methods
// live alongside it (auth.go, and gardens/objects/… in later issues).
package service
import (
"crypto/rand"
"crypto/sha256"
"encoding/base64"
"encoding/hex"
"fmt"
"time"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/store"
)
// timeLayout is the ISO-8601 UTC format used for every timestamp pansy stores.
// It matches the schema's strftime('%Y-%m-%dT%H:%M:%SZ') so string comparison
// (e.g. session expiry) is equivalent to time comparison.
const timeLayout = "2006-01-02T15:04:05Z"
// sessionTTL is how long a session lives from its last use (sliding expiry).
const sessionTTL = 30 * 24 * time.Hour
// Service holds the dependencies shared by every operation.
type Service struct {
store *store.DB
cfg *config.Config
// now is the clock, injectable so tests can advance time (session expiry).
now func() time.Time
// dummyHash is a valid argon2id hash Login verifies against when an email is
// unknown, so response time doesn't reveal whether an account exists. It is
// produced by timingHash (fixed salt, no RNG) so it is always present — an
// empty one would silently re-open account enumeration.
dummyHash string
}
// New constructs a Service.
func New(st *store.DB, cfg *config.Config) *Service {
Review

🔴 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>
return &Service{
store: st,
cfg: cfg,
now: time.Now,
dummyHash: timingHash(),
}
}
// formatTime renders a time as pansy's canonical UTC string.
func formatTime(t time.Time) string { return t.UTC().Format(timeLayout) }
// parseTime parses a canonical pansy timestamp.
func parseTime(s string) (time.Time, error) { return time.Parse(timeLayout, s) }
// newSessionToken returns a fresh URL-safe random bearer token (32 bytes of
// entropy). The raw token goes in the cookie; only its hash is persisted.
func newSessionToken() (string, error) {
b := make([]byte, 32)
if _, err := rand.Read(b); err != nil {
return "", fmt.Errorf("service: generate session token: %w", err)
}
return base64.RawURLEncoding.EncodeToString(b), nil
}
// hashToken maps a raw bearer token to the hex sha256 stored as the session's
// primary key.
func hashToken(raw string) string {
sum := sha256.Sum256([]byte(raw))
return hex.EncodeToString(sum[:])
}
@@ -0,0 +1,7 @@
-- 0002_sessions_expires_index.sql — index sessions.expires_at.
--
-- The periodic sweep (and lazy cleanup) run `DELETE FROM sessions WHERE
-- expires_at <= ?`; without this index that is a full table scan. The table is
-- small at household scale, but the index is cheap insurance and keeps the sweep
-- O(expired) rather than O(all sessions).
CREATE INDEX idx_sessions_expires ON sessions (expires_at);
+80
View File
@@ -0,0 +1,80 @@
package store
import (
"context"
"database/sql"
"errors"
"fmt"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// CreateSession inserts a session row. The raw bearer token is never stored;
// s.TokenHash is its sha256 (computed by the service layer). ExpiresAt is an
// ISO-8601 UTC string, lexicographically comparable to the schema's timestamps.
func (d *DB) CreateSession(ctx context.Context, s *domain.Session) error {
_, err := d.sql.ExecContext(ctx,
`INSERT INTO sessions (token_hash, user_id, expires_at) VALUES (?, ?, ?)`,
s.TokenHash, s.UserID, s.ExpiresAt,
)
if err != nil {
return fmt.Errorf("store: insert session: %w", err)
}
return nil
}
// GetSession returns the session for a token hash, or domain.ErrNotFound. It
// does not check expiry — that is the service layer's concern (which also
// implements sliding renewal).
func (d *DB) GetSession(ctx context.Context, tokenHash string) (*domain.Session, error) {
var s domain.Session
err := d.sql.QueryRowContext(ctx,
`SELECT token_hash, user_id, expires_at, created_at FROM sessions WHERE token_hash = ?`,
tokenHash,
).Scan(&s.TokenHash, &s.UserID, &s.ExpiresAt, &s.CreatedAt)
if errors.Is(err, sql.ErrNoRows) {
return nil, domain.ErrNotFound
}
if err != nil {
return nil, fmt.Errorf("store: get session: %w", err)
}
return &s, nil
}
// TouchSession moves a session's expiry forward (sliding-expiry renewal).
func (d *DB) TouchSession(ctx context.Context, tokenHash, expiresAt string) error {
_, err := d.sql.ExecContext(ctx,
`UPDATE sessions SET expires_at = ? WHERE token_hash = ?`, expiresAt, tokenHash,
)
if err != nil {
return fmt.Errorf("store: touch session: %w", err)
}
return nil
}
// DeleteSession removes a session (logout, or lazy cleanup of an expired one).
// Deleting a nonexistent session is not an error.
func (d *DB) DeleteSession(ctx context.Context, tokenHash string) error {
if _, err := d.sql.ExecContext(ctx,
`DELETE FROM sessions WHERE token_hash = ?`, tokenHash,
); err != nil {
return fmt.Errorf("store: delete session: %w", err)
}
return nil
}
// DeleteExpiredSessions removes every session that expired at or before now
// (an ISO-8601 UTC string) and returns how many rows were deleted.
func (d *DB) DeleteExpiredSessions(ctx context.Context, now string) (int64, error) {
res, err := d.sql.ExecContext(ctx,
`DELETE FROM sessions WHERE expires_at <= ?`, now,
)
if err != nil {
return 0, fmt.Errorf("store: delete expired sessions: %w", err)
}
n, err := res.RowsAffected()
if err != nil {
return 0, fmt.Errorf("store: expired sessions affected: %w", err)
}
return n, nil
}
+5 -5
View File
@@ -42,15 +42,15 @@ func TestMigrateCreatesSchema(t *testing.T) {
if err := db.SQL().QueryRow(`SELECT max(version) FROM schema_migrations`).Scan(&version); err != nil {
t.Fatalf("read schema_migrations: %v", err)
}
if version != 1 {
t.Errorf("schema version = %d, want 1", version)
if version != 2 {
t.Errorf("schema version = %d, want 2", version)
}
}
func TestMigrateIsIdempotent(t *testing.T) {
db := openTestDB(t)
// A second Migrate must be a no-op and leave exactly one recorded version.
// A second Migrate must be a no-op and leave exactly one row per migration.
if err := db.Migrate(context.Background()); err != nil {
t.Fatalf("second Migrate: %v", err)
}
@@ -58,8 +58,8 @@ func TestMigrateIsIdempotent(t *testing.T) {
if err := db.SQL().QueryRow(`SELECT count(*) FROM schema_migrations`).Scan(&count); err != nil {
t.Fatalf("count schema_migrations: %v", err)
}
if count != 1 {
t.Errorf("schema_migrations rows = %d, want 1", count)
if count != 2 {
t.Errorf("schema_migrations rows = %d, want 2", count)
}
}
+131
View File
@@ -0,0 +1,131 @@
package store
import (
"context"
"database/sql"
"errors"
"fmt"
"strings"
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
)
// userColumns lists the users columns in the fixed order scanUser expects.
const userColumns = `id, email, display_name, password_hash, oidc_issuer, oidc_subject, is_admin, version, created_at, updated_at`
// scanner is satisfied by both *sql.Row and *sql.Rows, so scanUser works for
// single-row and multi-row queries alike.
type scanner interface {
Scan(dest ...any) error
}
// scanUser reads one users row. is_admin is stored as INTEGER 0/1 (the driver
Review

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

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **scanUser comment says is_admin read into an int but code declares int64** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
// returns it as int64, which does not convert straight to bool), so it is read
// into an int64 and mapped.
func scanUser(s scanner) (*domain.User, error) {
var (
u domain.User
isAdmin int64
)
if err := s.Scan(
&u.ID, &u.Email, &u.DisplayName, &u.PasswordHash,
&u.OIDCIssuer, &u.OIDCSubject, &isAdmin, &u.Version,
&u.CreatedAt, &u.UpdatedAt,
); err != nil {
return nil, err
}
u.IsAdmin = isAdmin != 0
return &u, nil
}
// CreateUser inserts a new user and returns the stored row (with generated id
// and timestamps). PasswordHash/OIDCIssuer/OIDCSubject may be nil.
//
// Two invariants are enforced inside the single INSERT statement so concurrent
// registrations can't violate them (SQLite serializes writers, and the COUNT
// subqueries see the latest committed state):
// - is_admin is set iff this is the first user — no read-then-write window in
// which two "first" registrations both become admin.
// - the row is inserted only when the table is empty (bootstrap) or allowSignup
// is true; otherwise zero rows are affected and ErrRegistrationClosed is
// returned. This is the authoritative registration gate.
//
// A duplicate email trips the UNIQUE index and maps to ErrEmailTaken.
func (d *DB) CreateUser(ctx context.Context, u *domain.User, allowSignup bool) (*domain.User, error) {
res, err := d.sql.ExecContext(ctx,
`INSERT INTO users (email, display_name, password_hash, oidc_issuer, oidc_subject, is_admin)
SELECT ?, ?, ?, ?, ?, (SELECT count(*) FROM users) = 0
WHERE (SELECT count(*) FROM users) = 0 OR ?`,
u.Email, u.DisplayName, u.PasswordHash, u.OIDCIssuer, u.OIDCSubject, boolToInt(allowSignup),
)
if err != nil {
if isUniqueViolation(err) {
return nil, domain.ErrEmailTaken
}
return nil, fmt.Errorf("store: insert user: %w", err)
}
n, err := res.RowsAffected()
if err != nil {
return nil, fmt.Errorf("store: user insert rows: %w", err)
}
if n == 0 {
return nil, domain.ErrRegistrationClosed
}
id, err := res.LastInsertId()
if err != nil {
return nil, fmt.Errorf("store: user insert id: %w", err)
}
return d.GetUserByID(ctx, id)
}
// GetUserByID returns the user with the given id, or domain.ErrNotFound.
func (d *DB) GetUserByID(ctx context.Context, id int64) (*domain.User, error) {
u, err := scanUser(d.sql.QueryRowContext(ctx,
`SELECT `+userColumns+` FROM users WHERE id = ?`, id))
if errors.Is(err, sql.ErrNoRows) {
return nil, domain.ErrNotFound
}
if err != nil {
return nil, fmt.Errorf("store: get user by id: %w", err)
}
return u, nil
}
// GetUserByEmail returns the user with the given email (case-insensitive via the
// column's NOCASE collation), or domain.ErrNotFound.
func (d *DB) GetUserByEmail(ctx context.Context, email string) (*domain.User, error) {
u, err := scanUser(d.sql.QueryRowContext(ctx,
`SELECT `+userColumns+` FROM users WHERE email = ?`, email))
if errors.Is(err, sql.ErrNoRows) {
return nil, domain.ErrNotFound
}
if err != nil {
return nil, fmt.Errorf("store: get user by email: %w", err)
}
return u, nil
}
// CountUsers returns the number of user rows. Used to decide first-user-is-admin
// and to allow bootstrap registration when signup is otherwise closed.
func (d *DB) CountUsers(ctx context.Context) (int, error) {
var n int
if err := d.sql.QueryRowContext(ctx, `SELECT count(*) FROM users`).Scan(&n); err != nil {
return 0, fmt.Errorf("store: count users: %w", err)
}
return n, nil
}
// boolToInt maps a Go bool to the 0/1 SQLite stores for INTEGER "boolean" columns.
func boolToInt(b bool) int {
if b {
return 1
}
return 0
}
// isUniqueViolation reports whether err is a SQLite UNIQUE-constraint failure.
// modernc surfaces these in the error text; the message is stable across SQLite
// versions ("UNIQUE constraint failed: <table>.<column>").
func isUniqueViolation(err error) bool {
return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed")
}