feat: fix getUrl generation

examples/http2/http2.js

@@ -22,10 +22,7 @@ srv.on(
     restify.plugins.auditLogger({
         event: 'after',
         body: true,
-        log: pino(
-            { name: 'audit' },
-            process.stdout
-        )
+        log: pino({ name: 'audit' }, process.stdout)
     })
 );
 

lib/index.js

@@ -74,6 +74,15 @@ var InternalError = errors.InternalError;
 function createServer(options) {
     assert.optionalObject(options, 'options');
 
+    var host = process.env.HOST || 'localhost';
+    if (
+        host.includes('://') ||
+        /[\/?#@]/.test(host) ||
+        !URL.canParse('http://' + host)
+    ) {
+        assert.fail('Invalid HOST environment variable');
+    }
+
     var opts = shallowCopy(options || {});
     var server;
 

lib/request.js

@@ -8,6 +8,7 @@ var crypto = require('crypto');
 var assert = require('assert-plus');
 var mime = require('mime');
 var Negotiator = require('negotiator');
+var BadRequestError = require('restify-errors').BadRequestError;
 
 var dtrace = require('./dtrace');
 
@@ -82,22 +83,8 @@ function patch(Request) {
             return base.origin + path;
         }
 
-        if (firstChar === '?' || firstChar === '#' || path === '') {
-            // query-only / fragment-only reference: keep the current
-            // (slash-terminated) path, append path as-is.
-            return base.origin + base.pathname + '/' + path;
-        }
-
-        if (path.indexOf('..') === -1) {
-            // plain relative segment (e.g. "child") with no dot-segments to
-            // resolve: safe to join directly without invoking `new URL()`.
-            return base.origin + base.pathname + '/' + path;
-        }
-
-        // relative reference containing ".."/"." segments - fall back to
-        // `new URL()` since correctly resolving those requires the real
-        // relative-URL-resolution algorithm.
-        return new URL(path, base.origin + base.pathname + '/').href;
+        var separator = base.pathname.endsWith('/') ? '' : '/';
+        return new URL(path, base.origin + base.pathname + separator).href;
     };
 
     /**
@@ -280,7 +267,8 @@ function patch(Request) {
      * @returns {String} URI
      */
     Request.prototype.getHref = function getHref() {
-        return this.getUrl().href;
+        var url = this.getUrl();
+        return `${url.pathname}${url.search}`;
     };
 
     /**
@@ -294,7 +282,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());
-     *     // => http://localhost:3000/foo/bar?a=1
+     *     // => /foo/bar?a=1
      * });
      */
     Request.prototype.href = Request.prototype.getHref;
@@ -457,20 +445,25 @@ function patch(Request) {
     Request.prototype.getUrl = function getUrl() {
         if (this._cacheURL !== 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);
+            var host = process.env.HOST || 'localhost';
+            var lowercaseUrl = this.url.toLowerCase();
+
+            // this.url can not be absolute.
+            if (
+                URL.canParse(this.url) ||
+                lowercaseUrl.startsWith('http://') ||
+                lowercaseUrl.startsWith('https://')
+            ) {
+                throw new BadRequestError('Invalid URL');
+            }
+
+            // OPTIONS may use "*" to target the entire server; represent it
+            // as a valid URL path.
+            // https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Methods/OPTIONS
+            var requestUrl = this.url === '*' ? '/*' : this.url;
+            var baseUrl = `${protocol}${host}`;
+
+            this._url = new URL(`${baseUrl}${requestUrl}`);
             this._cacheURL = this.url;
         }
         return this._url;

lib/response.js

@@ -748,12 +748,12 @@ function patch(Response) {
         //    as a way to do a reload of the current page.
         if (arg1 && !isNaN(arg1)) {
             statusCode = arg1;
-            finalUri = arg2;
+            redirectLocation = arg2;
             next = arg3;
         } else if (typeof arg1 === 'string') {
             // 2) this is signaure number 2
             // otherwise, it's a string, and use it directly
-            finalUri = arg1;
+            redirectLocation = arg1;
             next = arg2;
         } else if (typeof arg1 === 'object') {
             // 3) signature number 3, using an options object.
@@ -762,7 +762,7 @@ function patch(Response) {
 
             var req = self.req;
             var opt = arg1 || {};
-            var currentFullPath = req.href();
+            var currentFullPath = req.getUrl().href;
             var secure = opt.hasOwnProperty('secure')
                 ? opt.secure
                 : req.isSecure();
@@ -840,30 +840,24 @@ function patch(Response) {
             if (opt.permanent) {
                 statusCode = 301;
             }
+
+            redirectLocation =
+                opt.hostname || opt.port || finalHostname
+                    ? finalUri.toString()
+                    : finalUri.pathname + (finalUri.search || '');
         }
 
         // if we're missing a next we should probably throw. if user wanted
         // to redirect but we were unable to do so, we should not continue
         // down the handler stack.
         assert.func(next, 'res.redirect() requires a next param');
 
-        // if we are missing a finalized uri
+        // if we are missing a redirectLocation
         // by this point, pass an error to next.
-        if (!finalUri) {
+        if (!redirectLocation) {
             return next(new InternalServerError('could not construct url'));
         }
 
-        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);
 
         // now we're done constructing url, send the res

lib/server.js

@@ -837,7 +837,7 @@
         // get each route's handler chain
         var routes = _.map(routeInfo, function mapValues(route) {
             route.handlers = Array.prototype.concat.call(
                 // TODO: should it contain use handlers?
                 useHandlers,
                 route.handlers.map(funcNameMapper)
             );
@@ -1112,7 +1112,14 @@
 Server.prototype._runRoute = function _runRoute(req, res) {
     var self = this;
 
-    var routeHandler = self.router.lookup(req, res);
+    var routeHandler;
+    try {
+        routeHandler = self.router.lookup(req, res);
+    } catch (err) {
+        self._onHandlerError(err, req, res);
+        self._finishReqResCycle(req, res, err);
+        return;
+    }
 
     if (!routeHandler) {
         self.router.defaultRoute(req, res, function afterRouter(err) {

test/formatter-optional.test.js

@@ -17,7 +17,6 @@ var before = helper.before;
 var test = helper.test;
 
 var CLIENT;
-var LOCALHOST;
 var PORT = process.env.UNIT_TEST_PORT || 0;
 var SERVER;
 
@@ -37,7 +36,6 @@ before(module, function(callback) {
                 dtrace: helper.dtrace,
                 retry: false
             });
-            LOCALHOST = 'http://' + '127.0.0.1:' + PORT;
             callback();
         });
     } catch (e) {
@@ -69,7 +67,7 @@ test(
             return next();
         });
 
-        CLIENT.get(LOCALHOST + '/11', function(err, _, res) {
+        CLIENT.get('/11', function(err, _, res) {
             t.ifError(err);
             t.equal(res.statusCode, 200);
             t.equal(res.headers['content-type'], 'application/hal+json');

test/plugins/bodyReader.test.js

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