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); } const tick = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); /** * Waits for the uploads directory to reach the expected state, then returns it. * * `discardUnlessAccepted` unlinks from a `res.on('close')` handler, so nothing * awaits it and nothing can — it hangs off a response event. `await request()` * resolves when the response completes, which is when `close` fires, so a read * taken straight afterwards races the unlink it is meant to observe. The * property is an eventual one, so asserting it has to be eventual too. See #228. * * The final read is returned rather than a boolean, so the caller still asserts * on real contents and a failure names the files that were actually there. */ async function filesSettlingTo(matches: (files: string[]) => boolean): Promise { const deadline = Date.now() + 3000; let files = await storedFiles(); while (!matches(files) && Date.now() < deadline) { await tick(25); files = await storedFiles(); } return files; } /** * Empties the uploads directory before each test in this file. * * The race runs in both directions, and this is the half that is easy to miss: * a deletion still pending from the *previous* test corrupts the next test's * baseline before its request is even sent. Polling cannot fix that — the * baseline is already wrong — and waiting for the directory to look quiet only * works while the unlink is faster than the wait, which is precisely the * assumption #228 is about. * * Starting from empty removes the baseline as a variable entirely. Any orphan * left by an earlier suite goes with it, which is correct: this directory is a * temporary one and nothing outside these tests owns its contents. */ beforeEach(async () => { for (const name of await storedFiles()) { await fs.unlink(path.join(UPLOADS_DIR, name)).catch(() => undefined); } }); 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: string[] = []; await createItem().attach('images', HTML_BYTES, { filename: 'photo.jpg', contentType: 'image/jpeg' }); expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); // 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: string[] = []; 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 filesSettlingTo((files) => files.length === 0)).toEqual([]); }); 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)); } }); }); /** * Bounding what one request may write is only half of bounding what the volume * accumulates. multer writes to disk before any route logic runs, so a request * refused *after* the write leaves its bytes behind with nothing referencing * them — no database row, no way to find them again, and no upper bound on how * many an attacker with admin access can pile up. Found reviewing the security * hotspots in this file (#180). */ describe('what it leaves on disk', () => { it('removes the upload when the request is refused for its other fields', async () => { const before: string[] = []; const res = await request(app) .post('/api/admin/items') .field('name', 'Photographed thing') .field('description', '') .field('price', '30') // Valid image, invalid sibling field: the file is already written by the // time the route reads this and returns 400. .field('category_id', 'not-a-number') .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' }); expect(res.status).toBe(400); expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); it('removes the upload when the tags field is refused', async () => { const before: string[] = []; const res = await request(app) .post('/api/admin/items') .field('name', 'Photographed thing') .field('description', '') .field('price', '30') .field('tags', '[not json') .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' }); expect(res.status).toBe(400); expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); // The accepted case must not be swept up by the same cleanup: these files are // the ones the item now points at. it('keeps the upload when the request succeeds', async () => { const before: string[] = []; const res = await createItem().attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' }); expect(res.status).toBe(200); // No unlink is scheduled for an accepted request, so this settles at once. expect(await filesSettlingTo((files) => files.length === 1)).toHaveLength(1); }); });