diff --git a/README.md b/README.md index 8a766fc..f1b308b 100755 --- a/README.md +++ b/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. diff --git a/backend/src/imageProcessing.ts b/backend/src/imageProcessing.ts new file mode 100644 index 0000000..0314fb2 --- /dev/null +++ b/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 { + 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; + } +} diff --git a/backend/tests/unit/imageProcessing.test.ts b/backend/tests/unit/imageProcessing.test.ts new file mode 100644 index 0000000..849879a --- /dev/null +++ b/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); + }); +});