auth0/auth0-vue

Uncaught error when error thrown inside custom Auth0 Action

freddiescadding opened this issue · 16 comments

Describe the problem

When an error is thrown inside a custom post-login auth0 action, our unauthenticated callback URI /auth/callback is throwing an error and not being redirected.

The URL is: http://localhost:8009/auth/callback?error=access_denied&error_description=my_custom_error_msg&state=XXX

Error is:

Uncaught (in promise) Error: my_custom_error_msg
    at ee.handleRedirectCallback (auth0-spa-js.production.esm.js:1:31086)
    at plugin.ts:128:20
    at ae.__proxy (plugin.ts:166:22)
    at ae.handleRedirectCallback (plugin.ts:127:17)
    at ae.__checkSession (plugin.ts:140:33)
    at ae.install (plugin.ts:66:10)
    at Object.use (runtime-core.esm-bundler.js:4399:28)
    at main.ts:40:5

I'm not sure whether I am meant to be catching this error myself in the application or whether the SDK should be catching the error here.

What was the expected behavior?

Based on this function and this function, I expected this to set an error value on the plugin, and reload the URL from appState.

Reproduction

Add a custom post-login action that throws an error, and create a Vue 3 app with an unauthenticated callback URL.
Post-login action e.g.:

exports.onExecutePostLogin = async (event, api) => {
  return api.access.deny('my_custom_error_msg');
};

Environment

  • Version of auth0-vue used: 2.1.0
  • Version of vue used: 3.2.47
  • Which browsers have you tested in? Chrome, Safari
  • Other modules/plugins/libraries that might be involved: Vue Router v4.0.3

It looks like the checkSession method does not use the proxy, so errors aren't caught as you expect.

Will look into fixing this, thanks.

It looks like the checkSession method does not use the proxy, so errors aren't caught as you expect.

Will look into fixing this, thanks.

Thanks @frederikprijck, much appreciated!

@frederikprijck Please could you provide a rough timeframe estimate for this fix, so that my team can decide whether to pause or abort our current migration to this package? Many thanks for your help

Thanks, I was looking into this, but can you help me understand why this would result in aborting to migrate to our SDK?

Thanks, I was looking into this, but it might be a breaking change to avoid throwing the error. People may be relying on it. So I am not sure we can change the behavior.

If it is the intended behaviour of the SDK, wouldn't breaking changes still be appropriate for a new vsn if documented in the Changelog?

Can you help me understand why this would result in aborting to migrate to our SDK?

We are using a different vue/auth0 SDK at the moment, but have decided to move towards the auth0/auth0-vue SDK for a few reasons. We will need to decide if having to add a workaround for this issue impacts whether we still wish to make the migration at this time. It would still be our preference to complete this migration. Thanks

I was incorrect about the breaking change. I believe __checkSession should never throw, as it would break plugin installation. I have opened a PR that I would believe should solve it. I will need to do some additional testing, so added it as a draft.

To help speed things up, it would be helpful if you could verify if the fix solves your use case as well.

Thanks @frederikprijck I have tested this. Because this is now just swallowing the error, the user is no longer redirected from the callback URL. Would we expect the desired behaviour to be as I outlined in the What was the expected behavior? section above: namely that the SDK would add the error to the errors property but still continue to reload the route stored in appState? The flow of the __checkSession() fn seems to be that even in the case of an error query param from Auth0, the router will still be pushed to its original target.

Thanks for your help with this!

The way that our callback route is set up, which I believe is the intended way, is that it is expected for the SDK to perform a redirect from that route. If I were to add a workaround to handle your proposed PR, I imagine I would perform my own redirect in the case of error/error_description params being present. However, I would imagine that sometimes these params may be present but not resulting in an internal SDK error, and so the SDK would successfully then redirect on its own. I would need to be able to reliably identify scenarios in which the SDK is not performing the redirect itself. Hope that makes sense!

I would imagine that sometimes these params may be present but not resulting in an internal SDK error

Any error present in the URL will cause the SDK to throw an error, see: https://github.com/auth0/auth0-spa-js/blob/master/src/Auth0Client.ts#L501

The user should most likely not be redirected to appState.target when handleRedirectCallback failed. We only redirect back to appState.target when it was successful.

What we could do is have the SDK take an optional errorPath (defaulting to /), which we would use to redirect to in this case. That would be the same as how our Angular SDK works: https://github.com/auth0/auth0-angular/blob/master/projects/auth0-angular/src/lib/auth.service.ts#L100

That way, our SDK always redirects, just not always to appState.target. That's only done on success.

Oh ok, perfect! Well that works I think - if we can guarantee that the presence of error query param results in this scenario, as drafted in your PR, then we can handle the redirect at that route. The errorPath suggestion sounds ideal, but I'm happy with a quick fix if easier for you!

I am not sure you can, as the presence of an error doesnt indicate it happend at this time.

So ye ... every error in the URL results in the error set internally. But not every error being set comes from an error param in the URL.

That makes sense, I can handle this scenario - happy for you to continue as per your draft PR. Thanks!

I have updated the PR to include an errorPath, would be great it you can have another look.

Once the PR is merged, we can pretty much cut a release instantly, which should unblock you 👍

@frederikprijck I've tested this and am very happy with the solution. Thanks so much for your help with this!

Released as 2.2.0, didn't go with a patch release because of the fact that we are adding errorPath. Arguably could have been a patch, but went with a minor.

Thanks again for reporting this!

@frederikprijck Thanks so much for moving so quickly on this 😊