fix: properly handle external redirects (#9590)

* fix: properly handle external redirects
* Handle missing loader with 400 instead of 405

Author: brophdawg11

.changeset/flat-trainers-speak.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+properly handle redirects to external domains

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
@@ -10312,6 +10343,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"));
@@ -11311,6 +11364,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([
@@ -11466,13 +11542,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` ' +
@@ -11481,7 +11557,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);

packages/router/router.ts

@@ -1596,6 +1596,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;
@@ -2185,7 +2195,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,
@@ -2578,26 +2588,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
@@ -2613,6 +2628,7 @@ async function callLoaderOrAction(
         status,
         location,
         revalidate: result.headers.get("X-Remix-Revalidate") !== null,
+        external,
       };
     }
 
@@ -2921,7 +2937,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}"`;
@@ -2931,17 +2954,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}"`;
     }

packages/router/utils.ts

@@ -41,6 +41,7 @@ export interface RedirectResult {
   status: number;
   location: string;
   revalidate: boolean;
+  external: boolean;
 }
 
 /**