diff --git a/HISTORY.md b/HISTORY.md index 287405a..454b29f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,8 @@ +unreleased +========== + + * Match routes iteratively to prevent stack overflows + 1.0.0-beta.1 / 2014-11-16 ========================= diff --git a/index.js b/index.js index a804489..3f3ae03 100644 --- a/index.js +++ b/index.js @@ -211,78 +211,103 @@ Router.prototype.handle = function handle(req, res, callback) { ? null : err - var layer = stack[idx++] - + // remove added slash if (slashAdded) { req.url = req.url.substr(1) slashAdded = false } + // restore altered req.url if (removed.length !== 0) { req.baseUrl = parentUrl req.url = protohost + removed + req.url.substr(protohost.length) removed = '' } - if (!layer) { + // no more matching layers + if (idx >= stack.length) { defer(done, layerError) return } + // get pathname of request + var path = getPathname(req) + + if (path == null) { + return done(layerError) + } + + // find next matching layer + var layer + var match + var route - self.match_layer(layer, req, res, function (err, path) { - if (err || path === undefined) { - next(layerError || err) - return + while (match !== true && idx < stack.length) { + layer = stack[idx++] + match = matchLayer(layer, path) + route = layer.route + + if (typeof match !== 'boolean') { + // hold on to layerError + layerError = layerError || match + } + + if (match !== true) { + continue } - // route object and not middleware - var route = layer.route + if (!route) { + // process non-route handlers normally + continue + } + + if (layerError) { + // routes do not match with a pending error + match = false + continue + } + + var method = req.method; + var has_method = route._handles_method(method) + + // build up automatic options response + if (!has_method && method === 'OPTIONS') { + options.push.apply(options, route._options()) + } + + // don't even bother matching route + if (!has_method && method !== 'HEAD') { + match = false + continue + } + } + + // no match + if (match !== true) { + return done(layerError) + } + + // store route for dispatch on change + if (route) { + req.route = route + } + + // Capture one-time layer values + req.params = self.mergeParams + ? mergeParams(layer.params, parentParams) + : layer.params + var layerPath = layer.path + + // this should be done for the layer + self.process_params(layer, paramcalled, req, res, function (err) { + if (err) { + return next(layerError || err) + } - // if final route, then we support options if (route) { - // we don't run any routes with error first - if (layerError) { - next(layerError) - return - } - - var method = req.method - var has_method = route._handles_method(method) - - // build up automatic options response - if (!has_method && method === 'OPTIONS') { - options.push.apply(options, route._options()) - } - - // don't even bother - if (!has_method && method !== 'HEAD') { - next() - return - } - - // we can now dispatch to the route - req.route = route + return layer.handle_request(req, res, next) } - // Capture one-time layer values - req.params = self.mergeParams - ? mergeParams(layer.params, parentParams) - : layer.params - var layerPath = layer.path - - // this should be done for the layer - self.process_params(layer, paramcalled, req, res, function (err) { - if (err) { - next(layerError || err) - return - } - - if (route) { - return layer.handle_request(req, res, next) - } - - trim_prefix(layer, layerError, layerPath, path) - }) + trim_prefix(layer, layerError, layerPath, path) }) } @@ -323,29 +348,6 @@ Router.prototype.handle = function handle(req, res, callback) { } } -/** - * Match request to a layer. - * - * @private - */ - -Router.prototype.match_layer = function match_layer(layer, req, res, done) { - var error = null - var path - - try { - path = parseUrl(req).pathname - - if (!layer.match(path)) { - path = undefined - } - } catch (err) { - error = err - } - - done(error, path) -} - /** * Process any parameters for the layer. * @@ -546,6 +548,37 @@ methods.concat('all').forEach(function(method){ } }) +/** + * Get pathname of request. + * + * @param {IncomingMessage} req + * @private + */ + +function getPathname(req) { + try { + return parseUrl(req).pathname; + } catch (err) { + return undefined; + } +} + +/** + * Match path to a layer. + * + * @param {Layer} layer + * @param {string} path + * @private + */ + +function matchLayer(layer, path) { + try { + return layer.match(path); + } catch (err) { + return err; + } +} + /** * Merge params with parent params * diff --git a/lib/route.js b/lib/route.js index edce0dc..22b6e45 100644 --- a/lib/route.js +++ b/lib/route.js @@ -93,13 +93,23 @@ Route.prototype.dispatch = function dispatch(req, res, done) { return done() } - var layer = stack[idx++] - if (!layer) { + // no more matching layers + if (idx >= stack.length) { return done(err) } - if (layer.method && layer.method !== method) { - return next(err) + var layer + var match + + // find next matching layer + while (match !== true && idx < stack.length) { + layer = stack[idx++] + match = !layer.method || layer.method === method + } + + // no match + if (match !== true) { + return done(err) } if (err) { diff --git a/test/router.js b/test/router.js index 79af36c..4255139 100644 --- a/test/router.js +++ b/test/router.js @@ -50,6 +50,21 @@ describe('Router', function () { }) }) + it('should not stack overflow with many registered routes', function (done) { + var router = new Router() + var server = createServer(router) + + for (var i = 0; i < 6000; i++) { + router.get('/thing' + i, helloWorld) + } + + router.get('/', helloWorld) + + request(server) + .get('/') + .expect(200, 'hello, world', done) + }) + describe('with "caseSensitive" option', function () { it('should not match paths case-sensitively by default', function (done) { var cb = after(3, done)