diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index c410dc4..4a04fc7 100755 --- a/backend/src/routes/admin.ts +++ b/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 { + 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 { + 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 { + 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); }); }; diff --git a/backend/src/uploadTypes.ts b/backend/src/uploadTypes.ts new file mode 100644 index 0000000..883e1ea --- /dev/null +++ b/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> = { + '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; + } +} diff --git a/backend/tests/integration/uploadValidation.integration.test.ts b/backend/tests/integration/uploadValidation.integration.test.ts new file mode 100644 index 0000000..d5b9b86 --- /dev/null +++ b/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('', '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 { + 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(''), { + 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)); + } + }); +}); diff --git a/backend/tests/unit/uploadTypes.test.ts b/backend/tests/unit/uploadTypes.test.ts new file mode 100644 index 0000000..b45dde4 --- /dev/null +++ b/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('