From e0264d189b32c1d9e7c47a999bf08858a6da78d5 Mon Sep 17 00:00:00 2001 From: yyyyaaa Date: Fri, 6 Nov 2026 20:31:15 +0700 Subject: [PATCH] fix(graphql-server): preserve request body parser errors --- .../__tests__/error-handler.test.ts | 88 +++++++++++++++++++ .../server/src/middleware/error-handler.ts | 52 +++++++++-- 2 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 graphql/server/src/middleware/__tests__/error-handler.test.ts diff --git a/graphql/server/src/middleware/__tests__/error-handler.test.ts b/graphql/server/src/middleware/__tests__/error-handler.test.ts new file mode 100644 index 0000000000..32ec2f678c --- /dev/null +++ b/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(); + }); +}); diff --git a/graphql/server/src/middleware/error-handler.ts b/graphql/server/src/middleware/error-handler.ts index 56862f52fa..510591969b 100644 --- a/graphql/server/src/middleware/error-handler.ts +++ b/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); };