Merge branch 'master' into plugin-persistent-storage

plugins/manager_loader.go

@@ -282,6 +282,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()

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("")

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))

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()

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)
 

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 {

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