diff --git a/.gitignore b/.gitignore index fc8eaac69..3567a7d90 100644 --- a/.gitignore +++ b/.gitignore @@ -40,3 +40,4 @@ openspec/ .agents go.work* .worktrees/ +.playwright-mcp/ diff --git a/cmd/root.go b/cmd/root.go index 9e2b38cd8..d9e766c32 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -394,7 +394,7 @@ func init() { rootCmd.Flags().String("albumplaycountmode", viper.GetString("albumplaycountmode"), "how to compute playcount for albums. absolute (default) or normalized") rootCmd.Flags().Bool("autoimportplaylists", viper.GetBool("autoimportplaylists"), "enable/disable .m3u playlist auto-import`") - rootCmd.Flags().Bool("prometheus.enabled", viper.GetBool("prometheus.enabled"), "enable/disable prometheus metrics endpoint`") + rootCmd.Flags().Bool("prometheus.enabled", viper.GetBool("prometheus.enabled"), "enable/disable prometheus metrics endpoint") rootCmd.Flags().String("prometheus.metricspath", viper.GetString("prometheus.metricspath"), "http endpoint for prometheus metrics") _ = viper.BindPFlag("address", rootCmd.Flags().Lookup("address")) diff --git a/conf/configuration.go b/conf/configuration.go index 83793bd43..9b49fb264 100644 --- a/conf/configuration.go +++ b/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. diff --git a/conf/configuration_test.go b/conf/configuration_test.go index e43c91a4b..4eaa8e3d8 100644 --- a/conf/configuration_test.go +++ b/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() { diff --git a/conf/export_test.go b/conf/export_test.go index acebca551..cbb64b3d0 100644 --- a/conf/export_test.go +++ b/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 diff --git a/conf/testdata/cfg_deprecated_search.toml b/conf/testdata/cfg_deprecated_search.toml new file mode 100644 index 000000000..cc6541e09 --- /dev/null +++ b/conf/testdata/cfg_deprecated_search.toml @@ -0,0 +1,2 @@ +MusicFolder = "/toml/music" +SearchFullString = true diff --git a/conf/testdata/cfg_nd_bogus.toml b/conf/testdata/cfg_nd_bogus.toml new file mode 100644 index 000000000..841b40998 --- /dev/null +++ b/conf/testdata/cfg_nd_bogus.toml @@ -0,0 +1,3 @@ +MusicFolder = "/toml/music" +ND_TOTALLY_BOGUS_OPTION = true +ND_SCANNER_SCHEDULE = "@every 1h" diff --git a/conf/testdata/cfg_runtime_fields.toml b/conf/testdata/cfg_runtime_fields.toml new file mode 100644 index 000000000..a9c09f619 --- /dev/null +++ b/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"] diff --git a/conf/testdata/cfg_unknown_casing.ini b/conf/testdata/cfg_unknown_casing.ini new file mode 100644 index 000000000..c88db9b1f --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.ini @@ -0,0 +1,3 @@ +[default] +MusicFolder = /ini/music +NotAnOption = true diff --git a/conf/testdata/cfg_unknown_casing.json b/conf/testdata/cfg_unknown_casing.json new file mode 100644 index 000000000..cd3a226cd --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.json @@ -0,0 +1,4 @@ +{ + "MusicFolder": "/json/music", + "NotAnOption": true +} diff --git a/conf/testdata/cfg_unknown_casing.toml b/conf/testdata/cfg_unknown_casing.toml new file mode 100644 index 000000000..f015c9cb2 --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.toml @@ -0,0 +1,2 @@ +MusicFolder = "/toml/music" +NotAnOption = true diff --git a/conf/testdata/cfg_unknown_casing.yaml b/conf/testdata/cfg_unknown_casing.yaml new file mode 100644 index 000000000..8987f6351 --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.yaml @@ -0,0 +1,2 @@ +MusicFolder: /yaml/music +NotAnOption: true diff --git a/conf/testdata/cfg_unknown_keys.toml b/conf/testdata/cfg_unknown_keys.toml new file mode 100644 index 000000000..a7ed55cb0 --- /dev/null +++ b/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"] diff --git a/conf/testdata/cfg_warning_output.toml b/conf/testdata/cfg_warning_output.toml new file mode 100644 index 000000000..a52c5ea54 --- /dev/null +++ b/conf/testdata/cfg_warning_output.toml @@ -0,0 +1,7 @@ +MusicFolder = "/toml/music" +LogLevel = "warn" +ArtistSplitExceptions = ["AC/DC"] +EnableDownlods = true + +[Scanner] +ArtistJoiner = " • " diff --git a/consts/consts.go b/consts/consts.go index f453ac125..e1a535c79 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -28,6 +28,7 @@ const ( UIAuthorizationHeader = "X-ND-Authorization" UIClientUniqueIDHeader = "X-ND-Client-Unique-Id" JWTSecretKey = "JWTSecret" + JWTPublicSecretKey = "JWTPublicSecret" JWTIssuer = "ND" DefaultSessionTimeout = 48 * time.Hour CookieExpiry = 365 * 24 * 3600 // One year diff --git a/core/archiver.go b/core/archiver.go index 5d1c090cd..8c42f8f49 100644 --- a/core/archiver.go +++ b/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 { diff --git a/core/archiver_test.go b/core/archiver_test.go index f432139d8..2ba8f1fc0 100644 --- a/core/archiver_test.go +++ b/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()) diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index c95371959..5bcd82f23 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -202,6 +202,7 @@ var _ = Describe("Artwork", func() { repoRoot, err := os.Getwd() Expect(err).ToNot(HaveOccurred()) folderRepo.result = []model.Folder{{ + ID: "f1", LibraryPath: testFileLibPath(repoRoot), Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"artist.png"}, diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go index d959b1d60..2079d37ce 100644 --- a/core/artwork/e2e/artist_test.go +++ b/core/artwork/e2e/artist_test.go @@ -80,6 +80,101 @@ var _ = Describe("Artist artwork resolution", func() { }) }) + When("ArtistArtPriority has no album/ fallback", func() { + // Artist/ + // ├── artist.jpg ← must resolve via the artist folder itself + // └── Album/ + // └── 01 - Track.mp3 + It("still resolves the artist folder and returns artist.*", func() { + conf.Server.ArtistArtPriority = "artist.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), + "Artist/artist.jpg": imageFile("artist-folder"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("the artist's only album has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins (artist.* before album/artist.*) + // └── Album/ + // ├── artist.jpg + // ├── CD1/01 - Track.mp3 + // └── CD2/02 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/Album/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("one album has disc subfolders and another sits at artist level", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/03 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album2/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album1/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("every album of the artist has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/ + // ├── CD1/03 - Track.mp3 + // └── CD2/04 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album2/CD1/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/Album2/CD2/04 - Track.mp3": trackFile(4, "Track 4", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album1/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + When("an artist has an uploaded image and a matching artist.* file", func() { // / // └── artwork/ diff --git a/core/artwork/e2e/helpers_test.go b/core/artwork/e2e/helpers_test.go index e3abca097..ddc926cec 100644 --- a/core/artwork/e2e/helpers_test.go +++ b/core/artwork/e2e/helpers_test.go @@ -171,7 +171,7 @@ func (n *noopProvider) UpdateArtistInfo(context.Context, string, int, bool) (*mo func (n *noopProvider) SimilarSongs(context.Context, string, int) (model.MediaFiles, error) { return nil, nil } -func (n *noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, error) { +func (n *noopProvider) TopSongs(context.Context, string, string, int) (model.MediaFiles, error) { return nil, nil } func (n *noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 8ad07773b..58a47c8ae 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -20,6 +20,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" + "github.com/navidrome/navidrome/utils/slice" ) type albumArtworkReader struct { @@ -103,17 +104,13 @@ func (a *albumArtworkReader) fromCoverArtPriority(ctx context.Context, ffmpeg ff return ff } -func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...model.Album) ([]string, []string, *time.Time, error) { - var folderIDs []string - for _, album := range albums { - folderIDs = append(folderIDs, album.FolderIDs...) - } - folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, album model.Album) ([]string, []string, *time.Time, error) { + folders, err := loadFolders(ctx, ds, album.FolderIDs) if err != nil { return nil, nil, nil, err } - parent, err := albumRootParent(ctx, ds, folders, folderIDs) + parent, err := albumRootParent(ctx, ds, folders, album.FolderIDs) if err != nil { return nil, nil, nil, err } @@ -121,11 +118,21 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo folders = append(folders, *parent) } - var paths []string + paths := slice.Map(folders, func(f model.Folder) string { return f.AbsolutePath() }) + imgFiles, updatedAt := folderImages(folders) + return paths, imgFiles, &updatedAt, nil +} + +func loadFolders(ctx context.Context, ds model.DataStore, folderIDs []string) ([]model.Folder, error) { + return ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +} + +// folderImages collects the folders' image files, sorted so files without +// numeric suffixes win (e.g. cover.jpg over cover.1.jpg). +func folderImages(folders []model.Folder) ([]string, time.Time) { var imgFiles []string var updatedAt time.Time for _, f := range folders { - paths = append(paths, f.AbsolutePath()) if f.ImagesUpdatedAt.After(updatedAt) { updatedAt = f.ImagesUpdatedAt } @@ -134,13 +141,8 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo imgFiles = append(imgFiles, path.Join(rel, img)) } } - - // Sort image files to ensure consistent selection of cover art - // This prioritizes files without numeric suffixes (e.g., cover.jpg over cover.1.jpg) - // by comparing base filenames without extensions slices.SortFunc(imgFiles, compareImageFiles) - - return paths, imgFiles, &updatedAt, nil + return imgFiles, updatedAt } // albumRootParent returns the common parent of the album's folders when it diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index fe4a1a545..c95ddedaa 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -3,6 +3,7 @@ package artwork import ( "context" "errors" + "path/filepath" "time" "github.com/navidrome/navidrome/model" @@ -339,6 +340,32 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("promotes the album root parent into the returned paths", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "artistFolder", + Path: ".", + Name: "Artist", + ParentID: "libraryRoot", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"folder.jpg"}, + } + + paths, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(paths).To(Equal([]string{filepath.Join("Artist", "Album"), "Artist"})) + }) + It("does not include parent images when other albums' audio lives under the parent", func() { // Simulates: Artist/folder.jpg with Artist/Album (no images) and // another album's tracks elsewhere under the artist folder diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 37b7b6dee..fe5bcb196 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -19,6 +19,7 @@ import ( "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" ) @@ -54,7 +55,7 @@ func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.A if err != nil { return nil, err } - albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, als...) + albumPaths, imgFiles, imagesUpdatedAt, err := loadArtistAlbumRoots(ctx, artwork.ds, als) if err != nil { return nil, err } @@ -232,17 +233,64 @@ func escapeGlobLiteral(s string) string { return b.String() } +// loadArtistAlbumRoots returns one path per album — the deepest folder holding +// all of that album's tracks — so an album split into disc subfolders can't +// pull the artist folder's common prefix below the artist level. +func loadArtistAlbumRoots(ctx context.Context, ds model.DataStore, albums model.Albums) ([]string, []string, *time.Time, error) { + var folderIDs []string + for _, album := range albums { + folderIDs = append(folderIDs, album.FolderIDs...) + } + folders, err := loadFolders(ctx, ds, folderIDs) + if err != nil { + return nil, nil, nil, err + } + + pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) { + return f.ID, f.AbsolutePath() + }) + var roots []string + for _, album := range albums { + var albumPaths []string + for _, fid := range album.FolderIDs { + if p, ok := pathByID[fid]; ok { + albumPaths = append(albumPaths, p) + } + } + if len(albumPaths) > 0 { + roots = append(roots, commonDir(albumPaths)) + } + } + + imgFiles, updatedAt := folderImages(folders) + return roots, imgFiles, &updatedAt, nil +} + +// commonDir returns the deepest directory containing all paths. Trailing +// separators keep the comparison on segment boundaries, so a shared name +// fragment (".../Album" and ".../Album2") is never read as a shared directory. +func commonDir(paths []string) string { + sep := string(filepath.Separator) + common := str.LongestCommonPrefix(slice.Map(paths, func(p string) string { return p + sep })) + if !strings.HasSuffix(common, sep) { + common, _ = filepath.Split(common) + } + return filepath.Clean(common) +} + func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) { if len(albums) == 0 { return "", time.Time{}, nil } libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries - folderPath := str.LongestCommonPrefix(paths) - if !strings.HasSuffix(folderPath, string(filepath.Separator)) { - folderPath, _ = filepath.Split(folderPath) + // paths holds one root per album: two or more distinct roots already meet at + // the artist folder, while a single root is an album folder needing a climb. + roots := slices.Compact(slices.Sorted(slices.Values(paths))) + folderPath := commonDir(roots) + if len(roots) < 2 { + folderPath = filepath.Dir(folderPath) } - folderPath = filepath.Dir(folderPath) // Manipulate the path to get the folder ID // TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 6d6d58fc5..c75f8447e 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -85,6 +85,19 @@ var _ = Describe("artistArtworkReader", func() { }) }) + When("two albums share the same folder", func() { + It("climbs above the shared album folder", func() { + paths = []string{ + filepath.FromSlash("/music/artist/split"), + filepath.FromSlash("/music/artist/split"), + } + folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) + Expect(err).ToNot(HaveOccurred()) + Expect(folder).To(Equal(filepath.FromSlash("/music/artist"))) + Expect(upd).To(Equal(expectedUpdTime)) + }) + }) + When("the album paths contain same prefix", func() { It("returns the common prefix", func() { paths = []string{ diff --git a/core/auth/auth.go b/core/auth/auth.go index 7b3511bdf..b1e2667bd 100644 --- a/core/auth/auth.go +++ b/core/auth/auth.go @@ -19,35 +19,41 @@ import ( ) var ( - once sync.Once + once sync.Once + // TokenAuth signs UI/API session tokens. Rotated by the id migration so stale sessions die. TokenAuth *jwtauth.JWTAuth + // PublicTokenAuth signs public-link tokens (artwork, share streams), on a separate secret that survives the rotation. + PublicTokenAuth *jwtauth.JWTAuth ) -// Init creates a JWTAuth object from the secret stored in the DB. -// If the secret is not found, it will create a new one and store it in the DB. +// Init creates the JWTAuth objects from the secrets stored in the DB. +// Missing or undecryptable secrets are regenerated and stored. func Init(ds model.DataStore) { once.Do(func() { ctx := context.TODO() log.Info("Setting Session Timeout", "value", conf.Server.SessionTimeout) - secret, err := ds.Property(ctx).Get(consts.JWTSecretKey) - if err != nil || secret == "" { - log.Info(ctx, "Creating new JWT secret, used for encrypting UI sessions") - secret = createNewSecret(ctx, ds) - } else { - if secret, err = utils.Decrypt(ctx, getEncKey(), secret); err != nil { - log.Error(ctx, "Could not decrypt JWT secret, creating a new one", err) - secret = createNewSecret(ctx, ds) - } - } - - TokenAuth = jwtauth.New("HS256", []byte(secret), nil) + TokenAuth = jwtauth.New("HS256", []byte(loadOrCreateSecret(ctx, ds, consts.JWTSecretKey)), nil) + PublicTokenAuth = jwtauth.New("HS256", []byte(loadOrCreateSecret(ctx, ds, consts.JWTPublicSecretKey)), nil) }) } +func loadOrCreateSecret(ctx context.Context, ds model.DataStore, key string) string { + secret, err := ds.Property(ctx).Get(key) + if err != nil || secret == "" { + log.Info(ctx, "Creating new JWT secret", "key", key) + return createNewSecret(ctx, ds, key) + } + if secret, err = utils.Decrypt(ctx, getEncKey(), secret); err != nil { + log.Error(ctx, "Could not decrypt JWT secret, creating a new one", "key", key, err) + return createNewSecret(ctx, ds, key) + } + return secret +} + func CreatePublicToken(claims Claims) (string, error) { claims.Issuer = consts.JWTIssuer - _, token, err := TokenAuth.Encode(claims.ToMap()) + _, token, err := PublicTokenAuth.Encode(claims.ToMap()) return token, err } @@ -56,7 +62,7 @@ func CreateExpiringPublicToken(exp time.Time, claims Claims) (string, error) { if !exp.IsZero() { claims.ExpiresAt = exp } - _, token, err := TokenAuth.Encode(claims.ToMap()) + _, token, err := PublicTokenAuth.Encode(claims.ToMap()) return token, err } @@ -91,6 +97,15 @@ func Validate(tokenStr string) (Claims, error) { return ClaimsFromToken(token), nil } +// ValidatePublic verifies a public-link token against the public secret. +func ValidatePublic(tokenStr string) (Claims, error) { + token, err := jwtauth.VerifyToken(PublicTokenAuth, tokenStr) + if err != nil { + return Claims{}, err + } + return ClaimsFromToken(token), nil +} + func WithAdminUser(ctx context.Context, ds model.DataStore) context.Context { u, err := ds.User(ctx).FindFirstAdmin() if err != nil { @@ -107,14 +122,14 @@ func WithAdminUser(ctx context.Context, ds model.DataStore) context.Context { return request.WithUser(ctx, *u) } -func createNewSecret(ctx context.Context, ds model.DataStore) string { +func createNewSecret(ctx context.Context, ds model.DataStore, key string) string { secret := id.NewRandom() encSecret, err := utils.Encrypt(ctx, getEncKey(), secret) if err != nil { log.Error(ctx, "Could not encrypt JWT secret", err) return secret } - if err := ds.Property(ctx).Put(consts.JWTSecretKey, encSecret); err != nil { + if err := ds.Property(ctx).Put(key, encSecret); err != nil { log.Error(ctx, "Could not save JWT secret in DB", err) } return secret diff --git a/core/auth/auth_test.go b/core/auth/auth_test.go index 3a3585e53..e5cbb2352 100644 --- a/core/auth/auth_test.go +++ b/core/auth/auth_test.go @@ -89,6 +89,51 @@ var _ = Describe("Auth", func() { }) }) + Describe("Session/Public secret split", func() { + claims := func() map[string]any { + return map[string]any{"iss": "issuer", "exp": time.Now().Add(1 * time.Minute).Unix()} + } + + It("verifies a session token via Validate but not ValidatePublic", func() { + _, tokenStr, err := auth.TokenAuth.Encode(claims()) + Expect(err).NotTo(HaveOccurred()) + _, err = auth.Validate(tokenStr) + Expect(err).NotTo(HaveOccurred()) + _, err = auth.ValidatePublic(tokenStr) + Expect(err).To(HaveOccurred()) + }) + + It("verifies a public token via ValidatePublic but not Validate", func() { + _, tokenStr, err := auth.PublicTokenAuth.Encode(claims()) + Expect(err).NotTo(HaveOccurred()) + _, err = auth.ValidatePublic(tokenStr) + Expect(err).NotTo(HaveOccurred()) + _, err = auth.Validate(tokenStr) + Expect(err).To(HaveOccurred()) + }) + + It("decodes public tokens minted by CreatePublicToken via PublicTokenAuth", func() { + tokenStr, err := auth.CreatePublicToken(auth.Claims{ID: "art-1"}) + Expect(err).NotTo(HaveOccurred()) + claims, err := auth.ValidatePublic(tokenStr) + Expect(err).NotTo(HaveOccurred()) + Expect(claims.ID).To(Equal("art-1")) + _, err = auth.Validate(tokenStr) + Expect(err).To(HaveOccurred()) + }) + + It("decodes expiring public tokens minted by CreateExpiringPublicToken via PublicTokenAuth", func() { + exp := time.Now().Add(1 * time.Hour) + tokenStr, err := auth.CreateExpiringPublicToken(exp, auth.Claims{ID: "art-2"}) + Expect(err).NotTo(HaveOccurred()) + claims, err := auth.ValidatePublic(tokenStr) + Expect(err).NotTo(HaveOccurred()) + Expect(claims.ID).To(Equal("art-2")) + _, err = auth.Validate(tokenStr) + Expect(err).To(HaveOccurred()) + }) + }) + Describe("TouchToken", func() { It("updates the expiration time", func() { yesterday := time.Now().Add(-oneDay) diff --git a/core/external/provider.go b/core/external/provider.go index 459e8a205..869841e06 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -34,7 +34,7 @@ type Provider interface { UpdateAlbumInfo(ctx context.Context, id string) (*model.Album, error) UpdateArtistInfo(ctx context.Context, id string, count int, includeNotPresent bool) (*model.Artist, error) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) - TopSongs(ctx context.Context, artist string, count int) (model.MediaFiles, error) + TopSongs(ctx context.Context, artist, artistId string, count int) (model.MediaFiles, error) ArtistImage(ctx context.Context, id string) (*url.URL, error) AlbumImage(ctx context.Context, id string) (*url.URL, error) } @@ -440,11 +440,16 @@ func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error) return url.Parse(img.URL) } -func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (model.MediaFiles, error) { - artist, err := e.findArtistByName(ctx, artistName) +func (e *provider) TopSongs(ctx context.Context, artistName, id string, count int) (model.MediaFiles, error) { + artist, err := e.findArtist(ctx, artistName, id) if err != nil { - log.Error(ctx, "Artist not found", "name", artistName, err) - return nil, nil + if errors.Is(err, model.ErrNotFound) { + log.Error(ctx, "Artist not found", "name", artistName, "id", id, err) + return nil, nil + } + + log.Error(ctx, "Failure occurred when trying to fetch artist", "name", artistName, "id", id, err) + return nil, err } songs, err := e.getMatchingTopSongs(ctx, e.ag, artist, count) @@ -713,7 +718,24 @@ func (e *provider) loadArtistsByName(ctx context.Context, similar []agents.Artis return matches, nil } -func (e *provider) findArtistByName(ctx context.Context, artistName string) (*auxArtist, error) { +func (e *provider) findArtist(ctx context.Context, artistName, id string) (*auxArtist, error) { + if id != "" { + artist, err := e.ds.Artist(ctx).Get(id) + if err == nil { + return &auxArtist{Artist: *artist}, nil + } + + if errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Could not find artist by id", "id", id, err) + } else { + return nil, err + } + } + + if artistName == "" { + return nil, model.ErrNotFound + } + artists, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ Filters: squirrel.Like{"artist.name": artistName}, Max: 1, diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 86f9110e2..795a7ff74 100644 --- a/core/external/provider_topsongs_test.go +++ b/core/external/provider_topsongs_test.go @@ -65,7 +65,7 @@ var _ = Describe("Provider - TopSongs", func() { song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "mbid-song-2"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -76,6 +76,59 @@ var _ = Describe("Provider - TopSongs", func() { mediaFileRepo.AssertExpectations(GinkgoT()) }) + Context("artist id and/or name", func() { + artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} + + BeforeEach(func() { + agentSongs := []agents.Song{{Name: "Song One", MBID: "mbid-song-1"}} + ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once() + + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"} + mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() + }) + + It("returns top songs for a known artist by id only", func() { + artistRepo.On("Get", "artist-1").Return(&artist1, nil).Once() + + songs, err := p.TopSongs(ctx, "", "artist-1", 2) + + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(HaveLen(1)) + Expect(songs[0].ID).To(Equal("song-1")) + artistRepo.AssertExpectations(GinkgoT()) + ag.AssertExpectations(GinkgoT()) + mediaFileRepo.AssertExpectations(GinkgoT()) + }) + + It("returns top songs for a known artist by id, falling back to name", func() { + artistRepo.On("Get", "fake-id").Return(nil, model.ErrNotFound).Once() + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + + songs, err := p.TopSongs(ctx, "Artist One", "fake-id", 2) + + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(HaveLen(1)) + Expect(songs[0].ID).To(Equal("song-1")) + artistRepo.AssertExpectations(GinkgoT()) + ag.AssertExpectations(GinkgoT()) + mediaFileRepo.AssertExpectations(GinkgoT()) + }) + }) + + It("bails if lookup by id returns a fatal error", func() { + artistRepo.On("Get", "artist-1").Return(nil, model.ErrInvalidAuth).Once() + songs, err := p.TopSongs(ctx, "Artist One", "artist-1", 2) + Expect(songs).To(BeEmpty()) + Expect(err).To(HaveOccurred()) + }) + + It("bails if lookup by name returns a fatal error", func() { + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(nil, model.ErrInvalidAuth).Once() + songs, err := p.TopSongs(ctx, "Artist One", "", 2) + Expect(songs).To(BeEmpty()) + Expect(err).To(HaveOccurred()) + }) + It("backfills name and MBID onto an unnamed primary credit (the queried artist) and matches", func() { artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil) @@ -96,7 +149,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{track}, nil) - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -116,7 +169,8 @@ var _ = Describe("Provider - TopSongs", func() { ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 1).Return(agentSongs, nil).Once() // Library track is credited to Artist One (the queried artist) under a same title. If the - // queried MBID were wrongly stamped onto the "Artist Two" credit, that mismatched name+MBID + // queried MBID were wrongly stamped onto the "Artist Two" credit, that mismatched name+MBID artistRepo.On("Get", "fake-id").Return(nil, model.ErrNotFound).Once() + // could mis-resolve. With the guard, "Artist Two" stays MBID-less and does not match One's track. track := model.MediaFile{ ID: "one-track", Title: "Collab Song", ArtistID: "artist-1", @@ -126,7 +180,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{track}, nil) - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) // "Artist Two" (named, MBID-less, not in the library) does not resolve to One's track. @@ -137,7 +191,7 @@ var _ = Describe("Provider - TopSongs", func() { // Mock artist not found artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{}, nil).Once() - songs, err := p.TopSongs(ctx, "Unknown Artist", 5) + songs, err := p.TopSongs(ctx, "Unknown Artist", "", 5) Expect(err).ToNot(HaveOccurred()) // TopSongs returns nil error if artist not found Expect(songs).To(BeNil()) @@ -154,7 +208,7 @@ var _ = Describe("Provider - TopSongs", func() { agentErr := errors.New("agent error") ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, agentErr).Once() - songs, err := p.TopSongs(ctx, "Artist One", 5) + songs, err := p.TopSongs(ctx, "Artist One", "", 5) Expect(err).To(MatchError(agentErr)) Expect(songs).To(BeNil()) @@ -170,7 +224,7 @@ var _ = Describe("Provider - TopSongs", func() { // Mock agent ErrNotFound ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, agents.ErrNotFound).Once() - songs, err := p.TopSongs(ctx, "Artist One", 5) + songs, err := p.TopSongs(ctx, "Artist One", "", 5) Expect(err).To(MatchError(model.ErrNotFound)) Expect(songs).To(BeNil()) @@ -191,7 +245,7 @@ var _ = Describe("Provider - TopSongs", func() { song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -220,7 +274,7 @@ var _ = Describe("Provider - TopSongs", func() { mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() // bulk MBID query mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() // title track-fetch for song2: no match - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -242,7 +296,7 @@ var _ = Describe("Provider - TopSongs", func() { ag.On("GetArtistTopSongs", canceledCtx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, context.Canceled).Once() cancel() // Cancel the context before calling - songs, err := p.TopSongs(canceledCtx, "Artist One", 5) + songs, err := p.TopSongs(canceledCtx, "Artist One", "", 5) Expect(err).To(MatchError(context.Canceled)) Expect(songs).To(BeNil()) @@ -276,7 +330,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -313,7 +367,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -341,7 +395,7 @@ var _ = Describe("Provider - TopSongs", func() { song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "mbid-song-2"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -369,7 +423,7 @@ var _ = Describe("Provider - TopSongs", func() { song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -397,7 +451,7 @@ var _ = Describe("Provider - TopSongs", func() { song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) diff --git a/core/publicurl/publicurl_test.go b/core/publicurl/publicurl_test.go index a195fb9cd..7e9ee8b8e 100644 --- a/core/publicurl/publicurl_test.go +++ b/core/publicurl/publicurl_test.go @@ -175,7 +175,7 @@ var _ = Describe("Public URL Utilities", func() { BeforeEach(func() { conf.Server.ShareURL = "https://share.example.com" // Initialize JWT auth for token generation - auth.TokenAuth = jwtauth.New("HS256", []byte("test secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test secret"), nil) }) It("generates a URL with the artwork token", func() { diff --git a/db/migrations/20260720015443_uniform_canonical_ids.go b/db/migrations/20260720015443_uniform_canonical_ids.go new file mode 100644 index 000000000..346f22050 --- /dev/null +++ b/db/migrations/20260720015443_uniform_canonical_ids.go @@ -0,0 +1,310 @@ +package migrations + +import ( + "context" + "crypto/md5" + "database/sql" + "encoding/hex" + "encoding/json" + "fmt" + "math/big" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/model/id" + "github.com/pressly/goose/v3" +) + +// canonicalID maps any historical Navidrome id shape to the canonical 22-char base62 encoding +// of a 128-bit value; unrecognized shapes (including empty and share ids) pass through unchanged. +func canonicalID(s string) string { + switch len(s) { + case 22: + v, ok := new(big.Int).SetString(s, 62) + if !ok || v.Sign() < 0 || v.BitLen() <= 128 { + return s + } + sum := md5.Sum([]byte(s)) + return id.Encode(sum) + case 32: + b, err := hex.DecodeString(s) + if err != nil { + return s + } + return id.Encode([16]byte(b)) + case 36: + if s[8] != '-' || s[13] != '-' || s[18] != '-' || s[23] != '-' { + return s + } + b, err := hex.DecodeString(s[:8] + s[9:13] + s[14:18] + s[19:23] + s[24:]) + if err != nil { + return s + } + return id.Encode([16]byte(b)) + } + return s +} + +func init() { + goose.AddMigrationContext(upUniformCanonicalIds, downUniformCanonicalIds) +} + +// idColumns is the exhaustive list of Navidrome-id-bearing columns; anything absent is deliberately exempt. +var idColumns = []struct{ table, col string }{ + {"media_file", "id"}, {"media_file", "pid"}, {"media_file", "artist_id"}, + {"media_file", "album_id"}, {"media_file", "album_artist_id"}, {"media_file", "folder_id"}, + {"album", "id"}, {"album", "album_artist_id"}, + {"artist", "id"}, + {"folder", "id"}, {"folder", "parent_id"}, + {"tag", "id"}, + {"library_artist", "artist_id"}, + {"user", "id"}, + {"user_props", "user_id"}, + {"playlist", "id"}, {"playlist", "owner_id"}, + {"playlist_tracks", "playlist_id"}, {"playlist_tracks", "media_file_id"}, + {"playlist_fields", "playlist_id"}, + {"annotation", "user_id"}, {"annotation", "item_id"}, + {"bookmark", "user_id"}, {"bookmark", "item_id"}, + {"player", "id"}, {"player", "user_id"}, {"player", "transcoding_id"}, + {"transcoding", "id"}, + {"radio", "id"}, + {"share", "user_id"}, + {"scrobble_buffer", "id"}, {"scrobble_buffer", "user_id"}, {"scrobble_buffer", "media_file_id"}, + {"playqueue", "id"}, {"playqueue", "user_id"}, + {"user_library", "user_id"}, + {"scrobbles", "user_id"}, {"scrobbles", "media_file_id"}, + {"media_file_artists", "media_file_id"}, {"media_file_artists", "artist_id"}, + {"album_artists", "album_id"}, {"album_artists", "artist_id"}, + {"library_tag", "tag_id"}, +} + +// embeddedIDColumns holds ids nested inside a larger value; the id-columns guard checks this +// list against the schema, so every JSON column must appear here or be exempted there. +var embeddedIDColumns = []struct { + table, col string + transform func(string) (string, bool) +}{ + {"playqueue", "items", canonicalizeIDList}, + {"share", "resource_ids", canonicalizeIDList}, + {"plugin", "users", canonicalizePluginUsers}, + {"playlist", "rules", canonicalizePlaylistRules}, +} + +func upUniformCanonicalIds(ctx context.Context, tx *sql.Tx) error { + if err := buildIDMap(ctx, tx); err != nil { + return err + } + for _, tc := range idColumns { + if err := applyIDMap(ctx, tx, tc.table, tc.col); err != nil { + return fmt.Errorf("canonicalizing %s.%s: %w", tc.table, tc.col, err) + } + } + for _, tc := range embeddedIDColumns { + if err := rewriteColumn(ctx, tx, tc.table, tc.col, tc.transform); err != nil { + return fmt.Errorf("canonicalizing %s.%s: %w", tc.table, tc.col, err) + } + } + // Legacy PID specs embed old-shaped album/track ids into composite pids; a full rescan + // rewrites every pid with the new encoding so path-based move matching stays consistent. + if strings.Contains(conf.Server.PID.Track, "legacy") || strings.Contains(conf.Server.PID.Album, "legacy") { + if err := forceFullRescan(ctx, tx); err != nil { + return err + } + } + if err := rotateSessionSecret(ctx, tx); err != nil { + return err + } + _, err := tx.ExecContext(ctx, "DROP TABLE _id_map") + return err +} + +// rotateSessionSecret renames the session JWT secret to the public key, so public-link tokens +// keep verifying while auth.Init mints a fresh session secret, killing every stale session. +func rotateSessionSecret(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, + `UPDATE property SET id = ? WHERE id = ? AND NOT EXISTS (SELECT 1 FROM property WHERE id = ?)`, + consts.JWTPublicSecretKey, consts.JWTSecretKey, consts.JWTPublicSecretKey) + return err +} + +// buildIDMap stages old->new pairs for every id that changes, indexed for the update joins. +func buildIDMap(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, + "CREATE TEMP TABLE _id_map (old_id TEXT PRIMARY KEY, new_id TEXT NOT NULL) WITHOUT ROWID") + if err != nil { + return err + } + ins, err := tx.PrepareContext(ctx, "INSERT OR IGNORE INTO _id_map (old_id, new_id) VALUES (?, ?)") + if err != nil { + return err + } + defer ins.Close() + for _, tc := range idColumns { + if err := collectColumn(ctx, tx, ins, tc.table, tc.col); err != nil { + return fmt.Errorf("collecting %s.%s: %w", tc.table, tc.col, err) + } + } + return nil +} + +func collectColumn(ctx context.Context, tx *sql.Tx, ins *sql.Stmt, table, col string) error { + rows, err := tx.QueryContext(ctx, fmt.Sprintf( + "SELECT DISTINCT %[2]s FROM %[1]s WHERE %[2]s IS NOT NULL AND %[2]s <> ''", table, col)) + if err != nil { + return err + } + defer rows.Close() + for rows.Next() { + var old string + if err := rows.Scan(&old); err != nil { + return err + } + if newID := canonicalID(old); newID != old { + if _, err := ins.ExecContext(ctx, old, newID); err != nil { + return err + } + } + } + return rows.Err() +} + +func applyIDMap(ctx context.Context, tx *sql.Tx, table, col string) error { + _, err := tx.ExecContext(ctx, fmt.Sprintf( + `UPDATE %[1]s SET %[2]s = (SELECT new_id FROM _id_map WHERE old_id = %[1]s.%[2]s) + WHERE %[2]s IN (SELECT old_id FROM _id_map)`, table, col)) + return err +} + +// canonicalizeIDList canonicalizes a comma-separated id list element-wise. +func canonicalizeIDList(s string) (string, bool) { + parts := strings.Split(s, ",") + changed := false + for i, p := range parts { + if n := canonicalID(p); n != p { + parts[i] = n + changed = true + } + } + if !changed { + return s, false + } + return strings.Join(parts, ","), true +} + +// rewriteColumn applies transform to each non-empty cell, updating only rows it changed. +func rewriteColumn(ctx context.Context, tx *sql.Tx, table, col string, transform func(string) (string, bool)) error { + rows, err := tx.QueryContext(ctx, fmt.Sprintf( + "SELECT rowid, %[2]s FROM %[1]s WHERE ifnull(%[2]s, '') <> ''", table, col)) + if err != nil { + return err + } + type change struct { + rowid int64 + val string + } + var changes []change + for rows.Next() { + var rowid int64 + var val string + if err := rows.Scan(&rowid, &val); err != nil { + _ = rows.Close() + return err + } + if newVal, ok := transform(val); ok { + changes = append(changes, change{rowid, newVal}) + } + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return err + } + _ = rows.Close() + if len(changes) == 0 { + return nil + } + upd, err := tx.PrepareContext(ctx, fmt.Sprintf("UPDATE %s SET %s = ? WHERE rowid = ?", table, col)) + if err != nil { + return err + } + defer upd.Close() + for _, c := range changes { + if _, err := upd.ExecContext(ctx, c.val, c.rowid); err != nil { + return err + } + } + return nil +} + +// canonicalizePluginUsers maps a JSON array of user ids; malformed JSON passes through untouched. +func canonicalizePluginUsers(s string) (string, bool) { + var users []string + if err := json.Unmarshal([]byte(s), &users); err != nil { + return s, false + } + changed := false + for i, u := range users { + if n := canonicalID(u); n != u { + users[i] = n + changed = true + } + } + if !changed { + return s, false + } + out, err := json.Marshal(users) + if err != nil { + return s, false + } + return string(out), true +} + +// canonicalizePlaylistRules rewrites inPlaylist/notInPlaylist ids in smart-playlist criteria; malformed JSON passes through. +func canonicalizePlaylistRules(s string) (string, bool) { + var root map[string]any + if err := json.Unmarshal([]byte(s), &root); err != nil { + return s, false + } + if !canonicalizeRulesNode(root) { + return s, false + } + out, err := json.Marshal(root) + if err != nil { + return s, false + } + return string(out), true +} + +// canonicalizeRulesNode walks the criteria tree, canonicalizing the id of any inPlaylist/notInPlaylist object. +func canonicalizeRulesNode(node any) bool { + changed := false + switch v := node.(type) { + case map[string]any: + for k, val := range v { + if lk := strings.ToLower(k); lk == "inplaylist" || lk == "notinplaylist" { + if obj, ok := val.(map[string]any); ok { + if id, ok := obj["id"].(string); ok { + if n := canonicalID(id); n != id { + obj["id"] = n + changed = true + } + } + } + } + if canonicalizeRulesNode(val) { + changed = true + } + } + case []any: + for _, item := range v { + if canonicalizeRulesNode(item) { + changed = true + } + } + } + return changed +} + +func downUniformCanonicalIds(ctx context.Context, tx *sql.Tx) error { + return nil // irreversible data migration +} diff --git a/db/migrations/id_canonical_test.go b/db/migrations/id_canonical_test.go new file mode 100644 index 000000000..0befbe469 --- /dev/null +++ b/db/migrations/id_canonical_test.go @@ -0,0 +1,47 @@ +package migrations + +import ( + "strings" + + "github.com/navidrome/navidrome/model/id" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("canonicalID", func() { + DescribeTable("transforms each historical id shape", + func(in, want string) { + Expect(canonicalID(in)).To(Equal(want)) + }, + Entry("hash-family id (fits 128 bits) is kept", "5cLJPkLA5DK2BADhoeotPk", "5cLJPkLA5DK2BADhoeotPk"), + Entry("overflowing random id is remapped via md5", "zzzzzzzzzzzzzzzzzzzzzz", "3LyqmwQBm5IRqlVjNYASwb"), + Entry("legacy 32-hex is re-encoded value-preserving", "e3b7fc2ae9447bbec37a13bf916e3cf6", "6VHl3uR4kss6sUPKA8Cwnk"), + Entry("playlist uuid is re-encoded value-preserving", "f47ac10b-58cc-4372-a567-0e02b2c3d479", "7rke2SAWaicSeSYzkhww6R"), + Entry("empty string passes through", "", ""), + Entry("share id (10 chars) passes through", "aB3xY9kQz1", "aB3xY9kQz1"), + Entry("truncated Finamp id (16 chars) passes through", "0123456789abcdef", "0123456789abcdef"), + Entry("22 chars with non-base62 char passes through", "!!!!!!!!!!!!!!!!!!!!!!", "!!!!!!!!!!!!!!!!!!!!!!"), + Entry("32 chars non-hex passes through", "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"), + Entry("36 chars without uuid dashes passes through", "000000000000000000000000000000000000", "000000000000000000000000000000000000"), + ) + + // The exemptions for participants/tags/folder_ids/similar_artists rest on this invariant. + It("is the identity on every NewHash id", func() { + for _, parts := range [][]string{ + {""}, {"a"}, {"The Beatles"}, {"genre", "electronic"}, + {"/music/Artist/Album", "1"}, {strings.Repeat("x", 500)}, + } { + h := id.NewHash(parts...) + Expect(h).To(HaveLen(22)) + Expect(canonicalID(h)).To(Equal(h), "NewHash(%v) = %q was rewritten", parts, h) + } + }) + + It("is idempotent for every shape", func() { + for _, s := range []string{"5cLJPkLA5DK2BADhoeotPk", "zzzzzzzzzzzzzzzzzzzzzz", + "e3b7fc2ae9447bbec37a13bf916e3cf6", "f47ac10b-58cc-4372-a567-0e02b2c3d479"} { + once := canonicalID(s) + Expect(canonicalID(once)).To(Equal(once)) + } + }) +}) diff --git a/db/migrations/id_columns_guard_test.go b/db/migrations/id_columns_guard_test.go new file mode 100644 index 000000000..c4cb3b241 --- /dev/null +++ b/db/migrations/id_columns_guard_test.go @@ -0,0 +1,119 @@ +package migrations + +import ( + "context" + "database/sql" + "os" + "strings" + + _ "github.com/mattn/go-sqlite3" + "github.com/pressly/goose/v3" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Version of the uniform-canonical-ids migration whose idColumns list this guard protects. +const idColumnsMigrationVersion = 20260720015443 + +var _ = Describe("idColumns inventory", func() { + It("covers every id-bearing column present at the migration version", func() { + ctx := context.Background() + db, err := sql.Open("sqlite3", "file::memory:") + Expect(err).ToNot(HaveOccurred()) + db.SetMaxOpenConns(1) // non-shared :memory: — a second conn would be an empty DB + DeferCleanup(func() { _ = db.Close() }) + _, err = db.ExecContext(ctx, "PRAGMA foreign_keys=off") + Expect(err).ToNot(HaveOccurred()) + + goose.SetBaseFS(os.DirFS(".")) + DeferCleanup(func() { goose.SetBaseFS(nil) }) + Expect(goose.SetDialect("sqlite3")).To(Succeed()) + Expect(goose.UpToContext(ctx, db, ".", idColumnsMigrationVersion)).To(Succeed()) + + covered := map[string]bool{} + for _, tc := range idColumns { + covered[tc.table+"."+tc.col] = true + } + for _, tc := range embeddedIDColumns { + covered[tc.table+"."+tc.col] = true + } + // Columns that are id-named or JSON but need no rewrite. + exempt := map[string]string{ + "share.id": "public share URLs, generated separately", + "property.id": "property key, not an entity id", + "plugin.id": "plugin name, not an entity id", + "media_file.participants": "hash-family artist ids, unchanged", + "album.participants": "hash-family artist ids, unchanged", + "media_file.tags": "hash-family tag ids, unchanged", + "album.tags": "hash-family tag ids, unchanged", + "album.folder_ids": "hash-family folder ids, unchanged", + "artist.similar_artists": "hash-family artist ids, unchanged", + "media_file.search_participants": "participant names for FTS, not ids", + "album.search_participants": "participant names for FTS, not ids", + "album.discs": "disc number -> title map", + "media_file.lyrics": "synced lyrics, no ids", + "folder.image_files": "image file names, no ids", + "plugin.manifest": "plugin-authored manifest, no Navidrome ids", + "plugin.config": "free-form plugin config, must not be rewritten", + "plugin.libraries": "integer library ids", + } + + tables, err := queryColumn(ctx, db, "SELECT name FROM sqlite_master WHERE type='table'") + Expect(err).ToNot(HaveOccurred()) + + var dangling []string + for _, table := range tables { + if strings.HasPrefix(table, "sqlite_") || table == "goose_db_version" || strings.Contains(table, "_fts") { + continue + } + rows, err := db.QueryContext(ctx, "SELECT name, type FROM pragma_table_info(?)", table) + Expect(err).ToNot(HaveOccurred()) + for rows.Next() { + var name, typ string + Expect(rows.Scan(&name, &typ)).To(Succeed()) + lname := strings.ToLower(name) + utyp := strings.ToUpper(typ) + // JSON can hide an id under any key, so every JSON column needs a verdict. + isJSON := strings.Contains(utyp, "JSON") + isIDName := lname == "id" || lname == "pid" || + strings.HasSuffix(lname, "_id") || strings.HasSuffix(lname, "_ids") + if !isJSON && !isIDName { + continue + } + if !isJSON && !strings.Contains(utyp, "TEXT") && !strings.Contains(utyp, "CHAR") { + continue // INTEGER ids (rowid PKs, library.id) are not canonical ids + } + if strings.HasPrefix(lname, "mbz_") { + continue // MusicBrainz UUIDs, not Navidrome ids + } + key := table + "." + name + if covered[key] || exempt[key] != "" { + continue + } + dangling = append(dangling, key) + } + Expect(rows.Err()).ToNot(HaveOccurred()) + Expect(rows.Close()).To(Succeed()) + } + Expect(dangling).To(BeEmpty(), + "id-bearing columns missing from idColumns; add them to the migration or exempt with a reason: %v", dangling) + }) +}) + +func queryColumn(ctx context.Context, db *sql.DB, query string) ([]string, error) { + rows, err := db.QueryContext(ctx, query) + if err != nil { + return nil, err + } + defer rows.Close() + var out []string + for rows.Next() { + var s string + if err := rows.Scan(&s); err != nil { + return nil, err + } + out = append(out, s) + } + return out, rows.Err() +} diff --git a/db/migrations/migrations_suite_test.go b/db/migrations/migrations_suite_test.go new file mode 100644 index 000000000..bb08ad8aa --- /dev/null +++ b/db/migrations/migrations_suite_test.go @@ -0,0 +1,16 @@ +package migrations + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// tests.Init is omitted: the tests package imports db, which imports this package. +func TestMigrations(t *testing.T) { + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "Migrations Suite") +} diff --git a/db/migrations/uniform_canonical_ids_test.go b/db/migrations/uniform_canonical_ids_test.go new file mode 100644 index 000000000..ced75cb7e --- /dev/null +++ b/db/migrations/uniform_canonical_ids_test.go @@ -0,0 +1,233 @@ +package migrations + +import ( + "context" + "database/sql" + "encoding/json" + + _ "github.com/mattn/go-sqlite3" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("upUniformCanonicalIds", func() { + var db *sql.DB + var tx *sql.Tx + ctx := context.Background() + + const ( + hashID = "5cLJPkLA5DK2BADhoeotPk" // canonical, kept + randOld = "zzzzzzzzzzzzzzzzzzzzzz" // overflows -> remapped + randNew = "3LyqmwQBm5IRqlVjNYASwb" + legacyOld = "e3b7fc2ae9447bbec37a13bf916e3cf6" // 32-hex -> re-encoded + legacyNew = "6VHl3uR4kss6sUPKA8Cwnk" + uuidOld = "f47ac10b-58cc-4372-a567-0e02b2c3d479" // uuid -> re-encoded + uuidNew = "7rke2SAWaicSeSYzkhww6R" + shareID = "aB3xY9kQz1" // exempt family + + sessionSecret = "encrypted-session-secret-sentinel" + ) + + BeforeEach(func() { + var err error + db, err = sql.Open("sqlite3", "file::memory:") + Expect(err).ToNot(HaveOccurred()) + db.SetMaxOpenConns(1) // non-shared :memory: — every new conn is a fresh empty DB + DeferCleanup(func() { _ = db.Close() }) + + // Minimal fixture: every table/column idColumns and the list rewrites touch. + _, err = db.Exec(` + CREATE TABLE media_file (id text, pid text, artist_id text, album_id text, album_artist_id text, folder_id text, mbz_recording_id text); + CREATE TABLE album (id text, album_artist_id text); + CREATE TABLE artist (id text); + CREATE TABLE folder (id text, parent_id text); + CREATE TABLE tag (id text); + CREATE TABLE library_artist (artist_id text); + CREATE TABLE user (id text); + CREATE TABLE user_props (user_id text); + CREATE TABLE playlist (id text, owner_id text, rules text); + CREATE TABLE playlist_tracks (playlist_id text, media_file_id text); + CREATE TABLE playlist_fields (playlist_id text); + CREATE TABLE annotation (user_id text, item_id text, item_type text); + CREATE TABLE bookmark (user_id text, item_id text); + CREATE TABLE player (id text, user_id text, transcoding_id text); + CREATE TABLE transcoding (id text); + CREATE TABLE radio (id text); + CREATE TABLE share (id text, user_id text, resource_ids text, contents text); + CREATE TABLE scrobble_buffer (id text, user_id text, media_file_id text); + CREATE TABLE playqueue (id text, user_id text, items text); + CREATE TABLE user_library (user_id text, library_id integer); + CREATE TABLE scrobbles (id integer, media_file_id text, user_id text); + CREATE TABLE media_file_artists (media_file_id text, artist_id text); + CREATE TABLE album_artists (album_id text, artist_id text); + CREATE TABLE library_tag (tag_id text, library_id integer); + CREATE TABLE plugin (id text, users text); + CREATE TABLE property (id text primary key, value text); + `) + Expect(err).ToNot(HaveOccurred()) + + seed := func(query string, args ...any) { + _, err := db.Exec(query, args...) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + } + // media_file: legacy id/pid, hash artist, legacy album, mbz uuid must stay untouched + seed(`INSERT INTO media_file VALUES (?, ?, ?, ?, '', '', ?)`, legacyOld, legacyOld, hashID, legacyOld, uuidOld) + seed(`INSERT INTO album VALUES (?, ?)`, legacyOld, hashID) + seed(`INSERT INTO user VALUES (?)`, randOld) + // uuid id, random owner, smart-playlist rules with an embedded inPlaylist id and a sibling operator + seed(`INSERT INTO playlist VALUES (?, ?, ?)`, uuidOld, randOld, `{"all":[{"inPlaylist":{"id":"`+uuidOld+`"}},{"inTheLast":{"lastPlayed":30}}]}`) + seed(`INSERT INTO annotation VALUES (?, ?, 'media_file')`, randOld, legacyOld) + seed(`INSERT INTO playqueue VALUES (?, ?, ?)`, randOld, randOld, legacyOld+","+hashID) + seed(`INSERT INTO share VALUES (?, ?, ?, 'Album Foo...')`, shareID, randOld, legacyOld+","+uuidOld) + seed(`INSERT INTO user_library VALUES (?, 1)`, randOld) + seed(`INSERT INTO scrobbles VALUES (1, ?, ?)`, legacyOld, randOld) + seed(`INSERT INTO media_file_artists VALUES (?, ?)`, legacyOld, hashID) + seed(`INSERT INTO album_artists VALUES (?, ?)`, legacyOld, hashID) + seed(`INSERT INTO library_tag VALUES (?, 1)`, hashID) + seed(`INSERT INTO plugin VALUES ('lastfm', ?)`, `["`+randOld+`","`+hashID+`"]`) + seed(`INSERT INTO plugin VALUES ('empty', '[]')`) // exempt: empty user list untouched + // malformed JSON in both a plugin list and a playlist rule: must pass through byte-for-byte + seed(`INSERT INTO plugin VALUES ('broken', 'not-json')`) + seed(`INSERT INTO playlist VALUES (?, ?, '{broken')`, hashID, hashID) + seed(`INSERT INTO property VALUES (?, ?)`, consts.JWTSecretKey, sessionSecret) + }) + + JustBeforeEach(func() { + var err error + tx, err = db.Begin() + Expect(err).ToNot(HaveOccurred()) + Expect(upUniformCanonicalIds(ctx, tx)).To(Succeed()) + Expect(tx.Commit()).To(Succeed()) + }) + + get := func(query string) string { + var s string + ExpectWithOffset(1, db.QueryRow(query).Scan(&s)).To(Succeed()) + return s + } + + It("canonicalizes every id family and keeps references consistent", func() { + Expect(get(`SELECT id FROM media_file`)).To(Equal(legacyNew)) + Expect(get(`SELECT pid FROM media_file`)).To(Equal(legacyNew)) + Expect(get(`SELECT artist_id FROM media_file`)).To(Equal(hashID)) + Expect(get(`SELECT album_id FROM media_file`)).To(Equal(legacyNew)) + Expect(get(`SELECT id FROM album`)).To(Equal(legacyNew)) + Expect(get(`SELECT id FROM user`)).To(Equal(randNew)) + Expect(get(`SELECT id FROM playlist WHERE owner_id='` + randNew + `'`)).To(Equal(uuidNew)) + Expect(get(`SELECT owner_id FROM playlist WHERE id='` + uuidNew + `'`)).To(Equal(randNew)) + Expect(get(`SELECT item_id FROM annotation`)).To(Equal(legacyNew)) + Expect(get(`SELECT user_id FROM annotation`)).To(Equal(randNew)) + }) + + It("rewrites list columns element-wise", func() { + Expect(get(`SELECT items FROM playqueue`)).To(Equal(legacyNew + "," + hashID)) + Expect(get(`SELECT resource_ids FROM share`)).To(Equal(legacyNew + "," + uuidNew)) + }) + + It("leaves exempt columns alone", func() { + Expect(get(`SELECT id FROM share`)).To(Equal(shareID)) + Expect(get(`SELECT contents FROM share`)).To(Equal("Album Foo...")) + Expect(get(`SELECT mbz_recording_id FROM media_file`)).To(Equal(uuidOld)) + }) + + It("canonicalizes junction and membership tables", func() { + Expect(get(`SELECT user_id FROM user_library`)).To(Equal(randNew)) + Expect(get(`SELECT media_file_id FROM scrobbles`)).To(Equal(legacyNew)) + Expect(get(`SELECT user_id FROM scrobbles`)).To(Equal(randNew)) + Expect(get(`SELECT media_file_id FROM media_file_artists`)).To(Equal(legacyNew)) + Expect(get(`SELECT artist_id FROM media_file_artists`)).To(Equal(hashID)) // hash-family, unchanged + Expect(get(`SELECT album_id FROM album_artists`)).To(Equal(legacyNew)) + Expect(get(`SELECT artist_id FROM album_artists`)).To(Equal(hashID)) // hash-family, unchanged + Expect(get(`SELECT tag_id FROM library_tag`)).To(Equal(hashID)) // hash-family, unchanged + }) + + It("rewrites JSON columns element-wise", func() { + Expect(get(`SELECT users FROM plugin WHERE id='lastfm'`)).To(Equal(`["` + randNew + `","` + hashID + `"]`)) + Expect(get(`SELECT id FROM plugin WHERE id='lastfm'`)).To(Equal("lastfm")) // plugin name, untouched + + var rules map[string]any + Expect(json.Unmarshal([]byte(get(`SELECT rules FROM playlist WHERE id='`+uuidNew+`'`)), &rules)).To(Succeed()) + all, ok := rules["all"].([]any) + Expect(ok).To(BeTrue()) + Expect(all).To(HaveLen(2)) + inPl := all[0].(map[string]any)["inPlaylist"].(map[string]any) + Expect(inPl["id"]).To(Equal(uuidNew)) + }) + + It("preserves sibling operators alongside a rewritten inPlaylist id", func() { + var rules map[string]any + Expect(json.Unmarshal([]byte(get(`SELECT rules FROM playlist WHERE id='`+uuidNew+`'`)), &rules)).To(Succeed()) + all := rules["all"].([]any) + var sawInPlaylist, sawInTheLast bool + for _, e := range all { + op := e.(map[string]any) + if pl, ok := op["inPlaylist"].(map[string]any); ok { + Expect(pl["id"]).To(Equal(uuidNew)) + sawInPlaylist = true + } + if last, ok := op["inTheLast"].(map[string]any); ok { + Expect(last["lastPlayed"]).To(Equal(float64(30))) + sawInTheLast = true + } + } + Expect(sawInPlaylist).To(BeTrue()) + Expect(sawInTheLast).To(BeTrue()) + }) + + It("leaves exempt JSON rows untouched", func() { + Expect(get(`SELECT users FROM plugin WHERE id='empty'`)).To(Equal("[]")) + }) + + It("passes malformed JSON columns through byte-for-byte", func() { + Expect(get(`SELECT users FROM plugin WHERE id='broken'`)).To(Equal("not-json")) + Expect(get(`SELECT rules FROM playlist WHERE id='` + hashID + `'`)).To(Equal("{broken")) + }) + + rescanCount := func() int { + var count int + ExpectWithOffset(1, db.QueryRow( + `SELECT count(*) FROM property WHERE id = ?`, consts.FullScanAfterMigrationFlagKey).Scan(&count)).To(Succeed()) + return count + } + + It("does not force a full rescan for the default PID config", func() { + Expect(rescanCount()).To(Equal(0)) + }) + + Context("with a legacy PID configuration", func() { + BeforeEach(func() { + prev := conf.Server.PID.Album + conf.Server.PID.Album = "album_legacy" + DeferCleanup(func() { conf.Server.PID.Album = prev }) + }) + + It("forces a full rescan so composite pids are rewritten", func() { + Expect(rescanCount()).To(Equal(1)) + }) + }) + + propCount := func(key string) int { + var count int + ExpectWithOffset(1, db.QueryRow(`SELECT count(*) FROM property WHERE id = ?`, key).Scan(&count)).To(Succeed()) + return count + } + + It("rotates the session secret to the public key", func() { + Expect(propCount(consts.JWTSecretKey)).To(Equal(0)) + Expect(get(`SELECT value FROM property WHERE id = '` + consts.JWTPublicSecretKey + `'`)).To(Equal(sessionSecret)) + }) + + Context("with no stored session secret", func() { + BeforeEach(func() { + _, err := db.Exec(`DELETE FROM property WHERE id = ?`, consts.JWTSecretKey) + Expect(err).ToNot(HaveOccurred()) + }) + + It("is a no-op", func() { + Expect(propCount(consts.JWTSecretKey)).To(Equal(0)) + Expect(propCount(consts.JWTPublicSecretKey)).To(Equal(0)) + }) + }) +}) diff --git a/model/id/id.go b/model/id/id.go index b54542898..2885bc6ad 100644 --- a/model/id/id.go +++ b/model/id/id.go @@ -2,20 +2,36 @@ package id import ( "crypto/md5" + "crypto/rand" "fmt" "math/big" "strings" - - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/utils/nanoid" ) func NewRandom() string { - id, err := nanoid.Generate("0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz", 22) - if err != nil { - log.Error("Could not generate new ID", err) + var b [16]byte + _, _ = rand.Read(b[:]) + return Encode(b) +} + +// Encode renders a 16-byte value as the canonical 22-char zero-padded base62 id. +func Encode(b [16]byte) string { + return fmt.Sprintf("%022s", new(big.Int).SetBytes(b[:]).Text(62)) +} + +// Decode is the exact inverse of Encode. +func Decode(s string) ([]byte, error) { + if len(s) != 22 { + return nil, fmt.Errorf("invalid id length %d", len(s)) } - return id + v, ok := new(big.Int).SetString(s, 62) + if !ok || v.Sign() < 0 { + return nil, fmt.Errorf("invalid base62 id %q", s) + } + if v.BitLen() > 128 { + return nil, fmt.Errorf("id %q overflows 128 bits", s) + } + return v.FillBytes(make([]byte, 16)), nil } func NewHash(data ...string) string { @@ -24,11 +40,7 @@ func NewHash(data ...string) string { hash.Write([]byte(d)) hash.Write([]byte(string('\u200b'))) } - h := hash.Sum(nil) - bi := big.NewInt(0) - bi.SetBytes(h) - s := bi.Text(62) - return fmt.Sprintf("%022s", s) + return Encode([16]byte(hash.Sum(nil))) } func NewTagID(name, value string) string { diff --git a/model/id/id_suite_test.go b/model/id/id_suite_test.go new file mode 100644 index 000000000..e8a41f4e5 --- /dev/null +++ b/model/id/id_suite_test.go @@ -0,0 +1,17 @@ +package id_test + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestID(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "ID Suite") +} diff --git a/model/id/id_test.go b/model/id/id_test.go new file mode 100644 index 000000000..407560a75 --- /dev/null +++ b/model/id/id_test.go @@ -0,0 +1,64 @@ +package id_test + +import ( + "github.com/navidrome/navidrome/model/id" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Encode/Decode", func() { + It("encodes 16 bytes as 22-char zero-padded base62", func() { + Expect(id.Encode([16]byte{})).To(Equal("0000000000000000000000")) + allFF := [16]byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff} + Expect(id.Encode(allFF)).To(Equal("7N42dgm5tFLK9N8MT7fHC7")) + }) + + It("round-trips arbitrary 16-byte values", func() { + b := [16]byte{0xe3, 0xb7, 0xfc, 0x2a, 0xe9, 0x44, 0x7b, 0xbe, + 0xc3, 0x7a, 0x13, 0xbf, 0x91, 0x6e, 0x3c, 0xf6} + s := id.Encode(b) + Expect(s).To(Equal("6VHl3uR4kss6sUPKA8Cwnk")) + Expect(id.Decode(s)).To(Equal(b[:])) + }) + + It("rejects invalid input", func() { + _, err := id.Decode("short") + Expect(err).To(HaveOccurred()) + _, err = id.Decode("!!!!!!!!!!!!!!!!!!!!!!") // 22 chars, not base62 + Expect(err).To(HaveOccurred()) + _, err = id.Decode("-000000000000000000001") // sign is not part of the alphabet + Expect(err).To(HaveOccurred()) + _, err = id.Decode("zzzzzzzzzzzzzzzzzzzzzz") // > 2^128 + Expect(err).To(HaveOccurred()) + }) +}) + +var _ = Describe("NewRandom", func() { + It("emits 22-char canonical ids that always fit 128 bits", func() { + seen := make(map[string]struct{}) + for range 1000 { + s := id.NewRandom() + Expect(s).To(HaveLen(22)) + _, err := id.Decode(s) + Expect(err).ToNot(HaveOccurred(), "id %q must decode to 128 bits", s) + seen[s] = struct{}{} + } + Expect(seen).To(HaveLen(1000)) + }) +}) + +var _ = Describe("NewHash", func() { + It("keeps its historical output byte-for-byte (golden)", func() { + Expect(id.NewHash("test")).To(Equal("5cLJPkLA5DK2BADhoeotPk")) + Expect(id.NewHash("[unknown artist]")).To(Equal("7lsE5pS09fPS1VuFqwXbia")) + Expect(id.NewTagID("genre", "electronic")).To(Equal("7bLYq0Np81m1Wgy5N31nuG")) + }) + + It("always emits 22 decodable chars", func() { + h := id.NewHash("anything", "at", "all") + Expect(h).To(HaveLen(22)) + _, err := id.Decode(h) + Expect(err).ToNot(HaveOccurred()) + }) +}) diff --git a/model/metadata/legacy_ids.go b/model/metadata/legacy_ids.go index 18a273550..7156c4f50 100644 --- a/model/metadata/legacy_ids.go +++ b/model/metadata/legacy_ids.go @@ -9,17 +9,19 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" ) -// These are the legacy ID functions that were used in the original Navidrome ID generation. -// They are kept here for backwards compatibility with existing databases. +// These legacy ID functions hash the same inputs as the original Navidrome ID generation, +// now emitted in the canonical base62 encoding (matching what the uniform-ids migration stores). func legacyTrackID(mf model.MediaFile, prependLibId bool) string { - id := mf.Path + key := mf.Path if prependLibId && mf.LibraryID != model.DefaultLibraryID { - id = fmt.Sprintf("%d\\%s", mf.LibraryID, id) + key = fmt.Sprintf("%d\\%s", mf.LibraryID, key) } - return fmt.Sprintf("%x", md5.Sum([]byte(id))) + sum := md5.Sum([]byte(key)) + return id.Encode(sum) } func legacyAlbumID(mf model.MediaFile, md Metadata, prependLibId bool) string { @@ -33,7 +35,8 @@ func legacyAlbumID(mf model.MediaFile, md Metadata, prependLibId bool) string { if prependLibId && mf.LibraryID != model.DefaultLibraryID { albumPath = fmt.Sprintf("%d\\%s", mf.LibraryID, albumPath) } - return fmt.Sprintf("%x", md5.Sum([]byte(albumPath))) + sum := md5.Sum([]byte(albumPath)) + return id.Encode(sum) } func legacyMapAlbumArtistName(md Metadata) string { diff --git a/model/metadata/persistent_ids_test.go b/model/metadata/persistent_ids_test.go index eb66d11d1..8e38bbd42 100644 --- a/model/metadata/persistent_ids_test.go +++ b/model/metadata/persistent_ids_test.go @@ -218,6 +218,21 @@ var _ = Describe("getPID", func() { }) Context("legacy specs", func() { + It("emits canonical 22-char base62 ids", func() { + mf := model.MediaFile{Path: "/music/a.mp3", LibraryID: 1} + // md5("/music/a.mp3") = e3b7fc2ae9447bbec37a13bf916e3cf6 re-encoded as base62 + Expect(legacyTrackID(mf, false)).To(Equal("6VHl3uR4kss6sUPKA8Cwnk")) + }) + It("prepends the library id for a non-default library", func() { + mf := model.MediaFile{Path: "/music/a.mp3", LibraryID: 2} + // id.Encode(md5.Sum([]byte("2\\/music/a.mp3"))) + Expect(legacyTrackID(mf, true)).To(Equal("4EK5DHQBMeFuDHw6S3iooO")) + }) + It("emits a canonical album id (golden)", func() { + mf := model.MediaFile{LibraryID: 1} + // id.Encode(md5.Sum([]byte("[unknown artist]\\[unknown album]"))) + Expect(legacyAlbumID(mf, Metadata{}, false)).To(Equal("6xBmxSAUFJSQuW7UvwCq8X")) + }) Context("track_legacy", func() { When("library ID is default (1)", func() { It("should not prepend library ID even when prependLibId is true", func() { diff --git a/plugins/host_artwork_test.go b/plugins/host_artwork_test.go index ed8a0e810..218d3d892 100644 --- a/plugins/host_artwork_test.go +++ b/plugins/host_artwork_test.go @@ -225,7 +225,7 @@ func decodeArtworkURL(artworkURL string) model.ArtworkID { } // Decode JWT token - token, err := auth.TokenAuth.Decode(tokenPart) + token, err := auth.PublicTokenAuth.Decode(tokenPart) Expect(err).ToNot(HaveOccurred(), "Failed to decode JWT token") c := auth.ClaimsFromToken(token) diff --git a/plugins/host_storage_test.go b/plugins/host_storage_test.go index af8242b8a..d6f895bff 100644 --- a/plugins/host_storage_test.go +++ b/plugins/host_storage_test.go @@ -66,11 +66,8 @@ var _ = Describe("Storage Host Function", Ordered, func() { const ID = "test-storage-plugin" var ( - manager *Manager - tmpDir string - router *fakeSubsonicRouter - userRepo *tests.MockedUserRepo - dataStore *tests.MockDataStore + manager *Manager + tmpDir string ) BeforeAll(func() { @@ -79,9 +76,9 @@ var _ = Describe("Storage Host Function", Ordered, func() { Expect(err).ToNot(HaveOccurred()) // Setup mock router and data store - router = &fakeSubsonicRouter{} - userRepo = tests.CreateMockUserRepo() - dataStore = &tests.MockDataStore{MockedUser: userRepo} + router := &fakeSubsonicRouter{} + userRepo := tests.CreateMockUserRepo() + dataStore := &tests.MockDataStore{MockedUser: userRepo} // Create and configure manager manager = &Manager{ @@ -142,75 +139,76 @@ var _ = Describe("Storage Host Function", Ordered, func() { }) }) - Describe("Read", func() { - var plugin *plugin + var instance *extism.Plugin + BeforeEach(func() { + var err error + manager.mu.RLock() + plugin := manager.plugins[ID] + manager.mu.RUnlock() + Expect(plugin).ToNot(BeNil()) + ctx := GinkgoT().Context() + instance, err = plugin.instance(ctx) + Expect(err).ToNot(HaveOccurred()) + + DeferCleanup(func() { + instance.Close(ctx) + }) + }) + + Describe("Read", func() { BeforeAll(func() { path := filepath.Join(getHostStoragePath(ID), "real") err := os.WriteFile(path, []byte("1234"), 0600) Expect(err).ToNot(HaveOccurred()) - }) - BeforeEach(func() { - manager.mu.RLock() - plugin = manager.plugins[ID] - manager.mu.RUnlock() - Expect(plugin).ToNot(BeNil()) + DeferCleanup(func() { + _ = os.Remove(path) + }) }) It("should fail to read missing file", func() { - instance, err := plugin.instance(GinkgoT().Context()) - Expect(err).ToNot(HaveOccurred()) - defer instance.Close(GinkgoT().Context()) - exit, _, err := instance.Call("call_read", []byte("missing")) Expect(exit).To(Equal(uint32(1))) Expect(err).To(HaveOccurred()) }) It("should read an existing file", func() { - instance, err := plugin.instance(GinkgoT().Context()) - Expect(err).ToNot(HaveOccurred()) - defer instance.Close(GinkgoT().Context()) - exit, output, err := instance.Call("call_read", []byte("real")) Expect(exit).To(Equal(uint32(0))) Expect(err).ToNot(HaveOccurred()) Expect(output).To(Equal([]byte("1234"))) }) + + It("should not escape read", func() { + path := filepath.Join(getHostStoragePath(ID), "..", "outside") + err := os.WriteFile(path, []byte("outside"), 0600) + Expect(err).ToNot(HaveOccurred()) + + exit, _, err := instance.Call("call_read", []byte("../outside")) + Expect(exit).To(Equal(uint32(1))) + Expect(err).To(HaveOccurred()) + }) }) Describe("Write", func() { - var p *plugin - BeforeAll(func() { path := filepath.Join(getHostStoragePath(ID), "real") err := os.WriteFile(path, []byte("1234"), 0600) Expect(err).ToNot(HaveOccurred()) - }) - BeforeEach(func() { - manager.mu.RLock() - p = manager.plugins[ID] - manager.mu.RUnlock() - Expect(p).ToNot(BeNil()) + DeferCleanup(func() { + _ = os.Remove(path) + }) }) It("should fail to write to nested file", func() { - instance, err := p.instance(GinkgoT().Context()) - Expect(err).ToNot(HaveOccurred()) - defer instance.Close(GinkgoT().Context()) - exit, _, err := instance.Call("call_write", []byte(`{"path":"nested/file","contents":"1234"}`)) Expect(exit).To(Equal(uint32(1))) Expect(err).To(HaveOccurred()) }) It("should write to a file", func() { - instance, err := p.instance(GinkgoT().Context()) - Expect(err).ToNot(HaveOccurred()) - defer instance.Close(GinkgoT().Context()) - exit, _, err := instance.Call("call_write", []byte(`{"path":"new","contents":"contents"}`)) Expect(exit).To(Equal(uint32(0))) Expect(err).ToNot(HaveOccurred()) @@ -225,6 +223,16 @@ var _ = Describe("Storage Host Function", Ordered, func() { Expect(output).To(Equal([]byte("contents"))) }) + It("should not escape writing a file", func() { + path := filepath.Join(getHostStoragePath(ID), "..", "outside") + err := os.WriteFile(path, []byte("outside"), 0600) + Expect(err).ToNot(HaveOccurred()) + + exit, _, err := instance.Call("call_write", []byte(`{"path":"../new","contents":"contents"}`)) + Expect(exit).To(Equal(uint32(1))) + Expect(err).To(HaveOccurred()) + }) + It("should have independent storage for multiple plugins", func() { manager.mu.RLock() plugin2 := manager.plugins[ID+"-2"] @@ -232,17 +240,15 @@ var _ = Describe("Storage Host Function", Ordered, func() { Expect(plugin2).ToNot(BeNil()) - plugins := []*plugin{p, plugin2} - instances := []*extism.Plugin{} + secondInstance, err := plugin2.instance(GinkgoT().Context()) + Expect(err).ToNot(HaveOccurred()) + defer secondInstance.Close(GinkgoT().Context()) + + instances := []*extism.Plugin{instance, secondInstance} names := []string{ID, ID + "-2"} - for idx := range plugins { - instance, err := plugins[idx].instance(GinkgoT().Context()) - instances = append(instances, instance) - Expect(err).ToNot(HaveOccurred()) - defer instance.Close(GinkgoT().Context()) - - exit, _, err := instance.Call("call_write", fmt.Appendf(nil, `{"path":"new","contents":"%s"}`, names[idx])) + for idx := range instances { + exit, _, err := instances[idx].Call("call_write", fmt.Appendf(nil, `{"path":"new","contents":"%s"}`, names[idx])) Expect(exit).To(Equal(uint32(0))) Expect(err).ToNot(HaveOccurred()) } @@ -253,7 +259,7 @@ var _ = Describe("Storage Host Function", Ordered, func() { Expect(data).To(Equal([]byte(names[idx]))) } - for idx := range plugins { + for idx := range instances { exit, output, err := instances[idx].Call("call_read", []byte("new")) Expect(exit).To(Equal(uint32(0))) Expect(err).ToNot(HaveOccurred()) diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 3b07600de..2c09fc575 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "maps" "time" extism "github.com/extism/go-sdk" @@ -319,24 +318,12 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return fmt.Errorf("opening package: %w", err) } - // Build extism manifest - pluginManifest := extism.Manifest{ - Wasm: []extism.Wasm{ - extism.WasmData{Data: pkg.WasmBytes, Name: "main"}, - }, - Config: pluginConfig, - Timeout: uint64(defaultTimeout.Milliseconds()), - } + pluginManifest := buildExtismManifest(pkg, pluginConfig) - if pkg.Manifest.Permissions != nil && pkg.Manifest.Permissions.Http != nil { - if hosts := pkg.Manifest.Permissions.Http.RequiredHosts; len(hosts) > 0 { - pluginManifest.AllowedHosts = hosts - } - } - - // Configure filesystem access for library permission + // Configure filesystem access for library permission, applied per instance + var fsConfig wazero.FSConfig if pkg.Manifest.HasLibraryFilesystemPermission() || pkg.Manifest.HasStoragePermissions() { - allowedPaths := map[string]string{} + mounts := []mount{} if pkg.Manifest.HasLibraryFilesystemPermission() { adminCtx := adminContext(ctx) @@ -344,16 +331,22 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { if err != nil { return fmt.Errorf("failed to get libraries for filesystem access: %w", err) } - - libraryPaths := buildAllowedPaths(ctx, libraries, allowedLibraries, p.AllLibraries, p.AllowWriteAccess) - maps.Copy(allowedPaths, libraryPaths) + mounts = buildMounts(ctx, libraries, allowedLibraries, p.AllLibraries, p.AllowWriteAccess) } if pkg.Manifest.HasStoragePermissions() { - allowedPaths[getHostStoragePath(p.ID)] = storageMount + pluginStore := getHostStoragePath(p.ID) + log.Info(ctx, "Granting read-write filesystem access to plugin storage", "path", pluginStore, "id", p.ID) + + mounts = append(mounts, mount{ + hostPath: pluginStore, + guestPath: storageMount, + }) } - pluginManifest.AllowedPaths = allowedPaths + log.Info(ctx, "mounts", "plugin", p.ID, "mounts", mounts) + + fsConfig = buildFSConfig(mounts) } // Build host functions based on permissions from manifest @@ -408,7 +401,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { } // Create instance to detect capabilities - instance, err := compiled.Instance(ctx, extism.PluginInstanceConfig{}) + instance, err := compiled.Instance(ctx, instanceConfig(fsConfig)) if err != nil { compiled.Close(ctx) return fmt.Errorf("creating instance: %w", err) @@ -435,6 +428,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { allowedUserIDs: allowedUsers, allUsers: p.AllUsers, libraries: newLibraryAccess(allowedLibraries, p.AllLibraries), + fsConfig: fsConfig, lyricsSem: make(chan struct{}, maxConcurrentLyricsCalls), } m.mu.Unlock() @@ -481,31 +475,18 @@ func parsePluginConfig(configJSON string) (map[string]string, error) { return pluginConfig, nil } -// buildAllowedPaths constructs the extism AllowedPaths map for filesystem access. -// When allowWriteAccess is false (default), paths are prefixed with "ro:" for read-only. -// Only libraries that match the allowed set (or all libraries if allLibraries is true) are included. -func buildAllowedPaths(ctx context.Context, libraries model.Libraries, allowedLibraryIDs []int, allLibraries, allowWriteAccess bool) map[string]string { - allowedLibrarySet := make(map[int]struct{}, len(allowedLibraryIDs)) - for _, id := range allowedLibraryIDs { - allowedLibrarySet[id] = struct{}{} +// buildExtismManifest describes the plugin to extism. It must never set +// AllowedPaths: extism would replace our jailed FSConfig with plain dir mounts. +func buildExtismManifest(pkg *ndpPackage, pluginConfig map[string]string) extism.Manifest { + manifest := extism.Manifest{ + Wasm: []extism.Wasm{extism.WasmData{Data: pkg.WasmBytes, Name: "main"}}, + Config: pluginConfig, + Timeout: uint64(defaultTimeout.Milliseconds()), } - allowedPaths := make(map[string]string) - for _, lib := range libraries { - _, allowed := allowedLibrarySet[lib.ID] - if allLibraries || allowed { - mountPoint := toPluginMountPoint(int32(lib.ID)) - hostPath := lib.Path - if !allowWriteAccess { - hostPath = "ro:" + hostPath - } - allowedPaths[hostPath] = mountPoint - log.Trace(ctx, "Added library to allowed paths", "libraryID", lib.ID, "mountPoint", mountPoint, "writeAccess", allowWriteAccess, "hostPath", hostPath) + if pkg.Manifest.Permissions != nil && pkg.Manifest.Permissions.Http != nil { + if hosts := pkg.Manifest.Permissions.Http.RequiredHosts; len(hosts) > 0 { + manifest.AllowedHosts = hosts } } - if allowWriteAccess { - log.Info(ctx, "Granting read-write filesystem access to libraries", "libraryCount", len(allowedPaths), "allLibraries", allLibraries) - } else { - log.Debug(ctx, "Granting read-only filesystem access to libraries", "libraryCount", len(allowedPaths), "allLibraries", allLibraries) - } - return allowedPaths + return manifest } diff --git a/plugins/manager_loader_test.go b/plugins/manager_loader_test.go index cc07f0611..5e6851481 100644 --- a/plugins/manager_loader_test.go +++ b/plugins/manager_loader_test.go @@ -1,11 +1,32 @@ package plugins import ( - "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) +var _ = Describe("buildExtismManifest", func() { + var pkg *ndpPackage + + BeforeEach(func() { + pkg = &ndpPackage{ + WasmBytes: []byte("wasm"), + Manifest: &Manifest{Permissions: &Permissions{ + Library: &LibraryPermission{Reason: new("test"), Filesystem: true}, + Http: &HTTPPermission{Reason: new("test"), RequiredHosts: []string{"example.com"}}, + }}, + } + }) + + It("never sets AllowedPaths, even with filesystem permission", func() { + Expect(buildExtismManifest(pkg, nil).AllowedPaths).To(BeEmpty()) + }) + + It("carries the hosts the plugin is allowed to reach", func() { + Expect(buildExtismManifest(pkg, nil).AllowedHosts).To(Equal([]string{"example.com"})) + }) +}) + var _ = Describe("parsePluginConfig", func() { It("returns nil for empty string", func() { result, err := parsePluginConfig("") @@ -57,66 +78,3 @@ var _ = Describe("parsePluginConfig", func() { Expect(result).ToNot(BeNil()) }) }) - -var _ = Describe("buildAllowedPaths", func() { - var libraries model.Libraries - - BeforeEach(func() { - libraries = model.Libraries{ - {ID: 1, Path: "/music/library1"}, - {ID: 2, Path: "/music/library2"}, - {ID: 3, Path: "/music/library3"}, - } - }) - - Context("read-only (default)", func() { - It("mounts all libraries with ro: prefix when allLibraries is true", func() { - result := buildAllowedPaths(nil, libraries, nil, true, false) - Expect(result).To(HaveLen(3)) - Expect(result).To(HaveKeyWithValue("ro:/music/library1", "/libraries/1")) - Expect(result).To(HaveKeyWithValue("ro:/music/library2", "/libraries/2")) - Expect(result).To(HaveKeyWithValue("ro:/music/library3", "/libraries/3")) - }) - - It("mounts only selected libraries with ro: prefix", func() { - result := buildAllowedPaths(nil, libraries, []int{1, 3}, false, false) - Expect(result).To(HaveLen(2)) - Expect(result).To(HaveKeyWithValue("ro:/music/library1", "/libraries/1")) - Expect(result).To(HaveKeyWithValue("ro:/music/library3", "/libraries/3")) - Expect(result).ToNot(HaveKey("ro:/music/library2")) - }) - }) - - Context("read-write (allowWriteAccess=true)", func() { - It("mounts all libraries without ro: prefix when allLibraries is true", func() { - result := buildAllowedPaths(nil, libraries, nil, true, true) - Expect(result).To(HaveLen(3)) - Expect(result).To(HaveKeyWithValue("/music/library1", "/libraries/1")) - Expect(result).To(HaveKeyWithValue("/music/library2", "/libraries/2")) - Expect(result).To(HaveKeyWithValue("/music/library3", "/libraries/3")) - }) - - It("mounts only selected libraries without ro: prefix", func() { - result := buildAllowedPaths(nil, libraries, []int{2}, false, true) - Expect(result).To(HaveLen(1)) - Expect(result).To(HaveKeyWithValue("/music/library2", "/libraries/2")) - }) - }) - - Context("edge cases", func() { - It("returns empty map when no libraries match", func() { - result := buildAllowedPaths(nil, libraries, []int{99}, false, false) - Expect(result).To(BeEmpty()) - }) - - It("returns empty map when libraries list is empty", func() { - result := buildAllowedPaths(nil, nil, []int{1}, false, false) - Expect(result).To(BeEmpty()) - }) - - It("returns empty map when allLibraries is false and no IDs provided", func() { - result := buildAllowedPaths(nil, libraries, nil, false, false) - Expect(result).To(BeEmpty()) - }) - }) -}) diff --git a/plugins/manager_plugin.go b/plugins/manager_plugin.go index 155663781..13375a70f 100644 --- a/plugins/manager_plugin.go +++ b/plugins/manager_plugin.go @@ -24,16 +24,24 @@ type plugin struct { allowedUserIDs []string // User IDs this plugin can access (from DB configuration) allUsers bool // If true, plugin can access all users libraries libraryAccess - lyricsSem chan struct{} // Caps concurrent lyrics calls (see LyricsPlugin.GetLyrics) + lyricsSem chan struct{} // Caps concurrent lyrics calls (see LyricsPlugin.GetLyrics) + fsConfig wazero.FSConfig // Sandboxed library mounts, nil if no filesystem permission +} + +// instanceConfig is used by every call site, so all instances get the sandboxed mounts. +func instanceConfig(fsConfig wazero.FSConfig) extism.PluginInstanceConfig { + moduleConfig := wazero.NewModuleConfig().WithSysWalltime().WithRandSource(rand.Reader) + if fsConfig != nil { + moduleConfig = moduleConfig.WithFSConfig(fsConfig) + } + return extism.PluginInstanceConfig{ModuleConfig: moduleConfig} } // instance creates a new plugin instance for the given context. // The context is used for cancellation - if cancelled during a call, // the module will be terminated and the instance becomes unusable. func (p *plugin) instance(ctx context.Context) (*extism.Plugin, error) { - instance, err := p.compiled.Instance(ctx, extism.PluginInstanceConfig{ - ModuleConfig: wazero.NewModuleConfig().WithSysWalltime().WithRandSource(rand.Reader), - }) + instance, err := p.compiled.Instance(ctx, instanceConfig(p.fsConfig)) if err != nil { return nil, err } diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index bb081988e..77956e2f9 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strings" "testing" "time" @@ -81,46 +82,43 @@ func createTestManagerWithPlugins(pluginConfig map[string]map[string]string, plu return createTestManagerWithPluginsAndMetrics(pluginConfig, noopMetricsRecorder{}, plugins...) } +// installTestPlugins copies the given .ndp packages into dir and returns their +// enabled DB rows, so callers can grant whatever access the test needs. +func installTestPlugins(dir string, plugins ...string) model.Plugins { + var rows model.Plugins + for _, plugin := range plugins { + data, err := os.ReadFile(filepath.Join(testdataDir, plugin)) + Expect(err).ToNot(HaveOccurred()) + destPath := filepath.Join(dir, plugin) + Expect(os.WriteFile(destPath, data, 0600)).To(Succeed()) + + hash := sha256.Sum256(data) + rows = append(rows, model.Plugin{ + ID: strings.TrimSuffix(plugin, PackageExtension), + Path: destPath, + SHA256: hex.EncodeToString(hash[:]), + Enabled: true, + }) + } + return rows +} + // createTestManagerWithPluginsAndMetrics creates a new plugin Manager with the given plugin config, -// metrics recorder, and specified plugins. It creates a temp directory, copies the specified plugins, and starts the manager. -// Returns the manager and temp directory path. +// metrics recorder, and specified plugins. It creates a temp directory, copies the specified plugins, +// and starts the manager. Returns the manager and temp directory path. func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]string, metrics PluginMetricsRecorder, plugins ...string) (*Manager, string) { // Create temp directory tmpDir, err := os.MkdirTemp("", "plugins-test-*") Expect(err).ToNot(HaveOccurred()) - // Copy test plugins to temp dir and build plugin list with SHA256 - var enabledPlugins model.Plugins - for _, plugin := range plugins { - srcPath := filepath.Join(testdataDir, plugin) - destPath := filepath.Join(tmpDir, plugin) - data, err := os.ReadFile(srcPath) - Expect(err).ToNot(HaveOccurred()) - err = os.WriteFile(destPath, data, 0600) - Expect(err).ToNot(HaveOccurred()) - - // Compute SHA256 for the plugin - hash := sha256.Sum256(data) - hashHex := hex.EncodeToString(hash[:]) - pluginName := plugin[:len(plugin)-len(PackageExtension)] // Remove .ndp extension - - // Build config JSON if provided - configJSON := "" - if pluginConfig != nil && pluginConfig[pluginName] != nil { - // Encode config to JSON - configBytes, err := json.Marshal(pluginConfig[pluginName]) + enabledPlugins := installTestPlugins(tmpDir, plugins...) + for i, p := range enabledPlugins { + enabledPlugins[i].AllUsers = true // Allow all users by default in tests + if pluginConfig[p.ID] != nil { + configBytes, err := json.Marshal(pluginConfig[p.ID]) Expect(err).ToNot(HaveOccurred()) - configJSON = string(configBytes) + enabledPlugins[i].Config = string(configBytes) } - - enabledPlugins = append(enabledPlugins, model.Plugin{ - ID: pluginName, - Path: destPath, - SHA256: hashHex, - Enabled: true, - Config: configJSON, - AllUsers: true, // Allow all users by default in tests - }) } // Setup config diff --git a/plugins/sandbox_fs.go b/plugins/sandbox_fs.go new file mode 100644 index 000000000..57bba85af --- /dev/null +++ b/plugins/sandbox_fs.go @@ -0,0 +1,157 @@ +package plugins + +import ( + "context" + "io/fs" + "path/filepath" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/tetratelabs/wazero" + experimentalsys "github.com/tetratelabs/wazero/experimental/sys" + "github.com/tetratelabs/wazero/experimental/sysfs" + "github.com/tetratelabs/wazero/sys" +) + +// jailedFS confines a mount to its root: wazero resolves guest paths by bare +// concatenation. Named field, not embedding, so a new sys.FS method can't slip through. +type jailedFS struct { + fs experimentalsys.FS +} + +// escapes reports whether a guest path can resolve outside the mount root. +// WASI only validates "/"-separated paths, missing `..\`, `C:x` and device names. +func escapes(path string) bool { + switch path { + case "", ".", "/": // how wazero opens the mount root itself + return false + } + return !filepath.IsLocal(path) +} + +func (jailedFS) Symlink(string, string) experimentalsys.Errno { + return experimentalsys.EPERM +} + +func (j jailedFS) OpenFile(path string, flag experimentalsys.Oflag, perm fs.FileMode) (experimentalsys.File, experimentalsys.Errno) { + if escapes(path) { + return nil, experimentalsys.EPERM + } + return j.fs.OpenFile(path, flag, perm) +} + +func (j jailedFS) Lstat(path string) (sys.Stat_t, experimentalsys.Errno) { + if escapes(path) { + return sys.Stat_t{}, experimentalsys.EPERM + } + return j.fs.Lstat(path) +} + +func (j jailedFS) Stat(path string) (sys.Stat_t, experimentalsys.Errno) { + if escapes(path) { + return sys.Stat_t{}, experimentalsys.EPERM + } + return j.fs.Stat(path) +} + +func (j jailedFS) Mkdir(path string, perm fs.FileMode) experimentalsys.Errno { + if escapes(path) { + return experimentalsys.EPERM + } + return j.fs.Mkdir(path, perm) +} + +func (j jailedFS) Chmod(path string, perm fs.FileMode) experimentalsys.Errno { + if escapes(path) { + return experimentalsys.EPERM + } + return j.fs.Chmod(path, perm) +} + +func (j jailedFS) Rename(from, to string) experimentalsys.Errno { + if escapes(from) || escapes(to) { + return experimentalsys.EPERM + } + return j.fs.Rename(from, to) +} + +func (j jailedFS) Rmdir(path string) experimentalsys.Errno { + if escapes(path) { + return experimentalsys.EPERM + } + return j.fs.Rmdir(path) +} + +func (j jailedFS) Unlink(path string) experimentalsys.Errno { + if escapes(path) { + return experimentalsys.EPERM + } + return j.fs.Unlink(path) +} + +func (j jailedFS) Link(oldPath, newPath string) experimentalsys.Errno { + if escapes(oldPath) || escapes(newPath) { + return experimentalsys.EPERM + } + return j.fs.Link(oldPath, newPath) +} + +func (j jailedFS) Readlink(path string) (string, experimentalsys.Errno) { + if escapes(path) { + return "", experimentalsys.EPERM + } + return j.fs.Readlink(path) +} + +func (j jailedFS) Utimens(path string, atim, mtim int64) experimentalsys.Errno { + if escapes(path) { + return experimentalsys.EPERM + } + return j.fs.Utimens(path, atim, mtim) +} + +// mount is a host directory exposed to a plugin at guestPath. +type mount struct { + hostPath string + guestPath string + readOnly bool +} + +// buildMounts lists the libraries the plugin may reach through the filesystem. +func buildMounts(ctx context.Context, libraries model.Libraries, allowedLibraryIDs []int, allLibraries, allowWriteAccess bool) []mount { + allowedLibrarySet := make(map[int]struct{}, len(allowedLibraryIDs)) + for _, id := range allowedLibraryIDs { + allowedLibrarySet[id] = struct{}{} + } + var mounts []mount + for _, lib := range libraries { + _, allowed := allowedLibrarySet[lib.ID] + if allLibraries || allowed { + m := mount{hostPath: lib.Path, guestPath: toPluginMountPoint(int32(lib.ID)), readOnly: !allowWriteAccess} + mounts = append(mounts, m) + log.Trace(ctx, "Added library to plugin mounts", "libraryID", lib.ID, "mountPoint", m.guestPath, "readOnly", m.readOnly, "hostPath", m.hostPath) + } + } + if allowWriteAccess { + log.Info(ctx, "Granting read-write filesystem access to libraries", "libraryCount", len(mounts), "allLibraries", allLibraries) + } else { + log.Debug(ctx, "Granting read-only filesystem access to libraries", "libraryCount", len(mounts), "allLibraries", allLibraries) + } + return mounts +} + +// buildFSConfig mounts each host directory jailed to its root. +func buildFSConfig(mounts []mount) wazero.FSConfig { + if len(mounts) == 0 { + return nil + } + cfg := wazero.NewFSConfig() + for _, m := range mounts { + var mounted experimentalsys.FS = jailedFS{fs: sysfs.DirFS(m.hostPath)} + if m.readOnly { + mounted = &sysfs.ReadFS{FS: mounted} + } + cfg = cfg.(sysfs.FSConfig).WithSysFSMount(mounted, m.guestPath) + } + return cfg +} diff --git a/plugins/sandbox_fs_internal_test.go b/plugins/sandbox_fs_internal_test.go new file mode 100644 index 000000000..73011225d --- /dev/null +++ b/plugins/sandbox_fs_internal_test.go @@ -0,0 +1,127 @@ +package plugins + +import ( + "os" + "path/filepath" + "runtime" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + experimentalsys "github.com/tetratelabs/wazero/experimental/sys" + "github.com/tetratelabs/wazero/experimental/sysfs" +) + +var _ = Describe("buildMounts", func() { + var libraries model.Libraries + + BeforeEach(func() { + libraries = model.Libraries{ + {ID: 1, Path: "/music/library1"}, + {ID: 2, Path: "/music/library2"}, + {ID: 3, Path: "/music/library3"}, + } + }) + + It("mounts all libraries read-only by default", func() { + Expect(buildMounts(nil, libraries, nil, true, false)).To(Equal([]mount{ + {hostPath: "/music/library1", guestPath: "/libraries/1", readOnly: true}, + {hostPath: "/music/library2", guestPath: "/libraries/2", readOnly: true}, + {hostPath: "/music/library3", guestPath: "/libraries/3", readOnly: true}, + })) + }) + + It("mounts only the selected libraries", func() { + Expect(buildMounts(nil, libraries, []int{1, 3}, false, false)).To(Equal([]mount{ + {hostPath: "/music/library1", guestPath: "/libraries/1", readOnly: true}, + {hostPath: "/music/library3", guestPath: "/libraries/3", readOnly: true}, + })) + }) + + It("mounts writable when write access is granted", func() { + Expect(buildMounts(nil, libraries, []int{2}, false, true)).To(Equal([]mount{ + {hostPath: "/music/library2", guestPath: "/libraries/2", readOnly: false}, + })) + }) + + DescribeTable("mounts nothing", + func(libs model.Libraries, allowedIDs []int, allLibraries bool) { + Expect(buildMounts(nil, libs, allowedIDs, allLibraries, false)).To(BeEmpty()) + }, + Entry("when no library matches", libraries, []int{99}, false), + Entry("when there are no libraries", model.Libraries(nil), []int{1}, false), + Entry("when no library is selected", libraries, nil, false), + ) +}) + +var _ = Describe("jailedFS", func() { + var ( + fs jailedFS + root string + outsideDir string + ) + + BeforeEach(func() { + tmpDir := GinkgoT().TempDir() + root = filepath.Join(tmpDir, "root") + outsideDir = filepath.Join(tmpDir, "outside") + Expect(os.MkdirAll(root, 0755)).To(Succeed()) + Expect(os.MkdirAll(outsideDir, 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(outsideDir, "secret.txt"), []byte("secret"), 0600)).To(Succeed()) + + fs = jailedFS{sysfs.DirFS(root)} + }) + + It("denies creating symlinks", func() { + Expect(fs.Symlink("../outside", "link")).To(Equal(experimentalsys.EPERM)) + }) + + It("denies reading through a path that escapes the root", func() { + _, errno := fs.OpenFile("../outside/secret.txt", experimentalsys.O_RDONLY, 0) + Expect(errno).To(Equal(experimentalsys.EPERM)) + }) + + It("denies traversal with the host separator", func() { + _, errno := fs.OpenFile(`..\outside\secret.txt`, experimentalsys.O_RDONLY, 0) + + if runtime.GOOS == "windows" { + Expect(errno).To(Equal(experimentalsys.EPERM)) + } else { + // A backslash is a legal filename character on POSIX, not a separator + Expect(errno).To(Equal(experimentalsys.ENOENT)) + } + }) + + It("denies absolute paths", func() { + _, errno := fs.OpenFile("/etc/passwd", experimentalsys.O_RDONLY, 0) + Expect(errno).To(Equal(experimentalsys.EPERM)) + }) + + It("denies creating a directory outside the root", func() { + Expect(fs.Mkdir("../outside/new-dir", 0755)).To(Equal(experimentalsys.EPERM)) + Expect(filepath.Join(outsideDir, "new-dir")).ToNot(BeADirectory()) + }) + + It("denies escaping through either side of a rename", func() { + Expect(fs.Rename("../outside/secret.txt", "stolen.txt")).To(Equal(experimentalsys.EPERM)) + Expect(fs.Rename("inside.txt", "../outside/leaked.txt")).To(Equal(experimentalsys.EPERM)) + }) + + // Music libraries symlink in folders from elsewhere; following them is intended + It("follows symlinks that already exist in the mount", func() { + if err := os.Symlink(outsideDir, filepath.Join(root, "linked")); err != nil { + Skip("cannot create symlinks here: " + err.Error()) // Windows without privileges + } + + _, errno := fs.OpenFile("linked/secret.txt", experimentalsys.O_RDONLY, 0) + Expect(errno).To(BeZero()) + }) + + It("allows access within the root", func() { + Expect(fs.Mkdir("sub", 0755)).To(BeZero()) + f, errno := fs.OpenFile("sub/file.txt", experimentalsys.O_CREAT|experimentalsys.O_WRONLY, 0600) + Expect(errno).To(BeZero()) + Expect(f.Close()).To(BeZero()) + Expect(filepath.Join(root, "sub", "file.txt")).To(BeAnExistingFile()) + }) +}) diff --git a/plugins/sandbox_fs_test.go b/plugins/sandbox_fs_test.go new file mode 100644 index 000000000..ea033283f --- /dev/null +++ b/plugins/sandbox_fs_test.go @@ -0,0 +1,182 @@ +//go:build !windows + +package plugins + +import ( + "context" + "encoding/json" + "net/http" + "os" + "path/filepath" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +type sandboxInput struct { + Operation string `json:"operation"` + MountPoint string `json:"mount_point,omitempty"` + FilePath string `json:"file_path,omitempty"` + Content string `json:"content,omitempty"` + Target string `json:"target,omitempty"` +} + +type sandboxOutput struct { + FileContent string `json:"file_content,omitempty"` + Error *string `json:"error,omitempty"` +} + +// startSandboxManager loads test-library against libraryDir with the given grant. +func startSandboxManager(tmpDir, libraryDir string, grant func(*model.Plugin)) *Manager { + GinkgoHelper() + installed := installTestPlugins(tmpDir, "test-library"+PackageExtension) + grant(&installed[0]) + + DeferCleanup(configtest.SetupConfig()) + conf.Server.Plugins.Enabled = true + conf.Server.Plugins.Folder = conf.NewDir(tmpDir) + conf.Server.Plugins.AutoReload = false + + mockPluginRepo := tests.CreateMockPluginRepo() + mockPluginRepo.Permitted = true + mockPluginRepo.SetData(installed) + + mockLibraryRepo := &tests.MockLibraryRepo{} + mockLibraryRepo.SetData(model.Libraries{{ID: 1, Name: "Test Library", Path: libraryDir}}) + + manager := &Manager{ + plugins: make(map[string]*plugin), + ds: &tests.MockDataStore{MockedPlugin: mockPluginRepo, MockedLibrary: mockLibraryRepo}, + subsonicRouter: http.NotFoundHandler(), + } + Expect(manager.Start(GinkgoT().Context())).To(Succeed()) + DeferCleanup(func() { _ = manager.Stop() }) + return manager +} + +// callSandbox runs one filesystem operation inside the plugin sandbox. +func callSandbox(manager *Manager, input sandboxInput) sandboxOutput { + GinkgoHelper() + manager.mu.RLock() + p := manager.plugins["test-library"] + manager.mu.RUnlock() + Expect(p).ToNot(BeNil()) + + instance, err := p.instance(context.Background()) + Expect(err).ToNot(HaveOccurred()) + defer instance.Close(context.Background()) + + inputBytes, err := json.Marshal(input) + Expect(err).ToNot(HaveOccurred()) + _, outputBytes, err := instance.Call("nd_test_library", inputBytes) + Expect(err).ToNot(HaveOccurred()) + + var output sandboxOutput + Expect(json.Unmarshal(outputBytes, &output)).To(Succeed()) + return output +} + +var _ = Describe("Plugin filesystem sandbox", Ordered, ContinueOnFailure, func() { + var ( + manager *Manager + libraryDir string + outsideDir string + secretFile string + mountPoint string + ) + + call := func(input sandboxInput) sandboxOutput { + GinkgoHelper() + input.MountPoint = mountPoint + return callSandbox(manager, input) + } + + BeforeAll(func() { + tmpDir := GinkgoT().TempDir() + libraryDir = filepath.Join(tmpDir, "music-library") + Expect(os.MkdirAll(libraryDir, 0755)).To(Succeed()) + + // Sibling of the mount root: anything reached here escaped the sandbox + outsideDir = filepath.Join(tmpDir, "outside") + Expect(os.MkdirAll(outsideDir, 0755)).To(Succeed()) + secretFile = filepath.Join(outsideDir, "secret.txt") + Expect(os.WriteFile(secretFile, []byte("secret"), 0600)).To(Succeed()) + + mountPoint = toPluginMountPoint(1) + manager = startSandboxManager(tmpDir, libraryDir, func(p *model.Plugin) { + p.AllLibraries = true + p.AllowWriteAccess = true + }) + }) + + It("allows writing inside the mount", func() { + out := call(sandboxInput{Operation: "write_file", FilePath: "inside.txt", Content: "hello"}) + + Expect(out.Error).To(BeNil()) + Expect(os.ReadFile(filepath.Join(libraryDir, "inside.txt"))).To(BeEquivalentTo("hello")) + }) + + It("cannot escape the mount by creating a symlink", func() { + linked := call(sandboxInput{Operation: "symlink", FilePath: "escape", Target: "../outside"}) + Expect(linked.Error).ToNot(BeNil()) + _, err := os.Lstat(filepath.Join(libraryDir, "escape")) + Expect(err).To(MatchError(os.ErrNotExist), "the plugin created a symlink out of the mount") + + wrote := call(sandboxInput{Operation: "write_file", FilePath: "escape/via-created-symlink.txt", Content: "escaped"}) + + Expect(wrote.Error).ToNot(BeNil()) + Expect(filepath.Join(outsideDir, "via-created-symlink.txt")).ToNot(BeAnExistingFile()) + }) + + // Accepted residual, pinned so a future tightening can't happen silently + It("still follows a symlink planted in the mount by something else", func() { + Expect(os.Symlink(outsideDir, filepath.Join(libraryDir, "planted"))).To(Succeed()) + + out := call(sandboxInput{Operation: "write_file", FilePath: "planted/via-symlink.txt", Content: "escaped"}) + + Expect(out.Error).To(BeNil()) + Expect(filepath.Join(outsideDir, "via-symlink.txt")).To(BeAnExistingFile()) + }) + + It("cannot read outside the mount with ..", func() { + out := call(sandboxInput{Operation: "read_file", FilePath: "../outside/secret.txt"}) + + Expect(out.Error).ToNot(BeNil()) + Expect(out.FileContent).ToNot(Equal("secret")) + }) + + It("cannot write outside the mount with ..", func() { + out := call(sandboxInput{Operation: "write_file", FilePath: "../outside/via-dotdot.txt", Content: "escaped"}) + + Expect(out.Error).ToNot(BeNil()) + Expect(filepath.Join(outsideDir, "via-dotdot.txt")).ToNot(BeAnExistingFile()) + }) +}) + +var _ = Describe("Plugin filesystem sandbox without library access", Ordered, func() { + var manager *Manager + var libraryDir string + + BeforeAll(func() { + tmpDir := GinkgoT().TempDir() + libraryDir = filepath.Join(tmpDir, "music-library") + Expect(os.MkdirAll(libraryDir, 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(libraryDir, "track.txt"), []byte("audio"), 0600)).To(Succeed()) + + manager = startSandboxManager(tmpDir, libraryDir, func(*model.Plugin) {}) + }) + + It("mounts nothing when no library is granted", func() { + manager.mu.RLock() + p := manager.plugins["test-library"] + manager.mu.RUnlock() + Expect(p.fsConfig).To(BeNil()) + + out := callSandbox(manager, sandboxInput{Operation: "read_file", MountPoint: toPluginMountPoint(1), FilePath: "track.txt"}) + Expect(out.Error).ToNot(BeNil()) + }) +}) diff --git a/plugins/testdata/test-library/main.go b/plugins/testdata/test-library/main.go index 9c04c5f9b..e83c9aec6 100644 --- a/plugins/testdata/test-library/main.go +++ b/plugins/testdata/test-library/main.go @@ -14,10 +14,12 @@ import ( // TestLibraryInput is the input for nd_test_library callback. type TestLibraryInput struct { - Operation string `json:"operation"` // "get_library", "get_all_libraries", "read_file", "list_dir" + Operation string `json:"operation"` // "get_library", "get_all_libraries", "read_file", "list_dir", "write_file", "symlink" LibraryID int32 `json:"library_id,omitempty"` MountPoint string `json:"mount_point,omitempty"` // For filesystem operations FilePath string `json:"file_path,omitempty"` // For read_file operation (relative to mount point) + Content string `json:"content,omitempty"` // For write_file operation + Target string `json:"target,omitempty"` // For symlink operation } // TestLibraryOutput is the output from nd_test_library callback. @@ -88,6 +90,28 @@ func ndTestLibrary() int32 { pdk.OutputJSON(TestLibraryOutput{DirEntries: names}) return 0 + case "write_file": + // Write a file to the mounted library directory + fullPath := filepath.Join(input.MountPoint, input.FilePath) + if err := os.WriteFile(fullPath, []byte(input.Content), 0600); err != nil { + errStr := err.Error() + pdk.OutputJSON(TestLibraryOutput{Error: &errStr}) + return 0 + } + pdk.OutputJSON(TestLibraryOutput{}) + return 0 + + case "symlink": + // Create a symlink inside the mounted library directory + fullPath := filepath.Join(input.MountPoint, input.FilePath) + if err := os.Symlink(input.Target, fullPath); err != nil { + errStr := err.Error() + pdk.OutputJSON(TestLibraryOutput{Error: &errStr}) + return 0 + } + pdk.OutputJSON(TestLibraryOutput{}) + return 0 + default: errStr := "unknown operation: " + input.Operation pdk.OutputJSON(TestLibraryOutput{Error: &errStr}) diff --git a/resources/i18n/zh-Hans.json b/resources/i18n/zh-Hans.json index 21778506a..24f55357a 100644 --- a/resources/i18n/zh-Hans.json +++ b/resources/i18n/zh-Hans.json @@ -32,7 +32,7 @@ "rating": "评分", "quality": "品质", "bpm": "BPM", - "playDate": "最后一次播放", + "playDate": "上次播放", "createdAt": "加入日期", "grouping": "分组", "mood": "情绪", @@ -184,7 +184,7 @@ "maxBitRate": "最大比特率", "client": "客户端", "userName": "用户名", - "lastSeen": "上次浏览", + "lastSeen": "上次使用", "reportRealPath": "报告真实路径", "scrobbleEnabled": "发送个性化记录到外部服务" } @@ -210,7 +210,8 @@ "songCount": "歌曲数", "comment": "注释", "sync": "自动导入", - "path": "导入路径" + "path": "导入路径", + "starred": "收藏" }, "actions": { "selectPlaylist": "选择歌单", @@ -530,7 +531,7 @@ "no_results": "未找到结果", "no_more_results": "页码 %{page} 超出范围,尝试返回上一页", "page_out_of_boundaries": "页码 %{page} 超出范围", - "page_out_from_end": "已经最后一页", + "page_out_from_end": "已经是最后一页", "page_out_from_begin": "已经是第一页", "page_range_info": "%{offsetBegin}-%{offsetEnd} / %{total}", "page_rows_per_page": "每页项目数:", @@ -636,6 +637,7 @@ "albumList": "专辑", "playlists": "歌单", "sharedPlaylists": "共享的歌单", + "onlyFavourites": "仅显示收藏项", "about": "关于" }, "player": { @@ -668,10 +670,10 @@ "homepage": "主页", "source": "源代码", "featureRequests": "功能需求", - "lastInsightsCollection": "最近的分析收集", + "lastInsightsCollection": "上次的匿名数据收集", "insights": { - "disabled": "禁用", - "waiting": "等待" + "disabled": "已禁用", + "waiting": "等待中" } }, "tabs": { diff --git a/server/jellyfin/README.md b/server/jellyfin/README.md index 17d2797ab..13ec79aa8 100644 --- a/server/jellyfin/README.md +++ b/server/jellyfin/README.md @@ -90,9 +90,9 @@ skipped, so it doesn't create a nameless player. Navidrome item ids are **hex-encoded at the API boundary** (`dto.EncodeID`/`DecodeID`): every id is hex-encoded on the way out and hex-decoded on the way in. This is required because some clients parse ids as radix-16 — Finamp's queue `packIds`, for instance, does `int.parse(chunk, radix:16)`, -which chokes on Navidrome's base-62 nanoids (e.g. `5QFKvMsJrd57QE2Le2dKKo`). Because a raw MD5 id -from an old migrated library is itself valid hex, correctness depends on every emit path encoding -and every receive path decoding — see `dto/ids.go`. +which chokes on Navidrome's base62 ids (e.g. `5QFKvMsJrd57QE2Le2dKKo`). Because a base62 id can +itself be valid hex, correctness depends on every emit path encoding and every receive path +decoding — see `dto/ids.go`. ## Multi-library behavior @@ -184,13 +184,13 @@ their Navidrome `ArtworkID`. Real Jellyfin item ids are GUIDs — 128-bit values, always 32 hex characters. Finamp relies on that when persisting its play queue across restarts: `packIds()` bit-packs every id into exactly 16 -bytes. Navidrome ids are longer (nanoid ids can exceed 128 bits, so they cannot be mapped into -GUIDs), which means Finamp silently stores only the first 16 characters of each id and asks for -those **truncated ids** back when restoring the queue — item lookups, then streaming, images, -favorites and playback reports for the restored tracks. +bytes. Navidrome ids are 22-character base62 strings, not 32-hex GUIDs, which means Finamp silently +stores only the first 16 characters of each id and asks for those **truncated ids** back when +restoring the queue — item lookups, then streaming, images, favorites and playback reports for the +restored tracks. This API compensates server-side (`truncated_ids.go`): a 16-character id — a length no Navidrome -id family uses — is resolved to the full id by unique-prefix lookup (an indexed range scan; +id uses — is resolved to the full id by unique-prefix lookup (an indexed range scan; ambiguity is detected and fails safe). The `/Items?ids=` batch response echoes the id **as requested**, because Finamp matches restored items back to its stored ids, and the other item endpoints accept truncated ids transparently. diff --git a/server/jellyfin/dto/ids.go b/server/jellyfin/dto/ids.go index 3490ba260..2ac7e2a79 100644 --- a/server/jellyfin/dto/ids.go +++ b/server/jellyfin/dto/ids.go @@ -3,7 +3,7 @@ package dto import "encoding/hex" // EncodeID renders a Navidrome id as lowercase hex; Jellyfin clients parse ids as radix-16 (e.g. -// Finamp's queue packing) and crash on Navidrome's base62 nanoids if emitted as-is. +// Finamp's queue packing) and crash on Navidrome's base62 ids if emitted as-is. func EncodeID(id string) string { if id == "" { return "" diff --git a/server/jellyfin/dto/ids_test.go b/server/jellyfin/dto/ids_test.go index 26a957604..d85786d99 100644 --- a/server/jellyfin/dto/ids_test.go +++ b/server/jellyfin/dto/ids_test.go @@ -6,7 +6,7 @@ import ( ) var _ = Describe("id codec", func() { - It("round-trips a base62 nanoid through Encode/Decode", func() { + It("round-trips a base62 id", func() { id := "5QFKvMsJrd57QE2Le2dKKo" Expect(DecodeID(EncodeID(id))).To(Equal(id)) }) diff --git a/server/jellyfin/truncated_ids.go b/server/jellyfin/truncated_ids.go index b58296391..bb3e4ce9b 100644 --- a/server/jellyfin/truncated_ids.go +++ b/server/jellyfin/truncated_ids.go @@ -11,8 +11,8 @@ import ( ) // truncatedIDLen is what Finamp's saved-queue persistence cuts item ids to (16 bytes, assuming -// Jellyfin GUIDs). No Navidrome id family is 16 chars (nanoid=22, legacy MD5=32, playlist -// UUID=36), so the length alone identifies a truncated id. See README. +// Jellyfin GUIDs). All Navidrome ids are 22 chars (share ids 10), so length alone flags a +// truncated id. See README. // // Handlers taking an item id resolve it via resolveItemID/resolveItemIDs; playlist-write handlers // and ParentId scoping don't (a restored queue never edits playlists or browses by container id). diff --git a/server/public/handle_downloads.go b/server/public/handle_downloads.go index 6aa35c341..0012c4b35 100644 --- a/server/public/handle_downloads.go +++ b/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) } diff --git a/server/public/handle_downloads_test.go b/server/public/handle_downloads_test.go new file mode 100644 index 000000000..1a97f4379 --- /dev/null +++ b/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()) + }) +}) diff --git a/server/public/handle_images.go b/server/public/handle_images.go index 50f9238e5..633665e0f 100644 --- a/server/public/handle_images.go +++ b/server/public/handle_images.go @@ -68,7 +68,7 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) { } func decodeArtworkID(tokenString string) (model.ArtworkID, error) { - token, err := auth.TokenAuth.Decode(tokenString) + token, err := auth.PublicTokenAuth.Decode(tokenString) if err != nil { return model.ArtworkID{}, err } diff --git a/server/public/handle_images_test.go b/server/public/handle_images_test.go index 6895241f6..669f879a6 100644 --- a/server/public/handle_images_test.go +++ b/server/public/handle_images_test.go @@ -9,7 +9,7 @@ import ( var _ = Describe("decodeArtworkID", func() { BeforeEach(func() { - auth.TokenAuth = jwtauth.New("HS256", []byte("super secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("super secret"), nil) }) It("fails to decode an invalid token", func() { diff --git a/server/public/handle_shares.go b/server/public/handle_shares.go index 76f674483..13a7e4c32 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -111,8 +111,8 @@ func (pub *Router) mapShareToM3U(r *http.Request, s model.Share) *model.Share { // 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. +// necessarily already holds the public-link signing secret, a full-server +// compromise that is out of scope for the share boundary specifically. func encodeMediafileShare(s model.Share, id string) string { claims := auth.Claims{ ID: id, diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index 15abab693..3d624f661 100644 --- a/server/public/handle_streams.go +++ b/server/public/handle_streams.go @@ -107,7 +107,7 @@ func shareContainsTrack(share *model.Share, mediaFileID string) bool { // 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) + c, err := auth.ValidatePublic(tokenString) if err != nil { return shareTrackInfo{}, err } diff --git a/server/public/handle_streams_test.go b/server/public/handle_streams_test.go index 2f32ea6f2..870dfa8ef 100644 --- a/server/public/handle_streams_test.go +++ b/server/public/handle_streams_test.go @@ -29,7 +29,7 @@ func (m *mockStreamer) NewStream(_ context.Context, _ *model.MediaFile, r stream var _ = Describe("decodeStreamInfo", func() { BeforeEach(func() { - auth.TokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) }) It("decodes a valid token with all fields", func() { @@ -81,7 +81,7 @@ var _ = Describe("decodeStreamInfo", func() { var _ = Describe("encodeMediafileShare", func() { BeforeEach(func() { - auth.TokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) }) It("includes the share ID in the token", func() { @@ -113,7 +113,7 @@ var _ = Describe("handleStream", func() { var pub *Router BeforeEach(func() { - auth.TokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test-secret"), nil) ds = &tests.MockDataStore{} shareRepo = &tests.MockShareRepo{} ds.MockedShare = shareRepo diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index f6a7047c4..59d267321 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -381,13 +381,14 @@ func (api *Router) GetSimilarSongs2(r *http.Request) (*responses.Subsonic, error func (api *Router) GetTopSongs(r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() p := req.Params(r) + id, idErr := p.String("id") artist, err := p.String("artist") - if err != nil { + if err != nil && idErr != nil { return nil, err } count := p.IntOr("count", 50) - songs, err := api.provider.TopSongs(ctx, artist, count) + songs, err := api.provider.TopSongs(ctx, artist, id, count) if err != nil && !errors.Is(err, model.ErrNotFound) { return nil, err } diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 6875b6370..97d7fb40a 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/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 } @@ -353,7 +353,7 @@ func (n noopProvider) SimilarSongs(context.Context, string, int) (model.MediaFil return nil, nil } -func (n noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, error) { +func (n noopProvider) TopSongs(context.Context, string, string, int) (model.MediaFiles, error) { return nil, nil } diff --git a/server/subsonic/e2e/subsonic_browsing_test.go b/server/subsonic/e2e/subsonic_browsing_test.go index 55aeb8e9e..992f9e0fb 100644 --- a/server/subsonic/e2e/subsonic_browsing_test.go +++ b/server/subsonic/e2e/subsonic_browsing_test.go @@ -13,6 +13,15 @@ var _ = Describe("Browsing Endpoints", func() { setupTestDB() }) + getBeatlesId := func() string { + artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"name": "The Beatles"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(artists).ToNot(BeEmpty()) + return artists[0].ID + } + Describe("getMusicFolders", func() { It("returns the configured music library", func() { resp := doReq("getMusicFolders") @@ -85,12 +94,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getMusicDirectory", func() { It("returns an artist directory with its albums as children", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getMusicDirectory", "id", beatlesID) @@ -125,12 +129,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtist", func() { It("returns artist with albums in ID3 format", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getArtist", "id", beatlesID) @@ -141,12 +140,7 @@ var _ = Describe("Browsing Endpoints", func() { }) It("returns album names for the artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getArtist", "id", beatlesID) @@ -381,13 +375,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtistInfo", func() { It("returns artist info for a valid artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID - + beatlesID := getBeatlesId() resp := doReq("getArtistInfo", "id", beatlesID) Expect(resp.Status).To(Equal(responses.StatusOK)) @@ -397,13 +385,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtistInfo2", func() { It("returns artist info2 for a valid artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID - + beatlesID := getBeatlesId() resp := doReq("getArtistInfo2", "id", beatlesID) Expect(resp.Status).To(Equal(responses.StatusOK)) @@ -426,6 +408,28 @@ var _ = Describe("Browsing Endpoints", func() { Expect(resp.TopSongs).ToNot(BeNil()) Expect(resp.TopSongs.Song).To(BeEmpty()) }) + + It("returns an error if no arguments are specified", func() { + resp := doReq("getTopSongs") + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.TopSongs).To(BeNil()) + }) + + It("returns a response only by id", func() { + beatlesID := getBeatlesId() + resp := doReq("getTopSongs", "id", beatlesID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TopSongs).ToNot(BeNil()) + }) + + It("returns a response when both id and name are included", func() { + beatlesID := getBeatlesId() + resp := doReq("getTopSongs", "id", beatlesID, "artist", "The Beatles") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TopSongs).ToNot(BeNil()) + }) }) Describe("getSimilarSongs", func() { diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index 3741462a2..8db624649 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -22,6 +22,7 @@ var _ = Describe("helpers", func() { BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) auth.TokenAuth = jwtauth.New("HS256", []byte("test secret"), nil) + auth.PublicTokenAuth = jwtauth.New("HS256", []byte("test public secret"), nil) }) Describe("fakePath", func() { diff --git a/server/subsonic/opensubsonic.go b/server/subsonic/opensubsonic.go index 97b3cafcc..2b2a31bf3 100644 --- a/server/subsonic/opensubsonic.go +++ b/server/subsonic/opensubsonic.go @@ -15,6 +15,7 @@ func (api *Router) GetOpenSubsonicExtensions(_ *http.Request) (*responses.Subson {Name: "indexBasedQueue", Versions: []int32{1}}, {Name: "transcoding", Versions: []int32{1}}, {Name: "playbackReport", Versions: []int32{1}}, + {Name: "topSongsByArtistId", Versions: []int32{1}}, } if api.sonic != nil && api.sonic.HasProvider() { extensions = append(extensions, responses.OpenSubsonicExtension{ diff --git a/server/subsonic/opensubsonic_test.go b/server/subsonic/opensubsonic_test.go index e4217303f..2615a652d 100644 --- a/server/subsonic/opensubsonic_test.go +++ b/server/subsonic/opensubsonic_test.go @@ -55,13 +55,14 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(6), + HaveLen(7), ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), )) Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), @@ -85,7 +86,7 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(7), + HaveLen(8), ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), @@ -93,6 +94,7 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), )) }) }) diff --git a/ui/public/offline.html b/ui/public/offline.html index 72f2d04ae..e3c44e8d3 100644 --- a/ui/public/offline.html +++ b/ui/public/offline.html @@ -5,6 +5,6 @@

It looks like we are having trouble connecting.
-Please check your internet connection and try again.

+Please check your internet connection, or try again in a moment.

\ No newline at end of file diff --git a/ui/src/authProvider.js b/ui/src/authProvider.js index 813a4f5b4..18badbc9c 100644 --- a/ui/src/authProvider.js +++ b/ui/src/authProvider.js @@ -1,7 +1,6 @@ import { jwtDecode } from 'jwt-decode' import { baseUrl } from './utils' import config from './config' -import { removeHomeCache } from './utils/removeHomeCache' // config sent from server may contain authentication info, for example when the user is authenticated // by a reverse proxy request header @@ -49,7 +48,6 @@ const authProvider = { storeAuthenticationInfo(response) // Avoid "going to create admin" dialog after logout/login without a refresh config.firstTime = false - removeHomeCache() return response }) .catch((error) => { diff --git a/ui/src/dataProvider/httpClient.js b/ui/src/dataProvider/httpClient.js index 3820f658d..706486fa6 100644 --- a/ui/src/dataProvider/httpClient.js +++ b/ui/src/dataProvider/httpClient.js @@ -3,7 +3,6 @@ import { v4 as uuidv4 } from 'uuid' import { baseUrl } from '../utils' import config from '../config' import { jwtDecode } from 'jwt-decode' -import { removeHomeCache } from '../utils/removeHomeCache' const customAuthorizationHeader = 'X-ND-Authorization' export const clientUniqueIdHeader = 'X-ND-Client-Unique-Id' @@ -27,7 +26,6 @@ const httpClient = (url, options = {}) => { localStorage.setItem('userId', decoded.uid) // Avoid going to create admin dialog after logout/login without a refresh config.firstTime = false - removeHomeCache() } return response }) diff --git a/ui/src/share/SharePlayer.jsx b/ui/src/share/SharePlayer.jsx index a3a15e50a..2384866ea 100644 --- a/ui/src/share/SharePlayer.jsx +++ b/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 = () => { ) } diff --git a/ui/src/share/SharePlayer.test.jsx b/ui/src/share/SharePlayer.test.jsx new file mode 100644 index 000000000..68d1eff59 --- /dev/null +++ b/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
+ }, +})) + +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() + + 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() + + 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() + + 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() + + 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() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + act(() => { + playerProps.customDownloader() + }) + unmount() + act(() => { + vi.runAllTimers() + }) + + expect(errorSpy).not.toHaveBeenCalled() + }) +}) diff --git a/ui/src/sw.js b/ui/src/sw.js index f4a5664b8..edc272850 100644 --- a/ui/src/sw.js +++ b/ui/src/sw.js @@ -1,5 +1,7 @@ /* eslint-disable */ +import { createNavigationHandler } from './swNavigation' + // documentation: https://developers.google.com/web/tools/workbox/modules/workbox-sw importScripts('3rdparty/workbox/workbox-sw.js') @@ -36,17 +38,13 @@ self.addEventListener('install', async (event) => { }) const networkOnly = new workbox.strategies.NetworkOnly() -const navigationHandler = async (params) => { - try { - // Attempt a network request. - return await networkOnly.handle(params) - } catch (error) { - // If it fails, return the cached HTML. - return caches.match(FALLBACK_HTML_URL, { - cacheName: CACHE_NAME, - }) - } -} +const fallbackToCachedHtml = () => + caches.match(FALLBACK_HTML_URL, { cacheName: CACHE_NAME }) + +const navigationHandler = createNavigationHandler( + networkOnly, + fallbackToCachedHtml, +) // self.__WB_MANIFEST is default injection point workbox.precaching.precacheAndRoute(self.__WB_MANIFEST) diff --git a/ui/src/swNavigation.js b/ui/src/swNavigation.js new file mode 100644 index 000000000..28d01eaef --- /dev/null +++ b/ui/src/swNavigation.js @@ -0,0 +1,14 @@ +// Lives outside sw.js so it can be unit tested: sw.js only loads inside a +// service worker, where workbox arrives via importScripts. +export const createNavigationHandler = + (networkOnly, offlineFallback) => async (params) => { + try { + // Attempt a network request. + const response = await networkOnly.handle(params) + // A 5xx reaches us as a normal response, but carries no usable app + return response.status >= 500 ? offlineFallback() : response + } catch (error) { + // If it fails, return the cached HTML. + return offlineFallback() + } + } diff --git a/ui/src/swNavigation.test.js b/ui/src/swNavigation.test.js new file mode 100644 index 000000000..8ab749757 --- /dev/null +++ b/ui/src/swNavigation.test.js @@ -0,0 +1,62 @@ +import { describe, it, expect, vi } from 'vitest' +import { createNavigationHandler } from './swNavigation' + +const OFFLINE = { offlinePage: true } + +const setup = (networkResult) => { + const networkOnly = { + handle: vi.fn(() => + networkResult instanceof Error + ? Promise.reject(networkResult) + : Promise.resolve(networkResult), + ), + } + const offlineFallback = vi.fn(() => Promise.resolve(OFFLINE)) + return { + networkOnly, + offlineFallback, + handler: createNavigationHandler(networkOnly, offlineFallback), + } +} + +describe('createNavigationHandler', () => { + it('serves the response from the network', async () => { + const response = { status: 200 } + const { handler, offlineFallback } = setup(response) + + await expect(handler({})).resolves.toBe(response) + expect(offlineFallback).not.toHaveBeenCalled() + }) + + it('falls back to the offline page when the network fails', async () => { + const { handler } = setup(new Error('Failed to fetch')) + + await expect(handler({})).resolves.toBe(OFFLINE) + }) + + it.each([500, 502, 503])( + 'falls back to the offline page on %i', + async (status) => { + const { handler } = setup({ status }) + + await expect(handler({})).resolves.toBe(OFFLINE) + }, + ) + + it.each([304, 401, 404])('passes %i through untouched', async (status) => { + const response = { status } + const { handler, offlineFallback } = setup(response) + + await expect(handler({})).resolves.toBe(response) + expect(offlineFallback).not.toHaveBeenCalled() + }) + + it('passes the navigation params to the network strategy', async () => { + const params = { request: { url: '/app/' } } + const { handler, networkOnly } = setup({ status: 200 }) + + await handler(params) + + expect(networkOnly.handle).toHaveBeenCalledWith(params) + }) +}) diff --git a/ui/src/utils/removeHomeCache.js b/ui/src/utils/removeHomeCache.js deleted file mode 100644 index 08ed720e0..000000000 --- a/ui/src/utils/removeHomeCache.js +++ /dev/null @@ -1,20 +0,0 @@ -export const removeHomeCache = async () => { - try { - const workboxKey = (await caches.keys()).find((key) => - key.startsWith('workbox-precache'), - ) - if (!workboxKey) return - - const workboxCache = await caches.open(workboxKey) - const indexKey = (await workboxCache.keys()).find((key) => - key.url.includes('app/index.html'), - ) - - if (indexKey) { - await workboxCache.delete(indexKey) - } - } catch (e) { - // eslint-disable-next-line no-console - console.error('error reading cache', e) - } -} diff --git a/ui/vite.config.js b/ui/vite.config.js index 9d9c845f1..182e825b0 100644 --- a/ui/vite.config.js +++ b/ui/vite.config.js @@ -16,6 +16,9 @@ export default defineConfig({ filename: 'sw.js', injectManifest: { maximumFileSizeToCacheInBytes: 3 * 1024 * 1024, // 3 MiB + // index.html is rendered per-user by the server, so a precached copy + // would pin one user's config (and auth payload) across logins + globIgnores: ['index.html'], }, devOptions: { enabled: true,