From 42fd263cdffdbb861b734e11d11ad3fd7de72845 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 23 Jul 2026 11:25:10 -0400 Subject: [PATCH] fix(artwork): clamp negative sizes to full-size; convert imghttp test to Ginkgo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A negative size (Subsonic size / Jellyfin maxwidth accept signed ints) reached resizeStaticImage, where the square path builds image.NewNRGBA(Rect(0,0,size,size)) — a giant rectangle that panics/OOMs. Clamp size<0 to 0 (full-size) at the Service entry. Positive sizes were already clamped to the original. - imghttp used a plain func Test with a table; convert to a Ginkgo DescribeTable with the suite entry point in imghttp_suite_test.go (AGENTS.md test-framework requirement). --- core/artwork/serving.go | 3 + core/artwork/serving_test.go | 8 ++ server/imghttp/headers_test.go | 196 +++++++-------------------- server/imghttp/imghttp_suite_test.go | 17 +++ 4 files changed, 76 insertions(+), 148 deletions(-) create mode 100644 server/imghttp/imghttp_suite_test.go diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 4a1b9737a..f4e67cd19 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -76,6 +76,9 @@ func (s *service) Get(ctx context.Context, artID model.ArtworkID, size int, squa if artID.ID == "" { return nil, ErrUnavailable } + if size < 0 { + size = 0 // a negative size is a full-size request, not a giant (OOM) resize rectangle + } switch artID.Kind { case model.KindDiscArtwork: return s.serveDisc(ctx, artID, size, square) diff --git a/core/artwork/serving_test.go b/core/artwork/serving_test.go index 9dc4db90d..0f344e4a0 100644 --- a/core/artwork/serving_test.go +++ b/core/artwork/serving_test.go @@ -126,6 +126,14 @@ var _ = Describe("Service", func() { }).Should(Succeed()) }) + It("treats a negative size as a full-size request, not a giant resize", func() { + seedFoundStore("al", "alneg", coverBytes) + + img, err := svc.Get(ctx, model.MustParseArtworkID("al-alneg"), -2000000000, false) + Expect(err).ToNot(HaveOccurred()) + Expect(readAll(img)).To(Equal(coverBytes), "original bytes, no resize (would OOM)") + }) + It("streams a file-backed found image at full size", func() { dir := GinkgoT().TempDir() imgPath := filepath.Join(dir, "cover.jpg") diff --git a/server/imghttp/headers_test.go b/server/imghttp/headers_test.go index 2d72c3563..52a549556 100644 --- a/server/imghttp/headers_test.go +++ b/server/imghttp/headers_test.go @@ -5,14 +5,16 @@ import ( "net/http" "net/http/httptest" "strings" - "testing" "time" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/server/imghttp" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) const testHash = "0123456789abcdef" +const testRepTag = testHash + ".300.false.q75" var lastMod = time.Date(2024, 1, 2, 3, 4, 5, 0, time.UTC) @@ -28,8 +30,6 @@ func placeholder() *artwork.Image { return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("PH")), Placeholder: true} } -const testRepTag = testHash + ".300.false.q75" - // resized carries a representation ETag distinct from the pixel hash (as a resized/re-encoded // response does), so the validator versions with the encode settings. func resized() *artwork.Image { @@ -41,9 +41,8 @@ func resized() *artwork.Image { } } -func TestWriteImageHeaders(t *testing.T) { - tests := []struct { - name string +var _ = Describe("WriteImageHeaders", func() { + type testCase struct { img *artwork.Image requestedHash string ifNoneMatch string @@ -51,155 +50,56 @@ func TestWriteImageHeaders(t *testing.T) { wantCache string wantETag string wantLastMod bool - }{ - { - name: "placeholder is never cached and carries no validators", - img: placeholder(), - wantCache: "no-store", - }, - { - name: "found with matching requested hash is immutable", - img: found(), - requestedHash: testHash, - wantCache: "public, max-age=31536000, immutable", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "found with bare id revalidates via no-cache", - img: found(), - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "found with mismatched requested hash revalidates", - img: found(), - requestedHash: "ffffffffffffffff", - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "resized keeps pixel-hash immutable but serves the representation ETag", - img: resized(), - requestedHash: testHash, - wantCache: "public, max-age=31536000, immutable", - wantETag: `"` + testRepTag + `"`, - wantLastMod: true, - }, - { - name: "resized 304s on the representation ETag, not the pixel hash", - img: resized(), - ifNoneMatch: `"` + testRepTag + `"`, - want304: true, - wantCache: "public, no-cache", - wantETag: `"` + testRepTag + `"`, - wantLastMod: true, - }, - { - name: "resized does not 304 on a stale pixel-hash validator (config changed)", - img: resized(), - ifNoneMatch: `"` + testHash + `"`, - want304: false, - wantCache: "public, no-cache", - wantETag: `"` + testRepTag + `"`, - wantLastMod: true, - }, - { - name: "If-None-Match matching the hash yields 304", - img: found(), - requestedHash: testHash, - ifNoneMatch: `"` + testHash + `"`, - want304: true, - wantCache: "public, max-age=31536000, immutable", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "weak If-None-Match matches (weak comparison)", - img: found(), - ifNoneMatch: `W/"` + testHash + `"`, - want304: true, - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "If-None-Match with multiple values matches one", - img: found(), - ifNoneMatch: `"deadbeefdeadbeef", W/"` + testHash + `", "cafecafecafecafe"`, - want304: true, - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "If-None-Match star matches any current representation", - img: found(), - ifNoneMatch: "*", - want304: true, - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "non-matching If-None-Match serves body", - img: found(), - ifNoneMatch: `"deadbeefdeadbeef"`, - want304: false, - wantCache: "public, no-cache", - wantETag: `"` + testHash + `"`, - wantLastMod: true, - }, - { - name: "placeholder ignores If-None-Match and never 304s", - img: placeholder(), - ifNoneMatch: "*", - want304: false, - wantCache: "no-store", - }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { + DescribeTable("applies the artwork caching contract", + func(c testCase) { w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/img", nil) - if tc.ifNoneMatch != "" { - r.Header.Set("If-None-Match", tc.ifNoneMatch) + if c.ifNoneMatch != "" { + r.Header.Set("If-None-Match", c.ifNoneMatch) } - got := imghttp.WriteImageHeaders(w, r, tc.img, tc.requestedHash) - if got != tc.want304 { - t.Fatalf("wrote304 = %v, want %v", got, tc.want304) - } + Expect(imghttp.WriteImageHeaders(w, r, c.img, c.requestedHash)).To(Equal(c.want304)) h := w.Header() - if got := h.Get("Cache-Control"); got != tc.wantCache { - t.Errorf("Cache-Control = %q, want %q", got, tc.wantCache) + Expect(h.Get("Cache-Control")).To(Equal(c.wantCache)) + Expect(h.Get("ETag")).To(Equal(c.wantETag)) + if c.img.Placeholder { + Expect(h.Get("ETag")).To(BeEmpty(), "placeholder must not set an ETag") + Expect(h.Get("Last-Modified")).To(BeEmpty(), "placeholder must not set Last-Modified") } - if got := h.Get("ETag"); got != tc.wantETag { - t.Errorf("ETag = %q, want %q", got, tc.wantETag) + Expect(h.Get("Last-Modified") != "").To(Equal(c.wantLastMod)) + if c.want304 { + Expect(w.Code).To(Equal(http.StatusNotModified)) + Expect(w.Body.Len()).To(BeZero(), "a 304 must have an empty body") } - if tc.img.Placeholder { - if h.Get("ETag") != "" { - t.Errorf("placeholder must not set ETag, got %q", h.Get("ETag")) - } - if h.Get("Last-Modified") != "" { - t.Errorf("placeholder must not set Last-Modified, got %q", h.Get("Last-Modified")) - } - } - if hasLM := h.Get("Last-Modified") != ""; hasLM != tc.wantLastMod { - t.Errorf("has Last-Modified = %v, want %v", hasLM, tc.wantLastMod) - } - if tc.want304 { - if w.Code != http.StatusNotModified { - t.Errorf("status = %d, want 304", w.Code) - } - if w.Body.Len() != 0 { - t.Errorf("304 must have empty body, got %q", w.Body.String()) - } - } - }) - } -} + }, + Entry("placeholder is never cached and carries no validators", + testCase{img: placeholder(), wantCache: "no-store"}), + Entry("found with matching requested hash is immutable", + testCase{img: found(), requestedHash: testHash, wantCache: "public, max-age=31536000, immutable", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("found with bare id revalidates via no-cache", + testCase{img: found(), wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("found with mismatched requested hash revalidates", + testCase{img: found(), requestedHash: "ffffffffffffffff", wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("resized keeps pixel-hash immutable but serves the representation ETag", + testCase{img: resized(), requestedHash: testHash, wantCache: "public, max-age=31536000, immutable", wantETag: `"` + testRepTag + `"`, wantLastMod: true}), + Entry("resized 304s on the representation ETag, not the pixel hash", + testCase{img: resized(), ifNoneMatch: `"` + testRepTag + `"`, want304: true, wantCache: "public, no-cache", wantETag: `"` + testRepTag + `"`, wantLastMod: true}), + Entry("resized does not 304 on a stale pixel-hash validator (config changed)", + testCase{img: resized(), ifNoneMatch: `"` + testHash + `"`, want304: false, wantCache: "public, no-cache", wantETag: `"` + testRepTag + `"`, wantLastMod: true}), + Entry("If-None-Match matching the hash yields 304", + testCase{img: found(), requestedHash: testHash, ifNoneMatch: `"` + testHash + `"`, want304: true, wantCache: "public, max-age=31536000, immutable", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("weak If-None-Match matches (weak comparison)", + testCase{img: found(), ifNoneMatch: `W/"` + testHash + `"`, want304: true, wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("If-None-Match with multiple values matches one", + testCase{img: found(), ifNoneMatch: `"deadbeefdeadbeef", W/"` + testHash + `", "cafecafecafecafe"`, want304: true, wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("If-None-Match star matches any current representation", + testCase{img: found(), ifNoneMatch: "*", want304: true, wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("non-matching If-None-Match serves body", + testCase{img: found(), ifNoneMatch: `"deadbeefdeadbeef"`, want304: false, wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), + Entry("placeholder ignores If-None-Match and never 304s", + testCase{img: placeholder(), ifNoneMatch: "*", want304: false, wantCache: "no-store"}), + ) +}) diff --git a/server/imghttp/imghttp_suite_test.go b/server/imghttp/imghttp_suite_test.go new file mode 100644 index 000000000..7a747db5b --- /dev/null +++ b/server/imghttp/imghttp_suite_test.go @@ -0,0 +1,17 @@ +package imghttp_test + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestImgHTTP(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "ImgHTTP Suite") +}