From 55142600c3a3212acadec2d909428e29d97b3be1 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 29 Sep 2022 18:51:11 -0400 Subject: [PATCH 1/6] fix: detect and error on invalid pathnames --- .../__tests__/useNavigate-test.tsx | 76 +++++++++++++++++++ packages/router/utils.ts | 27 ++++++- 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx index e51313c881..16309ce079 100644 --- a/packages/react-router/__tests__/useNavigate-test.tsx +++ b/packages/react-router/__tests__/useNavigate-test.tsx @@ -164,6 +164,82 @@ describe("useNavigate", () => { `); }); + it("throws on invalid destination path objects", () => { + function Home() { + let navigate = useNavigate(); + + return ( +
+

Home

+ + + + +
+ ); + } + + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + } /> + + + ); + }); + + expect(() => + TestRenderer.act(() => { + renderer.root.findAllByType("button")[0].props.onClick(); + }) + ).toThrowErrorMatchingInlineSnapshot( + `"Cannot include a '?' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + ); + + expect(() => + TestRenderer.act(() => { + renderer.root.findAllByType("button")[1].props.onClick(); + }) + ).toThrowErrorMatchingInlineSnapshot( + `"Cannot include a '#' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + ); + + expect(() => + TestRenderer.act(() => { + renderer.root.findAllByType("button")[2].props.onClick(); + }) + ).toThrowErrorMatchingInlineSnapshot( + `"Cannot include a '?' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + ); + + expect(() => + TestRenderer.act(() => { + renderer.root.findAllByType("button")[3].props.onClick(); + }) + ).toThrowErrorMatchingInlineSnapshot( + `"Cannot include a '#' character in a manually specified \`to.search\` field - please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + ); + }); + describe("with state", () => { it("adds the state to location.state", () => { function Home() { diff --git a/packages/router/utils.ts b/packages/router/utils.ts index cb3e89b85d..6651894883 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -777,7 +777,32 @@ export function resolveTo( locationPathname: string, isPathRelative = false ): Path { - let to = typeof toArg === "string" ? parsePath(toArg) : { ...toArg }; + let to: Partial; + if (typeof toArg === "string") { + to = parsePath(toArg); + } else { + to = { ...toArg }; + + let pathError = (char: string, field: string, dest: string) => + `Cannot include a '${char}' character in a manually specified ` + + `\`to.${field}\` field - please separate it out to the \`to.${dest}\` ` + + `field. Alternatively you may provide the full path as a string and the ` + + `router will parse it for you`; + + invariant( + to.pathname && !to.pathname.includes("?"), + pathError("?", "pathname", "search") + ); + invariant( + to.pathname && !to.pathname.includes("#"), + pathError("#", "pathname", "hash") + ); + invariant( + to.search && !to.search.includes("#"), + pathError("#", "search", "hash") + ); + } + let isEmptyPath = toArg === "" || to.pathname === ""; let toPathname = isEmptyPath ? "/" : to.pathname; From 72621e21eb0652211f7bc36470340f1d718805de Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 29 Sep 2022 18:57:33 -0400 Subject: [PATCH 2/6] Enhance error message --- .../__tests__/useNavigate-test.tsx | 8 +++--- packages/router/utils.ts | 27 ++++++++++++------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx index 16309ce079..4890be64bd 100644 --- a/packages/react-router/__tests__/useNavigate-test.tsx +++ b/packages/react-router/__tests__/useNavigate-test.tsx @@ -212,7 +212,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[0].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '?' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` ); expect(() => @@ -220,7 +220,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[1].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '#' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + `"Cannot include a '#' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` ); expect(() => @@ -228,7 +228,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[2].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '?' character in a manually specified \`to.pathname\` field - please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search#hash\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` ); expect(() => @@ -236,7 +236,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[3].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '#' character in a manually specified \`to.search\` field - please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you"` + `"Cannot include a '#' character in a manually specified \`to.search\` field [{\\"pathname\\":\\"/about/thing\\",\\"search\\":\\"?search#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` ); }); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 6651894883..ddad94db5b 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -783,23 +783,30 @@ export function resolveTo( } else { to = { ...toArg }; - let pathError = (char: string, field: string, dest: string) => + let pathError = ( + char: string, + field: string, + dest: string, + path: Partial + ) => `Cannot include a '${char}' character in a manually specified ` + - `\`to.${field}\` field - please separate it out to the \`to.${dest}\` ` + - `field. Alternatively you may provide the full path as a string and the ` + - `router will parse it for you`; + `\`to.${field}\` field [${JSON.stringify( + path + )}]. Please separate it out to the ` + + `\`to.${dest}\` field. Alternatively you may provide the full path as ` + + `a string and the router will parse it for you.`; invariant( - to.pathname && !to.pathname.includes("?"), - pathError("?", "pathname", "search") + !to.pathname || !to.pathname.includes("?"), + pathError("?", "pathname", "search", to) ); invariant( - to.pathname && !to.pathname.includes("#"), - pathError("#", "pathname", "hash") + !to.pathname || !to.pathname.includes("#"), + pathError("#", "pathname", "hash", to) ); invariant( - to.search && !to.search.includes("#"), - pathError("#", "search", "hash") + !to.search || !to.search.includes("#"), + pathError("#", "search", "hash", to) ); } From 0d252751c42312c5fe4cfe78edbdc76d1ce39f89 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 29 Sep 2022 18:58:34 -0400 Subject: [PATCH 3/6] add changeset --- .changeset/dirty-yaks-repair.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/dirty-yaks-repair.md diff --git a/.changeset/dirty-yaks-repair.md b/.changeset/dirty-yaks-repair.md new file mode 100644 index 0000000000..b2896efa27 --- /dev/null +++ b/.changeset/dirty-yaks-repair.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"@remix-run/router": patch +--- + +fix: throw error when receiving invalid path object (#9375) From 03fdc85cfdb76c960fce630532f01aeee0cd216d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 29 Sep 2022 19:00:08 -0400 Subject: [PATCH 4/6] Extract function --- packages/router/utils.ts | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/packages/router/utils.ts b/packages/router/utils.ts index ddad94db5b..9474b42034 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -768,6 +768,22 @@ function resolvePathname(relativePath: string, fromPathname: string): string { return segments.length > 1 ? segments.join("/") : "/"; } +function getInvalidPathError( + char: string, + field: string, + dest: string, + path: Partial +) { + return ( + `Cannot include a '${char}' character in a manually specified ` + + `\`to.${field}\` field [${JSON.stringify( + path + )}]. Please separate it out to the ` + + `\`to.${dest}\` field. Alternatively you may provide the full path as ` + + `a string and the router will parse it for you.` + ); +} + /** * @private */ @@ -783,30 +799,17 @@ export function resolveTo( } else { to = { ...toArg }; - let pathError = ( - char: string, - field: string, - dest: string, - path: Partial - ) => - `Cannot include a '${char}' character in a manually specified ` + - `\`to.${field}\` field [${JSON.stringify( - path - )}]. Please separate it out to the ` + - `\`to.${dest}\` field. Alternatively you may provide the full path as ` + - `a string and the router will parse it for you.`; - invariant( !to.pathname || !to.pathname.includes("?"), - pathError("?", "pathname", "search", to) + getInvalidPathError("?", "pathname", "search", to) ); invariant( !to.pathname || !to.pathname.includes("#"), - pathError("#", "pathname", "hash", to) + getInvalidPathError("#", "pathname", "hash", to) ); invariant( !to.search || !to.search.includes("#"), - pathError("#", "search", "hash", to) + getInvalidPathError("#", "search", "hash", to) ); } From 716098ade65662bf0fdef2879de6f617d55a56e0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 30 Sep 2022 10:48:53 -0400 Subject: [PATCH 5/6] Update warning message Co-authored-by: Michael Jackson --- 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 9474b42034..9b593cd1bf 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -780,7 +780,7 @@ function getInvalidPathError( path )}]. Please separate it out to the ` + `\`to.${dest}\` field. Alternatively you may provide the full path as ` + - `a string and the router will parse it for you.` + `a string in and the router will parse it for you.` ); } From 16291bf94f816c5a39c4488d3b2202b5ffb3475f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 30 Sep 2022 10:55:23 -0400 Subject: [PATCH 6/6] fix test error message --- packages/react-router/__tests__/useNavigate-test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx index 4890be64bd..6e0dc55006 100644 --- a/packages/react-router/__tests__/useNavigate-test.tsx +++ b/packages/react-router/__tests__/useNavigate-test.tsx @@ -212,7 +212,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[0].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` + `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string in and the router will parse it for you."` ); expect(() => @@ -220,7 +220,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[1].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '#' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` + `"Cannot include a '#' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string in and the router will parse it for you."` ); expect(() => @@ -228,7 +228,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[2].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search#hash\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` + `"Cannot include a '?' character in a manually specified \`to.pathname\` field [{\\"pathname\\":\\"/about/thing?search#hash\\"}]. Please separate it out to the \`to.search\` field. Alternatively you may provide the full path as a string in and the router will parse it for you."` ); expect(() => @@ -236,7 +236,7 @@ describe("useNavigate", () => { renderer.root.findAllByType("button")[3].props.onClick(); }) ).toThrowErrorMatchingInlineSnapshot( - `"Cannot include a '#' character in a manually specified \`to.search\` field [{\\"pathname\\":\\"/about/thing\\",\\"search\\":\\"?search#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string and the router will parse it for you."` + `"Cannot include a '#' character in a manually specified \`to.search\` field [{\\"pathname\\":\\"/about/thing\\",\\"search\\":\\"?search#hash\\"}]. Please separate it out to the \`to.hash\` field. Alternatively you may provide the full path as a string in and the router will parse it for you."` ); });