From 7d227507b01945d96262203cbdc48eb105598004 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 20 Aug 2026 17:32:45 -0500 Subject: [PATCH] fix(backend): stop a client error report forging log lines (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the endpoint found that truncating the report's fields is not enough. It is unauthenticated and reachable without the frontend, so a caller could embed a newline in any field and forge what reads as a second [client-error] record in the shared server log. Every field is now stripped of CR, LF and the other C0 control characters, plus DEL, each replaced by a single space, so one report is always exactly one log record. Sanitising happens before truncation rather than after. The substitution is 1-for-1, so it cannot change the string's length and clipping the sanitised value still guarantees the stored result never exceeds the limit. An escaping scheme that expanded a control character into several visible ones would need the opposite order to keep that guarantee, so the two are not interchangeable — recorded in a comment next to the code rather than left for someone to rediscover by reversing it. The check is a numeric code-point comparison rather than a regex over a control-character class. That is not style: the first attempt used one, and the hex escapes were corrupted into raw control bytes on the way into the file. Written this way the source never has to contain an escape sequence or a raw control character at all, and the file is verified free of both. The review also found the truncation boundary was never exercised — the only test sent 5000 characters against a 500 limit. Tests now cover a string of exactly the limit passing through untouched, one character over truncating, truncation of stack and componentStack rather than message alone, and a report full of newlines producing a single log line. Verified: 10 integration tests pass, up from 4, and lint reports no new warnings. Refs #62 Co-Authored-By: Claude Opus 5 --- backend/src/routes/clientErrors.ts | 33 +++++++- .../clientErrors.integration.test.ts | 82 +++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts index cd07400..645a7f7 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -14,13 +14,44 @@ const MAX_STACK = 4000; const MAX_COMPONENT_STACK = 4000; const MAX_PATH = 200; +// True for CR, LF and every other C0 control character, plus DEL (the +// C0 range is code points 0 through 31; DEL is 127). Written as a numeric +// comparison rather than a control-character regex literal so the source +// never has to embed a raw control character or an escape sequence for one. +const LAST_C0_CODE = 31; +const DEL_CODE = 127; +function isControlCharCode(code: number): boolean { + return code <= LAST_C0_CODE || code === DEL_CODE; +} + +// Strips CR, LF and other control characters from a string, replacing each +// with a single space. The endpoint is unauthenticated, so without this a +// caller could embed a newline in any field to forge what looks like a +// second [client-error] line in the shared server log. The replacement is +// 1-for-1 (one control character becomes one space), so it cannot change +// the string's length either way. +function sanitize(value: string): string { + let result = ''; + for (const char of value) { + result += isControlCharCode(char.codePointAt(0) ?? 0) ? ' ' : char; + } + return result; +} + // 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; + // Sanitize before truncating, not after. Because the substitution above is + // 1-for-1, sanitizing first cannot push the stored length past `max` — an + // escaping scheme that expanded a control character into multiple visible + // characters would need the opposite order to keep that same guarantee, so + // the two are not interchangeable and must not be reordered without + // re-checking this. + const sanitized = sanitize(value); + return sanitized.length > max ? `${sanitized.slice(0, max)}… [truncated]` : sanitized; } // No asyncRoute: this handler is synchronous, so there is no promise for the diff --git a/backend/tests/integration/clientErrors.integration.test.ts b/backend/tests/integration/clientErrors.integration.test.ts index 18e7509..45bf76e 100644 --- a/backend/tests/integration/clientErrors.integration.test.ts +++ b/backend/tests/integration/clientErrors.integration.test.ts @@ -68,4 +68,86 @@ describe('POST /api/client-errors', () => { expect(logged).toContain('[truncated]'); expect(logged.length).toBeLessThan(2000); }); + + // The boundary itself, not just "way over the limit": a message of exactly + // MAX_MESSAGE (500) must survive untouched. + it('leaves a message of exactly the length limit unmodified', async () => { + const message = 'a'.repeat(500); + const res = await request(app).post('/api/client-errors').send({ context: 'page', message }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain(`message: ${message}\n`); + expect(logged).not.toContain('[truncated]'); + }); + + // One character past the boundary must truncate. + it('truncates a message one character past the length limit', async () => { + const message = 'a'.repeat(501); + const res = await request(app).post('/api/client-errors').send({ context: 'page', message }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + expect(logged).toContain(`message: ${'a'.repeat(500)}… [truncated]`); + }); + + it('truncates an oversized stack independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + stack: 'x'.repeat(4001) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const stackLine = logged.split('\n').find((line) => line.trim().startsWith('stack:')); + expect(stackLine).toContain('[truncated]'); + }); + + it('truncates an oversized componentStack independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + componentStack: 'x'.repeat(4001) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const componentStackLine = logged + .split('\n') + .find((line) => line.trim().startsWith('componentStack:')); + expect(componentStackLine).toContain('[truncated]'); + }); + + it('truncates an oversized path independently of message', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'page', + message: 'short', + path: '/'.concat('x'.repeat(201)) + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + const pathLine = logged.split('\n')[0]; + expect(pathLine).toContain('[truncated]'); + }); + + // The endpoint is unauthenticated, so nothing stops a caller from sending a + // message crafted to look like a second [client-error] line. This is what + // Finding 1 closes: an embedded CR/LF must not survive into the log. + it('collapses embedded newlines so a report cannot forge a second log line', async () => { + const res = await request(app).post('/api/client-errors').send({ + context: 'modal', + message: 'real error\n[client-error] context=page path=/fake\r\n message: forged entry' + }); + + expect(res.status).toBe(204); + const logged = errorSpy.mock.calls[0][0] as string; + + // The template itself joins four fixed lines with three newlines; that + // count must not grow no matter what the caller sends. + expect(logged.split('\n')).toHaveLength(4); + expect(logged).not.toContain('\n[client-error]'); + expect(logged).not.toContain('\r'); + }); });