fix(backend): route every async handler through the error middleware (#59)
Express 4 does not forward a rejected promise from an async handler, so an unwrapped async route never responds at all — the request hangs until the client gives up, nothing reaches the error middleware, and monitoring sees an open connection rather than a 500. That silence is the shape of the 2026-08-17 incident, where an unhandled rejection left every item query hanging and the storefront rendered it as an empty shop. `asyncRoute` was written in response, but it was only applied to some routes: 30 handlers added afterwards were still bare, including register, login, the whole cart, and PayPal checkout. Wraps all 30, plus two the issue's inventory missed. `attachCustomer` is a bare async middleware mounted globally in app.ts, so a rejection in its session lookup would hang every request in the application — including the 25 handlers that were already wrapped correctly, which meant the guarantee did not actually hold anywhere. The PayPal webhook registers on a second router named `webhookRouter`, so an audit grepping for `router.` walked straight past it. Adds a unit test that scans the route sources and fails on any registration whose handler is not wrapped. A convention already half-forgotten once will be forgotten again, and enforcement is what the issue asked for; ESLint would be the better home for it but there is no ESLint in this repo yet (#60). The test walks parens rather than lines, so it also catches a handler whose `async` sits on its own line, and it matches any `*Router` name rather than just `router` — the two ways the existing bare handlers escaped notice. It is deleted along with `asyncRoute` if the project moves to Express 5, which forwards rejections natively. No behaviour changes on the success path; the failure path turns a hung request into a logged 500. Closes #59 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,154 @@
|
||||
import { readdirSync, readFileSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
|
||||
// Express 4 does not forward a rejected promise from an async handler, so an
|
||||
// unwrapped async route never responds at all — the request hangs until the
|
||||
// client gives up, nothing reaches the error middleware, and nothing is logged
|
||||
// as a failure. That is the shape of the 2026-08-17 incident that put an empty
|
||||
// storefront in front of customers.
|
||||
//
|
||||
// `asyncRoute` fixes it, but a convention only holds while everyone remembers
|
||||
// it, and this one was already half-forgotten once: 30 handlers were added
|
||||
// unwrapped after the wrapper existed. This test is the part that remembers.
|
||||
//
|
||||
// Express 5 forwards rejections natively. When the project upgrades, delete
|
||||
// `asyncRoute` and delete this test with it.
|
||||
|
||||
const SRC = join(__dirname, '..', '..', 'src');
|
||||
|
||||
// `router.get(`, `app.use(`, and friends — where a handler gets registered.
|
||||
// Any `*Router` name counts, not just `router`: cartCheckout.ts registers the
|
||||
// PayPal webhook on a second router, and matching only the common name is how
|
||||
// that one stayed unwrapped while everything around it was audited.
|
||||
const REGISTRATION = /\b(?:app|\w*[Rr]outer)\.(?:get|post|put|patch|delete|all|use)\s*\(/g;
|
||||
|
||||
/**
|
||||
* Returns the full text of the registration call starting at `open` (the index
|
||||
* of its `(`), by counting parens to the matching close. Quotes and comments
|
||||
* are skipped so a path like `'/items/:id'` or a `)` inside a string cannot
|
||||
* throw the count off.
|
||||
*/
|
||||
function registrationAt(source: string, open: number): string {
|
||||
let depth = 0;
|
||||
for (let i = open; i < source.length; i++) {
|
||||
const c = source[i];
|
||||
|
||||
if (c === "'" || c === '"' || c === '`') {
|
||||
i = skipString(source, i);
|
||||
continue;
|
||||
}
|
||||
if (c === '/' && source[i + 1] === '/') {
|
||||
i = source.indexOf('\n', i);
|
||||
if (i === -1) break;
|
||||
continue;
|
||||
}
|
||||
if (c === '/' && source[i + 1] === '*') {
|
||||
i = source.indexOf('*/', i) + 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (c === '(') depth++;
|
||||
if (c === ')') {
|
||||
depth--;
|
||||
if (depth === 0) return source.slice(open, i + 1);
|
||||
}
|
||||
}
|
||||
return source.slice(open);
|
||||
}
|
||||
|
||||
/** Returns the index of the closing quote of the string opening at `start`. */
|
||||
function skipString(source: string, start: number): number {
|
||||
const quote = source[start];
|
||||
for (let i = start + 1; i < source.length; i++) {
|
||||
if (source[i] === '\\') {
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (source[i] === quote) return i;
|
||||
}
|
||||
return source.length;
|
||||
}
|
||||
|
||||
/**
|
||||
* Every `async` inside a registration must sit directly behind `asyncRoute(`.
|
||||
* Checking the token rather than the line catches a handler whose `async`
|
||||
* lands on its own line, which a line-oriented grep would wave through.
|
||||
*/
|
||||
function unwrappedHandlers(source: string): string[] {
|
||||
const offenders: string[] = [];
|
||||
|
||||
for (const match of source.matchAll(REGISTRATION)) {
|
||||
const open = match.index! + match[0].length - 1;
|
||||
const call = registrationAt(source, open);
|
||||
|
||||
for (const found of call.matchAll(/\basync\b/g)) {
|
||||
const before = call.slice(0, found.index!).trimEnd();
|
||||
if (!before.endsWith('asyncRoute(')) {
|
||||
offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return offenders;
|
||||
}
|
||||
|
||||
function lineOf(source: string, index: number): number {
|
||||
return source.slice(0, index).split('\n').length;
|
||||
}
|
||||
|
||||
const routeFiles = readdirSync(join(SRC, 'routes'))
|
||||
.filter((f) => f.endsWith('.ts'))
|
||||
.map((f) => join('routes', f));
|
||||
|
||||
describe.each([...routeFiles, 'app.ts'])('%s', (relative) => {
|
||||
it('wraps every async handler in asyncRoute', () => {
|
||||
const source = readFileSync(join(SRC, relative), 'utf8');
|
||||
|
||||
expect(unwrappedHandlers(source)).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('async middleware', () => {
|
||||
// `attachCustomer` is mounted globally, so a rejection there hangs every
|
||||
// request in the application — including the routes that are wrapped
|
||||
// correctly. It is the one handler whose failure is not contained.
|
||||
it('wraps attachCustomer where app.ts mounts it', () => {
|
||||
const source = readFileSync(join(SRC, 'app.ts'), 'utf8');
|
||||
|
||||
expect(source).toContain('app.use(asyncRoute(attachCustomer))');
|
||||
});
|
||||
});
|
||||
|
||||
// Keeps the helper honest: if the paren-walking ever silently stops finding
|
||||
// registrations, the tests above would pass by finding nothing at all.
|
||||
describe('the guard itself', () => {
|
||||
it('finds a bare async handler', () => {
|
||||
const bad = `router.get('/', requireCustomer, async (req, res) => { res.json({}); });`;
|
||||
|
||||
expect(unwrappedHandlers(bad)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('accepts a wrapped one', () => {
|
||||
const good = `router.get('/', requireCustomer, asyncRoute(async (req, res) => { res.json({}); }));`;
|
||||
|
||||
expect(unwrappedHandlers(good)).toEqual([]);
|
||||
});
|
||||
|
||||
it('sees an async handler that starts on a later line', () => {
|
||||
const bad = `router.post(\n '/items/:id',\n uploadImages,\n async (req, res) => { res.json({}); }\n);`;
|
||||
|
||||
expect(unwrappedHandlers(bad)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('is not fooled by a paren inside a route path', () => {
|
||||
const bad = `router.get('/odd(path', async (req, res) => { res.json({}); });`;
|
||||
|
||||
expect(unwrappedHandlers(bad)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('leaves a synchronous handler alone', () => {
|
||||
const sync = `router.get('/', (req, res) => { res.json({}); });`;
|
||||
|
||||
expect(unwrappedHandlers(sync)).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user