From 32ae9b50357bf1e70933537cfcd53449d561dad7 Mon Sep 17 00:00:00 2001 From: johnjbarton Date: Thu, 16 Aug 2018 14:41:29 -0700 Subject: [PATCH] fix(middleware): simplify stripHost. The stripHost middleware only adds a modified url to the request. That modified url is only used one place. By converting the middleware to a module, the code is simpler and beforeMiddleware modules can reuse karma middleware. (One alternative considered was to move the stripHost in the chain before the beforeMiddleware, but this form seems better). Clean up regex per offline suggestion from zzo@ --- lib/middleware/karma.js | 7 ++- lib/middleware/strip_host.js | 15 ++---- lib/web-server.js | 2 - test/unit/middleware/strip_host.spec.js | 65 ++++++------------------- 4 files changed, 26 insertions(+), 63 deletions(-) diff --git a/lib/middleware/karma.js b/lib/middleware/karma.js index 6ef06ef63..d47e898a0 100644 --- a/lib/middleware/karma.js +++ b/lib/middleware/karma.js @@ -17,6 +17,7 @@ var url = require('url') var _ = require('lodash') var log = require('../logger').create('middleware:karma') +var stripHost = require('./strip_host').stripHost function urlparse (urlStr) { var urlObj = url.parse(urlStr, true) @@ -90,7 +91,11 @@ function createKarmaMiddleware ( var customClientContextFile = injector.get('config.customClientContextFile') var includeCrossOriginAttribute = injector.get('config.crossOriginAttribute') - var requestUrl = request.normalizedUrl.replace(/\?.*/, '') + var normalizedUrl = stripHost(request.url) || request.url + // For backwards compatibility in middleware plugins, remove in v4. + request.normalizedUrl = normalizedUrl + + var requestUrl = normalizedUrl.replace(/\?.*/, '') var requestedRangeHeader = request.headers['range'] // redirect /__karma__ to /__karma__ (trailing slash) diff --git a/lib/middleware/strip_host.js b/lib/middleware/strip_host.js index 984f89f2d..539be0aad 100644 --- a/lib/middleware/strip_host.js +++ b/lib/middleware/strip_host.js @@ -1,18 +1,11 @@ /** - * Strip host middleware is responsible for stripping hostname from request path + * Strip hostname from request path * This to handle requests that uses (normally over proxies) an absoluteURI as request path */ -function createStripHostMiddleware () { - return function (request, response, next) { - function stripHostFromUrl (url) { - return url.replace(/^http[s]?:\/\/([a-z\-.:\d]+)\//, '/') - } - - request.normalizedUrl = stripHostFromUrl(request.url) || request.url - next() - } +function stripHostFromUrl (url) { + return url.replace(/^https?:\/\/[a-z.:\d-]+\//, '/') } // PUBLIC API -exports.create = createStripHostMiddleware +exports.stripHost = stripHostFromUrl diff --git a/lib/web-server.js b/lib/web-server.js index bd63481d7..3bda89827 100644 --- a/lib/web-server.js +++ b/lib/web-server.js @@ -10,7 +10,6 @@ const Promise = require('bluebird') const common = require('./middleware/common') const runnerMiddleware = require('./middleware/runner') const stopperMiddleware = require('./middleware/stopper') -const stripHostMiddleware = require('./middleware/strip_host') const karmaMiddleware = require('./middleware/karma') const sourceFilesMiddleware = require('./middleware/source_files') const proxyMiddleware = require('./middleware/proxy') @@ -65,7 +64,6 @@ function createWebServer (injector, config) { handler.use(injector.invoke(runnerMiddleware.create)) handler.use(injector.invoke(stopperMiddleware.create)) - handler.use(injector.invoke(stripHostMiddleware.create)) handler.use(injector.invoke(karmaMiddleware.create)) handler.use(injector.invoke(sourceFilesMiddleware.create)) // TODO(vojta): extract the proxy into a plugin diff --git a/test/unit/middleware/strip_host.spec.js b/test/unit/middleware/strip_host.spec.js index 5983e61c8..5315f08f1 100644 --- a/test/unit/middleware/strip_host.spec.js +++ b/test/unit/middleware/strip_host.spec.js @@ -1,61 +1,28 @@ -var mocks = require('mocks') - describe('middleware.strip_host', function () { - var nextSpy - var HttpRequestMock = mocks.http.ServerRequest - - var createStripHostMiddleware = require('../../../lib/middleware/strip_host').create - - var handler = nextSpy = null - - beforeEach(function () { - nextSpy = sinon.spy() - handler = createStripHostMiddleware(null, null, '/base/path') - return handler - }) + const stripHost = require('../../../lib/middleware/strip_host').stripHost - it('should strip request with IP number', function (done) { - var request = new HttpRequestMock('http://192.12.31.100/base/a.js?123345') - handler(request, null, nextSpy) - - expect(request.normalizedUrl).to.equal('/base/a.js?123345') - expect(nextSpy).to.have.been.called - return done() + it('should strip request with IP number', function () { + const normalizedUrl = stripHost('http://192.12.31.100/base/a.js?123345') + expect(normalizedUrl).to.equal('/base/a.js?123345') }) - it('should strip request with absoluteURI', function (done) { - var request = new HttpRequestMock('http://localhost/base/a.js?123345') - handler(request, null, nextSpy) - - expect(request.normalizedUrl).to.equal('/base/a.js?123345') - expect(nextSpy).to.have.been.called - return done() + it('should strip request with absoluteURI', function () { + const normalizedUrl = stripHost('http://localhost/base/a.js?123345') + expect(normalizedUrl).to.equal('/base/a.js?123345') }) - it('should strip request with absoluteURI and port', function (done) { - var request = new HttpRequestMock('http://localhost:9876/base/a.js?123345') - handler(request, null, nextSpy) - - expect(request.normalizedUrl).to.equal('/base/a.js?123345') - expect(nextSpy).to.have.been.called - return done() + it('should strip request with absoluteURI and port', function () { + const normalizedUrl = stripHost('http://localhost:9876/base/a.js?123345') + expect(normalizedUrl).to.equal('/base/a.js?123345') }) - it('should strip request with absoluteURI over HTTPS', function (done) { - var request = new HttpRequestMock('https://karma-runner.github.io/base/a.js?123345') - handler(request, null, nextSpy) - - expect(request.normalizedUrl).to.equal('/base/a.js?123345') - expect(nextSpy).to.have.been.called - return done() + it('should strip request with absoluteURI over HTTPS', function () { + const normalizedUrl = stripHost('https://karma-runner.github.io/base/a.js?123345') + expect(normalizedUrl).to.equal('/base/a.js?123345') }) - return it('should return same url as passed one', function (done) { - var request = new HttpRequestMock('/base/b.js?123345') - handler(request, null, nextSpy) - - expect(request.normalizedUrl).to.equal('/base/b.js?123345') - expect(nextSpy).to.have.been.called - return done() + it('should return same url as passed one', function () { + const normalizedUrl = stripHost('/base/b.js?123345') + expect(normalizedUrl).to.equal('/base/b.js?123345') }) })