From 2566ef43fbdf1441e6d31a105cd551e86646b17e Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 14 Nov 2022 15:02:20 -0500 Subject: [PATCH 1/5] fix: properly handle external redirects --- packages/router/__tests__/router-test.ts | 56 +++++++++++++++++++++++- packages/router/router.ts | 52 ++++++++++++++-------- packages/router/utils.ts | 1 + 3 files changed, 89 insertions(+), 20 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 0b19d09eaf..f89627d409 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -5433,7 +5433,7 @@ describe("a router", () => { it("preserves query and hash in redirects", async () => { let t = setup({ routes: REDIRECT_ROUTES }); - let nav1 = await t.fetch("/parent/child", { + let nav1 = await t.navigate("/parent/child", { formMethod: "post", formData: createFormData({}), }); @@ -5459,7 +5459,7 @@ describe("a router", () => { it("preserves query and hash in relative redirects", async () => { let t = setup({ routes: REDIRECT_ROUTES }); - let nav1 = await t.fetch("/parent/child", { + let nav1 = await t.navigate("/parent/child", { formMethod: "post", formData: createFormData({}), }); @@ -5484,6 +5484,30 @@ describe("a router", () => { errors: null, }); }); + + it("processes external redirects if window is present", async () => { + // This is gross, don't blame me, blame SO :) + // https://stackoverflow.com/a/60697570 + let oldLocation = window.location; + const location = new URL(window.location.href) as unknown as Location; + location.replace = jest.fn(); + delete (window as any).location; + window.location = location as unknown as Location; + + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + await A.actions.child.redirectReturn("http://remix.run/blog"); + expect(window.location.replace).toHaveBeenCalledWith( + "http://remix.run/blog" + ); + + window.location = oldLocation; + }); }); describe("scroll restoration", () => { @@ -10297,6 +10321,18 @@ describe("a router", () => { expect((response as Response).headers.get("Location")).toBe("/parent"); }); + it("should handle external redirect Responses", async () => { + let { query } = createStaticHandler([ + { path: "/", loader: () => redirect("http://remix.run/blog") }, + ]); + let response = await query(createRequest("/")); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe( + "http://remix.run/blog" + ); + }); + it("should handle 404 navigations", async () => { let { query } = createStaticHandler(SSR_ROUTES); let context = await query(createRequest("/not/found")); @@ -11264,6 +11300,22 @@ describe("a router", () => { expect((response as Response).headers.get("Location")).toBe("/parent"); }); + it("should handle external redirect Responses", async () => { + let { queryRoute } = createStaticHandler([ + { + id: "root", + path: "/", + loader: () => redirect("http://remix.run/blog"), + }, + ]); + let response = await queryRoute(createRequest("/"), "root"); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe( + "http://remix.run/blog" + ); + }); + it("should not unwrap responses returned from loaders", async () => { let response = json({ key: "value" }); let { queryRoute } = createStaticHandler([ diff --git a/packages/router/router.ts b/packages/router/router.ts index 51e02187de..c4ed04a9d3 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1582,6 +1582,16 @@ export function createRouter(init: RouterInit): Router { navigation.location, "Expected a location on the redirect navigation" ); + + if ( + redirect.external && + typeof window !== "undefined" && + typeof window.location !== "undefined" + ) { + window.location.replace(redirect.location); + return; + } + // There's no need to abort on redirects, since we don't detect the // redirect until the action/loaders have settled pendingNavigationController = null; @@ -2554,26 +2564,31 @@ async function callLoaderOrAction( "Redirects returned/thrown from loaders/actions must have a Location header" ); - // Support relative routing in redirects - let activeMatches = matches.slice(0, matches.indexOf(match) + 1); - let routePathnames = getPathContributingMatches(activeMatches).map( - (match) => match.pathnameBase - ); - let requestPath = createURL(request.url).pathname; - let resolvedLocation = resolveTo(location, routePathnames, requestPath); - invariant( - createPath(resolvedLocation), - `Unable to resolve redirect location: ${result.headers.get("Location")}` - ); + // Check if this an external redirect that goes to a new origin + let external = createURL(location).origin !== createURL("/").origin; - // Prepend the basename to the redirect location if we have one - if (basename) { - let path = resolvedLocation.pathname; - resolvedLocation.pathname = - path === "/" ? basename : joinPaths([basename, path]); - } + // Support relative routing in internal redirects + if (!external) { + let activeMatches = matches.slice(0, matches.indexOf(match) + 1); + let routePathnames = getPathContributingMatches(activeMatches).map( + (match) => match.pathnameBase + ); + let requestPath = createURL(request.url).pathname; + let resolvedLocation = resolveTo(location, routePathnames, requestPath); + invariant( + createPath(resolvedLocation), + `Unable to resolve redirect location: ${location}` + ); - location = createPath(resolvedLocation); + // Prepend the basename to the redirect location if we have one + if (basename) { + let path = resolvedLocation.pathname; + resolvedLocation.pathname = + path === "/" ? basename : joinPaths([basename, path]); + } + + location = createPath(resolvedLocation); + } // Don't process redirects in the router during static requests requests. // Instead, throw the Response and let the server handle it with an HTTP @@ -2589,6 +2604,7 @@ async function callLoaderOrAction( status, location, revalidate: result.headers.get("X-Remix-Revalidate") !== null, + external, }; } diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 5628fd7f45..007605d374 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -41,6 +41,7 @@ export interface RedirectResult { status: number; location: string; revalidate: boolean; + external: boolean; } /** From 38fed46270e8d4ec10e6290dfd808537c3f55029 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 14 Nov 2022 15:05:36 -0500 Subject: [PATCH 2/5] add changeset --- .changeset/flat-trainers-speak.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/flat-trainers-speak.md diff --git a/.changeset/flat-trainers-speak.md b/.changeset/flat-trainers-speak.md new file mode 100644 index 0000000000..c6bcb41f3a --- /dev/null +++ b/.changeset/flat-trainers-speak.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +properly handle redirects to external domains From 882f4bd63397bed89dd09540b34886fb67a16e75 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 15 Nov 2022 16:32:26 -0500 Subject: [PATCH 3/5] Handle missing loader with 400 instead of 405 --- packages/router/__tests__/router-test.ts | 8 +++++--- packages/router/router.ts | 26 ++++++++++++------------ 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index f89627d409..b79a9ea976 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6846,7 +6846,7 @@ describe("a router", () => { 405, "Method Not Allowed", new Error( - 'You made a post request to "/" but did not provide a `loader` ' + + 'You made a post request to "/" but did not provide an `action` ' + 'for route "root", so there is no way to handle the request.' ), true @@ -11471,13 +11471,13 @@ describe("a router", () => { } }); - it("should handle not found action/loader submissions with a 405 Response", async () => { + it("should handle missing loaders with a 400 Response", async () => { try { await queryRoute(createRequest("/"), "root"); expect(false).toBe(true); } catch (data) { expect(isRouteErrorResponse(data)).toBe(true); - expect(data.status).toBe(405); + expect(data.status).toBe(400); expect(data.error).toEqual( new Error( 'You made a GET request to "/" but did not provide a `loader` ' + @@ -11486,7 +11486,9 @@ describe("a router", () => { ); expect(data.internal).toBe(true); } + }); + it("should handle missing actions with a 405 Response", async () => { try { await queryRoute(createSubmitRequest("/"), "root"); expect(false).toBe(true); diff --git a/packages/router/router.ts b/packages/router/router.ts index c4ed04a9d3..f54bf04362 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2171,7 +2171,7 @@ export function unstable_createStaticHandler( // Short circuit if we have no loaders to run (queryRoute()) if (isRouteRequest && !routeMatch?.route.loader) { - throw getInternalRouterError(405, { + throw getInternalRouterError(400, { method: request.method, pathname: createURL(request.url).pathname, routeId: routeMatch?.route.id, @@ -2913,7 +2913,14 @@ function getInternalRouterError( if (status === 400) { statusText = "Bad Request"; - errorMessage = "Cannot submit binary form data using GET"; + if (method && pathname && routeId) { + errorMessage = + `You made a ${method} request to "${pathname}" but ` + + `did not provide a \`loader\` for route "${routeId}", ` + + `so there is no way to handle the request.`; + } else { + errorMessage = "Cannot submit binary form data using GET"; + } } else if (status === 403) { statusText = "Forbidden"; errorMessage = `Route "${routeId}" does not match URL "${pathname}"`; @@ -2923,17 +2930,10 @@ function getInternalRouterError( } else if (status === 405) { statusText = "Method Not Allowed"; if (method && pathname && routeId) { - if (validActionMethods.has(method)) { - errorMessage = - `You made a ${method} request to "${pathname}" but ` + - `did not provide an \`action\` for route "${routeId}", ` + - `so there is no way to handle the request.`; - } else { - errorMessage = - `You made a ${method} request to "${pathname}" but ` + - `did not provide a \`loader\` for route "${routeId}", ` + - `so there is no way to handle the request.`; - } + errorMessage = + `You made a ${method} request to "${pathname}" but ` + + `did not provide an \`action\` for route "${routeId}", ` + + `so there is no way to handle the request.`; } else { errorMessage = `Invalid request method "${method}"`; } From 5eff24170428095f2fb3df6a2b651ed80a40eb97 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 23 Nov 2022 15:27:43 -0500 Subject: [PATCH 4/5] Enhance tests --- packages/router/__tests__/router-test.ts | 102 ++++++++++++++--------- 1 file changed, 63 insertions(+), 39 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index b79a9ea976..9da997b588 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -5486,27 +5486,34 @@ describe("a router", () => { }); it("processes external redirects if window is present", async () => { - // This is gross, don't blame me, blame SO :) - // https://stackoverflow.com/a/60697570 - let oldLocation = window.location; - const location = new URL(window.location.href) as unknown as Location; - location.replace = jest.fn(); - delete (window as any).location; - window.location = location as unknown as Location; + let urls = [ + "http://remix.run/blog", + "https://remix.run/blog", + "//remix.run/blog", + "app://whatever", + ]; - let t = setup({ routes: REDIRECT_ROUTES }); + for (let url of urls) { + // This is gross, don't blame me, blame SO :) + // https://stackoverflow.com/a/60697570 + let oldLocation = window.location; + const location = new URL(window.location.href) as unknown as Location; + location.replace = jest.fn(); + delete (window as any).location; + window.location = location as unknown as Location; - let A = await t.navigate("/parent/child", { - formMethod: "post", - formData: createFormData({}), - }); + let t = setup({ routes: REDIRECT_ROUTES }); - await A.actions.child.redirectReturn("http://remix.run/blog"); - expect(window.location.replace).toHaveBeenCalledWith( - "http://remix.run/blog" - ); + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + await A.actions.child.redirectReturn(url); + expect(window.location.replace).toHaveBeenCalledWith(url); - window.location = oldLocation; + window.location = oldLocation; + } }); }); @@ -10322,15 +10329,25 @@ describe("a router", () => { }); it("should handle external redirect Responses", async () => { - let { query } = createStaticHandler([ - { path: "/", loader: () => redirect("http://remix.run/blog") }, - ]); - let response = await query(createRequest("/")); - expect(response instanceof Response).toBe(true); - expect((response as Response).status).toBe(302); - expect((response as Response).headers.get("Location")).toBe( - "http://remix.run/blog" - ); + let urls = [ + "http://remix.run/blog", + "https://remix.run/blog", + "//remix.run/blog", + "app://whatever", + ]; + + for (let url of urls) { + let handler = createStaticHandler([ + { + path: "/", + loader: () => redirect(url), + }, + ]); + let response = await handler.query(createRequest("/")); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe(url); + } }); it("should handle 404 navigations", async () => { @@ -11301,19 +11318,26 @@ describe("a router", () => { }); it("should handle external redirect Responses", async () => { - let { queryRoute } = createStaticHandler([ - { - id: "root", - path: "/", - loader: () => redirect("http://remix.run/blog"), - }, - ]); - let response = await queryRoute(createRequest("/"), "root"); - expect(response instanceof Response).toBe(true); - expect((response as Response).status).toBe(302); - expect((response as Response).headers.get("Location")).toBe( - "http://remix.run/blog" - ); + let urls = [ + "http://remix.run/blog", + "https://remix.run/blog", + "//remix.run/blog", + "app://whatever", + ]; + + for (let url of urls) { + let handler = createStaticHandler([ + { + id: "root", + path: "/", + loader: () => redirect(url), + }, + ]); + let response = await handler.queryRoute(createRequest("/"), "root"); + expect(response instanceof Response).toBe(true); + expect((response as Response).status).toBe(302); + expect((response as Response).headers.get("Location")).toBe(url); + } }); it("should not unwrap responses returned from loaders", async () => { From 21d58a2bf639c7ebf0e92dff068da5123c969533 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 23 Nov 2022 15:37:58 -0500 Subject: [PATCH 5/5] bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 46a937dd23..bbce7aa04e 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "34.5 kB" + "none": "35 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB"