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

reproduction case for CORS issue with server functions #24

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joeholdcroft
Copy link

Steps to reproduce

  1. Pull down the repo, npm i, npm run dev
  2. Set environment variables (notably you want auth callback to be on the path /auth/callback)
  3. Open http://localhost:3000/
  4. Authenticate (for some reason I have to do this twice, I think I know why but it's unrelated to my issue so have ignored)
  5. Navigate to http://localhost:3000/test and open the console
  6. Open a second window with the app
  7. Sign Out on your second window
  8. In your original console, you should still appear to be logged in. Now click the "Trigger server function" button, and see the errors you get in console

@PaulAsjes
Copy link
Contributor

Thanks for this, I was able to reproduce and know what the problem is.

In a nutshell, the client expects a server action but is given a redirect instead as authkit-nextjs detects that the user is logged out and tries to redirect to AuthKit. Browser says no because of CORS and you get this error.

We're going to have a think on what the best practise is here, I'm initially thinking we return an error instead of trying to hijack the request and letting the client handle a redirect.

@joeholdcroft
Copy link
Author

@PaulAsjes indeed! Thanks for taking a look. Your approach seems reasonable. Another other idea I had was to create an interstitial page that you can redirect to instead on the same app, which in turn redirects to the login page.

@PaulAsjes
Copy link
Contributor

So after a bunch of back-and-forth we've settled on what I think is the "best" (albeit not great) solution: workos/authkit-nextjs#68

Keen to hear your thoughts!

@joeholdcroft
Copy link
Author

Hey @PaulAsjes - thanks for getting back to me!

I don't love it, mainly because the suggestion is that we make an assumption that an error of any type in a server action is due to the user being logged out, and we redirect them to the login screen. In reality, there are many reasons we might get an error from a server action, and only for this specific case would redirecting to the login screen make sense. Or am I missing something, is there some way to detect that the error is due to the user being logged out?

Did you consider my approach too?

Another other idea I had was to create an interstitial page that you can redirect to instead on the same app, which in turn redirects to the login page.

@PaulAsjes
Copy link
Contributor

Hey @joeholdcroft, we agreed with your assessment and came up with a solution that we think works much better. By wrapping the app code in an <AuthKitProvider> we can now detect when there's a stale session after a the tab is active again and then either reload the page or execute some custom code.

Have a look at the updated README for instructions: https://github.com/workos/authkit-nextjs

@joeholdcroft
Copy link
Author

@PaulAsjes that's great news, thank you! I just tried bumping the SDK version and simply wrapping our app in the AuthKitProvider (our layout component is a client component). Weirdly, I get this error, which is very strange considering we don't use Impersonation anywhere in our app at all. Any ideas?

image

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

Successfully merging this pull request may close these issues.

2 participants