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'); + }); });