From 116fc5b853fe3bcd776438b95294511bfa3afaf3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 23:53:22 -0400 Subject: [PATCH] fix(artwork): bound decoded dimensions and stop the clamp at serve start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes to the inline updater. The tee's 20MB cap bounds compressed input only, so a small file declaring a huge raster could allocate GBs on decode; DecodeConfig now rejects anything over ~36M pixels from the header alone. And the write-side version clamp no longer advances past the serve's start time: a version change that lands mid-serve is not provably covered by the bytes being streamed, so the clamp stops there, the DTO omits, and the next serve of the new bytes heals — while structural read-side over-approximation (which always predates the serve) still clamps fully. --- core/artwork/artwork.go | 3 +- core/artwork/blurhash_updater.go | 23 +++++-- .../artwork/blurhash_updater_internal_test.go | 65 +++++++++++++++---- 3 files changed, 73 insertions(+), 18 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 6fa60c816..c4d3c6b75 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -93,8 +93,9 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa // changes precisely when the served cover changes. Placeholder bytes (playlist fallback) clear. // The tee wraps r directly, so Close reaches the underlying stream (no fd leak). version := capAtNow(artReader.LastUpdated()) + start := time.Now() reader = newTeeReader(r, maxTeeBytes, - func(data []byte) { a.blurHashes.update(ctx, artID, data, version) }) + func(data []byte) { a.blurHashes.update(ctx, artID, data, version, start) }) } return reader, artReader.LastUpdated(), nil } diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 04a03ad09..edcd0573a 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -46,9 +46,13 @@ func eligibleKind(artID model.ArtworkID) bool { return false } +// maxDecodePixels bounds the decoded raster: the tee's byte cap limits compressed size only, and a +// small file can declare huge dimensions that would allocate GBs on decode (decompression bomb). +const maxDecodePixels = 36_000_000 // ~6000x6000; decoded RGBA tops out around 144MB + // update hashes the exact bytes served for artID and persists the result. Placeholder bytes mean the -// entity has no artwork anymore, so they clear a stored hash instead. -func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, data []byte, version time.Time) { +// entity has no artwork anymore, so they clear a stored hash instead. start is when the serve began. +func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, data []byte, version, start time.Time) { // Decoding arbitrary image bytes can panic; the serve already succeeded, so just log it. defer func() { if r := recover(); r != nil { @@ -66,9 +70,14 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat sum := checksum(data) hash := u.cachedHash(artID, sum) if hash == "" { + cfg, _, err := image.DecodeConfig(bytes.NewReader(data)) + if err != nil || cfg.Width*cfg.Height > maxDecodePixels { + // Undecodable or oversized served bytes are not proof of change; keep the stored hash. + log.Trace(ctx, "BlurHash: skipping served bytes", "artID", artID, "width", cfg.Width, "height", cfg.Height, err) + return + } img, _, err := image.Decode(bytes.NewReader(data)) if err != nil { - // Undecodable served bytes are not proof of change; leave the stored hash intact. log.Trace(ctx, "BlurHash: served bytes not decodable, keeping stored hash", "artID", artID, err) return } @@ -82,12 +91,16 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat if err != nil { return } + // Clamp the persisted version up to the entity's, but never past the serve's start: a version that + // predates the serve is provably covered by the served bytes, one that landed mid-serve is not — + // there the clamp stops, the DTO omits, and the next serve of the new bytes heals. + if entityVersion.After(start) { + entityVersion = start + } if stored == hash && storedAt != nil && !storedAt.Before(entityVersion) { u.remember(artID, blurHashState{sum: sum, hash: hash}) return } - // Clamp the persisted version up to the entity's: the hash is fresh for what is served right now, - // so the DTO accepts it after any serve, however much the read-side version over-approximates. target := capAtNow(utils.TimeNewest(version, entityVersion)) if err := u.persist(ctx, artID, hash, target); err != nil { log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err) diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 6ea103d77..4fe076ea9 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -2,6 +2,8 @@ package artwork import ( "bytes" + "encoding/binary" + "hash/crc32" "image" "image/color" "image/png" @@ -13,6 +15,25 @@ import ( . "github.com/onsi/gomega" ) +// hugePNGHeader builds a valid PNG signature+IHDR declaring a 50000x50000 raster with no pixel data: +// enough for DecodeConfig to report the dimensions the decode gate must reject. +func hugePNGHeader() []byte { + var buf bytes.Buffer + buf.Write([]byte{0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a}) + ihdr := make([]byte, 13) + binary.BigEndian.PutUint32(ihdr[0:], 50000) + binary.BigEndian.PutUint32(ihdr[4:], 50000) + ihdr[8] = 8 // bit depth + ihdr[9] = 6 // RGBA + var chunk bytes.Buffer + chunk.WriteString("IHDR") + chunk.Write(ihdr) + _ = binary.Write(&buf, binary.BigEndian, uint32(13)) + buf.Write(chunk.Bytes()) + _ = binary.Write(&buf, binary.BigEndian, crc32.ChecksumIEEE(chunk.Bytes())) + return buf.Bytes() +} + // pngImage builds a deterministic 2x2 PNG image for a label (color derived from label bytes). func pngImage(label string) *image.RGBA { img := image.NewRGBA(image.Rect(0, 0, 2, 2)) @@ -61,7 +82,7 @@ var _ = Describe("blurHashUpdater", func() { It("persists a hash computed from the served bytes", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version}) - u.update(GinkgoT().Context(), id, realPNGBytes("x"), version) + u.update(GinkgoT().Context(), id, realPNGBytes("x"), version, time.Now()) al := stored("al-1") Expect(al.BlurHash).ToNot(BeEmpty()) Expect(al.BlurHashUpdatedAt).To(HaveValue(Equal(version))) @@ -69,23 +90,23 @@ var _ = Describe("blurHashUpdater", func() { It("clears the stored hash when the served bytes are a placeholder", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "OLD"}) - u.update(GinkgoT().Context(), id, placeholderImages()[0], version) + u.update(GinkgoT().Context(), id, placeholderImages()[0], version, time.Now()) Expect(stored("al-1").BlurHash).To(BeEmpty()) }) It("leaves the hash untouched on undecodable bytes", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "KEEP"}) - u.update(GinkgoT().Context(), id, []byte("not an image"), version) + u.update(GinkgoT().Context(), id, []byte("not an image"), version, time.Now()) Expect(stored("al-1").BlurHash).To(Equal("KEEP")) }) It("does not rewrite when the stored hash is current for the entity version", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version}) data := realPNGBytes("dedup") - u.update(GinkgoT().Context(), id, data, version) + u.update(GinkgoT().Context(), id, data, version, time.Now()) first := stored("al-1") // A later serve of the same bytes (newer tee version, unchanged entity) must not move the row. - u.update(GinkgoT().Context(), id, data, version.Add(time.Hour)) + u.update(GinkgoT().Context(), id, data, version.Add(time.Hour), time.Now()) Expect(stored("al-1").BlurHashUpdatedAt).To(HaveValue(Equal(*first.BlurHashUpdatedAt))) }) @@ -94,12 +115,12 @@ var _ = Describe("blurHashUpdater", func() { // by construction, so the write clamps up and the DTO accepts it — omission windows close. id := album(model.Album{ID: "al-1", UpdatedAt: version}) data := realPNGBytes("clamp") - u.update(GinkgoT().Context(), id, data, version) + u.update(GinkgoT().Context(), id, data, version, time.Now()) first := stored("al-1") newer := version.Add(time.Hour) album(model.Album{ID: "al-1", UpdatedAt: newer, BlurHash: first.BlurHash, BlurHashUpdatedAt: first.BlurHashUpdatedAt}) - u.update(GinkgoT().Context(), id, data, version) // same bytes, old tee version + u.update(GinkgoT().Context(), id, data, version, time.Now()) // same bytes, old tee version second := stored("al-1") Expect(second.BlurHash).To(Equal(first.BlurHash)) Expect(second.BlurHashUpdatedAt).To(HaveValue(Equal(newer))) @@ -108,16 +129,36 @@ var _ = Describe("blurHashUpdater", func() { It("restores the stored hash when it drifts from the served bytes", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version}) data := realPNGBytes("truth") - u.update(GinkgoT().Context(), id, data, version) + u.update(GinkgoT().Context(), id, data, version, time.Now()) truth := stored("al-1").BlurHash Expect(repo.UpdateBlurHash("al-1", "DRIFTED", version)).To(Succeed()) - u.update(GinkgoT().Context(), id, data, version) + u.update(GinkgoT().Context(), id, data, version, time.Now()) Expect(stored("al-1").BlurHash).To(Equal(truth)) }) + It("skips images whose declared dimensions exceed the decode bound", func() { + // The tee's byte cap limits compressed size only; a decompression bomb must be rejected from + // the header before the raster is allocated. + id := album(model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "KEEP"}) + u.update(GinkgoT().Context(), id, hugePNGHeader(), version, time.Now()) + Expect(stored("al-1").BlurHash).To(Equal("KEEP")) + }) + + It("does not mark older served bytes as current when the version advances mid-serve", func() { + // The cover was replaced and scanned after this serve started: the clamp must stop at the + // serve's start, so the DTO keeps omitting until the new bytes are served. + changedAt := version.Add(time.Hour) + id := album(model.Album{ID: "al-1", UpdatedAt: changedAt}) + u.update(GinkgoT().Context(), id, realPNGBytes("old-bytes"), version, version) + al := stored("al-1") + Expect(al.BlurHash).ToNot(BeEmpty()) + Expect(al.BlurHashUpdatedAt).To(HaveValue(Equal(version))) + Expect(al.BlurHashUpdatedAt.Before(al.ArtworkUpdatedAt())).To(BeTrue(), "must read as stale") + }) + It("does not write when a placeholder is served and nothing was ever stored", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version}) - u.update(GinkgoT().Context(), id, placeholderImages()[0], version) + u.update(GinkgoT().Context(), id, placeholderImages()[0], version, time.Now()) Expect(stored("al-1").BlurHashUpdatedAt).To(BeNil()) }) @@ -131,10 +172,10 @@ var _ = Describe("blurHashUpdater", func() { It("keys the decode cache by identity, ignoring the artwork id's embedded timestamp", func() { id := album(model.Album{ID: "al-1", UpdatedAt: version}) data := realPNGBytes("dedup") - u.update(GinkgoT().Context(), id, data, version) + u.update(GinkgoT().Context(), id, data, version, time.Now()) bumped := id bumped.LastUpdate = version.Add(time.Hour) - u.update(GinkgoT().Context(), bumped, data, version) + u.update(GinkgoT().Context(), bumped, data, version, time.Now()) Expect(u.seen).To(HaveLen(1)) }) })