From c6936720510d60bb08e0f26ceb382b8c59c67ffa Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 17:23:18 -0500 Subject: [PATCH] feat(backend): log client-side render errors to the server (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/app.ts | 2 + backend/src/rateLimit.ts | 21 ++++++ backend/src/routes/clientErrors.ts | 44 ++++++++++++ .../clientErrors.integration.test.ts | 71 +++++++++++++++++++ 4 files changed, 138 insertions(+) create mode 100644 backend/src/routes/clientErrors.ts create mode 100644 backend/tests/integration/clientErrors.integration.test.ts diff --git a/backend/src/app.ts b/backend/src/app.ts index 9172962..e0fcea5 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -13,6 +13,7 @@ import customersRouter from './routes/customers'; import publicRouter from './routes/public'; import cartRouter from './routes/cart'; import shippingAddressesRouter from './routes/shippingAddresses'; +import clientErrorsRouter from './routes/clientErrors'; import { attachCustomer } from './middleware/customerAuth'; import { asyncRoute } from './asyncRoute'; @@ -52,6 +53,7 @@ app.use('/api/admin/tags', adminTagsRouter); app.use('/api/admin', adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); app.use('/api/customers', customersRouter); +app.use('/api/client-errors', clientErrorsRouter); app.use('/', publicRouter); if (process.env.NODE_ENV !== 'test') { diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index 2cfa2ca..3a190e3 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -33,3 +33,24 @@ export const passwordResetRequestLimiter = rateLimit({ legacyHeaders: false, 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' } +}); diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts new file mode 100644 index 0000000..cd07400 --- /dev/null +++ b/backend/src/routes/clientErrors.ts @@ -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; diff --git a/backend/tests/integration/clientErrors.integration.test.ts b/backend/tests/integration/clientErrors.integration.test.ts new file mode 100644 index 0000000..18e7509 --- /dev/null +++ b/backend/tests/integration/clientErrors.integration.test.ts @@ -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); + }); +});