fix: normalize all-slash mount paths in app.use to '/'#7286
Open
youcefzemmar wants to merge 1 commit into
Open
fix: normalize all-slash mount paths in app.use to '/'#7286youcefzemmar wants to merge 1 commit into
youcefzemmar wants to merge 1 commit into
Conversation
Mounting with `app.use('//', subApp)` previously slipped past the
router's `/`-fast-path because `loosen('//')` strips trailing slashes
to `''`, leaving an empty pattern that `path-to-regexp` happily matches
against every request. The router then treats the layer as "any path"
without setting the `slash` fast-path flag — a behavior that diverges
from `app.use('/', subApp)` and surprised the reporter in expressjs#4557.
Collapse any all-slash string path (`'/'`, `'//'`, `'///'`, …) to `'/'`
in `app.use` before handing it to the router. Inner repeated slashes
(`'/foo//bar'`) are intentionally left alone — that is a broader
behavior change outside the scope of this issue.
Fixes expressjs#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.
For
'//',loosenstrips both slashes to''. The layer'sslashflag isfalse(becausepath !== '/'), so matching falls through topathRegexp.match('', { end: false, trailing: true }). That matcher matches every request — the layer behaves like a wildcard prefix, just without the fast-path metadata. Mounting at'///'and longer reduces to the same case.In the issue, the reporter mounted dev/test routes under
'//'expecting them to be unreachable from/, but the layer matched anyway and exposed those routes.Fix
One guard, right after
app.useresolves the path arg:The fix stays scoped to all-slash strings. Inner repeated slashes like
'/foo//bar'are deliberately left alone — collapsing them is a broader behavior change, the same one that took PR #7054 out of scope.Testing
npm test— 1250 passing, including the newshould treat all-slash paths as a root mountcase intest/app.use.jsthat exercises'//'and'///'end-to-end.npm run lint— clean.The new test locks in three things:
/and/secretreach the sub-app, an unrelated path like/missingstill 404s (i.e. the layer is not a global wildcard), and'///'normalizes the same way as'//'.Notes
Root cause technically lives in
router'sloosen()(treating'//'as''). Fixing it in express is a surface workaround — ifrouterever normalizes this itself, the guard here is harmless but redundant.Fixes #4557.