fix(fixRequestBody): check readableLength (#1097)

Author: chimurai

src/handlers/fix-request-body.ts

@@ -2,20 +2,15 @@ import type * as http from 'http';
 import type { Request } from '../types';
 import * as querystring from 'querystring';
 
-type HandleBadRequestArgs = {
-  proxyReq: http.ClientRequest;
-  req: http.IncomingMessage;
-  res: http.ServerResponse;
-};
-
 /**
  * Fix proxied body if bodyParser is involved.
  */
-export function fixRequestBody(
-  proxyReq: http.ClientRequest,
-  req: http.IncomingMessage,
-  res: http.ServerResponse
-): void {
+export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingMessage): void {
+  // skip fixRequestBody() when req.readableLength not 0 (bodyParser failure)
+  if (req.readableLength !== 0) {
+    return;
+  }
+
   const requestBody = (req as Request).body;
 
   if (!requestBody) {
@@ -28,18 +23,6 @@ export function fixRequestBody(
     return;
   }
 
-  // Handle bad request when unexpected "Connect: Upgrade" header is provided
-  if (/upgrade/gi.test(proxyReq.getHeader('Connection') as string)) {
-    handleBadRequest({ proxyReq, req, res });
-    return;
-  }
-
-  // Handle bad request when invalid request body is provided
-  if (hasInvalidKeys(requestBody)) {
-    handleBadRequest({ proxyReq, req, res });
-    return;
-  }
-
   const writeBody = (bodyData: string) => {
     // deepcode ignore ContentLengthInCode: bodyParser fix
     proxyReq.setHeader('Content-Length', Buffer.byteLength(bodyData));
@@ -52,14 +35,3 @@ export function fixRequestBody(
     writeBody(querystring.stringify(requestBody));
   }
 }
-
-function hasInvalidKeys(obj) {
-  return Object.keys(obj).some((key) => /[\n\r]/.test(key));
-}
-
-function handleBadRequest({ proxyReq, req, res }: HandleBadRequestArgs) {
-  res.writeHead(400);
-  res.end('Bad Request');
-  proxyReq.destroy();
-  req.destroy();
-}

test/unit/fix-request-body.spec.ts

@@ -17,14 +17,21 @@ const fakeProxyResponse = (): ServerResponse => {
   return res;
 };
 
+const createRequestWithBody = (body: unknown): Request => {
+  const req = new IncomingMessage(new Socket()) as Request;
+  req.url = '/test_path';
+  req.body = body;
+  return req;
+};
+
 describe('fixRequestBody', () => {
   it('should not write when body is undefined', () => {
     const proxyRequest = fakeProxyRequest();
 
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: undefined } as Request, fakeProxyResponse());
+    fixRequestBody(proxyRequest, createRequestWithBody(undefined));
 
     expect(proxyRequest.setHeader).not.toHaveBeenCalled();
     expect(proxyRequest.write).not.toHaveBeenCalled();
@@ -37,7 +44,7 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: {} } as Request, fakeProxyResponse());
+    fixRequestBody(proxyRequest, createRequestWithBody({}));
 
     expect(proxyRequest.setHeader).toHaveBeenCalled();
     expect(proxyRequest.write).toHaveBeenCalled();
@@ -50,11 +57,7 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(
-      proxyRequest,
-      { body: { someField: 'some value' } } as Request,
-      fakeProxyResponse()
-    );
+    fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
 
     const expectedBody = JSON.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -68,11 +71,7 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(
-      proxyRequest,
-      { body: { someField: 'some value' } } as Request,
-      fakeProxyResponse()
-    );
+    fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
 
     const expectedBody = querystring.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -86,11 +85,7 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(
-      proxyRequest,
-      { body: { someField: 'some value' } } as Request,
-      fakeProxyResponse()
-    );
+    fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
 
     const expectedBody = querystring.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -104,62 +99,34 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(
-      proxyRequest,
-      { body: { someField: 'some value' } } as Request,
-      fakeProxyResponse()
-    );
+    fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
 
     const expectedBody = JSON.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
     expect(proxyRequest.write).toHaveBeenCalledTimes(1);
     expect(proxyRequest.write).toHaveBeenCalledWith(expectedBody);
   });
 
-  it('should return 400 and abort request on "Connection: Upgrade" header', () => {
+  it('should not fixRequestBody() when there bodyParser fails', () => {
     const proxyRequest = fakeProxyRequest();
-    const request = { body: { someField: 'some value' } } as Request;
-
-    proxyRequest.destroy = jest.fn();
-    request.destroy = jest.fn();
-
-    const proxyResponse = fakeProxyResponse();
-    proxyRequest.setHeader('connection', 'upgrade');
-    proxyRequest.setHeader('content-type', 'application/x-www-form-urlencoded');
-
-    jest.spyOn(proxyRequest, 'destroy');
-    jest.spyOn(request, 'destroy');
-    jest.spyOn(proxyResponse, 'writeHead');
-    jest.spyOn(proxyResponse, 'end');
-
-    fixRequestBody(proxyRequest, request, proxyResponse);
-
-    expect(proxyResponse.writeHead).toHaveBeenCalledWith(400);
-    expect(proxyResponse.end).toHaveBeenCalledTimes(1);
-    expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
-    expect(request.destroy).toHaveBeenCalledTimes(1);
-  });
-
-  it('should return 400 and abort request on invalid request data', () => {
-    const proxyRequest = fakeProxyRequest();
-    const request = { body: { 'INVALID \n\r DATA': '' } } as Request;
-
-    proxyRequest.destroy = jest.fn();
-    request.destroy = jest.fn();
+    const request = {
+      get readableLength() {
+        return 4444; // simulate bodyParser failure
+      },
+    } as Request;
 
     const proxyResponse = fakeProxyResponse();
     proxyRequest.setHeader('content-type', 'application/x-www-form-urlencoded');
 
+    jest.spyOn(proxyRequest, 'write');
     jest.spyOn(proxyRequest, 'destroy');
-    jest.spyOn(request, 'destroy');
     jest.spyOn(proxyResponse, 'writeHead');
     jest.spyOn(proxyResponse, 'end');
 
-    fixRequestBody(proxyRequest, request, proxyResponse);
+    fixRequestBody(proxyRequest, request);
 
-    expect(proxyResponse.writeHead).toHaveBeenCalledWith(400);
-    expect(proxyResponse.end).toHaveBeenCalledTimes(1);
-    expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
-    expect(request.destroy).toHaveBeenCalledTimes(1);
+    expect(proxyResponse.end).toHaveBeenCalledTimes(0);
+    expect(proxyRequest.write).toHaveBeenCalledTimes(0);
+    expect(proxyRequest.destroy).toHaveBeenCalledTimes(0);
   });
 });