Merge branch 'master' into fix-artist-artwork-path-resolving

conf/configuration.go

@@ -2,14 +2,18 @@ package conf
 
 import (
 	"cmp"
+	"encoding"
 	"encoding/json"
 	"fmt"
 	"net/url"
 	"os"
 	"path/filepath"
+	"reflect"
+	"regexp"
 	"runtime"
 	"slices"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/bmatcuk/doublestar/v4"
@@ -21,11 +25,12 @@ import (
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/scheduler"
 	"github.com/navidrome/navidrome/utils/run"
+	"github.com/navidrome/navidrome/utils/slice"
 	"github.com/spf13/viper"
 )
 
 type configOptions struct {
-	ConfigFile                      string
+	ConfigFile                      string `conf:"-"`
 	Address                         string
 	Port                            int
 	UnixSocketPerm                  string
@@ -201,7 +206,7 @@ type lastfmOptions struct {
 	ScrobbleFirstArtistOnly bool
 
 	// Computed values
-	Languages []string // Computed from Language, split by comma
+	Languages []string `conf:"-"` // Computed from Language, split by comma
 }
 
 type deezerOptions struct {
@@ -209,7 +214,7 @@ type deezerOptions struct {
 	Language string
 
 	// Computed values
-	Languages []string // Computed from Language, split by comma
+	Languages []string `conf:"-"` // Computed from Language, split by comma
 }
 
 type listenBrainzOptions struct {
@@ -340,12 +345,11 @@ func Load(noConfigDump bool) {
 	remapEnvVarKeysFromConfig()
 
 	// Map deprecated options to their new names for backwards compatibility
-	mapDeprecatedOption("ReverseProxyWhitelist", "ExtAuth.TrustedSources")
-	mapDeprecatedOption("ReverseProxyUserHeader", "ExtAuth.UserHeader")
-	mapDeprecatedOption("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions")
-	mapDeprecatedOption("CoverJpegQuality", "CoverArtQuality")
-	mapDeprecatedOption("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold")
-	mapDeprecatedOption("EnableTranscodingCancellation", "Transcoding.EnableCancellation")
+	for _, o := range deprecatedOptions {
+		if o.replacement != "" {
+			mapDeprecatedOption(o.name, o.replacement)
+		}
+	}
 
 	err := viper.Unmarshal(&Server, viper.DecodeHook(
 		mapstructure.ComposeDecodeHookFunc(
@@ -403,6 +407,13 @@ func Load(noConfigDump bool) {
 	log.SetLogSourceLine(Server.DevLogSourceLine)
 	log.SetRedacting(Server.EnableLogRedacting)
 
+	// Log deprecated, removed and unknown options
+	for _, o := range deprecatedOptions {
+		logDeprecatedOptions(o.name, o.replacement)
+	}
+	logRemovedOptions(removedOptions...)
+	logUnknownOptions()
+
 	err = run.Sequentially(
 		validateScanSchedule,
 		validateBackupSchedule,
@@ -461,21 +472,6 @@ func Load(noConfigDump bool) {
 	// Parse Deezer.Language into Languages slice (comma-separated, with fallback to DefaultInfoLanguage)
 	Server.Deezer.Languages = parseLanguages(Server.Deezer.Language)
 
-	// Deprecated options
-	logDeprecatedOptions("Scanner.GenreSeparators", "")
-	logDeprecatedOptions("Scanner.GroupAlbumReleases", "")
-	logDeprecatedOptions("DevEnableBufferedScrobble", "") // Deprecated: Buffered scrobbling is now always enabled and this option is ignored
-	logDeprecatedOptions("SearchFullString", "Search.FullString")
-	logDeprecatedOptions("ReverseProxyWhitelist", "ExtAuth.TrustedSources")
-	logDeprecatedOptions("ReverseProxyUserHeader", "ExtAuth.UserHeader")
-	logDeprecatedOptions("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions")
-	logDeprecatedOptions("CoverJpegQuality", "CoverArtQuality")
-	logDeprecatedOptions("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold")
-	logDeprecatedOptions("EnableTranscodingCancellation", "Transcoding.EnableCancellation")
-
-	// Removed options
-	logRemovedOptions("Spotify.ID", "Spotify.Secret")
-
 	// Validate other options
 	if Server.UICoverArtSize < 200 || Server.UICoverArtSize > 1200 {
 		newValue := max(200, min(1200, Server.UICoverArtSize))
@@ -489,9 +485,26 @@ func Load(noConfigDump bool) {
 	}
 }
 
+// deprecatedOptions still work, but will be removed in a future release. An empty
+// replacement means the option is now ignored.
+var deprecatedOptions = []struct{ name, replacement string }{
+	{"Scanner.GenreSeparators", ""},
+	{"Scanner.GroupAlbumReleases", ""},
+	{"DevEnableBufferedScrobble", ""},
+	{"SearchFullString", "Search.FullString"},
+	{"ReverseProxyWhitelist", "ExtAuth.TrustedSources"},
+	{"ReverseProxyUserHeader", "ExtAuth.UserHeader"},
+	{"HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions"},
+	{"CoverJpegQuality", "CoverArtQuality"},
+	{"SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold"},
+	{"EnableTranscodingCancellation", "Transcoding.EnableCancellation"},
+}
+
+var removedOptions = []string{"Spotify.ID", "Spotify.Secret"}
+
 func logDeprecatedOptions(oldName, newName string) {
-	envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(oldName, ".", "_"))
-	newEnvVar := "ND_" + strings.ToUpper(strings.ReplaceAll(newName, ".", "_"))
+	envVar := envVarName(oldName)
+	newEnvVar := envVarName(newName)
 	logWarning := func(oldName, newName string) {
 		if newName != "" {
 			log.Warn(fmt.Sprintf("Option '%s' is deprecated and will be ignored in a future release. Please use the new '%s'", oldName, newName))
@@ -511,7 +524,7 @@ func logDeprecatedOptions(oldName, newName string) {
 // not available anymore
 func logRemovedOptions(options ...string) {
 	for _, option := range options {
-		envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_"))
+		envVar := envVarName(option)
 		logWarning := func(option string) {
 			log.Warn(fmt.Sprintf("Option '%s' is not available anymore and will be ignored. Please remove it from your config", option))
 		}
@@ -532,35 +545,193 @@ func remapEnvVarKeysFromConfig() {
 			continue
 		}
 		stripped := strings.TrimPrefix(key, "nd_")
-		canonicalKey := strings.ReplaceAll(stripped, "_", ".")
+		canonicalKey := ndKeyToCanonical(key)
 		displayNDKey := "ND_" + strings.ToUpper(stripped)
-		displayCanonical := toPascalCase(canonicalKey)
+		canonicalName := canonicalOptionName(canonicalKey)
 
 		if viper.InConfig(canonicalKey) {
 			logFatal(fmt.Sprintf(
 				"Config file contains both '%s' and '%s'. Remove the ND_-prefixed version. "+
 					"The 'ND_' prefix is only needed for environment variables, not config file keys.",
-				displayNDKey, displayCanonical,
+				displayNDKey, cmp.Or(canonicalName, toPascalCase(canonicalKey)),
 			))
 			return
 		}
 
 		viper.Set(canonicalKey, viper.Get(key))
-		_, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+
-			"The 'ND_' prefix is only needed for environment variables.\n",
-			displayNDKey, displayCanonical,
-		)
+		// Unknown keys get no advice here, logUnknownOptions reports them instead
+		if canonicalName != "" {
+			_, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+
+				"The 'ND_' prefix is only needed for environment variables.\n",
+				displayNDKey, canonicalName,
+			)
+		}
 	}
 }
 
 // mapDeprecatedOption is used to provide backwards compatibility for deprecated options. It should be called after
 // the config has been read by viper, but before unmarshalling it into the Config struct.
 func mapDeprecatedOption(legacyName, newName string) {
-	if viper.IsSet(legacyName) {
+	// viper.Set outranks the config file, so an explicit replacement must win over the legacy value
+	if viper.IsSet(legacyName) && !explicitlySet(newName) {
 		viper.Set(newName, viper.Get(legacyName))
 	}
 }
 
+// explicitlySet reports whether the user provided the option, ignoring defaults,
+// which viper.IsSet counts as set. The ND_ spelling is also accepted in the config
+// file, and remapEnvVarKeysFromConfig has already moved it out of InConfig's reach.
+func explicitlySet(name string) bool {
+	envVar := envVarName(name)
+	return viper.InConfig(name) || os.Getenv(envVar) != "" || viper.InConfig(strings.ToLower(envVar))
+}
+
+func envVarName(option string) string {
+	if option == "" {
+		return ""
+	}
+	return "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_"))
+}
+
+func logUnknownOptions() {
+	for _, key := range unknownConfigKeys() {
+		msg := fmt.Sprintf("Option '%s' is not recognized and will be ignored", key)
+		if matches := suggestOptions(key); len(matches) > 0 {
+			msg += fmt.Sprintf(". Did you mean '%s'?", strings.Join(matches, "' or '"))
+		}
+		log.Warn(msg)
+	}
+}
+
+// suggestOptions returns the known options sharing the last segment with key,
+// catching options written outside their section.
+func suggestOptions(key string) []string {
+	key = strings.ToLower(key)
+	leaf := leafKey(key)
+	canonical, _ := configKeys()
+	var matches []string
+	for known, name := range canonical {
+		// Removed options are known only so they get their own warning, never suggest them
+		if known != key && leafKey(known) == leaf && !slices.Contains(removedOptions, name) {
+			matches = append(matches, name)
+		}
+	}
+	slices.Sort(matches)
+	return matches
+}
+
+func leafKey(key string) string {
+	return key[strings.LastIndex(key, ".")+1:]
+}
+
+// unknownConfigKeys returns config file keys that don't match any known option, so
+// typos and options written outside their section don't fail silently.
+func unknownConfigKeys() []string {
+	// INI files keep the original [default] section alongside the merged one
+	skipDefault := strings.EqualFold(filepath.Ext(viper.ConfigFileUsed()), ".ini")
+
+	var unknown []string
+	for _, key := range viper.AllKeys() {
+		if !viper.InConfig(key) || canonicalOptionName(key) != "" {
+			continue
+		}
+		if skipDefault && strings.HasPrefix(key, "default.") {
+			continue
+		}
+		// Only ND_-prefixed keys that remapEnvVarKeysFromConfig could resolve are valid
+		if strings.HasPrefix(key, "nd_") && canonicalOptionName(ndKeyToCanonical(key)) != "" {
+			continue
+		}
+		unknown = append(unknown, key)
+	}
+	slices.Sort(unknown)
+	return asWrittenInConfigFile(unknown)
+}
+
+func ndKeyToCanonical(key string) string {
+	return strings.ReplaceAll(strings.TrimPrefix(key, "nd_"), "_", ".")
+}
+
+// canonicalOptionName returns the documented spelling of a known option key, or ""
+// if it matches no option. Subkeys of free-form maps have no fixed spelling.
+func canonicalOptionName(key string) string {
+	keys, prefixes := configKeys()
+	if name, ok := keys[key]; ok {
+		return name
+	}
+	if slices.ContainsFunc(prefixes, func(p string) bool { return strings.HasPrefix(key, p) }) {
+		return toPascalCase(key)
+	}
+	return ""
+}
+
+// asWrittenInConfigFile restores the casing the keys have in the config file, as
+// viper lowercases every key it loads.
+func asWrittenInConfigFile(keys []string) []string {
+	if len(keys) == 0 {
+		return nil
+	}
+	data, err := os.ReadFile(viper.ConfigFileUsed())
+	if err != nil {
+		return keys
+	}
+	casing := map[string]string{}
+	for _, match := range configFileKeyRx.FindAllStringSubmatch(string(data), -1) {
+		for segment := range strings.SplitSeq(match[1], ".") {
+			lower := strings.ToLower(segment)
+			casing[lower] = cmp.Or(casing[lower], segment)
+		}
+	}
+	return slice.Map(keys, func(key string) string {
+		segments := strings.Split(key, ".")
+		for i, s := range segments {
+			segments[i] = cmp.Or(casing[s], s)
+		}
+		return strings.Join(segments, ".")
+	})
+}
+
+// Matches keys and section headers in all supported config formats.
+var configFileKeyRx = regexp.MustCompile(`(?m)^\s*\[?\s*"?([\w.]+)"?\s*[]=:]`)
+
+// configKeys maps every accepted option name, lowercased, to its canonical spelling,
+// plus the prefixes of free-form map options (Tags, DevLogLevels).
+var configKeys = sync.OnceValues(func() (map[string]string, []string) {
+	keys := map[string]string{}
+	var prefixes []string
+
+	var collect func(t reflect.Type, prefix string)
+	collect = func(t reflect.Type, prefix string) {
+		for field := range t.Fields() {
+			// `conf:"-"` marks values computed during Load, not settable in the config
+			if !field.IsExported() || field.Tag.Get("conf") == "-" {
+				continue
+			}
+			name := prefix + field.Name
+			if field.Type.Kind() == reflect.Struct && !reflect.PointerTo(field.Type).Implements(textUnmarshalerType) {
+				collect(field.Type, name+".")
+				continue
+			}
+			lower := strings.ToLower(name)
+			keys[lower] = name
+			if field.Type.Kind() == reflect.Map {
+				prefixes = append(prefixes, lower+".")
+			}
+		}
+	}
+	collect(reflect.TypeFor[configOptions](), "")
+
+	for _, o := range deprecatedOptions {
+		keys[strings.ToLower(o.name)] = o.name
+	}
+	for _, o := range removedOptions {
+		keys[strings.ToLower(o)] = o
+	}
+	return keys, prefixes
+})
+
+var textUnmarshalerType = reflect.TypeFor[encoding.TextUnmarshaler]()
+
 // parseIniFileConfiguration is used to parse the config file when it is in INI format. For INI files, it
 // would require a nested structure, so instead we unmarshal it to a map and then merge the nested [default]
 // section into the root level.

conf/configuration_test.go

@@ -1,12 +1,14 @@
 package conf_test
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/log"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/spf13/viper"
@@ -178,6 +180,123 @@ var _ = Describe("Configuration", func() {
 		})
 	})
 
+	Describe("unknownConfigKeys", func() {
+		BeforeEach(func() {
+			viper.Reset()
+			conf.SetViperDefaults()
+			viper.SetDefault("datafolder", GinkgoT().TempDir())
+			viper.SetDefault("loglevel", "error")
+			conf.ResetConf()
+		})
+
+		It("reports misplaced and misspelled options, as spelled in the config file", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg_unknown_keys.toml"), false)
+			conf.Load(true)
+
+			Expect(conf.UnknownConfigKeys()).To(ConsistOf(
+				"ArtistSplitExceptions", "EnableDownlods", "Whatever.Foo",
+			))
+		})
+
+		DescribeTable("recovers the original casing in all supported formats",
+			func(file string) {
+				conf.InitConfig(filepath.Join("testdata", file), false)
+				conf.Load(true)
+
+				Expect(conf.UnknownConfigKeys()).To(ConsistOf("NotAnOption"))
+			},
+			Entry("TOML", "cfg_unknown_casing.toml"),
+			Entry("YAML", "cfg_unknown_casing.yaml"),
+			Entry("JSON", "cfg_unknown_casing.json"),
+			Entry("INI", "cfg_unknown_casing.ini"),
+		)
+
+		It("does not report valid, deprecated or free-form keys", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg.toml"), false)
+			conf.Load(true)
+
+			Expect(conf.UnknownConfigKeys()).To(BeEmpty())
+		})
+
+		It("does not report the [default] section of INI files", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg.ini"), false)
+			conf.Load(true)
+
+			Expect(conf.UnknownConfigKeys()).To(BeEmpty())
+		})
+
+		DescribeTable("SuggestOptions",
+			func(key string, expected []string) {
+				Expect(conf.SuggestOptions(key)).To(Equal(expected))
+			},
+			Entry("suggests the section of a misplaced option", "artistsplitexceptions",
+				[]string{"Scanner.ArtistSplitExceptions"}),
+			Entry("suggests the section of a misplaced nested option", "backup.fuzzythreshold",
+				[]string{"Matcher.FuzzyThreshold"}),
+			Entry("suggests every section defining the option", "schedule",
+				[]string{"Backup.Schedule", "Scanner.Schedule"}),
+			Entry("suggests nothing for a typo", "enabledownlods", nil),
+		)
+
+		It("does not report ND_-prefixed keys, as they are remapped", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg_nd_keys.toml"), false)
+			conf.Load(true)
+
+			Expect(conf.UnknownConfigKeys()).To(BeEmpty())
+		})
+
+		It("reports ND_-prefixed keys that remap to no known option", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg_nd_bogus.toml"), false)
+			conf.Load(true)
+
+			Expect(conf.UnknownConfigKeys()).To(ConsistOf("ND_TOTALLY_BOGUS_OPTION"))
+			Expect(conf.Server.Scanner.Schedule).To(Equal("@every 1h"))
+		})
+
+		It("migrates every deprecated option that has a replacement", func() {
+			conf.InitConfig(filepath.Join("testdata", "cfg_deprecated_search.toml"), false)
+			conf.Load(true)
+
+			Expect(conf.Server.Search.FullString).To(BeTrue())
+			Expect(conf.UnknownConfigKeys()).To(BeEmpty())
+		})
+
+		It("warns about each unrecognized option at startup", func() {
+			var logBuf bytes.Buffer
+			log.SetOutput(&logBuf)
+			DeferCleanup(func() { log.SetOutput(GinkgoWriter) })
+
+			conf.InitConfig(filepath.Join("testdata", "cfg_warning_output.toml"), false)
+			conf.Load(true)
+
+			Expect(logBuf.String()).To(ContainSubstring(
+				"Option 'ArtistSplitExceptions' is not recognized and will be ignored. " +
+					"Did you mean 'Scanner.ArtistSplitExceptions'?"))
+			Expect(logBuf.String()).To(ContainSubstring(
+				"Option 'EnableDownlods' is not recognized and will be ignored"))
+			Expect(logBuf.String()).ToNot(ContainSubstring("ArtistJoiner"))
+		})
+
+		Context("with runtime-computed and removed options in the config", func() {
+			BeforeEach(func() {
+				conf.InitConfig(filepath.Join("testdata", "cfg_runtime_fields.toml"), false)
+				conf.Load(true)
+			})
+
+			It("reports values computed during Load, which the config cannot set", func() {
+				Expect(conf.UnknownConfigKeys()).To(ContainElements("ConfigFile", "LastFM.Languages"))
+			})
+
+			It("never suggests a removed option", func() {
+				Expect(conf.SuggestOptions("id")).To(BeEmpty())
+			})
+
+			It("keeps an explicit replacement over the deprecated value", func() {
+				Expect(conf.Server.Search.FullString).To(BeFalse())
+			})
+		})
+	})
+
 	Describe("logFatal", func() {
 		var invalidPath string
 		BeforeEach(func() {

conf/export_test.go

@@ -32,3 +32,7 @@ func SetLogFatal(f func(...any)) func() {
 	logFatal = f
 	return func() { logFatal = old }
 }
+
+var UnknownConfigKeys = unknownConfigKeys
+
+var SuggestOptions = suggestOptions

conf/testdata/cfg_deprecated_search.toml

@@ -0,0 +1,2 @@
+MusicFolder = "/toml/music"
+SearchFullString = true

conf/testdata/cfg_nd_bogus.toml

@@ -0,0 +1,3 @@
+MusicFolder = "/toml/music"
+ND_TOTALLY_BOGUS_OPTION = true
+ND_SCANNER_SCHEDULE = "@every 1h"

conf/testdata/cfg_runtime_fields.toml

@@ -0,0 +1,10 @@
+MusicFolder = "/toml/music"
+SearchFullString = true
+ConfigFile = "/somewhere/else"
+ID = "oops"
+
+[Search]
+FullString = false
+
+[LastFM]
+Languages = ["pt"]

conf/testdata/cfg_unknown_casing.ini

@@ -0,0 +1,3 @@
+[default]
+MusicFolder = /ini/music
+NotAnOption = true

conf/testdata/cfg_unknown_casing.json

@@ -0,0 +1,4 @@
+{
+  "MusicFolder": "/json/music",
+  "NotAnOption": true
+}

conf/testdata/cfg_unknown_casing.toml

@@ -0,0 +1,2 @@
+MusicFolder = "/toml/music"
+NotAnOption = true

conf/testdata/cfg_unknown_casing.yaml

@@ -0,0 +1,2 @@
+MusicFolder: /yaml/music
+NotAnOption: true

conf/testdata/cfg_unknown_keys.toml

@@ -0,0 +1,18 @@
+MusicFolder = "/toml/music"
+
+# Valid option, but written at the root level instead of under Scanner
+ArtistSplitExceptions = ["AC/DC", "Tyler, the creator"]
+
+# Misspelled option
+EnableDownlods = true
+
+# Unknown section
+[Whatever]
+Foo = "bar"
+
+# Valid options, must not be reported
+[Scanner]
+ArtistJoiner = " • "
+
+[Tags.custom]
+aliases = ["toml", "test"]

conf/testdata/cfg_warning_output.toml

@@ -0,0 +1,7 @@
+MusicFolder = "/toml/music"
+LogLevel = "warn"
+ArtistSplitExceptions = ["AC/DC"]
+EnableDownlods = true
+
+[Scanner]
+ArtistJoiner = " • "

core/archiver.go

@@ -21,7 +21,7 @@ import (
 type Archiver interface {
 	ZipAlbum(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
 	ZipArtist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
-	ZipShare(ctx context.Context, id string, w io.Writer) error
+	ZipShare(ctx context.Context, s *model.Share, w io.Writer) error
 	ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
 }
 
@@ -100,16 +100,14 @@ func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc
 	return fmt.Sprintf("%s/%s", str.SanitizeFilename(mf.Album), file)
 }
 
-func (a *archiver) ZipShare(ctx context.Context, id string, out io.Writer) error {
-	s, err := a.shares.Load(ctx, id)
-	if err != nil {
-		return err
-	}
+// ZipShare takes an already-loaded share: Share.Load records a visit, so
+// loading it again here would count every download twice.
+func (a *archiver) ZipShare(ctx context.Context, s *model.Share, out io.Writer) error {
 	if !s.Downloadable {
 		return model.ErrNotAuthorized
 	}
 	log.Debug(ctx, "Zipping share", "name", s.ID, "format", s.Format, "bitrate", s.MaxBitRate, "numTracks", len(s.Tracks))
-	return a.zipMediaFiles(ctx, id, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false)
+	return a.zipMediaFiles(ctx, s.ID, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false)
 }
 
 func (a *archiver) ZipPlaylist(ctx context.Context, id string, format string, bitrate int, out io.Writer) error {

core/archiver_test.go

@@ -130,13 +130,16 @@ var _ = Describe("Archiver", func() {
 				Tracks:       mfs,
 			}
 
-			sh.On("Load", mock.Anything, "1").Return(share, nil)
 			ms.On("NewStream", mock.Anything, mock.Anything, stream.Request{Format: "mp3", BitRate: 128}).Return(io.NopCloser(strings.NewReader("test")), nil).Times(2)
 
 			out := new(bytes.Buffer)
-			err := arch.ZipShare(context.Background(), "1", out)
+			err := arch.ZipShare(context.Background(), share, out)
 			Expect(err).To(BeNil())
 
+			// Share.Load records a visit; re-loading here would double-count
+			// every download.
+			sh.AssertNotCalled(GinkgoT(), "Load", mock.Anything, mock.Anything)
+
 			zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len()))
 			Expect(err).To(BeNil())
 

server/public/handle_downloads.go

@@ -1,18 +1,41 @@
 package public
 
 import (
+	"cmp"
+	"fmt"
 	"net/http"
+	"strings"
 
+	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/utils/req"
+	"github.com/navidrome/navidrome/utils/str"
 )
 
 func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
 	id, err := req.Params(r).String(":id")
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
 
-	err = pub.archiver.ZipShare(r.Context(), id, w)
-	checkShareError(r.Context(), w, err, id)
+	// Load the share before streaming: once ZipShare writes its first byte the
+	// status is locked at 200, so errors could no longer be reported.
+	s, err := pub.share.Load(ctx, id)
+	if err != nil {
+		checkShareError(ctx, w, err, id)
+		return
+	}
+	if !s.Downloadable {
+		checkShareError(ctx, w, model.ErrNotAuthorized, id)
+		return
+	}
+
+	name := str.SanitizeFilename(cmp.Or(s.Description, s.ID))
+	name = strings.ReplaceAll(name, ",", "_")
+	w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip"))
+	w.Header().Set("Content-Type", "application/zip")
+
+	err = pub.archiver.ZipShare(ctx, s, w)
+	checkShareError(ctx, w, err, id)
 }

server/public/handle_downloads_test.go

@@ -0,0 +1,134 @@
+package public
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"time"
+
+	"github.com/navidrome/navidrome/core"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+type mockArchiver struct {
+	called bool
+	err    error
+}
+
+func (m *mockArchiver) ZipAlbum(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipArtist(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipPlaylist(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipShare(_ context.Context, _ *model.Share, w io.Writer) error {
+	m.called = true
+	if m.err != nil {
+		return m.err
+	}
+	_, _ = w.Write([]byte("zip-contents"))
+	return nil
+}
+
+var _ = Describe("handleDownloads", func() {
+	var ds *tests.MockDataStore
+	var shareRepo *tests.MockShareRepo
+	var archiver *mockArchiver
+	var pub *Router
+
+	BeforeEach(func() {
+		ds = &tests.MockDataStore{}
+		shareRepo = &tests.MockShareRepo{}
+		ds.MockedShare = shareRepo
+		archiver = &mockArchiver{}
+		pub = &Router{ds: ds, archiver: archiver, share: core.NewShare(ds)}
+	})
+
+	shareIs := func(s *model.Share) {
+		shareRepo.ID = s.ID
+		shareRepo.Entity = s
+	}
+
+	makeRequest := func(id string) *httptest.ResponseRecorder {
+		r := httptest.NewRequest("GET", "/public/d/"+id+"?%3Aid="+id, nil)
+		w := httptest.NewRecorder()
+		pub.handleDownloads(w, r)
+		return w
+	}
+
+	It("sets a Content-Disposition filename from the share description", func() {
+		shareIs(&model.Share{ID: "abc123", Description: "My Mixtape", Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusOK))
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"`))
+		Expect(w.Header().Get("Content-Type")).To(Equal("application/zip"))
+		Expect(archiver.called).To(BeTrue())
+		Expect(w.Body.String()).To(Equal("zip-contents"))
+	})
+
+	It("falls back to the share ID when there is no description", func() {
+		shareIs(&model.Share{ID: "abc123", Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`))
+	})
+
+	It("sanitizes characters that are unsafe in a filename", func() {
+		shareIs(&model.Share{ID: "abc123", Description: `AC/DC: Live, 1979`, Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`))
+	})
+
+	It("returns 403 without invoking the archiver when the share is not downloadable", func() {
+		shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusForbidden))
+		Expect(archiver.called).To(BeFalse())
+		Expect(w.Header().Get("Content-Disposition")).To(BeEmpty())
+	})
+
+	It("returns 404 when the share does not exist", func() {
+		shareIs(&model.Share{ID: "other", Downloadable: true})
+
+		w := makeRequest("missing")
+
+		Expect(w.Code).To(Equal(http.StatusNotFound))
+		Expect(archiver.called).To(BeFalse())
+	})
+
+	It("returns 410 when the share has expired", func() {
+		shareIs(&model.Share{ID: "abc123", Downloadable: true, ExpiresAt: new(time.Now().Add(-time.Hour))})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusGone))
+		Expect(archiver.called).To(BeFalse())
+	})
+
+	It("returns 500 when the share lookup fails", func() {
+		shareRepo.Error = errors.New("db error")
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusInternalServerError))
+		Expect(archiver.called).To(BeFalse())
+	})
+})

server/subsonic/e2e/e2e_suite_test.go

@@ -330,7 +330,7 @@ func (n noopArchiver) ZipArtist(context.Context, string, string, int, io.Writer)
 	return model.ErrNotFound
 }
 
-func (n noopArchiver) ZipShare(context.Context, string, io.Writer) error {
+func (n noopArchiver) ZipShare(context.Context, *model.Share, io.Writer) error {
 	return model.ErrNotFound
 }
 

ui/src/share/SharePlayer.jsx

@@ -1,15 +1,24 @@
 import ReactJkMusicPlayer from 'navidrome-music-player'
+import { useCallback, useEffect, useRef, useState } from 'react'
 import config, { shareInfo } from '../config'
 import { shareCoverUrl, shareDownloadUrl, shareStreamUrl } from '../utils'
 
 import { makeStyles } from '@material-ui/core/styles'
 
+// How long the download button stays inert after a click. The browser needs a
+// moment to show its own download UI; until then the page looks unresponsive.
+export const DOWNLOAD_FEEDBACK_MS = 5000
+
 const useStyle = makeStyles({
   player: {
     '& .group .next-audio': {
       pointerEvents: (props) => props.single && 'none',
       opacity: (props) => props.single && 0.65,
     },
+    '& .group.audio-download': {
+      pointerEvents: (props) => props.downloading && 'none',
+      opacity: (props) => props.downloading && 0.65,
+    },
     '@media (min-width: 768px)': {
       '& .react-jinke-music-player-mobile > div': {
         width: 768,
@@ -23,7 +32,14 @@ const useStyle = makeStyles({
 })
 
 const SharePlayer = () => {
-  const classes = useStyle({ single: shareInfo?.tracks.length === 1 })
+  const [downloading, setDownloading] = useState(false)
+  const timer = useRef(null)
+  const classes = useStyle({
+    single: shareInfo?.tracks.length === 1,
+    downloading,
+  })
+
+  useEffect(() => () => clearTimeout(timer.current), [])
 
   const list = shareInfo?.tracks.map((s) => {
     return {
@@ -34,11 +50,23 @@ const SharePlayer = () => {
       duration: s.duration,
     }
   })
-  const onBeforeAudioDownload = () => {
-    return Promise.resolve({
-      src: shareDownloadUrl(shareInfo?.id),
-    })
-  }
+  // An anchor, not a navigation: the service worker's NavigationRoute would
+  // intercept the streamed archive and fail it.
+  const customDownloader = useCallback(() => {
+    const link = document.createElement('a')
+    link.href = shareDownloadUrl(shareInfo?.id)
+    link.download = ''
+    document.body.appendChild(link)
+    link.click()
+    document.body.removeChild(link)
+
+    setDownloading(true)
+    clearTimeout(timer.current)
+    timer.current = setTimeout(
+      () => setDownloading(false),
+      DOWNLOAD_FEEDBACK_MS,
+    )
+  }, [])
   const options = {
     audioLists: list,
     mode: 'full',
@@ -59,7 +87,7 @@ const SharePlayer = () => {
     <ReactJkMusicPlayer
       {...options}
       className={classes.player}
-      onBeforeAudioDownload={onBeforeAudioDownload}
+      customDownloader={customDownloader}
     />
   )
 }

ui/src/share/SharePlayer.test.jsx

@@ -0,0 +1,139 @@
+import { render, act } from '@testing-library/react'
+import SharePlayer, { DOWNLOAD_FEEDBACK_MS } from './SharePlayer'
+
+let playerProps
+let renderCount
+
+vi.mock('navidrome-music-player', () => ({
+  default: (props) => {
+    playerProps = props
+    renderCount++
+    return <div data-testid="player" />
+  },
+}))
+
+vi.mock('../config', () => ({
+  default: { enableDownloads: true },
+  shareInfo: {
+    id: 'share-1',
+    downloadable: true,
+    tracks: [{ id: 't1', title: 'One', artist: 'A', duration: 100 }],
+  },
+}))
+
+vi.mock('../utils', () => ({
+  shareDownloadUrl: (id) => `/share/d/${id}`,
+  shareStreamUrl: (id) => `/share/s/${id}`,
+  shareCoverUrl: (id) => `/share/img/${id}`,
+}))
+
+describe('SharePlayer', () => {
+  let clickSpy
+
+  beforeEach(() => {
+    vi.useFakeTimers()
+    playerProps = null
+    renderCount = 0
+    // Downloading for real would navigate the jsdom window.
+    clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, 'click')
+      .mockImplementation(() => {})
+  })
+
+  afterEach(() => {
+    vi.runOnlyPendingTimers()
+    vi.useRealTimers()
+    vi.restoreAllMocks()
+  })
+
+  it('downloads via an anchor so the service worker does not intercept it', () => {
+    render(<SharePlayer />)
+
+    let anchor
+    clickSpy.mockImplementation(function () {
+      anchor = { href: this.href, download: this.download }
+    })
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    expect(clickSpy).toHaveBeenCalledTimes(1)
+    expect(anchor.href).toContain('/share/d/share-1')
+    // Empty, so the server's Content-Disposition filename wins.
+    expect(anchor.download).toBe('')
+  })
+
+  it('removes the anchor from the document after clicking', () => {
+    render(<SharePlayer />)
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    expect(document.querySelectorAll('a[download]')).toHaveLength(0)
+  })
+
+  // The inert styling itself is driven by JSS function values, which jsdom does
+  // not evaluate; it is verified in a browser. What is checked here is the
+  // state machine feeding it -- that the component re-renders on download and
+  // again when the window closes.
+  it('re-renders when the feedback window opens and closes', () => {
+    render(<SharePlayer />)
+
+    const beforeDownload = renderCount
+    act(() => {
+      playerProps.customDownloader()
+    })
+    expect(renderCount).toBeGreaterThan(beforeDownload)
+
+    const beforeExpiry = renderCount
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(renderCount).toBeGreaterThan(beforeExpiry)
+  })
+
+  it('restarts the feedback window on a repeat download', () => {
+    render(<SharePlayer />)
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+    const elapsedBeforeRepeat = Math.floor(DOWNLOAD_FEEDBACK_MS / 2)
+    act(() => {
+      vi.advanceTimersByTime(elapsedBeforeRepeat)
+    })
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    // Past the first timer's deadline, which it would have fired at had the
+    // repeat download not replaced it.
+    const beforeOriginalDeadline = renderCount
+    act(() => {
+      vi.advanceTimersByTime(DOWNLOAD_FEEDBACK_MS - elapsedBeforeRepeat + 1)
+    })
+    expect(renderCount).toBe(beforeOriginalDeadline)
+
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(renderCount).toBeGreaterThan(beforeOriginalDeadline)
+  })
+
+  it('does not update state after unmount', () => {
+    const { unmount } = render(<SharePlayer />)
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+    unmount()
+    act(() => {
+      vi.runAllTimers()
+    })
+
+    expect(errorSpy).not.toHaveBeenCalled()
+  })
+})