From 23548f40a09f5f8bf9cf1e1bc2d4c2b236777560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 28 Jul 2026 16:13:44 -0400 Subject: [PATCH 1/6] fix(share): give visual feedback when downloading from a share (#5865) * fix(share): give visual feedback when downloading a share The share page handed the download URL to navidrome-music-player, which fell through to downloadjs and buffered the whole ZIP into memory via XHR before saving it. Nothing was handed to the browser until the last byte arrived, so a large share produced a long silent window with no player feedback and no browser download UI, inviting repeat clicks that each spawn another server-side zip+transcode. Use the player's customDownloader prop to trigger a synthetic anchor instead, so the browser performs the download and reports its own progress. An anchor rather than assigning window.location.href: the share page's service worker registers a NavigationRoute over all navigations, which intercepts the streamed archive and fails it into the offline fallback (observed as HTTP 503 in Chrome). handleDownloads now loads the share before streaming so it can set Content-Disposition and Content-Type. This also fixes error reporting: ZipShare previously wrote to the ResponseWriter before checkShareError ran, locking the status at 200, so expired, missing and non-downloadable shares all returned 200. They now correctly return 410, 404 and 403. * feat(share): acknowledge the download click in the player The browser's download UI is the real progress indicator, but nothing in the page itself reacted to the click, so the moment before the browser catches up still read as unresponsive. Dim the download button and make it unclickable for two seconds after a download starts, reusing the JSS function-value pattern the existing single-track styling already uses. A repeat download restarts the window instead of extending the original, and the timer is cleared on unmount. This also blunts repeat clicking, where every extra click costs another server-side zip and transcode. Add SharePlayer tests covering the download mechanism and this state machine. The dimming itself is verified in a browser rather than jsdom: JSS function values are not evaluated there, so the rule is never emitted and a CSS assertion would pass or fail for the wrong reason. Signed-off-by: Deluan * test(share): assert render counts in SharePlayer feedback tests The two acknowledgement tests compared the props object across renders, which React may reuse, so they passed without proving anything and then failed once the surrounding assertions changed. Count renders instead, and let the pending timer run out rather than advancing exactly to its deadline, which does not cross it. The repeat-download test now also asserts that no render happens at the original deadline, proving the timer was replaced rather than merely that one eventually fired. * fix(share): count one visit per share download The preflight share load added in this branch made every download record two visits: handleDownloads called Share.Load, and ZipShare then loaded the share again internally. Share.Load increments and persists VisitCount, so the counter advanced twice per download and the repository work was duplicated. Pass the already-loaded share into ZipShare instead of its id. handleDownloads is its only production caller, and it now has the share in hand for the Content-Disposition header anyway. The archiver test asserts Load is not called, so the double-load cannot come back unnoticed. Verified against a running server: the counter now advances by one per download. * test(share): derive feedback-window timings from the constant The acknowledgement tests hardcoded clock advances tuned to a 2000ms window. Raising DOWNLOAD_FEEDBACK_MS to 5000 left them advancing 1500ms and 1001ms, which no longer reach the deadline they are meant to cross, so the repeat- download test passed without proving the timer had been replaced. Export the constant and derive the advances from it, and let the pending timer run out in the unmount test rather than advancing a fixed amount. Changing the duration can no longer silently strand a test short of its deadline. --------- Signed-off-by: Deluan --- core/archiver.go | 12 +-- core/archiver_test.go | 7 +- server/public/handle_downloads.go | 27 ++++- server/public/handle_downloads_test.go | 134 ++++++++++++++++++++++++ server/subsonic/e2e/e2e_suite_test.go | 2 +- ui/src/share/SharePlayer.jsx | 42 ++++++-- ui/src/share/SharePlayer.test.jsx | 139 +++++++++++++++++++++++++ 7 files changed, 344 insertions(+), 19 deletions(-) create mode 100644 server/public/handle_downloads_test.go create mode 100644 ui/src/share/SharePlayer.test.jsx diff --git a/core/archiver.go b/core/archiver.go index 5d1c090cd..8c42f8f49 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -21,7 +21,7 @@ import ( type Archiver interface { ZipAlbum(ctx context.Context, id string, format string, bitrate int, w io.Writer) error ZipArtist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error - ZipShare(ctx context.Context, id string, w io.Writer) error + ZipShare(ctx context.Context, s *model.Share, w io.Writer) error ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error } @@ -100,16 +100,14 @@ func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc return fmt.Sprintf("%s/%s", str.SanitizeFilename(mf.Album), file) } -func (a *archiver) ZipShare(ctx context.Context, id string, out io.Writer) error { - s, err := a.shares.Load(ctx, id) - if err != nil { - return err - } +// ZipShare takes an already-loaded share: Share.Load records a visit, so +// loading it again here would count every download twice. +func (a *archiver) ZipShare(ctx context.Context, s *model.Share, out io.Writer) error { if !s.Downloadable { return model.ErrNotAuthorized } log.Debug(ctx, "Zipping share", "name", s.ID, "format", s.Format, "bitrate", s.MaxBitRate, "numTracks", len(s.Tracks)) - return a.zipMediaFiles(ctx, id, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false) + return a.zipMediaFiles(ctx, s.ID, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false) } func (a *archiver) ZipPlaylist(ctx context.Context, id string, format string, bitrate int, out io.Writer) error { diff --git a/core/archiver_test.go b/core/archiver_test.go index f432139d8..2ba8f1fc0 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -130,13 +130,16 @@ var _ = Describe("Archiver", func() { Tracks: mfs, } - sh.On("Load", mock.Anything, "1").Return(share, nil) ms.On("NewStream", mock.Anything, mock.Anything, stream.Request{Format: "mp3", BitRate: 128}).Return(io.NopCloser(strings.NewReader("test")), nil).Times(2) out := new(bytes.Buffer) - err := arch.ZipShare(context.Background(), "1", out) + err := arch.ZipShare(context.Background(), share, out) Expect(err).To(BeNil()) + // Share.Load records a visit; re-loading here would double-count + // every download. + sh.AssertNotCalled(GinkgoT(), "Load", mock.Anything, mock.Anything) + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) Expect(err).To(BeNil()) diff --git a/server/public/handle_downloads.go b/server/public/handle_downloads.go index 6aa35c341..0012c4b35 100644 --- a/server/public/handle_downloads.go +++ b/server/public/handle_downloads.go @@ -1,18 +1,41 @@ package public import ( + "cmp" + "fmt" "net/http" + "strings" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() id, err := req.Params(r).String(":id") if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - err = pub.archiver.ZipShare(r.Context(), id, w) - checkShareError(r.Context(), w, err, id) + // Load the share before streaming: once ZipShare writes its first byte the + // status is locked at 200, so errors could no longer be reported. + s, err := pub.share.Load(ctx, id) + if err != nil { + checkShareError(ctx, w, err, id) + return + } + if !s.Downloadable { + checkShareError(ctx, w, model.ErrNotAuthorized, id) + return + } + + name := str.SanitizeFilename(cmp.Or(s.Description, s.ID)) + name = strings.ReplaceAll(name, ",", "_") + w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip")) + w.Header().Set("Content-Type", "application/zip") + + err = pub.archiver.ZipShare(ctx, s, w) + checkShareError(ctx, w, err, id) } diff --git a/server/public/handle_downloads_test.go b/server/public/handle_downloads_test.go new file mode 100644 index 000000000..1a97f4379 --- /dev/null +++ b/server/public/handle_downloads_test.go @@ -0,0 +1,134 @@ +package public + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "time" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +type mockArchiver struct { + called bool + err error +} + +func (m *mockArchiver) ZipAlbum(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipArtist(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipPlaylist(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipShare(_ context.Context, _ *model.Share, w io.Writer) error { + m.called = true + if m.err != nil { + return m.err + } + _, _ = w.Write([]byte("zip-contents")) + return nil +} + +var _ = Describe("handleDownloads", func() { + var ds *tests.MockDataStore + var shareRepo *tests.MockShareRepo + var archiver *mockArchiver + var pub *Router + + BeforeEach(func() { + ds = &tests.MockDataStore{} + shareRepo = &tests.MockShareRepo{} + ds.MockedShare = shareRepo + archiver = &mockArchiver{} + pub = &Router{ds: ds, archiver: archiver, share: core.NewShare(ds)} + }) + + shareIs := func(s *model.Share) { + shareRepo.ID = s.ID + shareRepo.Entity = s + } + + makeRequest := func(id string) *httptest.ResponseRecorder { + r := httptest.NewRequest("GET", "/public/d/"+id+"?%3Aid="+id, nil) + w := httptest.NewRecorder() + pub.handleDownloads(w, r) + return w + } + + It("sets a Content-Disposition filename from the share description", func() { + shareIs(&model.Share{ID: "abc123", Description: "My Mixtape", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"`)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/zip")) + Expect(archiver.called).To(BeTrue()) + Expect(w.Body.String()).To(Equal("zip-contents")) + }) + + It("falls back to the share ID when there is no description", func() { + shareIs(&model.Share{ID: "abc123", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`)) + }) + + It("sanitizes characters that are unsafe in a filename", func() { + shareIs(&model.Share{ID: "abc123", Description: `AC/DC: Live, 1979`, Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`)) + }) + + It("returns 403 without invoking the archiver when the share is not downloadable", func() { + shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusForbidden)) + Expect(archiver.called).To(BeFalse()) + Expect(w.Header().Get("Content-Disposition")).To(BeEmpty()) + }) + + It("returns 404 when the share does not exist", func() { + shareIs(&model.Share{ID: "other", Downloadable: true}) + + w := makeRequest("missing") + + Expect(w.Code).To(Equal(http.StatusNotFound)) + Expect(archiver.called).To(BeFalse()) + }) + + It("returns 410 when the share has expired", func() { + shareIs(&model.Share{ID: "abc123", Downloadable: true, ExpiresAt: new(time.Now().Add(-time.Hour))}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusGone)) + Expect(archiver.called).To(BeFalse()) + }) + + It("returns 500 when the share lookup fails", func() { + shareRepo.Error = errors.New("db error") + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + Expect(archiver.called).To(BeFalse()) + }) +}) diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 6875b6370..4e9039d68 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -330,7 +330,7 @@ func (n noopArchiver) ZipArtist(context.Context, string, string, int, io.Writer) return model.ErrNotFound } -func (n noopArchiver) ZipShare(context.Context, string, io.Writer) error { +func (n noopArchiver) ZipShare(context.Context, *model.Share, io.Writer) error { return model.ErrNotFound } diff --git a/ui/src/share/SharePlayer.jsx b/ui/src/share/SharePlayer.jsx index a3a15e50a..2384866ea 100644 --- a/ui/src/share/SharePlayer.jsx +++ b/ui/src/share/SharePlayer.jsx @@ -1,15 +1,24 @@ import ReactJkMusicPlayer from 'navidrome-music-player' +import { useCallback, useEffect, useRef, useState } from 'react' import config, { shareInfo } from '../config' import { shareCoverUrl, shareDownloadUrl, shareStreamUrl } from '../utils' import { makeStyles } from '@material-ui/core/styles' +// How long the download button stays inert after a click. The browser needs a +// moment to show its own download UI; until then the page looks unresponsive. +export const DOWNLOAD_FEEDBACK_MS = 5000 + const useStyle = makeStyles({ player: { '& .group .next-audio': { pointerEvents: (props) => props.single && 'none', opacity: (props) => props.single && 0.65, }, + '& .group.audio-download': { + pointerEvents: (props) => props.downloading && 'none', + opacity: (props) => props.downloading && 0.65, + }, '@media (min-width: 768px)': { '& .react-jinke-music-player-mobile > div': { width: 768, @@ -23,7 +32,14 @@ const useStyle = makeStyles({ }) const SharePlayer = () => { - const classes = useStyle({ single: shareInfo?.tracks.length === 1 }) + const [downloading, setDownloading] = useState(false) + const timer = useRef(null) + const classes = useStyle({ + single: shareInfo?.tracks.length === 1, + downloading, + }) + + useEffect(() => () => clearTimeout(timer.current), []) const list = shareInfo?.tracks.map((s) => { return { @@ -34,11 +50,23 @@ const SharePlayer = () => { duration: s.duration, } }) - const onBeforeAudioDownload = () => { - return Promise.resolve({ - src: shareDownloadUrl(shareInfo?.id), - }) - } + // An anchor, not a navigation: the service worker's NavigationRoute would + // intercept the streamed archive and fail it. + const customDownloader = useCallback(() => { + const link = document.createElement('a') + link.href = shareDownloadUrl(shareInfo?.id) + link.download = '' + document.body.appendChild(link) + link.click() + document.body.removeChild(link) + + setDownloading(true) + clearTimeout(timer.current) + timer.current = setTimeout( + () => setDownloading(false), + DOWNLOAD_FEEDBACK_MS, + ) + }, []) const options = { audioLists: list, mode: 'full', @@ -59,7 +87,7 @@ const SharePlayer = () => { ) } diff --git a/ui/src/share/SharePlayer.test.jsx b/ui/src/share/SharePlayer.test.jsx new file mode 100644 index 000000000..68d1eff59 --- /dev/null +++ b/ui/src/share/SharePlayer.test.jsx @@ -0,0 +1,139 @@ +import { render, act } from '@testing-library/react' +import SharePlayer, { DOWNLOAD_FEEDBACK_MS } from './SharePlayer' + +let playerProps +let renderCount + +vi.mock('navidrome-music-player', () => ({ + default: (props) => { + playerProps = props + renderCount++ + return
+ }, +})) + +vi.mock('../config', () => ({ + default: { enableDownloads: true }, + shareInfo: { + id: 'share-1', + downloadable: true, + tracks: [{ id: 't1', title: 'One', artist: 'A', duration: 100 }], + }, +})) + +vi.mock('../utils', () => ({ + shareDownloadUrl: (id) => `/share/d/${id}`, + shareStreamUrl: (id) => `/share/s/${id}`, + shareCoverUrl: (id) => `/share/img/${id}`, +})) + +describe('SharePlayer', () => { + let clickSpy + + beforeEach(() => { + vi.useFakeTimers() + playerProps = null + renderCount = 0 + // Downloading for real would navigate the jsdom window. + clickSpy = vi + .spyOn(HTMLAnchorElement.prototype, 'click') + .mockImplementation(() => {}) + }) + + afterEach(() => { + vi.runOnlyPendingTimers() + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('downloads via an anchor so the service worker does not intercept it', () => { + render() + + let anchor + clickSpy.mockImplementation(function () { + anchor = { href: this.href, download: this.download } + }) + + act(() => { + playerProps.customDownloader() + }) + + expect(clickSpy).toHaveBeenCalledTimes(1) + expect(anchor.href).toContain('/share/d/share-1') + // Empty, so the server's Content-Disposition filename wins. + expect(anchor.download).toBe('') + }) + + it('removes the anchor from the document after clicking', () => { + render() + + act(() => { + playerProps.customDownloader() + }) + + expect(document.querySelectorAll('a[download]')).toHaveLength(0) + }) + + // The inert styling itself is driven by JSS function values, which jsdom does + // not evaluate; it is verified in a browser. What is checked here is the + // state machine feeding it -- that the component re-renders on download and + // again when the window closes. + it('re-renders when the feedback window opens and closes', () => { + render() + + const beforeDownload = renderCount + act(() => { + playerProps.customDownloader() + }) + expect(renderCount).toBeGreaterThan(beforeDownload) + + const beforeExpiry = renderCount + act(() => { + vi.runAllTimers() + }) + expect(renderCount).toBeGreaterThan(beforeExpiry) + }) + + it('restarts the feedback window on a repeat download', () => { + render() + + act(() => { + playerProps.customDownloader() + }) + const elapsedBeforeRepeat = Math.floor(DOWNLOAD_FEEDBACK_MS / 2) + act(() => { + vi.advanceTimersByTime(elapsedBeforeRepeat) + }) + act(() => { + playerProps.customDownloader() + }) + + // Past the first timer's deadline, which it would have fired at had the + // repeat download not replaced it. + const beforeOriginalDeadline = renderCount + act(() => { + vi.advanceTimersByTime(DOWNLOAD_FEEDBACK_MS - elapsedBeforeRepeat + 1) + }) + expect(renderCount).toBe(beforeOriginalDeadline) + + act(() => { + vi.runAllTimers() + }) + expect(renderCount).toBeGreaterThan(beforeOriginalDeadline) + }) + + it('does not update state after unmount', () => { + const { unmount } = render() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + act(() => { + playerProps.customDownloader() + }) + unmount() + act(() => { + vi.runAllTimers() + }) + + expect(errorSpy).not.toHaveBeenCalled() + }) +}) From add0a6dc9b8360db480f76793fa928499bf51741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 29 Jul 2026 13:44:42 -0400 Subject: [PATCH 2/6] feat(config): warn about unrecognized options in the config file (#5870) * feat(config): warn about unrecognized options in the config file Options that don't match any known name were silently discarded, so a typo or an option written outside its section looked like it was applied. In #5869 the user set `ArtistSplitExceptions` at the root level instead of under `Scanner`, and got no feedback that the option was being ignored. The known names are derived by reflection over configOptions, so the check stays in sync with the struct. Free-form maps (Tags, DevLogLevels) accept any subkey. When an unknown key matches the last segment of a known one, the warning suggests it. Keys are reported as spelled in the config file, recovered by scanning it, since viper lowercases every key it loads. Also fixes two gaps this surfaced: - remapEnvVarKeysFromConfig accepted any ND_-prefixed key and advised a canonical name built by string substitution, so `ND_SCANNER_WATCHERENABLED` (not an option) suggested `Scanner.Watcherenabled` (not an option either). It now only advises names that exist, with their documented spelling, and leaves the rest to the unrecognized-option warning. - The deprecated option list drove only the warnings, while the value migration kept a second hardcoded list. They had drifted: SearchFullString warned about `Search.FullString` but never migrated to it. Both now come from deprecatedOptions. * fix(config): address Codex review on the unrecognized-option warning - Values computed during Load (ConfigFile, LastFM.Languages, Deezer.Languages) were accepted as valid keys, so setting them in the config file stayed silent even though Load overwrites them. They are now marked `conf:"-"` at the declaration, so the exclusion can't drift from the struct. - Removed options are in the known-key set only so they get their own warning, but suggestOptions drew from the same set, so an unknown `ID` advised `Spotify.ID`, a key Navidrome explicitly ignores. They are now filtered out of suggestions. - mapDeprecatedOption uses viper.Set, which outranks the config file, so a deprecated value overrode an explicitly configured replacement. It now skips the migration when the replacement was provided. viper.IsSet counts defaults as set, so the check is InConfig plus the env var. envVarName also returns "" for an empty option, so a deprecated option with no replacement no longer advises "Please use the new 'ND_'". * fix(config): cover the ND_ spelling of a replacement, and the warning output - explicitlySet missed the case where the replacement is given in the config file under its ND_ spelling: remapEnvVarKeysFromConfig moves it to the override layer, out of InConfig's reach, so the deprecated value still won. It now also checks the ND_-prefixed config key. - The tests asserted only the helpers' return values, so removing the logUnknownOptions call from Load left them green. Added a spec that captures the logger and checks the emitted warning and suggestion text; verified it fails when the call is removed. --- conf/configuration.go | 241 +++++++++++++++++++---- conf/configuration_test.go | 119 +++++++++++ conf/export_test.go | 4 + conf/testdata/cfg_deprecated_search.toml | 2 + conf/testdata/cfg_nd_bogus.toml | 3 + conf/testdata/cfg_runtime_fields.toml | 10 + conf/testdata/cfg_unknown_casing.ini | 3 + conf/testdata/cfg_unknown_casing.json | 4 + conf/testdata/cfg_unknown_casing.toml | 2 + conf/testdata/cfg_unknown_casing.yaml | 2 + conf/testdata/cfg_unknown_keys.toml | 18 ++ conf/testdata/cfg_warning_output.toml | 7 + 12 files changed, 380 insertions(+), 35 deletions(-) create mode 100644 conf/testdata/cfg_deprecated_search.toml create mode 100644 conf/testdata/cfg_nd_bogus.toml create mode 100644 conf/testdata/cfg_runtime_fields.toml create mode 100644 conf/testdata/cfg_unknown_casing.ini create mode 100644 conf/testdata/cfg_unknown_casing.json create mode 100644 conf/testdata/cfg_unknown_casing.toml create mode 100644 conf/testdata/cfg_unknown_casing.yaml create mode 100644 conf/testdata/cfg_unknown_keys.toml create mode 100644 conf/testdata/cfg_warning_output.toml diff --git a/conf/configuration.go b/conf/configuration.go index 83793bd43..9b49fb264 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -2,14 +2,18 @@ package conf import ( "cmp" + "encoding" "encoding/json" "fmt" "net/url" "os" "path/filepath" + "reflect" + "regexp" "runtime" "slices" "strings" + "sync" "time" "github.com/bmatcuk/doublestar/v4" @@ -21,11 +25,12 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/scheduler" "github.com/navidrome/navidrome/utils/run" + "github.com/navidrome/navidrome/utils/slice" "github.com/spf13/viper" ) type configOptions struct { - ConfigFile string + ConfigFile string `conf:"-"` Address string Port int UnixSocketPerm string @@ -201,7 +206,7 @@ type lastfmOptions struct { ScrobbleFirstArtistOnly bool // Computed values - Languages []string // Computed from Language, split by comma + Languages []string `conf:"-"` // Computed from Language, split by comma } type deezerOptions struct { @@ -209,7 +214,7 @@ type deezerOptions struct { Language string // Computed values - Languages []string // Computed from Language, split by comma + Languages []string `conf:"-"` // Computed from Language, split by comma } type listenBrainzOptions struct { @@ -340,12 +345,11 @@ func Load(noConfigDump bool) { remapEnvVarKeysFromConfig() // Map deprecated options to their new names for backwards compatibility - mapDeprecatedOption("ReverseProxyWhitelist", "ExtAuth.TrustedSources") - mapDeprecatedOption("ReverseProxyUserHeader", "ExtAuth.UserHeader") - mapDeprecatedOption("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions") - mapDeprecatedOption("CoverJpegQuality", "CoverArtQuality") - mapDeprecatedOption("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold") - mapDeprecatedOption("EnableTranscodingCancellation", "Transcoding.EnableCancellation") + for _, o := range deprecatedOptions { + if o.replacement != "" { + mapDeprecatedOption(o.name, o.replacement) + } + } err := viper.Unmarshal(&Server, viper.DecodeHook( mapstructure.ComposeDecodeHookFunc( @@ -403,6 +407,13 @@ func Load(noConfigDump bool) { log.SetLogSourceLine(Server.DevLogSourceLine) log.SetRedacting(Server.EnableLogRedacting) + // Log deprecated, removed and unknown options + for _, o := range deprecatedOptions { + logDeprecatedOptions(o.name, o.replacement) + } + logRemovedOptions(removedOptions...) + logUnknownOptions() + err = run.Sequentially( validateScanSchedule, validateBackupSchedule, @@ -461,21 +472,6 @@ func Load(noConfigDump bool) { // Parse Deezer.Language into Languages slice (comma-separated, with fallback to DefaultInfoLanguage) Server.Deezer.Languages = parseLanguages(Server.Deezer.Language) - // Deprecated options - logDeprecatedOptions("Scanner.GenreSeparators", "") - logDeprecatedOptions("Scanner.GroupAlbumReleases", "") - logDeprecatedOptions("DevEnableBufferedScrobble", "") // Deprecated: Buffered scrobbling is now always enabled and this option is ignored - logDeprecatedOptions("SearchFullString", "Search.FullString") - logDeprecatedOptions("ReverseProxyWhitelist", "ExtAuth.TrustedSources") - logDeprecatedOptions("ReverseProxyUserHeader", "ExtAuth.UserHeader") - logDeprecatedOptions("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions") - logDeprecatedOptions("CoverJpegQuality", "CoverArtQuality") - logDeprecatedOptions("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold") - logDeprecatedOptions("EnableTranscodingCancellation", "Transcoding.EnableCancellation") - - // Removed options - logRemovedOptions("Spotify.ID", "Spotify.Secret") - // Validate other options if Server.UICoverArtSize < 200 || Server.UICoverArtSize > 1200 { newValue := max(200, min(1200, Server.UICoverArtSize)) @@ -489,9 +485,26 @@ func Load(noConfigDump bool) { } } +// deprecatedOptions still work, but will be removed in a future release. An empty +// replacement means the option is now ignored. +var deprecatedOptions = []struct{ name, replacement string }{ + {"Scanner.GenreSeparators", ""}, + {"Scanner.GroupAlbumReleases", ""}, + {"DevEnableBufferedScrobble", ""}, + {"SearchFullString", "Search.FullString"}, + {"ReverseProxyWhitelist", "ExtAuth.TrustedSources"}, + {"ReverseProxyUserHeader", "ExtAuth.UserHeader"}, + {"HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions"}, + {"CoverJpegQuality", "CoverArtQuality"}, + {"SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold"}, + {"EnableTranscodingCancellation", "Transcoding.EnableCancellation"}, +} + +var removedOptions = []string{"Spotify.ID", "Spotify.Secret"} + func logDeprecatedOptions(oldName, newName string) { - envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(oldName, ".", "_")) - newEnvVar := "ND_" + strings.ToUpper(strings.ReplaceAll(newName, ".", "_")) + envVar := envVarName(oldName) + newEnvVar := envVarName(newName) logWarning := func(oldName, newName string) { if newName != "" { log.Warn(fmt.Sprintf("Option '%s' is deprecated and will be ignored in a future release. Please use the new '%s'", oldName, newName)) @@ -511,7 +524,7 @@ func logDeprecatedOptions(oldName, newName string) { // not available anymore func logRemovedOptions(options ...string) { for _, option := range options { - envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_")) + envVar := envVarName(option) logWarning := func(option string) { log.Warn(fmt.Sprintf("Option '%s' is not available anymore and will be ignored. Please remove it from your config", option)) } @@ -532,35 +545,193 @@ func remapEnvVarKeysFromConfig() { continue } stripped := strings.TrimPrefix(key, "nd_") - canonicalKey := strings.ReplaceAll(stripped, "_", ".") + canonicalKey := ndKeyToCanonical(key) displayNDKey := "ND_" + strings.ToUpper(stripped) - displayCanonical := toPascalCase(canonicalKey) + canonicalName := canonicalOptionName(canonicalKey) if viper.InConfig(canonicalKey) { logFatal(fmt.Sprintf( "Config file contains both '%s' and '%s'. Remove the ND_-prefixed version. "+ "The 'ND_' prefix is only needed for environment variables, not config file keys.", - displayNDKey, displayCanonical, + displayNDKey, cmp.Or(canonicalName, toPascalCase(canonicalKey)), )) return } viper.Set(canonicalKey, viper.Get(key)) - _, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+ - "The 'ND_' prefix is only needed for environment variables.\n", - displayNDKey, displayCanonical, - ) + // Unknown keys get no advice here, logUnknownOptions reports them instead + if canonicalName != "" { + _, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+ + "The 'ND_' prefix is only needed for environment variables.\n", + displayNDKey, canonicalName, + ) + } } } // mapDeprecatedOption is used to provide backwards compatibility for deprecated options. It should be called after // the config has been read by viper, but before unmarshalling it into the Config struct. func mapDeprecatedOption(legacyName, newName string) { - if viper.IsSet(legacyName) { + // viper.Set outranks the config file, so an explicit replacement must win over the legacy value + if viper.IsSet(legacyName) && !explicitlySet(newName) { viper.Set(newName, viper.Get(legacyName)) } } +// explicitlySet reports whether the user provided the option, ignoring defaults, +// which viper.IsSet counts as set. The ND_ spelling is also accepted in the config +// file, and remapEnvVarKeysFromConfig has already moved it out of InConfig's reach. +func explicitlySet(name string) bool { + envVar := envVarName(name) + return viper.InConfig(name) || os.Getenv(envVar) != "" || viper.InConfig(strings.ToLower(envVar)) +} + +func envVarName(option string) string { + if option == "" { + return "" + } + return "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_")) +} + +func logUnknownOptions() { + for _, key := range unknownConfigKeys() { + msg := fmt.Sprintf("Option '%s' is not recognized and will be ignored", key) + if matches := suggestOptions(key); len(matches) > 0 { + msg += fmt.Sprintf(". Did you mean '%s'?", strings.Join(matches, "' or '")) + } + log.Warn(msg) + } +} + +// suggestOptions returns the known options sharing the last segment with key, +// catching options written outside their section. +func suggestOptions(key string) []string { + key = strings.ToLower(key) + leaf := leafKey(key) + canonical, _ := configKeys() + var matches []string + for known, name := range canonical { + // Removed options are known only so they get their own warning, never suggest them + if known != key && leafKey(known) == leaf && !slices.Contains(removedOptions, name) { + matches = append(matches, name) + } + } + slices.Sort(matches) + return matches +} + +func leafKey(key string) string { + return key[strings.LastIndex(key, ".")+1:] +} + +// unknownConfigKeys returns config file keys that don't match any known option, so +// typos and options written outside their section don't fail silently. +func unknownConfigKeys() []string { + // INI files keep the original [default] section alongside the merged one + skipDefault := strings.EqualFold(filepath.Ext(viper.ConfigFileUsed()), ".ini") + + var unknown []string + for _, key := range viper.AllKeys() { + if !viper.InConfig(key) || canonicalOptionName(key) != "" { + continue + } + if skipDefault && strings.HasPrefix(key, "default.") { + continue + } + // Only ND_-prefixed keys that remapEnvVarKeysFromConfig could resolve are valid + if strings.HasPrefix(key, "nd_") && canonicalOptionName(ndKeyToCanonical(key)) != "" { + continue + } + unknown = append(unknown, key) + } + slices.Sort(unknown) + return asWrittenInConfigFile(unknown) +} + +func ndKeyToCanonical(key string) string { + return strings.ReplaceAll(strings.TrimPrefix(key, "nd_"), "_", ".") +} + +// canonicalOptionName returns the documented spelling of a known option key, or "" +// if it matches no option. Subkeys of free-form maps have no fixed spelling. +func canonicalOptionName(key string) string { + keys, prefixes := configKeys() + if name, ok := keys[key]; ok { + return name + } + if slices.ContainsFunc(prefixes, func(p string) bool { return strings.HasPrefix(key, p) }) { + return toPascalCase(key) + } + return "" +} + +// asWrittenInConfigFile restores the casing the keys have in the config file, as +// viper lowercases every key it loads. +func asWrittenInConfigFile(keys []string) []string { + if len(keys) == 0 { + return nil + } + data, err := os.ReadFile(viper.ConfigFileUsed()) + if err != nil { + return keys + } + casing := map[string]string{} + for _, match := range configFileKeyRx.FindAllStringSubmatch(string(data), -1) { + for segment := range strings.SplitSeq(match[1], ".") { + lower := strings.ToLower(segment) + casing[lower] = cmp.Or(casing[lower], segment) + } + } + return slice.Map(keys, func(key string) string { + segments := strings.Split(key, ".") + for i, s := range segments { + segments[i] = cmp.Or(casing[s], s) + } + return strings.Join(segments, ".") + }) +} + +// Matches keys and section headers in all supported config formats. +var configFileKeyRx = regexp.MustCompile(`(?m)^\s*\[?\s*"?([\w.]+)"?\s*[]=:]`) + +// configKeys maps every accepted option name, lowercased, to its canonical spelling, +// plus the prefixes of free-form map options (Tags, DevLogLevels). +var configKeys = sync.OnceValues(func() (map[string]string, []string) { + keys := map[string]string{} + var prefixes []string + + var collect func(t reflect.Type, prefix string) + collect = func(t reflect.Type, prefix string) { + for field := range t.Fields() { + // `conf:"-"` marks values computed during Load, not settable in the config + if !field.IsExported() || field.Tag.Get("conf") == "-" { + continue + } + name := prefix + field.Name + if field.Type.Kind() == reflect.Struct && !reflect.PointerTo(field.Type).Implements(textUnmarshalerType) { + collect(field.Type, name+".") + continue + } + lower := strings.ToLower(name) + keys[lower] = name + if field.Type.Kind() == reflect.Map { + prefixes = append(prefixes, lower+".") + } + } + } + collect(reflect.TypeFor[configOptions](), "") + + for _, o := range deprecatedOptions { + keys[strings.ToLower(o.name)] = o.name + } + for _, o := range removedOptions { + keys[strings.ToLower(o)] = o + } + return keys, prefixes +}) + +var textUnmarshalerType = reflect.TypeFor[encoding.TextUnmarshaler]() + // parseIniFileConfiguration is used to parse the config file when it is in INI format. For INI files, it // would require a nested structure, so instead we unmarshal it to a map and then merge the nested [default] // section into the root level. diff --git a/conf/configuration_test.go b/conf/configuration_test.go index e43c91a4b..4eaa8e3d8 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -1,12 +1,14 @@ package conf_test import ( + "bytes" "fmt" "os" "path/filepath" "testing" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/log" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/spf13/viper" @@ -178,6 +180,123 @@ var _ = Describe("Configuration", func() { }) }) + Describe("unknownConfigKeys", func() { + BeforeEach(func() { + viper.Reset() + conf.SetViperDefaults() + viper.SetDefault("datafolder", GinkgoT().TempDir()) + viper.SetDefault("loglevel", "error") + conf.ResetConf() + }) + + It("reports misplaced and misspelled options, as spelled in the config file", func() { + conf.InitConfig(filepath.Join("testdata", "cfg_unknown_keys.toml"), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(ConsistOf( + "ArtistSplitExceptions", "EnableDownlods", "Whatever.Foo", + )) + }) + + DescribeTable("recovers the original casing in all supported formats", + func(file string) { + conf.InitConfig(filepath.Join("testdata", file), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(ConsistOf("NotAnOption")) + }, + Entry("TOML", "cfg_unknown_casing.toml"), + Entry("YAML", "cfg_unknown_casing.yaml"), + Entry("JSON", "cfg_unknown_casing.json"), + Entry("INI", "cfg_unknown_casing.ini"), + ) + + It("does not report valid, deprecated or free-form keys", func() { + conf.InitConfig(filepath.Join("testdata", "cfg.toml"), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(BeEmpty()) + }) + + It("does not report the [default] section of INI files", func() { + conf.InitConfig(filepath.Join("testdata", "cfg.ini"), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(BeEmpty()) + }) + + DescribeTable("SuggestOptions", + func(key string, expected []string) { + Expect(conf.SuggestOptions(key)).To(Equal(expected)) + }, + Entry("suggests the section of a misplaced option", "artistsplitexceptions", + []string{"Scanner.ArtistSplitExceptions"}), + Entry("suggests the section of a misplaced nested option", "backup.fuzzythreshold", + []string{"Matcher.FuzzyThreshold"}), + Entry("suggests every section defining the option", "schedule", + []string{"Backup.Schedule", "Scanner.Schedule"}), + Entry("suggests nothing for a typo", "enabledownlods", nil), + ) + + It("does not report ND_-prefixed keys, as they are remapped", func() { + conf.InitConfig(filepath.Join("testdata", "cfg_nd_keys.toml"), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(BeEmpty()) + }) + + It("reports ND_-prefixed keys that remap to no known option", func() { + conf.InitConfig(filepath.Join("testdata", "cfg_nd_bogus.toml"), false) + conf.Load(true) + + Expect(conf.UnknownConfigKeys()).To(ConsistOf("ND_TOTALLY_BOGUS_OPTION")) + Expect(conf.Server.Scanner.Schedule).To(Equal("@every 1h")) + }) + + It("migrates every deprecated option that has a replacement", func() { + conf.InitConfig(filepath.Join("testdata", "cfg_deprecated_search.toml"), false) + conf.Load(true) + + Expect(conf.Server.Search.FullString).To(BeTrue()) + Expect(conf.UnknownConfigKeys()).To(BeEmpty()) + }) + + It("warns about each unrecognized option at startup", func() { + var logBuf bytes.Buffer + log.SetOutput(&logBuf) + DeferCleanup(func() { log.SetOutput(GinkgoWriter) }) + + conf.InitConfig(filepath.Join("testdata", "cfg_warning_output.toml"), false) + conf.Load(true) + + Expect(logBuf.String()).To(ContainSubstring( + "Option 'ArtistSplitExceptions' is not recognized and will be ignored. " + + "Did you mean 'Scanner.ArtistSplitExceptions'?")) + Expect(logBuf.String()).To(ContainSubstring( + "Option 'EnableDownlods' is not recognized and will be ignored")) + Expect(logBuf.String()).ToNot(ContainSubstring("ArtistJoiner")) + }) + + Context("with runtime-computed and removed options in the config", func() { + BeforeEach(func() { + conf.InitConfig(filepath.Join("testdata", "cfg_runtime_fields.toml"), false) + conf.Load(true) + }) + + It("reports values computed during Load, which the config cannot set", func() { + Expect(conf.UnknownConfigKeys()).To(ContainElements("ConfigFile", "LastFM.Languages")) + }) + + It("never suggests a removed option", func() { + Expect(conf.SuggestOptions("id")).To(BeEmpty()) + }) + + It("keeps an explicit replacement over the deprecated value", func() { + Expect(conf.Server.Search.FullString).To(BeFalse()) + }) + }) + }) + Describe("logFatal", func() { var invalidPath string BeforeEach(func() { diff --git a/conf/export_test.go b/conf/export_test.go index acebca551..cbb64b3d0 100644 --- a/conf/export_test.go +++ b/conf/export_test.go @@ -32,3 +32,7 @@ func SetLogFatal(f func(...any)) func() { logFatal = f return func() { logFatal = old } } + +var UnknownConfigKeys = unknownConfigKeys + +var SuggestOptions = suggestOptions diff --git a/conf/testdata/cfg_deprecated_search.toml b/conf/testdata/cfg_deprecated_search.toml new file mode 100644 index 000000000..cc6541e09 --- /dev/null +++ b/conf/testdata/cfg_deprecated_search.toml @@ -0,0 +1,2 @@ +MusicFolder = "/toml/music" +SearchFullString = true diff --git a/conf/testdata/cfg_nd_bogus.toml b/conf/testdata/cfg_nd_bogus.toml new file mode 100644 index 000000000..841b40998 --- /dev/null +++ b/conf/testdata/cfg_nd_bogus.toml @@ -0,0 +1,3 @@ +MusicFolder = "/toml/music" +ND_TOTALLY_BOGUS_OPTION = true +ND_SCANNER_SCHEDULE = "@every 1h" diff --git a/conf/testdata/cfg_runtime_fields.toml b/conf/testdata/cfg_runtime_fields.toml new file mode 100644 index 000000000..a9c09f619 --- /dev/null +++ b/conf/testdata/cfg_runtime_fields.toml @@ -0,0 +1,10 @@ +MusicFolder = "/toml/music" +SearchFullString = true +ConfigFile = "/somewhere/else" +ID = "oops" + +[Search] +FullString = false + +[LastFM] +Languages = ["pt"] diff --git a/conf/testdata/cfg_unknown_casing.ini b/conf/testdata/cfg_unknown_casing.ini new file mode 100644 index 000000000..c88db9b1f --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.ini @@ -0,0 +1,3 @@ +[default] +MusicFolder = /ini/music +NotAnOption = true diff --git a/conf/testdata/cfg_unknown_casing.json b/conf/testdata/cfg_unknown_casing.json new file mode 100644 index 000000000..cd3a226cd --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.json @@ -0,0 +1,4 @@ +{ + "MusicFolder": "/json/music", + "NotAnOption": true +} diff --git a/conf/testdata/cfg_unknown_casing.toml b/conf/testdata/cfg_unknown_casing.toml new file mode 100644 index 000000000..f015c9cb2 --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.toml @@ -0,0 +1,2 @@ +MusicFolder = "/toml/music" +NotAnOption = true diff --git a/conf/testdata/cfg_unknown_casing.yaml b/conf/testdata/cfg_unknown_casing.yaml new file mode 100644 index 000000000..8987f6351 --- /dev/null +++ b/conf/testdata/cfg_unknown_casing.yaml @@ -0,0 +1,2 @@ +MusicFolder: /yaml/music +NotAnOption: true diff --git a/conf/testdata/cfg_unknown_keys.toml b/conf/testdata/cfg_unknown_keys.toml new file mode 100644 index 000000000..a7ed55cb0 --- /dev/null +++ b/conf/testdata/cfg_unknown_keys.toml @@ -0,0 +1,18 @@ +MusicFolder = "/toml/music" + +# Valid option, but written at the root level instead of under Scanner +ArtistSplitExceptions = ["AC/DC", "Tyler, the creator"] + +# Misspelled option +EnableDownlods = true + +# Unknown section +[Whatever] +Foo = "bar" + +# Valid options, must not be reported +[Scanner] +ArtistJoiner = " • " + +[Tags.custom] +aliases = ["toml", "test"] diff --git a/conf/testdata/cfg_warning_output.toml b/conf/testdata/cfg_warning_output.toml new file mode 100644 index 000000000..a52c5ea54 --- /dev/null +++ b/conf/testdata/cfg_warning_output.toml @@ -0,0 +1,7 @@ +MusicFolder = "/toml/music" +LogLevel = "warn" +ArtistSplitExceptions = ["AC/DC"] +EnableDownlods = true + +[Scanner] +ArtistJoiner = " • " From 4f29465d27d35444683bd7fae70097583ab4b01f Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Thu, 30 Jul 2026 01:48:42 +0000 Subject: [PATCH 3/6] fix(artwork): do not promote artist folder to album folder (#5856) * fix(artwork): do not promote artist folder to album folder * use filesystem join for path instead * fix(artwork): resolve artist folder for albums with disc subfolders Dropping the promoted album root from the artist reader's paths fixed the flat-album case but broke albums whose tracks live in disc subfolders: the promoted root was what kept the byte-wise longest common prefix on a directory boundary. Without it, CD1/CD2 siblings share the fragment "Album/CD", so the artist folder resolved one level too deep and album/artist.* won over artist.*. loadAlbumFoldersPaths now collapses each album to its own root before the paths are compared across albums, so a disc-split album contributes its album folder rather than each disc folder. Folders no album claims (the promoted parent) are still returned unchanged, keeping the album, disc and mediafile readers unaffected. The prefix math moves into commonDir, which appends a trailing separator so the comparison lands on segment boundaries - this also fixes sibling folders sharing a name prefix (Album/Album2). Adds e2e coverage for the case the branch fixes (artist.* with no album/ fallback, which failed before this branch), the disc-subfolder regression, and a multi-album guard that pins the scope boundary. * test(artwork): cover artist whose albums are all disc-split The existing specs cover a single disc-split album and a disc-split album alongside a flat one, but not an artist where every album is split into disc subfolders. That layout resolves correctly because the albums diverge one level above the disc folders, which re-anchors the common prefix - a property worth pinning so a future change to the path math can't silently break it. * fix(artwork): resolve artist folder when albums share a folder loadArtistFolder decided whether to climb above the common directory by comparing len(paths) against 2. That count stands in for "how many distinct album roots are there", and the two diverge when an artist has two albums in the same folder: the slice holds two identical entries, the climb is skipped, and the artist folder resolves to the album folder. Deduplicate the roots so the branch tests the property it means to test. Replace the includeParent flag with two functions split by audience. The flag was a caller-identity switch - constant true at the album, disc and mediafile readers, all of which discard the returned paths, and constant false at the only caller that reads them. The artist path now has loadArtistAlbumRoots, which collapses each album to its own root and never consults albumRootParent; loadAlbumFoldersPaths goes back to taking a single album and always promoting the parent. Both share the folder load and image aggregation. This also makes albumRootParent's contract structural. Its guard only rejects an artist folder when it can find audio outside the album being resolved, so passing every album of an artist at once made the check meaningless; taking a single album means it can no longer be called that way. Drops the per-album grouping and unclaimed-folder reconciliation from the album path, where the result was discarded, and removes 15 mechanical true arguments from the tests. --------- Co-authored-by: Deluan --- core/artwork/artwork_internal_test.go | 1 + core/artwork/e2e/artist_test.go | 95 +++++++++++++++++++++++++++ core/artwork/reader_album.go | 32 ++++----- core/artwork/reader_album_test.go | 27 ++++++++ core/artwork/reader_artist.go | 58 ++++++++++++++-- core/artwork/reader_artist_test.go | 13 ++++ 6 files changed, 206 insertions(+), 20 deletions(-) diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index c95371959..5bcd82f23 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -202,6 +202,7 @@ var _ = Describe("Artwork", func() { repoRoot, err := os.Getwd() Expect(err).ToNot(HaveOccurred()) folderRepo.result = []model.Folder{{ + ID: "f1", LibraryPath: testFileLibPath(repoRoot), Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"artist.png"}, diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go index d959b1d60..2079d37ce 100644 --- a/core/artwork/e2e/artist_test.go +++ b/core/artwork/e2e/artist_test.go @@ -80,6 +80,101 @@ var _ = Describe("Artist artwork resolution", func() { }) }) + When("ArtistArtPriority has no album/ fallback", func() { + // Artist/ + // ├── artist.jpg ← must resolve via the artist folder itself + // └── Album/ + // └── 01 - Track.mp3 + It("still resolves the artist folder and returns artist.*", func() { + conf.Server.ArtistArtPriority = "artist.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), + "Artist/artist.jpg": imageFile("artist-folder"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("the artist's only album has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins (artist.* before album/artist.*) + // └── Album/ + // ├── artist.jpg + // ├── CD1/01 - Track.mp3 + // └── CD2/02 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/Album/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("one album has disc subfolders and another sits at artist level", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/03 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album2/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album1/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + + When("every album of the artist has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/ + // ├── CD1/03 - Track.mp3 + // └── CD2/04 - Track.mp3 + It("prefers the artist-folder image over the album-folder one", func() { + conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}), + "Artist/Album2/CD1/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/Album2/CD2/04 - Track.mp3": trackFile(4, "Track 4", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": imageFile("artist-folder"), + "Artist/Album1/artist.jpg": imageFile("album-artist"), + }) + scan() + + ar := soleArtist() + artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) + Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) + }) + }) + When("an artist has an uploaded image and a matching artist.* file", func() { // / // └── artwork/ diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 8ad07773b..58a47c8ae 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -20,6 +20,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" + "github.com/navidrome/navidrome/utils/slice" ) type albumArtworkReader struct { @@ -103,17 +104,13 @@ func (a *albumArtworkReader) fromCoverArtPriority(ctx context.Context, ffmpeg ff return ff } -func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...model.Album) ([]string, []string, *time.Time, error) { - var folderIDs []string - for _, album := range albums { - folderIDs = append(folderIDs, album.FolderIDs...) - } - folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, album model.Album) ([]string, []string, *time.Time, error) { + folders, err := loadFolders(ctx, ds, album.FolderIDs) if err != nil { return nil, nil, nil, err } - parent, err := albumRootParent(ctx, ds, folders, folderIDs) + parent, err := albumRootParent(ctx, ds, folders, album.FolderIDs) if err != nil { return nil, nil, nil, err } @@ -121,11 +118,21 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo folders = append(folders, *parent) } - var paths []string + paths := slice.Map(folders, func(f model.Folder) string { return f.AbsolutePath() }) + imgFiles, updatedAt := folderImages(folders) + return paths, imgFiles, &updatedAt, nil +} + +func loadFolders(ctx context.Context, ds model.DataStore, folderIDs []string) ([]model.Folder, error) { + return ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +} + +// folderImages collects the folders' image files, sorted so files without +// numeric suffixes win (e.g. cover.jpg over cover.1.jpg). +func folderImages(folders []model.Folder) ([]string, time.Time) { var imgFiles []string var updatedAt time.Time for _, f := range folders { - paths = append(paths, f.AbsolutePath()) if f.ImagesUpdatedAt.After(updatedAt) { updatedAt = f.ImagesUpdatedAt } @@ -134,13 +141,8 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo imgFiles = append(imgFiles, path.Join(rel, img)) } } - - // Sort image files to ensure consistent selection of cover art - // This prioritizes files without numeric suffixes (e.g., cover.jpg over cover.1.jpg) - // by comparing base filenames without extensions slices.SortFunc(imgFiles, compareImageFiles) - - return paths, imgFiles, &updatedAt, nil + return imgFiles, updatedAt } // albumRootParent returns the common parent of the album's folders when it diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index fe4a1a545..c95ddedaa 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -3,6 +3,7 @@ package artwork import ( "context" "errors" + "path/filepath" "time" "github.com/navidrome/navidrome/model" @@ -339,6 +340,32 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("promotes the album root parent into the returned paths", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "artistFolder", + Path: ".", + Name: "Artist", + ParentID: "libraryRoot", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"folder.jpg"}, + } + + paths, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(paths).To(Equal([]string{filepath.Join("Artist", "Album"), "Artist"})) + }) + It("does not include parent images when other albums' audio lives under the parent", func() { // Simulates: Artist/folder.jpg with Artist/Album (no images) and // another album's tracks elsewhere under the artist folder diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 37b7b6dee..fe5bcb196 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -19,6 +19,7 @@ import ( "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" ) @@ -54,7 +55,7 @@ func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.A if err != nil { return nil, err } - albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, als...) + albumPaths, imgFiles, imagesUpdatedAt, err := loadArtistAlbumRoots(ctx, artwork.ds, als) if err != nil { return nil, err } @@ -232,17 +233,64 @@ func escapeGlobLiteral(s string) string { return b.String() } +// loadArtistAlbumRoots returns one path per album — the deepest folder holding +// all of that album's tracks — so an album split into disc subfolders can't +// pull the artist folder's common prefix below the artist level. +func loadArtistAlbumRoots(ctx context.Context, ds model.DataStore, albums model.Albums) ([]string, []string, *time.Time, error) { + var folderIDs []string + for _, album := range albums { + folderIDs = append(folderIDs, album.FolderIDs...) + } + folders, err := loadFolders(ctx, ds, folderIDs) + if err != nil { + return nil, nil, nil, err + } + + pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) { + return f.ID, f.AbsolutePath() + }) + var roots []string + for _, album := range albums { + var albumPaths []string + for _, fid := range album.FolderIDs { + if p, ok := pathByID[fid]; ok { + albumPaths = append(albumPaths, p) + } + } + if len(albumPaths) > 0 { + roots = append(roots, commonDir(albumPaths)) + } + } + + imgFiles, updatedAt := folderImages(folders) + return roots, imgFiles, &updatedAt, nil +} + +// commonDir returns the deepest directory containing all paths. Trailing +// separators keep the comparison on segment boundaries, so a shared name +// fragment (".../Album" and ".../Album2") is never read as a shared directory. +func commonDir(paths []string) string { + sep := string(filepath.Separator) + common := str.LongestCommonPrefix(slice.Map(paths, func(p string) string { return p + sep })) + if !strings.HasSuffix(common, sep) { + common, _ = filepath.Split(common) + } + return filepath.Clean(common) +} + func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) { if len(albums) == 0 { return "", time.Time{}, nil } libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries - folderPath := str.LongestCommonPrefix(paths) - if !strings.HasSuffix(folderPath, string(filepath.Separator)) { - folderPath, _ = filepath.Split(folderPath) + // paths holds one root per album: two or more distinct roots already meet at + // the artist folder, while a single root is an album folder needing a climb. + roots := slices.Compact(slices.Sorted(slices.Values(paths))) + folderPath := commonDir(roots) + if len(roots) < 2 { + folderPath = filepath.Dir(folderPath) } - folderPath = filepath.Dir(folderPath) // Manipulate the path to get the folder ID // TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 6d6d58fc5..c75f8447e 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -85,6 +85,19 @@ var _ = Describe("artistArtworkReader", func() { }) }) + When("two albums share the same folder", func() { + It("climbs above the shared album folder", func() { + paths = []string{ + filepath.FromSlash("/music/artist/split"), + filepath.FromSlash("/music/artist/split"), + } + folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) + Expect(err).ToNot(HaveOccurred()) + Expect(folder).To(Equal(filepath.FromSlash("/music/artist"))) + Expect(upd).To(Equal(expectedUpdTime)) + }) + }) + When("the album paths contain same prefix", func() { It("returns the common prefix", func() { paths = []string{ From 600ea5482c36d3705fbca1d9ab749d9bdafd6f80 Mon Sep 17 00:00:00 2001 From: fxj368 <62541194+fxj368@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:36:37 +0800 Subject: [PATCH 4/6] fix(ui): update Chinese (Simplified) translation (#5873) Signed-off-by: Deluan --- resources/i18n/zh-Hans.json | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/resources/i18n/zh-Hans.json b/resources/i18n/zh-Hans.json index 21778506a..24f55357a 100644 --- a/resources/i18n/zh-Hans.json +++ b/resources/i18n/zh-Hans.json @@ -32,7 +32,7 @@ "rating": "评分", "quality": "品质", "bpm": "BPM", - "playDate": "最后一次播放", + "playDate": "上次播放", "createdAt": "加入日期", "grouping": "分组", "mood": "情绪", @@ -184,7 +184,7 @@ "maxBitRate": "最大比特率", "client": "客户端", "userName": "用户名", - "lastSeen": "上次浏览", + "lastSeen": "上次使用", "reportRealPath": "报告真实路径", "scrobbleEnabled": "发送个性化记录到外部服务" } @@ -210,7 +210,8 @@ "songCount": "歌曲数", "comment": "注释", "sync": "自动导入", - "path": "导入路径" + "path": "导入路径", + "starred": "收藏" }, "actions": { "selectPlaylist": "选择歌单", @@ -530,7 +531,7 @@ "no_results": "未找到结果", "no_more_results": "页码 %{page} 超出范围,尝试返回上一页", "page_out_of_boundaries": "页码 %{page} 超出范围", - "page_out_from_end": "已经最后一页", + "page_out_from_end": "已经是最后一页", "page_out_from_begin": "已经是第一页", "page_range_info": "%{offsetBegin}-%{offsetEnd} / %{total}", "page_rows_per_page": "每页项目数:", @@ -636,6 +637,7 @@ "albumList": "专辑", "playlists": "歌单", "sharedPlaylists": "共享的歌单", + "onlyFavourites": "仅显示收藏项", "about": "关于" }, "player": { @@ -668,10 +670,10 @@ "homepage": "主页", "source": "源代码", "featureRequests": "功能需求", - "lastInsightsCollection": "最近的分析收集", + "lastInsightsCollection": "上次的匿名数据收集", "insights": { - "disabled": "禁用", - "waiting": "等待" + "disabled": "已禁用", + "waiting": "等待中" } }, "tabs": { From d23b68a4385d42b647cb2c349ba5e1ac36fc4c1e Mon Sep 17 00:00:00 2001 From: Mateusz Piotrowski Date: Fri, 31 Jul 2026 14:54:29 +0200 Subject: [PATCH 5/6] fix: typo in description of --prometheus.enabled (#5878) Remove a stray back tick. --- cmd/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/root.go b/cmd/root.go index 9e2b38cd8..d9e766c32 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -394,7 +394,7 @@ func init() { rootCmd.Flags().String("albumplaycountmode", viper.GetString("albumplaycountmode"), "how to compute playcount for albums. absolute (default) or normalized") rootCmd.Flags().Bool("autoimportplaylists", viper.GetBool("autoimportplaylists"), "enable/disable .m3u playlist auto-import`") - rootCmd.Flags().Bool("prometheus.enabled", viper.GetBool("prometheus.enabled"), "enable/disable prometheus metrics endpoint`") + rootCmd.Flags().Bool("prometheus.enabled", viper.GetBool("prometheus.enabled"), "enable/disable prometheus metrics endpoint") rootCmd.Flags().String("prometheus.metricspath", viper.GetString("prometheus.metricspath"), "http endpoint for prometheus metrics") _ = viper.BindPFlag("address", rootCmd.Flags().Lookup("address")) 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 6/6] 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})