error query string param is breaking auth0

[igorbarbashin]

By submitting an Issue to this repository, you agree to the terms within the Auth0 Code of Conduct.

Provide a clear and concise description of the issue

We just spent 3 days trying to debug the problem when the app enters an infinite loop of getting logged out/logging back in for no apparent reason.

Turns out that the auth0-react package doesn't like when ?error=something query string appears in our app.
We've added a workaround of avoiding error name in the query strings. Had to add an extra redirect that renames error to something more descriptive like loginWithAmazonError. But still, it seems that it would be better if our query string params didn't conflict.

What was the expected behavior?

Auth0 could ignore arbitrary ?error query strings values.
Or use a more namespaced version of the variable name (e.g.auth0error)

Relevant line:
https://github.com/auth0/auth0-react/blob/master/src/utils.tsx#L5

Reproduction

• Log in
• Add ?error=123 to the url and hit enter
• See authentication breaking

Can the behavior be reproduced using the React SDK Playground?

Haven't tried. A bit limited in resources to try

[frederikprijck]

Hi @igorbarbashin ,

This can be solved by using the skipRedirectCallback property that is available on the Auth0Provider: https://auth0.github.io/auth0-react/interfaces/auth0_provider.auth0provideroptions.html#skipredirectcallback

This ensures the Auth0 SDK only handles error as well as code query string parameters when the callback is for Auth0, allowing u to use other callback for things like Amazon.

Some additional information here, we are using error as defined in the OAuth specification. Renaming that to auth0error would break that, see https://tools.ietf.org/html/rfc6749#section-4.1.2.1

Let me know if that doesn't help you.

[stevehobbsdev]

I think the actual logic is wrong here though. We should be using (code || error) && state (like what we do in auth0-angular), whereas here we're doing (code && state) || error, which means we'll process any error param when actually we should only process it in conjunction with a state param. Or am I missing some context? Either way, we have a disparity between the two.

[frederikprijck]

I think that is a fair point yes.

Fixing that would resolve the fact that we do not read every error querystring parameter, but it would still conflict with other providers when both error and state are in the URL simultaneous, unless using skipRedirectCallback.

[adamjmcgrath]

Thanks for raising this @igorbarbashin - I'll go ahead an implement @stevehobbsdev's suggestion in #225 (comment)