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

Make sure 'code' returns None instead of crashing if key is missing. #592

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

andersnauman
Copy link
Contributor

DO NOT SEND ANY SECURITY FIX HERE. Please read "Security Reporting" section
on README.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Other, please describe:

  • You consent that the copyright of your pull request source code belongs to Authlib's author.

Instead of having the library users keeping track of which requests.args that are needed, change the behavior to not crash if variables requirements are not met.

Example code before change:

authorize = Blueprint('authorize', __name__)

@authorize.route('/<client_name>')
def oauth(client_name):
    """ Verify oauth """
    args = request.args
    if None in (args.get("code"), args.get("state")):
        abort(500)
    client = _oauth.create_client(client_name)
    if not client:
        abort(404)
    token = client.authorize_access_token()

Example code after change:
Now the correct raise will occur which we can check for as expected:

from authlib.integrations.base_client.errors import MismatchingStateError
authorize = Blueprint('authorize', __name__)

@authorize.route('/<client_name>')
def oauth(client_name):
    """ Verify oauth """
    client = _oauth.create_client(client_name)
    if not client:
        abort(404)

    try:
        token = client.authorize_access_token()
    except MismatchingStateError:
        abort(500)

@lepture
Copy link
Owner

lepture commented Nov 21, 2023

It will not crash, Flask will raise a 400 bad request for this case, unless Flask has changed this behavior.

@andersnauman
Copy link
Contributor Author

You are correct that Flask will catch the crash and respond accordingly.
If we look into the context of the library, it still crashes. And while this might not pose a fatal error for the running process, it still makes the code behave unpredictable and stop further actions.

This can easily become a religious discussion, but I would suggest that any library should never crash but to return (or at least raise an error) a None state or any other way of giving a signal that something went wrong. The library user should not be forced to understand the underlying code just to run it in a safe matter.

Please also note that this code-change have already been made in other parts of your library. E.g Here

@lepture lepture merged commit 610622e into lepture:master Apr 8, 2024
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

Successfully merging this pull request may close these issues.

3 participants