mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 02:17:25 +02:00
fix(server): enforce player ownership on create and registration (#6184)
POST /api/player passed the ownership check using the userId from the request body, then saved with the body id. When that id belonged to another user's player, the save became an update with no owner restriction, overwriting the row and moving it to the caller. Save now always creates a new player and ignores any id in the body; edits keep going through the owner-scoped Update. Player registration also reused a player by the id sent in the Subsonic player cookie or the Jellyfin DeviceId without checking its owner, letting a user attach to another user's player and overwrite its name, user agent and IP. Register now only reuses a player owned by the requesting user, falling back to the user's own players otherwise.
This commit is contained in:
parent
237276efcd
commit
9e8811e4c5
4 changed files with 36 additions and 4 deletions
|
|
@ -38,7 +38,7 @@ func (p *players) Register(ctx context.Context, playerID, client, userAgent, ip
|
|||
user, _ := request.UserFrom(ctx)
|
||||
if playerID != "" {
|
||||
plr, err = p.ds.Player(ctx).Get(playerID)
|
||||
if err == nil && plr.Client != client {
|
||||
if err == nil && (plr.Client != client || plr.UserId != user.ID) {
|
||||
playerID = ""
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -61,8 +61,19 @@ var _ = Describe("Players", func() {
|
|||
Expect(trc).To(BeNil())
|
||||
})
|
||||
|
||||
It("does not reuse another user's player by ID", func() {
|
||||
plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "otheruser", UserAgent: "Pixel", TranscodingId: "1"}
|
||||
repo.add(plr)
|
||||
p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(p.ID).ToNot(Equal("123"))
|
||||
Expect(p.UserId).To(Equal("userid"))
|
||||
Expect(repo.lastSaved).To(Equal(p))
|
||||
Expect(trc).To(BeNil())
|
||||
})
|
||||
|
||||
It("finds players by ID", func() {
|
||||
plr := &model.Player{ID: "123", Name: "A Player", Client: "client", LastSeen: time.Time{}}
|
||||
plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "userid", LastSeen: time.Time{}}
|
||||
repo.add(plr)
|
||||
p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
|
@ -93,7 +104,7 @@ var _ = Describe("Players", func() {
|
|||
})
|
||||
|
||||
It("finds player by ID and return its transcoding", func() {
|
||||
plr := &model.Player{ID: "123", Name: "A Player", Client: "client", LastSeen: time.Time{}, TranscodingId: "1"}
|
||||
plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "userid", LastSeen: time.Time{}, TranscodingId: "1"}
|
||||
repo.add(plr)
|
||||
p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
|
|
|||
|
|
@ -126,7 +126,7 @@ func (r *playerRepository) Save(entity any) (string, error) {
|
|||
if !r.isPermitted(t) {
|
||||
return "", rest.ErrPermissionDenied
|
||||
}
|
||||
return r.put(t.ID, t)
|
||||
return r.put("", t) // Save only creates; edits go through the owner-scoped Update
|
||||
}
|
||||
|
||||
func (r *playerRepository) Update(id string, entity any, cols ...string) error {
|
||||
|
|
|
|||
|
|
@ -288,6 +288,27 @@ var _ = Describe("PlayerRepository", func() {
|
|||
Expect(*stored).To(Equal(adminPlayer1))
|
||||
})
|
||||
|
||||
It("does not let a regular user overwrite another user's player via Save with a spoofed id", func() {
|
||||
spoofed := model.Player{
|
||||
ID: adminPlayer1.ID,
|
||||
Name: "HIJACKED",
|
||||
UserId: regularUser.ID,
|
||||
ReportRealPath: true,
|
||||
}
|
||||
|
||||
id, err := regularRepo.Save(&spoofed)
|
||||
Expect(err).To(BeNil())
|
||||
Expect(id).ToNot(Equal(adminPlayer1.ID))
|
||||
|
||||
stored, err := adminRepo.Get(adminPlayer1.ID)
|
||||
Expect(err).To(BeNil())
|
||||
Expect(*stored).To(Equal(adminPlayer1))
|
||||
|
||||
created, err := adminRepo.Get(id)
|
||||
Expect(err).To(BeNil())
|
||||
Expect(created.UserId).To(Equal(regularUser.ID))
|
||||
})
|
||||
|
||||
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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue