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

NextAuth should not invalidate the client-side session when the browser is offline #2215

Closed
igordanchenko opened this issue Jun 19, 2021 · 31 comments
Labels
client Client related code enhancement New feature or request stale Did not receive any activity for 60 days

Comments

@igordanchenko
Copy link
Contributor

igordanchenko commented Jun 19, 2021

Description 🐜

NextAuth invalidates the client-side session when the browser is offline and the NextAuth client cannot communicate with the NextAuth backend. From an application perspective, the expectation is that the NextAuth library must provide the last-known session state unless session TTL runs out or the NextAuth client receives an authoritative response from the NextAuth backend.

How to reproduce ☕️

git clone https://github.com/nextauthjs/next-auth.git
cd next-auth 
cp app/.env.local.example app/.env.local 
npm install
npm run dev:setup
npm run dev
  • Open http://localhost:3000/protected in Chrome
  • Sign In with credentials using "password" as password
  • Observe that the page says "This is protected content. You can access this content because you are signed in."
  • Open Chrome dev tools Network tab and select the "Offline" mode
  • Minimize the browser window
  • Restore the browser window
  • Observe that the page says "Access Denied. You must be signed in to view this page"

Screenshots / Logs 📽

Logs in browser console:

react_devtools_backend.js:2560 [next-auth][error][client_fetch_error] 
https://next-auth.js.org/errors#client_fetch_error session TypeError: Failed to fetch

asyncToGenerator.js?1da1:6 Uncaught (in promise) TypeError: Failed to fetch

Environment 🖥

  System:
    OS: macOS 11.4
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 1.37 GB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.17.1 - /usr/local/bin/node
    npm: 6.14.13 - ~/Temp/next-auth/node_modules/.bin/npm
  Browsers:
    Chrome: 91.0.4472.106
    Firefox: 89.0
    Safari: 14.1.1

Contributing 🙌🏽

I'm more than happy to author a PR but I'm just not sure about the logic behind

if (clientMaxAge === 0 && triggredByEvent !== true) {
Is there any specific reason to force clientMaxAge=0 session re-validation on keep-alive timer or visibilitychange events? IMO clientMaxAge=0 session must be presumed valid unless the backend says the session is no longer valid (which would be triggered by 'storage' event).

@igordanchenko igordanchenko added the bug Something isn't working label Jun 19, 2021
@balazsorban44
Copy link
Member

balazsorban44 commented Jun 19, 2021

Hi there, I wouldn't flag this as a bug, more of an enhancement as this has been the expected behavior until now. The thing you experience happens, because we re-fetch the session when the tab/window gets refocused. Currently this behavior cannot be turned off, but in v4, there will be an option for it. See #2171

(refetchOnWindowFocus={false})

@igordanchenko
Copy link
Contributor Author

Thanks for pointing me to that enhancement. However, the more I think about this, the stronger I feel that current behavior is incorrect. And to be clear, my main concern is with the way NextAuth handles network failures / offline mode and not the 'visibilitychange' event.

Let's imagine you have an email client app that uses the following session settings:

clientMaxAge: 120,
keepAlive: 5,

These settings establish the following contract:
a) let's use session data on the client for up to 120 minutes before considering it stale
b) and also let's ping the server every 5 minutes to renew the session and reset the 120-minute rolling window

But what is happening is that NextAuth evicts client-side session data even when a single keep-alive call to the server fails and returns no fresh data.

Now imagine you are commuting to work by train reading emails, and all of a sudden, your favorite email app tells you "Hey, your session is no longer good, and you need to sign in again" all just because the train went through a tunnel and you were offline for couple minutes. This is a bad user experience in my books, and I do not see a clear way for an app developer to work this around unless NextAuth correctly handles failed keep-alive / session refresh calls.

What are your thoughts on this?

@igordanchenko
Copy link
Contributor Author

Here is a PR that I believe addresses this issue.

@balazsorban44
Copy link
Member

balazsorban44 commented Jun 23, 2021

Thank you for your detailed example. I can see how it would be a problem, but I still think that any request to a new session should invalidate the old one as well. Let's say the session contains an access_token that is about to expire, and that is why you set up polling, so it gets refreshed before expiry. Now if the refresh fails and you are stuck with the old token, you might risk unauthenticated calls to your APIs.

The problem you are describing can be worked around in user land in my opinion. You could set up an event listener for network changes, and whenever you experience that the network is unavailable, you could clear the polling by setting keepAlive to 0.

Suggestion for _app.jsx:

import * as React from "react"
import { Provider } from "next-auth/client"

function useOnlineStatus() {
  const [online, setOnline] = React.useState(() => navigator.onLine)

  React.useEffect(() => {
    const handler = () => setOnline(navigator.onLine)
    window.addEventListener("online", handler)
    window.addEventListener("offline", handler)
    return () => {
      window.removeEventListener("online", handler)
      window.removeEventListener("offline", handler)
    }
  }, [])

  return online
}

export default function App({ Component, pageProps: { session, ...pageProps } }) {
  // Poll session every five minutes, but stop polling when the internet drops.
  const keepAlive = useOnlineStatus() ? 5 * 60 : 0
  return (
    <Provider options={{ keepAlive }}>
      <Component {...pageProps} />
    </Provider>
  )
}

UPDATE:

I checked, and our current implementation wouldn't actually work, since we return early if keepAlive is falsy, and thus we wouldn't actually clear the polling when keepAlive changes.

To be frank, I see a MUCH bigger problem. We use setTimeout instead of setInterval, so I am not even sure if keepAlive works as we advertise currently. 👀 This is fixed in the next branch.

See the relevant current code here:

if (keepAlive) {
__NEXTAUTH.keepAlive = keepAlive
if (typeof window === "undefined") return
// Clear existing timer (if there is one)
if (__NEXTAUTH._clientSyncTimer !== null) {
clearTimeout(__NEXTAUTH._clientSyncTimer)
}
// Set next timer to trigger in number of seconds
__NEXTAUTH._clientSyncTimer = setTimeout(async () => {
// Only invoke keepalive when a session exists
if (!__NEXTAUTH._clientSession) return
await __NEXTAUTH._getSession({ event: "timer" })
}, keepAlive * 1000)
}

And the new code here:

React.useEffect(() => {
const { refetchInterval } = props
// Set up polling
if (refetchInterval) {
const refetchIntervalTimer = setInterval(async () => {
if (__NEXTAUTH._session) {
await __NEXTAUTH._getSession({ event: "poll" })
}
}, refetchInterval * 1000)
return () => clearInterval(refetchIntervalTimer)
}
}, [props.refetchInterval])

Note that the new implementation would work with my suggestion, as it is correctly defined in a useEffect, meaning the cleanup (return) function will be called whenever props.refetchInterval (new name for keepAlive) changes.

@igordanchenko
Copy link
Contributor Author

Thank you for the PR, although I am going to close this for now.

First of all, we are in the process of releasing a new major version (v4), and the client code has been refactored significantly on the next branch. Any significant changes should therefore be made against the next branch. (See the new client code at: https://github.com/nextauthjs/next-auth/blob/next/src/client/react.js)

I'm more than happy to create a PR against the v4 branch as the code change would be pretty much identical.

Furthermore, I am not really sure about this proposal. If there is a network error while fetching the session, then there is a big chance that this network error will propagate through the app, thus keeping the old session wouldn't be that useful, in my opinion.

And that's exactly what's happening because NextAuth Provider is sitting at the top of the components tree in the _app.js, and triggers a re-render of the whole app when session state changes. It is entirely legit behavior when the session state changes (a user signs in or signs out in another tab), but it's not quite logical when an opportunistic keepAlive call fails due to network or server error. Maybe we are treating keepAlive calls differently. In my view, keepAlive calls are opportunistic "heartbeats" that are meant to renew session expiration TTL and fetch an updated session state if it changes. But it's totally okay to skip some heartbeats when the user is offline. For example, users would expect their email client app or book reading app to stay open and mounted in the browser when they briefly go offline.

Would you be more open to adding a configuration option that would define keepAlive semantic and allow it to work both ways?

@igordanchenko
Copy link
Contributor Author

Thank you for the creative proposal utilizing navigator.onLine. However, it can't reliably cover all possible use-cases of why keepAlive call may fail. Think of a network issue anywhere between your router and Next backend server (your direct ISP or any intermediate ISP), or intermittent issue with the backend server itself causing it to respond with 500 error. navigator.onLine would miss this kind of issue, but keepAlive call would still fail.

@igordanchenko
Copy link
Contributor Author

Let's say the session contains an access_token that is about to expire, and that is why you set up polling, so it gets refreshed before expiry. Now if the refresh fails and you are stuck with the old token, you might risk unauthenticated calls to your APIs.

In this case I would setup keepAlive interval of at least 1/10th of access_token TTL, and if it happens that all 10 heartbeats couldn't reach the server then I would deal with the expired access token on the application level (and it would be much easier problem to solve rather than not being able to differentiate user sign out vs failed keepAlive call as both flip the session to null)

@balazsorban44
Copy link
Member

Thanks for the additional info. You bring up logical reasons, so I am willing to give in, but I would like someone else's opinion on the matter.

Would be nice to check how others have implemented this:

Before opening a new PR, would you be up to creating a small codesandbox.io example using swr and react-query to see how they behave? I am happy to accept your proposal if any of these keep the stale data around on failed polling requests, as they are well established solutions and I trust them more than my word on this matter.

@igordanchenko
Copy link
Contributor Author

igordanchenko commented Jun 24, 2021

Here is code sandbox example using swr and react-query - https://codesandbox.io/s/nextauth-2215-gxoks

With both libraries the app stays mounted and displays previously fetched data.

@anguskeatinge
Copy link

Some options to handle offline-ness would be pretty helpful.
A few of my users have complained about being sporadically logged out when they are on patchy internet.

I ended up solving this with useSWR #596 (comment)

@balazsorban44 balazsorban44 added enhancement New feature or request and removed bug Something isn't working labels Jul 19, 2021
@balazsorban44 balazsorban44 changed the title NextAuth invalidates the client-side session when the browser is offline NextAuth should not invalidate the client-side session when the browser is offline Jul 19, 2021
@balazsorban44 balazsorban44 added the client Client related code label Jul 19, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Sep 17, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Sep 18, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Sep 18, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Nov 17, 2021
@balazsorban44 balazsorban44 removed the stale Did not receive any activity for 60 days label Nov 17, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Nov 17, 2021
@stale
Copy link

stale bot commented Jan 17, 2022

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Jan 17, 2022
@stale
Copy link

stale bot commented Jan 24, 2022

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

@stale stale bot closed this as completed Jan 24, 2022
@jozefhruska
Copy link

It seems this behavior is unchanged in the current release (v4.9.0). I'm using next-auth with SWR on one of my projects where I keep custom access tokens in the JWT token. The fact that the session is invalidated when a request fails makes it hard to utilize SWR's revalidateOnFocus feature.

  1. The app is mounted, and SWR fetches data successfully.
  2. The host device goes offline => /session request fails => session is invalidated (null).
  3. The host device goes back online, and the window focus event is triggered.
  4. SWR tries to fetch the data but sends unauthenticated requests because the session is null, resulting in an error.

Perhaps we could add a new flag similar to the SWR's refreshWhenOffline like you suggested? This wouldn't introduce any breaking change, but it'd make the behavior configurable by the user. I'm happy to help if you agree.

@anguskeatinge
Copy link

Thanks for posting this @jozefhruska, I'd wondered what the latest version was up to.

I only had success with this issue when I stripped all of those built in functions out and just handled it myself.

So using SWR, getting rid of the local storage stuff, and managing the cookies myself. Works a charm now.

@igordanchenko
Copy link
Contributor Author

I'm still using my own patch package based on PR #2237, and it's working like a charm - no random session invalidations due to network errors whatsoever, all next-auth functionality fully preserved, and no extra SWR hoops to jump. Happy to share it if you are still on v3 and would like to try it out.

@TimNZ
Copy link

TimNZ commented Aug 14, 2022

@igordanchenko I'm still on v3 and need to handle offline robustly, please hook me up with your patch package

@igordanchenko
Copy link
Contributor Author

igordanchenko commented Aug 14, 2022

@TimNZ
Copy link

TimNZ commented Aug 14, 2022

@igordanchenko [spends 20 mins going down the rabbit hole of patch-package and figuring out a patch file name that works 😅 ]
Thanks! Got it applying.
image

But it doesn't work as I expected.

My goal in applying that patch was to stop useSession() clearing the session on an offline/internet disconnected scenario as discussed above.
I simulated an 'offline' in Chrome DevTools, did tab changes to trigger 'visibilitychange' to error next-auth, and then went 'online', and the session was still null.

My _app.tsx usage

        <NextAuthProvider
          options={{
            clientMaxAge: 10, // parseInt(process.env.THISAPP_NEXTAUTH_CLIENT_CACHE_SECONDS), // Re-fetch session if cache is older than 10 seconds
            keepAlive: 300, // parseInt(process.env.THISAPP_NEXTAUTH_CLIENT_KEEPALIVE_SECONDS) // Send keepAlive message every 5 minutes
          }}
        >

What should I be doing?

@igordanchenko
Copy link
Contributor Author

I'm pretty sure the behavior you are seeing is cause by clientMaxAge: 10. You are basically instructing next-auth to use the cached session object for up to 10 seconds, so the above patch won't be of much help (unless you go offline and back online within under 10 seconds)

Here is the logic implemented in this patch:

const newClientSessionData = await getSession(
  { triggerEvent: !triggeredByStorageEvent },
  // fail over to the current session if the update wasn't triggered by a storage event or max age timeout
  !triggeredByStorageEvent && (clientMaxAge === 0 || currentTime < clientLastSync + clientMaxAge) ? clientSession : null
)

@TimNZ
Copy link

TimNZ commented Aug 14, 2022

Yeah I think that too.

I set it to 0 after I commented and will probably leave it that way for now until I feel like reassessing the implications around freshness of client data in the UI.

@igordanchenko
Copy link
Contributor Author

igordanchenko commented Aug 24, 2022

I've updated my project to next-auth@4.10.3 today, and unfortunately, v4 handles unreliable network conditions even worse than v3. It takes just one /api/auth/session network request to fail, and next-auth flips the session to null and completely stops trying to re-fetch the session (even when refetchInterval is defined).

Here is an updated patch for v4: https://gist.github.com/igordanchenko/e1e40c2da14ccdc2823c7fc31a5b2de2

This patch changes next-auth's bail-out-on-any-network-error implementation to use-stale-while-revalidate semantics. With this patch, the useSession hook consistently returns the last known session state regardless of network errors (browser offline, flaky network, DNS issue, routing issue, backend temporarily offline, etc.)

Installation:

  1. Download the patch file as ./patches/next-auth+4.10.3.patch (the actual file name will be changing as I keep updating the patch for newly released versions)
  2. Follow the patch-package installation instructions - https://www.npmjs.com/package/patch-package

Patch source code: igordanchenko@0918a5f

@anguskeatinge
Copy link

Good on ya mate, I honestly don't understand how you can be expected to handle real customers without this.

Is nextauth only for people doing hacky projects or for people building actual businesses?

@ajaykarthikr
Copy link

@balazsorban44 Is a fix planned for this issue?

@SoftMemes
Copy link

I've also just run into this issue and was taken by surprised by the current behaviour. In our case, this is affecting users on mobile devices where the connection may be intermittent and the browser suspended at any point leading to one-off errors.

The patch looks great, what I would really love to see is another status that will show the difference between "loading" (not yet known), "authenticated" (good for sure), "unauthenticated" (we have positive proof that there isn't a valid session) and an additional "limbo" state where recent attempt(s) to validate the session have failed for a non specific reason (offline, temporary server errors).

For now, we have reverted to using long lifetime access tokens with Auth0 rather than shorter lived access tokens periodically refreshed which would be my preference.

@SawkaDev
Copy link

Same issue. Following!

@jozefhruska
Copy link

@Sliffcak Since I last commented on this issue, a new refetchWhenOffline option has been added (#4935 (comment)). Could you try setting this to false and see if it helps?

@igordanchenko
Copy link
Contributor Author

igordanchenko commented Feb 28, 2023

@jozefhruska, the refetchWhenOffline option does an excellent job at hiding this issue in most cases, but it doesn't eliminate it since it doesn't fix the root cause. It just makes this issue less frequent but even more challenging to reproduce and troubleshoot, which is a poor customer experience in my books.

Here is a simple demo illustrating this issue - https://pnlkxs-3000.preview.csb.app (source code)

  1. Open the test page
  2. Click the "Sign In" button
  3. Sign in with any username and password
  4. Wait till the countdown timer is about to hit 0
  5. Switch your phone into airplane mode or kill the wi-fi on your laptop (you may need to repeat this step several times to catch the bug)
  6. Observe the session state flipped to "unauthenticated"
  7. Bring your device back online
  8. The session state remains "unauthenticated"

And here is the same demo with a patched next-auth version that does not exhibit this issue - https://m7219t-3000.preview.csb.app (source code)

@nfadili
Copy link

nfadili commented Apr 30, 2024

Running into this as well using next-auth 4.24.5 - the patch discussed above seems like reasonable behavior for SessionProvider to have but has fallen out of date and is a brittle long term solution.. Has anyone solved this problem in another way or have a more recent issue open with next-auth regarding this? I've poked around a bit but haven't seen anything 😬

@igordanchenko
Copy link
Contributor Author

Well, it's been almost 3 years with no alternative solution in sight, so I'm treating the above patch as a permanent solution for my projects that still use next-auth. I've updated the patch to make it compatible with the latest version.

@OmkarArora
Copy link

OmkarArora commented Jul 4, 2024

Thanks for the patch fix @igordanchenko!
Do you see any chance of this getting solved in v5?

@FINDarkside
Copy link

Now that it's shown that it cannot be worked around in userland and that SWR nor react query invalidate all data on network errors either can the fix be finally accepted to this project @balazsorban44?

Are there some arguments for keeping this behaviour that are left uncovered? Shouldn't it at least be an option to allow not invalidating sessions like this? Imagine if Netflix logged people out every time they have connectivity issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Client related code enhancement New feature or request stale Did not receive any activity for 60 days
Projects
None yet
Development

Successfully merging a pull request may close this issue.