From 44f440c544a6828f1964f8b000169b3d182ce62b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 19 Sep 2022 09:03:52 -0400 Subject: [PATCH 1/3] fix: update matchPath to avoid false positives on dash-separated segments --- .../__tests__/nav-link-active-test.tsx | 70 +++++++++++++++++++ .../react-router/__tests__/matchPath-test.tsx | 4 ++ packages/router/utils.ts | 29 +++++--- 3 files changed, 94 insertions(+), 9 deletions(-) diff --git a/packages/react-router-dom/__tests__/nav-link-active-test.tsx b/packages/react-router-dom/__tests__/nav-link-active-test.tsx index 4cc70728d3..34851b7d9d 100644 --- a/packages/react-router-dom/__tests__/nav-link-active-test.tsx +++ b/packages/react-router-dom/__tests__/nav-link-active-test.tsx @@ -218,6 +218,76 @@ describe("NavLink", () => { expect(anchor.props.className).not.toMatch("active"); }); + + it("does not match when path is a subset of the active url", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + Go to /user + + Go to /user-preferences + + + + } + > + Index

} /> + User

} /> + User Preferences

} + /> + + + + ); + }); + + let anchors = renderer.root.findAllByType("a"); + + expect(anchors.map((a) => a.props.className)).toEqual(["", "active"]); + }); + + it("does not match when active url is a subset of a segment", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + Go to /user + + Go to /user-preferences + + + + } + > + Index

} /> + User

} /> + User Preferences

} + /> + + + + ); + }); + + let anchors = renderer.root.findAllByType("a"); + + expect(anchors.map((a) => a.props.className)).toEqual(["active", ""]); + }); }); describe("when it matches just the beginning but not to the end", () => { diff --git a/packages/react-router/__tests__/matchPath-test.tsx b/packages/react-router/__tests__/matchPath-test.tsx index 795e889ce4..e36f73ed8a 100644 --- a/packages/react-router/__tests__/matchPath-test.tsx +++ b/packages/react-router/__tests__/matchPath-test.tsx @@ -156,6 +156,10 @@ describe("matchPath", () => { it("fails to match a pathname where the segments do not match", () => { expect(matchPath({ path: "/users", end: false }, "/")).toBeNull(); expect(matchPath({ path: "/users", end: false }, "/users2")).toBeNull(); + expect(matchPath({ path: "/users", end: false }, "/users-2")).toBeNull(); + expect(matchPath({ path: "/users", end: false }, "/users~2")).toBeNull(); + expect(matchPath({ path: "/users", end: false }, "/users@2")).toBeNull(); + expect(matchPath({ path: "/users", end: false }, "/users.2")).toBeNull(); expect( matchPath({ path: "/users/mj", end: false }, "/users/mj2") ).toBeNull(); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index cb3e89b85d..0709e91ec3 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -634,16 +634,27 @@ function compilePath( path === "*" || path === "/*" ? "(.*)$" // Already matched the initial /, just match the rest : "(?:\\/(.+)|\\/*)$"; // Don't include the / in params["*"] + } else if (end) { + // When matching to the end, ignore trailing slashes + regexpSource += "\\/*$"; + } else if (path !== "" && path !== "/") { + // If our path is non-empty and contains anything beyond an initial slash, + // then we have _some_ form of path and we do not want to include the + // positive lookahead below, otherwise we incorrectly match things like + // /user-preferences for path /user. So, we look for an optional non-captured + // trailing slash (to match a portion of the URL) or the end of the path + // (if we've matched to the end). We used to do this with a word boundary + // but that gives false positives on routes like /user-preferences since + // `-` counts as a word boundary. + regexpSource += "(?:(?=\\/|$))"; } else { - regexpSource += end - ? "\\/*$" // When matching to the end, ignore trailing slashes - : // Otherwise, match a word boundary or a proceeding /. The word boundary restricts - // parent routes to matching only their own words and nothing more, e.g. parent - // route "/home" should not match "/home2". - // Additionally, allow paths starting with `.`, `-`, `~`, and url-encoded entities, - // but do not consume the character in the matched path so they can match against - // nested paths. - "(?:(?=[@.~-]|%[0-9A-F]{2})|\\b|\\/|$)"; + // Otherwise, if we are an empty path, match a word boundary or a + // proceeding /. The word boundary restricts parent routes to matching only + // their own words and nothing more, e.g. parent route "/home" should not + // match "/home2". Additionally, allow paths starting with `.`, `-`, `~`, + // and url-encoded entities, but do not consume the character in the + // matched path so they can match against nested paths. + regexpSource += "(?:(?=[@.~-]|%[0-9A-F]{2})|\\b|\\/|$)"; } let matcher = new RegExp(regexpSource, caseSensitive ? undefined : "i"); From 2aa1581522d94ed35e51ab02d82c8b5836f4020f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 19 Sep 2022 09:09:29 -0400 Subject: [PATCH 2/3] Add changeset --- .changeset/sweet-chicken-suffer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/sweet-chicken-suffer.md diff --git a/.changeset/sweet-chicken-suffer.md b/.changeset/sweet-chicken-suffer.md new file mode 100644 index 0000000000..7cbb3004da --- /dev/null +++ b/.changeset/sweet-chicken-suffer.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +fix: update matchPath to avoid false positives on dash-separated segments (#9300) From decf69ba68f3c0371d92692fbb3291bb8ba2a4b7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 29 Sep 2022 17:06:18 -0400 Subject: [PATCH 3/3] remove uneeded else branch in matching empty paths --- packages/router/utils.ts | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 0709e91ec3..e60d2d901a 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -639,22 +639,15 @@ function compilePath( regexpSource += "\\/*$"; } else if (path !== "" && path !== "/") { // If our path is non-empty and contains anything beyond an initial slash, - // then we have _some_ form of path and we do not want to include the - // positive lookahead below, otherwise we incorrectly match things like - // /user-preferences for path /user. So, we look for an optional non-captured - // trailing slash (to match a portion of the URL) or the end of the path - // (if we've matched to the end). We used to do this with a word boundary - // but that gives false positives on routes like /user-preferences since - // `-` counts as a word boundary. + // then we have _some_ form of path in our regex so we should expect to + // match only if we find the end of this path segment. Look for an optional + // non-captured trailing slash (to match a portion of the URL) or the end + // of the path (if we've matched to the end). We used to do this with a + // word boundary but that gives false positives on routes like + // /user-preferences since `-` counts as a word boundary. regexpSource += "(?:(?=\\/|$))"; } else { - // Otherwise, if we are an empty path, match a word boundary or a - // proceeding /. The word boundary restricts parent routes to matching only - // their own words and nothing more, e.g. parent route "/home" should not - // match "/home2". Additionally, allow paths starting with `.`, `-`, `~`, - // and url-encoded entities, but do not consume the character in the - // matched path so they can match against nested paths. - regexpSource += "(?:(?=[@.~-]|%[0-9A-F]{2})|\\b|\\/|$)"; + // Nothing to match for "" or "/" } let matcher = new RegExp(regexpSource, caseSensitive ? undefined : "i");