From 9ce68b677b83336a439072680a57abbb9ad8d132 Mon Sep 17 00:00:00 2001 From: bobalong79 Date: Mon, 1 Mar 2021 11:30:44 +0000 Subject: [PATCH 1/4] Fix for proxy-backend plugin when global-agent is enabled --- .changeset/nervous-dogs-pull.md | 5 +++++ plugins/proxy-backend/src/service/router.ts | 18 ++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) create mode 100644 .changeset/nervous-dogs-pull.md diff --git a/.changeset/nervous-dogs-pull.md b/.changeset/nervous-dogs-pull.md new file mode 100644 index 0000000000..bf8ac8c3ca --- /dev/null +++ b/.changeset/nervous-dogs-pull.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-proxy-backend': minor +--- + +Fix for proxy-backend plugin when global-agent is enabled diff --git a/plugins/proxy-backend/src/service/router.ts b/plugins/proxy-backend/src/service/router.ts index 036e3a8d1b..1798c56cc6 100644 --- a/plugins/proxy-backend/src/service/router.ts +++ b/plugins/proxy-backend/src/service/router.ts @@ -85,11 +85,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( [ @@ -104,15 +99,18 @@ 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 + 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 From 510c2b90be107c71b3e8a4a518746b9468857c42 Mon Sep 17 00:00:00 2001 From: bobalong79 Date: Mon, 1 Mar 2021 17:54:36 +0000 Subject: [PATCH 2/4] Update changeset to patch --- .changeset/nervous-dogs-pull.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/nervous-dogs-pull.md b/.changeset/nervous-dogs-pull.md index bf8ac8c3ca..fd62b751d8 100644 --- a/.changeset/nervous-dogs-pull.md +++ b/.changeset/nervous-dogs-pull.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-proxy-backend': minor +'@backstage/plugin-proxy-backend': patch --- Fix for proxy-backend plugin when global-agent is enabled From 316b7bff6197260002b0e7c5c461d1df354c85b8 Mon Sep 17 00:00:00 2001 From: bobalong79 Date: Mon, 1 Mar 2021 18:01:44 +0000 Subject: [PATCH 3/4] Add rationale explaining why we do the header modifications in the filter --- plugins/proxy-backend/src/service/router.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/proxy-backend/src/service/router.ts b/plugins/proxy-backend/src/service/router.ts index 1798c56cc6..c4da4151bb 100644 --- a/plugins/proxy-backend/src/service/router.ts +++ b/plugins/proxy-backend/src/service/router.ts @@ -102,6 +102,11 @@ export function buildMiddleware( // 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 => { From 1fc9627a1b4e776dc80450812b41321b1c568364 Mon Sep 17 00:00:00 2001 From: bobalong79 Date: Mon, 1 Mar 2021 18:18:10 +0000 Subject: [PATCH 4/4] Update tests after moving header processing from `onProxyReq` to `filter` --- .../proxy-backend/src/service/router.test.ts | 145 ++++++++++-------- 1 file changed, 78 insertions(+), 67 deletions(-) diff --git a/plugins/proxy-backend/src/service/router.test.ts b/plugins/proxy-backend/src/service/router.test.ts index d0eca55ee0..7f7fa07391 100644 --- a/plugins/proxy-backend/src/service/router.test.ts +++ b/plugins/proxy-backend/src/service/router.test.ts @@ -68,11 +68,11 @@ describe('buildMiddleware', () => { (pathname: string, req: Partial) => 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) => 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) => 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; + 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; + 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) => boolean, + ]; - const testClientRequest = { - getHeaderNames: () => ['authorization', 'Cookie'], - removeHeader: jest.fn(), - } as Partial; + const testHeaders = { + authorization: 'mocked', + cookie: 'mocked', + } as Partial; + 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) => boolean, + ]; - const testClientRequest = { - getHeaderNames: () => ['authorization', 'Cookie', 'X-Auth-Request-User'], - removeHeader: jest.fn(), - } as Partial; + const testHeaders = { + authorization: 'mocked', + cookie: 'mocked', + 'x-auth-request-user': 'mocked', + } as Partial; + 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; 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; + expect(config).toBeDefined(); + expect(config.onProxyRes).toBeDefined(); + config.onProxyRes!( testClientResponse as http.IncomingMessage, {} as http.IncomingMessage,