Address Gadfly findings on #80

Security / correctness (multi-model):
- Wrap the decoders in a recover (decodeSafely): a malformed HEIC/WebP that
  panics libheif-via-WASM or x/image/webp now fails this one request as
  ErrUnsupported instead of taking the process down.
- Make the pixel-bomb guard overflow-safe: check each side against maxDimension
  BEFORE multiplying, so a header claiming ~2^32 on a side can't wrap
  int64(w)*int64(h) negative and slip past the pixel-count cap.
- Lower maxDecodePixels 100 MP → 50 MP (~200 MB peak), still above any current
  phone sensor, capping the amplification a small hostile header can force.
- Sentinel errors use errors.New, not fmt.Errorf without a verb.

The finding 5 models agreed on: the decompression-bomb guard was UNTESTED and a
stale comment implied otherwise. Added TestNormalizeRejectsPixelBomb, which
crafts a ~40-byte PNG (valid IHDR + CRC) claiming a huge canvas and asserts
ErrTooLarge from DecodeConfig alone, before any bitmap is allocated — covering
both the per-side and the area trip. Fixed the stale comment.

Error-handling / docs:
- format is now "" on ALL error paths (was populated on some, empty on others).
- Doc rewritten to state the actual error contract (read/encode I/O → wrapped,
  not a sentinel) and to stop referencing a non-existent TestNormalize / TODO.
- Guard io.LimitReader's +1 against an int64 overflow at an absurd MaxBytes.

Tests / provenance:
- Downscale test derives its numbers from DefaultMaxDim instead of hard-coding.
- Check the previously-ignored DecodeConfig error in the small-image case.
- testdata/README documents where sample.heic/webp came from.

Deferred to the #81 upload handler, documented in the Normalize doc: EXIF
orientation (best fixed and tested with a real oriented photo end-to-end) and
context cancellation (image.Decode isn't cancellable mid-decode; the caller
runs it under a timeout, and the size guards keep the work finite regardless).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

internal/imagenorm/imagenorm.go

@@ -22,11 +22,13 @@
 // Go's image decoders register via blank imports, and the failure mode is
 // backwards from intuition: forget "image/png" and PNG uploads fail with
 // "unknown format" while the exotic HEIC still works. So the blank imports below
-// are load-bearing, and TestNormalize exercises all four formats to keep them so.
+// are load-bearing, and TestNormalizeAllFormats exercises all four formats to
+// keep them so.
 package imagenorm
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"image"
 	"image/jpeg"
