feat(backend): accept only real images in the inventory upload (#95)

The upload bounded size and count and nothing else: POST /api/admin/items would take a PDF, a zip or an executable and store it as an item image, under an extension copied from whatever the caller named their file. Those files are served by express.static from the application's own origin, so a stored .html came back as text/html and a .svg as image/svg+xml — both able to run script as the site.

Three types are accepted: JPEG, PNG and WebP. SVG is excluded deliberately even though it is an image, because it executes script when navigated to directly, which is the exposure #103 describes; a photograph of a one-of-a-kind item is never a vector drawing, so nothing real is lost. GIF is excluded as simply not wanted for product stills.

Validation happens twice, because once is not enough. The declared content type is checked in multer's fileFilter, before a byte is written — that catches picking a PDF by accident, which is most of what goes wrong. But file.mimetype is whatever the caller wrote in the multipart headers, so the bytes are checked too: each stored file's leading bytes must match the format it claimed. That is what stops evil.html renamed to photo.jpg and declared image/jpeg, which an allowlist on the declared type alone waves straight through.

The byte check cannot live in fileFilter — that runs before multer has read the stream, so there is nothing to look at yet. It runs after the write instead, and a failure removes every file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows.

The stored name now takes its extension from the validated type rather than from path.extname(file.originalname), so the name on disk cannot disagree with what the file is. The random UUID is unchanged — that was already right, and its comment explains why.

The picker offers exactly those three types rather than image/*, so a choice the API will refuse is not on the menu in the first place. That is a convenience, not a control: the operating system's All files option remains, drag-and-drop ignores accept, and anything calling the API directly never sees it. The server is the control.

Nine integration tests, and they are the first in this project to upload real file content — which is why none of this was noticed. They cover a genuine PNG accepted, a PDF refused, SVG refused, HTML wearing image/jpeg refused, nothing left on the volume after a refusal, a mixed request discarding its valid file too, and no item created when the upload fails. Plus 21 unit tests on the pure signature checks, including a RIFF container that is not WebP.

Verified: 162 unit, 178 integration, 94 end-to-end on a fresh container. Backend lint holds at 4 warnings — it caught the now-unused path import, which is exactly what it is for.

Refs #95
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/routes/admin.ts

@@ -1,6 +1,6 @@
 import { Router, Request, Response, NextFunction } from 'express';
 import multer from 'multer';
-import path from 'path';
+import { promises as fs } from 'fs';
 import { randomUUID } from 'crypto';
 import { PoolClient } from 'pg';
 import { pool } from '../db';
@@ -8,6 +8,13 @@ import { ADMIN_ITEM_SELECT } from '../itemSelect';
 import { asyncRoute } from '../asyncRoute';
 import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters';
 import { tagColorFor } from '../utils';
+import {
+  ALLOWED_IMAGE_TYPES,
+  SIGNATURE_BYTES,
+  extensionFor,
+  isAllowedImageType,
+  signatureMatches
+} from '../uploadTypes';
 import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts';
 
 const router = Router();
@@ -25,13 +32,30 @@ const MAX_IMAGE_BYTES = 8_000_000;
 const MAX_TEXT_FIELDS = 8;
 const MAX_TEXT_FIELD_BYTES = 64 * 1024;
 
+// Refused before a byte is written. This catches the honest mistake — picking a
+// PDF by accident — and nothing more, because file.mimetype is whatever the
+// caller wrote in the multipart headers. The bytes are checked after the write;
+// see verifyUploadedImages.
+class UnsupportedImageTypeError extends Error {}
+
 const storage = multer.diskStorage({
   destination: UPLOADS_DIR,
   // Stored names come from a CSPRNG rather than a timestamp plus Math.random,
   // which is predictable enough that a caller could guess (or collide with)
   // another upload's path.
+  //
+  // The extension comes from the validated content type rather than from
+  // path.extname(file.originalname), so the name on disk cannot disagree with
+  // what the file claims to be — a caller cannot get `.html` onto the uploads
+  // volume by naming their file that way.
   filename: (_req, file, cb) => {
-    const ext = path.extname(file.originalname);
+    const ext = extensionFor(file.mimetype);
+    if (!ext) {
+      // Unreachable while fileFilter runs first, and here so that it stays
+      // unreachable rather than silently writing a file with no extension.
+      cb(new UnsupportedImageTypeError(`unsupported image type ${file.mimetype}`), '');
+      return;
+    }
     cb(null, `${randomUUID()}${ext}`);
   }
 });
@@ -43,18 +67,93 @@ const upload = multer({
     files: MAX_IMAGES_PER_REQUEST,
     fields: MAX_TEXT_FIELDS,
     fieldSize: MAX_TEXT_FIELD_BYTES
+  },
+  fileFilter: (_req, file, cb) => {
+    if (!isAllowedImageType(file.mimetype)) {
+      cb(new UnsupportedImageTypeError(
+        `${file.mimetype} is not an accepted image type — allowed: ${ALLOWED_IMAGE_TYPES.join(', ')}`
+      ));
+      return;
+    }
+    cb(null, true);
   }
 });
 
+// Reads only the leading bytes — enough to identify a format, not enough to
+// care how large the file is. The handle is closed before anything is unlinked,
+// because an open handle makes the unlink fail on Windows.
+async function readHead(filePath: string): Promise<Buffer> {
+  const handle = await fs.open(filePath, 'r');
+  try {
+    const buffer = Buffer.alloc(SIGNATURE_BYTES);
+    const { bytesRead } = await handle.read(buffer, 0, SIGNATURE_BYTES, 0);
+    return buffer.subarray(0, bytesRead);
+  } finally {
+    await handle.close();
+  }
+}
+
+// Best effort: a file that cannot be removed should not turn a 400 into a 500,
+// but it must not be left behind quietly either.
+async function discardUploads(files: Express.Multer.File[]): Promise<void> {
+  await Promise.all(
+    files.map((file) =>
+      fs.unlink(file.path).catch((err: unknown) => {
+        console.error(`[upload] could not remove rejected file ${file.path}:`, err);
+      })
+    )
+  );
+}
+
+/**
+ * Confirms each stored file actually is what it was declared to be.
+ *
+ * This cannot happen in multer's fileFilter, which runs before the stream has
+ * been read — there are no bytes to look at yet. So the check runs after the
+ * write, and a failure removes *every* file from the request rather than only
+ * the offending one: a half-accepted upload would leave files on the volume
+ * that the request was refused for.
+ *
+ * Returns the message to refuse with, or null when everything checks out.
+ */
+async function verifyUploadedImages(req: Request): Promise<string | null> {
+  const files = (req.files as Express.Multer.File[]) || [];
+
+  for (const file of files) {
+    const head = await readHead(file.path);
+    if (!signatureMatches(file.mimetype, head)) {
+      await discardUploads(files);
+      return `${file.originalname} does not contain ${file.mimetype} data`;
+    }
+  }
+
+  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) => {
   upload.array('images', MAX_IMAGES_PER_REQUEST)(req, res, (err: unknown) => {
+    if (err instanceof UnsupportedImageTypeError) {
+      return res.status(400).json({ error: err.message });
+    }
     if (err instanceof multer.MulterError) {
       const status = err.code === 'LIMIT_FILE_SIZE' ? 413 : 400;
       return res.status(status).json({ error: err.message });
     }
+    if (err) {
       return next(err);
+    }
+
+    verifyUploadedImages(req)
+      .then((problem) => {
+        if (problem) {
+          res.status(400).json({ error: problem });
+          return;
+        }
+        next();
+      })
+      .catch(next);
   });
 };
 

backend/src/uploadTypes.ts

@@ -0,0 +1,90 @@
+/**
+ * What the inventory upload will accept, and how to tell whether a file is
+ * actually what it says it is.
+ *
+ * Kept apart from the route so the rules are pure and can be tested directly.
+ * A mistake here is not a cosmetic one: uploads are served by express.static
+ * from the application's own origin, so a file that gets through and is later
+ * navigated to runs as same-origin content. See #95 and #103.
+ */
+
+/**
+ * Deliberately three types, not `image/*`.
+ *
+ * SVG is excluded even though it is an image: it can carry script that executes
+ * when the file is navigated to directly, which is precisely the exposure #103
+ * describes. A photograph of a one-of-a-kind item is never a vector drawing, so
+ * nothing real is lost.
+ *
+ * GIF is excluded as simply not wanted for product stills rather than for any
+ * security reason. Adding it later means adding its signature below too.
+ *
+ * The frontend's `accept` attribute lists these same three so the file picker
+ * offers exactly what the server will take. The list unavoidably exists in two
+ * runtimes; if it changes here, change it there.
+ */
+export const ALLOWED_IMAGE_TYPES: readonly string[] = ['image/jpeg', 'image/png', 'image/webp'];
+
+/**
+ * How many bytes of a file are needed to check any signature below. WebP is the
+ * longest reach: it needs byte 8 onwards.
+ */
+export const SIGNATURE_BYTES = 12;
+
+const EXTENSION_FOR_TYPE: Readonly<Record<string, string>> = {
+  'image/jpeg': '.jpg',
+  'image/png': '.png',
+  'image/webp': '.webp'
+};
+
+export function isAllowedImageType(mimetype: string): boolean {
+  return ALLOWED_IMAGE_TYPES.includes(mimetype);
+}
+
+/**
+ * The extension a stored file should carry, derived from its validated type.
+ *
+ * Returns null for anything unrecognised so a caller has to handle it, rather
+ * than defaulting to an empty string and writing a file with no extension at
+ * all. The stored name comes from this instead of from the submitted filename,
+ * so the name on disk cannot disagree with what the file is.
+ */
+export function extensionFor(mimetype: string): string | null {
+  return EXTENSION_FOR_TYPE[mimetype] ?? null;
+}
+
+function startsWithBytes(head: Buffer, offset: number, expected: readonly number[]): boolean {
+  if (head.length < offset + expected.length) {
+    return false;
+  }
+  return expected.every((byte, index) => head[offset + index] === byte);
+}
+
+const ASCII_RIFF = [0x52, 0x49, 0x46, 0x46];
+const ASCII_WEBP = [0x57, 0x45, 0x42, 0x50];
+
+/**
+ * Whether a file's leading bytes agree with the content type it was declared as.
+ *
+ * `file.mimetype` comes from the client's multipart headers and is whatever the
+ * caller chose to write there, so the allowlist alone stops honest mistakes and
+ * nothing else. This is what stops `evil.html` renamed to `photo.jpg` and sent
+ * as `image/jpeg`.
+ *
+ * Fails closed on a short read and on any type not in the allowlist, so a
+ * truncated file or an unexpected type is refused rather than assumed fine.
+ */
+export function signatureMatches(mimetype: string, head: Buffer): boolean {
+  switch (mimetype) {
+    case 'image/jpeg':
+      return startsWithBytes(head, 0, [0xff, 0xd8, 0xff]);
+    case 'image/png':
+      return startsWithBytes(head, 0, [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]);
+    // A RIFF container is not necessarily a WebP — a .wav opens the same way —
+    // so both the container marker and the format marker are checked.
+    case 'image/webp':
+      return startsWithBytes(head, 0, ASCII_RIFF) && startsWithBytes(head, 8, ASCII_WEBP);
+    default:
+      return false;
+  }
+}

backend/tests/integration/uploadValidation.integration.test.ts

@@ -0,0 +1,155 @@
+import request from 'supertest';
+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;
+
+// A genuine 1x1 PNG, so the accepted case exercises the whole path rather than
+// a buffer that merely starts with the right bytes.
+const REAL_PNG = Buffer.from(
+  'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
+  'base64'
+);
+const HTML_BYTES = Buffer.from('<!doctype html><script>alert(1)</script>', 'utf8');
+const PDF_BYTES = Buffer.from('%PDF-1.4\n%����\n', 'binary');
+
+// No test has ever attached a file before this one, so the uploads directory
+// has never had to exist. multer.diskStorage does not create its destination.
+beforeAll(async () => {
+  await fs.mkdir(UPLOADS_DIR, { recursive: true });
+});
+
+beforeEach(async () => {
+  await resetDb();
+});
+
+afterAll(async () => {
+  await pool.end();
+  await closeDb();
+});
+
+async function storedFiles(): Promise<string[]> {
+  return fs.readdir(UPLOADS_DIR);
+}
+
+function createItem() {
+  return request(app)
+    .post('/api/admin/items')
+    .field('name', 'Photographed thing')
+    .field('description', '')
+    .field('price', '30');
+}
+
+describe('what the inventory upload accepts', () => {
+  it('accepts a real PNG and stores it', async () => {
+    const res = await createItem().attach('images', REAL_PNG, {
+      filename: 'photo.png',
+      contentType: 'image/png'
+    });
+
+    expect(res.status).toBe(200);
+    expect(res.body.images).toHaveLength(1);
+    expect(res.body.images[0].image_path).toMatch(/^\/uploads\/[0-9a-f-]+\.png$/);
+  });
+
+  // The stored name is derived from the validated type, so a caller cannot put
+  // a .html path onto a volume that express.static serves from the app's own
+  // origin just by naming their file that way. See #103.
+  it('names the stored file from its type, not from the submitted filename', async () => {
+    const res = await createItem().attach('images', REAL_PNG, {
+      filename: 'evil.html',
+      contentType: 'image/png'
+    });
+
+    expect(res.status).toBe(200);
+    expect(res.body.images[0].image_path).toMatch(/\.png$/);
+    expect(res.body.images[0].image_path).not.toContain('html');
+  });
+});
+
+describe('what it refuses', () => {
+  it('refuses a type that is not an accepted image, naming what is', async () => {
+    const res = await createItem().attach('images', PDF_BYTES, {
+      filename: 'brochure.pdf',
+      contentType: 'application/pdf'
+    });
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('application/pdf');
+    expect(res.body.error).toContain('image/png');
+  });
+
+  // SVG is an image and is deliberately not accepted, because it executes
+  // script when navigated to directly.
+  it('refuses SVG even though it is an image', async () => {
+    const res = await createItem().attach('images', Buffer.from('<svg xmlns="..."/>'), {
+      filename: 'logo.svg',
+      contentType: 'image/svg+xml'
+    });
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('image/svg+xml');
+  });
+
+  // The case an allowlist on the declared type alone waves straight through.
+  it('refuses a document wearing an image content type', async () => {
+    const res = await createItem().attach('images', HTML_BYTES, {
+      filename: 'photo.jpg',
+      contentType: 'image/jpeg'
+    });
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('does not contain');
+  });
+
+  it('leaves nothing on the volume when it refuses the content', async () => {
+    const before = await storedFiles();
+
+    await createItem().attach('images', HTML_BYTES, {
+      filename: 'photo.jpg',
+      contentType: 'image/jpeg'
+    });
+
+    expect(await storedFiles()).toHaveLength(before.length);
+  });
+
+  // A rejection must take the whole request with it. Accepting the good file
+  // from a refused upload would leave a file behind that nothing references.
+  it('discards the valid files from a request that also carried an invalid one', async () => {
+    const before = await storedFiles();
+
+    const res = await createItem()
+      .attach('images', REAL_PNG, { filename: 'good.png', contentType: 'image/png' })
+      .attach('images', HTML_BYTES, { filename: 'bad.jpg', contentType: 'image/jpeg' });
+
+    expect(res.status).toBe(400);
+    expect(await storedFiles()).toHaveLength(before.length);
+  });
+
+  it('does not create the item when the upload is refused', async () => {
+    await createItem().attach('images', HTML_BYTES, {
+      filename: 'photo.jpg',
+      contentType: 'image/jpeg'
+    });
+
+    const { rows } = await pool.query(`SELECT COUNT(*)::int AS n FROM items`);
+    expect(rows[0].n).toBe(0);
+  });
+});
+
+describe('the uploads directory', () => {
+  it('only ever gains files with an extension the allowlist produced', async () => {
+    await createItem().attach('images', REAL_PNG, {
+      filename: 'whatever.jpeg',
+      contentType: 'image/png'
+    });
+
+    const files = await storedFiles();
+    for (const file of files) {
+      expect(['.jpg', '.png', '.webp']).toContain(path.extname(file));
+    }
+  });
+});

backend/tests/unit/uploadTypes.test.ts

@@ -0,0 +1,107 @@
+import {
+  ALLOWED_IMAGE_TYPES,
+  SIGNATURE_BYTES,
+  extensionFor,
+  isAllowedImageType,
+  signatureMatches
+} from '../../src/uploadTypes';
+
+// Heads long enough to satisfy the WebP check, which needs twelve bytes.
+const JPEG_HEAD = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]);
+const PNG_HEAD = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]);
+const WEBP_HEAD = Buffer.concat([
+  Buffer.from('RIFF', 'ascii'),
+  Buffer.from([0, 0, 0, 0]),
+  Buffer.from('WEBP', 'ascii')
+]);
+// What an attacker actually sends: a document declared as an image.
+const HTML_HEAD = Buffer.from('<!doctype html><script>', 'ascii');
+
+describe('isAllowedImageType', () => {
+  it.each(['image/jpeg', 'image/png', 'image/webp'])('accepts %s', (type) => {
+    expect(isAllowedImageType(type)).toBe(true);
+  });
+
+  // SVG is an image and can carry script that runs when the file is navigated
+  // to directly, which is the whole reason it is excluded rather than the
+  // generic image/* being accepted. See #103.
+  it('refuses SVG despite it being an image', () => {
+    expect(isAllowedImageType('image/svg+xml')).toBe(false);
+  });
+
+  it.each(['text/html', 'application/pdf', 'image/gif', 'application/octet-stream', ''])(
+    'refuses %p',
+    (type) => {
+      expect(isAllowedImageType(type)).toBe(false);
+    }
+  );
+
+  it('exposes exactly the three types the frontend offers', () => {
+    expect([...ALLOWED_IMAGE_TYPES].sort()).toEqual(['image/jpeg', 'image/png', 'image/webp']);
+  });
+});
+
+describe('extensionFor', () => {
+  it.each([
+    ['image/jpeg', '.jpg'],
+    ['image/png', '.png'],
+    ['image/webp', '.webp']
+  ])('maps %s to %s', (type, ext) => {
+    expect(extensionFor(type)).toBe(ext);
+  });
+
+  // The stored name is derived from this rather than from the submitted
+  // filename, so an unmapped type must not silently produce an empty extension.
+  it('returns null for a type it does not know', () => {
+    expect(extensionFor('image/svg+xml')).toBeNull();
+    expect(extensionFor('text/html')).toBeNull();
+  });
+});
+
+describe('signatureMatches', () => {
+  it('accepts each type whose bytes agree with its declared type', () => {
+    expect(signatureMatches('image/jpeg', JPEG_HEAD)).toBe(true);
+    expect(signatureMatches('image/png', PNG_HEAD)).toBe(true);
+    expect(signatureMatches('image/webp', WEBP_HEAD)).toBe(true);
+  });
+
+  // The case declared-type checking alone waves straight through: rename
+  // evil.html to photo.jpg and say it is image/jpeg.
+  it('refuses a document wearing an image content type', () => {
+    expect(signatureMatches('image/jpeg', HTML_HEAD)).toBe(false);
+    expect(signatureMatches('image/png', HTML_HEAD)).toBe(false);
+    expect(signatureMatches('image/webp', HTML_HEAD)).toBe(false);
+  });
+
+  // One real image type declared as another is still a mismatch — the stored
+  // extension comes from the declared type, so the name would lie about it.
+  it('refuses a real image whose bytes are a different format', () => {
+    expect(signatureMatches('image/png', JPEG_HEAD)).toBe(false);
+    expect(signatureMatches('image/jpeg', PNG_HEAD)).toBe(false);
+  });
+
+  // WebP is RIFF at 0 and WEBP at 8; a RIFF container that is not WebP (a wav,
+  // say) must not pass on the first four bytes alone.
+  it('refuses a RIFF container that is not WebP', () => {
+    const wav = Buffer.concat([
+      Buffer.from('RIFF', 'ascii'),
+      Buffer.from([0, 0, 0, 0]),
+      Buffer.from('WAVE', 'ascii')
+    ]);
+    expect(signatureMatches('image/webp', wav)).toBe(false);
+  });
+
+  // A truncated read must fail closed rather than throwing or passing.
+  it('refuses a head too short to identify', () => {
+    expect(signatureMatches('image/webp', Buffer.from([0x52, 0x49]))).toBe(false);
+    expect(signatureMatches('image/png', Buffer.alloc(0))).toBe(false);
+  });
+
+  it('refuses a type it was never told to allow', () => {
+    expect(signatureMatches('image/svg+xml', Buffer.from('<svg', 'ascii'))).toBe(false);
+  });
+
+  it('reads enough bytes for the longest signature it checks', () => {
+    expect(SIGNATURE_BYTES).toBeGreaterThanOrEqual(12);
+  });
+});

frontend/src/admin/Admin.tsx

@@ -319,7 +319,15 @@ function Inventory() {
               </Form.Item>
             )}
             <Form.Item label={editingItem ? 'Add More Images' : 'Images (front, back, etc.)'}>
-              <Upload fileList={fileList} beforeUpload={() => false} onChange={({ fileList }) => setFileList(fileList.slice(-6))}
+              {/* The exact three types the server accepts, not image/*. Offering
+                  a type the API will refuse — SVG, notably — turns a picker
+                  choice into a 400 the admin has to decode. The operating
+                  system's own "All files" option is unaffected: `accept`
+                  chooses the default filter, it does not remove that escape
+                  hatch, and it is not a control either way. The list is
+                  enforced in backend/src/uploadTypes.ts; change both. */}
+              <Upload accept="image/jpeg,image/png,image/webp"
+                fileList={fileList} beforeUpload={() => false} onChange={({ fileList }) => setFileList(fileList.slice(-6))}
                 maxCount={6} multiple listType="picture-card">
                 <div><UploadOutlined /><div style={{ marginTop: 8 }}>Upload</div></div>
               </Upload>