feat(uploads): re-encode images to strip metadata and bound dimensions (#226)

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. The same reasoning that makes uploadTypes.ts an allowlist rather than a denylist.

`needsProcessing` is pure and separately tested because it is the whole of the backfill's idempotency argument: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Being wrong there degrades every image a little more on every run. Anything sharp cannot describe is processed rather than skipped, since a file we understand least is not one to assume is safe.

Verified end to end on a real image before wiring anything up: 3000x2000 with EXIF present became 2000x1333 with EXIF absent, and no temporary file was left behind.

Corrects something this README claimed an hour ago. Installing under a Node below 20.9.0 does produce a broken sharp, because npm skips the optional platform binary when the engine check fails and still reports success. But once that binary is present sharp loads and runs fine on 18.16.1 — `engines` is enforced at install time, not at require time. The README said the runtime was blocked, which would have sent someone switching Node versions to fix a problem that only the install created.

Ref #226

README.md

@@ -43,7 +43,9 @@ Both scripts run `nvm use latest` first and verify the result is Node 20 or newe
 
 The Node 20 floor is not arbitrary: `node-pg-migrate` pulls in an `lru-cache` that calls `diagnostics_channel.tracingChannel()`, which does not exist before Node 19.9. On Node 18 migrations die inside minified library code with `(0 , U.tracingChannel) is not a function`, which says nothing about versions.
 
-Since #226 the floor also applies to `npm install`, and it fails in a nastier way. `sharp` declares `>=20.9.0`, and its platform binary — the part that does the actual work — is an **optional** dependency. npm skips an optional dependency whose engine check fails, and reports success. So installing on the machine's default 18.16.1 produces a `node_modules` that looks complete and then throws `Could not load the "sharp" module using the win32-x64 runtime` at require time, which reads as a broken package rather than as a wrong Node version. `backend/package.json` now declares `engines` so npm at least warns; installing through `start-local.ps1` or `run-tests.ps1` avoids it entirely, because both switch first. If you hit it, `npm install --include=optional sharp` under Node 20+ repairs it.
+Since #226 there is a second Node floor, and it bites at **install** time rather than at run time. `sharp` declares `>=20.9.0`, and the platform binary that does its actual work is an **optional** dependency. npm silently skips an optional dependency whose engine check fails and still reports success — so `npm install` on the machine's default 18.16.1 produces a `node_modules` that looks complete and then throws `Could not load the "sharp" module using the win32-x64 runtime` at require time. That message names a runtime rather than a version and sends you looking in the wrong place.
+
+Once the binary is installed, sharp loads and runs perfectly well on 18.16.1 — `engines` is advisory at run time. So this is purely about how the install was done, not about which Node runs the tests. Install through `start-local.ps1` or `run-tests.ps1`, which switch to Node 20+ first; if you have already hit it, `npm install --include=optional sharp` under Node 20+ repairs it in place.
 
 `run-tests.ps1` brings up whatever a suite needs: the integration suite gets its own throwaway Postgres, started and stopped around the run (`-KeepTestDb` leaves it up, `-TestDbPort` moves it if the default is taken or Hyper-V has reserved it). The e2e suite needs the app stack, so start it with `start-local.ps1` first — the script checks and says so rather than letting every spec fail on a refused connection. `-Filter` passes through to the runner to select tests by file or name.
 

backend/src/imageProcessing.ts

@@ -0,0 +1,108 @@
+import sharp, { type 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, mimetype: string): 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;
+  }
+}

backend/tests/unit/imageProcessing.test.ts

@@ -0,0 +1,31 @@
+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);
+  });
+});