Address Gadfly review on #5: OIDC identity guard, verified-email, timeouts

Fixes from the PR #24 adversarial review (graded 23 real / 1 false positive):

Security / correctness
- LinkOIDC no longer overwrites a different stored identity: the UPDATE
  matches only when the row has no identity yet or already carries this
  exact one, so a second IdP asserting the same verified email can't
  hijack or lock out an account (returns ErrOIDCIdentityConflict). Uses
  a single UPDATE...RETURNING (also fixes the ignored-RowsAffected /
  misleading-ErrNotFound path and the round-trip).
- Provisioning now requires a verified email for BOTH linking and JIT
  creation (was: linking only), so an unverified-email identity can't
  create an account — nor become the first admin on a fresh instance,
  nor squat an email a real user later owns.
- OIDC-identity collisions surface as the dedicated ErrOIDCIdentityConflict
  instead of the email-specific ErrEmailTaken.

Robustness
- readOIDCTxCookie requires a non-empty nonce (an empty one would make the
  callback's nonce check pass vacuously).
- Callback token exchange + verify run under a 15s context timeout so a
  slow IdP can't outlast the server write timeout.
- ensure() performs discovery outside the mutex, so concurrent cold-start
  requests don't serialize behind one another's full timeout.
- setOIDCTxCookie returns its marshal error; oidcLogin aborts rather than
  redirecting to the IdP with no tx cookie.

Maintainability
- redirectAuthError helper dedups the ~dozen callback redirects (and the
  empty-code path now logs like the rest).
- Distinct login error codes (no_email / email_unverified / oidc_conflict)
  for the UI; writeServiceError maps the OIDC sentinels; shared test issuer
  const.

Tests: unverified email refused for both link and JIT; identity-overwrite
refused while the original identity keeps working.

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

internal/api/auth.go

@@ -223,6 +223,12 @@ func writeServiceError(c *gin.Context, err error) {
 		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.ErrOIDCNoEmail):
+		writeAPIError(c, http.StatusBadRequest, "OIDC_NO_EMAIL", "the identity provider returned no email")
+	case errors.Is(err, domain.ErrOIDCEmailUnverified):
+		writeAPIError(c, http.StatusForbidden, "OIDC_EMAIL_UNVERIFIED", "the identity provider's email is not verified")
+	case errors.Is(err, domain.ErrOIDCIdentityConflict):
+		writeAPIError(c, http.StatusConflict, "OIDC_IDENTITY_CONFLICT", "this identity conflicts with an existing account")
 	case errors.Is(err, domain.ErrInvalidInput):
 		writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "invalid input")
 	default:

internal/api/oidc.go

