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

Help users by more robustly handling an incorrect redirect_uri parameter #506

Closed
thclark opened this issue Feb 16, 2023 · 3 comments
Closed

Comments

@thclark
Copy link

thclark commented Feb 16, 2023

Describe the problem you'd like to have solved

I've just spent a frustrating 2 hours trying to get started (failing to log in correctly), because:

  • docs and tutorials from across the web including auth0's own pages are still using the old API from v1 (spread parameters instead of authorizationParams)
  • the redirect_uri parameter in the authorizationParams object needs to be snake cased (???!!!)
  • instead of realising these (fairly obvious when you see it) the server response and logs led me on a wild goose chase of reasoning, by:
    • stating the problem... Unable to issue redirect for OAuth 2.0 transaction ...
    • ...but not stating the reason or anything helpful (eg invalid redirect_uri, check 'authorizationParams' and your 'callback_urls' setting)
    • including a "riskAssessment" field in the details with mysterious labels like UntrustedIP in it
    • returning an incorrect error status 500, suggesting something is wrong server-side, instead of wrong with the request I sent

Describe the ideal solution

  1. When breaking backward compatibility, it's generally helpful to give users a warning or indication that they might have fallen foul of it.

  2. When writing in Javascript, accepting camelCased parameters might be sensible because its totally instinctive for JS developers to do that... so if typing out from your quickstart, instead of copy/pasting, it's a natural error.

I'd recommend (from my own experience, and those of this person and this person) to:

  • Handle the case where a camelCased redirectUri is added to the authorizationParams object, possibly with a warning or simply converted under the hood.
  • Issue an error to the console if redirect_uri is missing, or appears to be passed incorrectly (since it's a required parameter??)
  • In the logging (I know this isn't specifically the auth0-react functionality, perhaps the maintainers know better where to raise this to?), if you're going to fail to issue the redirect, state the reason as well as the problem (per my example above)
  • Return a Bad Request (400) error code instead of a Server Error (500) error code, to indicate that the problem is on my side, not internal to Auth0.

More details

Here's an example of a log that I was trying to find the problems from. You'll see the "details" field has absolutely nothing to do with why the error arose, but does have a lot of other stuff that threw me way off the scent:
{
  "date": "2023-02-15T18:28:04.233Z",
  "type": "f",
  "description": "Unable to issue redirect for OAuth 2.0 transaction",
  "connection": "github",
  "connection_id": "con_redacted",
  "client_id": "redacted",
  "client_name": "redacted",
  "ip": "redacted",
  "user_agent": "Chrome 109.0.0 / Mac OS X 10.15.7",
  "details": {
    "body": {},
    "qs": {
      "state": "redacted"
    },
    "connection": "github",
    "error": {
      "message": "Unable to issue redirect for OAuth 2.0 transaction",
      "oauthError": "server_error",
      "type": "oauth-authorization"
    },
    "session_id": "redacted",
    "riskAssessment": {
      "confidence": "high",
      "version": "1",
      "assessments": {
        "UntrustedIP": {
          "confidence": "high",
          "code": "not_found_on_deny_list"
        },
        "NewDevice": {
          "confidence": "high",
          "code": "match",
          "details": {
            "device": "known",
            "useragent": "known"
          }
        },
        "ImpossibleTravel": {
          "confidence": "high",
          "code": "minimal_travel_from_last_login"
        }
      }
    },
    "stats": {
      "loginsCount": 4
    }
  },
  "hostname": "redacted",
  "user_id": "redacted",
  "user_name": "redacted",
  "strategy": "github",
  "strategy_type": "social",
  "audience": "https://redacted.eu.auth0.com/userinfo",
  "scope": [
    "openid",
    "profile",
    "email"
  ],
  "log_id": "redacted",
  "_id": "redacted",
  "isMobile": false,
  "id": "redacted"
}

@thclark thclark changed the title Help users by passing warning on incorrect redirect_uri parameter Help users by more robustly handling an incorrect redirect_uri parameter Feb 16, 2023
@frederikprijck
Copy link
Member

frederikprijck commented Feb 16, 2023

Thank you for reaching out and sorry for the issues you've experienced.

docs and tutorials from across the web including auth0's own pages are still using the old API from v1 (spread parameters instead of authorizationParams)

I have reached out to the corresponding team to get them updated, or at least call out is uses an older version. I do recommend looking at our own Gatsby sample instead: https://github.com/auth0/auth0-react/tree/master/examples/gatsby-app, which we (the SDK team) guarantee to keep updated.

the redirect_uri parameter in the authorizationParams object needs to be snake cased (???!!!)
When writing in Javascript, accepting camelCased parameters might be sensible because its totally instinctive for JS developers to do that... so if typing out from your quickstart, instead of copy/pasting, it's a natural error.

Any parameter in authorizationParams is solely intended to send to Auth0 as is. Auth0 accepts all kinds of properties, some are first-class properties, others aren't. With v2, we have decided to stick to the casing used with Auth0, instead of changing cases in our own SDK. This makes it easier when following along Auth0 Server documentation such as here: https://auth0.com/docs/api/authentication#authorization-code-flow

We have explicitly called out the change in casing for redirectUri to redirect_uri in our MIGRATION_GUIDE. We encourage everyone to read this entire guide thoughoughy when upgrading to avoid issues like this. We are happy to make any changes to this guide if needed to improve it.

That said, let me have a conversation internally to see if we can improve the experience here.

In the logging (I know this isn't specifically the auth0-react functionality, perhaps the maintainers know better where to raise this to?), if you're going to fail to issue the redirect, state the reason as well as the problem (per my example above)
Return a Bad Request (400) error code instead of a Server Error (500) error code, to indicate that the problem is on my side, not internal to Auth0.

We have no control over Auth0 Server and how it does its logging or return codes. I do encourage providing product feedback if you believe these should be improved/changed.

@frederikprijck
Copy link
Member

frederikprijck commented Feb 16, 2023

Having had a conversation internally, we will try and patch it in such a way that when authorizationParams.redirectUri is used, we add a console warning and ensure it works again, something along the lines of:

  if (options.redirectUri) {
    console.warn(
      'Using redirectUri has been deprecated, please use `authorizationParams.redirect_uri` instead as `redirectUri` will be no longer supported in a future version'
    );
    options.authorizationParams.redirect_uri = options.redirectUri;
  }

  if (options?.authorizationParams?.redirectUri) {
    console.warn(
      'Using authorizationParams.redirectUri has been deprecated, please use `authorizationParams.redirect_uri` instead as `authorizationParams.redirectUri` will be removed in a future version'
    );
    options.authorizationParams.redirect_uri = options.authorizationParams.redirectUri;
  }

@frederikprijck
Copy link
Member

Closing as we merged the PR, will see to cut a release soon.

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