From 80d58440eea6201dc55edcb21c8561799b56ee5b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 21 Oct 2022 14:01:25 -0400 Subject: [PATCH] fix: fix encoding/matching issues with special chars (#9477) * fix: fix encoding/matching issues with special chars * add changeset * Bump bundle --- .changeset/hot-badgers-grow.md | 6 + .../__tests__/navigate-encode-params-test.tsx | 4 +- .../__tests__/special-characters-test.tsx | 712 ++++++++++++++++++ packages/router/history.ts | 4 +- packages/router/router.ts | 14 + packages/router/utils.ts | 23 +- 6 files changed, 759 insertions(+), 4 deletions(-) create mode 100644 .changeset/hot-badgers-grow.md create mode 100644 packages/react-router-dom/__tests__/special-characters-test.tsx diff --git a/.changeset/hot-badgers-grow.md b/.changeset/hot-badgers-grow.md new file mode 100644 index 000000000..9f3f4da65 --- /dev/null +++ b/.changeset/hot-badgers-grow.md @@ -0,0 +1,6 @@ +--- +"react-router-dom": patch +"@remix-run/router": patch +--- + +fix encoding/matching issues with special chars diff --git a/packages/react-router-dom/__tests__/navigate-encode-params-test.tsx b/packages/react-router-dom/__tests__/navigate-encode-params-test.tsx index bf9bc52e7..47aba5c45 100644 --- a/packages/react-router-dom/__tests__/navigate-encode-params-test.tsx +++ b/packages/react-router-dom/__tests__/navigate-encode-params-test.tsx @@ -12,6 +12,7 @@ import { describe("navigate with params", () => { let node: HTMLDivElement; beforeEach(() => { + global.history.pushState({}, "", "/"); node = document.createElement("div"); document.body.appendChild(node); }); @@ -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/); }); }); }); diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx new file mode 100644 index 000000000..3d8439385 --- /dev/null +++ b/packages/react-router-dom/__tests__/special-characters-test.tsx @@ -0,0 +1,712 @@ +/* eslint-disable jest/expect-expect */ + +import { JSDOM } from "jsdom"; +import * as React from "react"; +import { + cleanup, + render, + fireEvent, + waitFor, + screen, +} from "@testing-library/react"; +import type { Location, Params } from "react-router-dom"; +import { + BrowserRouter, + Link, + Routes, + Route, + RouterProvider, + createBrowserRouter, + createRoutesFromElements, + useLocation, + useParams, +} from "react-router-dom"; + +/** + * Here's all the special characters we want to test against. This list was + * generated using the following utility in the Chrome DevTools console for + * maximum accuracy. This is instead of programmatically generating during + * these tests where JSDOM or a bad URL polyfill might not be trustworthy. + * + * function generateCharDef(char) { + * return { + * char, + * pathChar: new URL('/' + char, window.location.origin).pathname.replace(/^\//, ''), + * searchChar: new URL('/?=' + char, window.location.origin).search.replace(/^\?=/, ''), + * hashChar: new URL('/#' + char, window.location.origin).hash.replace(/^#/, ''), + * }; + * } + */ + +let specialChars = [ + // This set of characters never gets encoded by window.location + { char: "x", pathChar: "x", searchChar: "x", hashChar: "x" }, + { char: "X", pathChar: "X", searchChar: "X", hashChar: "X" }, + { char: "~", pathChar: "~", searchChar: "~", hashChar: "~" }, + { char: "!", pathChar: "!", searchChar: "!", hashChar: "!" }, + { char: "@", pathChar: "@", searchChar: "@", hashChar: "@" }, + { char: "$", pathChar: "$", searchChar: "$", hashChar: "$" }, + { char: "*", pathChar: "*", searchChar: "*", hashChar: "*" }, + { char: "(", pathChar: "(", searchChar: "(", hashChar: "(" }, + { char: ")", pathChar: ")", searchChar: ")", hashChar: ")" }, + { char: "_", pathChar: "_", searchChar: "_", hashChar: "_" }, + { char: "-", pathChar: "-", searchChar: "-", hashChar: "-" }, + { char: "+", pathChar: "+", searchChar: "+", hashChar: "+" }, + { char: "=", pathChar: "=", searchChar: "=", hashChar: "=" }, + { char: "[", pathChar: "[", searchChar: "[", hashChar: "[" }, + { char: "]", pathChar: "]", searchChar: "]", hashChar: "]" }, + { char: ":", pathChar: ":", searchChar: ":", hashChar: ":" }, + { char: ";", pathChar: ";", searchChar: ";", hashChar: ";" }, + { char: ",", pathChar: ",", searchChar: ",", hashChar: "," }, + + // These chars should only get encoded when in the pathname, but JSDOM + // seems to have a bug as it does not encode them, so don't test this + // case for now + // { char: "^", pathChar: "%5E", searchChar: "^", hashChar: "^" }, + // { char: "|", pathChar: "%7C", searchChar: "|", hashChar: "|" }, + + // These chars get conditionally encoded based on what portion of the + // URL they occur in + { char: "{", pathChar: "%7B", searchChar: "{", hashChar: "{" }, + { char: "}", pathChar: "%7D", searchChar: "}", hashChar: "}" }, + { char: "`", pathChar: "%60", searchChar: "`", hashChar: "%60" }, + { char: "'", pathChar: "'", searchChar: "%27", hashChar: "'" }, + { char: '"', pathChar: "%22", searchChar: "%22", hashChar: "%22" }, + { char: "<", pathChar: "%3C", searchChar: "%3C", hashChar: "%3C" }, + { char: ">", pathChar: "%3E", searchChar: "%3E", hashChar: "%3E" }, + + // These chars get encoded in all portions of the URL + { + char: "🤯", + pathChar: "%F0%9F%A4%AF", + searchChar: "%F0%9F%A4%AF", + hashChar: "%F0%9F%A4%AF", + }, + { + char: "✅", + pathChar: "%E2%9C%85", + searchChar: "%E2%9C%85", + hashChar: "%E2%9C%85", + }, + { + char: "🔥", + pathChar: "%F0%9F%94%A5", + searchChar: "%F0%9F%94%A5", + hashChar: "%F0%9F%94%A5", + }, + { char: "ä", pathChar: "%C3%A4", searchChar: "%C3%A4", hashChar: "%C3%A4" }, + { char: "Ä", pathChar: "%C3%84", searchChar: "%C3%84", hashChar: "%C3%84" }, + { char: "ø", pathChar: "%C3%B8", searchChar: "%C3%B8", hashChar: "%C3%B8" }, + { + char: "山", + pathChar: "%E5%B1%B1", + searchChar: "%E5%B1%B1", + hashChar: "%E5%B1%B1", + }, + { + char: "人", + pathChar: "%E4%BA%BA", + searchChar: "%E4%BA%BA", + hashChar: "%E4%BA%BA", + }, + { + char: "口", + pathChar: "%E5%8F%A3", + searchChar: "%E5%8F%A3", + hashChar: "%E5%8F%A3", + }, + { + char: "刀", + pathChar: "%E5%88%80", + searchChar: "%E5%88%80", + hashChar: "%E5%88%80", + }, + { + char: "木", + pathChar: "%E6%9C%A8", + searchChar: "%E6%9C%A8", + hashChar: "%E6%9C%A8", + }, + + // Add a few multi-char space use cases for good measure + { char: "a b", pathChar: "a%20b", searchChar: "a%20b", hashChar: "a%20b" }, + { char: "a+b", pathChar: "a+b", searchChar: "a+b", hashChar: "a+b" }, +]; + +describe("special character tests", () => { + // Mutable vars we'll use to capture the useLocation/useParams values during + // the render pass. this avoids stringifying them into the DOM and parsing + // them back out which can mess with the encoding + let renderedUseLocation: Omit | null = null; + let renderedParams: Params | null = null; + + function CaptureLocation() { + let location = { + ...useLocation(), + state: undefined, + key: undefined, + }; + let params = useParams(); + renderedUseLocation = { + pathname: location.pathname, + search: location.search, + hash: location.hash, + }; + renderedParams = params; + return ( + <> +

{location.pathname}

+ Link to reset + + ); + } + + beforeEach(() => { + renderedUseLocation = null; + renderedParams = null; + }); + + describe("when matching as param values", () => { + async function testParamValues( + navigatePath: string, + expectedHeading: string, + expectedLocation: Omit, + expectedParams = {} + ) { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { + url: "https://remix.run/", + }); + let testWindow = dom.window as unknown as Window; + testWindow.history.replaceState(null, "", navigatePath); + + function Comp({ heading }) { + return ( + <> +

{heading}

+ + + ); + } + + let routeElements = ( + <> + } /> + } + /> + + } + /> + + } + /> + + } + /> + + Link to path} + /> + } /> + + ); + + // Render BrowserRouter at the initialized location and confirm we get + // the right route match, window.location, useLocation(), and useParams() + // values + let ctx = render( + + {routeElements} + + ); + + expect(ctx.container.querySelector("h1")?.innerHTML).toBe( + expectedHeading + ); + + let windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + // Now client side away to a /reset route and back to the original path to confirm that + // client-side history.push calls also match the same expectations + await fireEvent.click(screen.getByText("Link to reset")); + await waitFor(() => screen.getByText(/Link to path/)); + await fireEvent.click(screen.getByText("Link to path")); + await waitFor(() => screen.getByText(/Link to reset/)); + + expect(ctx.container.querySelector("h1")?.innerHTML).toBe( + expectedHeading + ); + + windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + // Reset state + ctx.unmount(); + cleanup(); + renderedUseLocation = null; + renderedParams = null; + + // Now run the same initialized-location render through a data router + // and confirm all the same assertions + let routes = createRoutesFromElements(routeElements); + let router = createBrowserRouter(routes, { window: testWindow }); + ctx = render(); + + expect(ctx.container.querySelector("h1")?.innerHTML).toBe( + expectedHeading + ); + + windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + // Assert both window.location and useLocation() match what we expect + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + // Now client side away to a /reset route and back to the original path to confirm that + // client-side router.navigate calls also match the same expectations + await fireEvent.click(screen.getByText("Link to reset")); + await waitFor(() => screen.getByText(/Link to path/)); + await fireEvent.click(screen.getByText("Link to path")); + await waitFor(() => screen.getByText(/Link to reset/)); + + expect(ctx.container.querySelector("h1")!.innerHTML).toBe( + expectedHeading + ); + + windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + // Assert both window.location and useLocation() match what we expect + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + ctx.unmount(); + cleanup(); + renderedUseLocation = null; + renderedParams = null; + } + + it("handles special chars in inline nested param route paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/inline-param/${char}`, + "Inline Nested Param Route", + { + pathname: `/inline-param/${pathChar}`, + search: "", + hash: "", + }, + { slug: char } + ); + + await testParamValues( + `/inline-param/foo${char}bar`, + "Inline Nested Param Route", + { + pathname: `/inline-param/foo${pathChar}bar`, + search: "", + hash: "", + }, + { slug: `foo${char}bar` } + ); + } + }); + + it("handles special chars in parent nested param route paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/param/${char}`, + "Parent Nested Param Route", + { + pathname: `/param/${pathChar}`, + search: "", + hash: "", + }, + { slug: char } + ); + + await testParamValues( + `/param/foo${char}bar`, + "Parent Nested Param Route", + { + pathname: `/param/foo${pathChar}bar`, + search: "", + hash: "", + }, + { slug: `foo${char}bar` } + ); + } + }); + + it("handles special chars in inline nested splat routes", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/inline-splat/${char}`, + "Inline Nested Splat Route", + { + pathname: `/inline-splat/${pathChar}`, + search: "", + hash: "", + }, + { "*": char } + ); + + await testParamValues( + `/inline-splat/foo${char}bar`, + "Inline Nested Splat Route", + { + pathname: `/inline-splat/foo${pathChar}bar`, + search: "", + hash: "", + }, + { "*": `foo${char}bar` } + ); + } + }); + + it("handles special chars in nested splat routes", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/splat/${char}`, + "Parent Nested Splat Route", + { + pathname: `/splat/${pathChar}`, + search: "", + hash: "", + }, + { "*": char } + ); + + await testParamValues( + `/splat/foo${char}bar`, + "Parent Nested Splat Route", + { + pathname: `/splat/foo${pathChar}bar`, + search: "", + hash: "", + }, + { "*": `foo${char}bar` } + ); + } + }); + + it("handles special chars in nested splat routes with separators", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/splat/foo/bar${char}`, + "Parent Nested Splat Route", + { + pathname: `/splat/foo/bar${pathChar}`, + search: "", + hash: "", + }, + { "*": `foo/bar${char}` } + ); + } + }); + + it("handles special chars in root splat routes", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/${char}`, + "Root Splat Route", + { + pathname: `/${pathChar}`, + search: "", + hash: "", + }, + { "*": char } + ); + + await testParamValues( + `/foo${char}bar`, + "Root Splat Route", + { + pathname: `/foo${pathChar}bar`, + search: "", + hash: "", + }, + { "*": `foo${char}bar` } + ); + } + }); + + it("handles special chars in root splat routes with separators", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + await testParamValues( + `/foo/bar${char}`, + "Root Splat Route", + { + pathname: `/foo/bar${pathChar}`, + search: "", + hash: "", + }, + { "*": `foo/bar${char}` } + ); + } + }); + + it("handles special chars in search params", async () => { + for (let charDef of specialChars) { + let { char, searchChar } = charDef; + await testParamValues(`/path?key=${char}`, "Static Route", { + pathname: `/path`, + search: `?key=${searchChar}`, + hash: "", + }); + } + }); + + it("handles special chars in hash values", async () => { + for (let charDef of specialChars) { + let { char, hashChar } = charDef; + await testParamValues(`/path#hash-${char}`, "Static Route", { + pathname: `/path`, + search: "", + hash: `#hash-${hashChar}`, + }); + } + }); + }); + + describe("when matching as part of the defined route path", () => { + async function assertRouteMatch( + char, + path: string, + expectedHeading: string, + expectedLocation: Omit, + expectedParams = {} + ) { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { + url: "https://remix.run/", + }); + let testWindow = dom.window as unknown as Window; + testWindow.history.replaceState(null, "", path); + + let renderedUseLocation: Omit | null = null; + let renderedParams: Params | null = null; + + function CaptureLocation() { + let location = { + ...useLocation(), + state: undefined, + key: undefined, + }; + let params = useParams(); + renderedUseLocation = location; + renderedParams = params; + return ( + <> +

