Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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'));
Expand Down
169 changes: 169 additions & 0 deletions src/middlewares/body-parse-error.middleware.test.ts
Original file line number Diff line number Diff line change
@@ -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<Response> {
const res: Partial<Response> = {};
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();
});
});
});
68 changes: 68 additions & 0 deletions src/middlewares/body-parse-error.middleware.ts
Original file line number Diff line number Diff line change
@@ -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',
});
};
96 changes: 96 additions & 0 deletions src/utils/client-ip.utils.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
Loading
Loading