From 8ffe51c508d014bc737ed1a2e02dd96ffe10062e Mon Sep 17 00:00:00 2001
From: Julian Dice <19397727+windoze95@users.noreply.github.com>
Date: Sun, 12 Jul 2026 22:08:15 -0500
Subject: [PATCH] =?UTF-8?q?feat:=20harden=20PUT=20/users/me=20=E2=80=94=20?=
 =?UTF-8?q?validate=20display=20name,=20re-verify=20on=20email=20change?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two problems in the profile-update path, found while looking at first_name.

The real one: changing your email did not un-verify the account. Since
email_verified_at now gates the AI-cost endpoints and drives the stale-signup
sweep, a user could verify a throwaway address, swap in an address nobody ever
proved, and keep "verified" status. The address change now clears verification
and mails a fresh code to the NEW address; when verification isn't live the new
address is auto-verified instead, mirroring what signup does in that mode. Email
and verification state move in one statement so an account can never be left
verified on an unproved address.

Also on that path:
- first_name was unvalidated free text on an unbounded text column, at both
  signup and update. It now allows any Unicode letter plus the punctuation real
  names use (Mary-Jane, O'Brien, J. R., José, 李) and rejects digits, emoji,
  symbols and control characters, capped at 50. Deliberately NOT
  profanity-checked: the name is only ever shown back to its owner, so a filter
  would buy no moderation and would reject people named Dick or Fanny.
- A taken email returned 500. It now returns 409, using the sentinel the repo
  was already mapping.
- Email length is capped at 254 (RFC 5321); the column is unbounded text.
- Re-typing your own address in different case no longer counts as a change, so
  it can't cost you your verified status.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194PdH4wDTnz5SWfzyoKagc
---
 internal/handlers/user.go               |  47 +++++-
 internal/repository/interfaces.go       |   2 +-
 internal/repository/user.go             |  13 +-
 internal/router/router.go               |   2 +
 internal/service/profile_update_test.go | 197 ++++++++++++++++++++++++
 internal/service/user.go                | 109 ++++++++++++-
 internal/testutil/mocks.go              |   3 +-
 7 files changed, 363 insertions(+), 10 deletions(-)
 create mode 100644 internal/service/profile_update_test.go

diff --git a/internal/handlers/user.go b/internal/handlers/user.go
index cc44b6f..0ecb5bf 100755
--- a/internal/handlers/user.go
+++ b/internal/handlers/user.go
@@ -59,6 +59,12 @@ func (h *UserHandler) CreateUser(c *gin.Context) {
 		return
 	}
 
+	// Validate the optional display name
+	if err := h.Service.ValidateFirstName(newUser.FirstName); err != nil {
+		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+		return
+	}
+
 	// Validate email
 	if err := h.Service.ValidateEmail(newUser.Email); err != nil {
 		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
@@ -354,13 +360,50 @@ func (h *UserHandler) UpdateUser(c *gin.Context) {
 		return
 	}
 
+	req.FirstName = strings.TrimSpace(req.FirstName)
+	req.Email = strings.TrimSpace(req.Email)
+
+	if err := h.Service.ValidateFirstName(req.FirstName); err != nil {
+		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+		return
+	}
+
+	// Note the address change before the update rewrites the in-memory user.
+	emailChanged := req.Email != "" && !strings.EqualFold(req.Email, user.Email)
+	if emailChanged {
+		if err := h.Service.ValidateEmail(req.Email); err != nil {
+			c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+			return
+		}
+	}
+
 	if err := h.Service.UpdateUser(user, req.FirstName, req.Email); err != nil {
+		if errors.Is(err, repository.ErrEmailTaken) {
+			c.JSON(http.StatusConflict, gin.H{"error": "email already in use"})
+			return
+		}
 		logger.Get().Error("failed to update user", zap.Uint("user_id", user.ID), zap.Error(err))
-		c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+		c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to update user"})
 		return
 	}
 
-	c.JSON(http.StatusOK, gin.H{"message": "User updated successfully"})
+	// The address changed, so UpdateUser un-verified the account. Send a code to
+	// the new address — best-effort, exactly like signup: a mail hiccup must not
+	// fail the update, and the verify screen offers a resend.
+	verificationRequired := false
+	if emailChanged && h.EmailVerification != nil && h.EmailVerification.Enabled() {
+		verificationRequired = true
+		if err := h.EmailVerification.StartVerification(c.Request.Context(), user); err != nil {
+			logger.Get().Warn("failed to send verification email after address change",
+				zap.Uint("user_id", user.ID), zap.Error(err))
+		}
+	}
+
+	c.JSON(http.StatusOK, gin.H{
+		"message": "User updated successfully",
+		// Tells a client it has to collect a code before AI features work again.
+		"email_verification_required": verificationRequired,
+	})
 }
 
 // UpdateSettings updates a user's settings.
diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go
index 52f72e2..65a939c 100644
--- a/internal/repository/interfaces.go
+++ b/internal/repository/interfaces.go
@@ -137,7 +137,7 @@ type UserRepo interface {
 	GetUserAuthByUsername(username string) (*models.User, error)
 	GetUserAuthByEmail(email string) (*models.User, error)
 	UpdateUserFirstName(userID uint, firstName string) error
-	UpdateUserEmail(userID uint, email string) error
+	UpdateUserEmail(userID uint, email string, verifiedAt *time.Time) error
 	UpdateUserSettingsKeepScreenAwake(userID uint, keepScreenAwake bool) error
 	UpdatePersonalization(userID uint, update *models.PersonalizationUpdate) error
 	UsernameExists(username string) (bool, error)
diff --git a/internal/repository/user.go b/internal/repository/user.go
index 9f46bde..42ab1f9 100755
--- a/internal/repository/user.go
+++ b/internal/repository/user.go
@@ -124,11 +124,18 @@ func (r *UserRepository) UpdateUserFirstName(userID uint, firstName string) erro
 	return err
 }
 
-// UpdateUserEmail updates a user's email address.
-func (r *UserRepository) UpdateUserEmail(userID uint, email string) error {
+// UpdateUserEmail updates a user's email address and, in the same statement,
+// its verification state: a nil verifiedAt marks the new address unverified.
+// Both columns move together so an account can never be left looking verified
+// on an address nobody proved. A map is used rather than a struct because GORM
+// skips zero values on struct updates, and nil is the value that matters here.
+func (r *UserRepository) UpdateUserEmail(userID uint, email string, verifiedAt *time.Time) error {
 	err := r.DB.Model(&models.User{}).
 		Where("id = ?", userID).
-		Update("Email", email).Error
+		Updates(map[string]interface{}{
+			"email":             email,
+			"email_verified_at": verifiedAt,
+		}).Error
 	if err != nil {
 		logger.Get().Error("failed to update user email", zap.Uint("user_id", userID), zap.Error(err))
 		return mapUserUniqueViolation(err)
diff --git a/internal/router/router.go b/internal/router/router.go
index af5a144..ef32300 100755
--- a/internal/router/router.go
+++ b/internal/router/router.go
@@ -133,6 +133,8 @@ func SetupRouter(cfg *config.Config, database *gorm.DB) *gin.Engine {
 	emailVerificationService := service.NewEmailVerificationService(cfg, userRepo, emailVerificationRepo, emailSender)
 	emailVerificationHandler := handlers.NewEmailVerificationHandler(emailVerificationService)
 	userHandler.EmailVerification = emailVerificationService
+	// Lets UpdateUser decide whether a changed address starts out unverified.
+	userService.Verification = emailVerificationService
 
 	// Gate for AI-cost endpoints: throwaway signups must verify their email
 	// before they can spend AI quota. No-op while verification is disabled.
diff --git a/internal/service/profile_update_test.go b/internal/service/profile_update_test.go
new file mode 100644
index 0000000..a21d3a2
--- /dev/null
+++ b/internal/service/profile_update_test.go
@@ -0,0 +1,197 @@
+package service
+
+import (
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/windoze95/saltybytes-api/internal/models"
+	"github.com/windoze95/saltybytes-api/internal/repository"
+	"github.com/windoze95/saltybytes-api/internal/testutil"
+)
+
+// stubVerification stands in for EmailVerificationService.
+type stubVerification struct{ enabled bool }
+
+func (s stubVerification) Enabled() bool { return s.enabled }
+
+// newProfileHarness builds a user service with a verified account already in it.
+// The returned user is the object the repo holds — tests should pass a
+// sessionCopy of it to UpdateUser, not this pointer.
+func newProfileHarness(t *testing.T, verificationLive bool) (*UserService, *testutil.MockUserRepo, *models.User) {
+	t.Helper()
+
+	repo := testutil.NewMockUserRepo()
+	svc := &UserService{
+		Cfg:          verificationEnabledConfig(),
+		Repo:         repo,
+		Verification: stubVerification{enabled: verificationLive},
+	}
+
+	verifiedAt := time.Now()
+	user := &models.User{
+		Username:        "cook",
+		Email:           "cook@example.com",
+		EmailVerifiedAt: &verifiedAt,
+		Auth:            &models.UserAuth{AuthType: models.Standard},
+	}
+	repo.CreateUser(user)
+	return svc, repo, user
+}
+
+// sessionCopy mimics what the handler actually passes in: a user loaded from the
+// database, which is a *different* object from the row the repo will write.
+//
+// This matters. MockUserRepo hands back the very pointer it stores, so calling
+// UpdateUser with the harness's user would make every "did it persist?"
+// assertion vacuous — they'd pass on the service's in-memory mutation alone,
+// even if it never touched the repo.
+func sessionCopy(stored *models.User) *models.User {
+	c := *stored
+	return &c
+}
+
+func TestUpdateUser_EmailChangeUnverifiesAccount(t *testing.T) {
+	svc, repo, stored := newProfileHarness(t, true)
+	user := sessionCopy(stored)
+
+	if err := svc.UpdateUser(user, "", "new@example.com"); err != nil {
+		t.Fatalf("UpdateUser error: %v", err)
+	}
+
+	// The in-memory user has to move too: the handler mails user.Email next, and
+	// StartVerification refuses to send to an account that still looks verified.
+	if user.Email != "new@example.com" {
+		t.Errorf("in-memory email = %q, want new@example.com", user.Email)
+	}
+	if user.EmailVerified() {
+		t.Error("in-memory user still looks verified after an address change")
+	}
+
+	// And it has to be persisted, not just mutated in memory.
+	written, _ := repo.GetUserByID(stored.ID)
+	if written.Email != "new@example.com" {
+		t.Errorf("stored email = %q, want new@example.com", written.Email)
+	}
+	if written.EmailVerified() {
+		t.Error("stored account still verified on an address nobody proved")
+	}
+}
+
+func TestUpdateUser_EmailChangeAutoVerifiesWhileVerificationOff(t *testing.T) {
+	svc, repo, stored := newProfileHarness(t, false)
+
+	if err := svc.UpdateUser(sessionCopy(stored), "", "new@example.com"); err != nil {
+		t.Fatalf("UpdateUser error: %v", err)
+	}
+
+	written, _ := repo.GetUserByID(stored.ID)
+	if !written.EmailVerified() {
+		t.Error("with verification off, a changed address must be auto-verified — the gate is a no-op and nothing would ever send a code")
+	}
+}
+
+func TestUpdateUser_SameAddressDifferentCaseKeepsVerification(t *testing.T) {
+	svc, repo, stored := newProfileHarness(t, true)
+
+	if err := svc.UpdateUser(sessionCopy(stored), "", "Cook@Example.com"); err != nil {
+		t.Fatalf("UpdateUser error: %v", err)
+	}
+
+	written, _ := repo.GetUserByID(stored.ID)
+	if !written.EmailVerified() {
+		t.Error("retyping the same address in different case must not cost the user their verified status")
+	}
+}
+
+func TestUpdateUser_TakenEmailIsRejected(t *testing.T) {
+	svc, repo, stored := newProfileHarness(t, true)
+
+	otherVerified := time.Now()
+	repo.CreateUser(&models.User{
+		Username:        "taken",
+		Email:           "taken@example.com",
+		EmailVerifiedAt: &otherVerified,
+		Auth:            &models.UserAuth{AuthType: models.Standard},
+	})
+
+	err := svc.UpdateUser(sessionCopy(stored), "", "taken@example.com")
+	if !errors.Is(err, repository.ErrEmailTaken) {
+		t.Errorf("UpdateUser with a taken email = %v, want ErrEmailTaken", err)
+	}
+
+	written, _ := repo.GetUserByID(stored.ID)
+	if written.Email != "cook@example.com" || !written.EmailVerified() {
+		t.Error("a rejected email change must leave the account untouched")
+	}
+}
+
+func TestUpdateUser_FirstNameOnlyKeepsVerification(t *testing.T) {
+	svc, repo, stored := newProfileHarness(t, true)
+
+	if err := svc.UpdateUser(sessionCopy(stored), "Julia", ""); err != nil {
+		t.Fatalf("UpdateUser error: %v", err)
+	}
+
+	written, _ := repo.GetUserByID(stored.ID)
+	if written.FirstName != "Julia" {
+		t.Errorf("stored first name = %q, want Julia", written.FirstName)
+	}
+	if !written.EmailVerified() {
+		t.Error("changing only the display name must not touch verification")
+	}
+}
+
+func TestValidateFirstName(t *testing.T) {
+	svc := newTestUserService(testutil.NewMockUserRepo())
+
+	tests := []struct {
+		name      string
+		firstName string
+		wantErr   bool
+	}{
+		{"empty is allowed (optional)", "", false},
+		{"plain", "Julian", false},
+		{"accented", "José", false},
+		{"umlaut", "Zoë", false},
+		{"non-latin script", "李", false},
+		{"hyphenated", "Mary-Jane", false},
+		{"apostrophe", "O'Brien", false},
+		{"curly apostrophe", "O’Brien", false},
+		{"initials", "J. R.", false},
+		{"two words", "Ann Marie", false},
+		{"at the length limit", strings.Repeat("a", maxFirstNameLength), false},
+
+		{"over the length limit", strings.Repeat("a", maxFirstNameLength+1), true},
+		{"absurdly long", strings.Repeat("a", 5000), true},
+		{"digits", "Chef2000", true},
+		{"emoji", "Julian🍳", true},
+		{"newline", "Julian\nAdmin", true},
+		{"tab", "Julian\tAdmin", true},
+		{"control character", "Julian\x00", true},
+		{"punctuation only", "---", true},
+		{"symbols", "<script>", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := svc.ValidateFirstName(tt.firstName)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ValidateFirstName(%q) error = %v, wantErr %v", tt.firstName, err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestValidateEmail_LengthCap(t *testing.T) {
+	svc := newTestUserService(testutil.NewMockUserRepo())
+
+	long := strings.Repeat("a", 250) + "@example.com" // valid shape, 262 chars
+	if err := svc.ValidateEmail(long); err == nil {
+		t.Error("ValidateEmail: an over-long address should fail")
+	}
+	if err := svc.ValidateEmail("fine@example.com"); err != nil {
+		t.Errorf("ValidateEmail: a normal address should pass, got %v", err)
+	}
+}
diff --git a/internal/service/user.go b/internal/service/user.go
index 5aef1d4..d7b3258 100755
--- a/internal/service/user.go
+++ b/internal/service/user.go
@@ -7,6 +7,8 @@ import (
 	"strconv"
 	"strings"
 	"time"
+	"unicode"
+	"unicode/utf8"
 
 	"github.com/asaskevich/govalidator"
 	"github.com/windoze95/saltybytes-api/internal/config"
@@ -18,12 +20,36 @@ import (
 	"gorm.io/gorm"
 )
 
+// EmailVerificationStatus is the sliver of EmailVerificationService that
+// UserService needs: whether verification is actually live (switched on in
+// config AND backed by a working sender). A nil implementation means not live.
+type EmailVerificationStatus interface {
+	Enabled() bool
+}
+
 // UserService is the business logic layer for user-related operations.
 type UserService struct {
 	Cfg  *config.Config
 	Repo repository.UserRepo
+	// Verification is assigned by the router once the verification service
+	// exists. Nil (in tests, or before wiring) reads as "not live".
+	Verification EmailVerificationStatus
+}
+
+// verificationLive reports whether an address has to be proved before it counts
+// as verified.
+func (s *UserService) verificationLive() bool {
+	return s.Verification != nil && s.Verification.Enabled()
 }
 
+// Profile field bounds. Both columns are unbounded text, so these are the only
+// thing between a client and a 5,000-character display name. Username bounds
+// live in username_policy.go.
+const (
+	maxFirstNameLength = 50
+	maxEmailLength     = 254 // RFC 5321 caps the whole path at 254
+)
+
 // UserResponse is the response object for user-related operations.
 type UserResponse struct {
 	ID              string                  `json:"id"`
@@ -282,20 +308,54 @@ func (s *UserService) UpdatePersonalization(user *models.User, update *models.Pe
 	return s.Repo.UpdatePersonalization(user.ID, update)
 }
 
-// UpdateUser updates a user's profile fields (first name, email).
+// UpdateUser updates a user's profile fields (first name, email). Callers are
+// expected to have validated both already.
+//
+// Changing the address un-verifies the account. EmailVerifiedAt is what gates
+// the AI-cost endpoints and what the stale-signup sweep keys off, so it has to
+// mean "we verified THIS address" — otherwise anyone could verify a throwaway
+// and then swap in an address nobody ever proved. The caller sends the fresh
+// code; when verification isn't live the new address is marked verified
+// immediately, mirroring what signup does in that mode.
+//
+// The in-memory user is updated to match, so the caller mails the new address
+// and not the old one.
 func (s *UserService) UpdateUser(user *models.User, firstName, email string) error {
-	if email != "" && email != user.Email {
+	// EqualFold: retyping the same address with different capitalisation is not
+	// a change, and must not cost the user their verified status.
+	if email != "" && !strings.EqualFold(email, user.Email) {
 		if err := s.ValidateEmail(email); err != nil {
 			return err
 		}
-		if err := s.Repo.UpdateUserEmail(user.ID, email); err != nil {
+
+		// Same question signup asks — is this address claimable? — including
+		// the release of one squatted by a stale unverified signup, which also
+		// keeps the unique constraint from firing underneath us.
+		taken, err := s.EmailTakenForSignup(email)
+		if err != nil {
+			return err
+		}
+		if taken {
+			return repository.ErrEmailTaken
+		}
+
+		var verifiedAt *time.Time
+		if !s.verificationLive() {
+			now := time.Now()
+			verifiedAt = &now
+		}
+		if err := s.Repo.UpdateUserEmail(user.ID, email, verifiedAt); err != nil {
 			return err
 		}
+		user.Email = email
+		user.EmailVerifiedAt = verifiedAt
 	}
+
 	if firstName != "" {
 		if err := s.Repo.UpdateUserFirstName(user.ID, firstName); err != nil {
 			return err
 		}
+		user.FirstName = firstName
 	}
 	return nil
 }
@@ -332,6 +392,49 @@ func (s *UserService) ValidateEmail(email string) error {
 	if !govalidator.IsEmail(email) {
 		return fmt.Errorf("invalid email format")
 	}
+	// RFC 5321 caps a path at 254 characters. govalidator's pattern doesn't,
+	// and the column is unbounded text.
+	if utf8.RuneCountInString(email) > maxEmailLength {
+		return fmt.Errorf("email must be %d characters or less", maxEmailLength)
+	}
+	return nil
+}
+
+// ValidateFirstName validates the optional display name.
+//
+// Names are a minefield, so the rule is "any letter, plus the punctuation that
+// shows up in real names": Mary-Jane, O'Brien, J. R., José, Zoë, 李 all pass.
+// Everything else is rejected — digits, emoji, symbols, and control characters,
+// which is what makes a newline-stuffed or 5,000-character name impossible.
+//
+// Deliberately NOT profanity-checked. The name is private: the app only ever
+// shows it back to its owner (the settings screen prefers it over the username),
+// it is not published and never reaches an AI prompt. A filter would buy no
+// moderation here and would reject people genuinely named Dick or Fanny. If
+// first names ever become visible to other users — a family roster, an author
+// byline on a shared recipe — revisit this.
+func (s *UserService) ValidateFirstName(firstName string) error {
+	if firstName == "" {
+		return nil // optional
+	}
+	if utf8.RuneCountInString(firstName) > maxFirstNameLength {
+		return fmt.Errorf("first name must be %d characters or less", maxFirstNameLength)
+	}
+
+	hasLetter := false
+	for _, r := range firstName {
+		switch {
+		case unicode.IsLetter(r):
+			hasLetter = true
+		case unicode.IsMark(r): // combining accents
+		case r == ' ' || r == '-' || r == '\'' || r == '’' || r == '.':
+		default:
+			return fmt.Errorf("first name can only contain letters, spaces, hyphens, apostrophes and periods")
+		}
+	}
+	if !hasLetter {
+		return fmt.Errorf("first name must contain at least one letter")
+	}
 	return nil
 }
 
diff --git a/internal/testutil/mocks.go b/internal/testutil/mocks.go
index bc5a7d2..7bd5526 100644
--- a/internal/testutil/mocks.go
+++ b/internal/testutil/mocks.go
@@ -713,12 +713,13 @@ func (m *MockUserRepo) UpdateUserFirstName(userID uint, firstName string) error
 	return nil
 }
 
-func (m *MockUserRepo) UpdateUserEmail(userID uint, email string) error {
+func (m *MockUserRepo) UpdateUserEmail(userID uint, email string, verifiedAt *time.Time) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
 	if u, ok := m.Users[userID]; ok {
 		u.Email = email
+		u.EmailVerifiedAt = verifiedAt
 	}
 	return nil
 }