From 241f2d4b7a0d091b0c634304c6f5f121a8833bca Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 6 Feb 2024 10:17:07 -0500 Subject: [PATCH] Fix issues with pre-encoded param names not being properly decoded (#11199) --- .changeset/fifty-pugs-boil.md | 6 + package.json | 4 +- .../lib/components.tsx | 4 + .../__tests__/special-characters-test.tsx | 265 +++++++++++------- packages/react-router-dom/server.tsx | 4 + packages/react-router/lib/hooks.tsx | 25 +- packages/router/history.ts | 4 + packages/router/utils.ts | 42 +-- 8 files changed, 214 insertions(+), 140 deletions(-) create mode 100644 .changeset/fifty-pugs-boil.md diff --git a/.changeset/fifty-pugs-boil.md b/.changeset/fifty-pugs-boil.md new file mode 100644 index 0000000000..59d5dff2f3 --- /dev/null +++ b/.changeset/fifty-pugs-boil.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +Fix encoding/decoding issues with pre-encoded dynamic parameter values diff --git a/package.json b/package.json index ec4a586323..35e7d49de0 100644 --- a/package.json +++ b/package.json @@ -119,10 +119,10 @@ "none": "50.4 kB" }, "packages/react-router/dist/react-router.production.min.js": { - "none": "14.7 kB" + "none": "14.73 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { - "none": "17.1 kB" + "none": "17.2 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { "none": "17.0 kB" diff --git a/packages/react-router-dom-v5-compat/lib/components.tsx b/packages/react-router-dom-v5-compat/lib/components.tsx index 3f68eb2d05..cdf5e7eb1b 100644 --- a/packages/react-router-dom-v5-compat/lib/components.tsx +++ b/packages/react-router-dom-v5-compat/lib/components.tsx @@ -96,6 +96,10 @@ export function StaticRouter({ }, encodeLocation(to: To) { let href = typeof to === "string" ? to : createPath(to); + // Treating this as a full URL will strip any trailing spaces so we need to + // pre-encode them since they might be part of a matching splat param from + // an ancestor route + href = href.replace(/ $/, "%20"); let encoded = ABSOLUTE_URL_REGEX.test(href) ? new URL(href) : new URL(href, "http://localhost"); diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx index 0f6a5ddb6e..ce0dfcb2c7 100644 --- a/packages/react-router-dom/__tests__/special-characters-test.tsx +++ b/packages/react-router-dom/__tests__/special-characters-test.tsx @@ -26,6 +26,7 @@ import { useNavigate, useParams, } from "react-router-dom"; +import getHtml from "../../react-router/__tests__/utils/getHtml"; /** * Here's all the special characters we want to test against. This list was @@ -33,6 +34,15 @@ import { * maximum accuracy. This is instead of programmatically generating during * these tests where JSDOM or a bad URL polyfill might not be trustworthy. * + * + * | Field | Description | + * |-------------|----------------------------------------------------------------------| + * | char | The (usually decoded) verbatim "character" you put in your | + * | pathChar | The value we expect to receive from location.pathname | + * | searchChar | The value we expect to receive from location.search | + * | hashChar | The value we expect to receive from location.hash | + * | decodedChar | The decoded value we expect to receive from params | + * * function generateCharDef(char) { * return { * char, @@ -43,26 +53,27 @@ import { * } */ +// prettier-ignore 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: "," }, + { char: "x", pathChar: "x", searchChar: "x", hashChar: "x", decodedChar: "x" }, + { char: "X", pathChar: "X", searchChar: "X", hashChar: "X", decodedChar: "X" }, + { char: "~", pathChar: "~", searchChar: "~", hashChar: "~", decodedChar: "~" }, + { char: "!", pathChar: "!", searchChar: "!", hashChar: "!", decodedChar: "!" }, + { char: "@", pathChar: "@", searchChar: "@", hashChar: "@", decodedChar: "@" }, + { char: "$", pathChar: "$", searchChar: "$", hashChar: "$", decodedChar: "$" }, + { char: "*", pathChar: "*", searchChar: "*", hashChar: "*", decodedChar: "*" }, + { char: "(", pathChar: "(", searchChar: "(", hashChar: "(", decodedChar: "(" }, + { char: ")", pathChar: ")", searchChar: ")", hashChar: ")", decodedChar: ")" }, + { char: "_", pathChar: "_", searchChar: "_", hashChar: "_", decodedChar: "_" }, + { char: "-", pathChar: "-", searchChar: "-", hashChar: "-", decodedChar: "-" }, + { char: "+", pathChar: "+", searchChar: "+", hashChar: "+", decodedChar: "+" }, + { char: "=", pathChar: "=", searchChar: "=", hashChar: "=", decodedChar: "=" }, + { char: "[", pathChar: "[", searchChar: "[", hashChar: "[", decodedChar: "[" }, + { char: "]", pathChar: "]", searchChar: "]", hashChar: "]", decodedChar: "]" }, + { char: ":", pathChar: ":", searchChar: ":", hashChar: ":", decodedChar: ":" }, + { char: ";", pathChar: ";", searchChar: ";", hashChar: ";", decodedChar: ";" }, + { char: ",", pathChar: ",", searchChar: ",", hashChar: ",", decodedChar: "," }, // 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 @@ -72,70 +83,39 @@ let specialChars = [ // 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" }, + { char: "{", pathChar: "%7B", searchChar: "{", hashChar: "{", decodedChar: "{" }, + { char: "}", pathChar: "%7D", searchChar: "}", hashChar: "}", decodedChar: "}" }, + { char: "`", pathChar: "%60", searchChar: "`", hashChar: "%60", decodedChar: "`" }, + { char: "'", pathChar: "'", searchChar: "%27", hashChar: "'", decodedChar: "'" }, + { char: '"', pathChar: "%22", searchChar: "%22", hashChar: "%22", decodedChar: '"' }, + { char: "<", pathChar: "%3C", searchChar: "%3C", hashChar: "%3C", decodedChar: "<" }, + { char: ">", pathChar: "%3E", searchChar: "%3E", hashChar: "%3E", decodedChar: ">" }, // 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", - }, + { char: "🤯", pathChar: "%F0%9F%A4%AF", searchChar: "%F0%9F%A4%AF", hashChar: "%F0%9F%A4%AF", decodedChar: "🤯" }, + { char: "✅", pathChar: "%E2%9C%85", searchChar: "%E2%9C%85", hashChar: "%E2%9C%85", decodedChar: "✅" }, + { char: "🔥", pathChar: "%F0%9F%94%A5", searchChar: "%F0%9F%94%A5", hashChar: "%F0%9F%94%A5", decodedChar: "🔥" }, + { char: "ä", pathChar: "%C3%A4", searchChar: "%C3%A4", hashChar: "%C3%A4", decodedChar: "ä" }, + { char: "Ä", pathChar: "%C3%84", searchChar: "%C3%84", hashChar: "%C3%84", decodedChar: "Ä" }, + { char: "ø", pathChar: "%C3%B8", searchChar: "%C3%B8", hashChar: "%C3%B8", decodedChar: "ø" }, + { char: "山", pathChar: "%E5%B1%B1", searchChar: "%E5%B1%B1", hashChar: "%E5%B1%B1", decodedChar: "山" }, + { char: "人", pathChar: "%E4%BA%BA", searchChar: "%E4%BA%BA", hashChar: "%E4%BA%BA", decodedChar: "人" }, + { char: "口", pathChar: "%E5%8F%A3", searchChar: "%E5%8F%A3", hashChar: "%E5%8F%A3", decodedChar: "口" }, + { char: "刀", pathChar: "%E5%88%80", searchChar: "%E5%88%80", hashChar: "%E5%88%80", decodedChar: "刀" }, + { char: "木", pathChar: "%E6%9C%A8", searchChar: "%E6%9C%A8", hashChar: "%E6%9C%A8", decodedChar: "木" }, // 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" }, + { char: "a b", pathChar: "a%20b", searchChar: "a%20b", hashChar: "a%20b", decodedChar: "a b" }, + { char: "a+b", pathChar: "a+b", searchChar: "a+b", hashChar: "a+b", decodedChar: "a+b" }, + + // Edge case scenarios where the incoming `char` (or string) is pre-encoded + // because it contains special characters such as `&`, `%`, or `#`. For these + // we provide a `decodedChar` so we can assert the param value gets decoded + // properly and so we can ensure we can match these decoded values in static + // paths + { char: "a%25b", pathChar: "a%25b", searchChar: "a%25b", hashChar: "a%25b", decodedChar: "a%b" }, + { char: "a%23b%25c", pathChar: "a%23b%25c", searchChar: "a%23b%25c", hashChar: "a%23b%25c", decodedChar: "a#b%c" }, + { char: "a%26b%25c", pathChar: "a%26b%25c", searchChar: "a%26b%25c", hashChar: "a%26b%25c", decodedChar: "a&b%c" }, ]; describe("special character tests", () => { @@ -333,7 +313,7 @@ describe("special character tests", () => { it("handles special chars in inline nested param route paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/inline-param/${char}`, "Inline Nested Param Route", @@ -342,7 +322,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { slug: char } + { slug: decodedChar } ); await testParamValues( @@ -353,14 +333,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { slug: `foo${char}bar` } + { slug: `foo${decodedChar}bar` } ); } }); it("handles special chars in parent nested param route paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/param/${char}`, "Parent Nested Param Route", @@ -369,7 +349,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { slug: char } + { slug: decodedChar } ); await testParamValues( @@ -380,14 +360,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { slug: `foo${char}bar` } + { slug: `foo${decodedChar}bar` } ); } }); it("handles special chars in inline nested splat routes", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/inline-splat/${char}`, "Inline Nested Splat Route", @@ -396,7 +376,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": char } + { "*": decodedChar } ); await testParamValues( @@ -407,14 +387,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": `foo${char}bar` } + { "*": `foo${decodedChar}bar` } ); } }); it("handles special chars in nested splat routes", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/splat/${char}`, "Parent Nested Splat Route", @@ -423,7 +403,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": char } + { "*": decodedChar } ); await testParamValues( @@ -434,14 +414,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": `foo${char}bar` } + { "*": `foo${decodedChar}bar` } ); } }); it("handles special chars in nested splat routes with separators", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/splat/foo/bar${char}`, "Parent Nested Splat Route", @@ -450,14 +430,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": `foo/bar${char}` } + { "*": `foo/bar${decodedChar}` } ); } }); it("handles special chars in root splat routes", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/${char}`, "Root Splat Route", @@ -466,7 +446,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": char } + { "*": decodedChar } ); await testParamValues( @@ -477,14 +457,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": `foo${char}bar` } + { "*": `foo${decodedChar}bar` } ); } }); it("handles special chars in root splat routes with separators", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/foo/bar${char}`, "Root Splat Route", @@ -493,14 +473,14 @@ describe("special character tests", () => { search: "", hash: "", }, - { "*": `foo/bar${char}` } + { "*": `foo/bar${decodedChar}` } ); } }); it("handles special chars in descendant routes paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; await testParamValues( `/descendant/${char}/match`, @@ -510,7 +490,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { param: char, "*": "match" } + { param: decodedChar, "*": "match" } ); await testParamValues( @@ -521,7 +501,7 @@ describe("special character tests", () => { search: "", hash: "", }, - { param: `foo${char}bar`, "*": "match" } + { param: `foo${decodedChar}bar`, "*": "match" } ); } }); @@ -547,6 +527,79 @@ describe("special character tests", () => { }); } }); + + it("does not trim trailing spaces on ancestor splat route segments", async () => { + let ctx = render( + + + + ); + + expect(getHtml(ctx.container)).toMatchInlineSnapshot(` + "
+ + Link to grandchild + +
" + `); + + await fireEvent.click(screen.getByText("Link to grandchild")); + await waitFor(() => screen.getByText("Grandchild")); + + expect(getHtml(ctx.container)).toMatchInlineSnapshot(` + "
+ + Link to grandchild + +

+ Grandchild +

+ 
+            {"*":"grandchild","param":"  param  "}
+
+
" + `); + + function App() { + return ( + + } /> + + ); + } + + function Parent() { + return ( + + } /> + + ); + } + + function Child() { + return ( + <> + Link to grandchild + + } /> + + + ); + } + + function Grandchild() { + return ( + <> +

Grandchild

+ 
{JSON.stringify(useParams())}
+ + ); + } + }); }); describe("when matching as part of the defined route path", () => { @@ -697,12 +750,12 @@ describe("special character tests", () => { it("handles special chars in root route paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; // Skip * which is just a splat route if (char === "*") { continue; } - await assertRouteMatch(char, `/${char}`, "Matched Root", { + await assertRouteMatch(decodedChar, `/${char}`, "Matched Root", { pathname: `/${pathChar}`, search: "", hash: "", @@ -712,13 +765,13 @@ describe("special character tests", () => { it("handles special chars in static nested route paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; // Skip * which is just a splat route if (char === "*") { continue; } await assertRouteMatch( - char, + decodedChar, `/nested/${char}`, "Matched Static Nested", { @@ -732,13 +785,13 @@ describe("special character tests", () => { it("handles special chars in nested param route paths", async () => { for (let charDef of specialChars) { - let { char, pathChar } = charDef; + let { char, pathChar, decodedChar } = charDef; // Skip * which is just a splat route if (char === "*") { continue; } await assertRouteMatch( - char, + decodedChar, `/foo/${char}`, "Matched Param Nested", { diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 6b75be9ed9..0e07d98d35 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -389,6 +389,10 @@ function createHref(to: To) { function encodeLocation(to: To): Path { let href = typeof to === "string" ? to : createPath(to); + // Treating this as a full URL will strip any trailing spaces so we need to + // pre-encode them since they might be part of a matching splat param from + // an ancestor route + href = href.replace(/ $/, "%20"); let encoded = ABSOLUTE_URL_REGEX.test(href) ? new URL(href) : new URL(href, "http://localhost"); diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 5d705553e7..0feda4720f 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -422,10 +422,27 @@ export function useRoutesImpl( } let pathname = location.pathname || "/"; - let remainingPathname = - parentPathnameBase === "/" - ? pathname - : pathname.slice(parentPathnameBase.length) || "/"; + + let remainingPathname = pathname; + if (parentPathnameBase !== "/") { + // Determine the remaining pathname by removing the # of URL segments the + // parentPathnameBase has, instead of removing based on character count. + // This is because we can't guarantee that incoming/outgoing encodings/ + // decodings will match exactly. + // We decode paths before matching on a per-segment basis with + // decodeURIComponent(), but we re-encode pathnames via `new URL()` so they + // match what `window.location.pathname` would reflect. Those don't 100% + // align when it comes to encoded URI characters such as % and &. + // + // So we may end up with: + // pathname: "/descendant/a%25b/match" + // parentPathnameBase: "/descendant/a%b" + // + // And the direct substring removal approach won't work :/ + let parentSegments = parentPathnameBase.replace(/^\//, "").split("/"); + let segments = pathname.replace(/^\//, "").split("/"); + remainingPathname = "/" + segments.slice(parentSegments.length).join("/"); + } let matches = matchRoutes(routes, { pathname: remainingPathname }); diff --git a/packages/router/history.ts b/packages/router/history.ts index ad4bd44b09..335645db1c 100644 --- a/packages/router/history.ts +++ b/packages/router/history.ts @@ -690,6 +690,10 @@ function getUrlBasedHistory( : window.location.href; let href = typeof to === "string" ? to : createPath(to); + // Treating this as a full URL will strip any trailing spaces so we need to + // pre-encode them since they might be part of a matching splat param from + // an ancestor route + href = href.replace(/ $/, "%20"); invariant( base, `No window.location.(origin|href) available to create URL for href: ${href}` diff --git a/packages/router/utils.ts b/packages/router/utils.ts index c845d0285f..4bb9f48e1d 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -485,16 +485,14 @@ export function matchRoutes< let matches = null; for (let i = 0; matches == null && i < branches.length; ++i) { - matches = matchRouteBranch( - branches[i], - // 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) - ); + // 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. + let decoded = decodePath(pathname); + matches = matchRouteBranch(branches[i], decoded); } return matches; @@ -930,7 +928,7 @@ export function matchPath< if (isOptional && !value) { memo[paramName] = undefined; } else { - memo[paramName] = safelyDecodeURIComponent(value || "", paramName); + memo[paramName] = (value || "").replace(/%2F/g, "/"); } return memo; }, @@ -1002,9 +1000,12 @@ function compilePath( return [matcher, params]; } -function safelyDecodeURI(value: string) { +function decodePath(value: string) { try { - return decodeURI(value); + return value + .split("/") + .map((v) => decodeURIComponent(v).replace(/\//g, "%2F")) + .join("/"); } catch (error) { warning( false, @@ -1017,21 +1018,6 @@ function safelyDecodeURI(value: string) { } } -function safelyDecodeURIComponent(value: string, paramName: string) { - try { - return decodeURIComponent(value); - } catch (error) { - warning( - false, - `The value for the URL param "${paramName}" will not be decoded because` + - ` the string "${value}" is a malformed URL segment. This is probably` + - ` due to a bad percent encoding (${error}).` - ); - - return value; - } -} - /** * @private */