From 235b87b02b4be42d0cfc2703b18066bcc24bc316 Mon Sep 17 00:00:00 2001 From: scottinet Date: Mon, 2 Dec 2019 17:44:35 +0100 Subject: [PATCH 1/3] Fix URL parsing w/ trailing slash & querystring --- lib/core/httpRouter/index.js | 12 +++---- lib/core/httpRouter/routePart.js | 24 ++++++++++--- test/core/httpRouter/httpRouter.test.js | 47 +++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/lib/core/httpRouter/index.js b/lib/core/httpRouter/index.js index c382b26380..ff0629d7d7 100644 --- a/lib/core/httpRouter/index.js +++ b/lib/core/httpRouter/index.js @@ -28,9 +28,7 @@ const RoutePart = require('./routePart'), { Request, errors: { KuzzleError } } = require('kuzzle-common-objects'); -const - LeadingSlashRegex = /\/+$/, - CharsetRegex = /charset=([\w-]+)/i; +const CharsetRegex = /charset=([\w-]+)/i; /** * Attach handler to routes and dispatch a HTTP @@ -148,8 +146,6 @@ class Router { return this.routeUnhandledHttpMethod(message, cb); } - message.url = message.url.replace(LeadingSlashRegex, ''); - let routeHandler; try { @@ -245,10 +241,10 @@ class Router { * @param {RoutePart} target */ function attach(url, handler, target) { - const cleanedUrl = url.replace(LeadingSlashRegex, ''); + const sanitized = url[url.length - 1] === '/' ? url.slice(0, -1) : url; - if (!attachParts(cleanedUrl.split('/'), handler, target)) { - errorsManager.throw('duplicate_url', cleanedUrl); + if (!attachParts(sanitized.split('/'), handler, target)) { + errorsManager.throw('duplicate_url', sanitized); } } diff --git a/lib/core/httpRouter/routePart.js b/lib/core/httpRouter/routePart.js index 4a55a24540..1f6b26da37 100644 --- a/lib/core/httpRouter/routePart.js +++ b/lib/core/httpRouter/routePart.js @@ -24,7 +24,6 @@ const querystring = require('querystring'), RouteHandler = require('./routeHandler'), - URL = require('url'), { has } = require('../../util/safeObject'); /** @@ -73,10 +72,25 @@ class RoutePart { * @return {RouteHandler} registered function handler */ getHandler(message) { - const - parsed = URL.parse(message.url, true), - pathname = parsed.pathname || '', // pathname is set to null if empty - routeHandler = new RouteHandler(pathname, parsed.query, message); + // we need a fake URL base because the WHATWG API currently doesn't handle + // relative URLs, and the http.IncomingMessage class doesn't provide an + // easy way (and an inexpensive one) of getting an absolute URL + // See: https://github.com/nodejs/node/issues/12682 + const parsed = new URL( + message.url, + message.url[0] === '/' ? 'http://fake' : null); + + // limit calls to "parsed.pathname", it's a getter joining an array + let pathname = parsed.pathname; + + if (pathname[pathname.length - 1] === '/') { + pathname = pathname.slice(0, -1); + } + + const qs = {}; + parsed.searchParams.forEach((v, k) => (qs[k] = v)); + + const routeHandler = new RouteHandler(pathname, qs, message); return getHandlerPart(this, pathname.split('/'), routeHandler); } diff --git a/test/core/httpRouter/httpRouter.test.js b/test/core/httpRouter/httpRouter.test.js index 181936d155..85e473771f 100644 --- a/test/core/httpRouter/httpRouter.test.js +++ b/test/core/httpRouter/httpRouter.test.js @@ -65,8 +65,11 @@ describe('core/httpRouter', () => { it('should raise an internal error when trying to add a duplicate', () => { router.post('/foo/bar', handler); + should(function () { router.post('/foo/bar', handler); }) .throw(InternalError, { id: 'network.http.duplicate_url' }); + should(function () { router.post('/foo/bar/', handler); }) + .throw(InternalError, { id: 'network.http.duplicate_url' }); }); }); @@ -166,6 +169,50 @@ describe('core/httpRouter', () => { }); }); + it('should properly handle querystrings (w/o url trailing slash)', done => { + router.post('/foo/bar', handler); + + rq.url = '/foo/bar?foo=bar'; + rq.method = 'POST'; + + router.route(rq, () => { + try { + should(handler).be.calledOnce(); + + const payload = handler.firstCall.args[0]; + should(payload).be.instanceOf(Request); + should(payload.input.args.foo).eql('bar'); + + done(); + } + catch (e) { + done(e); + } + }); + }); + + it('should properly handle querystrings (w/ url trailing slash)', done => { + router.post('/foo/bar', handler); + + rq.url = '/foo/bar/?foo=bar'; + rq.method = 'POST'; + + router.route(rq, () => { + try { + should(handler).be.calledOnce(); + + const payload = handler.firstCall.args[0]; + should(payload).be.instanceOf(Request); + should(payload.input.args.foo).eql('bar'); + + done(); + } + catch (e) { + done(e); + } + }); + }); + it('should amend the request object if a body is found in the content', done => { router.post('/foo/bar', handler); From 1f7485d8450c80652f81f37c30841d141b269ed3 Mon Sep 17 00:00:00 2001 From: scottinet Date: Mon, 2 Dec 2019 17:49:18 +0100 Subject: [PATCH 2/3] stfu sonar, you're outdated --- lib/core/httpRouter/routePart.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/core/httpRouter/routePart.js b/lib/core/httpRouter/routePart.js index 1f6b26da37..ca89106958 100644 --- a/lib/core/httpRouter/routePart.js +++ b/lib/core/httpRouter/routePart.js @@ -88,7 +88,7 @@ class RoutePart { } const qs = {}; - parsed.searchParams.forEach((v, k) => (qs[k] = v)); + parsed.searchParams.forEach((v, k) => (qs[k] = v)); // NOSONAR (false positive -_-) const routeHandler = new RouteHandler(pathname, qs, message); From c4af16ab4c36563ad2097ab3e7a84d5115f28a7c Mon Sep 17 00:00:00 2001 From: scottinet Date: Tue, 3 Dec 2019 17:00:45 +0100 Subject: [PATCH 3/3] [fix] revert to the legacy URL class --- lib/core/httpRouter/routePart.js | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/core/httpRouter/routePart.js b/lib/core/httpRouter/routePart.js index ca89106958..0717fbd33c 100644 --- a/lib/core/httpRouter/routePart.js +++ b/lib/core/httpRouter/routePart.js @@ -22,6 +22,7 @@ 'use strict'; const + URL = require('url'), querystring = require('querystring'), RouteHandler = require('./routeHandler'), { has } = require('../../util/safeObject'); @@ -72,25 +73,18 @@ class RoutePart { * @return {RouteHandler} registered function handler */ getHandler(message) { - // we need a fake URL base because the WHATWG API currently doesn't handle - // relative URLs, and the http.IncomingMessage class doesn't provide an - // easy way (and an inexpensive one) of getting an absolute URL - // See: https://github.com/nodejs/node/issues/12682 - const parsed = new URL( - message.url, - message.url[0] === '/' ? 'http://fake' : null); - - // limit calls to "parsed.pathname", it's a getter joining an array - let pathname = parsed.pathname; + // Do not use WHATWG API yet, stick with the legacy (and deprecated) URL + // There are two issues: + // - Heavy performance impact: https://github.com/nodejs/node/issues/30334 + // - Double slash bug: https://github.com/nodejs/node/issues/30776 + const parsed = URL.parse(message.url, true); + let pathname = parsed.pathname || ''; // pathname is set to null if empty if (pathname[pathname.length - 1] === '/') { pathname = pathname.slice(0, -1); } - const qs = {}; - parsed.searchParams.forEach((v, k) => (qs[k] = v)); // NOSONAR (false positive -_-) - - const routeHandler = new RouteHandler(pathname, qs, message); + const routeHandler = new RouteHandler(pathname, parsed.query, message); return getHandlerPart(this, pathname.split('/'), routeHandler); }