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

Proper handling of max-age and expires for cookies #7925

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@watzon
Copy link

commented Jun 26, 2019

This PR is based off of #5042. Seeing as it has been sitting there for some time with no movement, and the current Cookie parsing is breaking with large max-age values, I decided to get to ball rolling again.

Chris Watson
@@ -194,36 +194,80 @@ module HTTP

it "parse max-age as seconds from current time" do

This comment has been minimized.

Copy link
@Sija

Sija Jun 26, 2019

Contributor

I guess this description should follow-up with your changes.

This comment has been minimized.

Copy link
@watzon

watzon Jun 26, 2019

Author

Good catch. I've updated it.

This comment has been minimized.

Copy link
@Sija

Sija Jun 26, 2019

Contributor

Actually I thought about the from current time part ;)

This comment has been minimized.

Copy link
@watzon

watzon Jun 26, 2019

Author

Oops, yeah you're right 😂

Chris Watson

@bcardiff bcardiff requested a review from straight-shoota Jun 26, 2019

Chris Watson
@watzon

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

It looks like the failures are due to a memory leak of some kind, any idea what could be causing it?

@asterite

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

I think compiler specs are leaking. I just noticed that codegen specs that don't use the prelude are allocating memory using Pointer.malloc or Foo.new which end up calling LibC.mallocand that memory is never freed.

However, specs usually don't allocate that much memory in tests so maybe there's something else going on... but it's hard to debug. Plus it only happens on linux 32, mostly.

@watzon

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

It's weird. I can't even run the full library specs on my machine or it eats up all my RAM. I've had to force shutdown my laptop twice. The cookie specs themselves pass though.

@RX14

RX14 approved these changes Jul 16, 2019

@asterite
Copy link
Member

left a comment

Looks good!

However, the time comparisons seem a bit fragile, for example if there's a hiccup in CI or something. I think we should bring timecop into the standard library to be able to properly test these things. But we can do it later.

def expired?(time_reference = @creation_time)
if @max_age == 0.seconds
true
elsif (time = expiration_time(time_reference)) && time < Time.now

This comment has been minimized.

Copy link
@asterite

asterite Jul 17, 2019

Member

Maybe use Time.utc_now to avoid fetching location information? Not sure.

This comment has been minimized.

Copy link
@watzon

watzon Jul 17, 2019

Author

Good question..

domain = @domain
samesite = @samesite
String.build do |header|
header << "#{URI.escape @name}=#{URI.escape value}"
header << "; domain=#{domain}" if domain
header << "; path=#{path}" if path
header << "; expires=#{HTTP.format_time(expires)}" if expires
header << "; max-age=#{max_age.total_seconds}" if max_age

This comment has been minimized.

Copy link
@asterite

asterite Jul 17, 2019

Member

I think total_seconds it a Float64. Does max-age allow float values in the RFC?

This comment has been minimized.

Copy link
@watzon

watzon Jul 17, 2019

Author

According to the RFC, Max-Age has to be a non-zero digit. It doesn't say anything about Float vs Int though.

This comment has been minimized.

Copy link
@Blacksmoke16

Blacksmoke16 Jul 17, 2019

Contributor

https://tools.ietf.org/html/rfc6265#section-5.2.2

Mentions:

   Let delta-seconds be the attribute-value converted to an integer.

   If delta-seconds is less than or equal to zero (0), let expiry-time
   be the earliest representable date and time.  Otherwise, let the
   expiry-time be the current date and time plus delta-seconds seconds.

   Append an attribute to the cookie-attribute-list with an attribute-
   name of Max-Age and an attribute-value of expiry-time.

so looks like it should be converted to an int.

This comment has been minimized.

Copy link
@Sija

Sija Jul 17, 2019

Contributor

Seems like max_age.to_i (number of full seconds) should be enough.

This comment has been minimized.

Copy link
@watzon

watzon Jul 17, 2019

Author

At the very least I feel like it should be an Int64. I have run into cases where Max-Age is too large and can't be parsed into an Int32.

This comment has been minimized.

Copy link
@Sija

Sija Jul 17, 2019

Contributor

@watzon Time::Span#to_i returns an Int64, so no problem here.

This comment has been minimized.

Copy link
@watzon

watzon Jul 17, 2019

Author

Oh it does? Well awesome then

This comment has been minimized.

Copy link
@watzon

watzon added some commits Jul 17, 2019

Show resolved Hide resolved src/http/cookie.cr Outdated
Remove total_seconds and replace with max_age.to_i
Co-Authored-By: Blacksmoke16 <yomoejoe@gmail.com>
@watzon

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

Holy moley, the specs actually passed

@RX14

RX14 approved these changes Jul 19, 2019

@watzon

This comment has been minimized.

Copy link
Author

commented Jul 19, 2019

Looks like we just need to get @straight-shoota in here for review

@Sija

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@watzon Two approvals is what's needed (would gr8 to have @straight-shoota review nevertheless).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.