Validate decoded photo size and require a photos row to serve

decodePhoto checked the GIF logical screen size, but gif.Decode returns
the first frame, which may be smaller; a 400x400 screen with a 1x1 first
frame passed and was scaled. The short-side check now measures the
decoded image's bounds. The pixel cap still comes from the header, before
decoding.

OpenPhoto served any file whose name was a canonical ULID and stored
size, with no photos row required. It now looks the id up with the new
GetPhoto query and reports an id with no row as fs.ErrNotExist, so
GET /media/photos/{id}-{size}.jpg is 404 for it even when a file exists.
OpenPhoto takes the request context for the lookup.

The media test fixture records a photos row for its served photo and
adds an orphan photo whose files exist without a row; both its sizes
must be 404.

Maestro task: 8a432108-d3ba-4013-ae8c-11963981d641

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs/ARCHITECTURE.md

@@ -141,7 +141,8 @@ automatic** (D-021).
   `X-MealQuest-CSRF` header.
 - `/auth/*` — the OIDC flow.
 - `/media/*` — cached photos, behind the session cookie.
-  `GET /media/photos/{id}-{400|1200}.jpg` serves a stored size with
+  `GET /media/photos/{id}-{400|1200}.jpg` serves a stored size of a photo
+  with a `photos` row, with
   `Cache-Control: private, max-age=31536000, immutable`; anything else is
   404.
 - `/health` — liveness.
@@ -269,8 +270,8 @@ automatic** (D-021).
 - Photos are cached locally (`photos`) and processed with
   `golang.org/x/image`. `recipes.PhotoCache.CachePhoto` downloads through
   the Fetcher's `FetchImage` (JPEG, PNG, WebP or GIF content types, 8 MB
