-
-
Notifications
You must be signed in to change notification settings - Fork 421
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
Comments
If you can still easily reproduce this, what's the status code of that response? |
Which status response, where can I fetch it from? There is a code 100 in |
That's not what I'm after, I'm after the HTTP status code. Where you print |
Aye, 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. |
Yeah, that was my line of thinking. We should probably just call |
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.
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 |
Hang on, what's the exact flow here? Why weren't we hitting this before the change? |
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. |
So this seems to be a bug in your code:
Does the status exception still get thrown? |
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):
|
So in case 3, can you show me what exception was actually thrown? Stack trace and all, please. =) |
I will create a minimal example as soon as I get round to it, will take some days. |
That's alright, there's no rush. =) |
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
Without my patch, this results in
With the patch, the raise function fails with following exception:
|
I'd be happy to merge that patch if you'd like to open a pull request. |
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. |
Verify return value of OAuth2 response (#178)
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. |
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.
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. |
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 behttps://example.org/
) at facebook, but used the same URI without slashhttps://example.org
in the python code.r.text
inoauth2_session.py
contained a helpful error message: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:
The text was updated successfully, but these errors were encountered: