From 7dce0da55756b8b10bcefef090cd0b46d0fe4875 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 6 Jun 2025 14:54:28 -0400 Subject: [PATCH] feat(api): restrict access to library and config endpoints to admin users Signed-off-by: Deluan --- server/nativeapi/config.go | 6 - server/nativeapi/config_test.go | 225 +++++++++++++++++++++----------- server/nativeapi/inspect.go | 6 - server/nativeapi/library.go | 15 --- server/nativeapi/native_api.go | 24 +++- 5 files changed, 172 insertions(+), 104 deletions(-) diff --git a/server/nativeapi/config.go b/server/nativeapi/config.go index d708d72f9..9a86a9add 100644 --- a/server/nativeapi/config.go +++ b/server/nativeapi/config.go @@ -9,7 +9,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model/request" ) // sensitiveFieldsPartialMask contains configuration field names that should be redacted @@ -99,11 +98,6 @@ func applySensitiveFieldMasking(ctx context.Context, config map[string]interface func getConfig(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - user, _ := request.UserFrom(ctx) - if !user.IsAdmin { - http.Error(w, "Config endpoint is only available to admin users", http.StatusUnauthorized) - return - } // Marshal the actual configuration struct to preserve original field names configBytes, err := json.Marshal(*conf.Server) diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 52baef83a..206c17327 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -1,109 +1,170 @@ package nativeapi import ( + "bytes" + "context" "encoding/json" "net/http" "net/http/httptest" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -var _ = Describe("getConfig", func() { +var _ = Describe("Config API", func() { + var ds model.DataStore + var router http.Handler + var adminUser, regularUser model.User + BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) + conf.Server.DevUIShowConfig = true // Enable config endpoint for tests + ds = &tests.MockDataStore{} + auth.Init(ds) + nativeRouter := New(ds, nil, nil, nil) + router = server.JWTVerifier(nativeRouter) + + // Create test users + adminUser = model.User{ + ID: "admin-1", + UserName: "admin", + Name: "Admin User", + IsAdmin: true, + NewPassword: "adminpass", + } + regularUser = model.User{ + ID: "user-1", + UserName: "regular", + Name: "Regular User", + IsAdmin: false, + NewPassword: "userpass", + } + + // Store in mock datastore + Expect(ds.User(context.TODO()).Put(&adminUser)).To(Succeed()) + Expect(ds.User(context.TODO()).Put(®ularUser)).To(Succeed()) }) - Context("when user is not admin", func() { - It("returns unauthorized", func() { - req := httptest.NewRequest("GET", "/config", nil) - w := httptest.NewRecorder() - ctx := request.WithUser(req.Context(), model.User{IsAdmin: false}) + Describe("GET /api/config", func() { + Context("as admin user", func() { + var adminToken string - getConfig(w, req.WithContext(ctx)) + BeforeEach(func() { + var err error + adminToken, err = auth.CreateToken(&adminUser) + Expect(err).ToNot(HaveOccurred()) + }) - Expect(w.Code).To(Equal(http.StatusUnauthorized)) - }) - }) + It("returns config successfully", func() { + req := createAuthenticatedConfigRequest(adminToken) + w := httptest.NewRecorder() - Context("when user is admin", func() { - It("returns config successfully", func() { - req := httptest.NewRequest("GET", "/config", nil) - w := httptest.NewRecorder() - ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) + router.ServeHTTP(w, req) - getConfig(w, req.WithContext(ctx)) + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + Expect(resp.ID).To(Equal("config")) + Expect(resp.ConfigFile).To(Equal(conf.Server.ConfigFile)) + Expect(resp.Config).ToNot(BeEmpty()) + }) - Expect(w.Code).To(Equal(http.StatusOK)) - var resp configResponse - Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) - Expect(resp.ID).To(Equal("config")) - Expect(resp.ConfigFile).To(Equal(conf.Server.ConfigFile)) - Expect(resp.Config).ToNot(BeEmpty()) + It("redacts sensitive fields", func() { + conf.Server.LastFM.ApiKey = "secretapikey123" + conf.Server.Spotify.Secret = "spotifysecret456" + conf.Server.PasswordEncryptionKey = "encryptionkey789" + conf.Server.DevAutoCreateAdminPassword = "adminpassword123" + conf.Server.Prometheus.Password = "prometheuspass" + + req := createAuthenticatedConfigRequest(adminToken) + w := httptest.NewRecorder() + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + + // Check LastFM.ApiKey (partially masked) + lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(lastfm["ApiKey"]).To(Equal("s*************3")) + + // Check Spotify.Secret (partially masked) + spotify, ok := resp.Config["Spotify"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(spotify["Secret"]).To(Equal("s**************6")) + + // Check PasswordEncryptionKey (fully masked) + Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("****")) + + // Check DevAutoCreateAdminPassword (fully masked) + Expect(resp.Config["DevAutoCreateAdminPassword"]).To(Equal("****")) + + // Check Prometheus.Password (fully masked) + prometheus, ok := resp.Config["Prometheus"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(prometheus["Password"]).To(Equal("****")) + }) + + It("handles empty sensitive values", func() { + conf.Server.LastFM.ApiKey = "" + conf.Server.PasswordEncryptionKey = "" + + req := createAuthenticatedConfigRequest(adminToken) + w := httptest.NewRecorder() + + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + var resp configResponse + Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + + // Check LastFM.ApiKey - should be preserved because it's sensitive + lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) + Expect(ok).To(BeTrue()) + Expect(lastfm["ApiKey"]).To(Equal("")) + + // Empty sensitive values should remain empty - should be preserved because it's sensitive + Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("")) + }) }) - It("redacts sensitive fields", func() { - conf.Server.LastFM.ApiKey = "secretapikey123" - conf.Server.Spotify.Secret = "spotifysecret456" - conf.Server.PasswordEncryptionKey = "encryptionkey789" - conf.Server.DevAutoCreateAdminPassword = "adminpassword123" - conf.Server.Prometheus.Password = "prometheuspass" + Context("as regular user", func() { + var userToken string - req := httptest.NewRequest("GET", "/config", nil) - w := httptest.NewRecorder() - ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) - getConfig(w, req.WithContext(ctx)) + BeforeEach(func() { + var err error + userToken, err = auth.CreateToken(®ularUser) + Expect(err).ToNot(HaveOccurred()) + }) - Expect(w.Code).To(Equal(http.StatusOK)) - var resp configResponse - Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) + It("denies access with forbidden status", func() { + req := createAuthenticatedConfigRequest(userToken) + w := httptest.NewRecorder() - // Check LastFM.ApiKey (partially masked) - lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) - Expect(ok).To(BeTrue()) - Expect(lastfm["ApiKey"]).To(Equal("s*************3")) + router.ServeHTTP(w, req) - // Check Spotify.Secret (partially masked) - spotify, ok := resp.Config["Spotify"].(map[string]interface{}) - Expect(ok).To(BeTrue()) - Expect(spotify["Secret"]).To(Equal("s**************6")) - - // Check PasswordEncryptionKey (fully masked) - Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("****")) - - // Check DevAutoCreateAdminPassword (fully masked) - Expect(resp.Config["DevAutoCreateAdminPassword"]).To(Equal("****")) - - // Check Prometheus.Password (fully masked) - prometheus, ok := resp.Config["Prometheus"].(map[string]interface{}) - Expect(ok).To(BeTrue()) - Expect(prometheus["Password"]).To(Equal("****")) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) }) - It("handles empty sensitive values", func() { - conf.Server.LastFM.ApiKey = "" - conf.Server.PasswordEncryptionKey = "" + Context("without authentication", func() { + It("denies access with unauthorized status", func() { + req := createUnauthenticatedConfigRequest("GET", "/config/", nil) + w := httptest.NewRecorder() - req := httptest.NewRequest("GET", "/config", nil) - w := httptest.NewRecorder() - ctx := request.WithUser(req.Context(), model.User{IsAdmin: true}) - getConfig(w, req.WithContext(ctx)) + router.ServeHTTP(w, req) - Expect(w.Code).To(Equal(http.StatusOK)) - var resp configResponse - Expect(json.Unmarshal(w.Body.Bytes(), &resp)).To(Succeed()) - - // Check LastFM.ApiKey - should be preserved because it's sensitive - lastfm, ok := resp.Config["LastFM"].(map[string]interface{}) - Expect(ok).To(BeTrue()) - Expect(lastfm["ApiKey"]).To(Equal("")) - - // Empty sensitive values should remain empty - should be preserved because it's sensitive - Expect(resp.Config["PasswordEncryptionKey"]).To(Equal("")) + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) }) }) }) @@ -145,3 +206,21 @@ var _ = Describe("redactValue function", func() { Expect(redactValue("LastFM.ApiKey", "abcdefg")).To(Equal("a*****g")) }) }) + +// Helper functions + +func createAuthenticatedConfigRequest(token string) *http.Request { + req := httptest.NewRequest(http.MethodGet, "/config/config", nil) + req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + return req +} + +func createUnauthenticatedConfigRequest(method, path string, body *bytes.Buffer) *http.Request { + if body == nil { + body = &bytes.Buffer{} + } + req := httptest.NewRequest(method, path, body) + req.Header.Set("Content-Type", "application/json") + return req +} diff --git a/server/nativeapi/inspect.go b/server/nativeapi/inspect.go index e74dc99c0..3178395ce 100644 --- a/server/nativeapi/inspect.go +++ b/server/nativeapi/inspect.go @@ -9,7 +9,6 @@ import ( "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/req" ) @@ -30,11 +29,6 @@ func inspect(ds model.DataStore) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - user, _ := request.UserFrom(ctx) - if !user.IsAdmin { - http.Error(w, "Inspect is only available to admin users", http.StatusUnauthorized) - } - p := req.Params(r) id, err := p.String("id") diff --git a/server/nativeapi/library.go b/server/nativeapi/library.go index d9d5fe890..071ddddfa 100644 --- a/server/nativeapi/library.go +++ b/server/nativeapi/library.go @@ -11,13 +11,11 @@ import ( "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" ) // Library management endpoints (admin only) func (n *Router) addLibraryRoute(r chi.Router) { r.Route("/library", func(r chi.Router) { - r.Use(adminOnlyMiddleware) r.Get("/", getLibraries(n.ds)) r.Post("/", createLibrary(n.ds)) r.Route("/{id}", func(r chi.Router) { @@ -32,25 +30,12 @@ func (n *Router) addLibraryRoute(r chi.Router) { // User-library association endpoints (admin only) func (n *Router) addUserLibraryRoute(r chi.Router) { r.Route("/user/{id}/library", func(r chi.Router) { - r.Use(adminOnlyMiddleware) r.Use(parseUserIDMiddleware) r.Get("/", getUserLibraries(n.ds)) r.Put("/", setUserLibraries(n.ds)) }) } -// Middleware to ensure only admin users can access endpoints -func adminOnlyMiddleware(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - user, ok := request.UserFrom(r.Context()) - if !ok || !user.IsAdmin { - http.Error(w, "Access denied: admin privileges required", http.StatusForbidden) - return - } - next.ServeHTTP(w, r) - }) -} - // Middleware to parse library ID from URL func parseIDMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 6b7428deb..be5e38030 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server" ) @@ -62,12 +63,15 @@ func (n *Router) routes() http.Handler { n.addSongPlaylistsRoute(r) n.addQueueRoute(r) n.addMissingFilesRoute(r) - n.addInspectRoute(r) - n.addConfigRoute(r) n.addKeepAliveRoute(r) n.addInsightsRoute(r) - n.addLibraryRoute(r) - n.addUserLibraryRoute(r) + + r.With(adminOnlyMiddleware).Group(func(r chi.Router) { + n.addInspectRoute(r) + n.addConfigRoute(r) + n.addLibraryRoute(r) + n.addUserLibraryRoute(r) + }) }) return r @@ -229,3 +233,15 @@ func (n *Router) addInsightsRoute(r chi.Router) { } }) } + +// Middleware to ensure only admin users can access endpoints +func adminOnlyMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + user, ok := request.UserFrom(r.Context()) + if !ok || !user.IsAdmin { + http.Error(w, "Access denied: admin privileges required", http.StatusForbidden) + return + } + next.ServeHTTP(w, r) + }) +}