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

Failed refresh attempt should remove access token cookie #790

Closed
michael-pxr opened this issue Feb 5, 2024 · 3 comments
Closed

Failed refresh attempt should remove access token cookie #790

michael-pxr opened this issue Feb 5, 2024 · 3 comments
Assignees

Comments

@michael-pxr
Copy link

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.

  • The access token is expired. In this case, an attempt should be made to refresh the access token.
  • The refresh token is expired. In this case, no attempt should be made to refresh the access token.

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:

  • I've set the access token validity to 5 seconds and the refresh token validity to 1 minute.
  • Sign in on example.com/auth.
    • The sAccessToken cookie is set. The cookie will expire next year. The JWT token expires in 5 seconds.
    • The sRefreshToken cookie is set. It will expire in 1 minute.
    • The FrontToken header is set.
  • Wait 1 minute.
  • Go to the page that does a server side request to the API (example.com/my-info). This fails with status 401 and the client is told to attempt a refresh.
  • On the client side a request is made to api.example.com/auth/session/refresh with cookies
    • sAccessToken
    • st-last-access-token-update
    • sFrontToken
  • The response has status code 401 and body {"message":"unauthorised"}. There are no Front-Token or Set-Cookie headers.
  • The sAccessToken cookie is still set.

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

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

No branches or pull requests

3 participants