Excluded outputPath from URI escaping to fix #297. (#303)

* Excluded outputPath from URI escaping to fix #297.

* Resolved linting issues.

* Moved Windows-specific test case to the Windows-specific test case block.

* Removed `.only` from test fixture.

Author: stephenmartindale

lib/util.js

@@ -63,7 +63,6 @@ module.exports = {
     const localPrefix = parse(publicPath || '/', false, true);
     const urlObject = parse(url);
     let filename;
-    let result = '';
 
     // publicPath has the hostname that is not the same as request url's, should fail
     if (localPrefix.hostname !== null && urlObject.hostname !== null &&
@@ -89,24 +88,31 @@ module.exports = {
 
     let uri = outputPath;
 
+    /* istanbul ignore if */
+    if (process.platform === 'win32') {
+      // Path Handling for Microsoft Windows
+      if (filename) {
+        uri = urlJoin((outputPath || ''), querystring.unescape(filename));
+
+        if (!pathabs.win32(uri)) {
+          uri = `/${uri}`;
+        }
+      }
+
+      return uri;
+    }
+
+    // Path Handling for all other operating systems
     if (filename) {
       uri = urlJoin((outputPath || ''), filename);
 
-      if (!pathabs.posix(uri) && !pathabs.win32(uri)) {
+      if (!pathabs.posix(uri)) {
         uri = `/${uri}`;
       }
     }
 
     // if no matches, use outputPath as filename
-    result = querystring.unescape(uri);
-
-    // fixes #284, on windows path spaces should resolve to %20
-    /* istanbul ignore if */
-    if (process.platform === 'win32') {
-      result = result.replace(/\s/g, '%20');
-    }
-
-    return result;
+    return querystring.unescape(uri);
   },
 
   handleRangeHeaders(content, req, res) {

test/tests/util.js

@@ -121,15 +121,7 @@ describe('GetFilenameFromUrl', () => {
       url: '/pathname%20with%20spaces.js',
       outputPath: '/',
       publicPath: '/',
-      // ref #284 - windows paths should persist %20
-      expected: isWindows ? '/pathname%20with%20spaces.js' : '/pathname with spaces.js'
-    },
-    {
-      url: '/test/windows.txt',
-      outputPath: 'c:\\foo',
-      publicPath: '/test',
-      // this is weird, but it's legal-ish, and what URI parsing produces
-      expected: 'c://\\foo/windows.txt'
+      expected: '/pathname with spaces.js'
     },
     {
       url: '/js/sample.js',
@@ -279,17 +271,55 @@ describe('GetFilenameFromUrl', () => {
     });
   }
 
+  // Explicit Tests for Microsoft Windows
   if (isWindows) {
-    // explicit test for #284
-    const test = {
-      url: '/test/windows.txt',
-      outputPath: 'C:\\My%20Path\\wwwroot',
-      publicPath: '/test',
-      expected: 'C://\\My%20Path\\wwwroot/windows.txt'
-    };
+    const windowsTests = [
+      {
+        url: '/test/windows.txt',
+        outputPath: 'c:\\foo',
+        publicPath: '/test',
+        // this is weird, but it's legal-ish, and what URI parsing produces
+        expected: 'c://\\foo/windows.txt'
+      },
+      // Tests for #284
+      {
+        url: '/test/windows.txt',
+        outputPath: 'C:\\My%20Path\\wwwroot',
+        publicPath: '/test',
+        expected: 'C://\\My%20Path\\wwwroot/windows.txt'
+      },
+      {
+        url: '/test/windows%202.txt',
+        outputPath: 'C:\\My%20Path\\wwwroot',
+        publicPath: '/test',
+        expected: 'C://\\My%20Path\\wwwroot/windows 2.txt'
+      },
+      // Tests for #297
+      {
+        url: '/test/windows.txt',
+        outputPath: 'C:\\My Path\\wwwroot',
+        publicPath: '/test',
+        expected: 'C://\\My Path\\wwwroot/windows.txt'
+      },
+      {
+        url: '/test/windows%202.txt',
+        outputPath: 'C:\\My Path\\wwwroot',
+        publicPath: '/test',
+        expected: 'C://\\My Path\\wwwroot/windows 2.txt'
+      },
+      // Tests for #284 & #297
+      {
+        url: '/windows%20test/test%20%26%20test%20%26%20%2520.txt',
+        outputPath: 'C:\\My %20 Path\\wwwroot',
+        publicPath: '/windows%20test',
+        expected: 'C://\\My %20 Path\\wwwroot/test & test & %20.txt'
+      }
+    ];
 
-    it(`windows: should process ${test.url} -> ${test.expected}`, () => {
-      testUrl(test);
-    });
+    for (const test of windowsTests) {
+      it(`windows: should process ${test.url} -> ${test.expected}`, () => {
+        testUrl(test);
+      });
+    }
   }
 });