cmd/minstrel/main.go

@@ -385,6 +385,11 @@ func run() error {
 		Addr:              cfg.Server.Address,
 		Handler:           srv.Router(),
 		ReadHeaderTimeout: 10 * time.Second,
+		// Closes keep-alive connections nobody is using. Deliberately no
+		// ReadTimeout or WriteTimeout: either would cut off audio streams and
+		// the SSE event stream. Request bodies get their own deadline in the
+		// server's limitRequestBody middleware instead.
+		IdleTimeout: 120 * time.Second,
 	}
 
 	errCh := make(chan error, 1)

internal/api/media.go

@@ -117,7 +117,7 @@ func (h *handlers) handleGetCover(w http.ResponseWriter, r *http.Request) {
 	// clients skip the conditional GET for the bulk of a session, but
 	// stale art clears within 24h after a re-scan. ServeContent below
 	// still emits Last-Modified for the conditional path when needed.
-	w.Header().Set("Cache-Control", "public, max-age=86400, must-revalidate")
+	w.Header().Set("Cache-Control", "private, max-age=86400, must-revalidate")
 	http.ServeContent(w, r, filepath.Base(path), info.ModTime(), f)
 }
 
@@ -197,6 +197,6 @@ func (h *handlers) handleGetStream(w http.ResponseWriter, r *http.Request) {
 	// max-age + immutable lets the client cache (LockCachingAudioSource
 	// on the Flutter side, browser cache on web) skip even the
 	// conditional GET on repeat plays.
-	w.Header().Set("Cache-Control", "public, max-age=31536000, immutable")
+	w.Header().Set("Cache-Control", "private, max-age=31536000, immutable")
 	http.ServeContent(w, r, filepath.Base(track.FilePath), info.ModTime(), f)
 }

internal/api/playlists.go

@@ -406,7 +406,7 @@ func (h *handlers) handleGetPlaylistCover(w http.ResponseWriter, r *http.Request
 	// (system playlists re-rendered on rebuild, user playlists when
 	// modified). 5 minutes is short enough for normal edits to feel
 	// fresh, long enough to skip repeat fetches during a session.
-	w.Header().Set("Cache-Control", "public, max-age=300, must-revalidate")
+	w.Header().Set("Cache-Control", "private, max-age=300, must-revalidate")
 	http.ServeFile(w, r, full)
 }
 

internal/server/hygiene.go

@@ -0,0 +1,115 @@
+package server
+
+import (
+	"io"
+	"mime"
+	"net/http"
+	"strings"
+	"time"
+
+	"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
+)
+
+// maxRequestBody caps every request body. The largest legitimate one is a
+// playlist save of a few thousand track ids, well under 1 MiB; 4 MiB leaves
+// room without letting one request hold arbitrary memory.
+const maxRequestBody = 4 << 20
+
+// bodyReadTimeout bounds how long a client may take to send a request body.
+const bodyReadTimeout = 30 * time.Second
+
+// limitRequestBody caps the body size and the time allowed to deliver it.
+//
+// Why not http.Server.ReadTimeout: that deadline stays armed for the whole
+// request, and once a handler has consumed the body, net/http's background
+// read hits it and cancels the request context. That would cut off audio
+// streams and the SSE event stream at the timeout. Here the deadline is set
+// only on requests that carry a body, and cleared the moment the body has
+// been read, so long-running responses are never affected.
+func limitRequestBody(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Body == nil || r.Body == http.NoBody {
+			next.ServeHTTP(w, r)
+			return
+		}
+		rc := http.NewResponseController(w)
+		// Best-effort: a ResponseWriter that can't set deadlines (a test
+		// recorder) still gets the size limit.
+		armed := rc.SetReadDeadline(time.Now().Add(bodyReadTimeout)) == nil
+		body := http.MaxBytesReader(w, r.Body, maxRequestBody)
+		if armed {
+			body = &deadlineClearingBody{ReadCloser: body, rc: rc}
+		}
+		r.Body = body
+		next.ServeHTTP(w, r)
+	})
+}
+
+// deadlineClearingBody lifts the read deadline once the body is exhausted.
+// A deadline change applies to pending reads too, so this also releases the
+// background read net/http starts at end-of-body.
+type deadlineClearingBody struct {
+	io.ReadCloser
+	rc      *http.ResponseController
+	cleared bool
+}
+
+func (b *deadlineClearingBody) Read(p []byte) (int, error) {
+	n, err := b.ReadCloser.Read(p)
+	if err != nil && !b.cleared {
+		b.cleared = true
+		_ = b.rc.SetReadDeadline(time.Time{})
+	}
+	return n, err
+}
+
+func (b *deadlineClearingBody) Close() error {
+	if !b.cleared {
+		b.cleared = true
+		_ = b.rc.SetReadDeadline(time.Time{})
+	}
+	return b.ReadCloser.Close()
+}
+
+// requireJSONForCookieWrites refuses a state-changing /api request that is
+// authenticated by the session cookie unless its body is JSON.
+//
+// SameSite=Strict already keeps the cookie off cross-site requests. This is
+// the backstop for the case SameSite cannot see: a sibling app on the same
+// registrable domain (another *.fabledsword.com service) counts as same-site.
+// An HTML form or a no-preflight fetch can only send form-encoded, multipart
+// or text/plain bodies, so demanding application/json closes that path.
+// Bearer-token requests and /rest (query-string auth) carry nothing a browser
+// attaches on its own, so they are not checked. The Android app does send
+// the cookie, but every body it sends is JSON (Retrofit's kotlinx converter)
+// and its bodiless writes carry no Content-Type, so it passes unchanged.
+func requireJSONForCookieWrites(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.Method {
+		case http.MethodGet, http.MethodHead, http.MethodOptions:
+			next.ServeHTTP(w, r)
+			return
+		}
+		if !strings.HasPrefix(r.URL.Path, "/api/") {
+			next.ServeHTTP(w, r)
+			return
+		}
+		if c, err := r.Cookie(auth.SessionCookieName); err != nil || c.Value == "" {
+			next.ServeHTTP(w, r)
+			return
+		}
+		ct := r.Header.Get("Content-Type")
+		if ct == "" && (r.ContentLength == 0 || r.Body == nil || r.Body == http.NoBody) {
+			// No body, no content type: nothing a form could have sent.
+			next.ServeHTTP(w, r)
+			return
+		}
+		if mt, _, err := mime.ParseMediaType(ct); err != nil || mt != "application/json" {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusUnsupportedMediaType)
+			_, _ = io.WriteString(w, `{"error":{"code":"unsupported_media_type","message":"request body must be application/json"}}`)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}

