diff --git a/.changeset/nervous-dogs-pull.md b/.changeset/nervous-dogs-pull.md new file mode 100644 index 0000000000..fd62b751d8 --- /dev/null +++ b/.changeset/nervous-dogs-pull.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-proxy-backend': patch +--- + +Fix for proxy-backend plugin when global-agent is enabled diff --git a/plugins/proxy-backend/src/service/router.test.ts b/plugins/proxy-backend/src/service/router.test.ts index 3005b12766..ab293cc9b7 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, diff --git a/plugins/proxy-backend/src/service/router.ts b/plugins/proxy-backend/src/service/router.ts index 917ec2383c..5c306de966 100644 --- a/plugins/proxy-backend/src/service/router.ts +++ b/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( [ @@ -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