diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1787868..206a29a 100644 --- a/docs/ARCHITECTURE.md +++ b/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. diff --git a/internal/db/photos.sql.go b/internal/db/photos.sql.go index 884ba4d..9f799a7 100644 --- a/internal/db/photos.sql.go +++ b/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 = ? diff --git a/internal/db/querier.go b/internal/db/querier.go index 825889c..4afff6a 100644 --- a/internal/db/querier.go +++ b/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) diff --git a/internal/recipes/photo.go b/internal/recipes/photo.go index 50408e4..107c00d 100644 --- a/internal/recipes/photo.go +++ b/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 } diff --git a/internal/recipes/photo_test.go b/internal/recipes/photo_test.go index f852a1e..a8c08bd 100644 --- a/internal/recipes/photo_test.go +++ b/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) } } diff --git a/internal/server/media.go b/internal/server/media.go index 609ec06..68fee0d 100644 --- a/internal/server/media.go +++ b/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 diff --git a/internal/server/media_test.go b/internal/server/media_test.go index 1785d0f..936d556 100644 --- a/internal/server/media_test.go +++ b/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", diff --git a/migrations/queries/photos.sql b/migrations/queries/photos.sql index 983d5ab..79a62eb 100644 --- a/migrations/queries/photos.sql +++ b/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 = ?;