Skip to content

Commit

Permalink
fix: update matchPath to avoid false positives on dash-separated segm…
Browse files Browse the repository at this point in the history
…ents (#9300)

* fix: update matchPath to avoid false positives on dash-separated segments

* Add changeset

* remove uneeded else branch in matching empty paths
  • Loading branch information
brophdawg11 committed Sep 30, 2022
1 parent 9e386f5 commit 779d4af
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .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)
70 changes: 70 additions & 0 deletions packages/react-router-dom/__tests__/nav-link-active-test.tsx
Expand Up @@ -218,6 +218,76 @@ describe("NavLink", () => {

expect(anchor.props.className).not.toMatch("active");
});

it("does not match when <Link to> path is a subset of the active url", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/user-preferences"]}>
<Routes>
<Route
path="/"
element={
<div>
<NavLink to="user">Go to /user</NavLink>
<NavLink to="user-preferences">
Go to /user-preferences
</NavLink>
<Outlet />
</div>
}
>
<Route index element={<p>Index</p>} />
<Route path="user" element={<p>User</p>} />
<Route
path="user-preferences"
element={<p>User Preferences</p>}
/>
</Route>
</Routes>
</MemoryRouter>
);
});

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 <Route path> segment", () => {
let renderer: TestRenderer.ReactTestRenderer;
TestRenderer.act(() => {
renderer = TestRenderer.create(
<MemoryRouter initialEntries={["/user"]}>
<Routes>
<Route
path="/"
element={
<div>
<NavLink to="user">Go to /user</NavLink>
<NavLink to="user-preferences">
Go to /user-preferences
</NavLink>
<Outlet />
</div>
}
>
<Route index element={<p>Index</p>} />
<Route path="user" element={<p>User</p>} />
<Route
path="user-preferences"
element={<p>User Preferences</p>}
/>
</Route>
</Routes>
</MemoryRouter>
);
});

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", () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-router/__tests__/matchPath-test.tsx
Expand Up @@ -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();
Expand Down
22 changes: 13 additions & 9 deletions packages/router/utils.ts
Expand Up @@ -634,16 +634,20 @@ 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 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 {
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|\\/|$)";
// Nothing to match for "" or "/"
}

let matcher = new RegExp(regexpSource, caseSensitive ? undefined : "i");
Expand Down

0 comments on commit 779d4af

Please sign in to comment.