diff --git a/server/auth.go b/server/auth.go index cc82845c8..2aaa93e63 100644 --- a/server/auth.go +++ b/server/auth.go @@ -159,7 +159,6 @@ func createAdminUser(ctx context.Context, ds model.DataStore, username, password } err := ds.User(ctx).Put(&initialUser) if err != nil { - // Log the username only: initialUser carries the password in clear text log.Error(ctx, "Could not create initial user", "user", initialUser.UserName, err) return fmt.Errorf("creating initial user: %w", err) } diff --git a/server/auth_test.go b/server/auth_test.go index 9fdd84d06..abe144a12 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -76,8 +76,6 @@ 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")) diff --git a/server/initial_setup.go b/server/initial_setup.go index f033d4a51..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,14 +60,8 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { NewPassword: initialPassword, IsAdmin: true, } - // 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 fmt.Errorf("could not create initial admin user: %w", err) } } return nil diff --git a/server/initial_setup_test.go b/server/initial_setup_test.go index b6d97d178..0ce8a39fa 100644 --- a/server/initial_setup_test.go +++ b/server/initial_setup_test.go @@ -10,8 +10,6 @@ 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 @@ -47,11 +45,15 @@ var _ = Describe("initial_setup", func() { 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)) }) + + 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 index 9517d3b47..7d911d6cd 100644 --- a/server/nativeapi/delete_many_response_test.go +++ b/server/nativeapi/delete_many_response_test.go @@ -29,8 +29,6 @@ var _ = Describe("writeDeleteManyResponse", 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\`)) }) @@ -43,8 +41,9 @@ var _ = Describe("writeDeleteManyResponse", func() { Expect(write("a&b")).To(HaveKeyWithValue("id", "a&b")) }) - It("responds 200 on success", func() { + 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 3476ec5a0..a7c53df09 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -2,7 +2,6 @@ package nativeapi import ( "context" - "encoding/json" "net/http" "strconv" "time" @@ -205,9 +204,6 @@ func (api *Router) addMissingFilesRoute(r chi.Router) { } func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []string) { - // 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 { payload = struct { @@ -218,13 +214,7 @@ func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []strin Ids []string `json:"ids"` }{Ids: ids} } - 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 { + 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 b69549e7c..367bff501 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -58,7 +58,6 @@ func (pub *Router) handleM3U(w http.ResponseWriter, r *http.Request) { } s = pub.mapShareToM3U(r, *s) - // 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 index b299f4cfd..1bf631fd4 100644 --- a/server/public/handle_shares_test.go +++ b/server/public/handle_shares_test.go @@ -30,7 +30,6 @@ var _ = Describe("handleM3U", func() { 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 @@ -39,9 +38,7 @@ var _ = Describe("handleM3U", func() { 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. + // 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")) })