Merge pull request 'feat(backend): accept only real images in the inventory upload (#95)' (#104) from feature/95-upload-type-validation into main

Reviewed-on: #104

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 });
     }
-    return next(err);
+    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>