feat(backend): log client-side render errors to the server (#62)
The frontend's error boundaries need somewhere to report to. A boundary that only shows a customer a message leaves nobody knowing it happened, which is the failure shape this project has designed against three times already. POST /api/client-errors takes a report, truncates its fields, logs it with a [client-error] prefix and returns 204. No storage: the container log is where this project's operational visibility already lives, and a table with a retention policy and an admin screen is a subsystem larger than the issue. An unrecognised context is a 400 rather than a log line under a guessed label, following parseItemFilters, which refuses a malformed filter instead of coercing it. Oversized fields go the other way and are truncated rather than refused, because an over-long report is still the only record of the failure. The endpoint gets its own rate limiter rather than reusing passwordResetRequestLimiter, whose comment already warns that its caller-and-email key collapses every caller into one shared bucket on an endpoint without an email. The new one takes the default key generator, which also avoids the ERR_ERL_KEY_GEN_IPV6 warning the custom key produces. Verified: 138 integration tests pass, 4 of them new, and 79 unit. The unit count rose by one without a test being written — routesAreWrapped.test.ts runs describe.each over the files in src/routes, so a new route file generates a case. The handler is synchronous and needs no asyncRoute wrapper. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -13,6 +13,7 @@ import customersRouter from './routes/customers';
|
|||||||
import publicRouter from './routes/public';
|
import publicRouter from './routes/public';
|
||||||
import cartRouter from './routes/cart';
|
import cartRouter from './routes/cart';
|
||||||
import shippingAddressesRouter from './routes/shippingAddresses';
|
import shippingAddressesRouter from './routes/shippingAddresses';
|
||||||
|
import clientErrorsRouter from './routes/clientErrors';
|
||||||
import { attachCustomer } from './middleware/customerAuth';
|
import { attachCustomer } from './middleware/customerAuth';
|
||||||
import { asyncRoute } from './asyncRoute';
|
import { asyncRoute } from './asyncRoute';
|
||||||
|
|
||||||
@@ -52,6 +53,7 @@ app.use('/api/admin/tags', adminTagsRouter);
|
|||||||
app.use('/api/admin', adminRouter);
|
app.use('/api/admin', adminRouter);
|
||||||
app.use('/api/customers/me/addresses', shippingAddressesRouter);
|
app.use('/api/customers/me/addresses', shippingAddressesRouter);
|
||||||
app.use('/api/customers', customersRouter);
|
app.use('/api/customers', customersRouter);
|
||||||
|
app.use('/api/client-errors', clientErrorsRouter);
|
||||||
app.use('/', publicRouter);
|
app.use('/', publicRouter);
|
||||||
|
|
||||||
if (process.env.NODE_ENV !== 'test') {
|
if (process.env.NODE_ENV !== 'test') {
|
||||||
|
|||||||
@@ -33,3 +33,24 @@ export const passwordResetRequestLimiter = rateLimit({
|
|||||||
legacyHeaders: false,
|
legacyHeaders: false,
|
||||||
message: { error: 'too many attempts, please try again later' }
|
message: { error: 'too many attempts, please try again later' }
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Client error reports carry no email, so this one is keyed on the caller
|
||||||
|
// alone — deliberately not reusing passwordResetRequestLimiter, whose comment
|
||||||
|
// above explains why its key is wrong for an endpoint without an email.
|
||||||
|
//
|
||||||
|
// `trust proxy` is set in app.ts, so `req.ip` is the real client address from
|
||||||
|
// X-Forwarded-For rather than Nginx Proxy Manager's, making this a per-customer
|
||||||
|
// allowance rather than one shared by everybody behind the proxy.
|
||||||
|
//
|
||||||
|
// Generous, because hitting the limit is harmless: the reporter ignores the
|
||||||
|
// response either way. It exists so a render loop cannot fill the log.
|
||||||
|
const CLIENT_ERROR_WINDOW_MS = 15 * 60 * 1000;
|
||||||
|
const CLIENT_ERROR_MAX_REQUESTS = 30;
|
||||||
|
|
||||||
|
export const clientErrorLimiter = rateLimit({
|
||||||
|
windowMs: CLIENT_ERROR_WINDOW_MS,
|
||||||
|
limit: CLIENT_ERROR_MAX_REQUESTS,
|
||||||
|
standardHeaders: 'draft-7',
|
||||||
|
legacyHeaders: false,
|
||||||
|
message: { error: 'too many reports' }
|
||||||
|
});
|
||||||
|
|||||||
@@ -0,0 +1,44 @@
|
|||||||
|
import { Router, Request, Response } from 'express';
|
||||||
|
import { clientErrorLimiter } from '../rateLimit';
|
||||||
|
|
||||||
|
const router = Router();
|
||||||
|
|
||||||
|
// The three error boundaries in the frontend. An unrecognised context means the
|
||||||
|
// client and the server disagree about something, which is worth surfacing
|
||||||
|
// rather than logging under a guessed label — the same reasoning as
|
||||||
|
// parseItemFilters refusing a malformed filter instead of coercing it.
|
||||||
|
const CONTEXTS: readonly string[] = ['page', 'catalogue', 'modal'];
|
||||||
|
|
||||||
|
const MAX_MESSAGE = 500;
|
||||||
|
const MAX_STACK = 4000;
|
||||||
|
const MAX_COMPONENT_STACK = 4000;
|
||||||
|
const MAX_PATH = 200;
|
||||||
|
|
||||||
|
// Anything that is not a string becomes empty rather than 'undefined' or
|
||||||
|
// '[object Object]', so a malformed field cannot dress itself up as content.
|
||||||
|
function clip(value: unknown, max: number): string {
|
||||||
|
if (typeof value !== 'string') {
|
||||||
|
return '';
|
||||||
|
}
|
||||||
|
return value.length > max ? `${value.slice(0, max)}… [truncated]` : value;
|
||||||
|
}
|
||||||
|
|
||||||
|
// No asyncRoute: this handler is synchronous, so there is no promise for the
|
||||||
|
// error middleware to miss.
|
||||||
|
router.post('/', clientErrorLimiter, (req: Request, res: Response) => {
|
||||||
|
const context: unknown = req.body?.context;
|
||||||
|
if (typeof context !== 'string' || !CONTEXTS.includes(context)) {
|
||||||
|
return res.status(400).json({ error: 'invalid context' });
|
||||||
|
}
|
||||||
|
|
||||||
|
console.error(
|
||||||
|
`[client-error] context=${context} path=${clip(req.body?.path, MAX_PATH)}\n` +
|
||||||
|
` message: ${clip(req.body?.message, MAX_MESSAGE)}\n` +
|
||||||
|
` stack: ${clip(req.body?.stack, MAX_STACK)}\n` +
|
||||||
|
` componentStack: ${clip(req.body?.componentStack, MAX_COMPONENT_STACK)}`
|
||||||
|
);
|
||||||
|
|
||||||
|
res.status(204).end();
|
||||||
|
});
|
||||||
|
|
||||||
|
export default router;
|
||||||
@@ -0,0 +1,71 @@
|
|||||||
|
import request from 'supertest';
|
||||||
|
import app from '../../src/app';
|
||||||
|
import { pool } from '../../src/db';
|
||||||
|
import { closeDb } from './setup/testDb';
|
||||||
|
|
||||||
|
// No resetDb: this endpoint never touches the database. The pool still has to
|
||||||
|
// be closed or Jest reports an open handle.
|
||||||
|
afterAll(async () => {
|
||||||
|
await pool.end();
|
||||||
|
await closeDb();
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('POST /api/client-errors', () => {
|
||||||
|
let errorSpy: jest.SpyInstance;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined);
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
errorSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('accepts a well-formed report and logs it', async () => {
|
||||||
|
const res = await request(app).post('/api/client-errors').send({
|
||||||
|
context: 'catalogue',
|
||||||
|
message: 'Cannot read properties of undefined',
|
||||||
|
stack: 'Error: Cannot read properties of undefined\n at ItemCard',
|
||||||
|
componentStack: '\n at Catalogue\n at App',
|
||||||
|
path: '/?category=3'
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(res.status).toBe(204);
|
||||||
|
expect(errorSpy).toHaveBeenCalledTimes(1);
|
||||||
|
const logged = errorSpy.mock.calls[0][0] as string;
|
||||||
|
expect(logged).toContain('[client-error]');
|
||||||
|
expect(logged).toContain('context=catalogue');
|
||||||
|
expect(logged).toContain('Cannot read properties of undefined');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a context it does not recognise rather than logging under a guess', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/client-errors')
|
||||||
|
.send({ context: 'checkout', message: 'boom' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(res.body.error).toBe('invalid context');
|
||||||
|
expect(errorSpy).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a report with no context at all', async () => {
|
||||||
|
const res = await request(app).post('/api/client-errors').send({ message: 'boom' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(errorSpy).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Truncated rather than refused: an over-long report is still the only record
|
||||||
|
// of the failure, and dropping it would lose the thing worth having.
|
||||||
|
it('truncates an oversized message instead of refusing it', async () => {
|
||||||
|
const res = await request(app).post('/api/client-errors').send({
|
||||||
|
context: 'page',
|
||||||
|
message: 'x'.repeat(5000)
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(res.status).toBe(204);
|
||||||
|
const logged = errorSpy.mock.calls[0][0] as string;
|
||||||
|
expect(logged).toContain('[truncated]');
|
||||||
|
expect(logged.length).toBeLessThan(2000);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user