Merge pull request #4740 from Liverpool-Victoria/bug/plugin-backend-proxy-with-agent

Fix for proxy-backend plugin when global-agent is enabled

plugins/proxy-backend/src/service/router.test.ts

@@ -68,11 +68,11 @@ describe('buildMiddleware', () => {
       (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
       ProxyMiddlewareConfig,
     ];
-    expect(filter('', { method: 'GET' })).toBe(true);
-    expect(filter('', { method: 'POST' })).toBe(true);
-    expect(filter('', { method: 'PUT' })).toBe(true);
-    expect(filter('', { method: 'PATCH' })).toBe(true);
-    expect(filter('', { method: 'DELETE' })).toBe(true);
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
 
     expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
     expect(fullConfig.changeOrigin).toBe(true);
@@ -91,11 +91,11 @@ describe('buildMiddleware', () => {
       (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
       ProxyMiddlewareConfig,
     ];
-    expect(filter('', { method: 'GET' })).toBe(true);
-    expect(filter('', { method: 'POST' })).toBe(false);
-    expect(filter('', { method: 'PUT' })).toBe(false);
-    expect(filter('', { method: 'PATCH' })).toBe(false);
-    expect(filter('', { method: 'DELETE' })).toBe(true);
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(false);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(false);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(false);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
 
     expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
     expect(fullConfig.changeOrigin).toBe(true);
@@ -109,38 +109,37 @@ describe('buildMiddleware', () => {
 
     expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
 
-    const config = mockCreateProxyMiddleware.mock
-      .calls[0][1] as ProxyMiddlewareConfig;
+    const [filter] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+    ];
 
-    const testClientRequest = {
-      getHeaderNames: () => [
-        'cache-control',
-        'content-language',
-        'content-length',
-        'content-type',
-        'expires',
-        'last-modified',
-        'pragma',
-        'host',
-        'accept',
-        'accept-language',
-        'user-agent',
-        'cookie',
-      ],
-      removeHeader: jest.fn(),
-    } as Partial<http.ClientRequest>;
+    const testHeaders = {
+      'cache-control': 'mocked',
+      'content-language': 'mocked',
+      'content-length': 'mocked',
+      'content-type': 'mocked',
+      expires: 'mocked',
+      'last-modified': 'mocked',
+      pragma: 'mocked',
+      host: 'mocked',
+      accept: 'mocked',
+      'accept-language': 'mocked',
+      'user-agent': 'mocked',
+      cookie: 'mocked',
+    } as Partial<http.IncomingHttpHeaders>;
+    const expectedHeaders = { ...testHeaders } as Partial<
+      http.IncomingHttpHeaders
+    >;
+    delete expectedHeaders.cookie;
 
-    expect(config).toBeDefined();
-    expect(config.onProxyReq).toBeDefined();
+    expect(testHeaders).toBeDefined();
+    expect(expectedHeaders).toBeDefined();
+    expect(testHeaders).not.toEqual(expectedHeaders);
+    expect(filter).toBeDefined();
 
-    config.onProxyReq!(
-      testClientRequest as http.ClientRequest,
-      {} as http.IncomingMessage,
-      {} as http.ServerResponse,
-    );
+    filter!('', { method: 'GET', headers: testHeaders });
 
-    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
-    expect(testClientRequest.removeHeader).toHaveBeenCalledWith('cookie');
+    expect(testHeaders).toEqual(expectedHeaders);
   });
 
   it('permits default and configured headers', async () => {
@@ -153,22 +152,27 @@ describe('buildMiddleware', () => {
 
     expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
 
-    const config = mockCreateProxyMiddleware.mock
-      .calls[0][1] as ProxyMiddlewareConfig;
+    const [filter] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+    ];
 
-    const testClientRequest = {
-      getHeaderNames: () => ['authorization', 'Cookie'],
-      removeHeader: jest.fn(),
-    } as Partial<http.ClientRequest>;
+    const testHeaders = {
+      authorization: 'mocked',
+      cookie: 'mocked',
+    } as Partial<http.IncomingHttpHeaders>;
+    const expectedHeaders = { ...testHeaders } as Partial<
+      http.IncomingHttpHeaders
+    >;
+    delete expectedHeaders.cookie;
 
-    config.onProxyReq!(
-      testClientRequest as http.ClientRequest,
-      {} as http.IncomingMessage,
-      {} as http.ServerResponse,
-    );
+    expect(testHeaders).toBeDefined();
+    expect(expectedHeaders).toBeDefined();
+    expect(testHeaders).not.toEqual(expectedHeaders);
+    expect(filter).toBeDefined();
 
-    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
-    expect(testClientRequest.removeHeader).toHaveBeenCalledWith('Cookie');
+    filter!('', { method: 'GET', headers: testHeaders });
+
+    expect(testHeaders).toEqual(expectedHeaders);
   });
 
   it('permits configured headers', async () => {
@@ -179,24 +183,28 @@ describe('buildMiddleware', () => {
 
     expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
 
-    const config = mockCreateProxyMiddleware.mock
-      .calls[0][1] as ProxyMiddlewareConfig;
+    const [filter] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+    ];
 
-    const testClientRequest = {
-      getHeaderNames: () => ['authorization', 'Cookie', 'X-Auth-Request-User'],
-      removeHeader: jest.fn(),
-    } as Partial<http.ClientRequest>;
+    const testHeaders = {
+      authorization: 'mocked',
+      cookie: 'mocked',
+      'x-auth-request-user': 'mocked',
+    } as Partial<http.IncomingHttpHeaders>;
+    const expectedHeaders = { ...testHeaders } as Partial<
+      http.IncomingHttpHeaders
+    >;
+    delete expectedHeaders['x-auth-request-user'];
 
-    config.onProxyReq!(
-      testClientRequest as http.ClientRequest,
-      {} as http.IncomingMessage,
-      {} as http.ServerResponse,
-    );
+    expect(testHeaders).toBeDefined();
+    expect(expectedHeaders).toBeDefined();
+    expect(testHeaders).not.toEqual(expectedHeaders);
+    expect(filter).toBeDefined();
 
-    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
-    expect(testClientRequest.removeHeader).toHaveBeenCalledWith(
-      'X-Auth-Request-User',
-    );
+    filter!('', { method: 'GET', headers: testHeaders });
+
+    expect(testHeaders).toEqual(expectedHeaders);
   });
 
   it('responds default headers', async () => {
@@ -223,7 +231,7 @@ describe('buildMiddleware', () => {
     } as Partial<http.IncomingMessage>;
 
     expect(config).toBeDefined();
-    expect(config.onProxyReq).toBeDefined();
+    expect(config.onProxyRes).toBeDefined();
 
     config.onProxyRes!(
       testClientResponse as http.IncomingMessage,
@@ -260,6 +268,9 @@ describe('buildMiddleware', () => {
       },
     } as Partial<http.IncomingMessage>;
 
+    expect(config).toBeDefined();
+    expect(config.onProxyRes).toBeDefined();
+
     config.onProxyRes!(
       testClientResponse as http.IncomingMessage,
       {} as http.IncomingMessage,

plugins/proxy-backend/src/service/router.ts

@@ -94,11 +94,6 @@ export function buildMiddleware(
   // Attach the logger to the proxy config
   fullConfig.logProvider = () => logger;
 
-  // Only permit the allowed HTTP methods if configured
-  const filter = (_pathname: string, req: http.IncomingMessage): boolean => {
-    return fullConfig?.allowedMethods?.includes(req.method!) ?? true;
-  };
-
   // Only return the allowed HTTP headers to not forward unwanted secret headers
   const requestHeaderAllowList = new Set<string>(
     [
@@ -113,15 +108,23 @@ export function buildMiddleware(
     ].map(h => h.toLocaleLowerCase()),
   );
 
-  // only forward the allowed headers in client->backend
-  fullConfig.onProxyReq = (proxyReq: http.ClientRequest) => {
-    const headerNames = proxyReq.getHeaderNames();
-
+  // Use the custom middleware filter to do two things:
+  //  1. Remove any headers not in the allow list to stop them being forwarded
+  //  2. Only permit the allowed HTTP methods if configured
+  //
+  // We are filtering the proxy request headers here rather than in
+  // `onProxyReq` becuase when global-agent is enabled then `onProxyReq`
+  // fires _after_ the agent has already sent the headers to the proxy
+  // target, causing a ERR_HTTP_HEADERS_SENT crash
+  const filter = (_pathname: string, req: http.IncomingMessage): boolean => {
+    const headerNames = Object.keys(req.headers);
     headerNames.forEach(h => {
       if (!requestHeaderAllowList.has(h.toLocaleLowerCase())) {
-        proxyReq.removeHeader(h);
+        delete req.headers[h];
       }
     });
+
+    return fullConfig?.allowedMethods?.includes(req.method!) ?? true;
   };
 
   // Only forward the allowed HTTP headers to not forward unwanted secret headers