Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly handle external redirects #9590

Merged
merged 5 commits into from
Nov 23, 2022

Conversation

brophdawg11
Copy link
Contributor

Properly handle external redirects from actions and loaders:

function loader() {
  if (condition) {
    return redirect("https://otherdomain.com/path");
  }
  ...
}

Client-side we detect and process these via window.location.replace like we do in Remix currently. In SSR scenarios we return the redirect location untouched.

This fixes an issue discovered while testing the "Remix on React Router 6.4" work (remix-run/remix#4570)

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2022

🦋 Changeset detected

Latest commit: 21d58a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

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

@@ -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", {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These didn't need to be fetches, that was just as copy/paste from a prior test

) {
window.location.replace(redirect.location);
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just navigate directly for external redirects instead of "starting" a new router navigation

path === "/" ? basename : joinPaths([basename, path]);
}
// Support relative routing in internal redirects
if (!external) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do any post-processing of the redirect location if it's an external location

Copy link
Contributor

@machour machour Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in the middle of migrating parts of a PHP website to remix, using nginx to route some urls to remix. In my very particular case, if I redirect() from Remix to a PHP page, the origin would be the same, and this check would fail 😬

Would testing for a scheme pattern (regex, or a loose check on ://) instead negatively impact perfs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good call. Is that something you can do in Remix today? Or does it have this same issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with a manual check, I think we'd need to check for both protocol-less URLs (//google.com/path) as well as maybe a loose protocol check for ://. Perf shouldn't be a concern since this isn't a particularly hot code path - O(n) where n is the # of loaders/actions being run on a given navigation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remix has the same issue today.

I'm not sure we want to support protocol-less urls, as far as I know, those are only meant to apply the current scheme in HTML.

MDN says relative or absolute, and RFC 9110 seems to require a : in absolute URIs.
But that's a long document and my nerdiness is failing me 😅

Also, if my http:// remix app, is behind a https:// nginx proxy, what scheme would //my-domain/page refer to? Doesn't seem that trivial from a server side perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok yeah I figured it should since I stole the approach from there ;)

I haven't dug into the spec, but was more considering this from a browser-emulator perspective. I did some quick testing and protocol-less URLs work in both of the redirect mechanisms at play here - neither of which are truly "handled" by RR/Remix - we're just handing off a location provided by the user to a built-in browser mechanism.

Document redirects - returning a 301 with Location: //google.com works
Client-side redirects - window.location.replace('//google.com') works

So I don't think we need to implement the logic on how to handle the redirect, so much as decide when to hand off the Location provided by the user directly to the browser instead of assuming it's a path inside our app. If the browser is doing something not-technically-spec-compliant, we might be able to leave that as a browser concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@machour Do you mind creating a separate proposal for "same domain hard redirects"? I'm going to merge this since we need this external redirect handling in the RRR work - but I don't want to lose sight of that potential enhancement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #9634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants