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

Bad error message for wrong redirect URI #178

Open
JensErat opened this issue Mar 29, 2015 · 18 comments
Open

Bad error message for wrong redirect URI #178

JensErat opened this issue Mar 29, 2015 · 18 comments

Comments

@JensErat
Copy link
Contributor

I just worked through the Facebook authentication tutorial, and was stuck for quite some time until I started tracing the logs.

How to Reproduce

My problem was I registered a URI with trailing / (let it be https://example.org/) at facebook, but used the same URI without slash https://example.org in the python code.

r.text in oauth2_session.py contained a helpful error message:

{"error":{"message":"Error validating verification code. Please make sure your redirect_uri is identical to the one you used in the OAuth dialog request","type":"OAuthException","code":100}}

Expected Output

The error message is printed to the developer (eg., a reasonable exception is thrown).

Actual Output

Instead it was hidden from me, and I got a barely helpful and misleading traceback instead:

Traceback (most recent call last):
  File "./test.py", line 30, in <module>
    facebook.fetch_token(token_url, client_secret=client_secret, authorization_response=redirect_response)
  File "/usr/local/lib/python2.7/dist-packages/requests_oauthlib/oauth2_session.py", line 199, in fetch_token
    self._client.parse_request_body_response(r.text, scope=self.scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/clients/web_application.py", line 271, in parse_request_body_response
    self.token = parse_token_response(body, scope=scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/parameters.py", line 303, in parse_token_response
    validate_token_parameters(params, scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/parameters.py", line 313, in validate_token_parameters
    raise MissingTokenError(description="Missing access token parameter.")
oauthlib.oauth2.rfc6749.errors.MissingTokenError
@Lukasa
Copy link
Member

Lukasa commented Mar 29, 2015

If you can still easily reproduce this, what's the status code of that response?

@JensErat
Copy link
Contributor Author

Which status response, where can I fetch it from? There is a code 100 in r.text, I guess that's not what you're after?

@Lukasa
Copy link
Member

Lukasa commented Mar 29, 2015

That's not what I'm after, I'm after the HTTP status code. Where you print r.text you can also print r.status_code.

@JensErat
Copy link
Contributor Author

Aye, r for response. ;)

Seems to be a reasonable error code, 400. But indeed, at least in the lines between sending the request and continuing with the response contents I see no code verifying the response code. I guess somewhere around line 199, an exception should be thrown if the response code is in the 400 or 500 range.

@Lukasa
Copy link
Member

Lukasa commented Mar 29, 2015

Yeah, that was my line of thinking. We should probably just call r.raise_for_status() just after we invoke the compliance hooks.

JensErat added a commit to JensErat/requests-oauthlib that referenced this issue Mar 30, 2015
@JensErat
Copy link
Contributor Author

Your proposal raises an error if something's broken (and nother otherwise) -- but sadlly when using the facebook compaibility layer, raising the error raises an error.

30.03.2015 18:06:56 DEBUG Error trying to decode a non urlencoded string. Found invalid characters: set([u':', u' ', u'{', u'"', u'}']) in the string: '{"error":{"message":"Error validating verification code. Please make sure your redirect_uri is identical to the one you used in the OAuth dialog request","type":"OAuthException","code":100}}'. Please ensure the request/response body is x-www-form-urlencoded.

I guess the problem is around line 29 of the facebook hook, but I didn't investigate further as I'm unsure what's happening why here.

Putting the r.raise_for_status() above that resolves the issue, but I agree it should occur after (we might want to fix a bad return code).

@Lukasa
Copy link
Member

Lukasa commented Mar 30, 2015

Hang on, what's the exact flow here? Why weren't we hitting this before the change?

@Lukasa
Copy link
Member

Lukasa commented Mar 30, 2015

Oh nevermind, that doesn't appear to be an actual exception, so it wouldn't previously have blocked execution. It's weird that oauthlib doesn't raise exceptions here.

@Lukasa
Copy link
Member

Lukasa commented Mar 30, 2015

So this seems to be a bug in your code:

Error validating verification code. Please make sure your redirect_uri is identical to the one you used in the OAuth dialog request.

Does the status exception still get thrown?

@JensErat
Copy link
Contributor Author

Error validating verification code. Please make sure your redirect_uri is identical to the one you used in the OAuth dialog request.

Well, this was my original problem (and that's what I am using to trigger a non-successfull (non-200) HTTP status code. By now it is resolved for my actual code, but for testing this I broke it again.

As far as I understand the issue following is happening (a different situation per bullet point):

  • I don't have an issue in my code, everything's fine.
  • Given I have broken code (eg., a wrong redirect URI leading to the message quoted above) and the r.raise_for_status() line is not included: I get a weird and barely helpful error message, see my original post.
  • I have broken code, but with the line included after the Facebook hook was called: I get an exception that contains the error message Facebook is sending, but because the message was in an unexpected format/encoding. This is the "wrong exception", it seems to be thrown by the requests module while it tries to raise the expected exception.
  • I have broken code, but included the r.raise_for_status() line before the Facebook hook is called: everything's fine, an exception is thrown indicating a non-successful HTTP response was returned.

@Lukasa
Copy link
Member

Lukasa commented Mar 31, 2015

So in case 3, can you show me what exception was actually thrown? Stack trace and all, please. =)

@JensErat
Copy link
Contributor Author

I will create a minimal example as soon as I get round to it, will take some days.

@Lukasa
Copy link
Member

Lukasa commented Mar 31, 2015

That's alright, there's no rush. =)

@JensErat
Copy link
Contributor Author

Too bad, the error is resolved in such that Facebook now prints an error message instead of redirecting anyway if the URI is different. I'm not able to trigger such an error message any more. :/

I had to apply a small hack, and generated a new facebook instance. This is an example, derived from the original facebook example:

#!/usr/bin/env python
# Credentials you get from registering a new application
client_id = ''
client_secret = ''

# OAuth endpoints given in the Facebook API documentation
authorization_base_url = 'https://www.facebook.com/dialog/oauth'
token_url = 'https://graph.facebook.com/oauth/access_token'
redirect_uri = 'https://example.org/'     # Should match Site URL

from requests_oauthlib import OAuth2Session
from requests_oauthlib.compliance_fixes import facebook_compliance_fix
facebook = OAuth2Session(client_id, redirect_uri=redirect_uri)
facebook = facebook_compliance_fix(facebook)

# Redirect user to Facebook for authorization
authorization_url, state = facebook.authorization_url(authorization_base_url)
print 'Please go here and authorize,', authorization_url

redirect_uri = 'https://example.org'     # Like above, but without trailing slash
facebook = OAuth2Session(client_id, redirect_uri=redirect_uri)
facebook = facebook_compliance_fix(facebook)

# Get the authorization verifier code from the callback url
redirect_response = raw_input('Paste the full redirect URL here:')

# Fetch the access token
facebook.fetch_token(token_url, client_secret=client_secret,
                     authorization_response=redirect_response)

# Fetch a protected resource, i.e. user profile
r = facebook.get('https://graph.facebook.com/me?')
print r.content

Without my patch, this results in

  File "./minimal.py", line 29, in <module>
    authorization_response=redirect_response)
  File "/usr/local/lib/python2.7/dist-packages/requests_oauthlib/oauth2_session.py", line 199, in fetch_token
    self._client.parse_request_body_response(r.text, scope=self.scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/clients/web_application.py", line 271, in parse_request_body_response
    self.token = parse_token_response(body, scope=scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/parameters.py", line 303, in parse_token_response
    validate_token_parameters(params, scope)
  File "/usr/lib/python2.7/dist-packages/oauthlib/oauth2/rfc6749/parameters.py", line 313, in validate_token_parameters
    raise MissingTokenError(description="Missing access token parameter.")
oauthlib.oauth2.rfc6749.errors.MissingTokenError

With the patch, the raise function fails with following exception:

  File "./minimal.py", line 29, in <module>
    authorization_response=redirect_response)
  File "/usr/local/lib/python2.7/dist-packages/requests_oauthlib/oauth2_session.py", line 199, in fetch_token
    r.raise_for_status()
  File "/usr/lib/python2.7/dist-packages/requests/models.py", line 825, in raise_for_status
    raise HTTPError(http_error_msg, response=self)

@Lukasa
Copy link
Member

Lukasa commented Apr 15, 2015

I'd be happy to merge that patch if you'd like to open a pull request.

@JensErat
Copy link
Contributor Author

I added a pull request (and the patch at least fixes the issue that the return code is not verified). But I'm still not sure if there's not yet another problem as described above with the facebook code (as I think another, more specific exception should have been thrown).

I think the Facebook compatibility layer somehow tries to parse/change the returned message body, but I'm not sure what the code is doing why.

Lukasa added a commit that referenced this issue Apr 15, 2015
Verify return value of OAuth2 response (#178)
@RR2DO2
Copy link
Contributor

RR2DO2 commented Sep 22, 2015

The code change from the pull request has actually broken our code; historically oauthlib would process certain errors from within parse_request_body_response and raise if it wasn't handled (though this would happen via validate_token_parameters and not via checking the status code).

This can be seen here: https://github.com/idan/oauthlib/blob/master/oauthlib/oauth2/rfc6749/parameters.py#L382

With the change from this issue the OAuthlib specific errors from https://github.com/idan/oauthlib/blob/master/oauthlib/oauth2/rfc6749/errors.py aren't raised any longer.

RR2DO2 added a commit to RR2DO2/requests-oauthlib that referenced this issue Sep 29, 2015
As per requests#178 (comment) - the addition of this line caused a regression; the HTTP errors aren't passed on to OAuthlib anymore breaking the exceptions that would normally be raised.
@RR2DO2
Copy link
Contributor

RR2DO2 commented Sep 29, 2015

Looking at the original issue, this is due to oauthlib raising MissingTokenError if 'access_token' is not part of the response body. It doesn't handle the 'error' provided by Facebook as it is not a known oauth2 error (as per spec); causing errors.raise_from_error in oauthlib not to raise an exception. The next fall-through there is the missing token exception.

The error handling in oauthlib could probably be improved, both by adding support for registering custom errors (right now this has to be done by monkey-patching raise_from_error) and by having smarter handling about an 'error' key with an unsupported error. Technically that's against the oauth spec so it should become an 'invalid response' error, but with pluggable error handling requests-oauthlib could add a compliance fix for Facebook.

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