-
-
Notifications
You must be signed in to change notification settings - Fork 802
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
Replace expires cookie attribute with max-age attribute #823
Replace expires cookie attribute with max-age attribute #823
Conversation
Why not use both? That's what Django seems to do. |
cc2bfb1
to
b792120
Compare
I checked the Not using both has these benefits:
|
b792120
to
376ea72
Compare
I investigated this more and Flask also uses both:
Thus, in the end, this PR sets both, |
@@ -17,6 +17,7 @@ Unreleased | |||
- Use `datetime.now(timezone.utc)` instead of deprecated `datetime.utcnow`. #758 | |||
- Never look at the `X-Forwarded-For` header, always use `request.remote_addr`, | |||
requiring the developer to configure `ProxyFix` appropriately. #700 | |||
- Replace `expires` attribute with `max-age` in "remember_me" cookie. #823 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should read as additive since it's no longer replacing.
@@ -654,11 +647,8 @@ def test_remember_me_custom_duration_uses_custom_cookie(self): | |||
c.get("/login-notch-remember-custom") | |||
cookie = c.get_cookie(name, domain, path) | |||
self.assertIsNotNone(cookie) | |||
self.assertIsNotNone(cookie.expires) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be removed. Likewise below.
@@ -693,19 +680,6 @@ def login_notch_remember_custom_invalid(): | |||
result = c.get("/login-notch-remember-custom-invalid") | |||
self.assertEqual(result.status_code, 500) | |||
|
|||
def test_set_cookie_with_invalid_duration_raises_exception(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being removed?
|
||
def test_remember_me_refresh_each_request(self): | ||
with patch("flask_login.login_manager.datetime") as mock_dt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here.
Internet Explorer doesn't support max-age cookie attribute. I don't know any other reason to use expires instead of max-age.I was checking wrong "max-age". Even Internet Explorer supports max-age since IE 9 (source)Max-age works even if client or server clocks have wrong time.