Skip to content
Permalink
Browse files

Fix URL parsing w/ trailing slash & querystring (#1539)

# Description

Fix a bug where an URL containing both a trailing slash and a querystring would be incorrectly parsed.

# How to reproduce

The following HTTP GET request works fine:

`http://localhost:7512/_now?pretty`

But not this one without this PR:

`http://localhost:7512/_now/?pretty`

Both requests are equivalent and should be regarded as identical.

# Other changes

~Use the WHATWG URL parsing API instead of the legacy (and deprecated) Node.js URL parser.
Unfortunately, the latter was much more adapted to URL parsing server-side than the former. 😑~

Reverted to the legacy URL API because of:
* nodejs/node#30334 (huge performance hit with WHATWG)
* nodejs/node#30776 (bug with relative URLs starting with double-slashes)
  • Loading branch information
scottinet committed Dec 6, 2019
1 parent ab01bc3 commit 6e887841ab43ae9b91c440b1894ab846a3df80d2
Showing with 64 additions and 13 deletions.
  1. +4 −8 lib/core/httpRouter/index.js
  2. +13 −5 lib/core/httpRouter/routePart.js
  3. +47 −0 test/core/httpRouter/httpRouter.test.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);
}
}

@@ -22,9 +22,9 @@
'use strict';

const
URL = require('url'),
querystring = require('querystring'),
RouteHandler = require('./routeHandler'),
URL = require('url'),
{ has } = require('../../util/safeObject');

/**
@@ -73,10 +73,18 @@ 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);
// 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 routeHandler = new RouteHandler(pathname, parsed.query, message);

return getHandlerPart(this, pathname.split('/'), routeHandler);
}
@@ -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);

0 comments on commit 6e88784

Please sign in to comment.
You can’t perform that action at this time.