Fix a url regression

The change for nodejs#954 introduced a regression that would cause the url parser to fail on special chars found in the auth segment. Fix that, and also don't create invalid urls when format() is called on an object containing an auth member containing '@' characters or delimiters.
jrobertson · May 10, 2011 · 307f39c · 307f39c
1 parent 11beac7
commit 307f39c
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 7 deletions.
diff --git a/lib/url.js b/lib/url.js
@@ -42,6 +42,7 @@ var protocolPattern = /^([a-z0-9]+:)/i,
     // them.
     nonHostChars = ['%', '/', '?', ';', '#']
       .concat(unwise).concat(autoEscape),
+    nonAuthChars = ['/', '@', '?', '#'].concat(delims),
     hostnameMaxLen = 255,
     hostnamePartPattern = /^[a-zA-Z0-9][a-z0-9A-Z-]{0,62}$/,
     hostnamePartStart = /^([a-zA-Z0-9][a-z0-9A-Z-]{0,62})(.*)$/,
@@ -123,12 +124,37 @@ function urlParse(url, parseQueryString, slashesDenoteHost) {
     // there's a hostname.
     // the first instance of /, ?, ;, or # ends the host.
     // don't enforce full RFC correctness, just be unstupid about it.
+
+    // If there is an @ in the hostname, then non-host chars *are* allowed
+    // to the left of the first @ sign, unless some non-auth character
+    // comes *before* the @-sign.
+    // URLs are obnoxious.
+    var atSign = rest.indexOf('@');
+    if (atSign !== -1) {
+      // there *may be* an auth
+      var hasAuth = true;
+      for (var i = 0, l = nonAuthChars.length; i < l; i++) {
+        var index = rest.indexOf(nonAuthChars[i]);
+        if (index !== -1 && index < atSign) {
+          // not a valid auth.  Something like http://foo.com/bar@baz/
+          hasAuth = false;
+          break;
+        }
+      }
+      if (hasAuth) {
+        // pluck off the auth portion.
+        out.auth = rest.substr(0, atSign);
+        rest = rest.substr(atSign + 1);
+      }
+    }
+
     var firstNonHost = -1;
     for (var i = 0, l = nonHostChars.length; i < l; i++) {
       var index = rest.indexOf(nonHostChars[i]);
       if (index !== -1 &&
           (firstNonHost < 0 || index < firstNonHost)) firstNonHost = index;
     }
+
     if (firstNonHost !== -1) {
       out.host = rest.substr(0, firstNonHost);
       rest = rest.substr(firstNonHost);
@@ -137,8 +163,9 @@ function urlParse(url, parseQueryString, slashesDenoteHost) {
       rest = '';
     }
 
-    // pull out the auth and port.
+    // pull out port.
     var p = parseHost(out.host);
+    if (out.auth) out.host = out.auth + '@' + out.host;
     var keys = Object.keys(p);
     for (var i = 0, l = keys.length; i < l; i++) {
       var key = keys[i];
@@ -250,10 +277,19 @@ function urlFormat(obj) {
   // to clean up potentially wonky urls.
   if (typeof(obj) === 'string') obj = urlParse(obj);
 
+  var auth = obj.auth;
+  if (auth) {
+    auth = auth.split('@').join('%40');
+    for (var i = 0, l = nonAuthChars.length; i < l; i++) {
+      var nAC = nonAuthChars[i];
+      auth = auth.split(nAC).join(encodeURIComponent(nAC));
+    }
+  }
+
   var protocol = obj.protocol || '',
       host = (obj.host !== undefined) ? obj.host :
           obj.hostname !== undefined ? (
-              (obj.auth ? obj.auth + '@' : '') +
+              (auth ? auth + '@' : '') +
               obj.hostname +
               (obj.port ? ':' + obj.port : '')
           ) :
@@ -476,11 +512,6 @@ function urlResolveObject(source, relative) {
 
 function parseHost(host) {
   var out = {};
-  var at = host.indexOf('@');
-  if (at !== -1) {
-    out.auth = host.substr(0, at);
-    host = host.substr(at + 1); // drop the @
-  }
   var port = portPattern.exec(host);
   if (port) {
     port = port[0];

diff --git a/test/simple/test-url.js b/test/simple/test-url.js
@@ -236,6 +236,18 @@ var parseTests = {
     'host': 'isaacschlueter@jabber.org',
     'auth': 'isaacschlueter',
     'hostname': 'jabber.org'
+  },
+  'http://atpass:foo%40bar@127.0.0.1:8080/path?search=foo#bar' : {
+    'href' : 'http://atpass:foo%40bar@127.0.0.1:8080/path?search=foo#bar',
+    'protocol' : 'http:',
+    'host' : 'atpass:foo%40bar@127.0.0.1:8080',
+    'auth' : 'atpass:foo%40bar',
+    'hostname' : '127.0.0.1',
+    'port' : '8080',
+    'pathname': '/path',
+    'search' : '?search=foo',
+    'query' : 'search=foo',
+    'hash' : '#bar'
   }
 };
 for (var u in parseTests) {
@@ -367,6 +379,20 @@ var formatTests = {
     'host': 'isaacschlueter@jabber.org',
     'auth': 'isaacschlueter',
     'hostname': 'jabber.org'
+  },
+  'http://atpass:foo%40bar@127.0.0.1/' : {
+    'href': 'http://atpass:foo%40bar@127.0.0.1/',
+    'auth': 'atpass:foo@bar',
+    'hostname': '127.0.0.1',
+    'protocol': 'http:',
+    'pathname': '/'
+  },
+  'http://atslash%2F%40:%2F%40@foo/' : {
+    'href': 'http://atslash%2F%40:%2F%40@foo/',
+    'auth': 'atslash/@:/@',
+    'hostname': 'foo',
+    'protocol': 'http:',
+    'pathname': '/'
   }
 };
 for (var u in formatTests) {