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

Question: mitigate checks.state argument is missing error when messing with concurrent login. #467

Closed
maxime-bc opened this issue Apr 21, 2023 · 12 comments

Comments

@maxime-bc
Copy link

maxime-bc commented Apr 21, 2023

Problem description and reproduction steps

I am currently using the 01-Login sample from the auth0-express-webapp-sample repository to connect via OpenID to a Keycloak 21.1.0 server, and it is working fine.

In server.js, I only updated the config object so that the sample works with my Keycloak server, as follows :

const config = {
  authRequired: false,
  idpLogout: true,
  authorizationParams: {
    response_type: "code",
  },
};

In Keycloak, I have set up a realm named dummy, and in this realm, I have created a client named dummyclient.
I kept the configuration bare minimum and only followed the "Create client" form : I set the "Client authentication" switch to "On" as I want my client private and then set all the URLs.

But when I run the following steps, I get a 💣 400: checks.state argument is missing error :

1 - Open two tabs and try to access the protected /profile route. As we are not logged in, we are redirected to the Keycloak login page in both tabs.
2 - Log in in the second tab: we are redirected to the /profile page.
3 - Refresh the first tab where we are still not logged in and the error appears. If instead of refreshing the page you fill out the login form, Keycloak will tell you "You are already logged in." and display a "« Back to Application" link. Clicking it does not raise an error.

I also just found out that in step 2, if you log in from the first tab instead of the second, the following error is raised: 💣 400: invalid_grant (PKCE verification failed).

How can I mitigate these errors? I observed the same behaviour when I was using passport + openid-client.

To not affect the user with this error, I have updated the sample error handler as follows:

  app.use((err, req, res, next) => {
      if (res.headersSent) {
          return next(err);
      }
      else if (err.name === "BadRequestError") { // catch the "checks.state argument is missing" error
          logger.warn(err.message);
          res.redirect("/");
      } else {
          res.status(err.status || 500);
          res.render('error', {
              message: err.message,
              error: process.env.NODE_ENV !== 'production' ? err : {}
          });
      }
  });

It seems to do the job, but I'm wondering if there is any cleaner way of handling this?

Thanks for any help!

Environment

@maxime-bc maxime-bc changed the title Question : mitigate checks.state argument is missing error when messing with concurrent tabs. Question: mitigate checks.state argument is missing error when messing with concurrent tabs. Apr 21, 2023
@maxime-bc maxime-bc changed the title Question: mitigate checks.state argument is missing error when messing with concurrent tabs. Question: mitigate checks.state argument is missing error when messing with concurrent login. Apr 24, 2023
@adamjmcgrath
Copy link
Contributor

Hi @maxime-bc - thanks for raising this

This is expected behaviour - you drop a state cookie when you login and check it when you return to your app. If you open a new tab and start a 2nd login, you overwrite the state cookie on the first tab - so if you finish login on this tab it will fail since the state cookie is stale.

You should catch the error and prompt the user to login again (which should not require interaction, since the AS will have a session)

The error is not great, but we plan to make better more granular errors in the next major

@maxime-bc
Copy link
Author

Hi, thanks for your answer.

You should catch the error and prompt the user to login again (which should not require interaction, since the AS will have a > session)

If I understood you well, in my error handler, I should redirect the user to the /login page when a BadRequestError occurs? And, as a session already exists for the user, it should automatically redirect him to the app?

@adamjmcgrath
Copy link
Contributor

Hi @maxime-bc

I would prompt the user rather than automatically redirect (a message to say "try again" and a login button). You may get a 400 that the user can't recover from by logging in again, then you would get into an infinite loop.

@maxime-bc
Copy link
Author

When you say prompt the user to log in again, should the server send back something like this instead of the redirect?

res.send('<p>A login error occurred. Please try to login again.</p><a href="/login">Log In</a>');

@adamjmcgrath
Copy link
Contributor

Yep, exactly - however you would do something like this in your UI

@maxime-bc
Copy link
Author

So, server-side, when a BadRequestError occurs, I redirect the user to, say for example /login-error. Then, if I handle my UI server-side with templating such as in the auth0-express-webapp-sample example, I create a new view template named login-error.ejs that prompts the user to log in again (with a link to /login) and displays an error message saying that an unexpected error occurred during the login?

@adamjmcgrath
Copy link
Contributor

Yeah - that would work 👍

@maxime-bc
Copy link
Author

maxime-bc commented Apr 28, 2023

Ok, thanks for your answers! It's clearer for me now.

In this issue, I took the auth0-express-webapp-sample as an example to provide a minimal working example.
But the app I'm working on consists of an Express.js server configured like this :

  1. the server receives an incoming request,
  2. the server ensures that the user is authenticated with the auth() middleware,
  3. the server tries to match the request to any of its API routes,
  4. if no match, the server sends by default a built Vue.js SPA as a static file and let the vue router handle the request,
  5. finally, if there is still no match, the vue router redirects to a 404 error page in the SPA.

So, in this case, how could I handle a BadRequestError?

I could create a route in my Vue.js app router, say /login-error that displays a message and provide a link to login again. Then, in my server, I could redirect the user to /login-error when the error occurs. But by redirecting the user, the request will be intercepted by the auth() middleware and thus prompt the user to login, which is not what we want as it could lead to an infinite loop?

@adamjmcgrath
Copy link
Contributor

Hi @maxime-bc

But by redirecting the user, the request will be intercepted by the auth() middleware and thus prompt the user to login, which is not what we want as it could lead to an infinite loop?

If you put the /login-error handler before the auth() middleware it wont require authentication.

@maxime-bc
Copy link
Author

Hi @adamjmcgrath,

I could do this but I'm not generating views server-side, so when an error occurs I would like to let my SPA handle the error and display a nice error message to the user.

What I wanted to do:

Server-side, I redirect to /login-error when a BadRequestError occurs. As no route named /login-error is declared on my Express server, it sends the SPA by default (step 4). And in my SPA, the route /login-error exists. So the vue router should show my corresponding view (e.g. LoginError.vue) that contains a nice error message and a button that prompts the user to log in again.

But that won't work since the auth() middleware will block the /login-error redirect as we are not logged-in.

Only solution I see:

As you said, put a /login-error route server-side before the auth() middleware and redirect to this route when a BadRequestError occurs. But the /login-error route will only send a simple error message like this as I'm not rendering views server-side.

@adamjmcgrath
Copy link
Contributor

@maxime-bc - If you have a server side app (even if it is serving a spa), you still need to handle and display server side errors. How you decide to do that is up to the you, the application author, and not something that this SDK can help with.

@maxime-bc
Copy link
Author

Yes, I understand. Thanks for taking time to answer me.

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

2 participants