diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index fc9296427..5238a4755 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -271,6 +271,10 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return fmt.Errorf("manager is stopped") } + if !validPluginID(p.ID) { + return fmt.Errorf("invalid plugin ID %q", p.ID) + } + // Track this operation m.loadWg.Add(1) defer m.loadWg.Done() diff --git a/plugins/manager_loader_test.go b/plugins/manager_loader_test.go index 5e6851481..6326b2218 100644 --- a/plugins/manager_loader_test.go +++ b/plugins/manager_loader_test.go @@ -1,6 +1,7 @@ package plugins import ( + "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -27,6 +28,18 @@ var _ = Describe("buildExtismManifest", func() { }) }) +var _ = Describe("loadPluginWithConfig", func() { + // Discovery already rejects these, but a row predating that check, or one + // left behind by a failed sync, must not reach the mount setup + It("refuses a plugin whose ID is not usable as a directory name", func() { + m := &Manager{plugins: make(map[string]*plugin)} + + err := m.loadPluginWithConfig(&model.Plugin{ID: "..", Path: "/does/not/matter.ndp"}) + + Expect(err).To(MatchError(ContainSubstring("invalid plugin ID"))) + }) +}) + var _ = Describe("parsePluginConfig", func() { It("returns nil for empty string", func() { result, err := parsePluginConfig("") diff --git a/plugins/manager_sync.go b/plugins/manager_sync.go index 2119f1a5a..480fa1bb9 100644 --- a/plugins/manager_sync.go +++ b/plugins/manager_sync.go @@ -152,7 +152,11 @@ func (m *Manager) syncPlugins(ctx context.Context, folder string) error { log.Trace(ctx, "Skipping non-plugin entry", "name", entry.Name(), "isDir", entry.IsDir()) continue } - name := strings.TrimSuffix(entry.Name(), PackageExtension) + name, ok := pluginIDFromPath(entry.Name()) + if !ok { + log.Warn(ctx, "Skipping plugin with unusable name", "name", entry.Name()) + continue + } filesOnDisk[name] = filepath.Join(folder, entry.Name()) } log.Debug(ctx, "Plugin sync: scanned folder", "folder", folder, "entriesTotal", len(entries), "pluginsFound", len(filesOnDisk)) diff --git a/plugins/manager_sync_test.go b/plugins/manager_sync_test.go index 26da2079b..dd64dcd3f 100644 --- a/plugins/manager_sync_test.go +++ b/plugins/manager_sync_test.go @@ -12,6 +12,46 @@ import ( . "github.com/onsi/gomega" ) +var _ = Describe("syncPlugins", func() { + var m *Manager + var repo *tests.MockPluginRepo + var folder string + + BeforeEach(func() { + folder = GinkgoT().TempDir() + repo = tests.CreateMockPluginRepo() + repo.SetData(model.Plugins{}) + m = &Manager{ds: &tests.MockDataStore{MockedPlugin: repo}} + }) + + writePackage := func(name string) { + GinkgoHelper() + manifest := &Manifest{Name: "Test Plugin", Author: "Test Author", Version: "1.0.0"} + wasm := []byte{0x00, 0x61, 0x73, 0x6d} // Minimal wasm header + Expect(createTestPackage(filepath.Join(folder, name), manifest, wasm)).To(Succeed()) + } + + It("registers a plugin with a usable ID", func() { + writePackage("my-plugin" + PackageExtension) + + Expect(m.syncPlugins(context.Background(), folder)).To(Succeed()) + + _, err := repo.Get("my-plugin") + Expect(err).ToNot(HaveOccurred()) + }) + + It("skips packages whose name yields a path-like ID", func() { + writePackage("." + PackageExtension) + writePackage(".." + PackageExtension) + + Expect(m.syncPlugins(context.Background(), folder)).To(Succeed()) + + all, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(all).To(BeEmpty()) + }) +}) + var _ = Describe("removePluginFromDB", func() { It("discards buffered scrobbles for the removed plugin", func() { ctx := context.Background() diff --git a/plugins/manager_watcher.go b/plugins/manager_watcher.go index d666d8620..f7f658be9 100644 --- a/plugins/manager_watcher.go +++ b/plugins/manager_watcher.go @@ -89,7 +89,11 @@ func (m *Manager) handleWatcherEvent(event notify.EventInfo) { return } - pluginName := strings.TrimSuffix(filepath.Base(path), PackageExtension) + pluginName, ok := pluginIDFromPath(path) + if !ok { + log.Warn(m.ctx, "Ignoring plugin file with unusable name", "path", path) + return + } log.Trace(m.ctx, "Plugin file event", "plugin", pluginName, "event", event.Event(), "path", path) diff --git a/plugins/package.go b/plugins/package.go index e0405d4d8..77cb6e5ac 100644 --- a/plugins/package.go +++ b/plugins/package.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "io" + "path/filepath" + "strings" ) const ( @@ -18,6 +20,26 @@ const ( wasmFileName = "plugin.wasm" ) +// validPluginID reports whether id is usable. It names a directory under +// DataFolder/plugins, so a path-like one would point at another plugin's data. +func validPluginID(id string) bool { + if id == "." || id == ".." || strings.ContainsAny(id, `/\`) || !filepath.IsLocal(id) { + return false + } + // Windows drops trailing dots and spaces, so "foo." and "foo" would end up + // sharing a directory + return strings.TrimRight(id, ". ") == id +} + +// pluginIDFromPath derives the plugin ID from a package path. +func pluginIDFromPath(path string) (string, bool) { + id := strings.TrimSuffix(filepath.Base(path), PackageExtension) + if !validPluginID(id) { + return "", false + } + return id, true +} + // ndpPackage represents a loaded .ndp plugin package. // It contains the manifest and wasm bytes read from the archive. type ndpPackage struct { diff --git a/plugins/package_test.go b/plugins/package_test.go index 4a37f4352..953797750 100644 --- a/plugins/package_test.go +++ b/plugins/package_test.go @@ -11,6 +11,36 @@ import ( . "github.com/onsi/gomega" ) +var _ = Describe("pluginIDFromPath", func() { + DescribeTable("derives the ID from the package filename", + func(path, expected string) { + id, ok := pluginIDFromPath(path) + Expect(ok).To(BeTrue()) + Expect(id).To(Equal(expected)) + }, + Entry("plain name", "/plugins/discord-rich-presence.ndp", "discord-rich-presence"), + Entry("name with spaces", "/plugins/My Plugin.ndp", "My Plugin"), + Entry("leading dot", "/plugins/.hidden.ndp", ".hidden"), + Entry("dots inside", "/plugins/v1.2.3.ndp", "v1.2.3"), + ) + + // The ID becomes a directory name under DataFolder/plugins, so a path-like + // one would let a plugin reach another plugin's data + DescribeTable("rejects IDs that are unsafe as a directory name", + func(path string) { + _, ok := pluginIDFromPath(path) + Expect(ok).To(BeFalse()) + }, + Entry("empty", "/plugins/.ndp"), + Entry("current directory", "/plugins/..ndp"), + Entry("parent directory", "/plugins/...ndp"), + // Windows drops trailing dots and spaces, so these would share a + // directory with "foo" + Entry("trailing dot", "/plugins/foo..ndp"), + Entry("trailing space", "/plugins/foo .ndp"), + ) +}) + var _ = Describe("ndpPackage", func() { var tmpDir string