fix(server): validate X-Request-Id charset, not just length (#314)

server.js

@@ -49,11 +49,22 @@ app.use(pinoHttp({
     // a fresh one. The id lands on req.id, is echoed back on the
     // X-Request-Id response header, and is included in every log
     // line via pino-http's reqId binding.
+    //
+    // Validate the incoming value's character set, not just its
+    // length: Node's `res.setHeader` throws ERR_INVALID_CHAR on
+    // anything outside the printable-ASCII range (e.g. CR, LF, NUL),
+    // and a thrown setHeader inside pino-http's request hook
+    // crashes the request handler and surfaces as a 500. Limiting
+    // to printable ASCII [0x21..0x7e] (no spaces, no control chars)
+    // matches the W3C trace-id alphabet and the de-facto format used
+    // by every proxy / mesh / APM agent we'd be propagating from.
     genReqId: (req, res) => {
         const incoming = req.headers['x-request-id'];
-        const reqId = (typeof incoming === 'string' && incoming.length > 0 && incoming.length <= 128)
-            ? incoming
-            : crypto.randomUUID();
+        const valid = typeof incoming === 'string'
+            && incoming.length > 0
+            && incoming.length <= 128
+            && /^[\x21-\x7e]+$/.test(incoming);
+        const reqId = valid ? incoming : crypto.randomUUID();
         res.setHeader('X-Request-Id', reqId);
         return reqId;
     },

tests/api/request-id.test.js

@@ -29,9 +29,11 @@ beforeAll(async () => {
         logger: log,
         genReqId: (req, res) => {
             const incoming = req.headers['x-request-id'];
-            const reqId = (typeof incoming === 'string' && incoming.length > 0 && incoming.length <= 128)
-                ? incoming
-                : crypto.randomUUID();
+            const valid = typeof incoming === 'string'
+                && incoming.length > 0
+                && incoming.length <= 128
+                && /^[\x21-\x7e]+$/.test(incoming);
+            const reqId = valid ? incoming : crypto.randomUUID();
             res.setHeader('X-Request-Id', reqId);
             return reqId;
         },
@@ -72,4 +74,35 @@ describe('X-Request-Id', () => {
         const r2 = await request(app).get('/healthz');
         expect(r1.headers['x-request-id']).not.toBe(r2.headers['x-request-id']);
     });
+
+    test('replaces an incoming value with whitespace with a generated UUID', async () => {
+        // Embedded space (0x20) is outside the printable-ASCII range
+        // [0x21..0x7e] we accept. Without the charset guard, this
+        // value would land in `res.setHeader('X-Request-Id', ...)`
+        // — Node accepts spaces in header values fine, but the
+        // looser check is also a foothold for CR/LF injection
+        // through proxies that don't sanitize. Treat anything
+        // outside the printable-no-space range as invalid.
+        const res = await request(app)
+            .get('/healthz')
+            .set('X-Request-Id', 'has spaces');
+        const id = res.headers['x-request-id'];
+        expect(id).not.toBe('has spaces');
+        expect(id).toMatch(/^[0-9a-f-]+$/i);
+    });
+
+    test('replaces an incoming value with a tab character with a generated UUID', async () => {
+        // Tabs (0x09) are outside the printable-ASCII range and would
+        // be smuggled through some intermediate proxies as a header
+        // continuation. Treat them as invalid like the whitespace case
+        // above. Supertest accepts the byte (unlike NUL or CR/LF
+        // which it rejects pre-flight) so we can drive the path
+        // end-to-end here.
+        const res = await request(app)
+            .get('/healthz')
+            .set('X-Request-Id', 'has\ttab');
+        const id = res.headers['x-request-id'];
+        expect(id).not.toBe('has\ttab');
+        expect(id).toMatch(/^[0-9a-f-]+$/i);
+    });
 });