supertokens / supertokens-node

Node SDK for SuperTokens core
https://supertokens.com
Other
278 stars 72 forks source link

Failed refresh attempt should remove access token cookie #790

Closed michael-pxr closed 1 month ago

michael-pxr commented 5 months ago

Hi,

I have a NextJS application that calls an API (a NodeJS server) for its data. The API implements the server-side SuperTokens SDK. The NextJS application only implements the client-side SDK. I.e. only the API communicates with the supertokens-core. When the NextJS app does a server-side request to a protected API endpoint, it is possible that the access token has expired. If so, the NextJS app must communicate this to the client, such that the client can try to refresh the access token.

The NextJS server-side code communicates the 'try refresh token' message back to the client:

// pages/my-info.tsx
// Based on https://supertokens.com/docs/thirdpartyemailpassword/nextjs/session-verification/in-ssr#1-we-use-getsession-in-getserversideprops
export const getServerSideProps = async () => {
    const props = {};
    try {
        // Try to fetch protected data from the API.
        props.data = await fetchProtectedDataFromAPI();
    } catch (e) {
        if (e.status === 401) {
            // Let the client know that the access token needs to be refreshed.
            props.fromSupertokens = 'needs-refresh';
        }
    }
    return { props };
}

If the client needs to refresh, it calls Session.attemptRefreshingSession and reloads the page on success. Note that if the refresh attempt fails, it does not redirect the user to the login page. Instead, the page will render different content if the user is logged in or not.

// pages/_app.tsx
// Based on https://supertokens.com/docs/thirdpartyemailpassword/nextjs/session-verification/in-ssr#2-doing-manual-refresh-on-the-frontend
function MyApp({ Component, pageProps }: AppProps<{fromSupertokens: string}>) {
    const [needsToRefresh, setNeedsToRefresh] = useState<boolean>(
        pageProps.fromSupertokens === 'needs-refresh',
    );
    useEffect(() => {
        async function doRefresh() {
            if (needsToRefresh) {
                if (await Session.attemptRefreshingSession()) {
                    // post session refreshing, we reload the page. This will
                    // send the new access token to the server, and then 
                    // getServerSideProps will succeed
                    location.reload()
                } else {
                    // the user's session has expired.
                    // I do not want to redirect a user to the login page.
                    setNeedsToRefresh(false);
                }
            }
        }
        doRefresh()
    }, [needsToRefresh]);
    if (needsToRefresh) {
        // in case the frontend needs to refresh, we show nothing.
        // Alternatively, you can show a spinner.
        return null
    }
    return (
        <SuperTokensWrapper>
            <Component {...pageProps} />
        </SuperTokensWrapper>
    )
}

export default MyApp

Currently, the NextJS server tell the client to attempt a refresh if the API request returns a 401. However, there are different reasons why the API might return a 401. E.g.

Ideally, the NextJS server should make the distinction between these two cases by checking if the sAccessToken cookie is set. However, the sAccessToken cookie is still set when the refresh token has expired. I've tested this as follows:

In a Discord support-questions thread I was told that the expected behaviour is that the sAccessToken should be removed when a refresh attempt fails. However, this is not the case.

Since the sAccessToken cookie is still set, the NextJS server cannot use this cookie to determine whether a refresh attempt should be made or not. (As a work around, the NextJS server can check to see if the sFrontToken cookie exists or not.)

In conclusion: If an attempt to refresh an access token fails, the access token is not removed. This means the server application cannot use the existence of the sAccessToken cookie to determine whether or not the client should attempt a refresh. To fix this, a failed attempt to refresh an access token must remove the access token.

rishabhpoddar commented 5 months ago

Another unexpected behaviour observed by @michael-pxr is that the await Session. attemptRefreshingSession() function throws an error instead of returning false when the API returns a 401.

rishabhpoddar commented 3 months ago

Another way of getting to this state is if the dev changes the apiBasePath. This would cause issues cause the refresh API path would change, which in turn would cause the existing refresh token that the client has not to be sent to the new refresh API path, causing it to send a 401 which would put the user in a stuck state

rishabhpoddar commented 1 month ago

This has been fixed in the latest versions of our backend SDKs.