fix: prevent use with multi-slash path from matching all routes#190
Open
MOHIT51196 wants to merge 1 commit intopillarjs:masterfrom
Open
fix: prevent use with multi-slash path from matching all routes#190MOHIT51196 wants to merge 1 commit intopillarjs:masterfrom
use with multi-slash path from matching all routes#190MOHIT51196 wants to merge 1 commit intopillarjs:masterfrom
Conversation
Paths consisting entirely of slashes (e.g. `//`) were reduced to an
empty string by `loosen()`, causing `router.use('//', handler)` to
incorrectly match every request. Preserve the original path when
trailing slash removal would produce an empty string.
Fixes expressjs/express#4557
Signed-off-by: Mohit Malhotra <dev.mohitmalhotra@gmail.com>
Author
|
@wesleytodd can you review this PR with a fix for an open issue expressjs/express#4557 |
|
@UlisesGascon can you review this PR with a fix for an open issue expressjs/express#4557 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
router.use('//', handler)incorrectly matches every incoming request, including/and/foo.A sub-router mounted at
//should only respond to requests whose path actually begins with//.Reported upstream in expressjs/express#4557. Also related to expressjs/express#3798.
Reproduction
Expected: GET / does not match the inner router.
Actual: GET / matches inner.get('/') and serves the test route list.
Root cause
The loosen() helper in lib/layer.js strips trailing slashes from mount paths using the regex //+$/ so that non-strict matching works (e.g. /foo/ loosens to /foo).
For a path consisting entirely of slashes like //:
String('//').replace(/\/+$/, '') → '' (empty string)An empty string passed to path-to-regexp.match('', { end: false }) creates a pattern that matches every path, because every path starts with an empty prefix. This is the bug.
Fix
A one-line guard in loosen(): if stripping trailing slashes produces an empty string, preserve the original path instead.
This ensures:
Changes