Use calendar.timegm when calculating cookie expiration #1860

Merged
merged 1 commit into from Dec 18, 2015

Conversation

Projects
None yet
3 participants
Owner

sigmavirus24 commented Jan 12, 2014

Fixes #1859

Credit: @Lukasa

Owner

Lukasa commented Jan 12, 2014

We need to confirm that this will match the actual parsing logic of cookies before we merge this: we don't want to start messing up cookies created in this way.

Owner

sigmavirus24 commented Jan 12, 2014

That's a good point @Lukasa. I'm guessing @gazpachoking might have a good idea. I'm just too tired to think this hard right now =P

Owner

Lukasa commented Jan 12, 2014

Me too, I'll take a look at this tomorrow.

Owner

kennethreitz commented Jan 23, 2014

Any update?

Owner

sigmavirus24 commented Jan 23, 2014

Sorry I never got around to that. @Lukasa how about you?

Owner

Lukasa commented Jan 24, 2014

Uh, no, totally dropped the ball here. I'll put it on my list. =)

Owner

sigmavirus24 commented Jan 30, 2014

@Lukasa how should we make sure this doesn't mess anything up?

Owner

Lukasa commented Jan 30, 2014

I'm not sure, to be honest. I'm wondering if there's a good way we can confirm our cookie-parsing code works in all locales.

@sigmavirus24 sigmavirus24 referenced this pull request Feb 3, 2014

Closed

Brittle test #1859

Owner

kennethreitz commented May 12, 2014

Closing due to inactivity.

@sigmavirus24 sigmavirus24 deleted the fix-1859 branch Mar 4, 2015

@Lukasa Lukasa referenced this pull request Oct 13, 2015

Closed

Test failure for 2.8.0 #2824

@Lukasa Lukasa restored the fix-1859 branch Dec 16, 2015

@Lukasa Lukasa reopened this Dec 16, 2015

Owner

Lukasa commented Dec 16, 2015

Ok, I'm reopening this because I've finally validated that the stdlib does this. That suggests that it's the right thing to do. I'll have to do the merge manually, but I'd like to have this.

@sigmavirus24, one question: could we meaningfully add this to a 2.9.1, or should it wait for 2.10.0?

Owner

sigmavirus24 commented Dec 16, 2015

I think this could be included in 2.9.1

@@ -393,8 +394,8 @@ def morsel_to_cookie(morsel):
expires = time.time() + morsel['max-age']
elif morsel['expires']:
time_template = '%a, %d-%b-%Y %H:%M:%S GMT'
@sigmavirus24

sigmavirus24 Dec 16, 2015

Owner

Should the template have "GMT" in it?

@Lukasa

Lukasa Dec 16, 2015

Owner

The honest answer is "probably not", to be honest: the RFC really doesn't require it at all, and instead only specifies that the time is to be parsed as UTC.

@Lukasa Lukasa merged commit 87abd9c into master Dec 18, 2015

1 check was pending

default Build triggered. sha1 is original commit.
Owner

Lukasa commented Dec 18, 2015

\o/ Finally fixed, nearly two years after the original patch was proposed!

@kennethreitz kennethreitz deleted the fix-1859 branch Feb 2, 2016

@CaoZ CaoZ referenced this pull request in CaoZ/JD-Coin Aug 1, 2017

Closed

mac下,qt-browser分支登录时候报错. #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment