From 0a7287ae46f3333f245bcf65db24a0a139c5e1b4 Mon Sep 17 00:00:00 2001 From: Ahmed Amsal Date: Thu, 9 Feb 2023 02:46:57 +0500 Subject: [PATCH 1/5] fix(router): "hard" redirect to different paths on the same origin if redirect location does not contain basename --- packages/router/router.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 874be611d6..355554e1d6 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1915,12 +1915,16 @@ export function createRouter(init: RouterInit): Router { // Check if this an absolute external redirect that goes to a new origin if ( - ABSOLUTE_URL_REGEX.test(redirect.location) && + (ABSOLUTE_URL_REGEX.test(redirect.location) || (init.basename !== undefined && init.basename !== "/")) && isBrowser && typeof window?.location !== "undefined" ) { let newOrigin = init.history.createURL(redirect.location).origin; - if (window.location.origin !== newOrigin) { + + // Only the last trailing slash is replaced + const BASENAME_URL_REGEX = new RegExp("^" + init.basename?.replace(/\/$/, '') + "([/|?.*|?.#]|$)", 'i'); + + if (window.location.origin !== newOrigin || !BASENAME_URL_REGEX.test(redirect.location)) { if (replace) { window.location.replace(redirect.location); } else { From b2cc4322178b46f526b18223f8e968e9a61b2120 Mon Sep 17 00:00:00 2001 From: Ahmed Amsal Date: Thu, 9 Feb 2023 02:47:04 +0500 Subject: [PATCH 2/5] Sign CLA --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index adb8a0746c..3ada0d6b96 100644 --- a/contributors.yml +++ b/contributors.yml @@ -9,6 +9,7 @@ - alany411 - alexlbr - AmRo045 +- amsal - andreiduca - arnassavickas - aroyan From 63f9d8b495ff0c8deac7f922afcdc378ebb9bbe5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 22 Feb 2023 11:33:49 -0500 Subject: [PATCH 3/5] Add tests --- packages/router/__tests__/router-test.ts | 58 ++++++++++++++++++++++++ packages/router/router.ts | 22 ++++----- 2 files changed, 69 insertions(+), 11 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 05181d86d2..9d94824d7a 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -630,6 +630,10 @@ function setup({ let popHref = history.createHref(history.location); if (currentRouter?.basename) { popHref = stripBasename(popHref, currentRouter.basename) as string; + invariant( + popHref, + "href passed to navigate should start with basename" + ); } helpers = getNavigationHelpers(popHref, navigationId); unsubscribe(); @@ -645,6 +649,10 @@ function setup({ let navHref = href; if (currentRouter.basename) { navHref = stripBasename(navHref, currentRouter.basename) as string; + invariant( + navHref, + "href passed to t.navigate() should start with basename" + ); } helpers = getNavigationHelpers(navHref, navigationId); shims?.forEach((routeId) => @@ -6293,6 +6301,56 @@ describe("a router", () => { }); }); + it("properly handles same-origin absolute URLs when using a basename", async () => { + let t = setup({ routes: REDIRECT_ROUTES, basename: "/base" }); + + let A = await t.navigate("/base/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + let B = await A.actions.child.redirectReturn( + "http://localhost/base/parent", + undefined, + undefined, + ["parent"] + ); + await B.loaders.parent.resolve("PARENT"); + expect(t.router.state.location).toMatchObject({ + hash: "", + pathname: "/base/parent", + search: "", + state: { + _isRedirect: true, + }, + }); + }); + + it("treats same-origin absolute URLs as external if they don't match the basename", 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.assign = jest.fn(); + location.replace = jest.fn(); + delete (window as any).location; + window.location = location as unknown as Location; + + let t = setup({ routes: REDIRECT_ROUTES, basename: "/base" }); + + let A = await t.navigate("/base/parent/child", { + formMethod: "post", + formData: createFormData({}), + }); + + let url = "http://localhost/not/the/same/basename"; + await A.actions.child.redirectReturn(url); + expect(window.location.assign).toHaveBeenCalledWith(url); + expect(window.location.replace).not.toHaveBeenCalled(); + + window.location = oldLocation; + }); + describe("redirect status code handling", () => { it("should not treat 300 as a redirect", async () => { let t = setup({ routes: REDIRECT_ROUTES }); diff --git a/packages/router/router.ts b/packages/router/router.ts index e0e093a002..9767a3f328 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -33,6 +33,7 @@ import { joinPaths, matchRoutes, resolveTo, + stripBasename, warning, } from "./utils"; @@ -1935,19 +1936,17 @@ export function createRouter(init: RouterInit): Router { redirectLocation, "Expected a location on the redirect navigation" ); - // Check if this an absolute external redirect that goes to a new origin if ( - (ABSOLUTE_URL_REGEX.test(redirect.location) || (init.basename !== undefined && init.basename !== "/")) && + ABSOLUTE_URL_REGEX.test(redirect.location) && isBrowser && typeof window?.location !== "undefined" ) { - let newOrigin = init.history.createURL(redirect.location).origin; - - // Only the last trailing slash is replaced - const BASENAME_URL_REGEX = new RegExp("^" + init.basename?.replace(/\/$/, '') + "([/|?.*|?.#]|$)", 'i'); + let url = init.history.createURL(redirect.location); + let isDifferentBasename = + stripBasename(url.pathname, init.basename || "/") == null; - if (window.location.origin !== newOrigin || !BASENAME_URL_REGEX.test(redirect.location)) { + if (window.location.origin !== url.origin || isDifferentBasename) { if (replace) { window.location.replace(redirect.location); } else { @@ -3177,14 +3176,15 @@ 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 + // Strip off the protocol+origin for same-origin + same-basename absolute + // redirects. If this is a static request, 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) { + let isSameBasename = stripBasename(url.pathname, basename) != null; + if (url.origin === currentUrl.origin && isSameBasename) { location = url.pathname + url.search + url.hash; } } From 381d59f4515b0efeabb488459bff1e14d075af33 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 22 Feb 2023 11:34:53 -0500 Subject: [PATCH 4/5] Add changeset --- .changeset/wet-dogs-check.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/wet-dogs-check.md diff --git a/.changeset/wet-dogs-check.md b/.changeset/wet-dogs-check.md new file mode 100644 index 0000000000..4bf4f7e447 --- /dev/null +++ b/.changeset/wet-dogs-check.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Correctly perform a "hard" redirect for same-origin absolute URLs outside of the router basename From d996f1e8c95b06e62f4cf13256b477e4ac4d0377 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 22 Feb 2023 11:39:28 -0500 Subject: [PATCH 5/5] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 607528bd9b..446186dc2d 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "41.6 kB" + "none": "41.7 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13 kB"