Escaped literal parenthesis miscounts RegExp capture groups during parameter extraction

[sl4x0]

Summary

Layer.prototype in lib/layer.js builds the list of route parameter keys for RegExp routes by scanning the regex source with:

const MATCHING_GROUP_REGEXP = /\((?:\?<(.*?)>)?(?!\?)/g

This scan does not skip escaped literal parentheses (\( / \)). Every ( in the source (other than (?: non-capturing groups) is counted as a capture group, even when the backslash makes it a literal character. The generated keys list therefore becomes misaligned with the real capture groups returned by RegExp#exec, so req.params is populated with the wrong indices.

Repro

const express = require('express')
const app = express()

// escaped literal paren group ahead of two named groups
app.get(
  /\/tenant\(v\d+\)-(?<tenant>[a-z]+)-(?<user>[a-z]+)$/,
  (req, res) => res.json(req.params),
)

app.listen(3000)

GET /tenant(v1)-acme-boss
Actual   : { "tenant": "boss" }        // user missing, tenant got user's value
Expected : { "tenant": "acme", "user": "boss" }

The route still matches and no error is thrown; only the named parameter identity is corrupted.

Why

Real exec() capture groups: (?<tenant>...), (?<user>...) -> match.length === 3.
Scanner-produced keys: \( (counted), tenant, user -> 3 keys, but the key for capture index 1 is {name: 0} and index 2 is tenant, leaving user unmapped. params.tenant receives match[2], params.user is never set.

Suggested fix direction

Only scan parens that are not backslash-escaped, e.g.:

const MATCHING_GROUP_REGEXP = /(?<!\\)\((?:\?<(.*?)>)?(?!\?)/g

Scope note

Deterministic and developer-observable; the pattern is authored by the developer, not attacker-controlled, so this is a correctness issue rather than a security boundary (matches the maintainer classification of the related advisory GHSA-h9x4-4pf4-ffx8). Happy to adjust the fix or add a regression test.

[krzysdz]

Can confirm that this is a bug. The description contains a small mistake in the example. Matched params are not lost, but have wrong keys:

GET /tenant(v1)-acme-boss
Actual   : { '0': 'acme', tenant: 'boss' }       // user missing, tenant got user's value, tenant is stored as 0
Expected : { "tenant": "acme", "user": "boss" }

I can't find the referenced advisory, but maybe it is not public (sounds like it was rejected?).

[sl4x0]

Thanks for confirming @krzysdz — and you're right on both counts. I verified the exact behavior you described against the current package:

Confirmed actual output (router@2.2.0, the version Express 5 resolves to):

GET /tenant(v1)-acme-boss
req.params -> { '0': 'acme', tenant: 'boss' }

So the params are not lost, they're misbound: the escaped-literal ( is counted as a phantom key (0), shifting every real group down one slot. user is never set and tenant receives user's value. Your corrected expectation { "tenant": "acme", "user": "boss" } is exactly right.

Root cause is MATCHING_GROUP_REGEXP in lib/layer.js:

const MATCHING_GROUP_REGEXP = /\((?:\?<(.*?)>)?(?!\?)/g

The old path-to-regexp 0.1.x scanner was /\\.|\((?:\?<(.*?)>)?(?!\?)/g and skipped backslash-escaped chars (if (m[0][0] === '\\') continue) — so \( was never treated as a group and the key list stayed aligned with exec(). The router-side rewrite dropped the \\. alternative and the backslash guard, so every \( becomes a phantom key. The "Why" paragraph in my issue is inaccurate (it claims the key count stays at 3 with user unmapped); the real mechanism is the phantom 0 shifting group indices.

Fix — verified by patching the installed router@2.2.0 locally (restore the 0.1.x behavior):

const MATCHING_GROUP_REGEXP = /\\.|\((?:\?<(.*?)>)?(?!\?)/g

and skip escaped matches in the scan loop:

while (m = MATCHING_GROUP_REGEXP.exec(_path.source)) {
  if (m[0][0] === '\\') continue   // escaped literal paren or char, not a group
  keys.push({ name: m[1] || name++, offset: m.index })
}

With that patch: GET /tenant(v1)-acme-boss -> { "tenant": "acme", "user": "boss" } ✓. I also ran a control set to confirm no regressions: (?:…) non-capturing and lookaheads are still correctly skipped, \/(\d+)-(?<id>\w+)$ (no escaped literal) is unchanged, and the named groups on \/(foo)\((x)\)(\d+)-(?<id>\w+)$ come back aligned (previously the id was misbound too).

On the advisory reference — good catch, GHSA-h9x4-4pf4-ffx8 does not resolve via the GitHub API (404). That reference in my issue body is invalid; I'm retracting it. Honest taxonomy: this is a correctness bug with a security-adjacent edge — a RegExp route using escaped literal parens (common for /api/v1)-style version tags or %28%29 filenames) with authorization/introspection on a named param reads the wrong identity. Deterministic, developer-authored (not attacker-controlled), so not a security boundary in the advisory sense — matches "bug worth a fast fix + regression test" rather than a CVE.

Happy to open a PR with the fix plus regression tests (escaped-literal-paren-before-named-groups, plus a control keeping (?:)/lookahead behavior intact). Let me know.