From 2d300cebed161098165fef87fd9bfec6b37f9a85 Mon Sep 17 00:00:00 2001 From: zapisanchez Date: Wed, 5 Aug 2026 10:12:45 +0200 Subject: [PATCH] fix(server): stop swallowing errors and correct two response bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server/auth.go | 4 +- server/auth_test.go | 15 +++++ server/initial_setup.go | 13 +++-- server/initial_setup_test.go | 21 +++++++ server/nativeapi/delete_many_response_test.go | 50 +++++++++++++++++ server/nativeapi/native_api.go | 26 +++++---- server/public/handle_shares.go | 3 +- server/public/handle_shares_test.go | 55 +++++++++++++++++++ 8 files changed, 170 insertions(+), 17 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 6a25f1406..2371301de 100644 --- a/server/auth.go +++ b/server/auth.go @@ -147,7 +147,9 @@ 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 the username only: initialUser carries the password in clear text + log.Error(ctx, "Could not create initial user", "user", initialUser.UserName, err) + return err } return nil } diff --git a/server/auth_test.go b/server/auth_test.go index f6af6f0d6..e78e9e0b5 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/md5" "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" @@ -63,6 +64,20 @@ var _ = Describe("Auth", func() { }) }) + // createAdminUser used to swallow the Put error and return nil, so a failure + // fell through to doLogin and surfaced as a misleading 401. + 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..f033d4a51 100644 --- a/server/initial_setup.go +++ b/server/initial_setup.go @@ -57,12 +57,17 @@ 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) + // The Put error used to be assigned to a shadowed err and dropped, so a + // failure was reported as success and initialSetup went on to commit the + // "setup complete" flag — leaving no admin user, and skipping setup on + // every later boot. + if err := users.Put(&initialUser); err != nil { + // Log the username rather than the whole struct, as everywhere else + log.Error("Could not create initial admin user", "user", consts.DevInitialUserName, err) + return err } } - return err + return nil } func checkFFmpegInstallation() { diff --git a/server/initial_setup_test.go b/server/initial_setup_test.go index 982046f78..b6d97d178 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,19 @@ import ( . "github.com/onsi/gomega" ) +// failingPutUserRepo counts users normally but fails to store them, to exercise the +// error path of the initial user creation. +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 +46,12 @@ var _ = Describe("initial_setup", func() { Expect(createInitialAdminUser(ds, "second")).To(BeNil()) Expect(ur.CountAll()).To(Equal(int64(1))) }) + + // The error was assigned to a shadowed err and dropped, so the caller saw + // success and went on to commit the "setup complete" flag. + 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)) + }) }) }) diff --git a/server/nativeapi/delete_many_response_test.go b/server/nativeapi/delete_many_response_test.go new file mode 100644 index 000000000..9517d3b47 --- /dev/null +++ b/server/nativeapi/delete_many_response_test.go @@ -0,0 +1,50 @@ +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"))) + }) + + // ids come straight from the query string. html.EscapeString, used previously to + // build the single-id body, does not escape backslashes, so `{"id":"a\"}` went out. + 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 on success", func() { + write("abc123") + Expect(w.Code).To(Equal(http.StatusOK)) + }) +}) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 5a7023eb6..36c252a70 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -3,7 +3,6 @@ package nativeapi import ( "context" "encoding/json" - "html" "net/http" "strconv" "time" @@ -199,22 +198,27 @@ func (api *Router) addMissingFilesRoute(r chi.Router) { } func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []string) { - var resp []byte - var err error + // Marshal both shapes instead of building the single-id case by hand: ids come + // from the query string, and html.EscapeString leaves backslashes untouched, so + // an id containing one produced a malformed body. + 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 + resp, err := json.Marshal(payload) if err != nil { + log.Error(r.Context(), "Error marshaling response", "ids", ids, err) http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + if _, err := w.Write(resp); 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 13a7e4c32..f70ff9b74 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -58,8 +58,9 @@ func (pub *Router) handleM3U(w http.ResponseWriter, r *http.Request) { } s = pub.mapShareToM3U(r, *s) - w.WriteHeader(http.StatusOK) + // Content-Type must be set before WriteHeader, otherwise it is dropped 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..b299f4cfd --- /dev/null +++ b/server/public/handle_shares_test.go @@ -0,0 +1,55 @@ +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 + } + + // Content-Type used to be set after WriteHeader, which silently drops it. + 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() reports the headers as they were when WriteHeader ran, which is + // what the client actually receives. w.Header() would still show a + // Content-Type set too late to be sent. + 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)) + }) +})