diff --git a/backend/src/app.ts b/backend/src/app.ts index 617e428e..a0b3c9eb 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -14,6 +14,7 @@ import { sandboxMiddleware } from "./middleware/sandbox.middleware.js"; import { globalRateLimiter, healthRateLimiter } from "./middleware/rate-limiter.middleware.js"; import { metricsMiddleware } from "./middleware/metrics.middleware.js"; import { requestIdMiddleware } from "./middleware/requestId.js"; +import { getRequestId } from "./lib/request-context.js"; import v1Routes from "./routes/v1/index.js"; import healthRoutes from "./routes/health.routes.js"; import metricsRoutes from "./routes/metrics.routes.js"; @@ -31,12 +32,14 @@ if (!process.env.CORS_ALLOWED_ORIGINS && !isProduction) { allowedOrigins.push("http://localhost:3000"); } -// Apply global rate limiter first -app.use(globalRateLimiter); - -// Request ID tracing +// Request ID tracing must be the very first middleware so that every response +// carries X-Request-ID — including responses short-circuited by later +// middleware such as the rate limiter's 429 or the CORS 403 (Issue #1494). app.use(requestIdMiddleware); +// Apply global rate limiter +app.use(globalRateLimiter); + // Request counting/latency for the Prometheus registry app.use(metricsMiddleware); @@ -90,7 +93,14 @@ app.use( // Convert CORS errors into 403 responses so callers get a clear status code app.use((err: unknown, req: Request, res: Response, next: NextFunction) => { if (err instanceof Error && err.message === "CORS origin not allowed") { - res.status(403).json({ error: "CORS origin not allowed" }); + const requestId = getRequestId(); + res + .status(403) + .json( + requestId + ? { error: "CORS origin not allowed", requestId } + : { error: "CORS origin not allowed" }, + ); return; } next(err); diff --git a/backend/src/lib/request-context.ts b/backend/src/lib/request-context.ts new file mode 100644 index 00000000..aa27992b --- /dev/null +++ b/backend/src/lib/request-context.ts @@ -0,0 +1,20 @@ +/** + * Request-scoped context shared by the logging and error layers (Issue #1494). + * + * The request id lives in a dedicated AsyncLocalStorage so the winston logger + * and `sendApiError` can both read it without importing each other, and so + * unit tests that mock the logger still propagate a request id into error + * payloads. + */ +import { AsyncLocalStorage } from 'node:async_hooks'; + +export interface RequestContext { + requestId: string; +} + +export const requestContext = new AsyncLocalStorage(); + +/** The request id bound to the current async execution, if any. */ +export function getRequestId(): string | undefined { + return requestContext.getStore()?.requestId; +} diff --git a/backend/src/logger.ts b/backend/src/logger.ts index 565e13f9..69635550 100644 --- a/backend/src/logger.ts +++ b/backend/src/logger.ts @@ -1,7 +1,9 @@ -import { AsyncLocalStorage } from 'async_hooks'; import { createLogger, format, transports } from 'winston'; +import { requestContext } from './lib/request-context.js'; -export const requestContext = new AsyncLocalStorage<{ requestId: string }>(); +// Re-exported so callers that already read the request context from the logger +// module (worker correlation ids, SSE controller) keep working unchanged. +export { requestContext }; const logger = createLogger({ level: process.env.LOG_LEVEL || 'info', diff --git a/backend/src/middleware/rate-limiter.middleware.ts b/backend/src/middleware/rate-limiter.middleware.ts index 67ae7f1a..f82eaddf 100644 --- a/backend/src/middleware/rate-limiter.middleware.ts +++ b/backend/src/middleware/rate-limiter.middleware.ts @@ -1,4 +1,6 @@ import { rateLimit, type Options } from 'express-rate-limit'; +import type { NextFunction, Request, Response } from 'express'; +import { getRequestId } from '../lib/request-context.js'; /** * Shared factory to create an express-rate-limit instance with common configuration. @@ -14,11 +16,32 @@ export function createRateLimiter(options: Partial) { }); } +/** + * Default 429 handler: keeps the configured JSON message and stamps the + * request id (Issue #1494), so throttled responses stay traceable in the logs + * even though they never reach a route handler or the global error handler. + */ +export function rateLimitHandler( + _req: Request, + res: Response, + _next: NextFunction, + options: Options, +): void { + const message = options.message; + const body: Record = + typeof message === 'object' && message !== null + ? { ...(message as Record) } + : { message }; + const requestId = getRequestId(); + if (requestId) body.requestId = requestId; + res.status(options.statusCode).json(body); +} + export const globalRateLimiter = createRateLimiter({ windowMs: 1 * 60 * 1000, // 1 minute max: 100, // Limit each IP to 100 requests per `window` (here, per minute) - standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers - legacyHeaders: false, // Disable the `X-RateLimit-*` headers + standardHeaders: true, + legacyHeaders: false, // The Prometheus scrape endpoint must never be throttled: a 429 would make // Prometheus mark the target down and blind the whole alerting pipeline. // It is protected by its own network/token guard instead. @@ -27,6 +50,7 @@ export const globalRateLimiter = createRateLimiter({ message: 'Too many requests, please try again later.', status: 429, }, + handler: rateLimitHandler, }); /** * Dedicated limiter for `/health` (issue #1511). diff --git a/backend/src/middleware/requestId.ts b/backend/src/middleware/requestId.ts index c95f5650..ca9c2916 100644 --- a/backend/src/middleware/requestId.ts +++ b/backend/src/middleware/requestId.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'crypto'; import type { Request, Response, NextFunction } from 'express'; -import logger, { requestContext } from '../logger.js'; +import logger from '../logger.js'; +import { requestContext } from '../lib/request-context.js'; const MAX_REQUEST_ID_LENGTH = 128; @@ -18,6 +19,9 @@ export function requestIdMiddleware(req: Request, res: Response, next: NextFunct const header = req.headers['x-request-id']; const requestId = typeof header === 'string' && isValidRequestId(header) ? header : randomUUID(); + // Attach to the request as well as the response so downstream handlers, + // error formatters and tests can read the same id without re-parsing headers. + (req as Request & { id?: string }).id = requestId; res.setHeader('X-Request-ID', requestId); const startMs = Date.now(); diff --git a/backend/src/types/api-error.ts b/backend/src/types/api-error.ts index d7cf3334..05c3f368 100644 --- a/backend/src/types/api-error.ts +++ b/backend/src/types/api-error.ts @@ -1,9 +1,15 @@ import type { Response } from "express"; +import { getRequestId } from "../lib/request-context.js"; export interface ApiErrorBody { code: string; message: string; details?: unknown; + /** + * Correlation id for the failing request (Issue #1494). Clients can quote it + * to support engineers, who can then find the matching winston log lines. + */ + requestId?: string; } export class ApiError extends Error { @@ -29,5 +35,7 @@ export function sendApiError( ) { const error: ApiErrorBody = { code, message }; if (details !== undefined) error.details = details; + const requestId = getRequestId(); + if (requestId) error.requestId = requestId; return res.status(statusCode).json({ error }); -} \ No newline at end of file +} diff --git a/backend/tests/error.middleware.test.ts b/backend/tests/error.middleware.test.ts index 4e3eaff7..b353f88c 100644 --- a/backend/tests/error.middleware.test.ts +++ b/backend/tests/error.middleware.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { errorHandler } from '../src/middleware/error.middleware.js'; import { ZodError } from 'zod'; import { Prisma } from '../src/generated/prisma/index.js'; +import { requestContext } from '../src/lib/request-context.js'; import type { Request, Response, NextFunction } from 'express'; describe('Error Middleware', () => { @@ -59,4 +60,44 @@ describe('Error Middleware', () => { expect(res.status).not.toHaveBeenCalled(); expect(res.json).not.toHaveBeenCalled(); }); + + it('includes the bound requestId in a 500 error payload (Issue #1494)', () => { + const error = new Error('Generic error'); + + requestContext.run({ requestId: 'req-error-1' }, () => { + errorHandler(error, req as Request, res as Response, next); + }); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + error: expect.objectContaining({ + code: 'INTERNAL_SERVER_ERROR', + requestId: 'req-error-1', + }), + })); + }); + + it('includes the bound requestId in a validation error payload (Issue #1494)', () => { + const error = new ZodError([{ path: ['field'], message: 'invalid', code: 'custom' }]); + + requestContext.run({ requestId: 'req-error-2' }, () => { + errorHandler(error, req as Request, res as Response, next); + }); + + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + error: expect.objectContaining({ + code: 'VALIDATION_ERROR', + requestId: 'req-error-2', + }), + })); + }); + + it('omits requestId when no request context is bound', () => { + const error = new Error('Generic error'); + + errorHandler(error, req as Request, res as Response, next); + + const body = (res.json as any).mock.calls[0][0]; + expect(body.error.requestId).toBeUndefined(); + }); }); diff --git a/backend/tests/requestId.test.ts b/backend/tests/requestId.test.ts index 1667a1ec..32951743 100644 --- a/backend/tests/requestId.test.ts +++ b/backend/tests/requestId.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import request from 'supertest'; import { requestIdMiddleware } from '../src/middleware/requestId.js'; +import { rateLimitHandler } from '../src/middleware/rate-limiter.middleware.js'; +import { requestContext } from '../src/lib/request-context.js'; +import app from '../src/app.js'; import type { Request, Response, NextFunction } from 'express'; describe('RequestId Middleware', () => { @@ -57,4 +61,65 @@ describe('RequestId Middleware', () => { const call = (res.setHeader as any).mock.calls[0]; expect(call[1]).toMatch(/^[a-f0-9-]+$/i); }); + + it('exposes the same id on the request object and the response header (Issue #1494)', () => { + requestIdMiddleware(req as Request, res as Response, next); + const headerValue = (res.setHeader as any).mock.calls[0][1]; + expect((req as Request & { id?: string }).id).toBe(headerValue); + }); +}); + +describe('X-Request-ID propagation (Issue #1494)', () => { + it('sets the header on a normal response', async () => { + const response = await request(app).get('/'); + expect(response.status).toBe(200); + expect(response.headers['x-request-id']).toBeTruthy(); + }); + + it('echoes a valid client-supplied request id', async () => { + const response = await request(app).get('/').set('X-Request-ID', 'client-trace-42'); + expect(response.headers['x-request-id']).toBe('client-trace-42'); + }); + + it('includes requestId in an error payload produced by sendApiError', async () => { + const response = await request(app) + .get('/v1/streams/not-a-number') + .set('X-Request-ID', 'err-trace-7'); + + expect(response.status).toBe(400); + expect(response.headers['x-request-id']).toBe('err-trace-7'); + expect(response.body.error.code).toBe('INVALID_STREAM_ID'); + expect(response.body.error.requestId).toBe('err-trace-7'); + }); + + it('sets the header on a response short-circuited before the routers (CORS 403)', async () => { + const response = await request(app) + .get('/') + .set('Origin', 'https://evil.example') + .set('X-Request-ID', 'cors-trace-9'); + + expect(response.status).toBe(403); + expect(response.headers['x-request-id']).toBe('cors-trace-9'); + expect(response.body.requestId).toBe('cors-trace-9'); + }); +}); + +describe('Rate limit 429 request id', () => { + it('stamps requestId from the request context onto the throttle body', () => { + const res = { + status: vi.fn().mockReturnThis(), + json: vi.fn().mockReturnThis(), + }; + const options = { + statusCode: 429, + message: { message: 'Too many requests' }, + } as any; + + requestContext.run({ requestId: 'rl-trace-1' }, () => { + rateLimitHandler({} as Request, res as unknown as Response, vi.fn(), options); + }); + + expect(res.status).toHaveBeenCalledWith(429); + expect(res.json).toHaveBeenCalledWith({ message: 'Too many requests', requestId: 'rl-trace-1' }); + }); });