medialize / URI.js

Javascript URL mutation library
http://medialize.github.io/URI.js/
MIT License
6.26k stars 476 forks source link

Simplify host parsing fix #403

Closed alesandroortiz closed 3 years ago

alesandroortiz commented 3 years ago

Two test cases currently fail because they expect host/hostname mutators to throw exceptions when they contain backslashes. Will address after discussion in PR.

alesandroortiz commented 3 years ago

@rodneyrehm: 👋🏻 Proposed fix per email.

rodneyrehm commented 3 years ago

If we change the tests from expecting an error to verifying the result

diff --git a/test/test.js b/test/test.js
index 672b3f9..6ac368f 100644
--- a/test/test.js
+++ b/test/test.js
@@ -306,9 +306,9 @@
     equal(u.hostname(), 'some_where.exa_mple.org', 'hostname changed');
     equal(u+'', 'http://some_where.exa_mple.org/foo.html', 'hostname changed url');

-    raises(function() {
-      u.hostname('foo\\bar.com');
-    }, TypeError, 'Failing backslash detection in hostname');
+    u.hostname('foo\\bar.com');
+    equal(u.hostname(), 'foo', 'hostname changed');
+    equal(u+'', 'http://foo/foo.html', 'hostname changed url');

     // instance does not fall back to global setting
     URI.preventInvalidHostname = true;
@@ -471,9 +471,10 @@
     equal(u.port(), '44', 'port restored');
     equal(u+'', 'http://some_where.exa_mple.org:44/foo.html', 'host modified url #2');

-    raises(function() {
-      u.host('foo\\bar.com');
-    }, TypeError, 'Failing backslash detection in host');
+    u.host('foo\\bar.com');
+    equal(u.hostname(), 'foo', 'host modified hostname');
+    equal(u.port(), '', 'host removed port');
+    equal(u+'', 'http://foo/foo.html', 'host modified url');
   });
   test('origin', function () {
     var u = new URI('http://foo.bar/foo.html');

we'll see both tests fail with

mutating basics: hostname
  hostname changed
    Expected: "foo"
      Result: "foo\bar.com"
  hostname changed url
    Expected: "http://foo/foo.html"
      Result: "http://foo\bar.com/foo.html"
rodneyrehm commented 3 years ago

preserving the existing tests and adding your new case:

diff --git a/src/URI.js b/src/URI.js
index 715c097..92c0481 100644
--- a/src/URI.js
+++ b/src/URI.js
@@ -612,19 +612,22 @@
   };
   URI.parseUserinfo = function(string, parts) {
     // extract username:password
+    var _string = string
     var firstBackSlash = string.indexOf('\\');
+    if (firstBackSlash !== -1) {
+      string = string.replace(/\\/g, '/')
+    }
     var firstSlash = string.indexOf('/');
-    var slash = firstBackSlash === -1 ? firstSlash : (firstSlash !== -1 ? Math.min(firstBackSlash, firstSlash): firstSlash)
     var pos = string.lastIndexOf('@', firstSlash > -1 ? firstSlash : string.length - 1);
     var t;

     // authority@ must come before /path or \path
-    if (pos > -1 && (slash === -1 || pos < slash)) {
+    if (pos > -1 && (firstSlash === -1 || pos < firstSlash)) {
       t = string.substring(0, pos).split(':');
       parts.username = t[0] ? URI.decode(t[0]) : null;
       t.shift();
       parts.password = t[0] ? URI.decode(t.join(':')) : null;
-      string = string.substring(pos + 1);
+      string = _string.substring(pos + 1);
     } else {
       parts.username = null;
       parts.password = null;
diff --git a/test/urls.js b/test/urls.js
index 5e0c06e..14255c1 100644
--- a/test/urls.js
+++ b/test/urls.js
@@ -2033,6 +2033,55 @@ var urls = [{
         idn: false,
         punycode: false
       }
+    }, {
+      name: 'backslashes authority, no ending slash',
+      url: 'https://attacker.com\\@example.com',
+      _url: 'https://attacker.com/@example.com',
+      parts: {
+        protocol: 'https',
+        username: null,
+        password: null,
+        hostname: 'attacker.com',
+        port: null,
+        path: '/@example.com',
+        query: null,
+        fragment: null
+      },
+      accessors: {
+        protocol: 'https',
+        username: '',
+        password: '',
+        port: '',
+        path: '/@example.com',
+        query: '',
+        fragment: '',
+        resource: '/@example.com',
+        authority: 'attacker.com',
+        origin: 'https://attacker.com',
+        userinfo: '',
+        subdomain: '',
+        domain: 'attacker.com',
+        tld: 'com',
+        directory: '/',
+        filename: '@example.com',
+        suffix: 'com',
+        hash: '',
+        search: '',
+        host: 'attacker.com',
+        hostname: 'attacker.com'
+      },
+      is: {
+        urn: false,
+        url: true,
+        relative: false,
+        name: true,
+        sld: false,
+        ip: false,
+        ip4: false,
+        ip6: false,
+        idn: false,
+        punycode: false
+      }
     }
 ];
alesandroortiz commented 3 years ago

Your patch is great, thanks for iterating on this. I've reverted my patch to src/URI.js and applied your patch to this branch (will push in a minute).

Feel free to either merge this PR or merge the changes from your branch.

rodneyrehm commented 3 years ago

released as v1.19.4, thanks for your help :)