Fix absolute redirect detection #9689
Conversation
🦋 Changeset detectedLatest commit: 55776c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Check if this an external redirect that goes to a new origin | ||
| let currentUrl = new URL(request.url); | ||
| let currentOrigin = currentUrl.origin; | ||
| let newOrigin = new URL(location, currentOrigin).origin; |
There was a problem hiding this comment.
This was the problem - since the origin for a test://test.com URL used in Remix integration tests is null 😕 , and new URL(path, "null") throws an error.
| let newOrigin = new URL(location, currentOrigin).origin; | ||
| let external = newOrigin !== currentOrigin; | ||
| let isAbsolute = | ||
| /^[a-z+]+:\/\//i.test(location) || location.startsWith("//"); |
There was a problem hiding this comment.
Instead we'll just look for the presence of a leading protocol or a protocol-less URL to determine if we can skip relative routing logic
| if ( | ||
| redirect.external && | ||
| typeof window !== "undefined" && | ||
| typeof window.location !== "undefined" | ||
| ) { | ||
| if (replace) { | ||
| window.location.replace(redirect.location); | ||
| } else { | ||
| window.location.assign(redirect.location); | ||
| // Check if this an external redirect that goes to a new origin | ||
| if (typeof window?.location !== "undefined") { | ||
| let newOrigin = createClientSideURL(redirect.location).origin; | ||
| if (window.location.origin !== newOrigin) { | ||
| if (replace) { | ||
| window.location.replace(redirect.location); | ||
| } else { | ||
| window.location.assign(redirect.location); | ||
| } | ||
| return; | ||
| } | ||
| return; |
There was a problem hiding this comment.
Got rid of the redirect.external flag in favor of just doing the external detection here in client-side only code.
|
This is a follow up to the initial fix for remix-run/remix#4740 |
Ran into issues with our
URLcreation fix with Remix integration tests using atest://test.combase, so re-thought the approach a bit. We were sort of conflating 2 things - whether a redirect was external and whether it was absolute. In both cases we want to skip the relative redirect logic, but we do not need to check for external on the server since the browser will handle redirecting to the external domain. Instead, we can do a simplerisAbsolutecheck in shared code (noURLcreation required) and limit ourisExternalcheck to client side code where we can more reliably create URLs withwindow.location.originFollow up to #9682
Closes remix-run/remix#4740