vimalloc / flask-jwt-extended

An open source Flask extension that provides JWT support (with batteries included)!
http://flask-jwt-extended.readthedocs.io/en/stable/
MIT License
1.55k stars 239 forks source link

Malformed token raises an uncaught ValueError #246

Closed vimalloc closed 5 years ago

vimalloc commented 5 years ago

See https://stackoverflow.com/q/55917908/272689

vimalloc commented 5 years ago

We shouldn’t add an error handler directly for the value error to handle this, as that could easily start catching errors unrelated to JWTs. Instead we should catch the value error at the PyJWT decode call, and reraise it as something more specific.

gk-patel commented 5 years ago

Hi @vimalloc,

Firstly, you are maintaining a very nice library here, thank you and others for that.

Is it not possible for you to temporarily catch the error in your library and give a minor bug-fix-release ? Later, you could implement the full thing with catching value error at the PyJWT decode call.

vimalloc commented 5 years ago

Having a temporary errorhandler that catches ValueErrors is not a viable option, it could make debugging someone’s own code significantly harder. However fixing this up properly won’t be hard to do.

As this doesn’t present a security issue and shouldn’t ever show up during a normal users workflow (they would need to explicitly send in garbage to the API to trigger this), it hasn’t been a priority to work on this; it has been a really busy month for me. I’ll try to find time to tackle this as soon as I can, probably in the next couple weeks. Or if you want to submit a pull request to fix this up, I would be happy to help get that merged :+1:

gk-patel commented 5 years ago

Ya. You are right, having a quick-fix could be problematic for debugging. Also, it does not present a security issue, but in my case it does come-up during normal user-workflow.

In my implementation of the frontend, I use the refresh_token to ask the server for a new access_token, only when my current access_token has been indicated as invalid -- i.e. I don't use a timer to regularly fetch new access_tokens, rather I get it when my current access_token is invalided. In order to do this, I would need the correct error code (401 or something similar, but I get 500). The console output is as follows,

raise ExpiredSignatureError('Signature has expired')
jwt.exceptions.ExpiredSignatureError: Signature has expired

I am using version 3.18 with flask-restful.

And about the PR, I feel really bad, but I won't be able to find time for that. Because, unlike you, I don't spend my free time in trying to make the web secure (maybe I should), but rather I organize events and so on. Sorry buddy, but I do appreciate your motivation and work. Thanks again, and sorry for the delayed reply.

vimalloc commented 5 years ago

An expired token will not hit this error, that is already handled here: https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/jwt_manager.py#L96

This error will only show up if you try to pass in data that does not resemble a JWT at all, for example a string that does not have three dots and three different sections per the JWT spec. If you are getting a 500 error on an expired token you have something else going wrong.

If you are using something like Flask-Restful, you need to set app.config['PROPAGATE_EXCEPTIONS'] = True for the error to be properly returned to the caller. Or if you are using Flask-Restplus there is an bug with their error handling, which can be worked around like such: https://github.com/vimalloc/flask-jwt-extended/issues/86#issuecomment-335509456

mybooc commented 3 years ago

Thank you for your great work. Also bounced into something like this caused by a bad header, e.g. 'Bearer ,'. This causes a 500 error:

File "/home/nginxwww/xena/httpdocs/flask_test/venvs/rest_swag2m/lib/python3.8/site-packages/flask_jwt_extended/view_decorators.py", line 152, in <listcomp> jwt_header = [s for s in field_values if s.split()[0] == header_type] IndexError: list index out of range

This is caused by the line in view_decorators.py:

jwt_header = [s for s in field_values if s.split()[0] == header_type]

My suggestion is to handle this by changing the code and raising a NoAuthorizationError:

`

try:
   jwt_header = [s for s in field_values if s.split()[0] == header_type]
except Exception as e:
    raise NoAuthorizationError("Failed to decode Header: {}".format(auth_header))

` I do not see any harm in doing this.