From 44400907dc6f94e8836f6670207e777cf44f6da2 Mon Sep 17 00:00:00 2001 From: Daniil Samoylov Date: Thu, 9 Feb 2023 23:43:25 +1300 Subject: [PATCH 1/3] fix: generatePath incorrectly applying parameters #9051 generatePath was doing multiple passes on the `path` using string replace, the first two passes were applying parameters, the third pass was doing a cleanup and the fourth path was applying the `splat`. It was possible to get incorrect results while applying `splat` when the last parameter value ended with `*`: ```ts const path = generatePath("/route/:name", { name: encodeURIComponent("includes *asterisk at the end*"), }) ``` ``` Expected: "/route/includes *asterisk at the end*" Received: "/route/includes *asterisk at the end" ``` results of the first two passes return the value of `/route/*asterisk at the end*` which was later treated as path with the splat resulting in the last asterisk removed. it was also possible to inject the splat value unintentionally ```ts generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" }) ``` ``` Expected: "/courses/foo*" Received: "/courses/foosplat_should_not_be_added" ``` A safer option, instead of mutating a global path multiple times, is to split the path into segments, process each segment in isolation and then join them back together. fixes #9051 --- .changeset/old-camels-accept.md | 5 ++ contributors.yml | 1 + .../__tests__/generatePath-test.tsx | 14 ++++ .../react-router/__tests__/matchPath-test.tsx | 8 ++ packages/router/utils.ts | 77 +++++++++---------- 5 files changed, 65 insertions(+), 40 deletions(-) create mode 100644 .changeset/old-camels-accept.md diff --git a/.changeset/old-camels-accept.md b/.changeset/old-camels-accept.md new file mode 100644 index 0000000000..a809412ad3 --- /dev/null +++ b/.changeset/old-camels-accept.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Fix `generatePath` incorrectly applying parameters in some cases diff --git a/contributors.yml b/contributors.yml index adb8a0746c..bb1c8117fd 100644 --- a/contributors.yml +++ b/contributors.yml @@ -130,6 +130,7 @@ - ms10596 - ned-park - noisypigeon +- Obi-Dann - omar-moquete - p13i - parched diff --git a/packages/react-router/__tests__/generatePath-test.tsx b/packages/react-router/__tests__/generatePath-test.tsx index 26f036c3e0..babc21834c 100644 --- a/packages/react-router/__tests__/generatePath-test.tsx +++ b/packages/react-router/__tests__/generatePath-test.tsx @@ -27,6 +27,20 @@ describe("generatePath", () => { ); expect(generatePath("/*", {})).toBe("/"); }); + it("handles * in parameter values", () => { + expect(generatePath("/courses/:name", { name: "foo*" })).toBe( + "/courses/foo*" + ); + expect(generatePath("/courses/:name", { name: "*foo" })).toBe( + "/courses/*foo" + ); + expect(generatePath("/courses/:name", { name: "*f*oo*" })).toBe( + "/courses/*f*oo*" + ); + expect(generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })).toBe( + "/courses/foo*" + ); + }); }); describe("with extraneous params", () => { diff --git a/packages/react-router/__tests__/matchPath-test.tsx b/packages/react-router/__tests__/matchPath-test.tsx index e36f73ed8a..337734e0e4 100644 --- a/packages/react-router/__tests__/matchPath-test.tsx +++ b/packages/react-router/__tests__/matchPath-test.tsx @@ -300,6 +300,14 @@ describe("matchPath *", () => { pathnameBase: "/", }); }); + + it("resolves params containing '*' correctly", () => { + expect(matchPath("/users/:name/*", "/users/foo*/splat")).toMatchObject({ + params: { name: "foo*", "*": "splat" }, + pathname: "/users/foo*/splat", + pathnameBase: "/users/foo*", + }); + }); }); describe("matchPath warnings", () => { diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 172d52c36a..7f2d1acfde 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -621,7 +621,7 @@ export function generatePath( [key in PathParam]: string | null; } = {} as any ): string { - let path = originalPath; + let path: string = originalPath; if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) { warning( false, @@ -633,49 +633,46 @@ export function generatePath( path = path.replace(/\*$/, "/*") as Path; } - return ( - path - .replace( - /^:(\w+)(\??)/g, - (_, key: PathParam, optional: string | undefined) => { - let param = params[key]; - if (optional === "?") { - return param == null ? "" : param; - } - if (param == null) { - invariant(false, `Missing ":${key}" param`); - } - return param; - } - ) - .replace( - /\/:(\w+)(\??)/g, - (_, key: PathParam, optional: string | undefined) => { - let param = params[key]; - if (optional === "?") { - return param == null ? "" : `/${param}`; - } - if (param == null) { - invariant(false, `Missing ":${key}" param`); - } - return `/${param}`; - } - ) - // Remove any optional markers from optional static segments - .replace(/\?/g, "") - .replace(/(\/?)\*/, (_, prefix, __, str) => { + // ensure `/` is added at the beginning if the path is absolute + const prefix = path.startsWith("/") ? "/" : ""; + + const segments = path + .split(/\/+/) + .map((segment, index, array) => { + const isLastSegment = index === array.length - 1; + + // only apply the splat if it's the last segment + if (isLastSegment && segment === "*") { const star = "*" as PathParam; + const starParam = params[star]; - if (params[star] == null) { - // If no splat was provided, trim the trailing slash _unless_ it's - // the entire path - return str === "/*" ? "/" : ""; + // Apply the splat + return starParam; + } + + const keyMatch = segment.match(/^:(\w+)(\??)$/); + if (keyMatch) { + const [, key, optional] = keyMatch; + let param = params[key as PathParam]; + + if (optional === "?") { + return param == null ? "" : param; } - // Apply the splat - return `${prefix}${params[star]}`; - }) - ); + if (param == null) { + invariant(false, `Missing ":${key}" param`); + } + + return param; + } + + // Remove any optional markers from optional static segments + return segment.replace(/\?$/g, ""); + }) + // Remove empty segments + .filter((segment) => !!segment); + + return prefix + segments.join("/"); } /** From eef790c8b039ab95c6ea1ce8e61d596f04bb3975 Mon Sep 17 00:00:00 2001 From: Daniil Samoylov Date: Thu, 9 Feb 2023 23:58:33 +1300 Subject: [PATCH 2/3] fix: incorrect typings for PathParam when path is `/*` paramName for the splat wasn't correctly resolved when the pas is `/*` --- packages/router/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 7f2d1acfde..34557263d4 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -217,7 +217,7 @@ type _PathParam = */ type PathParam = // check if path is just a wildcard - Path extends "*" + Path extends "*" | "/*" ? "*" : // look for wildcard at the end of the path Path extends `${infer Rest}/*` From 3e63bd0deb90bf43a51bbee829cda1d7a69b405f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 3 Mar 2023 16:25:08 -0500 Subject: [PATCH 3/3] Add @remix-run/router to changeset --- .changeset/old-camels-accept.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/old-camels-accept.md b/.changeset/old-camels-accept.md index a809412ad3..1728d6dcf8 100644 --- a/.changeset/old-camels-accept.md +++ b/.changeset/old-camels-accept.md @@ -1,4 +1,5 @@ --- +"@remix-run/router": patch "react-router": patch ---