lepture / authlib

The ultimate Python library in building OAuth, OpenID Connect clients and servers. JWS,JWE,JWK,JWA,JWT included.
https://authlib.org/
BSD 3-Clause "New" or "Revised" License
4.45k stars 445 forks source link

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

Closed andersnauman closed 4 months ago

andersnauman commented 9 months ago

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)



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 commented 9 months ago

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

andersnauman commented 9 months ago

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