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

[Bug]: absolute redirect fails while using base url #10052

Closed
amsal opened this issue Feb 5, 2023 · 11 comments · Fixed by #10076 or #10135
Closed

[Bug]: absolute redirect fails while using base url #10052

amsal opened this issue Feb 5, 2023 · 11 comments · Fixed by #10076 or #10135
Labels

Comments

@amsal
Copy link
Contributor

amsal commented Feb 5, 2023

What version of React Router are you using?

6.8.0

Steps to Reproduce

  • Have a react router application hosted in a sub directory e.g. https://example.com/app1
  • https://example.com and https://example.com/app1 are two independent applications. Served using a reverse proxy

app1 is the base path and it is set using basename

const router = createBrowserRouter(
  [
    {
      id: "root",
      path: "/",
      element: (
        <>
          <div>Root</div>
          <Link to="/logout">Logout</Link>
        </>
      ),
    },
    {
      path: "/logout",
      loader: LogoutLoader,
    },
  ],
  {
    basename: "app1",
  }
);
export const LogoutLoader = async () => {
  return redirect("https://example.com");
};
  • Go to logout url https://example.com/app1/logout

Expected Behavior

LogoutLoader should redirect user to https://example.com as if it was an external redirect to somewhere like https://google.com. It should be a redirect via browser instead of react-router.

Actual Behavior

React router changes the browser URL but the content of the site is still from https://example.com/app1 instead of https://example.com. In the browser dev tools, no request is made to redirect URL.

Possible bug might be here

if (url.origin === currentUrl.origin) {

@amsal amsal added the bug label Feb 5, 2023
@timdorr
Copy link
Member

timdorr commented Feb 5, 2023

This should be fixed by #10033, which is available via 6.8.1-pre.0

@timdorr timdorr closed this as completed Feb 5, 2023
@amsal
Copy link
Contributor Author

amsal commented Feb 7, 2023

hi @timdorr

This doesn't seem to resolve the issue (v6.8.1). I have created a minimal repo to reproduce this.

https://github.com/amsal/rr-basename-redirect.git

Assuming port 5173 is free and used by Vite for this.

  • run npm run dev
  • go to http://localhost:5173/app1
  • click Logout link on the page
  • redirect to http://localhost:5173 is not done as an external redirect

Thanks

@brophdawg11
Copy link
Contributor

This is arguably arguably part of #9859 but with the basename we should be able to deduce that this is not an in-app URL and fall into the window.location.assign flow. If anyone is interested in taking a stab at a PR for this, the offending code is inside startRedirectNavigation:

    if (isBrowser && typeof window?.location !== "undefined") {
      let newOrigin = init.history.createURL(redirect.location).origin;

      // ⚠️ This should probably include "OR pathname doesn't start with the basename"
      if (window.location.origin !== newOrigin) {
        if (replace) {
          window.location.replace(redirect.location);
        } else {
          window.location.assign(redirect.location);
        }
        return;
      }
    }

@tomfridental1
Copy link

Any update on this one?

@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Feb 22, 2023
@brophdawg11 brophdawg11 self-assigned this Feb 22, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.8.2-pre.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@tomfridental1
Copy link

tomfridental1 commented Feb 22, 2023

Seems the issue is not fixed, basename: '/v2' <Link to="https://sameorigin.com/someroute" /> still considered as internal url

@brophdawg11
Copy link
Contributor

This issue is for redirect, not Link 😉 . I can take a look into handling this in Link as well though

@tomfridental1
Copy link

That will be great 😎

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.8.2-pre.3 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@tomfridental1
Copy link

tomfridental1 commented Feb 24, 2023

Tested pre.3 looking good, everything works as expected. thank you!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.8.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Feb 27, 2023
@brophdawg11 brophdawg11 removed their assignment Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment