From 1f1da97bc42675ae8076ad14d8994c2210778433 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Thu, 22 Oct 2026 15:33:18 +0000 Subject: [PATCH 1/2] feat: fix getUrl generation --- examples/http2/http2.js | 5 +- lib/request.js | 65 ++++---- lib/response.js | 26 ++- lib/server.js | 9 +- test/formatter-optional.test.js | 4 +- test/plugins/bodyReader.test.js | 5 +- test/request.test.js | 282 +++++++++++++++++++++++++++++++- test/response.test.js | 179 +++++++------------- test/server.test.js | 42 ++++- 9 files changed, 421 insertions(+), 196 deletions(-) diff --git a/examples/http2/http2.js b/examples/http2/http2.js index 3d9d64424..7fce750b4 100644 --- a/examples/http2/http2.js +++ b/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) }) ); diff --git a/lib/request.js b/lib/request.js index c97afb96e..7e8b4b15a 100644 --- a/lib/request.js +++ b/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,33 @@ 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}`; + + if ( + host.includes('://') || + /[\/?#@]/.test(host) || + !URL.canParse(baseUrl) + ) { + throw new BadRequestError('Invalid host'); + } + + this._url = new URL(`${baseUrl}${requestUrl}`); this._cacheURL = this.url; } return this._url; diff --git a/lib/response.js b/lib/response.js index 0d8dc3ca2..7119bb150 100644 --- a/lib/response.js +++ b/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,6 +840,11 @@ 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 @@ -847,23 +852,12 @@ function patch(Response) { // 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 diff --git a/lib/server.js b/lib/server.js index 09c54098f..c795426c5 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1112,7 +1112,14 @@ Server.prototype._afterPre = function _afterPre(err, req, res) { 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) { diff --git a/test/formatter-optional.test.js b/test/formatter-optional.test.js index 39f364740..92e949024 100644 --- a/test/formatter-optional.test.js +++ b/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'); diff --git a/test/plugins/bodyReader.test.js b/test/plugins/bodyReader.test.js index cd20fb949..3e48a4298 100644 --- a/test/plugins/bodyReader.test.js +++ b/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(); } diff --git a/test/request.test.js b/test/request.test.js index 31dd3db3f..c2360eec9 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -295,13 +295,11 @@ test( 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.href, 'http://localhost/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.hostname, 'localhost'); t.equal(u.protocol, 'http:'); res.send(); return next(); @@ -319,15 +317,13 @@ 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.href, 'http://localhost/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.hostname, 'localhost'); t.equal(u.protocol, 'http:'); res.send(); return next(); @@ -343,9 +339,11 @@ test(module, 'getUrl should return correct query string', function(t) { test(module, 'getUrl should handle OPTIONS * request-target', function(t) { SERVER.opts('*', function(req, res, next) { var u = req.getUrl(); + t.equal(req.url, '*'); 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.href, 'http://localhost/*'); t.equal(u.pathname, '/*'); t.equal(u.search, ''); res.send(200); @@ -359,6 +357,24 @@ test(module, 'getUrl should handle OPTIONS * request-target', function(t) { }); }); +test(module, 'getUrl should route a literal * path segment', function(t) { + SERVER.get('/api/test/:asterisk/some_test', function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal(u.href, 'http://localhost/api/test/*/some_test'); + t.equal(u.pathname, '/api/test/*/some_test'); + t.equal(req.params.asterisk, '*'); + res.send(200); + return next(); + }); + + CLIENT.get('/api/test/*/some_test', 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(); @@ -374,3 +390,253 @@ test(module, 'getUrl result is cached across calls', function(t) { t.end(); }); }); + +test(module, 'getUrl should handle a path starting with // correctly', function( + t +) { + SERVER.get('//foo/bar', function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal(u.href, 'http://localhost//foo/bar'); + t.equal(u.hostname, 'localhost'); + t.equal(u.pathname, '//foo/bar'); + t.equal(u.search, ''); + res.send(); + return next(); + }); + + CLIENT.get('//foo/bar', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +test(module, 'getUrl should not match // path to / route', function(t) { + SERVER.get('/foo/bar', function(req, res, next) { + t.ok(false, 'route handler should not be called'); + res.send(); + return next(); + }); + + CLIENT.get('//foo/bar', function(err, _, res) { + t.ok(err); + t.equal(res.statusCode, 404); + t.end(); + }); +}); + +test( + module, + 'getUrl should parse special characters after // as a path', + function(t) { + SERVER.get('//images', function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal(u.href, 'http://localhost//images;:@example.com'); + t.equal(u.hostname, 'localhost'); + t.equal(u.username, ''); + t.equal(u.pathname, '//images;:@example.com'); + res.send(); + return next(); + }); + + CLIENT.get('//images;:@example.com', function(err, _, res) { + t.ok(err); + t.equal(res.statusCode, 404); + t.end(); + }); + } +); + +test(module, 'getUrl should preserve special query characters', function(t) { + SERVER.get('/geturl-special-query', function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal( + u.href, + 'http://localhost/geturl-special-query?a=1&b=test\\@gmail.com' + ); + t.equal(u.pathname, '/geturl-special-query'); + t.equal(u.hostname, 'localhost'); + t.equal(u.search, '?a=1&b=test\\@gmail.com'); + t.equal(u.searchParams.get('a'), '1'); + t.equal(u.searchParams.get('b'), 'test\\@gmail.com'); + res.send(); + return next(); + }); + + CLIENT.get('/geturl-special-query?a=1&b=test\\@gmail.com', function( + err, + _, + res + ) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +test(module, 'getUrl should preserve encoded path characters', function(t) { + SERVER.get('/geturl-encoded/:value', function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal( + u.href, + 'http://localhost/geturl-encoded/a%2Fb%20c?email=test%40gmail.com' + ); + t.equal(u.pathname, '/geturl-encoded/a%2Fb%20c'); + t.equal(u.search, '?email=test%40gmail.com'); + t.equal(u.searchParams.get('email'), 'test@gmail.com'); + res.send(); + return next(); + }); + + CLIENT.get('/geturl-encoded/a%2Fb%20c?email=test%40gmail.com', function( + err, + _, + res + ) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +[ + { host: 'example.com', hostname: 'example.com', port: '' }, + { host: 'example.com:8080', hostname: 'example.com', port: '8080' }, + { host: '127.0.0.1', hostname: '127.0.0.1', port: '' }, + { host: '[::1]', hostname: '[::1]', port: '' }, + { + host: '[2001:db8::1]:8080', + hostname: '[2001:db8::1]', + port: '8080' + } +].forEach(function(testCase, index) { + test(module, 'getUrl should support HOST ' + testCase.host, function(t) { + var originalHost = process.env.HOST; + var path = '/geturl-valid-host-' + index; + + SERVER.get(path, function(req, res, next) { + var u = req.getUrl(); + t.ok(u instanceof URL); + t.equal(u.href, 'http://' + testCase.host + path + '?a=1'); + t.equal(u.hostname, testCase.hostname); + t.equal(u.port, testCase.port); + t.equal(u.pathname, path); + t.equal(u.search, '?a=1'); + res.send(); + return next(); + }); + + process.env.HOST = testCase.host; + CLIENT.get(path + '?a=1', function(err, _, res) { + if (originalHost === undefined) { + delete process.env.HOST; + } else { + process.env.HOST = originalHost; + } + + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); + }); +}); + +[ + '::1', + '[::1', + '[gggg::1]', + 'localhost:not-a-port', + 'localhost:65536', + 'bad host', + '999.999.999.999', + 'http://example.com', + '%' +].forEach(function(host, index) { + test(module, 'getUrl should reject HOST ' + host, function(t) { + var originalHost = process.env.HOST; + var path = '/geturl-invalid-host-' + index; + + SERVER.get(path, function(req, res, next) { + t.ok(false, 'route handler should not be called'); + res.send(); + return next(); + }); + + process.env.HOST = host; + CLIENT.get(path, function(err, _, res) { + if (originalHost === undefined) { + delete process.env.HOST; + } else { + process.env.HOST = originalHost; + } + + t.ok(err); + t.equal(res.statusCode, 400); + t.end(); + }); + }); +}); + +test(module, 'getUrl should use localhost when HOST is missing', function(t) { + var originalHost = process.env.HOST; + delete process.env.HOST; + + SERVER.get('/geturl-missing-host', function(req, res, next) { + var u = req.getUrl(); + t.equal(u.href, 'http://localhost/geturl-missing-host'); + t.equal(u.hostname, 'localhost'); + res.send(); + return next(); + }); + + CLIENT.get('/geturl-missing-host', function(err, _, res) { + if (originalHost !== undefined) { + process.env.HOST = originalHost; + } + + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +[ + 'http://example.com/foo?a=1', + 'https://example.com/foo', + 'http://[::1]:8080/foo', + 'http://', + 'http://[::1', + 'https://bad host/foo', + 'http://example.com:bad/foo' +].forEach(function(requestUrl, index) { + test(module, 'getUrl should reject absoluteURL ' + requestUrl, function(t) { + var path = '/geturl-absolute-' + index; + + SERVER.get(path, function(req, res, next) { + var originalUrl = req.url; + req.url = requestUrl; + + try { + req.getUrl(); + } catch (err) { + req.url = originalUrl; + return next(err); + } + + req.url = originalUrl; + t.ok(false, 'getUrl should throw'); + res.send(200); + return next(); + }); + + CLIENT.get(path, function(err, _, res) { + t.ok(err); + t.equal(res.statusCode, 400); + t.end(); + }); + }); +}); diff --git a/test/response.test.js b/test/response.test.js index be7bdb097..8db5874b6 100644 --- a/test/response.test.js +++ b/test/response.test.js @@ -22,9 +22,6 @@ var CLIENT; var STRING_CLIENT; var SERVER; -var LOCALHOST; -var SLOCALHOST; - before(module, function(cb) { try { SERVER = restify.createServer({ @@ -46,8 +43,6 @@ before(module, function(cb) { dtrace: helper.dtrace, retry: false }); - LOCALHOST = 'http://' + '127.0.0.1:' + PORT; - SLOCALHOST = 'https://' + '127.0.0.1:' + PORT; cb(); }); @@ -72,18 +67,12 @@ after(module, function(cb) { } }); -// helper for joining array into strings -function join() { - var args = [].slice.call(arguments, 0); - return args.join(''); -} - test(module, 'redirect to new string url as-is', function(t) { SERVER.get('/1', function(req, res, next) { res.redirect('www.foo.com', next); }); - CLIENT.get(join(LOCALHOST, '/1'), function(err, _, res) { + CLIENT.get('/1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, 'www.foo.com'); @@ -96,7 +85,7 @@ test(module, 'redirect to new relative string url as-is', function(t) { res.redirect('/1', next); }); - CLIENT.get(join(LOCALHOST, '/20?a=1'), function(err, _, res) { + CLIENT.get('/20?a=1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, '/1'); @@ -114,10 +103,10 @@ test(module, 'redirect to current url (reload)', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/2'), function(err, _, res) { + CLIENT.get('/2', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(LOCALHOST, '/2')); + t.equal(res.headers.location, '/2'); t.end(); }); }); @@ -132,10 +121,10 @@ test(module, 'redirect to current url from http -> https', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/3'), function(err, _, res) { + CLIENT.get('/3', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(SLOCALHOST, '/3')); + t.equal(res.headers.location, '/3'); t.end(); }); }); @@ -151,10 +140,10 @@ test(module, 'redirect to current url from https -> http', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/3'), function(err, _, res) { + CLIENT.get('/3', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(LOCALHOST, '/3')); + t.equal(res.headers.location, '/3'); t.end(); }); }); @@ -169,10 +158,10 @@ test(module, 'redirect by changing path', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/4'), function(err, _, res) { + CLIENT.get('/4', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(LOCALHOST, '/1')); + t.equal(res.headers.location, '/1'); t.end(); }); }); @@ -222,10 +211,10 @@ test(module, 'redirect should add query params', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/5'), function(err, _, res) { + CLIENT.get('/5', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(LOCALHOST, '/5?a=1')); + t.equal(res.headers.location, '/5?a=1'); t.end(); }); }); @@ -242,15 +231,11 @@ test(module, 'redirect should extend existing query params', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/6?a=1'), function(err, _, res) { + CLIENT.get('/6?a=1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = new URL(res.headers.location); - t.equal(parsedUrl.searchParams.get('a'), '1'); - t.equal(parsedUrl.searchParams.get('b'), '2'); - t.equal(parsedUrl.pathname, '/6'); - // t.equal(res.headers.location, join(LOCALHOST, '/6?a=1&b=2')); + t.equal(res.headers.location, '/6?b=2&a=1'); t.end(); }); }); @@ -268,10 +253,10 @@ test(module, 'redirect should stomp over existing query params', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/7?a=1'), function(err, _, res) { + CLIENT.get('/7?a=1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, join(LOCALHOST, '/7?b=2')); + t.equal(res.headers.location, '/7?b=2'); t.end(); }); }); @@ -286,10 +271,10 @@ test(module, 'redirect with 301 status code', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/8'), function(err, _, res) { + CLIENT.get('/8', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 301); - t.equal(res.headers.location, join(LOCALHOST, '/8')); + t.equal(res.headers.location, '/8'); t.end(); }); }); @@ -299,7 +284,7 @@ test(module, 'redirect with 301 status code ising string url', function(t) { res.redirect(301, '/foo', next); }); - CLIENT.get(join(LOCALHOST, '/30'), function(err, _, res) { + CLIENT.get('/30', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 301); t.equal(res.headers.location, '/foo'); @@ -321,7 +306,7 @@ test(module, 'redirect using options.url', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/8'), function(err, _, res) { + CLIENT.get('/8', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, 'http://www.foo.com/8?a=1'); @@ -339,7 +324,7 @@ test(module, 'redirect using opts.port', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { + CLIENT.get('/9', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); var parsedUrl = new URL(res.headers.location); @@ -360,7 +345,7 @@ test(module, 'redirect using external url and custom port', function(t) { ); }); - CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { + CLIENT.get('/9', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); var parsedUrl = new URL(res.headers.location); @@ -381,14 +366,10 @@ test(module, 'redirect using default hostname with custom port', function(t) { next ); }); - - CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { + CLIENT.get('/9', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = new URL(res.headers.location); - t.equal(Number(parsedUrl.port), 3000); - t.equal(parsedUrl.pathname, '/99'); - t.equal(res.headers.location, 'http://127.0.0.1:3000/99'); + t.equal(res.headers.location, 'http://localhost:3000/99'); t.end(); }); }); @@ -398,10 +379,10 @@ test(module, 'redirect opts.port exact location header', function(t) { res.redirect({ port: 3000 }, next); }); - CLIENT.get(join(LOCALHOST, '/port-pin'), function(err, _, res) { + CLIENT.get('/port-pin', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - t.equal(res.headers.location, 'http://127.0.0.1:3000/port-pin'); + t.equal(res.headers.location, 'http://localhost:3000/port-pin'); t.end(); }); }); @@ -417,7 +398,7 @@ test( ); }); - CLIENT.get(join(LOCALHOST, '/extport-pin'), function(err, _, res) { + CLIENT.get('/extport-pin', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, 'http://www.foo.com:3000/bar'); @@ -431,14 +412,11 @@ test(module, 'redirect extend query params exact location header', function(t) { res.redirect({ query: { b: '2' } }, next); }); - CLIENT.get(join(LOCALHOST, '/extqs-pin?a=1'), function(err, _, res) { + CLIENT.get('/extqs-pin?a=1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var loc = res.headers.location; - var u = new URL(loc); - t.equal(u.searchParams.get('a'), '1'); - t.equal(u.searchParams.get('b'), '2'); - t.equal(u.pathname, '/extqs-pin'); + + t.equal(res.headers.location, '/extqs-pin?b=2&a=1'); t.end(); }); }); @@ -449,11 +427,10 @@ test(module, 'redirect should repeat query param when same key', function(t) { res.redirect({ query: { a: '2' } }, next); }); - CLIENT.get(join(LOCALHOST, '/dupqs-pin?a=1'), function(err, _, res) { + CLIENT.get('/dupqs-pin?a=1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var u = new URL(res.headers.location); - t.deepEqual(u.searchParams.getAll('a'), ['2', '1']); + t.deepEqual(res.headers.location, '/dupqs-pin?a=2&a=1'); t.end(); }); }); @@ -467,12 +444,10 @@ test( res.redirect({ pathname: '/other' }, next); }); - CLIENT.get(join(LOCALHOST, '/rel-path-pin'), function(err, _, res) { + CLIENT.get('/rel-path-pin', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var u = new URL(res.headers.location); - t.equal(u.pathname, '/other'); - t.equal(u.hostname, '127.0.0.1'); + t.equal(res.headers.location, '/other'); t.end(); }); } @@ -490,13 +465,10 @@ test( ); }); - CLIENT.get(join(LOCALHOST, '/rel-qs-pin'), function(err, _, res) { + CLIENT.get('/rel-qs-pin', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var u = new URL(res.headers.location); - t.equal(u.pathname, '/other'); - t.equal(u.searchParams.get('x'), '1'); - t.equal(u.searchParams.get('y'), '2'); + t.equal(res.headers.location, '/other?x=1&y=2'); t.end(); }); } @@ -511,7 +483,7 @@ test( res.redirect(); }); - CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res, body) { + CLIENT.get('/9', function(err, _, res, body) { t.equal(res.statusCode, 500); // json parse the response @@ -543,7 +515,7 @@ test( SERVER.get('/10', [A, redirect, B]); - CLIENT.get(join(LOCALHOST, '/10'), function(err, _, res) { + CLIENT.get('/10', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, '/10'); @@ -572,7 +544,7 @@ test(module, 'redirect should emit a redirect event', function(t) { SERVER.get('/10', [preRedirectHandler, redirect]); - CLIENT.get(join(LOCALHOST, '/10'), function(err, _, res) { + CLIENT.get('/10', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); t.equal(res.headers.location, '/10'); @@ -600,7 +572,7 @@ test(module, 'writeHead should emit a header event', function(t) { SERVER.get('/10', [handler]); - CLIENT.get(join(LOCALHOST, '/10'), function(err, _, res) { + CLIENT.get('/10', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); @@ -621,7 +593,7 @@ test(module, 'should fail to set header due to missing formatter', function(t) { return next(); }); - CLIENT.get(join(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/octet-stream'); @@ -635,7 +607,7 @@ test(module, 'should not fail to send null as body', function(t) { return next(); }); - CLIENT.get(join(LOCALHOST, '/12'), function(err, _, res) { + CLIENT.get('/12', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); t.end(); @@ -651,7 +623,7 @@ test( return next(); }); - CLIENT.get(join(LOCALHOST, '/13'), function(err, _, res) { + CLIENT.get('/13', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); t.end(); @@ -668,7 +640,7 @@ test( return next(); }); - CLIENT.get(join(LOCALHOST, '/14'), function(err, _, res, body) { + CLIENT.get('/14', function(err, _, res, body) { t.ifError(err); t.equal(res.statusCode, 200); // ensure error body was still sent @@ -691,7 +663,7 @@ test(module, 'GH-951: should send without formatting', function(t) { return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/15'), function(err, _, res, body) { + STRING_CLIENT.get('/15', function(err, _, res, body) { t.ifError(err); t.equal( body, @@ -724,7 +696,7 @@ test(module, 'GH-951: sendRaw accepts only strings or buffers', function(t) { }); // throw away response, we don't need it. - STRING_CLIENT.get(join(LOCALHOST, '/16')); + STRING_CLIENT.get('/16'); }); test(module, 'GH-1429: setting code with res.status not respected', function( @@ -735,7 +707,7 @@ test(module, 'GH-1429: setting code with res.status not respected', function( res.send(null); }); - CLIENT.get(join(LOCALHOST, '/404'), function(err, _, res) { + CLIENT.get('/404', function(err, _, res) { t.equal(res.statusCode, 404); t.end(); }); @@ -748,7 +720,7 @@ test(module, 'should support multiple set-cookie headers', function(t) { res.send(null); }); - CLIENT.get(join(LOCALHOST, '/set-cookie'), function(err, _, res) { + CLIENT.get('/set-cookie', function(err, _, res) { t.equal(res.headers['set-cookie'].length, 2); t.end(); }); @@ -762,20 +734,10 @@ test(module, 'GH-1607: should send bools with explicit status code', function( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/bool/false'), function( - err, - req, - res, - data - ) { + STRING_CLIENT.get('/bool/false', function(err, req, res, data) { t.equal(data, 'false'); - STRING_CLIENT.get(join(LOCALHOST, '/bool/true'), function( - err2, - req2, - res2, - data2 - ) { + STRING_CLIENT.get('/bool/true', function(err2, req2, res2, data2) { t.equal(data2, 'true'); t.end(); }); @@ -795,14 +757,9 @@ test(module, 'GH-1607: should send numbers with explicit status code', function( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/zero'), function(err, req, res, data) { + STRING_CLIENT.get('/zero', function(err, req, res, data) { t.equal(data, '0'); - STRING_CLIENT.get(join(LOCALHOST, '/one'), function( - err2, - req2, - res2, - data2 - ) { + STRING_CLIENT.get('/one', function(err2, req2, res2, data2) { t.equal(data2, '1'); t.end(); }); @@ -816,7 +773,7 @@ test(module, 'GH-1791: should send 0 as 0 with application/json', function(t) { return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/zero'), function(err, req, res, data) { + STRING_CLIENT.get('/zero', function(err, req, res, data) { t.equal(data, '0'); t.end(); }); @@ -832,12 +789,7 @@ test( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/false'), function( - err, - req, - res, - data - ) { + STRING_CLIENT.get('/false', function(err, req, res, data) { t.equal(data, 'false'); t.end(); }); @@ -855,12 +807,7 @@ test( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/empty'), function( - err, - req, - res, - data - ) { + STRING_CLIENT.get('/empty', function(err, req, res, data) { t.equal(data, '""'); t.end(); }); @@ -877,12 +824,7 @@ test( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/null'), function( - err, - req, - res, - data - ) { + STRING_CLIENT.get('/null', function(err, req, res, data) { t.equal(data, 'null'); t.end(); }); @@ -900,12 +842,7 @@ test( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/undef'), function( - err, - req, - res, - data - ) { + STRING_CLIENT.get('/undef', function(err, req, res, data) { t.equal(data, ''); t.end(); }); @@ -921,7 +858,7 @@ test(module, 'GH-1791: should send NaN as null with application/json', function( return next(); }); - STRING_CLIENT.get(join(LOCALHOST, '/nan'), function(err, req, res, data) { + STRING_CLIENT.get('/nan', function(err, req, res, data) { t.equal(data, 'null'); t.end(); }); diff --git a/test/server.test.js b/test/server.test.js index cbda2aac1..08a3f2865 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1157,18 +1157,17 @@ test( 'gh-757 req.absoluteUri() defaults path segment to req.path()', function(t) { SERVER.get('/the-original-path', function(req, res, next) { - var prefix = 'http://127.0.0.1:' + PORT; t.equal( req.absoluteUri('?key=value'), - prefix + '/the-original-path/?key=value' + 'http://localhost/the-original-path/?key=value' ); t.equal( req.absoluteUri('#fragment'), - prefix + '/the-original-path/#fragment' + 'http://localhost/the-original-path/#fragment' ); t.equal( req.absoluteUri('?key=value#fragment'), - prefix + '/the-original-path/?key=value#fragment' + 'http://localhost/the-original-path/?key=value#fragment' ); res.send(); next(); @@ -1188,9 +1187,17 @@ test( 'req.absoluteUri() resolves plain subpath relative to current path', function(t) { SERVER.get('/base-path', function(req, res, next) { - var prefix = 'http://127.0.0.1:' + PORT; - t.equal(req.absoluteUri('child'), prefix + '/base-path/child'); - t.equal(req.absoluteUri('/absolute'), prefix + '/absolute'); + t.equal( + req.absoluteUri('child'), + 'http://localhost/base-path/child' + ); + t.equal(req.absoluteUri(''), 'http://localhost/base-path/'); + t.equal( + req.absoluteUri('./child'), + 'http://localhost/base-path/child' + ); + t.equal(req.absoluteUri('../sibling'), 'http://localhost/sibling'); + t.equal(req.absoluteUri('/absolute'), 'http://localhost/absolute'); res.send(); next(); }); @@ -1692,6 +1699,27 @@ test( } ); +test( + module, + 'req.absoluteUri() avoids duplicate slash after trailing slash', + function(t) { + SERVER.get('/base-path/', function(req, res, next) { + t.equal( + req.absoluteUri('child'), + 'http://localhost/base-path/child' + ); + res.send(); + next(); + }); + + CLIENT.get('/base-path/', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); + } +); + test(module, 'GH-1078: server name should default to restify', function(t) { var myServer = restify.createServer(); var port = 3000; From fb8525ff7f1708fc54882eb5a2ecf5354664ee55 Mon Sep 17 00:00:00 2001 From: Alessia Bellisario Date: Thu, 22 Oct 2026 16:43:03 +0000 Subject: [PATCH 2/2] fix: move HOST validation to createServer --- lib/index.js | 9 +++++++++ lib/request.js | 8 -------- test/request.test.js | 31 ++++++++++++++++--------------- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/lib/index.js b/lib/index.js index c8962a907..933ad9e71 100644 --- a/lib/index.js +++ b/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; diff --git a/lib/request.js b/lib/request.js index 7e8b4b15a..ceaf2ff83 100644 --- a/lib/request.js +++ b/lib/request.js @@ -463,14 +463,6 @@ function patch(Request) { var requestUrl = this.url === '*' ? '/*' : this.url; var baseUrl = `${protocol}${host}`; - if ( - host.includes('://') || - /[\/?#@]/.test(host) || - !URL.canParse(baseUrl) - ) { - throw new BadRequestError('Invalid host'); - } - this._url = new URL(`${baseUrl}${requestUrl}`); this._cacheURL = this.url; } diff --git a/test/request.test.js b/test/request.test.js index c2360eec9..bd5f9c05d 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -531,6 +531,9 @@ test(module, 'getUrl should preserve encoded path characters', function(t) { }); process.env.HOST = testCase.host; + t.doesNotThrow(function() { + restify.createServer(); + }); CLIENT.get(path + '?a=1', function(err, _, res) { if (originalHost === undefined) { delete process.env.HOST; @@ -554,30 +557,28 @@ test(module, 'getUrl should preserve encoded path characters', function(t) { 'bad host', '999.999.999.999', 'http://example.com', + 'example.com/path', + 'example.com?query', + 'example.com#fragment', + 'user@example.com', '%' -].forEach(function(host, index) { - test(module, 'getUrl should reject HOST ' + host, function(t) { +].forEach(function(host) { + test(module, 'createServer should reject HOST ' + host, function(t) { var originalHost = process.env.HOST; - var path = '/geturl-invalid-host-' + index; - - SERVER.get(path, function(req, res, next) { - t.ok(false, 'route handler should not be called'); - res.send(); - return next(); - }); process.env.HOST = host; - CLIENT.get(path, function(err, _, res) { + try { + t.throws(function() { + restify.createServer(); + }, /Invalid HOST environment variable/); + } finally { if (originalHost === undefined) { delete process.env.HOST; } else { process.env.HOST = originalHost; } - - t.ok(err); - t.equal(res.statusCode, 400); - t.end(); - }); + } + t.end(); }); });