diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index d447a7bf..a616e673 100644 --- a/cmd/minstrel/main.go +++ b/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) diff --git a/internal/api/media.go b/internal/api/media.go index b2be8ed1..a4009a44 100644 --- a/internal/api/media.go +++ b/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) } diff --git a/internal/api/playlists.go b/internal/api/playlists.go index af262bd7..eab70042 100644 --- a/internal/api/playlists.go +++ b/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) } diff --git a/internal/server/hygiene.go b/internal/server/hygiene.go new file mode 100644 index 00000000..77901ae2 --- /dev/null +++ b/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) + }) +} diff --git a/internal/server/hygiene_test.go b/internal/server/hygiene_test.go new file mode 100644 index 00000000..3e432b96 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index b0398379..510278ed 100644 --- a/internal/server/server.go +++ b/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)