From a75f820fa3e62e12d787b6460ba00bd841663887 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 27 May 2026 20:09:29 -0300 Subject: [PATCH] test(playlists): cover rules-only PUT + case-variant owner-change guard Follow-ups from manual testing and code review of the prior commit: - Manual testing confirmed Feishin-style rules-only PUT works correctly on the fix; add ginkgo regression tests for rules-only update, name+ rules combined, idempotent rules PUT (no-op), and bulk Make-Public preserving rules on smart playlists. - Keep the non-admin owner-change permission check gated on the deserialized entity content (not on `sent("ownerId")`) so a case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403 to a silent 200. Go's json decoder is case-insensitive on struct field matching but rest.Put's field-name extraction is case- sensitive; the entity-based guard catches both spellings. The apply-side gating on ownerChanged still prevents the actual mutation, so this was a behavioral (not security) regression, but worth fixing. Adds a regression test asserting the case-variant key still returns rest.ErrPermissionDenied. - Correct misleading doc on applyContentUpdate: the path does not rewrite the backing M3U file; it goes through updateMetadata which bumps updatedAt and invalidates cached cover-art URLs. --- core/playlists/rest_adapter.go | 11 ++-- core/playlists/rest_adapter_test.go | 84 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 9795fa0a7..036c6f196 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -105,7 +105,11 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity usr, _ := request.UserFrom(ctx) ownerChanged := sent("ownerId") && entity.OwnerID != "" && entity.OwnerID != current.OwnerID - if !usr.IsAdmin && ownerChanged { + // Permission check uses the deserialized entity directly (not gated by `sent`) + // so a non-admin can't smuggle in an owner change via a case-variant JSON key + // like {"OwnerId":"x"} — Go's json decoder is case-insensitive on field match + // but rest.Put's field-name extraction is case-sensitive. + if !usr.IsAdmin && entity.OwnerID != "" && entity.OwnerID != current.OwnerID { return rest.ErrPermissionDenied } @@ -121,8 +125,9 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity } // applyContentUpdate handles updates that change at least one of name/comment/ -// owner/rules — the path that goes through updateMetadata and may rewrite the -// backing M3U file. Pointer args are nil for fields not present in the request. +// owner/rules. It goes through updateMetadata, which always bumps updatedAt +// (invalidating cached cover-art URLs). Pointer args are nil for fields not +// present in the request. func (s *playlists) applyContentUpdate(ctx context.Context, current, entity *model.Playlist, sent func(string) bool, nameChanged, commentChanged, ownerChanged, rulesChanged bool, ) error { diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 07b8616a9..5a15d04e3 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -125,6 +125,18 @@ var _ = Describe("REST Adapter", func() { Expect(err).To(Equal(rest.ErrPermissionDenied)) }) + It("denies regular user even when ownerId arrives under a case-variant JSON key", func() { + // rest.Put's field-name extraction is case-sensitive, but Go's json + // decoder is case-insensitive on struct fields, so {"OwnerId":"x"} + // populates entity.OwnerID while cols carries "OwnerId" instead of + // "ownerId". The permission gate must still fire. + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{OwnerID: "other-user"} + err := repo.Update("pls-1", pls, "OwnerId") + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + It("updates smart playlist rules", func() { mockPlsRepo.Data["smart-1"] = &model.Playlist{ ID: "smart-1", @@ -276,6 +288,78 @@ var _ = Describe("REST Adapter", func() { Expect(mockPlsRepo.Last.Name).To(Equal("Original Name")) }) + It("updates rules-only on a smart playlist (Feishin-style edit)", func() { + mockPlsRepo.Data["smart-partial"] = &model.Playlist{ + ID: "smart-partial", + Name: "Smart Original", + Comment: "smart comment", + OwnerID: "user-1", + Public: true, + Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}}, + } + repo = ps.NewRepository(ctx).(rest.Persistable) + newRules := &criteria.Criteria{Expression: criteria.Is{"genre": "Jazz"}, Sort: "year DESC"} + err := repo.Update("smart-partial", &model.Playlist{Rules: newRules}, "rules") + Expect(err).ToNot(HaveOccurred()) + Expect(mockPlsRepo.Last.Rules).To(Equal(newRules)) + Expect(mockPlsRepo.Last.Name).To(Equal("Smart Original")) + Expect(mockPlsRepo.Last.Comment).To(Equal("smart comment")) + Expect(mockPlsRepo.Last.Public).To(BeTrue()) + }) + + It("updates name and rules together (smart-playlist Edit form)", func() { + mockPlsRepo.Data["smart-edit"] = &model.Playlist{ + ID: "smart-edit", + Name: "Smart Original", + Comment: "smart comment", + OwnerID: "user-1", + Rules: &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}}, + } + repo = ps.NewRepository(ctx).(rest.Persistable) + newRules := &criteria.Criteria{Expression: criteria.Is{"artist": "Miles Davis"}, Sort: "album"} + err := repo.Update("smart-edit", + &model.Playlist{Name: "Smart Renamed", Rules: newRules}, + "name", "rules") + Expect(err).ToNot(HaveOccurred()) + Expect(mockPlsRepo.Last.Name).To(Equal("Smart Renamed")) + Expect(mockPlsRepo.Last.Rules).To(Equal(newRules)) + Expect(mockPlsRepo.Last.Comment).To(Equal("smart comment")) + }) + + It("does not bump the saved rules on an idempotent rules-only PUT", func() { + rules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}} + mockPlsRepo.Data["smart-idempotent"] = &model.Playlist{ + ID: "smart-idempotent", + Name: "Smart Idempotent", + OwnerID: "user-1", + Rules: rules, + } + repo = ps.NewRepository(ctx).(rest.Persistable) + // Same rules sent back — rulesEqual should report no change and + // the request should no-op (no Put call). + sameRules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}} + err := repo.Update("smart-idempotent", &model.Playlist{Rules: sameRules}, "rules") + Expect(err).ToNot(HaveOccurred()) + Expect(mockPlsRepo.Last).To(BeNil()) // no Put happened + }) + + It("preserves rules when only public is sent (smart playlist + bulk Make Public)", func() { + rules := &criteria.Criteria{Expression: criteria.Is{"genre": "Rock"}} + mockPlsRepo.Data["smart-public"] = &model.Playlist{ + ID: "smart-public", + Name: "Smart Public", + OwnerID: "user-1", + Public: false, + Rules: rules, + } + repo = ps.NewRepository(ctx).(rest.Persistable) + err := repo.Update("smart-public", &model.Playlist{Public: true}, "public") + Expect(err).ToNot(HaveOccurred()) + Expect(mockPlsRepo.Last.Public).To(BeTrue()) + Expect(mockPlsRepo.Last.Rules).To(Equal(rules)) + Expect(mockPlsRepo.Last.Name).To(Equal("Smart Public")) + }) + It("does not treat a missing ownerId as an ownership transfer attempt", func() { // A non-admin user sending only {public:true} should not be blocked // just because OwnerID is the zero value in the deserialized entity.