fix(graphql-server): return HTTP 413 for oversized JSON

graphql/server/src/middleware/__tests__/error-handler.test.ts

@@ -0,0 +1,88 @@
+import { Logger } from '@pgpmjs/logger';
+import express from 'express';
+import supertest from 'supertest';
+
+import { createAgenticRouter } from '../../agentic';
+import { errorHandler } from '../error-handler';
+
+const jsonLimit = 100 * 1024;
+const validJson = '{"query":"{ __typename }"}';
+const testBody = validJson.padEnd(jsonLimit);
+
+function createTestApp() {
+  const app = express();
+  app.use(createAgenticRouter());
+  app.post('/graphql', (req, res) => res.json({ data: req.body }));
+  app.get('/internal-error', () => {
+    throw new Error('Server failure');
+  });
+  app.use(errorHandler);
+  return supertest(app);
+}
+
+describe('HTTP request body errors', () => {
+  const request = createTestApp();
+
+  beforeEach(() => {
+    jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
+    jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  it('accepts valid GraphQL JSON at the Express body limit', async () => {
+    const res = await request.post('/graphql').type('json').send(testBody);
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({ data: { query: '{ __typename }' } });
+  });
+
+  it('returns a GraphQL 413 with the parser limit for an oversized JSON body', async () => {
+    const res = await request.post('/graphql').type('json').send(testBody + ' ');
+    expect(res.status).toBe(413);
+    expect(res.body).toEqual({
+      errors: [{
+        message: 'The request body is too large.',
+        extensions: { code: 'REQUEST_TOO_LARGE', class: 'public', http: 413, context: { limit: jsonLimit } }
+      }]
+    });
+    expect(Logger.prototype.warn).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 413 }));
+    expect(Logger.prototype.error).not.toHaveBeenCalled();
+  });
+
+  it('returns a GraphQL 400 without reflecting malformed JSON in the response or log', async () => {
+    const malformed = '{"query": "example", "value": invalid-private-input}';
+    const res = await request.post('/graphql').type('json').send(malformed);
+    expect(res.status).toBe(400);
+    expect(res.body).toEqual({
+      errors: [{
+        message: 'The request body contains invalid JSON.',
+        extensions: { code: 'BAD_USER_INPUT', class: 'public', http: 400 }
+      }]
+    });
+    expect(Logger.prototype.warn).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 400 }));
+    expect(JSON.stringify(jest.mocked(Logger.prototype.warn).mock.calls)).not.toContain('invalid-private-input');
+    expect(Logger.prototype.error).not.toHaveBeenCalled();
+  });
+
+  it('preserves a parser 415 for unsupported encoding with the REST JSON envelope', async () => {
+    const res = await request.post('/v1/threads')
+      .type('json')
+      .set('Content-Encoding', 'unsupported')
+      .send('{}');
+    expect(res.status).toBe(415);
+    expect(res.body).toEqual({
+      error: { code: 'INVALID_REQUEST_BODY', message: 'Unsupported Media Type' }
+    });
+    expect(Logger.prototype.warn).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 415 }));
+  });
+
+  it('still responds with HTTP 500 and logs unexpected errors at error level', async () => {
+    const res = await request.get('/internal-error').accept('json');
+    expect(res.status).toBe(500);
+    expect(res.body.error.code).toBe('INTERNAL_ERROR');
+    expect(Logger.prototype.error).toHaveBeenCalled();
+    expect(Logger.prototype.warn).not.toHaveBeenCalled();
+  });
+});

graphql/server/src/middleware/error-handler.ts

@@ -1,11 +1,15 @@
 import './types';
 
+import { STATUS_CODES } from 'node:http';
+
+import { ConstructiveError, errors } from '@constructive-io/errors';
 import { Logger } from '@pgpmjs/logger';
 import type { ErrorRequestHandler, NextFunction, Request, Response } from 'express';
 
 import errorPage50x from '../errors/50x';
 import errorPage404Message from '../errors/404-message';
 import { isApiError } from '../errors/api-errors';
+import { respondWithGraphQLError } from '../errors/graphql-response';
 
 const log = new Logger('error-handler');
 