-  cap, not cached), decodes the image (a GIF's first frame), refuses one
-  under 300 px on its short side, scales it so its long side is 1200 px and
+  cap, not cached), decodes the image (a GIF's first frame), refuses a
+  decoded image under 300 px on its short side, scales it so its long side is 1200 px and
   400 px, and stores each as JPEG quality 82 at
   `$MEALQUEST_DATA_DIR/media/photos/{id}-{size}.jpg`. Images are
   deduplicated by the SHA-256 of the downloaded bytes.

internal/db/photos.sql.go

@@ -59,6 +59,28 @@ func (q *Queries) CreatePhoto(ctx context.Context, arg CreatePhotoParams) (Photo
 	return i, err
 }
 
+const getPhoto = `-- name: GetPhoto :one
+SELECT id, source_url, local_path, width, height, sha256, fetched_at, created_at, updated_at FROM photos
+WHERE id = ?
+`
+
+func (q *Queries) GetPhoto(ctx context.Context, id string) (Photo, error) {
+	row := q.db.QueryRowContext(ctx, getPhoto, id)
+	var i Photo
+	err := row.Scan(
+		&i.ID,
+		&i.SourceUrl,
+		&i.LocalPath,
+		&i.Width,
+		&i.Height,
+		&i.Sha256,
+		&i.FetchedAt,
+		&i.CreatedAt,
+		&i.UpdatedAt,
+	)
+	return i, err
+}
+
 const getPhotoBySHA256 = `-- name: GetPhotoBySHA256 :one
 SELECT id, source_url, local_path, width, height, sha256, fetched_at, created_at, updated_at FROM photos
 WHERE sha256 = ?

internal/db/querier.go

@@ -43,6 +43,7 @@ type Querier interface {
 	// Returns sql.ErrNoRows until the row exists.
 	GetIntegrationsGoogle(ctx context.Context) (IntegrationsGoogle, error)
 	GetJob(ctx context.Context, id string) (Job, error)
+	GetPhoto(ctx context.Context, id string) (Photo, error)
 	// Finds stored image bytes by content hash. The schema does not make sha256
 	// unique, so the earliest-created (lowest id) match is returned.
 	GetPhotoBySHA256(ctx context.Context, sha256 string) (Photo, error)

internal/recipes/photo.go

@@ -165,13 +165,18 @@ func (c *PhotoCache) CachePhoto(ctx context.Context, rawURL string) (db.Photo, e
 }
 
 // OpenPhoto opens the stored file of photo id at size. An id that is not a
-// canonical ULID, a size not in PhotoSizes, or a photo that is not stored
-// is an error matching fs.ErrNotExist.
-func (c *PhotoCache) OpenPhoto(id string, size int) (*os.File, error) {
+// canonical ULID, a size not in PhotoSizes, an id with no photos row, or a
+// photo whose file is not stored is an error matching fs.ErrNotExist.
+func (c *PhotoCache) OpenPhoto(ctx context.Context, id string, size int) (*os.File, error) {
 	parsed, err := ulid.ParseStrict(id)
 	if err != nil || parsed.String() != id || !slices.Contains(PhotoSizes, size) {
 		return nil, fmt.Errorf("recipes: photo %q at size %d: %w", id, size, fs.ErrNotExist)
 	}
+	if _, err := db.New(c.store.Reader).GetPhoto(ctx, id); errors.Is(err, sql.ErrNoRows) {
+		return nil, fmt.Errorf("recipes: photo %q: %w", id, fs.ErrNotExist)
+	} else if err != nil {
+		return nil, fmt.Errorf("recipes: look up photo %q: %w", id, err)
+	}
 	return os.Open(c.path(id, size))
 }
 
@@ -183,8 +188,9 @@ func photoFileName(id string, size int) string {
 	return id + "-" + strconv.Itoa(size) + ".jpg"
 }
 
-// decodePhoto decodes a JPEG, PNG, WebP or GIF image, checking its size
-// from the header before decoding the pixels.
+// decodePhoto decodes a JPEG, PNG, WebP or GIF image, checking its pixel
+// count from the header before decoding the pixels, and the shorter side of
+// the decoded image: a GIF's first frame may be smaller than its screen.
 func decodePhoto(b []byte) (image.Image, error) {
 	cfg, format, err := image.DecodeConfig(bytes.NewReader(b))
 	if err != nil {
@@ -197,13 +203,13 @@ func decodePhoto(b []byte) (image.Image, error) {
 	if int64(cfg.Width)*int64(cfg.Height) > PhotoMaxPixels {
 		return nil, fmt.Errorf("%w: %dx%d", ErrPhotoTooLarge, cfg.Width, cfg.Height)
 	}
-	if min(cfg.Width, cfg.Height) < PhotoMinShortSide {
-		return nil, fmt.Errorf("%w: %dx%d, shorter side under %d px", ErrPhotoTooSmall, cfg.Width, cfg.Height, PhotoMinShortSide)
-	}
 	img, err := decode(b)
 	if err != nil {
 		return nil, fmt.Errorf("%w: %w", ErrPhotoFormat, err)
 	}
+	if size := img.Bounds().Size(); min(size.X, size.Y) < PhotoMinShortSide {
+		return nil, fmt.Errorf("%w: %dx%d, shorter side under %d px", ErrPhotoTooSmall, size.X, size.Y, PhotoMinShortSide)
+	}
 	return img, nil
 }
 

internal/recipes/photo_test.go

@@ -204,6 +204,33 @@ func TestCachePhotoGIFUsesFirstFrame(t *testing.T) {
 	}
 }
 
+func TestCachePhotoRejectsSmallGIFFirstFrame(t *testing.T) {
+	t.Parallel()
+	// A 400x400 screen whose first frame is 1x1: the frame is what would be
+	// stored, so it is what must measure PhotoMinShortSide.
+	var b bytes.Buffer
+	anim := &gif.GIF{
+		Image:  []*image.Paletted{image.NewPaletted(image.Rect(0, 0, 1, 1), color.Palette{color.Black})},
+		Delay:  []int{10},
+		Config: image.Config{ColorModel: color.Palette{color.Black}, Width: 400, Height: 400},
+	}
+	if err := gif.EncodeAll(&b, anim); err != nil {
+		t.Fatal(err)
+	}
+	cache, st, dataDir, _ := newTestPhotoCache(t, map[string]photoFile{
+		"/speck.gif": {"image/gif", b.Bytes()},
+	})
+	if _, err := cache.CachePhoto(t.Context(), "https://example.com/speck.gif"); !errors.Is(err, ErrPhotoTooSmall) {
+		t.Errorf("CachePhoto error = %v, want ErrPhotoTooSmall", err)
+	}
+	if n := countPhotos(t, st); n != 0 {
+		t.Errorf("%d photos recorded for a refused image", n)
+	}
+	if names := photoFiles(t, dataDir); len(names) != 0 {
+		t.Errorf("files stored for a refused image: %v", names)
+	}
+}
+
 func TestCachePhotoFillsTransparencyWhite(t *testing.T) {
 	t.Parallel()
 	cache, _, dataDir, _ := newTestPhotoCache(t, map[string]photoFile{
@@ -320,7 +347,7 @@ func TestCachePhotoDeduplicatesBySHA256(t *testing.T) {
 
 func TestOpenPhoto(t *testing.T) {
 	t.Parallel()
-	cache, _, _, _ := newTestPhotoCache(t, map[string]photoFile{
+	cache, _, dataDir, _ := newTestPhotoCache(t, map[string]photoFile{
 		"/a.png": {"image/png", encodePNG(t, gradient(400, 300))},
 	})
 	photo, err := cache.CachePhoto(t.Context(), "https://example.com/a.png")
@@ -328,23 +355,29 @@ func TestOpenPhoto(t *testing.T) {
 		t.Fatalf("CachePhoto: %v", err)
 	}
 	for _, size := range PhotoSizes {
-		f, err := cache.OpenPhoto(photo.ID, size)
+		f, err := cache.OpenPhoto(t.Context(), photo.ID, size)
 		if err != nil {
 			t.Fatalf("OpenPhoto %d: %v", size, err)
 		}
 		f.Close()
 	}
+	// A file with no photos row is not a stored photo.
+	orphan := store.NewID()
+	if err := os.WriteFile(filepath.Join(dataDir, "media", "photos", orphan+"-400.jpg"), []byte("jpeg"), 0o600); err != nil {
+		t.Fatal(err)
+	}
 	for _, tc := range []struct {
 		id   string
 		size int
 	}{
 		{photo.ID, 800},
 		{store.NewID(), 400},
+		{orphan, 400},
 		{strings.ToLower(photo.ID), 400},
 		{"../" + photo.ID, 400},
 		{"", 400},
 	} {
-		if _, err := cache.OpenPhoto(tc.id, tc.size); !errors.Is(err, fs.ErrNotExist) {
+		if _, err := cache.OpenPhoto(t.Context(), tc.id, tc.size); !errors.Is(err, fs.ErrNotExist) {
 			t.Errorf("OpenPhoto(%q, %d) error = %v, want fs.ErrNotExist", tc.id, tc.size, err)
 		}
 	}

internal/server/media.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"context"
 	"errors"
 	"io/fs"
 	"log/slog"
@@ -20,7 +21,7 @@ const photoCacheControl = "private, max-age=31536000, immutable"
 // PhotoFiles opens a cached photo's file at one of its stored sizes. An
 // unknown photo or size is an error matching fs.ErrNotExist.
 type PhotoFiles interface {
-	OpenPhoto(id string, size int) (*os.File, error)
+	OpenPhoto(ctx context.Context, id string, size int) (*os.File, error)
 }
 
 // getPhoto answers GET /media/photos/{id}-{size}.jpg with the stored JPEG.
@@ -32,7 +33,7 @@ func getPhoto(logger *slog.Logger, photos PhotoFiles) http.HandlerFunc {
 			notFound(w, r)
 			return
 		}
-		f, err := photos.OpenPhoto(id, size)
+		f, err := photos.OpenPhoto(r.Context(), id, size)
 		if errors.Is(err, fs.ErrNotExist) {
 			notFound(w, r)
 			return

internal/server/media_test.go

@@ -10,39 +10,58 @@ import (
 	"testing"
 	"testing/fstest"
 
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store"
 )
 
 // newMediaRouter returns a router over a photo cache whose data directory
-// holds both sizes of one stored photo, and that photo's id. Each size's
-// file holds its size as text.
-func newMediaRouter(t *testing.T) (http.Handler, string) {
+// holds both sizes of two photos, the id of the one with a photos row, and
+// the id of the orphan without one. Each size's file holds its size as
+// text.
+func newMediaRouter(t *testing.T) (r http.Handler, id, orphan string) {
 	t.Helper()
 	dataDir := t.TempDir()
-	photos, err := recipes.NewPhotoCache(recipes.NewFetcher(), newTestStore(t), dataDir)
+	st := newTestStore(t)
+	photos, err := recipes.NewPhotoCache(recipes.NewFetcher(), st, dataDir)
 	if err != nil {
 		t.Fatal(err)
 	}
-	id := store.NewID()
-	for _, size := range []string{"400", "1200"} {
-		p := filepath.Join(dataDir, "media", "photos", id+"-"+size+".jpg")
-		if err := os.WriteFile(p, []byte("jpeg "+size), 0o600); err != nil {
-			t.Fatal(err)
+	id, orphan = store.NewID(), store.NewID()
+	for _, stem := range []string{id, orphan} {
+		for _, size := range []string{"400", "1200"} {
+			p := filepath.Join(dataDir, "media", "photos", stem+"-"+size+".jpg")
+			if err := os.WriteFile(p, []byte("jpeg "+size), 0o600); err != nil {
+				t.Fatal(err)
+			}
 		}
 	}
+	now := store.Now().String()
+	if _, err := db.New(st.Writer).CreatePhoto(t.Context(), db.CreatePhotoParams{
+		ID:        id,
+		SourceUrl: "https://example.com/a.jpg",
+		LocalPath: "media/photos/" + id + "-1200.jpg",
+		Width:     1600,
+		Height:    1200,
+		Sha256:    "sha",
+		FetchedAt: now,
+		CreatedAt: now,
+		UpdatedAt: now,
+	}); err != nil {
+		t.Fatal(err)
+	}
 	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
-	r, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestRunner(t),
+	r, err = NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestRunner(t),
 		photos, fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}})
 	if err != nil {
 		t.Fatal(err)
 	}
-	return r, id
+	return r, id, orphan
 }
 
 func TestGetPhotoServesEachSize(t *testing.T) {
 	t.Parallel()
-	router, id := newMediaRouter(t)
+	router, id, _ := newMediaRouter(t)
 	for _, size := range []string{"400", "1200"} {
 		req := httptest.NewRequest(http.MethodGet, "/media/photos/"+id+"-"+size+".jpg", nil)
 		req.AddCookie(validSessionCookie)
@@ -66,7 +85,7 @@ func TestGetPhotoServesEachSize(t *testing.T) {
 
 func TestGetPhotoRequiresSession(t *testing.T) {
 	t.Parallel()
-	router, id := newMediaRouter(t)
+	router, id, _ := newMediaRouter(t)
 	req := httptest.NewRequest(http.MethodGet, "/media/photos/"+id+"-400.jpg", nil)
 	rec := httptest.NewRecorder()
 	router.ServeHTTP(rec, req)
@@ -84,10 +103,12 @@ func TestGetPhotoRequiresSession(t *testing.T) {
 
 func TestGetPhotoUnknownNotFound(t *testing.T) {
 	t.Parallel()
-	router, id := newMediaRouter(t)
+	router, id, orphan := newMediaRouter(t)
 	for _, name := range []string{
 		store.NewID() + "-400.jpg", // no such photo
+		orphan + "-400.jpg",        // file stored, no photos row
 		id + "-800.jpg",            // size not stored
+		orphan + "-1200.jpg",
 		id + "-0400.jpg",
 		id + "-+400.jpg",
 		id + "-400.png",

migrations/queries/photos.sql

@@ -15,3 +15,7 @@ SELECT * FROM photos
 WHERE sha256 = ?
 ORDER BY id
 LIMIT 1;
+
+-- name: GetPhoto :one
+SELECT * FROM photos
+WHERE id = ?;