diff --git a/backend/__tests__/migrations/178_reset_legacy_preview_paths.test.js b/backend/__tests__/migrations/178_reset_legacy_preview_paths.test.js new file mode 100644 index 00000000..46909c71 --- /dev/null +++ b/backend/__tests__/migrations/178_reset_legacy_preview_paths.test.js @@ -0,0 +1,83 @@ +/** + * Legacy preview keys must not survive the encoder change. + * + * The old generator kept the SOURCE basename verbatim while always writing + * JPEG, so a `.webp` upload produced `preview_shot.webp` holding a JPEG. The + * route now derives Content-Type from the key, and sets `nosniff` — so that + * legacy object would be announced as image/webp and render as a broken image. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const migration = require('../../migrations/core/178_reset_legacy_preview_paths'); + +describe('migration 178 — legacy preview keys (#1166 follow-up)', () => { + let knex; let tmpDir; + + beforeAll(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'picpeak-mig188-')); + knex = require('knex')({ + client: 'sqlite3', + connection: { filename: path.join(tmpDir, 'db.sqlite') }, + useNullAsDefault: true, + }); + }); + + afterAll(async () => { + if (knex) await knex.destroy(); + await fs.promises.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(async () => { + await knex.schema.dropTableIfExists('photos'); + await knex.schema.createTable('photos', (t) => { + t.increments('id').primary(); + t.string('preview_path'); + t.string('thumbnail_path'); + }); + }); + + it('clears the mislabelled .webp keys that would render broken', async () => { + await knex('photos').insert({ preview_path: 'previews/preview_shot.webp' }); + + await migration.up(knex); + + expect((await knex('photos').first()).preview_path).toBeNull(); + }); + + it('clears .jpg keys too, because a byte-correct one can still be flattened', async () => { + // A legacy .jpg key is valid JPEG, but it may be a flattened rendition of a + // transparent or animated source, and nothing in the key says so. One lazy + // regeneration is cheaper than reasoning about which of them lied. + await knex('photos').insert([ + { preview_path: 'previews/preview_a.jpg' }, + { preview_path: 'previews/preview_b.png' }, + ]); + + await migration.up(knex); + + expect(await knex('photos').whereNotNull('preview_path').count('* as c').first()).toEqual({ c: 0 }); + }); + + it('leaves thumbnails alone — they are a different cache', async () => { + await knex('photos').insert({ preview_path: 'previews/p.jpg', thumbnail_path: 'thumbnails/t.jpg' }); + + await migration.up(knex); + + expect((await knex('photos').first()).thumbnail_path).toBe('thumbnails/t.jpg'); + }); + + it('is idempotent and safe with nothing to clear', async () => { + await migration.up(knex); + await expect(migration.up(knex)).resolves.toBeUndefined(); + }); + + it('no-ops before 104 has added the column', async () => { + await knex.schema.dropTableIfExists('photos'); + await knex.schema.createTable('photos', (t) => { t.increments('id').primary(); }); + + await expect(migration.up(knex)).resolves.toBeUndefined(); + }); +}); diff --git a/backend/__tests__/services/generatePreviewImage.formats.test.js b/backend/__tests__/services/generatePreviewImage.formats.test.js new file mode 100644 index 00000000..8a1663ae --- /dev/null +++ b/backend/__tests__/services/generatePreviewImage.formats.test.js @@ -0,0 +1,134 @@ +/** + * The preview tier must not destroy what it is previewing. + * + * generatePreviewImage encoded JPEG unconditionally. JPEG has no alpha channel + * and no second frame, so a transparent PNG came back flattened onto a solid + * background and an animated GIF came back as its first frame — for every + * consumer of this tier, not just the lightbox: the slideshow (#1015), admin + * previews, and the face avatars that read it as a whole-frame rendition. + * + * Driven against real Sharp output, because the whole question is what is in + * the encoded bytes. + */ + +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); +const sharp = require('sharp'); + +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); +const storageModule = require('../../src/services/storage'); + +/** A 2x2 GIF89a with two frames and a NETSCAPE loop block. */ +const ANIMATED_GIF = Buffer.from([ + 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, + 0x02, 0x00, 0x02, 0x00, + 0xF0, 0x00, 0x00, + 0x00, 0x00, 0x00, 0xFF, 0xFF, 0xFF, + 0x21, 0xFF, 0x0B, 0x4E, 0x45, 0x54, 0x53, 0x43, 0x41, 0x50, 0x45, + 0x32, 0x2E, 0x30, 0x03, 0x01, 0x00, 0x00, 0x00, + 0x21, 0xF9, 0x04, 0x00, 0x0A, 0x00, 0x00, 0x00, + 0x2C, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x02, 0x00, 0x00, + 0x02, 0x02, 0x44, 0x01, 0x00, + 0x21, 0xF9, 0x04, 0x00, 0x0A, 0x00, 0x00, 0x00, + 0x2C, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x02, 0x00, 0x00, + 0x02, 0x02, 0x4C, 0x01, 0x00, + 0x3B, +]); + +// No width-tier case here: the responsive `?w=` renditions (#1095) are +// main-only, so this branch has a single canonical preview per photo. +describe('generatePreviewImage encodes for the source (#1166 follow-up)', () => { + let storage; let storageRoot; let srcDir; let imageProcessor; + + beforeAll(async () => { + storageRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-prevfmt-store-')); + srcDir = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-prevfmt-src-')); + storage = new LocalFsStorage({ root: storageRoot }); + await storage.init(); + storageModule.setStorageForTesting(storage); + + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(storageRoot, { recursive: true, force: true }).catch(() => {}); + await fs.rm(srcDir, { recursive: true, force: true }).catch(() => {}); + }); + + const outMeta = async (key) => sharp(storage.resolveLocalPath(key)).metadata(); + + it('keeps transparency, as WebP, for a PNG with alpha', async () => { + const src = path.join(srcDir, 'logo.png'); + await sharp({ + create: { width: 800, height: 600, channels: 4, background: { r: 0, g: 0, b: 0, alpha: 0 } }, + }).png().toFile(src); + + const key = await imageProcessor.generatePreviewImage(src, { regenerate: true }); + + expect(key).toBe('previews/preview_logo.webp'); + const meta = await outMeta(key); + expect(meta.format).toBe('webp'); + // The regression, stated directly: JPEG would have flattened this. + expect(meta.hasAlpha).toBe(true); + }); + + it('keeps every frame, as WebP, for an animated GIF', async () => { + const src = path.join(srcDir, 'wave.gif'); + // Hand-assembled rather than produced by Sharp: writing a multi-page image + // needs pageHeight threaded through the pipeline, and a fixture that + // silently comes out single-page would make this test pass for the wrong + // reason. 2x2, two frames, black then white. + await fs.writeFile(src, ANIMATED_GIF); + // Precondition: the fixture really is animated. + expect((await sharp(src, { animated: true }).metadata()).pages).toBe(2); + + const key = await imageProcessor.generatePreviewImage(src, { regenerate: true }); + + expect(key).toBe('previews/preview_wave.webp'); + const meta = await sharp(storage.resolveLocalPath(key), { animated: true }).metadata(); + expect(meta.format).toBe('webp'); + // The regression, stated directly: JPEG kept only the first frame. + expect(meta.pages).toBe(2); + }); + + it('still writes plain JPEG for an ordinary photo', async () => { + // The common path must not pay for the two cases above: JPEG is smaller + // than WebP at the quality this tier uses, and every existing preview is + // one. + const src = path.join(srcDir, 'shot.jpg'); + await sharp({ create: { width: 2400, height: 1600, channels: 3, background: { r: 90, g: 90, b: 90 } } }) + .jpeg().toFile(src); + + const key = await imageProcessor.generatePreviewImage(src, { regenerate: true }); + + expect(key).toBe('previews/preview_shot.jpg'); + const meta = await outMeta(key); + expect(meta.format).toBe('jpeg'); + // 2400x1600 capped at the 1920 long edge, aspect preserved — unchanged. + expect([meta.width, meta.height]).toEqual([1920, 1280]); + }); + + it('names the output for what it wrote, not for the source', async () => { + // A PNG source used to produce `preview_x.png` holding JPEG bytes. Harmless + // while the route hard-coded image/jpeg; wrong once the encoding varies, + // and the route now reads the extension. + const src = path.join(srcDir, 'opaque.png'); + await sharp({ create: { width: 400, height: 400, channels: 3, background: { r: 1, g: 2, b: 3 } } }) + .png().toFile(src); + + const key = await imageProcessor.generatePreviewImage(src, { regenerate: true }); + + expect(key).toBe('previews/preview_opaque.jpg'); + expect((await outMeta(key)).format).toBe('jpeg'); + }); + + it('returns null on an unreadable source instead of throwing', async () => { + const src = path.join(srcDir, 'not-an-image.jpg'); + await fs.writeFile(src, 'plain text'); + + await expect(imageProcessor.generatePreviewImage(src, { regenerate: true })).resolves.toBeNull(); + }); +}); diff --git a/backend/migrations/core/178_reset_legacy_preview_paths.js b/backend/migrations/core/178_reset_legacy_preview_paths.js new file mode 100644 index 00000000..874399e4 --- /dev/null +++ b/backend/migrations/core/178_reset_legacy_preview_paths.js @@ -0,0 +1,55 @@ +/** + * Migration 178: drop preview keys written by the old generator. + * + * generatePreviewImage used to keep the SOURCE basename verbatim, extension and + * all, while always writing JPEG bytes. So a `.webp` upload produced + * `previews/preview_shot.webp` holding a JPEG, and a `.png` upload produced + * `preview_logo.png` holding a JPEG. + * + * That was harmless while the preview route hard-coded `Content-Type: + * image/jpeg`. It stopped being harmless the moment the encoding started + * varying: the route now reads the extension, so a legacy `.webp` key is + * announced as `image/webp` while containing JPEG — and the response carries + * `X-Content-Type-Options: nosniff`, so the browser will not quietly correct + * it. The lightbox shows a broken image for every photo that happened to be + * uploaded as WebP. + * + * The legacy `.png`-keyed previews are wrong in the other direction: they are + * flattened JPEGs of what may have been a transparent source, which is the + * defect the new encoder fixes and which `isPreviewValid` would otherwise let + * stand forever. + * + * Clearing the column is the whole repair. Previews are lazily regenerated by + * ensurePreviewImage on the next open, under the new naming and the new + * encoder, so the only cost is one regeneration per photo that is actually + * viewed. Nothing is deleted from storage — a migration is the wrong place to + * reach into a backend that may be S3 — so the old objects linger as + * unreferenced bytes, which the storage breakdown counts honestly. + * + * Deliberately clears ALL of them, not just the ones whose extension looks + * suspicious. A `.jpg`-keyed legacy preview is byte-correct, but it may still + * be a flattened rendition of a transparent or animated source, and there is + * no way to tell from the key. One lazy regeneration is cheaper than reasoning + * about which of them lied. + */ + +exports.up = async function(knex) { + if (!(await knex.schema.hasTable('photos'))) return; + if (!(await knex.schema.hasColumn('photos', 'preview_path'))) return; + + const cleared = await knex('photos') + .whereNotNull('preview_path') + .update({ preview_path: null }); + + if (cleared) { + console.log(`178_reset_legacy_preview_paths: cleared ${cleared} preview key(s); they regenerate on next view`); + } +}; + +/** + * Irreversible by design, and harmless: the column held a cache key, and the + * cache rebuilds itself. There is nothing to restore. + */ +exports.down = async function() { + console.log('178_reset_legacy_preview_paths: rollback is a no-op (preview keys are a regenerable cache)'); +}; diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index dd1f0e91..36df1498 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -1963,7 +1963,12 @@ router.get('/:slug/preview/:photoId', } res.set({ - 'Content-Type': 'image/jpeg', + // From the key, not hard-coded: a preview of a transparent or animated + // source is WebP, because JPEG carries neither. `nosniff` below means + // getting this wrong shows a broken image rather than being silently + // corrected by the browser. Pre-existing keys have no .webp suffix and + // are JPEG, so they keep their old header. + 'Content-Type': previewPath.endsWith('.webp') ? 'image/webp' : 'image/jpeg', // Cache aggressively — preview only changes on photo // re-upload (which generates a new preview key) or settings // regenerate (which writes a new mtime + ETag). @@ -1975,6 +1980,16 @@ router.get('/:slug/preview/:photoId', }); if (watermarkSettings && watermarkSettings.enabled) { + // No Content-Type override here. applyWatermark PRESERVES the source + // format (watermarkService.js: png -> png, webp -> webp, else jpeg), + // and its input is this preview — so the output format matches the key + // the header was already derived from. Forcing image/jpeg would + // mislabel a watermarked WebP preview, and `nosniff` means the browser + // will not correct it. + // + // What is still lost is the animation: the compositor flattens a + // multi-frame source to one frame while keeping the WebP container. + // That is a separate problem and a much larger one. const watermarkedBuffer = await withLocalCopy(previewPath, (localPath) => watermarkService.applyWatermark(localPath, watermarkSettings) ); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 5539a1f3..94cb35d3 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -556,22 +556,50 @@ async function ensureHeroImage(photo) { * Output to `previews/preview_` so an admin who flips the * setting back off can wipe the folder cleanly without touching * thumbnails or heroes. + * + * ENCODING follows the source, it is not always JPEG. JPEG has no alpha + * channel and no second frame, so encoding everything as JPEG flattened a + * transparent PNG onto a solid background and reduced an animated GIF to its + * first frame — for every consumer of this tier, not just the lightbox. + * Sources with alpha or more than one page are encoded as WebP instead, which + * carries both and is still far smaller than the original. + * + * The output extension is rewritten to match what was actually written. + * Previously the source basename was kept verbatim, so a PNG source produced + * `preview_foo.png` holding JPEG bytes — harmless while the route hard-coded + * image/jpeg, and actively wrong now that the encoding varies. Old keys keep + * working: they are still JPEG and still served as such. */ async function generatePreviewImage(imagePath, options = {}) { // outputBasename lets callers disambiguate sources that share a basename // (external mounts, see ensurePreviewImage) — same contract as // generateThumbnail. const filename = options.outputBasename || path.basename(imagePath); - const previewFilename = `preview_${filename}`; - const previewRelKey = path.posix.join('previews', previewFilename); const storage = getStorage(); + // Probed BEFORE the key is built: the extension has to match the encoding, + // and the encoding depends on what the source turns out to be. + let probe; + try { + probe = await sharp(imagePath).metadata(); + } catch (error) { + const msg = (error && error.message) ? error.message : String(error); + logger.error(`Failed to read metadata for preview of ${filename}: ${msg}`); + return null; + } + const isAnimated = (probe.pages || 1) > 1; + const needsWebp = isAnimated || probe.hasAlpha === true; + + const base = filename.replace(/\.[^./\\]+$/, ''); + const previewFilename = `preview_${base}.${needsWebp ? 'webp' : 'jpg'}`; + const previewRelKey = path.posix.join('previews', previewFilename); + if (options.regenerate) { await storage.delete(previewRelKey).catch(() => {}); } try { - const metadata = await sharp(imagePath).metadata(); + const metadata = probe; if (!metadata.width || !metadata.height) { throw new Error('Invalid image metadata - file may be incomplete'); } @@ -583,6 +611,12 @@ async function generatePreviewImage(imagePath, options = {}) { limitInputPixels: 268402689, // ~16k x 16k max sequentialRead: true, failOn: 'none', + // Without this an animated source is opened as its first frame only, and + // every later frame is discarded before the resize ever sees it. + // limitInputPixels still applies, and sharp counts an animated input as + // width x (height x pages) — so a pathological GIF is rejected rather + // than decoded, and the caller falls back to the original. + animated: isAnimated, }); // Strip EXIF — same privacy reasoning as thumbnails/heroes. @@ -596,18 +630,18 @@ async function generatePreviewImage(imagePath, options = {}) { fit: 'inside', }); - sharpInstance = sharpInstance.jpeg({ - quality, - progressive: true, - mozjpeg: true, - }); + sharpInstance = needsWebp + ? sharpInstance.webp({ quality }) + : sharpInstance.jpeg({ quality, progressive: true, mozjpeg: true }); const buffer = await sharpInstance.toBuffer(); if (!buffer || buffer.length === 0) { throw new Error('Generated preview image is empty'); } - await storage.put(previewRelKey, buffer, { contentType: 'image/jpeg' }); + await storage.put(previewRelKey, buffer, { + contentType: needsWebp ? 'image/webp' : 'image/jpeg', + }); logger.info(`Generated preview image for ${filename} → ${previewRelKey}`); return previewRelKey; diff --git a/frontend/src/components/gallery/__tests__/lightboxImageUrl.test.ts b/frontend/src/components/gallery/__tests__/lightboxImageUrl.test.ts index 6f4e5465..d366f8d5 100644 --- a/frontend/src/components/gallery/__tests__/lightboxImageUrl.test.ts +++ b/frontend/src/components/gallery/__tests__/lightboxImageUrl.test.ts @@ -59,39 +59,30 @@ describe('lightboxImageUrl (#1166)', () => { })).toBe('/api/gallery/g/preview/47?wm=3'); }); - it.each(['image/gif', 'image/apng', 'image/png'])( - 'keeps the original for %s, which the preview tier would flatten', + it.each(['image/gif', 'image/apng', 'image/png', 'image/webp', 'image/jpeg'])( + 'uses the preview tier for %s — the backend preserves alpha and frames now', (mime_type) => { - // generatePreviewImage encodes JPEG: no second frame, no alpha channel. - expect(lightboxImageUrl({ ...PHOTO, mime_type })).toBe('/api/gallery/g/photo/47'); + // The bypass list this replaces existed because generatePreviewImage + // always encoded JPEG. Previews of alpha or multi-page sources are WebP + // now, so there is nothing left for the frontend to guess at. + expect(lightboxImageUrl({ ...PHOTO, mime_type } as Parameters[0])) + .toBe('/api/gallery/g/preview/47'); + }, + ); + it.each(['image/gif', 'image/apng', 'image/png', 'image/webp', 'image/jpeg'])( + 'uses the preview tier for %s — the backend preserves alpha and frames now', + (mime_type) => { + // The bypass list this replaces existed because generatePreviewImage + // always encoded JPEG. Previews of alpha or multi-page sources are WebP + // now, so there is nothing left for the frontend to guess at — including + // the filename check that worked around migration 039's mislabelling. + expect(lightboxImageUrl({ ...PHOTO, mime_type } as Parameters[0])) + .toBe('/api/gallery/g/preview/47'); }, ); - it('catches a PNG that migration 039 mislabelled as image/jpeg', () => { - // 039 backfilled every pre-existing photo's mime_type to image/jpeg, and - // the external-media importer inserts rows with none at all — so trusting - // MIME alone lets exactly the transparent photos through. - expect(lightboxImageUrl({ - url: '/api/gallery/g/photo/47', - preview_url: null, - slideshow_url: '/api/gallery/g/preview/47', - mime_type: 'image/jpeg', - filename: 'logo-with-alpha.png', - })).toBe('/api/gallery/g/photo/47'); - }); - - it('catches one with no mime_type at all, as external imports write them', () => { - expect(lightboxImageUrl({ - url: '/api/gallery/g/photo/47', - preview_url: null, - slideshow_url: '/api/gallery/g/preview/47', - filename: 'animation.gif', - })).toBe('/api/gallery/g/photo/47'); - }); - - it('still uses the preview tier for ordinary still formats', () => { - for (const mime_type of ['image/jpeg', 'image/webp', undefined]) { - expect(lightboxImageUrl({ ...PHOTO, mime_type })).toBe('/api/gallery/g/preview/47'); - } + it('ignores a filename that used to force the original', () => { + expect(lightboxImageUrl({ ...PHOTO, filename: 'legacy.png' } as Parameters[0])) + .toBe('/api/gallery/g/preview/47'); }); }); diff --git a/frontend/src/components/gallery/imageTiers.ts b/frontend/src/components/gallery/imageTiers.ts index 6bcddc57..550e3d99 100644 --- a/frontend/src/components/gallery/imageTiers.ts +++ b/frontend/src/components/gallery/imageTiers.ts @@ -31,34 +31,15 @@ export function lightboxImageUrl(photo: { url: string; preview_url?: string | null; slideshow_url?: string | null; - mime_type?: string; - filename?: string; - original_filename?: string | null; }): string { - // Animated and transparent formats keep the original. generatePreviewImage - // encodes JPEG, which has neither a second frame nor an alpha channel, so - // routing these through the preview tier would replace an animation with its - // first frame and flatten transparency onto a solid background — a - // regression the toggle-off default never had. - // - // PNG is in the list because that is where transparency is the norm, and - // because an APNG is normally reported as image/png rather than image/apng. - // Animated or alpha WebP declares image/webp exactly like an ordinary still - // and cannot be told apart from MIME. - // - // The proper fix is backend-side, encoding WebP for alpha or multi-page - // sources; when that lands this list goes away entirely. - // Checked against the FILENAME as well as the MIME, because mime_type is not - // trustworthy here: migration 039 backfilled every pre-existing photo as - // image/jpeg regardless of what it was, and the external-media importer - // inserts rows without a mime_type at all. A mislabelled PNG would otherwise - // sail past this and come back flattened. - const ORIGINAL_ONLY = ['image/gif', 'image/apng', 'image/png']; - const ORIGINAL_ONLY_EXT = /\.(gif|apng|png)$/i; - const name = photo.original_filename || photo.filename || ''; - if ((photo.mime_type && ORIGINAL_ONLY.includes(photo.mime_type)) || ORIGINAL_ONLY_EXT.test(name)) { - return photo.url; - } - + // No format is excluded any more. This used to bypass the preview tier for + // GIF, APNG and PNG because generatePreviewImage always encoded JPEG, which + // has neither an alpha channel nor a second frame — so a transparent source + // came back flattened and an animated one came back as a still. That is + // fixed at the source: previews of alpha or multi-page images are now WebP, + // which carries both, and the guess-by-MIME this file could never make + // correctly (a still and an animated WebP declare the same type) is gone + // with it — including the filename fallback the previous commit needed + // because migration 039 made mime_type untrustworthy. return photo.preview_url || photo.slideshow_url || photo.url; }