@@ -21,8 +25,37 @@ interface ErrorResponse {
   code: string;
   message: string;
   logLevel: 'warn' | 'error';
+  bodyError?: ConstructiveError;
+}
+
+interface BodyParserError extends Error {
+  expose?: boolean;
+  type?: string;
+  status?: number;
+  statusCode?: number;
+  limit?: number;
 }
 
+const getRequestBodyError = (err: BodyParserError): ConstructiveError | undefined => {
+  const status = err.statusCode ?? err.status;
+  if (err.expose !== true || typeof err.type !== 'string' || !Number.isInteger(status) || status < 400 || status >= 500) {
+    return undefined;
+  }
+  if (err.type === 'entity.too.large' && status === 413) {
+    return errors.REQUEST_TOO_LARGE(typeof err.limit === 'number' ? { limit: err.limit } : {});
+  }
+  if (err.type === 'entity.parse.failed' && status === 400) {
+    // JSON.parse errors can include fragments of the request body.
+    return errors.BAD_USER_INPUT({}, 'The request body contains invalid JSON.');
+  }
+  return new ConstructiveError({
+    code: 'INVALID_REQUEST_BODY',
+    message: STATUS_CODES[status] ?? 'The request body is invalid.',
+    errorClass: 'public',
+    http: status
+  });
+};
+
 const isCsrfError = (err: Error): boolean => {
   const code = (err as unknown as { code?: string }).code;
   return typeof code === 'string' && code.startsWith('CSRF_');
@@ -41,6 +74,10 @@ const categorizeError = (err: Error): ErrorResponse => {
     const code = (err as unknown as { code: string }).code;
     return { statusCode: 403, code, message: err.message, logLevel: 'warn' };
   }
+  const bodyError = getRequestBodyError(err);
+  if (bodyError) {
+    return { statusCode: bodyError.http, code: bodyError.code, message: bodyError.message, logLevel: 'warn', bodyError };
+  }
   if (err.message?.includes('ECONNREFUSED') || err.message?.includes('connection terminated')) {
     return { statusCode: 503, code: 'SERVICE_UNAVAILABLE', message: err.message, logLevel: 'error' };
   }
@@ -50,15 +87,18 @@ const categorizeError = (err: Error): ErrorResponse => {
   return { statusCode: 500, code: 'INTERNAL_ERROR', message: err.message, logLevel: 'error' };
 };
 
-const sendResponse = (req: Request, res: Response, { statusCode, code, message }: ErrorResponse): void => {
-  if (wantsJson(req)) {
+const sendResponse = (req: Request, res: Response, { statusCode, code, message, bodyError }: ErrorResponse): void => {
+  if (bodyError && req.path === '/graphql') {
+    respondWithGraphQLError(res, bodyError, { status: statusCode });
+  } else if (wantsJson(req)) {
     res.status(statusCode).json({ error: { code, message, requestId: req.requestId } });
   } else {
     res.status(statusCode).send(statusCode >= 500 ? errorPage50x : errorPage404Message(message));
   }
 };
 
-const logError = (err: Error, req: Request, level: 'warn' | 'error'): void => {
+const logError = (err: Error, req: Request, response: ErrorResponse): void => {
+  const { statusCode, code, message, logLevel: level, bodyError } = response;
   const context = {
     requestId: req.requestId,
     path: req.path,
@@ -69,8 +109,8 @@ const logError = (err: Error, req: Request, level: 'warn' | 'error'): void => {
     clientIp: req.clientIp,
   };
 
-  if (isApiError(err)) {
-    log[level]({ event: 'api_error', code: err.code, statusCode: err.statusCode, message: err.message, ...context });
+  if (bodyError || isApiError(err)) {
+    log[level]({ event: bodyError ? 'request_body_error' : 'api_error', code, statusCode, message, ...context });
   } else {
     log[level]({ event: 'unexpected_error', name: err.name, message: err.message, stack: err.stack, ...context });
   }
@@ -83,7 +123,7 @@ export const errorHandler: ErrorRequestHandler = (err: Error, req: Request, res:
   }
 
   const response = categorizeError(err);
-  logError(err, req, response.logLevel);
+  logError(err, req, response);
   sendResponse(req, res, response);
 };