fix(middleware): simplify stripHost. (#3115)

johnjbarton · web-flow · commit d65e911c80f0 · 2018-09-06T12:12:18.000-07:00
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@
diff --git 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
@@ -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
@@ -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
@@ -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('https://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('https://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('https://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('https://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('https://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('https://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')
   })
 })
