diff --git a/server/auth.go b/server/auth.go index 37a318a83..341b9f80b 100644 --- a/server/auth.go +++ b/server/auth.go @@ -149,7 +149,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 e021c82a8..ca1ab7286 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" @@ -64,6 +65,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 57a712a20..8dc5653f2 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" @@ -203,22 +202,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)) + }) +})