diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index ff74358..f021f10 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; use anyhow::{Context, Result}; -use image::ImageDecoder; use sqlx::PgPool; use tokio::sync::{Semaphore, broadcast}; use uuid::Uuid; @@ -205,38 +204,9 @@ impl CompressionWorker { // Run blocking image operations in a spawn_blocking task tokio::task::spawn_blocking(move || -> Result<()> { - // Reject decompression bombs *before* fully decoding: the upload body - // cap bounds the file size on disk, but a small file can still decode to - // enormous dimensions (e.g. a ~1 MB image expanding to 50k×50k px → - // gigabytes), OOM-ing the box during decode/resize. 12000×12000 covers - // any real phone photo; max_alloc hard-caps the decode allocation. - let mut reader = image::ImageReader::open(&original) - .context("failed to open image")? - .with_guessed_format() - .context("failed to read image header")?; - let mut limits = image::Limits::default(); - limits.max_image_width = Some(12_000); - limits.max_image_height = Some(12_000); - limits.max_alloc = Some(256 * 1024 * 1024); - reader.limits(limits); - - // Apply the EXIF orientation. Phones do not rotate the sensor data — they record - // the physical camera orientation in a tag and store the pixels as shot. `decode()` - // hands back those raw pixels, and the JPEG re-encode below writes no EXIF at all, - // so skipping this stores EVERY portrait photo sideways in the feed preview, the - // 2048px diashow display and the keepsake — while the untouched original still - // renders upright, which is why it looks like a viewer bug rather than a pipeline - // one. `into_decoder` carries the limits set above through to the decoder, so the - // decompression-bomb guard is unaffected. - let mut decoder = reader.into_decoder().context("failed to decode image")?; - // A missing or malformed tag is not a failure: most images simply have none. - let orientation = decoder - .orientation() - .unwrap_or(image::metadata::Orientation::NoTransforms); - let mut img = - image::DynamicImage::from_decoder(decoder).context("failed to decode image")?; - img.apply_orientation(orientation); - let img = img; + // Decompression-bomb limits + EXIF orientation, both in one place — see + // services::imaging for why neither may be skipped. + let img = crate::services::imaging::decode_oriented(&original)?; // Preview: max 800px, preserving aspect ratio (data-saver feed). img.resize( diff --git a/backend/src/services/export.rs b/backend/src/services/export.rs index 4622066..3abea0f 100644 --- a/backend/src/services/export.rs +++ b/backend/src/services/export.rs @@ -791,7 +791,12 @@ async fn run_html_export_inner( let thumb_path_clone = thumb_path.clone(); let thumb_result = tokio::task::spawn_blocking(move || -> Result<()> { - let img = image::open(&src_clone).context("failed to open image for thumbnail")?; + // `decode_oriented`, not `image::open`: the latter ignores the EXIF + // orientation tag AND applies no decode limits. Using it here is why every + // portrait photo came out sideways in the keepsake's HTML viewer grid — the + // re-encode below drops the tag, so the viewer cannot recover it. + let img = crate::services::imaging::decode_oriented(&src_clone) + .context("failed to open image for thumbnail")?; let resized = img.resize(400, 400, image::imageops::FilterType::Lanczos3); resized .save_with_format(&thumb_path_clone, image::ImageFormat::Jpeg) @@ -812,8 +817,12 @@ async fn run_html_export_inner( let full_path_clone = full_path.clone(); let compress_result = tokio::task::spawn_blocking(move || -> Result<()> { - let img = - image::open(&src_clone).context("failed to open image for compression")?; + // Same reason as the thumbnail above. This branch only runs for originals + // over 5 MB, which is why the viewer's full image looked correct for small + // photos and sideways for large ones — an inconsistency that reads as a + // viewer bug rather than an export one. + let img = crate::services::imaging::decode_oriented(&src_clone) + .context("failed to open image for compression")?; let resized = img.resize(2000, 2000, image::imageops::FilterType::Lanczos3); resized .save_with_format(&full_path_clone, image::ImageFormat::Jpeg) diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs new file mode 100644 index 0000000..94dcdab --- /dev/null +++ b/backend/src/services/imaging.rs @@ -0,0 +1,52 @@ +//! Shared image decoding. +//! +//! Exists so there is exactly ONE way to turn a file on disk into a `DynamicImage` in this +//! codebase. Two properties have to hold everywhere an image is decoded, and both were +//! previously re-derived per call site — which is how they drifted apart: +//! +//! - **EXIF orientation must be applied.** Phones do not rotate sensor data; they record how +//! the camera was held in a tag and store the pixels as shot. `image::open` and +//! `ImageReader::decode` both hand back the raw pixels and ignore that tag, and re-encoding +//! to JPEG writes no EXIF, so the derivative is permanently sideways while the untouched +//! original still renders upright. The compression worker was fixed; the export worker was +//! not, so every portrait photo came out sideways in the keepsake's HTML viewer. +//! - **Decode limits must be set.** The upload body cap bounds the file on disk, but a small +//! file can decode to enormous dimensions (a ~1 MB image expanding to 50k×50k px), OOM-ing +//! the box. `image::open` applies NO limits at all, so the export path was also decoding +//! arbitrary user-supplied images unbounded. + +use anyhow::{Context, Result}; +use image::{DynamicImage, ImageDecoder}; +use std::path::Path; + +/// Bounds for any decode of user-supplied image data. 12000×12000 covers any real phone +/// photo; `max_alloc` hard-caps the decode allocation. +fn decode_limits() -> image::Limits { + let mut limits = image::Limits::default(); + limits.max_image_width = Some(12_000); + limits.max_image_height = Some(12_000); + limits.max_alloc = Some(256 * 1024 * 1024); + limits +} + +/// Decode an image from disk with decompression-bomb limits applied and its EXIF +/// orientation baked into the pixels. +/// +/// Blocking — call inside `spawn_blocking`. +pub fn decode_oriented(path: &Path) -> Result { + let mut reader = image::ImageReader::open(path) + .context("failed to open image")? + .with_guessed_format() + .context("failed to read image header")?; + reader.limits(decode_limits()); + + // `into_decoder` carries the limits above through, so reading the tag costs nothing in + // safety. A missing or malformed tag is not an error — most images simply have none. + let mut decoder = reader.into_decoder().context("failed to decode image")?; + let orientation = decoder + .orientation() + .unwrap_or(image::metadata::Orientation::NoTransforms); + let mut img = DynamicImage::from_decoder(decoder).context("failed to decode image")?; + img.apply_orientation(orientation); + Ok(img) +} diff --git a/backend/src/services/mod.rs b/backend/src/services/mod.rs index 08a97b2..8f6a126 100644 --- a/backend/src/services/mod.rs +++ b/backend/src/services/mod.rs @@ -2,6 +2,7 @@ pub mod compression; pub mod config; pub mod disk; pub mod export; +pub mod imaging; pub mod maintenance; pub mod rate_limiter; pub mod sse_tickets; diff --git a/e2e/specs/06-export/exif-orientation.spec.ts b/e2e/specs/06-export/exif-orientation.spec.ts new file mode 100644 index 0000000..56cccc9 --- /dev/null +++ b/e2e/specs/06-export/exif-orientation.spec.ts @@ -0,0 +1,117 @@ +/** + * Regression guard — the keepsake must not be sideways. + * + * Round 1 taught the compression worker to apply EXIF orientation, which fixed the live app + * (feed preview + diashow display). The export worker was missed: it does NOT reuse those + * derivatives — it re-decodes the originals itself with `image::open`, which ignores the + * orientation tag — and then re-encodes to JPEG, which drops the tag, so the viewer has no + * way to recover it. + * + * The resulting damage was oddly shaped, which is what made it read as a viewer bug: + * - Gallery.zip originals → correct (byte-copied, EXIF intact) + * - Memories viewer grid thumbnails → ALWAYS sideways + * - Memories viewer full image >5 MB → sideways (re-encoded at 2000px) + * - Memories viewer full image ≤5 MB → correct (streamed byte-for-byte) + * + * So clicking a small photo silently "fixed" it and a large one didn't. This pins the + * thumbnail, which is the path every photo takes. + */ +import { test, expect } from '../../fixtures/test'; +import { uploadRaw } from '../../helpers/upload-client'; +import { BASE } from '../../helpers/env'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +// 40x20 landscape pixels tagged Orientation=6 ("rotate 90° CW to display"), so anything +// that honours the tag emits a PORTRAIT derivative. +const EXIF_FIXTURE = join(process.cwd(), 'fixtures', 'media', 'portrait-exif6.jpg'); + +/** Pixel dimensions from a JPEG's SOF marker — avoids an image dep for one assertion. */ +function jpegSize(buf: Buffer): { width: number; height: number } { + let i = 2; + while (i < buf.length) { + if (buf[i] !== 0xff) { + i++; + continue; + } + const marker = buf[i + 1]; + if (marker >= 0xc0 && marker <= 0xcf && marker !== 0xc4 && marker !== 0xc8 && marker !== 0xcc) { + return { height: buf.readUInt16BE(i + 5), width: buf.readUInt16BE(i + 7) }; + } + i += 2 + buf.readUInt16BE(i + 2); + } + throw new Error('no SOF marker found — not a JPEG?'); +} + +test.describe('Export — EXIF orientation in the keepsake', () => { + test('the Memories viewer thumbnail of a rotated photo is upright', async ({ host, db }) => { + test.setTimeout(90_000); + const bearer = { Authorization: `Bearer ${host.jwt}` }; + + const src = readFileSync(EXIF_FIXTURE); + // Sanity: the SOURCE really is stored landscape, or this test proves nothing. + const srcSize = jpegSize(src); + expect(srcSize.width).toBeGreaterThan(srcSize.height); + + const up = await uploadRaw(host.jwt, src, { + filename: 'hochkant.jpg', + contentType: 'image/jpeg', + caption: 'hochkant', + }); + expect(up.status).toBe(201); + const { id } = (await up.json()) as { id: string }; + await expect.poll(() => db.compressionStatus(id), { timeout: 30_000 }).toBe('done'); + + const rel = await fetch(`${BASE}/api/v1/host/gallery/release`, { + method: 'POST', + headers: bearer, + }); + expect(rel.status).toBe(204); + + await expect + .poll( + async () => { + const res = await fetch(`${BASE}/api/v1/export/status`, { headers: bearer }); + return (await res.json()).html?.status; + }, + { timeout: 60_000, intervals: [500] } + ) + .toBe('done'); + + const ticketRes = await fetch(`${BASE}/api/v1/export/ticket`, { + method: 'POST', + headers: bearer, + }); + const { ticket } = (await ticketRes.json()) as { ticket: string }; + const dl = await fetch(`${BASE}/api/v1/export/html?ticket=${encodeURIComponent(ticket)}`); + expect(dl.status).toBe(200); + + const dir = mkdtempSync(join(tmpdir(), 'eventsnap-exif-')); + try { + const zipPath = join(dir, 'Memories.zip'); + writeFileSync(zipPath, Buffer.from(await dl.arrayBuffer())); + + const entries = execFileSync('unzip', ['-Z1', zipPath], { encoding: 'utf8' }) + .split('\n') + .filter(Boolean); + const thumbEntry = entries.find((e) => e.includes(`${id}_thumb`)); + expect(thumbEntry, `no thumbnail for ${id} in Memories.zip`).toBeTruthy(); + + // `-p` streams the entry to stdout. Extracting to disk instead fails with EACCES: + // the archive preserves the container's file mode, which the test user can't read. + const thumb = execFileSync('unzip', ['-p', zipPath, thumbEntry!], { + maxBuffer: 64 * 1024 * 1024, + }); + const { width, height } = jpegSize(thumb); + + expect( + height, + `the keepsake grid thumbnail is ${width}x${height} — EXIF orientation was not applied` + ).toBeGreaterThan(width); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +});