fix: properly handle external redirects

.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,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", () => {
@@ -6822,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
@@ -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([
@@ -11419,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` ' +
@@ -11434,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);

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}"`;
     }

packages/router/utils.ts

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