From 5befdcdeb1eba19d7d0d7529fe11b40699159def Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 24 Oct 2022 13:16:11 -0400 Subject: [PATCH 1/4] fix: make url-encoding history-aware --- .../__tests__/special-characters-test.tsx | 272 +++++++++++++++++- packages/router/history.ts | 31 ++ packages/router/router.ts | 36 +-- packages/router/utils.ts | 9 +- 4 files changed, 319 insertions(+), 29 deletions(-) diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx index 3d8439385..75e1f5d0a 100644 --- a/packages/react-router-dom/__tests__/special-characters-test.tsx +++ b/packages/react-router-dom/__tests__/special-characters-test.tsx @@ -9,7 +9,15 @@ import { waitFor, screen, } from "@testing-library/react"; -import type { Location, Params } from "react-router-dom"; +import { + createHashRouter, + createMemoryRouter, + HashRouter, + Location, + MemoryRouter, + Params, + useNavigate, +} from "react-router-dom"; import { BrowserRouter, Link, @@ -709,4 +717,266 @@ describe("special character tests", () => { } }); }); + + describe("encodes characters based on history implementation", () => { + function ShowPath() { + let { pathname, search, hash } = useLocation(); + return 
{JSON.stringify({ pathname, search, hash })}
; + } + + describe("memory routers", () => { + it("does not encode characters in MemoryRouter", () => { + let ctx = render( + + + } /> + + + ); + + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("does not encode characters in MemoryRouter (navigate)", () => { + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + let ctx = render( + + + } /> + } /> + + + ); + + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("does not encode characters in createMemoryRouter", () => { + let router = createMemoryRouter( + [{ path: "/with space", element: }], + { initialEntries: ["/with space"] } + ); + let ctx = render(); + + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("does not encode characters in createMemoryRouter (navigate)", () => { + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + let router = createMemoryRouter([ + { path: "/", element: }, + { path: "/with space", element: }, + ]); + let ctx = render(); + + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + }); + + describe("browser routers", () => { + let testWindow: Window; + + beforeEach(() => { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { + url: "https://remix.run/", + }); + testWindow = dom.window as unknown as Window; + testWindow.history.pushState({}, "", "/"); + }); + + it("encodes characters in BrowserRouter", () => { + testWindow.history.replaceState(null, "", "/with space"); + + let ctx = render( + + + } /> + + + ); + + expect(testWindow.location.pathname).toBe("/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in BrowserRouter (navigate)", () => { + testWindow.history.replaceState(null, "", "/"); + + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + + let ctx = render( + + + } /> + } /> + + + ); + + expect(testWindow.location.pathname).toBe("/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in createBrowserRouter", () => { + testWindow.history.replaceState(null, "", "/with space"); + + let router = createBrowserRouter( + [{ path: "/with space", element: }], + { window: testWindow } + ); + let ctx = render(); + + expect(testWindow.location.pathname).toBe("/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in createBrowserRouter (navigate)", () => { + testWindow.history.replaceState(null, "", "/with space"); + + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + + let router = createBrowserRouter( + [ + { path: "/", element: }, + { path: "/with space", element: }, + ], + { window: testWindow } + ); + let ctx = render(); + + expect(testWindow.location.pathname).toBe("/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + }); + + describe("hash routers", () => { + let testWindow: Window; + + beforeEach(() => { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { + url: "https://remix.run/", + }); + testWindow = dom.window as unknown as Window; + testWindow.history.pushState({}, "", "/"); + }); + + it("encodes characters in HashRouter", () => { + testWindow.history.replaceState(null, "", "/#/with space"); + + let ctx = render( + + + } /> + + + ); + + expect(testWindow.location.pathname).toBe("/"); + expect(testWindow.location.hash).toBe("#/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in HashRouter (navigate)", () => { + testWindow.history.replaceState(null, "", "/"); + + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + + let ctx = render( + + + } /> + } /> + + + ); + + expect(testWindow.location.pathname).toBe("/"); + expect(testWindow.location.hash).toBe("#/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in createHashRouter", () => { + testWindow.history.replaceState(null, "", "/#/with space"); + + let router = createHashRouter( + [{ path: "/with space", element: }], + { window: testWindow } + ); + let ctx = render(); + + expect(testWindow.location.pathname).toBe("/"); + expect(testWindow.location.hash).toBe("#/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + + it("encodes characters in createHashRouter (navigate)", () => { + testWindow.history.replaceState(null, "", "/"); + + function Start() { + let navigate = useNavigate(); + React.useEffect(() => navigate("/with space"), []); + return null; + } + + let router = createHashRouter( + [ + { path: "/", element: }, + { path: "/with space", element: }, + ], + { window: testWindow } + ); + let ctx = render(); + + expect(testWindow.location.pathname).toBe("/"); + expect(testWindow.location.hash).toBe("#/with%20space"); + expect(ctx.container.innerHTML).toMatchInlineSnapshot( + `"
{\\"pathname\\":\\"/with%20space\\",\\"search\\":\\"\\",\\"hash\\":\\"\\"}
"` + ); + }); + }); + }); }); diff --git a/packages/router/history.ts b/packages/router/history.ts index 35d0c5134..84fcae3aa 100644 --- a/packages/router/history.ts +++ b/packages/router/history.ts @@ -125,6 +125,15 @@ export interface History { */ createHref(to: To): string; + /** + * Encode a location the same way window.history would do (no-op for memory + * history) so we ensure our PUSH/REPLAC e navigations for data routers + * behave the same as POP + * + * @param location The incoming location from router.navigate() + */ + encodeLocation(location: Location): Location; + /** * Pushes a new location onto the history stack, increasing its length by one. * If there were any entries in the stack after the current one, they are @@ -259,6 +268,9 @@ export function createMemoryHistory( createHref(to) { return typeof to === "string" ? to : createPath(to); }, + encodeLocation(location) { + return location; + }, push(to, state) { action = Action.Push; let nextLocation = createMemoryLocation(to, state); @@ -527,6 +539,15 @@ export function parsePath(path: string): Partial { return parsedPath; } +export function createURL(location: Location | string): URL { + let base = + typeof window !== "undefined" && typeof window.location !== "undefined" + ? window.location.origin + : "unknown://unknown"; + let href = typeof location === "string" ? location : createPath(location); + return new URL(href, base); +} + export interface UrlHistory extends History {} export type UrlHistoryOptions = { @@ -610,6 +631,16 @@ function getUrlBasedHistory( createHref(to) { return createHref(window, to); }, + encodeLocation(location) { + // Encode a Location the same way window.location would + let url = createURL(createPath(location)); + return { + ...location, + pathname: url.pathname, + search: url.search, + hash: url.hash, + }; + }, push, replace, go(n) { diff --git a/packages/router/router.ts b/packages/router/router.ts index b409ffc38..800f8a12d 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1,9 +1,12 @@ +import { pathExists } from "fs-extra"; import type { History, Location, Path, To } from "./history"; import { Action as HistoryAction, createLocation, createPath, + createURL, parsePath, + safelyDecodeURI, } from "./history"; import type { DataResult, @@ -769,13 +772,7 @@ export function createRouter(init: RouterInit): Router { // 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, - }; + location = init.history.encodeLocation(location); let historyAction = (opts && opts.replace) === true || submission != null @@ -2038,14 +2035,13 @@ export function unstable_createStaticHandler( ): Promise | Response> { let result: DataResult; if (!actionMatch.route.action) { - let href = createServerHref(new URL(request.url)); if (isRouteRequest) { throw createRouterErrorResponse(null, { status: 405, statusText: "Method Not Allowed", }); } - result = getMethodNotAllowedResult(href); + result = getMethodNotAllowedResult(request.url); } else { result = await callLoaderOrAction( "action", @@ -2288,7 +2284,7 @@ function normalizeNavigateOptions( path, submission: { formMethod: opts.formMethod, - formAction: createServerHref(parsePath(path)), + formAction: stripHashFromPath(path), formEncType: (opts && opts.formEncType) || "application/x-www-form-urlencoded", formData: opts.formData, @@ -2644,7 +2640,7 @@ function createRequest( signal: AbortSignal, submission?: Submission ): Request { - let url = createURL(location).toString(); + let url = createURL(stripHashFromPath(location)).toString(); let init: RequestInit = { signal }; if (submission) { @@ -2894,7 +2890,7 @@ function getMethodNotAllowedMatches(routes: AgnosticDataRouteObject[]) { } function getMethodNotAllowedResult(path: Location | string): ErrorResult { - let href = typeof path === "string" ? path : createServerHref(path); + let href = typeof path === "string" ? path : createPath(path); console.warn( "You're trying to submit to a route that does not have an action. To " + "fix this, please add an `action` function to the route for " + @@ -2916,11 +2912,6 @@ function findRedirect(results: DataResult[]): RedirectResult | undefined { } } -// Create an href to represent a "server" URL without the hash -function createServerHref(location: Partial | Location | URL) { - return (location.pathname || "") + (location.search || ""); -} - function isHashChangeOnly(a: Location, b: Location): boolean { return ( a.pathname === b.pathname && a.search === b.search && a.hash !== b.hash @@ -3059,13 +3050,8 @@ function getTargetMatch( return pathMatches[pathMatches.length - 1]; } -function createURL(location: Location | string): URL { - let base = - typeof window !== "undefined" && typeof window.location !== "undefined" - ? window.location.origin - : "unknown://unknown"; - let href = - typeof location === "string" ? location : createServerHref(location); - return new URL(href, base); +function stripHashFromPath(path: To) { + let parsedPath = typeof path === "string" ? parsePath(path) : path; + return createPath({ ...parsedPath, hash: "" }); } //#endregion diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 70761f614..016e19bf2 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -331,9 +331,12 @@ export function matchRoutes< for (let i = 0; matches == null && i < branches.length; ++i) { 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 + // Incoming pathnames are generally encoded from either window.location + // or from router.navigate, but we want to match against the unencoded + // paths in the route definitions. Memory router locations won't be + // encoded here but there also shouldn't be anything to decode so this + // should be a safe operation. This avoids needing matchRoutes to be + // history-aware. safelyDecodeURI(pathname) ); } From f411b9df48542f8990f1b6e49ccbcaa52e7addc1 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 24 Oct 2022 13:17:06 -0400 Subject: [PATCH 2/4] add changeset --- .changeset/ninety-countries-cheat.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/ninety-countries-cheat.md diff --git a/.changeset/ninety-countries-cheat.md b/.changeset/ninety-countries-cheat.md new file mode 100644 index 000000000..e54b33c6d --- /dev/null +++ b/.changeset/ninety-countries-cheat.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +make url-encoding history-aware From ba7ba49d5635418fb53794ad10823260968d5b75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 24 Oct 2022 13:27:39 -0400 Subject: [PATCH 3/4] fix lint warnings --- .../__tests__/special-characters-test.tsx | 19 +++++++++++-------- packages/router/router.ts | 4 +--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx index 75e1f5d0a..ff66a9558 100644 --- a/packages/react-router-dom/__tests__/special-characters-test.tsx +++ b/packages/react-router-dom/__tests__/special-characters-test.tsx @@ -9,24 +9,21 @@ import { waitFor, screen, } from "@testing-library/react"; +import type { Location, Params } from "react-router-dom"; import { - createHashRouter, - createMemoryRouter, + BrowserRouter, HashRouter, - Location, MemoryRouter, - Params, - useNavigate, -} from "react-router-dom"; -import { - BrowserRouter, Link, Routes, Route, RouterProvider, createBrowserRouter, + createHashRouter, + createMemoryRouter, createRoutesFromElements, useLocation, + useNavigate, useParams, } from "react-router-dom"; @@ -742,6 +739,7 @@ describe("special character tests", () => { it("does not encode characters in MemoryRouter (navigate)", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } @@ -774,6 +772,7 @@ describe("special character tests", () => { it("does not encode characters in createMemoryRouter (navigate)", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } @@ -823,6 +822,7 @@ describe("special character tests", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } @@ -862,6 +862,7 @@ describe("special character tests", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } @@ -917,6 +918,7 @@ describe("special character tests", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } @@ -958,6 +960,7 @@ describe("special character tests", () => { function Start() { let navigate = useNavigate(); + // eslint-disable-next-line react-hooks/exhaustive-deps React.useEffect(() => navigate("/with space"), []); return null; } diff --git a/packages/router/router.ts b/packages/router/router.ts index 800f8a12d..652915b71 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1,12 +1,10 @@ -import { pathExists } from "fs-extra"; -import type { History, Location, Path, To } from "./history"; +import type { History, Location, To } from "./history"; import { Action as HistoryAction, createLocation, createPath, createURL, parsePath, - safelyDecodeURI, } from "./history"; import type { DataResult, From 68b2478fa622ee2ec0aaf8535b18ac58f4eb8ae6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 24 Oct 2022 13:40:54 -0400 Subject: [PATCH 4/4] organize code --- packages/router/router.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 652915b71..700b5c463 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2910,6 +2910,11 @@ function findRedirect(results: DataResult[]): RedirectResult | undefined { } } +function stripHashFromPath(path: To) { + let parsedPath = typeof path === "string" ? parsePath(path) : path; + return createPath({ ...parsedPath, hash: "" }); +} + function isHashChangeOnly(a: Location, b: Location): boolean { return ( a.pathname === b.pathname && a.search === b.search && a.hash !== b.hash @@ -3047,9 +3052,4 @@ function getTargetMatch( let pathMatches = getPathContributingMatches(matches); return pathMatches[pathMatches.length - 1]; } - -function stripHashFromPath(path: To) { - let parsedPath = typeof path === "string" ? parsePath(path) : path; - return createPath({ ...parsedPath, hash: "" }); -} //#endregion