Expires header not being set when setting cookies

[crw]

Please see these config options:

https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/config.py#L27

Not being respected in this file:

https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/utils.py#L396

This means the cookies will be deleted every time the browser is closed, requiring the user to re-login. Is there something I am missing about expiring these cookies at the end of the session? (updated to clean up the intention of the issue.)

[vimalloc]

It's setting the expires option in the JWT itself here (https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/utils.py#L81) which is saved in the cookie, not as a lifetime for the actual cookie. The result of this is that the cookie will not expire, but if the JWT in the cookie expires and you try to access a jwt_required endpoint, you will get back a 401 unauthorized error, with the specific error message that you have a valid token except that it is expired

[crw]

The confusion for me is in this statement:

The result of this is that the cookie will not expire [...]

In Chrome, these are being set as Session cookies and are deleted when the browser is closed. Per the flask documentation, if you do not set a max_age parameter the default is to create a session cookie. (see http://flask.pocoo.org/docs/0.12/api/#flask.Response.set_cookie).

Here are the cookies as seen by Chrome:

(xsrf_token and access_token are the flask_jwt_extended cookies as set by set_access_cookies; the other cookies are either the flask session or my own cookies.)

If the intention is for the cookies to "never" expire, they will need a far future expiration date (see: https://stackoverflow.com/questions/3290424/set-a-cookie-to-never-expire).

Hopefully this makes sense, let me know if I am missing something. I would be happy to submit a pull request but I want to make sure I am not missing something here. Thanks!

[crw]

(FWIW, flask_jwt_extended is really awesome. I very much want to keep using it so am motivated to see this issue fixed. :) )

[vimalloc]

Ah, I understand now. You are absolutely correct. If you want to make a pull request for this that would be awesome, or I would be happy to look into a fix for this later this week :)

I think a new config option for this should be in order, something along the lines of JWT_SESSION_COOKIE = True, where we leave it as a session cookie if true, or set it to some huge time in the future if it's false. I think the default value should be True, to avoid any breaking changes for people who already expect this behavior. Does this sound good to you?

(ps, glad you like the extension, that is always good to hear!) 👍

[vimalloc]

I've been meaning to update the options documentation page anyways, break it into different sections to make it easier to find what you need. This may be a good time to look into that as well.

[crw]

#36

I updated the docs but not sure how the formatting will turn out. This is just a "best stab" at matching your style and practices, I will not be offended if you drastically edit this pull request.

I have never run tox before and was unclear how to get all of the version of python it needed setup. Basically I was being lazy. Just FYI.

[vimalloc]

It looks great! And no worries with tox, I don't have all the versions installed either. Thanks to travis ci it automatically runs all the unit tests with different python versions for us when making a commit.

I'll get a new version pushed out asap. Thanks for contributing! 👍

[crw]

With regards to splitting up the config doc: honestly, I am a big fan of the "big list" approach. In fact, I was ecstatic when I found the very simple, self-explanatory config.py file. So fresh and so clean! I would link to that file from the docs, if possible, so that it is easy to find config options omitted from the docs.

[vimalloc]

That's good to know. Thanks for the feedback :)

I just pushed the new version (1.3.2) to pypi, it should show up shortly. Cheers.

[wittlesouth]

This still seems broken. If JWT_SESSION_COOKIE is set to true(default), both access token cookies and refresh token cookies are session only. If JWT_SESSION_COOKIE is set to false, the cookies have an expiration date 70 years in the future. There doesn't seem to be a way to have the cookie expiration date match the values set in JWT_ACCESS_TOKEN_EXPIRES and/or JWT_REFRESH_TOKEN_EXPIRES. This hasn't been a major impact for me with browsers, but the lack of expiration date on the token is causing loss of session for my iOS app every time it goes into the background.

EDIT:

Never mind, reading through the code, my understanding of the approach for setting cookie expiration was incorrect; you can set cookie expiration by a third optional argument to set_access_cookies and set_refresh_cookies. I'm not sure that I can envision a use case why the cookie expiration would differ from the token expiration; it would seem simpler to default the cookie duration to match the token duration if JWT_SESSION_COOKIE is false. I also have not seen this in the documentation. If people feel that either documentation updates or a potential feature change around JWT_SESSION_COOKIE would be interesting, I may spend a day or so seeing if I can put a change together.

[vimalloc]

Never mind, reading through the code, my understanding of the approach for setting cookie expiration was incorrect; you can set cookie expiration by a third optional argument to set_access_cookies and set_refresh_cookies

EDIT:

Disregard the statement bellow, you were refering to the max_age kwarg, which does do exactly what you said.

That isn't actually correct. That optional argument will change the exp time in the JWT itself, but it will not update the cookie expiration. IIRC the cookie expiration was set to 70 years in the future as it was a very simple way to fix session cookie settings. The thought behind it is even if your JWT is expired, it may be beneficial to still have the option to send that token to an endpoint, and receive a specific error message telling you that the token was expired, that can then be handled as needed on the frontend.

If this is causing problems for you I would totally be game to try and update this. My only concern is if people are currently looking for the 401 token expired message instead when an access token expires while using cookies, and we change it to send back the 401 token not found error message instead, it might break their app out from under them. I would like to make the update in a way that is backwords compatible with the current approach.

[wittlesouth]

I can easily live with the available argument to set_access_cookies / set_refresh cookies, it solves the problem I was having with my IOS app. I agree that changing the default behavior is a potentially breaking change, and so I'd be fine with just updating the docs. It seems reasonable for app developer who wants an expiration date on their tokens that matches (or has some delta from) the token expiration duration to use a constant in their app. That constant could be used for the existing configuration options for token expiration, and as an argument to the set_cookies method. This is non-breaking, and probably just needs an example (or even comments in the existing example) in the docs.