@@ -55,10 +57,19 @@ const (
 	// MiB leaves headroom for a large HEIC without inviting a decompression bomb
 	// as an unbounded read. The re-encoded output is far smaller.
 	DefaultMaxBytes = 25 << 20
-	// maxDecodePixels bounds the decoded bitmap regardless of input byte size — a
-	// small file can claim enormous dimensions (a decompression bomb). 100 MP is
-	// ~400 MB at 4 bytes/px decoded, already generous for any real photo.
-	maxDecodePixels = 100_000_000
+	// maxDecodePixels bounds the DECODED bitmap regardless of input byte size, so
+	// a small file claiming enormous dimensions (a decompression bomb) is refused
+	// before its ~4-bytes/px bitmap is allocated. 50 MP ≈ 200 MB peak — above any
+	// current phone camera (a 48 MP sensor is 48 MP) while capping the
+	// amplification a hostile header can force. maxDecodePixels and maxDimension
+	// are internal safety floors, not knobs — unlike MaxDim/MaxBytes there's no
+	// reason for a caller to raise them.
+	maxDecodePixels = 50_000_000
+	// maxDimension caps EACH side independently. It exists to make the pixel-count
+	// check overflow-safe: without it, a header claiming ~2^32 on a side could
+	// wrap int64(w)*int64(h) negative and slip past maxDecodePixels. No real image
+	// is 50k px on a side.
+	maxDimension = 50_000
 	// jpegQuality for the normalized output. 85 is visually clean and keeps the
 	// file small; the model reads text off it, not fine gradients.
 	jpegQuality = 85
@@ -85,56 +96,98 @@ func (o Options) maxBytes() int {
 }
 
 // ErrTooLarge means the input exceeded the byte cap or decoded to an absurd
-// pixel count. ErrUnsupported means the bytes weren't a decodable image format.
+// pixel count. ErrUnsupported means the bytes weren't a decodable image format —
+// or a decoder panicked on them (see the recover in Normalize).
 var (
-	ErrTooLarge    = fmt.Errorf("imagenorm: image too large")
-	ErrUnsupported = fmt.Errorf("imagenorm: unsupported or corrupt image")
+	ErrTooLarge    = errors.New("imagenorm: image too large")
+	ErrUnsupported = errors.New("imagenorm: unsupported or corrupt image")
 )
 
 // Normalize reads an image of any supported format (JPEG, PNG, HEIC, WebP),
 // downscales it to fit opts.MaxDim on its longest edge, and returns it re-encoded
-// as JPEG. It also reports the decoded format name (e.g. "heic"), which is handy
-// for logging what a phone actually sent.
+// as JPEG, plus the decoded format name (e.g. "heic") — handy for logging what a
+// phone actually sent. On any error the returned bytes are nil and format is "".
 //
-// It reads at most opts.MaxBytes and refuses a bitmap over maxDecodePixels, so a
-// hostile upload can neither exhaust memory by byte count nor by claimed
-// dimensions. A read/format error maps to ErrUnsupported; an over-limit input to
-// ErrTooLarge.
+// Errors, by cause:
+//   - input over opts.MaxBytes, or a decoded canvas over maxDecodePixels /
+//     maxDimension → ErrTooLarge, refused before the bitmap is allocated;
+//   - bytes that aren't a decodable image, or a decoder that panics on them →
+//     ErrUnsupported;
+//   - a genuine read or JPEG-encode I/O failure → a wrapped error (not a
+//     sentinel), since those are the caller's stream/environment, not the image.
+//
+// It bounds work against a hostile upload three ways: the byte cap, the
+// pre-decode pixel/dimension check, and a recover around the third-party decoders
+// (a malformed HEIC/WebP shouldn't take the process down).
+//
+// Two known gaps, both deferred to the upload handler that wires this in (#81):
+//   - EXIF ORIENTATION is not applied, so a portrait phone photo tagged
+//     "rotate 90°" comes out sideways. That's best fixed and tested with a real
+//     oriented photo end-to-end, which the library has no consumer for yet.
+//   - There is no context: image.Decode is CPU-bound and not cancellable
+//     mid-decode, so a caller that needs a hard deadline should run Normalize
+//     under its own timeout. The size guards keep the work finite regardless.
 func Normalize(r io.Reader, opts Options) (out []byte, format string, err error) {
 	// Cap the read at MaxBytes+1 so we can tell "exactly at the cap" from "over".
-	limited := io.LimitReader(r, int64(opts.maxBytes())+1)
-	raw, err := io.ReadAll(limited)
+	// maxBytes() is always a sane positive (default 25 MiB); guard the +1 anyway.
+	byteCap := opts.maxBytes()
+	limit := int64(byteCap) + 1
+	if limit < 1 {
+		limit = int64(DefaultMaxBytes) + 1
+	}
+	raw, err := io.ReadAll(io.LimitReader(r, limit))
 	if err != nil {
 		return nil, "", fmt.Errorf("imagenorm: read: %w", err)
 	}
-	if len(raw) > opts.maxBytes() {
+	if len(raw) > byteCap {
 		return nil, "", ErrTooLarge
 	}
 
 	// Check dimensions BEFORE a full decode, so a decompression bomb is refused
-	// before it allocates its bitmap.
-	cfg, format, err := image.DecodeConfig(bytes.NewReader(raw))
+	// before it allocates its bitmap. The per-side maxDimension check runs first
+	// so the pixel-count multiply below can't overflow.
+	cfg, _, err := image.DecodeConfig(bytes.NewReader(raw))
 	if err != nil {
 		return nil, "", ErrUnsupported
 	}
-	if int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels {
-		return nil, format, ErrTooLarge
+	if cfg.Width <= 0 || cfg.Height <= 0 ||
+		cfg.Width > maxDimension || cfg.Height > maxDimension ||
+		int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels {
+		return nil, "", ErrTooLarge
 	}
 
-	img, _, err := image.Decode(bytes.NewReader(raw))
+	img, format, err := decodeSafely(raw)
 	if err != nil {
-		return nil, format, ErrUnsupported
+		return nil, "", err
 	}
 
 	img = downscale(img, opts.maxDim())
 
 	var buf bytes.Buffer
 	if err := jpeg.Encode(&buf, img, &jpeg.Options{Quality: jpegQuality}); err != nil {
-		return nil, format, fmt.Errorf("imagenorm: encode jpeg: %w", err)
+		return nil, "", fmt.Errorf("imagenorm: encode jpeg: %w", err)
 	}
 	return buf.Bytes(), format, nil
 }
 
+// decodeSafely decodes raw, converting both a decode error and a decoder PANIC
+// into ErrUnsupported. The recover matters because the image comes from an
+// untrusted upload and the HEIC/WebP decoders are third-party (libheif via WASM,
+// x/image/webp): a malformed file that panics one of them must fail this one
+// request, not crash the process.
+func decodeSafely(raw []byte) (img image.Image, format string, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			img, format, err = nil, "", ErrUnsupported
+		}
+	}()
+	img, format, err = image.Decode(bytes.NewReader(raw))
+	if err != nil {
+		return nil, "", ErrUnsupported
+	}
+	return img, format, nil
+}
+
 // downscale returns img shrunk so its longest edge is at most maxDim, preserving
 // aspect ratio. An image already within bounds is returned unchanged (no
 // re-sampling, no quality loss beyond the JPEG round-trip). Uses Catmull-Rom for
@@ -142,10 +195,7 @@ func Normalize(r io.Reader, opts Options) (out []byte, format string, err error)
 func downscale(img image.Image, maxDim int) image.Image {
 	b := img.Bounds()
 	w, h := b.Dx(), b.Dy()
-	longest := w
-	if h > longest {
-		longest = h
-	}
+	longest := max(w, h)
 	if longest <= maxDim || longest == 0 {
 		return img
 	}

internal/imagenorm/imagenorm_test.go

@@ -2,6 +2,8 @@ package imagenorm
 
 import (
 	"bytes"
+	"encoding/binary"
+	"hash/crc32"
 	"image"
 	"image/jpeg"
 	"image/png"
@@ -85,8 +87,11 @@ func TestNormalizeAllFormats(t *testing.T) {
 // TestNormalizeDownscales checks a large image is shrunk to fit MaxDim on its
 // longest edge with aspect ratio preserved, and a small one is left alone.
 func TestNormalizeDownscales(t *testing.T) {
-	// 4000x1000 → longest 4000, MaxDim 2048 → scaled to 2048x512.
-	big := pngBytes(t, 4000, 1000)
+	// A 2:1 image twice as wide as DefaultMaxDim → clamped to DefaultMaxDim on the
+	// long edge with aspect preserved. Derived from the constant, not hard-coded,
+	// so the test tracks the default rather than silently asserting a magic number.
+	longEdge := DefaultMaxDim * 2
+	big := pngBytes(t, longEdge, longEdge/2)
 	out, _, err := Normalize(bytes.NewReader(big), Options{})
 	if err != nil {
 		t.Fatalf("Normalize: %v", err)
@@ -95,11 +100,11 @@ func TestNormalizeDownscales(t *testing.T) {
 	if err != nil {
 		t.Fatalf("decode out: %v", err)
 	}
-	if cfg.Width != 2048 {
-		t.Errorf("width = %d, want 2048 (longest edge clamped)", cfg.Width)
+	if cfg.Width != DefaultMaxDim {
+		t.Errorf("width = %d, want %d (longest edge clamped)", cfg.Width, DefaultMaxDim)
 	}
-	if cfg.Height != 512 {
-		t.Errorf("height = %d, want 512 (aspect preserved)", cfg.Height)
+	if cfg.Height != DefaultMaxDim/2 {
+		t.Errorf("height = %d, want %d (aspect preserved)", cfg.Height, DefaultMaxDim/2)
 	}
 
 	// A small image within bounds keeps its dimensions.
@@ -108,7 +113,10 @@ func TestNormalizeDownscales(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Normalize small: %v", err)
 	}
-	cfg, _, _ = image.DecodeConfig(bytes.NewReader(out))
+	cfg, _, err = image.DecodeConfig(bytes.NewReader(out))
+	if err != nil {
+		t.Fatalf("decode small out: %v", err)
+	}
 	if cfg.Width != 100 || cfg.Height != 80 {
 		t.Errorf("small image resized to %dx%d, want 100x80", cfg.Width, cfg.Height)
 	}
@@ -124,9 +132,8 @@ func TestNormalizeRejectsOversizeInput(t *testing.T) {
 	}
 }
 
-// TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is
-// refused before the bitmap is allocated. A tiny MaxDim doesn't matter — the
-// guard is on the DECODED pixel count, checked from DecodeConfig.
+// TestNormalizeRejectsGarbage: unreadable-as-image bytes and a truncated image
+// both fail cleanly with ErrUnsupported, not a panic.
 func TestNormalizeRejectsGarbage(t *testing.T) {
 	_, _, err := Normalize(bytes.NewReader([]byte("not an image at all")), Options{})
 	if err != ErrUnsupported {
@@ -139,3 +146,57 @@ func TestNormalizeRejectsGarbage(t *testing.T) {
 		t.Errorf("truncated image err = %v, want ErrUnsupported", err)
 	}
 }
+
+// pngHeader builds a valid PNG signature + IHDR chunk (with a correct CRC, which
+// DecodeConfig verifies) for the given dimensions, and nothing else. It's enough
+// for image.DecodeConfig to report width/height without a real bitmap — exactly
+// what's needed to exercise the pre-decode size guard with a tiny input.
+func pngHeader(w, h uint32) []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:], w)
+	binary.BigEndian.PutUint32(ihdr[4:], h)
+	ihdr[8] = 8 // bit depth
+	ihdr[9] = 6 // colour type: RGBA
+	// compression/filter/interlace already 0.
+	binary.Write(&buf, binary.BigEndian, uint32(len(ihdr)))
+	chunk := append([]byte("IHDR"), ihdr...)
+	buf.Write(chunk)
+	binary.Write(&buf, binary.BigEndian, crc32.ChecksumIEEE(chunk))
+	return buf.Bytes()
+}
+
+// TestNormalizeRejectsPixelBomb is the guard the review found untested: a small
+// input (a bare ~40-byte PNG header) claiming an enormous canvas is refused with
+// ErrTooLarge from DecodeConfig alone, before image.Decode allocates anything.
+// Covers both the per-side maxDimension trip and the pixel-count trip — and, via
+// the near-2^16-per-side case, that the count math doesn't overflow.
+func TestNormalizeRejectsPixelBomb(t *testing.T) {
+	cases := []struct {
+		name string
+		w, h uint32
+	}{
+		{"huge single side", 60000, 10},             // > maxDimension on width
+		{"huge area within side cap", 40000, 40000}, // sides < cap, area 1.6 GP > maxDecodePixels
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			hdr := pngHeader(tc.w, tc.h)
+			if len(hdr) > 100 {
+				t.Fatalf("header unexpectedly large (%d bytes) — not a bomb test", len(hdr))
+			}
+			// Sanity: the header really does decode to those dimensions.
+			cfg, _, err := image.DecodeConfig(bytes.NewReader(hdr))
+			if err != nil {
+				t.Fatalf("crafted PNG header didn't parse: %v", err)
+			}
+			if uint32(cfg.Width) != tc.w || uint32(cfg.Height) != tc.h {
+				t.Fatalf("header reports %dx%d, want %dx%d", cfg.Width, cfg.Height, tc.w, tc.h)
+			}
+			if _, _, err := Normalize(bytes.NewReader(hdr), Options{}); err != ErrTooLarge {
+				t.Errorf("pixel bomb %dx%d err = %v, want ErrTooLarge", tc.w, tc.h, err)
+			}
+		})
+	}
+}

internal/imagenorm/testdata/README.md

@@ -0,0 +1,14 @@
+# imagenorm test fixtures
+
+Go has no encoder for HEIC or WebP, so these small samples are committed rather
+than generated at test time. They are used by `TestNormalizeAllFormats` to prove
+every accepted format round-trips to JPEG (and, mainly, to catch a dropped blank
+import — see the package doc).
+
+- `sample.heic` — a 240×160 gradient, created from a Go-generated PNG with macOS
+  `sips -s format heic`. HEVC still-image profile.
+- `sample.webp` — a 16×16 image copied from CPython's stdlib test corpus
+  (`Lib/test/test_email/data/python.webp`), verified to decode with
+  `golang.org/x/image/webp` before committing. Used only as a decode fixture.
+
+Neither contains anything meaningful; they exist purely to be decoded.