From 4fd788ba4c5b64b47c62b9cb919fd6b3d6ae8ee9 Mon Sep 17 00:00:00 2001 From: dvinakur Date: Tue, 16 Jun 2026 07:33:37 +0200 Subject: [PATCH 1/5] feat: replace deprecated url.resolve() and url.parse() with WHATWG URL --- lib/request.js | 6 +- lib/response.js | 23 +++---- lib/utils.js | 120 ++++++++++++++++++++++++++++++++- test/request.test.js | 79 ++++++++++++++++++++++ test/response.test.js | 116 ++++++++++++++++++++++++++++---- test/server.test.js | 17 +++++ test/utils.test.js | 153 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 484 insertions(+), 30 deletions(-) diff --git a/lib/request.js b/lib/request.js index 60f15fed7..2a4d3ccdd 100644 --- a/lib/request.js +++ b/lib/request.js @@ -2,7 +2,6 @@ 'use strict'; -var url = require('url'); var sprintf = require('util').format; var assert = require('assert-plus'); @@ -11,6 +10,7 @@ var Negotiator = require('negotiator'); var uuid = require('uuid'); var dtrace = require('./dtrace'); +var utils = require('./utils'); ///-- Helpers /** @@ -74,7 +74,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; }; /** @@ -424,7 +424,7 @@ function patch(Request) { */ Request.prototype.getUrl = function getUrl() { if (this._cacheURL !== this.url) { - this._url = url.parse(this.url); + this._url = utils.parseRequestUrl(this.url); this._cacheURL = this.url; } return this._url; diff --git a/lib/response.js b/lib/response.js index 87aced7ad..1e666257d 100644 --- a/lib/response.js +++ b/lib/response.js @@ -4,7 +4,6 @@ var http = require('http'); var sprintf = require('util').format; -var url = require('url'); var assert = require('assert-plus'); var mime = require('mime'); @@ -770,18 +769,16 @@ function patch(Response) { // 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. + var parsedRaw = utils.parseRequestUrl( + opt.hostname || currentFullPath + ); finalUri = { - port: parsedUri.port, - hostname: parsedUri.hostname, - query: parsedUri.query, - pathname: parsedUri.pathname + port: parsedRaw.port, + hostname: parsedRaw.hostname, + query: utils.parseUrlQuery( + new URLSearchParams(parsedRaw.query || '') + ), + pathname: parsedRaw.pathname }; // start building url based on options. @@ -832,7 +829,7 @@ function patch(Response) { return next(new InternalServerError('could not construct url')); } - redirectLocation = url.format(finalUri); + redirectLocation = utils.formatUrl(finalUri); self.emit('redirect', redirectLocation); diff --git a/lib/utils.js b/lib/utils.js index 77da5389e..9d4423071 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -54,9 +54,127 @@ function mergeQs(obj1, obj2) { return merged; } +/** + * Converts a URLSearchParams instance into a plain object, + * accumulating duplicate keys into arrays (mirrors url.parse behavior). + * + * @public + * @function parseUrlQuery + * @param {URLSearchParams} searchParams - URLSearchParams instance from a parsed URL + * @returns {Object} plain object with string or string-array values + */ +function parseUrlQuery(searchParams) { + var query = {}; + searchParams.forEach(function forEach(v, k) { + if (query.hasOwnProperty(k)) { + query[k] = [].concat(query[k], v); + } else { + query[k] = v; + } + }); + return query; +} + +/** + * Formats a URI descriptor object into a URL string, + * replacing url.format(). Handles array query values as repeated params. + * + * @public + * @function formatUrl + * @param {String|Object} uri - a string or {protocol, hostname, port, pathname, query} + * @returns {String} formatted URL string + */ +function formatUrl(uri) { + if (typeof uri === 'string') { + return uri; + } + + var loc = ''; + if (uri.hostname) { + loc = + (uri.protocol || 'http') + + '://' + + uri.hostname + + (uri.port ? ':' + uri.port : ''); + } + + var pathname = uri.pathname || '/'; + loc += pathname[0] === '/' ? pathname : '/' + pathname; + + if (uri.query && Object.keys(uri.query).length > 0) { + var params = new URLSearchParams(); + Object.keys(uri.query).forEach(function forEach(k) { + var v = uri.query[k]; + if (Array.isArray(v)) { + v.forEach(function forEachItem(item) { + params.append(k, item); + }); + } else { + params.set(k, v); + } + }); + loc += '?' + params.toString(); + } + + return loc; +} + +/** + * Parses a raw HTTP request URL string into a legacy url.parse-shaped object. + * Handles the special OPTIONS '*' request-target. + * + * @public + * @function parseRequestUrl + * @param {String} rawUrl - the raw request URL string + * @returns {Object} url descriptor with pathname, search, query, href etc. + */ +function parseRequestUrl(rawUrl) { + var pathname, search, hash, port, hostname; + + if (rawUrl === '*') { + pathname = '*'; + search = null; + hash = null; + port = null; + hostname = null; + } else if (rawUrl.indexOf('://') !== -1) { + var absParsed = new URL(rawUrl); + pathname = absParsed.pathname; + search = absParsed.search || null; + hash = absParsed.hash || null; + port = absParsed.port || null; + hostname = absParsed.hostname || null; + } else { + var relParsed = new URL(rawUrl, 'http://localhost'); + pathname = relParsed.pathname; + search = relParsed.search || null; + hash = relParsed.hash || null; + port = null; + hostname = null; + } + + return { + protocol: null, + slashes: null, + auth: null, + host: null, + port: port, + hostname: hostname, + hash: hash, + search: search, + query: search ? search.slice(1) : null, + pathname: pathname, + path: pathname + (search || ''), + href: rawUrl + }; +} + ///--- Exports module.exports = { shallowCopy: shallowCopy, - mergeQs: mergeQs + mergeQs: mergeQs, + parseUrlQuery: parseUrlQuery, + formatUrl: formatUrl, + parseRequestUrl: parseRequestUrl }; diff --git a/test/request.test.js b/test/request.test.js index 44764505f..3f0708c79 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -275,3 +275,82 @@ test('should emit restifyDone event when request is fully served with error', fu clientDone = true; }); }); + +test('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.equal(u.href, '/geturl-plain'); + t.equal(u.pathname, '/geturl-plain'); + t.equal(u.search, null); + t.equal(u.query, null); + t.equal(u.hash, null); + t.equal(u.host, null); + t.equal(u.hostname, null); + t.equal(u.port, null); + t.equal(u.protocol, null); + res.send(); + return next(); + }); + + CLIENT.get('/geturl-plain', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +test('getUrl should return correct query string', function(t) { + SERVER.get('/geturl-qs', function(req, res, next) { + var u = req.getUrl(); + t.equal(u.href, '/geturl-qs?a=1&b=2'); + t.equal(u.pathname, '/geturl-qs'); + t.equal(u.search, '?a=1&b=2'); + t.equal(u.query, 'a=1&b=2'); + t.equal(u.hash, null); + t.equal(u.host, null); + t.equal(u.hostname, null); + t.equal(u.port, null); + t.equal(u.protocol, null); + 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('getUrl should handle OPTIONS * request-target', function(t) { + SERVER.opts('*', function(req, res, next) { + var u = req.getUrl(); + t.equal(u.pathname, '*'); + t.equal(u.search, null); + t.equal(u.query, null); + res.send(200); + return next(); + }); + + CLIENT.opts('*', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + +test('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(); + }); +}); diff --git a/test/response.test.js b/test/response.test.js index 373bc2145..773375b95 100644 --- a/test/response.test.js +++ b/test/response.test.js @@ -1,7 +1,6 @@ 'use strict'; /* eslint-disable func-names */ -var url = require('url'); var restifyClients = require('restify-clients'); var errs = require('restify-errors'); @@ -245,12 +244,9 @@ test('redirect should extend existing query params', function(t) { CLIENT.get(join(LOCALHOST, '/6?a=1'), function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = url.parse(res.headers.location, true); - t.deepEqual(parsedUrl.query, { - a: 1, - b: 2 - }); - t.equal(parsedUrl.query.b, 2); + 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')); @@ -345,8 +341,8 @@ test('redirect using opts.port', function(t) { CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = url.parse(res.headers.location, true); - t.equal(parsedUrl.port, 3000); + var parsedUrl = new URL(res.headers.location); + t.equal(Number(parsedUrl.port), 3000); t.end(); }); }); @@ -366,8 +362,8 @@ test('redirect using external url and custom port', function(t) { CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = url.parse(res.headers.location, true); - t.equal(parsedUrl.port, 3000); + var parsedUrl = new URL(res.headers.location); + t.equal(Number(parsedUrl.port), 3000); t.equal(parsedUrl.hostname, 'www.foo.com'); t.equal(parsedUrl.pathname, '/99'); t.end(); @@ -388,14 +384,108 @@ test('redirect using default hostname with custom port', function(t) { CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 302); - var parsedUrl = url.parse(res.headers.location, true); - t.equal(parsedUrl.port, 3000); + 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.end(); }); }); +test('redirect opts.port exact location header', function(t) { + SERVER.get('/port-pin', function(req, res, next) { + res.redirect({ port: 3000 }, next); + }); + + CLIENT.get(join(LOCALHOST, '/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.end(); + }); +}); + +test('redirect external hostname and port exact location header', function(t) { + SERVER.get('/extport-pin', function(req, res, next) { + res.redirect( + { hostname: 'www.foo.com', pathname: '/bar', port: 3000 }, + next + ); + }); + + CLIENT.get(join(LOCALHOST, '/extport-pin'), function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 302); + t.equal(res.headers.location, 'http://www.foo.com:3000/bar'); + t.end(); + }); +}); + +test('redirect extend query params exact location header', function(t) { + SERVER.get('/extqs-pin', function(req, res, next) { + res.redirect({ query: { b: '2' } }, next); + }); + + CLIENT.get(join(LOCALHOST, '/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.end(); + }); +}); + +// eslint-disable-next-line max-len +test('redirect should repeat query param when same key exists in both current and new query', function(t) { + SERVER.get('/dupqs-pin', function(req, res, next) { + res.redirect({ query: { a: '2' } }, next); + }); + + CLIENT.get(join(LOCALHOST, '/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.end(); + }); +}); + +// eslint-disable-next-line max-len +test('redirect with only pathname (no hostname) uses current host', function(t) { + SERVER.get('/rel-path-pin', function(req, res, next) { + res.redirect({ pathname: '/other' }, next); + }); + + CLIENT.get(join(LOCALHOST, '/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.end(); + }); +}); + +// eslint-disable-next-line max-len +test('redirect with pathname and query (no hostname) preserves query string', function(t) { + SERVER.get('/rel-qs-pin', function(req, res, next) { + res.redirect({ pathname: '/other', query: { x: '1', y: '2' } }, next); + }); + + CLIENT.get(join(LOCALHOST, '/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.end(); + }); +}); + // eslint-disable-next-line test('redirect should cause InternalError when invoked without next', function(t) { SERVER.get('/9', function(req, res, next) { diff --git a/test/server.test.js b/test/server.test.js index c1d2cbc89..730c19c35 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1161,6 +1161,23 @@ test('gh-757 req.absoluteUri() defaults path segment to req.path()', function(t) }); }); +// eslint-disable-next-line max-len +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'); + res.send(); + next(); + }); + + CLIENT.get('/base-path', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); + test('GH-693 sending multiple response header values', function(t) { SERVER.get('/', function(req, res, next) { res.link('/', 'self'); diff --git a/test/utils.test.js b/test/utils.test.js index 37792010e..8275e02e2 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -2,6 +2,9 @@ /* eslint-disable func-names */ var mergeQs = require('../lib/utils').mergeQs; +var parseRequestUrl = require('../lib/utils').parseRequestUrl; +var parseUrlQuery = require('../lib/utils').parseUrlQuery; +var formatUrl = require('../lib/utils').formatUrl; if (require.cache[__dirname + '/lib/helper.js']) { delete require.cache[__dirname + '/lib/helper.js']; @@ -12,6 +15,156 @@ var helper = require('./lib/helper.js'); var test = helper.test; +// parseRequestUrl + +test('parseRequestUrl: plain path', function(t) { + var r = parseRequestUrl('/foo/bar'); + + t.equal(r.pathname, '/foo/bar'); + t.equal(r.search, null); + t.equal(r.query, null); + t.equal(r.hash, null); + t.equal(r.path, '/foo/bar'); + t.equal(r.href, '/foo/bar'); + t.equal(r.hostname, null); + t.equal(r.port, null); + t.done(); +}); + +test('parseRequestUrl: path with query string', function(t) { + var r = parseRequestUrl('/foo?a=1&b=2'); + + t.equal(r.pathname, '/foo'); + t.equal(r.search, '?a=1&b=2'); + t.equal(r.query, 'a=1&b=2'); + t.equal(r.path, '/foo?a=1&b=2'); + t.equal(r.href, '/foo?a=1&b=2'); + t.done(); +}); + +test('parseRequestUrl: path with hash', function(t) { + var r = parseRequestUrl('/foo/bar#section'); + + t.equal(r.pathname, '/foo/bar'); + t.equal(r.hash, '#section'); + t.equal(r.search, null); + t.equal(r.href, '/foo/bar#section'); + t.done(); +}); + +test('parseRequestUrl: absolute URL', function(t) { + var r = parseRequestUrl('http://example.com/foo?x=1'); + + t.equal(r.pathname, '/foo'); + t.equal(r.search, '?x=1'); + t.equal(r.query, 'x=1'); + t.equal(r.hostname, 'example.com'); + t.equal(r.port, null); + t.equal(r.path, '/foo?x=1'); + t.done(); +}); + +test('parseRequestUrl: absolute URL with port', function(t) { + var r = parseRequestUrl('http://example.com:8080/foo'); + + t.equal(r.pathname, '/foo'); + t.equal(r.hostname, 'example.com'); + t.equal(r.port, '8080'); + t.equal(r.search, null); + t.done(); +}); + +test('parseRequestUrl: OPTIONS * request-target', function(t) { + var r = parseRequestUrl('*'); + + t.equal(r.pathname, '*'); + t.equal(r.search, null); + t.equal(r.query, null); + t.equal(r.hash, null); + t.equal(r.path, '*'); + t.equal(r.href, '*'); + t.done(); +}); + +// parseUrlQuery + +test('parseUrlQuery: simple params become object', function(t) { + var sp = new URL('/foo?a=1&b=2', 'http://localhost').searchParams; + + t.deepEqual(parseUrlQuery(sp), { a: '1', b: '2' }); + t.done(); +}); + +test('parseUrlQuery: duplicate keys accumulate into array', function(t) { + var sp = new URL('/foo?a=1&a=2&b=3', 'http://localhost').searchParams; + + t.deepEqual(parseUrlQuery(sp), { a: ['1', '2'], b: '3' }); + t.done(); +}); + +test('parseUrlQuery: no query returns empty object', function(t) { + var sp = new URL('/foo', 'http://localhost').searchParams; + + t.deepEqual(parseUrlQuery(sp), {}); + t.done(); +}); + +// formatUrl + +test('formatUrl: pathname only', function(t) { + t.equal(formatUrl({ pathname: '/foo/bar' }), '/foo/bar'); + t.done(); +}); + +test('formatUrl: hostname + pathname', function(t) { + t.equal( + formatUrl({ + protocol: 'http', + hostname: 'example.com', + pathname: '/foo' + }), + 'http://example.com/foo' + ); + t.done(); +}); + +test('formatUrl: hostname + port + pathname', function(t) { + t.equal( + formatUrl({ + protocol: 'http', + hostname: 'example.com', + port: '8080', + pathname: '/foo' + }), + 'http://example.com:8080/foo' + ); + t.done(); +}); + +test('formatUrl: pathname + query object', function(t) { + t.equal( + formatUrl({ pathname: '/foo', query: { a: '1', b: '2' } }), + '/foo?a=1&b=2' + ); + t.done(); +}); + +test('formatUrl: string passthrough', function(t) { + t.equal(formatUrl('/already/a/string'), '/already/a/string'); + t.equal(formatUrl('http://example.com/foo'), 'http://example.com/foo'); + t.done(); +}); + +test('formatUrl: array query values become repeated params', function(t) { + t.equal( + formatUrl({ pathname: '/foo', query: { a: ['1', '2'] } }), + '/foo?a=1&a=2' + ); + t.done(); +}); + +// mergeQs + test('merge qs', function(t) { var qs1 = mergeQs(undefined, { a: 1 }); t.deepEqual(qs1, { a: 1 }); From 395d820148ba54f5f80736dae3f7e4b9341786ba Mon Sep 17 00:00:00 2001 From: dvinakur Date: Fri, 17 Jul 2026 11:23:19 +0200 Subject: [PATCH 2/5] fix: replace parseUrlQuery and formatUrl for URL --- lib/response.js | 71 +++++++++++++++++++++++++++-------------- lib/utils.js | 67 --------------------------------------- test/utils.test.js | 79 ---------------------------------------------- 3 files changed, 47 insertions(+), 170 deletions(-) diff --git a/lib/response.js b/lib/response.js index 1e666257d..3a22bf4b4 100644 --- a/lib/response.js +++ b/lib/response.js @@ -10,7 +10,6 @@ var mime = require('mime'); var errors = require('restify-errors'); var httpDate = require('./http_date'); -var utils = require('./utils'); ///--- Globals @@ -739,6 +738,7 @@ function patch(Response) { var self = this; var statusCode = 302; var finalUri; + var finalHostname; var redirectLocation; var next; @@ -767,30 +767,32 @@ function patch(Response) { ? opt.secure : req.isSecure(); - // if hostname is passed in, use that as the base, - // otherwise fall back on current url. - var parsedRaw = utils.parseRequestUrl( - opt.hostname || currentFullPath - ); - finalUri = { - port: parsedRaw.port, - hostname: parsedRaw.hostname, - query: utils.parseUrlQuery( - new URLSearchParams(parsedRaw.query || '') - ), - pathname: parsedRaw.pathname - }; + // if hostname is passed in, use that as the base, otherwise + // fall back on current url. mutate a URL instance directly + // instead of bouncing through a plain-object descriptor. + var raw = opt.hostname || currentFullPath; + var isAbsolute = raw.indexOf('://') !== -1; + finalUri = isAbsolute + ? new URL(raw) + : new URL(raw, 'http://localhost'); - // start building url based on options. - // start with the host if (opt.hostname) { - finalUri.hostname = opt.hostname; + // an explicit hostname replaces the current path entirely, + // so start from a clean slate. + finalUri.pathname = '/'; + finalUri.search = ''; } + // hostname is only known if it was passed in explicitly, or if + // the current url was itself absolute (e.g. proxied requests). + finalHostname = + opt.hostname || (isAbsolute ? 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 @@ -805,11 +807,23 @@ function patch(Response) { // then add query params if (opt.query) { - if (opt.overrideQuery === true) { - finalUri.query = opt.query; - } else { - finalUri.query = utils.mergeQs(opt.query, finalUri.query); + var merged = new URLSearchParams(); + + Object.keys(opt.query).forEach(function forEach(k) { + var v = opt.query[k]; + var values = Array.isArray(v) ? v : [v]; + values.forEach(function forEachItem(item) { + merged.append(k, item); + }); + }); + + if (opt.overrideQuery !== true) { + finalUri.searchParams.forEach(function forEach(v, k) { + merged.append(k, v); + }); } + + finalUri.search = merged.toString(); } // change status code to 301 permanent if specified @@ -829,7 +843,16 @@ function patch(Response) { return next(new InternalServerError('could not construct url')); } - redirectLocation = utils.formatUrl(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); diff --git a/lib/utils.js b/lib/utils.js index 9d4423071..bcdc00aca 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -54,71 +54,6 @@ function mergeQs(obj1, obj2) { return merged; } -/** - * Converts a URLSearchParams instance into a plain object, - * accumulating duplicate keys into arrays (mirrors url.parse behavior). - * - * @public - * @function parseUrlQuery - * @param {URLSearchParams} searchParams - URLSearchParams instance from a parsed URL - * @returns {Object} plain object with string or string-array values - */ -function parseUrlQuery(searchParams) { - var query = {}; - searchParams.forEach(function forEach(v, k) { - if (query.hasOwnProperty(k)) { - query[k] = [].concat(query[k], v); - } else { - query[k] = v; - } - }); - return query; -} - -/** - * Formats a URI descriptor object into a URL string, - * replacing url.format(). Handles array query values as repeated params. - * - * @public - * @function formatUrl - * @param {String|Object} uri - a string or {protocol, hostname, port, pathname, query} - * @returns {String} formatted URL string - */ -function formatUrl(uri) { - if (typeof uri === 'string') { - return uri; - } - - var loc = ''; - if (uri.hostname) { - loc = - (uri.protocol || 'http') + - '://' + - uri.hostname + - (uri.port ? ':' + uri.port : ''); - } - - var pathname = uri.pathname || '/'; - loc += pathname[0] === '/' ? pathname : '/' + pathname; - - if (uri.query && Object.keys(uri.query).length > 0) { - var params = new URLSearchParams(); - Object.keys(uri.query).forEach(function forEach(k) { - var v = uri.query[k]; - if (Array.isArray(v)) { - v.forEach(function forEachItem(item) { - params.append(k, item); - }); - } else { - params.set(k, v); - } - }); - loc += '?' + params.toString(); - } - - return loc; -} - /** * Parses a raw HTTP request URL string into a legacy url.parse-shaped object. * Handles the special OPTIONS '*' request-target. @@ -174,7 +109,5 @@ function parseRequestUrl(rawUrl) { module.exports = { shallowCopy: shallowCopy, mergeQs: mergeQs, - parseUrlQuery: parseUrlQuery, - formatUrl: formatUrl, parseRequestUrl: parseRequestUrl }; diff --git a/test/utils.test.js b/test/utils.test.js index 8275e02e2..9eab6b85a 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -3,8 +3,6 @@ var mergeQs = require('../lib/utils').mergeQs; var parseRequestUrl = require('../lib/utils').parseRequestUrl; -var parseUrlQuery = require('../lib/utils').parseUrlQuery; -var formatUrl = require('../lib/utils').formatUrl; if (require.cache[__dirname + '/lib/helper.js']) { delete require.cache[__dirname + '/lib/helper.js']; @@ -86,83 +84,6 @@ test('parseRequestUrl: OPTIONS * request-target', function(t) { t.done(); }); -// parseUrlQuery - -test('parseUrlQuery: simple params become object', function(t) { - var sp = new URL('/foo?a=1&b=2', 'http://localhost').searchParams; - - t.deepEqual(parseUrlQuery(sp), { a: '1', b: '2' }); - t.done(); -}); - -test('parseUrlQuery: duplicate keys accumulate into array', function(t) { - var sp = new URL('/foo?a=1&a=2&b=3', 'http://localhost').searchParams; - - t.deepEqual(parseUrlQuery(sp), { a: ['1', '2'], b: '3' }); - t.done(); -}); - -test('parseUrlQuery: no query returns empty object', function(t) { - var sp = new URL('/foo', 'http://localhost').searchParams; - - t.deepEqual(parseUrlQuery(sp), {}); - t.done(); -}); - -// formatUrl - -test('formatUrl: pathname only', function(t) { - t.equal(formatUrl({ pathname: '/foo/bar' }), '/foo/bar'); - t.done(); -}); - -test('formatUrl: hostname + pathname', function(t) { - t.equal( - formatUrl({ - protocol: 'http', - hostname: 'example.com', - pathname: '/foo' - }), - 'http://example.com/foo' - ); - t.done(); -}); - -test('formatUrl: hostname + port + pathname', function(t) { - t.equal( - formatUrl({ - protocol: 'http', - hostname: 'example.com', - port: '8080', - pathname: '/foo' - }), - 'http://example.com:8080/foo' - ); - t.done(); -}); - -test('formatUrl: pathname + query object', function(t) { - t.equal( - formatUrl({ pathname: '/foo', query: { a: '1', b: '2' } }), - '/foo?a=1&b=2' - ); - t.done(); -}); - -test('formatUrl: string passthrough', function(t) { - t.equal(formatUrl('/already/a/string'), '/already/a/string'); - t.equal(formatUrl('http://example.com/foo'), 'http://example.com/foo'); - t.done(); -}); - -test('formatUrl: array query values become repeated params', function(t) { - t.equal( - formatUrl({ pathname: '/foo', query: { a: ['1', '2'] } }), - '/foo?a=1&a=2' - ); - t.done(); -}); - // mergeQs test('merge qs', function(t) { From 2fa68410f072a6d87fcd6502501e40f3608a0e43 Mon Sep 17 00:00:00 2001 From: dvinakur Date: Fri, 17 Jul 2026 12:53:59 +0200 Subject: [PATCH 3/5] fix: change finalUri.query generation --- lib/response.js | 21 ++------- test/request.test.js | 65 ++++++++++++++------------ test/response.test.js | 103 ++++++++++++++++++++++++------------------ test/server.test.js | 32 +++++++------ test/utils.test.js | 14 +++--- 5 files changed, 124 insertions(+), 111 deletions(-) diff --git a/lib/response.js b/lib/response.js index 3a22bf4b4..7f857d180 100644 --- a/lib/response.js +++ b/lib/response.js @@ -10,6 +10,7 @@ var mime = require('mime'); var errors = require('restify-errors'); var httpDate = require('./http_date'); +var utils = require('./utils'); ///--- Globals @@ -807,23 +808,11 @@ function patch(Response) { // then add query params if (opt.query) { - var merged = new URLSearchParams(); - - Object.keys(opt.query).forEach(function forEach(k) { - var v = opt.query[k]; - var values = Array.isArray(v) ? v : [v]; - values.forEach(function forEachItem(item) { - merged.append(k, item); - }); - }); - - if (opt.overrideQuery !== true) { - finalUri.searchParams.forEach(function forEach(v, k) { - merged.append(k, v); - }); + if (opt.overrideQuery === true) { + finalUri.query = opt.query; + } else { + finalUri.query = utils.mergeQs(opt.query, finalUri.query); } - - finalUri.search = merged.toString(); } // change status code to 301 permanent if specified diff --git a/test/request.test.js b/test/request.test.js index 3e37618d0..971125a60 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -280,37 +280,42 @@ test( return next(myErr); }); - CLIENT.get('/', function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 500); - clientDone = true; - }); -}); + CLIENT.get('/', function(err, _, res) { + t.ok(err); + t.equal(res.statusCode, 500); + clientDone = true; + }); + } +); -test('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.equal(u.href, '/geturl-plain'); - t.equal(u.pathname, '/geturl-plain'); - t.equal(u.search, null); - t.equal(u.query, null); - t.equal(u.hash, null); - t.equal(u.host, null); - t.equal(u.hostname, null); - t.equal(u.port, null); - t.equal(u.protocol, null); - res.send(); - return next(); - }); +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.equal(u.href, '/geturl-plain'); + t.equal(u.pathname, '/geturl-plain'); + t.equal(u.search, null); + t.equal(u.query, null); + t.equal(u.hash, null); + t.equal(u.host, null); + t.equal(u.hostname, null); + t.equal(u.port, null); + t.equal(u.protocol, null); + res.send(); + return next(); + }); - CLIENT.get('/geturl-plain', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.end(); - }); -}); + CLIENT.get('/geturl-plain', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); + } +); -test('getUrl should return correct query string', function(t) { +test(module, 'getUrl should return correct query string', function(t) { SERVER.get('/geturl-qs', function(req, res, next) { var u = req.getUrl(); t.equal(u.href, '/geturl-qs?a=1&b=2'); @@ -333,7 +338,7 @@ test('getUrl should return correct query string', function(t) { }); }); -test('getUrl should handle OPTIONS * request-target', function(t) { +test(module, 'getUrl should handle OPTIONS * request-target', function(t) { SERVER.opts('*', function(req, res, next) { var u = req.getUrl(); t.equal(u.pathname, '*'); @@ -350,7 +355,7 @@ test('getUrl should handle OPTIONS * request-target', function(t) { }); }); -test('getUrl result is cached across calls', function(t) { +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(); diff --git a/test/response.test.js b/test/response.test.js index fb8956264..be7bdb097 100644 --- a/test/response.test.js +++ b/test/response.test.js @@ -393,7 +393,7 @@ test(module, 'redirect using default hostname with custom port', function(t) { }); }); -test('redirect opts.port exact location header', function(t) { +test(module, 'redirect opts.port exact location header', function(t) { SERVER.get('/port-pin', function(req, res, next) { res.redirect({ port: 3000 }, next); }); @@ -406,23 +406,27 @@ test('redirect opts.port exact location header', function(t) { }); }); -test('redirect external hostname and port exact location header', function(t) { - SERVER.get('/extport-pin', function(req, res, next) { - res.redirect( - { hostname: 'www.foo.com', pathname: '/bar', port: 3000 }, - next - ); - }); +test( + module, + 'redirect external hostname and port exact location header', + function(t) { + SERVER.get('/extport-pin', function(req, res, next) { + res.redirect( + { hostname: 'www.foo.com', pathname: '/bar', port: 3000 }, + next + ); + }); - CLIENT.get(join(LOCALHOST, '/extport-pin'), function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 302); - t.equal(res.headers.location, 'http://www.foo.com:3000/bar'); - t.end(); - }); -}); + CLIENT.get(join(LOCALHOST, '/extport-pin'), function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 302); + t.equal(res.headers.location, 'http://www.foo.com:3000/bar'); + t.end(); + }); + } +); -test('redirect extend query params exact location header', function(t) { +test(module, 'redirect extend query params exact location header', function(t) { SERVER.get('/extqs-pin', function(req, res, next) { res.redirect({ query: { b: '2' } }, next); }); @@ -440,7 +444,7 @@ test('redirect extend query params exact location header', function(t) { }); // eslint-disable-next-line max-len -test('redirect should repeat query param when same key exists in both current and new query', function(t) { +test(module, 'redirect should repeat query param when same key', function(t) { SERVER.get('/dupqs-pin', function(req, res, next) { res.redirect({ query: { a: '2' } }, next); }); @@ -455,37 +459,48 @@ test('redirect should repeat query param when same key exists in both current an }); // eslint-disable-next-line max-len -test('redirect with only pathname (no hostname) uses current host', function(t) { - SERVER.get('/rel-path-pin', function(req, res, next) { - res.redirect({ pathname: '/other' }, next); - }); +test( + module, + 'redirect with only pathname (no hostname) uses current host', + function(t) { + SERVER.get('/rel-path-pin', function(req, res, next) { + res.redirect({ pathname: '/other' }, next); + }); - CLIENT.get(join(LOCALHOST, '/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.end(); - }); -}); + CLIENT.get(join(LOCALHOST, '/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.end(); + }); + } +); // eslint-disable-next-line max-len -test('redirect with pathname and query (no hostname) preserves query string', function(t) { - SERVER.get('/rel-qs-pin', function(req, res, next) { - res.redirect({ pathname: '/other', query: { x: '1', y: '2' } }, next); - }); +test( + module, + 'redirect with pathname and query (no hostname) preserves query string', + function(t) { + SERVER.get('/rel-qs-pin', function(req, res, next) { + res.redirect( + { pathname: '/other', query: { x: '1', y: '2' } }, + next + ); + }); - CLIENT.get(join(LOCALHOST, '/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.end(); - }); -}); + CLIENT.get(join(LOCALHOST, '/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.end(); + }); + } +); // eslint-disable-next-line test( diff --git a/test/server.test.js b/test/server.test.js index a29f0b9f8..69daceb7f 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1183,21 +1183,25 @@ test( ); // eslint-disable-next-line max-len -test(module, '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'); - res.send(); - next(); - }); +test( + module, + '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'); + res.send(); + next(); + }); - CLIENT.get('/base-path', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.end(); - }); -}); + CLIENT.get('/base-path', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); + } +); test(module, 'GH-693 sending multiple response header values', function(t) { SERVER.get('/', function(req, res, next) { diff --git a/test/utils.test.js b/test/utils.test.js index 9eab6b85a..813ef438b 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -15,7 +15,7 @@ var test = helper.test; // parseRequestUrl -test('parseRequestUrl: plain path', function(t) { +test(module, 'parseRequestUrl: plain path', function(t) { var r = parseRequestUrl('/foo/bar'); t.equal(r.pathname, '/foo/bar'); @@ -29,7 +29,7 @@ test('parseRequestUrl: plain path', function(t) { t.done(); }); -test('parseRequestUrl: path with query string', function(t) { +test(module, 'parseRequestUrl: path with query string', function(t) { var r = parseRequestUrl('/foo?a=1&b=2'); t.equal(r.pathname, '/foo'); @@ -40,7 +40,7 @@ test('parseRequestUrl: path with query string', function(t) { t.done(); }); -test('parseRequestUrl: path with hash', function(t) { +test(module, 'parseRequestUrl: path with hash', function(t) { var r = parseRequestUrl('/foo/bar#section'); t.equal(r.pathname, '/foo/bar'); @@ -50,7 +50,7 @@ test('parseRequestUrl: path with hash', function(t) { t.done(); }); -test('parseRequestUrl: absolute URL', function(t) { +test(module, 'parseRequestUrl: absolute URL', function(t) { var r = parseRequestUrl('http://example.com/foo?x=1'); t.equal(r.pathname, '/foo'); @@ -62,7 +62,7 @@ test('parseRequestUrl: absolute URL', function(t) { t.done(); }); -test('parseRequestUrl: absolute URL with port', function(t) { +test(module, 'parseRequestUrl: absolute URL with port', function(t) { var r = parseRequestUrl('http://example.com:8080/foo'); t.equal(r.pathname, '/foo'); @@ -72,7 +72,7 @@ test('parseRequestUrl: absolute URL with port', function(t) { t.done(); }); -test('parseRequestUrl: OPTIONS * request-target', function(t) { +test(module, 'parseRequestUrl: OPTIONS * request-target', function(t) { var r = parseRequestUrl('*'); t.equal(r.pathname, '*'); @@ -86,7 +86,7 @@ test('parseRequestUrl: OPTIONS * request-target', function(t) { // mergeQs -test('merge qs', function(t) { +test(module, 'merge qs', function(t) { var qs1 = mergeQs(undefined, { a: 1 }); t.deepEqual(qs1, { a: 1 }); From 039fbab2df969498c7bd50c6f107c649bda5d23c Mon Sep 17 00:00:00 2001 From: dvinakur Date: Fri, 31 Jul 2026 10:50:18 +0200 Subject: [PATCH 4/5] fix: fix redirect logic --- lib/request.js | 27 ++++++++++--- lib/response.js | 71 ++++++++++++++++++++------------ lib/router.js | 5 ++- lib/utils.js | 53 +----------------------- test/plugins/bodyReader.test.js | 5 ++- test/plugins/static.test.js | 25 +++++++----- test/request.test.js | 40 +++++++++--------- test/server.test.js | 14 ++++++- test/utils.test.js | 72 --------------------------------- 9 files changed, 124 insertions(+), 188 deletions(-) diff --git a/lib/request.js b/lib/request.js index aa8750b6a..30ac90bff 100644 --- a/lib/request.js +++ b/lib/request.js @@ -10,7 +10,6 @@ var Negotiator = require('negotiator'); var uuid = require('uuid'); var dtrace = require('./dtrace'); -var utils = require('./utils'); ///-- Helpers /** @@ -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 = utils.parseRequestUrl(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; diff --git a/lib/response.js b/lib/response.js index 7f857d180..0d8dc3ca2 100644 --- a/lib/response.js +++ b/lib/response.js @@ -10,7 +10,6 @@ var mime = require('mime'); var errors = require('restify-errors'); var httpDate = require('./http_date'); -var utils = require('./utils'); ///--- Globals @@ -768,26 +767,28 @@ function patch(Response) { ? opt.secure : req.isSecure(); - // if hostname is passed in, use that as the base, otherwise - // fall back on current url. mutate a URL instance directly - // instead of bouncing through a plain-object descriptor. - var raw = opt.hostname || currentFullPath; - var isAbsolute = raw.indexOf('://') !== -1; - finalUri = isAbsolute - ? new URL(raw) - : new URL(raw, 'http://localhost'); - - if (opt.hostname) { - // an explicit hostname replaces the current path entirely, - // so start from a clean slate. - finalUri.pathname = '/'; - finalUri.search = ''; - } - - // hostname is only known if it was passed in explicitly, or if - // the current url was itself absolute (e.g. proxied requests). + // 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 || (isAbsolute ? finalUri.hostname : null); + opt.hostname || + (req.url.indexOf('://') !== -1 ? finalUri.hostname : null); // then set protocol IFF hostname is set - otherwise we end up with // malformed URL. @@ -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 diff --git a/lib/router.js b/lib/router.js index 8cadd0001..896e42dcd 100644 --- a/lib/router.js +++ b/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 === '*'; }; /** diff --git a/lib/utils.js b/lib/utils.js index bcdc00aca..77da5389e 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -54,60 +54,9 @@ function mergeQs(obj1, obj2) { return merged; } -/** - * Parses a raw HTTP request URL string into a legacy url.parse-shaped object. - * Handles the special OPTIONS '*' request-target. - * - * @public - * @function parseRequestUrl - * @param {String} rawUrl - the raw request URL string - * @returns {Object} url descriptor with pathname, search, query, href etc. - */ -function parseRequestUrl(rawUrl) { - var pathname, search, hash, port, hostname; - - if (rawUrl === '*') { - pathname = '*'; - search = null; - hash = null; - port = null; - hostname = null; - } else if (rawUrl.indexOf('://') !== -1) { - var absParsed = new URL(rawUrl); - pathname = absParsed.pathname; - search = absParsed.search || null; - hash = absParsed.hash || null; - port = absParsed.port || null; - hostname = absParsed.hostname || null; - } else { - var relParsed = new URL(rawUrl, 'http://localhost'); - pathname = relParsed.pathname; - search = relParsed.search || null; - hash = relParsed.hash || null; - port = null; - hostname = null; - } - - return { - protocol: null, - slashes: null, - auth: null, - host: null, - port: port, - hostname: hostname, - hash: hash, - search: search, - query: search ? search.slice(1) : null, - pathname: pathname, - path: pathname + (search || ''), - href: rawUrl - }; -} - ///--- Exports module.exports = { shallowCopy: shallowCopy, - mergeQs: mergeQs, - parseRequestUrl: parseRequestUrl + mergeQs: mergeQs }; diff --git a/test/plugins/bodyReader.test.js b/test/plugins/bodyReader.test.js index 3e48a4298..cd20fb949 100644 --- a/test/plugins/bodyReader.test.js +++ b/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(); } diff --git a/test/plugins/static.test.js b/test/plugins/static.test.js index c98ec1b2e..b606061bb 100644 --- a/test/plugins/static.test.js +++ b/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(); }); diff --git a/test/request.test.js b/test/request.test.js index 971125a60..31dd3db3f 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -294,15 +294,15 @@ test( function(t) { SERVER.get('/geturl-plain', function(req, res, next) { var u = req.getUrl(); - t.equal(u.href, '/geturl-plain'); + 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, null); - t.equal(u.query, null); - t.equal(u.hash, null); - t.equal(u.host, null); - t.equal(u.hostname, null); - t.equal(u.port, null); - t.equal(u.protocol, null); + 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(); }); @@ -318,15 +318,17 @@ test( test(module, 'getUrl should return correct query string', function(t) { SERVER.get('/geturl-qs', function(req, res, next) { var u = req.getUrl(); - t.equal(u.href, '/geturl-qs?a=1&b=2'); + 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.query, 'a=1&b=2'); - t.equal(u.hash, null); - t.equal(u.host, null); - t.equal(u.hostname, null); - t.equal(u.port, null); - t.equal(u.protocol, null); + 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(); }); @@ -341,9 +343,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(u.pathname, '*'); - t.equal(u.search, null); - t.equal(u.query, null); + 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(); }); diff --git a/test/server.test.js b/test/server.test.js index 69daceb7f..c24856ba4 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -122,7 +122,7 @@ test(module, 'listen and close (socketPath)', function(t) { // Run IPv6 tests only if IPv6 network is available if (!SKIP_IP_V6) { - test('gh-751 IPv4/IPv6 server URL', function(t) { + test(module, 'gh-751 IPv4/IPv6 server URL', function(t) { t.equal(SERVER.url, 'http://127.0.0.1:' + PORT, 'ipv4 url'); var server = restify.createServer(); @@ -1359,6 +1359,14 @@ test( // Dirty hack to capture the log record using a ring buffer. var numCount = 0; + // Guard against the 'after' event never firing for the aborted + // request (e.g. a timing race between the handler chain and the + // client timeout), which would otherwise hang this test forever. + var safetyTimer = setTimeout(function() { + t.ok(false, 'timed out waiting for audit "after" event on v=2'); + t.end(); + }, 5000); + // FAST_CLIENT times out at 500ms, should capture two records then close // the request. SERVER.get('/audit', [ @@ -1396,7 +1404,9 @@ test( ); SERVER.on('after', function(req, res, route, err) { - if (req.href() === '/audit?v=2') { + if (req.getPath() === '/audit' && req.getQuery() === 'v=2') { + clearTimeout(safetyTimer); + // should request timeout error t.ok(err); t.equal(err.name, 'RequestCloseError'); diff --git a/test/utils.test.js b/test/utils.test.js index 813ef438b..f4b56d32f 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -2,7 +2,6 @@ /* eslint-disable func-names */ var mergeQs = require('../lib/utils').mergeQs; -var parseRequestUrl = require('../lib/utils').parseRequestUrl; if (require.cache[__dirname + '/lib/helper.js']) { delete require.cache[__dirname + '/lib/helper.js']; @@ -13,77 +12,6 @@ var helper = require('./lib/helper.js'); var test = helper.test; -// parseRequestUrl - -test(module, 'parseRequestUrl: plain path', function(t) { - var r = parseRequestUrl('/foo/bar'); - - t.equal(r.pathname, '/foo/bar'); - t.equal(r.search, null); - t.equal(r.query, null); - t.equal(r.hash, null); - t.equal(r.path, '/foo/bar'); - t.equal(r.href, '/foo/bar'); - t.equal(r.hostname, null); - t.equal(r.port, null); - t.done(); -}); - -test(module, 'parseRequestUrl: path with query string', function(t) { - var r = parseRequestUrl('/foo?a=1&b=2'); - - t.equal(r.pathname, '/foo'); - t.equal(r.search, '?a=1&b=2'); - t.equal(r.query, 'a=1&b=2'); - t.equal(r.path, '/foo?a=1&b=2'); - t.equal(r.href, '/foo?a=1&b=2'); - t.done(); -}); - -test(module, 'parseRequestUrl: path with hash', function(t) { - var r = parseRequestUrl('/foo/bar#section'); - - t.equal(r.pathname, '/foo/bar'); - t.equal(r.hash, '#section'); - t.equal(r.search, null); - t.equal(r.href, '/foo/bar#section'); - t.done(); -}); - -test(module, 'parseRequestUrl: absolute URL', function(t) { - var r = parseRequestUrl('http://example.com/foo?x=1'); - - t.equal(r.pathname, '/foo'); - t.equal(r.search, '?x=1'); - t.equal(r.query, 'x=1'); - t.equal(r.hostname, 'example.com'); - t.equal(r.port, null); - t.equal(r.path, '/foo?x=1'); - t.done(); -}); - -test(module, 'parseRequestUrl: absolute URL with port', function(t) { - var r = parseRequestUrl('http://example.com:8080/foo'); - - t.equal(r.pathname, '/foo'); - t.equal(r.hostname, 'example.com'); - t.equal(r.port, '8080'); - t.equal(r.search, null); - t.done(); -}); - -test(module, 'parseRequestUrl: OPTIONS * request-target', function(t) { - var r = parseRequestUrl('*'); - - t.equal(r.pathname, '*'); - t.equal(r.search, null); - t.equal(r.query, null); - t.equal(r.hash, null); - t.equal(r.path, '*'); - t.equal(r.href, '*'); - t.done(); -}); - // mergeQs test(module, 'merge qs', function(t) { From 2b6e1a05d9d17d25f01ae0d9dd35c9039ea08a19 Mon Sep 17 00:00:00 2001 From: Dziyana Vinakur <55085195+dianager@users.noreply.github.com> Date: Fri, 17 Jul 2026 11:30:57 +0200 Subject: [PATCH 5/5] feat: update dependencies (#2002) * feat: remove production vulnerabilities * feat: replace uuid for crypto --------- Co-authored-by: dvinakur --- lib/request.js | 4 ++-- lib/router.js | 4 ++-- package.json | 5 ++--- test/plugins/staticFiles.test.js | 8 ++++---- test/server.test.js | 10 +++++----- 5 files changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/request.js b/lib/request.js index 30ac90bff..a14cf7834 100644 --- a/lib/request.js +++ b/lib/request.js @@ -3,11 +3,11 @@ 'use strict'; var sprintf = require('util').format; +var crypto = require('crypto'); var assert = require('assert-plus'); var mime = require('mime'); var Negotiator = require('negotiator'); -var uuid = require('uuid'); var dtrace = require('./dtrace'); @@ -289,7 +289,7 @@ function patch(Request) { return this._id; } - this._id = uuid.v4(); + this._id = crypto.randomUUID(); return this._id; }; diff --git a/lib/router.js b/lib/router.js index 896e42dcd..e21e42f6d 100644 --- a/lib/router.js +++ b/lib/router.js @@ -3,11 +3,11 @@ var EventEmitter = require('events').EventEmitter; var util = require('util'); var http = require('http'); +var crypto = require('crypto'); var _ = require('lodash'); var assert = require('assert-plus'); var errors = require('restify-errors'); -var uuid = require('uuid'); var Chain = require('./chain'); var RouterRegistryRadix = require('./routerRegistryRadix'); @@ -366,7 +366,7 @@ Router.prototype._getRouteName = function _getRouteName(name, method, path) { // Avoid name conflict: GH-401 if (this._registry.get()[name]) { - name += uuid.v4().substr(0, 7); + name += crypto.randomUUID().substr(0, 7); } return name; diff --git a/package.json b/package.json index 26e4bf9e6..ebbc092f2 100644 --- a/package.json +++ b/package.json @@ -97,7 +97,7 @@ "csv": "^6.2.2", "escape-regexp-component": "^1.0.2", "ewma": "^2.0.1", - "find-my-way": "^7.6.0", + "find-my-way": "^9.6.0", "formidable": "^1.2.1", "http-signature": "^1.3.6", "lodash": "^4.17.11", @@ -110,8 +110,7 @@ "qs": "^6.7.0", "restify-errors": "^8.0.2", "semver": "^7.3.8", - "send": "^0.18.0", - "uuid": "^9.0.0", + "send": "^1.2.1", "vasync": "^2.2.0" }, "optionalDependencies": { diff --git a/test/plugins/staticFiles.test.js b/test/plugins/staticFiles.test.js index 3b44dcd9c..d26eabf23 100644 --- a/test/plugins/staticFiles.test.js +++ b/test/plugins/staticFiles.test.js @@ -65,8 +65,8 @@ describe('staticFiles plugin - no options', function() { // Verify headers assert.equal(res.headers['cache-control'], 'public, max-age=0'); assert.equal( - res.headers['content-type'], - contentType //'text/html; charset=UTF-8' + res.headers['content-type'].toLowerCase(), + contentType.toLowerCase() //'text/html; charset=UTF-8' ); assert.exists(res.headers.etag); assert.equal( @@ -164,8 +164,8 @@ describe('staticFiles plugin - with options', function() { // Verify headers assert.equal(res.headers['cache-control'], 'public, max-age=3600'); assert.equal( - res.headers['content-type'], - contentType //'text/html; charset=UTF-8' + res.headers['content-type'].toLowerCase(), + contentType.toLowerCase() //'text/html; charset=UTF-8' ); assert.notExists(res.headers.etag); assert.equal( diff --git a/test/server.test.js b/test/server.test.js index c24856ba4..cbda2aac1 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -7,11 +7,11 @@ const { AsyncLocalStorage } = require('async_hooks'); var assert = require('assert-plus'); var childprocess = require('child_process'); var http = require('http'); +var crypto = require('crypto'); var pino = require('pino'); var errors = require('restify-errors'); var restifyClients = require('restify-clients'); -var uuid = require('uuid'); var RestError = errors.RestError; var restify = require('../lib'); @@ -113,7 +113,7 @@ test(module, 'listen and close (port only) w/ port number as string', function( test(module, 'listen and close (socketPath)', function(t) { var server = restify.createServer(); - server.listen('/tmp/.' + uuid.v4(), function() { + server.listen('/tmp/.' + crypto.randomUUID(), function() { server.close(function() { t.end(); }); @@ -812,7 +812,7 @@ test(module, 'gh-278 missing router error events (404)', function(t) { res.send(404, 'foo'); }); - CLIENT.get('/' + uuid.v4(), function(err, _, res) { + CLIENT.get('/' + crypto.randomUUID(), function(err, _, res) { t.ok(err); t.equal(err.message, '"foo"'); t.equal(res.statusCode, 404); @@ -821,7 +821,7 @@ test(module, 'gh-278 missing router error events (404)', function(t) { }); test(module, 'gh-278 missing router error events (405)', function(t) { - var p = '/' + uuid.v4(); + var p = '/' + crypto.randomUUID(); SERVER.post(p, function(req, res, next) { res.send(201); next(); @@ -1134,7 +1134,7 @@ test(module, 'error handler defers "after" event', async function(t) { // do not fire prematurely t.notOk(true); }); - CLIENT.get('/' + uuid.v4(), function(err, _, res) { + CLIENT.get('/' + crypto.randomUUID(), function(err, _, res) { t.ok(err); t.equal(res.statusCode, 404); clientResolve();