diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index 8c714945e..86a1055f8 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -96,6 +96,27 @@ jobs: exit 1 fi + validate-migrations: + name: Validate DB migrations + runs-on: ubuntu-latest + # PR-only gate is at step level: a job-level skip would propagate through + # the needs chain (actions/runner#491) and skip all release jobs on tag pushes. + steps: + - uses: actions/checkout@v7 + if: github.event_name == 'pull_request' + with: + fetch-depth: 0 + # Refresh the base branch so the check compares against its CURRENT tip, + # not the (possibly stale) commit the PR was opened against. + - name: Fetch latest base branch + if: github.event_name == 'pull_request' + run: git fetch --no-tags origin "+refs/heads/${{ github.event.pull_request.base.ref }}:refs/remotes/origin/${{ github.event.pull_request.base.ref }}" + - name: Validate migration ordering and naming + if: github.event_name == 'pull_request' + env: + BASE_REF: origin/${{ github.event.pull_request.base.ref }} + run: ./.github/workflows/validate-migrations.sh + go: name: Test Go code runs-on: ubuntu-latest @@ -257,7 +278,7 @@ jobs: build: name: Build - needs: [js, go, go-windows, go-lint, i18n-lint, git-version, check-push-enabled] + needs: [js, go, go-windows, go-lint, i18n-lint, git-version, check-push-enabled, validate-migrations] strategy: matrix: platform: [ linux/amd64, linux/arm64, linux/arm/v5, linux/arm/v6, linux/arm/v7, linux/386, linux/riscv64, darwin/amd64, darwin/arm64, windows/amd64, windows/386 ] diff --git a/.github/workflows/validate-migrations.sh b/.github/workflows/validate-migrations.sh new file mode 100755 index 000000000..07d05c3a6 --- /dev/null +++ b/.github/workflows/validate-migrations.sh @@ -0,0 +1,150 @@ +#!/usr/bin/env bash +# +# Validates DB migrations added by a pull request: +# 1. Ordering - an added migration must be NEWER than the latest migration +# already on the base branch. Goose applies migrations in +# timestamp order, so an older-timestamped migration would be +# silently skipped on databases already upgraded past it. +# 2. Uniqueness - no two migration files may share a timestamp. +# 3. Naming - files must match YYYYMMDDHHMMSS_lower_snake_name.(sql|go). +# +# On failure it prints a human-readable message and, when running in GitHub +# Actions, emits an error annotation bound to the offending file so the message +# also renders inline in the PR "Files changed" tab. +# +# Compares HEAD against $BASE_REF (default origin/master). Requires full history +# (fetch-depth: 0 in CI). +# -e is intentionally omitted: the script accumulates violations into $status +# and must not exit on the first non-zero command (grep no-match, a false [[ ]] +# in an if, `is_migration || continue`). +set -uo pipefail +export LC_ALL=C + +MIGRATIONS_DIR="db/migrations" +BASE_REF="${BASE_REF:-origin/master}" +NAME_RE='^[0-9]{14}_[a-z0-9_]+\.(sql|go)$' + +status=0 + +# Log a message to stderr and mark the run as failed. +fail() { + printf '%s\n' "$1" >&2 + status=1 +} + +# Emit a GitHub Actions error annotation bound to a file, so the message renders +# inline on the offending migration in the PR "Files changed" tab. No-op outside +# CI. `%`, newline and CR are encoded as required by the workflow-command syntax +# (the `%` replacement must run first so the encodings we add aren't re-escaped). +annotate() { # $1=file $2=message + [ "${GITHUB_ACTIONS:-}" = "true" ] || return 0 + local msg="$2" + msg="${msg//'%'/%25}" + msg="${msg//$'\n'/%0A}" + msg="${msg//$'\r'/%0D}" + printf '::error file=%s,line=1::%s\n' "$1" "$msg" +} + +# Report a migration problem: log it, annotate the offending file, mark failed. +report() { # $1=file $2=message + fail "$2" + printf '\n' >&2 + annotate "$1" "$2" +} + +human_ts() { + local t="$1" + printf '%s-%s-%s %s:%s:%s' "${t:0:4}" "${t:4:2}" "${t:6:2}" "${t:8:2}" "${t:10:2}" "${t:12:2}" +} + +is_migration() { # $1=basename -> 0 if a .sql/.go file with a 14-digit prefix + local b="$1" + case "$b" in + *.sql | *.go) ;; + *) return 1 ;; + esac + [[ "${b%%_*}" =~ ^[0-9]{14}$ ]] +} + +if ! git rev-parse --verify --quiet "$BASE_REF" >/dev/null; then + printf '❌ Cannot resolve base ref "%s". In CI, check out with fetch-depth: 0.\n' "$BASE_REF" >&2 + exit 1 +fi + +# --- Newest timestamp already on the base branch --- +base_max="" +base_max_file="" +while IFS= read -r f; do + [ -z "$f" ] && continue + b="$(basename "$f")" + is_migration "$b" || continue + ts="${b%%_*}" + if [[ "$ts" > "$base_max" ]]; then + base_max="$ts" + base_max_file="$f" + fi +done < <(git ls-tree -r --name-only "$BASE_REF" -- "$MIGRATIONS_DIR" 2>/dev/null) + +# --- Ordering + naming on files added by this PR --- +while IFS= read -r f; do + [ -z "$f" ] && continue + b="$(basename "$f")" + case "$b" in + *.sql) ;; # any .sql in this dir must be a migration + *.go) [[ "$b" == [0-9]* ]] || continue ;; # non-timestamped .go = helper (e.g. migration.go), skip + *) continue ;; + esac + if [ "${f%/*}" != "$MIGRATIONS_DIR" ]; then + report "$f" "❌ Migration file in a subdirectory: $f + Migrations must live directly in $MIGRATIONS_DIR/ — only $MIGRATIONS_DIR/*.sql (and + top-level .go migrations) are embedded, so a nested file would be SILENTLY SKIPPED. + Move it to $MIGRATIONS_DIR/$b." + continue + fi + if ! [[ "$b" =~ $NAME_RE ]]; then + report "$f" "❌ Malformed migration filename: $f + Expected YYYYMMDDHHMMSS_lower_snake_name.(sql|go); the name segment must be lowercase. + Regenerate with: make migration-sql name= (or make migration-go name=)" + continue + fi + ts="${b%%_*}" + if [[ -n "$base_max" ]] && ! [[ "$ts" > "$base_max" ]]; then + report "$f" "❌ Migration ordering error: $f ($(human_ts "$ts")) + is older than (or equal to) the newest migration already on ${BASE_REF#origin/}: + $base_max_file ($(human_ts "$base_max")) + + Goose applies migrations in timestamp order, so databases already upgraded + past that point would SILENTLY SKIP your migration. + + Fix: regenerate it with a current timestamp: + make migration-sql name= (or make migration-go name=) + then move your SQL/Go body into the new file and delete the old one." + fi +done < <(git diff --diff-filter=A --name-only "$BASE_REF"...HEAD -- "$MIGRATIONS_DIR" 2>/dev/null) + +# --- Duplicate timestamps across the merged set (HEAD) --- +all_migs="$(git ls-tree -r --name-only HEAD -- "$MIGRATIONS_DIR" 2>/dev/null)" +dups="$(printf '%s\n' "$all_migs" | while IFS= read -r f; do + b="$(basename "$f")" + is_migration "$b" || continue + printf '%s\n' "${b%%_*}" +done | sort | uniq -d)" +if [ -n "$dups" ]; then + while IFS= read -r ts; do + [ -z "$ts" ] && continue + colliding="$(printf '%s\n' "$all_migs" | grep "/${ts}_" || true)" + printf '❌ Duplicate migration timestamp %s used by multiple files:\n' "$ts" >&2 + while IFS= read -r cf; do + [ -z "$cf" ] && continue + printf ' %s\n' "$cf" >&2 + annotate "$cf" "Duplicate migration timestamp $ts — shared by another migration. Timestamps must be unique; regenerate one with make migration-*." + done <<< "$colliding" + printf ' Every migration needs a unique timestamp. Regenerate one with make migration-*.\n' >&2 + status=1 + done <<< "$dups" +fi + +if [ "$status" -eq 0 ]; then + echo "✅ DB migrations OK (ordering, uniqueness, naming)." +fi +exit "$status" diff --git a/core/playlists/parse_nsp_test.go b/core/playlists/parse_nsp_test.go index 516a5355d..d6d69866f 100644 --- a/core/playlists/parse_nsp_test.go +++ b/core/playlists/parse_nsp_test.go @@ -113,6 +113,20 @@ var _ = Describe("parseNSP", func() { Expect(err.Error()).To(ContainSubstring("SmartPlaylist")) }) + It("rejects a NSP that mixes top-level 'any' and 'all' instead of silently dropping a group", func() { + nsp := `{ + "name": "Overplayed Favorites", + "any": [{"inPlaylist": {"path": "most-played-favorites.nsp"}}], + "all": [{"notInPlaylist": {"path": "favorites-not-played-in-4-yrs.nsp"}}], + "sort": "playCount, lastPlayed" + }` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("SmartPlaylist")) + Expect(err.Error()).To(And(ContainSubstring("all"), ContainSubstring("any"))) + }) + It("gracefully handles non-string name field", func() { nsp := `{"name": 123, "all": [{"is": {"loved": true}}]}` pls := &model.Playlist{Name: "Original"} diff --git a/core/storage/interface.go b/core/storage/interface.go index dc08ca00a..02c1d14d9 100644 --- a/core/storage/interface.go +++ b/core/storage/interface.go @@ -17,6 +17,14 @@ type MusicFS interface { ReadTags(path ...string) (map[string]metadata.Info, error) } +// SymlinkResolverFS is an optional interface for MusicFS implementations backed by a real +// filesystem. ResolveSymlink resolves the whole symlink chain of the named entry at the OS +// level and returns the final target's path — including targets outside the FS root, which +// fs.ReadLink-based resolution cannot follow. +type SymlinkResolverFS interface { + ResolveSymlink(name string) (string, error) +} + // Watcher is a storage with the ability watch the FS and notify changes type Watcher interface { // Start starts a watcher on the whole FS and returns a channel to send detected changes. diff --git a/core/storage/local/local.go b/core/storage/local/local.go index 5384581e0..32aff0955 100644 --- a/core/storage/local/local.go +++ b/core/storage/local/local.go @@ -54,12 +54,23 @@ func (s *localStorage) FS() (storage.MusicFS, error) { if _, err := os.Stat(path); err != nil { //nolint:gosec return nil, fmt.Errorf("%w: %s", err, path) } - return &localFS{FS: os.DirFS(path), extractor: s.extractor}, nil + return &localFS{FS: os.DirFS(path), extractor: s.extractor, root: path}, nil } type localFS struct { fs.FS extractor Extractor + root string +} + +// ResolveSymlink implements storage.SymlinkResolverFS. It resolves the whole chain at the +// OS level, so links whose targets live outside the library folder (not reachable through +// the fs.FS abstraction) still resolve to their final target. +func (lfs *localFS) ResolveSymlink(name string) (string, error) { + if !fs.ValidPath(name) { + return "", &fs.PathError{Op: "resolvesymlink", Path: name, Err: fs.ErrInvalid} + } + return filepath.EvalSymlinks(filepath.Join(lfs.root, filepath.FromSlash(name))) } func (lfs *localFS) ReadTags(path ...string) (map[string]metadata.Info, error) { diff --git a/core/storage/local/local_test.go b/core/storage/local/local_test.go index d65d8214a..90bdd4b5b 100644 --- a/core/storage/local/local_test.go +++ b/core/storage/local/local_test.go @@ -199,6 +199,78 @@ var _ = Describe("LocalStorage", func() { }) }) + Describe("localFS.ResolveSymlink", func() { + var musicFS storage.MusicFS + + BeforeEach(func() { + if runtime.GOOS == "windows" { + Skip("symlink semantics") + } + u, err := storage.LocalPathToURL(tempDir) + Expect(err).ToNot(HaveOccurred()) + musicFS, err = newLocalStorage(u).FS() + Expect(err).ToNot(HaveOccurred()) + }) + + It("implements storage.SymlinkResolverFS", func() { + _, ok := musicFS.(storage.SymlinkResolverFS) + Expect(ok).To(BeTrue()) + }) + + It("resolves a chain that leaves the library folder to its final target", func() { + outside, err := os.MkdirTemp("", "navidrome-symlink-outside-") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { os.RemoveAll(outside) }) + + target := filepath.Join(outside, "final.txt") + Expect(os.WriteFile(target, []byte("data"), 0600)).To(Succeed()) + mid := filepath.Join(outside, "mid.wav") + Expect(os.Symlink(target, mid)).To(Succeed()) + Expect(os.Symlink(mid, filepath.Join(tempDir, "link.wav"))).To(Succeed()) + + resolved, err := musicFS.(storage.SymlinkResolverFS).ResolveSymlink("link.wav") + Expect(err).ToNot(HaveOccurred()) + expected, err := filepath.EvalSymlinks(target) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(expected)) + }) + + It("resolves entries in subfolders (slash-separated fs paths)", func() { + Expect(os.MkdirAll(filepath.Join(tempDir, "sub"), 0755)).To(Succeed()) + target := filepath.Join(tempDir, "real.mp3") + Expect(os.WriteFile(target, []byte("audio"), 0600)).To(Succeed()) + Expect(os.Symlink(target, filepath.Join(tempDir, "sub", "link.mp3"))).To(Succeed()) + + resolved, err := musicFS.(storage.SymlinkResolverFS).ResolveSymlink("sub/link.mp3") + Expect(err).ToNot(HaveOccurred()) + expected, err := filepath.EvalSymlinks(target) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(expected)) + }) + + It("returns an error for a broken symlink", func() { + Expect(os.Symlink(filepath.Join(tempDir, "missing.mp3"), filepath.Join(tempDir, "broken.mp3"))).To(Succeed()) + + _, err := musicFS.(storage.SymlinkResolverFS).ResolveSymlink("broken.mp3") + Expect(err).To(HaveOccurred()) + }) + + It("rejects names that are not valid fs paths", func() { + for _, name := range []string{"../outside.mp3", "/etc/hosts", "sub/../../outside.mp3", ""} { + _, err := musicFS.(storage.SymlinkResolverFS).ResolveSymlink(name) + Expect(err).To(MatchError(fs.ErrInvalid), name) + } + }) + + It("returns an error for a symlink loop", func() { + Expect(os.Symlink(filepath.Join(tempDir, "loop2.mp3"), filepath.Join(tempDir, "loop1.mp3"))).To(Succeed()) + Expect(os.Symlink(filepath.Join(tempDir, "loop1.mp3"), filepath.Join(tempDir, "loop2.mp3"))).To(Succeed()) + + _, err := musicFS.(storage.SymlinkResolverFS).ResolveSymlink("loop1.mp3") + Expect(err).To(HaveOccurred()) + }) + }) + Describe("localFS.ReadTags", func() { var testFile string diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index 31d208d08..8c3d183a9 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -103,21 +103,26 @@ func (c Criteria) MarshalJSON() ([]byte, error) { func (c *Criteria) UnmarshalJSON(data []byte) error { var aux struct { - All unmarshalConjunctionType `json:"all"` - Any unmarshalConjunctionType `json:"any"` - Sort string `json:"sort"` - Order string `json:"order"` - Limit int `json:"limit"` - LimitPercent int `json:"limitPercent"` - Offset int `json:"offset"` + All optionalConjunction `json:"all"` + Any optionalConjunction `json:"any"` + Sort string `json:"sort"` + Order string `json:"order"` + Limit int `json:"limit"` + LimitPercent int `json:"limitPercent"` + Offset int `json:"offset"` } if err := json.Unmarshal(data, &aux); err != nil { return err } - if len(aux.Any) > 0 { - c.Expression = Any(aux.Any) - } else if len(aux.All) > 0 { - c.Expression = All(aux.All) + // A Criteria has a single top-level group. Reject files that provide both keys + // (even when one is [] or null) rather than silently dropping one of them. + if aux.All.present && aux.Any.present { + return errors.New("invalid criteria json: 'all' and 'any' cannot both be used at the top level; nest one inside the other instead") + } + if len(aux.Any.rules) > 0 { + c.Expression = Any(aux.Any.rules) + } else if len(aux.All.rules) > 0 { + c.Expression = All(aux.All.rules) } else { return errors.New("invalid criteria json. missing rules (key 'all' or 'any')") } diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 092cfd36a..7f214e703 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -80,6 +80,28 @@ var _ = Describe("Criteria", func() { }) }) + Context("with both top-level 'all' and 'any'", func() { + It("returns an error instead of silently dropping one of the groups", func() { + jsonStr := `{"any":[{"inPlaylist":{"path":"a.nsp"}}],"all":[{"notInPlaylist":{"path":"b.nsp"}}]}` + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any"))) + }) + + DescribeTable("rejects both keys even when one group is present but empty", + func(jsonStr string) { + var c Criteria + err := json.Unmarshal([]byte(jsonStr), &c) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any"))) + }, + Entry("empty any", `{"any":[],"all":[{"is":{"loved":true}}]}`), + Entry("empty all", `{"all":[],"any":[{"is":{"loved":true}}]}`), + Entry("null any", `{"any":null,"all":[{"is":{"loved":true}}]}`), + ) + }) + Describe("LimitPercent", func() { Describe("JSON round-trip", func() { It("marshals and unmarshals limitPercent", func() { diff --git a/model/criteria/json.go b/model/criteria/json.go index beded9d1f..d0f453524 100644 --- a/model/criteria/json.go +++ b/model/criteria/json.go @@ -33,6 +33,20 @@ func (uc *unmarshalConjunctionType) UnmarshalJSON(data []byte) error { return nil } +// optionalConjunction is a top-level "all"/"any" value that remembers whether its +// key was present at all, so a Criteria providing both can be rejected. encoding/json +// calls UnmarshalJSON even for a JSON null, so present is set whenever the key appears +// — including as [] or null — while an absent key leaves it false. +type optionalConjunction struct { + present bool + rules unmarshalConjunctionType +} + +func (o *optionalConjunction) UnmarshalJSON(data []byte) error { + o.present = true + return json.Unmarshal(data, &o.rules) +} + func unmarshalExpression(opName string, rawValue json.RawMessage) Expression { m := make(map[string]any) err := json.Unmarshal(rawValue, &m) diff --git a/plugins/host_taskqueue.go b/plugins/host_taskqueue.go index a5db3344f..2f74c0aa4 100644 --- a/plugins/host_taskqueue.go +++ b/plugins/host_taskqueue.go @@ -82,7 +82,8 @@ type taskQueueServiceImpl struct { } // newTaskQueueService creates a new taskQueueServiceImpl with its own SQLite database. -func newTaskQueueService(pluginName string, manager *Manager, maxConcurrency int32) (*taskQueueServiceImpl, error) { +// The given ctx bounds the service's background work (queue workers, cleanup loop). +func newTaskQueueService(ctx context.Context, pluginName string, manager *Manager, maxConcurrency int32) (*taskQueueServiceImpl, error) { dataDir := filepath.Join(conf.Server.DataFolder.String(), "plugins", pluginName) if err := os.MkdirAll(dataDir, 0700); err != nil { return nil, fmt.Errorf("creating plugin data directory: %w", err) @@ -102,7 +103,7 @@ func newTaskQueueService(pluginName string, manager *Manager, maxConcurrency int return nil, fmt.Errorf("creating taskqueue schema: %w", err) } - ctx, cancel := context.WithCancel(manager.ctx) //nolint:gosec // cancel is stored in struct and called in Close() + ctx, cancel := context.WithCancel(ctx) //nolint:gosec // cancel is stored in struct and called in Close() s := &taskQueueServiceImpl{ pluginName: pluginName, diff --git a/plugins/host_taskqueue_test.go b/plugins/host_taskqueue_test.go index faff79c8e..d459fd69b 100644 --- a/plugins/host_taskqueue_test.go +++ b/plugins/host_taskqueue_test.go @@ -42,15 +42,11 @@ var _ = Describe("TaskQueueService", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.DataFolder = conf.NewDir(tmpDir) - // Create a mock manager with context - managerCtx, cancel := context.WithCancel(ctx) manager = &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx, } - DeferCleanup(cancel) - service, err = newTaskQueueService("test_plugin", manager, 5) + service, err = newTaskQueueService(ctx, "test_plugin", manager, 5) Expect(err).ToNot(HaveOccurred()) }) @@ -730,14 +726,11 @@ var _ = Describe("TaskQueueService", func() { service.Close() // Create a new service pointing to the same DB - managerCtx2, cancel2 := context.WithCancel(ctx) - DeferCleanup(cancel2) manager2 := &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx2, } - service, err = newTaskQueueService("test_plugin", manager2, 5) + service, err = newTaskQueueService(ctx, "test_plugin", manager2, 5) Expect(err).ToNot(HaveOccurred()) // Override callback to succeed @@ -775,14 +768,11 @@ var _ = Describe("TaskQueueService", func() { Describe("Plugin isolation", func() { It("uses separate databases for different plugins", func() { - managerCtx2, cancel2 := context.WithCancel(ctx) - DeferCleanup(cancel2) manager2 := &Manager{ plugins: make(map[string]*plugin), - ctx: managerCtx2, } - service2, err := newTaskQueueService("other_plugin", manager2, 5) + service2, err := newTaskQueueService(ctx, "other_plugin", manager2, 5) Expect(err).ToNot(HaveOccurred()) defer service2.Close() diff --git a/plugins/host_websocket.go b/plugins/host_websocket.go index eef1e6236..90403f4c0 100644 --- a/plugins/host_websocket.go +++ b/plugins/host_websocket.go @@ -54,6 +54,7 @@ type wsConnection struct { // webSocketServiceImpl implements host.WebSocketService. // It provides plugins with WebSocket communication capabilities. type webSocketServiceImpl struct { + baseCtx context.Context // bounds the read loops, which outlive the Connect() call pluginName string manager *Manager requiredHosts []string @@ -63,8 +64,9 @@ type webSocketServiceImpl struct { } // newWebSocketService creates a new WebSocketService for a plugin. -func newWebSocketService(pluginName string, manager *Manager, permission *WebSocketPermission) *webSocketServiceImpl { +func newWebSocketService(ctx context.Context, pluginName string, manager *Manager, permission *WebSocketPermission) *webSocketServiceImpl { return &webSocketServiceImpl{ + baseCtx: ctx, pluginName: pluginName, manager: manager, requiredHosts: permission.RequiredHosts, @@ -129,11 +131,12 @@ func (s *webSocketServiceImpl) Connect(ctx context.Context, urlStr string, heade s.connections[connectionID] = wsConn s.mu.Unlock() - // Start read goroutine with manager's context. - // We use manager.ctx instead of the caller's ctx because the readLoop must - // outlive the Connect() call. The manager's context is cancelled during - // application shutdown, ensuring graceful cleanup. - go s.readLoop(s.manager.ctx, connectionID, wsConn) + // Start read goroutine with the service's base context instead of the + // caller's ctx, because the readLoop must outlive the Connect() call. + // Connections are closed by Close() when the plugin is unloaded, which ends + // the readLoop; the base context is a backstop that also ends it on server + // shutdown (it is never cancelled in one-shot CLI runs). + go s.readLoop(s.baseCtx, connectionID, wsConn) log.Debug(ctx, "WebSocket connected", "plugin", s.pluginName, "connectionID", connectionID, "url", urlStr) return connectionID, nil diff --git a/plugins/manager_loader.go b/plugins/manager_loader.go index 604fba3a7..757ededb5 100644 --- a/plugins/manager_loader.go +++ b/plugins/manager_loader.go @@ -30,11 +30,23 @@ type serviceContext struct { allLibraries bool // If true, plugin can access all libraries } +// baseCtx returns the manager's lifecycle context, for host services that +// outlive the plugin call that created them. It falls back to +// context.Background() when the manager was never started, which is the case +// for CLI commands (e.g. `navidrome plugin enable`) that load plugins without +// calling Start. +func (c *serviceContext) baseCtx() context.Context { + if c.manager.ctx == nil { + return context.Background() + } + return c.manager.ctx +} + // hostServiceEntry defines a host service for table-driven registration. type hostServiceEntry struct { name string hasPermission func(*Permissions) bool - create func(*serviceContext) ([]extism.HostFunction, io.Closer) + create func(*serviceContext) ([]extism.HostFunction, io.Closer, error) } // hostServices defines all available host services. @@ -43,119 +55,117 @@ var hostServices = []hostServiceEntry{ { name: "Config", hasPermission: func(p *Permissions) bool { return true }, // Always available, no permission required - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newConfigService(ctx.pluginName, ctx.config) - return host.RegisterConfigHostFunctions(service), nil + return host.RegisterConfigHostFunctions(service), nil, nil }, }, { name: "SubsonicAPI", hasPermission: func(p *Permissions) bool { return p != nil && p.Subsonicapi != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newSubsonicAPIService(ctx.pluginName, ctx.manager.subsonicRouter, ctx.manager.ds, newUserAccess(ctx.allowedUsers, ctx.allUsers)) - return host.RegisterSubsonicAPIHostFunctions(service), nil + return host.RegisterSubsonicAPIHostFunctions(service), nil, nil }, }, { name: "Scheduler", hasPermission: func(p *Permissions) bool { return p != nil && p.Scheduler != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newSchedulerService(ctx.pluginName, ctx.manager, scheduler.GetInstance()) - return host.RegisterSchedulerHostFunctions(service), service + return host.RegisterSchedulerHostFunctions(service), service, nil }, }, { name: "WebSocket", hasPermission: func(p *Permissions) bool { return p != nil && p.Websocket != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Websocket - service := newWebSocketService(ctx.pluginName, ctx.manager, perm) - return host.RegisterWebSocketHostFunctions(service), service + service := newWebSocketService(ctx.baseCtx(), ctx.pluginName, ctx.manager, perm) + return host.RegisterWebSocketHostFunctions(service), service, nil }, }, { name: "Artwork", hasPermission: func(p *Permissions) bool { return p != nil && p.Artwork != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newArtworkService() - return host.RegisterArtworkHostFunctions(service), nil + return host.RegisterArtworkHostFunctions(service), nil, nil }, }, { name: "Cache", hasPermission: func(p *Permissions) bool { return p != nil && p.Cache != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newCacheService(ctx.pluginName) - return host.RegisterCacheHostFunctions(service), service + return host.RegisterCacheHostFunctions(service), service, nil }, }, { name: "Library", hasPermission: func(p *Permissions) bool { return p != nil && p.Library != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Library service := newLibraryService(ctx.manager.ds, perm, ctx.allowedLibraries, ctx.allLibraries) - return host.RegisterLibraryHostFunctions(service), nil + return host.RegisterLibraryHostFunctions(service), nil, nil }, }, { name: "KVStore", hasPermission: func(p *Permissions) bool { return p != nil && p.Kvstore != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Kvstore - service, err := newKVStoreService(ctx.manager.ctx, ctx.pluginName, perm) + service, err := newKVStoreService(ctx.baseCtx(), ctx.pluginName, perm) if err != nil { - log.Error("Failed to create KVStore service", "plugin", ctx.pluginName, err) - return nil, nil + return nil, nil, err } - return host.RegisterKVStoreHostFunctions(service), service + return host.RegisterKVStoreHostFunctions(service), service, nil }, }, { name: "Users", hasPermission: func(p *Permissions) bool { return p != nil && p.Users != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { service := newUsersService(ctx.manager.ds, ctx.allowedUsers, ctx.allUsers) - return host.RegisterUsersHostFunctions(service), nil + return host.RegisterUsersHostFunctions(service), nil, nil }, }, { name: "Matcher", hasPermission: func(p *Permissions) bool { return p != nil && p.Matcher != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { hasFilesystemPerm := ctx.permissions.Library != nil && ctx.permissions.Library.Filesystem service := newMatcherService( ctx.manager.ds, hasFilesystemPerm, newUserAccess(ctx.allowedUsers, ctx.allUsers), newLibraryAccess(ctx.allowedLibraries, ctx.allLibraries), ) - return host.RegisterMatcherHostFunctions(service), nil + return host.RegisterMatcherHostFunctions(service), nil, nil }, }, { name: "HTTP", hasPermission: func(p *Permissions) bool { return p != nil && p.Http != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Http service := newHTTPService(ctx.pluginName, perm) - return host.RegisterHTTPHostFunctions(service), nil + return host.RegisterHTTPHostFunctions(service), nil, nil }, }, { name: "Task", hasPermission: func(p *Permissions) bool { return p != nil && p.Taskqueue != nil }, - create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer) { + create: func(ctx *serviceContext) ([]extism.HostFunction, io.Closer, error) { perm := ctx.permissions.Taskqueue maxConcurrency := int32(1) if perm.MaxConcurrency > 0 { maxConcurrency = int32(perm.MaxConcurrency) } - service, err := newTaskQueueService(ctx.pluginName, ctx.manager, maxConcurrency) + service, err := newTaskQueueService(ctx.baseCtx(), ctx.pluginName, ctx.manager, maxConcurrency) if err != nil { - log.Error("Failed to create Task service", "plugin", ctx.pluginName, err) - return nil, nil + return nil, nil, err } - return host.RegisterTaskHostFunctions(service), service + return host.RegisterTaskHostFunctions(service), service, nil }, }, } @@ -256,6 +266,7 @@ func (m *Manager) loadEnabledPlugins(ctx context.Context) error { // loadPluginWithConfig loads a plugin with configuration from DB. // The p.Path should point to an .ndp package file. func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { + // NewContext falls back to context.Background() when m.ctx is nil (unstarted manager) ctx := log.NewContext(m.ctx, "plugin", p.ID) if m.stopped.Load() { @@ -328,6 +339,15 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { // Build host functions based on permissions from manifest var hostFunctions []extism.HostFunction var closers []io.Closer + loaded := false + // On success the closers are owned by the registered plugin; on any + // failure past this point, close them so partially-created services + // don't leak goroutines or file handles. + defer func() { + if !loaded { + closeAll(closers) + } + }() svcCtx := &serviceContext{ pluginName: p.ID, @@ -341,7 +361,10 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { } for _, entry := range hostServices { if entry.hasPermission(pkg.Manifest.Permissions) { - funcs, closer := entry.create(svcCtx) + funcs, closer, err := entry.create(svcCtx) + if err != nil { + return fmt.Errorf("creating %s service: %w", entry.name, err) + } hostFunctions = append(hostFunctions, funcs...) if closer != nil { closers = append(closers, closer) @@ -400,6 +423,7 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { libraries: newLibraryAccess(allowedLibraries, p.AllLibraries), } m.mu.Unlock() + loaded = true // Call plugin init function callPluginInit(ctx, m.plugins[p.ID]) @@ -407,6 +431,14 @@ func (m *Manager) loadPluginWithConfig(p *model.Plugin) error { return nil } +// closeAll closes host service closers accumulated before a load failure, +// so partially-created services don't leak goroutines or file handles. +func closeAll(closers []io.Closer) { + for _, c := range closers { + _ = c.Close() + } +} + // parsePluginConfig parses a JSON config string into a map of string values. // For Extism, all config values must be strings, so non-string values are serialized as JSON. func parsePluginConfig(configJSON string) (map[string]string, error) { diff --git a/plugins/manager_loader_load_test.go b/plugins/manager_loader_load_test.go new file mode 100644 index 000000000..8f35548af --- /dev/null +++ b/plugins/manager_loader_load_test.go @@ -0,0 +1,78 @@ +//go:build !windows + +package plugins + +import ( + "crypto/sha256" + "encoding/hex" + "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" +) + +var _ = Describe("loadPluginWithConfig", func() { + var manager *Manager + var dataDir string + + BeforeEach(func() { + pluginsDir := GinkgoT().TempDir() + dataDir = GinkgoT().TempDir() + + src := filepath.Join(testdataDir, "test-taskqueue"+PackageExtension) + data, err := os.ReadFile(src) + Expect(err).ToNot(HaveOccurred()) + dest := filepath.Join(pluginsDir, "test-taskqueue"+PackageExtension) + Expect(os.WriteFile(dest, data, 0600)).To(Succeed()) + hash := sha256.Sum256(data) + + DeferCleanup(configtest.SetupConfig()) + conf.Server.Plugins.Enabled = true + conf.Server.Plugins.Folder = conf.NewDir(pluginsDir) + conf.Server.Plugins.AutoReload = false + conf.Server.DataFolder = conf.NewDir(dataDir) + + repo := tests.CreateMockPluginRepo() + repo.Permitted = true + repo.SetData(model.Plugins{{ + ID: "test-taskqueue", + Path: dest, + SHA256: hex.EncodeToString(hash[:]), + Enabled: false, + }}) + manager = &Manager{ + plugins: make(map[string]*plugin), + ds: &tests.MockDataStore{MockedPlugin: repo}, + metrics: noopMetricsRecorder{}, + subsonicRouter: http.NotFoundHandler(), + } + }) + + Describe("host service creation failures", func() { + It("reports the Task service creation error instead of a missing host function", func() { + Expect(manager.Start(GinkgoT().Context())).To(Succeed()) + DeferCleanup(func() { _ = manager.Stop() }) + + // Block the taskqueue data dir by creating a file where the directory should be + Expect(os.WriteFile(filepath.Join(dataDir, "plugins"), nil, 0600)).To(Succeed()) + + err := manager.EnablePlugin(GinkgoT().Context(), "test-taskqueue") + Expect(err).To(MatchError(ContainSubstring("creating Task service"))) + Expect(err).ToNot(MatchError(ContainSubstring("not exported"))) + }) + }) + + Describe("unstarted manager", func() { + It("enables a taskqueue plugin on a manager that was never started", func() { + // CLI commands (navidrome plugin enable) use the manager without calling Start + Expect(manager.EnablePlugin(GinkgoT().Context(), "test-taskqueue")).To(Succeed()) + DeferCleanup(func() { _ = manager.unloadPlugin("test-taskqueue") }) + }) + }) +}) diff --git a/scanner/scanner_suite_test.go b/scanner/scanner_suite_test.go index 9ee6fc89b..10be0401f 100644 --- a/scanner/scanner_suite_test.go +++ b/scanner/scanner_suite_test.go @@ -2,17 +2,34 @@ package scanner_test import ( "context" + "io/fs" "os" "testing" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/storage/local" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model/metadata" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "go.uber.org/goleak" ) +// The local storage is registered in this test binary, so any spec (or background watcher) +// touching a file:// library needs a default extractor to avoid a startup fatal. +type noopSuiteExtractor struct{} + +func (noopSuiteExtractor) Parse(...string) (map[string]metadata.Info, error) { return nil, nil } +func (noopSuiteExtractor) Version() string { return "0" } + +func init() { + local.RegisterExtractor(consts.DefaultScannerExtractor, func(fs.FS, string) local.Extractor { + return noopSuiteExtractor{} + }) +} + func TestScanner(t *testing.T) { // Only run goleak checks when the GOLEAK env var is set if os.Getenv("GOLEAK") != "" { diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 7f3dca775..cc3720717 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -6,6 +6,7 @@ import ( "errors" "path/filepath" "testing/fstest" + "time" "github.com/Masterminds/squirrel" "github.com/google/uuid" @@ -212,6 +213,15 @@ var _ = Describe("Scanner", Ordered, func() { _, err := db.Db().ExecContext(ctx, "UPDATE artist SET search_normalized = '' WHERE name = 'GØGGS'") Expect(err).ToNot(HaveOccurred()) + // Backdate the folder so the next full scan reliably sees it as outdated. + // isOutdated() compares folder.updated_at (written by this scan) against the + // next scan's last_scan_started_at with a strict Before(); back-to-back scans + // can capture both within one clock tick on Windows (coarse wall-clock), making + // the refresh flaky. Backdating forces the comparison to be unambiguous. + _, err = db.Db().ExecContext(ctx, + "UPDATE folder SET updated_at = ?", time.Now().Add(-time.Hour)) + Expect(err).ToNot(HaveOccurred()) + Expect(runScanner(ctx, true)).To(Succeed()) Expect(searchNormalized()).To(Equal("GOGGS")) }) diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 55bbab684..887344b1b 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -5,11 +5,13 @@ import ( "io/fs" "maps" "path" + "path/filepath" "slices" "sort" "strings" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils" @@ -232,6 +234,20 @@ func resolveEntryName(ctx context.Context, fsys fs.FS, dirPath string, entry fs. log.Trace(ctx, "Scanner: Skipping symlink, following is disabled", "path", linkPath) return "", false } + // OS-backed filesystems can resolve the whole chain, even when it leaves the FS root + // (e.g. a link into another folder/drive), so the final target is always what gets + // classified. The fs.ReadLink loop below can't see past the root: it classifies by the + // last in-chain name it can reach. + if resolver, ok := fsys.(storage.SymlinkResolverFS); ok { + target, err := resolver.ResolveSymlink(linkPath) + if err != nil { + log.Trace(ctx, "Scanner: Skipping symlink, cannot resolve target", "path", linkPath, err) + return "", false + } + resolved := filepath.Base(target) + log.Trace(ctx, "Scanner: Resolved symlink", "path", linkPath, "target", target, "name", resolved) + return resolved, true + } cur := linkPath for hop := 0; hop < maxSymlinkHops; hop++ { target, err := fs.ReadLink(fsys, cur) diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index f3b13a4ef..9fb650c4d 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -432,6 +432,79 @@ var _ = Describe("walk_dir_tree", func() { }) }) + // Regression for #5752: the production localFS must resolve file symlinks. + // It wraps os.DirFS behind the fs.FS interface, so fs.ReadLink-based + // resolution is not available and full OS-level resolution is required. + Context("production local storage FS", func() { + var libRoot string + var musicFS storage.MusicFS + + BeforeEach(func() { + conf.Server.Scanner.FollowSymlinks = true + + // Reproduces the reported layout: a "pool" with the real files and a + // library containing only symlinks into the pool. + base := GinkgoT().TempDir() + pool := filepath.Join(base, "pool") + libRoot = filepath.Join(base, "userlib") + Expect(os.MkdirAll(pool, 0755)).To(Succeed()) + Expect(os.MkdirAll(libRoot, 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(pool, "real.mp3"), []byte("AUDIO"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(pool, "secrets.txt"), []byte("TOPSECRET"), 0600)).To(Succeed()) + // mid.wav lives OUTSIDE the library and has an audio name, but points at a + // non-audio file. A chain through it must be classified by the FINAL target. + Expect(os.Symlink(filepath.Join(pool, "secrets.txt"), filepath.Join(pool, "mid.wav"))).To(Succeed()) + + Expect(os.Symlink("../pool/real.mp3", filepath.Join(libRoot, "relative.mp3"))).To(Succeed()) + Expect(os.Symlink(filepath.Join(pool, "real.mp3"), filepath.Join(libRoot, "absolute.mp3"))).To(Succeed()) + Expect(os.Symlink(filepath.Join(pool, "mid.wav"), filepath.Join(libRoot, "evil.wav"))).To(Succeed()) + Expect(os.Symlink(filepath.Join(pool, "missing.mp3"), filepath.Join(libRoot, "broken.mp3"))).To(Succeed()) + + u, err := storage.LocalPathToURL(libRoot) + Expect(err).ToNot(HaveOccurred()) + s, err := storage.For(u.String()) + Expect(err).ToNot(HaveOccurred()) + musicFS, err = s.FS() + Expect(err).ToNot(HaveOccurred()) + }) + + walkRoot := func() *folderEntry { + job := &scanJob{fs: musicFS, lib: model.Library{Path: libRoot}} + results, err := walkDirTree(GinkgoT().Context(), job) + Expect(err).ToNot(HaveOccurred()) + var root *folderEntry + for folder := range results { + if folder.path == "." { + root = folder + } + } + Expect(root).ToNot(BeNil()) + return root + } + + It("imports symlinks to out-of-library audio files", func() { + root := walkRoot() + Expect(root.audioFiles).To(HaveKey("relative.mp3")) + Expect(root.audioFiles).To(HaveKey("absolute.mp3")) + }) + + It("rejects a chain that ends in a non-audio file, even through an audio-named intermediate", func() { + root := walkRoot() + Expect(root.audioFiles).ToNot(HaveKey("evil.wav")) + }) + + It("skips broken symlinks", func() { + root := walkRoot() + Expect(root.audioFiles).ToNot(HaveKey("broken.mp3")) + }) + + It("skips all file symlinks when FollowSymlinks is disabled", func() { + conf.Server.Scanner.FollowSymlinks = false + root := walkRoot() + Expect(root.audioFiles).To(BeEmpty()) + }) + }) + Context("out-of-tree escape (temp dir)", func() { var root string BeforeEach(func() { diff --git a/scanner/watcher_test.go b/scanner/watcher_test.go index ffe9f8b15..15e49e195 100644 --- a/scanner/watcher_test.go +++ b/scanner/watcher_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -30,10 +31,14 @@ var _ = Describe("Watcher", func() { ctx, cancel = context.WithCancel(GinkgoT().Context()) DeferCleanup(cancel) + // Use a fake storage scheme: watchLibrary goroutines spawned by Run/Watch are not + // joined on spec teardown, and the real file:// storage reads conf.Server on + // construction, racing with the configtest cleanup that restores the config. + storagetest.Register("fake-watcher", &storagetest.FakeFS{}) lib = &model.Library{ ID: 1, Name: "Test Library", - Path: "/test/library", + Path: "fake-watcher:///test/library", } // Set up mocks @@ -234,7 +239,7 @@ var _ = Describe("Watcher", func() { lib2 = &model.Library{ ID: 2, Name: "Test Library 2", - Path: "/test/library2", + Path: "fake-watcher:///test/library2", } mockLibRepo := mockDS.MockedLibrary.(*tests.MockLibraryRepo)