feat: replace deprecated url.resolve() and url.parse() with WHATWG URL

lib/request.js

@@ -2,7 +2,6 @@
 
 'use strict';
 
-var url = require('url');
 var sprintf = require('util').format;
 var crypto = require('crypto');
 
@@ -74,7 +73,7 @@ function patch(Request) {
 
         var protocol = this.isSecure() ? 'https://' : 'http://';
         var hostname = this.headers.host;
-        return url.resolve(protocol + hostname + this.path() + '/', path);
+        return new URL(path, protocol + hostname + this.path() + '/').href;
     };
 
     /**
@@ -271,7 +270,7 @@ function patch(Request) {
      * // incoming request is http://localhost:3000/foo/bar?a=1
      * server.get('/:x/bar', function(req, res, next) {
      *     console.warn(req.href());
-     *     // => /foo/bar/?a=1
+     *     // => http://localhost:3000/foo/bar?a=1
      * });
      */
     Request.prototype.href = Request.prototype.getHref;
@@ -382,8 +381,10 @@ function patch(Request) {
     Request.prototype.getQuery = function getQuery() {
         // always return a string, because this is the raw query string.
         // if the queryParser plugin is used, req.query will provide an empty
-        // object fallback.
-        return this.getUrl().query || '';
+        // object fallback. a URL instance's `.search` includes the leading
+        // '?', which the raw query string form does not.
+        var search = this.getUrl().search;
+        return search ? search.slice(1) : '';
     };
 
     /**
@@ -420,11 +421,25 @@ function patch(Request) {
      * @memberof Request
      * @instance
      * @function getUrl
-     * @returns  {Object} url
+     * @returns  {URL} url
      */
     Request.prototype.getUrl = function getUrl() {
         if (this._cacheURL !== this.url) {
-            this._url = url.parse(this.url);
+            var protocol = this.isSecure() ? 'https://' : 'http://';
+            var base = protocol + (this.headers.host || 'localhost');
+            // For origin-form targets (starting with "/"), concatenate into
+            // one URL string instead of new URL(this.url, base), which would
+            // treat a leading "//" as a network-path reference and drop part
+            // of the path into the hostname. E.g. with this.url ===
+            // "//foo/bar", new URL(this.url, base) resolves the hostname
+            // to "foom" and the path to "/bar", instead of keeping the
+            // path as "//evil.com/x" on the original host. Other forms
+            // (e.g. "*" for "OPTIONS *") don't start with "/" and keep the
+            // two-arg form.
+            this._url =
+                this.url.charAt(0) === '/'
+                    ? new URL(base + this.url)
+                    : new URL(this.url, base);
             this._cacheURL = this.url;
         }
         return this._url;

lib/response.js

@@ -4,14 +4,12 @@
 
 var http = require('http');
 var sprintf = require('util').format;
-var url = require('url');
 
 var assert = require('assert-plus');
 var mime = require('mime');
 var errors = require('restify-errors');
 
 var httpDate = require('./http_date');
-var utils = require('./utils');
 
 ///--- Globals
 
@@ -740,6 +738,7 @@ function patch(Response) {
         var self = this;
         var statusCode = 302;
         var finalUri;
+        var finalHostname;
         var redirectLocation;
         var next;
 
@@ -768,32 +767,34 @@ function patch(Response) {
                 ? opt.secure
                 : req.isSecure();
 
-            // if hostname is passed in, use that as the base,
-            // otherwise fall back on current url.
-            var parsedUri = url.parse(opt.hostname || currentFullPath, true);
-
-            // create the object we'll use to format for the final uri.
-            // this object will eventually get passed to url.format().
-            // can't use parsedUri to seed it, as it confuses the url module
-            // with some existing parsed state. instead, we'll pick the things
-            // we want and use that as a starting point.
-            finalUri = {
-                port: parsedUri.port,
-                hostname: parsedUri.hostname,
-                query: parsedUri.query,
-                pathname: parsedUri.pathname
-            };
-
-            // start building url based on options.
-            // start with the host
-            if (opt.hostname) {
-                finalUri.hostname = opt.hostname;
-            }
+            // opt.hostname is documented/used as a bare host (e.g.
+            // 'www.foo.com'), never a full URL, so it can't be fed into
+            // `new URL()` directly - that throws, since a bare host has no
+            // scheme for the URL parser to key off. so when it's set, start
+            // from an empty URL and let it be applied as a plain `.hostname`
+            // assignment below. otherwise, seed finalUri from the current
+            // request (req.href() is always absolute) so we're redirecting
+            // within the same host by default.
+            finalUri = opt.hostname
+                ? new URL('http://localhost/')
+                : new URL(currentFullPath);
+
+            // finalHostname decides absolute vs relative Location (below):
+            // use opt.hostname if given; otherwise, only if the original
+            // req.url was itself absolute (e.g. 'http://foo.com/bar'),
+            // redirect to that same host - if req.url was relative, stay
+            // null so the redirect stays relative too. can't fall back to
+            // finalUri.hostname instead: it's always populated, which would
+            // make every redirect absolute.
+            finalHostname =
+                opt.hostname ||
+                (req.url.indexOf('://') !== -1 ? finalUri.hostname : null);
 
             // then set protocol IFF hostname is set - otherwise we end up with
             // malformed URL.
-            if (finalUri.hostname) {
-                finalUri.protocol = secure === true ? 'https' : 'http';
+            if (finalHostname) {
+                finalUri.protocol = secure === true ? 'https:' : 'http:';
+                finalUri.hostname = finalHostname;
             }
 
             // then set current path after the host
@@ -806,13 +807,33 @@ function patch(Response) {
                 finalUri.port = opt.port;
             }
 
-            // then add query params
+            // then add query params. a URL instance has no `.query` setter -
+            // querystring changes must go through `.search`. build the new
+            // search string from opt.query first, then append the existing
+            // params verbatim (unless overriding) - this preserves repeated
+            // keys on both sides instead of collapsing them, which is what
+            // `Object.fromEntries(searchParams)` would do for duplicate keys.
             if (opt.query) {
-                if (opt.overrideQuery === true) {
-                    finalUri.query = opt.query;
-                } else {
-                    finalUri.query = utils.mergeQs(opt.query, finalUri.query);
+                var existingParams = Array.from(
+                    finalUri.searchParams.entries()
+                );
+                var newSearchParams = new URLSearchParams();
+                Object.keys(opt.query).forEach(function forEach(key) {
+                    var value = opt.query[key];
+                    if (Array.isArray(value)) {
+                        value.forEach(function forEach2(v) {
+                            newSearchParams.append(key, v);
+                        });
+                    } else {
+                        newSearchParams.append(key, value);
+                    }
+                });
+                if (opt.overrideQuery !== true) {
+                    existingParams.forEach(function forEach(pair) {
+                        newSearchParams.append(pair[0], pair[1]);
+                    });
                 }
+                finalUri.search = newSearchParams.toString();
             }
 
             // change status code to 301 permanent if specified
@@ -832,7 +853,16 @@ function patch(Response) {
             return next(new InternalServerError('could not construct url'));
         }
 
-        redirectLocation = url.format(finalUri);
+        if (typeof finalUri === 'string') {
+            redirectLocation = finalUri;
+        } else {
+            // a URL instance can't represent a host-less location, so build
+            // that case by hand; otherwise the instance already has
+            // everything it needs to serialize itself.
+            redirectLocation = finalHostname
+                ? finalUri.toString()
+                : finalUri.pathname + (finalUri.search || '');
+        }
 
         self.emit('redirect', redirectLocation);
 

lib/router.js

@@ -294,8 +294,9 @@ Router.prototype.getRoutes = function getRoutes() {
  * @returns  {Boolean} is options error
  */
 Router._optionsError = function _optionsError(req, res) {
-    var pathname = req.getUrl().pathname;
-    return req.method === 'OPTIONS' && pathname === '*';
+    // OPTIONS * is a literal request-target, not a path - a URL instance
+    // can't represent it, so check the raw request url instead.
+    return req.method === 'OPTIONS' && req.url === '*';
 };
 
 /**

test/plugins/bodyReader.test.js

@@ -157,7 +157,10 @@ describe('body reader', function() {
             });
 
             SERVER.on('after', function(req2) {
-                if (req2.href() === '/compressed?v=2') {
+                if (
+                    req2.href() ===
+                    'http://127.0.0.1:' + PORT + '/compressed?v=2'
+                ) {
                     assert.equal(SERVER.inflightRequests(), 0);
                     done();
                 }

test/plugins/static.test.js

@@ -270,14 +270,19 @@ describe('static resource plugin', function() {
     });
 
     var TMP_PATH = path.join(__dirname, '../', '.tmp');
-    var RAW_REQUEST =
-        'GET /index.html HTTP/1.1\r\n' +
-        'Host: 127.0.0.1:' +
-        PORT +
-        '\r\n' +
-        'User-Agent: curl/7.48.0\r\n' +
-        'Accept: */*\r\n' +
-        '\r\n';
+    // built lazily (not at describe-registration time) since PORT is only
+    // assigned once beforeEach has run for the test that's about to use it.
+    function rawRequest() {
+        return (
+            'GET /index.html HTTP/1.1\r\n' +
+            'Host: 127.0.0.1:' +
+            PORT +
+            '\r\n' +
+            'User-Agent: curl/7.48.0\r\n' +
+            'Accept: */*\r\n' +
+            '\r\n'
+        );
+    }
 
     it(
         'static does not leak the file stream and next() is properly called ' +
@@ -322,7 +327,7 @@ describe('static resource plugin', function() {
                 });
 
                 socket.connect({ host: '127.0.0.1', port: PORT }, function() {
-                    socket.write(RAW_REQUEST, 'utf-8', function(err2, data) {
+                    socket.write(rawRequest(), 'utf-8', function(err2, data) {
                         assert.ifError(err2);
                     });
                 });
@@ -366,7 +371,7 @@ describe('static resource plugin', function() {
                 });
 
                 socket.connect({ host: '127.0.0.1', port: PORT }, function() {
-                    socket.write(RAW_REQUEST, 'utf-8', function(err2, data) {
+                    socket.write(rawRequest(), 'utf-8', function(err2, data) {
                         assert.ifError(err2);
                         socket.end();
                     });

test/request.test.js

@@ -287,3 +287,90 @@ test(
         });
     }
 );
+
+test(
+    module,
+    'getUrl should return correct shape for path with no query',
+    function(t) {
+        SERVER.get('/geturl-plain', function(req, res, next) {
+            var u = req.getUrl();
+            t.ok(u instanceof URL);
+            t.equal(u.href, 'http://127.0.0.1:' + PORT + '/geturl-plain');
+            t.equal(u.pathname, '/geturl-plain');
+            t.equal(u.search, '');
+            t.equal(u.hash, '');
+            t.equal(u.host, '127.0.0.1:' + PORT);
+            t.equal(u.hostname, '127.0.0.1');
+            t.equal(u.port, String(PORT));
+            t.equal(u.protocol, 'http:');
+            res.send();
+            return next();
+        });
+
+        CLIENT.get('/geturl-plain', function(err, _, res) {
+            t.ifError(err);
+            t.equal(res.statusCode, 200);
+            t.end();
+        });
+    }
+);
+
+test(module, 'getUrl should return correct query string', function(t) {
+    SERVER.get('/geturl-qs', function(req, res, next) {
+        var u = req.getUrl();
+        t.ok(u instanceof URL);
+        t.equal(u.href, 'http://127.0.0.1:' + PORT + '/geturl-qs?a=1&b=2');
+        t.equal(u.pathname, '/geturl-qs');
+        t.equal(u.search, '?a=1&b=2');
+        t.equal(u.searchParams.get('a'), '1');
+        t.equal(u.searchParams.get('b'), '2');
+        t.equal(u.hash, '');
+        t.equal(u.host, '127.0.0.1:' + PORT);
+        t.equal(u.hostname, '127.0.0.1');
+        t.equal(u.port, String(PORT));
+        t.equal(u.protocol, 'http:');
+        res.send();
+        return next();
+    });
+
+    CLIENT.get('/geturl-qs?a=1&b=2', function(err, _, res) {
+        t.ifError(err);
+        t.equal(res.statusCode, 200);
+        t.end();
+    });
+});
+
+test(module, 'getUrl should handle OPTIONS * request-target', function(t) {
+    SERVER.opts('*', function(req, res, next) {
+        var u = req.getUrl();
+        t.ok(u instanceof URL);
+        // a URL instance can't represent the literal '*' request-target -
+        // it resolves against the base path, so it comes out as '/*'.
+        t.equal(u.pathname, '/*');
+        t.equal(u.search, '');
+        res.send(200);
+        return next();
+    });
+
+    CLIENT.opts('*', function(err, _, res) {
+        t.ifError(err);
+        t.equal(res.statusCode, 200);
+        t.end();
+    });
+});
+
+test(module, 'getUrl result is cached across calls', function(t) {
+    SERVER.get('/geturl-cache', function(req, res, next) {
+        var u1 = req.getUrl();
+        var u2 = req.getUrl();
+        t.strictEqual(u1, u2);
+        res.send();
+        return next();
+    });
+
+    CLIENT.get('/geturl-cache', function(err, _, res) {
+        t.ifError(err);
+        t.equal(res.statusCode, 200);
+        t.end();
+    });
+});