Skip to content
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

Fix cookie with Max-Age processing #532

Closed
wants to merge 1 commit into from
Closed

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 2, 2017

When cookie has Max-Age set it tries to get expiration by adding
to a timestamp. Without this patch the timestamp would be set to
None and thus the addition of timestamp + max_age fails

https://pagure.io/freeipa/issue/6718

tiran
tiran previously requested changes Mar 2, 2017
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTC

ipalib/rpc.py Outdated
session_cookie = Cookie.get_named_cookie_from_string(cookie_string, COOKIE_NAME)
session_cookie = Cookie.get_named_cookie_from_string(
cookie_string, COOKIE_NAME,
timestamp=datetime.datetime.now())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datetime.datetime.utcnow()

Our cookie library uses UTC but now() returns a naive localized time stamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

When cookie has Max-Age set it tries to get expiration by adding
to a timestamp. Without this patch the timestamp would be set to
None and thus the addition of timestamp + max_age fails

https://pagure.io/freeipa/issue/6718
@simo5
Copy link
Contributor

simo5 commented Mar 2, 2017

Do we really care for calculating the expiration time ?
Should we just set timestamp to 0 or even remove the whole thing ?

@stlaz
Copy link
Contributor Author

stlaz commented Mar 2, 2017

If I read the code well, in a well-set-up cookie, during store_session_cookie() when Cookie.get_named_cookie_from_string() is called, the expiration gets normalized which basically means removing the Max-Age attribute and replacing it with the Expires attribute in the cookie string (see Cookie.normalize_expiration() and Cookie.__str__()). When later retrieving the cookie, it should not have the Max-Age attribute anymore but only Expires. Therefore we need to calculate it or change the way normalize_expiration() behaves.

@simo5
Copy link
Contributor

simo5 commented Mar 2, 2017

Ok, sorry for some reason I thought this was on the server side, where we do not care what the cookie looks like, but on the client side we indeed care.

@apophys
Copy link
Contributor

apophys commented Mar 3, 2017

Hi, can this PR get little more attention? The issue seems to be a cause for a lot of failures in our integration tests. (I'm not 100% sure though)

@simo5 simo5 added the ack Pull Request approved, can be merged label Mar 3, 2017
@simo5
Copy link
Contributor

simo5 commented Mar 3, 2017

LGTM, please merge

@simo5 simo5 dismissed tiran’s stale review March 3, 2017 20:02

Change applied

@HonzaCholasta
Copy link
Contributor

master:

  • 24eeb4d Fix cookie with Max-Age processing

@HonzaCholasta HonzaCholasta added the pushed Pull Request has already been pushed label Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants