bermudalambandClaude Opus 5
0b6cc85c4f
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
248 lines
9.0 KiB
TypeScript
248 lines
9.0 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);
|
|
}
|
|
|
|
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<string[]> {
|
|
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('<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 () => {
|
|
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 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 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 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 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);
|
|
});
|
|
});
|