Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,17 @@ Router.prototype.use = function use (handler) {
throw new TypeError('argument handler must be a function')
}

// Detect router cycles
if (fn && typeof fn.handle === 'function' && fn.stack) {
if (detectRouterCycle(this, fn)) {
process.emitWarning(
'Detected router mounted on itself or in a cycle. ' +
'This will cause requests to hang indefinitely.',
'RouterCycleWarning'
)
}
}

// add the middleware
debug('use %o %s', path, fn.name || '<anonymous>')

Expand All @@ -409,6 +420,51 @@ Router.prototype.use = function use (handler) {
return this
}

/**
* Detect if mounting a router would create a cycle.
*
* @param {Router} parentRouter - The router being mounted onto
* @param {Router} childRouter - The router being mounted
* @param {Set} visited - Set of visited routers to avoid infinite recursion
* @return {boolean}
* @private
*/

function detectRouterCycle (parentRouter, childRouter, visited) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You run this function each time, so why not adding a Set/Map to avoid repetitive code?

// Direct self-mount
if (parentRouter === childRouter) {
return true
}

// Initialize visited set on first call
if (!visited) {
visited = new Set()
}

// Avoid infinite recursion - if we've already checked this router, skip it
if (visited.has(childRouter)) {
return false
}

visited.add(childRouter)

// Check if childRouter already contains parentRouter in its stack
if (childRouter.stack) {
for (let i = 0; i < childRouter.stack.length; i++) {
const layer = childRouter.stack[i]
if (layer.handle === parentRouter) {
return true
}
// Check nested routers recursively
if (layer.handle && layer.handle.stack && detectRouterCycle(parentRouter, layer.handle, visited)) {
return true
}
}
}

return false
}

/**
* Create a new Route for the given path.
*
Expand Down
103 changes: 103 additions & 0 deletions test/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -1487,3 +1487,106 @@ function sawBase (req, res) {
res.setHeader('Content-Type', 'text/plain')
res.end(msg)
}

describe('Router cycle detection', function () {
it('should emit warning when router is mounted on itself', function (done) {
const router = new Router()
const warnings = []
const originalEmit = process.emitWarning

process.emitWarning = function (message, type) {
warnings.push({ message, type })
}

router.use('/', router)

process.emitWarning = originalEmit

assert.strictEqual(warnings.length, 1)
assert.strictEqual(warnings[0].type, 'RouterCycleWarning')
assert.ok(warnings[0].message.includes('cycle'))
done()
})

it('should emit warning for indirect router cycle', function (done) {
const routerA = new Router()
const routerB = new Router()
const warnings = []
const originalEmit = process.emitWarning

process.emitWarning = function (message, type) {
warnings.push({ message, type })
}

routerA.use('/b', routerB)
routerB.use('/a', routerA)

process.emitWarning = originalEmit

assert.strictEqual(warnings.length, 1)
assert.strictEqual(warnings[0].type, 'RouterCycleWarning')
assert.ok(warnings[0].message.includes('cycle'))
done()
})

it('should emit warning for nested indirect cycles', function (done) {
const routerA = new Router()
const routerB = new Router()
const routerC = new Router()
const warnings = []
const originalEmit = process.emitWarning

process.emitWarning = function (message, type) {
warnings.push({ message, type })
}

routerA.use('/b', routerB)
routerB.use('/c', routerC)
routerC.use('/a', routerA)

process.emitWarning = originalEmit

assert.strictEqual(warnings.length, 1)
assert.strictEqual(warnings[0].type, 'RouterCycleWarning')
done()
})

it('should not emit warning for non-cyclic router mounts', function (done) {
const routerA = new Router()
const routerB = new Router()
const routerC = new Router()
const warnings = []
const originalEmit = process.emitWarning

process.emitWarning = function (message, type) {
warnings.push({ message, type })
}

routerA.use('/b', routerB)
routerA.use('/c', routerC)

process.emitWarning = originalEmit

assert.strictEqual(warnings.length, 0)
done()
})

it('should allow cyclic mount but warn (behavior unchanged)', function (done) {
const router = new Router()
const warnings = []
const originalEmit = process.emitWarning

process.emitWarning = function (message, type) {
warnings.push({ message, type })
}

router.get('/test', helloWorld)
router.use('/', router)

process.emitWarning = originalEmit

assert.strictEqual(warnings.length, 1)
assert.strictEqual(router.stack.length, 2) // route + cyclic mount
done()
})
})