feat(dispatcher): url normalization (#90)

ArnaudBuchholz · Feb 4, 2024 · 8e97582 · 8e97582
1 parent 4f61a0a
commit 8e97582
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 98 deletions.
diff --git a/reserve/src/dispatcher.js b/reserve/src/dispatcher.js
@@ -3,6 +3,7 @@
 const { performance } = require('./node-api')
 const logError = require('./logError')
 const interpolate = require('./interpolate')
+const normalize = require('./normalize')
 const {
   EVENT_INCOMING,
   EVENT_ERROR,
@@ -135,16 +136,7 @@ function dispatch (context, url, index = 0) {
 }
 
 module.exports = function (configuration, request, response) {
-  let { url } = request
-  if (url.indexOf('.') !== -1 || url.indexOf('%') !== -1) {
-    try {
-      const { pathname, search, hash } = new URL(url, 'p:/')
-      url = decodeURIComponent(pathname.replace(/%0\d|%1\d/g, '')) + search + hash
-    } catch (e) {
-      url = 400
-    }
-  }
-
+  const url = normalize(request.url)
   const {
     [$configurationRequests]: configurationRequests,
     [$configurationEventEmitter]: emit

diff --git a/reserve/src/dispatcher.spec.js b/reserve/src/dispatcher.spec.js
@@ -492,90 +492,4 @@ describe('dispatcher', () => {
       )
     })
   })
-
-  describe('URL normalization for security', () => {
-    it('avoids path traversal by normalizing the URL if needed (using root ../)', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/../file.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 200)
-          assert.strictEqual(response.headers['Content-Type'], textMimeType)
-          assert.strictEqual(response.toString(), 'Hello World!')
-        })
-    })
-
-    it('avoids path traversal by normalizing the URL if needed (using intermediate ../)', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/test/../file.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 200)
-          assert.strictEqual(response.headers['Content-Type'], textMimeType)
-          assert.strictEqual(response.toString(), 'Hello World!')
-        })
-    })
-
-    it('avoids path traversal by normalizing the URL if needed (using root %2E%2E/)', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/%2E%2E/file.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 200)
-          assert.strictEqual(response.headers['Content-Type'], textMimeType)
-          assert.strictEqual(response.toString(), 'Hello World!')
-        })
-    })
-
-    it('avoids path traversal by normalizing the URL if needed (using intermediate %2E%2E/)', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/test/%2E%2E/file.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 200)
-          assert.strictEqual(response.headers['Content-Type'], textMimeType)
-          assert.strictEqual(response.toString(), 'Hello World!')
-        })
-    })
-
-    it('works around poison null attack', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/test/%2E%2E/file%00.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 200)
-          assert.strictEqual(response.headers['Content-Type'], textMimeType)
-          assert.strictEqual(response.toString(), 'Hello World!')
-        })
-    })
-
-    for (let i = 1; i < 32; ++i) {
-      it('works around forbidden chars', () => {
-        const request = new Request('GET')
-        request.setForgedUrl(`/test/%2E%2E/file%${Number(i).toString(8).padStart(2, '0')}.txt`)
-        return dispatch({ request })
-          .then(({ emitted, response }) => {
-            assert.ok(!hasError(emitted))
-            assert.strictEqual(response.statusCode, 200)
-            assert.strictEqual(response.headers['Content-Type'], textMimeType)
-            assert.strictEqual(response.toString(), 'Hello World!')
-          })
-      })
-    }
-
-    it('rejects malformed URLs', () => {
-      const request = new Request('GET')
-      request.setForgedUrl('/test/%2E%2E/file%0.txt')
-      return dispatch({ request })
-        .then(({ emitted, response }) => {
-          assert.ok(!hasError(emitted))
-          assert.strictEqual(response.statusCode, 400)
-        })
-    })
-  })
 })
diff --git a/reserve/src/normalize.js b/reserve/src/normalize.js
@@ -0,0 +1,13 @@
+'use strict'
+
+module.exports = url => {
+  if (url.indexOf('./') !== -1 || url.indexOf('%') !== -1 || url.indexOf('/') === -1) {
+    try {
+      const { pathname, search, hash } = new URL(url, 'p:/')
+      url = decodeURIComponent(pathname.replace(/%(0|1)(\d|[a-f])/ig, '')) + search + hash
+    } catch (e) {
+      url = 400
+    }
+  }
+  return url
+}
diff --git a/reserve/src/normalize.spec.js b/reserve/src/normalize.spec.js
@@ -0,0 +1,30 @@
+'use strict'
+
+const { describe, it } = require('mocha')
+const assert = require('assert')
+const normalize = require('./normalize')
+
+describe('normalize', () => {
+  const tests = {
+    'file.txt': '/file.txt',
+    '/../file.txt': '/file.txt',
+    '/test/../file.txt': '/file.txt',
+    '/%2E%2E/file.txt': '/file.txt',
+    '/test/%2E%2E/file.txt': '/file.txt',
+    '/file%00.txt': '/file.txt',
+    '/file%0.txt': 400,
+    '/file%-.txt': 400,
+    '/file%%.txt': 400
+  }
+
+  for (let i = 1; i < 32; ++i) {
+    tests[`/file${i}%${Number(i).toString(16).padStart(2, '0')}.txt`] = `/file${i}.txt`
+  }
+
+  Object.keys(tests).forEach(url => {
+    const expected = tests[url]
+    it(`${url} ⇒ ${expected}`, () => {
+      assert.strictEqual(normalize(url), expected)
+    })
+  })
+})
diff --git a/reserve/tests/integration.mjs b/reserve/tests/integration.mjs
@@ -249,9 +249,9 @@ async function test (config, base) {
     statusCode: 200,
     headers: {
       'content-type': 'text/plain',
-      'content-length': '17'
+      'content-length': '13'
     },
-    body: 'Hello%20World%20!'
+    body: 'Hello World !'
   })
   await match({
     method: 'POST',