From 09c75fe15b33377fd9b0baa4f76010e52f42bc13 Mon Sep 17 00:00:00 2001 From: Terin Stock Date: Tue, 10 Mar 2015 14:56:55 -0700 Subject: [PATCH] fix: upgrade http-proxy module for bug fixes The http-proxy module was significantly outdated, and was missing many bugfixes to support node newer than 0.10, as we as a refactor and simplification of their codebase. This commit upgrades the http-proxy dependency, and refactors the proxy middleware to utilize the new API. `parseProxyConfig` (an internal function) now takes the proxy configuration object and converts it into an array of "ProxyRecord" objects, sorted by path. A ProxyRecord object contains: * `host` The remote proxy host * `port` The remote proxy port * `baseUrl` The remote proxy path * `path` The local URL path (that triggers the rewrite) * `https` Boolean to determine if the remote connection will be made with SSL. * `proxy` The instance of http-proxy. This change was necessitated by the removal of http-proxy's RoutingProxy API, and the desire to only create one proxy instance per configuration item. The middleware simply determines if the current request matches a known proxy path, and calls the proxy's `.web` and `.ws` methods with the current request and response objects. The relevant unit tests were rewritten in light of these changes. --- lib/middleware/proxy.js | 126 ++++++------ package.json | 3 +- test/unit/middleware/proxy.spec.coffee | 258 +++++++++++++++++-------- test/unit/mocha-globals.coffee | 1 + 4 files changed, 239 insertions(+), 149 deletions(-) diff --git a/lib/middleware/proxy.js b/lib/middleware/proxy.js index e439c525f..867ea41b4 100644 --- a/lib/middleware/proxy.js +++ b/lib/middleware/proxy.js @@ -2,19 +2,18 @@ var url = require('url'); var httpProxy = require('http-proxy'); var log = require('../logger').create('proxy'); +var _ = require('../helper')._; var parseProxyConfig = function(proxies, config) { - var proxyConfig = {}; var endsWithSlash = function(str) { return str.substr(-1) === '/'; }; if (!proxies) { - return proxyConfig; + return []; } - Object.keys(proxies).forEach(function(proxyPath) { - var proxyUrl = proxies[proxyPath]; + return _.sortBy(_.map(proxies, function(proxyUrl, proxyPath) { var proxyDetails = url.parse(proxyUrl); var pathname = proxyDetails.pathname; @@ -31,75 +30,74 @@ var parseProxyConfig = function(proxies, config) { proxyPath += '/'; } - if (pathname === '/' && !endsWithSlash(proxyUrl)) { + if (pathname === '/' && !endsWithSlash(proxyUrl)) { pathname = ''; } - proxyConfig[proxyPath] = { - host: proxyDetails.hostname, - port: proxyDetails.port, - baseProxyUrl: pathname, - https: proxyDetails.protocol === 'https:' - }; - - if (!proxyConfig[proxyPath].port) { - if (!proxyConfig[proxyPath].host) { - proxyConfig[proxyPath].host = config.hostname; - proxyConfig[proxyPath].port = config.port; + var hostname = proxyDetails.hostname || config.hostname; + var port = proxyDetails.port|| config.port || + (proxyDetails.protocol === 'https:' ? '443' : '80'); + var https = proxyDetails.protocol === 'https:'; + + var proxy = new httpProxy.createProxyServer({ + target: { + host: hostname, + port: port, + https: https + }, + xfwd: true, + secure: config.proxyValidateSSL + }); + + proxy.on('error', function proxyError(err, req, res) { + if (err.code === 'ECONNRESET' && req.socket.destroyed) { + log.debug('failed to proxy %s (browser hung up the socket)', req.url); } else { - proxyConfig[proxyPath].port = proxyConfig[proxyPath].https ? '443' : '80'; + log.warn('failed to proxy %s (%s)', req.url, err.message); } - } - }); - return proxyConfig; + res.destroy(); + }); + + return { + path: proxyPath, + baseUrl: pathname, + host: hostname, + port: port, + https: https, + proxy: proxy + }; + }), 'path').reverse(); }; /** * Returns a handler which understands the proxies and its redirects, along with the proxy to use - * @param proxy A http-proxy.RoutingProxy object with the proxyRequest method - * @param proxies a map of routes to proxy url + * @param proxies An array of proxy record objects + * @param urlRoot The URL root that karma is mounted on * @return {Function} handler function */ -var createProxyHandler = function(proxy, proxyConfig, proxyValidateSSL, urlRoot, config) { - var proxies = parseProxyConfig(proxyConfig, config); - var proxiesList = Object.keys(proxies).sort().reverse(); - - if (!proxiesList.length) { +var createProxyHandler = function(proxies, urlRoot) { + if (!proxies.length) { var nullProxy = function createNullProxy(request, response, next) { return next(); }; - nullProxy.upgrade = function upgradeNullProxy() { - }; + nullProxy.upgrade = function upgradeNullProxy() {}; return nullProxy; } - proxy.on('proxyError', function(err, req) { - if (err.code === 'ECONNRESET' && req.socket.destroyed) { - log.debug('failed to proxy %s (browser hung up the socket)', req.url); - } else { - log.warn('failed to proxy %s (%s)', req.url, err.message); - } - }); - var middleware = function createProxy(request, response, next) { - for (var i = 0; i < proxiesList.length; i++) { - if (request.url.indexOf(proxiesList[i]) === 0) { - var proxiedUrl = proxies[proxiesList[i]]; - - log.debug('proxying request - %s to %s:%s', request.url, proxiedUrl.host, proxiedUrl.port); - request.url = request.url.replace(proxiesList[i], proxiedUrl.baseProxyUrl); - proxy.proxyRequest(request, response, { - host: proxiedUrl.host, - port: proxiedUrl.port, - target: {https: proxiedUrl.https, rejectUnauthorized: proxyValidateSSL} - }); - return; - } + var proxyRecord = _.find(proxies, function(p) { + return request.url.indexOf(p.path) === 0; + }); + + if (!proxyRecord) { + return next(); } - return next(); + log.debug('proxying request - %s to %s:%s', request.url, proxyRecord.host, proxyRecord.port); + request.url = request.url.replace(proxyRecord.path, proxyRecord.baseUrl); + proxyRecord.proxy.web(request, response); }; middleware.upgrade = function upgradeProxy(request, socket, head) { @@ -108,22 +106,24 @@ var createProxyHandler = function(proxy, proxyConfig, proxyValidateSSL, urlRoot, log.debug('NOT upgrading proxyWebSocketRequest %s', request.url); return; } - for (var i = 0; i < proxiesList.length; i++) { - if (request.url.indexOf(proxiesList[i]) === 0) { - var proxiedUrl = proxies[proxiesList[i]]; - log.debug('upgrade proxyWebSocketRequest %s to %s:%s', - request.url, proxiedUrl.host, proxiedUrl.port); - proxy.proxyWebSocketRequest(request, socket, head, - {host: proxiedUrl.host, port: proxiedUrl.port}); - } + + var proxyRecord = _.find(proxies, function(p) { + return request.url.indexOf(p.path) === 0; + }); + + if (!proxyRecord) { + return; } + + log.debug('upgrade proxyWebSocketRequest %s to %s:%s', + request.url, proxyRecord.host, proxyRecord.port); + request.url = request.url.replace(proxyRecord.path, proxyRecord.baseUrl); + proxyRecord.proxy.ws(request, socket, head); }; return middleware; }; -exports.create = function(/* config */ config, /* config.proxies */ proxies, - /* config.proxyValidateSSL */ validateSSL) { - return createProxyHandler(new httpProxy.RoutingProxy({changeOrigin: true}), - proxies, validateSSL, config.urlRoot, config); +exports.create = function(/* config */ config, /* config.proxies */ proxies) { + return createProxyHandler(parseProxyConfig(proxies, config), config.urlRoot); }; diff --git a/package.json b/package.json index ec8835e9d..886228bf5 100644 --- a/package.json +++ b/package.json @@ -167,7 +167,7 @@ "chokidar": ">=0.8.2", "glob": "~3.2.7", "minimatch": "~0.2", - "http-proxy": "~0.10", + "http-proxy": "~1.8.1", "optimist": "~0.6.0", "rimraf": "~2.2.5", "q": "~0.9.7", @@ -184,6 +184,7 @@ "LiveScript": "~1.2.0", "chai": "~1.9.1", "chai-as-promised": "~4.1.0", + "chai-subset": "~0.3.0", "coffee-errors": "~0.8.6", "coffee-script": "~1.7.1", "cucumber": "^0.4.7", diff --git a/test/unit/middleware/proxy.spec.coffee b/test/unit/middleware/proxy.spec.coffee index 84213a085..5c9e80ea3 100644 --- a/test/unit/middleware/proxy.spec.coffee +++ b/test/unit/middleware/proxy.spec.coffee @@ -2,109 +2,127 @@ # lib/proxy.js module #============================================================================== describe 'middleware.proxy', -> - fsMock = require('mocks').fs httpMock = require('mocks').http loadFile = require('mocks').loadFile - actualOptions = requestedUrl = response = nextSpy = null - - m = loadFile __dirname + '/../../../lib/middleware/proxy.js', {'http-proxy': {}} - c = require('../../../lib/constants') - - mockProxy = - on: -> - proxyRequest: (req, res, opt) -> - actualOptions = opt - requestedUrl = req.url - res.writeHead 200 - res.end 'DONE' + actualOptions = requestedUrl = response = nextSpy = type = null + + m = loadFile __dirname + '/../../../lib/middleware/proxy.js' + + mockProxies = [{ + path: '/proxy', + baseUrl: '', + host: 'localhost', + port: '9000', + proxy: { + web: (req, res) -> + type = 'web' + requestedUrl = req.url + res.writeHead 200 + res.end 'DONE' + ws: (req, socket, head) -> + type = 'ws' + requestedUrl = req.url + } + }, { + path: '/static', + baseUrl: '', + host: 'gstatic.com', + port: '80', + proxy: { + web: (req, res) -> + type = 'web' + requestedUrl = req.url + res.writeHead 200 + res.end 'DONE' + ws: (req, socket, head) -> + type = 'ws' + requestedUrl = req.url + } + }, { + path: '/sub/some', + baseUrl: '/something', + host: 'gstatic.com', + port: '80', + proxy: { + web: (req, res) -> + type = 'web' + requestedUrl = req.url + res.writeHead 200 + res.end 'DONE' + ws: (req, socket, head) -> + type = 'ws' + requestedUrl = req.url + } + }, { + path: '/sub', + baseUrl: '', + host: 'localhost', + port: '9000', + proxy: { + web: (req, res) -> + type = 'web' + requestedUrl = req.url + res.writeHead 200 + res.end 'DONE' + ws: (req, socket, head) -> + type = 'ws' + requestedUrl = req.url + } + }] beforeEach -> actualOptions = {} requestedUrl = '' + type = '' response = new httpMock.ServerResponse nextSpy = sinon.spy() it 'should proxy requests', (done) -> - proxy = m.createProxyHandler mockProxy, {'/proxy': 'http://localhost:9000'}, true - proxy new httpMock.ServerRequest('/proxy/test.html'), response, nextSpy - - expect(nextSpy).not.to.have.been.called - expect(requestedUrl).to.equal '/test.html' - expect(actualOptions).to.deep.equal { - host: 'localhost', - port: '9000', - target:{https:false, rejectUnauthorized:true} - } - done() - - it 'should enable https', (done) -> - proxy = m.createProxyHandler mockProxy, {'/proxy': 'https://localhost:9000'}, true + proxy = m.createProxyHandler mockProxies, true, '/', {} proxy new httpMock.ServerRequest('/proxy/test.html'), response, nextSpy expect(nextSpy).not.to.have.been.called expect(requestedUrl).to.equal '/test.html' - expect(actualOptions).to.deep.equal { - host: 'localhost', - port: '9000', - target:{https:true, rejectUnauthorized:true} - } + expect(type).to.equal 'web' done() - it 'disable ssl validation', (done) -> - proxy = m.createProxyHandler mockProxy, {'/proxy': 'https://localhost:9000'}, false - proxy new httpMock.ServerRequest('/proxy/test.html'), response, nextSpy + it 'should proxy websocket requests', (done) -> + proxy = m.createProxyHandler mockProxies, true, '/', {} + proxy.upgrade new httpMock.ServerRequest('/proxy/test.html'), response, nextSpy expect(nextSpy).not.to.have.been.called expect(requestedUrl).to.equal '/test.html' - expect(actualOptions).to.deep.equal { - host: 'localhost', - port: '9000', - target:{https:true, rejectUnauthorized:false} - } + expect(type).to.equal 'ws' done() it 'should support multiple proxies', -> - proxy = m.createProxyHandler mockProxy, { - '/proxy': 'http://localhost:9000' - '/static': 'http://gstatic.com' - }, true + proxy = m.createProxyHandler mockProxies, true, '/', {} proxy new httpMock.ServerRequest('/static/test.html'), response, nextSpy expect(nextSpy).not.to.have.been.called expect(requestedUrl).to.equal '/test.html' - expect(actualOptions).to.deep.equal { - host: 'gstatic.com', - port: '80', - target:{https:false, rejectUnauthorized:true} - } + expect(type).to.equal 'web' it 'should handle nested proxies', -> - proxy = m.createProxyHandler mockProxy, { - '/sub': 'http://localhost:9000' - '/sub/some': 'http://gstatic.com/something' - }, true + proxy = m.createProxyHandler mockProxies, true, '/', {} proxy new httpMock.ServerRequest('/sub/some/Test.html'), response, nextSpy expect(nextSpy).not.to.have.been.called expect(requestedUrl).to.equal '/something/Test.html' - expect(actualOptions).to.deep.equal { - host: 'gstatic.com', - port: '80', - target:{https:false, rejectUnauthorized:true} - } + expect(type).to.equal 'web' it 'should call next handler if the path is not proxied', -> - proxy = m.createProxyHandler mockProxy, {'/proxy': 'http://localhost:9000'} + proxy = m.createProxyHandler mockProxies, true, '/', {} proxy new httpMock.ServerRequest('/non/proxy/test.html'), response, nextSpy expect(nextSpy).to.have.been.called it 'should call next handler if no proxy defined', -> - proxy = m.createProxyHandler mockProxy, {} + proxy = m.createProxyHandler {}, true, '/', {} proxy new httpMock.ServerRequest('/non/proxy/test.html'), response, nextSpy expect(nextSpy).to.have.been.called @@ -112,64 +130,134 @@ describe 'middleware.proxy', -> it 'should parse a simple proxy config', -> proxy = {'/base/': 'http://localhost:8000/'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base/': {host: 'localhost', port: '8000', baseProxyUrl: '/', https:false} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '8000', + baseUrl: '/', + path: '/base/', + https: false } + expect(parsedProxyConfig[0].proxy).to.exist it 'should set defualt http port', -> proxy = {'/base/': 'http://localhost/'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base/': {host: 'localhost', port: '80', baseProxyUrl: '/', https:false} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '80', + baseUrl: '/', + path: '/base/', + https: false } + expect(parsedProxyConfig[0].proxy).to.exist it 'should set defualt https port', -> proxy = {'/base/': 'https://localhost/'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base/': {host: 'localhost', port: '443', baseProxyUrl: '/', https:true} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '443', + baseUrl: '/', + path: '/base/', + https: true } + expect(parsedProxyConfig[0].proxy).to.exist it 'should handle proxy configs with paths', -> proxy = {'/base': 'http://localhost:8000/proxy'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base': {host: 'localhost', port: '8000', baseProxyUrl: '/proxy', https:false} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '8000', + baseUrl: '/proxy', + path: '/base', + https: false } + expect(parsedProxyConfig[0].proxy).to.exist it 'should determine protocol', -> proxy = {'/base':'https://localhost:8000'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base': {host: 'localhost', port: '8000', baseProxyUrl: '', https: true} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '8000', + baseUrl: '', + path: '/base', + https: true } + expect(parsedProxyConfig[0].proxy).to.exist it 'should handle proxy configs with only basepaths', -> proxy = {'/base': '/proxy/test'} config = {port: 9877, hostname: 'localhost'} parsedProxyConfig = m.parseProxyConfig proxy, config - expect(parsedProxyConfig).to.deep.equal { - '/base': {host: config.hostname, port: config.port, - baseProxyUrl: '/proxy/test', https:false} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: 9877, + baseUrl: '/proxy/test', + path: '/base', + https: false } + expect(parsedProxyConfig[0].proxy).to.exist it 'should normalize proxy url with only basepaths', -> proxy = {'/base/': '/proxy/test'} config = {port: 9877, hostname: 'localhost'} parsedProxyConfig = m.parseProxyConfig proxy, config - expect(parsedProxyConfig).to.deep.equal { - '/base/': {host: config.hostname, port: config.port, - baseProxyUrl: '/proxy/test/', https:false} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: 9877, + baseUrl: '/proxy/test/', + path: '/base/', + https: false } + expect(parsedProxyConfig[0].proxy).to.exist it 'should normalize proxy url', -> proxy = {'/base/': 'http://localhost:8000/proxy/test'} - parsedProxyConfig = m.parseProxyConfig proxy - expect(parsedProxyConfig).to.deep.equal { - '/base/': {host: 'localhost', port: '8000', baseProxyUrl: '/proxy/test/', https:false} + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 1 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'localhost', + port: '8000', + baseUrl: '/proxy/test/', + path: '/base/', + https: false + } + expect(parsedProxyConfig[0].proxy).to.exist + + it 'should parse nested proxy config', -> + proxy = { + '/sub': 'http://localhost:9000' + '/sub/some': 'http://gstatic.com/something' + } + parsedProxyConfig = m.parseProxyConfig proxy, {} + expect(parsedProxyConfig).to.have.length 2 + expect(parsedProxyConfig[0]).to.containSubset { + host: 'gstatic.com', + port: '80', + baseUrl: '/something', + path: '/sub/some', + https: false + } + expect(parsedProxyConfig[0].proxy).to.exist + expect(parsedProxyConfig[1]).to.containSubset { + host: 'localhost', + port: '9000', + baseUrl: '', + path: '/sub', + https: false } + expect(parsedProxyConfig[1].proxy).to.exist it 'should handle empty proxy config', -> - expect(m.parseProxyConfig {}).to.deep.equal({}) + expect(m.parseProxyConfig {}).to.deep.equal([]) diff --git a/test/unit/mocha-globals.coffee b/test/unit/mocha-globals.coffee index 672c50e28..8417e5eba 100644 --- a/test/unit/mocha-globals.coffee +++ b/test/unit/mocha-globals.coffee @@ -12,6 +12,7 @@ global.sinon = sinon # chai plugins chai.use(require 'chai-as-promised') chai.use(require 'sinon-chai') +chai.use(require 'chai-subset') beforeEach -> global.sinon = sinon.sandbox.create()