bermudalamb
b616b9f0ab
SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one. The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing. That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable. The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads. `verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss. The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it. Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass. Refs #180
212 lines
7.2 KiB
TypeScript
212 lines
7.2 KiB
TypeScript
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));
|
|
}
|
|
});
|
|
});
|
|
|
|
/**
|
|
* 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 = await storedFiles();
|
|
|
|
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 storedFiles()).toEqual(before);
|
|
});
|
|
|
|
it('removes the upload when the tags field is refused', async () => {
|
|
const before = await storedFiles();
|
|
|
|
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 storedFiles()).toEqual(before);
|
|
});
|
|
|
|
// 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 = await storedFiles();
|
|
|
|
const res = await createItem().attach('images', REAL_PNG, {
|
|
filename: 'photo.png',
|
|
contentType: 'image/png'
|
|
});
|
|
|
|
expect(res.status).toBe(200);
|
|
expect((await storedFiles()).length).toBe(before.length + 1);
|
|
});
|
|
});
|