fix(fixRequestBody): handle invalid request (#1091)

Author: chimurai

src/handlers/fix-request-body.ts

@@ -2,10 +2,20 @@ 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): void {
+export function fixRequestBody(
+  proxyReq: http.ClientRequest,
+  req: http.IncomingMessage,
+  res: http.ServerResponse
+): void {
   const requestBody = (req as Request).body;
 
   if (!requestBody) {
@@ -18,6 +28,18 @@ export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingM
     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));
@@ -30,3 +52,14 @@ export function fixRequestBody(proxyReq: http.ClientRequest, req: http.IncomingM
     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

@@ -1,4 +1,5 @@
-import { ClientRequest } from 'http';
+import { Socket } from 'net';
+import { ClientRequest, ServerResponse, IncomingMessage } from 'http';
 import * as querystring from 'querystring';
 
 import { fixRequestBody } from '../../src/handlers/fix-request-body';
@@ -11,14 +12,19 @@ const fakeProxyRequest = () => {
   return proxyRequest;
 };
 
+const fakeProxyResponse = (): ServerResponse => {
+  const res = new ServerResponse(new IncomingMessage(new Socket()));
+  return res;
+};
+
 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);
+    fixRequestBody(proxyRequest, { body: undefined } as Request, fakeProxyResponse());
 
     expect(proxyRequest.setHeader).not.toHaveBeenCalled();
     expect(proxyRequest.write).not.toHaveBeenCalled();
@@ -31,7 +37,7 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: {} } as Request);
+    fixRequestBody(proxyRequest, { body: {} } as Request, fakeProxyResponse());
 
     expect(proxyRequest.setHeader).toHaveBeenCalled();
     expect(proxyRequest.write).toHaveBeenCalled();
@@ -44,7 +50,11 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
+    fixRequestBody(
+      proxyRequest,
+      { body: { someField: 'some value' } } as Request,
+      fakeProxyResponse()
+    );
 
     const expectedBody = JSON.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -58,7 +68,11 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
+    fixRequestBody(
+      proxyRequest,
+      { body: { someField: 'some value' } } as Request,
+      fakeProxyResponse()
+    );
 
     const expectedBody = querystring.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -72,7 +86,11 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
+    fixRequestBody(
+      proxyRequest,
+      { body: { someField: 'some value' } } as Request,
+      fakeProxyResponse()
+    );
 
     const expectedBody = querystring.stringify({ someField: 'some value' });
     expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -86,11 +104,62 @@ describe('fixRequestBody', () => {
     jest.spyOn(proxyRequest, 'setHeader');
     jest.spyOn(proxyRequest, 'write');
 
-    fixRequestBody(proxyRequest, { body: { someField: 'some value' } } as Request);
+    fixRequestBody(
+      proxyRequest,
+      { body: { someField: 'some value' } } as Request,
+      fakeProxyResponse()
+    );
 
     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', () => {
+    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 proxyResponse = fakeProxyResponse();
+    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);
+  });
 });