Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them. A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not. The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis. Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail. The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time. This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301. Closes #300 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
175 lines
6.4 KiB
TypeScript
175 lines
6.4 KiB
TypeScript
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<Buffer> {
|
|
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();
|
|
}
|
|
|
|
/**
|
|
* A photo the way a phone actually writes one: pixels in the sensor's own
|
|
* orientation, and an EXIF tag saying which way up to display them.
|
|
*
|
|
* Orientation 6 means "rotate 90° clockwise to show this". So these 400x200
|
|
* landscape pixels are what a portrait photograph looks like on disk, and every
|
|
* viewer that honours the tag — the camera roll, the mail client, the browser
|
|
* the sender attached it from — shows it as 200x400. That is why the sender
|
|
* sees an upright photo and has no idea the file is sideways.
|
|
*/
|
|
async function portraitPhoto(): Promise<Buffer> {
|
|
return sharp({
|
|
create: { width: 400, height: 200, channels: 3, background: { r: 20, g: 60, b: 120 } }
|
|
})
|
|
// withMetadata rather than withExif, and the difference is not cosmetic:
|
|
// withExif({ IFD0: { Orientation: '6' } }) writes a tag that sharp reads
|
|
// back as orientation 1, so a fixture built that way carries no orientation
|
|
// at all and would let this test pass against the unfixed code. Checked
|
|
// rather than assumed — the first version of this test did exactly that.
|
|
.withMetadata({ orientation: 6 })
|
|
.jpeg()
|
|
.toBuffer();
|
|
}
|
|
|
|
async function storedPathFor(itemId: number): Promise<string> {
|
|
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/<name>'; 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<number> {
|
|
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);
|
|
});
|
|
|
|
/**
|
|
* The other half of stripping metadata, and the half #226 missed (#300).
|
|
*
|
|
* Dropping EXIF is right — a product photo should not publish where it was
|
|
* taken. But orientation lives in EXIF too, and deleting it without first
|
|
* applying it to the pixels does not leave the photo alone: it leaves the
|
|
* pixels sideways with nothing left to explain them. The sender sees an
|
|
* upright photo, uploads it, and finds it rotated.
|
|
*/
|
|
it('applies the orientation before throwing the tag away', async () => {
|
|
const itemId = await createItemWith(await portraitPhoto(), 'portrait.jpg');
|
|
|
|
const stored = await sharp(await storedPathFor(itemId)).metadata();
|
|
|
|
// 400x200 on disk with Orientation 6 is a 200x400 photograph. Stored
|
|
// upright, the dimensions swap.
|
|
expect(stored.width).toBe(200);
|
|
expect(stored.height).toBe(400);
|
|
});
|
|
|
|
// And the tag itself still goes, so nothing rotates it a second time.
|
|
it('does not leave the orientation tag behind after applying it', async () => {
|
|
const itemId = await createItemWith(await portraitPhoto(), 'portrait.jpg');
|
|
|
|
const stored = await sharp(await storedPathFor(itemId)).metadata();
|
|
|
|
expect(stored.exif).toBeUndefined();
|
|
// sharp reports 1 — the identity — for a file with nothing to say about it.
|
|
expect(stored.orientation ?? 1).toBe(1);
|
|
});
|
|
|
|
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([]);
|
|
});
|
|
});
|