diff --git a/internal/api/api.go b/internal/api/api.go index be57af0..87ef607 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -44,6 +44,9 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { 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); diff --git a/internal/api/auth.go b/internal/api/auth.go index 69a7e0a..ca06dec 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -4,6 +4,7 @@ import ( "errors" "log/slog" "net/http" + "net/url" "strings" "time" @@ -30,7 +31,7 @@ type registerRequest struct { type loginRequest struct { Email string `json:"email" binding:"required,email"` - Password string `json:"password" binding:"required"` + Password string `json:"password" binding:"required,max=1024"` } // register creates a local account and logs it in (sets the session cookie). @@ -47,15 +48,11 @@ func (h *handlers) register(c *gin.Context) { Password: req.Password, }) if err != nil { - h.writeServiceError(c, err) + writeServiceError(c, err) return } - if err := h.startSession(c, user.ID); err != nil { - writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not start session") - return - } - c.JSON(http.StatusOK, user) + h.startSessionAndRespond(c, user) } // login verifies credentials and sets the session cookie. @@ -68,15 +65,11 @@ func (h *handlers) login(c *gin.Context) { user, err := h.svc.Login(c.Request.Context(), req.Email, req.Password) if err != nil { - h.writeServiceError(c, err) + writeServiceError(c, err) return } - if err := h.startSession(c, user.ID); err != nil { - writeAPIError(c, http.StatusInternalServerError, "INTERNAL", "could not start session") - return - } - c.JSON(http.StatusOK, user) + h.startSessionAndRespond(c, user) } // logout deletes the current session (if any) and clears the cookie. It is @@ -102,37 +95,84 @@ func (h *handlers) providers(c *gin.Context) { } // 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. +// 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 == "" { - writeAPIError(c, http.StatusUnauthorized, "UNAUTHENTICATED", "authentication required") - c.Abort() + abortUnauthenticated(c) return } - user, err := h.svc.ResolveSession(c.Request.Context(), token) + user, expiresAt, err := h.svc.ResolveSession(c.Request.Context(), token) if err != nil { // Any resolution failure (missing/expired/tampered) is 401, never 404: // the client should log in, not think a resource is gone. - writeAPIError(c, http.StatusUnauthorized, "UNAUTHENTICATED", "authentication required") - c.Abort() + 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() } } -// startSession issues a session and writes the cookie. -func (h *handlers) startSession(c *gin.Context, userID int64) error { - token, expiresAt, err := h.svc.CreateSession(c.Request.Context(), userID) +// 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 + } + return func(c *gin.Context) { + if wantHost == "" { + c.Next() + return + } + 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. +func (h *handlers) startSessionAndRespond(c *gin.Context, user *domain.User) { + token, expiresAt, err := h.svc.CreateSession(c.Request.Context(), user.ID) if err != nil { - return err + // 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) - return nil + c.JSON(http.StatusOK, user) } // cookieSecure reports whether the session cookie should carry the Secure @@ -143,6 +183,10 @@ func (h *handlers) cookieSecure() bool { } 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) @@ -161,7 +205,7 @@ func mustActor(c *gin.Context) *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 (h *handlers) writeServiceError(c *gin.Context, err error) { +func writeServiceError(c *gin.Context, err error) { switch { case errors.Is(err, domain.ErrInvalidCredentials): writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password") diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index 60215d9..af0b338 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -35,16 +35,22 @@ func localCfg() *config.Config { return &config.Config{Registration: config.RegistrationOpen, LocalAuth: true} } -// 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 { +// 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 body != nil { - if err := json.NewEncoder(&buf).Encode(body); err != nil { + if v != nil { + if err := json.NewEncoder(&buf).Encode(v); err != nil { t.Fatalf("encode body: %v", err) } } - req := httptest.NewRequest(method, path, &buf) + 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) @@ -153,6 +159,71 @@ func TestProvidersEndpoint(t *testing.T) { } } +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": "a@example.com", "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": "a@example.com", "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": "a@example.com", "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" diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 7561146..57e4766 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -117,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. @@ -157,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"` } diff --git a/internal/service/auth.go b/internal/service/auth.go index c5da735..6d284b2 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -3,12 +3,17 @@ 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 @@ -46,27 +51,22 @@ func (s *Service) Register(ctx context.Context, in RegisterInput) (*domain.User, email := normalizeEmail(in.Email) displayName := strings.TrimSpace(in.DisplayName) - if email == "" || displayName == "" || in.Password == "" { + if email == "" || displayName == "" || in.Password == "" || len(in.Password) > maxPasswordLen { return nil, domain.ErrInvalidInput } - count, err := s.store.CountUsers(ctx) - if err != nil { - return nil, err - } - // Closed registration still allows the very first account so a locked-down - // instance can be bootstrapped without editing config. - if count > 0 && !s.cfg.RegistrationOpen() { - return nil, domain.ErrRegistrationClosed - } - - // Friendly duplicate check. The UNIQUE index is the real guard against a - // race; that path surfaces as a generic insert error (500), acceptable at - // household scale. - if _, err := s.store.GetUserByEmail(ctx, email); err == nil { - return nil, domain.ErrEmailTaken - } else if !errors.Is(err, domain.ErrNotFound) { - return nil, err + // 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) @@ -78,8 +78,7 @@ func (s *Service) Register(ctx context.Context, in RegisterInput) (*domain.User, Email: email, DisplayName: displayName, PasswordHash: &hash, - IsAdmin: count == 0, - }) + }, s.cfg.RegistrationOpen()) } // Login verifies an email/password pair and returns the user. Unknown-email and @@ -89,6 +88,10 @@ func (s *Service) Login(ctx context.Context, email, password string) (*domain.Us 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 { @@ -105,7 +108,13 @@ func (s *Service) Login(ctx context.Context, email, password string) (*domain.Us } ok, err := verifyPassword(*u.PasswordHash, password) - if err != nil || !ok { + 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 @@ -129,38 +138,53 @@ func (s *Service) CreateSession(ctx context.Context, userID int64) (token string return raw, exp, nil } -// ResolveSession validates a raw bearer token and returns its user. 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, error) { +// ResolveSession validates a raw bearer token and returns its user and the +// 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, domain.ErrNotFound + return nil, time.Time{}, domain.ErrNotFound } hash := hashToken(rawToken) sess, err := s.store.GetSession(ctx, hash) if err != nil { - return nil, err + return nil, time.Time{}, err } 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) - return nil, domain.ErrNotFound + 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) { - _ = s.store.DeleteSession(ctx, hash) - return nil, domain.ErrNotFound + 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 { - _ = s.store.TouchSession(ctx, hash, formatTime(newExp)) + 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 + } } - return s.store.GetUserByID(ctx, sess.UserID) + 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. diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index f583452..5a85bde 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -3,6 +3,7 @@ package service import ( "context" "errors" + "strings" "testing" "time" @@ -98,6 +99,20 @@ func TestRegistrationClosedAllowsBootstrapThenBlocks(t *testing.T) { } } +func TestRejectsOverlongPassword(t *testing.T) { + s := newTestService(t, openConfig()) + long := strings.Repeat("a", maxPasswordLen+1) + + if _, err := s.Register(context.Background(), RegisterInput{Email: "a@example.com", DisplayName: "A", Password: long}); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("register overlong err = %v, want ErrInvalidInput", err) + } + + mustRegister(t, s, "b@example.com", "Bob", "password123") + if _, err := s.Login(context.Background(), "b@example.com", 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 @@ -139,7 +154,7 @@ func TestLoginRejectsOIDCOnlyUser(t *testing.T) { iss, sub := "https://idp.example", "subject-1" if _, err := s.store.CreateUser(context.Background(), &domain.User{ Email: "oidc@example.com", DisplayName: "O", OIDCIssuer: &iss, OIDCSubject: &sub, - }); err != nil { + }, true); err != nil { t.Fatalf("seed oidc user: %v", err) } if _, err := s.Login(context.Background(), "oidc@example.com", "anything"); !errors.Is(err, domain.ErrInvalidCredentials) { @@ -156,29 +171,32 @@ func TestSessionLifecycle(t *testing.T) { t.Fatalf("CreateSession: %v", err) } - got, err := s.ResolveSession(context.Background(), token) + 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) { + 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) { + 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) { + if _, _, err := s.ResolveSession(context.Background(), ""); !errors.Is(err, domain.ErrNotFound) { t.Errorf("empty token err = %v, want ErrNotFound", err) } } @@ -197,7 +215,7 @@ func TestSessionExpiryAndLazyDeletion(t *testing.T) { // 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) { + 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. @@ -219,9 +237,13 @@ func TestSessionSlidingRenewal(t *testing.T) { // Use it 10 days later; expiry should slide forward. s.now = func() time.Time { return base.Add(10 * 24 * time.Hour) } - if _, err := s.ResolveSession(context.Background(), token); err != nil { + _, 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) diff --git a/internal/service/password.go b/internal/service/password.go index fa34ae6..634b554 100644 --- a/internal/service/password.go +++ b/internal/service/password.go @@ -11,13 +11,13 @@ import ( "golang.org/x/crypto/argon2" ) -// argon2id parameters. ~64 MiB memory / 1 pass / 4 lanes is the interactive -// profile recommended by the argon2 authors and is comfortable on a -// self-hosted box. They are encoded into every stored hash, so raising them -// later leaves old hashes verifiable. +// 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 = 1 + argonTime = 3 argonThreads = 4 argonKeyLen = 32 argonSaltLen = 16 @@ -25,12 +25,27 @@ const ( // 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 -// but should log it. +// 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) { @@ -57,26 +72,31 @@ func verifyPassword(encoded, password string) (bool, error) { } // decodeHash parses a PHC-format argon2id string into its parameters, salt, and -// derived key. +// 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) { - parts := strings.Split(encoded, "$") - // ["", "argon2id", "v=19", "m=..,t=..,p=..", "", ""] - if len(parts) != 6 || parts[1] != "argon2id" { + fail := func() (uint32, uint32, uint8, []byte, []byte, error) { return 0, 0, 0, nil, nil, errBadHash } - var version int - if _, err := fmt.Sscanf(parts[2], "v=%d", &version); err != nil || version != argon2.Version { - return 0, 0, 0, nil, nil, errBadHash + parts := strings.Split(encoded, "$") + // ["", "argon2id", "v=19", "m=..,t=..,p=..", "", ""] + if len(parts) != 6 || parts[1] != "argon2id" { + return fail() } - if _, err := fmt.Sscanf(parts[3], "m=%d,t=%d,p=%d", &mem, &t, &threads); err != nil { - return 0, 0, 0, nil, nil, errBadHash + + 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 0, 0, 0, nil, nil, errBadHash + return fail() } if key, err = b64.DecodeString(parts[5]); err != nil || len(key) == 0 { - return 0, 0, 0, nil, nil, errBadHash + return fail() } return mem, t, threads, salt, key, nil } diff --git a/internal/service/password_test.go b/internal/service/password_test.go index 8470555..8e6b486 100644 --- a/internal/service/password_test.go +++ b/internal/service/password_test.go @@ -45,7 +45,7 @@ func TestVerifyPasswordRejectsMalformedHash(t *testing.T) { "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 + "$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) diff --git a/internal/service/service.go b/internal/service/service.go index bf6aadc..b33cedd 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -1,9 +1,11 @@ -// 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 -// live here rather than in the HTTP handlers. 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 is pansy's business-logic seam: all permission checks and +// invariants live here rather than in the HTTP handlers. Resource operations +// 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 ( @@ -12,7 +14,6 @@ import ( "encoding/base64" "encoding/hex" "fmt" - "log/slog" "time" "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" @@ -33,23 +34,21 @@ type Service struct { 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 of a throwaway password. Login verifies - // against it when an email is unknown so the response time doesn't reveal - // whether an account exists. + // 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. 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. +// New constructs a Service. func New(st *store.DB, cfg *config.Config) *Service { - s := &Service{store: st, cfg: cfg, now: time.Now} - if h, err := hashPassword("pansy-timing-equalizer-not-a-real-password"); err != nil { - slog.Warn("service: could not precompute login timing hash", "error", err) - } else { - s.dummyHash = h + return &Service{ + store: st, + cfg: cfg, + now: time.Now, + dummyHash: timingHash(), } - return s } // formatTime renders a time as pansy's canonical UTC string. diff --git a/internal/store/migrations/0002_sessions_expires_index.sql b/internal/store/migrations/0002_sessions_expires_index.sql new file mode 100644 index 0000000..a956310 --- /dev/null +++ b/internal/store/migrations/0002_sessions_expires_index.sql @@ -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); diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 9cd5154..9c5e4a3 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -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) } } diff --git a/internal/store/users.go b/internal/store/users.go index e92a36b..a7e3548 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -5,6 +5,7 @@ import ( "database/sql" "errors" "fmt" + "strings" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" ) @@ -20,7 +21,7 @@ type scanner interface { // 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 -// into an int and mapped. +// into an int64 and mapped. func scanUser(s scanner) (*domain.User, error) { var ( u domain.User @@ -39,15 +40,37 @@ func scanUser(s scanner) (*domain.User, error) { // CreateUser inserts a new user and returns the stored row (with generated id // and timestamps). PasswordHash/OIDCIssuer/OIDCSubject may be nil. -func (d *DB) CreateUser(ctx context.Context, u *domain.User) (*domain.User, error) { +// +// 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) - VALUES (?, ?, ?, ?, ?, ?)`, - u.Email, u.DisplayName, u.PasswordHash, u.OIDCIssuer, u.OIDCSubject, boolToInt(u.IsAdmin), + 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) @@ -99,3 +122,10 @@ func boolToInt(b bool) int { } 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: ."). +func isUniqueViolation(err error) bool { + return err != nil && strings.Contains(err.Error(), "UNIQUE constraint failed") +}