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

Unexpected behaviour when redirecting in action #2997

Closed
liegeandlief opened this issue Apr 27, 2022 · 23 comments
Closed

Unexpected behaviour when redirecting in action #2997

liegeandlief opened this issue Apr 27, 2022 · 23 comments

Comments

@liegeandlief
Copy link

liegeandlief commented Apr 27, 2022

What version of Remix are you using?

1.4.1

Steps to Reproduce

Create a Remix project (with the built in Remix server) with the following routes:

index.tsx:

import { memo } from 'react'

const IndexRoute = memo(() => {
  return <h1>Home</h1>
})
IndexRoute.displayName = 'IndexRoute'

export default IndexRoute

magic-link.tsx:

import { memo, useEffect } from 'react'
import { LoaderFunction, json, useLoaderData, useSubmit } from 'remix'
import { emailFormFieldName, codeFormFieldName } from '~/routes/verify-magic-link'

type LoaderData = {
  email: string,
  code: string
}

const loader: LoaderFunction = async ({ request }) => {
  const url = new URL(request.url)
  const email = url.searchParams.get("email") ?? ''
  const code = url.searchParams.get("code") ?? ''

  return json<LoaderData>({
    email,
    code
  })
}

const MagicLinkRoute = memo(() => {
  const loaderData = useLoaderData<LoaderData>()
  const submit = useSubmit()

  useEffect(() => {
    const formData = new FormData()
    formData.append(emailFormFieldName, loaderData.email)
    formData.append(codeFormFieldName, loaderData.code)
    submit(formData, { method: 'post', action: '/verify-magic-link' })
  }, [loaderData.email, loaderData.code, submit])

  return (
    <p>Some pending UI</p>
  )
})
MagicLinkRoute.displayName = 'MagicLinkRoute'

export default MagicLinkRoute
export { loader }

verify-magic-link.tsx:

import { memo } from 'react'
import { ActionFunction, json, redirect, useActionData } from 'remix'

export const emailFormFieldName = 'email'
export const codeFormFieldName = 'code'

// fake verifyMagicLink function with forced failure
const verifyMagicLink = ({ email, code } : { email: string, code: string  }) => Promise.resolve(false)

type ActionData = Awaited<ReturnType<typeof verifyMagicLink>>

const action: ActionFunction = async ({ request }) => {
  const body = await request.formData()
  const email = body.get(emailFormFieldName)?.toString() ?? ''
  const code = body.get(codeFormFieldName)?.toString() ?? ''

  const verifyMagicLinkResult = await verifyMagicLink({ email, code })

  if (verifyMagicLinkResult === true) {
    return redirect('/')
  }

  return json<ActionData>(verifyMagicLinkResult)
}

const VerifyMagicLinkRoute = memo(() => {
  const actionData = useActionData<ActionData>()

  return (
    <p>{JSON.stringify(actionData, null, 2)}</p>
  )
})
VerifyMagicLinkRoute.displayName = 'VerifyMagicLinkRoute'

export default VerifyMagicLinkRoute
export { action }

In the entry.server.tsx handleRequest function put the following just before returning the response:

  console.log({
    type: 'handleRequest',
    url: request.url,
    method: request.method,
    response: newResponse.status
  })

In the entry.server.tsx handleDataRequest function put the following just before returning the response:

  console.log({
    type: 'handleDataRequest',
    url: request.url,
    method: request.method,
    response: newResponse.status
  })

Go to https://localhost:3000/magic-link?code=123456&email=test@test.com in your browser. In the terminal you should see:

{
  type: 'handleRequest',
  url: 'http://localhost:3000/magic-link?code=123456&email=test@test.com',
  method: 'GET',
  response: 200
}

{
  type: 'handleDataRequest',
  url: 'http://localhost:3000/verify-magic-link?_data=routes%2Fverify-magic-link',
  method: 'POST',
  response: 200
}

{
  type: 'handleDataRequest',
  url: 'http://localhost:3000/verify-magic-link?_data=root',
  method: 'GET',
  response: 200
}

This is what I would expect to see.

Now in verify-magic-link.tsx change const verifyMagicLink = ({ email, code } : { email: string, code: string }) => Promise.resolve(false) to const verifyMagicLink = ({ email, code } : { email: string, code: string }) => Promise.resolve(true).

Run the same request in your browser again.

Expected Behavior

Terminal should show:

{
  type: 'handleRequest',
  url: 'http://localhost:3000/magic-link?code=123456&email=test@test.com',
  method: 'GET',
  response: 200
}

{
  type: 'handleDataRequest',
  url: 'http://localhost:3000/verify-magic-link?_data=routes%2Fverify-magic-link',
  method: 'POST',
  response: 302
}

{
  type: 'handleDataRequest',
  url: 'http://localhost:3000/?_data=root',
  method: 'GET',
  response: 200
}

Actual Behavior

Terminal shows:

{
  type: 'handleRequest',
  url: 'http://localhost:3000/magic-link?code=123456&email=test@test.com',
  method: 'GET',
  response: 200
}

{
  type: 'handleDataRequest',
  url: 'http://localhost:3000/?_data=root',
  method: 'GET',
  response: 200
}

The POST request does not get logged by either handleRequest or handleDataRequest.

Even though I am taken to the index.tsx route as expected the 302 redirect response never seems to be returned to the browser. Instead when I look in the browser network tab I can see that the the response to the POST request was a 204 response.

This is causing me issues because I would like to add a set-cookie header on the redirect response but the cookie never gets set because the redirect response doesn't make it to the browser.

@kiliman
Copy link
Collaborator

kiliman commented Apr 27, 2022

When a loader/action returns a redirect from a client-side navigation, Remix uses status 204 and a header X-Remix-Redirect. This is so that the actual fetch call doesn't try to redirect, but instead Remix will handle the redirect. Also, if the response includes a set-cookie header, it will add X-Remix-Revalidate to ensure that all loaders are revalidated.

