Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix encoding/matching issues with special chars #9477

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

brophdawg11
Copy link
Contributor

tl;dr;
Replaces #9458 with a fairly comprehensive test suite to ensure that we can handle paths with special characters, match against them, and received properly encoded values from useLocation that match what window.location would report.

Background
In 6.3 and prior we were always in a reactive mode to window.history so for example, we would history.pushState and then call the listeners with the value from window.location. This flow meant that we inherited the built-in URL encoding performed by window.location. Therefore, if you clicked <Link to="/✅"> you got { pathname: "/%E2%9C%85", ... } from useLocation().

In 6.4, we go through the new @remix-run/router first and do all of our data fetching and updateState before history.pushState - so we weren't getting the encoding. So at the moment, if you click <Link to="/✅"> you incorrectly get { pathname: "/✅", ... } from useLocation().

We also have historically had trouble with defining route paths with special characters since we were matching encoded paths from window.location to unencoded paths in our <Route> objects.

This PR fixes these 3 main things surrounding special characters:

  • history should always send through the window.location to listeners to ensure full backwards compatibility with 6.3
  • When routing in a data router via router.navigate we manually encode the incoming location using new URL() so we get the same encoding behavior as window.location. This ensures that useLocation in .4 matches what it would have returned in a 6.3 app
  • Since we are now consistently operating on incoming encoded paths, but we want to preserve the ability to match routes defined with special characters we gained from fix: update matchPath to avoid false positives on dash-separated segments #9300, we now decode the incoming pathname prior to matching it against our <Route path> values - allowing us to do <Route path="✅" />

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2022

🦋 Changeset detected

Latest commit: 314888d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router-dom Patch
@remix-run/router Patch
react-router Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -88,7 +89,8 @@ describe("navigate with params", () => {
let pathname = window.location.pathname.replace(/%20/g, "+");
expect(pathname).toEqual("/blog/react+router");

expect(node.innerHTML).toMatch(/react router/);
// Note decodeURIComponent doesn't decode +
expect(node.innerHTML).toMatch(/react\+router/);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a false positive test previously - and was seemingly only because we weren't resetting history state between tests. decodeURIComponent doesn't decode the + sign

@@ -570,7 +570,7 @@ function getUrlBasedHistory(
}

if (v5Compat && listener) {
listener({ action, location });
listener({ action, location: history.location });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

History should pass the encoded location (from window.location after pushState) and not the incoming unencoded location

pathname: url.pathname,
search: url.search,
hash: url.hash,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encode all incoming router.navigate() locations the same way window.location would

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self - move this to a history.encodeLocation method since we don't need to encode for memory histories

// incoming pathnames are always encoded from either window.location or
// from route.navigate, but we want to match against the unencoded paths
// in the route definitions
safelyDecodeURI(pathname)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decode incoming pathnames for matching against our route definitions

* }
*/

let specialChars = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large set of special character "definitions" of the character, and what we expect it to show up as in location.pathname, location.search, and location.hash since it varies according to the spec: https://url.spec.whatwg.org/#c0-control-percent-encode-set

Comment on lines +192 to +221
let routeElements = (
<>
<Route path="/path" element={<Comp heading="Static Route" />} />
<Route
path="/inline-param/:slug"
element={<Comp heading="Inline Nested Param Route" />}
/>
<Route path="/param">
<Route
path=":slug"
element={<Comp heading="Parent Nested Param Route" />}
/>
</Route>
<Route
path="/inline-splat/*"
element={<Comp heading="Inline Nested Splat Route" />}
/>
<Route path="/splat">
<Route
path="*"
element={<Comp heading="Parent Nested Splat Route" />}
/>
</Route>
<Route
path="/reset"
element={<Link to={navigatePath}>Link to path</Link>}
/>
<Route path="/*" element={<Comp heading="Root Splat Route" />} />
</>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test that we can support matching special characters when used in dynamic/splat params in root and nested scenarios

</>
);

// Render BrowserRouter at the initialized location and confirm we get
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section here and below basically checks 4 things:

  • We match routes properly on initial BrowserRouter load (reading from window.location)
  • We match routes properly on client-side BrowserRouter navigations (through history.push)
  • We match routes properly on initial createBrowserRouter/RouterProvider load (reading from window.location)
  • We match route properly on client-side createBrowserRouter/RouterProvider navigations (through router.navigate)

Comment on lines +571 to +582
<>
<Route path={`/${char}`} element={<Root />} />
<Route path="/nested">
<Route path={char} element={<StaticNested />} />
</Route>
<Route path="/:param">
<Route path={char} element={<ParamNested />} />
</Route>
<Route path="/reset" element={<Link to={path}>Link to path</Link>} />
<Route path="*" element={<h1>Not Found</h1>} />
</>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test that we can define and match against route paths containing special characters

packages/router/utils.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants