From 810b14ed571412e7a7ddb568ebee69ac1498faea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sun, 2 Aug 2026 12:27:08 -0400 Subject: [PATCH 1/3] fix(plugins): confine plugin filesystem mounts to their root (#5881) * fix(plugins): confine plugin filesystem mounts to their root A plugin granted read-write filesystem access could escape its mount by creating a relative symlink inside it and then writing through that link, reaching any path the server process can write, including navidrome.db. wazero resolves guest paths by concatenating them onto the host root. Its WASI layer validates every path argument except the symlink target, which path_symlink forwards unvalidated by design, and fs.ValidPath splits on "/" only, so on Windows a "..\" path escapes the mount as well. Mounts now go through a jailedFS wrapper that denies symlink creation and rejects any path that is not filepath.IsLocal. That requires bypassing extism's AllowedPaths, which discards any FSConfig passed alongside it, so the mounts are built directly and applied per instance instead. Following symlinks that already exist in a mount is unchanged: music libraries rely on it, and read-only mounts already reject creating new ones. * test(plugins): guard against setting extism AllowedPaths Extracts the extism manifest construction so a test can assert AllowedPaths is never set. Setting it makes extism build its own FSConfig and discard the jailed mounts, silently restoring the symlink escape. Verified by simulating the regression: with AllowedPaths populated for plugins holding the filesystem permission, the new spec fails, as do two of the end-to-end sandbox specs. --- plugins/manager_loader.go | 59 +++------ plugins/manager_loader_test.go | 86 ++++-------- plugins/manager_plugin.go | 16 ++- plugins/plugins_suite_test.go | 62 +++++---- plugins/sandbox_fs.go | 157 ++++++++++++++++++++++ plugins/sandbox_fs_internal_test.go | 127 ++++++++++++++++++ plugins/sandbox_fs_test.go | 182 ++++++++++++++++++++++++++ plugins/testdata/test-library/main.go | 26 +++- 8 files changed, 572 insertions(+), 143 deletions(-) create mode 100644 plugins/sandbox_fs.go create mode 100644 plugins/sandbox_fs_internal_test.go create mode 100644 plugins/sandbox_fs_test.go diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 0ba5fcf76..fc9296427 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -307,22 +307,10 @@ 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() { adminCtx := adminContext(ctx) libraries, err := m.ds.Library(adminCtx).GetAll() @@ -330,8 +318,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return fmt.Errorf("failed to get libraries for filesystem access: %w", err) } - allowedPaths := buildAllowedPaths(ctx, libraries, allowedLibraries, p.AllLibraries, p.AllowWriteAccess) - pluginManifest.AllowedPaths = allowedPaths + fsConfig = buildFSConfig(buildMounts(ctx, libraries, allowedLibraries, p.AllLibraries, p.AllowWriteAccess)) } // Build host functions based on permissions from manifest @@ -386,7 +373,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) @@ -413,6 +400,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() @@ -459,31 +447,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}) From b40b41584ad5e4e08430aabcd2a0d6484c998bc2 Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Sun, 2 Aug 2026 09:39:51 -0700 Subject: [PATCH 2/3] feat(subsonic): Implement OpenSubsonic topSongsByArtistId extension (#5853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * implement topSongsByArtistId extension * do not look up by artist name if empty --------- Co-authored-by: Deluan Quintão --- core/artwork/e2e/helpers_test.go | 2 +- core/external/provider.go | 34 ++++++-- core/external/provider_topsongs_test.go | 84 +++++++++++++++---- server/subsonic/browsing.go | 5 +- server/subsonic/e2e/e2e_suite_test.go | 2 +- server/subsonic/e2e/subsonic_browsing_test.go | 68 ++++++++------- server/subsonic/opensubsonic.go | 1 + server/subsonic/opensubsonic_test.go | 6 +- 8 files changed, 143 insertions(+), 59 deletions(-) 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/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/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 4e9039d68..97d7fb40a 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -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/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}}), )) }) }) From f853ca604a534e6916ea9a2e2bda5a27dc185bb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sun, 2 Aug 2026 12:58:53 -0400 Subject: [PATCH 3/3] refactor(db): migrate all ids to a uniform canonical 128-bit base62 encoding (#5824) * refactor(model): extract canonical 128-bit base62 id codec * feat(model): generate random ids as canonical 128-bit base62 values * feat(scanner): emit legacy PIDs in canonical base62 encoding * feat(db): add id canonicalization transform for the uniform-ids migration * feat(db): migrate all ids to canonical 128-bit base62 encoding * fix(db): canonicalize ids in junction tables and JSON columns * chore(jellyfin): update id-family notes for uniform canonical ids * test(ids): harden codec input contract and migration edge coverage * refactor(model): use log.Fatal for Encode128 contract guard per project convention * fix(db): force full rescan after id migration for legacy PID configs * test(db): guard id-column inventory against schema drift * refactor(ids): compile-time Encode128 contract and unified column rewrite helper * refactor(db): apply review feedback to id migration Filter empty strings in collectColumn's SQL, reuse a prepared statement for rewriteColumn updates, and clarify the legacy ID functions' comment now that they emit the canonical encoding. * feat(auth): split session and public-link JWT secrets, rotating sessions on id migration * test(subsonic): initialize public token secret in helpers suite The suite sets auth.TokenAuth directly instead of calling auth.Init, so the new PublicTokenAuth was nil whenever Ginkgo's spec order ran a helpers spec before any spec that calls auth.Init, panicking in publicurl.ImageURL. * refactor(db): inline canonicalID into its only consumer, the uniform-ids migration * refactor(model): rename Encode128/Decode128 to Encode/Decode With every id now exactly 128 bits, the width suffix is redundant; the package-qualified id.Encode/id.Decode carries the same information. * test(db): make the id-columns guard classify JSON columns too The guard only inspected columns named id/pid/*_id, so it could not see ids embedded in JSON. Widen it to *_ids and to every JSON column, and drive the "covered" set from a new embeddedIDColumns list instead of the inline calls in the migration. Every JSON column the schema has now carries a verdict. The four denormalized caches -- media_file/album.participants, media_file/album.tags, album.folder_ids and artist.similar_artists -- hold only artist, tag and folder ids. Those all come from id.NewHash, whose 22-char base62 encoding of a 128-bit MD5 is already in canonical range, so canonicalID is the identity on them and the migration correctly leaves them alone. A new codec test pins that invariant, since the exemptions depend on it. Verified on a copy of a 727MB/96k-track production database: canonicalizing those four columns changed zero rows, and artist, tag and folder ids were themselves unchanged by the migration (only media_file ids moved, 95108 of 96666). --- consts/consts.go | 1 + core/auth/auth.go | 53 +-- core/auth/auth_test.go | 45 +++ core/publicurl/publicurl_test.go | 2 +- .../20260720015443_uniform_canonical_ids.go | 310 ++++++++++++++++++ db/migrations/id_canonical_test.go | 47 +++ db/migrations/id_columns_guard_test.go | 119 +++++++ db/migrations/migrations_suite_test.go | 16 + db/migrations/uniform_canonical_ids_test.go | 233 +++++++++++++ model/id/id.go | 36 +- model/id/id_suite_test.go | 17 + model/id/id_test.go | 64 ++++ model/metadata/legacy_ids.go | 15 +- model/metadata/persistent_ids_test.go | 15 + plugins/host_artwork_test.go | 2 +- server/jellyfin/README.md | 16 +- server/jellyfin/dto/ids.go | 2 +- server/jellyfin/dto/ids_test.go | 2 +- server/jellyfin/truncated_ids.go | 4 +- server/public/handle_images.go | 2 +- server/public/handle_images_test.go | 2 +- server/public/handle_shares.go | 4 +- server/public/handle_streams.go | 2 +- server/public/handle_streams_test.go | 6 +- server/subsonic/helpers_test.go | 1 + 25 files changed, 957 insertions(+), 59 deletions(-) create mode 100644 db/migrations/20260720015443_uniform_canonical_ids.go create mode 100644 db/migrations/id_canonical_test.go create mode 100644 db/migrations/id_columns_guard_test.go create mode 100644 db/migrations/migrations_suite_test.go create mode 100644 db/migrations/uniform_canonical_ids_test.go create mode 100644 model/id/id_suite_test.go create mode 100644 model/id/id_test.go 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/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/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/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_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/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() {