Skip to content

fix: prevent use with multi-slash path from matching all routes#190

Open
MOHIT51196 wants to merge 1 commit intopillarjs:masterfrom
MOHIT51196:fix/issue-express-4557
Open

fix: prevent use with multi-slash path from matching all routes#190
MOHIT51196 wants to merge 1 commit intopillarjs:masterfrom
MOHIT51196:fix/issue-express-4557

Conversation

@MOHIT51196
Copy link

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

const router = Router();
const inner = Router();

inner.get('/', listTestRoutes);
inner.get('/drop/table/:table', dropTable);

router.use('//', inner);
router.listen();

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.

const loose = str.replace(TRAILING_SLASH_REGEXP, '')
return loose || str

This ensures:

  • /foo/ still loosens to /foo (normal behavior, unchanged)
  • // is preserved as // (no longer collapses to empty string)
  • /// is preserved as ///

Changes

  • lib/layer.js — Guard loosen() against reducing slash-only paths to empty string
  • test/router.js — 3 new test cases:
    • use('//') must not match / or /foo
    • use('///') must not match / or /foo
    • Sub-router mounted at // must not expose its routes via single /

All 697 tests pass (694 existing + 3 new). Linter clean.
Fixes expressjs/express#4557

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>
@MOHIT51196
Copy link
Author

@wesleytodd can you review this PR with a fix for an open issue expressjs/express#4557

@mohitmalhotra-celigo
Copy link

@UlisesGascon can you review this PR with a fix for an open issue expressjs/express#4557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App.use(path, ...) Does Not Mount at '//'

2 participants