if (isRedirectResponse(response)) {
// We don't have any way to prevent a fetch request from following
// redirects. So we use the `X-Remix-Redirect` header to indicate the
// next URL, and then "follow" the redirect manually on the client.
let headers = new Headers(response.headers);
headers.set("X-Remix-Redirect", headers.get("Location")!);
headers.delete("Location");
if (response.headers.get("Set-Cookie") !== null) {
headers.set("X-Remix-Revalidate", "yes");
}
return new Response(null, {
status: 204,
headers,
});

@liegeandlief
Copy link
Author

Okay that makes sense but in that case it seems to be bypassing handleDataRequest when returning the 204 response. In handleDataRequest I make some changes to the response by adding extra headers and I need these headers to be returned. But these headers don't get added to the 204 response.

If Remix needs to amend the response status should it not do it after handleDataRequest has completed?

@kiliman
Copy link
Collaborator

kiliman commented Apr 27, 2022

handleRequest and handleDataRequest are the last things that happen before returning to the client. Remix has already done its processing by then. These are not pre request handlers... but post Remix handlers.

@liegeandlief
Copy link
Author

But the point is that handleDataRequest is not being run in this scenario and Remix is returning to the client before running it. Should it not always run for every data request? It seems strange to have a way to hook into the request lifecycle which only runs for some requests. Without an understanding of the internals of Remix it is hard to understand when it will or won't be executed.

Would this not be a simple change in packages/remix-server-runtime/server.ts from:

      return new Response(null, {
        status: 204,
        headers,
      });

to:

      response = new Response(null, {
        status: 204,
        headers,
      });

@davemurphysf
Copy link

I just ran into this issue but on Microsoft Edge only; everything worked fine on Chrome and Firefox. I am redirecting on an Action submission (passing OAuth info to the backend) and I can see both the X-Remix-Redirect and X-Remix-Revalidate headers (as well as my session being set) on the response. I ended up having to redirect on the client side (as well?) to make it work consistently:

    const submiter = useFetcher();
    const [searchParams] = useSearchParams();
    const redirectTo = searchParams.get('redirectTo') ?? '/actions';
    const navigate = useNavigate();

    useEffect(() => {
        if (!isBrowser || submiter.type !== 'init') return;

        const formData = new FormData();
        const contentHash = window.location.hash;

        if (hashContainsKnownProperties(contentHash)) {
            const hash = getDeserializedHash(contentHash);

            if (hash.error) {
                formData.append('error', hash.error);
            } else {
                Object.entries(hash).forEach(([key, value]) =>
                    formData.append(key, value as string),
                );
            }

            window.location.hash = '';
        }

        formData.append('redirect_to', redirectTo);
        submiter.submit(formData, {
            action: '/oauth/callback',
            method: 'post',
        });
    }, [submiter, redirectTo]);

    useEffect(() => {
        if (submiter.type === 'done') {
            navigate(redirectTo, { replace: true });
        }
    }, [navigate, redirectTo, submiter.type]);

    return null;

@fknop
Copy link

fknop commented Aug 9, 2022

I seem to have the same issue with the remix-auth library, the redirect to the Github OAuth flow seems to not happen properly and I have a 204. It used to work with remix 1.4.3

Update:
It actually still works in 1.6.4, but starts breaking in 1.6.5

Update 2:

I think I managed to pinpoint my own issue, I don't know if this is related to the original issue of this thread, but remix-auth-github properly redirects when <LiveReload /> is commented. The only change I see from 1.6.5 is this PR: #859

@danieltott
Copy link

@fknop I had the same issue - lots of weird things happening with LiveReload and remix-auth, especially re: redirecting when trying to log in. I verified that using the old version of LiveReload stops this problem from happening. Here's the "custom" LiveReload function I'm using instead of the production one from remix (which is just the 1.6.4 version with no other changes):

export const LiveReload =
  process.env.NODE_ENV !== 'development'
    ? () => null
    : function LiveReload({
        port = Number(process.env.REMIX_DEV_SERVER_WS_PORT || 8002),
        nonce = undefined,
      }) {
        let js = String.raw;
        return (
          <script
            nonce={nonce}
            suppressHydrationWarning
            dangerouslySetInnerHTML={{
              __html: js`
              (() => {
                  let protocol = location.protocol === "https:" ? "wss:" : "ws:";
                  let host = location.hostname;
                  let socketPath = protocol + "//" + host + ":" + ${String(
                    port,
                  )} + "/socket";
                  let ws = new WebSocket(socketPath);
                  ws.onmessage = (message) => {
                    let event = JSON.parse(message.data);
                    if (event.type === "LOG") {
                      console.log(event.message);
                    }
                    if (event.type === "RELOAD") {
                      console.log("💿 Reloading window ...");
                      window.location.reload();
                    }
                  };

                  ws.onerror = (error) => {
                    console.log("Remix dev asset server web socket error:");
                    console.error(error);
                  };
                })();
              `,
            }}
          />
        );
      };

danieltott added a commit to Virtual-Coffee/virtualcoffee.io that referenced this issue Sep 7, 2022
@haakonmt
Copy link

haakonmt commented Sep 21, 2022

The old version of LiveReload solves my issues as well. I believe it breaks due to this added block:

ws.onclose = (error) => {
  console.log("Remix dev asset server web socket closed. Reconnecting...");
  setTimeout(
    () =>
      remixLiveReloadConnect({
        onOpen: () => window.location.reload(),
      }),
    1000
  );
};

More specifically, I think the arbitrary 1000 ms timeout before reloading creates a race condition, where, during a redirect, the route you're navigating from will reload itself before the redirect has been able to complete. By increasing the timeout, the issues disappeared for my part, but that does not seem like a viable permanent fix.

@juandjara
Copy link

Hello, I have tested this behaviour with last remix version as of now (1.7.2) and it is still happening.

I am running a 2020 Macbook Air with M1 Proccesor (big sur, 11.6 os version( and, weirdly, this bug happens only on Firefox (version 104.0.2). On Chrome and Safari, redirects work ok.

I tried replacing the LiveReload component with the one suggested in this thread and it worked, problem was solved. With this I can be 100% sure that the bug is caused by the setTimeout on the LiveReload code

danieltott added a commit to Virtual-Coffee/virtualcoffee.io that referenced this issue Sep 28, 2022
* Get basic auth running with errors

* add login method

* logging

* add register endpoint

* stop using forked version of form

* testing error states

* fix dev settings

* fix up error handling and add some new methods

* Add additional auth utilities

* add schema to response

* begin adding join route

* rename CmsAuth

* remove logging

* begin adding some cms actions

* begin adding event fetching

* Add ts packages

* use tsconfig

# Conflicts:
#	tsconfig.json

* remove some comments

* add remix type defs

* update server.js to match remix example

* convert entry files to ts

* use ts

* allow redirect on success

* add event forwarding

* query all calendars

* display message

* move auth into membership route

* use ts

* add route protection back in to dashboard

* return value from loader

* move files into frontend route

* update paths from rename

* Move app files into __app

* Add tailwind

* remove tailwind.css from vc

* ignore tailwind

* add spread params to svgs

* move around auth routes and add styles

* rename dashboard to membership index

* adjust styles

* move join into auth

* remove old thing

* renive extra links

* fix up auth

* add getCalendars

* move auth into pathless

* move layouts into correct folder

* use new singletask layout

* convert to ts

* use updated types

* add first pass of events

* add inter font

* use classNames

* rename method

* add events calendar

* add events calendar views

* description

* update calendar formatting

* add popover for event

* remove console.log

* go stacked layout

* adjust for stacked layout

* add meta data

* begin adding more components and details

* Add ts versions

* move files

* reset dependencies

* check for null data

* fix some ts issues

* update dependencies

* update utility types

* update heroicons

* abstract some components

* clean up styling

* use sky instead of indigo

* upgrade remix dependencies and scripts

* add tiny-invariant

* convert to ts

* allow for netlify dev

* use globals

* open in new tab

* continue debugging

* auth debugging go away

* ignore

* convert to ts and wrap in global

* split out cms functions

* removing logging

* check for user

* move tls reject to env variable

* read user via getUser

* removing

* clean up events

* authenticate

* use split versions

* remove esbuild from node bundler

* turn on defaults

* turn off esnext for now

* update login form

* add note

* fix this thing

* remove console.log call

* use custom livereload from 1.6.4 of remix

see remix-run/remix#2997 (comment)

* remove debugging help

* move file

* fix faker fn

* tweak Button styling and add fullWidth prop

* Add TextInput and FieldGroup

* style reset password form

* clean up auth forms

* convert to ts

* convert some ts

* add wide option

* add FieldSet, TextArea, and addons for TextInput

* clean up register

* fix up register form / hide form for now

* use solid icons

* Abstract events list and add ics link

* use updated imports

* add events to dashboard

* turn on esnext

* correct link address

* remove comment

* add getCalendar

* add black and white buttons

* Add passedRef prop

* Add subscribe links

* move join files

* fix links

* fix import

* Fix path for mdx loading

* abstract out not found content

* Add splat page to catch non-loader 404s

* do not default to empty string

* remove extra h1
@ErickTamayo
Copy link

I confirmed that <LiveReload /> is causing problems when dev with redirecting in Firefox (e.g. using remix-auth). It doesn't seem to cause any problems in Chrome or Safari.

@n1ghtmare
Copy link

Thank you @ErickTamayo I've spent like 2 hours going nuts why my redirect is not working, I can also confirm that it only happens in Firefox and if I remove <LiveReload /> everything works just fine.

@danieljb
Copy link

As mentioned before this was introduced in #859 to enable automatic re-connection when the dev server is restarted. Would it be possible to check for the close event code before setting the timeout? This would keep the auto reconnect behavior while fixing the Firefox behavior (at least in a quick test):

-                  ws.onclose = (error) => {
+                  ws.onclose = (event) => {
                    console.log("Remix dev asset server web socket closed. Reconnecting...");
-                    setTimeout(
+                    if (event.code === 1006) setTimeout(
                      () =>
                        remixLiveReloadConnect({
                          onOpen: () => window.location.reload(),
                        }),
                      1000
                    );
                  };
LiveReload component code

export const LiveReload =
  process.env.NODE_ENV !== "development"
    ? () => null
    : function LiveReload({
        port = Number(process.env.REMIX_DEV_SERVER_WS_PORT || 8002),
        nonce = undefined,
      }: {
        port?: number;
        /**
         * @deprecated this property is no longer relevant.
         */
        nonce?: string;
      }) {
        let js = String.raw;
        return (
          <script
            nonce={nonce}
            suppressHydrationWarning
            dangerouslySetInnerHTML={{
              __html: js`
                function remixLiveReloadConnect(config) {
                  let protocol = location.protocol === "https:" ? "wss:" : "ws:";
                  let host = location.hostname;
                  let socketPath = protocol + "//" + host + ":" + ${String(
                    port
                  )} + "/socket";
                  let ws = new WebSocket(socketPath);
                  ws.onmessage = (message) => {
                    let event = JSON.parse(message.data);
                    if (event.type === "LOG") {
                      console.log(event.message);
                    }
                    if (event.type === "RELOAD") {
                      console.log("💿 Reloading window ...");
                      window.location.reload();
                    }
                  };
                  ws.onopen = () => {
                    if (config && typeof config.onOpen === "function") {
                      config.onOpen();
                    }
                  };
                  ws.onclose = (event) => {
                    console.log("Remix dev asset server web socket closed. Reconnecting...");
                    if (event.code === 1006) setTimeout(
                      () =>
                        remixLiveReloadConnect({
                          onOpen: () => window.location.reload(),
                        }),
                      1000
                    );
                  };
                  ws.onerror = (error) => {
                    console.log("Remix dev asset server web socket error:");
                    console.error(error);
                  };
                }
                remixLiveReloadConnect();
              `,
            }}
          />
        );
      };

@OllyHodgson
Copy link

I've also done a quick test with @danieljb's event.code fix above and it appears to fix the issue in Firefox.

@hallowatcher
Copy link

Thanks guys! I had a headache for 2 days straight because I couldn't find the issue... Any PRs happening for this?

@OllyHodgson
Copy link

I believe PR 4725 fixes this issue.

@jdnichollsc
Copy link

I'm testing a redirection while authenticating users and the POST request is not triggering the action so the redirection is not happening until the user refresh the page. I'm using RemixJS 1.11.0 and React 18.2.0. Please check the current implementation:

const csrf = useAuthenticityToken();
const { web3AuthState } = useWeb3AuthContext();
const submit = useSubmit();

useEffect(() => {
  if (web3AuthState.state === 'connected') {
    console.log('Logged in with Web3Auth')

  submit(
    { ...web3AuthState, csrf },
    { replace: true, method: 'post' }
  );
  }
}, [web3AuthState]);

I added logs from the action of this route but they are not called at any time from Firefox 🤔

Any help is really appreciated!

@jdnichollsc
Copy link

While the useSubmit hook is fixed, I was able to solve this wrong behavior by using a hidden form element directly:

  const formRef = useRef<HTMLFormElement>(null);

  useEffect(() => {
    if (web3AuthState.state === 'connected' && formRef.current) {
      console.log('Logged in with Web3Auth')
      formRef.current?.submit(); // Fixed by replacing useSubmit in order to trigger the submit of the auth redirection
    }
  }, [web3AuthState, formRef]);

Let me know what you think folks! <3

@machour
Copy link
Collaborator

machour commented Jan 24, 2023

Hum, I feel like a lot of issues have been mixed into this one.
@liegeandlief, is this issue still relevant for you?

@machour machour added the needs-response We need a response from the original author about this issue/PR label Jan 24, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jan 24, 2023
@machour machour added the needs-response We need a response from the original author about this issue/PR label Jan 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

This issue has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from issues that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2023
@jdnichollsc
Copy link

@machour can you reopen this issue please?

@machour machour reopened this Mar 15, 2023
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Mar 15, 2023
@jdnichollsc
Copy link

jdnichollsc commented Mar 15, 2023

During my tests with useSubmit, I'm getting a 201 status code instead of a 302 redirection. can you validate that wrong behavior?

This is the code of that action:

export const action = async ({ request }: ActionArgs) => {
  try {
    // validations here, redirecting users with this action submit :)
    return redirect('/profile',
      {
        status: 302,
        headers: {
          'Set-Cookie': await saveLoginCookie(),
        }
      }
    );
  } catch (error) {
    console.error('Auth error', error);
    return json(`Sorry, we couldn't authenticate the user`, {
      status: 500,
    });
  }
};

@doctor
Copy link

doctor commented Mar 28, 2023

I am new to Remix. Just started last week.

I am using latest indie stack.

After I build the app for production, and run it in production mode using
cross-env NODE_ENV=production remix-serve build

Safari: After login it will not redirect to the Notes page.

Chrome: After login redirecting to Notes page just fine.

My observation: Safari seems to be not redirecting due to secure: true turned on in the createCookieSessionStorage setting.
When I forced secure to false it works fine. That explains why this is working okay in npm run dev and not npm start

export const sessionStorage = createCookieSessionStorage({
  cookie: {
    name: "__session",
    httpOnly: true,
    path: "/",
    sameSite: "lax",
    secrets: [process.env.SESSION_SECRET],
    secure: process.env.NODE_ENV === "test", // won't work when its set to production
  },
});

Let me know what I can do to improve?

@brophdawg11
Copy link
Contributor

Hey folks! As @machour noted this issue seems to now have split into at least 3-4 different potential issue discussions so it's near impossible to continue triaging. I'm going to close this out and ask the following:

  • If you still have an issue on the current release, please open a new issue with a minimal reproduction via https://remix.new
  • Please comment with any newly opened issues so folks can check before opening duplicates of one another

Thank you! This will make it much easier for us to debug any potential issues.

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

No branches or pull requests