From 37908d3cead9d982a1075b5f3fdd3314d32a87e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 4 Jun 2026 19:43:13 -0400 Subject: [PATCH] fix: enforce ownership atomically on player and share updates (#5563) * fix(player): enforce ownership atomically on player update The native API PUT /api/player/{id} authorized writes using the userId in the request body via isPermitted, while the actual write targeted the row by the URL id. A non-admin user could set userId to their own id in the body to pass the check, then overwrite and reassign ownership of another user's player row identified by the URL id (cross-tenant takeover). Add updateOwned on the base repository: an atomic, ownership-restricted UPDATE that folds the owner predicate (user_id = caller) into the WHERE clause for non-admins, so a row owned by another user simply does not match and no write happens. It also never writes user_id, so ownership is immutable on update and no caller (admin included) can reassign a player to a different owner. Unlike put, it never falls through to an INSERT, so a non-matching id returns ErrNotFound instead of creating a row. playerRepository.Update now uses updateOwned. Extract filterUpdateValues, shared by put and updateOwned, so the update-column filtering lives in one place. The create path (Save) keeps the body-based isPermitted check, which is correct for new records. Add regression tests covering the spoofed-userId hijack, regular-user and admin ownership reassignment, legitimate owner updates, and the nonexistent-player case. * fix(share): enforce ownership atomically on share update shareRepository.Update authorized writes with a separate checkOwnership SELECT, then wrote the row via put(). The check and the write were two statements (a TOCTOU window), put() could fall through to an INSERT on a missing id, and put() would write user_id if present in the update columns, so ownership was mutable on update. Switch Update to updateOwned, which folds the owner predicate into the UPDATE's WHERE clause, never writes user_id, and never inserts. This makes the write atomic and ownership immutable, and drops the extra ownership SELECT on the happy path. To preserve the previous 403/404 distinction, updateOwned now classifies a non-matching id: it runs a follow-up existence check only on the failure path (count == 0, where no write happened, so no TOCTOU) and returns ErrPermissionDenied when the row exists but is owned by another user, ErrNotFound when the id is missing. The player path inherits this: its tests now expect ErrPermissionDenied for a non-owner targeting an existing row, and ErrNotFound only for a genuinely missing id. Add share regression tests for the nonexistent-id and ownership- reassignment cases. checkOwnership remains in use by Delete. * refactor(persistence): extract canonical ownerFilter predicate The non-admin owner-restriction predicate (user_id = me, exempting admins and headless contexts) was spelled out independently in updateOwned and in playerRepository.addRestriction. The two copies had drifted: addRestriction did not exempt the headless/invalid user, so a headless context restricted to user_id = "-1" (matching nothing) while updateOwned exempted it. Extract sqlRepository.ownerFilter as the single definition and route both call sites through it. addRestriction now exempts the headless user too; that path is only reachable from the authenticated native API, so there is no production behavior change, but the latent divergence is removed. playlistRepository.userFilter is intentionally left alone: it encodes a different policy (public OR owner_id = me, on the owner_id column). * fix(share): preserve all-columns update path in Update shareRepository.Update unconditionally appended "updated_at" to cols. filterUpdateValues treats an empty cols as "update every column", so when a caller passes no columns, appending "updated_at" turned an all-columns update into an updated_at-only one, silently dropping every other field. The REST controller always populates cols from the request-body field names, so this path is not reachable through the native API and the behavior was latent (and pre-existing). Guard the append so the all-columns path is preserved, and add a regression test that updates with no columns and asserts the other fields persist. Signed-off-by: Deluan --------- Signed-off-by: Deluan --- persistence/player_repository.go | 20 +++--- persistence/player_repository_test.go | 87 +++++++++++++++++++++++- persistence/share_repository.go | 15 ++--- persistence/share_repository_test.go | 38 +++++++++++ persistence/sql_base_repository.go | 95 ++++++++++++++++++++++----- 5 files changed, 216 insertions(+), 39 deletions(-) diff --git a/persistence/player_repository.go b/persistence/player_repository.go index 6c8339378..c9c7d3b4b 100644 --- a/persistence/player_repository.go +++ b/persistence/player_repository.go @@ -67,11 +67,10 @@ func (r *playerRepository) addRestriction(sql ...Sqlizer) Sqlizer { if len(sql) > 0 { s = append(s, sql[0]) } - u := loggedUser(r.ctx) - if u.IsAdmin { - return s + if owner := r.ownerFilter(); owner != nil { + s = append(s, owner) } - return append(s, Eq{"user_id": u.ID}) + return s } func (r *playerRepository) CountByClient(options ...model.QueryOptions) (map[string]int64, error) { @@ -125,6 +124,10 @@ func (r *playerRepository) NewInstance() any { return &model.Player{} } +// isPermitted authorizes creating a new record, based on the owner declared in the request body. +// This is only safe for inserts: there is no stored row yet, and a non-admin may only create a +// player they own. Updates must not use this (the body owner is attacker-controlled); they go +// through updateOwned, which authorizes against the persisted user_id in the WHERE clause. func (r *playerRepository) isPermitted(p *model.Player) bool { u := loggedUser(r.ctx) return u.IsAdmin || p.UserId == u.ID @@ -145,14 +148,7 @@ func (r *playerRepository) Save(entity any) (string, error) { func (r *playerRepository) Update(id string, entity any, cols ...string) error { t := entity.(*model.Player) t.ID = id - if !r.isPermitted(t) { - return rest.ErrPermissionDenied - } - _, err := r.put(id, t, cols...) - if errors.Is(err, model.ErrNotFound) { - return rest.ErrNotFound - } - return err + return r.updateOwned(id, t, cols...) } func (r *playerRepository) Delete(id string) error { diff --git a/persistence/player_repository_test.go b/persistence/player_repository_test.go index f6c669493..f640cd9a4 100644 --- a/persistence/player_repository_test.go +++ b/persistence/player_repository_test.go @@ -215,9 +215,12 @@ var _ = Describe("PlayerRepository", func() { clone.MaxBitRate = 10000 err := repo.Update(clone.ID, &clone, "ip") - if clone.UserId == "" { + if player.UserId == "" { Expect(err).To(HaveOccurred()) } else if !admin && player.Username == adminPlayer1.Username { + // A non-admin cannot target another user's player: the ownership-restricted + // update matches no owned row, so it reports permission-denied rather than + // touching it. Expect(err).To(Equal(rest.ErrPermissionDenied)) clone.IP = player.IP } else { @@ -244,4 +247,86 @@ var _ = Describe("PlayerRepository", func() { Entry("admin context", true, players, adminPlayer1, regularPlayer), Entry("regular context", false, model.Players{regularPlayer}, regularPlayer, adminPlayer1), ) + + Describe("Ownership enforcement (cross-tenant write protection)", func() { + var regularRepo *playerRepository + + BeforeEach(func() { + ctx := log.NewContext(context.TODO()) + ctx = request.WithUser(ctx, regularUser) + regularRepo = NewPlayerRepository(ctx, database).(*playerRepository) + }) + + It("does not let a regular user hijack another user's player by spoofing userId in the body", func() { + // Attacker (regularUser) targets the victim's (adminUser) player by URL id, + // but sets userId in the body to their own id to try to pass the permission check. + spoofed := model.Player{ + ID: adminPlayer1.ID, + Name: "HIJACKED", + UserId: regularUser.ID, // attacker's own id, spoofed in the body + MaxBitRate: 1, + } + + // The ownership-restricted update matches no row owned by the attacker, so the write + // targets nothing and reports permission-denied rather than overwriting the victim's row. + err := regularRepo.Update(adminPlayer1.ID, &spoofed, "name", "user_id", "max_bit_rate") + Expect(err).To(Equal(rest.ErrPermissionDenied)) + + // The victim's player must remain untouched. + stored, err := adminRepo.Get(adminPlayer1.ID) + Expect(err).To(BeNil()) + Expect(*stored).To(Equal(adminPlayer1)) + }) + + It("does not let a regular user reassign their own player to another user", func() { + // Owner updates their own player but tries to give it away to the admin. The update + // succeeds for the other fields, but user_id is never written, so ownership stays put. + reassign := regularPlayer + reassign.UserId = adminUser.ID + reassign.Name = "given-away" + + err := regularRepo.Update(regularPlayer.ID, &reassign, "name", "user_id") + Expect(err).To(BeNil()) + + // Ownership must not have changed. + stored, err := adminRepo.Get(regularPlayer.ID) + Expect(err).To(BeNil()) + Expect(stored.UserId).To(Equal(regularUser.ID)) + }) + + It("does not let an admin reassign a player to another user", func() { + // Even an admin cannot change a player's owner via update. + reassign := regularPlayer + reassign.UserId = adminUser.ID + reassign.Name = "admin-renamed" + + err := adminRepo.Update(regularPlayer.ID, &reassign, "name", "user_id") + Expect(err).To(BeNil()) + + // The name change applies, but ownership must not have moved. + stored, err := adminRepo.Get(regularPlayer.ID) + Expect(err).To(BeNil()) + Expect(stored.Name).To(Equal("admin-renamed")) + Expect(stored.UserId).To(Equal(regularUser.ID)) + }) + + It("lets the owner update their own player", func() { + update := regularPlayer + update.Name = "renamed-by-owner" + + err := regularRepo.Update(regularPlayer.ID, &update, "name") + Expect(err).To(BeNil()) + + stored, err := adminRepo.Get(regularPlayer.ID) + Expect(err).To(BeNil()) + Expect(stored.Name).To(Equal("renamed-by-owner")) + Expect(stored.UserId).To(Equal(regularUser.ID)) + }) + + It("returns not found when updating a nonexistent player", func() { + ghost := model.Player{ID: "does-not-exist", Name: "ghost", UserId: regularUser.ID} + err := regularRepo.Update("does-not-exist", &ghost, "name") + Expect(err).To(Equal(rest.ErrNotFound)) + }) + }) }) diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 415109640..bcd13ff3e 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -53,6 +53,10 @@ func (r *shareRepository) checkOwnership(id string) error { return nil } +// TODO: this still uses the legacy checkOwnership SELECT-then-delete pattern (a TOCTOU window), +// the same shape removed from Update. Once a base-repo deleteOwned exists (built on ownerFilter, +// mirroring updateOwned), route Delete through it and drop checkOwnership entirely. playerRepository +// .Delete (which restricts via addRestriction) should adopt the same primitive. func (r *shareRepository) Delete(id string) error { if err := r.checkOwnership(id); err != nil { return err @@ -166,17 +170,12 @@ func sortByIdPosition(mfs model.MediaFiles, ids []string) model.MediaFiles { func (r *shareRepository) Update(id string, entity any, cols ...string) error { s := entity.(*model.Share) - if err := r.checkOwnership(id); err != nil { - return err - } s.ID = id s.UpdatedAt = time.Now() - cols = append(cols, "updated_at") - _, err := r.put(id, s, cols...) - if errors.Is(err, model.ErrNotFound) { - return rest.ErrNotFound + if len(cols) > 0 { + cols = append(cols, "updated_at") } - return err + return r.updateOwned(id, s, cols...) } func (r *shareRepository) Save(entity any) (string, error) { diff --git a/persistence/share_repository_test.go b/persistence/share_repository_test.go index 6988f323f..dcc84d66f 100644 --- a/persistence/share_repository_test.go +++ b/persistence/share_repository_test.go @@ -216,6 +216,44 @@ var _ = Describe("ShareRepository", func() { err := repo.(rest.Persistable).Update("headless-upd-share", &model.Share{Description: "Headless"}, "description") Expect(err).ToNot(HaveOccurred()) }) + + It("returns not found when updating a nonexistent share", func() { + ctx := request.WithUser(log.NewContext(context.TODO()), ownerUser) + repo := NewShareRepository(ctx, GetDBXBuilder()) + err := repo.(rest.Persistable).Update("does-not-exist", &model.Share{Description: "Ghost"}, "description") + Expect(err).To(Equal(rest.ErrNotFound)) + }) + + It("updates all columns when no specific columns are given", func() { + insertShare("all-cols-share", ownerUser.ID) + ctx := request.WithUser(log.NewContext(context.TODO()), ownerUser) + repo := NewShareRepository(ctx, GetDBXBuilder()) + // No cols: the update must write every column, not just updated_at. + err := repo.(rest.Persistable).Update("all-cols-share", + &model.Share{Description: "All Updated", MaxBitRate: 192, ResourceType: "album", ResourceIDs: "2002"}) + Expect(err).ToNot(HaveOccurred()) + + got, err := repo.(rest.Repository).Read("all-cols-share") + Expect(err).ToNot(HaveOccurred()) + share := got.(*model.Share) + Expect(share.Description).To(Equal("All Updated")) + Expect(share.MaxBitRate).To(Equal(192)) + Expect(share.ResourceType).To(Equal("album")) + }) + + It("does not let an owner reassign their share to another user", func() { + insertShare("reassign-share", ownerUser.ID) + ctx := request.WithUser(log.NewContext(context.TODO()), ownerUser) + repo := NewShareRepository(ctx, GetDBXBuilder()) + err := repo.(rest.Persistable).Update("reassign-share", + &model.Share{UserID: otherUser.ID, Description: "Given away"}, "user_id", "description") + Expect(err).ToNot(HaveOccurred()) + + // Ownership must not have moved, even though user_id was passed in the body and cols. + got, err := repo.(rest.Repository).Read("reassign-share") + Expect(err).ToNot(HaveOccurred()) + Expect(got.(*model.Share).UserID).To(Equal(ownerUser.ID)) + }) }) }) }) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index fd263d37b..55e83d544 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -13,6 +13,7 @@ import ( "time" . "github.com/Masterminds/squirrel" + "github.com/deluan/rest" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -57,6 +58,16 @@ func loggedUser(ctx context.Context) *model.User { } } +// ownerFilter returns the predicate restricting access to rows owned by the logged-in user, for +// tables with a user_id column. It returns nil for admins and for headless/system contexts (invalid +// user), meaning "no ownership restriction". Callers should skip the WHERE clause when it is nil. +func (r sqlRepository) ownerFilter() Sqlizer { + if usr := loggedUser(r.ctx); !usr.IsAdmin && usr.ID != invalidUserId { + return Eq{"user_id": usr.ID} + } + return nil +} + func (r *sqlRepository) registerModel(instance any, filters map[string]filterFunc) { if r.tableName == "" { r.tableName = strings.TrimPrefix(reflect.TypeOf(instance).String(), "*model.") @@ -382,6 +393,47 @@ func (r sqlRepository) exists(cond Sqlizer) (bool, error) { return res.Exist > 0, err } +// updateOwned performs an atomic, ownership-restricted update of the row identified by id, for +// repositories whose table has a user_id column. Non-admins can only update rows they own: the +// ownership predicate is part of the UPDATE's WHERE clause, so a row owned by another user simply +// does not match and no write happens. Ownership itself is immutable here: user_id is never written, +// so no caller (admin included) can reassign a row to a different owner via an update. Unlike put, +// it never falls through to an INSERT, so a non-matching id never creates a row. +// +// When the update matches no row it classifies the failure: if the row exists but is owned by +// another user it returns rest.ErrPermissionDenied, otherwise rest.ErrNotFound. The write itself is +// still atomic; the extra lookup happens only on the failure path (count == 0), where no write +// occurred, so there is no TOCTOU on the update. +func (r sqlRepository) updateOwned(id string, m any, colsToUpdate ...string) error { + values, err := toSQLArgs(m) + if err != nil { + return fmt.Errorf("error preparing values to write to DB: %w", err) + } + updateValues := filterUpdateValues(values, id, colsToUpdate...) + delete(updateValues, "user_id") // ownership is immutable on update + update := Update(r.tableName).Where(Eq{"id": id}).SetMap(updateValues) + if owner := r.ownerFilter(); owner != nil { + update = update.Where(owner) + } + count, err := r.executeSQL(update) + if err != nil { + return err + } + if count == 0 { + // The update matched no row: either the id is missing, or it exists but is owned by + // someone else. Disambiguate to return the more accurate error. + exists, err := r.exists(Eq{"id": id}) + if err != nil { + return err + } + if exists { + return rest.ErrPermissionDenied + } + return rest.ErrNotFound + } + return nil +} + func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) { countQuery = countQuery. RemoveColumns().Columns("count(distinct " + r.tableName + ".id) as count"). @@ -408,6 +460,30 @@ func (r sqlRepository) putByMatch(filter Sqlizer, id string, m any, colsToUpdate return r.put(res.ID, m, colsToUpdate...) } +// filterUpdateValues selects, from a marshaled column map, the values to write in an UPDATE on the +// row identified by id: only the requested colsToUpdate (or all columns when none are specified), +// dropping columns that must never be overwritten on update (created_at, birth_time). +func filterUpdateValues(values map[string]any, id string, colsToUpdate ...string) map[string]any { + updateValues := map[string]any{} + + // This is a map of the columns that need to be updated, if specified + c2upd := slice.ToMap(colsToUpdate, func(s string) (string, struct{}) { + return toSnakeCase(s), struct{}{} + }) + for k, v := range values { + if _, found := c2upd[k]; len(c2upd) == 0 || found { + updateValues[k] = v + } + } + + updateValues["id"] = id + delete(updateValues, "created_at") + // To avoid updating the media_file birth_time on each scan. Not the best solution, but it works for now + // TODO move to mediafile_repository when each repo has its own upsert method + delete(updateValues, "birth_time") + return updateValues +} + func (r sqlRepository) put(id string, m any, colsToUpdate ...string) (newId string, err error) { values, err := toSQLArgs(m) if err != nil { @@ -415,24 +491,7 @@ func (r sqlRepository) put(id string, m any, colsToUpdate ...string) (newId stri } // If there's an ID, try to update first if id != "" { - updateValues := map[string]any{} - - // This is a map of the columns that need to be updated, if specified - c2upd := slice.ToMap(colsToUpdate, func(s string) (string, struct{}) { - return toSnakeCase(s), struct{}{} - }) - for k, v := range values { - if _, found := c2upd[k]; len(c2upd) == 0 || found { - updateValues[k] = v - } - } - - updateValues["id"] = id - delete(updateValues, "created_at") - // To avoid updating the media_file birth_time on each scan. Not the best solution, but it works for now - // TODO move to mediafile_repository when each repo has its own upsert method - delete(updateValues, "birth_time") - update := Update(r.tableName).Where(Eq{"id": id}).SetMap(updateValues) + update := Update(r.tableName).Where(Eq{"id": id}).SetMap(filterUpdateValues(values, id, colsToUpdate...)) count, err := r.executeSQL(update) if err != nil { return "", err