diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 34a66ce..8f333a8 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -17,6 +17,7 @@ import { signatureMatches } from '../uploadTypes'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; +import { reencodeInPlace } from '../imageProcessing'; const router = Router(); @@ -178,6 +179,34 @@ async function verifyUploadedImages(req: Request): Promise { return null; } +/** + * Rebuilds every accepted file so it carries no metadata (#226). + * + * After verification, deliberately: re-encoding a file whose bytes do not match + * its declared type would be doing work on something already refused, and + * sharp's own error would replace the clearer message that check produces. + * + * A failure here refuses the upload rather than storing the original. Storing + * it would mean the one case where a photo keeps the coordinates it was taken + * at is the case nobody was told about. + * + * Returns the message to refuse with, or null when every file was rebuilt. + */ +async function stripUploadedImages(req: Request): Promise { + const files = (req.files as Express.Multer.File[]) || []; + + for (const file of files) { + try { + await reencodeInPlace(file.path, file.mimetype); + } catch (err) { + console.error(`[upload] could not re-encode ${file.path}:`, err); + return `${file.originalname} could not be processed`; + } + } + + return null; +} + // No error-handling middleware is mounted on the app, so translate multer's // limit errors here instead of letting them surface as a generic 500. const uploadImages = (req: Request, res: Response, next: NextFunction) => { @@ -200,6 +229,16 @@ const uploadImages = (req: Request, res: Response, next: NextFunction) => { verifyUploadedImages(req) .then((problem) => { + if (problem) { + res.status(400).json({ error: problem }); + return null; + } + return stripUploadedImages(req); + }) + .then((problem) => { + // The first stage returns null both when it answered and when it found + // nothing wrong, so the response itself is what distinguishes them. + if (res.headersSent) return; if (problem) { res.status(400).json({ error: problem }); return; diff --git a/backend/tests/integration/exifStripping.integration.test.ts b/backend/tests/integration/exifStripping.integration.test.ts new file mode 100644 index 0000000..6a529ed --- /dev/null +++ b/backend/tests/integration/exifStripping.integration.test.ts @@ -0,0 +1,119 @@ +import request from 'supertest'; +import sharp from 'sharp'; +import { promises as fs } from 'fs'; +import path from 'path'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +const UPLOADS_DIR = process.env.UPLOADS_DIR as string; + +beforeAll(async () => { + await fs.mkdir(UPLOADS_DIR, { recursive: true }); +}); + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** + * A JPEG carrying GPS EXIF, built rather than committed as a binary fixture so + * what it contains is readable in this file. This is the exact shape of the + * problem: a photograph that says where it was taken. + */ +async function photoWithLocation(): Promise { + return sharp({ + create: { width: 3000, height: 2000, channels: 3, background: { r: 120, g: 90, b: 60 } } + }) + // GPS tags live in IFD3 — that is the GPS IFD as libvips names it, and + // sharp's Exif type has no separate `GPS` key. Writing them into IFD0 + // instead would still produce EXIF, but not the tags this issue is + // actually about. + .withExif({ + IFD0: { Make: 'TestCam', Model: 'X1' }, + IFD3: { GPSLatitudeRef: 'N', GPSLatitude: '51/1 30/1 0/1', GPSLongitudeRef: 'W' } + }) + .jpeg() + .toBuffer(); +} + +async function storedPathFor(itemId: number): Promise { + const { rows } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + const row = rows[0]; + if (!row) throw new Error(`no item_images row for item ${itemId}`); + // image_path is '/uploads/'; the file is that name inside UPLOADS_DIR. + return path.join(UPLOADS_DIR, path.basename(row.image_path)); +} + +async function createItemWith(image: Buffer, filename: string): Promise { + const res = await request(app) + .post('/api/admin/items') + .field('name', 'Vase') + .field('description', '') + .field('price', '40') + .attach('images', image, filename); + + expect(res.status).toBe(200); + return res.body.id; +} + +describe('an uploaded photo does not keep where it was taken', () => { + it('has no EXIF once stored', async () => { + const withGps = await photoWithLocation(); + // Guard the fixture itself: if this ever stops carrying EXIF, the + // assertion below would pass while testing nothing at all. + expect((await sharp(withGps).metadata()).exif).toBeDefined(); + + const itemId = await createItemWith(withGps, 'vase.jpg'); + + const stored = await sharp(await storedPathFor(itemId)).metadata(); + expect(stored.exif).toBeUndefined(); + }); + + it('is bounded to the maximum dimension', async () => { + const itemId = await createItemWith(await photoWithLocation(), 'vase.jpg'); + + const stored = await sharp(await storedPathFor(itemId)).metadata(); + expect(stored.width).toBe(2000); + expect(stored.height).toBe(1333); + }); + + it('keeps the format, so the stored extension still describes the file', async () => { + const itemId = await createItemWith(await photoWithLocation(), 'vase.jpg'); + + const storedPath = await storedPathFor(itemId); + expect(path.extname(storedPath)).toBe('.jpg'); + expect((await sharp(storedPath).metadata()).format).toBe('jpeg'); + }); + + it('does not enlarge an image that is already small', async () => { + const small = await sharp({ + create: { width: 300, height: 200, channels: 3, background: { r: 1, g: 2, b: 3 } } + }) + .png() + .toBuffer(); + + const itemId = await createItemWith(small, 'tiny.png'); + + const stored = await sharp(await storedPathFor(itemId)).metadata(); + expect(stored.width).toBe(300); + expect(stored.height).toBe(200); + }); + + it('leaves no temporary re-encoding files on the volume', async () => { + await createItemWith(await photoWithLocation(), 'vase.jpg'); + + const leftovers = (await fs.readdir(UPLOADS_DIR)).filter((name) => + name.endsWith('.reencoding') + ); + expect(leftovers).toEqual([]); + }); +});