From 0deff239856a49500aa32ec33564f50c32e97fcb Mon Sep 17 00:00:00 2001 From: RXWatcher Date: Wed, 8 Jul 2026 17:20:27 +0200 Subject: [PATCH] fix(profiles): enforce per-account profile name uniqueness (#342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(profiles): enforce per-account profile name uniqueness Profile create and rename accepted any name, so one account could hold unlimited profiles all called "Laura" (every client allowed it too). Reject a create or rename whose trimmed, case-insensitive name matches another profile on the same account with 409 name_conflict. Scoping is per account by construction — the check runs against a single user's profile store, so different accounts can still each have a "Laura". Renames exclude the profile being updated, so re-saving a profile under its own name (e.g. avatar-only edits that resubmit the name) still works. Also reject whitespace-only names on create and rename; a name of " " previously passed the blank check. Additive-only per the v1 API rules: new 409 error code on existing endpoints, following the profile_limit_reached pattern. Co-Authored-By: Claude Fable 5 * fix(profiles): store the trimmed profile name The conflict check compared trimmed names but create/rename persisted the raw input, so " Laura " could land with stray whitespace and render inconsistently. Normalize to the trimmed form before storage on both paths. Addresses the CodeRabbit review finding on PR #342. Co-Authored-By: Claude Fable 5 * test(profiles): cover whitespace-only rejection and rename trimming Also document the check-then-write race in profileNameConflicts: the userstore backends carry no unique index on name, so concurrent creates can still race past the guard, same as profile_limit_reached. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: rxwatcher Co-authored-by: Claude Fable 5 Co-authored-by: Quick <31828688+Quick104@users.noreply.github.com> --- internal/api/handlers/profiles.go | 70 +++++++- internal/api/handlers/profiles_test.go | 214 +++++++++++++++++++++++++ 2 files changed, 281 insertions(+), 3 deletions(-) diff --git a/internal/api/handlers/profiles.go b/internal/api/handlers/profiles.go index 0b120cc9..a5e80e90 100644 --- a/internal/api/handlers/profiles.go +++ b/internal/api/handlers/profiles.go @@ -7,6 +7,7 @@ import ( "fmt" "log/slog" "net/http" + "strings" "time" "github.com/go-chi/chi/v5" @@ -213,6 +214,32 @@ func writeProfileManagementPermissionError(w http.ResponseWriter, err error) { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to check profile permissions") } +// profileNameConflicts reports whether a profile other than excludeID already +// uses name within this account's store, comparing the trimmed forms +// case-insensitively so "Laura" and " laura " count as the same household +// member. Scoping is per account by construction: callers pass the profile +// list of a single user's store, so another account's profiles can never +// conflict. +// +// This is a check-then-write guard with no store-level uniqueness constraint +// (the userstore's dual Postgres/SQLite backends carry no unique index on +// name), so two concurrent requests can both pass and insert duplicates — +// the same window the profile_limit_reached check accepts. Good enough for +// interactive profile management; a functional unique index is the fix if +// that ever stops being true. +func profileNameConflicts(profiles []userstore.Profile, name, excludeID string) bool { + trimmed := strings.TrimSpace(name) + for _, p := range profiles { + if p.ID == excludeID { + continue + } + if strings.EqualFold(strings.TrimSpace(p.Name), trimmed) { + return true + } + } + return false +} + // isAllowedSelfServiceProfileUpdate reports whether a non-admin update request // only touches fields the user is allowed to change on their own profiles. // Admin-only fields (access policy: library restrictions, content rating, @@ -272,7 +299,7 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ return } - if req.Name == "" { + if strings.TrimSpace(req.Name) == "" { writeError(w, http.StatusBadRequest, "bad_request", "Profile name is required") return } @@ -346,6 +373,16 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ } } + if profileNameConflicts(existingProfiles, req.Name, "") { + writeError( + w, + http.StatusConflict, + "name_conflict", + "A profile with this name already exists", + ) + return + } + showForcedSubtitles := true if req.ShowForcedSubtitles != nil { showForcedSubtitles = *req.ShowForcedSubtitles @@ -353,8 +390,10 @@ func (h *ProfileHandler) HandleCreateProfile(w http.ResponseWriter, r *http.Requ profileID := uuid.New().String() profile := userstore.Profile{ - ID: profileID, - Name: req.Name, + ID: profileID, + // Store the trimmed form the conflict check compared, so " Laura " + // doesn't persist with stray whitespace. + Name: strings.TrimSpace(req.Name), Avatar: avatarRef, IsChild: req.IsChild, MaxContentRating: req.MaxContentRating, @@ -502,6 +541,31 @@ func (h *ProfileHandler) HandleUpdateProfile(w http.ResponseWriter, r *http.Requ } } + if req.Name != nil { + // Normalize to the trimmed form up front: the conflict check compares + // it and the store persists it, so " Laura " never lands verbatim. + trimmedName := strings.TrimSpace(*req.Name) + if trimmedName == "" { + writeError(w, http.StatusBadRequest, "bad_request", "Profile name is required") + return + } + req.Name = &trimmedName + existingProfiles, err := store.ListProfiles(r.Context()) + if err != nil { + writeError(w, http.StatusInternalServerError, "internal_error", "Failed to list profiles") + return + } + if profileNameConflicts(existingProfiles, *req.Name, profileID) { + writeError( + w, + http.StatusConflict, + "name_conflict", + "A profile with this name already exists", + ) + return + } + } + input := userstore.UpdateProfileInput{ Name: req.Name, Avatar: avatarRef, diff --git a/internal/api/handlers/profiles_test.go b/internal/api/handlers/profiles_test.go index 0ca51c15..0278aeb6 100644 --- a/internal/api/handlers/profiles_test.go +++ b/internal/api/handlers/profiles_test.go @@ -165,6 +165,103 @@ func TestHandleCreateProfile_AllowsNonAdminUpToCap(t *testing.T) { } } +func TestHandleCreateProfile_RejectsDuplicateName(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + // The seeded store already holds "Main"; a trimmed, case-insensitive + // match must be rejected within the same account. + req := newAuthorizedProfileRequestWithRole( + http.MethodPost, + "/profiles", + `{"name":" main "}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleCreateProfile(rr, req) + + if rr.Code != http.StatusConflict { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + var resp errorResponse + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.Error != "name_conflict" { + t.Fatalf("error = %q, want %q", resp.Error, "name_conflict") + } + + profiles, err := store.ListProfiles(context.Background()) + if err != nil { + t.Fatalf("list profiles: %v", err) + } + if len(profiles) != 1 { + t.Fatalf("profile count = %d, want 1", len(profiles)) + } +} + +func TestHandleCreateProfile_RejectsWhitespaceOnlyName(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + req := newAuthorizedProfileRequestWithRole( + http.MethodPost, + "/profiles", + `{"name":" "}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleCreateProfile(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + profiles, err := store.ListProfiles(context.Background()) + if err != nil { + t.Fatalf("list profiles: %v", err) + } + if len(profiles) != 1 { + t.Fatalf("profile count = %d, want 1", len(profiles)) + } +} + +func TestHandleCreateProfile_TrimsStoredName(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + req := newAuthorizedProfileRequestWithRole( + http.MethodPost, + "/profiles", + `{"name":" Laura "}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleCreateProfile(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + profiles, err := store.ListProfiles(context.Background()) + if err != nil { + t.Fatalf("list profiles: %v", err) + } + for _, p := range profiles { + if p.Name == "Laura" { + return + } + } + t.Fatalf("no profile stored with trimmed name, profiles: %+v", profiles) +} + func TestHandleCreateProfile_BlocksNonPrimaryNonAdmin(t *testing.T) { store := newProfileTestStore(t) if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil { @@ -437,6 +534,123 @@ func TestHandleUpdateProfile_AllowsNonAdminToUpdateAnyOwnedProfile(t *testing.T) } } +func TestHandleUpdateProfile_RejectsDuplicateNameOnRename(t *testing.T) { + store := newProfileTestStore(t) + if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil { + t.Fatalf("create profile: %v", err) + } + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + req := newAuthorizedProfileRequestWithRole( + http.MethodPut, + "/profiles/profile-2", + `{"name":"MAIN"}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-2")) + + if rr.Code != http.StatusConflict { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + var resp errorResponse + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if resp.Error != "name_conflict" { + t.Fatalf("error = %q, want %q", resp.Error, "name_conflict") + } + + profile, err := store.GetProfile(context.Background(), "profile-2") + if err != nil { + t.Fatalf("get profile: %v", err) + } + if profile == nil || profile.Name != "Kids" { + t.Fatalf("profile name changed despite conflict: %+v", profile) + } +} + +func TestHandleUpdateProfile_AllowsKeepingOwnNameOnUpdate(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + // Saving a profile without renaming resubmits its own name; the + // conflict check must exclude the profile being updated. + req := newAuthorizedProfileRequestWithRole( + http.MethodPut, + "/profiles/profile-1", + `{"name":"Main","subtitle_mode":"always"}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1")) + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } +} + +func TestHandleUpdateProfile_RejectsWhitespaceOnlyName(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + req := newAuthorizedProfileRequestWithRole( + http.MethodPut, + "/profiles/profile-1", + `{"name":" "}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1")) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + profile, err := store.GetProfile(context.Background(), "profile-1") + if err != nil { + t.Fatalf("get profile: %v", err) + } + if profile == nil || profile.Name != "Main" { + t.Fatalf("profile name changed despite rejection: %+v", profile) + } +} + +func TestHandleUpdateProfile_TrimsStoredNameOnRename(t *testing.T) { + store := newProfileTestStore(t) + handler := NewProfileHandler(testUserStoreProvider{store: store}) + + req := newAuthorizedProfileRequestWithRole( + http.MethodPut, + "/profiles/profile-1", + `{"name":" Laura "}`, + "user", + "profile-1", + ) + rr := httptest.NewRecorder() + + handler.HandleUpdateProfile(rr, withProfileRouteParam(req, "id", "profile-1")) + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + + profile, err := store.GetProfile(context.Background(), "profile-1") + if err != nil { + t.Fatalf("get profile: %v", err) + } + if profile == nil || profile.Name != "Laura" { + t.Fatalf("stored name not trimmed: %+v", profile) + } +} + func TestHandleDeleteProfile_AllowsPrimaryToDeleteOther(t *testing.T) { store := newProfileTestStore(t) if err := store.CreateProfile(context.Background(), userstore.Profile{ID: "profile-2", Name: "Kids"}); err != nil {