{location.pathname}

+ Link to reset + + ); + } + + function Root() { + return ( + <> +

Matched Root

+ + + ); + } + + function StaticNested() { + return ( + <> +

Matched Static Nested

+ + + ); + } + + function ParamNested() { + return ( + <> +

Matched Param Nested

+ + + ); + } + + let routeElements = ( + <> + } /> + + } /> + + + } /> + + Link to path} /> + Not Found} /> + + ); + + // Render BrowserRouter at the initialized location and confirm we get + // the right route match, window.location, useLocation(), and useParams() + // values + let ctx = render( + + {routeElements} + + ); + + expect(ctx.container.querySelector("h1")!.innerHTML).toBe( + expectedHeading + ); + let windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + // Reset state + ctx.unmount(); + cleanup(); + renderedUseLocation = null; + renderedParams = null; + + // Now run the same initialized-location render through a data router + // and confirm all the same assertions + let routes = createRoutesFromElements(routeElements); + let router = createBrowserRouter(routes, { window: testWindow }); + ctx = render(); + + expect(ctx.container.querySelector("h1")!.innerHTML).toBe( + expectedHeading + ); + windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + // Now client side away to a /reset route and back to the original path to confirm that + // client-side router.navigate calls also match the same expectations + await fireEvent.click(screen.getByText("Link to reset")); + await waitFor(() => screen.getByText(/Link to path/)); + await fireEvent.click(screen.getByText("Link to path")); + await waitFor(() => screen.getByText(/Link to reset/)); + + expect(ctx.container.querySelector("h1")!.innerHTML).toBe( + expectedHeading + ); + windowLocation = { + pathname: testWindow.location.pathname, + search: testWindow.location.search, + hash: testWindow.location.hash, + }; + expect(windowLocation).toEqual(expectedLocation); + expect(renderedUseLocation).toEqual(expectedLocation); + expect(renderedParams).toEqual(expectedParams); + + ctx.unmount(); + cleanup(); + renderedUseLocation = null; + renderedParams = null; + } + + it("handles special chars in root route paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + // Skip * which is just a splat route + if (char === "*") { + continue; + } + await assertRouteMatch(char, `/${char}`, "Matched Root", { + pathname: `/${pathChar}`, + search: "", + hash: "", + }); + } + }); + + it("handles special chars in static nested route paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + // Skip * which is just a splat route + if (char === "*") { + continue; + } + await assertRouteMatch( + char, + `/nested/${char}`, + "Matched Static Nested", + { + pathname: `/nested/${pathChar}`, + search: "", + hash: "", + } + ); + } + }); + + it("handles special chars in nested param route paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + // Skip * which is just a splat route + if (char === "*") { + continue; + } + await assertRouteMatch( + char, + `/foo/${char}`, + "Matched Param Nested", + { + pathname: `/foo/${pathChar}`, + search: "", + hash: "", + }, + { + param: "foo", + } + ); + } + }); + }); +}); diff --git a/packages/router/history.ts b/packages/router/history.ts index ce6d0c1fc..35d0c5134 100644 --- a/packages/router/history.ts +++ b/packages/router/history.ts @@ -570,7 +570,7 @@ function getUrlBasedHistory( } if (v5Compat && listener) { - listener({ action, location }); + listener({ action, location: history.location }); } } @@ -584,7 +584,7 @@ function getUrlBasedHistory( globalHistory.replaceState(historyState, "", url); if (v5Compat && listener) { - listener({ action, location: location }); + listener({ action, location: history.location }); } } diff --git a/packages/router/router.ts b/packages/router/router.ts index f9cffb396..0e00c308d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -763,6 +763,20 @@ export function createRouter(init: RouterInit): Router { let { path, submission, error } = normalizeNavigateOptions(to, opts); let location = createLocation(state.location, path, opts && opts.state); + + // When using navigate as a PUSH/REPLACE we aren't reading an already-encoded + // URL from window.location, so we need to encode it here so the behavior + // remains the same as POP and non-data-router usages. new URL() does all + // the same encoding we'd get from a history.pushState/window.location read + // without having to touch history + let url = createURL(createPath(location)); + location = { + ...location, + pathname: url.pathname, + search: url.search, + hash: url.hash, + }; + let historyAction = (opts && opts.replace) === true || submission != null ? HistoryAction.Replace diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 6994ba152..308901d5a 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -329,7 +329,13 @@ export function matchRoutes< let matches = null; for (let i = 0; matches == null && i < branches.length; ++i) { - matches = matchRouteBranch(branches[i], pathname); + matches = matchRouteBranch( + branches[i], + // 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) + ); } return matches; @@ -702,6 +708,21 @@ function compilePath( return [matcher, paramNames]; } +function safelyDecodeURI(value: string) { + try { + return decodeURI(value); + } catch (error) { + warning( + false, + `The URL path "${value}" could not be decoded because it is is a ` + + `malformed URL segment. This is probably due to a bad percent ` + + `encoding (${error}).` + ); + + return value; + } +} + function safelyDecodeURIComponent(value: string, paramName: string) { try { return decodeURIComponent(value);