quintodrome/persistence/share_repository.go
Deluan Quintão 37908d3cea
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 <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>
2026-06-04 19:43:13 -04:00

229 lines
6.2 KiB
Go

package persistence
import (
"context"
"errors"
"fmt"
"strings"
"time"
. "github.com/Masterminds/squirrel"
"github.com/deluan/rest"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
"github.com/pocketbase/dbx"
)
type shareRepository struct {
sqlRepository
}
func NewShareRepository(ctx context.Context, db dbx.Builder) model.ShareRepository {
r := &shareRepository{}
r.ctx = ctx
r.db = db
r.registerModel(&model.Share{}, nil)
r.setSortMappings(map[string]string{
"username": "username",
})
return r
}
// TODO: Ownership checks should be moved to the service layer (core/share.go)
func (r *shareRepository) checkOwnership(id string) error {
usr := loggedUser(r.ctx)
if usr.IsAdmin || usr.ID == invalidUserId {
return nil
}
sel := r.newSelect().Columns("user_id").Where(Eq{"id": id})
var share struct {
UserID string `db:"user_id"`
}
err := r.queryOne(sel, &share)
if err != nil {
if errors.Is(err, model.ErrNotFound) {
return rest.ErrNotFound
}
return err
}
if share.UserID != usr.ID {
return rest.ErrPermissionDenied
}
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
}
err := r.delete(Eq{"id": id})
if errors.Is(err, model.ErrNotFound) {
return rest.ErrNotFound
}
return err
}
func (r *shareRepository) selectShare(options ...model.QueryOptions) SelectBuilder {
return r.newSelect(options...).Join("user u on u.id = share.user_id").
Columns("share.*", "user_name as username")
}
func (r *shareRepository) Exists(id string) (bool, error) {
return r.exists(Eq{"id": id})
}
func (r *shareRepository) Get(id string) (*model.Share, error) {
sel := r.selectShare().Where(Eq{"share.id": id})
var res model.Share
err := r.queryOne(sel, &res)
if err != nil {
return nil, err
}
err = r.loadMedia(&res)
return &res, err
}
func (r *shareRepository) GetAll(options ...model.QueryOptions) (model.Shares, error) {
sq := r.selectShare(options...)
res := model.Shares{}
err := r.queryAll(sq, &res)
if err != nil {
return nil, err
}
for i := range res {
err = r.loadMedia(&res[i])
if err != nil {
return nil, fmt.Errorf("error loading media for share %s: %w", res[i].ID, err)
}
}
return res, err
}
func (r *shareRepository) loadMedia(share *model.Share) error {
var err error
ids := strings.Split(share.ResourceIDs, ",")
if len(ids) == 0 {
return nil
}
noMissing := func(cond Sqlizer) Sqlizer {
return And{cond, Eq{"missing": false}}
}
switch share.ResourceType {
case "artist":
albumRepo := NewAlbumRepository(r.ctx, r.db)
share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_artist_id": ids}), Sort: "artist"})
if err != nil {
return err
}
mfRepo := NewMediaFileRepository(r.ctx, r.db)
share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_artist_id": ids}), Sort: "artist"})
return err
case "album":
albumRepo := NewAlbumRepository(r.ctx, r.db)
share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album.id": ids})})
if err != nil {
return err
}
mfRepo := NewMediaFileRepository(r.ctx, r.db)
share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_id": ids}), Sort: "album"})
return err
case "playlist":
// Create a context with a fake admin user, to be able to access all playlists
ctx := request.WithUser(r.ctx, model.User{IsAdmin: true})
plsRepo := NewPlaylistRepository(ctx, r.db)
tracks, err := plsRepo.Tracks(ids[0], true).GetAll(model.QueryOptions{Sort: "id", Filters: noMissing(Eq{})})
if err != nil {
return err
}
if len(tracks) >= 0 {
share.Tracks = tracks.MediaFiles()
}
return nil
case "media_file":
mfRepo := NewMediaFileRepository(r.ctx, r.db)
tracks, err := mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"media_file.id": ids})})
share.Tracks = sortByIdPosition(tracks, ids)
return err
}
log.Warn(r.ctx, "Unsupported Share ResourceType", "share", share.ID, "resourceType", share.ResourceType)
return nil
}
func sortByIdPosition(mfs model.MediaFiles, ids []string) model.MediaFiles {
m := map[string]int{}
for i, mf := range mfs {
m[mf.ID] = i
}
var sorted model.MediaFiles
for _, id := range ids {
if idx, ok := m[id]; ok {
sorted = append(sorted, mfs[idx])
}
}
return sorted
}
func (r *shareRepository) Update(id string, entity any, cols ...string) error {
s := entity.(*model.Share)
s.ID = id
s.UpdatedAt = time.Now()
if len(cols) > 0 {
cols = append(cols, "updated_at")
}
return r.updateOwned(id, s, cols...)
}
func (r *shareRepository) Save(entity any) (string, error) {
s := entity.(*model.Share)
// TODO Validate record
u := loggedUser(r.ctx)
if s.UserID == "" {
s.UserID = u.ID
}
s.CreatedAt = time.Now()
s.UpdatedAt = time.Now()
id, err := r.put(s.ID, s)
if errors.Is(err, model.ErrNotFound) {
return "", rest.ErrNotFound
}
return id, err
}
func (r *shareRepository) CountAll(options ...model.QueryOptions) (int64, error) {
return r.count(r.selectShare(), options...)
}
func (r *shareRepository) Count(options ...rest.QueryOptions) (int64, error) {
return r.CountAll(r.parseRestOptions(r.ctx, options...))
}
func (r *shareRepository) EntityName() string {
return "share"
}
func (r *shareRepository) NewInstance() any {
return &model.Share{}
}
func (r *shareRepository) Read(id string) (any, error) {
sel := r.selectShare().Where(Eq{"share.id": id})
var res model.Share
err := r.queryOne(sel, &res)
return &res, err
}
func (r *shareRepository) ReadAll(options ...rest.QueryOptions) (any, error) {
sq := r.selectShare(r.parseRestOptions(r.ctx, options...))
res := model.Shares{}
err := r.queryAll(sq, &res)
return res, err
}
var _ model.ShareRepository = (*shareRepository)(nil)
var _ rest.Repository = (*shareRepository)(nil)
var _ rest.Persistable = (*shareRepository)(nil)