From 27483a46dcda604b5e494b3b2010b617878e0593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20S=C3=A1nchez=20Zapico?= Date: Wed, 23 Sep 2026 18:10:25 +0200 Subject: [PATCH] fix(server): fail startup on initial setup errors and fix JSON/M3U response headers (#5897) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server): stop swallowing errors and correct two response bugs Four independent bugs found while reviewing the HTTP layer: initial_setup.go: createInitialAdminUser assigned the users.Put error to a shadowed err, so the outer err (always nil by then, since a CountAll failure panics) was returned instead. A failure to create the admin user was reported as success, and initialSetup went on to commit the "setup complete" property in the same transaction — so no admin user existed and initial setup was skipped on every later boot. auth.go: createAdminUser logged the Put error but returned nil, so createAdmin fell through to doLogin and answered 401 "Invalid username or password" instead of surfacing the real failure. It also logged the whole model.User, which puts the new admin's password in the log in clear text; every other call site logs user.UserName. native_api.go: writeDeleteManyResponse did not return after http.Error when marshaling failed, then wrote a nil body over the 500. It also built the single-id body by hand with html.EscapeString, which does not escape backslashes, so an id ending in one produced `{"id":"a\"}` — invalid JSON. Both shapes now go through json.Marshal. A failed Write is now logged rather than answered with http.Error, which could not work once the body had started. handle_shares.go: handleM3U set Content-Type after WriteHeader, so it was never sent and shared playlists were served with a sniffed type. Signed-off-by: zapisanchez * fix(server): address review feedback - writeDeleteManyResponse uses rest.RespondWithJSON, so the response now has Content-Type: application/json. This also removes a marshal error branch that could never run. - createInitialAdminUser returns the CountAll error instead of panicking, and wraps its errors. initialSetup now stops the server with log.Fatal when setup fails. Before, the error was dropped and the server started with a half-done setup. - Trim comments that described PR history. --------- Signed-off-by: zapisanchez Co-authored-by: Deluan --- server/auth.go | 2 +- server/auth_test.go | 12 +++++ server/initial_setup.go | 14 ++--- server/initial_setup_test.go | 23 ++++++++ server/nativeapi/delete_many_response_test.go | 49 +++++++++++++++++ server/nativeapi/native_api.go | 22 +++----- server/public/handle_shares.go | 2 +- server/public/handle_shares_test.go | 52 +++++++++++++++++++ 8 files changed, 154 insertions(+), 22 deletions(-) create mode 100644 server/nativeapi/delete_many_response_test.go create mode 100644 server/public/handle_shares_test.go diff --git a/server/auth.go b/server/auth.go index d9ade7b29..2aaa93e63 100644 --- a/server/auth.go +++ b/server/auth.go @@ -159,7 +159,7 @@ func createAdminUser(ctx context.Context, ds model.DataStore, username, password } err := ds.User(ctx).Put(&initialUser) if err != nil { - log.Error(ctx, "Could not create initial user", "user", initialUser, err) + log.Error(ctx, "Could not create initial user", "user", initialUser.UserName, err) return fmt.Errorf("creating initial user: %w", err) } return nil diff --git a/server/auth_test.go b/server/auth_test.go index a4d592c51..abe144a12 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -76,6 +76,18 @@ var _ = Describe("Auth", func() { }) }) + Describe("createAdmin when the user cannot be stored", func() { + It("responds 500 rather than falling through to login", func() { + failing := dsWithFailingPut(errors.New("db is down")) + req = httptest.NewRequest("POST", "/createAdmin", strings.NewReader(`{"username":"johndoe", "password":"secret"}`)) + resp = httptest.NewRecorder() + + createAdmin(failing)(resp, req) + + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + Describe("Login from HTTP headers", func() { const ( trustedIpv4 = "192.168.0.42" diff --git a/server/initial_setup.go b/server/initial_setup.go index 7e974dc21..e75220abe 100644 --- a/server/initial_setup.go +++ b/server/initial_setup.go @@ -16,7 +16,7 @@ import ( func initialSetup(ds model.DataStore) { ctx := context.TODO() - _ = ds.WithTx(func(tx model.DataStore) error { + err := ds.WithTx(func(tx model.DataStore) error { if err := tx.Library(ctx).StoreMusicFolder(); err != nil { return err } @@ -36,6 +36,9 @@ func initialSetup(ds model.DataStore) { err = properties.Put(consts.InitialSetupFlagKey, time.Now().String()) return err }, "initial setup") + if err != nil { + log.Fatal("Error running initial setup", err) + } } // If the Dev Admin user is not present, create it @@ -43,7 +46,7 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { users := ds.User(context.TODO()) c, err := users.CountAll(model.QueryOptions{Filters: squirrel.Eq{"user_name": consts.DevInitialUserName}}) if err != nil { - panic(fmt.Sprintf("Could not access User table: %s", err)) + return fmt.Errorf("could not access User table: %w", err) } if c == 0 { newID := id.NewRandom() @@ -57,12 +60,11 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { NewPassword: initialPassword, IsAdmin: true, } - err := users.Put(&initialUser) - if err != nil { - log.Error("Could not create initial admin user", "user", initialUser, err) + if err := users.Put(&initialUser); err != nil { + return fmt.Errorf("could not create initial admin user: %w", err) } } - return err + return nil } func checkFFmpegInstallation() { diff --git a/server/initial_setup_test.go b/server/initial_setup_test.go index 982046f78..0ce8a39fa 100644 --- a/server/initial_setup_test.go +++ b/server/initial_setup_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "errors" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" @@ -9,6 +10,17 @@ import ( . "github.com/onsi/gomega" ) +type failingPutUserRepo struct { + model.UserRepository + err error +} + +func (r *failingPutUserRepo) Put(*model.User) error { return r.err } + +func dsWithFailingPut(err error) model.DataStore { + return &tests.MockDataStore{MockedUser: &failingPutUserRepo{UserRepository: tests.CreateMockUserRepo(), err: err}} +} + var _ = Describe("initial_setup", func() { var ds model.DataStore @@ -32,5 +44,16 @@ var _ = Describe("initial_setup", func() { Expect(createInitialAdminUser(ds, "second")).To(BeNil()) Expect(ur.CountAll()).To(Equal(int64(1))) }) + + It("returns the error when the user cannot be stored", func() { + boom := errors.New("db is down") + Expect(createInitialAdminUser(dsWithFailingPut(boom), "pass123")).To(MatchError(boom)) + }) + + It("returns the error when the user table cannot be read", func() { + boom := errors.New("db is down") + ds = &tests.MockDataStore{MockedUser: &tests.MockedUserRepo{Error: boom}} + Expect(createInitialAdminUser(ds, "pass123")).To(MatchError(boom)) + }) }) }) diff --git a/server/nativeapi/delete_many_response_test.go b/server/nativeapi/delete_many_response_test.go new file mode 100644 index 000000000..7d911d6cd --- /dev/null +++ b/server/nativeapi/delete_many_response_test.go @@ -0,0 +1,49 @@ +package nativeapi + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("writeDeleteManyResponse", func() { + var w *httptest.ResponseRecorder + + write := func(ids ...string) map[string]any { + w = httptest.NewRecorder() + writeDeleteManyResponse(w, httptest.NewRequest("DELETE", "/missing", nil), ids) + + var body map[string]any + Expect(json.Unmarshal(w.Body.Bytes(), &body)).To(Succeed(), "response body must be valid JSON: %s", w.Body.String()) + return body + } + + It("returns a single id as an object", func() { + Expect(write("abc123")).To(HaveKeyWithValue("id", "abc123")) + }) + + It("returns multiple ids as a list", func() { + Expect(write("a", "b")).To(HaveKeyWithValue("ids", ConsistOf("a", "b"))) + }) + + It("stays valid JSON when the id contains a backslash", func() { + Expect(write(`a\`)).To(HaveKeyWithValue("id", `a\`)) + }) + + It("stays valid JSON when the id contains a quote", func() { + Expect(write(`a"b`)).To(HaveKeyWithValue("id", `a"b`)) + }) + + It("does not HTML-escape the id into entities", func() { + Expect(write("a&b")).To(HaveKeyWithValue("id", "a&b")) + }) + + It("responds 200 with a JSON content type", func() { + write("abc123") + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + }) +}) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index f931834c6..a7c53df09 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -2,8 +2,6 @@ package nativeapi import ( "context" - "encoding/json" - "html" "net/http" "strconv" "time" @@ -206,22 +204,18 @@ func (api *Router) addMissingFilesRoute(r chi.Router) { } func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []string) { - var resp []byte - var err error + var payload any if len(ids) == 1 { - resp = []byte(`{"id":"` + html.EscapeString(ids[0]) + `"}`) + payload = struct { + ID string `json:"id"` + }{ID: ids[0]} } else { - resp, err = json.Marshal(&struct { + payload = struct { Ids []string `json:"ids"` - }{Ids: ids}) - if err != nil { - log.Error(r.Context(), "Error marshaling response", "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) - } + }{Ids: ids} } - _, err = w.Write(resp) //nolint:gosec - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + if err := rest.RespondWithJSON(w, http.StatusOK, payload); err != nil { + log.Error(r.Context(), "Error writing response", "ids", ids, err) } } diff --git a/server/public/handle_shares.go b/server/public/handle_shares.go index d67cfe456..367bff501 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -58,8 +58,8 @@ func (pub *Router) handleM3U(w http.ResponseWriter, r *http.Request) { } s = pub.mapShareToM3U(r, *s) - w.WriteHeader(http.StatusOK) w.Header().Set("Content-Type", "audio/x-mpegurl") + w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(s.ToM3U8())) //nolint:gosec } diff --git a/server/public/handle_shares_test.go b/server/public/handle_shares_test.go new file mode 100644 index 000000000..1bf631fd4 --- /dev/null +++ b/server/public/handle_shares_test.go @@ -0,0 +1,52 @@ +package public + +import ( + "net/http" + "net/http/httptest" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("handleM3U", func() { + var ds *tests.MockDataStore + var shareRepo *tests.MockShareRepo + var pub *Router + + BeforeEach(func() { + ds = &tests.MockDataStore{} + shareRepo = &tests.MockShareRepo{} + ds.MockedShare = shareRepo + pub = &Router{ds: ds, share: core.NewShare(ds)} + }) + + makeRequest := func(id string) *httptest.ResponseRecorder { + r := httptest.NewRequest("GET", "/public/"+id+"/m3u?%3Aid="+id, nil) + w := httptest.NewRecorder() + pub.handleM3U(w, r) + return w + } + + It("sets the M3U content type", func() { + share := &model.Share{ID: "abc123", Tracks: model.MediaFiles{{ID: "t1", Title: "Track 1"}}} + shareRepo.ID = share.ID + shareRepo.Entity = share + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusOK)) + // Result() has the headers sent at WriteHeader time, unlike w.Header() + Expect(w.Result().Header.Get("Content-Type")).To(Equal("audio/x-mpegurl")) + Expect(w.Body.String()).To(HavePrefix("#EXTM3U")) + }) + + It("returns 404 when the share does not exist", func() { + shareRepo.ID = "other" + shareRepo.Entity = &model.Share{ID: "other"} + + Expect(makeRequest("missing").Code).To(Equal(http.StatusNotFound)) + }) +})