diff --git a/src/app.ts b/src/app.ts index 7f868ae..31098bb 100644 --- a/src/app.ts +++ b/src/app.ts @@ -15,6 +15,7 @@ import { apiVersionMiddleware } from './middlewares/api-version.middleware'; import { schemaVersionMiddleware } from './middlewares/schema-version.middleware'; import { requestLoggerMiddleware } from './middlewares/request-logger.middleware'; import { requestContextMiddleware } from './middlewares/request-context.middleware'; +import { bodyParseErrorMiddleware } from './middlewares/body-parse-error.middleware'; import { envConfig } from './config'; const app: Express = express(); @@ -29,6 +30,7 @@ app.use(requestIdMiddleware); app.use(corsMiddleware()); app.use(helmet()); app.use(express.json({ limit: '10mb' })); +app.use(bodyParseErrorMiddleware); if (!envConfig.ENABLE_REQUEST_LOGGING) { app.use(morgan('combined')); diff --git a/src/middlewares/body-parse-error.middleware.test.ts b/src/middlewares/body-parse-error.middleware.test.ts new file mode 100644 index 0000000..9cd7b32 --- /dev/null +++ b/src/middlewares/body-parse-error.middleware.test.ts @@ -0,0 +1,169 @@ +import { Request, Response, NextFunction } from 'express'; +import { bodyParseErrorMiddleware } from './body-parse-error.middleware'; +import { logger } from '../utils/logger.utils'; + +jest.mock('../utils/logger.utils', () => ({ + logger: { error: jest.fn() }, +})); + +jest.mock('../utils/client-ip.utils', () => ({ + getClientIp: jest.fn(() => '10.0.0.1'), +})); + +function makeReq(method: string, url = '/api/v1/resource'): Request { + return { + method, + originalUrl: url, + url, + requestId: 'req-test-id', + socket: { remoteAddress: '127.0.0.1' }, + headers: {}, + } as unknown as Request; +} + +function makeRes(): Partial { + const res: Partial = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + return res; +} + +function makeSyntaxError(): SyntaxError & { body: string } { + const err = new SyntaxError('Unexpected token } in JSON') as SyntaxError & { body: string }; + err.body = 'raw body'; + return err; +} + +function makeEntityTooLargeError() { + return Object.assign(new Error('request entity too large'), { + type: 'entity.too.large', + status: 413, + limit: 10 * 1024 * 1024, + }); +} + +describe('bodyParseErrorMiddleware', () => { + let next: jest.Mock; + + beforeEach(() => { + next = jest.fn(); + jest.clearAllMocks(); + }); + + describe('malformed JSON on mutation methods', () => { + const MUTATION_METHODS = ['POST', 'PUT', 'PATCH', 'DELETE']; + + it.each(MUTATION_METHODS)( + 'logs error and returns 400 for %s with invalid JSON', + (method) => { + const err = makeSyntaxError(); + const req = makeReq(method); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(logger.error).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'body_parse_failure', + method, + path: '/api/v1/resource', + requestId: 'req-test-id', + errorType: 'invalid_json', + }) + ); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ success: false, message: 'Invalid JSON in request body' }) + ); + expect(next).not.toHaveBeenCalled(); + } + ); + + it('does not include raw body in the log', () => { + const err = makeSyntaxError(); + const req = makeReq('POST'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + const logCall = (logger.error as jest.Mock).mock.calls[0][0]; + expect(logCall).not.toHaveProperty('body'); + expect(logCall).not.toHaveProperty('rawBody'); + }); + + it('logs and returns 413 for entity.too.large on mutation', () => { + const err = makeEntityTooLargeError(); + const req = makeReq('POST'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(logger.error).toHaveBeenCalledWith( + expect.objectContaining({ errorType: 'entity.too.large' }) + ); + expect(res.status).toHaveBeenCalledWith(413); + }); + }); + + describe('non-mutation methods', () => { + it('calls next() for GET with a SyntaxError (not a mutation)', () => { + const err = makeSyntaxError(); + const req = makeReq('GET'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(next).toHaveBeenCalledWith(err); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('calls next() for HEAD with a SyntaxError', () => { + const err = makeSyntaxError(); + const req = makeReq('HEAD'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(next).toHaveBeenCalledWith(err); + }); + }); + + describe('non-parse errors', () => { + it('calls next() for a generic Error on a mutation method', () => { + const err = new Error('Something else entirely'); + const req = makeReq('POST'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(next).toHaveBeenCalledWith(err); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('calls next() for an ApiError on a mutation method', () => { + const err = Object.assign(new Error('Not found'), { statusCode: 404 }); + const req = makeReq('PUT'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(next).toHaveBeenCalledWith(err); + }); + }); + + describe('client response behaviour unchanged', () => { + it('returns 400 JSON with success:false and does not call next', () => { + const err = makeSyntaxError(); + const req = makeReq('POST'); + const res = makeRes(); + + bodyParseErrorMiddleware(err, req, res as Response, next as NextFunction); + + expect(res.status).toHaveBeenCalledWith(400); + expect((res.json as jest.Mock).mock.calls[0][0]).toMatchObject({ + success: false, + }); + expect(next).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/middlewares/body-parse-error.middleware.ts b/src/middlewares/body-parse-error.middleware.ts new file mode 100644 index 0000000..36af7ad --- /dev/null +++ b/src/middlewares/body-parse-error.middleware.ts @@ -0,0 +1,68 @@ +import { Request, Response, NextFunction } from 'express'; +import { logger } from '../utils/logger.utils'; +import { getClientIp } from '../utils/client-ip.utils'; +import { ErrorCode } from '../constants/error.constants'; + +const MUTATION_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); + +/** + * Intercepts body-parsing errors on mutation endpoints (POST, PUT, PATCH, + * DELETE) and emits a structured error-level log entry before returning the + * client a 400 response. + * + * What IS logged: + * - endpoint path + * - HTTP method + * - request ID (for correlation) + * - client IP (extracted via the trusted-proxy-aware helper) + * - error type / code + * + * What is NOT logged: + * - raw request body (never read or forwarded) + * - request headers beyond what Express already exposes on req + */ +export const bodyParseErrorMiddleware = ( + err: any, + req: Request, + res: Response, + next: NextFunction +): void => { + const isSyntaxError = + err instanceof SyntaxError && 'body' in err; + const isEntityTooLarge = + err.type === 'entity.too.large' || + err.status === 413 || + err.statusCode === 413; + + const isParseFailure = isSyntaxError || isEntityTooLarge; + + if (!isParseFailure || !MUTATION_METHODS.has(req.method)) { + return next(err); + } + + const clientIp = getClientIp(req); + + logger.error({ + type: 'body_parse_failure', + method: req.method, + path: req.originalUrl || req.url, + requestId: req.requestId, + clientIp, + errorType: isEntityTooLarge ? 'entity.too.large' : 'invalid_json', + }); + + if (isEntityTooLarge) { + res.status(413).json({ + success: false, + code: ErrorCode.BAD_REQUEST, + message: 'Request payload too large', + }); + return; + } + + res.status(400).json({ + success: false, + code: ErrorCode.BAD_REQUEST, + message: 'Invalid JSON in request body', + }); +}; diff --git a/src/utils/client-ip.utils.test.ts b/src/utils/client-ip.utils.test.ts new file mode 100644 index 0000000..b761268 --- /dev/null +++ b/src/utils/client-ip.utils.test.ts @@ -0,0 +1,96 @@ +import { getClientIp } from './client-ip.utils'; +import { Request } from 'express'; + +function makeReq( + socketIp: string, + forwardedFor?: string +): Request { + return { + socket: { remoteAddress: socketIp }, + headers: forwardedFor ? { 'x-forwarded-for': forwardedFor } : {}, + } as unknown as Request; +} + +describe('getClientIp', () => { + describe('when socket is a trusted proxy', () => { + it('returns the first X-Forwarded-For IP', () => { + const req = makeReq('127.0.0.1', '203.0.113.5, 10.0.0.1'); + expect(getClientIp(req)).toBe('203.0.113.5'); + }); + + it('handles a single IP in X-Forwarded-For', () => { + const req = makeReq('10.0.0.2', '198.51.100.7'); + expect(getClientIp(req)).toBe('198.51.100.7'); + }); + + it('trims whitespace from the extracted IP', () => { + const req = makeReq('192.168.1.1', ' 1.2.3.4 , 10.0.0.5'); + expect(getClientIp(req)).toBe('1.2.3.4'); + }); + + it('falls back to socket address when X-Forwarded-For is absent', () => { + const req = makeReq('10.10.10.1'); + expect(getClientIp(req)).toBe('10.10.10.1'); + }); + + it('falls back to socket address when X-Forwarded-For is empty string', () => { + const req = makeReq('172.16.0.1', ''); + expect(getClientIp(req)).toBe('172.16.0.1'); + }); + + it('handles array-valued X-Forwarded-For header', () => { + const req = { + socket: { remoteAddress: '127.0.0.1' }, + headers: { 'x-forwarded-for': ['9.9.9.9, 10.0.0.1', '8.8.8.8'] }, + } as unknown as Request; + expect(getClientIp(req)).toBe('9.9.9.9'); + }); + + it('accepts private 172.16.x.x as a trusted proxy', () => { + const req = makeReq('172.16.5.10', '55.55.55.55'); + expect(getClientIp(req)).toBe('55.55.55.55'); + }); + + it('accepts IPv6 loopback ::1 as trusted proxy', () => { + const req = makeReq('::1', '203.0.113.99'); + expect(getClientIp(req)).toBe('203.0.113.99'); + }); + }); + + describe('when socket is NOT a trusted proxy', () => { + it('ignores X-Forwarded-For and returns the socket address', () => { + const req = makeReq('203.0.113.1', '1.2.3.4'); + expect(getClientIp(req)).toBe('203.0.113.1'); + }); + + it('returns socket address even when no forwarded header is present', () => { + const req = makeReq('8.8.8.8'); + expect(getClientIp(req)).toBe('8.8.8.8'); + }); + }); + + describe('custom trusted predicate', () => { + it('uses the override predicate instead of the default CIDR check', () => { + const req = makeReq('8.8.8.8', '203.0.113.42'); + // Treat all IPs as trusted via override + expect(getClientIp(req, () => true)).toBe('203.0.113.42'); + }); + + it('rejects all proxies when override always returns false', () => { + const req = makeReq('127.0.0.1', '203.0.113.42'); + expect(getClientIp(req, () => false)).toBe('127.0.0.1'); + }); + }); + + describe('edge cases', () => { + it('returns undefined when socket address is missing', () => { + const req = { socket: {}, headers: {} } as unknown as Request; + expect(getClientIp(req)).toBeUndefined(); + }); + + it('returns undefined when socket itself is undefined', () => { + const req = { headers: {} } as unknown as Request; + expect(getClientIp(req)).toBeUndefined(); + }); + }); +}); diff --git a/src/utils/client-ip.utils.ts b/src/utils/client-ip.utils.ts new file mode 100644 index 0000000..34ab670 --- /dev/null +++ b/src/utils/client-ip.utils.ts @@ -0,0 +1,63 @@ +import { Request } from 'express'; + +/** + * Trusted private/loopback CIDR ranges whose forwarded-for addresses are + * accepted without further validation. Requests arriving from outside these + * ranges are not considered trusted proxies, so their X-Forwarded-For header + * is ignored. + */ +const TRUSTED_PROXY_CIDRS = [ + { prefix: '127.', bits: 8 }, // 127.0.0.0/8 — loopback + { prefix: '10.', bits: 8 }, // 10.0.0.0/8 — private class A + { prefix: '172.16.', bits: 12 }, // 172.16.0.0/12 — private class B + { prefix: '192.168.', bits: 16 }, // 192.168.0.0/16 — private class C + { prefix: '::1', bits: 128 }, // IPv6 loopback + { prefix: 'fc00:', bits: 7 }, // IPv6 unique-local + { prefix: 'fd', bits: 8 }, // IPv6 unique-local (fd00::/8) +]; + +/** + * Returns true when `ip` originates from a trusted proxy address. + * + * Only private/loopback ranges are trusted by default. Pass a custom + * `isTrustedProxy` predicate to override (useful in tests or when the + * deployment uses a known set of proxy IPs). + */ +function isFromTrustedProxy(ip: string): boolean { + return TRUSTED_PROXY_CIDRS.some(({ prefix }) => + ip.startsWith(prefix) + ); +} + +/** + * Extracts the real client IP address from an Express request. + * + * Resolution order: + * 1. If the socket address is a trusted proxy, read the *first* IP from the + * `X-Forwarded-For` header (the leftmost value, which is the original + * client before any proxies appended their own address). + * 2. Fall back to the direct socket address (`req.socket.remoteAddress`). + * + * The raw body is never read and the function never mutates the request. + * + * @param req - Express Request object. + * @param trusted - Optional override predicate for trusted-proxy check. + * Defaults to the private/loopback CIDR check above. + */ +export function getClientIp( + req: Request, + trusted: (ip: string) => boolean = isFromTrustedProxy +): string | undefined { + const socketIp = req.socket?.remoteAddress ?? ''; + + if (trusted(socketIp)) { + const forwarded = req.headers['x-forwarded-for']; + if (forwarded) { + const raw = Array.isArray(forwarded) ? forwarded[0] : forwarded; + const firstIp = raw.split(',')[0].trim(); + if (firstIp) return firstIp; + } + } + + return socketIp || undefined; +}