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 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" diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 0b19d09eaf..9da997b588 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,37 @@ describe("a router", () => { errors: null, }); }); + + it("processes external redirects if window is present", async () => { + let urls = [ + "http://remix.run/blog", + "https://remix.run/blog", + "//remix.run/blog", + "app://whatever", + ]; + + 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 t = setup({ routes: REDIRECT_ROUTES }); + + 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; + } + }); }); describe("scroll restoration", () => { @@ -6822,7 +6853,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 @@ -10297,6 +10328,28 @@ describe("a router", () => { expect((response as Response).headers.get("Location")).toBe("/parent"); }); + it("should handle external redirect Responses", async () => { + 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 () => { let { query } = createStaticHandler(SSR_ROUTES); let context = await query(createRequest("/not/found")); @@ -11264,6 +11317,29 @@ describe("a router", () => { expect((response as Response).headers.get("Location")).toBe("/parent"); }); + it("should handle external redirect Responses", async () => { + 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 () => { let response = json({ key: "value" }); let { queryRoute } = createStaticHandler([ @@ -11419,13 +11495,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` ' + @@ -11434,7 +11510,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 51e02187de..f54bf04362 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; @@ -2161,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, @@ -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, }; } @@ -2897,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}"`; @@ -2907,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}"`; } 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; } /**