Merge pull request #44 from fastify/fixing-sec-issue-buildurl

Sanitising URL instance parameters

Author: jkyberneees

index.js

@@ -1,6 +1,5 @@
 'use strict'
 
-const URL = require('url').URL
 const pump = require('pump')
 const lru = require('tiny-lru')
 const querystring = require('querystring')
@@ -10,7 +9,8 @@ const {
   filterPseudoHeaders,
   copyHeaders,
   stripHttp1ConnectionHeaders,
-  filterHeaders
+  filterHeaders,
+  buildURL
 } = require('./lib/utils')
 
 function populateHeaders (headers, body, contentType) {
@@ -170,11 +170,11 @@ function getReqUrl (source, cache, base, opts) {
     const cacheKey = reqBase + source
     url = cache.get(cacheKey)
     if (!url) {
-      url = new URL(source, reqBase)
+      url = buildURL(source, reqBase)
       cache.set(cacheKey, url)
     }
   } else {
-    url = new URL(source, reqBase)
+    url = buildURL(source, reqBase)
   }
 
   return url

lib/utils.js

@@ -70,9 +70,17 @@ function filterHeaders (headers, filter) {
   return dest
 }
 
+function buildURL (source = '', reqBase) {
+  // issue ref: https://github.com/fastify/fast-proxy/issues/42
+  const cleanSource = source.replace(/\/+/g, '/')
+
+  return new URL(cleanSource, reqBase)
+}
+
 module.exports = {
   copyHeaders,
   stripHttp1ConnectionHeaders,
   filterPseudoHeaders,
-  filterHeaders
+  filterHeaders,
+  buildURL
 }

test/10.build-url.test.js

@@ -0,0 +1,53 @@
+/* global describe, it */
+'use strict'
+
+const expect = require('chai').expect
+const { buildURL } = require('./../lib/utils')
+
+describe('buildURL', () => {
+  it('should produce invalid URL - //10.0.0.10/', function () {
+    const url = new URL('//10.0.0.10/', 'http://localhost')
+    expect(url.origin).to.equal('http://10.0.0.10')
+    expect(url.pathname).to.equal('/')
+    expect(url.href).to.equal('http://10.0.0.10/')
+  })
+
+  it('should produce invalid URL - //httpbin.org/hi', function () {
+    const url = new URL('//httpbin.org/hi', 'http://localhost')
+    expect(url.origin).to.equal('http://httpbin.org')
+    expect(url.pathname).to.equal('/hi')
+    expect(url.href).to.equal('http://httpbin.org/hi')
+  })
+
+  it('should produce valid URL (2 params)', function () {
+    const url = buildURL('/hi', 'http://localhost')
+
+    expect(url.origin).to.equal('http://localhost')
+    expect(url.pathname).to.equal('/hi')
+    expect(url.href).to.equal('http://localhost/hi')
+  })
+
+  it('should produce valid URL (1 param)', function () {
+    const url = buildURL('http://localhost/hi')
+
+    expect(url.origin).to.equal('http://localhost')
+    expect(url.pathname).to.equal('/hi')
+    expect(url.href).to.equal('http://localhost/hi')
+  })
+
+  it('should sanitize invalid source (2 params) - //10.0.0.10/hi', function () {
+    const url = buildURL('//10.0.0.10/hi', 'http://localhost')
+
+    expect(url.origin).to.equal('http://localhost')
+    expect(url.pathname).to.equal('/10.0.0.10/hi')
+    expect(url.href).to.equal('http://localhost/10.0.0.10/hi')
+  })
+
+  it('should sanitize invalid source (2 params) - //httpbin.org/hi', function () {
+    const url = buildURL('//httpbin.org/hi', 'http://localhost')
+
+    expect(url.origin).to.equal('http://localhost')
+    expect(url.pathname).to.equal('/httpbin.org/hi')
+    expect(url.href).to.equal('http://localhost/httpbin.org/hi')
+  })
+})