Merge 5f766827f940c4c771cfa22ca610a96ce1cb9330 into dbd26ba2e71d0a5b79dba873a2beeff59f1cd8dd

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
 }

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"

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() {

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))
+		})
 	})
 })

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))
+	})
+})

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)
 	}
 }
 

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
 }
 

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))
+	})
+})