@@ -6,6 +6,7 @@ import (
 	"crypto/subtle"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"log/slog"
 	"net/http"
 	"sync"
@@ -16,6 +17,7 @@ import (
 	"golang.org/x/oauth2"
 
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
+	"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/service"
 )
 
@@ -30,6 +32,10 @@ const (
 	// oidcDiscoveryTimeout bounds a single lazy discovery attempt so a hung IdP
 	// can't wedge a request goroutine.
 	oidcDiscoveryTimeout = 10 * time.Second
+	// oidcExchangeTimeout bounds the callback's token exchange + ID-token
+	// verification (which also fetches JWKS) so a slow IdP can't outlast the
+	// server's write timeout and leave the user on a blank page.
+	oidcExchangeTimeout = 15 * time.Second
 )
 
 // oidcClient lazily performs OIDC discovery and holds the derived verifier and
@@ -59,11 +65,14 @@ func newOIDCClient(cfg *config.Config) *oidcClient {
 
 // ensure performs discovery once (idempotent) and builds the verifier + oauth2
 // config. Safe for concurrent callers; a failure leaves the client uninitialized
-// so the next request retries.
+// so the next request retries. The network discovery runs OUTSIDE the mutex, so
+// concurrent cold-start (or IdP-outage) requests don't serialize behind one
+// another's full timeout; the first to finish installs the result.
 func (o *oidcClient) ensure(ctx context.Context) error {
 	o.mu.Lock()
-	defer o.mu.Unlock()
+	ready := o.provider != nil
-	if o.provider != nil {
+	o.mu.Unlock()
+	if ready {
 		return nil
 	}
 
@@ -74,6 +83,12 @@ func (o *oidcClient) ensure(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+
+	o.mu.Lock()
+	defer o.mu.Unlock()
+	if o.provider != nil {
+		return nil // another goroutine won the race; keep its result.
+	}
 	o.provider = provider
 	o.verifier = provider.Verifier(&oidc.Config{ClientID: o.clientID})
 	o.oauth = &oauth2.Config{
@@ -96,12 +111,19 @@ type oidcTx struct {
 	Nonce    string `json:"n"`
 }
 
+// redirectAuthError sends the browser back to the login page with an error code
+// the UI (#6) can render. Codes: oidc_unavailable, state, no_email,
+// email_unverified, oidc_conflict, oidc (generic).
+func redirectAuthError(c *gin.Context, code string) {
+	c.Redirect(http.StatusFound, "/login?error="+code)
+}
+
 // oidcLogin starts the authorization-code + PKCE flow: it stashes fresh state,
 // PKCE verifier, and nonce in a short-lived cookie, then redirects to the IdP.
 func (h *handlers) oidcLogin(c *gin.Context) {
 	if err := h.oidc.ensure(c.Request.Context()); err != nil {
 		slog.Error("api: oidc discovery failed", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc_unavailable")
+		redirectAuthError(c, "oidc_unavailable")
 		return
 	}
 
@@ -109,12 +131,16 @@ func (h *handlers) oidcLogin(c *gin.Context) {
 	nonce, err2 := randToken()
 	if err1 != nil || err2 != nil {
 		slog.Error("api: oidc token generation failed", "state_err", err1, "nonce_err", err2)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 	verifier := oauth2.GenerateVerifier()
 
-	h.setOIDCTxCookie(c, oidcTx{State: state, Verifier: verifier, Nonce: nonce})
+	if err := h.setOIDCTxCookie(c, oidcTx{State: state, Verifier: verifier, Nonce: nonce}); err != nil {
+		slog.Error("api: set oidc tx cookie", "error", err)
+		redirectAuthError(c, "oidc")
+		return
+	}
 
 	authURL := h.oidc.oauth.AuthCodeURL(state,
 		oauth2.S256ChallengeOption(verifier),
@@ -128,15 +154,13 @@ func (h *handlers) oidcLogin(c *gin.Context) {
 // pansy session. Every failure clears the tx cookie and redirects to the login
 // page with an error code rather than leaking details to the browser.
 func (h *handlers) oidcCallback(c *gin.Context) {
-	ctx := c.Request.Context()
-
 	tx, haveTx := h.readOIDCTxCookie(c)
 	h.clearOIDCTxCookie(c)
 
 	// A provider-side error (e.g. user denied consent) comes back as ?error=.
 	if e := c.Query("error"); e != "" {
 		slog.Warn("api: oidc provider returned error", "error", e)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 
@@ -145,45 +169,51 @@ func (h *handlers) oidcCallback(c *gin.Context) {
 	state := c.Query("state")
 	if !haveTx || state == "" || subtle.ConstantTimeCompare([]byte(state), []byte(tx.State)) != 1 {
 		slog.Warn("api: oidc state mismatch or missing transaction")
-		c.Redirect(http.StatusFound, "/login?error=state")
+		redirectAuthError(c, "state")
 		return
 	}
 
+	// Bound the network work (token exchange + JWKS fetch during verify) so a slow
+	// IdP can't outlast the server's write timeout.
+	ctx, cancel := context.WithTimeout(c.Request.Context(), oidcExchangeTimeout)
+	defer cancel()
+
 	if err := h.oidc.ensure(ctx); err != nil {
 		slog.Error("api: oidc discovery failed on callback", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc_unavailable")
+		redirectAuthError(c, "oidc_unavailable")
 		return
 	}
 
 	code := c.Query("code")
 	if code == "" {
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		slog.Warn("api: oidc callback missing code")
+		redirectAuthError(c, "oidc")
 		return
 	}
 
 	token, err := h.oidc.oauth.Exchange(ctx, code, oauth2.VerifierOption(tx.Verifier))
 	if err != nil {
 		slog.Error("api: oidc code exchange failed", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 
 	rawIDToken, ok := token.Extra("id_token").(string)
 	if !ok || rawIDToken == "" {
 		slog.Error("api: oidc token response missing id_token")
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 
 	idToken, err := h.oidc.verifier.Verify(ctx, rawIDToken)
 	if err != nil {
 		slog.Error("api: oidc id_token verification failed", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 	if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(tx.Nonce)) != 1 {
 		slog.Warn("api: oidc nonce mismatch")
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 
@@ -195,7 +225,7 @@ func (h *handlers) oidcCallback(c *gin.Context) {
 	}
 	if err := idToken.Claims(&claims); err != nil {
 		slog.Error("api: oidc claims decode failed", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 	name := claims.Name
@@ -213,31 +243,47 @@ func (h *handlers) oidcCallback(c *gin.Context) {
 	})
 	if err != nil {
 		slog.Warn("api: oidc provisioning failed", "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, oidcProvisionErrorCode(err))
 		return
 	}
 
 	if err := h.startSession(c, user.ID); err != nil {
 		slog.Error("api: oidc session start failed", "user_id", user.ID, "error", err)
-		c.Redirect(http.StatusFound, "/login?error=oidc")
+		redirectAuthError(c, "oidc")
 		return
 	}
 	c.Redirect(http.StatusFound, "/gardens")
 }
 
+// oidcProvisionErrorCode maps a LoginOIDC failure to a login-page error code so
+// the UI can explain what went wrong instead of showing a generic message.
+func oidcProvisionErrorCode(err error) string {
+	switch {
+	case errors.Is(err, domain.ErrOIDCNoEmail):
+		return "no_email"
+	case errors.Is(err, domain.ErrOIDCEmailUnverified):
+		return "email_unverified"
+	case errors.Is(err, domain.ErrOIDCIdentityConflict):
+		return "oidc_conflict"
+	default:
+		return "oidc"
+	}
+}
+
 // setOIDCTxCookie writes the login transaction as a compact base64 JSON cookie.
-func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) {
+// It returns an error rather than swallowing a marshal failure, so oidcLogin
+// never redirects to the IdP with no way to complete the callback.
+func (h *handlers) setOIDCTxCookie(c *gin.Context, tx oidcTx) error {
 	b, err := json.Marshal(tx)
 	if err != nil {
-		// tx holds only our own generated strings, so this cannot fail in practice.
+		return err // tx holds only our own strings, so this can't happen in practice.
-		slog.Error("api: marshal oidc tx", "error", err)
-		return
 	}
 	value := base64.RawURLEncoding.EncodeToString(b)
 	// SameSite=Lax so the cookie survives the IdP's top-level redirect back to the
 	// callback (Strict would drop it on that cross-site navigation).
 	c.SetSameSite(http.SameSiteLaxMode)
 	c.SetCookie(oidcTxCookie, value, int(oidcTxMaxAge.Seconds()), "/", "", h.cookieSecure(), true)
+	return nil
 }
 
 func (h *handlers) readOIDCTxCookie(c *gin.Context) (oidcTx, bool) {
@@ -250,7 +296,9 @@ func (h *handlers) readOIDCTxCookie(c *gin.Context) (oidcTx, bool) {
 		return oidcTx{}, false
 	}
 	var tx oidcTx
-	if err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" {
+	// All three fields must be present: an empty nonce would make the callback's
+	// nonce check pass vacuously against an ID token that omitted the claim.
+	if err := json.Unmarshal(raw, &tx); err != nil || tx.State == "" || tx.Verifier == "" || tx.Nonce == "" {
 		return oidcTx{}, false
 	}
 	return tx, true

internal/domain/domain.go

@@ -39,10 +39,15 @@ var (
 	// ErrOIDCNoEmail means the IdP returned no email claim, so no account can be
 	// provisioned (email is the account's unique key). The email scope is required.
 	ErrOIDCNoEmail = errors.New("oidc identity has no email")
-	// ErrOIDCEmailUnverified means the IdP's email is unverified and it collides
+	// ErrOIDCEmailUnverified means the IdP asserted an email it hasn't verified;
-	// with an existing account; auto-linking it would enable account takeover, so
+	// pansy won't provision or link on an unverified email (it would enable
-	// it is refused.
+	// account takeover / squatting).
 	ErrOIDCEmailUnverified = errors.New("oidc email not verified")
+	// ErrOIDCIdentityConflict means the OIDC identity can't be attached: either
+	// the target account already carries a different identity (refusing to
+	// overwrite it prevents lockout/takeover) or the (issuer, subject) pair is
+	// already bound to another account. Mapped to 409.
+	ErrOIDCIdentityConflict = errors.New("oidc identity conflict")
 )
 
 // Enumerated string values mirrored from the schema CHECK constraints.

internal/service/auth.go

@@ -57,19 +57,23 @@ func (s *Service) Providers() Providers {
 // issuing no session itself (the caller does). The IdP has already gated access,
 // so PANSY_REGISTRATION does not apply. Resolution order:
 //  1. an existing user with the same (issuer, subject) — a returning OIDC user;
-//  2. else an existing user with the same *verified* email — linked to this
+//  2. else an existing user with the same email — linked to this identity (so
-//     identity (so one person isn't split across a local and an OIDC account);
+//     one person isn't split across a local and an OIDC account);
 //  3. else a new just-in-time account stamped with the identity.
 //
-// An unverified email that collides with an existing account is refused (it
+// Steps 2 and 3 require a verified email: linking on an unverified email would
-// would let anyone who can assert that email at the IdP take over the account).
+// let anyone who can assert that email at an IdP take over an account, and
-// An identity with no email can't be provisioned (email is the account key).
+// JIT-creating on one would let them squat an email a real user later owns
+// (then the real user's different identity would be refused by LinkOIDC). An
+// identity with no email can't be provisioned at all (email is the account key).
+// Returning users (step 1) skip the email check — their identity is already
+// proven.
 func (s *Service) LoginOIDC(ctx context.Context, id OIDCIdentity) (*domain.User, error) {
 	if id.Issuer == "" || id.Subject == "" {
 		return nil, domain.ErrInvalidInput
 	}
 
-	// 1. Returning OIDC user.
+	// 1. Returning OIDC user (identity already proven; email state irrelevant).
 	u, err := s.store.GetUserByOIDC(ctx, id.Issuer, id.Subject)
 	if err == nil {
 		return u, nil
@@ -78,18 +82,20 @@ func (s *Service) LoginOIDC(ctx context.Context, id OIDCIdentity) (*domain.User,
 		return nil, err
 	}
 
+	// Provisioning (link or create) requires a verified email.
 	email := normalizeEmail(id.Email)
 	if email == "" {
 		return nil, domain.ErrOIDCNoEmail
 	}
-
-	// 2. Link to an existing account by email — but only a verified one.
-	existing, err := s.store.GetUserByEmail(ctx, email)
-	switch {
-	case err == nil:
 	if !id.EmailVerified {
 		return nil, domain.ErrOIDCEmailUnverified
 	}
+
+	// 2. Link to an existing account by email. LinkOIDC refuses to overwrite a
+	// different stored identity (returns ErrOIDCIdentityConflict).
+	existing, err := s.store.GetUserByEmail(ctx, email)
+	switch {
+	case err == nil:
 		return s.store.LinkOIDC(ctx, existing.ID, id.Issuer, id.Subject)
 	case !errors.Is(err, domain.ErrNotFound):
 		return nil, err

internal/service/auth_test.go

@@ -290,7 +290,7 @@ func TestProvidersReflectsConfig(t *testing.T) {
 	// OIDC configured with a base URL → reported ready.
 	ready := openConfig()
 	ready.BaseURL = "https://pansy.example.com"
-	ready.OIDC = config.OIDCConfig{Issuer: "https://idp.example", ClientID: "cid", ButtonLabel: "Sign in with Authentik"}
+	ready.OIDC = config.OIDCConfig{Issuer: testOIDCIssuer, ClientID: "cid", ButtonLabel: "Sign in with Authentik"}
 	if got := newTestService(t, ready).Providers(); !got.OIDC || got.OIDCLabel != "Sign in with Authentik" {
 		t.Errorf("providers = %+v, want oidc=true with Authentik label", got)
 	}
@@ -303,8 +303,11 @@ func TestProvidersReflectsConfig(t *testing.T) {
 	}
 }
 
+// testOIDCIssuer is the issuer URL used across the OIDC service tests.
+const testOIDCIssuer = "https://idp.example"
+
 func oidcIdentity(sub, email, name string, verified bool) OIDCIdentity {
-	return OIDCIdentity{Issuer: "https://idp.example", Subject: sub, Email: email, EmailVerified: verified, Name: name}
+	return OIDCIdentity{Issuer: testOIDCIssuer, Subject: sub, Email: email, EmailVerified: verified, Name: name}
 }
 
 func TestLoginOIDCJITProvisionsThenReturnsSameUser(t *testing.T) {
@@ -351,14 +354,47 @@ func TestLoginOIDCLinksExistingLocalAccount(t *testing.T) {
 	}
 }
 
-func TestLoginOIDCRefusesUnverifiedEmailCollision(t *testing.T) {
+func TestLoginOIDCRefusesUnverifiedEmail(t *testing.T) {
 	s := newTestService(t, openConfig())
-	mustRegister(t, s, "[email protected]", "Carol", "password123")
 
-	_, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3", "[email protected]", "Carol", false))
+	// Unverified email colliding with an existing account is refused (takeover).
-	if !errors.Is(err, domain.ErrOIDCEmailUnverified) {
+	mustRegister(t, s, "[email protected]", "Carol", "password123")
+	if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3", "[email protected]", "Carol", false)); !errors.Is(err, domain.ErrOIDCEmailUnverified) {
 		t.Errorf("unverified collision err = %v, want ErrOIDCEmailUnverified", err)
 	}
+
+	// Unverified email with NO collision is also refused (can't JIT-provision on
+	// an unverified email — it could squat an address a real user later owns, and
+	// could make an unverified identity the first admin).
+	if _, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-3b", "[email protected]", "Fresh", false)); !errors.Is(err, domain.ErrOIDCEmailUnverified) {
+		t.Errorf("unverified JIT err = %v, want ErrOIDCEmailUnverified", err)
+	}
+}
+
+func TestLoginOIDCRefusesOverwritingDifferentIdentity(t *testing.T) {
+	s := newTestService(t, openConfig())
+
+	// A user links identity A.
+	first, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-A", "[email protected]", "Dana", true))
+	if err != nil {
+		t.Fatalf("initial link: %v", err)
+	}
+
+	// A different identity asserting the same verified email must NOT overwrite
+	// the stored identity (that would hijack/lock out the account).
+	_, err = s.LoginOIDC(context.Background(), oidcIdentity("sub-B", "[email protected]", "Dana", true))
+	if !errors.Is(err, domain.ErrOIDCIdentityConflict) {
+		t.Fatalf("overwrite attempt err = %v, want ErrOIDCIdentityConflict", err)
+	}
+
+	// The original identity still works and still points at the same account.
+	again, err := s.LoginOIDC(context.Background(), oidcIdentity("sub-A", "[email protected]", "Dana", true))
+	if err != nil || again.ID != first.ID {
+		t.Errorf("original identity broken: user=%v err=%v", again, err)
+	}
+	if again.OIDCSubject == nil || *again.OIDCSubject != "sub-A" {
+		t.Errorf("stored identity was overwritten: %v", again.OIDCSubject)
+	}
 }
 
 func TestLoginOIDCRequiresEmail(t *testing.T) {

internal/store/users.go

@@ -120,26 +120,37 @@ func (d *DB) GetUserByOIDC(ctx context.Context, issuer, subject string) (*domain
 }
 
 // LinkOIDC stamps an OIDC identity onto an existing user (first OIDC login for a
-// pre-existing local account) and returns the updated row. A collision with
+// pre-existing local account) and returns the updated row.
-// another user's identity pair trips the UNIQUE index and maps to ErrEmailTaken
+//
-// as a generic conflict (it shouldn't happen — the caller looks up by identity
+// The UPDATE only matches when the account carries no identity yet, or already
-// first — but the index is the backstop).
+// carries this exact one (idempotent) — it will NOT overwrite a different stored
+// identity, which would let anyone asserting the same email at a second IdP
+// hijack or lock out the account. A no-match (different identity, or the row is
+// gone) and a UNIQUE (issuer, subject) collision with another account both
+// surface as domain.ErrOIDCIdentityConflict. RETURNING folds the read-back into
+// the same statement.
 func (d *DB) LinkOIDC(ctx context.Context, userID int64, issuer, subject string) (*domain.User, error) {
-	_, err := d.sql.ExecContext(ctx,
+	u, err := scanUser(d.sql.QueryRowContext(ctx,
 		`UPDATE users
 		    SET oidc_issuer = ?, oidc_subject = ?,
 		        version = version + 1,
 		        updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
-		  WHERE id = ?`,
+		  WHERE id = ?
-		issuer, subject, userID,
+		    AND (oidc_subject IS NULL OR (oidc_issuer = ? AND oidc_subject = ?))
-	)
+		  RETURNING `+userColumns,
+		issuer, subject, userID, issuer, subject,
+	))
+	if errors.Is(err, sql.ErrNoRows) {
+		// The account already has a different identity (or no longer exists).
+		return nil, domain.ErrOIDCIdentityConflict
+	}
 	if err != nil {
 		if isUniqueViolation(err) {
-			return nil, domain.ErrEmailTaken
+			return nil, domain.ErrOIDCIdentityConflict
 		}
 		return nil, fmt.Errorf("store: link oidc: %w", err)
 	}
-	return d.GetUserByID(ctx, userID)
+	return u, nil
 }
 
 // CountUsers returns the number of user rows. Used to decide first-user-is-admin