From a05774ecb6a8c360f3ce028ffc4670c906b0a417 Mon Sep 17 00:00:00 2001 From: Jonathan Ginsburg Date: Wed, 27 Jan 2021 23:21:33 -0600 Subject: [PATCH 1/2] fix(server/proxy): clean up unused logic fix(server/proxy): config.protocol can control connection scheme fix(server/proxy): clean up unused logic --- lib/middleware/proxy.js | 32 +++--------- test/unit/middleware/proxy.spec.js | 80 +++++++++++++----------------- 2 files changed, 42 insertions(+), 70 deletions(-) diff --git a/lib/middleware/proxy.js b/lib/middleware/proxy.js index 7c9a43214..444effd31 100644 --- a/lib/middleware/proxy.js +++ b/lib/middleware/proxy.js @@ -1,6 +1,4 @@ const url = require('url') -const { Agent: httpAgent } = require('http') -const { Agent: httpsAgent } = require('https') const httpProxy = require('http-proxy') const _ = require('lodash') @@ -34,24 +32,17 @@ function parseProxyConfig (proxies, config) { const hostname = proxyDetails.hostname || config.hostname const protocol = proxyDetails.protocol || config.protocol - const https = proxyDetails.protocol === 'https:' - let port - if (proxyDetails.port) { - port = proxyDetails.port - } else if (proxyDetails.protocol) { - port = https ? '443' : '80' - } else { - port = config.port + const defaultPorts = { + 'http:': '80', + 'https:': '443' } + const port = proxyDetails.port || defaultPorts[proxyDetails.protocol] || config.port const changeOrigin = proxyConfiguration.changeOrigin || false - const Agent = https ? httpsAgent : httpAgent - const agent = new Agent({ keepAlive: true }) const proxy = httpProxy.createProxyServer({ - target: { host: hostname, port, https, protocol }, + target: { host: hostname, port, protocol }, xfwd: true, changeOrigin: changeOrigin, - secure: config.proxyValidateSSL, - agent + secure: config.proxyValidateSSL }) ;['proxyReq', 'proxyRes'].forEach(function (name) { @@ -71,7 +62,7 @@ function parseProxyConfig (proxies, config) { res.destroy() }) - return { path: proxyPath, baseUrl: pathname, host: hostname, port, https, proxy, agent } + return { path: proxyPath, baseUrl: pathname, host: hostname, port, proxy } }), 'path').reverse() } @@ -117,14 +108,7 @@ function createProxyHandler (proxies, urlRoot) { return createProxy } -exports.create = function (/* config */config, /* config.proxies */proxies, /* emitter */emitter) { +exports.create = function (/* config */config, /* config.proxies */proxies) { const proxyRecords = parseProxyConfig(proxies, config) - emitter.on('exit', (done) => { - log.debug('Destroying proxy agents') - proxyRecords.forEach((proxyRecord) => { - proxyRecord.agent.destroy() - }) - done() - }) return createProxyHandler(proxyRecords, config.urlRoot) } diff --git a/test/unit/middleware/proxy.spec.js b/test/unit/middleware/proxy.spec.js index 6bfc2c80c..d12f255ed 100644 --- a/test/unit/middleware/proxy.spec.js +++ b/test/unit/middleware/proxy.spec.js @@ -146,10 +146,16 @@ describe('middleware.proxy', () => { host: 'localhost', port: '8000', baseUrl: '/', - path: '/base/', - https: false + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist + expect(parsedProxyConfig[0].proxy).to.containSubset({ + options: { + target: { + protocol: 'http:' + } + } + }) }) it('should set default http port', () => { @@ -160,10 +166,16 @@ describe('middleware.proxy', () => { host: 'localhost', port: '80', baseUrl: '/', - path: '/base/', - https: false + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist + expect(parsedProxyConfig[0].proxy).to.containSubset({ + options: { + target: { + protocol: 'http:' + } + } + }) }) it('should set default https port', () => { @@ -174,8 +186,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: '443', baseUrl: '/', - path: '/base/', - https: true + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist expect(parsedProxyConfig[0].proxy).to.containSubset({ @@ -195,10 +206,16 @@ describe('middleware.proxy', () => { host: 'localhost', port: '8000', baseUrl: '/proxy', - path: '/base', - https: false + path: '/base' }) expect(parsedProxyConfig[0].proxy).to.exist + expect(parsedProxyConfig[0].proxy).to.containSubset({ + options: { + target: { + protocol: 'http:' + } + } + }) }) it('should determine protocol', () => { @@ -209,8 +226,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: '8000', baseUrl: '', - path: '/base', - https: true + path: '/base' }) expect(parsedProxyConfig[0].proxy).to.exist expect(parsedProxyConfig[0].proxy).to.containSubset({ @@ -231,8 +247,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: 9877, baseUrl: '/proxy/test', - path: '/base', - https: false + path: '/base' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -246,8 +261,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: 9877, baseUrl: '/proxy/test/', - path: '/base/', - https: false + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -261,8 +275,7 @@ describe('middleware.proxy', () => { host: 'krinkle.dev', port: '80', baseUrl: '/w', - path: '/w', - https: false + path: '/w' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -276,8 +289,7 @@ describe('middleware.proxy', () => { host: 'krinkle.dev', port: '443', baseUrl: '/w', - path: '/w', - https: true + path: '/w' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -290,8 +302,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: '8000', baseUrl: '/proxy/test/', - path: '/base/', - https: false + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -307,16 +318,14 @@ describe('middleware.proxy', () => { host: 'gstatic.com', port: '80', baseUrl: '/something', - path: '/sub/some', - https: false + path: '/sub/some' }) expect(parsedProxyConfig[0].proxy).to.exist expect(parsedProxyConfig[1]).to.containSubset({ host: 'localhost', port: '9000', baseUrl: '', - path: '/sub', - https: false + path: '/sub' }) expect(parsedProxyConfig[1].proxy).to.exist }) @@ -331,8 +340,7 @@ describe('middleware.proxy', () => { host: 'localhost', port: '8000', baseUrl: '/', - path: '/base/', - https: false + path: '/base/' }) expect(parsedProxyConfig[0].proxy).to.exist }) @@ -352,24 +360,4 @@ describe('middleware.proxy', () => { it('should handle empty proxy config', () => { expect(m.parseProxyConfig({})).to.deep.equal([]) }) - - it('should use http agent with keepAlive=true', () => { - const proxy = { '/base': 'http://localhost:8000/proxy' } - const parsedProxyConfig = m.parseProxyConfig(proxy, {}) - expect(parsedProxyConfig).to.have.length(1) - expect(parsedProxyConfig[0].proxy.options.agent).to.containSubset({ - keepAlive: true, - protocol: 'http:' - }) - }) - - it('should use https agent with keepAlive=true', () => { - const proxy = { '/base': 'https://localhost:8000/proxy' } - const parsedProxyConfig = m.parseProxyConfig(proxy, {}) - expect(parsedProxyConfig).to.have.length(1) - expect(parsedProxyConfig[0].proxy.options.agent).to.containSubset({ - keepAlive: true, - protocol: 'https:' - }) - }) }) From 3556ebd06ca287d0cff73f2a278c1fcc04a1a820 Mon Sep 17 00:00:00 2001 From: Jonathan Ginsburg Date: Tue, 4 May 2021 19:39:06 -0500 Subject: [PATCH 2/2] fix(server): clean up vestigial code from proxy When using the proxy feature of Karma, the target value can include the [scheme](https://tools.ietf.org/html/std66#section-3.1). It was used to determine the `https` variable to be sent to the [`http-proxy`](https://www.npmjs.com/package/http-proxy) `.createProxyServer` method. However, it is now disregarded by that package. Therefore, this PR cleans it up. --- lib/middleware/proxy.js | 18 +++++++++++++++--- test/unit/middleware/proxy.spec.js | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/lib/middleware/proxy.js b/lib/middleware/proxy.js index 444effd31..6b0fcf730 100644 --- a/lib/middleware/proxy.js +++ b/lib/middleware/proxy.js @@ -1,4 +1,6 @@ const url = require('url') +const { Agent: httpAgent } = require('http') +const { Agent: httpsAgent } = require('https') const httpProxy = require('http-proxy') const _ = require('lodash') @@ -38,11 +40,14 @@ function parseProxyConfig (proxies, config) { } const port = proxyDetails.port || defaultPorts[proxyDetails.protocol] || config.port const changeOrigin = proxyConfiguration.changeOrigin || false + const Agent = protocol === 'https:' ? httpsAgent : httpAgent + const agent = new Agent({ keepAlive: true }) const proxy = httpProxy.createProxyServer({ target: { host: hostname, port, protocol }, xfwd: true, changeOrigin: changeOrigin, - secure: config.proxyValidateSSL + secure: config.proxyValidateSSL, + agent }) ;['proxyReq', 'proxyRes'].forEach(function (name) { @@ -62,7 +67,7 @@ function parseProxyConfig (proxies, config) { res.destroy() }) - return { path: proxyPath, baseUrl: pathname, host: hostname, port, proxy } + return { path: proxyPath, baseUrl: pathname, host: hostname, port, proxy, agent } }), 'path').reverse() } @@ -108,7 +113,14 @@ function createProxyHandler (proxies, urlRoot) { return createProxy } -exports.create = function (/* config */config, /* config.proxies */proxies) { +exports.create = function (/* config */config, /* config.proxies */proxies, /* emitter */emitter) { const proxyRecords = parseProxyConfig(proxies, config) + emitter.on('exit', (done) => { + log.debug('Destroying proxy agents') + proxyRecords.forEach((proxyRecord) => { + proxyRecord.agent.destroy() + }) + done() + }) return createProxyHandler(proxyRecords, config.urlRoot) } diff --git a/test/unit/middleware/proxy.spec.js b/test/unit/middleware/proxy.spec.js index d12f255ed..bdc83e50b 100644 --- a/test/unit/middleware/proxy.spec.js +++ b/test/unit/middleware/proxy.spec.js @@ -360,4 +360,24 @@ describe('middleware.proxy', () => { it('should handle empty proxy config', () => { expect(m.parseProxyConfig({})).to.deep.equal([]) }) + + it('should use http agent with keepAlive=true', () => { + const proxy = { '/base': 'http://localhost:8000/proxy' } + const parsedProxyConfig = m.parseProxyConfig(proxy, {}) + expect(parsedProxyConfig).to.have.length(1) + expect(parsedProxyConfig[0].proxy.options.agent).to.containSubset({ + keepAlive: true, + protocol: 'http:' + }) + }) + + it('should use https agent with keepAlive=true', () => { + const proxy = { '/base': 'https://localhost:8000/proxy' } + const parsedProxyConfig = m.parseProxyConfig(proxy, {}) + expect(parsedProxyConfig).to.have.length(1) + expect(parsedProxyConfig[0].proxy.options.agent).to.containSubset({ + keepAlive: true, + protocol: 'https:' + }) + }) })