internal/server/hygiene_test.go

@@ -0,0 +1,90 @@
+package server
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
+)
+
+func okHandler() http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Body != nil {
+			if _, err := io.ReadAll(r.Body); err != nil {
+				http.Error(w, "too large", http.StatusRequestEntityTooLarge)
+				return
+			}
+		}
+		w.WriteHeader(http.StatusNoContent)
+	})
+}
+
+func TestLimitRequestBody_RejectsOversizedBody(t *testing.T) {
+	h := limitRequestBody(okHandler())
+
+	big := strings.NewReader(strings.Repeat("x", maxRequestBody+1))
+	w := httptest.NewRecorder()
+	h.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/api/playlists", big))
+	if w.Code != http.StatusRequestEntityTooLarge {
+		t.Errorf("oversized body: status = %d, want the handler's read to fail", w.Code)
+	}
+
+	small := strings.NewReader(`{"name":"ok"}`)
+	w = httptest.NewRecorder()
+	h.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/api/playlists", small))
+	if w.Code != http.StatusNoContent {
+		t.Errorf("small body: status = %d, want 204", w.Code)
+	}
+}
+
+// The cases a cross-site form or no-preflight fetch could produce must be
+// refused when the session cookie rides along; everything a real client
+// sends must pass.
+func TestRequireJSONForCookieWrites(t *testing.T) {
+	cookie := &http.Cookie{Name: auth.SessionCookieName, Value: "tok"}
+	tests := []struct {
+		name        string
+		method      string
+		path        string
+		contentType string
+		body        string
+		withCookie  bool
+		want        int
+	}{
+		{"form post with cookie", http.MethodPost, "/api/me/password", "application/x-www-form-urlencoded", "a=b", true, http.StatusUnsupportedMediaType},
+		{"text/plain post with cookie", http.MethodPost, "/api/me/password", "text/plain", `{"a":1}`, true, http.StatusUnsupportedMediaType},
+		{"multipart with cookie", http.MethodPut, "/api/me/profile", "multipart/form-data; boundary=x", "--x--", true, http.StatusUnsupportedMediaType},
+		{"body without content type", http.MethodPost, "/api/me/profile", "", `{"a":1}`, true, http.StatusUnsupportedMediaType},
+
+		{"json with cookie", http.MethodPost, "/api/me/password", "application/json", `{}`, true, http.StatusNoContent},
+		{"json with charset", http.MethodPost, "/api/me/password", "application/json; charset=utf-8", `{}`, true, http.StatusNoContent},
+		{"bodiless delete with cookie", http.MethodDelete, "/api/me/sessions/1", "", "", true, http.StatusNoContent},
+		{"form post without cookie (bearer client)", http.MethodPost, "/api/me/password", "text/plain", "x", false, http.StatusNoContent},
+		{"subsonic post is not /api", http.MethodPost, "/rest/scrobble", "application/x-www-form-urlencoded", "id=1", true, http.StatusNoContent},
+		{"GET is never checked", http.MethodGet, "/api/me", "text/plain", "", true, http.StatusNoContent},
+	}
+	h := requireJSONForCookieWrites(okHandler())
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var body io.Reader
+			if tc.body != "" {
+				body = strings.NewReader(tc.body)
+			}
+			req := httptest.NewRequest(tc.method, tc.path, body)
+			if tc.contentType != "" {
+				req.Header.Set("Content-Type", tc.contentType)
+			}
+			if tc.withCookie {
+				req.AddCookie(cookie)
+			}
+			w := httptest.NewRecorder()
+			h.ServeHTTP(w, req)
+			if w.Code != tc.want {
+				t.Errorf("status = %d, want %d", w.Code, tc.want)
+			}
+		})
+	}
+}

internal/server/server.go

@@ -139,6 +139,8 @@ func (s *Server) Router() http.Handler {
 	r.Use(middleware.RequestID)
 	r.Use(requestLog(s.Logger, netSettings.Hops))
 	r.Use(middleware.Recoverer)
+	r.Use(limitRequestBody)
+	r.Use(requireJSONForCookieWrites)
 
 	r.Get("/healthz", s.handleHealthz)