From 0da2352907993150ae8a772e9fe145b436c48243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 23 Jul 2025 20:46:47 -0400 Subject: [PATCH] fix: improve URL path handling in local storage for special characters (#4378) * refactor: improve URL path handling in local storage system Enhanced the local storage implementation to properly handle URL-decoded paths and fix issues with file paths containing special characters. Added decodedPath field to localStorage struct to separate URL parsing concerns from file system operations. Key changes: - Added decodedPath field to localStorage struct for proper URL decoding - Modified newLocalStorage to use decoded path instead of modifying original URL - Fixed Windows path handling to work with decoded paths - Improved URL escaping in storage.For() to handle special characters - Added comprehensive test suite covering URL decoding, symlink resolution, Windows paths, and edge cases - Refactored test extractor to use mockTestExtractor for better isolation This ensures that file paths with spaces, hash symbols, and other special characters are handled correctly throughout the storage system. Signed-off-by: Deluan * fix(tests): fix test file permissions and add missing tests.Init call * refactor(tests): remove redundant test Signed-off-by: Deluan * fix: URL building for Windows and remove redundant variable Signed-off-by: Deluan * refactor: simplify URL path escaping in local storage Signed-off-by: Deluan --------- Signed-off-by: Deluan --- core/storage/local/local_suite_test.go | 6 +- core/storage/local/local_test.go | 428 +++++++++++++++++++++++++ core/storage/storage.go | 11 +- core/storage/storage_test.go | 15 + 4 files changed, 458 insertions(+), 2 deletions(-) create mode 100644 core/storage/local/local_test.go diff --git a/core/storage/local/local_suite_test.go b/core/storage/local/local_suite_test.go index 98dfcbd4b..5934cde5d 100644 --- a/core/storage/local/local_suite_test.go +++ b/core/storage/local/local_suite_test.go @@ -3,11 +3,15 @@ package local import ( "testing" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) func TestLocal(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) RegisterFailHandler(Fail) - RunSpecs(t, "Local Storage Test Suite") + RunSpecs(t, "Local Storage Suite") } diff --git a/core/storage/local/local_test.go b/core/storage/local/local_test.go new file mode 100644 index 000000000..3ed01bbc4 --- /dev/null +++ b/core/storage/local/local_test.go @@ -0,0 +1,428 @@ +package local + +import ( + "io/fs" + "net/url" + "os" + "path/filepath" + "runtime" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/storage" + "github.com/navidrome/navidrome/model/metadata" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("LocalStorage", func() { + var tempDir string + var testExtractor *mockTestExtractor + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + + // Create a temporary directory for testing + var err error + tempDir, err = os.MkdirTemp("", "navidrome-local-storage-test-") + Expect(err).ToNot(HaveOccurred()) + + DeferCleanup(func() { + os.RemoveAll(tempDir) + }) + + // Create and register a test extractor + testExtractor = &mockTestExtractor{ + results: make(map[string]metadata.Info), + } + RegisterExtractor("test", func(fs.FS, string) Extractor { + return testExtractor + }) + conf.Server.Scanner.Extractor = "test" + }) + + Describe("newLocalStorage", func() { + Context("with valid path", func() { + It("should create a localStorage instance with correct path", func() { + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + localStorage := storage.(*localStorage) + + Expect(localStorage.u.Scheme).To(Equal("file")) + // Check that the path is set correctly (could be resolved to real path on macOS) + Expect(localStorage.u.Path).To(ContainSubstring("navidrome-local-storage-test")) + Expect(localStorage.resolvedPath).To(ContainSubstring("navidrome-local-storage-test")) + Expect(localStorage.extractor).ToNot(BeNil()) + }) + + It("should handle URL-decoded paths correctly", func() { + // Create a directory with spaces to test URL decoding + spacedDir := filepath.Join(tempDir, "test folder") + err := os.MkdirAll(spacedDir, 0755) + Expect(err).ToNot(HaveOccurred()) + + // Use proper URL construction instead of manual escaping + u := &url.URL{ + Scheme: "file", + Path: spacedDir, + } + + storage := newLocalStorage(*u) + localStorage, ok := storage.(*localStorage) + Expect(ok).To(BeTrue()) + + Expect(localStorage.u.Path).To(Equal(spacedDir)) + }) + + It("should resolve symlinks when possible", func() { + // Create a real directory and a symlink to it + realDir := filepath.Join(tempDir, "real") + linkDir := filepath.Join(tempDir, "link") + + err := os.MkdirAll(realDir, 0755) + Expect(err).ToNot(HaveOccurred()) + + err = os.Symlink(realDir, linkDir) + Expect(err).ToNot(HaveOccurred()) + + u, err := url.Parse("file://" + linkDir) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + localStorage, ok := storage.(*localStorage) + Expect(ok).To(BeTrue()) + + Expect(localStorage.u.Path).To(Equal(linkDir)) + // Check that the resolved path contains the real directory name + Expect(localStorage.resolvedPath).To(ContainSubstring("real")) + }) + + It("should use u.Path as resolvedPath when symlink resolution fails", func() { + // Use a non-existent path to trigger symlink resolution failure + nonExistentPath := filepath.Join(tempDir, "non-existent") + + u, err := url.Parse("file://" + nonExistentPath) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + localStorage, ok := storage.(*localStorage) + Expect(ok).To(BeTrue()) + + Expect(localStorage.u.Path).To(Equal(nonExistentPath)) + Expect(localStorage.resolvedPath).To(Equal(nonExistentPath)) + }) + }) + + Context("with Windows path", func() { + BeforeEach(func() { + if runtime.GOOS != "windows" { + Skip("Windows-specific test") + } + }) + + It("should handle Windows drive letters correctly", func() { + u, err := url.Parse("file://C:/music") + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + localStorage, ok := storage.(*localStorage) + Expect(ok).To(BeTrue()) + + Expect(localStorage.u.Path).To(Equal("C:/music")) + }) + }) + + Context("with invalid extractor", func() { + It("should handle extractor validation correctly", func() { + // Note: The actual implementation uses log.Fatal which exits the process, + // so we test the normal path where extractors exist + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + Expect(storage).ToNot(BeNil()) + }) + }) + }) + + Describe("localStorage.FS", func() { + Context("with existing directory", func() { + It("should return a localFS instance", func() { + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + Expect(musicFS).ToNot(BeNil()) + + _, ok := musicFS.(*localFS) + Expect(ok).To(BeTrue()) + }) + }) + + Context("with non-existent directory", func() { + It("should return an error", func() { + nonExistentPath := filepath.Join(tempDir, "non-existent") + u, err := url.Parse("file://" + nonExistentPath) + Expect(err).ToNot(HaveOccurred()) + + storage := newLocalStorage(*u) + _, err = storage.FS() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(nonExistentPath)) + }) + }) + }) + + Describe("localFS.ReadTags", func() { + var testFile string + + BeforeEach(func() { + // Create a test file + testFile = filepath.Join(tempDir, "test.mp3") + err := os.WriteFile(testFile, []byte("test data"), 0600) + Expect(err).ToNot(HaveOccurred()) + + // Reset extractor state + testExtractor.results = make(map[string]metadata.Info) + testExtractor.err = nil + }) + + Context("when extractor returns complete metadata", func() { + It("should return the metadata as-is", func() { + expectedInfo := metadata.Info{ + Tags: map[string][]string{ + "title": {"Test Song"}, + "artist": {"Test Artist"}, + }, + AudioProperties: metadata.AudioProperties{ + Duration: 180, + BitRate: 320, + }, + FileInfo: &testFileInfo{name: "test.mp3"}, + } + + testExtractor.results["test.mp3"] = expectedInfo + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + + results, err := musicFS.ReadTags("test.mp3") + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveKey("test.mp3")) + Expect(results["test.mp3"]).To(Equal(expectedInfo)) + }) + }) + + Context("when extractor returns metadata without FileInfo", func() { + It("should populate FileInfo from filesystem", func() { + incompleteInfo := metadata.Info{ + Tags: map[string][]string{ + "title": {"Test Song"}, + }, + FileInfo: nil, // Missing FileInfo + } + + testExtractor.results["test.mp3"] = incompleteInfo + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + + results, err := musicFS.ReadTags("test.mp3") + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveKey("test.mp3")) + + result := results["test.mp3"] + Expect(result.FileInfo).ToNot(BeNil()) + Expect(result.FileInfo.Name()).To(Equal("test.mp3")) + + // Should be wrapped in localFileInfo + _, ok := result.FileInfo.(localFileInfo) + Expect(ok).To(BeTrue()) + }) + }) + + Context("when filesystem stat fails", func() { + It("should return an error", func() { + incompleteInfo := metadata.Info{ + Tags: map[string][]string{"title": {"Test Song"}}, + FileInfo: nil, + } + + testExtractor.results["non-existent.mp3"] = incompleteInfo + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + + _, err = musicFS.ReadTags("non-existent.mp3") + Expect(err).To(HaveOccurred()) + }) + }) + + Context("when extractor fails", func() { + It("should return the extractor error", func() { + testExtractor.err = &extractorError{message: "extractor failed"} + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + + _, err = musicFS.ReadTags("test.mp3") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("extractor failed")) + }) + }) + + Context("with multiple files", func() { + It("should process all files correctly", func() { + // Create another test file + testFile2 := filepath.Join(tempDir, "test2.mp3") + err := os.WriteFile(testFile2, []byte("test data 2"), 0600) + Expect(err).ToNot(HaveOccurred()) + + info1 := metadata.Info{ + Tags: map[string][]string{"title": {"Song 1"}}, + FileInfo: &testFileInfo{name: "test.mp3"}, + } + info2 := metadata.Info{ + Tags: map[string][]string{"title": {"Song 2"}}, + FileInfo: nil, // This one needs FileInfo populated + } + + testExtractor.results["test.mp3"] = info1 + testExtractor.results["test2.mp3"] = info2 + + u, err := url.Parse("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + storage := newLocalStorage(*u) + musicFS, err := storage.FS() + Expect(err).ToNot(HaveOccurred()) + + results, err := musicFS.ReadTags("test.mp3", "test2.mp3") + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(2)) + + Expect(results["test.mp3"].FileInfo).To(Equal(&testFileInfo{name: "test.mp3"})) + Expect(results["test2.mp3"].FileInfo).ToNot(BeNil()) + Expect(results["test2.mp3"].FileInfo.Name()).To(Equal("test2.mp3")) + }) + }) + }) + + Describe("localFileInfo", func() { + var testFile string + var fileInfo fs.FileInfo + + BeforeEach(func() { + testFile = filepath.Join(tempDir, "test.mp3") + err := os.WriteFile(testFile, []byte("test data"), 0600) + Expect(err).ToNot(HaveOccurred()) + + fileInfo, err = os.Stat(testFile) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("BirthTime", func() { + It("should return birth time when available", func() { + lfi := localFileInfo{FileInfo: fileInfo} + birthTime := lfi.BirthTime() + + // Birth time should be a valid time (not zero value) + Expect(birthTime).ToNot(BeZero()) + // Should be around the current time (within last few minutes) + Expect(birthTime).To(BeTemporally("~", time.Now(), 5*time.Minute)) + }) + }) + + It("should delegate all other FileInfo methods", func() { + lfi := localFileInfo{FileInfo: fileInfo} + + Expect(lfi.Name()).To(Equal(fileInfo.Name())) + Expect(lfi.Size()).To(Equal(fileInfo.Size())) + Expect(lfi.Mode()).To(Equal(fileInfo.Mode())) + Expect(lfi.ModTime()).To(Equal(fileInfo.ModTime())) + Expect(lfi.IsDir()).To(Equal(fileInfo.IsDir())) + Expect(lfi.Sys()).To(Equal(fileInfo.Sys())) + }) + }) + + Describe("Storage registration", func() { + It("should register localStorage for file scheme", func() { + // This tests the init() function indirectly + storage, err := storage.For("file://" + tempDir) + Expect(err).ToNot(HaveOccurred()) + Expect(storage).To(BeAssignableToTypeOf(&localStorage{})) + }) + }) +}) + +// Test extractor for testing +type mockTestExtractor struct { + results map[string]metadata.Info + err error +} + +func (m *mockTestExtractor) Parse(files ...string) (map[string]metadata.Info, error) { + if m.err != nil { + return nil, m.err + } + + result := make(map[string]metadata.Info) + for _, file := range files { + if info, exists := m.results[file]; exists { + result[file] = info + } + } + return result, nil +} + +func (m *mockTestExtractor) Version() string { + return "test-1.0" +} + +type extractorError struct { + message string +} + +func (e *extractorError) Error() string { + return e.message +} + +// Test FileInfo that implements metadata.FileInfo +type testFileInfo struct { + name string + size int64 + mode fs.FileMode + modTime time.Time + isDir bool + birthTime time.Time +} + +func (t *testFileInfo) Name() string { return t.name } +func (t *testFileInfo) Size() int64 { return t.size } +func (t *testFileInfo) Mode() fs.FileMode { return t.mode } +func (t *testFileInfo) ModTime() time.Time { return t.modTime } +func (t *testFileInfo) IsDir() bool { return t.isDir } +func (t *testFileInfo) Sys() any { return nil } +func (t *testFileInfo) BirthTime() time.Time { + if t.birthTime.IsZero() { + return time.Now() + } + return t.birthTime +} diff --git a/core/storage/storage.go b/core/storage/storage.go index 84bcae0d6..b9fceb1fd 100644 --- a/core/storage/storage.go +++ b/core/storage/storage.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "sync" + + "github.com/navidrome/navidrome/utils/slice" ) const LocalSchemaID = "file" @@ -36,7 +38,14 @@ func For(uri string) (Storage, error) { if len(parts) < 2 { uri, _ = filepath.Abs(uri) uri = filepath.ToSlash(uri) - uri = LocalSchemaID + "://" + uri + + // Properly escape each path component using URL standards + pathParts := strings.Split(uri, "/") + escapedParts := slice.Map(pathParts, func(s string) string { + return url.PathEscape(s) + }) + + uri = LocalSchemaID + "://" + strings.Join(escapedParts, "/") } u, err := url.Parse(uri) diff --git a/core/storage/storage_test.go b/core/storage/storage_test.go index c74c7c6ed..60496e611 100644 --- a/core/storage/storage_test.go +++ b/core/storage/storage_test.go @@ -65,6 +65,21 @@ var _ = Describe("Storage", func() { _, err := For("webdav:///tmp") Expect(err).To(HaveOccurred()) }) + DescribeTable("should handle paths with special characters correctly", + func(inputPath string) { + s, err := For(inputPath) + Expect(err).ToNot(HaveOccurred()) + Expect(s).To(BeAssignableToTypeOf(&fakeLocalStorage{})) + Expect(s.(*fakeLocalStorage).u.Scheme).To(Equal("file")) + // The path should be exactly the same as the input - after URL parsing it gets decoded back + Expect(s.(*fakeLocalStorage).u.Path).To(Equal(inputPath)) + }, + Entry("hash symbols", "/tmp/test#folder/file.mp3"), + Entry("spaces", "/tmp/test folder/file with spaces.mp3"), + Entry("question marks", "/tmp/test?query/file.mp3"), + Entry("ampersands", "/tmp/test&/file.mp3"), + Entry("multiple special chars", "/tmp/Song #1 & More?.mp3"), + ) }) })