docs(uploads): plan the EXIF stripping and re-encode (#226)
Five tasks: prove sharp installs where it actually runs, the re-encode policy as a pure module, wiring it into the single middleware every upload path already passes through, the backfill over already-stored photos, and the deployment sequence. Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. Same reasoning that makes uploadTypes.ts an allowlist. Format is preserved rather than normalised to WebP. Converting would compress better but changes every stored extension, and therefore item_images.image_path, turning the backfill into a rename with a window where rows point at files that no longer exist. A privacy fix does not need that risk, and the backfill consequently touches no database rows at all. The backfill is lossy and irreversible, so it reports by default and needs --apply, writes to a temporary file and renames so an interruption cannot leave a half-written image being served, and is idempotent by construction: a file already stripped and already within bounds is skipped rather than put through a second lossy pass. That property is a pure function with its own unit test, because being wrong about it degrades every image a little more on every run. Two traps the plan handles that the issue only named. An animated WebP read without the animated flag decodes to a single frame and is silently written back as a still, so the flag is set for WebP and only WebP — it changes how resize reads height, which would be wrong for the other types. And sharp before 0.33 has no withExif, which the tests need to build their fixture; on an older version they fail as though the stripping were broken. Ref #226
This commit is contained in:
@@ -0,0 +1,782 @@
|
|||||||
|
# Strip EXIF and Re-encode Uploads Implementation Plan
|
||||||
|
|
||||||
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||||
|
|
||||||
|
**Goal:** No uploaded photo — new or already stored — carries the coordinates it was taken at, and stored bytes fall by roughly an order of magnitude.
|
||||||
|
|
||||||
|
**Architecture:** `sharp` re-encodes every accepted upload inside the existing `uploadImages` middleware, immediately after the magic-byte verification and before any route logic runs. Re-encoding rebuilds the file, so EXIF is gone as a consequence rather than as a deletion that could miss a tag. A separate one-off script applies the same transform to the photos already on disk.
|
||||||
|
|
||||||
|
**Tech Stack:** TypeScript, `sharp`, Express + multer, Jest + supertest, `tsx` for the script.
|
||||||
|
|
||||||
|
**Issue:** https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/226
|
||||||
|
|
||||||
|
## Global Constraints
|
||||||
|
|
||||||
|
- **Format is preserved, never converted.** JPEG stays JPEG, PNG stays PNG, WebP stays WebP. See the decision below — this is what keeps the stored extension, the served content type and `item_images.image_path` all consistent, and it is what makes the backfill a rewrite rather than a rename.
|
||||||
|
- **`sharp` is a production dependency.** The final Docker stage runs `npm install --omit=dev`, so a dev-dependency `sharp` would be absent at runtime and every upload would fail in production only.
|
||||||
|
- **Every route handler stays wrapped in `asyncRoute`** — `tests/unit/routesAreWrapped.test.ts` enforces it.
|
||||||
|
- **The backfill is lossy and irreversible.** It does not run against production without a verified backup and a QA run first.
|
||||||
|
- **Commit style:** Conventional Commits, subject ending `(#226)`, no hard wrapping in bodies.
|
||||||
|
|
||||||
|
## Decisions
|
||||||
|
|
||||||
|
| Question | Decision | Why |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| Output format | Keep the input's format | Converting everything to WebP compresses better, but changes every stored filename's extension, which changes `item_images.image_path`, which turns the backfill into a rename plus a database migration with a window where rows point at files that no longer exist. The privacy fix does not need that risk. Revisit as its own issue if storage becomes the binding problem |
|
||||||
|
| Maximum dimension | 2000px on the longest side, never upscaled | Comfortably above what the storefront displays, and the point where a phone photo stops being several megabytes |
|
||||||
|
| Quality | 82 for JPEG and WebP | The usual point where further reduction starts being visible on photographs |
|
||||||
|
| PNG | Re-encoded, not converted | It still loses EXIF and still downscales. It will not shrink much, which is accepted — a PNG photograph is rare here |
|
||||||
|
| Where it runs | Inside `uploadImages`, after verification | That middleware is the single choke point every upload route already passes through, so a route added later inherits the behaviour rather than having to remember it. This is the same reasoning the file's own comment gives for `discardUnlessAccepted` |
|
||||||
|
| Backfill idempotency | Skip a file that already has no EXIF and is within bounds | Needs no schema change and no marker file, and is correct by construction: a file that is already stripped and already small needs nothing done to it, so a second run cannot degrade it with another lossy pass |
|
||||||
|
| Originals | Not kept | Retaining them keeps the coordinates on the same volume, one proxy misconfiguration from being public again — which is the exposure this exists to close |
|
||||||
|
|
||||||
|
## File Structure
|
||||||
|
|
||||||
|
**Created:**
|
||||||
|
- `backend/src/imageProcessing.ts` — the re-encode policy and the transform
|
||||||
|
- `backend/tests/unit/imageProcessing.test.ts`
|
||||||
|
- `backend/tests/integration/exifStripping.integration.test.ts`
|
||||||
|
- `backend/scripts/backfill-image-reencode.ts`
|
||||||
|
|
||||||
|
**Modified:**
|
||||||
|
- `backend/package.json` — `sharp` as a production dependency, plus a script entry
|
||||||
|
- `backend/src/routes/admin.ts` — the re-encode call inside `uploadImages`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 1: Add sharp and prove it installs where it has to run
|
||||||
|
|
||||||
|
`sharp` ships prebuilt binaries per platform. This repository builds on `node:20-bookworm-slim` (Debian, glibc) and runs on a Synology NAS, so the prebuild has to exist for the image's architecture. Finding out it does not during a production deploy is the failure this task exists to prevent.
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `backend/package.json`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Install it as a production dependency**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npm install sharp
|
||||||
|
```
|
||||||
|
|
||||||
|
Confirm it landed under `"dependencies"` and not `"devDependencies"` in `backend/package.json`. The final Docker stage installs with `--omit=dev`, so this being in the wrong section produces a container that fails on the first upload and nowhere else.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Check it loads**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && node -e "const s=require('sharp'); console.log(s.versions); console.log(process.arch, process.platform)"
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: a version object, not a build error.
|
||||||
|
|
||||||
|
Also confirm the installed major version is **0.33 or newer**:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && node -e "console.log(require('sharp/package.json').version)"
|
||||||
|
```
|
||||||
|
|
||||||
|
The integration tests in Task 3 build their fixture with `.withExif()`, which does not exist before 0.33 — on an older version those tests fail in a way that looks like the stripping is broken when it is the fixture that cannot be made.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Prove it works in the actual image**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker build -t redefined-sharp-check .
|
||||||
|
docker run --rm redefined-sharp-check node -e "require('sharp'); console.log('sharp loaded', process.arch)"
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: `sharp loaded` and the architecture. **If this fails**, the prebuild is missing for the target platform and the Dockerfile needs `build-essential`, `libvips-dev` and `python3` in the build stage. Do not proceed on a dev-machine success alone — that is a different platform from the one that serves.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Confirm the deployment target's architecture matches**
|
||||||
|
|
||||||
|
The image must run on the NAS, not only build on this machine. If the NAS is ARM and the image is x64 (or the reverse), that is a problem this repository has regardless of `sharp`, but `sharp` is where it will first become visible.
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker run --rm redefined-sharp-check node -e "console.log(process.arch)"
|
||||||
|
```
|
||||||
|
|
||||||
|
Compare against `uname -m` on the NAS. Record the result in the commit message.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/package.json backend/package-lock.json
|
||||||
|
git commit -m "build(uploads): add sharp for image re-encoding (#226)"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 2: The re-encode policy and transform
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Create: `backend/src/imageProcessing.ts`
|
||||||
|
- Test: `backend/tests/unit/imageProcessing.test.ts`
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Produces:
|
||||||
|
- `MAX_DIMENSION: number`, `QUALITY: number`
|
||||||
|
- `needsProcessing(meta: {width?: number; height?: number; exif?: unknown}): boolean` — pure
|
||||||
|
- `reencodeInPlace(filePath: string, mimetype: string): Promise<void>`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the failing unit test**
|
||||||
|
|
||||||
|
Create `backend/tests/unit/imageProcessing.test.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
import { needsProcessing, MAX_DIMENSION } from '../../src/imageProcessing';
|
||||||
|
|
||||||
|
// The whole of the skip/process policy, kept pure so the backfill's
|
||||||
|
// idempotency can be reasoned about without a filesystem. The backfill is
|
||||||
|
// lossy and irreversible, so being wrong here is expensive.
|
||||||
|
describe('needsProcessing', () => {
|
||||||
|
it('processes anything carrying EXIF, however small', () => {
|
||||||
|
expect(needsProcessing({ width: 10, height: 10, exif: Buffer.from('x') })).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('processes an oversized image even with no EXIF', () => {
|
||||||
|
expect(needsProcessing({ width: MAX_DIMENSION + 1, height: 100 })).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('processes an image oversized on either axis', () => {
|
||||||
|
expect(needsProcessing({ width: 100, height: MAX_DIMENSION + 1 })).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The idempotency property the backfill depends on: a file already stripped
|
||||||
|
// and already within bounds is left alone, so a second run cannot put it
|
||||||
|
// through another lossy pass.
|
||||||
|
it('leaves a stripped, in-bounds image alone', () => {
|
||||||
|
expect(needsProcessing({ width: MAX_DIMENSION, height: MAX_DIMENSION })).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Unknown dimensions mean sharp could not read it as an image. Processing is
|
||||||
|
// the safe answer: the alternative is skipping a file we understand least.
|
||||||
|
it('processes an image whose dimensions could not be read', () => {
|
||||||
|
expect(needsProcessing({})).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run it to verify it fails**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.unit.config.js imageProcessing
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: FAIL — module not found.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Write the implementation**
|
||||||
|
|
||||||
|
Create `backend/src/imageProcessing.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
import sharp from 'sharp';
|
||||||
|
import { promises as fs } from 'fs';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Rebuilding an uploaded image so it carries nothing but the picture.
|
||||||
|
*
|
||||||
|
* A file arrives as the camera wrote it, and a camera writes EXIF — which
|
||||||
|
* routinely includes the coordinates the photo was taken at. Those files are
|
||||||
|
* served publicly from /uploads/, so an unmodified product photo publishes the
|
||||||
|
* location it was taken. Nobody sending in a photograph of a vase expects that.
|
||||||
|
*
|
||||||
|
* The fix is to re-encode rather than to delete tags. Deleting requires knowing
|
||||||
|
* every tag that could carry something sensitive, across formats and camera
|
||||||
|
* makers, forever. Re-encoding builds a new file from the decoded pixels, so
|
||||||
|
* there is nothing left that could have been missed — the same reasoning that
|
||||||
|
* makes uploadTypes.ts an allowlist rather than a denylist.
|
||||||
|
*
|
||||||
|
* Format is deliberately preserved. Converting to WebP would compress better,
|
||||||
|
* but it changes stored extensions, and therefore item_images.image_path, and
|
||||||
|
* therefore turns the backfill into a rename with a window where rows point at
|
||||||
|
* files that no longer exist. See #226.
|
||||||
|
*/
|
||||||
|
|
||||||
|
/** Comfortably larger than anything the storefront renders. */
|
||||||
|
export const MAX_DIMENSION = 2000;
|
||||||
|
|
||||||
|
/** Where further reduction starts to show on a photograph. */
|
||||||
|
export const QUALITY = 82;
|
||||||
|
|
||||||
|
interface ImageFacts {
|
||||||
|
width?: number;
|
||||||
|
height?: number;
|
||||||
|
exif?: unknown;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether a file still needs rebuilding.
|
||||||
|
*
|
||||||
|
* Pure, and the backfill's entire idempotency argument: a file with no EXIF
|
||||||
|
* that is already within bounds is already in its final state, so a re-run
|
||||||
|
* skips it rather than putting it through a second lossy pass. Anything
|
||||||
|
* unreadable is processed rather than skipped — a file we cannot describe is
|
||||||
|
* not one to assume is safe.
|
||||||
|
*/
|
||||||
|
export function needsProcessing(meta: ImageFacts): boolean {
|
||||||
|
if (meta.exif !== undefined && meta.exif !== null) return true;
|
||||||
|
if (meta.width === undefined || meta.height === undefined) return true;
|
||||||
|
return meta.width > MAX_DIMENSION || meta.height > MAX_DIMENSION;
|
||||||
|
}
|
||||||
|
|
||||||
|
function encoderFor(instance: sharp.Sharp, mimetype: string): sharp.Sharp {
|
||||||
|
switch (mimetype) {
|
||||||
|
case 'image/jpeg':
|
||||||
|
return instance.jpeg({ quality: QUALITY });
|
||||||
|
case 'image/webp':
|
||||||
|
return instance.webp({ quality: QUALITY });
|
||||||
|
case 'image/png':
|
||||||
|
// PNG is lossless, so quality does not apply and this will not shrink
|
||||||
|
// much. It still strips EXIF and still bounds the dimensions, which are
|
||||||
|
// the two things being bought here.
|
||||||
|
return instance.png({ compressionLevel: 9 });
|
||||||
|
default:
|
||||||
|
// Unreachable: the allowlist in uploadTypes.ts is these three. Throwing
|
||||||
|
// rather than passing the file through unmodified, because "we did not
|
||||||
|
// recognise it so we left the metadata in" is the failure mode this
|
||||||
|
// module exists to make impossible.
|
||||||
|
throw new Error(`cannot re-encode unsupported type ${mimetype}`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Rewrites the file at `filePath`, in its own format, stripped and bounded.
|
||||||
|
*
|
||||||
|
* Writes to a sibling temporary file and renames over the original, because
|
||||||
|
* writing in place would leave a half-written image being served if the process
|
||||||
|
* died mid-write — and sharp cannot read and write the same path in one pass
|
||||||
|
* anyway.
|
||||||
|
*
|
||||||
|
* `withoutEnlargement` so a small image is not blown up to the cap: the ceiling
|
||||||
|
* is a maximum, not a target.
|
||||||
|
*/
|
||||||
|
export async function reencodeInPlace(filePath: string, mimetype: string): Promise<void> {
|
||||||
|
const temporary = `${filePath}.reencoding`;
|
||||||
|
try {
|
||||||
|
await encoderFor(
|
||||||
|
// `animated` only for WebP, which is the one allowed type that can carry
|
||||||
|
// more than one frame. Reading an animated WebP without it decodes the
|
||||||
|
// first frame alone and silently writes back a still — destroying the
|
||||||
|
// uploader's image while reporting success. It is not set unconditionally
|
||||||
|
// because it changes how `resize` interprets height (the full frame
|
||||||
|
// strip, not one frame), which would be wrong for the other two.
|
||||||
|
sharp(filePath, mimetype === 'image/webp' ? { animated: true } : {}).resize({
|
||||||
|
width: MAX_DIMENSION,
|
||||||
|
height: MAX_DIMENSION,
|
||||||
|
fit: 'inside',
|
||||||
|
withoutEnlargement: true
|
||||||
|
}),
|
||||||
|
mimetype
|
||||||
|
// No withMetadata(): omitting it is what drops EXIF, ICC and everything
|
||||||
|
// else. Calling it would put the metadata back.
|
||||||
|
).toFile(temporary);
|
||||||
|
|
||||||
|
await fs.rename(temporary, filePath);
|
||||||
|
} catch (err) {
|
||||||
|
await fs.unlink(temporary).catch(() => undefined);
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 4: Run the test to verify it passes**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.unit.config.js imageProcessing
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: PASS, 5 tests.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/src/imageProcessing.ts backend/tests/unit/imageProcessing.test.ts
|
||||||
|
git commit -m "feat(uploads): re-encode images to strip metadata and bound dimensions (#226)"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 3: Apply it to every upload
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `backend/src/routes/admin.ts:183-211` (the `uploadImages` middleware)
|
||||||
|
- Test: `backend/tests/integration/exifStripping.integration.test.ts`
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Consumes: `reencodeInPlace` from `src/imageProcessing`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the failing integration test**
|
||||||
|
|
||||||
|
Create `backend/tests/integration/exifStripping.integration.test.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
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 } }
|
||||||
|
})
|
||||||
|
.withExif({
|
||||||
|
IFD0: { Make: 'TestCam', Model: 'X1' },
|
||||||
|
GPS: { GPSLatitudeRef: 'N', GPSLatitude: '51/1 30/1 0/1', GPSLongitudeRef: 'W' }
|
||||||
|
})
|
||||||
|
.jpeg()
|
||||||
|
.toBuffer();
|
||||||
|
}
|
||||||
|
|
||||||
|
async function storedPathFor(itemId: number): Promise<string> {
|
||||||
|
const { rows } = await pool.query(
|
||||||
|
`SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`,
|
||||||
|
[itemId]
|
||||||
|
);
|
||||||
|
// image_path is '/uploads/<name>'; the file is that name inside UPLOADS_DIR.
|
||||||
|
return path.join(UPLOADS_DIR, path.basename(rows[0].image_path));
|
||||||
|
}
|
||||||
|
|
||||||
|
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 res = await request(app)
|
||||||
|
.post('/api/admin/items')
|
||||||
|
.field('name', 'Vase')
|
||||||
|
.field('description', '')
|
||||||
|
.field('price', '40')
|
||||||
|
.attach('images', withGps, 'vase.jpg');
|
||||||
|
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
|
||||||
|
const stored = await sharp(await storedPathFor(res.body.id)).metadata();
|
||||||
|
expect(stored.exif).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('is bounded to the maximum dimension', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/items')
|
||||||
|
.field('name', 'Vase')
|
||||||
|
.field('description', '')
|
||||||
|
.field('price', '40')
|
||||||
|
.attach('images', await photoWithLocation(), 'vase.jpg');
|
||||||
|
|
||||||
|
const stored = await sharp(await storedPathFor(res.body.id)).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 res = await request(app)
|
||||||
|
.post('/api/admin/items')
|
||||||
|
.field('name', 'Vase')
|
||||||
|
.field('description', '')
|
||||||
|
.field('price', '40')
|
||||||
|
.attach('images', await photoWithLocation(), 'vase.jpg');
|
||||||
|
|
||||||
|
const storedPath = await storedPathFor(res.body.id);
|
||||||
|
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 res = await request(app)
|
||||||
|
.post('/api/admin/items')
|
||||||
|
.field('name', 'Tiny')
|
||||||
|
.field('description', '')
|
||||||
|
.field('price', '5')
|
||||||
|
.attach('images', small, 'tiny.png');
|
||||||
|
|
||||||
|
const stored = await sharp(await storedPathFor(res.body.id)).metadata();
|
||||||
|
expect(stored.width).toBe(300);
|
||||||
|
expect(stored.height).toBe(200);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run it to verify it fails**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.integration.config.js --runInBand exifStripping
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: FAIL — `stored.exif` is defined, and the width is 3000.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Wire it into the middleware**
|
||||||
|
|
||||||
|
In `backend/src/routes/admin.ts`, add the import:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
import { reencodeInPlace } from '../imageProcessing';
|
||||||
|
```
|
||||||
|
|
||||||
|
Add this beside `verifyUploadedImages`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
/**
|
||||||
|
* 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 its coordinates is the case
|
||||||
|
* nobody was told about.
|
||||||
|
*/
|
||||||
|
async function stripUploadedImages(req: Request): Promise<string | null> {
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Then extend the chain at the end of `uploadImages` — this is the single choke point every upload route passes through, so this covers the admin routes and any route added later:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
verifyUploadedImages(req)
|
||||||
|
.then((problem) => {
|
||||||
|
if (problem) {
|
||||||
|
res.status(400).json({ error: problem });
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
return stripUploadedImages(req);
|
||||||
|
})
|
||||||
|
.then((problem) => {
|
||||||
|
// null from either stage means "already answered" or "nothing wrong";
|
||||||
|
// distinguish by whether a response has gone out.
|
||||||
|
if (res.headersSent) return;
|
||||||
|
if (problem) {
|
||||||
|
res.status(400).json({ error: problem });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
next();
|
||||||
|
})
|
||||||
|
.catch(next);
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 4: Run the tests**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend
|
||||||
|
npx jest -c jest.integration.config.js --runInBand exifStripping
|
||||||
|
npm run test:unit
|
||||||
|
npm run test:integration
|
||||||
|
npm run lint && npm run build
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: PASS throughout. `uploadValidation` and `adminInventory` matter most — they exercise this middleware hardest, and the refusal paths must still refuse with the same messages.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/src/routes/admin.ts backend/tests/integration/exifStripping.integration.test.ts
|
||||||
|
git commit -m "feat(uploads): strip metadata from every accepted upload (#226)"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 4: Backfill the photos already stored
|
||||||
|
|
||||||
|
Task 3 does nothing for the existing catalogue. Without this, every photo on the storefront today keeps its coordinates.
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Create: `backend/scripts/backfill-image-reencode.ts`
|
||||||
|
- Modify: `backend/package.json` (script entry)
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the script**
|
||||||
|
|
||||||
|
Create `backend/scripts/backfill-image-reencode.ts`:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
/**
|
||||||
|
* Applies #226's re-encoding to the photos that were stored before it existed.
|
||||||
|
*
|
||||||
|
* New uploads are handled in the request path. Everything already on the volume
|
||||||
|
* still carries whatever the camera wrote, including the coordinates the photo
|
||||||
|
* was taken at, and is still served publicly. This is the other half.
|
||||||
|
*
|
||||||
|
* The transform is lossy and there is no undo, so:
|
||||||
|
*
|
||||||
|
* - It reports by default and changes nothing without --apply.
|
||||||
|
* - It is idempotent. `needsProcessing` skips a file that is already stripped
|
||||||
|
* and already within bounds, so a second run is not a second lossy pass.
|
||||||
|
* - It writes to a temporary file and renames, so an interruption cannot
|
||||||
|
* leave a half-written image being served.
|
||||||
|
* - It never renames the stored file, so item_images.image_path stays correct
|
||||||
|
* and no database write is needed at all.
|
||||||
|
*
|
||||||
|
* Usage
|
||||||
|
* -----
|
||||||
|
* npm run backfill:images # report only
|
||||||
|
* npm run backfill:images -- --apply # rewrite the files
|
||||||
|
*
|
||||||
|
* Point it at QA first. Compare a handful of images by eye before production,
|
||||||
|
* and take a backup that you have confirmed restores.
|
||||||
|
*/
|
||||||
|
|
||||||
|
import sharp from 'sharp';
|
||||||
|
import { promises as fs } from 'fs';
|
||||||
|
import path from 'path';
|
||||||
|
import { pool } from '../src/db';
|
||||||
|
import { needsProcessing, reencodeInPlace } from '../src/imageProcessing';
|
||||||
|
|
||||||
|
const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads';
|
||||||
|
const APPLY = process.argv.includes('--apply');
|
||||||
|
|
||||||
|
// The stored extension is the file's real type — uploadTypes.ts derives it from
|
||||||
|
// the validated content type on the way in, so it can be trusted on the way
|
||||||
|
// back out. Anything else is a file this application would refuse to serve.
|
||||||
|
const TYPE_FOR_EXTENSION: Record<string, string> = {
|
||||||
|
'.jpg': 'image/jpeg',
|
||||||
|
'.png': 'image/png',
|
||||||
|
'.webp': 'image/webp'
|
||||||
|
};
|
||||||
|
|
||||||
|
interface Totals {
|
||||||
|
seen: number;
|
||||||
|
missing: number;
|
||||||
|
skipped: number;
|
||||||
|
processed: number;
|
||||||
|
failed: number;
|
||||||
|
bytesBefore: number;
|
||||||
|
bytesAfter: number;
|
||||||
|
}
|
||||||
|
|
||||||
|
async function run(): Promise<void> {
|
||||||
|
const totals: Totals = {
|
||||||
|
seen: 0, missing: 0, skipped: 0, processed: 0, failed: 0, bytesBefore: 0, bytesAfter: 0
|
||||||
|
};
|
||||||
|
|
||||||
|
const { rows } = await pool.query<{ image_path: string }>(
|
||||||
|
`SELECT image_path FROM item_images ORDER BY id`
|
||||||
|
);
|
||||||
|
|
||||||
|
console.info(`[backfill] ${rows.length} image rows, ${APPLY ? 'APPLYING' : 'reporting only'}`);
|
||||||
|
|
||||||
|
for (const row of rows) {
|
||||||
|
totals.seen++;
|
||||||
|
const filePath = path.join(UPLOADS_DIR, path.basename(row.image_path));
|
||||||
|
const mimetype = TYPE_FOR_EXTENSION[path.extname(filePath).toLowerCase()];
|
||||||
|
|
||||||
|
if (!mimetype) {
|
||||||
|
console.warn(`[backfill] unrecognised extension, skipping: ${row.image_path}`);
|
||||||
|
totals.skipped++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
let before: number;
|
||||||
|
try {
|
||||||
|
before = (await fs.stat(filePath)).size;
|
||||||
|
} catch {
|
||||||
|
// A row pointing at nothing is a pre-existing inconsistency. Reported
|
||||||
|
// rather than fatal: it is not this script's job to fix, and stopping
|
||||||
|
// would leave the rest of the catalogue exposed.
|
||||||
|
console.warn(`[backfill] file missing for ${row.image_path}`);
|
||||||
|
totals.missing++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
const meta = await sharp(filePath).metadata();
|
||||||
|
if (!needsProcessing(meta)) {
|
||||||
|
totals.skipped++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!APPLY) {
|
||||||
|
console.info(
|
||||||
|
`[backfill] would process ${row.image_path} ` +
|
||||||
|
`(${meta.width}x${meta.height}, exif ${meta.exif ? 'present' : 'absent'}, ${before} bytes)`
|
||||||
|
);
|
||||||
|
totals.processed++;
|
||||||
|
totals.bytesBefore += before;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
await reencodeInPlace(filePath, mimetype);
|
||||||
|
const after = (await fs.stat(filePath)).size;
|
||||||
|
totals.processed++;
|
||||||
|
totals.bytesBefore += before;
|
||||||
|
totals.bytesAfter += after;
|
||||||
|
console.info(`[backfill] ${row.image_path}: ${before} -> ${after} bytes`);
|
||||||
|
} catch (err) {
|
||||||
|
console.error(`[backfill] failed on ${row.image_path}:`, err);
|
||||||
|
totals.failed++;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
console.info('[backfill] done', totals);
|
||||||
|
if (totals.failed > 0) {
|
||||||
|
// A non-zero exit so a partial run is visible to whatever invoked it,
|
||||||
|
// rather than reading as success because the summary printed.
|
||||||
|
process.exitCode = 1;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
run()
|
||||||
|
.catch((err) => {
|
||||||
|
console.error(err);
|
||||||
|
process.exitCode = 1;
|
||||||
|
})
|
||||||
|
.finally(() => pool.end());
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Add the script entry**
|
||||||
|
|
||||||
|
In `backend/package.json`, beside `bench:hashing`:
|
||||||
|
|
||||||
|
```json
|
||||||
|
"backfill:images": "tsx scripts/backfill-image-reencode.ts",
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Prove it on a local database first**
|
||||||
|
|
||||||
|
Bring up the stack, add an item through `/admin` with a photo, then — because Task 3 now strips on upload — put a file carrying EXIF on the volume by hand so there is something to find:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend
|
||||||
|
node -e "const sharp=require('sharp');const p=process.env.UPLOADS_DIR;sharp({create:{width:3000,height:2000,channels:3,background:{r:1,g:2,b:3}}}).withExif({IFD0:{Make:'TestCam'}}).jpeg().toFile(require('path').join(p,'legacy-test.jpg')).then(()=>console.log('written'))"
|
||||||
|
```
|
||||||
|
|
||||||
|
Point an `item_images` row at it — reusing an item you already created, so the row is otherwise ordinary:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
psql -h localhost -p 55432 -U redefined_test -d redefined_test -c \
|
||||||
|
"INSERT INTO item_images (item_id, image_path, sort_order) \
|
||||||
|
SELECT id, '/uploads/legacy-test.jpg', 99 FROM items LIMIT 1"
|
||||||
|
```
|
||||||
|
|
||||||
|
Then:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
npm run backfill:images
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: it reports `would process /uploads/legacy-test.jpg` and changes nothing. Confirm the file's bytes are unchanged.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Apply, then prove idempotency**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend
|
||||||
|
npm run backfill:images -- --apply
|
||||||
|
npm run backfill:images -- --apply
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: the first run processes the file and reports a smaller size; **the second run skips it**. That second run is the whole idempotency property — if it processes again, `needsProcessing` is wrong and the script will degrade images a little more on every run.
|
||||||
|
|
||||||
|
Then confirm the metadata is gone:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
node -e "require('sharp')(process.env.UPLOADS_DIR+'/legacy-test.jpg').metadata().then(m=>console.log({exif:m.exif,width:m.width}))"
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: `exif: undefined`, `width: 2000`.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/scripts/backfill-image-reencode.ts backend/package.json
|
||||||
|
git commit -m "feat(uploads): backfill re-encoding over already-stored photos (#226)"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 5: Deploy, QA first
|
||||||
|
|
||||||
|
Not code. This is the part where the irreversible thing happens to real images, and it is written down so it is not improvised.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Deploy to QA and check new uploads**
|
||||||
|
|
||||||
|
Deploy the built image to QA. Upload a photo with EXIF through `/admin`, then fetch it from `/uploads/` and confirm the metadata is gone and the image still looks right.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Report on QA's existing images**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker exec <qa-container> npm run backfill:images
|
||||||
|
```
|
||||||
|
|
||||||
|
Read the report. The count of files it would process should be plausible against the size of QA's catalogue — a count of zero, or a count far larger than the catalogue, both mean something is wrong with the path resolution rather than with the images.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Apply on QA, then look at them**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker exec <qa-container> npm run backfill:images -- --apply
|
||||||
|
```
|
||||||
|
|
||||||
|
Open the QA storefront and look at several items, including a detail view. This is the only step that checks the thing no assertion covers: whether the images still look good enough to sell from.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Back up production and confirm the backup restores**
|
||||||
|
|
||||||
|
Take the backup. **Confirm it restores** — an untested backup is not a backup, and this is the step whose omission cannot be recovered from. Back up the uploads volume, not only the database: the database rows are unchanged by this script, and the files are the only thing at risk.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Report, then apply, on production**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
docker exec <prod-container> npm run backfill:images
|
||||||
|
docker exec <prod-container> npm run backfill:images -- --apply
|
||||||
|
```
|
||||||
|
|
||||||
|
Read the report before applying. Then spot-check the live storefront.
|
||||||
|
|
||||||
|
- [ ] **Step 6: Confirm the exposure is closed**
|
||||||
|
|
||||||
|
Pick an image URL from the live storefront, download it, and read its metadata:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
curl -s https://<host>/uploads/<name>.jpg -o /tmp/check.jpg
|
||||||
|
node -e "require('sharp')('/tmp/check.jpg').metadata().then(m=>console.log(m.exif))"
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: `undefined`. That is the assertion this whole issue exists to be able to make.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Done when
|
||||||
|
|
||||||
|
- A photo uploaded through `/admin` is stored with no EXIF, bounded to 2000px, in its original format.
|
||||||
|
- A small image is not enlarged.
|
||||||
|
- An upload that cannot be re-encoded is refused rather than stored unmodified.
|
||||||
|
- `npm run backfill:images` reports without changing anything; `--apply` processes; a second `--apply` skips everything it already did.
|
||||||
|
- Production images fetched over HTTP carry no EXIF.
|
||||||
|
- `npm run test:unit`, `npm run test:integration`, `npm run lint` and `npm run build` all pass in `backend/`.
|
||||||
|
|
||||||
|
## Not in this issue
|
||||||
|
|
||||||
|
Converting formats for better compression — that changes stored extensions and `item_images.image_path`, and belongs in its own issue if storage becomes the binding constraint. Thumbnails and responsive sizes. Any change to what the storefront requests.
|
||||||
Reference in New Issue
Block a user