fix(share): enforce track membership on public share streams (#5769)

* fix(share): enforce track membership on public share streams

The public share stream endpoint (GET /share/s/{jwt}) validated that the
share existed, was unexpired, and that the share owner had library access
to the requested track, but it never verified that the track was actually
a member of the share. It also accepted stream tokens with no share id
(sid) claim, skipping share checks entirely.

Enforce that the requested media file belongs to share.Tracks, and make
the sid claim mandatory on the stream path. The only producer of stream
tokens (encodeMediafileShare) always sets sid, so no legitimate flow is
affected; the image endpoint decodes independently and is unchanged.

Also document why a JWT is used to represent a shared track: it is a
signed, scoped capability for a single public share, not part of
authentication.

* docs(share): clarify JWT usage comment wording

server/public/handle_shares.go

@@ -97,6 +97,22 @@ func (pub *Router) mapShareToM3U(r *http.Request, s model.Share) *model.Share {
 	return &s
 }
 
+// encodeMediafileShare builds the signed token embedded in a public share link
+// for a single track.
+//
+// NOTE ON JWT USAGE: This is deliberately NOT part of Navidrome's authentication.
+// The token is a signed, opaque capability that identifies one shared track
+// (plus its transcode format/bitrate and the parent share id). We use a JWT here
+// (reusing the library we already have) because it is a simple way to get three
+// properties for a public link: the embedded ids can't be enumerated by guessing,
+// the signature
+// makes the claims tamper-evident, and the self-contained exp lets us reject
+// stale links without a DB lookup. It carries no user identity (no subject, no
+// admin flag) and grants access to nothing beyond the share it belongs to; the
+// stream handler still verifies the share exists, is unexpired, and that the
+// track is actually a member of it. An attacker who can forge these tokens
+// necessarily already holds the signing secret, which also signs real user
+// sessions, so that scenario is out of scope for the share boundary specifically.
 func encodeMediafileShare(s model.Share, id string) string {
 	claims := auth.Claims{
 		ID:      id,

server/public/handle_streams.go

@@ -3,6 +3,7 @@ package public
 import (
 	"errors"
 	"net/http"
+	"slices"
 	"strconv"
 	"time"
 
@@ -25,23 +26,20 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	var shareOwner *model.User
-	if info.shareID != "" {
-		share, err := pub.ds.Share(ctx).Get(info.shareID)
-		if err != nil {
-			checkShareError(ctx, w, err, info.shareID)
-			return
-		}
-		if expiresAt := V(share.ExpiresAt); !expiresAt.IsZero() && expiresAt.Before(time.Now()) {
-			checkShareError(ctx, w, model.ErrExpired, info.shareID)
-			return
-		}
-		shareOwner, err = pub.ds.User(ctx).Get(share.UserID)
-		if err != nil {
-			log.Error(ctx, "Error retrieving share owner for shared stream", "share", info.shareID, "owner", share.UserID, err)
-			http.Error(w, "internal error", http.StatusInternalServerError)
-			return
-		}
+	share, err := pub.ds.Share(ctx).Get(info.shareID)
+	if err != nil {
+		checkShareError(ctx, w, err, info.shareID)
+		return
+	}
+	if expiresAt := V(share.ExpiresAt); !expiresAt.IsZero() && expiresAt.Before(time.Now()) {
+		checkShareError(ctx, w, model.ErrExpired, info.shareID)
+		return
+	}
+	shareOwner, err := pub.ds.User(ctx).Get(share.UserID)
+	if err != nil {
+		log.Error(ctx, "Error retrieving share owner for shared stream", "share", info.shareID, "owner", share.UserID, err)
+		http.Error(w, "internal error", http.StatusInternalServerError)
+		return
 	}
 
 	mf, err := pub.ds.MediaFile(ctx).Get(info.id)
@@ -56,7 +54,8 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// 404 rather than 403 so the response doesn't reveal whether the id exists.
-	if shareOwner != nil && !shareOwner.HasLibraryAccess(mf.LibraryID) {
+	// The track must belong to the share AND be within the owner's libraries.
+	if !shareContainsTrack(share, mf.ID) || !shareOwner.HasLibraryAccess(mf.LibraryID) {
 		http.Error(w, "not found", http.StatusNotFound)
 		return
 	}
@@ -98,6 +97,15 @@ type shareTrackInfo struct {
 	shareID string
 }
 
+func shareContainsTrack(share *model.Share, mediaFileID string) bool {
+	return slices.ContainsFunc(share.Tracks, func(mf model.MediaFile) bool {
+		return mf.ID == mediaFileID
+	})
+}
+
+// decodeStreamInfo decodes the signed share-link token. This is a scoped
+// public-share capability, not an auth credential; see encodeMediafileShare for
+// why a JWT is used here.
 func decodeStreamInfo(tokenString string) (shareTrackInfo, error) {
 	c, err := auth.Validate(tokenString)
 	if err != nil {
@@ -106,6 +114,9 @@ func decodeStreamInfo(tokenString string) (shareTrackInfo, error) {
 	if c.ID == "" {
 		return shareTrackInfo{}, errors.New("required claim \"id\" not found")
 	}
+	if c.ShareID == "" {
+		return shareTrackInfo{}, errors.New("required claim \"sid\" not found")
+	}
 	return shareTrackInfo{
 		id:      c.ID,
 		format:  c.Format,

server/public/handle_streams_test.go

@@ -71,14 +71,11 @@ var _ = Describe("decodeStreamInfo", func() {
 		Expect(err).To(HaveOccurred())
 	})
 
-	It("handles tokens without shareID (backward compat)", func() {
+	It("rejects a token without a shareID claim", func() {
 		claims := auth.Claims{ID: "mf-123", Format: "opus"}
 		token, _ := auth.CreatePublicToken(claims)
-		info, err := decodeStreamInfo(token)
-		Expect(err).NotTo(HaveOccurred())
-		Expect(info.id).To(Equal("mf-123"))
-		Expect(info.format).To(Equal("opus"))
-		Expect(info.shareID).To(BeEmpty())
+		_, err := decodeStreamInfo(token)
+		Expect(err).To(HaveOccurred())
 	})
 })
 
@@ -133,7 +130,7 @@ var _ = Describe("handleStream", func() {
 
 	shareOwnedBy := func(owner model.User, mf model.MediaFile) {
 		shareRepo.ID = "share123"
-		shareRepo.Entity = &model.Share{ID: "share123", UserID: owner.ID}
+		shareRepo.Entity = &model.Share{ID: "share123", UserID: owner.ID, Tracks: model.MediaFiles{mf}}
 		userRepo := tests.CreateMockUserRepo()
 		Expect(userRepo.Put(&owner)).To(Succeed())
 		ds.MockedUser = userRepo
@@ -171,6 +168,25 @@ var _ = Describe("handleStream", func() {
 		Expect(streamer.called).To(BeFalse())
 	})
 
+	It("returns 404 when the track is not a member of the share", func() {
+		owner := model.User{ID: "owner1", UserName: "owner1", IsAdmin: true}
+		userRepo := tests.CreateMockUserRepo()
+		Expect(userRepo.Put(&owner)).To(Succeed())
+		ds.MockedUser = userRepo
+		mfRepo := tests.CreateMockMediaFileRepo()
+		mfRepo.SetData(model.MediaFiles{{ID: "mf-shared"}, {ID: "mf-other"}})
+		ds.MockedMediaFile = mfRepo
+		shareRepo.ID = "share123"
+		shareRepo.Entity = &model.Share{ID: "share123", UserID: owner.ID, Tracks: model.MediaFiles{{ID: "mf-shared"}}}
+
+		claims := auth.Claims{ID: "mf-other", ShareID: "share123"}
+		token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims)
+		w := makeRequest(token)
+
+		Expect(w.Code).To(Equal(http.StatusNotFound))
+		Expect(streamer.called).To(BeFalse())
+	})
+
 	It("streams a track inside the share owner's libraries", func() {
 		shareOwnedBy(
 			model.User{ID: "owner1", UserName: "owner1", Libraries: model.Libraries{{ID: 1}}},
@@ -217,12 +233,12 @@ var _ = Describe("handleStream", func() {
 		Expect(w.Code).To(Equal(http.StatusInternalServerError))
 	})
 
-	It("skips share check for tokens without shareID (backward compat)", func() {
+	It("returns 400 for tokens without a shareID", func() {
 		claims := auth.Claims{ID: "mf-123"}
 		token, _ := auth.CreatePublicToken(claims)
 		w := makeRequest(token)
-		// Should get past share check, then fail on media file lookup (no mock data)
-		Expect(w.Code).To(Equal(http.StatusNotFound))
+		Expect(w.Code).To(Equal(http.StatusBadRequest))
+		Expect(streamer.called).To(BeFalse())
 	})
 
 	It("returns 400 for an invalid token", func() {