2024-08-03 09:37:21 -08:00
|
|
|
package persistence
|
|
|
|
|
|
|
|
|
|
import (
|
|
|
|
|
"context"
|
|
|
|
|
|
|
|
|
|
"github.com/deluan/rest"
|
|
|
|
|
"github.com/navidrome/navidrome/log"
|
|
|
|
|
"github.com/navidrome/navidrome/model"
|
|
|
|
|
"github.com/navidrome/navidrome/model/request"
|
|
|
|
|
. "github.com/onsi/ginkgo/v2"
|
|
|
|
|
. "github.com/onsi/gomega"
|
2024-11-18 04:52:13 -09:00
|
|
|
"github.com/pocketbase/dbx"
|
2024-08-03 09:37:21 -08:00
|
|
|
)
|
|
|
|
|
|
|
|
|
|
var _ = Describe("PlayerRepository", func() {
|
|
|
|
|
var adminRepo *playerRepository
|
2024-11-18 04:52:13 -09:00
|
|
|
var database *dbx.DB
|
2024-08-03 09:37:21 -08:00
|
|
|
|
|
|
|
|
var (
|
|
|
|
|
adminPlayer1 = model.Player{ID: "1", Name: "NavidromeUI [Firefox/Linux]", UserAgent: "Firefox/Linux", UserId: adminUser.ID, Username: adminUser.UserName, Client: "NavidromeUI", IP: "127.0.0.1", ReportRealPath: true, ScrobbleEnabled: true}
|
|
|
|
|
adminPlayer2 = model.Player{ID: "2", Name: "GenericClient [Chrome/Windows]", IP: "192.168.0.5", UserAgent: "Chrome/Windows", UserId: adminUser.ID, Username: adminUser.UserName, Client: "GenericClient", MaxBitRate: 128}
|
|
|
|
|
regularPlayer = model.Player{ID: "3", Name: "NavidromeUI [Safari/macOS]", UserAgent: "Safari/macOS", UserId: regularUser.ID, Username: regularUser.UserName, Client: "NavidromeUI", ReportRealPath: true, ScrobbleEnabled: false}
|
|
|
|
|
|
|
|
|
|
players = model.Players{adminPlayer1, adminPlayer2, regularPlayer}
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
ctx := log.NewContext(context.TODO())
|
|
|
|
|
ctx = request.WithUser(ctx, adminUser)
|
|
|
|
|
|
2024-11-18 04:52:13 -09:00
|
|
|
database = GetDBXBuilder()
|
2024-08-03 09:37:21 -08:00
|
|
|
adminRepo = NewPlayerRepository(ctx, database).(*playerRepository)
|
|
|
|
|
|
|
|
|
|
for idx := range players {
|
|
|
|
|
err := adminRepo.Put(&players[idx])
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
}
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
AfterEach(func() {
|
|
|
|
|
items, err := adminRepo.ReadAll()
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
players, ok := items.(model.Players)
|
|
|
|
|
Expect(ok).To(BeTrue())
|
|
|
|
|
for i := range players {
|
|
|
|
|
err = adminRepo.Delete(players[i].ID)
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
}
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("EntityName", func() {
|
|
|
|
|
It("returns the right name", func() {
|
|
|
|
|
Expect(adminRepo.EntityName()).To(Equal("player"))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("FindMatch", func() {
|
|
|
|
|
It("finds existing match", func() {
|
|
|
|
|
player, err := adminRepo.FindMatch(adminUser.ID, "NavidromeUI", "Firefox/Linux")
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(*player).To(Equal(adminPlayer1))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("doesn't find bad match", func() {
|
|
|
|
|
_, err := adminRepo.FindMatch(regularUser.ID, "NavidromeUI", "Firefox/Linux")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Get", func() {
|
|
|
|
|
It("Gets an existing item from user", func() {
|
|
|
|
|
player, err := adminRepo.Get(adminPlayer1.ID)
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(*player).To(Equal(adminPlayer1))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("Gets an existing item from another user", func() {
|
|
|
|
|
player, err := adminRepo.Get(regularPlayer.ID)
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(*player).To(Equal(regularPlayer))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("does not get nonexistent item", func() {
|
|
|
|
|
_, err := adminRepo.Get("i don't exist")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
DescribeTableSubtree("per context", func(admin bool, players model.Players, userPlayer model.Player, otherPlayer model.Player) {
|
|
|
|
|
var repo *playerRepository
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
if admin {
|
|
|
|
|
repo = adminRepo
|
|
|
|
|
} else {
|
|
|
|
|
ctx := log.NewContext(context.TODO())
|
|
|
|
|
ctx = request.WithUser(ctx, regularUser)
|
|
|
|
|
repo = NewPlayerRepository(ctx, database).(*playerRepository)
|
|
|
|
|
}
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
baseCount := int64(len(players))
|
|
|
|
|
|
|
|
|
|
Describe("Count", func() {
|
|
|
|
|
It("should return all", func() {
|
|
|
|
|
count, err := repo.Count()
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(count).To(Equal(baseCount))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Delete", func() {
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
It("deletes a player owned by the current user", func() {
|
|
|
|
|
err := repo.Delete(userPlayer.ID)
|
2024-08-03 09:37:21 -08:00
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
|
|
|
|
|
count, err := repo.Count()
|
|
|
|
|
Expect(err).To(BeNil())
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
Expect(count).To(Equal(baseCount - 1))
|
2024-08-03 09:37:21 -08:00
|
|
|
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
_, err = repo.Get(userPlayer.ID)
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
2024-08-03 09:37:21 -08:00
|
|
|
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
It("does not delete another user's player when not admin", func() {
|
|
|
|
|
err := repo.Delete(otherPlayer.ID)
|
|
|
|
|
|
|
|
|
|
if admin {
|
|
|
|
|
// Admins may delete any player.
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(repo.Count()).To(Equal(baseCount - 1))
|
|
|
|
|
_, err = repo.Get(otherPlayer.ID)
|
2024-08-03 09:37:21 -08:00
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
} else {
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
// The ownership-restricted delete matches no owned row, so it reports
|
|
|
|
|
// permission-denied and leaves the other user's player untouched.
|
|
|
|
|
Expect(err).To(Equal(rest.ErrPermissionDenied))
|
|
|
|
|
Expect(repo.Count()).To(Equal(baseCount))
|
|
|
|
|
item, err := repo.Get(otherPlayer.ID)
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(*item).To(Equal(otherPlayer))
|
2024-08-03 09:37:21 -08:00
|
|
|
}
|
fix(share): enforce per-user ownership on share reads
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
2026-06-05 11:50:59 -08:00
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns not-found for a nonexistent player", func() {
|
|
|
|
|
err := repo.Delete("i don't exist")
|
|
|
|
|
Expect(err).To(Equal(rest.ErrNotFound))
|
|
|
|
|
Expect(repo.Count()).To(Equal(baseCount))
|
|
|
|
|
})
|
2024-08-03 09:37:21 -08:00
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Read", func() {
|
|
|
|
|
It("can read from current user", func() {
|
|
|
|
|
player, err := repo.Read(userPlayer.ID)
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(player).To(Equal(&userPlayer))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("can read from other user or fail if not admin", func() {
|
|
|
|
|
player, err := repo.Read(otherPlayer.ID)
|
|
|
|
|
if admin {
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(player).To(Equal(&otherPlayer))
|
|
|
|
|
} else {
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
}
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("does not get nonexistent item", func() {
|
|
|
|
|
_, err := repo.Read("i don't exist")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("ReadAll", func() {
|
|
|
|
|
It("should get all items", func() {
|
|
|
|
|
data, err := repo.ReadAll()
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(data).To(Equal(players))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Save", func() {
|
|
|
|
|
DescribeTable("item type", func(player model.Player) {
|
|
|
|
|
clone := player
|
|
|
|
|
clone.ID = ""
|
|
|
|
|
clone.IP = "192.168.1.1"
|
|
|
|
|
id, err := repo.Save(&clone)
|
|
|
|
|
|
|
|
|
|
if clone.UserId == "" {
|
|
|
|
|
Expect(err).To(HaveOccurred())
|
|
|
|
|
} else if !admin && player.Username == adminPlayer1.Username {
|
|
|
|
|
Expect(err).To(Equal(rest.ErrPermissionDenied))
|
|
|
|
|
clone.UserId = ""
|
|
|
|
|
} else {
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
Expect(id).ToNot(BeEmpty())
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
count, err := repo.Count()
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
|
|
|
|
|
clone.ID = id
|
2024-09-13 16:18:00 -08:00
|
|
|
newItem, err := repo.Get(id)
|
2024-08-03 09:37:21 -08:00
|
|
|
|
|
|
|
|
if clone.UserId == "" {
|
|
|
|
|
Expect(count).To(Equal(baseCount))
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
} else {
|
|
|
|
|
Expect(count).To(Equal(baseCount + 1))
|
|
|
|
|
Expect(err).To(BeNil())
|
2024-09-13 16:18:00 -08:00
|
|
|
Expect(*newItem).To(Equal(clone))
|
2024-08-03 09:37:21 -08:00
|
|
|
}
|
|
|
|
|
},
|
|
|
|
|
Entry("same user", userPlayer),
|
|
|
|
|
Entry("other item", otherPlayer),
|
|
|
|
|
Entry("fake item", model.Player{}),
|
|
|
|
|
)
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Update", func() {
|
|
|
|
|
DescribeTable("item type", func(player model.Player) {
|
|
|
|
|
clone := player
|
|
|
|
|
clone.IP = "192.168.1.1"
|
|
|
|
|
clone.MaxBitRate = 10000
|
|
|
|
|
err := repo.Update(clone.ID, &clone, "ip")
|
|
|
|
|
|
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 15:43:13 -08:00
|
|
|
if player.UserId == "" {
|
2024-08-03 09:37:21 -08:00
|
|
|
Expect(err).To(HaveOccurred())
|
|
|
|
|
} else if !admin && player.Username == adminPlayer1.Username {
|
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 15:43:13 -08:00
|
|
|
// 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.
|
2024-08-03 09:37:21 -08:00
|
|
|
Expect(err).To(Equal(rest.ErrPermissionDenied))
|
|
|
|
|
clone.IP = player.IP
|
|
|
|
|
} else {
|
|
|
|
|
Expect(err).To(BeNil())
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
clone.MaxBitRate = player.MaxBitRate
|
2024-09-13 16:18:00 -08:00
|
|
|
newItem, err := repo.Get(clone.ID)
|
2024-08-03 09:37:21 -08:00
|
|
|
|
|
|
|
|
if player.UserId == "" {
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
} else if !admin && player.UserId == adminUser.ID {
|
2024-09-13 16:18:00 -08:00
|
|
|
Expect(*newItem).To(Equal(player))
|
2024-08-03 09:37:21 -08:00
|
|
|
} else {
|
2024-09-13 16:18:00 -08:00
|
|
|
Expect(*newItem).To(Equal(clone))
|
2024-08-03 09:37:21 -08:00
|
|
|
}
|
|
|
|
|
},
|
|
|
|
|
Entry("same user", userPlayer),
|
|
|
|
|
Entry("other item", otherPlayer),
|
|
|
|
|
Entry("fake item", model.Player{}),
|
|
|
|
|
)
|
|
|
|
|
})
|
|
|
|
|
},
|
|
|
|
|
Entry("admin context", true, players, adminPlayer1, regularPlayer),
|
|
|
|
|
Entry("regular context", false, model.Players{regularPlayer}, regularPlayer, adminPlayer1),
|
|
|
|
|
)
|
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 15:43:13 -08:00
|
|
|
|
|
|
|
|
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))
|
|
|
|
|
})
|
|
|
|
|
})
|
2024-08-03 09:37:21 -08:00
|
|
|
})
|