diff --git a/.changeset/rare-dancers-shave.md b/.changeset/rare-dancers-shave.md new file mode 100644 index 0000000000..656afc7979 --- /dev/null +++ b/.changeset/rare-dancers-shave.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix 404 bug with same-origin absolute redirects diff --git a/packages/router/__tests__/router-memory-test.ts b/packages/router/__tests__/router-memory-test.ts index f4d133c518..721d6f7188 100644 --- a/packages/router/__tests__/router-memory-test.ts +++ b/packages/router/__tests__/router-memory-test.ts @@ -105,4 +105,76 @@ describe("a memory router", () => { ); router.dispose(); }); + + it("properly handles same-origin absolute URLs", async () => { + let router = createRouter({ + routes: [ + { + path: "/", + children: [ + { + index: true, + }, + { + path: "a", + loader: () => + new Response(null, { + status: 302, + headers: { + Location: "http://localhost/b", + }, + }), + }, + { + path: "b", + }, + ], + }, + ], + history: createMemoryHistory(), + }); + + await router.navigate("/a"); + expect(router.state.location).toMatchObject({ + hash: "", + pathname: "/b", + search: "", + }); + }); + + it("properly handles protocol-less same-origin absolute URLs", async () => { + let router = createRouter({ + routes: [ + { + path: "/", + children: [ + { + index: true, + }, + { + path: "a", + loader: () => + new Response(null, { + status: 302, + headers: { + Location: "//localhost/b", + }, + }), + }, + { + path: "b", + }, + ], + }, + ], + history: createMemoryHistory(), + }); + + await router.navigate("/a"); + expect(router.state.location).toMatchObject({ + hash: "", + pathname: "/b", + search: "", + }); + }); }); diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 847312f7c3..14167f5d5e 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6223,6 +6223,31 @@ describe("a router", () => { } }); + it("properly handles same-origin absolute URLs", async () => { + let t = setup({ routes: REDIRECT_ROUTES }); + + let A = await t.navigate("/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + let B = await A.actions.child.redirectReturn( + "http://localhost/parent", + undefined, + undefined, + ["parent"] + ); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state.location).toMatchObject({ + hash: "", + pathname: "/parent", + search: "", + state: { + _isRedirect: true, + }, + }); + }); + describe("redirect status code handling", () => { it("should not treat 300 as a redirect", async () => { let t = setup({ routes: REDIRECT_ROUTES }); @@ -11293,6 +11318,20 @@ describe("a router", () => { }, ]; + // Regardless of if the URL is internal or external - all absolute URL + // responses should return untouched during SSR so the browser can handle + // them + let ABSOLUTE_URLS = [ + "http://localhost/", + "https://localhost/about", + "http://remix.run/blog", + "https://remix.run/blog", + "//remix.run/blog", + "app://whatever", + "mailto:hello@remix.run", + "web+remix:whatever", + ]; + function createRequest(path: string, opts?: RequestInit) { return new Request(`http://localhost${path}`, { signal: new AbortController().signal, @@ -11616,17 +11655,8 @@ 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", - "mailto:hello@remix.run", - "web+remix:whatever", - ]; - - for (let url of urls) { + it("should handle absolute redirect Responses", async () => { + for (let url of ABSOLUTE_URLS) { let handler = createStaticHandler([ { path: "/", @@ -12954,15 +12984,8 @@ 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) { + it("should handle absolute redirect Responses", async () => { + for (let url of ABSOLUTE_URLS) { let handler = createStaticHandler([ { id: "root", diff --git a/packages/router/router.ts b/packages/router/router.ts index a30160096d..0e0b668f9a 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1901,7 +1901,7 @@ export function createRouter(init: RouterInit): Router { ); // Check if this an external redirect that goes to a new origin - if (typeof window?.location !== "undefined") { + if (isBrowser && typeof window?.location !== "undefined") { let newOrigin = init.history.createURL(redirect.location).origin; if (window.location.origin !== newOrigin) { if (replace) { @@ -3115,6 +3115,17 @@ async function callLoaderOrAction( } location = createPath(resolvedLocation); + } else if (!isStaticRequest) { + // Strip off the protocol+origin for same-origin absolute redirects. + // If this is a static reques, we can let it go back to the browser + // as-is + let currentUrl = new URL(request.url); + let url = location.startsWith("//") + ? new URL(currentUrl.protocol + location) + : new URL(location); + if (url.origin === currentUrl.origin) { + location = url.pathname + url.search + url.hash; + } } // Don't process redirects in the router during static requests requests.